fix(ogc): report the service's own 403 reason, and name the URL

dataretrieval/ogc/errors.py

@@ -30,39 +30,77 @@ def _error_body(resp: httpx.Response) -> str:
 
         * **429** — predefined message describing the rate-limit and pointing
           at the API-token path; the response body is not consulted.
-        * **403** — predefined message describing the most common cause
-          (query exceeding server limits); the response body is not
-          consulted.
-        * **other statuses** — attempts ``resp.json()`` and renders
-          ``"<status>: <code>. <description>."`` from the JSON error
-          envelope. If the body is not JSON (e.g. an HTML 502 from a
-          gateway), falls back to ``"<status>: <reason>. <snippet>"`` with
-          the first 200 characters of ``resp.text``; an empty body
-          degrades to ``"<status>: <reason>."``.
+        * **every other status** — a supported JSON error body (the USGS
+          ``code``/``description`` envelope or a gateway ``message``) when
+          present; otherwise ``"<status>: <reason>. <snippet>"`` with the first
+          200 characters of ``resp.text``; an empty body degrades to
+          ``"<status>: <reason>."``, except **403**, which falls back to
+          :data:`_FORBIDDEN_CAUSES` so a credential problem is named.
+
+    :func:`_raise_for_non_200` appends ``" (URL: ...)"`` to whatever this
+    returns.
     """
     status = resp.status_code
     if status == 429:
         return (
             "429: Too many requests made. Please obtain an API token "
             "or try again later."
         )
-    elif status == 403:
-        return (
-            "403: Query request denied. Possible reasons include "
-            "query exceeding server limits."
-        )
+    detail = _json_error_detail(resp)
+    if detail is not None:
+        return f"{status}: {detail}"
+    snippet = (resp.text or "").strip()[:200]
+    reason = resp.reason_phrase or "Error"
+    if snippet:
+        return f"{status}: {reason}. {snippet}"
+    if status == 403:
+        return f"403: {_FORBIDDEN_CAUSES}"
+    return f"{status}: {reason}."
+
+
+#: What a 403 means when the service sends no error envelope. Both causes are
+#: named because the credential one is far more common and was omitted.
+_FORBIDDEN_CAUSES = (
+    "Query request denied. The API key may be missing, expired, or revoked "
+    "(see API_USGS_PAT), or the query may exceed server limits."
+)
+
+
+def _json_error_detail(resp: httpx.Response) -> str | None:
+    """Render a supported JSON error body, or ``None`` for another shape."""
     try:
-        j_txt = resp.json()
+        body = resp.json()
     except ValueError:
-        snippet = (resp.text or "").strip()[:200]
-        reason = resp.reason_phrase or "Error"
-        if snippet:
-            return f"{status}: {reason}. {snippet}"
-        return f"{status}: {reason}."
-    return (
-        f"{status}: {j_txt.get('code', 'Unknown type')}. "
-        f"{j_txt.get('description', 'No description provided')}."
-    )
+        return None
+    if not isinstance(body, dict):
+        return None
+
+    candidate = body.get("error")
+    if not isinstance(candidate, dict):
+        candidate = body
+
+    def clean(value: object | None) -> str | None:
+        if value is None:
+            return None
+        text = str(value).strip().rstrip(".")
+        return text or None
+
+    code = clean(candidate.get("code"))
+    detail = clean(candidate.get("description")) or clean(candidate.get("message"))
+    parts = [part for part in (code, detail) if part is not None]
+    return ". ".join(parts) + "." if parts else None
+
+
+def _url_suffix(resp: httpx.Response) -> str:
+    """`` (URL: ...)``, or empty when no request is attached.
+
+    ``httpx`` raises on ``.url`` for a hand-built response; an error path must
+    not fail while reporting a failure.
+    """
+    try:
+        return f" (URL: {resp.url})"
+    except RuntimeError:
+        return ""
 
 
 def _raise_for_non_200(resp: httpx.Response) -> None:
@@ -96,6 +134,6 @@ def _raise_for_non_200(resp: httpx.Response) -> None:
         return
     raise error_for_status(
         status,
-        _error_body(resp),
+        _error_body(resp) + _url_suffix(resp),
         retry_after=_parse_retry_after(resp.headers.get("Retry-After")),
     )

tests/waterdata_utils_test.py

@@ -942,6 +942,99 @@ def test_raise_for_non_200_attaches_retry_after_to_rate_limited():
     assert excinfo.value.retry_after == 60.0
 
 
+def test_403_reports_the_services_own_reason():
+    """A 403 envelope must reach the user rather than a canned guess.
+
+    The message was fixed text naming only "query exceeding server limits",
+    and never read the body -- so a revoked ``API_USGS_PAT``, the most common
+    real 403, was reported as a query-size problem.
+    """
+    resp = _make_response(
+        403,
+        '{"code": "Forbidden", "description": "API key revoked"}',
+        reason="Forbidden",
+        content_type="application/json",
+    )
+    with pytest.raises(HTTPError) as excinfo:
+        _raise_for_non_200(resp)
+    assert "API key revoked" in str(excinfo.value)
+
+
+@pytest.mark.parametrize(
+    ("body", "expected"),
+    [
+        ('{"message": "API key is invalid"}', "403: API key is invalid."),
+        (
+            '{"error": {"code": "API_KEY_INVALID", '
+            '"message": "An invalid api_key was supplied"}}',
+            "403: API_KEY_INVALID. An invalid api_key was supplied.",
+        ),
+        (
+            '{"code": "Forbidden", "description": null, "message": "API key revoked"}',
+            "403: Forbidden. API key revoked.",
+        ),
+        (
+            '{"code": "Forbidden", "description": "", "message": "API key revoked"}',
+            "403: Forbidden. API key revoked.",
+        ),
+    ],
+    ids=(
+        "flat-message",
+        "nested-live-envelope",
+        "null-description",
+        "empty-description",
+    ),
+)
+def test_403_with_a_gateway_json_message_shows_the_message(body, expected):
+    """Gateway JSON errors are rendered deliberately, not as raw JSON."""
+    resp = _make_response(
+        403,
+        body,
+        reason="Forbidden",
+        content_type="application/json",
+    )
+    with pytest.raises(HTTPError) as excinfo:
+        _raise_for_non_200(resp)
+    message = str(excinfo.value)
+    assert message == expected
+    assert "{" not in message
+
+
+def test_403_with_a_non_json_body_shows_it_like_any_other_status():
+    """A WAF 403 is plain text; discarding it was the bug this PR fixes, so
+    403 shares the snippet path rather than having its own."""
+    resp = _make_response(403, "Access Denied: API key revoked", reason="Forbidden")
+    with pytest.raises(HTTPError) as excinfo:
+        _raise_for_non_200(resp)
+    assert "API key revoked" in str(excinfo.value)
+
+
+def test_403_without_an_envelope_names_the_credential_cause():
+    """With no body to quote, both plausible causes are offered."""
+    resp = _make_response(403, "", reason="Forbidden")
+    with pytest.raises(HTTPError) as excinfo:
+        _raise_for_non_200(resp)
+    message = str(excinfo.value)
+    assert "API_USGS_PAT" in message and "server limits" in message
+
+
+def test_error_messages_name_the_url():
+    """Without the URL a failed chunk in a fan-out cannot be traced back to
+    the request that produced it -- the message is all the interruption
+    carries."""
+    request = httpx.Request("GET", "https://api.waterdata.usgs.gov/ogcapi/v0/x")
+    resp = httpx.Response(400, content=b"", request=request)
+    with pytest.raises(HTTPError) as excinfo:
+        _raise_for_non_200(resp)
+    assert "https://api.waterdata.usgs.gov/ogcapi/v0/x" in str(excinfo.value)
+
+
+def test_error_message_survives_a_response_with_no_request():
+    """An error path must not fail while reporting a failure."""
+    with pytest.raises(HTTPError):
+        _raise_for_non_200(_make_response(400, "", reason="Bad Request"))
+
+
 def test_raise_for_non_200_400_raises_http_error():
     """400 raises a fatal ``HTTPError`` (status_code=400) the chunker won't
     resume. It must NOT be a ``TransientError`` so the chunker's classifier