Skip to content
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

execution: Preserve extensions in parseValue errors #3785

Closed
wants to merge 1 commit into from

Conversation

glasser
Copy link
Contributor

@glasser glasser commented Nov 22, 2022

Previously, the only fields observed on an error thrown by (for example)
parseValue were message and originalError. Now, the error itself is
used as the originalError; this may be mildly backwards-incompatible
if you added extensions on the error itself which you for some reason
wanted to disappear, but that seems unlikely.

Addresses an issue raised in
apollographql/apollo-server#7178

@netlify
Copy link

netlify bot commented Nov 22, 2022

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit 5a76837
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/637ea52154daea0009201986
😎 Deploy Preview https://deploy-preview-3785--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

Hi @glasser, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

  • @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
  • @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

@glasser
Copy link
Contributor Author

glasser commented Nov 22, 2022

I'm not sure this is the best solution: another solution would just be to always pass originalError: error, and another would be to explicitly copy extensions: error.extensions along with originalError: error.originalError (and maybe other things like stack as well?). These both also cause my test to pass, and also ensure that extensions from error are honored even if it itself has its own originalError, so in fact originalError: error feels like the best approach. However, the approach taken in this PR feels a bit more backwards-compatible?

(Is there an appropriate pattern for "make a copy of an error but with the message changed"? Perhaps going through .toJSON()?)

glasser added a commit to apollographql/apollo-server that referenced this pull request Nov 22, 2022
Adding `BAD_USER_INPUT` is a nice default (and overrides the
inappropriate default of `INTERNAL_SERVER_ERROR`) but if somebody has
set a `code` already, we shouldn't override.

(Note that there's a separate issue where graphql-js throws out
extensions from the thrown error itself and only pays attention to
extensions on the error's originalError; we're trying to fix that in
graphql/graphql-js#3785 but this is orthogonal.)

Fixes #7178.
@glasser
Copy link
Contributor Author

glasser commented Nov 22, 2022

As a workaround, you can throw something like:

const e = new GraphQLError('message', {extensions: {my: 'extensions'}});
throw new GraphQLError(e.message, { originalError: e });

glasser added a commit to apollographql/apollo-server that referenced this pull request Nov 23, 2022
…7183)

Adding `BAD_USER_INPUT` is a nice default (and overrides the
inappropriate default of `INTERNAL_SERVER_ERROR`) but if somebody has
set a `code` already, we shouldn't override.

