-
Notifications
You must be signed in to change notification settings - Fork 257
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
Pass through errors encountered when running final execute #981
Comments
abernix
added a commit
that referenced
this issue
Aug 26, 2021
…xecute (#159)" This reverts commit 9045f55, which was from #159. See the comment in #981 which explains the justification as well, but briefly: While that PR was an improvement in principle, it: - has a bug that was first captured in #159 (comment) To quote that here: > I think there's a bug here. Let's say we have a query `{ x }` where `x` is a non-nullable field, and the federated execution has `x` throw an error. This turns all of `data` into `null` (not `{ x: null }`). But then this re-execution adds another error saying that the non-null field is null. > > Take a look at https://codesandbox.io/s/angry-raman-uuzuf?file=/src/index.js for an example of what's going on here. - Sometimes — unexpectedly to current users, at least — breaks client expectations. As of now, the pain points seem to be outweighing the gains. We'll need to revert this and revisit this when time allows with a slightly different approach and #981 tracks the need to do so. Ref: #159 Ref: apollographql/apollo-server#5550 Ref: #974 Ref: apollographql/apollo-server#4523
abernix
added a commit
that referenced
this issue
Aug 26, 2021
…xecute (#159)" (#982) This reverts commit 9045f55, which was from #159. See the comment in #981 which explains the justification as well, but briefly: While that PR was an improvement in principle, it: - has a bug that was first captured in #159 (comment) To quote that here: > I think there's a bug here. Let's say we have a query `{ x }` where `x` is a non-nullable field, and the federated execution has `x` throw an error. This turns all of `data` into `null` (not `{ x: null }`). But then this re-execution adds another error saying that the non-null field is null. > > Take a look at https://codesandbox.io/s/angry-raman-uuzuf?file=/src/index.js for an example of what's going on here. - Sometimes — unexpectedly to current users, at least — breaks client expectations. As of now, the pain points seem to be outweighing the gains. We'll need to revert this and revisit this when time allows with a slightly different approach and #981 tracks the need to do so. Ref: #159 Ref: apollographql/apollo-server#5550 Ref: #974 Ref: apollographql/apollo-server#4523
Gathering use-cases: Some examples of errors that may come from subgraphs that would be beneficial are things like:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
This is a tracking issue to account for the need to re-implement a different approach for achieving subgraph error pass-through behavior, following up on the first attempt to do so in #159 (comment).
While that was an improvement in principle, it:
has a bug that was first captured in Gateway: pass through errors encountered when running final execute #159 (comment) To quote that here:
Sometimes — unexpectedly to current users, at least — breaks client expectations.
As of now, the pain points seem to be outweighing the gains. I won't dispute the original PR having definitely solved a problem, but the original PR didn't have an issue that it closed nor did it introduce tests so I'm not super crisp on the extent. Given that we've now a non-zero amount of feedback that the approach is causing problems, we'll need to revert this and revisit this when time allows with a slightly different approach.
Ref: #159
Ref: apollographql/apollo-server#5550
Ref: #974
Ref: apollographql/apollo-server#4523
The text was updated successfully, but these errors were encountered: