diff --git a/CHANGELOG.md b/CHANGELOG.md index 649574ff910..3af20e457b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ # Change log +All notable changes to this project will be documented in this file. See [standard-version](https://github.com/conventional-changelog/standard-version) for commit guidelines. + ### 4.0.5 * Fixes a bug where schemas with scalars could not be merged when passed to diff --git a/src/stitching/defaultMergedResolver.ts b/src/stitching/defaultMergedResolver.ts index cbf754a03a8..ce5e65156f0 100644 --- a/src/stitching/defaultMergedResolver.ts +++ b/src/stitching/defaultMergedResolver.ts @@ -1,6 +1,16 @@ -import { GraphQLFieldResolver, responsePathAsArray } from 'graphql'; -import { locatedError } from 'graphql/error'; -import { getErrorsFromParent, annotateWithChildrenErrors } from './errors'; +import { + GraphQLFieldResolver, + responsePathAsArray, + getNullableType, + isObjectType, + isListType +} from 'graphql'; +import { + getErrorsFromParent, + annotateWithChildrenErrors, + combineErrors, + relocatedError +} from './errors'; import { getResponseKeyFromInfo } from './getResponseKeyFromInfo'; // Resolver that knows how to: @@ -12,26 +22,30 @@ const defaultMergedResolver: GraphQLFieldResolver = (parent, args, con } const responseKey = getResponseKeyFromInfo(info); - const errorResult = getErrorsFromParent(parent, responseKey); + const errors = getErrorsFromParent(parent, responseKey); - if (errorResult.kind === 'OWN') { - throw locatedError(new Error(errorResult.error.message), info.fieldNodes, responsePathAsArray(info.path)); + // check to see if parent is not a proxied result, i.e. if parent resolver was manually overwritten + // See https://github.com/apollographql/graphql-tools/issues/967 + if (!Array.isArray(errors)) { + return parent[info.fieldName]; } let result = parent[responseKey]; - if (result == null) { - result = parent[info.fieldName]; + // if null, throw all possible errors + if (!result && errors.length) { + throw relocatedError( + combineErrors(errors), + info.fieldNodes, + responsePathAsArray(info.path) + ); } - // subscription result mapping - if (!result && parent.data && parent.data[responseKey]) { - result = parent.data[responseKey]; + const nullableType = getNullableType(info.returnType); + if (isObjectType(nullableType) || isListType(nullableType)) { + annotateWithChildrenErrors(result, errors); } - if (errorResult.errors) { - result = annotateWithChildrenErrors(result, errorResult.errors); - } return result; }; diff --git a/src/stitching/errors.ts b/src/stitching/errors.ts index 94ba6851a25..e927f9f2477 100644 --- a/src/stitching/errors.ts +++ b/src/stitching/errors.ts @@ -1,11 +1,13 @@ import { GraphQLResolveInfo, responsePathAsArray, + getNullableType, + isObjectType, + isListType, ExecutionResult, - GraphQLFormattedError, GraphQLError, + ASTNode } from 'graphql'; -import { locatedError } from 'graphql/error'; import { getResponseKeyFromInfo } from './getResponseKeyFromInfo'; export let ERROR_SYMBOL: any; @@ -18,9 +20,36 @@ if ( ERROR_SYMBOL = '@@__subSchemaErrors'; } -export function annotateWithChildrenErrors(object: any, childrenErrors: ReadonlyArray): any { - if (!childrenErrors || childrenErrors.length === 0) { - // Nothing to see here, move along +export function relocatedError( + originalError: Error | GraphQLError, + nodes: ReadonlyArray, + path: ReadonlyArray +): GraphQLError { + if (Array.isArray((originalError as GraphQLError).path)) { + return new GraphQLError( + (originalError as GraphQLError).message, + (originalError as GraphQLError).nodes, + (originalError as GraphQLError).source, + (originalError as GraphQLError).positions, + path ? path : (originalError as GraphQLError).path, + (originalError as GraphQLError).originalError, + (originalError as GraphQLError).extensions + ); + } + + return new GraphQLError( + originalError && originalError.message, + (originalError && (originalError as any).nodes) || nodes, + originalError && (originalError as any).source, + originalError && (originalError as any).positions, + path, + originalError, + ); +} + +export function annotateWithChildrenErrors(object: any, childrenErrors: ReadonlyArray): any { + if (!Array.isArray(childrenErrors)) { + object[ERROR_SYMBOL] = []; return object; } @@ -33,55 +62,50 @@ export function annotateWithChildrenErrors(object: any, childrenErrors: Readonly } const index = error.path[1]; const current = byIndex[index] || []; - current.push({ - ...error, - path: error.path.slice(1) - }); + current.push( + relocatedError( + error, + error.nodes, + error.path ? error.path.slice(1) : undefined + ) + ); byIndex[index] = current; }); return object.map((item, index) => annotateWithChildrenErrors(item, byIndex[index])); } - return { - ...object, - [ERROR_SYMBOL]: childrenErrors.map(error => ({ - ...error, - ...(error.path ? { path: error.path.slice(1) } : {}) - })) - }; + object[ERROR_SYMBOL] = childrenErrors.map(error => { + const newError = relocatedError( + error, + error.nodes, + error.path ? error.path.slice(1) : undefined + ); + return newError; + }); + + return object; } export function getErrorsFromParent( object: any, fieldName: string -): - | { - kind: 'OWN'; - error: any; - } - | { - kind: 'CHILDREN'; - errors?: Array; - } { - const errors = (object && object[ERROR_SYMBOL]) || []; - const childrenErrors: Array = []; +): Array { + const errors = object && object[ERROR_SYMBOL]; + + if (!Array.isArray(errors)) { + return null; + } + + const childrenErrors = []; for (const error of errors) { - if (!error.path || (error.path.length === 1 && error.path[0] === fieldName)) { - return { - kind: 'OWN', - error - }; - } else if (error.path[0] === fieldName) { + if (!error.path || error.path[0] === fieldName) { childrenErrors.push(error); } } - return { - kind: 'CHILDREN', - errors: childrenErrors - }; + return childrenErrors; } class CombinedError extends Error { @@ -101,28 +125,38 @@ export function checkResultAndHandleErrors( responseKey = getResponseKeyFromInfo(info); } - if (result.errors && (!result.data || result.data[responseKey] == null)) { - // apollo-link-http & http-link-dataloader need the - // result property to be passed through for better error handling. - // If there is only one error, which contains a result property, pass the error through - const newError = - result.errors.length === 1 && hasResult(result.errors[0]) - ? result.errors[0] - : new CombinedError(concatErrors(result.errors), result.errors); - throw locatedError(newError, info.fieldNodes, responsePathAsArray(info.path)); + if (!result.data || !result.data[responseKey]) { + if (result.errors) { + throw relocatedError( + combineErrors(result.errors), + info.fieldNodes, + responsePathAsArray(info.path) + ); + } } + result.errors = result.errors || []; + let resultObject = result.data[responseKey]; - if (result.errors) { - resultObject = annotateWithChildrenErrors(resultObject, result.errors as ReadonlyArray); + const nullableType = getNullableType(info.returnType); + if (isObjectType(nullableType) || isListType(nullableType)) { + annotateWithChildrenErrors(resultObject, result.errors); } return resultObject; } -function concatErrors(errors: ReadonlyArray) { - return errors.map(error => error.message).join('\n'); -} - -function hasResult(error: any) { - return error.result || error.extensions || (error.originalError && error.originalError.result); +export function combineErrors(errors: ReadonlyArray): GraphQLError | CombinedError { + if (errors.length === 1) { + return new GraphQLError( + errors[0].message, + errors[0].nodes, + errors[0].source, + errors[0].positions, + errors[0].path, + errors[0].originalError, + errors[0].extensions + ); + } else { + return new CombinedError(errors.map(error => error.message).join('\n'), errors); + } } diff --git a/src/test/testErrors.ts b/src/test/testErrors.ts index 05bcf69c292..5758f0460a0 100644 --- a/src/test/testErrors.ts +++ b/src/test/testErrors.ts @@ -1,17 +1,14 @@ import { assert } from 'chai'; import { GraphQLResolveInfo, GraphQLError } from 'graphql'; -import { checkResultAndHandleErrors, getErrorsFromParent, ERROR_SYMBOL } from '../stitching/errors'; +import { + relocatedError, + checkResultAndHandleErrors, + getErrorsFromParent, + ERROR_SYMBOL +} from '../stitching/errors'; import 'mocha'; -class ErrorWithResult extends GraphQLError { - public result: any; - constructor(message: string, result: any) { - super(message); - this.result = result; - } -} - class ErrorWithExtensions extends GraphQLError { constructor(message: string, code: string) { super(message, null, null, null, null, null, { code }); @@ -19,28 +16,43 @@ class ErrorWithExtensions extends GraphQLError { } describe('Errors', () => { + describe('relocatedError', () => { + it('should adjust the path of a GraphqlError', () => { + const originalError = new GraphQLError('test', null, null, null, ['test']); + const newError = relocatedError(originalError, null, ['test', 1]); + const expectedError = new GraphQLError('test', null, null, null, ['test', 1]); + assert.deepEqual(newError, expectedError); + }); + + it('should also locate a non GraphQLError', () => { + const originalError = new Error('test'); + const newError = relocatedError(originalError, null, ['test', 1]); + const expectedError = new GraphQLError('test', null, null, null, ['test', 1]); + assert.deepEqual(newError, expectedError); + }); + }); + describe('getErrorsFromParent', () => { - it('should return OWN error kind if path is not defined', () => { + it('should return all errors including if path is not defined', () => { const mockErrors = { responseKey: '', [ERROR_SYMBOL]: [ { message: 'Test error without path' - } + } as GraphQLError ] }; - assert.deepEqual(getErrorsFromParent(mockErrors, 'responseKey'), { - kind: 'OWN', - error: mockErrors[ERROR_SYMBOL][0] - }); + assert.deepEqual(getErrorsFromParent(mockErrors, 'responseKey'), + [mockErrors[ERROR_SYMBOL][0]] + ); }); }); describe('checkResultAndHandleErrors', () => { - it('persists single error with a result', () => { + it('persists single error', () => { const result = { - errors: [new ErrorWithResult('Test error', 'result')] + errors: [new GraphQLError('Test error')] }; try { checkResultAndHandleErrors(result, {} as GraphQLResolveInfo, 'responseKey'); @@ -63,23 +75,6 @@ describe('Errors', () => { } }); - it('persists original errors without a result', () => { - const result = { - errors: [new GraphQLError('Test error')] - }; - try { - checkResultAndHandleErrors(result, {} as GraphQLResolveInfo, 'responseKey'); - } catch (e) { - assert.equal(e.message, 'Test error'); - assert.isNotEmpty(e.originalError); - assert.isNotEmpty(e.originalError.errors); - assert.lengthOf(e.originalError.errors, result.errors.length); - result.errors.forEach((error, i) => { - assert.deepEqual(e.originalError.errors[i], error); - }); - } - }); - it('combines errors and persists the original errors', () => { const result = { errors: [ diff --git a/src/test/testMergeSchemas.ts b/src/test/testMergeSchemas.ts index d1d24f56873..6bd1ded30f2 100644 --- a/src/test/testMergeSchemas.ts +++ b/src/test/testMergeSchemas.ts @@ -30,6 +30,8 @@ import { forAwaitEach } from 'iterall'; import { makeExecutableSchema } from '../makeExecutableSchema'; import { IResolvers } from '../Interfaces'; +const removeLocations = ({ locations, ...rest }: any): any => ({ ...rest }); + const testCombinations = [ { name: 'local', @@ -2152,13 +2154,12 @@ fragment BookingFragment on Booking { ${bookingFragment} }`, ); - expect(mergedResult).to.deep.equal({ - errors: propertyResult.errors, - data: { - ...propertyResult.data, - ...bookingResult.data, - }, + expect(mergedResult.data).to.deep.equal({ + ...propertyResult.data, + ...bookingResult.data, }); + expect(mergedResult.errors.map(removeLocations)).to.deep.equal( + propertyResult.errors.map(removeLocations)); const mergedResult2 = await graphql( mergedSchema, @@ -2170,21 +2171,13 @@ fragment BookingFragment on Booking { `, ); - expect(mergedResult2).to.deep.equal({ - errors: [ - { - locations: [ - { - column: 19, - line: 3, - }, - ], - message: 'Sample error non-null!', - path: ['errorTestNonNull'], - }, - ], - data: null, - }); + expect(mergedResult2.data).to.equal(null); + expect(mergedResult2.errors.map(removeLocations)).to.deep.equal([ + { + message: 'Sample error non-null!', + path: ['errorTestNonNull'], + }, + ]); }); it('nested errors', async () => { @@ -2205,114 +2198,102 @@ fragment BookingFragment on Booking { `, ); - expect(result).to.deep.equal({ - data: { - propertyById: { - bookings: [ - { - bookingErrorAlias: null, - error: null, - id: 'b1', - }, - { - bookingErrorAlias: null, - error: null, - id: 'b2', - }, - { - bookingErrorAlias: null, - error: null, - id: 'b3', - }, - ], - error: null, - errorAlias: null, + expect(result.data).to.deep.equal({ + propertyById: { + bookings: [ + { + bookingErrorAlias: null, + error: null, + id: 'b1', + }, + { + bookingErrorAlias: null, + error: null, + id: 'b2', + }, + { + bookingErrorAlias: null, + error: null, + id: 'b3', + }, + ], + error: null, + errorAlias: null, + } + }); + expect(result.errors.map(removeLocations)).to.deep.equal([ + { + extensions: { + code: 'SOME_CUSTOM_CODE' }, + message: 'Property.error error', + path: ['propertyById', 'error'], }, - errors: [ - { - locations: [ - { - column: 17, - line: 4, - }, - ], - message: 'Property.error error', - path: ['propertyById', 'error'], - }, - { - locations: [ - { - column: 17, - line: 5, - }, - ], - message: 'Property.error error', - path: ['propertyById', 'errorAlias'], - }, - { - locations: [ - { - column: 19, - line: 8, - }, - ], - message: 'Booking.error error', - path: ['propertyById', 'bookings', 0, 'error'], - }, - { - locations: [ - { - column: 19, - line: 9, - }, - ], - message: 'Booking.error error', - path: ['propertyById', 'bookings', 0, 'bookingErrorAlias'], - }, - { - locations: [ - { - column: 19, - line: 8, - }, - ], - message: 'Booking.error error', - path: ['propertyById', 'bookings', 1, 'error'], - }, - { - locations: [ - { - column: 19, - line: 9, - }, - ], - message: 'Booking.error error', - path: ['propertyById', 'bookings', 1, 'bookingErrorAlias'], - }, - { - locations: [ - { - column: 19, - line: 8, - }, - ], - message: 'Booking.error error', - path: ['propertyById', 'bookings', 2, 'error'], - }, - { - locations: [ - { - column: 19, - line: 9, - }, - ], - message: 'Booking.error error', - path: ['propertyById', 'bookings', 2, 'bookingErrorAlias'], + { + extensions: { + code: 'SOME_CUSTOM_CODE' }, - ], - }); + message: 'Property.error error', + path: ['propertyById', 'errorAlias'], + }, + { + message: 'Booking.error error', + path: ['propertyById', 'bookings', 0, 'error'], + }, + { + message: 'Booking.error error', + path: ['propertyById', 'bookings', 0, 'bookingErrorAlias'], + }, + { + message: 'Booking.error error', + path: ['propertyById', 'bookings', 1, 'error'], + }, + { + message: 'Booking.error error', + path: ['propertyById', 'bookings', 1, 'bookingErrorAlias'], + }, + { + message: 'Booking.error error', + path: ['propertyById', 'bookings', 2, 'error'], + }, + { + message: 'Booking.error error', + path: ['propertyById', 'bookings', 2, 'bookingErrorAlias'], + } + ]); }); + + it( + 'should preserve custom error extensions from the original schema, ' + + 'when merging schemas', + async () => { + const propertyQuery = ` + query { + properties(limit: 1) { + error + } + } + `; + + const propertyResult = await graphql( + propertySchema, + propertyQuery, + ); + + const mergedResult = await graphql( + mergedSchema, + propertyQuery, + ); + + [propertyResult, mergedResult].forEach((result) => { + expect(result.errors).to.exist; + expect(result.errors.length > 0).to.be.true; + const error = result.errors[0]; + expect(error.extensions).to.exist; + expect(error.extensions.code).to.equal('SOME_CUSTOM_CODE'); + }); + } + ); }); describe('types in schema extensions', () => { @@ -2729,7 +2710,7 @@ fragment BookingFragment on Booking { }); }); - it('defaultMergedResolver should work with non-root aliases', async () => { + it('defaultMergedResolver should work with aliases if parent merged resolver is manually overwritten', async () => { // Source: https://github.com/apollographql/graphql-tools/issues/967 const typeDefs = ` type Query { diff --git a/src/test/testingSchemas.ts b/src/test/testingSchemas.ts index 79c455d43be..b08bb474b79 100644 --- a/src/test/testingSchemas.ts +++ b/src/test/testingSchemas.ts @@ -407,7 +407,11 @@ const propertyResolvers: IResolvers = { Property: { error() { - throw new Error('Property.error error'); + const error = new Error('Property.error error'); + (error as any).extensions = { + code: 'SOME_CUSTOM_CODE', + }; + throw error; }, }, };