From 6223245d14f37b472825bc6136798052ea23077f Mon Sep 17 00:00:00 2001 From: Slava Kim Date: Fri, 10 Jun 2016 15:10:57 -0700 Subject: [PATCH] Errors thrown from resolvers have the execution path (#396) * Errors thrown from resolvers have the execution path This path is also passed in the `info` object to resolvers. This information is useful for ease of debugging and more detailed logging. * Remove PathedError * rename property executionPath to path * remove an unnecessary block * info.executionPath -> info.path * a minor tweak to make the body of executeFields look closer to executeFieldsSerially * remove the unnecessary clone of info * Add a test for a path with non-nullable fields * stylistic changes * remove stray property --- src/__tests__/starWarsIntrospection-test.js | 15 ++ src/__tests__/starWarsQuery-test.js | 164 ++++++++++++++++++++ src/__tests__/starWarsSchema.js | 21 +++ src/error/GraphQLError.js | 1 + src/error/locatedError.js | 4 +- src/execution/execute.js | 80 ++++++++-- src/type/definition.js | 1 + 7 files changed, 272 insertions(+), 14 deletions(-) diff --git a/src/__tests__/starWarsIntrospection-test.js b/src/__tests__/starWarsIntrospection-test.js index 6646825e34..e9faa7f95b 100644 --- a/src/__tests__/starWarsIntrospection-test.js +++ b/src/__tests__/starWarsIntrospection-test.js @@ -205,6 +205,13 @@ describe('Star Wars Introspection Tests', () => { kind: 'LIST' } }, + { + name: 'secretBackstory', + type: { + name: 'String', + kind: 'SCALAR' + } + }, { name: 'primaryFunction', type: { @@ -284,6 +291,14 @@ describe('Star Wars Introspection Tests', () => { } } }, + { + name: 'secretBackstory', + type: { + name: 'String', + kind: 'SCALAR', + ofType: null + } + }, { name: 'primaryFunction', type: { diff --git a/src/__tests__/starWarsQuery-test.js b/src/__tests__/starWarsQuery-test.js index 180e12b3af..ea3e51c798 100644 --- a/src/__tests__/starWarsQuery-test.js +++ b/src/__tests__/starWarsQuery-test.js @@ -11,6 +11,12 @@ import { expect } from 'chai'; import { describe, it } from 'mocha'; import { StarWarsSchema } from './starWarsSchema.js'; import { graphql } from '../graphql'; +import { + GraphQLObjectType, + GraphQLNonNull, + GraphQLSchema, + GraphQLString, +} from '../type'; // 80+ char lines are useful in describe/it, so ignore in this file. /* eslint-disable max-len */ @@ -364,4 +370,162 @@ describe('Star Wars Query Tests', () => { expect(result).to.deep.equal({ data: expected }); }); }); + + describe('Reporting errors raised in resolvers', () => { + it('Correctly reports error on accessing secretBackstory', async () => { + const query = ` + query HeroNameQuery { + hero { + name + secretBackstory + } + } + `; + const expected = { + hero: { + name: 'R2-D2', + secretBackstory: null + } + }; + const expectedErrors = [ 'secretBackstory is secret.' ]; + const result = await graphql(StarWarsSchema, query); + expect(result.data).to.deep.equal(expected); + expect(result.errors.map(e => e.message)).to.deep.equal(expectedErrors); + expect( + result.errors.map(e => e.path)).to.deep.equal( + [ [ 'hero', 'secretBackstory' ] ]); + }); + + it('Correctly reports error on accessing secretBackstory in a list', async () => { + const query = ` + query HeroNameQuery { + hero { + name + friends { + name + secretBackstory + } + } + } + `; + const expected = { + hero: { + name: 'R2-D2', + friends: [ + { + name: 'Luke Skywalker', + secretBackstory: null, + }, + { + name: 'Han Solo', + secretBackstory: null, + }, + { + name: 'Leia Organa', + secretBackstory: null, + }, + ] + } + }; + const expectedErrors = [ + 'secretBackstory is secret.', + 'secretBackstory is secret.', + 'secretBackstory is secret.', + ]; + const result = await graphql(StarWarsSchema, query); + expect(result.data).to.deep.equal(expected); + expect(result.errors.map(e => e.message)).to.deep.equal(expectedErrors); + expect( + result.errors.map(e => e.path) + ).to.deep.equal( + [ + [ 'hero', 'friends', 0, 'secretBackstory' ], + [ 'hero', 'friends', 1, 'secretBackstory' ], + [ 'hero', 'friends', 2, 'secretBackstory' ], + ]); + }); + + it('Correctly reports error on accessing through an alias', async () => { + const query = ` + query HeroNameQuery { + mainHero: hero { + name + story: secretBackstory + } + } + `; + const expected = { + mainHero: { + name: 'R2-D2', + story: null, + } + }; + const expectedErrors = [ + 'secretBackstory is secret.', + ]; + const result = await graphql(StarWarsSchema, query); + expect(result.data).to.deep.equal(expected); + expect(result.errors.map(e => e.message)).to.deep.equal(expectedErrors); + expect( + result.errors.map(e => e.path) + ).to.deep.equal([ [ 'mainHero', 'story' ] ]); + }); + + it('Full response path is included when fields are non-nullable', async () => { + const A = new GraphQLObjectType({ + name: 'A', + fields: () => ({ + nullableA: { + type: A, + resolve: () => ({}), + }, + nonNullA: { + type: new GraphQLNonNull(A), + resolve: () => ({}), + }, + throws: { + type: new GraphQLNonNull(GraphQLString), + resolve: () => { throw new Error('Catch me if you can'); }, + }, + }), + }); + const queryType = new GraphQLObjectType({ + name: 'query', + fields: () => ({ + nullableA: { + type: A, + resolve: () => ({}) + } + }), + }); + const schema = new GraphQLSchema({ + query: queryType, + }); + + const query = ` + query { + nullableA { + nullableA { + nonNullA { + nonNullA { + throws + } + } + } + } + } + `; + + const result = await graphql(schema, query); + const expected = { + nullableA: { + nullableA: null + } + }; + expect(result.data).to.deep.equal(expected); + expect( + result.errors.map(e => e.path)).to.deep.equal( + [ [ 'nullableA', 'nullableA', 'nonNullA', 'nonNullA', 'throws' ] ]); + }); + }); }); diff --git a/src/__tests__/starWarsSchema.js b/src/__tests__/starWarsSchema.js index e315986a66..0c9c34b637 100644 --- a/src/__tests__/starWarsSchema.js +++ b/src/__tests__/starWarsSchema.js @@ -102,6 +102,7 @@ const episodeEnum = new GraphQLEnumType({ * name: String * friends: [Character] * appearsIn: [Episode] + * secretBackstory: String * } */ const characterInterface = new GraphQLInterfaceType({ @@ -125,6 +126,10 @@ const characterInterface = new GraphQLInterfaceType({ type: new GraphQLList(episodeEnum), description: 'Which movies they appear in.', }, + secretBackstory: { + type: GraphQLString, + description: 'All secrets about their past.', + }, }), resolveType: character => { return getHuman(character.id) ? humanType : droidType; @@ -140,6 +145,7 @@ const characterInterface = new GraphQLInterfaceType({ * name: String * friends: [Character] * appearsIn: [Episode] + * secretBackstory: String * } */ const humanType = new GraphQLObjectType({ @@ -168,6 +174,13 @@ const humanType = new GraphQLObjectType({ type: GraphQLString, description: 'The home planet of the human, or null if unknown.', }, + secretBackstory: { + type: GraphQLString, + description: 'Where are they from and how they came to be who they are.', + resolve: () => { + throw new Error('secretBackstory is secret.'); + }, + }, }), interfaces: [ characterInterface ] }); @@ -181,6 +194,7 @@ const humanType = new GraphQLObjectType({ * name: String * friends: [Character] * appearsIn: [Episode] + * secretBackstory: String * primaryFunction: String * } */ @@ -206,6 +220,13 @@ const droidType = new GraphQLObjectType({ type: new GraphQLList(episodeEnum), description: 'Which movies they appear in.', }, + secretBackstory: { + type: GraphQLString, + description: 'Construction date and the name of the designer.', + resolve: () => { + throw new Error('secretBackstory is secret.'); + }, + }, primaryFunction: { type: GraphQLString, description: 'The primary function of the droid.', diff --git a/src/error/GraphQLError.js b/src/error/GraphQLError.js index 08fc5b883d..abd5760f6f 100644 --- a/src/error/GraphQLError.js +++ b/src/error/GraphQLError.js @@ -20,6 +20,7 @@ export class GraphQLError extends Error { source: Source; positions: Array; locations: any; + path: Array; originalError: ?Error; constructor( diff --git a/src/error/locatedError.js b/src/error/locatedError.js index 6bab9071ac..252fdf3cbd 100644 --- a/src/error/locatedError.js +++ b/src/error/locatedError.js @@ -18,7 +18,8 @@ import { GraphQLError } from './GraphQLError'; */ export function locatedError( originalError: ?Error, - nodes: Array + nodes: Array, + path: Array ): GraphQLError { const message = originalError ? originalError.message || String(originalError) : @@ -26,5 +27,6 @@ export function locatedError( const stack = originalError ? originalError.stack : null; const error = new GraphQLError(message, nodes, stack); error.originalError = originalError; + error.path = path; return error; } diff --git a/src/execution/execute.js b/src/execution/execute.js index 29e9de0e97..14cd51c847 100644 --- a/src/execution/execute.js +++ b/src/execution/execute.js @@ -237,10 +237,12 @@ function executeOperation( Object.create(null) ); + const exePath = []; + if (operation.operation === 'mutation') { - return executeFieldsSerially(exeContext, type, rootValue, fields); + return executeFieldsSerially(exeContext, type, rootValue, exePath, fields); } - return executeFields(exeContext, type, rootValue, fields); + return executeFields(exeContext, type, rootValue, exePath, fields); } /** @@ -287,16 +289,22 @@ function executeFieldsSerially( exeContext: ExecutionContext, parentType: GraphQLObjectType, sourceValue: mixed, + exePath: Array, fields: {[key: string]: Array} ): Promise { return Object.keys(fields).reduce( (prevPromise, responseName) => prevPromise.then(results => { const fieldASTs = fields[responseName]; + + const childExePath = exePath.slice(); + childExePath.push(responseName); + const result = resolveField( exeContext, parentType, sourceValue, - fieldASTs + fieldASTs, + childExePath ); if (result === undefined) { return results; @@ -322,6 +330,7 @@ function executeFields( exeContext: ExecutionContext, parentType: GraphQLObjectType, sourceValue: mixed, + exePath: Array, fields: {[key: string]: Array} ): Object { let containsPromise = false; @@ -329,11 +338,16 @@ function executeFields( const finalResults = Object.keys(fields).reduce( (results, responseName) => { const fieldASTs = fields[responseName]; + + const childExePath = exePath.slice(); + childExePath.push(responseName); + const result = resolveField( exeContext, parentType, sourceValue, - fieldASTs + fieldASTs, + childExePath ); if (result === undefined) { return results; @@ -526,7 +540,8 @@ function resolveField( exeContext: ExecutionContext, parentType: GraphQLObjectType, source: mixed, - fieldASTs: Array + fieldASTs: Array, + exePath: Array ): mixed { const fieldAST = fieldASTs[0]; const fieldName = fieldAST.name.value; @@ -565,6 +580,7 @@ function resolveField( rootValue: exeContext.rootValue, operation: exeContext.operation, variableValues: exeContext.variableValues, + path: exePath }; // Get the resolve function, regardless of if its result is normal @@ -576,6 +592,7 @@ function resolveField( returnType, fieldASTs, info, + exePath, result ); } @@ -605,12 +622,20 @@ function completeValueCatchingError( returnType: GraphQLType, fieldASTs: Array, info: GraphQLResolveInfo, + exePath: Array, result: mixed ): mixed { // If the field type is non-nullable, then it is resolved without any // protection from errors. if (returnType instanceof GraphQLNonNull) { - return completeValue(exeContext, returnType, fieldASTs, info, result); + return completeValue( + exeContext, + returnType, + fieldASTs, + info, + exePath, + result + ); } // Otherwise, error protection is applied, logging the error and resolving @@ -621,6 +646,7 @@ function completeValueCatchingError( returnType, fieldASTs, info, + exePath, result ); if (isThenable(completed)) { @@ -668,6 +694,7 @@ function completeValue( returnType: GraphQLType, fieldASTs: Array, info: GraphQLResolveInfo, + exePath: Array, result: mixed ): mixed { // If result is a Promise, apply-lift over completeValue. @@ -679,16 +706,17 @@ function completeValue( returnType, fieldASTs, info, + exePath, resolved ), // If rejected, create a located error, and continue to reject. - error => Promise.reject(locatedError(error, fieldASTs)) + error => Promise.reject(locatedError(error, fieldASTs, exePath)) ); } // If result is an Error, throw a located error. if (result instanceof Error) { - throw locatedError(result, fieldASTs); + throw locatedError(result, fieldASTs, exePath); } // If field type is NonNull, complete for inner type, and throw field error @@ -699,6 +727,7 @@ function completeValue( returnType.ofType, fieldASTs, info, + exePath, result ); if (completed === null) { @@ -718,7 +747,14 @@ function completeValue( // If field type is List, complete each item in the list with the inner type if (returnType instanceof GraphQLList) { - return completeListValue(exeContext, returnType, fieldASTs, info, result); + return completeListValue( + exeContext, + returnType, + fieldASTs, + info, + exePath, + result + ); } // If field type is a leaf type, Scalar or Enum, serialize to a valid value, @@ -737,6 +773,7 @@ function completeValue( returnType, fieldASTs, info, + exePath, result ); } @@ -748,6 +785,7 @@ function completeValue( returnType, fieldASTs, info, + exePath, result ); } @@ -768,6 +806,7 @@ function completeListValue( returnType: GraphQLList, fieldASTs: Array, info: GraphQLResolveInfo, + exePath: Array, result: mixed ): mixed { invariant( @@ -780,9 +819,21 @@ function completeListValue( // where the list contains no Promises by avoiding creating another Promise. const itemType = returnType.ofType; let containsPromise = false; - const completedResults = result.map(item => { - const completedItem = - completeValueCatchingError(exeContext, itemType, fieldASTs, info, item); + const completedResults = result.map((item, index) => { + // No need to modify the info object containing the path, + // since from here on it is not ever accessed by resolver functions. + const childExePath = exePath.slice(); + childExePath.push(index); + + const completedItem = completeValueCatchingError( + exeContext, + itemType, + fieldASTs, + info, + childExePath, + item + ); + if (!containsPromise && isThenable(completedItem)) { containsPromise = true; } @@ -814,6 +865,7 @@ function completeAbstractValue( returnType: GraphQLAbstractType, fieldASTs: Array, info: GraphQLResolveInfo, + exePath: Array, result: mixed ): mixed { const runtimeType = returnType.resolveType ? @@ -842,6 +894,7 @@ function completeAbstractValue( runtimeType, fieldASTs, info, + exePath, result ); } @@ -854,6 +907,7 @@ function completeObjectValue( returnType: GraphQLObjectType, fieldASTs: Array, info: GraphQLResolveInfo, + exePath: Array, result: mixed ): mixed { // If there is an isTypeOf predicate function, call it with the @@ -883,7 +937,7 @@ function completeObjectValue( } } - return executeFields(exeContext, returnType, result, subFieldASTs); + return executeFields(exeContext, returnType, result, exePath, subFieldASTs); } /** diff --git a/src/type/definition.js b/src/type/definition.js index 70f0bf2a6e..22d705ce2e 100644 --- a/src/type/definition.js +++ b/src/type/definition.js @@ -483,6 +483,7 @@ export type GraphQLResolveInfo = { rootValue: mixed, operation: OperationDefinition, variableValues: { [variableName: string]: mixed }, + path: Array } export type GraphQLFieldConfig = {