-
Notifications
You must be signed in to change notification settings - Fork 3
refactor(error-handling): add context to PlanExecutionError
#513
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
Conversation
✅
|
|
🐋 This PR was built and pushed to the following Docker images: Image Names: Platforms: Image Tags: Docker metadata{
"buildx.build.ref": "builder-c9405e6d-e6d6-42e9-b89d-544bc9eb83ad/builder-c9405e6d-e6d6-42e9-b89d-544bc9eb83ad0/r7khlt5t0q0z5qxo7dxrlfw77",
"containerimage.descriptor": {
"mediaType": "application/vnd.oci.image.index.v1+json",
"digest": "sha256:dd5491a7e87dd95b2496250ffff48ccfcf5c4c364bf66a3fb632cfafea969f4c",
"size": 1609
},
"containerimage.digest": "sha256:dd5491a7e87dd95b2496250ffff48ccfcf5c4c364bf66a3fb632cfafea969f4c",
"image.name": "ghcr.io/graphql-hive/router:pr-513,ghcr.io/graphql-hive/router:sha-2e4de3e"
} |
|
/gemini review |
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 introduces a more structured and performant approach to handling execution errors by adding context, such as subgraph name and path, in a lazy manner. This is a significant improvement over manual GraphQL error creation, as it avoids unnecessary work on successful execution paths. My review includes a couple of suggestions to further enhance the implementation: one for performance by avoiding an unnecessary clone during error conversion, and another to remove what appears to be dead code, improving maintainability. Overall, this is a solid contribution to improving the error handling mechanism.
8e5e4ea to
a2de476
Compare
PlanExecutionError
| #[derive(thiserror::Error, Debug, Clone, IntoStaticStr)] | ||
| pub enum PlanExecutionErrorKind { | ||
| #[error("Projection faiure: {0}")] | ||
| #[strum(serialize = "PROJECTION_FAILURE")] |
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 like strum usage here, it makes error code much simpler to deal with.
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.
LGTM.
Can you elaborate on the need for lazy init of the error-context fields? Is it to evaluate/clone the value only in case there's an actual error?
I will improve names soon, it's a draft anyway, I just wanted to showcase how we could do errors, instead of manually creating GraphQL Error with code and stuff in extensions.
^ is it still a draft? I did not merge it due to this note 😆
yes |
This commit adds context to the `PlanExecutionError` enum. It includes the subgraph name and the affected path in case of a projection or header propagation failure. It also adds a `ResultExt` trait to map `ProjectionError` and `HeaderRuleRuntimeError` to `PlanExecutionError`.
a2de476 to
83b8e9a
Compare
This commit adds context to the
PlanExecutionErrorenum. It includes the subgraph name and the affected path in case of a projection or header propagation failure.It also adds a
ResultExttrait to mapProjectionErrorandHeaderRuleRuntimeErrortoPlanExecutionError.I will improve names soon, it's a draft anyway, I just wanted to showcase how we could do errors, instead of manually creating GraphQL Error with code and stuff in extensions.