stac-utils / stac-fastapi

STAC API implementation with FastAPI.
https://stac-utils.github.io/stac-fastapi/
MIT License
245 stars 99 forks source link

Breaking Change on STAC Item datetime field #204

Closed jaysnm closed 2 years ago

jaysnm commented 3 years ago

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!

geospatial-jeff commented 3 years ago

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 (https://github.com/stac-utils/stac-pydantic/pull/75).

jaysnm commented 3 years ago

Thanks Jeff

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

jaysnm commented 3 years ago

Hi @geospatial-jeff

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

geospatial-jeff commented 3 years ago

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 commented 3 years ago

I just added this to the STAC API impl guide also https://github.com/radiantearth/stac-api-spec/blob/master/implementation.md#datetime-parameter-handling

philvarner commented 3 years ago

Also, this related issue https://github.com/stac-utils/stac-fastapi/issues/161

kylebarron commented 3 years ago

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

philvarner commented 3 years ago

I just ran my test datetime values against it ciso8601, and only failed when it should have passed https://github.com/closeio/ciso8601/issues/110

philvarner commented 3 years ago

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 commented 3 years ago

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 (https://github.com/closeio/ciso8601/pull/109); if accepted it should enable installing in environments where a C compiler isn't installed.

philvarner commented 3 years ago

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

philvarner commented 3 years ago

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.

geospatial-jeff commented 2 years ago

Closed with #368

alexamici commented 2 years ago

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

geospatial-jeff commented 2 years ago

@alexamici ciso8601 dependency was removed in 2.4.1