fix: Handle defaulted variables correctly during post-processing (#2443)

Post-processing didn't previously account for defaulted variable values in an
operation at all. This surfaces as a problem when using conditional directives
(@skip/@include) that use variables with defaulted values (and no
corresponding variable provided).

We should collect defaulted variable values from the operation and pass them
along with the variables we provide to computeResponse so these are
accounted for.

.changeset/gentle-rockets-roll.md

@@ -0,0 +1,11 @@
+---
+"@apollo/federation-internals": patch
+"@apollo/gateway": patch
+---
+
+Handle defaulted variables correctly during post-processing.
+
+Users who tried to use built-in conditional directives (skip/include) with _defaulted_ variables and no variable provided would encounter an error thrown by operation post-processing saying that the variables weren't provided. The defaulted values went unaccounted for, so the operation would validate but then fail an assertion while resolving the conditional.
+
+With this change, defaulted variable values are now collected and provided to post-processing (with defaults being overwritten by variables that are actually provided).
+

gateway-js/src/__tests__/resultShaping.test.ts

@@ -249,7 +249,7 @@ describe('gateway post-processing', () => {
         }
       `);
 
-      let res = computeResponse({
+      const res = computeResponse({
         operation: operationNonNullable,
         input,
         introspectionHandling,
@@ -280,7 +280,7 @@ describe('gateway post-processing', () => {
         }
       `);
 
-      let res = computeResponse({
+      const res = computeResponse({
         operation: operationNonNullable,
         input,
         introspectionHandling,
@@ -365,7 +365,7 @@ describe('gateway post-processing', () => {
     `);
 
     const input = {
-      "x": 'foo', 
+      "x": 'foo',
     }
 
     const operation = parseOperation(schema, `
@@ -530,4 +530,76 @@ describe('gateway post-processing', () => {
       }
     `);
   });
+
+  test('Handles defaulted `if` conditions', () => {
+    const schema = buildSchemaFromAST(gql`
+      type Query {
+        hello: String!
+      }
+    `);
+
+    const input = {
+      skipped: 'world',
+      included: 'world',
+    };
+
+    const operation = parseOperation(schema, `#graphql
+      query DefaultedIfCondition($if: Boolean = true) {
+        skipped: hello @skip(if: $if)
+        included: hello @include(if: $if)
+      }
+    `);
+
+    expect(
+      computeResponse({
+        operation,
+        input,
+        introspectionHandling,
+      }),
+    ).toMatchInlineSnapshot(`
+      Object {
+        "data": Object {
+          "included": "world",
+        },
+        "errors": Array [],
+      }
+    `);
+  });
+
+  test('Provided variables overwrite defaulted variable values', () => {
+    const schema = buildSchemaFromAST(gql`
+      type Query {
+        hello: String!
+      }
+    `);
+
+    const input = {
+      skipped: 'world',
+      included: 'world',
+    };
+
+    const operation = parseOperation(schema, `#graphql
+        # note that the default conditional is inverted from the previous test
+      query DefaultedIfCondition($if: Boolean = false) {
+        skipped: hello @skip(if: $if)
+        included: hello @include(if: $if)
+      }
+    `);
+
+    expect(
+      computeResponse({
+        operation,
+        input,
+        variables: { if: true },
+        introspectionHandling,
+      }),
+    ).toMatchInlineSnapshot(`
+      Object {
+        "data": Object {
+          "included": "world",
+        },
+        "errors": Array [],
+      }
+    `);
+  });
 })

gateway-js/src/resultShaping.ts

@@ -26,7 +26,7 @@ import { GraphQLError } from "graphql";
  * Performs post-query plan execution processing of internally fetched data to produce the final query response.
  *
  * The reason for this post-processing are the following ones:
- * 1. executing the query plan will usually query more fields that are strictly requested. That is because key, required 
+ * 1. executing the query plan will usually query more fields that are strictly requested. That is because key, required
  *   and __typename fields must often be requested to subgraphs even when they are not part of the query. So this method
  *   will filter out anything that has been fetched but isn't part of the user query.
  * 2. query plan execution does not guarantee that in the data fetched, the fields will respect the ordering that the
@@ -60,7 +60,7 @@ export function computeResponse({
   variables?: Record<string, any>,
   input: Record<string, any> | null | undefined,
   introspectionHandling: (introspectionSelection: FieldSelection) => any,
-}): { 
+}): {
   data: Record<string, any> | null | undefined,
   errors: GraphQLError[],
 } {
@@ -70,7 +70,11 @@ export function computeResponse({
 
   const parameters = {
     schema: operation.schema,
-    variables: variables ?? {},
+    variables: {
+      ...operation.collectDefaultedVariableValues(),
+      // overwrite any defaulted variables if they are provided
+      ...variables,
+    },
     errors: [],
     introspectionHandling,
   };
@@ -119,7 +123,7 @@ function ifValue(directive: Directive<any, { if: boolean | Variable }>, variable
   }
 }
 
-enum ApplyResult { OK, NULL_BUBBLE_UP };
+enum ApplyResult { OK, NULL_BUBBLE_UP }
 
 function typeConditionApplies(
   schema: Schema,

internals-js/src/operations.ts

@@ -240,7 +240,7 @@ export class Field<TArgs extends {[key: string]: any} = {[key: string]: any}> ex
     //    happens when we're building subgraph queries but using selections from the original query which is against the supergraph API schema.
     //  2. or they are not the same underlying type, and we only accept this if we're adding an interface field to a selection of one of its
     //    subtype, and this for convenience. Note that in that case too, `selectinParent` and `fieldParent` may or may be from the same exact
-    //    underlying schema, and so we avoid relying on `isDirectSubtype` in the check. 
+    //    underlying schema, and so we avoid relying on `isDirectSubtype` in the check.
     // In both cases, we just get the field from `selectionParent`, ensuring the return field parent _is_ `selectionParent`.
     const fieldParentType = this.definition.parent
     return parentType.name === fieldParentType.name
@@ -358,7 +358,7 @@ export class FragmentElement extends AbstractOperationElement<FragmentElement> {
     // schema.
     const { canRebase, rebasedCondition } = this.canRebaseOn(selectionParent);
     validate(
-      canRebase, 
+      canRebase,
       () => `Cannot add fragment of condition "${typeCondition}" (runtimes: [${possibleRuntimeTypes(typeCondition!)}]) to selection set of parent type "${selectionParent}" (runtimes: ${possibleRuntimeTypes(selectionParent)})`
     );
     return this.withUpdatedTypes(selectionParent, rebasedCondition);
@@ -693,6 +693,16 @@ export class Operation {
     };
   }
 
+  collectDefaultedVariableValues(): Record<string, any> {
+    const defaultedVariableValues: Record<string, any> = {};
+    for (const { variable, defaultValue } of this.variableDefinitions.definitions()) {
+      if (defaultValue !== undefined) {
+        defaultedVariableValues[variable.name] = defaultValue;
+      }
+    }
+    return defaultedVariableValues;
+  }
+
   toString(expandFragments: boolean = false, prettyPrint: boolean = true): string {
     return this.selectionSet.toOperationString(this.rootKind, this.variableDefinitions, this.name, expandFragments, prettyPrint);
   }