From 953adaa60e6b2d6f722acfae387d6c72ff53aa81 Mon Sep 17 00:00:00 2001 From: Adam Miskiewicz Date: Mon, 5 Aug 2019 20:05:22 -0700 Subject: [PATCH 1/3] [validation] Add "onError" option to allow for custom error handling behavior when performing validation --- src/validation/ValidationContext.js | 25 ++++++++++++++------- src/validation/__tests__/validation-test.js | 25 +++++++++++++++++++++ src/validation/validate.js | 16 ++++++++++--- 3 files changed, 55 insertions(+), 11 deletions(-) diff --git a/src/validation/ValidationContext.js b/src/validation/ValidationContext.js index 26400c33f1..ccead9669a 100644 --- a/src/validation/ValidationContext.js +++ b/src/validation/ValidationContext.js @@ -8,21 +8,21 @@ import { Kind } from '../language/kinds'; import { type ASTVisitor, visit, visitWithTypeInfo } from '../language/visitor'; import { type DocumentNode, + type FragmentDefinitionNode, + type FragmentSpreadNode, type OperationDefinitionNode, - type VariableNode, type SelectionSetNode, - type FragmentSpreadNode, - type FragmentDefinitionNode, + type VariableNode, } from '../language/ast'; import { type GraphQLSchema } from '../type/schema'; import { type GraphQLDirective } from '../type/directives'; import { - type GraphQLInputType, - type GraphQLOutputType, + type GraphQLArgument, type GraphQLCompositeType, type GraphQLField, - type GraphQLArgument, + type GraphQLInputType, + type GraphQLOutputType, } from '../type/definition'; import { TypeInfo } from '../utilities/TypeInfo'; @@ -41,6 +41,7 @@ type VariableUsage = {| */ export class ASTValidationContext { _ast: DocumentNode; + _onError: ?(err: Error) => void; _errors: Array; _fragments: ?ObjMap; _fragmentSpreads: Map>; @@ -49,16 +50,23 @@ export class ASTValidationContext { $ReadOnlyArray, >; - constructor(ast: DocumentNode): void { + constructor(ast: DocumentNode, onError?: (err: Error) => void): void { this._ast = ast; this._errors = []; this._fragments = undefined; this._fragmentSpreads = new Map(); this._recursivelyReferencedFragments = new Map(); + this._onError = undefined; + if (onError) { + this._onError = onError.bind(this); + } } reportError(error: GraphQLError): void { this._errors.push(error); + if (this._onError) { + this._onError(error); + } } getErrors(): $ReadOnlyArray { @@ -165,8 +173,9 @@ export class ValidationContext extends ASTValidationContext { schema: GraphQLSchema, ast: DocumentNode, typeInfo: TypeInfo, + onError?: (err: Error) => void, ): void { - super(ast); + super(ast, onError); this._schema = schema; this._typeInfo = typeInfo; this._variableUsages = new Map(); diff --git a/src/validation/__tests__/validation-test.js b/src/validation/__tests__/validation-test.js index 067255e2fa..75cc74c9c7 100644 --- a/src/validation/__tests__/validation-test.js +++ b/src/validation/__tests__/validation-test.js @@ -74,4 +74,29 @@ describe('Validate: Supports full validation', () => { 'Cannot query field "isHousetrained" on type "Dog". Did you mean "isHousetrained"?', ]); }); + + it('properly calls onError callback when passed', () => { + const doc = parse(` + query { + cat { + name + someNonExistentField + } + dog { + name + anotherNonExistentField + } + } + `); + + const expectedNumberOfErrors = 2; + let errorCount = 0; + validate(testSchema, doc, specifiedRules, undefined, (err, ctx) => { + expect(err).to.not.be.a('null'); + expect(ctx).to.not.be.a('null'); + expect(ctx.getErrors()).to.be.length(++errorCount); + }); + + expect(errorCount).to.be.equal(expectedNumberOfErrors); + }); }); diff --git a/src/validation/validate.js b/src/validation/validate.js index 4da3e6d9c7..450cfbb5f5 100644 --- a/src/validation/validate.js +++ b/src/validation/validate.js @@ -14,10 +14,10 @@ import { TypeInfo } from '../utilities/TypeInfo'; import { specifiedRules, specifiedSDLRules } from './specifiedRules'; import { - type SDLValidationRule, - type ValidationRule, SDLValidationContext, + type SDLValidationRule, ValidationContext, + type ValidationRule, } from './ValidationContext'; /** @@ -41,12 +41,22 @@ export function validate( documentAST: DocumentNode, rules?: $ReadOnlyArray = specifiedRules, typeInfo?: TypeInfo = new TypeInfo(schema), + onError?: (err: Error, ctx: ValidationContext) => void, ): $ReadOnlyArray { devAssert(documentAST, 'Must provide document'); // If the schema used for validation is invalid, throw an error. assertValidSchema(schema); - const context = new ValidationContext(schema, documentAST, typeInfo); + const context = new ValidationContext( + schema, + documentAST, + typeInfo, + function onErrorWithContext(err) { + if (onError) { + onError(err, this); + } + }, + ); // This uses a specialized visitor which runs multiple visitors in parallel, // while maintaining the visitor skip and break API. const visitor = visitInParallel(rules.map(rule => rule(context))); From 1e374309487652c4aa8b7a4f20158d738ed45ad6 Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Wed, 7 Aug 2019 13:25:59 +0300 Subject: [PATCH 2/3] Reverent unrelated changes in order of imports --- src/validation/ValidationContext.js | 12 ++++++------ src/validation/validate.js | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/validation/ValidationContext.js b/src/validation/ValidationContext.js index ccead9669a..b9e7880428 100644 --- a/src/validation/ValidationContext.js +++ b/src/validation/ValidationContext.js @@ -8,21 +8,21 @@ import { Kind } from '../language/kinds'; import { type ASTVisitor, visit, visitWithTypeInfo } from '../language/visitor'; import { type DocumentNode, - type FragmentDefinitionNode, - type FragmentSpreadNode, type OperationDefinitionNode, - type SelectionSetNode, type VariableNode, + type SelectionSetNode, + type FragmentSpreadNode, + type FragmentDefinitionNode, } from '../language/ast'; import { type GraphQLSchema } from '../type/schema'; import { type GraphQLDirective } from '../type/directives'; import { - type GraphQLArgument, - type GraphQLCompositeType, - type GraphQLField, type GraphQLInputType, type GraphQLOutputType, + type GraphQLCompositeType, + type GraphQLField, + type GraphQLArgument, } from '../type/definition'; import { TypeInfo } from '../utilities/TypeInfo'; diff --git a/src/validation/validate.js b/src/validation/validate.js index 450cfbb5f5..7fb1abffcd 100644 --- a/src/validation/validate.js +++ b/src/validation/validate.js @@ -14,10 +14,10 @@ import { TypeInfo } from '../utilities/TypeInfo'; import { specifiedRules, specifiedSDLRules } from './specifiedRules'; import { - SDLValidationContext, type SDLValidationRule, - ValidationContext, type ValidationRule, + SDLValidationContext, + ValidationContext, } from './ValidationContext'; /** From 9ff76adc718feb63bcabb0dd42a9577a10fbc4c0 Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Tue, 20 Aug 2019 19:29:41 +0300 Subject: [PATCH 3/3] Swithch to `maxErrors` and code cleanup --- src/validation/ValidationContext.js | 20 +++---- src/validation/__tests__/validation-test.js | 60 +++++++++++++-------- src/validation/validate.js | 45 ++++++++++++---- 3 files changed, 86 insertions(+), 39 deletions(-) diff --git a/src/validation/ValidationContext.js b/src/validation/ValidationContext.js index b9e7880428..a3a7bd0f08 100644 --- a/src/validation/ValidationContext.js +++ b/src/validation/ValidationContext.js @@ -41,7 +41,7 @@ type VariableUsage = {| */ export class ASTValidationContext { _ast: DocumentNode; - _onError: ?(err: Error) => void; + _onError: ?(err: GraphQLError) => void; _errors: Array; _fragments: ?ObjMap; _fragmentSpreads: Map>; @@ -50,16 +50,13 @@ export class ASTValidationContext { $ReadOnlyArray, >; - constructor(ast: DocumentNode, onError?: (err: Error) => void): void { + constructor(ast: DocumentNode, onError?: (err: GraphQLError) => void): void { this._ast = ast; this._errors = []; this._fragments = undefined; this._fragmentSpreads = new Map(); this._recursivelyReferencedFragments = new Map(); - this._onError = undefined; - if (onError) { - this._onError = onError.bind(this); - } + this._onError = onError; } reportError(error: GraphQLError): void { @@ -69,6 +66,7 @@ export class ASTValidationContext { } } + // @deprecated: use onError callback instead - will be removed in v15. getErrors(): $ReadOnlyArray { return this._errors; } @@ -148,8 +146,12 @@ export type ASTValidationRule = ASTValidationContext => ASTVisitor; export class SDLValidationContext extends ASTValidationContext { _schema: ?GraphQLSchema; - constructor(ast: DocumentNode, schema?: ?GraphQLSchema): void { - super(ast); + constructor( + ast: DocumentNode, + schema: ?GraphQLSchema, + onError: (err: GraphQLError) => void, + ): void { + super(ast, onError); this._schema = schema; } @@ -173,7 +175,7 @@ export class ValidationContext extends ASTValidationContext { schema: GraphQLSchema, ast: DocumentNode, typeInfo: TypeInfo, - onError?: (err: Error) => void, + onError?: (err: GraphQLError) => void, ): void { super(ast, onError); this._schema = schema; diff --git a/src/validation/__tests__/validation-test.js b/src/validation/__tests__/validation-test.js index 75cc74c9c7..4b1be1f6c4 100644 --- a/src/validation/__tests__/validation-test.js +++ b/src/validation/__tests__/validation-test.js @@ -74,29 +74,47 @@ describe('Validate: Supports full validation', () => { 'Cannot query field "isHousetrained" on type "Dog". Did you mean "isHousetrained"?', ]); }); +}); - it('properly calls onError callback when passed', () => { - const doc = parse(` - query { - cat { - name - someNonExistentField - } - dog { - name - anotherNonExistentField - } - } - `); +describe('Validate: Limit maximum number of validation errors', () => { + const query = ` + { + firstUnknownField + secondUnknownField + thirdUnknownField + } + `; + const doc = parse(query, { noLocation: true }); + + function validateDocument(options) { + return validate(testSchema, doc, undefined, undefined, options); + } - const expectedNumberOfErrors = 2; - let errorCount = 0; - validate(testSchema, doc, specifiedRules, undefined, (err, ctx) => { - expect(err).to.not.be.a('null'); - expect(ctx).to.not.be.a('null'); - expect(ctx.getErrors()).to.be.length(++errorCount); - }); + function invalidFieldError(fieldName) { + return { + message: `Cannot query field "${fieldName}" on type "QueryRoot".`, + locations: [], + }; + } - expect(errorCount).to.be.equal(expectedNumberOfErrors); + it('when maxErrors is equal to number of errors', () => { + const errors = validateDocument({ maxErrors: 3 }); + expect(errors).to.be.deep.equal([ + invalidFieldError('firstUnknownField'), + invalidFieldError('secondUnknownField'), + invalidFieldError('thirdUnknownField'), + ]); + }); + + it('when maxErrors is less than number of errors', () => { + const errors = validateDocument({ maxErrors: 2 }); + expect(errors).to.be.deep.equal([ + invalidFieldError('firstUnknownField'), + invalidFieldError('secondUnknownField'), + { + message: + 'Too many validation errors, error limit reached. Validation aborted.', + }, + ]); }); }); diff --git a/src/validation/validate.js b/src/validation/validate.js index 7fb1abffcd..1ccdd40c59 100644 --- a/src/validation/validate.js +++ b/src/validation/validate.js @@ -2,7 +2,7 @@ import devAssert from '../jsutils/devAssert'; -import { type GraphQLError } from '../error/GraphQLError'; +import { GraphQLError } from '../error/GraphQLError'; import { type DocumentNode } from '../language/ast'; import { visit, visitInParallel, visitWithTypeInfo } from '../language/visitor'; @@ -20,6 +20,8 @@ import { ValidationContext, } from './ValidationContext'; +export const ABORT_VALIDATION = Object.freeze({}); + /** * Implements the "Validation" section of the spec. * @@ -41,28 +43,45 @@ export function validate( documentAST: DocumentNode, rules?: $ReadOnlyArray = specifiedRules, typeInfo?: TypeInfo = new TypeInfo(schema), - onError?: (err: Error, ctx: ValidationContext) => void, + options?: {| maxErrors?: number |}, ): $ReadOnlyArray { devAssert(documentAST, 'Must provide document'); // If the schema used for validation is invalid, throw an error. assertValidSchema(schema); + const abortObj = Object.freeze({}); + const errors = []; + const maxErrors = options && options.maxErrors; const context = new ValidationContext( schema, documentAST, typeInfo, - function onErrorWithContext(err) { - if (onError) { - onError(err, this); + error => { + if (maxErrors != null && errors.length >= maxErrors) { + errors.push( + new GraphQLError( + 'Too many validation errors, error limit reached. Validation aborted.', + ), + ); + throw abortObj; } + errors.push(error); }, ); + // This uses a specialized visitor which runs multiple visitors in parallel, // while maintaining the visitor skip and break API. const visitor = visitInParallel(rules.map(rule => rule(context))); + // Visit the whole document with each instance of all provided rules. - visit(documentAST, visitWithTypeInfo(typeInfo, visitor)); - return context.getErrors(); + try { + visit(documentAST, visitWithTypeInfo(typeInfo, visitor)); + } catch (e) { + if (e !== abortObj) { + throw e; + } + } + return errors; } // @internal @@ -71,10 +90,18 @@ export function validateSDL( schemaToExtend?: ?GraphQLSchema, rules?: $ReadOnlyArray = specifiedSDLRules, ): $ReadOnlyArray { - const context = new SDLValidationContext(documentAST, schemaToExtend); + const errors = []; + const context = new SDLValidationContext( + documentAST, + schemaToExtend, + error => { + errors.push(error); + }, + ); + const visitors = rules.map(rule => rule(context)); visit(documentAST, visitInParallel(visitors)); - return context.getErrors(); + return errors; } /**