Fix assertion error with overlapping fragments and subtyping (#2594)

When checking for name fragments to reuse, when some fields implementing
interfaces use subtyping (meaning that the type of the field
implementation is a sub-type of the type declared for that field in the
interface), an assertion error of the form `Cannot add selection of
field X to selection set of parent type Y` was sometimes raised (see
the comments in the commit for mode details).

Fixes #2592.

Author: Sylvain Lebresne

.changeset/brave-dryers-drop.md

@@ -0,0 +1,9 @@
+---
+"@apollo/query-planner": patch
+"@apollo/federation-internals": patch
+---
+
+Fix assertion error in some overlapping fragment cases. In some cases, when fragments overlaps on some sub-selections
+and some interface field implementation relied on sub-typing, an assertion error could be raised with a message of
+the form `Cannot add selection of field X to selection set of parent type Y` and this fixes this problem.
+  

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

@@ -1,10 +1,11 @@
 import {
+    CompositeType,
   defaultRootName,
   Schema,
   SchemaRootKind,
 } from '../../dist/definitions';
 import { buildSchema } from '../../dist/buildSchema';
-import { MutableSelectionSet, Operation, operationFromDocument, parseOperation } from '../../dist/operations';
+import { MutableSelectionSet, Operation, operationFromDocument, parseOperation, parseSelectionSet } from '../../dist/operations';
 import './matchers';
 import { DocumentNode, FieldNode, GraphQLError, Kind, OperationDefinitionNode, OperationTypeNode, SelectionNode, SelectionSetNode } from 'graphql';
 
@@ -1264,3 +1265,62 @@ describe('unsatisfiable branches removal', () => {
     expect(withoutUnsatisfiableBranches(input)).toBe(output);
   });
 });
+
+test('contains ignores unecessary fragments even when subtyping is involved', () => {
+  const schema = parseSchema(`
+      type Query {
+        a: A!
+      }
+
+      interface IA1 {
+        b: IB1!
+      }
+
+      interface IA2 {
+        b: IB2!
+      }
+
+      type A implements IA1 & IA2 {
+        b: B!
+      }
+
+      interface IB1 {
+        v1: Int!
+      }
+
+      interface IB2 {
+        v2: Int!
+      }
+
+      type B implements IB1 & IB2 {
+        v1: Int!
+        v2: Int!
+      }
+  `);
+
+  const typeA = schema.type('A') as CompositeType;
+
+  const s1 = parseSelectionSet({
+    parentType: typeA,
+    source: '{ b { v1 v2 } }'
+  });
+
+  const s2 = parseSelectionSet({
+    parentType: typeA,
+    source: '{ ... on IA1 { b { v1 } } ... on IA2 { b { v2 } } }'
+  });
+
+  // Here, A is a concrete type, and IA1 and IA2 are just 2 of its interfaces, so
+  //   a { ... on IA1 { b { v1 } } ... on IA2 { b { v2 } } }
+  // is basically exactly the same as:
+  //   a { b { v1 } b { v2 } }
+  // which is the same as:
+  //   a { b { v1 v2 } }
+  // and that is why we want the `contains` below to work (note that a simple `contains`
+  // that doesn't handle this kind of subtlety could have its use too, it would just be
+  // a different contract, but we want "our" `contains` to handle this because of named
+  // fragments "reconstruction" where that kind of subtlety arises).
+  //
+  // Here, the added subtlety is that there is interface subtyping involved too.
+  expect(s2.contains(s1)).toBeTruthy();
+});

internals-js/src/operations.ts

@@ -46,6 +46,7 @@ import {
   isLeafType,
   Variables,
   isObjectType,
+  NamedType,
 } from "./definitions";
 import { isInterfaceObjectType } from "./federation";
 import { ERRORS } from "./error";
