Skip to content

Conversation

@utnapischtim
Copy link
Contributor

No description provided.

@utnapischtim utnapischtim marked this pull request as draft November 15, 2024 22:07
Comment on lines +137 to +139
def now():
"""Creates a datetime object."""
return datetime.now(timezone.utc).replace(tzinfo=None)
Copy link
Member

@slint slint Dec 16, 2024

Choose a reason for hiding this comment

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

IMHO I would prefer that we find a more "SQLAlchemy way" to avoid this footgun:

  • It's going to be difficult to teach newcomers and (unfortunately, manually) enforce thoughout the codebase doing from invenio_db import now everytime we store UTC/non-TZ-aware datetimes in the DB.
  • It's going to be difficult to catch datetime-related bugs in tests, since often we just "ignore" the values of created/updated fields in responses and focus more on "data content" (which is not a bad thing when applied consiously).
  • I would be more in favour towards finding some config/setting/param in SQLAlchemy datetime columns or even defining a new custom DateTime column type that "does the right thing"™️ on (de)serialization which we can then gradually replace throughout the codebase in models that:
    • Doesn't affect Alembic migrations (e.g. column types in Postgres are still TIMESTAMP WITHOUT TIME ZONE)
    • Makes sure that Python datetime objects with the UTC timezone, get transformed accordingly
    • Explicitly complains/errors when non-UTC timezone-aware datetimes are passed, so that it's not the humans/reviewers that have to check, but the code does it instead.

There's nothing to be done at the moment, but this should be discussed a bit in more detail, so we make a sound decision that we apply.


On a side note, I think we've never explicitly wrote this anywhere, but the common practice throughout Invenio is to store/persist UTC times and convert to/from local timezone-aware times in the UI. I don't know where to log this "decision", but this post is a good supplementary read, from which I get the feeling that for us storing UTC is better.

@utnapischtim
Copy link
Contributor Author

superseded by #179

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