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

Unhandled rejection if an object has both a synchronous and asynchronous "Cannot return null for non-nullable field" error #3528

Closed
asztal opened this issue Apr 5, 2022 · 0 comments · Fixed by #3706

Comments

@asztal
Copy link

asztal commented Apr 5, 2022

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 for population, which is synchronous, and they both return null (which they shouldn't, of course). The resolver for the country field is run, and returns a promise. Then the resolver for the population field is run, and because it's synchronous, the error is thrown straight away, and the promise returned by the country resolver is discarded. When that promise eventually rejects, the process crashes with an unhandledRejection error.

// File: testcase.mjs
import { graphql, GraphQLInt, GraphQLList, GraphQLNonNull, GraphQLObjectType, GraphQLSchema, GraphQLString } from "graphql";

process.on("unhandledRejection", reason => {
    console.error("Unhandled rejection!", reason);
});

const City = new GraphQLObjectType({
    name: "City",
    fields: {
        name: { type: new GraphQLNonNull(GraphQLString) },
        country: { type: new GraphQLNonNull(GraphQLString), resolve: () => Promise.resolve(null) },
        population: { type: new GraphQLNonNull(GraphQLInt), resolve: () => null },
    }
});

const Query = new GraphQLObjectType({
    name: "Query",
    fields: {
        cities: {
            type: new GraphQLNonNull(new GraphQLList(new GraphQLNonNull(City))),
            resolve() {
                return [{ name: "London", country: "GB", population: 8_982_000 }];
            }
        }
    }
});

const schema = new GraphQLSchema({
    types: [City],
    query: Query,
});

try {
    graphql(schema, `
        query {
            cities { name country population }
        }
    `).then(result => {
        if (result.errors)
            console.error("Error returned in result.errors[]:", result.errors);
        else
            console.log("Result:", result.data);
    }, error => {
        console.error("graphql() rejected:", error);
    });
} catch(err) {
    console.error("Error caught by try/catch:", err);
}

The output of this is:

Error returned in result.errors[]: [
  Error: Cannot return null for non-nullable field City.population.
      at completeValue (/home/lee/restapi/node_modules/graphql/execution/execute.js:566:13)
      at resolveField (/home/lee/restapi/node_modules/graphql/execution/execute.js:478:19)
      at executeFields (/home/lee/restapi/node_modules/graphql/execution/execute.js:292:18)
      at collectAndExecuteSubfields (/home/lee/restapi/node_modules/graphql/execution/execute.js:755:10)
      at completeObjectValue (/home/lee/restapi/node_modules/graphql/execution/execute.js:745:10)
      at completeValue (/home/lee/restapi/node_modules/graphql/execution/execute.js:597:12)
      at completeValue (/home/lee/restapi/node_modules/graphql/execution/execute.js:563:21)
      at /home/lee/restapi/node_modules/graphql/execution/execute.js:627:25
      at Array.map (<anonymous>)
      at safeArrayFrom (/home/lee/restapi/node_modules/graphql/jsutils/safeArrayFrom.js:36:23) {
    locations: [ [Object] ],
    path: [ 'cities', 0, 'population' ]
  }
]
Unhandled rejection! Error: Cannot return null for non-nullable field City.country.
    at completeValue (/home/lee/restapi/node_modules/graphql/execution/execute.js:566:13)
    at /home/lee/restapi/node_modules/graphql/execution/execute.js:475:16 {
  locations: [ { line: 3, column: 27 } ],
  path: [ 'cities', 0, 'country' ]
}
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
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
Labels
None yet
Projects
None yet
1 participant