diff --git a/dataretrieval/nwis.py b/dataretrieval/nwis.py index 6a1ed472..d4fb5795 100644 --- a/dataretrieval/nwis.py +++ b/dataretrieval/nwis.py @@ -130,6 +130,17 @@ def _parse_json_or_raise(response: httpx.Response) -> pd.DataFrame: raise +def _reject_non_json_format(kwargs: dict, func_name: str) -> None: + """Drop ``format`` from ``kwargs`` and reject any non-JSON value. + + These getters always request ``format="json"`` and parse the JSON body, so + a caller-supplied ``format`` (e.g. ``"rdb"``) would be silently overridden; + reject it explicitly instead. + """ + if kwargs.pop("format", "json") != "json": + raise ValueError(f"{func_name} returns JSON and does not accept a `format`.") + + def format_response( df: pd.DataFrame, service: str | None = None, **kwargs ) -> pd.DataFrame: @@ -276,7 +287,8 @@ def get_discharge_peaks( """ _check_sites_value_types(sites) - kwargs["site_no"] = kwargs.pop("site_no", sites) + if sites is not None: + kwargs.setdefault("site_no", sites) kwargs["begin_date"] = kwargs.pop("begin_date", start) kwargs["end_date"] = kwargs.pop("end_date", end) kwargs["multi_index"] = multi_index @@ -534,9 +546,11 @@ def get_dv( kwargs["startDT"] = kwargs.pop("startDT", start) kwargs["endDT"] = kwargs.pop("endDT", end) - kwargs["sites"] = kwargs.pop("sites", sites) + if sites is not None: + kwargs.setdefault("sites", sites) kwargs["multi_index"] = multi_index + _reject_non_json_format(kwargs, "get_dv") response = query_waterservices("dv", format="json", ssl_check=ssl_check, **kwargs) df = _parse_json_or_raise(response) @@ -719,9 +733,11 @@ def get_iv( kwargs["startDT"] = kwargs.pop("startDT", start) kwargs["endDT"] = kwargs.pop("endDT", end) - kwargs["sites"] = kwargs.pop("sites", sites) + if sites is not None: + kwargs.setdefault("sites", sites) kwargs["multi_index"] = multi_index + _reject_non_json_format(kwargs, "get_iv") response = query_waterservices( service="iv", format="json", ssl_check=ssl_check, **kwargs ) @@ -952,6 +968,12 @@ def get_record( if service not in WATERSERVICES_SERVICES + WATERDATA_SERVICES: raise TypeError(f"Unrecognized service: {service}") + # Forward the documented `state` filter as the NWIS `stateCd` major filter; + # it was previously accepted but silently ignored, which produced a + # confusing "Bad Request" when used without `sites`. + if state is not None: + kwargs.setdefault("stateCd", state) + if service == "iv": df, _ = get_iv( sites=sites, diff --git a/tests/nwis_test.py b/tests/nwis_test.py index f343f26e..fd5a2808 100644 --- a/tests/nwis_test.py +++ b/tests/nwis_test.py @@ -13,6 +13,7 @@ NWIS_Metadata, _read_rdb, get_discharge_measurements, + get_dv, get_gwlevels, get_info, get_iv, @@ -120,6 +121,46 @@ def test_preformat_peaks_response(): # Removed defunct gwlevels tests. +def test_get_dv_requires_major_filter(): + """Regression: get_dv() with no major filter must raise the documented + TypeError. The getters injected ``kwargs["sites"] = kwargs.pop("sites", + sites)``, which always set the key (None when absent) and so defeated + query_waterservices' filter check, yielding a confusing Bad Request.""" + with warnings.catch_warnings(): + warnings.simplefilter("ignore") + with pytest.raises(TypeError, match="major filter"): + get_dv() + + +def test_get_dv_rejects_non_json_format(): + """Regression: get_dv passed format= explicitly alongside **kwargs, so + get_dv(..., format="rdb") raised 'multiple values for format'. It now + rejects a non-json format with a clear ValueError (it parses JSON).""" + with warnings.catch_warnings(): + warnings.simplefilter("ignore") + with pytest.raises(ValueError, match="JSON"): + get_dv(sites="01646500", format="rdb") + + +def test_get_record_forwards_state_as_statecd(monkeypatch): + """Regression: get_record's documented `state` arg was accepted but never + forwarded. It now reaches the request as the NWIS `stateCd` major filter.""" + import dataretrieval.nwis as nwis_mod + + captured: dict = {} + + def fake_query_waterservices(service, format=None, ssl_check=True, **kw): + captured.update(kw) + raise RuntimeError("stop before network") + + monkeypatch.setattr(nwis_mod, "query_waterservices", fake_query_waterservices) + with warnings.catch_warnings(): + warnings.simplefilter("ignore") + with pytest.raises(RuntimeError, match="stop"): + get_record(state="OH", service="dv") + assert captured.get("stateCd") == "OH" + + class TestDeprecationWarnings: """Verify per-function DeprecationWarning fires with the right replacement.