From f136f6567d2330e9a8a7a6f61089cd4738098a8c Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Wed, 12 Aug 2026 17:14:42 -0500 Subject: [PATCH 1/2] refactor: one home for option validation; retire dead engine wrappers Two generalizations found by scanning for repeated shapes. **Closed-vocabulary rejection was written eleven times.** Every adapter checks an argument against a Literal's get_args(), a module constant, or a mapping's keys, and each hand-wrote the raise -- eight message phrasings for one concept, so each new check was a coin flip on wording. One had already lost that flip: `get_reference_table` told callers who passed a bad `collection` that their *code service* was invalid. The check had been copied from `samples.get_codes`, message and local variable name (`valid_code_services`) included, and the noun was never changed -- naming a parameter that function does not have. A regression test pins the corrected wording. `_validation.require_one_of` now owns the rejection; the vocabularies stay with the adapters that define them. Migrated: waterdata types (service, profile), samples, reference, cql, nearest, ogc.schema, wqp (dataProfile, service), nldi (find, navigation_mode). Messages are now uniform and name the parameter the caller passed; the pinned assertions move with them. Deliberately not migrated: `nwis` raises TypeError here rather than ValueError, and changing that on an ADR 0005 quarantined module is a behavior change, not a cleanup; `ratings.get_ratings` reports every invalid file_type at once, which the scalar helper would lose. **`ogc.engine._paginate` was a wrapper with one production caller.** It added two things over `transport.pagination.paginate`: the OGC raise-for-status default, and preferring the running drive's client over a new one. The second was written twice -- here and inside `run_paginated.fetch` -- so it moves into `_client_for`, where both get it and neither restates it. `_walk_pages` now calls `paginate` directly. Also removed, all verified to have zero consumers: `_DEFAULT_DIALECT` (whose comment claimed tests used it -- none do), `ogc.requests._get_args` (every `_get_args` in the repo resolves to waterdata.utils'), and `utils._network_error`. `utils.USER_AGENT` is left: un-underscored on a documented compatibility module, so removing it is a release decision. `_validation` is placed at the floor of the layers contract -- it imports nothing first-party -- so every layer can reject a bad option without reaching sideways. 788 passed, mypy --strict clean, all hooks including import-linter pass. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01BTaSm7HmVb94RSJiKW4WAS --- .importlinter | 4 ++ dataretrieval/_validation.py | 67 +++++++++++++++++++++++++++ dataretrieval/nldi.py | 13 ++---- dataretrieval/ogc/__init__.py | 3 +- dataretrieval/ogc/engine.py | 53 ++++++--------------- dataretrieval/ogc/requests.py | 4 -- dataretrieval/ogc/schema.py | 4 +- dataretrieval/transport/pagination.py | 14 ++++-- dataretrieval/utils.py | 1 - dataretrieval/waterdata/cql.py | 7 +-- dataretrieval/waterdata/nearest.py | 4 +- dataretrieval/waterdata/reference.py | 8 +--- dataretrieval/waterdata/samples.py | 8 +--- dataretrieval/waterdata/types.py | 21 ++++----- dataretrieval/wqp.py | 13 ++---- tests/nldi_test.py | 2 +- tests/waterdata_progress_test.py | 7 ++- tests/waterdata_test.py | 18 +++++-- tests/wqp_test.py | 7 +-- 19 files changed, 148 insertions(+), 110 deletions(-) create mode 100644 dataretrieval/_validation.py diff --git a/.importlinter b/.importlinter index 8379e909..764a4580 100644 --- a/.importlinter +++ b/.importlinter @@ -33,6 +33,10 @@ layers = _ambient | _response_metadata | codes | combining | interruptions | rdb credentials exceptions +; Argument validation sits at the floor: it raises the stdlib's ``ValueError`` +; and imports nothing first-party, so every layer above can reject a bad +; option without reaching sideways for a helper. + _validation ; Every top-level module must be placed in the stack deliberately. A new ; top-level module fails this contract until someone decides where it sits. exhaustive = True diff --git a/dataretrieval/_validation.py b/dataretrieval/_validation.py new file mode 100644 index 00000000..639cc252 --- /dev/null +++ b/dataretrieval/_validation.py @@ -0,0 +1,67 @@ +"""Argument checks shared by every adapter. + +Rejecting a value that is not in a closed vocabulary is the one validation +every adapter does, and it was written eleven times: eight message phrasings +for one concept, so each new check was a coin flip on wording. That is how +:func:`~dataretrieval.waterdata.get_reference_table` came to tell callers who +passed a bad ``collection`` that their *code service* was invalid -- the check +was copied from :mod:`~dataretrieval.waterdata.samples`, message and local +variable name included, and the noun was never changed. + +This module owns the wording so a new check cannot invent its own. It is a +leaf with no first-party imports: the vocabularies it validates against live +with the adapters that define them, and only the rejection is shared. +""" + +from __future__ import annotations + +from collections.abc import Collection + + +def _render(options: Collection[object]) -> str: + """Format *options* for a message: ``'a', 'b', 'c'``. + + Renders the values rather than their container so ``dict_keys([...])`` and + a bare tuple read the same to a caller, who never sees the container. + """ + return ", ".join(repr(option) for option in options) + + +def require_one_of( + value: object, + options: Collection[object], + *, + name: str, + context: str = "", +) -> None: + """Raise ``ValueError`` unless *value* is one of *options*. + + Parameters + ---------- + value + The argument the caller supplied. + options + The closed vocabulary it must belong to -- typically + ``get_args(SomeLiteral)``, a module constant, or a mapping's keys. + Rendered in iteration order, so pass a sorted view when the source is + unordered and the order would otherwise be arbitrary. + name + What the value *is*, as the caller's parameter names it (``"service"``, + ``"collection"``). It becomes the message's subject, so it must match + the parameter the caller actually passed. + context + Optional qualifier for a vocabulary that depends on another argument, + e.g. ``context="service 'wqp'"`` when the valid profiles differ per + service. + + Raises + ------ + ValueError + If *value* is not in *options*. + """ + if value in options: + return + qualifier = f" for {context}" if context else "" + raise ValueError( + f"Invalid {name}: {value!r}{qualifier}. Valid options are: {_render(options)}." + ) diff --git a/dataretrieval/nldi.py b/dataretrieval/nldi.py index c32efd10..cb216d48 100644 --- a/dataretrieval/nldi.py +++ b/dataretrieval/nldi.py @@ -14,6 +14,7 @@ from typing import Any, Literal, cast from dataretrieval._querying import _query_with_retry +from dataretrieval._validation import require_one_of __all__ = [ "get_flowlines", @@ -473,11 +474,7 @@ def search( raise ValueError("Both lat and long are required") find = cast("Literal['basin', 'flowlines', 'features']", find.lower()) - if find not in ("basin", "flowlines", "features"): - raise ValueError( - f"Invalid value for find: {find} - allowed values are:" - f" 'basin', 'flowlines', or 'features'" - ) + require_one_of(find, ("basin", "flowlines", "features"), name="find") if lat is not None and find != "features": raise ValueError( f"Invalid value for find: {find} - lat/long is to get features not {find}" @@ -559,11 +556,7 @@ def _validate_navigation_mode(navigation_mode: str | None) -> str: f"navigation_mode is required; allowed values are {_VALID_NAVIGATION_MODES}" ) normalized = navigation_mode.upper() - if normalized not in _VALID_NAVIGATION_MODES: - raise ValueError( - f"Invalid navigation mode '{navigation_mode}';" - f" allowed values are {_VALID_NAVIGATION_MODES}" - ) + require_one_of(normalized, _VALID_NAVIGATION_MODES, name="navigation_mode") return normalized diff --git a/dataretrieval/ogc/__init__.py b/dataretrieval/ogc/__init__.py index d7de1c7b..025d3fb1 100644 --- a/dataretrieval/ogc/__init__.py +++ b/dataretrieval/ogc/__init__.py @@ -10,8 +10,7 @@ Collection adapters (NGWMN, Water Data's generic wrapper) import from this facade rather than reaching into engine internals — every name here is usable through the facade alone. Generic execution policy lives in -:mod:`dataretrieval.transport`; the engine retains compatibility wrappers at -previous private paths. +:mod:`dataretrieval.transport`, which the engine now calls directly. """ from dataretrieval.ogc.engine import get_ogc_data diff --git a/dataretrieval/ogc/engine.py b/dataretrieval/ogc/engine.py index 5c5236d8..67895f11 100644 --- a/dataretrieval/ogc/engine.py +++ b/dataretrieval/ogc/engine.py @@ -27,10 +27,9 @@ import functools import logging from collections.abc import ( - Awaitable, Callable, ) -from typing import TYPE_CHECKING, Any, TypeVar +from typing import TYPE_CHECKING, Any import httpx import pandas as pd @@ -52,7 +51,7 @@ _switch_properties_id, ) from dataretrieval.ogc.shaping import GEOPANDAS, _finalize_ogc, _get_resp_data -from dataretrieval.transport.fanout import FanOut, active_client +from dataretrieval.transport.fanout import FanOut from dataretrieval.transport.links import resolve_next_url from dataretrieval.transport.pagination import paginate from dataretrieval.transport.retry import RetryPolicy @@ -63,9 +62,6 @@ # Set up logger for this module logger = logging.getLogger(__name__) -# Compatibility alias: the old name used internally and in tests. -_DEFAULT_DIALECT = DEFAULT_DIALECT - def _next_req_url( resp: httpx.Response, *, body: dict[str, Any] | None = None @@ -120,39 +116,16 @@ def _next_req_url( return None -_Cursor = TypeVar("_Cursor") - - -async def _paginate( - initial_req: httpx.Request, - *, - parse_response: Callable[[httpx.Response], tuple[pd.DataFrame, _Cursor | None]], - follow_up: Callable[[_Cursor, httpx.AsyncClient], Awaitable[httpx.Response]], - client: httpx.AsyncClient | None = None, - raise_for_status: Callable[[httpx.Response], None] = _raise_for_non_200, - row_cap: int | None = None, -) -> tuple[pd.DataFrame, httpx.Response]: - """Compatibility wrapper around collection-neutral cursor pagination.""" - session = client if client is not None else active_client() - return await paginate( - initial_req, - parse_response=parse_response, - follow_up=follow_up, - client=session, - raise_for_status=raise_for_status, - row_cap=row_cap, - ) - - def _ogc_parse_response( resp: httpx.Response, *, geopd: bool ) -> tuple[pd.DataFrame, str | None]: """Parse one OGC API page: extract the DataFrame and the next-page URL. The parse strategy :func:`_walk_pages` hands to - :func:`_paginate`. Coerces falsy cursors (empty href, etc.) to - ``None`` so the paginate loop's ``while cursor is not None`` - terminates instead of spinning on a meaningless value. + :func:`~dataretrieval.transport.pagination.paginate`. Coerces falsy + cursors (empty href, etc.) to ``None`` so the paginate loop's + ``while cursor is not None`` terminates instead of spinning on a + meaningless value. """ body = resp.json() return ( @@ -171,7 +144,8 @@ async def _walk_pages( """ Iterate paginated OGC API responses and aggregate them into one DataFrame. - Thin wrapper that hands off to :func:`_paginate` with + Thin wrapper that hands off to + :func:`~dataretrieval.transport.pagination.paginate` with OGC-specific strategies: pages are parsed via :func:`_get_resp_data` (through :func:`_ogc_parse_response`) and the next-page cursor is the URL from the response's ``links`` array (per :func:`_next_req_url`). @@ -184,7 +158,7 @@ async def _walk_pages( The initial HTTP request to send. client : httpx.AsyncClient, optional Caller-borrowed client; ``None`` defers client management to - :func:`_paginate`. + :func:`~dataretrieval.transport.pagination.paginate`. row_cap : int, optional Stop following pages once this many rows have accumulated and truncate to exactly this many. ``None`` (default) walks every page. @@ -203,9 +177,9 @@ async def _walk_pages( Raises ------ DataRetrievalError - See :func:`_paginate`. + See :func:`~dataretrieval.transport.pagination.paginate`. httpx.HTTPError - See :func:`_paginate`. + See :func:`~dataretrieval.transport.pagination.paginate`. """ method = req.method # ``httpx.Request.method`` is already upper-cased. headers = req.headers @@ -214,11 +188,12 @@ async def _walk_pages( async def follow_up(cursor: str, sess: httpx.AsyncClient) -> httpx.Response: return await sess.request(method, cursor, headers=headers, content=content) - return await _paginate( + return await paginate( req, parse_response=functools.partial(_ogc_parse_response, geopd=geopd), follow_up=follow_up, client=client, + raise_for_status=_raise_for_non_200, row_cap=row_cap, ) @@ -301,7 +276,7 @@ def get_ogc_data( _require_positive_int(max_rows, "max_rows") if dialect is None: - dialect = _DEFAULT_DIALECT + dialect = DEFAULT_DIALECT args = args.copy() args["collection"] = collection args = _switch_arg_id(args, id_name=output_id, collection=collection) diff --git a/dataretrieval/ogc/requests.py b/dataretrieval/ogc/requests.py index 2520d1d9..d3b76ae4 100644 --- a/dataretrieval/ogc/requests.py +++ b/dataretrieval/ogc/requests.py @@ -332,7 +332,3 @@ def prepare_request_args( else: args[k] = _normalize_str_iterable(v, k) return args - - -# Compatibility alias for existing private imports from ``ogc.engine``. -_get_args = prepare_request_args diff --git a/dataretrieval/ogc/schema.py b/dataretrieval/ogc/schema.py index 25df084f..f85b170a 100644 --- a/dataretrieval/ogc/schema.py +++ b/dataretrieval/ogc/schema.py @@ -13,6 +13,7 @@ import pandas as pd from dataretrieval._response_metadata import BaseMetadata +from dataretrieval._validation import require_one_of from dataretrieval.ogc.errors import _raise_for_non_200 from dataretrieval.transport.http import HTTPX_DEFAULTS from dataretrieval.transport.http import default_headers as _default_headers @@ -27,8 +28,7 @@ def _check_ogc_requests( ``base_url`` names the API to ask; it defaults to the one in scope for the current call rather than to any particular collection. """ - if req_type not in ("queryables", "schema"): - raise ValueError(f"req_type must be 'queryables' or 'schema', got {req_type!r}") + require_one_of(req_type, ("queryables", "schema"), name="req_type") url = f"{base_url}/collections/{endpoint}/{req_type}" response = _get(url, headers=_default_headers(url), **HTTPX_DEFAULTS) _raise_for_non_200(response) diff --git a/dataretrieval/transport/pagination.py b/dataretrieval/transport/pagination.py index 523463c6..052ad940 100644 --- a/dataretrieval/transport/pagination.py +++ b/dataretrieval/transport/pagination.py @@ -42,9 +42,15 @@ async def _client_for( client: httpx.AsyncClient | None, ) -> AsyncIterator[httpx.AsyncClient]: - """Borrow a caller client or open a guarded short-lived client.""" - if client is not None: - yield client + """Borrow a client: the caller's, else the running drive's, else a new one. + + Preferring the executor's published client over a fresh one keeps every + page of every request on one connection pool. Both callers wanted that and + each spelled it itself before it moved here. + """ + borrowed = client if client is not None else active_client() + if borrowed is not None: + yield borrowed return async with open_async_client() as new: yield new @@ -177,7 +183,7 @@ async def fetch(request: httpx.Request) -> tuple[pd.DataFrame, httpx.Response]: request, parse_response=parse_response, follow_up=follow_up, - client=client if client is not None else active_client(), + client=client, raise_for_status=raise_for_status, ) diff --git a/dataretrieval/utils.py b/dataretrieval/utils.py index 5e6bc09f..924f8881 100644 --- a/dataretrieval/utils.py +++ b/dataretrieval/utils.py @@ -30,7 +30,6 @@ USER_AGENT = _transport_http.USER_AGENT _default_headers = _transport_http.default_headers _get = _transport_http.get -_network_error = _transport_http.network_error # Public functions whose implementation moved to the private query module; this # is the path they are documented at. query = _querying.query diff --git a/dataretrieval/waterdata/cql.py b/dataretrieval/waterdata/cql.py index 1da51d75..0f82e9c4 100644 --- a/dataretrieval/waterdata/cql.py +++ b/dataretrieval/waterdata/cql.py @@ -14,6 +14,7 @@ import pandas as pd +from dataretrieval._validation import require_one_of from dataretrieval.waterdata.utils import ( _OUTPUT_ID_BY_COLLECTION, _accept_legacy_kwargs, @@ -136,11 +137,7 @@ def get_cql( ... ' "02070010%"]}', ... ) """ - if collection not in _OUTPUT_ID_BY_COLLECTION: - raise ValueError( - f"Unknown collection {collection!r}. Valid collections: " - f"{sorted(_OUTPUT_ID_BY_COLLECTION)}." - ) + require_one_of(collection, sorted(_OUTPUT_ID_BY_COLLECTION), name="collection") # ``dict`` is the pythonic input — serialize on the way out. ``str`` is sent # verbatim so callers who already have a CQL2 doc (e.g. imported from a diff --git a/dataretrieval/waterdata/nearest.py b/dataretrieval/waterdata/nearest.py index b9c6b498..1e9b83c0 100644 --- a/dataretrieval/waterdata/nearest.py +++ b/dataretrieval/waterdata/nearest.py @@ -10,6 +10,7 @@ import pandas as pd +from dataretrieval._validation import require_one_of from dataretrieval.waterdata.time_series import get_continuous if TYPE_CHECKING: @@ -205,8 +206,7 @@ def _check_nearest_kwargs(kwargs: dict[str, Any], on_tie: OnTie) -> None: f"get_nearest_continuous constructs its own {forbidden!r}; " "do not pass it directly" ) - if on_tie not in _VALID_ON_TIE: - raise ValueError(f"on_tie must be one of {_VALID_ON_TIE}; got {on_tie!r}") + require_one_of(on_tie, _VALID_ON_TIE, name="on_tie") def _build_window_or_filter(targets: pd.DatetimeIndex, window_td: pd.Timedelta) -> str: diff --git a/dataretrieval/waterdata/reference.py b/dataretrieval/waterdata/reference.py index 14a1884b..7e079567 100644 --- a/dataretrieval/waterdata/reference.py +++ b/dataretrieval/waterdata/reference.py @@ -12,6 +12,7 @@ import pandas as pd +from dataretrieval._validation import require_one_of from dataretrieval.ogc.schema import queryables_frame from dataretrieval.waterdata.types import ( METADATA_COLLECTIONS, @@ -93,12 +94,7 @@ def get_reference_table( ... query={"id": "00001,00002"}, ... ) """ - valid_code_services = get_args(METADATA_COLLECTIONS) - if collection not in valid_code_services: - raise ValueError( - f"Invalid code service: '{collection}'. " - f"Valid options are: {valid_code_services}." - ) + require_one_of(collection, get_args(METADATA_COLLECTIONS), name="collection") # Give the ID column the collection name, singularized and underscored. if collection == "counties": diff --git a/dataretrieval/waterdata/samples.py b/dataretrieval/waterdata/samples.py index 642fd05a..2f21d26c 100644 --- a/dataretrieval/waterdata/samples.py +++ b/dataretrieval/waterdata/samples.py @@ -20,6 +20,7 @@ from dataretrieval._querying import to_str from dataretrieval._response_metadata import BaseMetadata +from dataretrieval._validation import require_one_of from dataretrieval._wqx import _attach_datetime_columns from dataretrieval.ogc.errors import _raise_for_non_200 from dataretrieval.transport.http import ( @@ -63,12 +64,7 @@ def get_codes(code_service: CODE_SERVICES) -> tuple[pd.DataFrame, BaseMetadata]: md : :obj:`dataretrieval.utils.BaseMetadata` Metadata for the query (URL, query time, response headers). """ - valid_code_services = get_args(CODE_SERVICES) - if code_service not in valid_code_services: - raise ValueError( - f"Invalid code service: '{code_service}'. " - f"Valid options are: {valid_code_services}." - ) + require_one_of(code_service, get_args(CODE_SERVICES), name="code_service") url = f"{SAMPLES_URL}/codeservice/{code_service}?mimeType=application%2Fjson" diff --git a/dataretrieval/waterdata/types.py b/dataretrieval/waterdata/types.py index 65a61d48..bdc408e0 100644 --- a/dataretrieval/waterdata/types.py +++ b/dataretrieval/waterdata/types.py @@ -1,5 +1,7 @@ from typing import Literal, get_args +from dataretrieval._validation import require_one_of + __all__ = [ "CODE_SERVICES", "METADATA_COLLECTIONS", @@ -125,15 +127,10 @@ def _check_profiles( "locations_profiles", "activities_profiles", "projects_profiles" or "organizations_profiles". """ - valid_services = get_args(SERVICES) - if service not in valid_services: - raise ValueError( - f"Invalid service: '{service}'. Valid options are: {valid_services}." - ) - - valid_profiles = PROFILE_LOOKUP[service] - if profile not in valid_profiles: - raise ValueError( - f"Invalid profile: '{profile}' for service '{service}'. " - f"Valid options are: {valid_profiles}." - ) + require_one_of(service, get_args(SERVICES), name="service") + require_one_of( + profile, + PROFILE_LOOKUP[service], + name="profile", + context=f"service {service!r}", + ) diff --git a/dataretrieval/wqp.py b/dataretrieval/wqp.py index 2a41fd96..609350aa 100644 --- a/dataretrieval/wqp.py +++ b/dataretrieval/wqp.py @@ -17,6 +17,7 @@ import pandas as pd from dataretrieval._response_metadata import BaseMetadata +from dataretrieval._validation import require_one_of from ._querying import _query_with_retry from ._wqx import _attach_datetime_columns @@ -191,10 +192,9 @@ def get_results( url = wqx3_url("Result") profile = kwargs.get("dataProfile") - if profile is not None and profile not in valid_profiles: - raise ValueError( - f"dataProfile {profile!r} is not a valid {kind} profile. " - f"Valid options are {valid_profiles}." + if profile is not None: + require_one_of( + profile, valid_profiles, name="dataProfile", context=f"{kind} results" ) if legacy is not True and profile is None: kwargs["dataProfile"] = "fullPhysChem" @@ -624,10 +624,7 @@ def what_activity_metrics( def _validate_service(service: str, valid_services: list[str], profile: str) -> None: """Validate a service against one WQP profile's supported endpoints.""" - if service not in valid_services: - raise ValueError( - f"{profile} service not recognized. Valid options are {valid_services}." - ) + require_one_of(service, valid_services, name="service", context=profile) def wqp_url(service: str) -> str: diff --git a/tests/nldi_test.py b/tests/nldi_test.py index d60c3886..6356f8de 100644 --- a/tests/nldi_test.py +++ b/tests/nldi_test.py @@ -387,7 +387,7 @@ def test_search_flowlines_without_navigation_mode_raises_value_error(): def test_validate_navigation_mode_raises_value_error_for_invalid(): """Regression: previously raised TypeError; should be ValueError.""" - with pytest.raises(ValueError, match="Invalid navigation mode"): + with pytest.raises(ValueError, match="Invalid navigation_mode"): _validate_navigation_mode("XX") diff --git a/tests/waterdata_progress_test.py b/tests/waterdata_progress_test.py index 2f789f43..9179f1b3 100644 --- a/tests/waterdata_progress_test.py +++ b/tests/waterdata_progress_test.py @@ -19,13 +19,15 @@ from dataretrieval import progress as _progress from dataretrieval.ogc.chunking import ChunkedCall -from dataretrieval.ogc.engine import _paginate, _walk_pages +from dataretrieval.ogc.engine import _walk_pages +from dataretrieval.ogc.errors import _raise_for_non_200 from dataretrieval.ogc.planning import ChunkPlan from dataretrieval.progress import ( ProgressReporter, current, progress_context, ) +from dataretrieval.transport.pagination import paginate def _run_walk_pages(*, geopd, req, client): @@ -501,11 +503,12 @@ async def follow_up(cursor, sess): async def run(): with progress_context(service="continuous", stream=stream, enabled=True): - df, _ = await _paginate( + df, _ = await paginate( req, parse_response=parse_sync, follow_up=follow_up, client=client, + raise_for_status=_raise_for_non_200, ) return df diff --git a/tests/waterdata_test.py b/tests/waterdata_test.py index 7d8fe80f..a5d04768 100644 --- a/tests/waterdata_test.py +++ b/tests/waterdata_test.py @@ -419,13 +419,13 @@ def test_get_cql_service_keyword_is_deprecated_but_works(): The rename must not silently change behavior for callers using the old name. """ with pytest.warns(DeprecationWarning, match="use 'collection'"): - with pytest.raises(ValueError, match="Unknown collection"): + with pytest.raises(ValueError, match="Invalid collection"): get_cql(service="not-a-collection", cql="a=1") # The new spelling emits nothing. with warnings.catch_warnings(): warnings.simplefilter("error", DeprecationWarning) - with pytest.raises(ValueError, match="Unknown collection"): + with pytest.raises(ValueError, match="Invalid collection"): get_cql(collection="not-a-collection", cql="a=1") # Passing both spellings is ambiguous and refused, which the hand-rolled @@ -436,7 +436,7 @@ def test_get_cql_service_keyword_is_deprecated_but_works(): def test_get_cql_unknown_service_raises(): """An unknown collection is rejected before any network call.""" - with pytest.raises(ValueError, match="Unknown collection"): + with pytest.raises(ValueError, match="Invalid collection"): get_cql("not-a-collection", {"op": "isNull", "args": [{"property": "x"}]}) @@ -1137,6 +1137,18 @@ def test_get_reference_table(httpx_mock): assert hasattr(md, "url") and hasattr(md, "query_time") +def test_get_reference_table_rejects_unknown_collection_by_its_own_name(httpx_mock): + """The rejection names ``collection`` -- the parameter actually passed. + + Regression: this check was copied from ``get_codes``, message and local + variable name included, so a bad ``collection=`` was reported as an + invalid *code service* -- a parameter this function does not have. + """ + with pytest.raises(ValueError, match="Invalid collection: 'agency-codez'"): + get_reference_table("agency-codez") + assert not httpx_mock.get_requests(), "must reject before issuing a request" + + def test_get_reference_table_with_query(httpx_mock): """A ``query`` dict is merged into the request's query params.""" _mock_items(httpx_mock, "agency-codes") diff --git a/tests/wqp_test.py b/tests/wqp_test.py index cde3c84e..6577c9c6 100644 --- a/tests/wqp_test.py +++ b/tests/wqp_test.py @@ -152,15 +152,16 @@ def test_wqp_url_profiles(builder, service, expected, warning): def test_wqp_url_profiles_reject_unknown_service( builder, profile, valid_services, warning ): - """Shared validation preserves profile-specific error text and ordering.""" + """Shared validation names the offending service and its profile.""" with pytest.warns(warning): with pytest.raises( ValueError, - match=rf"^{profile} service not recognized\. Valid options are ", + match=rf"^Invalid service: 'unknown' for {profile}\. Valid options are: ", ) as exc_info: builder("unknown") - assert str(valid_services) in str(exc_info.value) + # Every valid service is offered, in declaration order. + assert ", ".join(repr(s) for s in valid_services) in str(exc_info.value) # Every WQP ``what_*`` wrapper issues the same query against its own service From 66687408e496a80323bb28d6830c0914e65c701b Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Wed, 12 Aug 2026 20:53:15 -0500 Subject: [PATCH 2/2] refactor(validation): refuse a string vocabulary ``str`` satisfies ``Collection[object]``, so ``require_one_of(fmt, "csv", name="format")`` type-checks under mypy --strict -- and then ``value in options`` silently means *substring*, accepting ``"cs"`` as valid. No current call site passes a string, but this is the one shared chokepoint every future check goes through, so it is worth closing here rather than in whichever adapter writes it first. Adds the validator's own tests, which it had none of. Found by code review of this PR. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01BTaSm7HmVb94RSJiKW4WAS --- dataretrieval/_validation.py | 4 ++++ tests/validation_test.py | 30 ++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+) create mode 100644 tests/validation_test.py diff --git a/dataretrieval/_validation.py b/dataretrieval/_validation.py index 639cc252..8c57d305 100644 --- a/dataretrieval/_validation.py +++ b/dataretrieval/_validation.py @@ -59,6 +59,10 @@ def require_one_of( ValueError If *value* is not in *options*. """ + if isinstance(options, str): + # ``str`` is a Collection, so this type-checks -- and then ``in`` + # silently means "substring", accepting any fragment of a valid option. + raise TypeError(f"options must be a collection of values, not {options!r}") if value in options: return qualifier = f" for {context}" if context else "" diff --git a/tests/validation_test.py b/tests/validation_test.py new file mode 100644 index 00000000..1f9f638a --- /dev/null +++ b/tests/validation_test.py @@ -0,0 +1,30 @@ +"""Tests for the shared closed-vocabulary check.""" + +import pytest + +from dataretrieval._validation import require_one_of + + +def test_accepts_a_valid_option(): + require_one_of("daily", ("daily", "continuous"), name="collection") + + +def test_message_names_the_parameter_and_the_options(): + with pytest.raises(ValueError) as excinfo: + require_one_of("hourly", ("daily", "continuous"), name="collection") + message = str(excinfo.value) + assert "Invalid collection: 'hourly'" in message + assert "'daily', 'continuous'" in message + + +def test_context_qualifies_a_vocabulary_that_depends_on_another_argument(): + with pytest.raises(ValueError, match="for service 'wqp'"): + require_one_of("x", ("a",), name="profile", context="service 'wqp'") + + +def test_a_string_vocabulary_is_refused(): + """``str`` is a Collection, so passing one type-checks -- and then ``in`` + silently degrades from membership to a substring test, accepting any + fragment of a valid option. Refuse it at the one shared chokepoint.""" + with pytest.raises(TypeError, match="not 'csv'"): + require_one_of("cs", "csv", name="format")