From cb50757c5fef9f07c3392cfd1ee0410c5a82fe4c Mon Sep 17 00:00:00 2001
From: Duckki Oe <duckki.oe@apollographql.com>
Date: Mon, 18 Mar 2024 15:56:01 -0700
Subject: [PATCH] fix: fixed invalid subgraph queries when
 reuseQueryFragments=true

- updated computeExpandedSelectionSetAtType function not to trim fragment's validators if the fragment's type condition is not an object type.
- This change is necessary because `FieldsInSetCanMerge` rule is more restrictive in that case.
- The untrimmed validator should avoid generiating invalid queries, but it may be less efficient.

https://github.com/apollographql/federation/issues/2952
---
 .changeset/invalid-reused-fragment.md         |  7 ++
 internals-js/src/__tests__/operations.test.ts | 67 ++++++++++++++++++-
 internals-js/src/operations.ts                |  8 +++
 3 files changed, 81 insertions(+), 1 deletion(-)
 create mode 100644 .changeset/invalid-reused-fragment.md

diff --git a/.changeset/invalid-reused-fragment.md b/.changeset/invalid-reused-fragment.md
new file mode 100644
index 0000000000..6fc78c2709
--- /dev/null
+++ b/.changeset/invalid-reused-fragment.md
@@ -0,0 +1,7 @@
+---
+"@apollo/query-planner": patch
+"@apollo/composition": patch
+"@apollo/federation-internals": patch
+---
+
+Fix a query planning bug where invalid subgraph queries are generated with `reuseQueryFragments` set true. ([#2952](https://github.com/apollographql/federation/issues/2952))
diff --git a/internals-js/src/__tests__/operations.test.ts b/internals-js/src/__tests__/operations.test.ts
index fe6f9fee2d..0206a6bc4e 100644
--- a/internals-js/src/__tests__/operations.test.ts
+++ b/internals-js/src/__tests__/operations.test.ts
@@ -3100,6 +3100,38 @@ describe('named fragment selection set restrictions at type', () => {
     return frag.expandedSelectionSetAtType(type);
   }
 
+  test('for fragment on object types', () => {
+    const schema = parseSchema(`
+      type Query {
+        t1: T1
+      }
+
+      type T1 {
+        x: Int
+        y: Int
+      }
+    `);
+
+    const operation = parseOperation(schema, `
+      {
+        t1 {
+          ...FonT1
+        }
+      }
+
+      fragment FonT1 on T1 {
+        x
+        y
+      }
+    `);
+
+    const frag = operation.fragments?.get('FonT1')!;
+
+    let { selectionSet, validator } = expandAtType(frag, schema, 'T1');
+    expect(selectionSet.toString()).toBe('{ x y }');
+    expect(validator?.toString()).toBeUndefined();
+  });
+
   test('for fragment on interfaces', () => {
     const schema = parseSchema(`
       type Query {
@@ -3159,10 +3191,24 @@ describe('named fragment selection set restrictions at type', () => {
 
     let { selectionSet, validator } = expandAtType(frag, schema, 'I1');
     expect(selectionSet.toString()).toBe('{ x ... on T1 { x } ... on T2 { x } ... on I2 { x } ... on I3 { x } }');
-    expect(validator?.toString()).toBeUndefined();
+    // Note: Due to `FieldsInSetCanMerge` rule, we can't use trimmed validators for
+    // fragments on non-object types.
+    expect(validator?.toString()).toMatchString(`
+      {
+        x: [
+          I1.x
+          T1.x
+          T2.x
+          I2.x
+          I3.x
+        ]
+      }
+      `);
 
     ({ selectionSet, validator } = expandAtType(frag, schema, 'T1'));
     expect(selectionSet.toString()).toBe('{ x }');
+    // Note: Fragments on non-object types always have the full validator and thus
+    // results in the same validator regardless of the type they are expanded at.
     // Note: one could remark that having `T1.x` in the `validator` below is a tad weird: this is
     // because in this case `normalized` removed that fragment and so when we do `normalized.minus(original)`,
     // then it shows up. It a tad difficult to avoid this however and it's ok for what we do (`validator`
@@ -3170,6 +3216,7 @@ describe('named fragment selection set restrictions at type', () => {
     expect(validator?.toString()).toMatchString(`
       {
         x: [
+          I1.x
           T1.x
           T2.x
           I2.x
@@ -3239,8 +3286,16 @@ describe('named fragment selection set restrictions at type', () => {
     // this happens due to the "lifting" of selection mentioned above, is a bit hard to avoid,
     // and is essentially harmess (it may result in a bit more cpu cycles in some cases but
     // that is likely negligible).
+    // Note: Due to `FieldsInSetCanMerge` rule, we can't use trimmed validators for
+    // fragments on non-object types.
     expect(validator?.toString()).toMatchString(`
       {
+        x: [
+          T1.x
+        ]
+        z: [
+          T2.z
+        ]
         y: [
           T1.y
         ]
@@ -3252,8 +3307,13 @@ describe('named fragment selection set restrictions at type', () => {
 
     ({ selectionSet, validator } = expandAtType(frag, schema, 'U2'));
     expect(selectionSet.toString()).toBe('{ ... on T1 { x y } }');
+    // Note: Fragments on non-object types always have the full validator and thus
+    // results in the same validator regardless of the type they are expanded at.
     expect(validator?.toString()).toMatchString(`
       {
+        x: [
+          T1.x
+        ]
         z: [
           T2.z
         ]
@@ -3268,11 +3328,16 @@ describe('named fragment selection set restrictions at type', () => {
 
     ({ selectionSet, validator } = expandAtType(frag, schema, 'U3'));
     expect(selectionSet.toString()).toBe('{ ... on T2 { z w } }');
+    // Note: Fragments on non-object types always have the full validator and thus
+    // results in the same validator regardless of the type they are expanded at.
     expect(validator?.toString()).toMatchString(`
       {
         x: [
           T1.x
         ]
+        z: [
+          T2.z
+        ]
         y: [
           T1.y
         ]
diff --git a/internals-js/src/operations.ts b/internals-js/src/operations.ts
index a656a578ec..71ab8b3e08 100644
--- a/internals-js/src/operations.ts
+++ b/internals-js/src/operations.ts
@@ -1219,6 +1219,14 @@ export class NamedFragmentDefinition extends DirectiveTargetElement<NamedFragmen
     const expandedSelectionSet = this.expandedSelectionSet();
     const selectionSet = expandedSelectionSet.normalize({ parentType: type });
 
+    if( ! isObjectType(this.typeCondition) ) {
+      // When the type condition of the fragment is not an object type, the `FieldsInSetCanMerge` rule is more
+      // restrictive and any fields can create conflicts. Thus, we have to use the full validator in this case.
+      // (see https://github.com/graphql/graphql-spec/issues/1085 for details.)
+      const validator = FieldsConflictValidator.build(expandedSelectionSet);
+      return { selectionSet, validator };
+    }
+
     // Note that `trimmed` is the difference of 2 selections that may not have been normalized on the same parent type,
     // so in practice, it is possible that `trimmed` contains some of the selections that `selectionSet` contains, but
     // that they have been simplified in `selectionSet` in such a way that the `minus` call does not see it. However,