fix(gateway): Ignore thrown errors from extendSchema

[trevor-scheer]

extendSchema throws errors in the case that we extend a type before it exists during composition. This should certainly result in composition errors (which we have validations for), but it shouldn't cause composeAndValidate to throw, which is unexpected.

This change catches and discards errors thrown by extendSchema. The reason for discarding and not capturing the error (Unknown type Bar.) is that it's duplicated when we capture errors from validateSDL immediately after: https://github.com/apollographql/federation/blob/8f77bf41d8c5075ee1b5de6d7018a70326f39acd/federation-js/src/composition/compose.ts#L421

In the extendSchema code path, there is only one throw case (excluding an initial assertSchema which I'm not concerned about) and this test case exercises it. However, I'm unsure about this approach because there could be other errors thrown in the future which may be valuable or worth capturing. Open to feedback here, or possibly an alternative solution.

Bit of an unrelated tangent here, but worth mentioning:
Modifying this area of the codebase made it tempting to return as early as possible once all errors are collected and stop returning both a schema and errors (which seems like odd behavior). However, we can provide more errors to the user when compose returns both, since composeAndValidate can take composition errors and append useful schema validation errors to the final set of errors provided.

It wouldn't be unreasonable at some point to stop returning both a schema and errors from composeAndValidate, however (this is already what we do with composedSdl).

[trevor-scheer]

As seen here in the new test case, when a type goes unrecognized, fields which return that type are abandoned. This handles the case where a field which is expected to exist cannot actually be found in the schema.

[martijnwalraven]

I think this is indeed a bit of an unconventional use of the graphql-js APIs. We're telling extendSchema to skip validating the passed in SDL, while we may actually have captured validation errors on the previous line through validateSDL. It isn't great to ignore errors, but I think the error that's being thrown would never occur under normal circumstances, because errors should be caught by validation (either in validateSDL or validateSchema). So for now, this seems like the best we can do. (I agree it may make sense to fail early instead, although that would keep us from reporting additional errors.)

That does make me wonder if we shouldn't also collect errors resulting from validateSchema in composeAndValidate (in addition to collecting errors from validateSDL). I don't have a good sense of what additional errors that might catch (and whether those are relevant for our use case), but extendSchema has quite a few comments that state they purposefully avoid throwing errors early because these will be caught by validateSchema later and that can provide more meaningful feedback.

[trevor-scheer]

break was a mistake here, but corrected in #481

[glasser]

I suggested to Trevor on Slack that we do something more like

try {
  extendSchema()
} catch (e) {
  if (e is not the one particular error we know we handle later) {
    throw Error("extendSchema threw an unexpected error type", e)  // or add to the errors list maybe
  }
}