Fix potential normalization assertion error for fragment on abstract types (#2725)

An abstract type (interface or union) can have different runtime types
in different subgraphs. For instance, a type `T` may implement some
interface `I` in subgraph A, but not in subgraph B _even_ if `I` is
otherwise defined in subgraph B (admittedly something not to be abused,
but it can be convenient as a temporary state when evolving schema).

For named fragment, this can create a case where a fragment on an
abstract type can be "rebased" on a subgraph, but where some of its
application (spread) in other fragments are invalid due to being
use in the context of a type that intersect the abstrat type in the
supergraph but not in the particular subgraph. When that's the case,
the invalid spread (for the subgraph) needs to be expanded, and that
expansion properly "rebased", but the code was not handling that case
at all. Instead it kept the invalid spread, and this led to some
later assertion errors.

Fixes #2721.

Author: Sylvain Lebresne

.changeset/nice-seahorses-cheer.md

@@ -0,0 +1,10 @@
+---
+"@apollo/query-planner": patch
+"@apollo/federation-internals": patch
+---
+
+Fix potential assertion error for named fragment on abstract types when the abstract type does not have the same
+possible runtime types in all subgraphs.
+
+The error manifested itself during query planning with an error message of the form `Cannot normalize X at Y ...`.
+  

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

@@ -3542,9 +3542,7 @@ describe('named fragment rebasing on subgraphs', () => {
         t {
           x
           y
-          ... on T {
-            z
-          }
+          z
         }
       }
     `);

internals-js/src/operations.ts

@@ -1406,7 +1406,10 @@ export class NamedFragments {
         return undefined;
       }
 
-      const rebasedSelection = fragment.selectionSet.rebaseOn({ parentType: rebasedType, fragments: newFragments, errorIfCannotRebase: false });
+      let rebasedSelection = fragment.selectionSet.rebaseOn({ parentType: rebasedType, fragments: newFragments, errorIfCannotRebase: false });
+      // Rebasing can leave some inefficiencies in some case (particularly when a spread has to be "expanded", see `FragmentSpreadSelection.rebaseOn`),
+      // so we do a top-level normalization to keep things clean.
+      rebasedSelection = rebasedSelection.normalize({ parentType: rebasedType });
       return this.selectionSetIsWorthUsing(rebasedSelection)
         ? new NamedFragmentDefinition(schema, fragment.name, rebasedType).setSelectionSet(rebasedSelection)
         : undefined;
@@ -1421,9 +1424,11 @@ export class NamedFragments {
         // dependency order, we know that `newFragments` will have every fragments that should be
         // kept/not expanded.
         const updatedSelectionSet = fragment.selectionSet.expandFragments(newFragments);
+        // Note that if we did expanded some fragments (the updated selection is not the original one), then the
+        // results may not be fully normalized, so we do it to be sure.
         return updatedSelectionSet === fragment.selectionSet
           ? fragment
-          : fragment.withUpdatedSelectionSet(updatedSelectionSet);
+          : fragment.withUpdatedSelectionSet(updatedSelectionSet.normalize({ parentType: updatedSelectionSet.parentType}));
       } else {
         return undefined;
       }
@@ -3558,7 +3563,8 @@ class FragmentSpreadSelection extends FragmentSelection {
     // If we're rebasing on a _different_ schema, then we *must* have fragments, since reusing
     // `this.fragments` would be incorrect. If we're on the same schema though, we're happy to default
     // to `this.fragments`.
-    assert(fragments || this.parentType.schema() === parentType.schema(), `Must provide fragments is rebasing on other schema`);
+    const rebaseOnSameSchema = this.parentType.schema() === parentType.schema();
+    assert(fragments || rebaseOnSameSchema, `Must provide fragments is rebasing on other schema`);
     const newFragments = fragments ?? this.fragments;
     const namedFragment = newFragments.get(this.namedFragment.name);
     // If we're rebasing on another schema (think a subgraph), then named fragments will have been rebased on that, and some
@@ -3569,6 +3575,31 @@ class FragmentSpreadSelection extends FragmentSelection {
       validate(!errorIfCannotRebase, () => `Cannot rebase ${this.toString(false)} if it isn't part of the provided fragments`);
       return undefined;
     }
+
+    // Lastly, if we rebase on a different schema, it's possible the fragment type does not intersect the
+    // parent type. For instance, the parent type could be some object type T while the fragment is an
+    // interface I, and T may implement I in the supergraph, but not in a particular subgraph (of course,
+    // if I don't exist at all in the subgraph, then we'll have exited above, but I may exist in the
+    // subgraph, just not be implemented by T for some reason). In that case, we can't reuse the fragment
+    // as its spread is essentially invalid in that position, so we have to replace it by the expansion
+    // of that fragment, which we rebase on the parentType (which in turn, will remove anythings within
+    // the fragment selection that needs removing, potentially everything).
+    if (!rebaseOnSameSchema && !runtimeTypesIntersects(parentType, namedFragment.typeCondition)) {
+      // Note that we've used the rebased `namedFragment` to check the type intersection because we needed to
+      // compare runtime types "for the schema we're rebasing into". But now that we're deciding to not reuse
+      // this rebased fragment, what we rebase is the selection set of the non-rebased fragment. And that's
+      // important because the very logic we're hitting here may need to happen inside the rebase do the
+      // fragment selection, but that logic would not be triggered if we used the rebased `namedFragment` since
+      // `rebaseOnSameSchema` would then be 'true'.
+      const expanded = this.namedFragment.selectionSet.rebaseOn({ parentType, fragments, errorIfCannotRebase });
+      // In theory, we could return the selection set directly, but making `Selection.rebaseOn` sometimes
+      // return a `SelectionSet` complicate things quite a bit. So instead, we encapsulate the selection set
+      // in an "empty" inline fragment. This make for non-really-optimal selection sets in the (relatively
+      // rare) case where this is triggered, but in practice this "inefficiency" is removed by future calls
+      // to `normalize`.
+      return expanded.isEmpty() ? undefined : new InlineFragmentSelection(new FragmentElement(parentType), expanded);
+    }
+
     return new FragmentSpreadSelection(
       parentType,
       newFragments,