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

PB-1495 Remove date comparison for property expires #537

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

asteiner-swisstopo
Copy link
Contributor

In the forecast extension, datetime is the forecast date. So datetime can be anywhere in the future and expires might be before that. So comparing these two dates makes no sense. Same for end_datetime.

The expires field is part of the Timestamps extension, so it might be used in different contexts where this check would actually make sense. But ultimately, it is the responsibility of the data owner that the data is consistent. So we remove the check to reduce complexity on our side.

ℹ️ The definition of the fields according to the forecast extension docs:

  • datetime: REQUIRED. The forecast datetime. It follows the definition in the STAC Common Metdata. If the forecast is not only for a specific instance in time but instead is for a certain period, you should use start_datetime and end_datetime and set datetime either to reflect the start_datetime (recommended) or to null.
  • expires: The datetime until the forecast is valid or gets superseded by a new forecast. It follows the definition in the Timestamps Extension.

In the forecast extension, `datetime` is the forecast date. So `datetime` can be anywhere in the future and `expires` might be before that. So comparing these two dates makes no sense. Same for `end_datetime`.

The `expires` field is part of the Timestamps extension, so it might be used in different contexts where this check would actually make sense. But ultimately, it is the responsibility of the data owner that the data is consistent. So we remove the check to reduce complexity on our side.
Copy link
Contributor

@boecklic boecklic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of just removing the check completely, we could think about adapting it to sth that makes more sense than what we have currently

Comment on lines -438 to -441
if properties_expires is not None:
if properties_expires < properties_datetime:
message = "Property expires can't refer to a date earlier than property datetime"
raise ValidationError(_(message), code='invalid')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could replace that with a meaningful check: having an expires timestamp in the past doesn't make sense because it would immediately be deleted again, so this is probably a case we don't want to allow.

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

Successfully merging this pull request may close these issues.

2 participants