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.

Sign up for GitHub

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

Add validation for ISO 8601 durations to MeasurementType.duration #16

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Add validation for ISO 8601 durations to MeasurementType.duration #16

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1fe5eb2
feat!: replace `period` with `duration` in `MeasurementType` model
mo-lukecarr Dec 16, 2024
672da94
fix: replaced `period` with `duration` in JSON test data
mo-lukecarr Dec 16, 2024
0df82ed
feat: added ISO 8601 validation for `MeasurementType.duration` field
mo-lukecarr Dec 16, 2024
dc93ac2
feat: added validation when `MeasurementType.mode` is `instantaneous`
mo-lukecarr Dec 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 35 additions & 4 deletions src/edr_pydantic/parameter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,54 @@
from datetime import timedelta
from typing import Annotated
from typing import Dict
from typing import List
from typing import Literal
from typing import Optional

from pydantic import AfterValidator
from pydantic import model_validator
from pydantic import RootModel
from pydantic import TypeAdapter

from .base_model import EdrBaseModel
from .extent import Extent
from .observed_property import ObservedProperty
from .unit import Unit

duration_adapter = TypeAdapter(timedelta)


def check_iso8601_duration(value: str) -> str:
if "/" in value:
parts = value.split("/")

if len(parts) != 2:
raise ValueError("Duration must have two parts if it contains a '/'")

duration_adapter.validate_python(parts[0])
duration_adapter.validate_python(parts[1])
else:
duration_adapter.validate_python(value)

return value
Comment on lines +21 to +33
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for making these improvements.

I don't think we want to keep supporting the duration field with a / in there, see my comment on your other PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

On the timedelta validation that you use, I think this relies on pydantic datetime validation, correct?
if I look here, it looks like this only supports part of the ISO8601 duration format:
[±]P[DD]DT[HH]H[MM]M[SS]S

I think this change would no longer allow periods like P1M, which is not consistent with the spec.

What is your view on this?



ISO8601Duration = Annotated[str, AfterValidator(check_iso8601_duration)]


class MeasurementType(EdrBaseModel):
method: str
# TODO: Add validation of ISO 8601 duration (including leading minus sign)
# TODO: Confusion in spec on the field name, duration versus period.
# See https://github.com/opengeospatial/ogcapi-environmental-data-retrieval/issues/560
period: str
duration: ISO8601Duration

@model_validator(mode="after")
def must_have_single_duration_if_instantaneous_method(self):
if self.method == "instantaneous" and "/" in self.duration:
raise ValueError(
"A measurement type object with 'instantaneous' method "
"MUST have a single duration."
)

return self
Comment on lines +43 to +51
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hesitant about this additional validation logic. I can't see anything formal in the specification that says the duration must be a single duration (e.g. PT0M) when the mode is instantaneous. However, this is the convention that I've seen in all EDR collections (including the JSON test data in this repository and the examples within the full EDR specification).



