From b3c383250dfa880004a538aed81ec362dec7a737 Mon Sep 17 00:00:00 2001 From: Matt Mahoney Date: Thu, 26 Jul 2018 16:50:29 -0400 Subject: [PATCH 1/4] Add ability to getDirectives() on a type --- src/type/definition.js | 140 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 140 insertions(+) diff --git a/src/type/definition.js b/src/type/definition.js index d588a565d1..376e899a80 100644 --- a/src/type/definition.js +++ b/src/type/definition.js @@ -35,6 +35,7 @@ import type { OperationDefinitionNode, FieldNode, FragmentDefinitionNode, + DirectiveNode, ValueNode, } from '../language/ast'; import type { GraphQLSchema } from './schema'; @@ -541,6 +542,8 @@ export class GraphQLScalarType { astNode: ?ScalarTypeDefinitionNode; extensionASTNodes: ?$ReadOnlyArray; + _directives: ?$ReadOnlyArray; + constructor(config: GraphQLScalarTypeConfig<*, *>): void { this.name = config.name; this.description = config.description; @@ -566,6 +569,28 @@ export class GraphQLScalarType { } } + getDirectives(): $ReadOnlyArray { + if (this._directives) { + return this._directives; + } + + const directives = []; + if (this.astNode && this.astNode.directives) { + directives.push(...this.astNode.directives); + } + const extensionASTNodes = this.extensionASTNodes; + if (extensionASTNodes) { + for (let i = 0; i < extensionASTNodes.length; i++) { + const extensionNode = extensionASTNodes[i]; + if (extensionNode.directives) { + directives.push(...extensionNode.directives); + } + } + } + this._directives = directives; + return directives; + } + toString(): string { return this.name; } @@ -641,6 +666,7 @@ export class GraphQLObjectType { _fields: Thunk>; _interfaces: Thunk>; + _directives: ?$ReadOnlyArray; constructor(config: GraphQLObjectTypeConfig<*, *>): void { this.name = config.name; @@ -659,6 +685,28 @@ export class GraphQLObjectType { } } + getDirectives(): $ReadOnlyArray { + if (this._directives) { + return this._directives; + } + + const directives = []; + if (this.astNode && this.astNode.directives) { + directives.push(...this.astNode.directives); + } + const extensionASTNodes = this.extensionASTNodes; + if (extensionASTNodes) { + for (let i = 0; i < extensionASTNodes.length; i++) { + const extensionNode = extensionASTNodes[i]; + if (extensionNode.directives) { + directives.push(...extensionNode.directives); + } + } + } + this._directives = directives; + return directives; + } + getFields(): GraphQLFieldMap<*, *> { if (typeof this._fields === 'function') { this._fields = this._fields(); @@ -894,6 +942,7 @@ export class GraphQLInterfaceType { resolveType: ?GraphQLTypeResolver<*, *>; _fields: Thunk>; + _directives: ?$ReadOnlyArray; constructor(config: GraphQLInterfaceTypeConfig<*, *>): void { this.name = config.name; @@ -911,6 +960,28 @@ export class GraphQLInterfaceType { } } + getDirectives(): $ReadOnlyArray { + if (this._directives) { + return this._directives; + } + + const directives = []; + if (this.astNode && this.astNode.directives) { + directives.push(...this.astNode.directives); + } + const extensionASTNodes = this.extensionASTNodes; + if (extensionASTNodes) { + for (let i = 0; i < extensionASTNodes.length; i++) { + const extensionNode = extensionASTNodes[i]; + if (extensionNode.directives) { + directives.push(...extensionNode.directives); + } + } + } + this._directives = directives; + return directives; + } + getFields(): GraphQLFieldMap<*, *> { if (typeof this._fields === 'function') { this._fields = this._fields(); @@ -972,6 +1043,7 @@ export class GraphQLUnionType { resolveType: ?GraphQLTypeResolver<*, *>; _types: Thunk>; + _directives: ?$ReadOnlyArray; constructor(config: GraphQLUnionTypeConfig<*, *>): void { this.name = config.name; @@ -989,6 +1061,28 @@ export class GraphQLUnionType { } } + getDirectives(): $ReadOnlyArray { + if (this._directives) { + return this._directives; + } + + const directives = []; + if (this.astNode && this.astNode.directives) { + directives.push(...this.astNode.directives); + } + const extensionASTNodes = this.extensionASTNodes; + if (extensionASTNodes) { + for (let i = 0; i < extensionASTNodes.length; i++) { + const extensionNode = extensionASTNodes[i]; + if (extensionNode.directives) { + directives.push(...extensionNode.directives); + } + } + } + this._directives = directives; + return directives; + } + getTypes(): Array { if (typeof this._types === 'function') { this._types = this._types(); @@ -1058,6 +1152,7 @@ export class GraphQLEnumType /* */ { astNode: ?EnumTypeDefinitionNode; extensionASTNodes: ?$ReadOnlyArray; + _directives: ?$ReadOnlyArray; _values: Array */>; _valueLookup: Map; _nameLookup: ObjMap; @@ -1076,6 +1171,28 @@ export class GraphQLEnumType /* */ { invariant(typeof config.name === 'string', 'Must provide name.'); } + getDirectives(): $ReadOnlyArray { + if (this._directives) { + return this._directives; + } + + const directives = []; + if (this.astNode && this.astNode.directives) { + directives.push(...this.astNode.directives); + } + const extensionASTNodes = this.extensionASTNodes; + if (extensionASTNodes) { + for (let i = 0; i < extensionASTNodes.length; i++) { + const extensionNode = extensionASTNodes[i]; + if (extensionNode.directives) { + directives.push(...extensionNode.directives); + } + } + } + this._directives = directives; + return directives; + } + getValues(): Array */> { return this._values; } @@ -1204,6 +1321,7 @@ export class GraphQLInputObjectType { astNode: ?InputObjectTypeDefinitionNode; extensionASTNodes: ?$ReadOnlyArray; + _directives: ?$ReadOnlyArray; _fields: Thunk; constructor(config: GraphQLInputObjectTypeConfig): void { @@ -1215,6 +1333,28 @@ export class GraphQLInputObjectType { invariant(typeof config.name === 'string', 'Must provide name.'); } + getDirectives(): $ReadOnlyArray { + if (this._directives) { + return this._directives; + } + + const directives = []; + if (this.astNode && this.astNode.directives) { + directives.push(...this.astNode.directives); + } + const extensionASTNodes = this.extensionASTNodes; + if (extensionASTNodes) { + for (let i = 0; i < extensionASTNodes.length; i++) { + const extensionNode = extensionASTNodes[i]; + if (extensionNode.directives) { + directives.push(...extensionNode.directives); + } + } + } + this._directives = directives; + return directives; + } + getFields(): GraphQLInputFieldMap { if (typeof this._fields === 'function') { this._fields = this._fields(); From b553a761f18edb97fd37667c68813b4fe5941b23 Mon Sep 17 00:00:00 2001 From: Matt Mahoney Date: Thu, 26 Jul 2018 19:24:42 -0400 Subject: [PATCH 2/4] Add validation for duplicate directive usages --- src/type/__tests__/validation-test.js | 169 ++++++++++++++++++++++++++ src/type/validate.js | 125 ++++++++++++++----- 2 files changed, 265 insertions(+), 29 deletions(-) diff --git a/src/type/__tests__/validation-test.js b/src/type/__tests__/validation-test.js index 7b914c5fa1..6f792e4bda 100644 --- a/src/type/__tests__/validation-test.js +++ b/src/type/__tests__/validation-test.js @@ -1827,3 +1827,172 @@ describe('Objects must adhere to Interface they implement', () => { ]); }); }); + +describe('Type System: Schema directives must validate', () => { + it('accepts a Schema with valid directives', () => { + const schema = buildSchema(` + schema @testA @testB { + query: Query + } + + type Query @testA @testB { + test: AnInterface @testB + } + + directive @testA on SCHEMA | OBJECT | INTERFACE | UNION | SCALAR | INPUT_OBJECT + directive @testB on SCHEMA | OBJECT | INTERFACE | UNION | SCALAR | INPUT_OBJECT + directive @testC on FIELD_DEFINITION | ARGUMENT_DEFINITION | ENUM_VALUE | INPUT_FIELD_DEFINITION + directive @testD on FIELD_DEFINITION | ARGUMENT_DEFINITION | ENUM_VALUE | INPUT_FIELD_DEFINITION + + interface AnInterface @testA { + field: String! @testB + } + + type TypeA implements AnInterface @testA { + field(arg: SomeInput @testC): String! @testC @testD + } + + type TypeB @testB @testA { + scalar_field: SomeScalar @testC + enum_field: SomeEnum @testC @testD + } + + union SomeUnion @testA = TypeA | TypeB + + scalar SomeScalar @testA @testB + + enum SomeEnum @testA @testB { + SOME_VALUE @testC + } + + input SomeInput @testA @testB { + some_input_field: String @testC + } + `); + expect(validateSchema(schema)).to.deep.equal([]); + }); + + it('rejects a Schema with directive defined multiple times', () => { + const schema = buildSchema(` + type Query { + test: String + } + + directive @testA on SCHEMA + directive @testA on SCHEMA + `); + expect(validateSchema(schema)).to.deep.equal([ + { + message: 'Directive @testA defined multiple times.', + locations: [{ line: 6, column: 7 }, { line: 7, column: 7 }], + }, + ]); + }); + + it('rejects a Schema with same schema directive used twice', () => { + const schema = buildSchema(` + schema @testA @testA { + query: Query + } + type Query { + test: String + } + + directive @testA on SCHEMA + `); + expect(validateSchema(schema)).to.deep.equal([ + { + message: 'Directive @testA used twice at the same location.', + locations: [{ line: 2, column: 14 }, { line: 2, column: 21 }], + }, + ]); + }); + + it('rejects a Schema with same definition directive used twice', () => { + const schema = buildSchema(` + directive @testA on OBJECT | INTERFACE | UNION | SCALAR | INPUT_OBJECT + + type Query implements SomeInterface @testA @testA { + test: String + } + + interface SomeInterface @testA @testA { + test: String + } + + union SomeUnion @testA @testA = Query + + scalar SomeScalar @testA @testA + + enum SomeEnum @testA @testA { + SOME_VALUE + } + + input SomeInput @testA @testA { + some_input_field: String + } + `); + expect(validateSchema(schema)).to.deep.equal([ + { + message: 'Directive @testA used twice at the same location.', + locations: [{ line: 4, column: 43 }, { line: 4, column: 50 }], + }, + { + message: 'Directive @testA used twice at the same location.', + locations: [{ line: 8, column: 31 }, { line: 8, column: 38 }], + }, + { + message: 'Directive @testA used twice at the same location.', + locations: [{ line: 12, column: 23 }, { line: 12, column: 30 }], + }, + { + message: 'Directive @testA used twice at the same location.', + locations: [{ line: 14, column: 25 }, { line: 14, column: 32 }], + }, + { + message: 'Directive @testA used twice at the same location.', + locations: [{ line: 16, column: 21 }, { line: 16, column: 28 }], + }, + { + message: 'Directive @testA used twice at the same location.', + locations: [{ line: 20, column: 23 }, { line: 20, column: 30 }], + }, + ]); + }); + + it('rejects a Schema with same field and arg directive used twice', () => { + const schema = buildSchema(` + directive @testA on FIELD_DEFINITION | ENUM_VALUE | INPUT_FIELD_DEFINITION | ARGUMENT_DEFINITION + + type Query implements SomeInterface { + test(arg: String @testA @testA): String @testA @testA + } + + interface SomeInterface { + test: String @testA @testA + } + + input SomeInput { + some_input_field: String @testA @testA + } + `); + expect(validateSchema(schema)).to.deep.equal([ + { + message: 'Directive @testA used twice at the same location.', + locations: [{ line: 5, column: 26 }, { line: 5, column: 33 }], + }, + { + message: 'Directive @testA used twice at the same location.', + locations: [{ line: 5, column: 49 }, { line: 5, column: 56 }], + }, + { + message: 'Directive @testA used twice at the same location.', + locations: [{ line: 9, column: 22 }, { line: 9, column: 29 }], + }, + { + message: 'Directive @testA used twice at the same location.', + locations: [{ line: 13, column: 34 }, { line: 13, column: 41 }], + }, + ]); + }); +}); diff --git a/src/type/validate.js b/src/type/validate.js index 1d8bdb13a1..a27b52dd18 100644 --- a/src/type/validate.js +++ b/src/type/validate.js @@ -7,44 +7,46 @@ * @flow strict */ -import { - isObjectType, - isInterfaceType, - isUnionType, - isEnumType, - isInputObjectType, - isNonNullType, - isNamedType, - isInputType, - isOutputType, -} from './definition'; import type { - GraphQLObjectType, - GraphQLInterfaceType, - GraphQLUnionType, + ASTNode, + DirectiveNode, + EnumValueDefinitionNode, + FieldDefinitionNode, + InputValueDefinitionNode, + NamedTypeNode, + TypeNode, +} from '../language/ast'; +import type { GraphQLEnumType, GraphQLInputObjectType, + GraphQLInterfaceType, + GraphQLObjectType, + GraphQLUnionType, } from './definition'; -import { isDirective } from './directives'; import type { GraphQLDirective } from './directives'; -import { isIntrospectionType } from './introspection'; -import { isSchema } from './schema'; import type { GraphQLSchema } from './schema'; -import inspect from '../jsutils/inspect'; + +import { GraphQLError } from '../error/GraphQLError'; import find from '../jsutils/find'; +import inspect from '../jsutils/inspect'; import invariant from '../jsutils/invariant'; import objectValues from '../jsutils/objectValues'; -import { GraphQLError } from '../error/GraphQLError'; -import type { - ASTNode, - FieldDefinitionNode, - EnumValueDefinitionNode, - InputValueDefinitionNode, - NamedTypeNode, - TypeNode, -} from '../language/ast'; import { isValidNameError } from '../utilities/assertValidName'; import { isEqualType, isTypeSubTypeOf } from '../utilities/typeComparators'; +import { + isEnumType, + isInputObjectType, + isInputType, + isInterfaceType, + isNamedType, + isNonNullType, + isObjectType, + isOutputType, + isUnionType, +} from './definition'; +import { isDirective } from './directives'; +import { isIntrospectionType } from './introspection'; +import { isSchema } from './schema'; /** * Implements the "Type Validation" sub-sections of the specification's @@ -70,7 +72,13 @@ export function validateSchema( // Validate the schema, producing a list of errors. const context = new SchemaValidationContext(schema); validateRootTypes(context); - validateDirectives(context); + validateDirectiveDefinitions(context); + + // Validate directives that are used on the schema + if (schema.astNode && schema.astNode.directives) { + validateNoDuplicateDirectives(context, schema.astNode.directives); + } + validateTypes(context); // Persist the results of validation before returning to ensure validation @@ -165,7 +173,10 @@ function getOperationTypeNode( return type.astNode; } -function validateDirectives(context: SchemaValidationContext): void { +function validateDirectiveDefinitions(context: SchemaValidationContext): void { + // Ensure no directive is defined multiple times + const directiveDefinitions = new Map(); + for (const directive of context.schema.getDirectives()) { // Ensure all directives are in fact GraphQL directives. if (!isDirective(directive)) { @@ -175,6 +186,9 @@ function validateDirectives(context: SchemaValidationContext): void { ); continue; } + const existingDefinitions = directiveDefinitions.get(directive.name) || []; + existingDefinitions.push(directive); + directiveDefinitions.set(directive.name, existingDefinitions); // Ensure they are named correctly. validateName(context, directive); @@ -209,6 +223,15 @@ function validateDirectives(context: SchemaValidationContext): void { } } } + + for (const [directiveName, directiveList] of directiveDefinitions) { + if (directiveList.length > 1) { + context.reportError( + `Directive @${directiveName} defined multiple times.`, + directiveList.map(directive => directive.astNode).filter(Boolean), + ); + } + } } function validateName( @@ -239,6 +262,8 @@ function validateTypes(context: SchemaValidationContext): void { continue; } + validateNoDuplicateDirectives(context, type.getDirectives()); + // Ensure it is named correctly (excluding introspection types). if (!isIntrospectionType(type)) { validateName(context, type); @@ -266,6 +291,28 @@ function validateTypes(context: SchemaValidationContext): void { } } +function validateNoDuplicateDirectives( + context: SchemaValidationContext, + directives: $ReadOnlyArray, +): void { + const directivesNamed = new Map(); + for (const directive of directives) { + const directiveName = directive.name.value; + const existingNodes = directivesNamed.get(directiveName) || []; + existingNodes.push(directive); + directivesNamed.set(directiveName, existingNodes); + } + + for (const [directiveName, directiveList] of directivesNamed) { + if (directiveList.length > 1) { + context.reportError( + `Directive @${directiveName} used twice at the same location.`, + directiveList, + ); + } + } +} + function validateFields( context: SchemaValidationContext, type: GraphQLObjectType | GraphQLInterfaceType, @@ -329,6 +376,16 @@ function validateFields( getFieldArgTypeNode(type, field.name, argName), ); } + + // Ensure argument definition directives are valid + if (arg.astNode && arg.astNode.directives) { + validateNoDuplicateDirectives(context, arg.astNode.directives); + } + } + + // Ensure any directives are valid + if (field.astNode && field.astNode.directives) { + validateNoDuplicateDirectives(context, field.astNode.directives); } } } @@ -520,6 +577,11 @@ function validateEnumValues( enumValue.astNode, ); } + + // Ensure valid directives + if (enumValue.astNode && enumValue.astNode.directives) { + validateNoDuplicateDirectives(context, enumValue.astNode.directives); + } } } @@ -551,6 +613,11 @@ function validateInputFields( field.astNode && field.astNode.type, ); } + + // Ensure valid directives + if (field.astNode && field.astNode.directives) { + validateNoDuplicateDirectives(context, field.astNode.directives); + } } } From 89710c89004dfd1f183d227d367140bd5d65e5b9 Mon Sep 17 00:00:00 2001 From: Matt Mahoney Date: Thu, 26 Jul 2018 20:26:30 -0400 Subject: [PATCH 3/4] Add directive location is correct validation --- src/type/__tests__/validation-test.js | 243 +++++++++++++++++++------- src/type/validate.js | 98 ++++++++++- 2 files changed, 272 insertions(+), 69 deletions(-) diff --git a/src/type/__tests__/validation-test.js b/src/type/__tests__/validation-test.js index 6f792e4bda..49eb6dafc7 100644 --- a/src/type/__tests__/validation-test.js +++ b/src/type/__tests__/validation-test.js @@ -20,6 +20,7 @@ import { GraphQLString, } from '../../'; import { parse } from '../../language/parser'; +import { Source } from '../../language/source'; import { validateSchema } from '../validate'; import { buildSchema } from '../../utilities/buildASTSchema'; import { extendSchema } from '../../utilities/extendSchema'; @@ -725,6 +726,10 @@ describe('Type System: Input Objects must have fields', () => { 'Input Object type SomeInputObject must define one or more fields.', locations: [{ line: 6, column: 7 }, { line: 4, column: 9 }], }, + { + message: 'Directive @test not allowed at INPUT_OBJECT location.', + locations: [{ line: 4, column: 38 }, { line: 2, column: 9 }], + }, ]); }); @@ -1836,16 +1841,16 @@ describe('Type System: Schema directives must validate', () => { } type Query @testA @testB { - test: AnInterface @testB + test: AnInterface @testC } - directive @testA on SCHEMA | OBJECT | INTERFACE | UNION | SCALAR | INPUT_OBJECT - directive @testB on SCHEMA | OBJECT | INTERFACE | UNION | SCALAR | INPUT_OBJECT + directive @testA on SCHEMA | OBJECT | INTERFACE | UNION | SCALAR | INPUT_OBJECT | ENUM + directive @testB on SCHEMA | OBJECT | INTERFACE | UNION | SCALAR | INPUT_OBJECT | ENUM directive @testC on FIELD_DEFINITION | ARGUMENT_DEFINITION | ENUM_VALUE | INPUT_FIELD_DEFINITION directive @testD on FIELD_DEFINITION | ARGUMENT_DEFINITION | ENUM_VALUE | INPUT_FIELD_DEFINITION interface AnInterface @testA { - field: String! @testB + field: String! @testC } type TypeA implements AnInterface @testA { @@ -1889,109 +1894,225 @@ describe('Type System: Schema directives must validate', () => { ]); }); - it('rejects a Schema with same schema directive used twice', () => { + it('rejects a Schema with same directive used twice per location', () => { const schema = buildSchema(` - schema @testA @testA { + directive @schema on SCHEMA + directive @object on OBJECT + directive @interface on INTERFACE + directive @union on UNION + directive @scalar on SCALAR + directive @input_object on INPUT_OBJECT + directive @enum on ENUM + directive @field_definition on FIELD_DEFINITION + directive @enum_value on ENUM_VALUE + directive @input_field_definition on INPUT_FIELD_DEFINITION + directive @argument_definition on ARGUMENT_DEFINITION + + schema @schema @schema { query: Query } - type Query { - test: String - } - directive @testA on SCHEMA - `); - expect(validateSchema(schema)).to.deep.equal([ - { - message: 'Directive @testA used twice at the same location.', - locations: [{ line: 2, column: 14 }, { line: 2, column: 21 }], - }, - ]); - }); - - it('rejects a Schema with same definition directive used twice', () => { - const schema = buildSchema(` - directive @testA on OBJECT | INTERFACE | UNION | SCALAR | INPUT_OBJECT - - type Query implements SomeInterface @testA @testA { - test: String + type Query implements SomeInterface @object @object { + test(arg: SomeInput @argument_definition @argument_definition): String } - interface SomeInterface @testA @testA { - test: String + interface SomeInterface @interface @interface { + test: String @field_definition @field_definition } - union SomeUnion @testA @testA = Query + union SomeUnion @union @union = Query - scalar SomeScalar @testA @testA + scalar SomeScalar @scalar @scalar - enum SomeEnum @testA @testA { - SOME_VALUE + enum SomeEnum @enum @enum { + SOME_VALUE @enum_value @enum_value } - input SomeInput @testA @testA { - some_input_field: String + input SomeInput @input_object @input_object { + some_input_field: String @input_field_definition @input_field_definition } `); expect(validateSchema(schema)).to.deep.equal([ { - message: 'Directive @testA used twice at the same location.', - locations: [{ line: 4, column: 43 }, { line: 4, column: 50 }], + message: 'Directive @schema used twice at the same location.', + locations: [{ line: 14, column: 14 }, { line: 14, column: 22 }], }, { - message: 'Directive @testA used twice at the same location.', - locations: [{ line: 8, column: 31 }, { line: 8, column: 38 }], + message: + 'Directive @argument_definition used twice at the same location.', + locations: [{ line: 19, column: 29 }, { line: 19, column: 50 }], }, { - message: 'Directive @testA used twice at the same location.', - locations: [{ line: 12, column: 23 }, { line: 12, column: 30 }], + message: 'Directive @object used twice at the same location.', + locations: [{ line: 18, column: 43 }, { line: 18, column: 51 }], }, { - message: 'Directive @testA used twice at the same location.', - locations: [{ line: 14, column: 25 }, { line: 14, column: 32 }], + message: 'Directive @field_definition used twice at the same location.', + locations: [{ line: 23, column: 22 }, { line: 23, column: 40 }], }, { - message: 'Directive @testA used twice at the same location.', - locations: [{ line: 16, column: 21 }, { line: 16, column: 28 }], + message: 'Directive @interface used twice at the same location.', + locations: [{ line: 22, column: 31 }, { line: 22, column: 42 }], + }, + { + message: + 'Directive @input_field_definition not allowed at FIELD_DEFINITION location.', + locations: [{ line: 35, column: 34 }, { line: 11, column: 7 }], + }, + { + message: + 'Directive @input_field_definition not allowed at FIELD_DEFINITION location.', + locations: [{ line: 35, column: 58 }, { line: 11, column: 7 }], + }, + { + message: + 'Directive @input_field_definition used twice at the same location.', + locations: [{ line: 35, column: 34 }, { line: 35, column: 58 }], + }, + { + message: 'Directive @input_object used twice at the same location.', + locations: [{ line: 34, column: 23 }, { line: 34, column: 37 }], + }, + { + message: 'Directive @union used twice at the same location.', + locations: [{ line: 26, column: 23 }, { line: 26, column: 30 }], + }, + { + message: 'Directive @scalar used twice at the same location.', + locations: [{ line: 28, column: 25 }, { line: 28, column: 33 }], + }, + { + message: 'Directive @enum_value used twice at the same location.', + locations: [{ line: 31, column: 20 }, { line: 31, column: 32 }], }, + { + message: 'Directive @enum used twice at the same location.', + locations: [{ line: 30, column: 21 }, { line: 30, column: 27 }], + }, + ]); + }); + + it('rejects a Schema with directive used again in extension', () => { + const schema = buildSchema( + new Source(` + directive @testA on OBJECT + + type Query @testA { + test: String + } + `), + 'schema.graphql', + ); + const extensions = parse( + new Source( + ` + extend type Query @testA + `, + 'extensions.graphql', + ), + ); + const extendedSchema = extendSchema(schema, extensions); + expect(validateSchema(extendedSchema)).to.deep.equal([ { message: 'Directive @testA used twice at the same location.', - locations: [{ line: 20, column: 23 }, { line: 20, column: 30 }], + locations: [{ line: 4, column: 20 }, { line: 2, column: 29 }], }, ]); }); - it('rejects a Schema with same field and arg directive used twice', () => { + it('rejects a Schema with directives used in wrong location', () => { const schema = buildSchema(` - directive @testA on FIELD_DEFINITION | ENUM_VALUE | INPUT_FIELD_DEFINITION | ARGUMENT_DEFINITION + directive @schema on SCHEMA + directive @object on OBJECT + directive @interface on INTERFACE + directive @union on UNION + directive @scalar on SCALAR + directive @input_object on INPUT_OBJECT + directive @enum on ENUM + directive @field_definition on FIELD_DEFINITION + directive @enum_value on ENUM_VALUE + directive @input_field_definition on INPUT_FIELD_DEFINITION + directive @argument_definition on ARGUMENT_DEFINITION + + schema @object { + query: Query + } - type Query implements SomeInterface { - test(arg: String @testA @testA): String @testA @testA + type Query implements SomeInterface @schema { + test(arg: SomeInput @field_definition): String } - interface SomeInterface { - test: String @testA @testA + interface SomeInterface @interface { + test: String @argument_definition + } + + union SomeUnion @interface = Query + + scalar SomeScalar @enum_value + + enum SomeEnum @input_object { + SOME_VALUE @enum } - input SomeInput { - some_input_field: String @testA @testA + input SomeInput @object { + some_input_field: String @union } `); - expect(validateSchema(schema)).to.deep.equal([ + const extensions = parse( + new Source( + ` + extend type Query @testA + `, + 'extensions.graphql', + ), + ); + const extendedSchema = extendSchema(schema, extensions); + expect(validateSchema(extendedSchema)).to.deep.equal([ { - message: 'Directive @testA used twice at the same location.', - locations: [{ line: 5, column: 26 }, { line: 5, column: 33 }], + message: 'Directive @object not allowed at SCHEMA location.', + locations: [{ line: 14, column: 14 }, { line: 3, column: 7 }], }, { - message: 'Directive @testA used twice at the same location.', - locations: [{ line: 5, column: 49 }, { line: 5, column: 56 }], + message: + 'Directive @field_definition not allowed at ARGUMENT_DEFINITION location.', + locations: [{ line: 19, column: 29 }, { line: 9, column: 7 }], }, { - message: 'Directive @testA used twice at the same location.', - locations: [{ line: 9, column: 22 }, { line: 9, column: 29 }], + message: 'Directive @schema not allowed at OBJECT location.', + locations: [{ line: 18, column: 43 }, { line: 2, column: 7 }], }, { - message: 'Directive @testA used twice at the same location.', - locations: [{ line: 13, column: 34 }, { line: 13, column: 41 }], + message: 'No directive @testA defined.', + locations: [{ line: 2, column: 29 }], + }, + { + message: + 'Directive @argument_definition not allowed at FIELD_DEFINITION location.', + locations: [{ line: 23, column: 22 }, { line: 12, column: 7 }], + }, + { + message: 'Directive @union not allowed at FIELD_DEFINITION location.', + locations: [{ line: 35, column: 34 }, { line: 5, column: 7 }], + }, + { + message: 'Directive @object not allowed at INPUT_OBJECT location.', + locations: [{ line: 34, column: 23 }, { line: 3, column: 7 }], + }, + { + message: 'Directive @interface not allowed at UNION location.', + locations: [{ line: 26, column: 23 }, { line: 4, column: 7 }], + }, + { + message: 'Directive @enum_value not allowed at SCALAR location.', + locations: [{ line: 28, column: 25 }, { line: 10, column: 7 }], + }, + { + message: 'Directive @enum not allowed at ENUM_VALUE location.', + locations: [{ line: 31, column: 20 }, { line: 8, column: 7 }], + }, + { + message: 'Directive @input_object not allowed at ENUM location.', + locations: [{ line: 30, column: 21 }, { line: 7, column: 7 }], }, ]); }); diff --git a/src/type/validate.js b/src/type/validate.js index a27b52dd18..c5161fc8eb 100644 --- a/src/type/validate.js +++ b/src/type/validate.js @@ -16,6 +16,8 @@ import type { NamedTypeNode, TypeNode, } from '../language/ast'; +import { DirectiveLocation } from '../language/directiveLocation'; +import type { DirectiveLocationEnum } from '../language/directiveLocation'; import type { GraphQLEnumType, GraphQLInputObjectType, @@ -42,6 +44,7 @@ import { isNonNullType, isObjectType, isOutputType, + isScalarType, isUnionType, } from './definition'; import { isDirective } from './directives'; @@ -76,7 +79,11 @@ export function validateSchema( // Validate directives that are used on the schema if (schema.astNode && schema.astNode.directives) { - validateNoDuplicateDirectives(context, schema.astNode.directives); + validateDirectivesAtLocation( + context, + schema.astNode.directives, + DirectiveLocation.SCHEMA, + ); } validateTypes(context); @@ -262,8 +269,6 @@ function validateTypes(context: SchemaValidationContext): void { continue; } - validateNoDuplicateDirectives(context, type.getDirectives()); - // Ensure it is named correctly (excluding introspection types). if (!isIntrospectionType(type)) { validateName(context, type); @@ -275,29 +280,90 @@ function validateTypes(context: SchemaValidationContext): void { // Ensure objects implement the interfaces they claim to. validateObjectInterfaces(context, type); + + // Ensure directives are valid + validateDirectivesAtLocation( + context, + type.getDirectives(), + DirectiveLocation.OBJECT, + ); } else if (isInterfaceType(type)) { // Ensure fields are valid. validateFields(context, type); + + // Ensure directives are valid + validateDirectivesAtLocation( + context, + type.getDirectives(), + DirectiveLocation.INTERFACE, + ); } else if (isUnionType(type)) { // Ensure Unions include valid member types. validateUnionMembers(context, type); + + // Ensure directives are valid + validateDirectivesAtLocation( + context, + type.getDirectives(), + DirectiveLocation.UNION, + ); } else if (isEnumType(type)) { // Ensure Enums have valid values. validateEnumValues(context, type); + + // Ensure directives are valid + validateDirectivesAtLocation( + context, + type.getDirectives(), + DirectiveLocation.ENUM, + ); } else if (isInputObjectType(type)) { // Ensure Input Object fields are valid. validateInputFields(context, type); + + // Ensure directives are valid + validateDirectivesAtLocation( + context, + type.getDirectives(), + DirectiveLocation.INPUT_OBJECT, + ); + } else if (isScalarType(type)) { + // Ensure directives are valid + validateDirectivesAtLocation( + context, + type.getDirectives(), + DirectiveLocation.SCALAR, + ); } } } -function validateNoDuplicateDirectives( +function validateDirectivesAtLocation( context: SchemaValidationContext, directives: $ReadOnlyArray, + location: DirectiveLocationEnum, ): void { const directivesNamed = new Map(); + const schema = context.schema; for (const directive of directives) { const directiveName = directive.name.value; + + // Ensure directive used is also defined + const schemaDirective = schema.getDirective(directiveName); + if (!schemaDirective) { + context.reportError(`No directive @${directiveName} defined.`, directive); + continue; + } + if (!schemaDirective.locations.includes(location)) { + const errorNodes = schemaDirective.astNode + ? [directive, schemaDirective.astNode] + : [directive]; + context.reportError( + `Directive @${directiveName} not allowed at ${location} location.`, + errorNodes, + ); + } + const existingNodes = directivesNamed.get(directiveName) || []; existingNodes.push(directive); directivesNamed.set(directiveName, existingNodes); @@ -379,13 +445,21 @@ function validateFields( // Ensure argument definition directives are valid if (arg.astNode && arg.astNode.directives) { - validateNoDuplicateDirectives(context, arg.astNode.directives); + validateDirectivesAtLocation( + context, + arg.astNode.directives, + DirectiveLocation.ARGUMENT_DEFINITION, + ); } } // Ensure any directives are valid if (field.astNode && field.astNode.directives) { - validateNoDuplicateDirectives(context, field.astNode.directives); + validateDirectivesAtLocation( + context, + field.astNode.directives, + DirectiveLocation.FIELD_DEFINITION, + ); } } } @@ -580,7 +654,11 @@ function validateEnumValues( // Ensure valid directives if (enumValue.astNode && enumValue.astNode.directives) { - validateNoDuplicateDirectives(context, enumValue.astNode.directives); + validateDirectivesAtLocation( + context, + enumValue.astNode.directives, + DirectiveLocation.ENUM_VALUE, + ); } } } @@ -616,7 +694,11 @@ function validateInputFields( // Ensure valid directives if (field.astNode && field.astNode.directives) { - validateNoDuplicateDirectives(context, field.astNode.directives); + validateDirectivesAtLocation( + context, + field.astNode.directives, + DirectiveLocation.FIELD_DEFINITION, + ); } } } From a4c73af21975d1d8e84e99f30f2d9482acbe51eb Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Fri, 27 Jul 2018 17:24:32 +0300 Subject: [PATCH 4/4] Replace '.getDirectives' with generic method (#1436) --- src/type/definition.js | 140 ----------------------------------------- src/type/validate.js | 31 +++++---- 2 files changed, 18 insertions(+), 153 deletions(-) diff --git a/src/type/definition.js b/src/type/definition.js index 376e899a80..d588a565d1 100644 --- a/src/type/definition.js +++ b/src/type/definition.js @@ -35,7 +35,6 @@ import type { OperationDefinitionNode, FieldNode, FragmentDefinitionNode, - DirectiveNode, ValueNode, } from '../language/ast'; import type { GraphQLSchema } from './schema'; @@ -542,8 +541,6 @@ export class GraphQLScalarType { astNode: ?ScalarTypeDefinitionNode; extensionASTNodes: ?$ReadOnlyArray; - _directives: ?$ReadOnlyArray; - constructor(config: GraphQLScalarTypeConfig<*, *>): void { this.name = config.name; this.description = config.description; @@ -569,28 +566,6 @@ export class GraphQLScalarType { } } - getDirectives(): $ReadOnlyArray { - if (this._directives) { - return this._directives; - } - - const directives = []; - if (this.astNode && this.astNode.directives) { - directives.push(...this.astNode.directives); - } - const extensionASTNodes = this.extensionASTNodes; - if (extensionASTNodes) { - for (let i = 0; i < extensionASTNodes.length; i++) { - const extensionNode = extensionASTNodes[i]; - if (extensionNode.directives) { - directives.push(...extensionNode.directives); - } - } - } - this._directives = directives; - return directives; - } - toString(): string { return this.name; } @@ -666,7 +641,6 @@ export class GraphQLObjectType { _fields: Thunk>; _interfaces: Thunk>; - _directives: ?$ReadOnlyArray; constructor(config: GraphQLObjectTypeConfig<*, *>): void { this.name = config.name; @@ -685,28 +659,6 @@ export class GraphQLObjectType { } } - getDirectives(): $ReadOnlyArray { - if (this._directives) { - return this._directives; - } - - const directives = []; - if (this.astNode && this.astNode.directives) { - directives.push(...this.astNode.directives); - } - const extensionASTNodes = this.extensionASTNodes; - if (extensionASTNodes) { - for (let i = 0; i < extensionASTNodes.length; i++) { - const extensionNode = extensionASTNodes[i]; - if (extensionNode.directives) { - directives.push(...extensionNode.directives); - } - } - } - this._directives = directives; - return directives; - } - getFields(): GraphQLFieldMap<*, *> { if (typeof this._fields === 'function') { this._fields = this._fields(); @@ -942,7 +894,6 @@ export class GraphQLInterfaceType { resolveType: ?GraphQLTypeResolver<*, *>; _fields: Thunk>; - _directives: ?$ReadOnlyArray; constructor(config: GraphQLInterfaceTypeConfig<*, *>): void { this.name = config.name; @@ -960,28 +911,6 @@ export class GraphQLInterfaceType { } } - getDirectives(): $ReadOnlyArray { - if (this._directives) { - return this._directives; - } - - const directives = []; - if (this.astNode && this.astNode.directives) { - directives.push(...this.astNode.directives); - } - const extensionASTNodes = this.extensionASTNodes; - if (extensionASTNodes) { - for (let i = 0; i < extensionASTNodes.length; i++) { - const extensionNode = extensionASTNodes[i]; - if (extensionNode.directives) { - directives.push(...extensionNode.directives); - } - } - } - this._directives = directives; - return directives; - } - getFields(): GraphQLFieldMap<*, *> { if (typeof this._fields === 'function') { this._fields = this._fields(); @@ -1043,7 +972,6 @@ export class GraphQLUnionType { resolveType: ?GraphQLTypeResolver<*, *>; _types: Thunk>; - _directives: ?$ReadOnlyArray; constructor(config: GraphQLUnionTypeConfig<*, *>): void { this.name = config.name; @@ -1061,28 +989,6 @@ export class GraphQLUnionType { } } - getDirectives(): $ReadOnlyArray { - if (this._directives) { - return this._directives; - } - - const directives = []; - if (this.astNode && this.astNode.directives) { - directives.push(...this.astNode.directives); - } - const extensionASTNodes = this.extensionASTNodes; - if (extensionASTNodes) { - for (let i = 0; i < extensionASTNodes.length; i++) { - const extensionNode = extensionASTNodes[i]; - if (extensionNode.directives) { - directives.push(...extensionNode.directives); - } - } - } - this._directives = directives; - return directives; - } - getTypes(): Array { if (typeof this._types === 'function') { this._types = this._types(); @@ -1152,7 +1058,6 @@ export class GraphQLEnumType /* */ { astNode: ?EnumTypeDefinitionNode; extensionASTNodes: ?$ReadOnlyArray; - _directives: ?$ReadOnlyArray; _values: Array */>; _valueLookup: Map; _nameLookup: ObjMap; @@ -1171,28 +1076,6 @@ export class GraphQLEnumType /* */ { invariant(typeof config.name === 'string', 'Must provide name.'); } - getDirectives(): $ReadOnlyArray { - if (this._directives) { - return this._directives; - } - - const directives = []; - if (this.astNode && this.astNode.directives) { - directives.push(...this.astNode.directives); - } - const extensionASTNodes = this.extensionASTNodes; - if (extensionASTNodes) { - for (let i = 0; i < extensionASTNodes.length; i++) { - const extensionNode = extensionASTNodes[i]; - if (extensionNode.directives) { - directives.push(...extensionNode.directives); - } - } - } - this._directives = directives; - return directives; - } - getValues(): Array */> { return this._values; } @@ -1321,7 +1204,6 @@ export class GraphQLInputObjectType { astNode: ?InputObjectTypeDefinitionNode; extensionASTNodes: ?$ReadOnlyArray; - _directives: ?$ReadOnlyArray; _fields: Thunk; constructor(config: GraphQLInputObjectTypeConfig): void { @@ -1333,28 +1215,6 @@ export class GraphQLInputObjectType { invariant(typeof config.name === 'string', 'Must provide name.'); } - getDirectives(): $ReadOnlyArray { - if (this._directives) { - return this._directives; - } - - const directives = []; - if (this.astNode && this.astNode.directives) { - directives.push(...this.astNode.directives); - } - const extensionASTNodes = this.extensionASTNodes; - if (extensionASTNodes) { - for (let i = 0; i < extensionASTNodes.length; i++) { - const extensionNode = extensionASTNodes[i]; - if (extensionNode.directives) { - directives.push(...extensionNode.directives); - } - } - } - this._directives = directives; - return directives; - } - getFields(): GraphQLInputFieldMap { if (typeof this._fields === 'function') { this._fields = this._fields(); diff --git a/src/type/validate.js b/src/type/validate.js index c5161fc8eb..5884e3cabc 100644 --- a/src/type/validate.js +++ b/src/type/validate.js @@ -19,6 +19,7 @@ import type { import { DirectiveLocation } from '../language/directiveLocation'; import type { DirectiveLocationEnum } from '../language/directiveLocation'; import type { + GraphQLNamedType, GraphQLEnumType, GraphQLInputObjectType, GraphQLInterfaceType, @@ -78,13 +79,11 @@ export function validateSchema( validateDirectiveDefinitions(context); // Validate directives that are used on the schema - if (schema.astNode && schema.astNode.directives) { - validateDirectivesAtLocation( - context, - schema.astNode.directives, - DirectiveLocation.SCHEMA, - ); - } + validateDirectivesAtLocation( + context, + getDirectives(schema), + DirectiveLocation.SCHEMA, + ); validateTypes(context); @@ -284,7 +283,7 @@ function validateTypes(context: SchemaValidationContext): void { // Ensure directives are valid validateDirectivesAtLocation( context, - type.getDirectives(), + getDirectives(type), DirectiveLocation.OBJECT, ); } else if (isInterfaceType(type)) { @@ -294,7 +293,7 @@ function validateTypes(context: SchemaValidationContext): void { // Ensure directives are valid validateDirectivesAtLocation( context, - type.getDirectives(), + getDirectives(type), DirectiveLocation.INTERFACE, ); } else if (isUnionType(type)) { @@ -304,7 +303,7 @@ function validateTypes(context: SchemaValidationContext): void { // Ensure directives are valid validateDirectivesAtLocation( context, - type.getDirectives(), + getDirectives(type), DirectiveLocation.UNION, ); } else if (isEnumType(type)) { @@ -314,7 +313,7 @@ function validateTypes(context: SchemaValidationContext): void { // Ensure directives are valid validateDirectivesAtLocation( context, - type.getDirectives(), + getDirectives(type), DirectiveLocation.ENUM, ); } else if (isInputObjectType(type)) { @@ -324,14 +323,14 @@ function validateTypes(context: SchemaValidationContext): void { // Ensure directives are valid validateDirectivesAtLocation( context, - type.getDirectives(), + getDirectives(type), DirectiveLocation.INPUT_OBJECT, ); } else if (isScalarType(type)) { // Ensure directives are valid validateDirectivesAtLocation( context, - type.getDirectives(), + getDirectives(type), DirectiveLocation.SCALAR, ); } @@ -735,6 +734,12 @@ function getAllSubNodes( return result; } +function getDirectives( + object: GraphQLSchema | GraphQLNamedType, +): $ReadOnlyArray { + return getAllSubNodes(object, node => node.directives); +} + function getImplementsInterfaceNode( type: GraphQLObjectType, iface: GraphQLInterfaceType,