diff --git a/dataretrieval/ogc/errors.py b/dataretrieval/ogc/errors.py index be8d8733..597f86e8 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 9081b701..e51d85c5 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