fix(errors): consistent error-status gateway + drop NWIS-flavored 413/414 leak

A /code-review (max) of the unified error path surfaced these:

- waterdata `_raise_for_non_200` guarded only `status == 200`, so a 2xx-non-200
  or 3xx was routed through `error_for_status` and raised as a fatal ClientError
  on a SUCCESS. Match the legacy `_raise_for_status`: guard `status < 400`.
  (Latent today -- the OGC API paginates with 200s -- but the two gateways now
  agree.)

- `_raise_for_status` special-cased 413/414 to `_url_too_long_error`, whose
  message is NWIS-specific ("use fewer sites", an `nwis.get_record()` example).
  nadp/streamstats (no sites) now route through this helper, so a 413/414 on a
  streamstats download would emit nonsensical NWIS guidance -- and 413/414 was
  mapped in two places. Drop the special-case: the factory owns 413/414 ->
  URLTooLong with a generic message; the NWIS prose stays only on query()'s
  client-side InvalidURL path (the common too-long-URL trigger).

- Status-first message ("HTTP {status} {reason}") so a non-standard status with
  an empty reason_phrase (520/599 WAF codes) no longer leaves a stray leading
  space.

- Extend `test_retryable_taxonomy` to pin that a typed waterdata 4xx
  (BadRequestError/ClientError, ValueError-based) is fatal -- never retried --
  closing a regression-guard gap (only a bare RuntimeError was covered).

- Fix a stale ratings.py comment that called the typed errors "all RuntimeError
  subclasses" (4xx is now ClientError/BadRequestError; the `except` already
  catches ValueError, so the skip-bad-feature behavior is intact).

ruff + mypy --strict clean; 483 passed / 2 skipped.

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

Author: thodson-usgs

dataretrieval/utils.py

@@ -293,16 +293,14 @@ def _raise_for_status(response: httpx.Response) -> None:
 
     Shared by the legacy :func:`query` path (and ``nadp`` / ``streamstats``).
     Delegates the status-to-type mapping to
-    :func:`dataretrieval.exceptions.error_for_status`; a 413/414 keeps the richer
-    "split your query" guidance via :func:`_url_too_long_error`.
+    :func:`dataretrieval.exceptions.error_for_status`.
     """
     status = response.status_code
     if status < 400:
         return
-    if status in (413, 414):
-        raise _url_too_long_error(f"API response reason: {response.reason_phrase}")
     raise error_for_status(
-        status, f"{response.reason_phrase} (HTTP {status}). URL: {response.url}"
+        status,
+        f"HTTP {status} {response.reason_phrase}".rstrip() + f" (URL: {response.url})",
     )
 
 

dataretrieval/waterdata/ratings.py

@@ -185,10 +185,11 @@ def get_ratings(
         try:
             out[fid] = _download_and_parse(feature, file_path, ssl_check)
         # _download_and_parse can raise the module's typed errors via
-        # _raise_for_non_200 (RateLimited / ServiceUnavailable / RuntimeError —
-        # all RuntimeError subclasses), and a feature missing its data asset
-        # raises LookupError. Catch those too so one bad feature is logged and
-        # skipped rather than aborting the whole multi-site batch.
+        # _raise_for_non_200 (RateLimited / ServiceUnavailable, RuntimeError
+        # subclasses; or BadRequestError / ClientError on a 4xx, ValueError
+        # subclasses), and a feature missing its data asset raises LookupError.
+        # Catch those too so one bad feature is logged and skipped rather than
+        # aborting the whole multi-site batch.
         except (
             httpx.HTTPError,
             RuntimeError,

dataretrieval/waterdata/utils.py

@@ -551,7 +551,7 @@ def _raise_for_non_200(resp: httpx.Response) -> None:
         ``TransientError``) the chunker won't resume.
     """
     status = resp.status_code
-    if status == 200:
+    if status < 400:
         return
     raise error_for_status(
         status,

tests/utils_test.py

@@ -57,7 +57,7 @@ class Test_error_taxonomy:
             (400, "BadRequestError", "400", ValueError),
             (403, "ClientError", "403", ValueError),
             (404, "NotFoundError", "404", ValueError),
-            (414, "URLTooLong", "Request URL too long", ValueError),
+            (414, "URLTooLong", "414", ValueError),
             (429, "RateLimited", "429", RuntimeError),
             (503, "ServiceUnavailable", "503", RuntimeError),
         ],

tests/waterdata_chunking_test.py

@@ -1603,13 +1603,19 @@ def test_retry_policy_from_env_honors_monkeypatched_constants(monkeypatch):
 
 
 def test_retryable_taxonomy():
+    from dataretrieval.exceptions import BadRequestError, ClientError
+
     assert _retryable(RateLimited("429", retry_after=5.0)) == (True, 5.0)
     assert _retryable(ServiceUnavailable("503")) == (True, None)
     assert _retryable(httpx.ReadTimeout("slow")) == (True, None)
     assert _retryable(httpx.ConnectError("down")) == (True, None)
     # InvalidURL is resumable but NOT retryable (a too-long cursor won't fix).
     assert _retryable(httpx.InvalidURL("too long")) == (False, None)
-    # Plain non-transient (e.g. a 4xx programmer error wrapped as RuntimeError).
+    # A typed waterdata 4xx is fatal: ValueError-based, not a TransientError, so
+    # the chunker must never retry it (BadRequestError/ClientError, plus the old
+    # bare RuntimeError for good measure).
+    assert _retryable(BadRequestError("400")) == (False, None)
+    assert _retryable(ClientError("403")) == (False, None)
     assert _retryable(RuntimeError("400")) == (False, None)