From 641b3f2ecb4e0b8232417acc4a50bb654e197202 Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Sun, 12 Aug 2018 20:47:52 +0300 Subject: [PATCH] Validate directive arguments inside SDL --- .../__tests__/KnownArgumentNames-test.js | 98 ++++++++++++++++- .../ProvidedRequiredArguments-test.js | 88 ++++++++++++++- src/validation/rules/KnownArgumentNames.js | 101 ++++++++++++------ src/validation/rules/KnownDirectives.js | 1 + .../rules/ProvidedRequiredArguments.js | 92 +++++++++++----- src/validation/specifiedRules.js | 4 + 6 files changed, 321 insertions(+), 63 deletions(-) diff --git a/src/validation/__tests__/KnownArgumentNames-test.js b/src/validation/__tests__/KnownArgumentNames-test.js index b0d155021d..8c8f85d51a 100644 --- a/src/validation/__tests__/KnownArgumentNames-test.js +++ b/src/validation/__tests__/KnownArgumentNames-test.js @@ -6,13 +6,24 @@ */ import { describe, it } from 'mocha'; -import { expectPassesRule, expectFailsRule } from './harness'; +import { buildSchema } from '../../utilities'; +import { + expectPassesRule, + expectFailsRule, + expectSDLErrorsFromRule, +} from './harness'; import { KnownArgumentNames, + KnownArgumentNamesOnDirectives, unknownArgMessage, unknownDirectiveArgMessage, } from '../rules/KnownArgumentNames'; +const expectSDLErrors = expectSDLErrorsFromRule.bind( + undefined, + KnownArgumentNamesOnDirectives, +); + function unknownArg(argName, fieldName, typeName, suggestedArgs, line, column) { return { message: unknownArgMessage(argName, fieldName, typeName, suggestedArgs), @@ -215,4 +226,89 @@ describe('Validate: Known argument names', () => { ], ); }); + + describe('within SDL', () => { + it('known arg on directive defined inside SDL', () => { + expectSDLErrors(` + type Query { + foo: String @test(arg: "") + } + + directive @test(arg: String) on FIELD_DEFINITION + `).to.deep.equal([]); + }); + + it('unknown arg on directive defined inside SDL', () => { + expectSDLErrors(` + type Query { + foo: String @test(unknown: "") + } + + directive @test(arg: String) on FIELD_DEFINITION + `).to.deep.equal([unknownDirectiveArg('unknown', 'test', [], 3, 29)]); + }); + + it('misspelled arg name is reported on directive defined inside SDL', () => { + expectSDLErrors(` + type Query { + foo: String @test(agr: "") + } + + directive @test(arg: String) on FIELD_DEFINITION + `).to.deep.equal([unknownDirectiveArg('agr', 'test', ['arg'], 3, 29)]); + }); + + it('unknown arg on standard directive', () => { + expectSDLErrors(` + type Query { + foo: String @deprecated(unknown: "") + } + `).to.deep.equal([ + unknownDirectiveArg('unknown', 'deprecated', [], 3, 35), + ]); + }); + + it('unknown arg on overrided standard directive', () => { + expectSDLErrors(` + type Query { + foo: String @deprecated(reason: "") + } + directive @deprecated(arg: String) on FIELD + `).to.deep.equal([ + unknownDirectiveArg('reason', 'deprecated', [], 3, 35), + ]); + }); + + it('unknown arg on directive defined in schema extension', () => { + const schema = buildSchema(` + type Query { + foo: String + } + `); + expectSDLErrors( + ` + directive @test(arg: String) on OBJECT + + extend type Query @test(unknown: "") + `, + schema, + ).to.deep.equal([unknownDirectiveArg('unknown', 'test', [], 4, 36)]); + }); + + it('unknown arg on directive used in schema extension', () => { + const schema = buildSchema(` + directive @test(arg: String) on OBJECT + + type Query { + foo: String + } + `); + expectSDLErrors( + ` + extend type Query @test(unknown: "") + `, + schema, + ).to.deep.equal([unknownDirectiveArg('unknown', 'test', [], 2, 35)]); + }); + }); }); diff --git a/src/validation/__tests__/ProvidedRequiredArguments-test.js b/src/validation/__tests__/ProvidedRequiredArguments-test.js index f003aab2a9..b03b3a299d 100644 --- a/src/validation/__tests__/ProvidedRequiredArguments-test.js +++ b/src/validation/__tests__/ProvidedRequiredArguments-test.js @@ -6,13 +6,24 @@ */ import { describe, it } from 'mocha'; -import { expectPassesRule, expectFailsRule } from './harness'; +import { buildSchema } from '../../utilities'; +import { + expectPassesRule, + expectFailsRule, + expectSDLErrorsFromRule, +} from './harness'; import { ProvidedRequiredArguments, + ProvidedRequiredArgumentsOnDirectives, missingFieldArgMessage, missingDirectiveArgMessage, } from '../rules/ProvidedRequiredArguments'; +const expectSDLErrors = expectSDLErrorsFromRule.bind( + undefined, + ProvidedRequiredArgumentsOnDirectives, +); + function missingFieldArg(fieldName, argName, typeName, line, column) { return { message: missingFieldArgMessage(fieldName, argName, typeName), @@ -278,4 +289,79 @@ describe('Validate: Provided required arguments', () => { ); }); }); + + describe('within SDL', () => { + it('Missing optional args on directive defined inside SDL', () => { + expectSDLErrors(` + type Query { + foo: String @test + } + + directive @test(arg1: String, arg2: String! = "") on FIELD_DEFINITION + `).to.deep.equal([]); + }); + + it('Missing arg on directive defined inside SDL', () => { + expectSDLErrors(` + type Query { + foo: String @test + } + + directive @test(arg: String!) on FIELD_DEFINITION + `).to.deep.equal([missingDirectiveArg('test', 'arg', 'String!', 3, 23)]); + }); + + it('Missing arg on standard directive', () => { + expectSDLErrors(` + type Query { + foo: String @include + } + `).to.deep.equal([ + missingDirectiveArg('include', 'if', 'Boolean!', 3, 23), + ]); + }); + + it('Missing arg on overrided standard directive', () => { + expectSDLErrors(` + type Query { + foo: String @deprecated + } + directive @deprecated(reason: String!) on FIELD + `).to.deep.equal([ + missingDirectiveArg('deprecated', 'reason', 'String!', 3, 23), + ]); + }); + + it('Missing arg on directive defined in schema extension', () => { + const schema = buildSchema(` + type Query { + foo: String + } + `); + expectSDLErrors( + ` + directive @test(arg: String!) on OBJECT + + extend type Query @test + `, + schema, + ).to.deep.equal([missingDirectiveArg('test', 'arg', 'String!', 4, 30)]); + }); + + it('Missing arg on directive used in schema extension', () => { + const schema = buildSchema(` + directive @test(arg: String!) on OBJECT + + type Query { + foo: String + } + `); + expectSDLErrors( + ` + extend type Query @test + `, + schema, + ).to.deep.equal([missingDirectiveArg('test', 'arg', 'String!', 2, 29)]); + }); + }); }); diff --git a/src/validation/rules/KnownArgumentNames.js b/src/validation/rules/KnownArgumentNames.js index 25566af9ca..4c69bb81e6 100644 --- a/src/validation/rules/KnownArgumentNames.js +++ b/src/validation/rules/KnownArgumentNames.js @@ -7,12 +7,16 @@ * @flow strict */ -import type { ValidationContext } from '../ValidationContext'; +import type { + ValidationContext, + SDLValidationContext, +} from '../ValidationContext'; import { GraphQLError } from '../../error/GraphQLError'; import type { ASTVisitor } from '../../language/visitor'; import suggestionList from '../../jsutils/suggestionList'; import quotedOrList from '../../jsutils/quotedOrList'; import { Kind } from '../../language/kinds'; +import { specifiedDirectives } from '../../type/directives'; export function unknownArgMessage( argName: string, @@ -49,48 +53,75 @@ export function unknownDirectiveArgMessage( */ export function KnownArgumentNames(context: ValidationContext): ASTVisitor { return { - Argument(node, key, parent, path, ancestors) { + ...KnownArgumentNamesOnDirectives(context), + Argument(argNode) { const argDef = context.getArgument(); - if (!argDef) { - const argumentOf = ancestors[ancestors.length - 1]; - if (argumentOf.kind === Kind.FIELD) { - const fieldDef = context.getFieldDef(); - const parentType = context.getParentType(); - if (fieldDef && parentType) { - context.reportError( - new GraphQLError( - unknownArgMessage( - node.name.value, - fieldDef.name, - parentType.name, - suggestionList( - node.name.value, - fieldDef.args.map(arg => arg.name), - ), - ), - [node], - ), - ); - } - } else if (argumentOf.kind === Kind.DIRECTIVE) { - const directive = context.getDirective(); - if (directive) { + const fieldDef = context.getFieldDef(); + const parentType = context.getParentType(); + + if (!argDef && fieldDef && parentType) { + const argName = argNode.name.value; + const knownArgsNames = fieldDef.args.map(arg => arg.name); + context.reportError( + new GraphQLError( + unknownArgMessage( + argName, + fieldDef.name, + parentType.name, + suggestionList(argName, knownArgsNames), + ), + argNode, + ), + ); + } + }, + }; +} + +// @internal +export function KnownArgumentNamesOnDirectives( + context: ValidationContext | SDLValidationContext, +): ASTVisitor { + const directiveArgs = Object.create(null); + + const schema = context.getSchema(); + const definedDirectives = schema + ? schema.getDirectives() + : specifiedDirectives; + for (const directive of definedDirectives) { + directiveArgs[directive.name] = directive.args.map(arg => arg.name); + } + + const astDefinitions = context.getDocument().definitions; + for (const def of astDefinitions) { + if (def.kind === Kind.DIRECTIVE_DEFINITION) { + directiveArgs[def.name.value] = def.arguments + ? def.arguments.map(arg => arg.name.value) + : []; + } + } + + return { + Directive(directiveNode) { + const directiveName = directiveNode.name.value; + const knownArgs = directiveArgs[directiveName]; + + if (directiveNode.arguments && knownArgs) { + for (const argNode of directiveNode.arguments) { + const argName = argNode.name.value; + if (knownArgs.indexOf(argName) === -1) { + const suggestions = suggestionList(argName, knownArgs); context.reportError( new GraphQLError( - unknownDirectiveArgMessage( - node.name.value, - directive.name, - suggestionList( - node.name.value, - directive.args.map(arg => arg.name), - ), - ), - [node], + unknownDirectiveArgMessage(argName, directiveName, suggestions), + argNode, ), ); } } } + + return false; }, }; } diff --git a/src/validation/rules/KnownDirectives.js b/src/validation/rules/KnownDirectives.js index afc804001f..52c9a7b005 100644 --- a/src/validation/rules/KnownDirectives.js +++ b/src/validation/rules/KnownDirectives.js @@ -38,6 +38,7 @@ export function KnownDirectives( context: ValidationContext | SDLValidationContext, ): ASTVisitor { const locationsMap = Object.create(null); + const schema = context.getSchema(); const definedDirectives = schema ? schema.getDirectives() diff --git a/src/validation/rules/ProvidedRequiredArguments.js b/src/validation/rules/ProvidedRequiredArguments.js index b140059743..7ed1c58ed8 100644 --- a/src/validation/rules/ProvidedRequiredArguments.js +++ b/src/validation/rules/ProvidedRequiredArguments.js @@ -7,12 +7,18 @@ * @flow strict */ -import type { ValidationContext } from '../ValidationContext'; +import type { + ValidationContext, + SDLValidationContext, +} from '../ValidationContext'; import { GraphQLError } from '../../error/GraphQLError'; +import { Kind } from '../../language/kinds'; import inspect from '../../jsutils/inspect'; import keyMap from '../../jsutils/keyMap'; -import { isRequiredArgument } from '../../type/definition'; +import { isType, isRequiredArgument } from '../../type/definition'; import type { ASTVisitor } from '../../language/visitor'; +import { print } from '../../language/printer'; +import { specifiedDirectives } from '../../type/directives'; export function missingFieldArgMessage( fieldName: string, @@ -46,14 +52,15 @@ export function ProvidedRequiredArguments( context: ValidationContext, ): ASTVisitor { return { + ...ProvidedRequiredArgumentsOnDirectives(context), Field: { // Validate on leave to allow for deeper errors to appear first. - leave(node) { + leave(fieldNode) { const fieldDef = context.getFieldDef(); if (!fieldDef) { return false; } - const argNodes = node.arguments || []; + const argNodes = fieldNode.arguments || []; const argNodeMap = keyMap(argNodes, arg => arg.name.value); for (const argDef of fieldDef.args) { @@ -62,44 +69,77 @@ export function ProvidedRequiredArguments( context.reportError( new GraphQLError( missingFieldArgMessage( - node.name.value, + fieldDef.name, argDef.name, inspect(argDef.type), ), - [node], + [fieldNode], ), ); } } }, }, + }; +} + +// @internal +export function ProvidedRequiredArgumentsOnDirectives( + context: ValidationContext | SDLValidationContext, +): ASTVisitor { + const requiredArgsMap = Object.create(null); + + const schema = context.getSchema(); + const definedDirectives = schema + ? schema.getDirectives() + : specifiedDirectives; + for (const directive of definedDirectives) { + requiredArgsMap[directive.name] = keyMap( + directive.args.filter(isRequiredArgument), + arg => arg.name, + ); + } + + const astDefinitions = context.getDocument().definitions; + for (const def of astDefinitions) { + if (def.kind === Kind.DIRECTIVE_DEFINITION) { + requiredArgsMap[def.name.value] = keyMap( + def.arguments ? def.arguments.filter(isRequiredArgumentNode) : [], + arg => arg.name.value, + ); + } + } + return { Directive: { // Validate on leave to allow for deeper errors to appear first. - leave(node) { - const directiveDef = context.getDirective(); - if (!directiveDef) { - return false; - } - const argNodes = node.arguments || []; - - const argNodeMap = keyMap(argNodes, arg => arg.name.value); - for (const argDef of directiveDef.args) { - const argNode = argNodeMap[argDef.name]; - if (!argNode && isRequiredArgument(argDef)) { - context.reportError( - new GraphQLError( - missingDirectiveArgMessage( - node.name.value, - argDef.name, - inspect(argDef.type), + leave(directiveNode) { + const directiveName = directiveNode.name.value; + const requiredArgs = requiredArgsMap[directiveName]; + if (requiredArgs) { + const argNodes = directiveNode.arguments || []; + const argNodeMap = keyMap(argNodes, arg => arg.name.value); + for (const argName of Object.keys(requiredArgs)) { + if (!argNodeMap[argName]) { + const argType = requiredArgs[argName].type; + context.reportError( + new GraphQLError( + missingDirectiveArgMessage( + directiveName, + argName, + isType(argType) ? inspect(argType) : print(argType), + ), + directiveNode, ), - [node], - ), - ); + ); + } } } }, }, }; } + +function isRequiredArgumentNode(arg) { + return arg.type.kind === Kind.NON_NULL_TYPE && arg.defaultValue == null; +} diff --git a/src/validation/specifiedRules.js b/src/validation/specifiedRules.js index 4bf2b31164..f4e1960bee 100644 --- a/src/validation/specifiedRules.js +++ b/src/validation/specifiedRules.js @@ -123,12 +123,16 @@ export const specifiedRules: $ReadOnlyArray = [ ]; import { LoneSchemaDefinition } from './rules/LoneSchemaDefinition'; +import { KnownArgumentNamesOnDirectives } from './rules/KnownArgumentNames'; +import { ProvidedRequiredArgumentsOnDirectives } from './rules/ProvidedRequiredArguments'; // @internal export const specifiedSDLRules: $ReadOnlyArray = [ LoneSchemaDefinition, KnownDirectives, UniqueDirectivesPerLocation, + KnownArgumentNamesOnDirectives, UniqueArgumentNames, UniqueInputFieldNames, + ProvidedRequiredArgumentsOnDirectives, ];