From 7d8d6a0ca672f945ae41b50af62fde657cc17421 Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Mon, 3 Aug 2020 13:46:15 -0400 Subject: [PATCH] fix(errors): preserve merged paths This is not the optimal way to address #1047 but is the most backwards compatible. --- packages/delegate/src/createMergedResolver.ts | 19 ++++++- packages/delegate/src/results/handleNull.ts | 42 ++++++-------- packages/delegate/src/results/handleResult.ts | 23 +++++++- packages/delegate/src/results/mergeFields.ts | 12 ++-- .../src/results/mergeProxiedResults.ts | 56 +++++++++++++++--- packages/stitch/tests/errors.test.ts | 35 ++++++++++++ packages/utils/src/errors.ts | 57 +++++++++++++++++++ 7 files changed, 200 insertions(+), 44 deletions(-) diff --git a/packages/delegate/src/createMergedResolver.ts b/packages/delegate/src/createMergedResolver.ts index 535c327e3dd..14305c6e0ef 100644 --- a/packages/delegate/src/createMergedResolver.ts +++ b/packages/delegate/src/createMergedResolver.ts @@ -1,6 +1,13 @@ import { GraphQLError } from 'graphql'; -import { IFieldResolver, getErrors, setErrors, relocatedError, ERROR_SYMBOL } from '@graphql-tools/utils'; +import { + IFieldResolver, + getErrors, + setErrors, + relocatedError, + ERROR_SYMBOL, + extendedError, +} from '@graphql-tools/utils'; import { OBJECT_SUBSCHEMA_SYMBOL } from './symbols'; @@ -54,7 +61,15 @@ function dehoistResult(parent: any, delimeter = '__gqltf__'): any { const path = error.path.slice(); const pathSegment = path.shift(); const expandedPathSegment: Array = (pathSegment as string).split(delimeter); - return relocatedError(error, expandedPathSegment.concat(path)); + const newPath = expandedPathSegment.concat(path); + const graphQLToolsMergedPath = error.extensions.graphQLToolsMergedPath; + let newGraphQLToolsMergedPath = graphQLToolsMergedPath.slice(); + newGraphQLToolsMergedPath.pop(); + newGraphQLToolsMergedPath = newGraphQLToolsMergedPath.concat(newPath); + return extendedError(relocatedError(error, newPath), { + ...error.extensions, + graphQLToolsMergedPath: newGraphQLToolsMergedPath, + }); } return error; diff --git a/packages/delegate/src/results/handleNull.ts b/packages/delegate/src/results/handleNull.ts index 9762d3ff9cf..52ecd2b68d7 100644 --- a/packages/delegate/src/results/handleNull.ts +++ b/packages/delegate/src/results/handleNull.ts @@ -1,36 +1,28 @@ import { GraphQLError } from 'graphql'; import AggregateError from '@ardatan/aggregate-error'; -import { getErrorsByPathSegment, relocatedError } from '@graphql-tools/utils'; + +import { relocatedError, unextendedError } from '@graphql-tools/utils'; export function handleNull(errors: ReadonlyArray) { if (errors.length) { - if (errors.some(error => !error.path || error.path.length < 2)) { - if (errors.length > 1) { - const combinedError = new AggregateError(errors); - return combinedError; - } - const error = errors[0]; - return error.originalError || relocatedError(error, null); - } else if (errors.some(error => typeof error.path[1] === 'string')) { - const childErrors = getErrorsByPathSegment(errors); - - const result = {}; - Object.keys(childErrors).forEach(pathSegment => { - result[pathSegment] = handleNull(childErrors[pathSegment]); - }); - - return result; + const graphQLToolsMergedPath = errors[0].extensions.graphQLToolsMergedPath; + const unannotatedErrors = errors.map(error => unextendedError(error, 'graphQLToolsMergedPath')); + + if (unannotatedErrors.length > 1) { + const combinedError = new AggregateError(unannotatedErrors); + return new GraphQLError( + combinedError.message, + undefined, + undefined, + undefined, + graphQLToolsMergedPath, + combinedError + ); } - const childErrors = getErrorsByPathSegment(errors); - - const result: Array = []; - Object.keys(childErrors).forEach(pathSegment => { - result.push(handleNull(childErrors[pathSegment])); - }); - - return result; + const error = unannotatedErrors[0]; + return relocatedError(error, graphQLToolsMergedPath); } return null; diff --git a/packages/delegate/src/results/handleResult.ts b/packages/delegate/src/results/handleResult.ts index 76cc53e5172..aa8592771af 100644 --- a/packages/delegate/src/results/handleResult.ts +++ b/packages/delegate/src/results/handleResult.ts @@ -6,6 +6,7 @@ import { isListType, GraphQLError, GraphQLSchema, + responsePathAsArray, } from 'graphql'; import { SubschemaConfig } from '../types'; @@ -13,6 +14,7 @@ import { SubschemaConfig } from '../types'; import { handleNull } from './handleNull'; import { handleObject } from './handleObject'; import { handleList } from './handleList'; +import { extendedError } from '@graphql-tools/utils'; export function handleResult( result: any, @@ -23,17 +25,32 @@ export function handleResult( returnType = info.returnType, skipTypeMerging?: boolean ): any { + const annotatedErrors = errors.map(error => { + if (error.extensions?.graphQLToolsMergedPath == null) { + return extendedError(error, { + ...error.extensions, + graphQLToolsMergedPath: + info == null + ? error.path + : error.path != null + ? [...responsePathAsArray(info.path), ...error.path.slice(1)] + : responsePathAsArray(info.path), + }); + } + return error; + }); + const type = getNullableType(returnType); if (result == null) { - return handleNull(errors); + return handleNull(annotatedErrors); } if (isLeafType(type)) { return type.parseValue(result); } else if (isCompositeType(type)) { - return handleObject(type, result, errors, subschema, context, info, skipTypeMerging); + return handleObject(type, result, annotatedErrors, subschema, context, info, skipTypeMerging); } else if (isListType(type)) { - return handleList(type, result, errors, subschema, context, info, skipTypeMerging); + return handleList(type, result, annotatedErrors, subschema, context, info, skipTypeMerging); } } diff --git a/packages/delegate/src/results/mergeFields.ts b/packages/delegate/src/results/mergeFields.ts index 25610b13b89..c0245e4409b 100644 --- a/packages/delegate/src/results/mergeFields.ts +++ b/packages/delegate/src/results/mergeFields.ts @@ -170,21 +170,21 @@ export function mergeFields( } let containsPromises = false; - const maybePromises: Promise | any = []; + const resultMap: Map | any, SelectionSetNode> = new Map(); delegationMap.forEach((selectionSet: SelectionSetNode, s: SubschemaConfig) => { const maybePromise = s.merge[typeName].resolve(object, context, info, s, selectionSet); - maybePromises.push(maybePromise); - if (!containsPromises && isPromise(maybePromise)) { + resultMap.set(maybePromise, selectionSet); + if (isPromise(maybePromise)) { containsPromises = true; } }); return containsPromises - ? Promise.all(maybePromises).then(results => + ? Promise.all(resultMap.keys()).then(results => mergeFields( mergedTypeInfo, typeName, - mergeProxiedResults(object, ...results), + mergeProxiedResults(info, object, results, Array.from(resultMap.values())), unproxiableFieldNodes, combineSubschemas(sourceSubschemaOrSourceSubschemas, proxiableSubschemas), nonProxiableSubschemas, @@ -195,7 +195,7 @@ export function mergeFields( : mergeFields( mergedTypeInfo, typeName, - mergeProxiedResults(object, ...maybePromises), + mergeProxiedResults(info, object, Array.from(resultMap.keys()), Array.from(resultMap.values())), unproxiableFieldNodes, combineSubschemas(sourceSubschemaOrSourceSubschemas, proxiableSubschemas), nonProxiableSubschemas, diff --git a/packages/delegate/src/results/mergeProxiedResults.ts b/packages/delegate/src/results/mergeProxiedResults.ts index 8dbb57de296..827ad7c15df 100644 --- a/packages/delegate/src/results/mergeProxiedResults.ts +++ b/packages/delegate/src/results/mergeProxiedResults.ts @@ -1,18 +1,51 @@ -import { mergeDeep, ERROR_SYMBOL } from '@graphql-tools/utils'; +import { GraphQLError, GraphQLResolveInfo, responsePathAsArray, SelectionSetNode, GraphQLObjectType } from 'graphql'; + +import { + mergeDeep, + ERROR_SYMBOL, + extendedError, + collectFields, + GraphQLExecutionContext, + relocatedError, +} from '@graphql-tools/utils'; import { SubschemaConfig } from '../types'; import { OBJECT_SUBSCHEMA_SYMBOL, FIELD_SUBSCHEMA_MAP_SYMBOL } from '../symbols'; -export function mergeProxiedResults(target: any, ...sources: Array): any { +export function mergeProxiedResults( + info: GraphQLResolveInfo, + target: any, + sources: Array, + selectionSets: Array +): any { const results: Array = []; - const errors: Array = []; + let errors: Array = []; + + const path = responsePathAsArray(info.path); - sources.forEach(source => { - if (source instanceof Error) { - errors.push(source); + sources.forEach((source, index) => { + if (source instanceof GraphQLError) { + const selectionSet = selectionSets[index]; + const fieldNodes = collectFields( + { + schema: info.schema, + variableValues: {}, + fragments: {}, + } as GraphQLExecutionContext, + info.schema.getType(target.__typename) as GraphQLObjectType, + selectionSet, + Object.create(null), + Object.create(null) + ); + const nullResult = {}; + Object.keys(fieldNodes).forEach(responseKey => { + errors.push(relocatedError(source, [responseKey])); + nullResult[responseKey] = null; + }); + results.push(nullResult); } else { + errors = errors.concat(source[ERROR_SYMBOL]); results.push(source); - errors.push(source[ERROR_SYMBOL]); } }); @@ -29,7 +62,14 @@ export function mergeProxiedResults(target: any, ...sources: Array): any { ? Object.assign({}, target[FIELD_SUBSCHEMA_MAP_SYMBOL], fieldSubschemaMap) : fieldSubschemaMap; - result[ERROR_SYMBOL] = target[ERROR_SYMBOL].concat(...errors); + const annotatedErrors = errors.map(error => { + return extendedError(error, { + ...error.extensions, + graphQLToolsMergedPath: error.path != null ? [...path, ...error.path] : responsePathAsArray(info.path), + }); + }); + + result[ERROR_SYMBOL] = target[ERROR_SYMBOL].concat(annotatedErrors); return result; } diff --git a/packages/stitch/tests/errors.test.ts b/packages/stitch/tests/errors.test.ts index c0caf77f734..2154a74dfee 100644 --- a/packages/stitch/tests/errors.test.ts +++ b/packages/stitch/tests/errors.test.ts @@ -38,6 +38,7 @@ describe('passes along errors for missing fields on list', () => { const originalResult = await graphql(schema, query); const stitchedResult = await graphql(stitchedSchema, query); expect(stitchedResult).toEqual(originalResult); + expect(stitchedResult.errors[0].path).toEqual(originalResult.errors[0].path); }); test('even if nullable', async () => { @@ -72,6 +73,7 @@ describe('passes along errors for missing fields on list', () => { const originalResult = await graphql(schema, query); const stitchedResult = await graphql(stitchedSchema, query); expect(stitchedResult).toEqual(originalResult); + expect(stitchedResult.errors[0].path).toEqual(originalResult.errors[0].path); }); }); @@ -108,6 +110,7 @@ describe('passes along errors when list field errors', () => { const originalResult = await graphql(schema, query); const stitchedResult = await graphql(stitchedSchema, query); expect(stitchedResult).toEqual(originalResult); + expect(stitchedResult.errors[0].path).toEqual(originalResult.errors[0].path); }); test('even if nullable', async () => { @@ -142,6 +145,38 @@ describe('passes along errors when list field errors', () => { const originalResult = await graphql(schema, query); const stitchedResult = await graphql(stitchedSchema, query); expect(stitchedResult).toEqual(originalResult); + expect(stitchedResult.errors[0].path).toEqual(originalResult.errors[0].path); + }); + + describe('passes along correct error when there are two non-null fields', () => { + test('should work', async () => { + const schema = makeExecutableSchema({ + typeDefs: ` + type Query { + getBoth: Both + } + type Both { + mandatoryField1: String! + mandatoryField2: String! + } + `, + resolvers: { + Query: { + getBoth: () => ({ mandatoryField1: 'test' }), + }, + }, + }); + + const stitchedSchema = stitchSchemas({ + subschemas: [schema], + }); + + const query = '{ getBoth { mandatoryField1 mandatoryField2 } }'; + const originalResult = await graphql(schema, query); + const stitchedResult = await graphql(stitchedSchema, query); + expect(stitchedResult).toEqual(originalResult); + expect(stitchedResult.errors[0].path).toEqual(originalResult.errors[0].path); + }); }); }); diff --git a/packages/utils/src/errors.ts b/packages/utils/src/errors.ts index 062993c8b53..85c8230aa9b 100644 --- a/packages/utils/src/errors.ts +++ b/packages/utils/src/errors.ts @@ -14,6 +14,63 @@ export function relocatedError(originalError: GraphQLError, path?: ReadonlyArray ); } +export function extendedError(originalError: GraphQLError, extensions: Record): GraphQLError { + return new GraphQLError( + originalError.message, + originalError.nodes, + originalError.source, + originalError.positions, + originalError.path, + originalError.originalError, + extensions + ); +} + +export function unextendedError(originalError: GraphQLError, extensionKey: string): GraphQLError { + const originalExtensions = originalError.extensions; + + if (originalExtensions == null) { + return originalError; + } + + const originalExtensionKeys = Object.keys(originalExtensions); + + if (!originalExtensionKeys.length) { + return originalError; + } + + const newExtensions = {}; + let extensionsPresent = false; + originalExtensionKeys.forEach(key => { + if (key !== extensionKey) { + newExtensions[key] = originalExtensions[key]; + extensionsPresent = true; + } + }); + + if (!extensionsPresent) { + return new GraphQLError( + originalError.message, + originalError.nodes, + originalError.source, + originalError.positions, + originalError.path, + originalError.originalError, + undefined + ); + } + + return new GraphQLError( + originalError.message, + originalError.nodes, + originalError.source, + originalError.positions, + originalError.path, + originalError.originalError, + newExtensions + ); +} + export function slicedError(originalError: GraphQLError) { return relocatedError(originalError, originalError.path != null ? originalError.path.slice(1) : undefined); }