enhance(stitch) better error handling for merge errors

[gmac]

Scenario: I have a not-null schema object attempting to merge a null result from a subschema. This is absolutely an error in my own data, and it justifiably errors. However, the returned error is:

"Cannot read property 'Symbol(initialSubschema)' of null"

That error indicates a failure in merge code – which is sort of true, but the source of the error is my own. This would be better for both developers and maintainers if such an error was contextualized in a way that doesn't make it look like a bug in stitching code.

Suggestion: wrap the point of the final merge in a try/catch, and have the catch delegate to some kind of onMergeError handler. It'd be useful for that handler to receive the merge type, the original selection set, and all fetched data that is attempting to merge. The default return from this handler could be something generic like error while merging subschema results

[gmac]

Example... here I've deliberately made one of the subservices return null rather than a real value, which causes the merge to fail:

import { makeExecutableSchema } from '@graphql-tools/schema';
import { stitchSchemas } from '@graphql-tools/stitch';
import { graphql } from 'graphql';

describe('Merge failure', () => {
  it('gives meaningful error on merge failure', async () => {
    const firstSchema = makeExecutableSchema({
      typeDefs: `
        type Thing {
          id: ID!
          name: String!
        }
        type Query {
          thing(id: ID!): Thing
        }
      `,
      resolvers: {
        Query: {
          thing: (root, { id }) => ({ id, name: 'The Thing' }),
        }
      }
    });

    const secondSchema = makeExecutableSchema({
      typeDefs: `
        type Thing {
          id: ID!
          description: String!
        }
        type Query {
          _thing(id: ID!): Thing
        }
      `,
      resolvers: {
        Query: {
          _thing: (root, { id }) => null,
        }
      }
    });

    const gatewaySchema = stitchSchemas({
      subschemas: [
        {
          schema: firstSchema,
          merge: {
            Thing: {
              selectionSet: '{ id }',
              fieldName: 'thing',
              args: ({ id }) => ({ id }),
            }
          }
        },
        {
          schema: secondSchema,
          merge: {
            Thing: {
              selectionSet: '{ id }',
              fieldName: '_thing',
              args: ({ id }) => ({ id }),
            }
          }
        },
      ],
      mergeTypes: true
    });

    const { data, errors } = await graphql(gatewaySchema, `
      query {
        thing(id: 23) {
          id
          name
          description
        }
      }
    `);

    console.log(errors);
  });
});

This results in the following error that looks like a merge error, but is really a subservice data issue. It would be better for both developers and maintainers if this was more expressive of something having failed in the service architecture and that this is really no fault of merging code:

[
  TypeError: Cannot read property 'Symbol(initialSubschema)' of null
      at /Users/greg.macwilliam/Projects/graphql-tools/packages/delegate/src/results/mergeProxiedResults.ts:9:29
      at Array.reduce (<anonymous>)
      at Object.mergeProxiedResults (/Users/greg.macwilliam/Projects/graphql-tools/packages/delegate/src/results/mergeProxiedResults.ts:8:37)
      at Object.mergeFields (/Users/greg.macwilliam/Projects/graphql-tools/packages/delegate/src/results/mergeFields.ts:198:9)
      at Object.handleObject (/Users/greg.macwilliam/Projects/graphql-tools/packages/delegate/src/results/handleObject.ts:48:10)
      at Object.handleResult (/Users/greg.macwilliam/Projects/graphql-tools/packages/delegate/src/results/handleResult.ts:35:12)
      at checkResultAndHandleErrors (/Users/greg.macwilliam/Projects/graphql-tools/packages/delegate/src/transforms/CheckResultAndHandleErrors.ts:56:10)
      at CheckResultAndHandleErrors.transformResult (/Users/greg.macwilliam/Projects/graphql-tools/packages/delegate/src/transforms/CheckResultAndHandleErrors.ts:32:12)
      at /Users/greg.macwilliam/Projects/graphql-tools/packages/delegate/src/Transformer.ts:40:38
      at Array.reduceRight (<anonymous>) {
    locations: [ [Object] ],
    path: [ 'thing' ]
  }
]

I got poking around in here but discovered I was a bit out of my league on understanding the constraints of responding to a failure there: https://github.com/gmac/graphql-tools/blob/master/packages/delegate/src/results/mergeProxiedResults.ts#L10-L12

[gmac]

@yaacovCR – would something like this be reasonable? It does more or less what a developer and/or consumer is looking to see:

export function mergeProxiedResults(target: any, ...sources: Array<any>): any {
  const results = sources.filter(source => !(source instanceof Error));
  try {
    const fieldSubschemaMap = results.reduce((acc: Record<any, SubschemaConfig>, source: any) => {
      const subschema = source[OBJECT_SUBSCHEMA_SYMBOL];
      Object.keys(source).forEach(key => {
        acc[key] = subschema;
      });
      return acc;
    }, {});
  } catch {
    return new GraphQLError('failed to merge subschema results');
  }

  ...
  return result;
}

This results in a much more meaningful error for the situation:

[
      GraphQLError [Object]: failed to merge subschema results
      ...
]

[yaacovCR]

Fixed in v7 #1935

See 63cbd13

[yaacovCR]

Or, at least should be fixed if valid GraphQL result returned... Including null case, may still make sense to wrap in try/catch PRs welcome on v7 branch.

[yaacovCR]

Released in v7. Very likely additional room for improvement, however....