-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Unhandled rejection if an object has both a synchronous and asynchronous "Cannot return null for non-nullable field" error #3528
Comments
asztal
added a commit
to asztal/graphql-js
that referenced
this issue
Apr 5, 2022
asztal
added a commit
to asztal/graphql-js
that referenced
this issue
Apr 14, 2022
asztal
added a commit
to asztal/graphql-js
that referenced
this issue
Apr 19, 2022
This was referenced May 11, 2022
This was referenced Aug 18, 2022
trevor-scheer
added a commit
to apollographql/apollo-server
that referenced
this issue
Oct 4, 2023
…ld` is called "late" (#7747) Porting #6398 from AS3. This fix was unintentionally left out of AS4. Fixes #7746 The minimum version of `graphql` officially supported by Apollo Server 4 as a peer dependency, v16.6.0, contains a [serious bug that can crash your Node server](graphql/graphql-js#3528). This bug is fixed in the immediate next version, `graphql@16.7.0`, and we **strongly encourage you to upgrade your installation of `graphql` to at least v16.7.0** to avoid this bug. (For backwards compatibility reasons, we cannot change Apollo Server 4's minimum peer dependency, but will change it when we release Apollo Server 5.) Apollo Server 4 contained a particular line of code that makes triggering this crashing bug much more likely. This line was already removed in Apollo Server v3.8.2 (see #6398) but the fix was accidentally not included in Apollo Server 4. We are now including this change in Apollo Server 4, which will **reduce** the likelihood of hitting this crashing bug for users of `graphql` v16.6.0. That said, taking this `@apollo/server` upgrade **does not prevent** this bug from being triggered in other ways, and the real fix to this crashing bug is to upgrade `graphql`. --------- Co-authored-by: David Glasser <glasser@apollographql.com>
trevor-scheer
pushed a commit
to apollographql/apollo-server
that referenced
this issue
Oct 4, 2023
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.9.4 ### Patch Changes - Updated dependencies \[[`ddce036e1`](ddce036)]: - @apollo/server@4.9.4 ## @apollo/server@4.9.4 ### Patch Changes - [#7747](#7747) [`ddce036e1`](ddce036) Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - The minimum version of `graphql` officially supported by Apollo Server 4 as a peer dependency, v16.6.0, contains a [serious bug that can crash your Node server](graphql/graphql-js#3528). This bug is fixed in the immediate next version, `graphql@16.7.0`, and we **strongly encourage you to upgrade your installation of `graphql` to at least v16.7.0** to avoid this bug. (For backwards compatibility reasons, we cannot change Apollo Server 4's minimum peer dependency, but will change it when we release Apollo Server 5.) Apollo Server 4 contained a particular line of code that makes triggering this crashing bug much more likely. This line was already removed in Apollo Server v3.8.2 (see #6398) but the fix was accidentally not included in Apollo Server 4. We are now including this change in Apollo Server 4, which will **reduce** the likelihood of hitting this crashing bug for users of `graphql` v16.6.0. That said, taking this `@apollo/server` upgrade **does not prevent** this bug from being triggered in other ways, and the real fix to this crashing bug is to upgrade `graphql`. Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Sorry about the title but it's a pretty specific scenario.
It looks like this error occurs because of how errors are handled for non-nullable fields. (I saw this with the
Cannot return null for non-nullable field
error, but it seems if the resolver throws an error it would be handled the same way.)The problem appears to be here: https://github.com/graphql/graphql-js/blob/main/src/execution/execute.ts#L450
Let's say in the example below, the City object type has a resolver for
country
, which is asynchronous, and a resolver forpopulation
, which is synchronous, and they both return null (which they shouldn't, of course). The resolver for thecountry
field is run, and returns a promise. Then the resolver for thepopulation
field is run, and because it's synchronous, the error is thrown straight away, and the promise returned by thecountry
resolver is discarded. When that promise eventually rejects, the process crashes with anunhandledRejection
error.The output of this is:
The text was updated successfully, but these errors were encountered: