Fragment args additional validation:

mjmahone · mjmahone · commit 094d2d6b4527 · 2023-01-02T14:08:45.000-05:00
Re-use all existing validation, updating (or updating tests for):
- ProvidedRequiredArguments
- ValuesOfCorrectType
- VariablesAreInputTypes
- UniqueVariableNames
- VariablesInAllowedPosition

Some of the implications here I'm not in love with: I think it's become more clear that fragment arguments ought to be ArgumentDefinitionNodes rather than VariableDefinitionNodes, even though when *used* they are still variables.

I'll try redoing this PR using ArgumentDefinitionNode and see how that simplifies or complicates the matter
diff --git a/src/utilities/TypeInfo.ts b/src/utilities/TypeInfo.ts
@@ -69,8 +69,6 @@ export class TypeInfo {
     /** @deprecated will be removed in 17.0.0 */
     getFieldDefFn?: GetFieldDefFn,
   ) {
-    this._fragmentDefinitions = Object.create(null);
-
     this._schema = schema;
     this._typeStack = [];
     this._parentTypeStack = [];
@@ -81,6 +79,7 @@ export class TypeInfo {
     this._argument = null;
     this._enumValue = null;
     this._fragmentDefinition = null;
+    this._fragmentDefinitions = Object.create(null);
     this._getFieldDef = getFieldDefFn ?? getFieldDef;
     if (initialType) {
       if (isInputType(initialType)) {
diff --git a/src/validation/__tests__/ProvidedRequiredArgumentsRule-test.ts b/src/validation/__tests__/ProvidedRequiredArgumentsRule-test.ts
@@ -356,4 +356,112 @@ describe('Validate: Provided required arguments', () => {
       ]);
     });
   });
+
+  describe('Fragment required arguments', () => {
+    it('ignores unknown arguments', () => {
+      expectValid(`
+        {
+          ...Foo(unknownArgument: true)
+        }
+        fragment Foo on Query {
+          dog
+        }
+      `);
+    });
+
+    // Query: should this be allowed?
+    // We could differentiate between required/optional (i.e. no default value)
+    // vs. nullable/non-nullable (i.e. no !), whereas now they are conflated.
+    // So today:
+    // $x: Int!     `x:` is required and must not be null (NOT a nullable variable)
+    // $x: Int! = 3 `x:` is not required and must not be null (MAY BE a nullable variable)
+    // $x: Int      `x:` is not required and may be null
+    // $x: Int = 3  `x:` is not required and may be null
+    //
+    // It feels weird to collapse the nullable cases but not the non-nullable ones.
+    // Whereas all four feel like they ought to mean something explicitly different.
+    //
+    // Potential proposal:
+    // $x: Int!     `x:` is required and must not be null (NOT a nullable variable)
+    // $x: Int! = 3 `x:` is not required and must not be null (NOT a nullable variable)
+    // $x: Int      `x:` is required and may be null
+    // $x: Int = 3  `x:` is not required and may be null
+    //
+    // Required then is whether there's a default value,
+    // and nullable is whether there's a !
+    it('Missing nullable argument with default is allowed', () => {
+      expectValid(`
+          {
+            ...F
+            
+          }
+          fragment F($x: Int = 3) on Query {
+            foo
+          }
+        `);
+    });
+    // Above proposal: this should be an error
+    it('Missing nullable argument is allowed', () => {
+      expectValid(`
+          {
+            ...F
+            
+          }
+          fragment F($x: Int) on Query {
+            foo
+          }
+        `);
+    });
+    it('Missing non-nullable argument with default is allowed', () => {
+      expectValid(`
+          {
+            ...F
+            
+          }
+          fragment F($x: Int! = 3) on Query {
+            foo
+          }
+        `);
+    });
+    it('Missing non-nullable argument is not allowed', () => {
+      expectErrors(`
+          {
+            ...F
+            
+          }
+          fragment F($x: Int!) on Query {
+            foo
+          }
+        `).toDeepEqual([
+        {
+          message:
+            'Fragment "F" argument "x" of type "{ kind: "NonNullType", type: { kind: "NamedType", name: [Object], loc: [Object] }, loc: [Object] }" is required, but it was not provided.',
+          locations: [
+            { line: 3, column: 13 },
+            { line: 6, column: 22 },
+          ],
+        },
+      ]);
+    });
+
+    it('Supplies required variables', () => {
+      expectValid(`
+          {
+            ...F(x: 3)
+            
+          }
+          fragment F($x: Int!) on Query {
+            foo
+          }
+        `);
+    });
+
+    it('Skips missing fragments', () => {
+      expectValid(`
+          {
+            ...Missing(x: 3)
+          }
+        `);
+    });
+  });
 });
