Fix issue optimizing @require when there is mutiple @skip/@include

The code that recognize (and optimize accordindly) when a `@require`
condition is taken _just after_ a key edge (which is the most common
case in practice) was mistakenly assuming that the selection set
"just after a key edge" was only starting by a single fragment.
In practice, when multiple conditions (`@skip`/`@include`) are
involved, all those conditions are preserved and may be layed out
on multiple conditions. That is, after a key edge, the selection
set may look like:
```graphql
... on EntityType @Skip(if: $c1) {
  ... on EntityType @include(if: $c2) {
    <rest of the selection>
  }
}
```
In that case, the wrong part of the code was taken and this
resulted in unoptimized query plans, ones having useless fetches.

The commit fixes the condition, and ensure that now that the proper
optimizations do apply, we streamline the created result sets by
avoiding repeating conditions multiple times.

gateway-js/CHANGELOG.md

@@ -6,6 +6,7 @@ This CHANGELOG pertains only to Apollo Federation packages in the 2.x range. The
 
 - Fix bug with type extension of empty type definition [PR #1821](https://github.com/apollographql/federation/pull/1821)
 - Fix output of `printSubgraphSchema` method, ensuring it can be read back by composition and `buildSubgraphSchema` [PR #1831](https://github.com/apollographql/federation/pull/1831).
+- Fix issue with `@requires` and conditional queries (`@include`/`@skip`) [1835](https://github.com/apollographql/federation/pull/1835).
 
 ## 2.0.2
 

internals-js/src/definitions.ts

@@ -276,6 +276,10 @@ export function runtimeTypesIntersects(t1: CompositeType, t2: CompositeType): bo
   return false;
 }
 
+export function isConditionalDirective(directive: Directive<any, any> | DirectiveDefinition<any>): boolean {
+  return ['include', 'skip'].includes(directive.name);
+}
+
 export const executableDirectiveLocations: DirectiveLocation[] = [
   DirectiveLocation.QUERY,
   DirectiveLocation.MUTATION,
@@ -3059,32 +3063,51 @@ export class Directive<
   }
 }
 
+export function sameDirectiveApplication(application1: Directive<any, any>, application2: Directive<any, any>): boolean {
+  return application1.name === application2.name && argumentsEquals(application1.arguments(), application2.arguments());
+}
+
+/**
+ * Checks whether the 2 provided "set" of directive applications are the same (same applications, regardless or order).
+ */
 export function sameDirectiveApplications(applications1: Directive<any, any>[], applications2: Directive<any, any>[]): boolean {
   if (applications1.length !== applications2.length) {
     return false;
   }
 
   for (const directive1 of applications1) {
-    if (!applications2.some(directive2 => directive1.name === directive2.name && argumentsEquals(directive1.arguments(), directive2.arguments()))) {
+    if (!applications2.some(directive2 => sameDirectiveApplication(directive1, directive2))) {
       return false;
     }
   }
   return true;
 }
 
+/**
+ * Checks whether a given array of directive applications (`maybeSubset`) is a sub-set of another array of directive applications (`applications`).
+ *
+ * Sub-set here means that all of the applications in `maybeSubset` appears in `applications`. 
+ */
 export function isDirectiveApplicationsSubset(applications: Directive<any, any>[], maybeSubset: Directive<any, any>[]): boolean {
   if (maybeSubset.length > applications.length) {
     return false;
   }
 
   for (const directive1 of maybeSubset) {
-    if (!applications.some(directive2 => directive1.name === directive2.name && argumentsEquals(directive1.arguments(), directive2.arguments()))) {
+    if (!applications.some(directive2 => sameDirectiveApplication(directive1, directive2))) {
       return false;
     }
   }
   return true;
 }
 
+/**
+ * Computes the difference between the set of directives applications `baseApplications` and the `toRemove` one.
+ */
+export function directiveApplicationsSubstraction(baseApplications: Directive<any, any>[], toRemove: Directive<any, any>[]): Directive<any, any>[] {
+  return baseApplications.filter((application) => !toRemove.some((other) => sameDirectiveApplication(application, other)));
+}
+
 export class Variable {
   constructor(readonly name: string) {}
 

internals-js/src/operations.ts

@@ -42,6 +42,8 @@ import {
   typenameFieldName,
   NamedType,
   sameDirectiveApplications,
+  isConditionalDirective,
+  isDirectiveApplicationsSubset,
 } from "./definitions";
 import { sameType } from "./types";
 import { assert, mapEntries, MapWithCachedArrays, MultiMap } from "./utils";
@@ -298,6 +300,13 @@ export function sameOperationPaths(p1: OperationPath, p2: OperationPath): boolea
   return true;
 }
 
+/**
+ * Returns all the "conditional" directive applications (`@skip` and `@include`) in the provided path.
+ */
+export function conditionalDirectivesInOperationPath(path: OperationPath): Directive<any, any>[] {
+  return path.map((e) => e.appliedDirectives).flat().filter((d) => isConditionalDirective(d));
+}
+
 export function concatOperationPaths(head: OperationPath, tail: OperationPath): OperationPath {
   // While this is mainly a simple array concatenation, we optimize slightly by recognizing if the
   // tail path starts by a fragment selection that is useless given the end of the head path.
@@ -308,14 +317,21 @@ export function concatOperationPaths(head: OperationPath, tail: OperationPath):
     return head;
   }
   const lastOfHead = head[head.length - 1];
-  const firstOfTail = tail[0];
-  if (isUselessFollowupElement(lastOfHead, firstOfTail)) {
+  const conditionals = conditionalDirectivesInOperationPath(head);
+  let firstOfTail = tail[0];
+  // Note that in practice, we may be able to eliminate a few elements at the beginning of the path
+  // due do conditionals ('@skip' and '@include'). Indeed, a (tail) path crossing multiple conditions
+  // may start with: [ ... on X @include(if: $c1), ... on X @ksip(if: $c2), (...)], but if `head`
+  // already ends on type `X` _and_ both the conditions on `$c1` and `$c2` are alredy found on `head`,
+  // then we can remove both fragments in `tail`.
+  while (firstOfTail && isUselessFollowupElement(lastOfHead, firstOfTail, conditionals)) {
     tail = tail.slice(1);
+    firstOfTail = tail[0];
   }
   return head.concat(tail);
 }
 
-function isUselessFollowupElement(first: OperationElement, followup: OperationElement): boolean {
+function isUselessFollowupElement(first: OperationElement, followup: OperationElement, conditionals: Directive<any, any>[]): boolean {
   const typeOfFirst = first.kind === 'Field'
     ? baseType(first.definition.type!)
     : first.typeCondition;
@@ -325,7 +341,7 @@ function isUselessFollowupElement(first: OperationElement, followup: OperationEl
   return !!typeOfFirst
     && followup.kind === 'FragmentElement'
     && !!followup.typeCondition
-    && followup.appliedDirectives.length === 0
+    && (followup.appliedDirectives.length === 0 || isDirectiveApplicationsSubset(conditionals, followup.appliedDirectives))
     && sameType(typeOfFirst, followup.typeCondition);
 }
 

query-planner-js/CHANGELOG.md

@@ -2,6 +2,10 @@
 
 This CHANGELOG pertains only to Apollo Federation packages in the 2.x range. The Federation v0.x equivalent for this package can be found [here](https://github.com/apollographql/federation/blob/version-0.x/query-planner-js/CHANGELOG.md) on the `version-0.x` branch of this repo.
 
+## vNext
+
+- Fix issue with `@requires` and conditional queries (`@include`/`@skip`) [1835](https://github.com/apollographql/federation/pull/1835).
+
 ## 2.0.2
 
 - Fix handling of @require "chains" (a @require whose fields have @require themselves) [PR #1790](https://github.com/apollographql/federation/pull/1790)

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

@@ -2140,6 +2140,114 @@ describe('@requires', () => {
         }
       `);
     });
+
+    it('handles a @requires where multiple conditional are involved', () => {
+      const subgraph1 = {
+        name: 'Subgraph1',
+        typeDefs: gql`
+          type Query {
+            a: A
+          }
+
+          type A @key(fields: "idA") {
+            idA: ID!
+          }
+        `
+      }
+
+      const subgraph2 = {
+        name: 'Subgraph2',
+        typeDefs: gql`
+          type A @key(fields: "idA") {
+            idA: ID!
+            b: [B]
+          }
+
+          type B @key(fields: "idB") {
+            idB: ID!
+            required: Int
+          }
+        `
+      }
+
+      const subgraph3 = {
+        name: 'Subgraph3',
+        typeDefs: gql`
+          type B @key(fields: "idB") {
+            idB: ID!
+            c: Int @requires(fields: "required")
+            required: Int @external
+          }
+        `
+      }
+
+      const [api, queryPlanner] = composeAndCreatePlanner(subgraph1, subgraph2, subgraph3);
+      const operation = operationFromDocument(api, gql`
+        query foo($test1: Boolean!, $test2: Boolean!){
+          a @include(if: $test1) {
+            b @include(if: $test2) {
+              c
+            }
+          }
+        }
+      `);
+
+      global.console = require('console');
+      const plan = queryPlanner.buildQueryPlan(operation);
+      expect(plan).toMatchInlineSnapshot(`
+        QueryPlan {
+          Sequence {
+            Fetch(service: "Subgraph1") {
+              {
+                a @include(if: $test1) {
+                  __typename
+                  idA
+                }
+              }
+            },
+            Flatten(path: "a") {
+              Fetch(service: "Subgraph2") {
+                {
+                  ... on A {
+                    __typename
+                    idA
+                  }
+                } =>
+                {
+                  ... on A @include(if: $test1) {
+                    b @include(if: $test2) {
+                      __typename
+                      idB
+                      required
+                    }
+                  }
+                }
+              },
+            },
+            Flatten(path: "a.b.@") {
+              Fetch(service: "Subgraph3") {
+                {
+                  ... on B {
+                    ... on B {
+                      __typename
+                      idB
+                      required
+                    }
+                  }
+                } =>
+                {
+                  ... on B @include(if: $test1) {
+                    ... on B @include(if: $test2) {
+                      c
+                    }
+                  }
+                }
+              },
+            },
+          },
+        }
+      `);
+    });
   });
 });