fix(stitching): change merged result format

Merged results that are null may carry errors from deeper within the query tree. Previous merged result format carries this metadata within a property on the result, but null has no properties.

New result format differs only in that a null result is transformed to an object with a special property signifying that the result was null, so that metadata can be added in the same way.

When merging within defaultMergedResolver, all null checks must check for this property as well. Because a result may be changed during annotation, the function is essentially no longer annotating only/i.e. modifying the object in place, and so it has been renamed to reflect that, with forEach changed to map when processing a list.

Fixes #26.

src/stitching/checkResultAndHandleErrors.ts

@@ -13,7 +13,7 @@ import { getResponseKeyFromInfo } from './getResponseKeyFromInfo';
 import {
   relocatedError,
   combineErrors,
-  annotateWithChildrenErrors
+  createMergedResult
 } from './errors';
 
 export function checkResultAndHandleErrors(
@@ -37,6 +37,10 @@ export function checkResultAndHandleErrors(
     }
   }
 
+  if (result.data[responseKey] == null) {
+    return handleNull(info, result.errors || []);
+  }
+
   return handleResult(info, result.data[responseKey], result.errors || []);
 }
 
@@ -45,22 +49,10 @@ export function handleResult(
   result: any,
   errors: ReadonlyArray<GraphQLError>
 ): any {
-  if (result == null) {
-    if (errors.length) {
-      throw relocatedError(
-        combineErrors(errors),
-        info.fieldNodes,
-        responsePathAsArray(info.path)
-      );
-    } else {
-      return null;
-    }
-  }
-
   const nullableType = getNullableType(info.returnType);
 
   if (isCompositeType(nullableType) || isListType(nullableType)) {
-    annotateWithChildrenErrors(result, errors);
+    result = createMergedResult(result, errors);
   }
 
   return parseOutputValue(nullableType, result);
@@ -74,3 +66,18 @@ function parseOutputValue(type: GraphQLType, value: any) {
   }
   return value;
 }
+
+export function handleNull(
+  info: GraphQLResolveInfo,
+  errors: ReadonlyArray<GraphQLError>,
+): null {
+  if (errors.length) {
+    throw relocatedError(
+      combineErrors(errors),
+      info.fieldNodes,
+      responsePathAsArray(info.path)
+    );
+  } else {
+    return null;
+  }
+}

src/stitching/createMergedResolver.ts

@@ -4,7 +4,7 @@ import {
   relocatedError,
   combineErrors,
   getErrorsFromParent,
-  annotateWithChildrenErrors,
+  createMergedResult,
 } from './errors';
 import defaultMergedResolver from './defaultMergedResolver';
 import { extractOneLevelOfFields } from './extractFields';
@@ -67,8 +67,7 @@ export function createMergedResolver({
             return null;
           }
         }
-        annotateWithChildrenErrors(result, errors);
-        parent = result;
+        parent = createMergedResult(result, errors);
       }
 
       fieldName = fromPath[fromPathLength - 1];

src/stitching/defaultMergedResolver.ts

@@ -1,6 +1,6 @@
 import { GraphQLFieldResolver, defaultFieldResolver } from 'graphql';
-import { getErrorsFromParent } from './errors';
-import { handleResult } from './checkResultAndHandleErrors';
+import { getErrorsFromParent, MERGED_NULL_SYMBOL } from './errors';
+import { handleResult, handleNull } from './checkResultAndHandleErrors';
 import { getResponseKeyFromInfo } from './getResponseKeyFromInfo';
 
 // Resolver that knows how to:
@@ -21,7 +21,13 @@ const defaultMergedResolver: GraphQLFieldResolver<any, any> = (parent, args, con
     return defaultFieldResolver(parent, args, context, info);
   }
 
-  return handleResult(info, parent[responseKey], errors);
+  const result = parent[responseKey];
+
+  if (result == null || result[MERGED_NULL_SYMBOL]) {
+    return handleNull(info, errors);
+  }
+
+  return handleResult(info, result, errors);
 };
 
 export default defaultMergedResolver;

src/stitching/errors.ts

@@ -40,14 +40,21 @@ export function relocatedError(
   );
 }
 
-export function annotateWithChildrenErrors(object: any, childrenErrors: ReadonlyArray<GraphQLError>): any {
-  if (!object) {
-    return object;
-  }
+export let MERGED_NULL_SYMBOL: any;
+if (
+  (typeof global !== 'undefined' && 'Symbol' in global) ||
+  (typeof window !== 'undefined' && 'Symbol' in window)
+) {
+  MERGED_NULL_SYMBOL = Symbol('mergedNull');
+} else {
+  MERGED_NULL_SYMBOL = '@@__mergedNull';
+}
 
-  if (!Array.isArray(childrenErrors)) {
-    object[ERROR_SYMBOL] = [];
-    return object;
+export function createMergedResult(object: any, childrenErrors: ReadonlyArray<GraphQLError> = []): any {
+  if (!object) {
+    object = {
+      [MERGED_NULL_SYMBOL]: true,
+    };
   }
 
   if (Array.isArray(object)) {
@@ -69,7 +76,7 @@ export function annotateWithChildrenErrors(object: any, childrenErrors: Readonly
       byIndex[index] = current;
     });
 
-    object.forEach((item, index) => annotateWithChildrenErrors(item, byIndex[index]));
+    object = object.map((item, index) => createMergedResult(item, byIndex[index]));
 
     return object;
   }

src/test/testErrors.ts

@@ -100,13 +100,13 @@ describe('Errors', () => {
 });
 
 describe('passes along errors for missing fields on list', () => {
-  it('hides error if null allowed', async () => {
+  it('if non-null', async () => {
     const typeDefs = `
       type Query {
         getOuter: Outer
       }
       type Outer {
-        innerList: [Inner]!
+        innerList: [Inner!]!
       }
       type Inner {
         mandatoryField: String!
@@ -130,20 +130,28 @@ describe('passes along errors for missing fields on list', () => {
     const result = await graphql(mergedSchema, `{ getOuter { innerList { mandatoryField } } }`);
     expect(result).to.deep.equal({
       data: {
-        getOuter: {
-          innerList: [null],
-        },
+        getOuter: null,
       },
+      errors: [{
+        locations: [{
+          column: 26,
+          line: 1,
+        }],
+        message: 'Cannot return null for non-nullable field Inner.mandatoryField.',
+        path: [
+          'getOuter',
+        ],
+      }]
     });
   });
 
-  it('if non-null', async () => {
+  it('even if nullable', async () => {
     const typeDefs = `
       type Query {
         getOuter: Outer
       }
       type Outer {
-        innerList: [Inner!]!
+        innerList: [Inner]!
       }
       type Inner {
         mandatoryField: String!
@@ -167,7 +175,9 @@ describe('passes along errors for missing fields on list', () => {
     const result = await graphql(mergedSchema, `{ getOuter { innerList { mandatoryField } } }`);
     expect(result).to.deep.equal({
       data: {
-        getOuter: null,
+        getOuter: {
+          innerList: [null],
+        },
       },
       errors: [{
         locations: [{
@@ -177,6 +187,9 @@ describe('passes along errors for missing fields on list', () => {
         message: 'Cannot return null for non-nullable field Inner.mandatoryField.',
         path: [
           'getOuter',
+          'innerList',
+          0,
+          'mandatoryField',
         ],
       }]
     });