From ae154ff854a7d4e45ca207851e2935590d8297de Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Wed, 20 Jun 2018 23:13:07 +0300 Subject: [PATCH 1/3] Switch 'forEach' to 'for of' --- src/type/validate.js | 93 ++++++++++++++++++++------------------------ 1 file changed, 43 insertions(+), 50 deletions(-) diff --git a/src/type/validate.js b/src/type/validate.js index b02134775f..1c3f5502f8 100644 --- a/src/type/validate.js +++ b/src/type/validate.js @@ -169,15 +169,14 @@ function getOperationTypeNode( } function validateDirectives(context: SchemaValidationContext): void { - const directives = context.schema.getDirectives(); - directives.forEach(directive => { + for (const directive of context.schema.getDirectives()) { // Ensure all directives are in fact GraphQL directives. if (!isDirective(directive)) { context.reportError( `Expected directive but got: ${inspect(directive)}.`, directive && directive.astNode, ); - return; + continue; } // Ensure they are named correctly. @@ -187,7 +186,7 @@ function validateDirectives(context: SchemaValidationContext): void { // Ensure the arguments are valid. const argNames = Object.create(null); - directive.args.forEach(arg => { + for (const arg of directive.args) { const argName = arg.name; // Ensure they are named correctly. @@ -199,7 +198,7 @@ function validateDirectives(context: SchemaValidationContext): void { `Argument @${directive.name}(${argName}:) can only be defined once.`, getAllDirectiveArgNodes(directive, argName), ); - return; // continue loop + continue; } argNames[argName] = true; @@ -211,8 +210,8 @@ function validateDirectives(context: SchemaValidationContext): void { getDirectiveArgTypeNode(directive, argName), ); } - }); - }); + } + } } function validateName( @@ -233,14 +232,14 @@ function validateName( function validateTypes(context: SchemaValidationContext): void { const typeMap = context.schema.getTypeMap(); - objectValues(typeMap).forEach(type => { + for (const type of objectValues(typeMap)) { // Ensure all provided types are in fact GraphQL type. if (!isNamedType(type)) { context.reportError( `Expected GraphQL named type but got: ${inspect(type)}.`, type && type.astNode, ); - return; + continue; } // Ensure it is named correctly (excluding introspection types). @@ -267,7 +266,7 @@ function validateTypes(context: SchemaValidationContext): void { // Ensure Input Object fields are valid. validateInputFields(context, type); } - }); + } } function validateFields( @@ -284,7 +283,7 @@ function validateFields( ); } - fields.forEach(field => { + for (const field of fields) { // Ensure they are named correctly. validateName(context, field); @@ -295,7 +294,7 @@ function validateFields( `Field ${type.name}.${field.name} can only be defined once.`, fieldNodes, ); - return; // continue loop + continue; } // Ensure the type is an output type @@ -309,7 +308,7 @@ function validateFields( // Ensure the arguments are valid const argNames = Object.create(null); - field.args.forEach(arg => { + for (const arg of field.args) { const argName = arg.name; // Ensure they are named correctly. @@ -333,8 +332,8 @@ function validateFields( getFieldArgTypeNode(type, field.name, argName), ); } - }); - }); + } + } } function validateObjectInterfaces( @@ -342,14 +341,14 @@ function validateObjectInterfaces( object: GraphQLObjectType, ): void { const implementedTypeNames = Object.create(null); - object.getInterfaces().forEach(iface => { + for (const iface of object.getInterfaces()) { if (!isInterfaceType(iface)) { context.reportError( `Type ${inspect(object)} must only implement Interface types, ` + `it cannot implement ${inspect(iface)}.`, getImplementsInterfaceNode(object, iface), ); - return; + continue; } if (implementedTypeNames[iface.name]) { @@ -357,11 +356,11 @@ function validateObjectInterfaces( `Type ${object.name} can only implement ${iface.name} once.`, getAllImplementsInterfaceNodes(object, iface), ); - return; // continue loop + continue; } implementedTypeNames[iface.name] = true; validateObjectImplementsInterface(context, object, iface); - }); + } } function validateObjectImplementsInterface( @@ -373,7 +372,7 @@ function validateObjectImplementsInterface( const ifaceFieldMap = iface.getFields(); // Assert each interface field is implemented. - Object.keys(ifaceFieldMap).forEach(fieldName => { + for (const fieldName of Object.keys(ifaceFieldMap)) { const objectField = objectFieldMap[fieldName]; const ifaceField = ifaceFieldMap[fieldName]; @@ -384,8 +383,7 @@ function validateObjectImplementsInterface( `${object.name} does not provide it.`, [getFieldNode(iface, fieldName), object.astNode], ); - // Continue loop over fields. - return; + continue; } // Assert interface field type is satisfied by object field type, by being @@ -403,7 +401,7 @@ function validateObjectImplementsInterface( } // Assert each interface field arg is implemented. - ifaceField.args.forEach(ifaceArg => { + for (const ifaceArg of ifaceField.args) { const argName = ifaceArg.name; const objectArg = find(objectField.args, arg => arg.name === argName); @@ -417,8 +415,7 @@ function validateObjectImplementsInterface( getFieldNode(object, fieldName), ], ); - // Continue loop over arguments. - return; + continue; } // Assert interface field arg type matches object field arg type. @@ -438,10 +435,10 @@ function validateObjectImplementsInterface( } // TODO: validate default values? - }); + } // Assert additional arguments must not be required. - objectField.args.forEach(objectArg => { + for (const objectArg of objectField.args) { const argName = objectArg.name; const ifaceArg = find(ifaceField.args, arg => arg.name === argName); if (!ifaceArg && isNonNullType(objectArg.type)) { @@ -455,8 +452,8 @@ function validateObjectImplementsInterface( ], ); } - }); - }); + } + } } function validateUnionMembers( @@ -473,14 +470,14 @@ function validateUnionMembers( } const includedTypeNames = Object.create(null); - memberTypes.forEach(memberType => { + for (const memberType of memberTypes) { if (includedTypeNames[memberType.name]) { context.reportError( `Union type ${union.name} can only include type ` + `${memberType.name} once.`, getUnionMemberTypeNodes(union, memberType.name), ); - return; // continue loop + continue; } includedTypeNames[memberType.name] = true; if (!isObjectType(memberType)) { @@ -490,7 +487,7 @@ function validateUnionMembers( getUnionMemberTypeNodes(union, String(memberType)), ); } - }); + } } function validateEnumValues( @@ -506,7 +503,7 @@ function validateEnumValues( ); } - enumValues.forEach(enumValue => { + for (const enumValue of enumValues) { const valueName = enumValue.name; // Ensure no duplicates. @@ -526,7 +523,7 @@ function validateEnumValues( enumValue.astNode, ); } - }); + } } function validateInputFields( @@ -543,7 +540,7 @@ function validateInputFields( } // Ensure the arguments are valid - fields.forEach(field => { + for (const field of fields) { // Ensure they are named correctly. validateName(context, field); @@ -557,7 +554,7 @@ function validateInputFields( field.astNode && field.astNode.type, ); } - }); + } } function getAllNodes(object: { @@ -584,15 +581,13 @@ function getAllImplementsInterfaceNodes( iface: GraphQLInterfaceType, ): $ReadOnlyArray { const implementsNodes = []; - const astNodes = getAllNodes(type); - for (let i = 0; i < astNodes.length; i++) { - const astNode = astNodes[i]; + for (const astNode of getAllNodes(type)) { if (astNode && astNode.interfaces) { - astNode.interfaces.forEach(node => { + for (const node of astNode.interfaces) { if (node.name.value === iface.name) { implementsNodes.push(node); } - }); + } } } return implementsNodes; @@ -610,15 +605,13 @@ function getAllFieldNodes( fieldName: string, ): $ReadOnlyArray { const fieldNodes = []; - const astNodes = getAllNodes(type); - for (let i = 0; i < astNodes.length; i++) { - const astNode = astNodes[i]; + for (const astNode of getAllNodes(type)) { if (astNode && astNode.fields) { - astNode.fields.forEach(node => { + for (const node of astNode.fields) { if (node.name.value === fieldName) { fieldNodes.push(node); } - }); + } } } return fieldNodes; @@ -648,11 +641,11 @@ function getAllFieldArgNodes( const argNodes = []; const fieldNode = getFieldNode(type, fieldName); if (fieldNode && fieldNode.arguments) { - fieldNode.arguments.forEach(node => { + for (const node of fieldNode.arguments) { if (node.name.value === argName) { argNodes.push(node); } - }); + } } return argNodes; } @@ -673,11 +666,11 @@ function getAllDirectiveArgNodes( const argNodes = []; const directiveNode = directive.astNode; if (directiveNode && directiveNode.arguments) { - directiveNode.arguments.forEach(node => { + for (const node of directiveNode.arguments) { if (node.name.value === argName) { argNodes.push(node); } - }); + } } return argNodes; } From ac624bc5405e46157cbf0248ffdb22c86a734ded Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Fri, 22 Jun 2018 21:58:12 +0300 Subject: [PATCH 2/3] Add 'getAllSubNodes' utility function --- src/type/validate.js | 77 +++++++++++++++++++++----------------------- 1 file changed, 36 insertions(+), 41 deletions(-) diff --git a/src/type/validate.js b/src/type/validate.js index 1c3f5502f8..bd6721111d 100644 --- a/src/type/validate.js +++ b/src/type/validate.js @@ -155,13 +155,10 @@ function getOperationTypeNode( type: GraphQLObjectType, operation: string, ): ?ASTNode { - for (const node of getAllNodes(schema)) { - if (node.operationTypes) { - for (const operationType of node.operationTypes) { - if (operationType.operation === operation) { - return operationType.type; - } - } + const operationNodes = getAllSubNodes(schema, node => node.operationTypes); + for (const node of operationNodes) { + if (node.operation === operation) { + return node.type; } } @@ -557,10 +554,14 @@ function validateInputFields( } } -function getAllNodes(object: { +type SDLDefinedObject = { +astNode: ?T, +extensionASTNodes?: ?$ReadOnlyArray, -}): $ReadOnlyArray { +}; + +function getAllNodes( + object: SDLDefinedObject, +): $ReadOnlyArray { const { astNode, extensionASTNodes } = object; return astNode ? extensionASTNodes @@ -569,6 +570,22 @@ function getAllNodes(object: { : extensionASTNodes || []; } +function getAllSubNodes( + object: SDLDefinedObject, + getter: (T | K) => ?(L | $ReadOnlyArray), +): $ReadOnlyArray { + let result = []; + for (const astNode of getAllNodes(object)) { + if (astNode) { + const subNodes = getter(astNode); + if (subNodes) { + result = result.concat(subNodes); + } + } + } + return result; +} + function getImplementsInterfaceNode( type: GraphQLObjectType, iface: GraphQLInterfaceType, @@ -580,17 +597,9 @@ function getAllImplementsInterfaceNodes( type: GraphQLObjectType, iface: GraphQLInterfaceType, ): $ReadOnlyArray { - const implementsNodes = []; - for (const astNode of getAllNodes(type)) { - if (astNode && astNode.interfaces) { - for (const node of astNode.interfaces) { - if (node.name.value === iface.name) { - implementsNodes.push(node); - } - } - } - } - return implementsNodes; + return getAllSubNodes(type, typeNode => typeNode.interfaces).filter( + ifaceNode => ifaceNode.name.value === iface.name, + ); } function getFieldNode( @@ -604,17 +613,9 @@ function getAllFieldNodes( type: GraphQLObjectType | GraphQLInterfaceType, fieldName: string, ): $ReadOnlyArray { - const fieldNodes = []; - for (const astNode of getAllNodes(type)) { - if (astNode && astNode.fields) { - for (const node of astNode.fields) { - if (node.name.value === fieldName) { - fieldNodes.push(node); - } - } - } - } - return fieldNodes; + return getAllSubNodes(type, typeNode => typeNode.fields).filter( + fieldNode => fieldNode.name.value === fieldName, + ); } function getFieldTypeNode( @@ -663,16 +664,10 @@ function getAllDirectiveArgNodes( directive: GraphQLDirective, argName: string, ): $ReadOnlyArray { - const argNodes = []; - const directiveNode = directive.astNode; - if (directiveNode && directiveNode.arguments) { - for (const node of directiveNode.arguments) { - if (node.name.value === argName) { - argNodes.push(node); - } - } - } - return argNodes; + return getAllSubNodes( + directive, + directiveNode => directiveNode.arguments, + ).filter(argNode => argNode.name.value === argName); } function getDirectiveArgTypeNode( From 424579896a9718997436084cd157e7dfe024441e Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Sat, 30 Jun 2018 23:20:14 +0300 Subject: [PATCH 3/3] validateSchema: use nodes from new extensionASTNodes properties Fixes #1387 --- src/type/__tests__/validation-test.js | 81 +++++++++++++++++++++++---- src/type/validate.js | 20 +++---- 2 files changed, 78 insertions(+), 23 deletions(-) diff --git a/src/type/__tests__/validation-test.js b/src/type/__tests__/validation-test.js index c3eb592bbe..11a1e872df 100644 --- a/src/type/__tests__/validation-test.js +++ b/src/type/__tests__/validation-test.js @@ -562,23 +562,33 @@ describe('Type System: Union types must be valid', () => { }); it('rejects a Union type with empty types', () => { - const schema = buildSchema(` + let schema = buildSchema(` type Query { test: BadUnion } union BadUnion `); + + schema = extendSchema( + schema, + parse(` + directive @test on UNION + + extend union BadUnion @test + `), + ); + expect(validateSchema(schema)).to.deep.equal([ { message: 'Union type BadUnion must define one or more member types.', - locations: [{ line: 6, column: 7 }], + locations: [{ line: 6, column: 7 }, { line: 4, column: 9 }], }, ]); }); it('rejects a Union type with duplicated member type', () => { - const schema = buildSchema(` + let schema = buildSchema(` type Query { test: BadUnion } @@ -596,16 +606,30 @@ describe('Type System: Union types must be valid', () => { | TypeB | TypeA `); + expect(validateSchema(schema)).to.deep.equal([ { message: 'Union type BadUnion can only include type TypeA once.', locations: [{ line: 15, column: 11 }, { line: 17, column: 11 }], }, ]); + + schema = extendSchema(schema, parse('extend union BadUnion = TypeB')); + + expect(validateSchema(schema)).to.deep.equal([ + { + message: 'Union type BadUnion can only include type TypeA once.', + locations: [{ line: 15, column: 11 }, { line: 17, column: 11 }], + }, + { + message: 'Union type BadUnion can only include type TypeB once.', + locations: [{ line: 16, column: 11 }, { line: 1, column: 25 }], + }, + ]); }); it('rejects a Union type with non-Object members types', () => { - const schema = buildSchema(` + let schema = buildSchema(` type Query { test: BadUnion } @@ -623,13 +647,20 @@ describe('Type System: Union types must be valid', () => { | String | TypeB `); + + schema = extendSchema(schema, parse('extend union BadUnion = Int')); + expect(validateSchema(schema)).to.deep.equal([ { message: - 'Union type BadUnion can only include Object types, ' + - 'it cannot include String.', + 'Union type BadUnion can only include Object types, it cannot include String.', locations: [{ line: 16, column: 11 }], }, + { + message: + 'Union type BadUnion can only include Object types, it cannot include Int.', + locations: [{ line: 1, column: 25 }], + }, ]); const badUnionMemberTypes = [ @@ -671,18 +702,28 @@ describe('Type System: Input Objects must have fields', () => { }); it('rejects an Input Object type with missing fields', () => { - const schema = buildSchema(` + let schema = buildSchema(` type Query { field(arg: SomeInputObject): String } input SomeInputObject `); + + schema = extendSchema( + schema, + parse(` + directive @test on ENUM + + extend input SomeInputObject @test + `), + ); + expect(validateSchema(schema)).to.deep.equal([ { message: 'Input Object type SomeInputObject must define one or more fields.', - locations: [{ line: 6, column: 7 }], + locations: [{ line: 6, column: 7 }, { line: 4, column: 9 }], }, ]); }); @@ -722,17 +763,27 @@ describe('Type System: Input Objects must have fields', () => { describe('Type System: Enum types must be well defined', () => { it('rejects an Enum type without values', () => { - const schema = buildSchema(` + let schema = buildSchema(` type Query { field: SomeEnum } enum SomeEnum `); + + schema = extendSchema( + schema, + parse(` + directive @test on ENUM + + extend enum SomeEnum @test + `), + ); + expect(validateSchema(schema)).to.deep.equal([ { message: 'Enum type SomeEnum must define one or more values.', - locations: [{ line: 6, column: 7 }], + locations: [{ line: 6, column: 7 }, { line: 4, column: 9 }], }, ]); }); @@ -1000,13 +1051,21 @@ describe('Type System: Interface extensions should be valid', () => { extend interface AnotherInterface { newField: String } + + extend type AnotherObject { + differentNewField: String + } `), ); expect(validateSchema(extendedSchema)).to.deep.equal([ { message: 'Interface field AnotherInterface.newField expected but AnotherObject does not provide it.', - locations: [{ line: 3, column: 11 }, { line: 10, column: 7 }], + locations: [ + { line: 3, column: 11 }, + { line: 10, column: 7 }, + { line: 6, column: 9 }, + ], }, ]); }); diff --git a/src/type/validate.js b/src/type/validate.js index bd6721111d..1d8bdb13a1 100644 --- a/src/type/validate.js +++ b/src/type/validate.js @@ -378,7 +378,7 @@ function validateObjectImplementsInterface( context.reportError( `Interface field ${iface.name}.${fieldName} expected but ` + `${object.name} does not provide it.`, - [getFieldNode(iface, fieldName), object.astNode], + [getFieldNode(iface, fieldName), ...getAllNodes(object)], ); continue; } @@ -462,7 +462,7 @@ function validateUnionMembers( if (memberTypes.length === 0) { context.reportError( `Union type ${union.name} must define one or more member types.`, - union.astNode, + getAllNodes(union), ); } @@ -496,7 +496,7 @@ function validateEnumValues( if (enumValues.length === 0) { context.reportError( `Enum type ${enumType.name} must define one or more values.`, - enumType.astNode, + getAllNodes(enumType), ); } @@ -532,7 +532,7 @@ function validateInputFields( if (fields.length === 0) { context.reportError( `Input Object type ${inputObj.name} must define one or more fields.`, - inputObj.astNode, + getAllNodes(inputObj), ); } @@ -682,10 +682,8 @@ function getUnionMemberTypeNodes( union: GraphQLUnionType, typeName: string, ): ?$ReadOnlyArray { - return ( - union.astNode && - union.astNode.types && - union.astNode.types.filter(type => type.name.value === typeName) + return getAllSubNodes(union, unionNode => unionNode.types).filter( + typeNode => typeNode.name.value === typeName, ); } @@ -693,9 +691,7 @@ function getEnumValueNodes( enumType: GraphQLEnumType, valueName: string, ): ?$ReadOnlyArray { - return ( - enumType.astNode && - enumType.astNode.values && - enumType.astNode.values.filter(value => value.name.value === valueName) + return getAllSubNodes(enumType, enumNode => enumNode.values).filter( + valueNode => valueNode.name.value === valueName, ); }