Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Adjust ItemProperties Validation. #131
Adjust ItemProperties Validation. #131
Changes from 13 commits
e4a284f
ed35e75
b026dc7
dd54306
af87bcf
28461bf
bd913a8
64360f4
202e33b
2aa30eb
f4426b4
72d1ace
3e6648d
8b7881b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.And the use of
.strftime("%Y-%m-%dT%H:%M:%S.%fZ")
is not guaranteed to be correct. Depending on the input timezone.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:
I would probably still go with Pydantic for parsing. Gives the most flexibility on input, and can still guarantee valid output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
and2020-01-01
.I added a possible solution in here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theZ
part is not always correct.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well IMO the Asset model should not herit from the
StacCommonMetadata
but fromStacBaseModel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can do this in another PR