Use alias in QP when querying conflicting fields

In a few situations, the query planner was generating queries where
the same response name was queried at the same "level" with incompatible
types, resulting in invalid queries (the queries were failing the
[`FieldsInSetCanMerge`](https://spec.graphql.org/draft/#FieldsInSetCanMerge()))
validation for the GraphQL sepc).

This commit detects this case, and when it would happen, aliases one
of the occurence in the fetch to make the query valid. Once receiving
the fetch result, the aliased value is rewritten to it's original
response name.

Fixes apollographql#1257

gateway-js/src/executeQueryPlan.ts

@@ -25,6 +25,7 @@ import {
   QueryPlanFieldNode,
   getResponseName,
   FetchDataInputRewrite,
+  FetchDataOutputRewrite,
 } from '@apollo/query-planner';
 import { deepMerge } from './utilities/deepMerge';
 import { isNotNullOrUndefined } from './utilities/array';
@@ -308,7 +309,7 @@ async function executeFetch(
         );
 
         for (const entity of entities) {
-          deepMerge(entity, dataReceivedFromService);
+          deepMerge(entity, withFetchRewrites(dataReceivedFromService, fetch.outputRewrites));
         }
       } else {
         const requires = fetch.requires;
@@ -370,7 +371,7 @@ async function executeFetch(
         }
 
         for (let i = 0; i < entities.length; i++) {
-          deepMerge(entities[representationToEntity[i]], receivedEntities[i]);
+          deepMerge(entities[representationToEntity[i]], withFetchRewrites(receivedEntities[i], filterEntityRewrites(representations[i], fetch.outputRewrites)));
         }
       }
     }
@@ -486,6 +487,56 @@ async function executeFetch(
   }
 }
 
+function applyOrMapRecursive(value: any | any[], fct: (v: any) => any | undefined): any | any[] | undefined {
+  if (Array.isArray(value)) {
+    const res = value.map((elt) => applyOrMapRecursive(elt, fct)).filter(isDefined);
+    return res.length === 0 ? undefined : res;
+  }
+  return fct(value);
+}
+
+function withFetchRewrites(fetchResult: ResultMap | null | void, rewrites: FetchDataOutputRewrite[] | undefined): ResultMap | null | void {
+  if (!rewrites || !fetchResult) {
+    return fetchResult;
+  }
+
+  for (const rewrite of rewrites) {
+    let obj: any = fetchResult;
+    let i = 0;
+    while (obj && i < rewrite.path.length - 1) {
+      const p = rewrite.path[i++];
+      if (p.startsWith('... on ')) {
+        const typename = p.slice('... on '.length);
+        // Filter only objects that match the condition.
+        obj = applyOrMapRecursive(obj, (elt) => elt[TypeNameMetaFieldDef.name] === typename ? elt : undefined);
+      } else {
+        obj = applyOrMapRecursive(obj, (elt) => elt[p]);
+      }
+    }
+    if (obj) {
+      applyOrMapRecursive(obj, (elt) => {
+        if (typeof elt === 'object') {
+          // We need to move the value at path[i] to `renameKeyTo`.
+          const removedKey = rewrite.path[i];
+          elt[rewrite.renameKeyTo] = elt[removedKey];
+          elt[removedKey] = undefined;
+        }
+      });
+    }
+  }
+  return fetchResult;
+}
+
+function filterEntityRewrites(entity: Record<string, any>, rewrites: FetchDataOutputRewrite[] | undefined): FetchDataOutputRewrite[] | undefined {
+  if (!rewrites) {
+    return undefined;
+  }
+
+  const typename = entity[TypeNameMetaFieldDef.name] as string;
+  const typenameAsFragment = `... on ${typename}`;
+  return rewrites.map((r) => r.path[0] === typenameAsFragment ? { ...r, path: r.path.slice(1) } : undefined).filter(isDefined)
+}
+
 function updateRewrites(rewrites: FetchDataInputRewrite[] | undefined, pathElement: string): {
   updated: FetchDataInputRewrite[],
   completeRewrite?: any,

internals-js/src/__tests__/subgraphValidation.test.ts

@@ -538,34 +538,6 @@ describe('root types', () => {
   });
 });
 
