From 693db206231b1ed44820b9942c504d9e945de84a Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Thu, 8 Feb 2018 18:40:03 +0200 Subject: [PATCH 1/4] Make 'ASTDefinitionBuilder' responsible only for build types from AST --- src/utilities/buildASTSchema.js | 41 ++++++-------- src/utilities/extendSchema.js | 97 ++++++++++++++++----------------- 2 files changed, 62 insertions(+), 76 deletions(-) diff --git a/src/utilities/buildASTSchema.js b/src/utilities/buildASTSchema.js index 248c9a6c38..afdcef3891 100644 --- a/src/utilities/buildASTSchema.js +++ b/src/utilities/buildASTSchema.js @@ -176,22 +176,20 @@ export function buildASTSchema( const operationTypes = schemaDef ? getOperationTypes(schemaDef) : { - query: nodeMap.Query ? 'Query' : null, - mutation: nodeMap.Mutation ? 'Mutation' : null, - subscription: nodeMap.Subscription ? 'Subscription' : null, + query: nodeMap.Query, + mutation: nodeMap.Mutation, + subscription: nodeMap.Subscription, }; const definitionBuilder = new ASTDefinitionBuilder( nodeMap, options, - typeName => { - throw new Error(`Type "${typeName}" not found in document.`); + typeRef => { + throw new Error(`Type "${typeRef.name.value}" not found in document.`); }, ); - const types = typeDefs.map(def => - definitionBuilder.buildType(def.name.value), - ); + const types = typeDefs.map(def => definitionBuilder.buildType(def)); const directives = directiveDefs.map(def => definitionBuilder.buildDirective(def), @@ -242,17 +240,14 @@ export function buildASTSchema( `Specified ${operation} type "${typeName}" not found in document.`, ); } - opTypes[operation] = typeName; + opTypes[operation] = operationType.type; }); return opTypes; } } type TypeDefinitionsMap = ObjMap; -type TypeResolver = ( - typeName: string, - node?: ?NamedTypeNode, -) => GraphQLNamedType; +type TypeResolver = (typeRef: NamedTypeNode) => GraphQLNamedType; export class ASTDefinitionBuilder { _typeDefinitionsMap: TypeDefinitionsMap; @@ -275,25 +270,21 @@ export class ASTDefinitionBuilder { ); } - _buildType(typeName: string, typeNode?: ?NamedTypeNode): GraphQLNamedType { + buildType(node: NamedTypeNode | TypeDefinitionNode): GraphQLNamedType { + const typeName = node.name.value; if (!this._cache[typeName]) { - const defNode = this._typeDefinitionsMap[typeName]; - if (defNode) { - this._cache[typeName] = this._makeSchemaDef(defNode); + if (node.kind === Kind.NAMED_TYPE) { + const defNode = this._typeDefinitionsMap[typeName]; + this._cache[typeName] = defNode + ? this._makeSchemaDef(defNode) + : this._resolveType(node); } else { - this._cache[typeName] = this._resolveType(typeName, typeNode); + this._cache[typeName] = this._makeSchemaDef(node); } } return this._cache[typeName]; } - buildType(ref: string | NamedTypeNode): GraphQLNamedType { - if (typeof ref === 'string') { - return this._buildType(ref); - } - return this._buildType(ref.name.value, ref); - } - _buildWrappedType(typeNode: TypeNode): GraphQLType { const typeDef = this.buildType(getNamedTypeNode(typeNode)); return buildWrappedType(typeDef, typeNode); diff --git a/src/utilities/extendSchema.js b/src/utilities/extendSchema.js index c7c7f6815f..80856db439 100644 --- a/src/utilities/extendSchema.js +++ b/src/utilities/extendSchema.js @@ -9,9 +9,11 @@ import invariant from '../jsutils/invariant'; import keyMap from '../jsutils/keyMap'; +import objectValues from '../jsutils/objectValues'; import { ASTDefinitionBuilder } from './buildASTSchema'; import { GraphQLError } from '../error/GraphQLError'; import { isSchema, GraphQLSchema } from '../type/schema'; +import { isIntrospectionType } from '../type/introspection'; import { isObjectType, @@ -194,56 +196,50 @@ export function extendSchema( return schema; } - const definitionBuilder = new ASTDefinitionBuilder( + const astBuilder = new ASTDefinitionBuilder( typeDefinitionMap, options, - (typeName, node) => { + typeRef => { + const typeName = typeRef.name.value; const existingType = schema.getType(typeName); if (existingType) { return extendType(existingType); } - if (node) { - throw new GraphQLError( - `Unknown type: "${typeName}". Ensure that this type exists ` + - 'either in the original schema, or is added in a type definition.', - [node], - ); - } - throw GraphQLError('Missing type from schema'); + throw new GraphQLError( + `Unknown type: "${typeName}". Ensure that this type exists ` + + 'either in the original schema, or is added in a type definition.', + [typeRef], + ); }, ); + const extendTypeCache = Object.create(null); + // Get the root Query, Mutation, and Subscription object types. // Note: While this could make early assertions to get the correctly // typed values below, that would throw immediately while type system // validation with validateSchema() will produce more actionable results. const existingQueryType = schema.getQueryType(); - const queryType = existingQueryType - ? (definitionBuilder.buildType(existingQueryType.name): any) - : null; + const queryType = existingQueryType ? extendType(existingQueryType) : null; const existingMutationType = schema.getMutationType(); const mutationType = existingMutationType - ? (definitionBuilder.buildType(existingMutationType.name): any) + ? extendType(existingMutationType) : null; const existingSubscriptionType = schema.getSubscriptionType(); const subscriptionType = existingSubscriptionType - ? (definitionBuilder.buildType(existingSubscriptionType.name): any) + ? extendType(existingSubscriptionType) : null; - // Iterate through all types, getting the type definition for each, ensuring - // that any type not directly referenced by a field will get created. - const typeMap = schema.getTypeMap(); - const types = Object.keys(typeMap).map(typeName => - definitionBuilder.buildType(typeName), - ); - - // Do the same with new types, appending to the list of defined types. - Object.keys(typeDefinitionMap).forEach(typeName => { - types.push(definitionBuilder.buildType(typeName)); - }); + const types = [ + // Iterate through all types, getting the type definition for each, ensuring + // that any type not directly referenced by a field will get created. + ...objectValues(schema.getTypeMap()).map(type => extendType(type)), + // Do the same with new types. + ...objectValues(typeDefinitionMap).map(type => astBuilder.buildType(type)), + ]; // Then produce and return a Schema with these types. return new GraphQLSchema({ @@ -275,30 +271,29 @@ export function extendSchema( const existingDirectives = schema.getDirectives(); invariant(existingDirectives, 'schema must have default directives'); - const newDirectives = directiveDefinitions.map(directiveNode => - definitionBuilder.buildDirective(directiveNode), + return existingDirectives.concat( + directiveDefinitions.map(node => astBuilder.buildDirective(node)), ); - return existingDirectives.concat(newDirectives); - } - - function getTypeFromDef(typeDef: T): T { - const type = definitionBuilder.buildType(typeDef.name); - return (type: any); } - // Given a type's introspection result, construct the correct - // GraphQLType instance. - function extendType(type: GraphQLNamedType): GraphQLNamedType { - if (isObjectType(type)) { - return extendObjectType(type); - } - if (isInterfaceType(type)) { - return extendInterfaceType(type); - } - if (isUnionType(type)) { - return extendUnionType(type); + function extendType(type: T): T { + let extendedType = extendTypeCache[type.name]; + + if (!extendedType) { + if (isIntrospectionType(type)) { + extendedType = type; + } else if (isObjectType(type)) { + extendedType = extendObjectType(type); + } else if (isInterfaceType(type)) { + extendedType = extendInterfaceType(type); + } else if (isUnionType(type)) { + extendedType = extendUnionType(type); + } else { + extendedType = type; + } + extendTypeCache[type.name] = extendedType; } - return type; + return (extendedType: any); } function extendObjectType(type: GraphQLObjectType): GraphQLObjectType { @@ -342,7 +337,7 @@ export function extendSchema( return new GraphQLUnionType({ name: type.name, description: type.description, - types: type.getTypes().map(getTypeFromDef), + types: type.getTypes().map(extendType), astNode: type.astNode, resolveType: type.resolveType, }); @@ -351,7 +346,7 @@ export function extendSchema( function extendImplementedInterfaces( type: GraphQLObjectType, ): Array { - const interfaces = type.getInterfaces().map(getTypeFromDef); + const interfaces = type.getInterfaces().map(extendType); // If there are any extensions to the interfaces, apply those here. const extensions = typeExtensionsMap[type.name]; @@ -361,7 +356,7 @@ 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. - interfaces.push((definitionBuilder.buildType(namedType): any)); + interfaces.push((astBuilder.buildType(namedType): any)); }); }); } @@ -397,7 +392,7 @@ export function extendSchema( [field], ); } - newFieldMap[fieldName] = definitionBuilder.buildField(field); + newFieldMap[fieldName] = astBuilder.buildField(field); }); }); } @@ -412,6 +407,6 @@ export function extendSchema( if (isNonNullType(typeDef)) { return (GraphQLNonNull(extendFieldType(typeDef.ofType)): any); } - return getTypeFromDef(typeDef); + return extendType(typeDef); } } From ca2cdc7e61c384d85c54800eb4984a231fa07680 Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Fri, 9 Feb 2018 20:22:01 +0200 Subject: [PATCH 2/4] Address review comments. --- src/utilities/extendSchema.js | 39 ++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/src/utilities/extendSchema.js b/src/utilities/extendSchema.js index 80856db439..f629c19354 100644 --- a/src/utilities/extendSchema.js +++ b/src/utilities/extendSchema.js @@ -203,7 +203,7 @@ export function extendSchema( const typeName = typeRef.name.value; const existingType = schema.getType(typeName); if (existingType) { - return extendType(existingType); + return getExtendedType(existingType); } throw new GraphQLError( @@ -221,22 +221,24 @@ export function extendSchema( // typed values below, that would throw immediately while type system // validation with validateSchema() will produce more actionable results. const existingQueryType = schema.getQueryType(); - const queryType = existingQueryType ? extendType(existingQueryType) : null; + const queryType = existingQueryType + ? getExtendedType(existingQueryType) + : null; const existingMutationType = schema.getMutationType(); const mutationType = existingMutationType - ? extendType(existingMutationType) + ? getExtendedType(existingMutationType) : null; const existingSubscriptionType = schema.getSubscriptionType(); const subscriptionType = existingSubscriptionType - ? extendType(existingSubscriptionType) + ? getExtendedType(existingSubscriptionType) : null; const types = [ // Iterate through all types, getting the type definition for each, ensuring // that any type not directly referenced by a field will get created. - ...objectValues(schema.getTypeMap()).map(type => extendType(type)), + ...objectValues(schema.getTypeMap()).map(type => getExtendedType(type)), // Do the same with new types. ...objectValues(typeDefinitionMap).map(type => astBuilder.buildType(type)), ]; @@ -276,23 +278,26 @@ export function extendSchema( ); } - function extendType(type: T): T { - let extendedType = extendTypeCache[type.name]; + function getExtendedType(type: T): T { + if (!extendTypeCache[type.name]) { + extendTypeCache[type.name] = extendType(type); + } + return (extendTypeCache[type.name]: any); + } - if (!extendedType) { - if (isIntrospectionType(type)) { - extendedType = type; - } else if (isObjectType(type)) { + // Should be called only once per type so only getExtendedType should call it. + function extendType(type: T): T { + let extendedType = type; + if (!isIntrospectionType(type)) { + if (isObjectType(type)) { extendedType = extendObjectType(type); } else if (isInterfaceType(type)) { extendedType = extendInterfaceType(type); } else if (isUnionType(type)) { extendedType = extendUnionType(type); - } else { - extendedType = type; } - extendTypeCache[type.name] = extendedType; } + // Workaround: Flow should figure out correct type, but it doesn't. return (extendedType: any); } @@ -337,7 +342,7 @@ export function extendSchema( return new GraphQLUnionType({ name: type.name, description: type.description, - types: type.getTypes().map(extendType), + types: type.getTypes().map(getExtendedType), astNode: type.astNode, resolveType: type.resolveType, }); @@ -346,7 +351,7 @@ export function extendSchema( function extendImplementedInterfaces( type: GraphQLObjectType, ): Array { - const interfaces = type.getInterfaces().map(extendType); + const interfaces = type.getInterfaces().map(getExtendedType); // If there are any extensions to the interfaces, apply those here. const extensions = typeExtensionsMap[type.name]; @@ -407,6 +412,6 @@ export function extendSchema( if (isNonNullType(typeDef)) { return (GraphQLNonNull(extendFieldType(typeDef.ofType)): any); } - return extendType(typeDef); + return getExtendedType(typeDef); } } From a8b9539cd23e88cdd81473c42f219095f7e9121f Mon Sep 17 00:00:00 2001 From: Lee Byron Date: Fri, 9 Feb 2018 12:13:23 -0800 Subject: [PATCH 3/4] Simplify extendType() --- src/utilities/extendSchema.js | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/src/utilities/extendSchema.js b/src/utilities/extendSchema.js index f629c19354..61b73352a4 100644 --- a/src/utilities/extendSchema.js +++ b/src/utilities/extendSchema.js @@ -285,20 +285,23 @@ export function extendSchema( return (extendTypeCache[type.name]: any); } - // Should be called only once per type so only getExtendedType should call it. + // To be called at most once per type. Only getExtendedType should call this. function extendType(type: T): T { - let extendedType = type; - if (!isIntrospectionType(type)) { - if (isObjectType(type)) { - extendedType = extendObjectType(type); - } else if (isInterfaceType(type)) { - extendedType = extendInterfaceType(type); - } else if (isUnionType(type)) { - extendedType = extendUnionType(type); - } + if (isIntrospectionType(type)) { + // Introspection types are not extended. + return type; + } + if (isObjectType(type)) { + return extendObjectType(type); + } + if (isInterfaceType(type)) { + return extendInterfaceType(type); + } + if (isUnionType(type)) { + return extendUnionType(type); } - // Workaround: Flow should figure out correct type, but it doesn't. - return (extendedType: any); + // This type is not yet extendable. + return type; } function extendObjectType(type: GraphQLObjectType): GraphQLObjectType { From 6f1867ca3de29e859eaa7e3a4bf91f93709dca3e Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Wed, 14 Feb 2018 12:19:43 +0200 Subject: [PATCH 4/4] Fix flow errors --- src/utilities/extendSchema.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utilities/extendSchema.js b/src/utilities/extendSchema.js index 61b73352a4..3a437b30a5 100644 --- a/src/utilities/extendSchema.js +++ b/src/utilities/extendSchema.js @@ -286,7 +286,7 @@ export function extendSchema( } // To be called at most once per type. Only getExtendedType should call this. - function extendType(type: T): T { + function extendType(type) { if (isIntrospectionType(type)) { // Introspection types are not extended. return type;