Rewrites gateway post-processing

[pcmanus]

With @interfaceObject, some queries can sometimes be entirely fullfilled by a sugraph using the @interfaceObject (when only fields provided by that subgraph are queried and the query does not necessitate knowledge of the underlying implementation type as neither __typename is queried, nor is there any type condition). But the post-processing that the gateway does on the internal results to compute the final response was currently using GraphQL-js execution, and this was breaking in those cases because we get results for what is an interface in the supergraph but with no __typename.

Additionally, using GraphQL-js execution for this post-processing has long been a "FIXME" of the code, as this is a heavy-handed way to do this processing.

This patch rewrite this post-processing to stop using GraphQL-js execution and instead implements the subset of what is truly necessary for that post-processing. Doing so allows to ensure that the code work as needed with the @interfaceObject cases mentioned above.

Fixes #2320

[netlify]

👷 Deploy request for apollo-federation-docs pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	8ec14ed

[changeset-bot]

🦋 Changeset detected

Latest commit: 8ec14ed

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages

Name	Type
@apollo/composition	Patch
apollo-federation-integration-testsuite	Patch
@apollo/gateway	Patch
@apollo/federation-internals	Patch
@apollo/query-graphs	Patch
@apollo/query-planner	Patch
@apollo/subgraph	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

[pcmanus]

Note to reviewers: this test was kind of artificial, in that it was parsing/validating the query using a version of the schema with nothing marked @inaccessible, but then it called executePlan with the schema where fields were marked @inaccessible. The real code will never ever do that, and there is plenty of problems you can get if you don't pass the same schema to different parts of the code dealing with a given query.

In reality, other tests ensures that @inacessible fields are not part of the API schema, but then @inaccessible fields cannot be queried in the first place. So the error we now get is the correct behaviour.

[pcmanus]

Note to reviewers: there is a bunch of comments on the background around this in executeQueryPlan, which is part about postProcessingErrors. The tl;dr is that the existing code was unconditionally ignoring post-processing errors, which some users have been complaining about in the past because it can make it very hard to debug issue with broken subgraphs, and an earlier patch tried to change this by unconditionally returning post-processing errors, but this was quickly reverted because post-processing errors often duplicate subgraphs errors and people were annoyed.

This patch implements a middle ground where we do not include post-processing errors at all if we have some query plan execution errors (thus make sure we can't create duplicates), but include those post-processing errors if there is no query plan execution errors whatsoever, to at least help in that case.

This typically make this test kind of work again: the test have a subgraph that returns an object for an @inacessible type, and the old was nullifying that object (correctly, we shouldn't return it) but was not indicating any error (which isn't great). The updated code bring back the error.

Do not that the new error message send differs from what graphQL-js would have produced, but I think the new message is a bit more helpful at pinpointing the problem.

Lastly, this code made me noticed that we still had some cleanErrorOfInaccessible code that was completely unused, so this patch removes it. Now that we control the code generating the errors, we have no reason to ever need to "clean" those error message: we can just make sure we don't leak anything in the first place (which is what is done here).

[pcmanus]

Note to reviewers: we do add an argument to an exported method, and I'll mention it in the changelog. We already added the supergraphSchema argument in a previous release the same way so I don't think it's an issue.

In general, the arguments of this method are a bit messy now because we pass the API schema as a Schema but we also have that same schema in the OperationContext as a GraphQLSchema, and while we do want to pass both versions to avoid costly conversions, we could maybe clean things up. I'll leave that for a followup though.

[pcmanus]

Note to reviewers: wrapSchemaWithAliasResolver was needed due to the use of GraphQL-js execution. Namely, when GraphQL-js execute is passed an object, it expects that object to match the schema and so to use field names from the schema. But the internal data we gather during query plan execution will have data under the "response name", and so under aliases when aliases was used. That method made sure things were still working.

The new code is written to use response name directly, so no need for that.