From c75f74ad74623e97091f8ecd09c331b67cc0ba37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20H=C3=A9rault?= Date: Thu, 15 Dec 2016 12:38:10 +0100 Subject: [PATCH 01/13] add failing tests --- .../__tests__/abstract-promise-test.js | 433 ++++++++++++++++++ 1 file changed, 433 insertions(+) create mode 100644 src/execution/__tests__/abstract-promise-test.js diff --git a/src/execution/__tests__/abstract-promise-test.js b/src/execution/__tests__/abstract-promise-test.js new file mode 100644 index 0000000000..954bdd2991 --- /dev/null +++ b/src/execution/__tests__/abstract-promise-test.js @@ -0,0 +1,433 @@ +/** + * Copyright (c) 2015, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + */ + +import { expect } from 'chai'; +import { describe, it } from 'mocha'; +import { + graphql, + GraphQLSchema, + GraphQLObjectType, + GraphQLInterfaceType, + GraphQLUnionType, + GraphQLList, + GraphQLString, + GraphQLBoolean, +} from '../../'; + +class Dog { + constructor(name, woofs) { + this.name = name; + this.woofs = woofs; + } +} + +class Cat { + constructor(name, meows) { + this.name = name; + this.meows = meows; + } +} + +class Human { + constructor(name) { + this.name = name; + } +} + +describe('Execute: Handles execution of abstract types with promises', () => { + + it('isTypeOf used to resolve runtime type for Interface', async () => { + const PetType = new GraphQLInterfaceType({ + name: 'Pet', + fields: { + name: { type: GraphQLString } + } + }); + + const DogType = new GraphQLObjectType({ + name: 'Dog', + interfaces: [ PetType ], + isTypeOf: obj => Promise.resolve(obj instanceof Dog), + fields: { + name: { type: GraphQLString }, + woofs: { type: GraphQLBoolean }, + } + }); + + const CatType = new GraphQLObjectType({ + name: 'Cat', + interfaces: [ PetType ], + isTypeOf: obj => Promise.resolve(obj instanceof Cat), + fields: { + name: { type: GraphQLString }, + meows: { type: GraphQLBoolean }, + } + }); + + const schema = new GraphQLSchema({ + query: new GraphQLObjectType({ + name: 'Query', + fields: { + pets: { + type: new GraphQLList(PetType), + resolve() { + return [ new Dog('Odie', true), new Cat('Garfield', false) ]; + } + } + } + }), + types: [ CatType, DogType ] + }); + + const query = `{ + pets { + name + ... on Dog { + woofs + } + ... on Cat { + meows + } + } + }`; + + const result = await graphql(schema, query); + + expect(result).to.deep.equal({ + data: { + pets: [ + { name: 'Odie', + woofs: true }, + { name: 'Garfield', + meows: false } ] } + }); + }); + + it('isTypeOf used to resolve runtime type for Union', async () => { + const DogType = new GraphQLObjectType({ + name: 'Dog', + isTypeOf: obj => Promise.resolve(obj instanceof Dog), + fields: { + name: { type: GraphQLString }, + woofs: { type: GraphQLBoolean }, + } + }); + + const CatType = new GraphQLObjectType({ + name: 'Cat', + isTypeOf: obj => Promise.resolve(obj instanceof Cat), + fields: { + name: { type: GraphQLString }, + meows: { type: GraphQLBoolean }, + } + }); + + const PetType = new GraphQLUnionType({ + name: 'Pet', + types: [ DogType, CatType ] + }); + + const schema = new GraphQLSchema({ + query: new GraphQLObjectType({ + name: 'Query', + fields: { + pets: { + type: new GraphQLList(PetType), + resolve() { + return [ new Dog('Odie', true), new Cat('Garfield', false) ]; + } + } + } + }) + }); + + const query = `{ + pets { + ... on Dog { + name + woofs + } + ... on Cat { + name + meows + } + } + }`; + + const result = await graphql(schema, query); + + expect(result).to.deep.equal({ + data: { + pets: [ + { name: 'Odie', + woofs: true }, + { name: 'Garfield', + meows: false } ] } + }); + }); + + it('resolveType on Interface yields useful error', async () => { + const PetType = new GraphQLInterfaceType({ + name: 'Pet', + resolveType(obj) { + return Promise.resolve(obj instanceof Dog ? DogType : + obj instanceof Cat ? CatType : + obj instanceof Human ? HumanType : + null); + }, + fields: { + name: { type: GraphQLString } + } + }); + + const HumanType = new GraphQLObjectType({ + name: 'Human', + fields: { + name: { type: GraphQLString }, + } + }); + + const DogType = new GraphQLObjectType({ + name: 'Dog', + interfaces: [ PetType ], + fields: { + name: { type: GraphQLString }, + woofs: { type: GraphQLBoolean }, + } + }); + + const CatType = new GraphQLObjectType({ + name: 'Cat', + interfaces: [ PetType ], + fields: { + name: { type: GraphQLString }, + meows: { type: GraphQLBoolean }, + } + }); + + const schema = new GraphQLSchema({ + query: new GraphQLObjectType({ + name: 'Query', + fields: { + pets: { + type: new GraphQLList(PetType), + resolve() { + return Promise.resolve([ + new Dog('Odie', true), + new Cat('Garfield', false), + new Human('Jon') + ]); + } + } + } + }), + types: [ CatType, DogType ] + }); + + const query = `{ + pets { + name + ... on Dog { + woofs + } + ... on Cat { + meows + } + } + }`; + + const result = await graphql(schema, query); + + expect(result).to.jsonEqual({ + data: { + pets: [ + { name: 'Odie', + woofs: true }, + { name: 'Garfield', + meows: false }, + null + ] + }, + errors: [ + { + message: + 'Runtime Object type "Human" is not a possible type for "Pet".', + locations: [ { line: 2, column: 7 } ], + path: [ 'pets', 2 ] + } + ] + }); + }); + + it('resolveType on Union yields useful error', async () => { + const HumanType = new GraphQLObjectType({ + name: 'Human', + fields: { + name: { type: GraphQLString }, + } + }); + + const DogType = new GraphQLObjectType({ + name: 'Dog', + fields: { + name: { type: GraphQLString }, + woofs: { type: GraphQLBoolean }, + } + }); + + const CatType = new GraphQLObjectType({ + name: 'Cat', + fields: { + name: { type: GraphQLString }, + meows: { type: GraphQLBoolean }, + } + }); + + const PetType = new GraphQLUnionType({ + name: 'Pet', + resolveType(obj) { + return Promise.resolve(obj instanceof Dog ? DogType : + obj instanceof Cat ? CatType : + obj instanceof Human ? HumanType : + null); + }, + types: [ DogType, CatType ] + }); + + + const schema = new GraphQLSchema({ + query: new GraphQLObjectType({ + name: 'Query', + fields: { + pets: { + type: new GraphQLList(PetType), + resolve() { + return [ + new Dog('Odie', true), + new Cat('Garfield', false), + new Human('Jon') + ]; + } + } + } + }) + }); + + const query = `{ + pets { + ... on Dog { + name + woofs + } + ... on Cat { + name + meows + } + } + }`; + + const result = await graphql(schema, query); + + expect(result).to.jsonEqual({ + data: { + pets: [ + { name: 'Odie', + woofs: true }, + { name: 'Garfield', + meows: false }, + null + ] + }, + errors: [ + { + message: + 'Runtime Object type "Human" is not a possible type for "Pet".', + locations: [ { line: 2, column: 7 } ], + path: [ 'pets', 2 ] + } + ] + }); + }); + + it('resolveType allows resolving with type name', async () => { + const PetType = new GraphQLInterfaceType({ + name: 'Pet', + resolveType(obj) { + return Promise.resolve(obj instanceof Dog ? 'Dog' : + obj instanceof Cat ? 'Cat' : + null); + }, + fields: { + name: { type: GraphQLString } + } + }); + + const DogType = new GraphQLObjectType({ + name: 'Dog', + interfaces: [ PetType ], + fields: { + name: { type: GraphQLString }, + woofs: { type: GraphQLBoolean }, + } + }); + + const CatType = new GraphQLObjectType({ + name: 'Cat', + interfaces: [ PetType ], + fields: { + name: { type: GraphQLString }, + meows: { type: GraphQLBoolean }, + } + }); + + const schema = new GraphQLSchema({ + query: new GraphQLObjectType({ + name: 'Query', + fields: { + pets: { + type: new GraphQLList(PetType), + resolve() { + return [ + new Dog('Odie', true), + new Cat('Garfield', false) + ]; + } + } + } + }), + types: [ CatType, DogType ] + }); + + const query = `{ + pets { + name + ... on Dog { + woofs + } + ... on Cat { + meows + } + } + }`; + + const result = await graphql(schema, query); + + expect(result).to.jsonEqual({ + data: { + pets: [ + { name: 'Odie', + woofs: true }, + { name: 'Garfield', + meows: false }, + ] + } + }); + }); + +}); From 458383449648e9658b16041f1449c159696185a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20H=C3=A9rault?= Date: Thu, 15 Dec 2016 15:08:52 +0100 Subject: [PATCH 02/13] promisify isTypeOf and resolveType --- src/execution/execute.js | 151 +++++++++++++++++++++++---------------- 1 file changed, 91 insertions(+), 60 deletions(-) diff --git a/src/execution/execute.js b/src/execution/execute.js index f1e55fef69..6f5e70cf1d 100644 --- a/src/execution/execute.js +++ b/src/execution/execute.js @@ -940,40 +940,53 @@ function completeAbstractValue( path: ResponsePath, result: mixed ): mixed { - let runtimeType = returnType.resolveType ? - returnType.resolveType(result, exeContext.contextValue, info) : - defaultResolveTypeFn(result, exeContext.contextValue, info, returnType); + const runtimeTypePromise = returnType.resolveType ? + Promise.resolve(returnType.resolveType( + result, + exeContext.contextValue, + info + )) : + resolveFirstValidType( + result, + exeContext.contextValue, + info, + info.schema.getPossibleTypes(returnType) + ); - // If resolveType returns a string, we assume it's a GraphQLObjectType name. - if (typeof runtimeType === 'string') { - runtimeType = exeContext.schema.getType(runtimeType); - } + return runtimeTypePromise.then(type => { + let runtimeType = type; - if (!(runtimeType instanceof GraphQLObjectType)) { - throw new GraphQLError( - `Abstract type ${returnType.name} must resolve to an Object type at ` + - `runtime for field ${info.parentType.name}.${info.fieldName} with ` + - `value "${String(result)}", received "${String(runtimeType)}".`, - fieldNodes - ); - } + // If resolveType returns a string, we assume it's a GraphQLObjectType name. + if (typeof runtimeType === 'string') { + runtimeType = exeContext.schema.getType(runtimeType); + } - if (!exeContext.schema.isPossibleType(returnType, runtimeType)) { - throw new GraphQLError( - `Runtime Object type "${runtimeType.name}" is not a possible type ` + - `for "${returnType.name}".`, - fieldNodes - ); - } + if (!(runtimeType instanceof GraphQLObjectType)) { + throw new GraphQLError( + `Abstract type ${returnType.name} must resolve to an Object type at ` + + `runtime for field ${info.parentType.name}.${info.fieldName} with ` + + `value "${String(result)}", received "${String(runtimeType)}".`, + fieldNodes + ); + } - return completeObjectValue( - exeContext, - runtimeType, - fieldNodes, - info, - path, - result - ); + if (!exeContext.schema.isPossibleType(returnType, runtimeType)) { + throw new GraphQLError( + `Runtime Object type "${runtimeType.name}" is not a possible type ` + + `for "${returnType.name}".`, + fieldNodes + ); + } + + return completeObjectValue( + exeContext, + runtimeType, + fieldNodes, + info, + path, + result + ); + }); } /** @@ -990,31 +1003,37 @@ function completeObjectValue( // If there is an isTypeOf predicate function, call it with the // current result. If isTypeOf returns false, then raise an error rather // than continuing execution. - if (returnType.isTypeOf && - !returnType.isTypeOf(result, exeContext.contextValue, info)) { - throw new GraphQLError( - `Expected value of type "${returnType.name}" but got: ${String(result)}.`, - fieldNodes - ); - } - - // Collect sub-fields to execute to complete this value. - let subFieldNodes = Object.create(null); - const visitedFragmentNames = Object.create(null); - for (let i = 0; i < fieldNodes.length; i++) { - const selectionSet = fieldNodes[i].selectionSet; - if (selectionSet) { - subFieldNodes = collectFields( - exeContext, - returnType, - selectionSet, - subFieldNodes, - visitedFragmentNames + return Promise.resolve(returnType.isTypeOf ? + returnType.isTypeOf(result, exeContext.contextValue, info) : + true + ) + .then(isTypeOfResult => { + if (!isTypeOfResult) { + throw new GraphQLError( + `Expected value of type "${returnType.name}" ` + + `but got: ${String(result)}.`, + fieldNodes ); } - } - return executeFields(exeContext, returnType, result, path, subFieldNodes); + // Collect sub-fields to execute to complete this value. + let subFieldNodes = Object.create(null); + const visitedFragmentNames = Object.create(null); + for (let i = 0; i < fieldNodes.length; i++) { + const selectionSet = fieldNodes[i].selectionSet; + if (selectionSet) { + subFieldNodes = collectFields( + exeContext, + returnType, + selectionSet, + subFieldNodes, + visitedFragmentNames + ); + } + } + + return executeFields(exeContext, returnType, result, path, subFieldNodes); + }); } /** @@ -1022,19 +1041,31 @@ function completeObjectValue( * used which tests each possible type for the abstract type by calling * isTypeOf for the object being coerced, returning the first type that matches. */ -function defaultResolveTypeFn( +function resolveFirstValidType( value: mixed, context: mixed, info: GraphQLResolveInfo, - abstractType: GraphQLAbstractType -): ?GraphQLObjectType { - const possibleTypes = info.schema.getPossibleTypes(abstractType); - for (let i = 0; i < possibleTypes.length; i++) { - const type = possibleTypes[i]; - if (type.isTypeOf && type.isTypeOf(value, context, info)) { + possibleTypes: Array, + i: number = 0, +): Promise { + if (i >= possibleTypes.length) { + return Promise.resolve(null); + } + + const type = possibleTypes[i]; + + if (!type.isTypeOf) { + return resolveFirstValidType(value, context, info, possibleTypes, i + 1); + } + + return Promise.resolve(type.isTypeOf(value, context, info)) + .then(result => { + if (result) { return type; } - } + + return resolveFirstValidType(value, context, info, possibleTypes, i + 1); + }); } /** From 036e4676f76c306c1d0098a8a3f5a4dbdd330c7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20H=C3=A9rault?= Date: Thu, 15 Dec 2016 15:39:44 +0100 Subject: [PATCH 03/13] add more tests --- .../__tests__/abstract-promise-test.js | 151 ++++++++++++++++++ 1 file changed, 151 insertions(+) diff --git a/src/execution/__tests__/abstract-promise-test.js b/src/execution/__tests__/abstract-promise-test.js index 954bdd2991..e1cee7c9a0 100644 --- a/src/execution/__tests__/abstract-promise-test.js +++ b/src/execution/__tests__/abstract-promise-test.js @@ -109,6 +109,79 @@ describe('Execute: Handles execution of abstract types with promises', () => { }); }); + it('isTypeOf can be rejected', async () => { + const PetType = new GraphQLInterfaceType({ + name: 'Pet', + fields: { + name: { type: GraphQLString } + } + }); + + const DogType = new GraphQLObjectType({ + name: 'Dog', + interfaces: [ PetType ], + isTypeOf: () => Promise.reject(new Error('We are testing this error')), + fields: { + name: { type: GraphQLString }, + woofs: { type: GraphQLBoolean }, + } + }); + + const CatType = new GraphQLObjectType({ + name: 'Cat', + interfaces: [ PetType ], + isTypeOf: obj => Promise.resolve(obj instanceof Cat), + fields: { + name: { type: GraphQLString }, + meows: { type: GraphQLBoolean }, + } + }); + + const schema = new GraphQLSchema({ + query: new GraphQLObjectType({ + name: 'Query', + fields: { + pets: { + type: new GraphQLList(PetType), + resolve() { + return [ new Dog('Odie', true), new Cat('Garfield', false) ]; + } + } + } + }), + types: [ CatType, DogType ] + }); + + const query = `{ + pets { + name + ... on Dog { + woofs + } + ... on Cat { + meows + } + } + }`; + + const result = await graphql(schema, query); + + expect(result).to.deep.equal({ + data: { + pets: [ + null, + { name: 'Garfield', + meows: false } ] }, + errors: [ + { + message: 'We are testing this error', + locations: [ { line: 2, column: 7 } ], + path: [ 'pets', 0 ] + } + ] + }); + }); + it('isTypeOf used to resolve runtime type for Union', async () => { const DogType = new GraphQLObjectType({ name: 'Dog', @@ -430,4 +503,82 @@ describe('Execute: Handles execution of abstract types with promises', () => { }); }); + it('resolveType can be caught', async () => { + const PetType = new GraphQLInterfaceType({ + name: 'Pet', + resolveType: () => Promise.reject('We are testing this error'), + fields: { + name: { type: GraphQLString } + } + }); + + const DogType = new GraphQLObjectType({ + name: 'Dog', + interfaces: [ PetType ], + fields: { + name: { type: GraphQLString }, + woofs: { type: GraphQLBoolean }, + } + }); + + const CatType = new GraphQLObjectType({ + name: 'Cat', + interfaces: [ PetType ], + fields: { + name: { type: GraphQLString }, + meows: { type: GraphQLBoolean }, + } + }); + + const schema = new GraphQLSchema({ + query: new GraphQLObjectType({ + name: 'Query', + fields: { + pets: { + type: new GraphQLList(PetType), + resolve() { + return [ + new Dog('Odie', true), + new Cat('Garfield', false) + ]; + } + } + } + }), + types: [ CatType, DogType ] + }); + + const query = `{ + pets { + name + ... on Dog { + woofs + } + ... on Cat { + meows + } + } + }`; + + const result = await graphql(schema, query); + + expect(result).to.jsonEqual({ + data: { + pets: [ null, null ] + }, + errors: [ + { + message: 'We are testing this error', + locations: [ { line: 2, column: 7 } ], + path: [ 'pets', 0 ] + }, + { + message: 'We are testing this error', + locations: [ { line: 2, column: 7 } ], + path: [ 'pets', 1 ] + } + ] + }); + }); + }); From 164cd3146296ae4bb514502fbb05583b802c0451 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20H=C3=A9rault?= Date: Fri, 23 Dec 2016 10:33:16 +0100 Subject: [PATCH 04/13] edit flow types --- src/type/definition.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/type/definition.js b/src/type/definition.js index 2cf5a61d86..be79a8d1e4 100644 --- a/src/type/definition.js +++ b/src/type/definition.js @@ -553,13 +553,13 @@ export type GraphQLTypeResolver = ( value: TSource, context: TContext, info: GraphQLResolveInfo -) => ?GraphQLObjectType | ?string; +) => ?GraphQLObjectType | ?string | ?Promise; export type GraphQLIsTypeOfFn = ( source: TSource, context: TContext, info: GraphQLResolveInfo -) => boolean; +) => boolean | Promise; export type GraphQLFieldResolver = ( source: TSource, From 7d4cd4b8d2808eaacae24d764e8970ec45d81be0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20H=C3=A9rault?= Date: Fri, 23 Dec 2016 10:37:47 +0100 Subject: [PATCH 05/13] allow fully sync abstract type resolution --- src/execution/execute.js | 169 ++++++++++++++++++++++++--------------- 1 file changed, 105 insertions(+), 64 deletions(-) diff --git a/src/execution/execute.js b/src/execution/execute.js index 6f5e70cf1d..ecc2bc1bec 100644 --- a/src/execution/execute.js +++ b/src/execution/execute.js @@ -940,12 +940,12 @@ function completeAbstractValue( path: ResponsePath, result: mixed ): mixed { - const runtimeTypePromise = returnType.resolveType ? - Promise.resolve(returnType.resolveType( + const runtimeType = returnType.resolveType ? + returnType.resolveType( result, exeContext.contextValue, info - )) : + ) : resolveFirstValidType( result, exeContext.contextValue, @@ -953,40 +953,113 @@ function completeAbstractValue( info.schema.getPossibleTypes(returnType) ); - return runtimeTypePromise.then(type => { - let runtimeType = type; + if (isThenable(runtimeType)) { + return ((runtimeType: any): Promise<*>).then(resolvedRuntimeType => ( + validateRuntimeTypeAndCompleteObjectValue( + exeContext, + returnType, + fieldNodes, + info, + path, + resolvedRuntimeType, + result + ) + )); + } - // If resolveType returns a string, we assume it's a GraphQLObjectType name. - if (typeof runtimeType === 'string') { - runtimeType = exeContext.schema.getType(runtimeType); - } + return validateRuntimeTypeAndCompleteObjectValue( + exeContext, + returnType, + fieldNodes, + info, + path, + runtimeType, + result + ); +} - if (!(runtimeType instanceof GraphQLObjectType)) { - throw new GraphQLError( - `Abstract type ${returnType.name} must resolve to an Object type at ` + - `runtime for field ${info.parentType.name}.${info.fieldName} with ` + - `value "${String(result)}", received "${String(runtimeType)}".`, - fieldNodes - ); - } +function validateRuntimeTypeAndCompleteObjectValue( + exeContext: ExecutionContext, + returnType: GraphQLAbstractType, + fieldNodes: Array, + info: GraphQLResolveInfo, + path: ResponsePath, + returnedRuntimeType: mixed, + result: mixed +): mixed { + let runtimeType = returnedRuntimeType; - if (!exeContext.schema.isPossibleType(returnType, runtimeType)) { - throw new GraphQLError( - `Runtime Object type "${runtimeType.name}" is not a possible type ` + - `for "${returnType.name}".`, - fieldNodes - ); - } + // If resolveType returns a string, we assume it's a GraphQLObjectType name. + if (typeof runtimeType === 'string') { + runtimeType = exeContext.schema.getType(runtimeType); + } - return completeObjectValue( - exeContext, - runtimeType, - fieldNodes, - info, - path, - result + if (!(runtimeType instanceof GraphQLObjectType)) { + throw new GraphQLError( + `Abstract type ${returnType.name} must resolve to an Object type at ` + + `runtime for field ${info.parentType.name}.${info.fieldName} with ` + + `value "${String(result)}", received "${String(runtimeType)}".`, + fieldNodes ); - }); + } + + if (!exeContext.schema.isPossibleType(returnType, runtimeType)) { + throw new GraphQLError( + `Runtime Object type "${runtimeType.name}" is not a possible type ` + + `for "${returnType.name}".`, + fieldNodes + ); + } + + return completeObjectValue( + exeContext, + runtimeType, + fieldNodes, + info, + path, + result + ); +} + +/** + * If a resolveType function is not given, then a default resolve behavior is + * used which tests each possible type for the abstract type by calling + * isTypeOf for the object being coerced, returning the first type that matches. + */ +function resolveFirstValidType( + value: mixed, + context: mixed, + info: GraphQLResolveInfo, + possibleTypes: Array, + i: number = 0, +): ?GraphQLObjectType | ?string | ?Promise { + if (i >= possibleTypes.length) { + return null; + } + + const type = possibleTypes[i]; + + if (!type.isTypeOf) { + return resolveFirstValidType(value, context, info, possibleTypes, i + 1); + } + + const isCorrectType = type.isTypeOf(value, context, info); + + if (isThenable(isCorrectType)) { + return ((isCorrectType: any): Promise<*>).then(result => { + if (result) { + return type; + } + + return resolveFirstValidType(value, context, info, possibleTypes, i + 1); + }); + } + + if (isCorrectType) { + return type; + } + + return resolveFirstValidType(value, context, info, possibleTypes, i + 1); } /** @@ -1036,38 +1109,6 @@ function completeObjectValue( }); } -/** - * If a resolveType function is not given, then a default resolve behavior is - * used which tests each possible type for the abstract type by calling - * isTypeOf for the object being coerced, returning the first type that matches. - */ -function resolveFirstValidType( - value: mixed, - context: mixed, - info: GraphQLResolveInfo, - possibleTypes: Array, - i: number = 0, -): Promise { - if (i >= possibleTypes.length) { - return Promise.resolve(null); - } - - const type = possibleTypes[i]; - - if (!type.isTypeOf) { - return resolveFirstValidType(value, context, info, possibleTypes, i + 1); - } - - return Promise.resolve(type.isTypeOf(value, context, info)) - .then(result => { - if (result) { - return type; - } - - return resolveFirstValidType(value, context, info, possibleTypes, i + 1); - }); -} - /** * If a resolve function is not given, then a default resolve behavior is used * which takes the property of the source object of the same name as the field From ae5295a2d3b3b19aa3e45129ac1a18b163ca9aea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20H=C3=A9rault?= Date: Thu, 5 Jan 2017 11:41:02 +0100 Subject: [PATCH 06/13] apply PR requested changes --- .../__tests__/abstract-promise-test.js | 11 +- src/execution/execute.js | 175 +++++++++++------- 2 files changed, 116 insertions(+), 70 deletions(-) diff --git a/src/execution/__tests__/abstract-promise-test.js b/src/execution/__tests__/abstract-promise-test.js index e1cee7c9a0..3edc15dbd4 100644 --- a/src/execution/__tests__/abstract-promise-test.js +++ b/src/execution/__tests__/abstract-promise-test.js @@ -168,15 +168,18 @@ describe('Execute: Handles execution of abstract types with promises', () => { expect(result).to.deep.equal({ data: { - pets: [ - null, - { name: 'Garfield', - meows: false } ] }, + pets: [ null, null ] + }, errors: [ { message: 'We are testing this error', locations: [ { line: 2, column: 7 } ], path: [ 'pets', 0 ] + }, + { + message: 'We are testing this error', + locations: [ { line: 2, column: 7 } ], + path: [ 'pets', 1 ] } ] }); diff --git a/src/execution/execute.js b/src/execution/execute.js index ecc2bc1bec..7b8ab29bb9 100644 --- a/src/execution/execute.js +++ b/src/execution/execute.js @@ -946,11 +946,11 @@ function completeAbstractValue( exeContext.contextValue, info ) : - resolveFirstValidType( + defaultResolveTypeFn( result, exeContext.contextValue, info, - info.schema.getPossibleTypes(returnType) + returnType ); if (isThenable(runtimeType)) { @@ -1022,91 +1022,134 @@ function validateRuntimeTypeAndCompleteObjectValue( } /** - * If a resolveType function is not given, then a default resolve behavior is - * used which tests each possible type for the abstract type by calling - * isTypeOf for the object being coerced, returning the first type that matches. + * Complete an Object value by executing all sub-selections. */ -function resolveFirstValidType( - value: mixed, - context: mixed, +function completeObjectValue( + exeContext: ExecutionContext, + returnType: GraphQLObjectType, + fieldNodes: Array, info: GraphQLResolveInfo, - possibleTypes: Array, - i: number = 0, -): ?GraphQLObjectType | ?string | ?Promise { - if (i >= possibleTypes.length) { - return null; - } - - const type = possibleTypes[i]; - - if (!type.isTypeOf) { - return resolveFirstValidType(value, context, info, possibleTypes, i + 1); + path: ResponsePath, + result: mixed +): mixed { + // If there is an isTypeOf predicate function, + // call it with the current result. + // Otherwise assume the type is correct + if (!returnType.isTypeOf) { + return validateResultTypeAndExecuteFields( + exeContext, + returnType, + fieldNodes, + info, + path, + result, + true + ); } - const isCorrectType = type.isTypeOf(value, context, info); - - if (isThenable(isCorrectType)) { - return ((isCorrectType: any): Promise<*>).then(result => { - if (result) { - return type; - } - - return resolveFirstValidType(value, context, info, possibleTypes, i + 1); - }); - } + const isTypeOf = returnType.isTypeOf(result, exeContext.contextValue, info); - if (isCorrectType) { - return type; + if (isThenable(isTypeOf)) { + return ((isTypeOf: any): Promise).then(isTypeOfResult => ( + validateResultTypeAndExecuteFields( + exeContext, + returnType, + fieldNodes, + info, + path, + result, + isTypeOfResult + ) + )); } - return resolveFirstValidType(value, context, info, possibleTypes, i + 1); + return validateResultTypeAndExecuteFields( + exeContext, + returnType, + fieldNodes, + info, + path, + result, + ((isTypeOf: any): boolean) + ); } -/** - * Complete an Object value by executing all sub-selections. - */ -function completeObjectValue( +function validateResultTypeAndExecuteFields( exeContext: ExecutionContext, returnType: GraphQLObjectType, fieldNodes: Array, info: GraphQLResolveInfo, path: ResponsePath, - result: mixed + result: mixed, + isTypeOfResult: boolean ): mixed { - // If there is an isTypeOf predicate function, call it with the - // current result. If isTypeOf returns false, then raise an error rather - // than continuing execution. - return Promise.resolve(returnType.isTypeOf ? - returnType.isTypeOf(result, exeContext.contextValue, info) : - true - ) - .then(isTypeOfResult => { - if (!isTypeOfResult) { - throw new GraphQLError( - `Expected value of type "${returnType.name}" ` + - `but got: ${String(result)}.`, - fieldNodes + // If isTypeOf returns false, then raise an error + // rather than continuing execution. + if (!isTypeOfResult) { + throw new GraphQLError( + `Expected value of type "${returnType.name}" ` + + `but got: ${String(result)}.`, + fieldNodes + ); + } + + // Collect sub-fields to execute to complete this value. + let subFieldNodes = Object.create(null); + const visitedFragmentNames = Object.create(null); + + for (let i = 0; i < fieldNodes.length; i++) { + const selectionSet = fieldNodes[i].selectionSet; + if (selectionSet) { + subFieldNodes = collectFields( + exeContext, + returnType, + selectionSet, + subFieldNodes, + visitedFragmentNames ); } + } - // Collect sub-fields to execute to complete this value. - let subFieldNodes = Object.create(null); - const visitedFragmentNames = Object.create(null); - for (let i = 0; i < fieldNodes.length; i++) { - const selectionSet = fieldNodes[i].selectionSet; - if (selectionSet) { - subFieldNodes = collectFields( - exeContext, - returnType, - selectionSet, - subFieldNodes, - visitedFragmentNames - ); + return executeFields(exeContext, returnType, result, path, subFieldNodes); +} + +/** + * If a resolveType function is not given, then a default resolve behavior is + * used which tests each possible type for the abstract type by calling + * isTypeOf for the object being coerced, returning the first type that matches. + */ +function defaultResolveTypeFn( + value: mixed, + context: mixed, + info: GraphQLResolveInfo, + abstractType: GraphQLAbstractType, +): ?GraphQLObjectType | ?Promise { + const possibleTypes = info.schema.getPossibleTypes(abstractType); + const promisedIsTypeOfResults = []; + const promisedIsTypeOfResultTypes = []; + + for (let i = 0; i < possibleTypes.length; i++) { + const type = possibleTypes[i]; + + if (type.isTypeOf) { + const isTypeOfResult = type.isTypeOf(value, context, info); + + if (isThenable(isTypeOfResult)) { + promisedIsTypeOfResults.push(isTypeOfResult); + promisedIsTypeOfResultTypes.push(type); + } else if (isTypeOfResult) { + return type; } } + } - return executeFields(exeContext, returnType, result, path, subFieldNodes); - }); + if (promisedIsTypeOfResults.length) { + return Promise.all(promisedIsTypeOfResults).then(isTypeOfResults => ( + promisedIsTypeOfResultTypes[ + isTypeOfResults.findIndex(isTypeOf => isTypeOf) + ] + )); + } } /** From 1305360bb13d7d5dea73f5f8a31ecd6516c8c10c Mon Sep 17 00:00:00 2001 From: Lee Byron Date: Fri, 20 Jan 2017 14:08:14 -0800 Subject: [PATCH 07/13] Reduce scope Just reverts a few unrelated parts of the change --- src/execution/execute.js | 48 ++++++++++++++++++---------------------- 1 file changed, 21 insertions(+), 27 deletions(-) diff --git a/src/execution/execute.js b/src/execution/execute.js index 7b8ab29bb9..815289eb00 100644 --- a/src/execution/execute.js +++ b/src/execution/execute.js @@ -941,30 +941,24 @@ function completeAbstractValue( result: mixed ): mixed { const runtimeType = returnType.resolveType ? - returnType.resolveType( - result, - exeContext.contextValue, - info - ) : - defaultResolveTypeFn( - result, - exeContext.contextValue, - info, - returnType - ); + returnType.resolveType(result, exeContext.contextValue, info) : + defaultResolveTypeFn(result, exeContext.contextValue, info, returnType); if (isThenable(runtimeType)) { - return ((runtimeType: any): Promise<*>).then(resolvedRuntimeType => ( + // Cast to Promise + const runtimeTypePromise: Promise = + (runtimeType: any); + return runtimeTypePromise.then(resolvedRuntimeType => validateRuntimeTypeAndCompleteObjectValue( exeContext, returnType, fieldNodes, info, path, - resolvedRuntimeType, + ensureType(exeContext.schema, resolvedRuntimeType), result ) - )); + ); } return validateRuntimeTypeAndCompleteObjectValue( @@ -973,27 +967,29 @@ function completeAbstractValue( fieldNodes, info, path, - runtimeType, + ensureType(exeContext.schema, runtimeType), result ); } +function ensureType( + schema: GraphQLSchema, + typeOrName: string | GraphQLObjectType +): GraphQLObjectType { + return typeof typeOrName === 'string' ? + schema.getType(typeOrName) : + typeOrName; +} + function validateRuntimeTypeAndCompleteObjectValue( exeContext: ExecutionContext, returnType: GraphQLAbstractType, fieldNodes: Array, info: GraphQLResolveInfo, path: ResponsePath, - returnedRuntimeType: mixed, + runtimeType: GraphQLObjectType, result: mixed ): mixed { - let runtimeType = returnedRuntimeType; - - // If resolveType returns a string, we assume it's a GraphQLObjectType name. - if (typeof runtimeType === 'string') { - runtimeType = exeContext.schema.getType(runtimeType); - } - if (!(runtimeType instanceof GraphQLObjectType)) { throw new GraphQLError( `Abstract type ${returnType.name} must resolve to an Object type at ` + @@ -1087,8 +1083,7 @@ function validateResultTypeAndExecuteFields( // rather than continuing execution. if (!isTypeOfResult) { throw new GraphQLError( - `Expected value of type "${returnType.name}" ` + - `but got: ${String(result)}.`, + `Expected value of type "${returnType.name}" but got: ${String(result)}.`, fieldNodes ); } @@ -1096,7 +1091,6 @@ function validateResultTypeAndExecuteFields( // Collect sub-fields to execute to complete this value. let subFieldNodes = Object.create(null); const visitedFragmentNames = Object.create(null); - for (let i = 0; i < fieldNodes.length; i++) { const selectionSet = fieldNodes[i].selectionSet; if (selectionSet) { @@ -1122,7 +1116,7 @@ function defaultResolveTypeFn( value: mixed, context: mixed, info: GraphQLResolveInfo, - abstractType: GraphQLAbstractType, + abstractType: GraphQLAbstractType ): ?GraphQLObjectType | ?Promise { const possibleTypes = info.schema.getPossibleTypes(abstractType); const promisedIsTypeOfResults = []; From 1fb45d3e2e2be9a9cd41f78293e6adf5213f407d Mon Sep 17 00:00:00 2001 From: Lee Byron Date: Fri, 20 Jan 2017 14:21:53 -0800 Subject: [PATCH 08/13] Factor out runtime type validation Moves runtime type validation to its own function. --- src/execution/execute.js | 51 ++++++++++++++++++++-------------------- 1 file changed, 26 insertions(+), 25 deletions(-) diff --git a/src/execution/execute.js b/src/execution/execute.js index 815289eb00..162e2209ae 100644 --- a/src/execution/execute.js +++ b/src/execution/execute.js @@ -949,47 +949,55 @@ function completeAbstractValue( const runtimeTypePromise: Promise = (runtimeType: any); return runtimeTypePromise.then(resolvedRuntimeType => - validateRuntimeTypeAndCompleteObjectValue( + completeObjectValue( exeContext, returnType, fieldNodes, info, path, - ensureType(exeContext.schema, resolvedRuntimeType), + ensureValidRuntimeType( + resolvedRuntimeType, + exeContext, + returnType, + fieldNodes, + info, + result + ), result ) ); } - return validateRuntimeTypeAndCompleteObjectValue( + return completeObjectValue( exeContext, returnType, fieldNodes, info, path, - ensureType(exeContext.schema, runtimeType), + ensureValidRuntimeType( + resolvedRuntimeType, + exeContext, + returnType, + fieldNodes, + info, + result + ), result ); } -function ensureType( - schema: GraphQLSchema, - typeOrName: string | GraphQLObjectType -): GraphQLObjectType { - return typeof typeOrName === 'string' ? - schema.getType(typeOrName) : - typeOrName; -} - -function validateRuntimeTypeAndCompleteObjectValue( +function ensureValidRuntimeType( + runtimeTypeOrName: string | GraphQLObjectType | void exeContext: ExecutionContext, returnType: GraphQLAbstractType, fieldNodes: Array, info: GraphQLResolveInfo, - path: ResponsePath, - runtimeType: GraphQLObjectType, result: mixed -): mixed { +): GraphQLObjectType { + const runtimeType = typeof runtimeTypeOrName === 'string' ? + exeContext.schema.getType(runtimeTypeOrName) : + runtimeTypeOrName; + if (!(runtimeType instanceof GraphQLObjectType)) { throw new GraphQLError( `Abstract type ${returnType.name} must resolve to an Object type at ` + @@ -1007,14 +1015,7 @@ function validateRuntimeTypeAndCompleteObjectValue( ); } - return completeObjectValue( - exeContext, - runtimeType, - fieldNodes, - info, - path, - result - ); + return runtimeType; } /** From b40d427864a848a6d95cf9dc89885501f56c126a Mon Sep 17 00:00:00 2001 From: Lee Byron Date: Fri, 20 Jan 2017 14:23:31 -0800 Subject: [PATCH 09/13] Fix completeObjectValue call bug --- src/execution/execute.js | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/execution/execute.js b/src/execution/execute.js index 162e2209ae..5cae963d5e 100644 --- a/src/execution/execute.js +++ b/src/execution/execute.js @@ -951,10 +951,6 @@ function completeAbstractValue( return runtimeTypePromise.then(resolvedRuntimeType => completeObjectValue( exeContext, - returnType, - fieldNodes, - info, - path, ensureValidRuntimeType( resolvedRuntimeType, exeContext, @@ -963,6 +959,9 @@ function completeAbstractValue( info, result ), + fieldNodes, + info, + path, result ) ); @@ -970,10 +969,6 @@ function completeAbstractValue( return completeObjectValue( exeContext, - returnType, - fieldNodes, - info, - path, ensureValidRuntimeType( resolvedRuntimeType, exeContext, @@ -982,6 +977,9 @@ function completeAbstractValue( info, result ), + fieldNodes, + info, + path, result ); } From c53efbe83c6e3a922c423e7dc5c43f94e84f38f0 Mon Sep 17 00:00:00 2001 From: Lee Byron Date: Fri, 20 Jan 2017 14:28:58 -0800 Subject: [PATCH 10/13] Missing comma --- src/execution/execute.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/execution/execute.js b/src/execution/execute.js index 5cae963d5e..ad9496ab3d 100644 --- a/src/execution/execute.js +++ b/src/execution/execute.js @@ -985,7 +985,7 @@ function completeAbstractValue( } function ensureValidRuntimeType( - runtimeTypeOrName: string | GraphQLObjectType | void + runtimeTypeOrName: string | GraphQLObjectType | void, exeContext: ExecutionContext, returnType: GraphQLAbstractType, fieldNodes: Array, From 9239253b2e4968b729822ddc854be728e7a80048 Mon Sep 17 00:00:00 2001 From: Lee Byron Date: Fri, 20 Jan 2017 15:09:11 -0800 Subject: [PATCH 11/13] Pass lint & flow --- src/execution/execute.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/execution/execute.js b/src/execution/execute.js index ad9496ab3d..6551f1631c 100644 --- a/src/execution/execute.js +++ b/src/execution/execute.js @@ -946,15 +946,15 @@ function completeAbstractValue( if (isThenable(runtimeType)) { // Cast to Promise - const runtimeTypePromise: Promise = + const runtimeTypePromise: Promise = (runtimeType: any); return runtimeTypePromise.then(resolvedRuntimeType => completeObjectValue( exeContext, ensureValidRuntimeType( resolvedRuntimeType, - exeContext, - returnType, + exeContext, + returnType, fieldNodes, info, result @@ -970,9 +970,9 @@ function completeAbstractValue( return completeObjectValue( exeContext, ensureValidRuntimeType( - resolvedRuntimeType, + ((runtimeType: any): ?GraphQLObjectType | string), exeContext, - returnType, + returnType, fieldNodes, info, result @@ -985,15 +985,15 @@ function completeAbstractValue( } function ensureValidRuntimeType( - runtimeTypeOrName: string | GraphQLObjectType | void, + runtimeTypeOrName: ?GraphQLObjectType | string, exeContext: ExecutionContext, returnType: GraphQLAbstractType, fieldNodes: Array, info: GraphQLResolveInfo, result: mixed ): GraphQLObjectType { - const runtimeType = typeof runtimeTypeOrName === 'string' ? - exeContext.schema.getType(runtimeTypeOrName) : + const runtimeType = typeof runtimeTypeOrName === 'string' ? + exeContext.schema.getType(runtimeTypeOrName) : runtimeTypeOrName; if (!(runtimeType instanceof GraphQLObjectType)) { From a3c746c511c88ca8a36d8fba313226dea2c3d5e9 Mon Sep 17 00:00:00 2001 From: Lee Byron Date: Tue, 24 Jan 2017 11:45:54 -0800 Subject: [PATCH 12/13] Factor out checking isTypeOf from subfield exe More single-purpose functions --- src/execution/execute.js | 82 +++++++++++++++++++--------------------- 1 file changed, 39 insertions(+), 43 deletions(-) diff --git a/src/execution/execute.js b/src/execution/execute.js index 6551f1631c..49a21d42f9 100644 --- a/src/execution/execute.js +++ b/src/execution/execute.js @@ -1027,66 +1027,62 @@ function completeObjectValue( path: ResponsePath, result: mixed ): mixed { - // If there is an isTypeOf predicate function, - // call it with the current result. - // Otherwise assume the type is correct - if (!returnType.isTypeOf) { - return validateResultTypeAndExecuteFields( - exeContext, - returnType, - fieldNodes, - info, - path, - result, - true - ); - } - - const isTypeOf = returnType.isTypeOf(result, exeContext.contextValue, info); + // If there is an isTypeOf predicate function, call it with the + // current result. If isTypeOf returns false, then raise an error rather + // than continuing execution. + if (returnType.isTypeOf) { + const isTypeOf = returnType.isTypeOf(result, exeContext.contextValue, info); + + if (isThenable(isTypeOf)) { + return ((isTypeOf: any): Promise).then(isTypeOfResult => { + if (!isTypeOfResult) { + throw invalidReturnTypeError(returnType, result, fieldNodes); + } + return collectAndExecuteSubfields( + exeContext, + returnType, + fieldNodes, + info, + path, + result + ); + }); + } - if (isThenable(isTypeOf)) { - return ((isTypeOf: any): Promise).then(isTypeOfResult => ( - validateResultTypeAndExecuteFields( - exeContext, - returnType, - fieldNodes, - info, - path, - result, - isTypeOfResult - ) - )); + if (!isTypeOf) { + throw invalidReturnTypeError(returnType, result, fieldNodes); + } } - return validateResultTypeAndExecuteFields( + return collectAndExecuteSubfields( exeContext, returnType, fieldNodes, info, path, - result, - ((isTypeOf: any): boolean) + result ); } -function validateResultTypeAndExecuteFields( +function invalidReturnTypeError( + returnType: GraphQLObjectType, + result: mixed, + fieldNodes: Array +): GraphQLError { + return new GraphQLError( + `Expected value of type "${returnType.name}" but got: ${String(result)}.`, + fieldNodes + ); +} + +function collectAndExecuteSubfields( exeContext: ExecutionContext, returnType: GraphQLObjectType, fieldNodes: Array, info: GraphQLResolveInfo, path: ResponsePath, - result: mixed, - isTypeOfResult: boolean + result: mixed ): mixed { - // If isTypeOf returns false, then raise an error - // rather than continuing execution. - if (!isTypeOfResult) { - throw new GraphQLError( - `Expected value of type "${returnType.name}" but got: ${String(result)}.`, - fieldNodes - ); - } - // Collect sub-fields to execute to complete this value. let subFieldNodes = Object.create(null); const visitedFragmentNames = Object.create(null); From 0417aad2129c4c31ceb9eafe00a783fdc62fb654 Mon Sep 17 00:00:00 2001 From: Lee Byron Date: Tue, 24 Jan 2017 12:02:09 -0800 Subject: [PATCH 13/13] Simplify default resolver function --- src/execution/execute.js | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/execution/execute.js b/src/execution/execute.js index 49a21d42f9..0ae1bdb979 100644 --- a/src/execution/execute.js +++ b/src/execution/execute.js @@ -1112,10 +1112,9 @@ function defaultResolveTypeFn( context: mixed, info: GraphQLResolveInfo, abstractType: GraphQLAbstractType -): ?GraphQLObjectType | ?Promise { +): ?GraphQLObjectType | Promise { const possibleTypes = info.schema.getPossibleTypes(abstractType); const promisedIsTypeOfResults = []; - const promisedIsTypeOfResultTypes = []; for (let i = 0; i < possibleTypes.length; i++) { const type = possibleTypes[i]; @@ -1124,8 +1123,7 @@ function defaultResolveTypeFn( const isTypeOfResult = type.isTypeOf(value, context, info); if (isThenable(isTypeOfResult)) { - promisedIsTypeOfResults.push(isTypeOfResult); - promisedIsTypeOfResultTypes.push(type); + promisedIsTypeOfResults[i] = isTypeOfResult; } else if (isTypeOfResult) { return type; } @@ -1133,11 +1131,13 @@ function defaultResolveTypeFn( } if (promisedIsTypeOfResults.length) { - return Promise.all(promisedIsTypeOfResults).then(isTypeOfResults => ( - promisedIsTypeOfResultTypes[ - isTypeOfResults.findIndex(isTypeOf => isTypeOf) - ] - )); + return Promise.all(promisedIsTypeOfResults).then(isTypeOfResults => { + for (let i = 0; i < isTypeOfResults.length; i++) { + if (isTypeOfResults[i]) { + return possibleTypes[i]; + } + } + }); } }