Fix case where reusing fragments can still lead to invalid selection …

…with conflicting fields (#2740)

Trying to reuse fragments, if done without specific case, can in some context result in invalid
selections due to fields conflicting. It is to avoid that problem that #2619 introduced the
`FieldsConflictValidator` mechanism (later improved by #2635).

Unfortunately, in some fairly specific setups with nested fragments, the
code was missing some data in the validation mentioned above, which led
to still having case where a subgraph fetch may be invalid due to some
fields (within reusing fragments) conflicting at some point of the
query.

This commit fix that issue by ensuring we take everything we should into
account when doing the aforementioned validation.

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

@@ -1988,6 +1988,142 @@ describe('fragments optimization', () => {
         }
       `);
     });
+
+    test('due to the trimmed selection of nested fragments', () => {
+      const schema = parseSchema(`
+        type Query {
+          u1: U
+          u2: U
+          u3: U
+        }
+
+        union U = S | T
+
+        type T  {
+          id: ID!
+          vt: Int
+        }
+
+        interface I {
+          vs: Int
+        }
+
+        type S implements I {
+          vs: Int!
+        }
+      `);
+      const gqlSchema = schema.toGraphQLJSSchema();
+
+      const operation = parseOperation(schema, `
+        {
+          u1 {
+            ...F1
+          }
+          u2 {
+            ...F3
+          }
+          u3 {
+            ...F3
+          }
+        }
+
+        fragment F1 on U {
+           ... on S {
+             __typename
+             vs
+           }
+           ... on T {
+             __typename
+             vt
+           }
+        }
+
+        fragment F2 on T {
+           __typename
+           vt
+        }
+
+        fragment F3 on U {
+           ... on I {
+             vs
+           }
+           ...F2
+        }
+      `);
+      expect(validate(gqlSchema, parse(operation.toString()))).toStrictEqual([]);
+
+      const withoutFragments = operation.expandAllFragments();
+      expect(withoutFragments.toString()).toMatchString(`
+        {
+          u1 {
+            ... on S {
+              __typename
+              vs
+            }
+            ... on T {
+              __typename
+              vt
+            }
+          }
+          u2 {
+            ... on I {
+              vs
+            }
+            ... on T {
+              __typename
+              vt
+            }
+          }
+          u3 {
+            ... on I {
+              vs
+            }
+            ... on T {
+              __typename
+              vt
+            }
+          }
+        }
+      `);
+
+      // We use `mapToExpandedSelectionSets` with a no-op mapper because this will still expand the selections
+      // and re-optimize them, which 1) happens to match what happens in the query planner and 2) is necessary
+      // for reproducing a bug that this test was initially added to cover.
+      const newFragments = operation.fragments!.mapToExpandedSelectionSets((s) => s);
+      const optimized = withoutFragments.optimize(newFragments, 2);
+      expect(validate(gqlSchema, parse(optimized.toString()))).toStrictEqual([]);
+
+      expect(optimized.toString()).toMatchString(`
+        fragment F3 on U {
+          ... on I {
+            vs
+          }
+          ... on T {
+            __typename
+            vt
+          }
+        }
+
+        {
+          u1 {
+            ... on S {
+              __typename
+              vs
+            }
+            ... on T {
+              __typename
+              vt
+            }
+          }
+          u2 {
+            ...F3
+          }
+          u3 {
+            ...F3
+          }
+        }
+      `);
+    });
   });
 
   test('does not leave unused fragments', () => {
@@ -3094,12 +3230,25 @@ describe('named fragment selection set restrictions at type', () => {
 
     const frag = operation.fragments?.get('FonU1')!;
 
-    // Note that with unions, the fragments inside the unions can be "lifted" and so they everyting normalize to just the
+    // Note that with unions, the fragments inside the unions can be "lifted" and so that everything normalizes to just the
     // possible runtimes.
 
     let { selectionSet, validator } = expandAtType(frag, schema, 'U1');
     expect(selectionSet.toString()).toBe('{ ... on T1 { x y } ... on T2 { z w } }');
-    expect(validator?.toString()).toBeUndefined();
+    // Similar remarks than on interfaces (the validator is strictly speaking not necessary, but
+    // 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).
+    expect(validator?.toString()).toMatchString(`
+      {
+        y: [
+          T1.y
+        ]
+        w: [
+          T2.w
+        ]
+      }
+    `);
 
     ({ selectionSet, validator } = expandAtType(frag, schema, 'U2'));
     expect(selectionSet.toString()).toBe('{ ... on T1 { x y } }');
@@ -3108,6 +3257,9 @@ describe('named fragment selection set restrictions at type', () => {
         z: [
           T2.z
         ]
+        y: [
+          T1.y
+        ]
         w: [
           T2.w
         ]
@@ -3124,6 +3276,9 @@ describe('named fragment selection set restrictions at type', () => {
         y: [
           T1.y
         ]
+        w: [
+          T2.w
+        ]
       }
     `);
 
@@ -3135,12 +3290,12 @@ describe('named fragment selection set restrictions at type', () => {
         x: [
           T1.x
         ]
-        y: [
-          T1.y
-        ]
         z: [
           T2.z
         ]
+        y: [
+          T1.y
+        ]
         w: [
           T2.w
         ]

internals-js/src/operations.ts

@@ -1073,13 +1073,6 @@ export class NamedFragmentDefinition extends DirectiveTargetElement<NamedFragmen
     return this._selectionSet;
   }
 
-  expandedSelectionSet(): SelectionSet {
-    if (!this._expandedSelectionSet) {
-      this._expandedSelectionSet = this.selectionSet.expandFragments().normalize({ parentType: this.typeCondition });
-    }
-    return this._expandedSelectionSet;
-  }
-
   withUpdatedSelectionSet(newSelectionSet: SelectionSet): NamedFragmentDefinition {
     return new NamedFragmentDefinition(this.schema(), this.name, this.typeCondition).setSelectionSet(newSelectionSet);
   }
@@ -1168,6 +1161,13 @@ export class NamedFragmentDefinition extends DirectiveTargetElement<NamedFragmen
     return isObjectType(type) || isUnionType(this.typeCondition);
   }
 
+  private expandedSelectionSet(): SelectionSet {
+    if (!this._expandedSelectionSet) {
+      this._expandedSelectionSet = this.selectionSet.expandFragments();
+    }
+    return this._expandedSelectionSet;
+  }
+
   /**
    * This methods *assumes* that `this.canApplyDirectlyAtType(type)` is `true` (and may crash if this is not true), and returns
    * a version fo this named fragment selection set that corresponds to the "expansion" of this named fragment at `type`
@@ -1187,11 +1187,6 @@ export class NamedFragmentDefinition extends DirectiveTargetElement<NamedFragmen
    * us that part.
    */
   expandedSelectionSetAtType(type: CompositeType): FragmentRestrictionAtType {
-    // First, if the candidate condition is an object or is the type passed, then there isn't any restriction to do.
-    if (sameType(type, this.typeCondition) || isObjectType(this.typeCondition)) {
-      return { selectionSet: this.expandedSelectionSet() };
-    }
-
     let cached = this.expandedSelectionSetsAtTypesCache.get(type.name);
     if (!cached) {
       cached = this.computeExpandedSelectionSetAtType(type);
@@ -1202,9 +1197,7 @@ export class NamedFragmentDefinition extends DirectiveTargetElement<NamedFragmen
 
   private computeExpandedSelectionSetAtType(type: CompositeType): FragmentRestrictionAtType {
     const expandedSelectionSet = this.expandedSelectionSet();
-    // Note that what we want is get any simplification coming from normalizing at `type`, but any such simplication
-    // stops as soon as we traverse a field, so no point in being recursive.
-    const selectionSet = expandedSelectionSet.normalize({ parentType: type, recursive: false });
+    const selectionSet = expandedSelectionSet.normalize({ parentType: type });
 
     // 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

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

@@ -6570,6 +6570,7 @@ describe("named fragments", () => {
           {
             t1 {
               other {
+                __typename
                 id
               }
             }