cesium-ml / baselayer

Fully customizable (scientific, compute-intensive) web application template
http://cesium-ml.org/baselayer/
31 stars 18 forks source link

Add timezone awareness to DateTime fields #187

Closed kmshin1397 closed 2 years ago

kmshin1397 commented 3 years ago

Allow database-agnostic timezone awareness to SA DateTime fields (https://docs.sqlalchemy.org/en/14/core/custom_types.html#store-timezone-aware-timestamps-as-timezone-naive-utc)

Using this approach allows proper usage of datetimes in API handler query logic without the developer having to remember it for every handler, automatically converts passed in values with non-UTC timezones, and does not necessitate a database migration as would've been necessary if we converted to Postgres-specific timestamp with timezone columns

One question I have is whether I should assume datetimes with no timezone info is in UTC (as it is now), or to throw an error requiring such info.

Also ran black formatting on models.py

Will follow up with a corresponding PR to SP for SP-specific DateTime fields

dannygoldstein commented 3 years ago

One question I have is whether I should assume datetimes with no timezone info is in UTC (as it is now), or to throw an error requiring such info.

I think it's fine (at least in the skyportal context) to assume UTC. That is a pretty standard astronomy convention.

pep8speaks commented 3 years ago

Hello @kmshin1397! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2021-05-03 20:06:24 UTC
kmshin1397 commented 3 years ago

@dannygoldstein @stefanv I think this is finally ready for review/re-review. Corresponding PR to baselayer_template_app adds tests for the model changes. I've confirmed locally that these tests will fail on the old model schemas using the base sa.DateTime types, as expected.

acrellin commented 3 years ago

There seem to be changes in the diff that are unrelated

acrellin commented 3 years ago

And there are merge conflicts, so it seems that this branch got into a wonky state

kmshin1397 commented 3 years ago

I would also double-check that our API docs specify that UTC is required.

Agreed. It's definitely there in some handlers but it'll be good to double-check all places where datetimes are passed as parameters when I go through and update SP to use this new type.

stefanv commented 2 years ago

@guynir42 You've looked at the timezone issues before; should we still try and salvage this PR?

guynir42 commented 2 years ago

@stefanv what I've seen is errors on some of the tests that are probably related to this, but only on my local machine. It may be worth doing but definitely not a high priority unless someone runs into trouble with their instance or something (in which case their tests will also fail).