Fix fragment expansion nuking @defer and other minor @defer updates (#…

…2093)

An optimisation recently made to the handling of fragments was mistakenly forgetting to look for directive applications on fragments and as a result those application were lost when that optimisation kicked in. In particular, this could make some `@defer` on a named fragments be completely ignored. This patch fixes this by simply avoiding the optimisation in question when there is applied directives, thus preserving them.

Additionally, this commit addresses comments from @glasser on #1958, namely:
- it removes/update some outdated TODOs.
- it fixes up the definition of @defer/@stream to match current spec.
- makes serialized query plan for condition hopefully a tad easier to parse (solely impact unit tests readability).
- adds a test to ensure that if the same field is queries both "normally" and deferred, we do query it twice (that's what spec specifies).

gateway-js/CHANGELOG.md

@@ -4,6 +4,7 @@ This CHANGELOG pertains only to Apollo Federation packages in the 2.x range. The
 
 ## vNext
 
+- Fix issue where fragment expansion can erase applied directives (most notably `@defer`) [PR #2093](https://github.com/apollographql/federation/pull/2093).
 - Fix abnormally high memory usage when extracting subgraphs for some fed1 supergraphs (and small other memory footprint improvements) [PR #2089](https://github.com/apollographql/federation/pull/2089).
 - Fix issue with fragment reusing code something mistakenly re-expanding fragments [PR #2098](https://github.com/apollographql/federation/pull/2098).
 - Fix issue when type is only reachable through a @provides [PR #2083](https://github.com/apollographql/federation/pull/2083).

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

@@ -817,17 +817,26 @@ test('correctly convert to a graphQL-js schema', () => {
   expect(printGraphQLjsSchema(graphqQLSchema)).toMatchString(sdl);
 });
 
-test('Conversion to graphQL-js schema can optionally include @defer definition', () => {
+test('Conversion to graphQL-js schema can optionally include @defer and/or @streams definition(s)', () => {
   const sdl = `
     type Query {
       x: Int
     }
   `;
   const schema = parseSchema(sdl);
 
-  const graphqQLSchema = schema.toGraphQLJSSchema({ includeDefer: true });
-  expect(printGraphQLjsSchema(graphqQLSchema)).toMatchString(`
-    directive @defer(label: String, if: Boolean) on FRAGMENT_SPREAD | INLINE_FRAGMENT
+  expect(printGraphQLjsSchema(schema.toGraphQLJSSchema({ includeDefer: true }))).toMatchString(`
+    directive @defer(label: String, if: Boolean! = true) on FRAGMENT_SPREAD | INLINE_FRAGMENT
+
+    type Query {
+      x: Int
+    }
+  `);
+
+  expect(printGraphQLjsSchema(schema.toGraphQLJSSchema({ includeDefer: true, includeStream:  true }))).toMatchString(`
+    directive @defer(label: String, if: Boolean! = true) on FRAGMENT_SPREAD | INLINE_FRAGMENT
+
+    directive @stream(label: String, initialCount: Int = 0, if: Boolean! = true) on FIELD
 
     type Query {
       x: Int

internals-js/src/definitions.ts

@@ -1128,15 +1128,16 @@ const graphQLBuiltInDirectivesSpecifications: readonly DirectiveSpecification[]
     locations: [DirectiveLocation.SCALAR],
     argumentFct: (schema) => ({ args: [{ name: 'url', type: new NonNullType(schema.stringType()) }], errors: [] })
   }),
-  // TODO: currently inconditionally adding @defer as the list of built-in. It's probably fine, but double check if we want to not do so when @defer-support is
-  // not enabled or something (it would probably be hard to handle it at that point anyway but well...).
+  // Note that @defer and @stream are unconditionally added to `Schema` even if they are technically "optional" built-in. _But_,
+  // the `Schema#toGraphQLJSSchema` method has an option to decide if @defer/@stream should be included or not in the resulting
+  // schema, which is how the gateway and router can, at runtime, decide to include or not include them based on actual support.
   createDirectiveSpecification({
     name: 'defer',
     locations: [DirectiveLocation.FRAGMENT_SPREAD, DirectiveLocation.INLINE_FRAGMENT],
     argumentFct: (schema) => ({
       args: [
         { name: 'label', type: schema.stringType() },
-        { name: 'if', type: schema.booleanType() },
+        { name: 'if', type: new NonNullType(schema.booleanType()), defaultValue: true },
       ],
       errors: [],
     })
@@ -1151,17 +1152,14 @@ const graphQLBuiltInDirectivesSpecifications: readonly DirectiveSpecification[]
       args: [
         { name: 'label', type: schema.stringType() },
         { name: 'initialCount', type: schema.intType(), defaultValue: 0 },
-        { name: 'if', type: schema.booleanType() },
+        { name: 'if', type: new NonNullType(schema.booleanType()), defaultValue: true },
       ],
       errors: [],
     })
   }),
 ];
 
 export type DeferDirectiveArgs = {
-  // TODO: we currently do not support variables for the defer label. Passing a label in a variable
-  // feels like a weird use case in the first place, but we should probably fix this nonetheless (or
-  // if we decide to have it be a known limitations, we should at least reject it cleanly).
   label?: string,
   if?: boolean | Variable,
 }
@@ -1299,8 +1297,9 @@ export class Schema {
     return nodes;
   }
 
-  toGraphQLJSSchema(config?: { includeDefer?: boolean }): GraphQLSchema {
+  toGraphQLJSSchema(config?: { includeDefer?: boolean, includeStream?: boolean }): GraphQLSchema {
     const includeDefer = config?.includeDefer ?? false;
+    const includeStream = config?.includeStream ?? false;
 
     let ast = this.toAST();
 
@@ -1312,6 +1311,9 @@ export class Schema {
     if (includeDefer) {
       additionalNodes.push(this.deferDirective().toAST());
     }
+    if (includeStream) {
+      additionalNodes.push(this.streamDirective().toAST());
+    }
     if (additionalNodes.length > 0) {
       ast = {
         kind: Kind.DOCUMENT,

internals-js/src/operations.ts

@@ -1894,7 +1894,7 @@ class FragmentSpreadSelection extends FragmentSelection {
     }
 
     const expandedSubSelections = this.selectionSet.expandFragments(names, updateSelectionSetFragments);
-    return sameType(this._element.parentType, this.namedFragment.typeCondition)
+    return sameType(this._element.parentType, this.namedFragment.typeCondition) && this._element.appliedDirectives.length === 0
       ? expandedSubSelections.selections()
       : new InlineFragmentSelection(this._element, expandedSubSelections);
   }

query-planner-js/CHANGELOG.md

@@ -3,6 +3,7 @@
 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.
 
 
+- Fix issue where fragment expansion can erase applied directives (most notably `@defer`) [PR #2093](https://github.com/apollographql/federation/pull/2093).
 - Fix issue with fragment reusing code something mistakenly re-expanding fragments [PR #2098](https://github.com/apollographql/federation/pull/2098).
 
 ## 2.1.0-alpha.4