Skip to content

Prevent infinite recursion on invalid unions #1427

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.

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

Merged
Merged
Changes from all commits
Commits
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
12 changes: 12 additions & 0 deletions src/utilities/__tests__/buildASTSchema-test.js
Original file line number Diff line number Diff line change
@@ -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 {
13 changes: 13 additions & 0 deletions src/utilities/__tests__/extendSchema-test.js
Original file line number Diff line number Diff line change
@@ -280,6 +280,19 @@ describe('extendSchema', () => {
expect(unionField.type).to.equal(someUnionType);
});

it('allows extension of union by adding itself', () => {
const extendedSchema = extendTestSchema(`
Copy link
Contributor

Choose a reason for hiding this comment

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

These are some nifty tests. Thanks for adding them!

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 {
59 changes: 18 additions & 41 deletions src/utilities/buildASTSchema.js
Original file line number Diff line number Diff line change
@@ -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,12 +244,6 @@ export class ASTDefinitionBuilder {
);
}

buildTypes(
nodes: $ReadOnlyArray<NamedTypeNode | TypeDefinitionNode>,
): Array<GraphQLNamedType> {
return nodes.map(node => this.buildType(node));
}

buildType(node: NamedTypeNode | TypeDefinitionNode): GraphQLNamedType {
const typeName = node.name.value;
if (!this._cache[typeName]) {
@@ -293,8 +260,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,16 +341,17 @@ export class ASTDefinitionBuilder {
}

_makeTypeDef(def: ObjectTypeDefinitionNode) {
const typeName = def.name.value;
const interfaces = def.interfaces;
const interfaces: ?$ReadOnlyArray<NamedTypeNode> = 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
// 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,
});
}
@@ -429,13 +405,14 @@ export class ASTDefinitionBuilder {
}

_makeUnionDef(def: UnionTypeDefinitionNode) {
const types: ?$ReadOnlyArray<NamedTypeNode> = 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,
});
}
32 changes: 17 additions & 15 deletions src/utilities/extendSchema.js
Original file line number Diff line number Diff line change
@@ -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.
@@ -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);
@@ -497,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<GraphQLObjectType> {
const possibleTypes = type.getTypes().map(extendNamedType);

// If there are any extensions to the union, apply those here.
const extensions = typeExtensionsMap[type.name];
@@ -507,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(