Skip to content

fix: Surface GraphQL errors returned in the response body#3684

Open
Poudel-Sanskriti wants to merge 1 commit into
meltano:mainfrom
Poudel-Sanskriti:fix/graphql-error-handling
Open

fix: Surface GraphQL errors returned in the response body#3684
Poudel-Sanskriti wants to merge 1 commit into
meltano:mainfrom
Poudel-Sanskriti:fix/graphql-error-handling

Conversation

@Poudel-Sanskriti

@Poudel-Sanskriti Poudel-Sanskriti commented Jun 29, 2026

Copy link
Copy Markdown

What does this PR do?

This adds a validate_response override to GraphQLStream so 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 an errors key. If errors are present and data is null, it raises FatalAPIError with the GraphQL error messages. If errors are present but data is 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:

  • A body that is not valid JSON raises FatalAPIError (chained from the original decode error) instead of propagating a raw JSONDecodeError.
  • A body that is JSON but not an object raises FatalAPIError instead of being treated as valid.
  • Nonstandard errors shapes (a non-list value, entries that are not objects, or objects without a message key) 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 errors key (for example {"errors": [{"message": "Your token has insufficient scope"}], "data": null}).

GraphQLStream inherited validate_response from RESTStream, 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:

  • The override lives in GraphQLStream rather than RESTStream, because body-level errors semantics are specific to GraphQL and should not affect plain REST streams.
  • Total failure (data is null) raises, while partial success (data present alongside errors) 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.
  • Malformed bodies (non-JSON or non-object) raise rather than pass silently, in line with the review feedback given on feat: Handle GraphQL error responses #3668.

What are the relevant issue numbers?

Closes #1421

Screenshots / Recordings (if applicable)

Tests in tests/core/rest/test_graphql.py cover error-only, multi-error, partial-success, clean, empty-errors, nonstandard error shapes, non-JSON body, and non-object body responses:

$ uv run pytest tests/core/rest/test_graphql.py -q
...........                                             [100%]
11 passed in 0.06s

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?

  • Tests added for new/changed behavior
  • All tests passing
  • Follows project style guide
  • No breaking changes introduced
  • Documentation updated (if applicable) - not needed; behavior is internal to response validation

Summary by Sourcery

Surface GraphQL body-level errors during response validation to avoid silent failures when HTTP status is 200.

Bug Fixes:

  • Raise a FatalAPIError when a GraphQL response contains errors and no data, preventing silent data loss.
  • Log a warning instead of failing when GraphQL responses include both errors and usable data, allowing partial results to be processed.

Tests:

  • Add GraphQLStream response validation tests covering error-only, partial-success, and success scenarios, including logging behavior.

@sourcery-ai

sourcery-ai Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Reviewer's Guide

Adds 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 handling

sequenceDiagram
    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
Loading

File-Level Changes

Change Details Files
GraphQLStream now overrides response validation to inspect the JSON body for GraphQL errors and either raise or warn based on presence of data.
  • Import FatalAPIError and requests typing to support new validation logic.
  • Override validate_response in GraphQLStream, calling the RESTStream implementation first for status-code checks.
  • Parse the JSON response, ignore non-dict bodies, and look for an errors key.
  • Aggregate error messages from the errors list into a single string.
  • If errors exist and data is missing/None, raise FatalAPIError with the aggregated message.
  • If errors exist and data is present, log a warning and allow processing to continue.
singer_sdk/streams/graphql.py
Introduce tests that exercise the new GraphQL response validation behavior, including failure, partial success, and warning logging.
  • Create a minimal SimpleGraphQLStream subclass for testing.
  • Add a helper to build fake HTTP 200 responses with JSON bodies.
  • Parametrize tests to cover error-only, error-plus-data, and no-error responses, asserting FatalAPIError is raised only when data is absent.
  • Add a dedicated test that verifies a warning log entry is produced when both errors and data are present.
tests/core/rest/test_graphql.py

Assessment against linked issues

Issue Objective Addressed Explanation
#1421 Update GraphQL response validation to detect and handle GraphQL-specific errors returned in the errors array of the response body, instead of silently accepting HTTP 200 responses with failures.
#1421 Ensure behavior is covered by tests so that GraphQL error handling (fatal when data is null, non-fatal when partial data is present) is verified.

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@read-the-docs-community

read-the-docs-community Bot commented Jun 29, 2026

Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've left some high level feedback:

  • 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.
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.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@codecov

codecov Bot commented Jun 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.23%. Comparing base (7c7932d) to head (ee74e6e).
⚠️ Report is 24 commits behind head on main.

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     
Flag Coverage Δ
core 83.10% <100.00%> (+0.16%) ⬆️
end-to-end 75.93% <30.00%> (-0.17%) ⬇️
optional-components 44.77% <5.00%> (-0.09%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Poudel-Sanskriti

Copy link
Copy Markdown
Author

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.

@codspeed-hq

codspeed-hq Bot commented Jun 29, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 14 untouched benchmarks


Comparing Poudel-Sanskriti:fix/graphql-error-handling (ee74e6e) with main (15dbfc5)

Open in CodSpeed

@edgarrmondragon

Copy link
Copy Markdown
Collaborator

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.

@edgarrmondragon edgarrmondragon added Community-Contributed PR HTTP HTTP based taps and targets such (REST, XML, etc.) labels Jun 29, 2026
@Poudel-Sanskriti Poudel-Sanskriti force-pushed the fix/graphql-error-handling branch 2 times, most recently from e017592 to 7869889 Compare July 3, 2026 05:20
@Poudel-Sanskriti Poudel-Sanskriti force-pushed the fix/graphql-error-handling branch from 7869889 to ee74e6e Compare July 3, 2026 05:29
@Poudel-Sanskriti

Copy link
Copy Markdown
Author

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.

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Community-Contributed PR HTTP HTTP based taps and targets such (REST, XML, etc.)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]: Handle GraphQL errors

2 participants