Adjust ItemProperties Validation. #131

eseglem · 2023-12-07T13:46:07Z

Fixes #130.

Rather than using python to parse / validate the values first, this lets pydantic do its thing and then ensures the fields are set properly. This means it no longer modifies the input values in place and is significantly faster.

Previously it was set as Union[dt, str] which I believe is not actually valid based on the spec. And does not match the other datetime fields in the library. As with the others, using Optional[dt] allows for the input null value, while requiring any string to be a datetime.

Before:

After:

Yes, the test_pydantic_model_validate is "slower" between the two, but that is because in the initial benchmarks it was modifying the input datetime in place and essentially cheating by already having it as a datetime for all the benchmarks.

Otherwise the test_pydantic_orjson_model_validate and test_pydantic_model_validate_json are much faster than they were before.

Benchmarks gist is here: https://gist.github.com/eseglem/ddae063eefab545c122c93a2afc6cb86

Fixes stac-utils#130.

vincentsarago · 2024-02-08T17:35:28Z

stac_pydantic/item.py

+        if not self.datetime and (not self.start_datetime or not self.end_datetime):
+            raise ValueError(
+                "start_datetime and end_datetime must be specified when datetime is null"
+            )


@eseglem 👋
The issue with this is that pydantic will allow a lot of datetime format while the stac specification says they should conform with https://datatracker.ietf.org/doc/html/rfc3339#section-5.6

I've updated this portion in #133

I didn't touch the overall logic of the validation but I think we could totally have:

before validator that convert string to datetime object

after check the datetime/start_datetime/end_datetime conflic

Note: because of the specification, the serialized is required

My primary purpose here was to avoid messing with the input data. Also, note that StacCommonMetadata just uses Optional[datetime] as well. So I was just matching up to it. Looks like #75 was where that got changed.

But yes, when parsing input data, Pydantic does allow some flexibility. I think the most important part is that the output is valid RFC3339, and allowing some flexibility on input is not a bad thing. And I would just use Pydantic for it. As there is a lot of efficiency gain there not passing values back and forth. And I figured they were already being parsed elsewhere too. But if others feel strongly about not supporting it, that is not the focus of this PR.

I believe the best way to integrate your changes would be something like this:

# Put this in shared maybe? rfc3339_datetime = Annotated[datetime, BeforeValidator(parse_rfc3339)] class ItemProperties(StacCommonMetadata): datetime: Optional[rfc3339_datetime] = Field(..., alias="datetime")

That would avoid the issue and force the format.

As a middle ground possibility: Pydantic has AwareDatetime which would avoid inputs without any timezone info that could possibly cause some output issues.

But yes, when parsing input data, Pydantic does allow some flexibility. I think the most important part is that the output is valid RFC3339, and allowing some flexibility on input is not a bad thing.

well if you use stac-pydantic to validate item, IMO it should fails if the datetime is not a valid rfc3339 string (per specification)

I improved the logic a bit since the original comment, but there are still a few formats that are accepted but are not actually to spec. I was aiming for "be permissive for inputs, strict on outputs" with this iteration. It is a middle ground between where things were (before your updates) and completely strict.

As of right now, this will allow space and underscore in addition to T that RFC3339 specifies. While anything other than T is out of spec, it is not the end of the world. Its still 100% clear what the meaning of the string is. And in all likelihood the string will have it since that is the standard way for it to be. It will also allow for seconds, and not just ms, to be optional in the input. Again, its clear what the meaning of that string is.

The thing not addressed by any of these things so far is that RFC3339 does allow for any timezone, though says you probably should use UTC. And the STAC spec specifies it must be in UTC. Which, to me, feels excessive. As long as it follows includes the timezone the machine does not care if the string says "2024-02-16T03:00:00+00:00" or "2024-02-15T22:00:00-05:00" they are the same thing.

So, sure these things violate the spec, but not in any meaningful way.

Well I kindly disagree (but I'm not using stac-pydantic a lot so I'll defer to you for the final 👍 😄)