-it('validates all implementations of interface field have same type if any has @external', () => {
-  const subgraph = gql`
-    type Query {
-      is: [I!]!
-    }
-
-    interface I {
-      f: Int
-    }
-
-    type T1 implements I {
-      f: Int
-    }
-
-    type T2 implements I {
-      f: Int!
-    }
-
-    type T3 implements I {
-      id: ID!
-      f: Int @external
-    }
-  `;
-    expect(buildForErrors(subgraph)).toStrictEqual([
-      ['INTERFACE_FIELD_IMPLEM_TYPE_MISMATCH', '[S] Some of the runtime implementations of interface field "I.f" are marked @external or have a @require ("T3.f") so all the implementations should use the same type (a current limitation of federation; see https://github.com/apollographql/federation/issues/1257), but "T1.f" and "T3.f" have type "Int" while "T2.f" has type "Int!".'],
-    ]);
-})
-
 describe('custom error message for misnamed directives', () => {
   it.each([
     { name: 'fed1', extraMsg: ' If so, note that it is a federation 2 directive but this schema is a federation 1 one. To be a federation 2 schema, it needs to @link to the federation specifcation v2.' },

internals-js/src/error.ts

@@ -395,11 +395,6 @@ const EXTERNAL_MISSING_ON_BASE = makeCodeDefinition(
   { addedIn: FED1_CODE },
 );
 
-const INTERFACE_FIELD_IMPLEM_TYPE_MISMATCH = makeCodeDefinition(
-  'INTERFACE_FIELD_IMPLEM_TYPE_MISMATCH',
-  'For an interface field, some of its concrete implementations have @external or @requires and there is difference in those implementations return type (which is currently not supported; see https://github.com/apollographql/federation/issues/1257)'
-);
-
 const INVALID_FIELD_SHARING = makeCodeDefinition(
   'INVALID_FIELD_SHARING',
   'A field that is non-shareable in at least one subgraph is resolved by multiple subgraphs.'
@@ -604,7 +599,6 @@ export const ERRORS = {
   ARGUMENT_DEFAULT_MISMATCH,
   EXTENSION_WITH_NO_BASE,
   EXTERNAL_MISSING_ON_BASE,
-  INTERFACE_FIELD_IMPLEM_TYPE_MISMATCH,
   INVALID_FIELD_SHARING,
   INVALID_SHAREABLE_USAGE,
   INVALID_LINK_DIRECTIVE_USAGE,
@@ -660,16 +654,18 @@ export const REMOVED_ERRORS = [
   ['REQUIRES_FIELDS_MISSING_ON_BASE', 'Fields in @requires can now be from any subgraph.'],
   ['REQUIRES_USED_ON_BASE', 'As there is not type ownership anymore, there is also no particular limitation as to which subgraph can use a @requires.'],
 
-  ['DUPLICATE_SCALAR_DEFINITION', 'As duplicate scalar definitions is invalid GraphQL, this will now be an error with code `INVALID_GRAPHQL`'],
-  ['DUPLICATE_ENUM_DEFINITION', 'As duplicate enum definitions is invalid GraphQL, this will now be an error with code `INVALID_GRAPHQL`'],
-  ['DUPLICATE_ENUM_VALUE', 'As duplicate enum values is invalid GraphQL, this will now be an error with code `INVALID_GRAPHQL`'],
+  ['DUPLICATE_SCALAR_DEFINITION', 'As duplicate scalar definitions is invalid GraphQL, this will now be an error with code `INVALID_GRAPHQL`.'],
+  ['DUPLICATE_ENUM_DEFINITION', 'As duplicate enum definitions is invalid GraphQL, this will now be an error with code `INVALID_GRAPHQL`.'],
+  ['DUPLICATE_ENUM_VALUE', 'As duplicate enum values is invalid GraphQL, this will now be an error with code `INVALID_GRAPHQL`.'],
 
-  ['ENUM_MISMATCH', 'Subgraph definitions for an enum are now merged by composition'],
+  ['ENUM_MISMATCH', 'Subgraph definitions for an enum are now merged by composition.'],
   ['VALUE_TYPE_NO_ENTITY', 'There is no strong different between entity and value types in the model (they are just usage pattern) and a type can have keys in one subgraph but not another.'],
-  ['VALUE_TYPE_UNION_TYPES_MISMATCH', 'Subgraph definitions for an union are now merged by composition'],
-  ['PROVIDES_FIELDS_SELECT_INVALID_TYPE', '@provides can now be used on field of interface, union and list types'],
-  ['RESERVED_FIELD_USED', 'This error was previously not correctly enforced: the _service and _entities, if present, were overridden; this is still the case'],
+  ['VALUE_TYPE_UNION_TYPES_MISMATCH', 'Subgraph definitions for an union are now merged by composition.'],
+  ['PROVIDES_FIELDS_SELECT_INVALID_TYPE', '@provides can now be used on field of interface, union and list types.'],
+  ['RESERVED_FIELD_USED', 'This error was previously not correctly enforced: the _service and _entities, if present, were overridden; this is still the case.'],
+
+  ['NON_REPEATABLE_DIRECTIVE_ARGUMENTS_MISMATCH', 'Since federation 2.1.0, the case this error used to cover is now a warning (with code `INCONSISTENT_NON_REPEATABLE_DIRECTIVE_ARGUMENTS`) instead of an error.'],
+  ['REQUIRES_FIELDS_HAS_ARGS', 'Since federation 2.1.1, using fields with arguments in a @requires is fully supported.'],
 
-  ['NON_REPEATABLE_DIRECTIVE_ARGUMENTS_MISMATCH', 'Since federation 2.1.0, the case this error used to cover is now a warning (with code `INCONSISTENT_NON_REPEATABLE_DIRECTIVE_ARGUMENTS`) instead of an error'],
-  ['REQUIRES_FIELDS_HAS_ARGS', 'Since federation 2.1.1, using fields with arguments in a @requires is fully supported'],
+  ['INTERFACE_FIELD_IMPLEM_TYPE_MISMATCH', 'This error was thrown by a validation introduced to avoid running into a known runtime bug. Since federation 2.3, the underlying runtime bug has been addressed and the validation/limitation was no longer necessary and has been removed.'],
 ];

internals-js/src/federation.ts

@@ -28,7 +28,7 @@ import {
   sourceASTs,
   UnionType,
 } from "./definitions";
-import { assert, joinStrings, MultiMap, printHumanReadableList, OrderedMap, mapValues } from "./utils";
+import { assert, MultiMap, printHumanReadableList, OrderedMap, mapValues } from "./utils";
 import { SDLValidationRule } from "graphql/validation/ValidationContext";
 import { specifiedSDLRules } from "graphql/validation/specifiedRules";
 import {
@@ -506,48 +506,6 @@ function validateInterfaceObjectsAreOnEntities(metadata: FederationMetadata, err
   }
 }
 
-/**
- * Register errors when, for an interface field, some of the implementations of that field are @external
- * _and_ not all of those field implementation have the same type (which otherwise allowed because field
- * implementation types can be a subtype of the interface field they implement).
- * This is done because if that is the case, federation may later generate invalid query plans (see details
- * on https://github.com/apollographql/federation/issues/1257).
- * This "limitation" will be removed when we stop generating invalid query plans for it.
- */
-function validateInterfaceRuntimeImplementationFieldsTypes(
-  itf: InterfaceType,
-  metadata: FederationMetadata,
-  errorCollector: GraphQLError[],
-): void {
-  const requiresDirective = federationMetadata(itf.schema())?.requiresDirective();
-  assert(requiresDirective, 'Schema should be a federation subgraph, but @requires directive not found');
-  const runtimeTypes = itf.possibleRuntimeTypes();
-  for (const field of itf.fields()) {
-    const withExternalOrRequires: FieldDefinition<ObjectType>[] = [];
-    const typeToImplems: MultiMap<string, FieldDefinition<ObjectType>> = new MultiMap();
-    const nodes: ASTNode[] = [];
-    for (const type of runtimeTypes) {
-      const implemField = type.field(field.name);
-      if (!implemField) continue;
-      if (implemField.sourceAST) {
-        nodes.push(implemField.sourceAST);
-      }
-      if (metadata.isFieldExternal(implemField) || implemField.hasAppliedDirective(requiresDirective)) {
-        withExternalOrRequires.push(implemField);
-      }
-      const returnType = implemField.type!;
-      typeToImplems.add(returnType.toString(), implemField);
-    }
-    if (withExternalOrRequires.length > 0 && typeToImplems.size > 1) {
-      const typeToImplemsArray = [...typeToImplems.entries()];
-      errorCollector.push(ERRORS.INTERFACE_FIELD_IMPLEM_TYPE_MISMATCH.err(
-        `Some of the runtime implementations of interface field "${field.coordinate}" are marked @external or have a @require (${withExternalOrRequires.map(printFieldCoordinate)}) so all the implementations should use the same type (a current limitation of federation; see https://github.com/apollographql/federation/issues/1257), but ${formatFieldsToReturnType(typeToImplemsArray[0])} while ${joinStrings(typeToImplemsArray.slice(1).map(formatFieldsToReturnType), ' and ')}.`,
-        { nodes },
-      ));
-    }
-  }
-}
-
 function validateShareableNotRepeatedOnSameDeclaration(
   element: ObjectType | FieldDefinition<ObjectType>,
   metadata: FederationMetadata,
@@ -589,13 +547,6 @@ function validateShareableNotRepeatedOnSameDeclaration(
   }
 }
 
-
-const printFieldCoordinate = (f: FieldDefinition<CompositeType>): string => `"${f.coordinate}"`;
-
-function formatFieldsToReturnType([type, implems]: [string, FieldDefinition<ObjectType>[]]) {
-  return `${joinStrings(implems.map(printFieldCoordinate))} ${implems.length == 1 ? 'has' : 'have'} type "${type}"`;
-}
-
 export class FederationMetadata {
   private _externalTester?: ExternalTester;
   private _sharingPredicate?: (field: FieldDefinition<CompositeType>) => boolean;
@@ -1034,10 +985,6 @@ export class FederationBlueprint extends SchemaBlueprint {
       }
     }
 
-    for (const itf of schema.interfaceTypes()) {
-      validateInterfaceRuntimeImplementationFieldsTypes(itf, metadata, errorCollector);
-    }
-
     // While @shareable is "repeatable", this is only so one can use it on both a main
     // type definition _and_ possible other type extensions. But putting 2 @shareable
     // on the same type definition or field is both useless, and suggest some miscomprehension,

internals-js/src/operations.ts

@@ -132,6 +132,15 @@ export class Field<TArgs extends {[key: string]: any} = {[key: string]: any}> ex
     return newField;
   }
 
+  withUpdatedAlias(newAlias: string | undefined): Field<TArgs> {
+    const newField = new Field<TArgs>(this.definition, this.args, this.variableDefinitions, newAlias);
+    for (const directive of this.appliedDirectives) {
+      newField.applyDirective(directive.definition!, directive.arguments());
+    }
+    this.copyAttachementsTo(newField);
+    return newField;
+  }
+
   appliesTo(type: ObjectType | InterfaceType): boolean {
     const definition = type.field(this.name);
     return !!definition && this.selects(definition);
@@ -981,6 +990,22 @@ export class SelectionSet extends Freezable<SelectionSet> {
     return this._cachedSelections;
   }
 
+  fieldsInSet(): { path: string[], field: FieldSelection, directParent: SelectionSet }[] {
+    const fields = new Array<{ path: string[], field: FieldSelection, directParent: SelectionSet }>();
+    for (const selection of this.selections()) {
+      if (selection.kind === 'FieldSelection') {
+        fields.push({ path: [], field: selection, directParent: this });
+      } else {
+        const condition = selection.element().typeCondition;
+        const header = condition ? [`... on ${condition}`] : [];
+        for (const { path, field, directParent } of selection.selectionSet.fieldsInSet()) {
+          fields.push({ path: header.concat(path), field, directParent });
+        }
+      }
+    }
+    return fields;
+  }
+
   usedVariables(): Variables {
     let variables: Variables = [];
     for (const byResponseName of this._selections.values()) {
@@ -1162,6 +1187,23 @@ export class SelectionSet extends Freezable<SelectionSet> {
     return toAdd;
   }
 
+  /**
+   * If this selection contains a selection of a field with provided response name at top level, removes it.
+   *
+   * @return whether a selection was removed.
+   */
+  removeTopLevelField(responseName: string): boolean {
+    // It's a bug to try to remove from a frozen selection set
+    assert(!this.isFrozen(), () => `Cannot remove from frozen selection: ${this}`);
+
+    const wasRemoved = this._selections.delete(responseName);
+    if (wasRemoved) {
+      --this._selectionCount;
+      this._cachedSelections = undefined;
+    }
+    return wasRemoved;
+  }
+
   addPath(path: OperationPath, onPathEnd?: (finalSelectionSet: SelectionSet | undefined) => void) {
     let previousSelections: SelectionSet = this;
     let currentSelections: SelectionSet | undefined = this;
@@ -1642,6 +1684,10 @@ export class FieldSelection extends Freezable<FieldSelection> {
     return new FieldSelection(this.field, newSubSelection);
   }
 
+  withUpdatedField(newField: Field<any>): FieldSelection {
+    return new FieldSelection(newField, this.selectionSet);
+  }
+
   equals(that: Selection): boolean {
     if (this === that) {
       return true;

query-planner-js/src/QueryPlan.ts

@@ -41,6 +41,8 @@ export interface FetchNode {
   operationDocumentNode?: DocumentNode;
   // Optionally describe a number of "rewrites" that query plan executors should apply to the data that is sent as input of this fetch.
   inputRewrites?: FetchDataInputRewrite[];
+  // Similar, but for optional "rewrites" to apply to the data that received from a fetch (and before it is apply to the current in-memory results).
+  outputRewrites?: FetchDataOutputRewrite[];
 }
 
 /**
@@ -53,6 +55,15 @@ export interface FetchNode {
  */
 export type FetchDataInputRewrite = FetchDataValueSetter;
 
+/**
+ * The type of rewrites currently supported on the output data of fetches.
+ *
+ * A rewrite usually identifies some subpart of the ouput data and some action to perform on that subpart.
+ * Note that ouput rewrites should only impact the outputs of the fetch they are applied to (meaning that
+ * the rewrites must be applied before the data from the fetch is merged to the  current in-memory result).
+ */
+export type FetchDataOutputRewrite = FetchDataKeyRenamer;
+
 /**
  * A rewrite that sets a value at the provided path of the data it is applied to.
  */
@@ -68,6 +79,14 @@ export interface FetchDataValueSetter {
   setValueTo: any,
 }
 
+export interface FetchDataKeyRenamer {
+  kind: 'KeyRenamer'
+  // Same format as in `FetchDataValueSetter`, but this renames the key at the very end of this path to the
+  // name from `renameKeyTo`.
+  path: string[],
+  renameKeyTo: string,
+}
+
 export interface FlattenNode {
   kind: 'Flatten';
   path: ResponsePath;

query-planner-js/src/__tests__/buildPlan.test.ts

@@ -1255,7 +1255,8 @@ describe('@requires', () => {
 
     const plan = queryPlanner.buildQueryPlan(operation);
     // The main goal of this test is to show that the 2 @requires for `f` gets handled seemlessly
-    // into the same fetch group.
+    // into the same fetch group. But note that because the type for `f` differs, the 2nd instance
+    // gets aliased (or the fetch would be invalid graphQL).
     expect(plan).toMatchInlineSnapshot(`
       QueryPlan {
         Sequence {
@@ -1274,7 +1275,7 @@ describe('@requires', () => {
                 ... on T3 {
                   __typename
                   id
-                  f
+                  f__alias_0: f
                 }
               }
             }