feat(errors): Pass through all possible errors.

Use new relocatedError function to update the original GraphQLErrors with the new path. Addresses ardatan#743, ardatan#1037, ardatan#1046, apollographql/apollo-server#1582.
yaacovCR · Sep 22, 2019 · 4de91b8 · 4de91b8
1 parent 1a51313
commit 4de91b8
Show file tree

Hide file tree

Showing 6 changed files with 257 additions and 231 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,7 +1,5 @@
 # Change log
 
-# Changelog
-
 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.
 
 ## [5.0.0](https://github.com/yaacovCR/graphql-tools-fork/compare/v4.0.4...v5.0.0) (2019-06-02)

diff --git 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<any, any> = (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
@@ -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<GraphQLFormattedError>): any {
-  if (!childrenErrors || childrenErrors.length === 0) {
-    // Nothing to see here, move along
+export function relocatedError(
+  originalError: Error | GraphQLError,
+  nodes: ReadonlyArray<ASTNode>,
+  path: ReadonlyArray<string | number>
+): 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<GraphQLError>): 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<GraphQLFormattedError>;
-    } {
-  const errors = (object && object[ERROR_SYMBOL]) || [];
-  const childrenErrors: Array<GraphQLFormattedError> = [];
+): Array<GraphQLError> {
+  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<GraphQLFormattedError>);
+  const nullableType = getNullableType(info.returnType);
+  if (isObjectType(nullableType) || isListType(nullableType)) {
+    annotateWithChildrenErrors(resultObject, result.errors);
   }
   return resultObject;
 }
 
-function concatErrors(errors: ReadonlyArray<GraphQLError>) {
-  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>): 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
@@ -1,46 +1,58 @@
 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 });
   }
 }
 
 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: [