From 765b374ac2c4bfd3ac7e711718a4de18e260fd97 Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Fri, 14 Aug 2026 15:15:12 +0000 Subject: [PATCH] fix(ogc): report the service's own 403 reason, and name the URL A 403 returned fixed text naming only "query exceeding server limits" and never read the body, so a missing, expired, or revoked API_USGS_PAT -- the most common real 403 against api.waterdata.usgs.gov -- was reported as a query-size problem while the service's own "API key revoked" was discarded. 403 now shares the generic path: the JSON error body when there is one (a USGS code/description envelope or a gateway message, flat or nested under "error"), the text snippet otherwise, and credential guidance only when there is no body to quote. No OGC error message named the URL. The legacy path appends it, this one never did, and nothing downstream restores it -- FanOutInterrupted builds its "Cause:" line from this same string -- so a fan-out that died on one bad chunk gave no indication of which request was in flight. Reading the URL is guarded: httpx raises on `.url` for a hand-built response, and an error path must not fail while reporting a failure. Not addressed here: _querying._raise_for_status and ogc.errors._raise_for_non_200 remain two implementations of one contract, and the 414 "split your query" remediation is still only on the legacy side. Unifying them means moving the message builder to a layer `ogc` may import (.importlinter forbids ogc -> _querying) -- a structural change rather than a bug fix. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01BTaSm7HmVb94RSJiKW4WAS --- dataretrieval/ogc/errors.py | 88 +++++++++++++++++++++++---------- tests/waterdata_utils_test.py | 93 +++++++++++++++++++++++++++++++++++ 2 files changed, 156 insertions(+), 25 deletions(-) diff --git a/dataretrieval/ogc/errors.py b/dataretrieval/ogc/errors.py index be8d8733f..597f86e80 100644 --- a/dataretrieval/ogc/errors.py +++ b/dataretrieval/ogc/errors.py @@ -30,15 +30,15 @@ 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 - ``": . ."`` from the JSON error - envelope. If the body is not JSON (e.g. an HTML 502 from a - gateway), falls back to ``": . "`` with - the first 200 characters of ``resp.text``; an empty body - degrades to ``": ."``. + * **every other status** — a supported JSON error body (the USGS + ``code``/``description`` envelope or a gateway ``message``) when + present; otherwise ``": . "`` with the first + 200 characters of ``resp.text``; an empty body degrades to + ``": ."``, 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: @@ -46,23 +46,61 @@ def _error_body(resp: httpx.Response) -> str: "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")), ) diff --git a/tests/waterdata_utils_test.py b/tests/waterdata_utils_test.py index 9081b7016..e51d85c5a 100644 --- a/tests/waterdata_utils_test.py +++ b/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