PB-847: Add item field 'expires' #450
Conversation
Add optional field 'expires' to item property.Field should conform to stac-extensions/timestamps (https://github.com/stac-extensions/timestamps). Adapt test cases and spec.
| @@ -374,32 +386,23 @@ def validate_item_properties_datetimes_dependencies( | |||
| Raises: | |||
There was a problem hiding this comment.
I think now the documentation is incomplete here
| @@ -364,8 +364,20 @@ def validate_geometry(geometry): | |||
| return geometry | |||
|
|
|||
|
|
|||
| def validate_item_properties_datetimes_dependencies( | |||
| properties_datetime, properties_start_datetime, properties_end_datetime | |||
| def validate_datetime_format(date_string): | |||
app/stac_api/validators.py
Outdated
| properties_datetime, properties_start_datetime, properties_end_datetime | ||
| def validate_datetime_format(date_string): | ||
| try: | ||
| if not isinstance(date_string, datetime) and date_string is not None: |
There was a problem hiding this comment.
Just some nitpick: wouldn't it make sense to first check for None and then for the instance?
|
|
||
|
|
||
| def validate_item_properties_datetimes( | ||
| properties_datetime, properties_start_datetime, properties_end_datetime, properties_expires |
There was a problem hiding this comment.
Just some higher-level thinking here, (it doesn't have to do with your change directly):
I think it might be better if the callers of this function made sure that what they pass in is either datetime or None (but not string). Instead, the above function, as a side-effect, parses the dates in case they are strings, even though this here is about validation... (I guess that's a result of not having strict typing 😁)
There was a problem hiding this comment.
I played around with different options, but nothing that is really nicer.
| @@ -1072,6 +1072,8 @@ components: | |||
| $ref: "#/components/schemas/datetime" | |||
| end_datetime: | |||
| $ref: "#/components/schemas/datetime" | |||
There was a problem hiding this comment.
Is it meant to be on both versions of our API (v0.9 and v1)
There was a problem hiding this comment.
AFAIK the v0.9 spec should not change anymore. The new feature should only be in the v1 spec.
To keep things easier (and as it is not a breaking change), I decided to add the field to response of both versions. We could also check for v0.9 and remove the 'expires' field from the response again.
Split validation function. Remove wrapper function validate_item_properties_datetimes_dependencies.
benschs
force-pushed
the
feat-PB-847-item-expires-field
branch
from
b9e949b to
bc3a96d
Compare
There was a problem hiding this comment.
Nice!
If we want to be perfect, the standard suggest to add extensions to the corresponding stac_extensions list, e.g. for items: https://github.com/radiantearth/stac-spec/blob/master/item-spec/item-spec.md#stac_extensions
We haven't done this so far though
| properties_end_datetime, | ||
| ) | ||
| if properties_expires is not None: | ||
| if properties_expires < properties_end_datetime: |
There was a problem hiding this comment.
maybe we should even enforce a minimal lifetime of an object, e.g. 24/72/... hours?
There was a problem hiding this comment.
As discussed offline, we are not sure about the different use cases, so for now we keep it simple and don't add more restrictions.
Add optional field 'expires' to item property.
New field should conform to stac-extensions/timestamps (https://github.com/stac-extensions/timestamps).
Refactor item properties validation function.