Skip to content

Conversation

ardatan
Copy link
Member

@ardatan ardatan commented Sep 26, 2025

Closes #228
After the execution of FlattenNode, the entity responses contain the path like ["_entities", 0] which doesn't reflect the correct path in the final response.
This PR collects the path segments in the final response for each entity only if there is an error, then re-creates the errors that have _entities in their path, then replaces this path with the correct path in the final response.

After this gets merged, I'll work on another PR that adds some extensions to the errors to specify which error belongs to which subgraph etc.

@ardatan ardatan requested a review from Copilot September 26, 2025 16:28
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances the executor to normalize flatten errors for the final response by tracking error paths during traversal and handling errors with correct path information. The changes enable proper error attribution when processing flattened entities across multiple subgraphs.

Key changes:

  • Enhanced the traverse_and_callback_mut function to track error paths during traversal
  • Modified error handling to normalize error paths for flattened entities
  • Added comprehensive test coverage for error path collection and normalization

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
lib/executor/src/utils/traverse.rs Enhanced traversal function to track error paths and added test coverage
lib/executor/src/response/graphql_error.rs Added PartialEq trait to GraphQLErrorPathSegment for testing
lib/executor/src/execution/plan.rs Modified execution logic to collect and normalize error paths for flatten operations
lib/executor/src/context.rs Enhanced error handling to process normalized error paths for entities

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@ardatan
Copy link
Member Author

ardatan commented Sep 26, 2025

@gemini-code-assist write a summary of the changes here

Copy link
Contributor

Summary of Changes

This pull request significantly refines the error handling within the executor, focusing on how errors from flattened subgraph responses are presented to the client. By introducing a robust system for normalizing error paths, it ensures that errors are accurately associated with their corresponding data points in the final merged GraphQL response, improving the clarity and utility of error messages for consumers of the API.

Highlights

  • Enhanced Error Normalization: Implemented a new mechanism to normalize and correctly map errors from subgraph responses, particularly for federated _entities, to their accurate paths in the final GraphQL response.
  • Updated Error Handling Logic: The handle_errors function in ExecutionContext now accepts an entity_index_error_map to facilitate precise error path reconstruction and duplicates errors for entities appearing in multiple paths.
  • Refactored Subgraph Response Processing: The process_subgraph_response function now returns the full SubgraphResponse object, allowing for deferred error handling and more granular control over error propagation.
  • Error Path Propagation during Traversal: The traverse_and_callback_mut utility function was extended to build and propagate error paths, ensuring that errors are correctly attributed to their respective locations within nested data structures.
  • New Test Cases: Added comprehensive unit tests to validate the correct normalization and collection of error paths for various scenarios, including nested lists and fields.
Changelog
  • lib/executor/src/context.rs
    • Imported GraphQLErrorPathSegment.
    • Modified handle_errors to accept an optional entity_index_error_map and to process _entities errors by duplicating them across all relevant paths.
  • lib/executor/src/execution/plan.rs
    • Imported GraphQLErrorPathSegment.
    • Changed process_subgraph_response return type from Value to SubgraphResponse.
    • Moved general error handling to process_job_result for Fetch jobs.
    • In FlattenFetch jobs, added logic to collect error paths during data traversal and pass them to handle_errors.
  • lib/executor/src/response/graphql_error.rs
    • Added PartialEq derive to GraphQLErrorPathSegment enum.
  • lib/executor/src/utils/traverse.rs
    • Modified traverse_and_callback_mut to accept an Option<Vec<GraphQLErrorPathSegment>> for building error paths.
    • Updated the callback signature to include the error path.
    • Implemented logic to construct error paths for array indices and object fields during traversal.
    • Added new test cases (normalize_entity_errors_correctly, test_collect_error_paths_one_level, test_collect_error_paths_two_levels) to verify error path collection.
Activity
  • The author, ardatan, initiated the PR summary and review process by invoking Gemini commands.
  • Gemini Code Assist clarified its capabilities, stating it provides feedback but doesn't execute commands.
  • A review comment from Copilot suggested an improvement to the capacity calculation for Vec in lib/executor/src/execution/plan.rs, which was incorporated into the code.

@ardatan ardatan marked this pull request as ready for review September 26, 2025 16:34
Copy link

