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

Breaking Change on STAC Item datetime field #204

Closed
jaysnm opened this issue Aug 5, 2021 · 15 comments
Closed

Breaking Change on STAC Item datetime field #204

jaysnm opened this issue Aug 5, 2021 · 15 comments
Assignees

Comments

@jaysnm
Copy link
Contributor

jaysnm commented Aug 5, 2021

Hi
Thanks a lot for this cool tool-kit.
I seem to have problem with STAC Item datetime field serialization. DATETIME_RFC339 introduced in stac_fastapi/sqlalchemy/stac_fastapi/sqlalchemy/serializers.py is causing timestamp format conversion error. Applying a temporary fix of assigning DATETIME_RFC339 = "%Y-%m-%dT%H:%M:%S.%fZ" solves the issue. The RFC339 standard is missing %f portion of datetime. I'm not sure whether use of the standard in test files has an impact, but I changed all occurrence of RFC339 to my temporary fix to be on the safe side!

@jaysnm jaysnm changed the title Breaking Changes on STAC Item date Breaking Change on STAC Item datetime field Aug 5, 2021
@geospatial-jeff
Copy link
Collaborator

I believe using pydantic's datetime parser should fix this instead of a single hard-coded datetime pattern. We made the same change to datetime parsing in stac-pydantic for similar reasons (stac-utils/stac-pydantic#75).

@jaysnm
Copy link
Contributor Author

jaysnm commented Aug 5, 2021

Thanks Jeff

Sounds promising that you already have an idea for a sustainable fix.

@jaysnm
Copy link
Contributor Author

jaysnm commented Aug 7, 2021

Hi @geospatial-jeff

What do you think about python-dateutil?
dateutil.parser.parse function takes care of date format conversion!

@geospatial-jeff
Copy link
Collaborator

I think python-dateutil is fine, used it before in other projects. But I'd prefer using pydantic's datetime parser in this situation to be consistent with the validation logic in stac-pydantic. Overall I don't think either python-dateutil or pydantic are what we will settle on long term, there is more context in #161

@philvarner
Copy link
Collaborator

@philvarner
Copy link
Collaborator

Also, this related issue #161

@kylebarron
Copy link
Contributor

I just came across ciso8601. It looks to be both faster and more popular than pyiso8601

@philvarner
Copy link
Collaborator

I just ran my test datetime values against it ciso8601, and only failed when it should have passed closeio/ciso8601#110

@philvarner
Copy link
Collaborator

I ran it against a few others:

ciso8601
Error: 1985-04-12T23:20:50,52Z did not parse. UTC offset is mandatory in RFC 3339 format.

arrow
Error: 1985-04-12t23:20:50.000z did not parse. Could not match input '1985-04-12t23:20:50.000z' to any of the following formats: YYYY-MM-DD, YYYY-M-DD, YYYY-M-D, YYYY/MM/DD, YYYY/M/DD, YYYY/M/D, YYYY.MM.DD, YYYY.M.DD, YYYY.M.D, YYYYMMDD, YYYY-DDDD, YYYYDDDD, YYYY-MM, YYYY/MM, YYYY.MM, YYYY, W.
Error: 1985-04-12 parsed, but should not have. result: 1985-04-12T00:00:00+00:00
Error: 1937-01-01T12:00:27.87+0100 parsed, but should not have. result: 1937-01-01T12:00:27.870000+01:00
Error: 1985-12-12T23:20:50.52 parsed, but should not have. result: 1985-12-12T23:20:50.520000+00:00

pendulum
Error: 1985-04-12t23:20:50.000z did not parse. Unable to parse string [1985-04-12t23:20:50.000z]
Error: 1985-04-12 parsed, but should not have. result: 1985-04-12T00:00:00+00:00
Error: 1937-01-01T12:00:27.87+0100 parsed, but should not have. result: 1937-01-01T12:00:27.870000+01:00
Error: 1985-12-12T23:20:50.52 parsed, but should not have. result: 1985-12-12T23:20:50.520000+00:00
Error: 1985-04-12T23:20:50.Z parsed, but should not have. result: 1985-04-12T23:20:50+00:00
Error: 1985-04-12T23:20:50,Z parsed, but should not have. result: 1985-04-12T23:20:50+00:00

iso8601
Error: 1985-04-12t23:20:50.000z did not parse. Unable to parse date string '1985-04-12t23:20:50.000z'
Error: 1985-04-12 parsed, but should not have. result: 1985-04-12 00:00:00+00:00
Error: 1937-01-01T12:00:27.87+0100 parsed, but should not have. result: 1937-01-01 12:00:27.870000+01:00
Error: 1985-12-12T23:20:50.52 parsed, but should not have. result: 1985-12-12 23:20:50.520000+00:00

for arrow, pendulum, and iso8601, non of them handle lowercase t or z (which is easy to remedy just by uppercasing the string).
None of them also have an RFC 3339 specific parser, so they accept strings that are ISO8601 but not RFC3339.

I'm most in favor of using ciso8601 and getting that one bug fixed with it.

@kylebarron
Copy link
Contributor

and only failed when it should have passed

I'm guessing you meant "only one failed"?

I'm most in favor of using ciso8601 and getting that one bug fixed with it.

Cool; I also made a PR there for building binary wheels on releases (closeio/ciso8601#109); if accepted it should enable installing in environments where a C compiler isn't installed.

@philvarner
Copy link
Collaborator

lol, yes -- only that one, and an uncommon one at that.

@philvarner
Copy link
Collaborator

So, that RFC 3339 datetime with the comma as a separator is actually invalid -- for some reason, the RFC 3339 spec has an ISO 8601 ABNF in it, and I didn't notice that that's what it was when I created these examples.

@philvarner philvarner self-assigned this Mar 2, 2022
@geospatial-jeff
Copy link
Collaborator

Closed with #368

@alexamici
Copy link

Merge of this PR that adds unconditional dependency on ciso8601 breaks the support for python 3.10 😢

@geospatial-jeff
Copy link
Collaborator

@alexamici ciso8601 dependency was removed in 2.4.1

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

No branches or pull requests

5 participants