Broken promise behaviour when types used in list have async field resolve functions.

[latentflip]

Okay, I'm still digging into this, and trying to simplify my examples, but it looks like something funky is going on with field resolve behaviour when promises are returned.

Here's example code which demonstrates the behaviour (as well as the results) https://gist.github.com/latentflip/f1c9520ac0d83b5237fc note how in the results, projectPromises differs from projectResults.

Okay, to explain further, the basic setup I have is:

type Org {
  id: String
  projects: [Project]
}

type Project {
  id: String
  versions: ProjectVersion
}

type ProjectVersion {
  id: String
}

and my root query just returns one org, so the query I'm executing is:

query Foo {
  org {
    id,
    projects {
      versions
    }
  }
}

which should return the org, all it's projects, and all their versions.

However, it seems if my projects.versions resolve function returns a promise, instead of immediately returning an array, something breaks. To demonstrate, in the actual code linked above, I've got a ProjectReturnType which just returns an array for it's versions and a ProjectPromiseType which returns the same array, but wrapped in a Promise.resolve(). The latter seems to break the projectPromises in the response completely, and it just returns {}.

As I understand it, a resolve function returning a value, or a resolve function returning a promise of that same value, should be exactly equivalent, right?

[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 notifications@github.com 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.