Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make 'ASTDefinitionBuilder' responsible only for build types from AST #1230

Merged
merged 4 commits into from
Feb 15, 2018
+58 −64
Merged

Make 'ASTDefinitionBuilder' responsible only for build types from AST #1230

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
693db20
Make 'ASTDefinitionBuilder' responsible only for build types from AST
IvanGoncharov Feb 8, 2018
ca2cdc7
Address review comments.
IvanGoncharov Feb 9, 2018
a8b9539
Simplify extendType()
leebyron Feb 9, 2018
6f1867c
Fix flow errors
IvanGoncharov Feb 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 16 additions & 25 deletions src/utilities/buildASTSchema.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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<TypeDefinitionNode>;
type TypeResolver = (
typeName: string,
node?: ?NamedTypeNode,
) => GraphQLNamedType;
type TypeResolver = (typeRef: NamedTypeNode) => GraphQLNamedType;

export class ASTDefinitionBuilder {
_typeDefinitionsMap: TypeDefinitionsMap;
Expand All @@ -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);
Expand Down
96 changes: 48 additions & 48 deletions src/utilities/extendSchema.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -194,56 +196,52 @@ 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);
return getExtendedType(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)
? getExtendedType(existingQueryType)
: null;

const existingMutationType = schema.getMutationType();
const mutationType = existingMutationType
? (definitionBuilder.buildType(existingMutationType.name): any)
? getExtendedType(existingMutationType)
: null;

const existingSubscriptionType = schema.getSubscriptionType();
const subscriptionType = existingSubscriptionType
? (definitionBuilder.buildType(existingSubscriptionType.name): any)
? getExtendedType(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 => getExtendedType(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({
Expand Down Expand Up @@ -275,30 +273,32 @@ 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<T: GraphQLNamedType>(typeDef: T): T {
const type = definitionBuilder.buildType(typeDef.name);
return (type: any);
function getExtendedType<T: GraphQLNamedType>(type: T): T {
if (!extendTypeCache[type.name]) {
extendTypeCache[type.name] = extendType(type);
}
return (extendTypeCache[type.name]: 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);
// Should be called only once per type so only getExtendedType should call it.
function extendType<T: GraphQLNamedType>(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);
}
}
return type;
// Workaround: Flow should figure out correct type, but it doesn't.
return (extendedType: any);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest factoring this into two functions, getExtendedType and extendType, the first one performing caching (with a comment that extendType need only be called once per type). That should allow you to remove or at least further isolate the any cast.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leebyron Great idea 👍 I will make the change in a few minutes.

Copy link
Member Author

@IvanGoncharov IvanGoncharov Feb 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leebyron Sadly there is some kind of bug with Flow which prevents the suggested change:
image

You can test it here online but demo broken for 0.65 at the moment so you need to switch it to 0.64. But I tested 0.65 locally and the result is the same.

What should I do?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cast through any internally while keeping the correct function definition, and we'll report it to the flow team

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leebyron Done. Can you please review?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than mutating a variable in each branch, just return the value in each branch. Flow can't figure out what the type of extendedType is, since there are so many different possible local mutations

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leebyron I did that initially but it forces me to do typecast in every branch. Plus explaining workaround is more complicated.

Flow can't figure out what the type of extendedType is, since there are so many different possible local mutations

Flow has exactly the same problem figuring types both with or without mutations. You can see it in CI logs for your commit and in Flow playground:
image
image

}

function extendObjectType(type: GraphQLObjectType): GraphQLObjectType {
Expand Down Expand Up @@ -342,7 +342,7 @@ export function extendSchema(
return new GraphQLUnionType({
name: type.name,
description: type.description,
types: type.getTypes().map(getTypeFromDef),
types: type.getTypes().map(getExtendedType),
astNode: type.astNode,
resolveType: type.resolveType,
});
Expand All @@ -351,7 +351,7 @@ export function extendSchema(
function extendImplementedInterfaces(
type: GraphQLObjectType,
): Array<GraphQLInterfaceType> {
const interfaces = type.getInterfaces().map(getTypeFromDef);
const interfaces = type.getInterfaces().map(getExtendedType);

// If there are any extensions to the interfaces, apply those here.
const extensions = typeExtensionsMap[type.name];
Expand All @@ -361,7 +361,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));
});
});
}
Expand Down Expand Up @@ -397,7 +397,7 @@ export function extendSchema(
[field],
);
}
newFieldMap[fieldName] = definitionBuilder.buildField(field);
newFieldMap[fieldName] = astBuilder.buildField(field);
});
});
}
Expand All @@ -412,6 +412,6 @@ export function extendSchema(
if (isNonNullType(typeDef)) {
return (GraphQLNonNull(extendFieldType(typeDef.ofType)): any);
}
return getTypeFromDef(typeDef);
return getExtendedType(typeDef);
}
}