-
Notifications
You must be signed in to change notification settings - Fork 1
feat(gw): error propagation #339
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @ardatan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly overhauls the error propagation mechanism within the GraphQL gateway, focusing on providing more precise and actionable error information to clients. The changes involve restructuring how errors are represented internally, introducing a sophisticated normalization layer to correctly map subgraph errors to the gateway's response paths, and enhancing data traversal to support this mapping. These improvements ensure that even in complex federated query scenarios, errors are clearly attributed and located, greatly aiding debugging and client-side error handling. The refactoring also includes internal naming clarifications and adoption of standard asynchronous primitives.
Highlights
- Enhanced Error Structure: The core error handling mechanism has been significantly upgraded.
GraphQLError
now supports structured extensions using aHashMap
, enabling richer metadata likeserviceName
and specific errorcode
s to be attached to errors. This provides clearer context for debugging and client-side error handling. - Intelligent Error Path Normalization: A new
error_normalization
module has been introduced to intelligently map errors originating from subgraphs (especially in federated queries where entities are flattened) back to their logical paths in the gateway's final response. This ensures that clients receive accurate error locations, even for complex nested data structures. - Improved Data Traversal for Error Mapping: The internal data traversal logic (
traverse_and_callback
) has been refined to track precise paths, including array indices, usingVecDeque
. This detailed path information is critical for the new error normalization process, allowing the gateway to pinpoint the exact location of an error within a deeply nested response. - Robust Testing for Error Propagation: The pull request includes comprehensive new test infrastructure, integrating
async-graphql
for easy subgraph simulation and dedicated fixtures. A newerror_propagation
test case thoroughly validates the end-to-end error handling, ensuring that errors are correctly captured, normalized, and propagated to the client. - Internal Refactorings and Simplifications: Several key internal components, such as
ExecutionContext
(nowQueryPlanExecutionContext
) andHttpExecutionRequest
(nowSubgraphExecutionRequest
), have been renamed for better clarity and consistency. The custom concurrency management has also been replaced withFuturesUnordered
for streamlined asynchronous operations.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this 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 significantly refactors error propagation within the GraphQL executor, introducing more structured handling, especially for federated queries. The changes include renaming several components for clarity, improving parallel execution logic, and adding comprehensive tests for error propagation scenarios. I've identified a critical bug where some subgraph errors were being dropped and an inconsistency in how error extensions are applied. Addressing these points will make the error handling much more robust. Overall, this is a great improvement.
This comment was marked as resolved.
This comment was marked as resolved.
✅
|
d62ddbf
to
13c3a6e
Compare
if let Some(target_paths) = | ||
job.filtered_representations_hashes.get_mut(hash) | ||
{ | ||
for indexes_in_path in target_paths { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In traverse_and_callback_mut
we give it a path and once it reaches the final segment, it executes the callback.
The traverse of final_response
starts at the root, then it moves to the first path segment until it hits the final segment.
In your code, we always start from root, for every element in the target_paths
vector.
At least from what I see, but maybe I'm missing something.
What was wrong with traverse_and_callback_mut
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually didn't touch the logic. But instead I hold the array indices (the actual values of @
in the flatten path) so I can use them for fixing the error paths in the final response. Since we already know the path, traverse_and_callback_mut is no longer needed because we already use this information to access the entity in the fina lresponse.
Nothing is wrong with it but mutable
version of traverse_and_callback
is no longer needed. Immutable version is still used to collect the entities and also indices in the paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the loop within a loop. We start from the final_response
for every for indexes_in_path in target_paths
fetch_node_id: i64, | ||
representation_hashes: Vec<u64>, | ||
representation_hash_to_index: HashMap<u64, usize>, | ||
filtered_representations_hashes: HashMap<u64, Vec<VecDeque<usize>>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment explaining the HashMap value? Or create a dedicated struct if it will make it more obvious?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are the indexes of the representations. Indexes are the actual values of @
in the flatten path.
job.representation_hash_to_index.get(&hash) | ||
{ | ||
if let Some(entity) = entities.get(*entity_index) { | ||
// SAFETY: `new_val` is a clone of an entity that lives for `'a`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is missing, it would be nice to include it, so we know why we use transmute.
if let Some(path_in_error) = &error.path { | ||
if let Some(GraphQLErrorPathSegment::String(first_path)) = path_in_error.first() { | ||
if first_path == "_entities" { | ||
if let Some(GraphQLErrorPathSegment::Index(entity_index)) = path_in_error.get(1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we somehow reduce the depth level here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try 👍
3066ce0
to
53e8bd5
Compare
e9864f4
to
a80cb5b
Compare
Rewrite of #299 within the new structure
_entities
into the real paths using flatten pathsserviceName
andcode
to the subgraph errors while passing it to the final gw responsetraverse_and_callback_mut
as it is no longer needed because we collect indexes in paths and use it with flatten path to get the mutable reference in the final response without traversing again