Prevent over-eager merging of fields which have differing directive a…

…pplications (#2713)

Fix over-eager merging of fields with different directive applications

Previously, the following query would incorrectly combine the selection set of `hello`,
with both fields ending up under the @Skip condition:

```graphql
query Test($skipField: Boolean!) {
  hello @Skip(if: $skipField) {
    world
  }
  hello {
    goodbye
  }
}
```

This change identifies those two selections on `hello` as unique while constructing our
operation representation so they aren't merged at all, leaving it to the subgraph to
handle the operation as-is.

.changeset/curvy-rockets-bow.md

@@ -0,0 +1,21 @@
+---
+"@apollo/query-planner": patch
+"@apollo/federation-internals": patch
+---
+
+Fix over-eager merging of fields with different directive applications
+
+Previously, the following query would incorrectly combine the selection set of `hello`, with both fields ending up under the @skip condition:
+```graphql
+query Test($skipField: Boolean!) {
+  hello @skip(if: $skipField) {
+    world
+  }
+  hello {
+    goodbye
+  }
+}
+```
+
+This change identifies those two selections on `hello` as unique while constructing our operation representation so they aren't merged at all, leaving it to the subgraph to handle the operation as-is.
+

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

@@ -9,6 +9,7 @@ import { FragmentRestrictionAtType, MutableSelectionSet, NamedFragmentDefinition
 import './matchers';
 import { DocumentNode, FieldNode, GraphQLError, Kind, OperationDefinitionNode, OperationTypeNode, parse, SelectionNode, SelectionSetNode, validate } from 'graphql';
 import { assert } from '../utils';
+import gql from 'graphql-tag';
 
 function parseSchema(schema: string): Schema {
   try {
@@ -2519,6 +2520,30 @@ describe('basic operations', () => {
       ['T', 'v2'],
     ]);
   })
+
+  test('fields are keyed on both name and directive applications', () => {
+    const operation = operationFromDocument(schema, gql`
+      query Test($skipIf: Boolean!) {
+        t {
+          v1
+        }
+        t @skip(if: $skipIf) {
+          v2
+        }
+      }
+    `);
+
+    expect(operation.toString()).toMatchString(`
+      query Test($skipIf: Boolean!) {
+        t {
+          v1
+        }
+        t @skip(if: $skipIf) {
+          v2
+        }
+      }
+    `);
+  })
 });
 
 describe('MutableSelectionSet', () => {

internals-js/src/operations.ts

@@ -146,7 +146,7 @@ export class Field<TArgs extends {[key: string]: any} = {[key: string]: any}> ex
   }
 
   key(): string {
-    return this.responseName();
+    return this.responseName() + this.appliedDirectivesToString();
   }
 
   asPathElement(): string {

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

@@ -0,0 +1,192 @@
+import { operationFromDocument } from '@apollo/federation-internals';
+import gql from 'graphql-tag';
+import { composeAndCreatePlanner } from './testHelper';
+
+describe('merging @skip / @include directives', () => {
+  const subgraph1 = {
+    name: 'S1',
+    typeDefs: gql`
+      type Query {
+        hello: Hello!
+        extraFieldToPreventSkipIncludeNodes: String!
+      }
+
+      type Hello {
+        world: String!
+        goodbye: String!
+      }
+    `,
+  };
+
+  const [api, queryPlanner] = composeAndCreatePlanner(subgraph1);
+
+  it('with fragment', () => {
+    const operation = operationFromDocument(
+      api,
+      gql`
+        query Test($skipField: Boolean!) {
+          ...ConditionalSkipFragment
+          hello {
+            world
+          }
+          extraFieldToPreventSkipIncludeNodes
+        }
+
+        fragment ConditionalSkipFragment on Query {
+          hello @skip(if: $skipField) {
+            goodbye
+          }
+        }
+      `,
+    );
+
+    const plan = queryPlanner.buildQueryPlan(operation);
+    expect(plan).toMatchInlineSnapshot(`
+      QueryPlan {
+        Fetch(service: "S1") {
+          {
+            hello @skip(if: $skipField) {
+              goodbye
+            }
+            hello {
+              world
+            }
+            extraFieldToPreventSkipIncludeNodes
+          }
+        },
+      }
+    `);
+  });
+
+  it('without fragment', () => {
+    const operation = operationFromDocument(
+      api,
+      gql`
+        query Test($skipField: Boolean!) {
+          hello @skip(if: $skipField) {
+            world
+          }
+          hello {
+            goodbye
+          }
+          extraFieldToPreventSkipIncludeNodes
+        }
+      `,
+    );
+
+    const plan = queryPlanner.buildQueryPlan(operation);
+    expect(plan).toMatchInlineSnapshot(`
+      QueryPlan {
+        Fetch(service: "S1") {
+          {
+            hello @skip(if: $skipField) {
+              world
+            }
+            hello {
+              goodbye
+            }
+            extraFieldToPreventSkipIncludeNodes
+          }
+        },
+      }
+    `);
+  });
+
+  it('multiple applications identical', () => {
+    const operation = operationFromDocument(
+      api,
+      gql`
+        query Test($skipField: Boolean!, $includeField: Boolean!) {
+          hello @skip(if: $skipField) @include(if: $includeField) {
+            world
+          }
+          hello @skip(if: $skipField) @include(if: $includeField) {
+            goodbye
+          }
+          extraFieldToPreventSkipIncludeNodes
+        }
+      `,
+    );
+
+    const plan = queryPlanner.buildQueryPlan(operation);
+    expect(plan).toMatchInlineSnapshot(`
+      QueryPlan {
+        Fetch(service: "S1") {
+          {
+            hello @skip(if: $skipField) @include(if: $includeField) {
+              world
+              goodbye
+            }
+            extraFieldToPreventSkipIncludeNodes
+          }
+        },
+      }
+    `);
+  });
+
+  it('multiple applications differing order', () => {
+    const operation = operationFromDocument(
+      api,
+      gql`
+        query Test($skipField: Boolean!, $includeField: Boolean!) {
+          hello @skip(if: $skipField) @include(if: $includeField) {
+            world
+          }
+          hello @include(if: $includeField) @skip(if: $skipField) {
+            goodbye
+          }
+          extraFieldToPreventSkipIncludeNodes
+        }
+      `,
+    );
+
+    const plan = queryPlanner.buildQueryPlan(operation);
+    expect(plan).toMatchInlineSnapshot(`
+      QueryPlan {
+        Fetch(service: "S1") {
+          {
+            hello @include(if: $includeField) @skip(if: $skipField) {
+              world
+              goodbye
+            }
+            extraFieldToPreventSkipIncludeNodes
+          }
+        },
+      }
+    `);
+  });
+
+  it('multiple applications differing quantity', () => {
+    const operation = operationFromDocument(
+      api,
+      gql`
+        query Test($skipField: Boolean!, $includeField: Boolean!) {
+          hello @skip(if: $skipField) @include(if: $includeField) {
+            world
+          }
+          hello @include(if: $includeField) {
+            goodbye
+          }
+          extraFieldToPreventSkipIncludeNodes
+        }
+      `,
+    );
+
+    const plan = queryPlanner.buildQueryPlan(operation);
+    expect(plan).toMatchInlineSnapshot(`
+      QueryPlan {
+        Fetch(service: "S1") {
+          {
+            hello @skip(if: $skipField) @include(if: $includeField) {
+              world
+            }
+            hello @include(if: $includeField) {
+              goodbye
+            }
+            extraFieldToPreventSkipIncludeNodes
+          }
+        },
+      }
+    `);
+  });
+});