@@ -152,7 +153,11 @@ export class Field<TArgs extends {[key: string]: any} = {[key: string]: any}> ex
   }
 
   isLeafField(): boolean {
-    return isLeafType(baseType(this.definition.type!));
+    return isLeafType(this.baseType());
+  }
+
+  baseType(): NamedType {
+    return baseType(this.definition.type!);
   }
 
   withUpdatedDefinition(newDefinition: FieldDefinition<any>): Field<TArgs> {
@@ -674,7 +679,7 @@ export function concatOperationPaths(head: OperationPath, tail: OperationPath):
 
 function isUselessFollowupElement(first: OperationElement, followup: OperationElement, conditionals: Directive<any, any>[]): boolean {
   const typeOfFirst = first.kind === 'Field'
-    ? baseType(first.definition.type!)
+    ? first.baseType()
     : first.typeCondition;
 
   // The followup is useless if it's a fragment (with no directives we would want to preserve) whose type
@@ -1430,7 +1435,73 @@ export class SelectionSet {
     for (const selection of selections) {
       mergedSubselections.add(selection.selectionSet!);
     }
-    return first.withUpdatedSelectionSet(mergedSubselections.toSelectionSet(first.selectionSet.parentType));
+
+    // We know all the `selections` are basically for the same element (same field or same inline fragment),
+    // and we want to return a single selection with the merged selections. There is a subtlety regarding
+    // the parent type of that merged selection however: we cannot safely rely on the parent type of any
+    // of the individual selections, because this can be incorrect. Let's illustrate.
+    // Consider that we have:
+    // ```graphql
+    //   type Query {
+    //     a: A!
+    //   }
+    //
+    //   interface IA1 {
+    //     b: IB1!
+    //   }
+    //
+    //   interface IA2 {
+    //     b: IB2!
+    //   }
+    //
+    //   type A implements IA1 & IA2 {
+    //     b: B!
+    //   }
+    //
+    //   interface IB1 {
+    //     v1: Int!
+    //   }
+    //
+    //   interface IB2 {
+    //     v2: Int!
+    //   }
+    //
+    //   type B implements IB1 & IB2 {
+    //     v1: Int!
+    //     v2: Int!
+    //   }
+    // ```
+    // and suppose that we're trying to check if selection set:
+    //  maybeSuperset = { ... on IA1 { b { v1 } } ... on IA2 { b { v2 } } }  // (parent type A)
+    // contains selection set:
+    //  maybeSubset   = { b { v1 v2 } }                                      // (parent type A)
+    //
+    // In that case, the `contains` method below will call this function with the 2 sub-selections
+    // from `maybeSuperset`, but with the unecessary interface fragment removed (reminder that the
+    // parent type is `A`, so the "casts" into the interfaces are semantically useless).
+    //
+    // And so in that case, the argument to this method will be:
+    //   [ b { v1 } (parent type IA1), b { v2 } (parent type IA2) ]
+    // but then, the sub-selection `{ v1 }` of the 1st value will have parent type IB1,
+    //       and the sub-selection `{ v2 }` of the 2nd value will have parent type IB2,
+    // neither of which work for the merge sub-selection.
+    //
+    // Instead, we want to use as parent type the type of field `b` the parent type of `this`
+    // (which is `maybeSupeset` in our example). Which means that we want to use type `B` for
+    // the sub-selection, which is now guaranteed to work (or `maybeSupergerset` wouldn't have
+    // been valid).
+    //
+    // Long story short, we get that type by rebasing any of the selection element (we use the
+    // first as we have it) on `this.parentType`, which gives use the element we want, and we
+    // use the type of that for the sub-selection.
+
+    if (first.kind === 'FieldSelection') {
+      const rebasedField = first.element.rebaseOn(this.parentType);
+      return new FieldSelection(rebasedField, mergedSubselections.toSelectionSet(rebasedField.baseType() as CompositeType));
+    } else {
+      const rebasedFragment = first.element.rebaseOn(this.parentType);
+      return new InlineFragmentSelection(rebasedFragment, mergedSubselections.toSelectionSet(rebasedFragment.castedType()));
+    }
   }
 
   contains(that: SelectionSet): boolean {
@@ -1790,7 +1861,7 @@ function makeSelection(parentType: CompositeType, updates: SelectionUpdate[], fr
   }
 
   const element = updateElement(first).rebaseOn(parentType);
-  const subSelectionParentType = element.kind === 'Field' ? baseType(element.definition.type!) : element.castedType();
+  const subSelectionParentType = element.kind === 'Field' ? element.baseType() : element.castedType();
   if (!isCompositeType(subSelectionParentType)) {
     // This is a leaf, so all updates should correspond ot the same field and we just use the first.
     return selectionOfElement(element);
@@ -2120,7 +2191,7 @@ export class FieldSelection extends AbstractSelection<Field<any>, undefined, Fie
 
   optimize(fragments: NamedFragments): Selection {
     let optimizedSelection = this.selectionSet ? this.selectionSet.optimizeSelections(fragments) : undefined;
-    const fieldBaseType = baseType(this.element.definition.type!);
+    const fieldBaseType = this.element.baseType();
     if (isCompositeType(fieldBaseType) && optimizedSelection) {
       const optimized = this.tryOptimizeSubselectionWithFragments({
         parentType: fieldBaseType,
@@ -2213,7 +2284,7 @@ export class FieldSelection extends AbstractSelection<Field<any>, undefined, Fie
       return this.withUpdatedElement(rebasedElement);
     }
 
-    const rebasedBase = baseType(rebasedElement.definition.type!);
+    const rebasedBase = rebasedElement.baseType();
     if (rebasedBase === this.selectionSet.parentType) {
       return this.withUpdatedElement(rebasedElement);
     }
@@ -2279,7 +2350,7 @@ export class FieldSelection extends AbstractSelection<Field<any>, undefined, Fie
       return this;
     }
 
-    const base = baseType(this.element.definition.type!)
+    const base = this.element.baseType()
     assert(isCompositeType(base), () => `Field ${this.element} should not have a sub-selection`);
     const trimmed = (options?.recursive ?? true) ? this.mapToSelectionSet((s) => s.trimUnsatisfiableBranches(base)) : this;
     // In rare caes, it's possible that everything in the sub-selection was trimmed away and so the

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

@@ -5672,3 +5672,79 @@ test('handles types with no common supertype at the same "mergeAt"', () => {
     }
   `);
 });
+
+test('does not error out handling fragments when interface subtyping is involved', () => {
+  // This test essentially make sure the issue in https://github.com/apollographql/federation/issues/2592
+  // is resolved.
+  const subgraph1 = {
+    name: 'Subgraph1',
+    typeDefs: gql`
+      type Query {
+        a: A!
+      }
+
+      interface IA {
+        b: IB!
+      }
+
+      type A implements IA {
+        b: B!
+      }
+
+      interface IB {
+        v1: Int!
+      }
+
+      type B implements IB {
+        v1: Int!
+        v2: Int!
+      }
+    `
+  }
+
+  const [api, queryPlanner] = composeAndCreatePlanner(subgraph1);
+  const operation = operationFromDocument(api, gql`
+    {
+      a {
+        ...F1
+        ...F2
+        ...F3
+      }
+    }
+
+    fragment F1 on A {
+      b {
+        v2
+      }
+    }
+
+    fragment F2 on IA {
+      b {
+        v1
+      }
+    }
+
+    fragment F3 on IA {
+      b {
+        __typename
+      }
+    }
+  `);
+
+  const plan = queryPlanner.buildQueryPlan(operation);
+  expect(plan).toMatchInlineSnapshot(`
+    QueryPlan {
+      Fetch(service: "Subgraph1") {
+        {
+          a {
+            b {
+              __typename
+              v1
+              v2
+            }
+          }
+        }
+      },
+    }
+  `);
+});