fix(nwis): extend major-filter & format guards to get_stats, get_info, peaks, ratings

Same package-review bug class as the rest of this PR, in the remaining
deprecated nwis getters:

- get_stats injected `sites=sites` unconditionally, defeating
  query_waterservices' major-filter check, so get_stats() (and
  get_record(service="stat")) hit the network and returned a confusing
  "Bad Request" instead of the documented TypeError. Inject only when given.
- get_record(service="site") forwarded sites=None into get_info, the only
  getter not guarding its own injection; drop an unset `sites` there too.
- get_discharge_peaks passed format="rdb" explicitly alongside **kwargs, so
  any caller-supplied `format` raised "multiple values for 'format'".
  Generalize _reject_non_json_format -> _reject_unexpected_format and apply
  it (json for dv/iv, rdb for peaks and stats).
- get_stats now rejects a non-RDB `format` instead of requesting it and
  mis-parsing the response.
- get_ratings is per-site; with no site it issued a request that returned an
  unhelpful error page. Fail fast with a clear TypeError (also covers
  get_record(service="ratings")).

Adds 7 regression tests; all fail on the prior commit. Full nwis suite (57)
passes; ruff clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: thodson-usgs

dataretrieval/nwis.py

@@ -130,15 +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.
+def _reject_unexpected_format(kwargs: dict, func_name: str, expected: str) -> None:
+    """Drop ``format`` from ``kwargs`` and reject any value other than ``expected``.
 
-    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.
+    These getters always request a fixed ``format`` and parse that specific body,
+    so a caller-supplied ``format`` would either collide with the explicit
+    ``format=`` argument or 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`.")
+    if kwargs.pop("format", expected) != expected:
+        raise ValueError(
+            f"{func_name} returns {expected.upper()} and does not accept a `format`."
+        )
 
 
 def format_response(
@@ -293,6 +295,7 @@ def get_discharge_peaks(
     kwargs["end_date"] = kwargs.pop("end_date", end)
     kwargs["multi_index"] = multi_index
 
+    _reject_unexpected_format(kwargs, "get_discharge_peaks", "rdb")
     response = query_waterdata("peaks", format="rdb", ssl_check=ssl_check, **kwargs)
 
     df = _read_rdb(response.text)
@@ -368,9 +371,16 @@ def get_stats(
     """
     _check_sites_value_types(sites)
 
-    response = query_waterservices(
-        service="stat", sites=sites, ssl_check=ssl_check, **kwargs
-    )
+    # Inject `sites` only when provided; unconditionally setting it (even to
+    # None) would defeat query_waterservices' major-filter check, yielding a
+    # confusing "Bad Request" instead of the intended TypeError.
+    if sites is not None:
+        kwargs.setdefault("sites", sites)
+
+    # get_stats parses an RDB body; reject a caller-supplied non-RDB `format`
+    # rather than request it and then mis-parse the response.
+    _reject_unexpected_format(kwargs, "get_stats", "rdb")
+    response = query_waterservices(service="stat", ssl_check=ssl_check, **kwargs)
 
     return _read_rdb(response.text), NWIS_Metadata(response, **kwargs)
 
