fix: Surface GraphQL errors returned in the response body#3684
fix: Surface GraphQL errors returned in the response body#3684Poudel-Sanskriti wants to merge 1 commit into
Conversation
Reviewer's GuideAdds GraphQL-specific response validation to surface errors returned in the response body, distinguishing between total failure (raise) and partial success (warn), and introduces focused tests around the new behavior. Sequence diagram for GraphQLStream validate_response body-level error handlingsequenceDiagram
participant GraphQLStream
participant RESTStream
participant Response
participant FatalAPIError
GraphQLStream->>RESTStream: validate_response(response)
RESTStream-->>GraphQLStream: status_code validation ok
GraphQLStream->>Response: json()
GraphQLStream-->>GraphQLStream: json_response
alt [errors present and data is null]
GraphQLStream->>FatalAPIError: FatalAPIError("GraphQL errors in response: ...")
else [errors present and data is present]
GraphQLStream->>GraphQLStream: logger.warning("GraphQL errors in response: ...")
else [no errors or non-dict json]
GraphQLStream-->>GraphQLStream: return
end
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Documentation build overview
26 files changed ·
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
GraphQLStream.validate_response, consider defensively handlingresponse.json()failures (e.g., catchingValueError/JSONDecodeError) so non-JSON or malformed responses don’t raise unexpected errors compared to the previous REST behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `GraphQLStream.validate_response`, consider defensively handling `response.json()` failures (e.g., catching `ValueError`/`JSONDecodeError`) so non-JSON or malformed responses don’t raise unexpected errors compared to the previous REST behavior.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3684 +/- ##
==========================================
+ Coverage 94.12% 94.23% +0.11%
==========================================
Files 73 73
Lines 6193 6225 +32
Branches 759 765 +6
==========================================
+ Hits 5829 5866 +37
+ Misses 270 267 -3
+ Partials 94 92 -2
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:
|
|
Hi @edgarrmondragon, this is my first contribution to the SDK. This PR addresses #1421 by surfacing GraphQL errors returned in a 200 response body instead of letting them pass silently. Would appreciate a review whenever you have time, and I am happy to adjust the partial-success handling if you would prefer a different default. |
|
Hi @Poudel-Sanskriti, did you see the other PR? I haven't had time to look at your PR, but something to keep in mind. |
e017592 to
7869889
Compare
7869889 to
ee74e6e
Compare
Thanks @edgarrmondragon, I hadn't seen #3668. Mine differs in keeping partial results (warn when errors come with usable data, raise only when data is null) and I've already applied the malformed-body feedback you gave there, so this PR is review-ready with all checks green. This is my first SDK contribution and I'm actively watching the thread, so I'll turn any feedback around quickly. Would love a review whenever you have time! |
What does this PR do?
This adds a
validate_responseoverride toGraphQLStreamso that errors reported in a GraphQL response body are no longer silently ignored. After the existing HTTP status-code checks run, the method inspects the response body for anerrorskey. If errors are present anddatais null, it raisesFatalAPIErrorwith the GraphQL error messages. If errors are present butdatais also returned (partial success), it logs a warning and allows the sync to continue so usable records are not discarded.The validation is also defensive about malformed responses:
FatalAPIError(chained from the original decode error) instead of propagating a rawJSONDecodeError.FatalAPIErrorinstead of being treated as valid.errorsshapes (a non-list value, entries that are not objects, or objects without amessagekey) are stringified into the error message instead of being silently dropped.Why was this PR needed?
GraphQL APIs return HTTP 200 even when the request fails, placing the actual error inside the response body under an
errorskey (for example{"errors": [{"message": "Your token has insufficient scope"}], "data": null}).GraphQLStreaminheritedvalidate_responsefromRESTStream, which only inspects the HTTP status code. Because the status was 200, these errors passed validation, the stream extracted zero records, and the sync completed with no error or warning. The reporter in the linked issue hit exactly this: a token with insufficient scope produced silent data loss that was difficult to diagnose. This change makes the failure visible at the point it occurs.Design choices:
GraphQLStreamrather thanRESTStream, because body-levelerrorssemantics are specific to GraphQL and should not affect plain REST streams.datais null) raises, while partial success (datapresent alongsideerrors) warns and continues, because the GraphQL spec allows responses that carry both errors and usable data. Taps that need stricter or retriable handling can override the method.What are the relevant issue numbers?
Closes #1421
Screenshots / Recordings (if applicable)
Tests in
tests/core/rest/test_graphql.pycover error-only, multi-error, partial-success, clean, empty-errors, nonstandard error shapes, non-JSON body, and non-object body responses:The behavior was also verified end to end with a minimal tap. Against the live countries GraphQL API, a query requesting an invalid field now aborts the sync with
FatalAPIError: GraphQL errors in response: Cannot query field "bogusField" on type "Country"., where previously the same sync completed "successfully" with zero records.Does this PR meet the acceptance criteria?
Summary by Sourcery
Surface GraphQL body-level errors during response validation to avoid silent failures when HTTP status is 200.
Bug Fixes:
Tests: