Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FSPT-208 Set the application date submitted property using sql alchemy utils #127

Closed
wants to merge 1 commit into from

Conversation

sfount
Copy link
Contributor

@sfount sfount commented Jan 7, 2025

This resolves the issue with timestamps by using an sql alchemy util that will use the databases now equivalent (CURRENT_TIMESTAMP in postgres).

Assuming that our we treat our Postgres dates as default UTC, this should have the added benefit of lining up with all of the other dates that are populated by the database. Relying on apps to generate accurate and consistent times can cause issues with minor drift and there can be benefits to having a single database instance own as much of date/ timestamp generation as possible.

In this case this resolves the issue as the date_submitted property that is then returned is read consistently from the model as it has come from the database - that means it won't include timezone information and will be serialised consistently with the previous application store method.


date_submitted is stored both in the application table and in the JSON representation of an application used by the assess tool.

Marshmallow will serialise the properties from the application model depending on if there is timezone information included or not. Previously the application store was first "submitting" an application (updating the status and setting the date_submitted property) in one database transaction and then separately fetching the application and putting in on a queue to be processed by assess.

When we changed this behaviour to update the application and write to the assessment table in one transaction we don't fetch a new application from the database.

When the date_submitted property is set directly in the code we have been including timezone.utc on the datetime property. When the application is fetched from the database it does not include (or store) the timezone information - I'm assuming everything by default is in UTC.

This means that we changed to serialising the JSON with timezone information (+00.00) which caused issues when the assessment code tried to serialise it.

`date_submitted` is stored both in the application table and in the JSON
representation of an application used by the assess tool.

Marshmallow will serialise the properties from the application model
depending on if there is timezone information included or not.
Previously the application store was first "submitting" an application
(updating the status and setting the `date_submitted` property) in
one database transaction and then separately fetching the application
and putting in on a queue to be processed by assess.

When we changed this behaviour to update the application and write to
the assessment table in one transaction we don't fetch a new application
from the database.

When the `date_submitted` property is set directly in the code we have
been including `timezone.utc` on the `datetime` property. When the
application is fetched from the database it does not include (or store)
the timezone information - I'm assuming everything by default is in UTC.

This means that we changed to serialising the JSON with timezone
information (`+00.00`) which caused issues when the assessment code
tried to serialise it.

This resolves the issue with timestamps by using an sql alchemy util
that will use the databases now equivalent (`CURRENT_TIMESTAMP` in
postgres).

Assuming that our we treat our Postgres dates as default UTC, this
should have the added benefit of lining up with all of the other dates
that are populated by the database. Relying on apps to generate
accurate and consistent times can cause issues and there can be benefits
to having a single database instance own as much of date/ timestamp
generation as possible.

In this case this resolves the issue as the `date_submitted` property
that is then returned is read consistently from the model as it has come
from the database - that means it won't include timezone information and
will be serialised consistently with the previous application store
method.
Copy link

sonarqubecloud bot commented Jan 7, 2025

@@ -277,7 +277,7 @@ def submit_application(application_id) -> Applications: # noqa: C901
application = process_files(application, all_application_files)

# Mark the application as submitted
application.date_submitted = datetime.now(timezone.utc)
application.date_submitted = func.now()
Copy link
Contributor

@samuelhwilliams samuelhwilliams Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, this one is a bit weird - until this is flushed to the DB, it's not a datetime instance, which maybe is unintuitive/weird? I think this would catch people out.

(Pdb) application.date_submitted
datetime.datetime(2025, 1, 7, 20, 11, 55, 267208)
(Pdb) n
> /app/application_store/db/queries/application/queries.py(285)submit_application()
-> application.status = ApplicationStatus.SUBMITTED
(Pdb) application.date_submitted
<sqlalchemy.sql.functions.now at 0xffff529a4fd0; now>

....
(Pdb)
2025-01-07 20:13:35,445 INFO sqlalchemy.engine.Engine UPDATE applications SET date_submitted=now() WHERE applications.id = %(applications_id)s::UUID
2025-01-07 20:13:35,445 INFO sqlalchemy.engine.Engine [generated in 0.00066s] {'applications_id': UUID('88383efc-af20-467b-b6de-c33ee8ce7929')}
2025-01-07 20:13:35,449 INFO sqlalchemy.engine.Engine SELECT applications.id, applications.account_id, applications.fund_id, applications.round_id, applications.key, applications.language, applications.reference, applications.project_name, applications.started_at, applications.status, applications.date_submitted, applications.last_edited
FROM applications
WHERE applications.id = %(id_1)s::UUID
2025-01-07 20:13:35,449 INFO sqlalchemy.engine.Engine [cached since 46.03s ago] {'id_1': '88383efc-af20-467b-b6de-c33ee8ce7929'}
> /app/application_store/db/queries/application/queries.py(291)submit_application()
(Pdb) application.date_submitted
datetime.datetime(2025, 1, 7, 20, 12, 49, 625238)

I like func.now() generally, esp that it is a fixed timestamp from the start of transactions so you'll generally have all timestamps be the same if you're setting them on different records within one transaction. But I wonder if doing just this change here might lead to more weirdness? Do we maybe need to do this a bit more ... wholesale and have some more certainty that someone trying to do this elsewhere isn't going to hit weird problems?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah generally agreed 👍 where right now we have datetime w/ timestamp (when its set), datetime without timestamp (when its read from the db) setting this and then actively using it would be a third state it could be in. We do actually use func.now() as a default value for dates in a lot of places this seems to be a bit of an exception. Very happy to revisit this wholesale while looking at dates being more robust across the code.

Thanks for the posting the logs too very interesting.

@sfount
Copy link
Contributor Author

sfount commented Jan 7, 2025

Closed in favour of #126

@sfount sfount closed this Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants