fix(ogc): report the service's own 403 reason, and name the URL - #374
Draft
thodson-usgs wants to merge 3 commits into
Draft
fix(ogc): report the service's own 403 reason, and name the URL#374thodson-usgs wants to merge 3 commits into
thodson-usgs wants to merge 3 commits into
Conversation
Two user-facing defects in the OGC error message.
**403 was a canned guess.** The message was 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. It now renders the JSON
envelope like every other status, and names both causes when there is no
envelope to quote.
was: 403: Query request denied. Possible reasons include query
exceeding server limits.
now: 403: Forbidden. API key revoked. (URL: .../items?parameter_code=00060)
**No 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 string. So a 32-chunk fan-out that died on one
bad parameter_code gave no indication of which chunk, sites, or values
were 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.
The JSON-envelope read is factored out so 403 and the generic path
cannot drift apart again.
Not addressed here: _querying._raise_for_status and
ogc.errors._raise_for_non_200 remain two implementations, and the 414
"split your query" remediation is still only on the legacy one. 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.
791 passed, mypy --strict clean, all hooks pass.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BTaSm7HmVb94RSJiKW4WAS
The first pass consulted only the USGS JSON envelope for a 403, which
left two shapes worse off than before:
- A gateway 403 carrying ``{"message": "Forbidden"}`` -- the standard AWS
API Gateway shape for a missing or invalid key, i.e. exactly the case
this fix exists for -- rendered as "403: Unknown type. No description
provided.", because the envelope reader answered for any JSON object.
That is worse than both the old canned text and the new guidance: the
user learns neither what the service said nor that their key may be
the problem.
- A WAF 403 with a plain-text body ("Access Denied: API key revoked")
had its body discarded wholesale, though every other status would
have shown the snippet.
403 now shares the generic path: envelope if the body carries one,
otherwise the snippet, with the credential guidance as the last resort
for an empty body only. The envelope reader answers ``None`` unless
``code`` or ``description`` is actually present, which also closes the
same blind spot on every other status.
The docstring still argued for the behavior the previous commit removed
("the response body is not consulted"), and never mentioned that
``_raise_for_non_200`` appends the URL; both corrected.
Found by code review of this PR.
793 passed, mypy --strict clean, all hooks pass.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BTaSm7HmVb94RSJiKW4WAS
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two user-facing defects in the OGC error message
1. A 403 was a canned guess that never read the body
The 403 branch returned fixed text and explicitly did not consult the response — its docstring said so. But a missing, expired, or revoked
API_USGS_PATis the most common real 403 againstapi.waterdata.usgs.gov, and that user was told to go shrink their query while the service's own"API key revoked"was discarded. The legacy_queryingpath would have surfaced it.It now renders the JSON envelope like every other status, and when there is no envelope to quote, names both plausible causes instead of only the rarer one.
2. No OGC error message named the URL
The legacy path appends
(URL: ...); this one never did, and nothing downstream restores it —FanOutInterruptedbuilds itsCause:line from this same string. So a 32-chunk fan-out that died on one badparameter_coderaised a message with no indication of which chunk, which sites, or which values were in flight.Reading the URL is guarded:
httpxraises on.urlfor a hand-built response, and an error path must not fail while reporting a failure.The JSON-envelope read is factored into one helper so the 403 branch and the generic path cannot drift apart again.
Not addressed here
_querying._raise_for_statusandogc.errors._raise_for_non_200remain two implementations of one contract, and each is still missing the other's improvement — the 414 "split your query" remediation exists only on the legacy side. Unifying them means moving the message builder down to a layerogcis permitted to import (.importlinterforbidsogc -> _querying), which is a structural change rather than a bug fix and deserves its own review.Testing
mypy --strict,ruff,xenon,complexipy,lint-importsall pass.Found by the scan in #372, split out because it changes user-visible behavior. Independent of #372 and #373 — branches off main.
🤖 Generated with Claude Code
https://claude.ai/code/session_01BTaSm7HmVb94RSJiKW4WAS