Skip to content

Commit fa78869

Browse files
committed
Polish PR 233: module-level constant, fix misleading comment, add 3 unit tests
Style alignment with the rest of `waterdata/utils.py`: - Hoist `_cql2_required_services` from a function-local lowercase `set` to a module-level `_CQL2_REQUIRED_SERVICES = frozenset(...)` to match the convention of `_DATE_RANGE_PARAMS`, `_NO_NORMALIZE_PARAMS`, `_MONITORING_LOCATION_ID_RE`, etc. - Drop the "Legacy path:" prefix in the inline comment. POST/CQL2 is still the current and required path for monitoring-locations — the API team hasn't promised to add comma-GET there. Rephrased the two branches symmetrically ("POST with CQL2 JSON" / "GET with comma- separated values") so neither reads as deprecated. New unit tests: - `test_construct_api_requests_single_value_stays_get` — confirms a scalar `monitoring_location_id="USGS-..."` still produces a clean GET with no `%2C`, i.e. existing single-site callers see no change. - `test_construct_api_requests_numeric_list_joins_with_str` — pins down that `water_year=[2020, 2021]` reaches the URL as `water_year=2020%2C2021`, exercising the `str(x) for x in v` generator that exists specifically to handle non-string list params (without it, `",".join` on a list of ints would TypeError). - `test_construct_api_requests_two_element_date_list_becomes_interval` — pins down the contract that a two-element date list (`time=["2024-01-01", "2024-01-31"]`) is interpreted as start/end of an OGC datetime interval (joined with `/`), NOT as two discrete dates. The OGC `datetime` parameter doesn't support "these N specific dates" — that would require a CQL filter. Test exists so this semantic choice can't be silently changed.
1 parent 7fe353b commit fa78869

2 files changed

Lines changed: 48 additions & 7 deletions

File tree

dataretrieval/waterdata/utils.py

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,10 @@ def _switch_properties_id(properties: list[str] | None, id_name: str, service: s
153153
{"datetime", "last_modified", "begin", "begin_utc", "end", "end_utc", "time"}
154154
)
155155

156+
# Services that don't support comma-separated values for multi-value GET
157+
# parameters and require POST with CQL2 JSON instead.
158+
_CQL2_REQUIRED_SERVICES = frozenset({"monitoring-locations"})
159+
156160

157161
def _parse_datetime(value: str) -> datetime | None:
158162
"""Parse a single datetime string against the supported formats.
@@ -450,10 +454,6 @@ def _construct_api_requests(
450454
"""
451455
service_url = f"{OGC_API_URL}/collections/{service}/items"
452456

453-
# The monitoring-locations endpoint does not support comma-separated values
454-
# for multi-value GET parameters; CQL2 POST is required for that service.
455-
_cql2_required_services = {"monitoring-locations"}
456-
457457
# Format date/time parameters to ISO8601 first — both routing paths need it.
458458
for key in _DATE_RANGE_PARAMS:
459459
if key in kwargs:
@@ -462,8 +462,8 @@ def _construct_api_requests(
462462
date=(service == "daily" and key != "last_modified"),
463463
)
464464

465-
if service in _cql2_required_services:
466-
# Legacy path: POST with CQL2 for multi-value params
465+
if service in _CQL2_REQUIRED_SERVICES:
466+
# POST with CQL2 JSON: multi-value params go in the request body.
467467
post_params = {
468468
k: v
469469
for k, v in kwargs.items()
@@ -473,7 +473,7 @@ def _construct_api_requests(
473473
}
474474
params = {k: v for k, v in kwargs.items() if k not in post_params}
475475
else:
476-
# Join list/tuple values with commas for multi-value GET parameters.
476+
# GET with comma-separated values: join list/tuple values into one string.
477477
post_params = {}
478478
params = {
479479
k: ",".join(str(x) for x in v) if isinstance(v, (list, tuple)) else v

tests/waterdata_test.py

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,47 @@ def test_construct_api_requests_monitoring_locations_post():
134134
assert req.body is not None
135135

136136

137+
def test_construct_api_requests_single_value_stays_get():
138+
"""A length-1 list (or scalar) reaches the URL as a plain value, not a
139+
comma-separated form, so existing single-site callers see no change."""
140+
req = _construct_api_requests(
141+
"daily",
142+
monitoring_location_id="USGS-05427718",
143+
parameter_code="00060",
144+
)
145+
assert req.method == "GET"
146+
assert "monitoring_location_id=USGS-05427718" in req.url
147+
assert "%2C" not in req.url # no comma-encoded multi-value
148+
149+
150+
def test_construct_api_requests_numeric_list_joins_with_str():
151+
"""Numeric-list params (e.g. ``water_year=[2020, 2021]`` on get_peaks)
152+
must reach the URL as a comma-joined string, not crash on ``",".join``
153+
of ints. The generator-of-``str(x)`` exists exactly for this case."""
154+
req = _construct_api_requests(
155+
"peaks",
156+
monitoring_location_id="USGS-05427718",
157+
water_year=[2020, 2021],
158+
)
159+
assert req.method == "GET"
160+
assert "water_year=2020%2C2021" in req.url
161+
162+
163+
def test_construct_api_requests_two_element_date_list_becomes_interval():
164+
"""A two-element date list is interpreted as start/end of an OGC datetime
165+
interval (joined with '/'), NOT as two discrete dates. The OGC `datetime`
166+
parameter does not support "these N specific dates" — that would require
167+
a CQL filter. Verifying so this contract is locked in."""
168+
req = _construct_api_requests(
169+
"daily",
170+
monitoring_location_id="USGS-05427718",
171+
time=["2024-01-01", "2024-01-31"],
172+
)
173+
assert req.method == "GET"
174+
# `/` URL-encodes to %2F. Confirms _format_api_dates ran before the join.
175+
assert "time=2024-01-01%2F2024-01-31" in req.url
176+
177+
137178
def test_samples_results():
138179
"""Test results call for proper columns"""
139180
df, _ = get_samples(

0 commit comments

Comments
 (0)