feat: Handle GraphQL error responses#3668
Open
Dexter2099 wants to merge 3 commits into
Open
Conversation
Contributor
Reviewer's GuideExtend GraphQLStream response validation to treat GraphQL error payloads (non-empty File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3668 +/- ##
==========================================
+ Coverage 94.13% 94.15% +0.01%
==========================================
Files 73 73
Lines 6203 6224 +21
Branches 763 766 +3
==========================================
+ Hits 5839 5860 +21
Misses 270 270
Partials 94 94
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
validate_responsesignature referencesrequests.Responsebutrequestsis only imported underTYPE_CHECKING, which will trigger aNameErrorat runtime on Python versions that evaluate annotations; either move the import out of theTYPE_CHECKINGblock, enablefrom __future__ import annotations, or quote the type annotation. - In
validate_response, JSON decode failures and non-dict payloads are silently ignored; consider at least logging or surfacing this scenario if it might indicate a misbehaving GraphQL API rather than a valid non-JSON response.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `validate_response` signature references `requests.Response` but `requests` is only imported under `TYPE_CHECKING`, which will trigger a `NameError` at runtime on Python versions that evaluate annotations; either move the import out of the `TYPE_CHECKING` block, enable `from __future__ import annotations`, or quote the type annotation.
- In `validate_response`, JSON decode failures and non-dict payloads are silently ignored; consider at least logging or surfacing this scenario if it might indicate a misbehaving GraphQL API rather than a valid non-JSON response.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Documentation build overview
26 files changed ·
|
Dexter2099
force-pushed
the
codex/handle-graphql-errors
branch
from
014116c to
85cdf6b
Compare
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
GraphQLStream.validate_response, theresponse: requests.Responsetype hint will raise aNameErrorat runtime becauserequestsis only imported underTYPE_CHECKING; either move the import out of the guard, quote the annotation, or enablefrom __future__ import annotationsto avoid this.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `GraphQLStream.validate_response`, the `response: requests.Response` type hint will raise a `NameError` at runtime because `requests` is only imported under `TYPE_CHECKING`; either move the import out of the guard, quote the annotation, or enable `from __future__ import annotations` to avoid this.
## Individual Comments
### Comment 1
<location path="tests/core/test_streams.py" line_range="397-420" />
<code_context>
assert list(records) == [{"id": 1, "value": "abc"}, {"id": 2, "value": "def"}]
+def test_graphql_validate_response_raises_for_errors(tap: Tap):
+ """Validate GraphQL error payloads are handled as response errors."""
+ fake_response = requests.Response()
+ fake_response.status_code = 200
+ fake_response.reason = "OK"
+ fake_response.url = GraphqlTestStream.url_base
+ fake_response._content = json.dumps(
+ {
+ "errors": [
+ {
+ "message": 'Cannot query field "nonexistentField" on type "Query".',
+ },
+ ],
+ "data": None,
+ },
+ ).encode()
+
+ stream = GraphqlTestStream(tap)
+
+ with pytest.raises(
+ FatalAPIError,
+ match='GraphQL API error: Cannot query field "nonexistentField"',
+ ):
+ stream.validate_response(fake_response)
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for multiple GraphQL errors to verify message aggregation logic
This test only exercises a single-entry `errors` list. Since `validate_response` joins multiple error messages with `'; '.join(...)`, please add a case with multiple errors and assert that the `FatalAPIError` message includes all of them, in order, to lock in the aggregation behavior and catch regressions.
```suggestion
def test_graphql_validate_response_raises_for_errors(tap: Tap):
"""Validate GraphQL error payloads are handled as response errors."""
fake_response = requests.Response()
fake_response.status_code = 200
fake_response.reason = "OK"
fake_response.url = GraphqlTestStream.url_base
fake_response._content = json.dumps(
{
"errors": [
{
"message": 'Cannot query field "nonexistentField" on type "Query".',
},
],
"data": None,
},
).encode()
stream = GraphqlTestStream(tap)
with pytest.raises(
FatalAPIError,
match='GraphQL API error: Cannot query field "nonexistentField"',
):
stream.validate_response(fake_response)
def test_graphql_validate_response_raises_for_multiple_errors(tap: Tap):
"""Validate that multiple GraphQL errors are aggregated into a single message."""
fake_response = requests.Response()
fake_response.status_code = 200
fake_response.reason = "OK"
fake_response.url = GraphqlTestStream.url_base
fake_response._content = json.dumps(
{
"errors": [
{"message": "First error."},
{"message": "Second error."},
],
"data": None,
},
).encode()
stream = GraphqlTestStream(tap)
# Expect the error messages to be joined with '; ' in order.
with pytest.raises(
FatalAPIError,
match=r"GraphQL API error: First error\.; Second error\.",
):
stream.validate_response(fake_response)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+397
to
+420
| def test_graphql_validate_response_raises_for_errors(tap: Tap): | ||
| """Validate GraphQL error payloads are handled as response errors.""" | ||
| fake_response = requests.Response() | ||
| fake_response.status_code = 200 | ||
| fake_response.reason = "OK" | ||
| fake_response.url = GraphqlTestStream.url_base | ||
| fake_response._content = json.dumps( | ||
| { | ||
| "errors": [ | ||
| { | ||
| "message": 'Cannot query field "nonexistentField" on type "Query".', | ||
| }, | ||
| ], | ||
| "data": None, | ||
| }, | ||
| ).encode() | ||
|
|
||
| stream = GraphqlTestStream(tap) | ||
|
|
||
| with pytest.raises( | ||
| FatalAPIError, | ||
| match='GraphQL API error: Cannot query field "nonexistentField"', | ||
| ): | ||
| stream.validate_response(fake_response) |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add coverage for multiple GraphQL errors to verify message aggregation logic
This test only exercises a single-entry errors list. Since validate_response joins multiple error messages with '; '.join(...), please add a case with multiple errors and assert that the FatalAPIError message includes all of them, in order, to lock in the aggregation behavior and catch regressions.
Suggested change
| def test_graphql_validate_response_raises_for_errors(tap: Tap): | |
| """Validate GraphQL error payloads are handled as response errors.""" | |
| fake_response = requests.Response() | |
| fake_response.status_code = 200 | |
| fake_response.reason = "OK" | |
| fake_response.url = GraphqlTestStream.url_base | |
| fake_response._content = json.dumps( | |
| { | |
| "errors": [ | |
| { | |
| "message": 'Cannot query field "nonexistentField" on type "Query".', | |
| }, | |
| ], | |
| "data": None, | |
| }, | |
| ).encode() | |
| stream = GraphqlTestStream(tap) | |
| with pytest.raises( | |
| FatalAPIError, | |
| match='GraphQL API error: Cannot query field "nonexistentField"', | |
| ): | |
| stream.validate_response(fake_response) | |
| def test_graphql_validate_response_raises_for_errors(tap: Tap): | |
| """Validate GraphQL error payloads are handled as response errors.""" | |
| fake_response = requests.Response() | |
| fake_response.status_code = 200 | |
| fake_response.reason = "OK" | |
| fake_response.url = GraphqlTestStream.url_base | |
| fake_response._content = json.dumps( | |
| { | |
| "errors": [ | |
| { | |
| "message": 'Cannot query field "nonexistentField" on type "Query".', | |
| }, | |
| ], | |
| "data": None, | |
| }, | |
| ).encode() | |
| stream = GraphqlTestStream(tap) | |
| with pytest.raises( | |
| FatalAPIError, | |
| match='GraphQL API error: Cannot query field "nonexistentField"', | |
| ): | |
| stream.validate_response(fake_response) | |
| def test_graphql_validate_response_raises_for_multiple_errors(tap: Tap): | |
| """Validate that multiple GraphQL errors are aggregated into a single message.""" | |
| fake_response = requests.Response() | |
| fake_response.status_code = 200 | |
| fake_response.reason = "OK" | |
| fake_response.url = GraphqlTestStream.url_base | |
| fake_response._content = json.dumps( | |
| { | |
| "errors": [ | |
| {"message": "First error."}, | |
| {"message": "Second error."}, | |
| ], | |
| "data": None, | |
| }, | |
| ).encode() | |
| stream = GraphqlTestStream(tap) | |
| # Expect the error messages to be joined with '; ' in order. | |
| with pytest.raises( | |
| FatalAPIError, | |
| match=r"GraphQL API error: First error\.; Second error\.", | |
| ): | |
| stream.validate_response(fake_response) |
Dexter2099
force-pushed
the
codex/handle-graphql-errors
branch
from
85cdf6b to
ed7e22e
Compare
edgarrmondragon
changed the title
Comment on lines
+46
to
+49
| try: | ||
| data = response.json() | ||
| except ValueError: | ||
| return |
Collaborator
There was a problem hiding this comment.
Returning here would mean processing of the response continues as if it was valid. Should we raise FatalAPIError(...) from ... here?
Author
There was a problem hiding this comment.
I replaced the malformed-response silent returns with FatalAPIError; the JSON decode path now uses raise ... from e so the original parsing failure is preserved.
I kept valid GraphQL payloads without errors (missing-errors / empty-errors) as non-fatal, and moved only malformed bodies into the raises test.
5 tasks
Poudel-Sanskriti
pushed a commit
to Poudel-Sanskriti/meltano-sdk
that referenced
this pull request
Jul 3, 2026
Align validate_response with review feedback on meltano#3668: - Catch ValueError from response.json() and raise FatalAPIError so non-JSON bodies fail loudly instead of propagating a decode error. - Raise FatalAPIError when the body is JSON but not an object, instead of silently treating the response as valid. - Stringify non-dict error entries and non-list errors values instead of silently dropping them. - Cover all new paths with tests, including multi-error aggregation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Poudel-Sanskriti
pushed a commit
to Poudel-Sanskriti/meltano-sdk
that referenced
this pull request
Jul 3, 2026
Align validate_response with review feedback on meltano#3668: - Catch ValueError from response.json() and raise FatalAPIError so non-JSON bodies fail loudly instead of propagating a decode error. - Raise FatalAPIError when the body is JSON but not an object, instead of silently treating the response as valid. - Stringify non-dict error entries and non-list errors values instead of silently dropping them. - Cover all new paths with tests, including multi-error aggregation.
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
2 participants
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 #1421
Summary
errorsfield during response validationFatalAPIErrorwith the GraphQL error message instead of treating the response as successfulTesting
uv run pytest tests/core/test_streams.py::test_graphql_validate_response_raises_for_errors -quv run pytest tests/core/test_streams.py -qnox -rs tests-3.11 -- tests/core/test_streams.pypre-commit run --files singer_sdk/streams/graphql.py tests/core/test_streams.pySummary by Sourcery
Handle GraphQL error payloads returned with HTTP 200 by validating the response body and surfacing errors as fatal API failures.
Bug Fixes:
Tests: