inital changes for re-attempt at #1047

Author: yaacovCR

packages/delegate/src/proxiedResult.ts

@@ -1,6 +1,11 @@
-import { GraphQLError } from 'graphql';
-
-import { mergeDeep, ERROR_SYMBOL, relocatedError, setErrors, getErrors } from '@graphql-tools/utils';
+import {
+  mergeDeep,
+  ERROR_SYMBOL,
+  relocatedError,
+  setErrors,
+  getErrors,
+  RelativeGraphQLError,
+} from '@graphql-tools/utils';
 
 import { handleNull } from './results/handleNull';
 
@@ -25,10 +30,7 @@ export function unwrapResult(parent: any, path: Array<string>): any {
       return handleNull(errors);
     }
 
-    setErrors(
-      object,
-      errors.map(error => relocatedError(error, error.path != null ? error.path.slice(1) : undefined))
-    );
+    setErrors(object, errors);
     setObjectSubschema(object, subschema);
 
     newParent = object;
@@ -51,15 +53,15 @@ export function dehoistResult(parent: any, delimeter = '__gqltf__'): any {
     obj[fieldName] = parent[alias];
   });
 
-  result[ERROR_SYMBOL] = parent[ERROR_SYMBOL].map((error: GraphQLError) => {
-    if (error.path != null) {
-      const path = error.path.slice();
-      const pathSegment = path.shift();
-      const expandedPathSegment: Array<string | number> = (pathSegment as string).split(delimeter);
-      return relocatedError(error, expandedPathSegment.concat(path));
-    }
-
-    return error;
+  result[ERROR_SYMBOL] = parent[ERROR_SYMBOL].map((error: RelativeGraphQLError) => {
+    const path = error.relativePath.slice();
+    const pathSegment = path.pop();
+    const expandedPathSegment: Array<string | number> = (pathSegment as string).split(delimeter);
+    return {
+      relativePath: path.concat(expandedPathSegment),
+      // setting path to null will cause issues for errors that bubble up from non nullable fields
+      graphQLError: relocatedError(error.graphQLError, null),
+    };
   });
 
   result[OBJECT_SUBSCHEMA_SYMBOL] = parent[OBJECT_SUBSCHEMA_SYMBOL];

packages/delegate/src/results/handleList.ts

@@ -1,7 +1,6 @@
 import {
   GraphQLList,
   GraphQLSchema,
-  GraphQLError,
   GraphQLResolveInfo,
   getNullableType,
   GraphQLType,
@@ -10,7 +9,7 @@ import {
   isListType,
 } from 'graphql';
 
-import { getErrorsByPathSegment } from '@graphql-tools/utils';
+import { getErrorsByPathSegment, RelativeGraphQLError } from '@graphql-tools/utils';
 
 import { handleNull } from './handleNull';
 import { handleObject } from './handleObject';
@@ -19,7 +18,7 @@ import { SubschemaConfig } from '../types';
 export function handleList(
   type: GraphQLList<any>,
   list: Array<any>,
-  errors: ReadonlyArray<GraphQLError>,
+  errors: Array<RelativeGraphQLError>,
   subschema: GraphQLSchema | SubschemaConfig,
   context: Record<string, any>,
   info: GraphQLResolveInfo,
@@ -43,7 +42,7 @@ export function handleList(
 function handleListMember(
   type: GraphQLType,
   listMember: any,
-  errors: ReadonlyArray<GraphQLError>,
+  errors: Array<RelativeGraphQLError>,
   subschema: GraphQLSchema | SubschemaConfig,
   context: Record<string, any>,
   info: GraphQLResolveInfo,

packages/delegate/src/results/handleNull.ts

@@ -1,36 +1,13 @@
-import { GraphQLError } from 'graphql';
-
 import AggregateError from 'aggregate-error';
-import { getErrorsByPathSegment, relocatedError } from '@graphql-tools/utils';
+import { RelativeGraphQLError } from '@graphql-tools/utils';
 
-export function handleNull(errors: ReadonlyArray<GraphQLError>) {
+export function handleNull(errors: Array<RelativeGraphQLError>) {
   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;
+    if (errors.length > 1) {
+      return new AggregateError(errors.map(error => error.graphQLError));
     }
 
-    const childErrors = getErrorsByPathSegment(errors);
-
-    const result: Array<any> = [];
-    Object.keys(childErrors).forEach(pathSegment => {
-      result.push(handleNull(childErrors[pathSegment]));
-    });
-
-    return result;
+    return errors[0].graphQLError;
   }
 
   return null;

packages/delegate/src/results/handleObject.ts

@@ -1,34 +1,29 @@
 import {
   GraphQLCompositeType,
-  GraphQLError,
   GraphQLSchema,
   isAbstractType,
   FieldNode,
   GraphQLObjectType,
   GraphQLResolveInfo,
 } from 'graphql';
 
-import { collectFields, GraphQLExecutionContext, setErrors, slicedError } from '@graphql-tools/utils';
+import { collectFields, GraphQLExecutionContext, setErrors, RelativeGraphQLError } from '@graphql-tools/utils';
 import { setObjectSubschema, isSubschemaConfig } from '../Subschema';
 import { mergeFields } from '../mergeFields';
 import { MergedTypeInfo, SubschemaConfig } from '../types';
 
 export function handleObject(
   type: GraphQLCompositeType,
   object: any,
-  errors: ReadonlyArray<GraphQLError>,
+  errors: Array<RelativeGraphQLError>,
   subschema: GraphQLSchema | SubschemaConfig,
   context: Record<string, any>,
   info: GraphQLResolveInfo,
   skipTypeMerging?: boolean
 ) {
   const stitchingInfo = info?.schema.extensions?.stitchingInfo;
 
-  setErrors(
-    object,
-    errors.map(error => slicedError(error))
-  );
-
+  setErrors(object, errors);
   setObjectSubschema(object, subschema);
 
   if (skipTypeMerging || !stitchingInfo) {

packages/delegate/src/results/handleResult.ts

@@ -1,12 +1,6 @@
-import {
-  GraphQLResolveInfo,
-  getNullableType,
-  isCompositeType,
-  isLeafType,
-  isListType,
-  GraphQLError,
-  GraphQLSchema,
-} from 'graphql';
+import { GraphQLResolveInfo, getNullableType, isCompositeType, isLeafType, isListType, GraphQLSchema } from 'graphql';
+
+import { RelativeGraphQLError } from '@graphql-tools/utils';
 
 import { SubschemaConfig } from '../types';
 
@@ -16,7 +10,7 @@ import { handleList } from './handleList';
 
 export function handleResult(
   result: any,
-  errors: ReadonlyArray<GraphQLError>,
+  errors: Array<RelativeGraphQLError>,
   subschema: GraphQLSchema | SubschemaConfig,
   context: Record<string, any>,
   info: GraphQLResolveInfo,

packages/delegate/src/transforms/CheckResultAndHandleErrors.ts

@@ -1,6 +1,6 @@
 import { GraphQLResolveInfo, ExecutionResult, GraphQLOutputType, GraphQLSchema } from 'graphql';
 
-import { Transform, getResponseKeyFromInfo } from '@graphql-tools/utils';
+import { Transform, getResponseKeyFromInfo, toRelativeErrors } from '@graphql-tools/utils';
 import { handleResult } from '../results/handleResult';
 import { SubschemaConfig } from '../types';
 
@@ -50,7 +50,7 @@ export function checkResultAndHandleErrors(
   returnType: GraphQLOutputType = info.returnType,
   skipTypeMerging?: boolean
 ): any {
-  const errors = result.errors != null ? result.errors : [];
+  const errors = result.errors != null ? toRelativeErrors(result.errors, info) : [];
   const data = result.data != null ? result.data[responseKey] : undefined;
 
   return handleResult(data, errors, subschema, context, info, returnType, skipTypeMerging);

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);
+    });
   });
 });
 

packages/utils/src/errors.ts

@@ -1,7 +1,12 @@
-import { GraphQLError } from 'graphql';
+import { GraphQLError, responsePathAsArray, GraphQLResolveInfo } from 'graphql';
 
 export const ERROR_SYMBOL = Symbol('subschemaErrors');
 
+export interface RelativeGraphQLError {
+  relativePath: Array<string | number>;
+  graphQLError: GraphQLError;
+}
+
 export function relocatedError(originalError: GraphQLError, path?: ReadonlyArray<string | number>): GraphQLError {
   return new GraphQLError(
     originalError.message,
@@ -14,33 +19,55 @@ export function relocatedError(originalError: GraphQLError, path?: ReadonlyArray
   );
 }
 
-export function slicedError(originalError: GraphQLError) {
-  return relocatedError(originalError, originalError.path != null ? originalError.path.slice(1) : undefined);
+export function sliceRelativeError(error: RelativeGraphQLError): RelativeGraphQLError {
+  return {
+    ...error,
+    relativePath: error.relativePath?.slice(1),
+  };
+}
+
+export function toRelativeErrors(
+  errors: ReadonlyArray<GraphQLError>,
+  info: GraphQLResolveInfo
+): Array<RelativeGraphQLError> {
+  return errors.map(error => {
+    const relativePath = error.path?.slice() || [];
+    const sourcePath = info != null ? responsePathAsArray(info.path) : [];
+    return {
+      relativePath,
+      graphQLError: relocatedError(error, sourcePath.concat(relativePath.slice(1))),
+    };
+  });
 }
 
-export function getErrorsByPathSegment(errors: ReadonlyArray<GraphQLError>): Record<string, Array<GraphQLError>> {
-  const record = Object.create(null);
+export function setErrors(result: any, errors: Array<RelativeGraphQLError>) {
+  result[ERROR_SYMBOL] = errors;
+}
+
+export function getErrorsByPathSegment(
+  errors: Array<RelativeGraphQLError>
+): Record<string, Array<RelativeGraphQLError>> {
+  const record: Record<string, Array<RelativeGraphQLError>> = Object.create(null);
   errors.forEach(error => {
-    if (!error.path || error.path.length < 2) {
+    if (!error.relativePath || error.relativePath.length < 2) {
       return;
     }
 
-    const pathSegment = error.path[1];
+    const pathSegment = error.relativePath[1];
 
-    const current = pathSegment in record ? record[pathSegment] : [];
-    current.push(slicedError(error));
+    const current: Array<RelativeGraphQLError> = pathSegment in record ? record[pathSegment] : [];
+    current.push({
+      relativePath: error.relativePath.slice(1),
+      graphQLError: error.graphQLError,
+    });
     record[pathSegment] = current;
   });
 
   return record;
 }
 
-export function setErrors(result: any, errors: Array<GraphQLError>) {
-  result[ERROR_SYMBOL] = errors;
-}
-
-export function getErrors(result: any, pathSegment: string): Array<GraphQLError> {
-  const errors = result != null ? result[ERROR_SYMBOL] : result;
+export function getErrors(result: any, pathSegment: string | number): Array<RelativeGraphQLError> {
+  const errors: Array<RelativeGraphQLError> = result != null ? result[ERROR_SYMBOL] : result;
 
   if (!Array.isArray(errors)) {
     return null;
@@ -49,8 +76,8 @@ export function getErrors(result: any, pathSegment: string): Array<GraphQLError>
   const fieldErrors = [];
 
   for (const error of errors) {
-    if (!error.path || error.path[0] === pathSegment) {
-      fieldErrors.push(error);
+    if (!error.relativePath || error.relativePath.length < 2 || error.relativePath[1] === pathSegment) {
+      fieldErrors.push(sliceRelativeError(error));
     }
   }