@@ -550,7 +560,7 @@ def get_dv(
         kwargs.setdefault("sites", sites)
     kwargs["multi_index"] = multi_index
 
-    _reject_non_json_format(kwargs, "get_dv")
+    _reject_unexpected_format(kwargs, "get_dv", "json")
     response = query_waterservices("dv", format="json", ssl_check=ssl_check, **kwargs)
     df = _parse_json_or_raise(response)
 
@@ -668,6 +678,12 @@ def get_info(ssl_check: bool = True, **kwargs) -> tuple[pd.DataFrame, BaseMetada
         # cannot have both seriesCatalogOutput and the expanded format
         kwargs["siteOutput"] = "Expanded"
 
+    # get_record(service="site") forwards sites=None; drop it so an unset filter
+    # doesn't defeat query_waterservices' major-filter check (other getters
+    # already guard their own injection).
+    if kwargs.get("sites") is None:
+        kwargs.pop("sites", None)
+
     response = query_waterservices("site", ssl_check=ssl_check, **kwargs)
 
     return _read_rdb(response.text), NWIS_Metadata(response, **kwargs)
@@ -737,7 +753,7 @@ def get_iv(
         kwargs.setdefault("sites", sites)
     kwargs["multi_index"] = multi_index
 
-    _reject_non_json_format(kwargs, "get_iv")
+    _reject_unexpected_format(kwargs, "get_iv", "json")
     response = query_waterservices(
         service="iv", format="json", ssl_check=ssl_check, **kwargs
     )
@@ -802,6 +818,11 @@ def get_ratings(
 
     """
     site = kwargs.pop("site_no", site)
+    # The ratings endpoint is per-site; without one it would issue a request
+    # that returns an unhelpful error page. Fail fast with a clear message
+    # (also covers get_record(service="ratings") called without a site).
+    if site is None:
+        raise TypeError("get_ratings requires a `site` (USGS site number).")
 
     payload = {}
     url = WATERDATA_BASE_URL + "nwisweb/get_ratings/"

tests/nwis_test.py

@@ -13,13 +13,15 @@
     NWIS_Metadata,
     _read_rdb,
     get_discharge_measurements,
+    get_discharge_peaks,
     get_dv,
     get_gwlevels,
     get_info,
     get_iv,
     get_pmcodes,
     get_qwdata,
     get_record,
+    get_stats,
     get_water_use,
     preformat_peaks_response,
     what_sites,
@@ -161,6 +163,73 @@ def fake_query_waterservices(service, format=None, ssl_check=True, **kw):
     assert captured.get("stateCd") == "OH"
 
 
+def test_get_stats_requires_major_filter():
+    """Regression: get_stats passed ``sites=sites`` explicitly, so the key was
+    always present (None when absent) and defeated query_waterservices' filter
+    check -- get_stats() reached the network and returned a confusing Bad
+    Request instead of the documented TypeError."""
+    with warnings.catch_warnings():
+        warnings.simplefilter("ignore")
+        with pytest.raises(TypeError, match="major filter"):
+            get_stats()
+
+
+@pytest.mark.parametrize("service", ["stat", "site"])
+def test_get_record_requires_major_filter(service):
+    """Regression: get_record(service="stat"/"site") forwarded sites=None into
+    get_stats/get_info, defeating the major-filter check. With no filter it must
+    raise the documented TypeError rather than reach the network."""
+    with warnings.catch_warnings():
+        warnings.simplefilter("ignore")
+        with pytest.raises(TypeError, match="major filter"):
+            get_record(service=service)
+
+
+def test_get_discharge_peaks_rejects_format():
+    """Regression: get_discharge_peaks passed format="rdb" explicitly alongside
+    **kwargs, so any caller-supplied format raised 'multiple values for format'.
+    A non-native format is now rejected with a clear ValueError (it parses RDB)."""
+    with warnings.catch_warnings():
+        warnings.simplefilter("ignore")
+        with pytest.raises(ValueError, match="RDB"):
+            get_discharge_peaks(sites="01491000", format="json")
+
+
+def test_get_discharge_peaks_accepts_native_format(monkeypatch):
+    """The reported collision was that even the *native* format="rdb" raised
+    'multiple values for format'. Popping it first resolves the collision, so
+    format="rdb" now reaches the request rather than crashing."""
+    import dataretrieval.nwis as nwis_mod
+
+    def fake_query_waterdata(service, format=None, ssl_check=True, **kw):
+        raise RuntimeError("stop before network")
+
+    monkeypatch.setattr(nwis_mod, "query_waterdata", fake_query_waterdata)
+    with warnings.catch_warnings():
+        warnings.simplefilter("ignore")
+        with pytest.raises(RuntimeError, match="stop"):
+            get_discharge_peaks(sites="01491000", format="rdb")
+
+
+def test_get_stats_rejects_non_rdb_format():
+    """get_stats parses RDB; a caller-supplied non-RDB format would be requested
+    and then mis-parsed. It is now rejected with a clear ValueError."""
+    with warnings.catch_warnings():
+        warnings.simplefilter("ignore")
+        with pytest.raises(ValueError, match="RDB"):
+            get_stats(sites="01646500", format="json")
+
+
+def test_get_ratings_requires_site():
+    """The ratings endpoint is per-site; get_ratings() / get_record(
+    service="ratings") with no site previously issued a request that returned an
+    unhelpful error page. It now fails fast with a clear TypeError."""
+    with warnings.catch_warnings():
+        warnings.simplefilter("ignore")
+        with pytest.raises(TypeError, match="requires a `site`"):
+            get_record(service="ratings")
+
+
 class TestDeprecationWarnings:
     """Verify per-function DeprecationWarning fires with the right replacement.