fix: handle non-JSON error response bodies in _error_body (#242)#274
Merged
thodson-usgs merged 3 commits intoMay 12, 2026
Merged
Conversation
Signed-off-by: SAY-5 <say.apm35@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves robustness of Waterdata OGC error handling by preventing _error_body from raising JSON decode exceptions when upstream services return non-JSON error bodies (e.g., HTML 5xx gateway pages), and adds regression tests for the new behavior.
Changes:
- Wrap
resp.json()in_error_bodywithtry/except ValueErrorand fall back to a status/reason + truncated body snippet for non-JSON responses. - Add unit tests covering HTML/non-JSON bodies, empty bodies, truncation to 200 chars, and JSON-body non-regression.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| dataretrieval/waterdata/utils.py | Makes _error_body resilient to non-JSON error bodies via a safe JSON-parse fallback path. |
| tests/waterdata_utils_test.py | Adds tests for non-JSON/empty body handling, truncation behavior, and JSON regression coverage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
The existing docstring described behavior that never matched the code: it said 429 returned a JSON `message` field (it returns a hardcoded string) and that other statuses returned "raw response text" (they return code/description from the JSON envelope, or a status/reason + snippet for non-JSON bodies). Spell out the three branches (429 hardcoded, 403 hardcoded, JSON-with-non-JSON-fallback) so the docstring matches what the function actually returns. Per Copilot review on PR DOI-USGS#274. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thodson-usgs
approved these changes
May 12, 2026
Collaborator
thodson-usgs
left a comment
thodson-usgs
left a comment
There was a problem hiding this comment.
looks good, thanks!
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
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.
Closes #242.
_error_bodyblindly calledresp.json()for any status other than 429 or 403. When the upstream API returned a non-JSON body (e.g. an openresty HTML 502), this raisedrequests.exceptions.JSONDecodeError, masking the real status code with a confusing JSON parse traceback.The fix wraps the JSON parse in
try/except ValueError(covers bothjson.JSONDecodeErrorandrequests.exceptions.JSONDecodeError) and falls back to a status/reason message with a 200-char body snippet when the body isn't JSON. Empty bodies degrade to"<status>: <reason>.".Tests added in
tests/waterdata_utils_test.py:code/description(no regression)Local run:
pytest tests/waterdata_utils_test.py-> 24 passed.