Broken promise behaviour when types used in list have async field resolve functions. about graphql-js

[sarkistlt]

Having almost same problem. I try to use type of "Customer" which fetch some data from SQL and some from MongoDb.

export let customerType = new GraphQLObjectType({
    name: 'customerType',
    description: 'Customer schema',
    fields: {
        id: {type: GraphQLInt},
        createdAt: {type: GraphQLString},
        updatedAt: {type: GraphQLString},
        deletedAt: {type: GraphQLString},
        firstName: {type: GraphQLString},
        lastName: {type: GraphQLString},
        email: {type: GraphQLString},
        password: {type: GraphQLString},
        ...
        ...
        ...
        creditCardExpDate: {type: GraphQLString},
        numLogins: {type: GraphQLInt},
        quiz: {
            type: new GraphQLList(quizType),
            resolve: (THIS) => Customers.quiz({id: THIS.id})
        }
    }
});

All fields comes from SQL besides "quiz", resolve method for "quiz" return Mongoose promise which then return array of object. But on this query:

{
  getCustomers(id: 45) {
    id
    firstName
    quiz {
      message
      recommendedProducts
    }
  }
}

I get:

{
  "data": {
    "getCustomers": [
      {
        "id": 45,
        "firstName": "Genesis",
        "quiz": [
          {
            "message": null,
            "recommendedProducts": null
          }
        ]
      }
    ]
  }
}

Any solution so far?

[latentflip]

Alright, well, I think I have a reasonable handle on the codebase now, thanks to a lot of node-inspectoring ;).

Here's a much simpler proof of the bug:

import {
  graphql,
  GraphQLSchema,
  GraphQLObjectType,
  GraphQLString,
  GraphQLNonNull,
  GraphQLList,
} from 'graphql';

var orgType = new GraphQLObjectType({
  name: 'Org',
  fields: () => ({
    id: {
      type: new GraphQLNonNull(GraphQLString),
      resolve: () => {
        return Promise.resolve(Math.random + '');
      }
    }
  })
});

var schema = new GraphQLSchema({
  query: new GraphQLObjectType({
    name: 'RootQueryType',
    fields: {
      orgs: {
        type: new GraphQLList(orgType),
        resolve: () => {
          return [{ id: 1 }]
        }
      }
    }
  }),
});


var query = `
query Foo {
  orgs {
    id,
  },
}
`

graphql(schema, query).then(result => {
  console.log('RESULT', JSON.stringify(result, null, 2));
}).catch(console.log);

Which results in

RESULT {
  "data": {
    "orgs": [
      {
      }
    ]
  }
}

but if you change orgType.id.resolve to just return a value, you get

RESULT {
  "data": {
    "orgs": [
      {
        "id": "0.8567653454374522"
      }
    ]
  }
}

as expected.

Here's what's happening:

1. Because org is a list type, https://github.com/graphql/graphql-js/blob/master/src/executor/executor.js#L599-L603 we hit this line of code, where we're mapping over the list of orgs, and completing fields for each one.
2. But each of those ends up returning a promise
3. This means that resolveFieldOrError ends up returning something that looks like [ Promise ]
4. Which ends up here: https://github.com/graphql/graphql-js/blob/master/src/executor/executor.js#L470-L476
5. Since we have an array with promises in it, we should be waiting for promise resolution, but we don't since an array doesn't pass the isThenable check.
6. This means our field ends up just getting the array of promises, and since promises serialize to {} we end up with [ {} ] in our result.

Fix?

The simplest fix would be to add the following between these two lines: https://github.com/graphql/graphql-js/blob/master/src/executor/executor.js#L469-L470 but I don't know the codebase well enough to know if this should move into isThenable in case there are similar issues elsewhere? Or if it should be in resolveFieldOrError maybe?

    //convert Array(<Promise>), to Promise(<Array>)
    if (Array.isArray(result) && result.some(isThenable)) {
      result = Promise.all(result);
    }

Thoughts? I can try and add a test to the star wars stuff that demonstrates this if required once I have a little more time.

[leebyron]

Thanks for your in-depth exploration! There are some minor issues with your fix surrounding nullable vs non-nullable error handling, but it's directionally sound. I'll investigate further.

[leebyron]

So here's the result of my further analysis:

The library hadn't supported this yet not as a bug but because we assumed if a resolve function would return a list of promises, that the resolve function itself would be responsible for calling Promise.all, the way we use GraphQL at Facebook has resulted in this particular way of describing field resolution being underserved.

As I was thinking about this further, I think we're also missing a case where a list without non-null item type [T] should handle a single item failure more gracefully.

So a list of promises:

resolve: () => [ fooPromise(), failingPromise(), barPromise() ]

should result in [ "foo", null, "bar" ] where I'm pretty sure this currently will bubble up the error and become null.

More investigation and test writing is necessary, but thanks again for highlighting!

[latentflip]

Thanks! I'm not sure I agree though, in this case my resolve functions are only ever returning individual promises, but it's the lib itself which is creating an array of promises when it maps over a list type, no?

Philip Roberts

On 4 Jul 2015, at 16:54, Lee Byron [email protected] wrote:

So here's the result of my further analysis:

The library hadn't supported this yet not as a bug but because we assumed if a resolve function would return a list of promises, that the resolve function itself would be responsible for calling Promise.all, the way we use GraphQL at Facebook has resulted in this particular way of describing field resolution being underserved.

As I was thinking about this further, I think we're also missing a case where a list without non-null item type [T] should handle a single item failure more gracefully.

So a list of promises:

resolve: () => [ fooPromise(), failingPromise(), barPromise() ]
should result in [ "foo", null, "bar" ] where I'm pretty sure this currently will bubble up the error and become null.

More investigation and test writing is necessary, but thanks again for highlighting!

—
Reply to this email directly or view it on GitHub.

[leebyron]

Ah yes, I think there are two subtle overlapping issues here. I see from your test case that there is another failure to resolve the promise before continuing. Thanks for providing them, I'll make sure they get encoded as runtime unit tests.

[latentflip]

Thanks @leebyron! Yeah, it was quite hard to produce a minimal testcase that demonstrated the behaviour, while still demonstrating the intent :)

[leebyron]

@latentflip please check out 0fd8be1 and let me know if the test cases I added are aligning with the issues you ran into.

[leebyron]

Version 0.1.2 is on npm that includes that commit

[latentflip]

@leebyron yup, just ran it on the test-cases + orm integration I've been working on and it's looking good. Thanks for the quick turnaround! 🎉

[leebyron]

Thank YOU for playing with it in such an early state and investigating. I may not have found this issue otherwise.