validation suggestions (#12)

* validation suggestions

* remove OperationSignature from ValidationContext

only used in one rule, does not need to be on context

remove dependency on getVariableSignature

move FRAGMENT_ARGUMENT below ARGUMENT

fragment arguments do not have location defaults

only variable defaults => so getDefaultValue(), which returns location defaults for use with the allowedVariableUsage helper, should never be called by getVariableUsages with respect to fragment arguments

fragment arguments therefore need not add anything to the default value stack

reduce diff from main

these diffs crept in before Kind.FRAGMENT_ARGUMENT was separated out

Author: yaacovCR

src/utilities/TypeInfo.ts

@@ -2,9 +2,10 @@ import type { Maybe } from '../jsutils/Maybe.js';
 
 import type {
   ASTNode,
+  DocumentNode,
   FieldNode,
   FragmentDefinitionNode,
-  FragmentSpreadNode,
+  VariableDefinitionNode,
 } from '../language/ast.js';
 import { isNode } from '../language/ast.js';
 import { Kind } from '../language/kinds.js';
@@ -36,7 +37,11 @@ import type { GraphQLDirective } from '../type/directives.js';
 import type { GraphQLSchema } from '../type/schema.js';
 
 import { typeFromAST } from './typeFromAST.js';
-import { valueFromAST } from './valueFromAST.js';
+
+export interface FragmentSignature {
+  readonly definition: FragmentDefinitionNode;
+  readonly variableDefinitions: Map<string, VariableDefinitionNode>;
+}
 
 /**
  * TypeInfo is a utility class which, given a GraphQL schema, can keep track
@@ -53,8 +58,12 @@ export class TypeInfo {
   private _directive: Maybe<GraphQLDirective>;
   private _argument: Maybe<GraphQLArgument>;
   private _enumValue: Maybe<GraphQLEnumValue>;
-  private _fragmentSpread: Maybe<FragmentSpreadNode>;
-  private _fragmentDefinitions: Map<string, FragmentDefinitionNode>;
+  private _fragmentSignaturesByName: (
+    fragmentName: string,
+  ) => Maybe<FragmentSignature>;
+
+  private _fragmentSignature: Maybe<FragmentSignature>;
+  private _fragmentArgument: Maybe<VariableDefinitionNode>;
   private _getFieldDef: GetFieldDefFn;
 
   constructor(
@@ -66,7 +75,10 @@ export class TypeInfo {
     initialType?: Maybe<GraphQLType>,
 
     /** @deprecated will be removed in 17.0.0 */
-    getFieldDefFn?: GetFieldDefFn,
+    getFieldDefFn?: Maybe<GetFieldDefFn>,
+    fragmentSignatures?: Maybe<
+      (fragmentName: string) => Maybe<FragmentSignature>
+    >,
   ) {
     this._schema = schema;
     this._typeStack = [];
@@ -77,8 +89,9 @@ export class TypeInfo {
     this._directive = null;
     this._argument = null;
     this._enumValue = null;
-    this._fragmentSpread = null;
-    this._fragmentDefinitions = new Map();
+    this._fragmentSignaturesByName = fragmentSignatures ?? (() => null);
+    this._fragmentSignature = null;
+    this._fragmentArgument = null;
     this._getFieldDef = getFieldDefFn ?? getFieldDef;
     if (initialType) {
       if (isInputType(initialType)) {
@@ -129,6 +142,20 @@ export class TypeInfo {
     return this._argument;
   }
 
+  getFragmentSignature(): Maybe<FragmentSignature> {
+    return this._fragmentSignature;
+  }
+
+  getFragmentSignatureByName(): (
+    fragmentName: string,
+  ) => Maybe<FragmentSignature> {
+    return this._fragmentSignaturesByName;
+  }
+
+  getFragmentArgument(): Maybe<VariableDefinitionNode> {
+    return this._fragmentArgument;
+  }
+
   getEnumValue(): Maybe<GraphQLEnumValue> {
     return this._enumValue;
   }
@@ -141,14 +168,9 @@ export class TypeInfo {
     // which occurs before guarantees of schema and document validity.
     switch (node.kind) {
       case Kind.DOCUMENT: {
-        // A document's fragment definitions are type signatures
-        // referenced via fragment spreads. Ensure we can use definitions
-        // before visiting their call sites.
-        for (const astNode of node.definitions) {
-          if (astNode.kind === Kind.FRAGMENT_DEFINITION) {
-            this._fragmentDefinitions.set(astNode.name.value, astNode);
-          }
-        }
+        const fragmentSignatures = getFragmentSignatures(node);
+        this._fragmentSignaturesByName = (fragmentName: string) =>
+          fragmentSignatures.get(fragmentName);
         break;
       }
       case Kind.SELECTION_SET: {
@@ -181,7 +203,9 @@ export class TypeInfo {
         break;
       }
       case Kind.FRAGMENT_SPREAD: {
-        this._fragmentSpread = node;
+        this._fragmentSignature = this.getFragmentSignatureByName()(
+          node.name.value,
+        );
         break;
       }
       case Kind.INLINE_FRAGMENT:
@@ -200,69 +224,33 @@ export class TypeInfo {
         );
         break;
       }
-      case Kind.FRAGMENT_ARGUMENT: {
+      case Kind.ARGUMENT: {
         let argDef;
         let argType: unknown;
-        const fragmentSpread = this._fragmentSpread;
-
-        const fragmentDef = this._fragmentDefinitions.get(
-          fragmentSpread!.name.value,
-        );
-        const fragVarDef = fragmentDef?.variableDefinitions?.find(
-          (varDef) => varDef.variable.name.value === node.name.value,
-        );
-        if (fragVarDef) {
-          const fragVarType = typeFromAST(schema, fragVarDef.type);
-          if (isInputType(fragVarType)) {
-            const fragVarDefault = fragVarDef.defaultValue
-              ? valueFromAST(fragVarDef.defaultValue, fragVarType)
-              : undefined;
-
-            // Minor hack: transform the FragmentArgDef
-            // into a schema Argument definition, to
-            // enable visiting identically to field/directive args
-            const schemaArgDef: GraphQLArgument = {
-              name: fragVarDef.variable.name.value,
-              type: fragVarType,
-              defaultValue: fragVarDefault,
-              description: undefined,
-              deprecationReason: undefined,
-              extensions: {},
-              astNode: {
-                ...fragVarDef,
-                kind: Kind.INPUT_VALUE_DEFINITION,
-                name: fragVarDef.variable.name,
-              },
-            };
-            argDef = schemaArgDef;
+        const fieldOrDirective = this.getDirective() ?? this.getFieldDef();
+        if (fieldOrDirective) {
+          argDef = fieldOrDirective.args.find(
+            (arg) => arg.name === node.name.value,
+          );
+          if (argDef) {
+            argType = argDef.type;
           }
         }
-
-        if (argDef) {
-          argType = argDef.type;
-        }
-
         this._argument = argDef;
         this._defaultValueStack.push(argDef ? argDef.defaultValue : undefined);
         this._inputTypeStack.push(isInputType(argType) ? argType : undefined);
         break;
       }
-      case Kind.ARGUMENT: {
-        let argDef;
+      case Kind.FRAGMENT_ARGUMENT: {
+        const fragmentSignature = this.getFragmentSignature();
+        const argDef = fragmentSignature?.variableDefinitions.get(
+          node.name.value,
+        );
+        this._fragmentArgument = argDef;
         let argType: unknown;
-        const directive = this.getDirective();
-        const fieldDef = this.getFieldDef();
-        if (directive) {
-          argDef = directive.args.find((arg) => arg.name === node.name.value);
-        } else if (fieldDef) {
-          argDef = fieldDef.args.find((arg) => arg.name === node.name.value);
-        }
         if (argDef) {
-          argType = argDef.type;
+          argType = typeFromAST(this._schema, argDef.type);
         }
-
-        this._argument = argDef;
-        this._defaultValueStack.push(argDef ? argDef.defaultValue : undefined);
         this._inputTypeStack.push(isInputType(argType) ? argType : undefined);
         break;
       }
@@ -311,7 +299,8 @@ export class TypeInfo {
   leave(node: ASTNode) {
     switch (node.kind) {
       case Kind.DOCUMENT:
-        this._fragmentDefinitions = new Map();
+        this._fragmentSignaturesByName = /* c8 ignore start */ () =>
+          null /* c8 ignore end */;
         break;
       case Kind.SELECTION_SET:
         this._parentTypeStack.pop();
@@ -324,7 +313,7 @@ export class TypeInfo {
         this._directive = null;
         break;
       case Kind.FRAGMENT_SPREAD:
-        this._fragmentSpread = null;
+        this._fragmentSignature = null;
         break;
       case Kind.OPERATION_DEFINITION:
       case Kind.INLINE_FRAGMENT:
@@ -334,12 +323,17 @@ export class TypeInfo {
       case Kind.VARIABLE_DEFINITION:
         this._inputTypeStack.pop();
         break;
-      case Kind.FRAGMENT_ARGUMENT:
       case Kind.ARGUMENT:
         this._argument = null;
         this._defaultValueStack.pop();
         this._inputTypeStack.pop();
         break;
+      case Kind.FRAGMENT_ARGUMENT: {
+        this._fragmentArgument = null;
+        this._defaultValueStack.pop();
+        this._inputTypeStack.pop();
+        break;
+      }
       case Kind.LIST:
       case Kind.OBJECT_FIELD:
         this._defaultValueStack.pop();
@@ -368,6 +362,25 @@ function getFieldDef(
   return schema.getField(parentType, fieldNode.name.value);
 }
 
+function getFragmentSignatures(
+  document: DocumentNode,
+): Map<string, FragmentSignature> {
+  const fragmentSignatures = new Map<string, FragmentSignature>();
+  for (const definition of document.definitions) {
+    if (definition.kind === Kind.FRAGMENT_DEFINITION) {
+      const variableDefinitions = new Map<string, VariableDefinitionNode>();
+      if (definition.variableDefinitions) {
+        for (const varDef of definition.variableDefinitions) {
+          variableDefinitions.set(varDef.variable.name.value, varDef);
+        }
+      }
+      const signature = { definition, variableDefinitions };
+      fragmentSignatures.set(definition.name.value, signature);
+    }
+  }
+  return fragmentSignatures;
+}
+
 /**
  * Creates a new visitor instance which maintains a provided TypeInfo instance
  * along with visiting visitor.

src/utilities/__tests__/TypeInfo-test.ts

@@ -68,6 +68,9 @@ describe('TypeInfo', () => {
     expect(typeInfo.getFieldDef()).to.equal(undefined);
     expect(typeInfo.getDefaultValue()).to.equal(undefined);
     expect(typeInfo.getDirective()).to.equal(null);
+    expect(typeInfo.getFragmentSignature()).to.equal(null);
+    expect(typeInfo.getFragmentSignatureByName()('')).to.equal(null);
+    expect(typeInfo.getFragmentArgument()).to.equal(null);
     expect(typeInfo.getArgument()).to.equal(null);
     expect(typeInfo.getEnumValue()).to.equal(null);
   });

src/validation/ValidationContext.ts

@@ -27,15 +27,15 @@ import type {
 import type { GraphQLDirective } from '../type/directives.js';
 import type { GraphQLSchema } from '../type/schema.js';
 
-import type { TypeInfo } from '../utilities/TypeInfo.js';
-import { visitWithTypeInfo } from '../utilities/TypeInfo.js';
+import type { FragmentSignature } from '../utilities/TypeInfo.js';
+import { TypeInfo, visitWithTypeInfo } from '../utilities/TypeInfo.js';
 
 type NodeWithSelectionSet = OperationDefinitionNode | FragmentDefinitionNode;
 interface VariableUsage {
   readonly node: VariableNode;
   readonly type: Maybe<GraphQLInputType>;
   readonly defaultValue: Maybe<unknown>;
-  readonly fragmentVarDef: Maybe<VariableDefinitionNode>;
+  readonly fragmentVariableDefinition: Maybe<VariableDefinitionNode>;
 }
 
 /**
@@ -202,25 +202,41 @@ export class ValidationContext extends ASTValidationContext {
     let usages = this._variableUsages.get(node);
     if (!usages) {
       const newUsages: Array<VariableUsage> = [];
-      const typeInfo = this._typeInfo;
-      const fragmentVariableDefinitions =
-        node.kind === Kind.FRAGMENT_DEFINITION
-          ? node.variableDefinitions
-          : undefined;
+      const typeInfo = new TypeInfo(
+        this._schema,
+        undefined,
+        undefined,
+        this._typeInfo.getFragmentSignatureByName(),
+      );
+      const fragmentDefinition =
+        node.kind === Kind.FRAGMENT_DEFINITION ? node : undefined;
       visit(
         node,
         visitWithTypeInfo(typeInfo, {
           VariableDefinition: () => false,
           Variable(variable) {
-            const fragmentVarDef = fragmentVariableDefinitions?.find(
-              (varDef) => varDef.variable.name.value === variable.name.value,
-            );
-            newUsages.push({
-              node: variable,
-              type: typeInfo.getInputType(),
-              defaultValue: typeInfo.getDefaultValue(),
-              fragmentVarDef,
-            });
+            let fragmentVariableDefinition;
+            if (fragmentDefinition) {
+              const fragmentSignature = typeInfo.getFragmentSignatureByName()(
+                fragmentDefinition.name.value,
+              );
+
+              fragmentVariableDefinition =
+                fragmentSignature?.variableDefinitions.get(variable.name.value);
+              newUsages.push({
+                node: variable,
+                type: typeInfo.getInputType(),
+                defaultValue: undefined, // fragment variables have a variable default but no location default, which is what this default value represents
+                fragmentVariableDefinition,
+              });
+            } else {
+              newUsages.push({
+                node: variable,
+                type: typeInfo.getInputType(),
+                defaultValue: typeInfo.getDefaultValue(),
+                fragmentVariableDefinition: undefined,
+              });
+            }
           },
         }),
       );
@@ -244,20 +260,6 @@ export class ValidationContext extends ASTValidationContext {
     return usages;
   }
 
-  getOperationVariableUsages(
-    operation: OperationDefinitionNode,
-  ): ReadonlyArray<VariableUsage> {
-    let usages = this._recursiveVariableUsages.get(operation);
-    if (!usages) {
-      usages = this.getVariableUsages(operation);
-      for (const frag of this.getRecursivelyReferencedFragments(operation)) {
-        usages = usages.concat(this.getVariableUsages(frag));
-      }
-      this._recursiveVariableUsages.set(operation, usages);
-    }
-    return usages.filter(({ fragmentVarDef }) => !fragmentVarDef);
-  }
-
   getType(): Maybe<GraphQLOutputType> {
     return this._typeInfo.getType();
   }
@@ -286,6 +288,10 @@ export class ValidationContext extends ASTValidationContext {
     return this._typeInfo.getArgument();
   }
 
+  getFragmentSignature(): Maybe<FragmentSignature> {
+    return this._typeInfo.getFragmentSignature();
+  }
+
   getEnumValue(): Maybe<GraphQLEnumValue> {
     return this._typeInfo.getEnumValue();
   }