Restrict selection set normalization during query planning to @key/@r…

…equires (#3165)

Normalization for conditions/field sets makes sense for recursive query
planning, but we can't quite do it for `@provides` (inline fragments
have a specific meaning there) and `@fromContext` (we'd like to forbid
non-top-level inline fragments there), and some composition validations
become more complex to explain/spot if we permit them to normalize. This
PR restricts that normalization to when we fetch the conditions for
`@key`/`@requires` field sets.

internals-js/src/federation.ts

@@ -2280,12 +2280,14 @@ export function parseFieldSetArgument({
   fieldAccessor,
   validate,
   decorateValidationErrors = true,
+  normalize = false,
 }: {
   parentType: CompositeType,
   directive: Directive<SchemaElement<any, any>, {fields: any}>,
   fieldAccessor?: (type: CompositeType, fieldName: string) => FieldDefinition<any> | undefined,
   validate?: boolean,
   decorateValidationErrors?: boolean,
+  normalize?: boolean,
 }): SelectionSet {
   try {
     const selectionSet = parseSelectionSet({
@@ -2302,7 +2304,9 @@ export function parseFieldSetArgument({
         }
       });
     }
-    return selectionSet.normalize({ parentType, recursive: true });
+    return normalize
+      ? selectionSet.normalize({ parentType, recursive: true })
+      : selectionSet;
   } catch (e) {
     if (!(e instanceof GraphQLError) || !decorateValidationErrors) {
       throw e;

query-graphs-js/src/querygraph.ts

@@ -757,7 +757,7 @@ function federateSubgraphs(supergraph: Schema, subgraphs: QueryGraph[]): QueryGr
           // an entity).
           assert(isInterfaceType(type) || isObjectType(type), () => `Invalid "@key" application on non Object || Interface type "${type}"`);
           const isInterfaceObject = subgraphMetadata.isInterfaceObjectType(type);
-          const conditions = parseFieldSetArgument({ parentType: type, directive: keyApplication });
+          const conditions = parseFieldSetArgument({ parentType: type, directive: keyApplication, normalize: true });
 
           // We'll look at adding edges from "other subgraphs" to the current type. So the tail of all the edges
           // we'll build in this branch is always going to be the same.
@@ -807,7 +807,7 @@ function federateSubgraphs(supergraph: Schema, subgraphs: QueryGraph[]): QueryGr
                 const implemType = implemVertice.type;
                 assert(isCompositeType(implemType), () => `${implemType} should be composite since it implements ${typeInSupergraph} in the supergraph`);
                 try {
-                  const implConditions = parseFieldSetArgument({ parentType: implemType, directive: keyApplication, validate: false });
+                  const implConditions = parseFieldSetArgument({ parentType: implemType, directive: keyApplication, validate: false, normalize: true });
                   builder.addEdge(implemHead, tail, new KeyResolution(), implConditions);
                 } catch (e) {
                   // Ignored on purpose: it just means the key is not usable on this subgraph.
@@ -824,7 +824,7 @@ function federateSubgraphs(supergraph: Schema, subgraphs: QueryGraph[]): QueryGr
           const field = e.transition.definition;
           assert(isCompositeType(type), () => `Non composite type "${type}" should not have field collection edge ${e}`);
           for (const requiresApplication of field.appliedDirectivesOf(requireDirective)) {
-            const conditions = parseFieldSetArgument({ parentType: type, directive: requiresApplication });
+            const conditions = parseFieldSetArgument({ parentType: type, directive: requiresApplication, normalize: true });
             const head = copyPointers[i].copiedVertex(e.head);
             // We rely on the fact that the edge indexes will be the same in the copied builder. But there is no real reason for
             // this to not be the case at this point so...