github-actions bot commented Sep 26, 2025

🐋 This PR was built and pushed to the following Docker images:

Image Names: ghcr.io/graphql-hive/router

Platforms: linux/amd64,linux/arm64

Image Tags: ghcr.io/graphql-hive/router:pr-454 ghcr.io/graphql-hive/router:sha-98b90aa

Docker metadata
{
"buildx.build.ref": "builder-99175c53-6758-44bd-8c3f-3de17dd887dd/builder-99175c53-6758-44bd-8c3f-3de17dd887dd0/byg7tntb2wzl8v7cyghf19615",
"containerimage.descriptor": {
  "mediaType": "application/vnd.oci.image.index.v1+json",
  "digest": "sha256:5433f26b150283cdea8f8b64e8609d1e47a8e7e288e23c35e1856952edd2a92f",
  "size": 1609
},
"containerimage.digest": "sha256:5433f26b150283cdea8f8b64e8609d1e47a8e7e288e23c35e1856952edd2a92f",
"image.name": "ghcr.io/graphql-hive/router:pr-454,ghcr.io/graphql-hive/router:sha-98b90aa"
}

@graphql-hive graphql-hive deleted a comment from gemini-code-assist bot Sep 26, 2025
@graphql-hive graphql-hive deleted a comment from gemini-code-assist bot Sep 26, 2025
Copy link

github-actions bot commented Sep 26, 2025

k6-benchmark results

     ✓ response code was 200
     ✓ no graphql errors
     ✓ valid response structure

     █ setup

     checks.........................: 100.00% ✓ 235794      ✗ 0    
     data_received..................: 6.9 GB  229 MB/s
     data_sent......................: 92 MB   3.1 MB/s
     http_req_blocked...............: avg=2.67µs   min=682ns  med=1.8µs   max=4.65ms   p(90)=2.55µs  p(95)=2.95µs  
     http_req_connecting............: avg=189ns    min=0s     med=0s      max=754.89µs p(90)=0s      p(95)=0s      
     http_req_duration..............: avg=18.62ms  min=1.91ms med=17.79ms max=111.03ms p(90)=25.57ms p(95)=28.48ms 
       { expected_response:true }...: avg=18.62ms  min=1.91ms med=17.79ms max=111.03ms p(90)=25.57ms p(95)=28.48ms 
     http_req_failed................: 0.00%   ✓ 0           ✗ 78618
     http_req_receiving.............: avg=134.04µs min=24.6µs med=39.2µs  max=73.75ms  p(90)=85.52µs p(95)=368.36µs
     http_req_sending...............: avg=23.15µs  min=5.51µs med=10.55µs max=25.77ms  p(90)=15.95µs p(95)=27.99µs 
     http_req_tls_handshaking.......: avg=0s       min=0s     med=0s      max=0s       p(90)=0s      p(95)=0s      
     http_req_waiting...............: avg=18.46ms  min=1.85ms med=17.66ms max=94.04ms  p(90)=25.31ms p(95)=28.17ms 
     http_reqs......................: 78618   2615.314282/s
     iteration_duration.............: avg=19.08ms  min=4.32ms med=18.14ms max=250.84ms p(90)=26ms    p(95)=28.98ms 
     iterations.....................: 78598   2614.64896/s
     vus............................: 50      min=50        max=50 
     vus_max........................: 50      min=50        max=50 

@ardatan
Copy link
Member Author

ardatan commented Sep 26, 2025

@gemini-code-assist review the PR

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request enhances error handling for FlattenNode executions by normalizing error paths that originate from entity fetches. The changes correctly map internal _entities paths to their corresponding paths in the final GraphQL response. My review focuses on the correctness of this new error handling logic and suggests some improvements for code clarity and robustness. I've identified a critical issue where errors could be silently dropped and a medium-severity issue for improving code style.

@ardatan
Copy link
Member Author

ardatan commented Sep 26, 2025

@gemini-code-assist review the PR

@ardatan ardatan requested a review from Copilot September 26, 2025 16:51
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request correctly remaps error paths from the internal _entities representation to the final response paths. The changes look good overall. I've identified one bug where an error could be silently dropped and suggested a fix that also improves performance. I've also left a few comments regarding code style and duplication to improve maintainability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error propagation
1 participant