class Parameter(EdrBaseModel, extra="allow"):
Expand Down
40 changes: 20 additions & 20 deletions tests/test_data/doc-example-collections.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@
},
"measurementType": {
"method": "mean",
"period": "-PT10M/PT0M"
"duration": "-PT10M/PT0M"
}
},
"Wind Speed": {
Expand All @@ -217,7 +217,7 @@
},
"measurementType": {
"method": "mean",
"period": "-PT10M/PT0M"
"duration": "-PT10M/PT0M"
}
},
"Wind Gust": {
Expand All @@ -236,7 +236,7 @@
},
"measurementType": {
"method": "maximum",
"period": "-PT10M/PT0M"
"duration": "-PT10M/PT0M"
}
},
"Air Temperature": {
Expand All @@ -255,7 +255,7 @@
},
"measurementType": {
"method": "instantaneous",
"period": "PT0M"
"duration": "PT0M"
}
},
"Weather": {
Expand All @@ -274,7 +274,7 @@
},
"measurementType": {
"method": "instantaneous",
"period": "PT0M"
"duration": "PT0M"
}
},
"Relative Humidity": {
Expand All @@ -293,7 +293,7 @@
},
"measurementType": {
"method": "instantaneous",
"period": "PT0M"
"duration": "PT0M"
}
},
"Dew point": {
Expand All @@ -312,7 +312,7 @@
},
"measurementType": {
"method": "instantaneous",
"period": "PT0M"
"duration": "PT0M"
}
},
"Pressure": {
Expand All @@ -331,7 +331,7 @@
},
"measurementType": {
"method": "instantaneous",
"period": "PT0M"
"duration": "PT0M"
}
},
"Pressure Tendancy": {
Expand All @@ -350,7 +350,7 @@
},
"measurementType": {
"method": "instantaneous",
"period": "PT0M"
"duration": "PT0M"
}
},
"Visibility": {
Expand All @@ -369,7 +369,7 @@
},
"measurementType": {
"method": "instantaneous",
"period": "PT0M"
"duration": "PT0M"
}
}
}
Expand Down Expand Up @@ -539,7 +539,7 @@
},
"measurementType": {
"method": "mean",
"period": "-PT10M/PT0M"
"duration": "-PT10M/PT0M"
}
},
"Wind Speed": {
Expand All @@ -558,7 +558,7 @@
},
"measurementType": {
"method": "mean",
"period": "-PT10M/PT0M"
"duration": "-PT10M/PT0M"
}
},
"Wind Gust": {
Expand All @@ -577,7 +577,7 @@
},
"measurementType": {
"method": "maximum",
"period": "-PT10M/PT0M"
"duration": "-PT10M/PT0M"
}
},
"Air Temperature": {
Expand All @@ -596,7 +596,7 @@
},
"measurementType": {
"method": "instantaneous",
"period": "PT0M"
"duration": "PT0M"
}
},
"Weather": {
Expand All @@ -615,7 +615,7 @@
},
"measurementType": {
"method": "instantaneous",
"period": "PT0M"
"duration": "PT0M"
}
},
"Relative Humidity": {
Expand All @@ -634,7 +634,7 @@
},
"measurementType": {
"method": "instantaneous",
"period": "PT0M"
"duration": "PT0M"
}
},
"Feels like temperature": {
Expand All @@ -653,7 +653,7 @@
},
"measurementType": {
"method": "instantaneous",
"period": "PT0M"
"duration": "PT0M"
}
},
"UV index": {
Expand All @@ -672,7 +672,7 @@
},
"measurementType": {
"method": "instantaneous",
"period": "PT0M"
"duration": "PT0M"
}
},
"Probability of precipitation": {
Expand All @@ -691,7 +691,7 @@
},
"measurementType": {
"method": "instantaneous",
"period": "PT0M"
"duration": "PT0M"
}
},
"Visibility": {
Expand All @@ -710,7 +710,7 @@
},
"measurementType": {
"method": "instantaneous",
"period": "PT0M"
"duration": "PT0M"
}
}
}
Expand Down
6 changes: 3 additions & 3 deletions tests/test_data/parameter-names.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
},
"measurementType": {
"method": "instantaneous",
"period": "PT0S"
"duration": "PT0S"
}
},
"u-component_of_wind_altitude_above_msl": {
Expand All @@ -34,7 +34,7 @@
},
"measurementType": {
"method": "instantaneous",
"period": "PT0S"
"duration": "PT0S"
}
},
"v-component_of_wind_altitude_above_msl": {
Expand All @@ -53,7 +53,7 @@
},
"measurementType": {
"method": "instantaneous",
"period": "PT0S"
"duration": "PT0S"
}
}
}
2 changes: 1 addition & 1 deletion tests/test_data/parameter-with-extent.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,6 @@
},
"measurementType": {
"method": "instantaneous",
"period": "PT0S"
"duration": "PT0S"
}
}