Revert change requiring config flag & more specific allowedInPosition…

… definition Based on discussion with @dschafer Adds getDefaultValue() to TypeInfo so the default value at any position in an AST visit is known.
graphql · Apr 18, 2018 · e38d042 · e38d042
1 parent 3a5602c
commit e38d042
Show file tree

Hide file tree

Showing 10 changed files with 96 additions and 128 deletions.
diff --git a/src/graphql.js b/src/graphql.js
@@ -9,14 +9,13 @@
 
 import { validateSchema } from './type/validate';
 import { parse } from './language/parser';
-import { validate, specifiedRules } from './validation';
+import { validate } from './validation/validate';
 import { execute } from './execution/execute';
 import type { ObjMap } from './jsutils/ObjMap';
-import type { ParseOptions, Source } from './language';
+import type { Source } from './language/source';
 import type { GraphQLFieldResolver } from './type/definition';
 import type { GraphQLSchema } from './type/schema';
 import type { ExecutionResult } from './execution/execute';
-import type { ValidationOptions } from './validation';
 import type { MaybePromise } from './jsutils/MaybePromise';
 
 /**
@@ -48,9 +47,6 @@ import type { MaybePromise } from './jsutils/MaybePromise';
  *    A resolver function to use when one is not provided by the schema.
  *    If not provided, the default field resolver is used (which looks for a
  *    value or method on the source value with the field's name).
- * options:
- *    An additional set of flags which enable legacy, compataibility, or
- *    experimental behavior.
  */
 export type GraphQLArgs = {|
   schema: GraphQLSchema,
@@ -60,7 +56,6 @@ export type GraphQLArgs = {|
   variableValues?: ?ObjMap<mixed>,
   operationName?: ?string,
   fieldResolver?: ?GraphQLFieldResolver<any, any>,
-  options?: ParseOptions & ValidationOptions,
 |};
 declare function graphql(GraphQLArgs, ..._: []): Promise<ExecutionResult>;
 /* eslint-disable no-redeclare */
@@ -72,7 +67,6 @@ declare function graphql(
   variableValues?: ?ObjMap<mixed>,
   operationName?: ?string,
   fieldResolver?: ?GraphQLFieldResolver<any, any>,
-  options?: ParseOptions & ValidationOptions,
 ): Promise<ExecutionResult>;
 export function graphql(
   argsOrSchema,
@@ -82,7 +76,6 @@ export function graphql(
   variableValues,
   operationName,
   fieldResolver,
-  options,
 ) {
   /* eslint-enable no-redeclare */
   // Always return a Promise for a consistent API.
@@ -98,7 +91,6 @@ export function graphql(
             argsOrSchema.variableValues,
             argsOrSchema.operationName,
             argsOrSchema.fieldResolver,
-            argsOrSchema.options,
           )
         : graphqlImpl(
             argsOrSchema,
@@ -108,7 +100,6 @@ export function graphql(
             variableValues,
             operationName,
             fieldResolver,
-            options,
           ),
     ),
   );
@@ -130,7 +121,6 @@ declare function graphqlSync(
   variableValues?: ?ObjMap<mixed>,
   operationName?: ?string,
   fieldResolver?: ?GraphQLFieldResolver<any, any>,
-  options?: ParseOptions & ValidationOptions,
 ): ExecutionResult;
 export function graphqlSync(
   argsOrSchema,
@@ -140,7 +130,6 @@ export function graphqlSync(
   variableValues,
   operationName,
   fieldResolver,
-  options,
 ) {
   // Extract arguments from object args if provided.
   const result =
@@ -153,7 +142,6 @@ export function graphqlSync(
           argsOrSchema.variableValues,
           argsOrSchema.operationName,
           argsOrSchema.fieldResolver,
-          argsOrSchema.options,
         )
       : graphqlImpl(
           argsOrSchema,
@@ -163,7 +151,6 @@ export function graphqlSync(
           variableValues,
           operationName,
           fieldResolver,
-          options,
         );
 
   // Assert that the execution was synchronous.
@@ -182,7 +169,6 @@ function graphqlImpl(
   variableValues,
   operationName,
   fieldResolver,
-  options,
 ): MaybePromise<ExecutionResult> {
   // Validate Schema
   const schemaValidationErrors = validateSchema(schema);
@@ -193,13 +179,13 @@ function graphqlImpl(
   // Parse
   let document;
   try {
-    document = parse(source, options);
+    document = parse(source);
   } catch (syntaxError) {
     return { errors: [syntaxError] };
   }
 
   // Validate
-  const validationErrors = validate(schema, document, specifiedRules, options);
+  const validationErrors = validate(schema, document);
   if (validationErrors.length > 0) {
     return { errors: validationErrors };
   }

diff --git a/src/index.js b/src/index.js
@@ -268,7 +268,6 @@ export { subscribe, createSourceEventStream } from './subscription';
 // Validate GraphQL queries.
 export {
   validate,
-  ValidationOptions,
   ValidationContext,
   // All validation rules in the GraphQL Specification.
   specifiedRules,

diff --git a/src/utilities/TypeInfo.js b/src/utilities/TypeInfo.js
@@ -27,6 +27,7 @@ import type {
   GraphQLCompositeType,
   GraphQLField,
   GraphQLArgument,
+  GraphQLInputField,
   GraphQLEnumValue,
 } from '../type/definition';
 import type { GraphQLDirective } from '../type/directives';
@@ -51,6 +52,7 @@ export class TypeInfo {
   _parentTypeStack: Array<?GraphQLCompositeType>;
   _inputTypeStack: Array<?GraphQLInputType>;
   _fieldDefStack: Array<?GraphQLField<*, *>>;
+  _defaultValueStack: Array<?mixed>;
   _directive: ?GraphQLDirective;
   _argument: ?GraphQLArgument;
   _enumValue: ?GraphQLEnumValue;
@@ -71,6 +73,7 @@ export class TypeInfo {
     this._parentTypeStack = [];
     this._inputTypeStack = [];
     this._fieldDefStack = [];
+    this._defaultValueStack = [];
     this._directive = null;
     this._argument = null;
     this._enumValue = null;
@@ -118,6 +121,12 @@ export class TypeInfo {
     }
   }
 
+  getDefaultValue(): ?mixed {
+    if (this._defaultValueStack.length > 0) {
+      return this._defaultValueStack[this._defaultValueStack.length - 1];
+    }
+  }
+
   getDirective(): ?GraphQLDirective {
     return this._directive;
   }
@@ -199,24 +208,31 @@ export class TypeInfo {
           }
         }
         this._argument = argDef;
+        this._defaultValueStack.push(argDef ? argDef.defaultValue : undefined);
         this._inputTypeStack.push(isInputType(argType) ? argType : undefined);
         break;
       case Kind.LIST:
         const listType: mixed = getNullableType(this.getInputType());
         const itemType: mixed = isListType(listType)
           ? listType.ofType
           : listType;
+        // List positions never have a default value.
+        this._defaultValueStack.push(undefined);
         this._inputTypeStack.push(isInputType(itemType) ? itemType : undefined);
         break;
       case Kind.OBJECT_FIELD:
         const objectType: mixed = getNamedType(this.getInputType());
-        let inputFieldType: mixed;
+        let inputFieldType: GraphQLInputType | void;
+        let inputField: GraphQLInputField | void;
         if (isInputObjectType(objectType)) {
-          const inputField = objectType.getFields()[node.name.value];
+          inputField = objectType.getFields()[node.name.value];
           if (inputField) {
             inputFieldType = inputField.type;
           }
         }
+        this._defaultValueStack.push(
+          inputField ? inputField.defaultValue : undefined,
+        );
         this._inputTypeStack.push(
           isInputType(inputFieldType) ? inputFieldType : undefined,
         );
@@ -254,10 +270,12 @@ export class TypeInfo {
         break;
       case Kind.ARGUMENT:
         this._argument = null;
+        this._defaultValueStack.pop();
         this._inputTypeStack.pop();
         break;
       case Kind.LIST:
       case Kind.OBJECT_FIELD:
+        this._defaultValueStack.pop();
         this._inputTypeStack.pop();
         break;
       case Kind.ENUM:

diff --git a/src/validation/ValidationContext.js b/src/validation/ValidationContext.js
@@ -31,22 +31,11 @@ import type { GraphQLDirective } from '../type/directives';
 import { TypeInfo } from '../utilities/TypeInfo';
 
 type NodeWithSelectionSet = OperationDefinitionNode | FragmentDefinitionNode;
-type VariableUsage = { node: VariableNode, type: ?GraphQLInputType };
-
-/**
- * Configuration options to control validation behavior.
- */
-export type ValidationOptions = {
-  /**
-   * If enabled, validation will allow nullable variables with default values
-   * to be used in non-null positions. Earlier versions explicitly allowed such
-   * operations due to a slightly different interpretation of default values and
-   * null values. GraphQL services accepting operations written before this
-   * version may continue to allow such operations by enabling this option,
-   * however GraphQL services established after this version should not.
-   */
-  allowNullableVariablesInNonNullPositions?: boolean,
-};
+type VariableUsage = {|
+  +node: VariableNode,
+  +type: ?GraphQLInputType,
+  +defaultValue: ?mixed,
+|};
 
 /**
  * An instance of this class is passed as the "this" context to all validators,
@@ -57,7 +46,6 @@ export default class ValidationContext {
   _schema: GraphQLSchema;
   _ast: DocumentNode;
   _typeInfo: TypeInfo;
-  options: ValidationOptions;
   _errors: Array<GraphQLError>;
   _fragments: ObjMap<FragmentDefinitionNode>;
   _fragmentSpreads: Map<SelectionSetNode, $ReadOnlyArray<FragmentSpreadNode>>;
@@ -75,12 +63,10 @@ export default class ValidationContext {
     schema: GraphQLSchema,
     ast: DocumentNode,
     typeInfo: TypeInfo,
-    options?: ValidationOptions,
   ): void {
     this._schema = schema;
     this._ast = ast;
     this._typeInfo = typeInfo;
-    this.options = options || {};
     this._errors = [];
     this._fragmentSpreads = new Map();
     this._recursivelyReferencedFragments = new Map();
@@ -181,7 +167,11 @@ export default class ValidationContext {
         visitWithTypeInfo(typeInfo, {
           VariableDefinition: () => false,
           Variable(variable) {
-            newUsages.push({ node: variable, type: typeInfo.getInputType() });
+            newUsages.push({
+              node: variable,
+              type: typeInfo.getInputType(),
+              defaultValue: typeInfo.getDefaultValue(),
+            });
           },
         }),
       );

diff --git a/src/validation/__tests__/VariablesInAllowedPosition-test.js b/src/validation/__tests__/VariablesInAllowedPosition-test.js
@@ -6,12 +6,7 @@
  */
 
 import { describe, it } from 'mocha';
-import {
-  expectPassesRule,
-  expectFailsRule,
-  expectPassesRuleWithOptions,
-  expectFailsRuleWithOptions,
-} from './harness';
+import { expectPassesRule, expectFailsRule } from './harness';
 import {
   VariablesInAllowedPosition,
   badVarPosMessage,
@@ -353,71 +348,56 @@ describe('Validate: Variables are in allowed positions', () => {
     );
   });
 
-  describe('allowNullableVariablesInNonNullPositions', () => {
-    it('Int => Int! with non-null default value without option', () => {
+  describe('Allows optional (nullable) variables with default values', () => {
+    it('Int => Int! fails when variable provides null default value', () => {
       expectFailsRule(
         VariablesInAllowedPosition,
         `
-        query Query($intVar: Int = 1) {
+        query Query($intVar: Int = null) {
           complicatedArgs {
             nonNullIntArgField(nonNullIntArg: $intVar)
           }
-        }
-      `,
+        }`,
         [
           {
             message: badVarPosMessage('intVar', 'Int', 'Int!'),
             locations: [{ line: 2, column: 21 }, { line: 4, column: 47 }],
-            path: undefined,
           },
         ],
       );
     });
 
-    it('Int => Int! with null default value fails with option', () => {
-      expectFailsRuleWithOptions(
-        { allowNullableVariablesInNonNullPositions: true },
+    it('Int => Int! when variable provides non-null default value', () => {
+      expectPassesRule(
         VariablesInAllowedPosition,
         `
-          query Query($intVar: Int = null) {
-            complicatedArgs {
-              nonNullIntArgField(nonNullIntArg: $intVar)
-            }
+        query Query($intVar: Int = 1) {
+          complicatedArgs {
+            nonNullIntArgField(nonNullIntArg: $intVar)
           }
-        `,
-        [
-          {
-            message: badVarPosMessage('intVar', 'Int', 'Int!'),
-            locations: [{ line: 2, column: 23 }, { line: 4, column: 49 }],
-            path: undefined,
-          },
-        ],
+        }`,
       );
     });
 
-    it('Int => Int! with non-null default value with option', () => {
-      expectPassesRuleWithOptions(
-        { allowNullableVariablesInNonNullPositions: true },
+    it('Int => Int! when optional argument provides default value', () => {
+      expectPassesRule(
         VariablesInAllowedPosition,
         `
-        query Query($intVar: Int = 1) {
+        query Query($intVar: Int) {
           complicatedArgs {
-            nonNullIntArgField(nonNullIntArg: $intVar)
+            nonNullFieldWithDefault(nonNullIntArg: $intVar)
           }
-        }
-      `,
+        }`,
       );
     });
 
     it('Boolean => Boolean! in directive with default value with option', () => {
-      expectPassesRuleWithOptions(
-        { allowNullableVariablesInNonNullPositions: true },
+      expectPassesRule(
         VariablesInAllowedPosition,
         `
-      query Query($boolVar: Boolean = false) {
-        dog @include(if: $boolVar)
-      }
-    `,
+        query Query($boolVar: Boolean = false) {
+          dog @include(if: $boolVar)
+        }`,
       );
     });
   });