diff --git a/src/validation/__tests__/ValuesOfCorrectTypeRule-test.ts b/src/validation/__tests__/ValuesOfCorrectTypeRule-test.ts
@@ -1198,4 +1198,35 @@ describe('Validate: Values of correct type', () => {
       ]);
     });
   });
+
+  describe('Fragment argument values', () => {
+    it('list variables with invalid item', () => {
+      expectErrors(`
+        fragment InvalidItem($a: [String] = ["one", 2]) on Query {
+          dog { name }
+        }
+      `).toDeepEqual([
+        {
+          message: 'String cannot represent a non string value: 2',
+          locations: [{ line: 2, column: 53 }],
+        },
+      ]);
+    });
+
+    it('fragment spread with invalid argument value', () => {
+      expectErrors(`
+        fragment GivesString on Query {
+          ...ExpectsInt(a: "three")
+        }
+        fragment ExpectsInt($a: Int) on Query {
+          dog { name }
+        }
+      `).toDeepEqual([
+        {
+          message: 'Int cannot represent non-integer value: "three"',
+          locations: [{ line: 3, column: 28 }],
+        },
+      ]);
+    });
+  });
 });
diff --git a/src/validation/__tests__/VariablesAreInputTypesRule-test.ts b/src/validation/__tests__/VariablesAreInputTypesRule-test.ts
@@ -18,6 +18,9 @@ describe('Validate: Variables are input types', () => {
       query Foo($a: Unknown, $b: [[Unknown!]]!) {
         field(a: $a, b: $b)
       }
+      fragment Bar($a: Unknown, $b: [[Unknown!]]!) on Query {
+        field(a: $a, b: $b)
+      }
     `);
   });
 
@@ -26,6 +29,9 @@ describe('Validate: Variables are input types', () => {
       query Foo($a: String, $b: [Boolean!]!, $c: ComplexInput) {
         field(a: $a, b: $b, c: $c)
       }
+      fragment Bar($a: String, $b: [Boolean!]!, $c: ComplexInput) on Query {
+        field(a: $a, b: $b, c: $c)
+      }
     `);
   });
 
@@ -49,4 +55,25 @@ describe('Validate: Variables are input types', () => {
       },
     ]);
   });
+
+  it('output types on fragment arguments are invalid', () => {
+    expectErrors(`
+      fragment Bar($a: Dog, $b: [[CatOrDog!]]!, $c: Pet) on Query {
+        field(a: $a, b: $b, c: $c)
+      }
+    `).toDeepEqual([
+      {
+        locations: [{ line: 2, column: 24 }],
+        message: 'Variable "$a" cannot be non-input type "Dog".',
+      },
+      {
+        locations: [{ line: 2, column: 33 }],
+        message: 'Variable "$b" cannot be non-input type "[[CatOrDog!]]!".',
+      },
+      {
+        locations: [{ line: 2, column: 53 }],
+        message: 'Variable "$c" cannot be non-input type "Pet".',
+      },
+    ]);
+  });
 });
diff --git a/src/validation/__tests__/VariablesInAllowedPositionRule-test.ts b/src/validation/__tests__/VariablesInAllowedPositionRule-test.ts
@@ -438,5 +438,27 @@ describe('Validate: Variables are in allowed positions', () => {
         },
       ]);
     });
+
+    it('Int fragment arg => Int! field arg fails even when shadowed by Int! variable', () => {
+      expectErrors(`
+        query Query($intVar: Int!) {
+          complicatedArgs {
+            ...A(i: $intVar)
+          }
+        }
+        fragment A($intVar: Int) on ComplicatedArgs {
+          nonNullIntArgField(nonNullIntArg: $intVar)
+        }
+      `).toDeepEqual([
+        {
+          message:
+            'Variable "$intVar" of type "Int" used in position expecting type "Int!".',
+          locations: [
+            { line: 7, column: 20 },
+            { line: 8, column: 45 },
+          ],
+        },
+      ]);
+    });
   });
 });
diff --git a/src/validation/rules/ProvidedRequiredArgumentsRule.ts b/src/validation/rules/ProvidedRequiredArgumentsRule.ts
@@ -4,7 +4,10 @@ import type { ObjMap } from '../../jsutils/ObjMap.js';
 
 import { GraphQLError } from '../../error/GraphQLError.js';
 