(Note that there's a separate issue where graphql-js throws out
extensions from the thrown error itself and only pays attention to
extensions on the error's originalError; we're trying to fix that in
graphql/graphql-js#3785 but this is orthogonal.)

Fixes #7178.

Co-authored-by: Trevor Scheer <trevor.scheer@gmail.com>
glasser pushed a commit to apollographql/apollo-server that referenced this pull request Nov 23, 2022
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @apollo/server-integration-testsuite@4.2.0

### Minor Changes

- [#7171](#7171)
[`37b3b7fb5`](37b3b7f)
Thanks [@glasser](https://github.com/glasser)! - If a POST body contains
a non-string `operationName` or a non-object `variables` or
`extensions`, fail with status code 400 instead of ignoring the field.

In addition to being a reasonable idea, this provides more compliance
with the "GraphQL over HTTP" spec.

This is a backwards incompatible change, but we are still early in the
Apollo Server 4 adoption cycle and this is in line with the change
already made in Apollo Server 4 to reject requests providing `variables`
or `extensions` as strings. If this causes major problems for users who
have already upgraded to Apollo Server 4 in production, we can consider
reverting or partially reverting this change.

### Patch Changes

- [#7170](#7170)
[`4ce738193`](4ce7381)
Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Update
@apollo/utils packages to v2 (dropping node 12 support)

- [#7179](#7179)
[`c8129c23f`](c8129c2)
Thanks [@renovate](https://github.com/apps/renovate)! - Fix a few tests
to support (but not require) TypeScript 4.9.

- [#7171](#7171)
[`37b3b7fb5`](37b3b7f)
Thanks [@glasser](https://github.com/glasser)! - The integration test
suite now incorporates the `graphql-http` package's audit suite for the
"GraphQL over HTTP" specification.

- [#7183](#7183)
[`46af8255c`](46af825)
Thanks [@glasser](https://github.com/glasser)! - Apollo Server tries to
detect if execution errors are variable coercion errors in order to give
them a `code` extension of `BAD_USER_INPUT` rather than
`INTERNAL_SERVER_ERROR`. Previously this would unconditionally set the
`code`; now, it only sets the `code` if no `code` is already set, so
that (for example) custom scalar `parseValue` methods can throw errors
with specific `code`s. (Note that a separate graphql-js bug can lead to
these extensions being lost; see
<graphql/graphql-js#3785> for details.)

- Updated dependencies
\[[`4ce738193`](4ce7381),
[`37b3b7fb5`](37b3b7f),
[`b1548c1d6`](b1548c1),
[`7ff96f533`](7ff96f5),
[`46af8255c`](46af825)]:
    -   @apollo/server@4.2.0

## @apollo/server@4.2.0

### Minor Changes

- [#7171](#7171)
[`37b3b7fb5`](37b3b7f)
Thanks [@glasser](https://github.com/glasser)! - If a POST body contains
a non-string `operationName` or a non-object `variables` or
`extensions`, fail with status code 400 instead of ignoring the field.

In addition to being a reasonable idea, this provides more compliance
with the "GraphQL over HTTP" spec.

This is a backwards incompatible change, but we are still early in the
Apollo Server 4 adoption cycle and this is in line with the change
already made in Apollo Server 4 to reject requests providing `variables`
or `extensions` as strings. If this causes major problems for users who
have already upgraded to Apollo Server 4 in production, we can consider
reverting or partially reverting this change.

- [#7184](#7184)
[`b1548c1d6`](b1548c1)
Thanks [@glasser](https://github.com/glasser)! - Don't automatically
install the usage reporting plugin in servers that appear to be hosting
a federated subgraph (based on the existence of a field `_Service.sdl:
String`). This is generally a misconfiguration. If an API key and graph
ref are provided to the subgraph, log a warning and do not enable the
usage reporting plugin. If the usage reporting plugin is explicitly
installed in a subgraph, log a warning but keep it enabled.

### Patch Changes

- [#7170](#7170)
[`4ce738193`](4ce7381)
Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Update
@apollo/utils packages to v2 (dropping node 12 support)

- [#7172](#7172)
[`7ff96f533`](7ff96f5)
Thanks [@trevor-scheer](https://github.com/trevor-scheer)! -
startStandaloneServer: Restore body-parser request limit to 50mb (as it
was in the `apollo-server` package in Apollo Server 3)

- [#7183](#7183)
[`46af8255c`](46af825)
Thanks [@glasser](https://github.com/glasser)! - Apollo Server tries to
detect if execution errors are variable coercion errors in order to give
them a `code` extension of `BAD_USER_INPUT` rather than
`INTERNAL_SERVER_ERROR`. Previously this would unconditionally set the
`code`; now, it only sets the `code` if no `code` is already set, so
that (for example) custom scalar `parseValue` methods can throw errors
with specific `code`s. (Note that a separate graphql-js bug can lead to
these extensions being lost; see
<graphql/graphql-js#3785> for details.)

- Updated dependencies
\[[`4ce738193`](4ce7381),
[`45856e1dd`](45856e1)]:
    -   @apollo/server-gateway-interface@1.0.6

## @apollo/server-gateway-interface@1.0.6

### Patch Changes

- [#7170](#7170)
[`4ce738193`](4ce7381)
Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Update
@apollo/utils packages to v2 (dropping node 12 support)

- [#7173](#7173)
[`45856e1dd`](45856e1)
Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Remove
unnecessary engines constraint on types-only package

## @apollo/server-plugin-response-cache@4.0.2

### Patch Changes

- [#7170](#7170)
[`4ce738193`](4ce7381)
Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Update
@apollo/utils packages to v2 (dropping node 12 support)

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@IvanGoncharov
Copy link
Member

These both also cause my test to pass, and also ensure that extensions from error are honored even if it itself has its own originalError, so in fact originalError: error feels like the best approach.

@glasser I agree, having correct semantic "chain of errors" make more sense.
Original error for "variable error" is "value error", and original error for "value error".

The entire mechanism of prefixing errors is a workaround for absence of proper way to represent "error chaining" in graphql, see graphql/graphql-spec#893
Moreover, single "variable error" can contain multiple "value error"s and it useful for IDE validation.

TL;DR; I'm for originalError: error solution (as breaking change) in main and for originalError: error.originalError ?? error solution for v15/v16 if needed.

What do you think?

Previously, the only fields observed on an error thrown by (for example)
parseValue were `message` and `originalError`. Now, the error itself is
used as the `originalError`; this may be mildly backwards-incompatible
if you added extensions on the error itself which you for some reason
wanted to disappear, but that seems unlikely.

Addresses an issue raised in
apollographql/apollo-server#7178
@glasser
Copy link
Contributor Author

glasser commented Nov 23, 2022

@IvanGoncharov OK, I changed this to originalError: error. I don't feel too much urgency for a backport (as long as v17 is coming out eventually!) since there's a workaround.

@yaacovCR
Copy link
Contributor

Looks like this fix was merged into main as https://github.com/graphql/graphql-js/pull/3837/files

Not sure the exact where’s and whys but thank you @glasser for working on this!

@yaacovCR yaacovCR closed this Sep 26, 2024
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.

3 participants