I'm just worry that people are going to use stac-pydantic to validate Items then put them in a database or whatever and then 2 cases:

the database will throw an error because the datetime is invalid

serve the items to the user with the incorrect datetime format

Again I appreciate the time you're spending on this PR

I am not much of a user either, so I would prefer to defer to others as well. So, I went towards the permissive input strict output direction.

Those are totally valid concerns, which I also had. I tried to address them in how I implemented it. And I do not mind spending the time on it to make sure things are done well. And, if possible, in a manner that makes everyone comfortable.

the database will throw an error because the datetime is invalid

Once the input data has been validated by pydantic and the object is created, they are all python datetime objects with timezone awareness which have been cast to UTC. At that point it is up to the user and whatever library they are using to interact with the database to get things correct. I am primarily experienced with sqlalchemy and psycopg2, and both of them will have no issues. And even if they are, for some reason, interacting with the database in a timezone naive manner, the datetime object is in UTC, so they should still be fine.

UtcDatetimeAdapter = TypeAdapter(UtcDatetime) EXPECTED = datetime(2024, 1, 1, 0, 0, 0, tzinfo=timezone.utc) all( UtcDatetimeAdapter.validate_strings(value) == EXPECTED for value in ( # Valid RFC3339 datetime strings in UTC "2024-01-01T00:00:00Z", "2024-01-01t00:00:00z", "2024-01-01T00:00:00.0Z", "2024-01-01t00:00:00.00000z", "2024-01-01T00:00:00+00:00", "2024-01-01t00:00:00+00:00", # Other valid RFC3339 datetime strings "2023-12-31T19:00:00-05:00", "2024-01-01T01:00:00+01:00", # Other formats pydantic accepts "2024-01-01T00:00Z", # No seconds "2024-01-01 00:00:00Z", # Space "2024-01-01_00:00:00Z", # Underscore ) ) # True

It will not accept these:

"2024-01-0100:00:00Z", # No T "2024-01-01T00:00:00", # No timezone "20240101T000000Z", # No separators "20240101000000+0000", # No separators "2024-01-01T00Z", # No minutes

It is worth noting, there is a bug / regression in pydantic 2.6 with "2024-01-01" incorrectly being accepted, but that was patched yesterday (2024-02-15). I believe it only affects 2.6.0 and 2.6.1, but I can dig up exact versions if there is a concern about that.

And the pydantic docs only really mention T so the space and underscore are either undocumented feature, or a bug. I'll see about digging into that more as well.

serve the items to the user with the incorrect datetime format

Under normal usage, I do not see a way for that to happen. If pydantic validated the value then the datetime object is in UTC. And either option for serializing produces valid a valid RFC3339 string in UTC. As required by the STAC spec.

EXPECTED.isoformat() # '2024-01-01T00:00:00+00:00' TypeAdapter(datetime).dump_json(EXPECTED) # b'"2024-01-01T00:00:00Z"'

Because pydantic does not validate on assignment by default, a user could assign a bad value and then output a bad value because of that. That is not a problem unique to datetimes though, and exists for every value within any library built on top of pydantic. Unfortunately there is not really much to do about that. The guardrails are in place, and if they choose not to use them, that is outside the library's control. I do have a few other ideas that could be explored here, but it feels wrong to try and address issues with un-validated values at serialization time. And they would all come with some loss of performance.

vincentsarago · 2024-02-08T20:14:19Z

stac_pydantic/item.py


-    @field_serializer("datetime")
-    def serialize_datetime(self, v: dt, _info: Any) -> str:
-        return v.strftime(DATETIME_RFC339)


I think we still need the serializer to make sure we respect the RFC3339 spec

This could be one implementation (to make sure we keep microseconds, if provided)

@field_serializer("datetime", "start_datetime", "end_datetime", when_used="always") def serialize_datetime(self, v: Optional[dt], _info: Any) -> str: if v: if str(v)[19] == ".": return v.strftime("%Y-%m-%dT%H:%M:%S.%fZ") else: return v.strftime("%Y-%m-%dT%H:%M:%SZ")

Assuming the datetime object has all the required values, then the pydantic output will always be valid RFC3339. But that is not always the case. Pydantic allows in time without seconds, but then the output would not be valid. If input is validated through parse_rfc3339 then it would be fine to use no serializer and let pydantic handle it.

If, however, we want to accept a wider range of inputs (AwareDatetime) then a serializer is probably necessary. I believe the best solution would actually be to do .isoformat() on the datetime object, not an .strftime. As RFC3339 allows any timezone.

time-numoffset = ("+" / "-") time-hour ":" time-minute time-offset = "Z" / time-numoffset

And the use of .strftime("%Y-%m-%dT%H:%M:%S.%fZ") is not guaranteed to be correct. Depending on the input timezone.

from ciso8601 import parse_rfc3339 from datetime import datetime, timezone from pydantic import TypeAdapter d = parse_rfc3339("2011-11-04T00:05:23.283+04:00") d.strftime("%Y-%m-%dT%H:%M:%S.%fZ") # '2011-11-04T00:05:23.283000Z' <- Not actually the correct timezone d.isoformat() # '2011-11-04T00:05:23.283000+04:00' d.astimezone(timezone.utc).strftime("%Y-%m-%dT%H:%M:%S.%fZ") # '2011-11-03T20:05:23.283000Z' d.astimezone(timezone.utc).isoformat() # '2011-11-03T20:05:23.283000+00:00' TypeAdapter(datetime).dump_json(d) # b'"2011-11-04T00:05:23.283000+04:00"' TypeAdapter(datetime).dump_json(d.astimezone(timezone.utc)) # b'"2011-11-03T20:05:23.283000Z"'

All of those are valid RFC3339 outputs, but one of them is lying about the timezone.

And now looking back at https://github.com/radiantearth/stac-spec/blob/v1.0.0/item-spec/item-spec.md#properties-object seems like there is actually a bit more of an issue. The spec requires UTC on top of the RFC3339. So even with parse_rfc3339 that part is not being enforced on the input. So it would depend on if that needs to be enforced on input or not.

I see a few possibilities:

def parse_rfc3339_asutc(value: Any) -> datetime: return parse_rfc3339(value).astimezone(timezone.utc) def parse_utc_rfc3339(value: Any) -> datetime: d = parse_rfc3339(value) if not d.tzinfo == timezone.utc: raise ValueError("Input must be UTC") return d # Flexible input as long as there is a timezone. Converts to UTC. Use isoformat to get valid RFC format. Annotated[AwareDatetime, AfterValidator(lambda d: d.astimezone(timezone.utc)), PlainSerializer(lambda d: d.isoformat())] # Input must be RFC, any timzone. Converts to UTC. Pydantic serializer outputs valid RFC format. Annotated[datetime, BeforeValidator(parse_rfc3339_asutc)] # Input must be RFC and UTC. Pydantic serializer outputs valid RFC format. Annotated[datetime, BeforeValidator(parse_utc_rfc3339)]

I would probably still go with Pydantic for parsing. Gives the most flexibility on input, and can still guarantee valid output.

I agree with @gadomski comments below in regards to "be permissive for inputs, strict on outputs."
Pystac solution for this uses dateutil.parser for converting strings and setting datetimes with missing timezone to UTC when parsing to json.

This has the advantage that any date format can be accepted including 2020, 2020-01 and 2020-01-01.

I added a possible solution in here

@thomas-maschler if going the dateutil.parser route, it would probably make sense to just let pydantic do its thing instead. It should be more performant.

The way it is current written in this PR is to accept a datetime with a timezone, and convert it to UTC. Rather than accept a naive value and assume UTC. Though the spec here does say it must be in UTC. So, its not a horrible thing to assume either. Tons of possible solutions. I was trying to optimize on keeping as much as possible inside the pydantic rust code and minimal python code to maintain here.

Its funny how a spec trying to simplify things or add clarity can make things more complicated than necessary.

eseglem · 2024-02-09T05:19:52Z

And probably worth a ping to @gadomski for any thoughts as well.

gadomski · 2024-02-09T13:08:49Z

And probably worth a ping to @gadomski for any thoughts as well.

I apologize but I don't have the bandwidth right now for a full dive-in, but I generally am in favor of "be permissive for inputs, strict on outputs." I'm not as familiar with pydantic as either yourself or @vincentsarago, so I'll have to defer on the correct spot to transition from permissiveness-to-strictness (whether that's on serialization or deserialization).

…c-130-validate-item-properties

tests/api/extensions/test_fields.py

tests/api/test_search.py

stac_pydantic/item.py

eseglem · 2024-02-14T06:02:57Z

stac_pydantic/shared.py

@@ -15,9 +22,16 @@

 SEMVER_REGEX = r"^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?$"

-# https://tools.ietf.org/html/rfc3339#section-5.6
-# Unused, but leaving it here since it's used by dependencies


Removed from the tests. And, if there are downstream users which use this outside of that, they should do their own formatting, as there are issues with this format. Its missing ms and the Z part is not always correct.

eseglem · 2024-02-14T06:09:47Z

@vincentsarago Hopefully all make sense. Let me know if there are concerns. Tests are still passing for me, so should be good on that front.

codecov-commenter · 2024-02-15T21:51:14Z

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (main@c03272e). Click here to learn what that means.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #131   +/-   ##
=======================================
  Coverage        ?   96.67%           
=======================================
  Files           ?       26           
  Lines           ?      572           
  Branches        ?        0           
=======================================
  Hits            ?      553           
  Misses          ?       19           
  Partials        ?        0

Flag	Coverage Δ
unittests	`96.67% <100.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

stac_pydantic/shared.py

tests/api/test_search.py

stac_pydantic/shared.py

eseglem · 2024-02-16T04:04:42Z

stac_pydantic/shared.py

+    AfterValidator(lambda d: d.astimezone(timezone.utc)),
+    # Use `isoformat` to serialize the value in an RFC3339 compatible format
+    # for example: "2024-01-01T00:00:00+00:00"
+    PlainSerializer(lambda d: d.isoformat()),


All the tests pass without this serializer. I was thinking there was a case where the seconds were not included in the pydantic serializer, but I cannot reproduce that right now. I will try run through some more testing and see what I can figure out. There may be differences in pydantic versions.

Would prefer to leave it the pydantic serializer unless absolutely necessary. Also has the advantage of going back to Z over +00:00.

vincentsarago · 2024-04-10T08:23:43Z

@eseglem I'm in favour of moving forward here and for this to be published on 3.1

Can we resolve the conflicts and update the changelog?

Maybe a quick addition in the Usage section of the Readme will be a good idea as well

vincentsarago · 2024-04-24T13:19:09Z

friendly ping @eseglem 😄

stac_pydantic/api/search.py

vincentsarago · 2024-05-10T14:52:45Z

@thomas-maschler @gadomski can I have your 👍 before we merge this 🙏

thomas-maschler

Overall looks good to me. I left two comments about validators and and moving datetime to StacCommonMetadata. This can be addressed in follow up PRs

My personal preference would be to be more permissive and convert naive datetime to UTC on input but we can try and see how others perceive this.

stac_pydantic/item.py

thomas-maschler · 2024-05-10T15:59:50Z

stac_pydantic/shared.py

+    start_datetime: Optional[UtcDatetime] = None
+    end_datetime: Optional[UtcDatetime] = None


As mentioned above, the datetime/ start_datetime-end_datetime validator should live here.
It must verify that either datetime OR start_datetime AND end_datetime are set.

Just to be clear. You are saying to move the datetime field to here along with the corresponding validator. Correct? Reading through your link, that does appear to be what the spec is looking for.

It also appears to be missing license from here. That would be a separate issue, but I wanted to at least point it out.

Edit: I ended up at this comment in the code review screen without the overall context. This is much clearer now from the PR screen.

stac_pydantic/api/search.py

eseglem · 2024-05-13T13:49:48Z

@vincentsarago Sorry I have not been responsive lately, been focused on other work. Trying to catch back up now. Is there anything you still need from me on this?

eseglem · 2024-05-13T17:11:18Z

Pushed two tweaks I had been messing with. Can roll them back if preferred.

I also pushed a commit moving datetime from ItemProperties to StacCommonMetadata to a separate branch: f4426b4

It was not quite as simple / clean as I initially thought, so I wanted to allow for a separate conversation if necessary. Can create a PR from it after this, or add it here. Whatever works.

vincentsarago · 2024-05-14T08:36:46Z

@eseglem thanks 🙏

feel free to push f4426b4 in this branch 👍

It also appears to be missing license from here. That would be a separate issue, but I wanted to at least point it out.

feel also free to add this here

…to HEAD

…thub.com/eseglem/stac-pydantic into HEAD

vincentsarago · 2024-05-20T19:13:11Z

FYI I pushed f4426b4 in this branch

vincentsarago · 2024-05-20T19:16:28Z

stac_pydantic/shared.py

    model_config = ConfigDict(
        populate_by_name=True, use_enum_values=True, extra="allow"
    )
+
+    @model_validator(mode="after")
+    def validate_datetime_or_start_end(self) -> Self:


well IMO the Asset model should not herit from the StacCommonMetadata but from StacBaseModel

we can do this in another PR

vincentsarago · 2024-05-20T19:29:18Z

thanks a ton @eseglem

excellent work as usual 🙏

eseglem added 2 commits December 7, 2023 08:31

Adjust ItemProperties Validation.

e4a284f

Fixes stac-utils#130.

Remove unused imports.

ed35e75

vincentsarago reviewed Feb 8, 2024

View reviewed changes

vincentsarago mentioned this pull request Feb 8, 2024

ItemProperties modifies input data in place #130

Closed

vincentsarago reviewed Feb 8, 2024

View reviewed changes

eseglem added 2 commits February 13, 2024 13:25

Merge remote-tracking branch 'upstream/main' into bugfix/stac-pydanti…

b026dc7

…c-130-validate-item-properties

Update datetime strategy.

dd54306

eseglem requested a review from vincentsarago February 14, 2024 05:52

eseglem commented Feb 14, 2024

View reviewed changes

tests/api/extensions/test_fields.py Show resolved Hide resolved

eseglem commented Feb 14, 2024

View reviewed changes

tests/api/test_search.py Show resolved Hide resolved

eseglem commented Feb 14, 2024

View reviewed changes

stac_pydantic/item.py Show resolved Hide resolved

eseglem commented Feb 14, 2024

View reviewed changes

vincentsarago reviewed Feb 15, 2024

View reviewed changes

stac_pydantic/shared.py Outdated Show resolved Hide resolved

vincentsarago reviewed Feb 15, 2024

View reviewed changes

tests/api/test_search.py Show resolved Hide resolved

vincentsarago mentioned this pull request Feb 15, 2024

Improve Item validation rules and add support for py3.12 #134

Closed

vincentsarago reviewed Feb 15, 2024

View reviewed changes

stac_pydantic/shared.py Outdated Show resolved Hide resolved

eseglem added 2 commits February 15, 2024 21:32

Remove unnecessary Fields / aliases.

af87bcf

Tweak and add tests.

28461bf

thomas-maschler mentioned this pull request Feb 16, 2024

Correctly set default values, fix start/end/datetime field validators #135

Closed

eseglem commented Feb 16, 2024

View reviewed changes

thomas-maschler mentioned this pull request Feb 20, 2024

Datetime as null is not accepted by the STAC FASTAPI stac-utils/stac-fastapi#637

Closed

vincentsarago mentioned this pull request Apr 10, 2024

release 3.1 or 4.0? #141

Closed

vincentsarago added 2 commits April 24, 2024 16:16

update from main

bd913a8

lint

64360f4

vincentsarago reviewed Apr 24, 2024

View reviewed changes

stac_pydantic/api/search.py Show resolved Hide resolved

vincentsarago requested review from thomas-maschler, gadomski and vincentsarago and removed request for thomas-maschler April 24, 2024 14:38

vincentsarago approved these changes Apr 24, 2024

View reviewed changes

vincentsarago mentioned this pull request Apr 26, 2024

update to pydantic 2 stac-utils/stac-fastapi#625

Merged

4 tasks

gadomski approved these changes May 10, 2024

View reviewed changes

thomas-maschler reviewed May 10, 2024

View reviewed changes

rhysrevans3 mentioned this pull request May 13, 2024

Extending temporal search stac-utils/stac-fastapi-elasticsearch-opensearch#182

Open

4 tasks

eseglem added 3 commits May 13, 2024 12:53

Adjust dict_match and remove arrow usage.

202e33b

Remove unnecessary serializer.

2aa30eb

Move datetime to StacCommonMetadata.

f4426b4

vincentsarago added 2 commits May 20, 2024 21:06

Merge branch 'main' of https://github.com/stac-utils/stac-pydantic in…

72d1ace

…to HEAD

Merge commit 'f4426b43b4aa89ae56ea943ac4e543ea209d0c2c' of https://gi…

3e6648d

…thub.com/eseglem/stac-pydantic into HEAD

vincentsarago reviewed May 20, 2024

View reviewed changes

update changelog

8b7881b

vincentsarago merged commit 7b1cb44 into stac-utils:main May 20, 2024
5 checks passed

vincentsarago mentioned this pull request May 20, 2024

DATETIME_RFC339 (sic) is not a correct regex for an RFC 3339 datetime #78

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust ItemProperties Validation. #131

Adjust ItemProperties Validation. #131

eseglem commented Dec 7, 2023

vincentsarago Feb 8, 2024

eseglem Feb 9, 2024

eseglem Feb 9, 2024

vincentsarago Feb 15, 2024

eseglem Feb 16, 2024

vincentsarago Feb 16, 2024

eseglem Feb 16, 2024

vincentsarago Feb 8, 2024

eseglem Feb 9, 2024

thomas-maschler Feb 16, 2024

eseglem Feb 16, 2024

eseglem commented Feb 9, 2024

gadomski commented Feb 9, 2024

eseglem Feb 14, 2024

eseglem commented Feb 14, 2024

codecov-commenter commented Feb 15, 2024 •

edited

Loading

eseglem Feb 16, 2024

vincentsarago commented Apr 10, 2024

vincentsarago commented Apr 24, 2024

vincentsarago commented May 10, 2024

thomas-maschler left a comment •

edited

Loading

thomas-maschler May 10, 2024

eseglem May 13, 2024 •

edited

Loading

eseglem commented May 13, 2024

eseglem commented May 13, 2024

vincentsarago commented May 14, 2024

vincentsarago commented May 20, 2024

vincentsarago May 20, 2024

vincentsarago May 20, 2024

vincentsarago commented May 20, 2024

		start_datetime: Optional[UtcDatetime] = None
		end_datetime: Optional[UtcDatetime] = None

Adjust ItemProperties Validation. #131

Adjust ItemProperties Validation. #131

Conversation

eseglem commented Dec 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eseglem commented Feb 9, 2024

gadomski commented Feb 9, 2024

Choose a reason for hiding this comment

eseglem commented Feb 14, 2024

codecov-commenter commented Feb 15, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

vincentsarago commented Apr 10, 2024

vincentsarago commented Apr 24, 2024

vincentsarago commented May 10, 2024

thomas-maschler left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eseglem May 13, 2024 • edited Loading

Choose a reason for hiding this comment

eseglem commented May 13, 2024

eseglem commented May 13, 2024

vincentsarago commented May 14, 2024

vincentsarago commented May 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincentsarago commented May 20, 2024

codecov-commenter commented Feb 15, 2024 •

edited

Loading

thomas-maschler left a comment •

edited

Loading

eseglem May 13, 2024 •

edited

Loading