-import type { InputValueDefinitionNode } from '../../language/ast.js';
+import type {
+  InputValueDefinitionNode,
+  VariableDefinitionNode,
+} from '../../language/ast.js';
 import { Kind } from '../../language/kinds.js';
 import { print } from '../../language/printer.js';
 import type { ASTVisitor } from '../../language/visitor.js';
@@ -56,6 +59,37 @@ export function ProvidedRequiredArgumentsRule(
         }
       },
     },
+    FragmentSpread: {
+      // Validate on leave to allow for directive errors to appear first.
+      leave(spreadNode) {
+        const fragmentDef = context.getFragment(spreadNode.name.value);
+        if (!fragmentDef) {
+          return false;
+        }
+
+        const providedArgs = new Set(
+          // FIXME: https://github.com/graphql/graphql-js/issues/2203
+          /* c8 ignore next */
+          spreadNode.arguments?.map((arg) => arg.name.value),
+        );
+        // FIXME: https://github.com/graphql/graphql-js/issues/2203
+        /* c8 ignore next */
+        for (const argDef of fragmentDef.argumentDefinitions ?? []) {
+          if (
+            !providedArgs.has(argDef.variable.name.value) &&
+            isRequiredArgumentNode(argDef)
+          ) {
+            const argTypeStr = inspect(argDef.type);
+            context.reportError(
+              new GraphQLError(
+                `Fragment "${spreadNode.name.value}" argument "${argDef.variable.name.value}" of type "${argTypeStr}" is required, but it was not provided.`,
+                { nodes: [spreadNode, argDef] },
+              ),
+            );
+          }
+        }
+      },
+    },
   };
 }
 
@@ -122,6 +156,8 @@ export function ProvidedRequiredArgumentsOnDirectivesRule(
   };
 }
 
-function isRequiredArgumentNode(arg: InputValueDefinitionNode): boolean {
+function isRequiredArgumentNode(
+  arg: InputValueDefinitionNode | VariableDefinitionNode,
+): boolean {
   return arg.type.kind === Kind.NON_NULL_TYPE && arg.defaultValue == null;
 }
diff --git a/src/validation/rules/UniqueVariableNamesRule.ts b/src/validation/rules/UniqueVariableNamesRule.ts
@@ -1,7 +1,9 @@
 import { groupBy } from '../../jsutils/groupBy.js';
+import type { Maybe } from '../../jsutils/Maybe.js';
 
 import { GraphQLError } from '../../error/GraphQLError.js';
 
+import type { VariableDefinitionNode } from '../../language/ast.js';
 import type { ASTVisitor } from '../../language/visitor.js';
 
 import type { ASTValidationContext } from '../ValidationContext.js';
@@ -16,25 +18,34 @@ export function UniqueVariableNamesRule(
 ): ASTVisitor {
   return {
     OperationDefinition(operationNode) {
-      // See: https://github.com/graphql/graphql-js/issues/2203
-      /* c8 ignore next */
-      const variableDefinitions = operationNode.variableDefinitions ?? [];
-
-      const seenVariableDefinitions = groupBy(
-        variableDefinitions,
-        (node) => node.variable.name.value,
-      );
-
-      for (const [variableName, variableNodes] of seenVariableDefinitions) {
-        if (variableNodes.length > 1) {
-          context.reportError(
-            new GraphQLError(
-              `There can be only one variable named "$${variableName}".`,
-              { nodes: variableNodes.map((node) => node.variable.name) },
-            ),
-          );
-        }
-      }
+      validateVariableDefinitions(operationNode.variableDefinitions, context);
+    },
+    FragmentDefinition(fragmentNode) {
+      validateVariableDefinitions(fragmentNode.argumentDefinitions, context);
     },
   };
 }
+
+function validateVariableDefinitions(
+  maybeVariableDefinitions: Maybe<ReadonlyArray<VariableDefinitionNode>>,
+  context: ASTValidationContext,
+) {
+  // See: https://github.com/graphql/graphql-js/issues/2203
+  /* c8 ignore next */
+  const variableDefinitions = maybeVariableDefinitions ?? [];
+  const seenVariableDefinitions = groupBy(
+    variableDefinitions,
+    (node) => node.variable.name.value,
+  );
+
+  for (const [variableName, variableNodes] of seenVariableDefinitions) {
+    if (variableNodes.length > 1) {
+      context.reportError(
+        new GraphQLError(
+          `There can be only one variable named "$${variableName}".`,
+          { nodes: variableNodes.map((node) => node.variable.name) },
+        ),
+      );
+    }
+  }
+}
diff --git a/src/validation/rules/VariablesInAllowedPositionRule.ts b/src/validation/rules/VariablesInAllowedPositionRule.ts
