From f370fc30e6410fda3db7b7b5573fdc87bb14e931 Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Fri, 20 Jul 2018 14:03:22 +0300 Subject: [PATCH 1/3] Small refactoring --- src/utilities/buildASTSchema.js | 48 +++++++++------------------------ src/utilities/extendSchema.js | 3 --- 2 files changed, 13 insertions(+), 38 deletions(-) diff --git a/src/utilities/buildASTSchema.js b/src/utilities/buildASTSchema.js index ce7ec95113..df757ec539 100644 --- a/src/utilities/buildASTSchema.js +++ b/src/utilities/buildASTSchema.js @@ -47,7 +47,6 @@ import type { } from '../type/definition'; import { - assertNullableType, GraphQLScalarType, GraphQLObjectType, GraphQLInterfaceType, @@ -92,31 +91,6 @@ export type BuildSchemaOptions = { commentDescriptions?: boolean, }; -function buildWrappedType( - innerType: GraphQLType, - inputTypeNode: TypeNode, -): GraphQLType { - if (inputTypeNode.kind === Kind.LIST_TYPE) { - return GraphQLList(buildWrappedType(innerType, inputTypeNode.type)); - } - if (inputTypeNode.kind === Kind.NON_NULL_TYPE) { - const wrappedType = buildWrappedType(innerType, inputTypeNode.type); - return GraphQLNonNull(assertNullableType(wrappedType)); - } - return innerType; -} - -function getNamedTypeNode(typeNode: TypeNode): NamedTypeNode { - let namedType = typeNode; - while ( - namedType.kind === Kind.LIST_TYPE || - namedType.kind === Kind.NON_NULL_TYPE - ) { - namedType = namedType.type; - } - return namedType; -} - /** * This takes the ast of a schema document produced by the parse function in * src/language/parser.js. @@ -190,7 +164,6 @@ export function buildASTSchema( }, ); - const types = definitionBuilder.buildTypes(typeDefs); const directives = directiveDefs.map(def => definitionBuilder.buildDirective(def), ); @@ -221,7 +194,7 @@ export function buildASTSchema( subscription: operationTypes.subscription ? (definitionBuilder.buildType(operationTypes.subscription): any) : null, - types, + types: typeDefs.map(node => definitionBuilder.buildType(node)), directives, astNode: schemaDef, assumeValid: options && options.assumeValid, @@ -271,9 +244,7 @@ export class ASTDefinitionBuilder { ); } - buildTypes( - nodes: $ReadOnlyArray, - ): Array { + buildTypes(nodes: $ReadOnlyArray): Array { return nodes.map(node => this.buildType(node)); } @@ -293,8 +264,16 @@ export class ASTDefinitionBuilder { } _buildWrappedType(typeNode: TypeNode): GraphQLType { - const typeDef = this.buildType(getNamedTypeNode(typeNode)); - return buildWrappedType(typeDef, typeNode); + if (typeNode.kind === Kind.LIST_TYPE) { + return GraphQLList(this._buildWrappedType(typeNode.type)); + } + if (typeNode.kind === Kind.NON_NULL_TYPE) { + return GraphQLNonNull( + // Note: GraphQLNonNull constructor validates this type + (this._buildWrappedType(typeNode.type): any), + ); + } + return this.buildType(typeNode); } buildDirective(directiveNode: DirectiveDefinitionNode): GraphQLDirective { @@ -366,10 +345,9 @@ export class ASTDefinitionBuilder { } _makeTypeDef(def: ObjectTypeDefinitionNode) { - const typeName = def.name.value; const interfaces = def.interfaces; return new GraphQLObjectType({ - name: typeName, + name: def.name.value, description: getDescription(def, this._options), fields: () => this._makeFieldDefMap(def), // Note: While this could make early assertions to get the correctly diff --git a/src/utilities/extendSchema.js b/src/utilities/extendSchema.js index 39c7fa6e7f..665b2a5bde 100644 --- a/src/utilities/extendSchema.js +++ b/src/utilities/extendSchema.js @@ -299,9 +299,6 @@ export function extendSchema( extendTypeCache[name] = extendEnumType(type); } else if (isInputObjectType(type)) { extendTypeCache[name] = extendInputObjectType(type); - } else { - // This type is not yet extendable. - extendTypeCache[name] = type; } } return (extendTypeCache[name]: any); From 0ad0e46064752f52e28c7ddc2c02746a9c57595c Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Fri, 20 Jul 2018 19:52:45 +0300 Subject: [PATCH 2/3] buildSchema: Fix infinite loop with recursive union --- src/utilities/__tests__/buildASTSchema-test.js | 12 ++++++++++++ src/utilities/buildASTSchema.js | 13 ++++++------- src/utilities/extendSchema.js | 2 +- 3 files changed, 19 insertions(+), 8 deletions(-) diff --git a/src/utilities/__tests__/buildASTSchema-test.js b/src/utilities/__tests__/buildASTSchema-test.js index b4b0545df3..48843784ce 100644 --- a/src/utilities/__tests__/buildASTSchema-test.js +++ b/src/utilities/__tests__/buildASTSchema-test.js @@ -356,6 +356,18 @@ describe('Schema Builder', () => { expect(output).to.equal(body); }); + it('Can build recursive Union', () => { + const schema = buildSchema(dedent` + union Hello = Hello + + type Query { + hello: Hello + } + `); + const errors = validateSchema(schema); + expect(errors.length).to.be.above(0); + }); + it('Specifying Union type using __typename', () => { const schema = buildSchema(dedent` type Query { diff --git a/src/utilities/buildASTSchema.js b/src/utilities/buildASTSchema.js index df757ec539..f6139d7f5d 100644 --- a/src/utilities/buildASTSchema.js +++ b/src/utilities/buildASTSchema.js @@ -244,10 +244,6 @@ export class ASTDefinitionBuilder { ); } - buildTypes(nodes: $ReadOnlyArray): Array { - return nodes.map(node => this.buildType(node)); - } - buildType(node: NamedTypeNode | TypeDefinitionNode): GraphQLNamedType { const typeName = node.name.value; if (!this._cache[typeName]) { @@ -345,7 +341,7 @@ export class ASTDefinitionBuilder { } _makeTypeDef(def: ObjectTypeDefinitionNode) { - const interfaces = def.interfaces; + const interfaces: ?$ReadOnlyArray = def.interfaces; return new GraphQLObjectType({ name: def.name.value, description: getDescription(def, this._options), @@ -353,7 +349,9 @@ export class ASTDefinitionBuilder { // Note: While this could make early assertions to get the correctly // typed values, that would throw immediately while type system // validation with validateSchema() will produce more actionable results. - interfaces: interfaces ? () => (this.buildTypes(interfaces): any) : [], + interfaces: interfaces + ? () => interfaces.map(ref => (this.buildType(ref): any)) + : [], astNode: def, }); } @@ -407,13 +405,14 @@ export class ASTDefinitionBuilder { } _makeUnionDef(def: UnionTypeDefinitionNode) { + const types: ?$ReadOnlyArray = def.types; return new GraphQLUnionType({ name: def.name.value, description: getDescription(def, this._options), // Note: While this could make assertions to get the correctly typed // values below, that would throw immediately while type system // validation with validateSchema() will produce more actionable results. - types: def.types ? (this.buildTypes(def.types): any) : [], + types: types ? () => types.map(ref => (this.buildType(ref): any)) : [], astNode: def, }); } diff --git a/src/utilities/extendSchema.js b/src/utilities/extendSchema.js index 665b2a5bde..d8e1888238 100644 --- a/src/utilities/extendSchema.js +++ b/src/utilities/extendSchema.js @@ -243,7 +243,7 @@ export function extendSchema( // that any type not directly referenced by a field will get created. ...objectValues(schema.getTypeMap()).map(type => extendNamedType(type)), // Do the same with new types. - ...astBuilder.buildTypes(objectValues(typeDefinitionMap)), + ...objectValues(typeDefinitionMap).map(type => astBuilder.buildType(type)), ]; // Support both original legacy names and extended legacy names. From 44ac2db2fe71d78caf9f47748f750f32c5389d56 Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Fri, 20 Jul 2018 20:03:12 +0300 Subject: [PATCH 3/3] extendSchema: Fix infinite loop with recursive union --- src/utilities/__tests__/extendSchema-test.js | 13 ++++++++++ src/utilities/extendSchema.js | 27 ++++++++++++-------- 2 files changed, 29 insertions(+), 11 deletions(-) diff --git a/src/utilities/__tests__/extendSchema-test.js b/src/utilities/__tests__/extendSchema-test.js index 2fb59c1a28..e56ffcc08c 100644 --- a/src/utilities/__tests__/extendSchema-test.js +++ b/src/utilities/__tests__/extendSchema-test.js @@ -280,6 +280,19 @@ describe('extendSchema', () => { expect(unionField.type).to.equal(someUnionType); }); + it('allows extension of union by adding itself', () => { + const extendedSchema = extendTestSchema(` + extend union SomeUnion = SomeUnion + `); + + const errors = validateSchema(extendedSchema); + expect(errors.length).to.be.above(0); + + expect(printTestSchemaChanges(extendedSchema)).to.equal(dedent` + union SomeUnion = Foo | Biz | SomeUnion + `); + }); + it('extends inputs by adding new fields', () => { const extendedSchema = extendTestSchema(` extend input SomeInput { diff --git a/src/utilities/extendSchema.js b/src/utilities/extendSchema.js index d8e1888238..c4e55b2d9f 100644 --- a/src/utilities/extendSchema.js +++ b/src/utilities/extendSchema.js @@ -494,7 +494,20 @@ export function extendSchema( ? type.extensionASTNodes.concat(typeExtensionsMap[name]) : typeExtensionsMap[name] : type.extensionASTNodes; - const unionTypes = type.getTypes().map(extendNamedType); + return new GraphQLUnionType({ + name, + description: type.description, + types: () => extendPossibleTypes(type), + astNode: type.astNode, + resolveType: type.resolveType, + extensionASTNodes, + }); + } + + function extendPossibleTypes( + type: GraphQLUnionType, + ): Array { + const possibleTypes = type.getTypes().map(extendNamedType); // If there are any extensions to the union, apply those here. const extensions = typeExtensionsMap[type.name]; @@ -504,19 +517,11 @@ export function extendSchema( // Note: While this could make early assertions to get the correctly // typed values, that would throw immediately while type system // validation with validateSchema() will produce more actionable results. - unionTypes.push((astBuilder.buildType(namedType): any)); + possibleTypes.push((astBuilder.buildType(namedType): any)); } } } - - return new GraphQLUnionType({ - name, - description: type.description, - types: unionTypes, - astNode: type.astNode, - resolveType: type.resolveType, - extensionASTNodes, - }); + return possibleTypes; } function extendImplementedInterfaces(