Skip to content

Commit 5abe152

Browse files
IvanGoncharovmjmahone
authored andcommitted
Prevent infinite recursion on invalid unions (#1427)
* Small refactoring * buildSchema: Fix infinite loop with recursive union * extendSchema: Fix infinite loop with recursive union
1 parent 77aa6a9 commit 5abe152

File tree

4 files changed

+60
-56
lines changed

4 files changed

+60
-56
lines changed

src/utilities/__tests__/buildASTSchema-test.js

+12
Original file line numberDiff line numberDiff line change
@@ -356,6 +356,18 @@ describe('Schema Builder', () => {
356356
expect(output).to.equal(body);
357357
});
358358

359+
it('Can build recursive Union', () => {
360+
const schema = buildSchema(dedent`
361+
union Hello = Hello
362+
363+
type Query {
364+
hello: Hello
365+
}
366+
`);
367+
const errors = validateSchema(schema);
368+
expect(errors.length).to.be.above(0);
369+
});
370+
359371
it('Specifying Union type using __typename', () => {
360372
const schema = buildSchema(dedent`
361373
type Query {

src/utilities/__tests__/extendSchema-test.js

+13
Original file line numberDiff line numberDiff line change
@@ -280,6 +280,19 @@ describe('extendSchema', () => {
280280
expect(unionField.type).to.equal(someUnionType);
281281
});
282282

283+
it('allows extension of union by adding itself', () => {
284+
const extendedSchema = extendTestSchema(`
285+
extend union SomeUnion = SomeUnion
286+
`);
287+
288+
const errors = validateSchema(extendedSchema);
289+
expect(errors.length).to.be.above(0);
290+
291+
expect(printTestSchemaChanges(extendedSchema)).to.equal(dedent`
292+
union SomeUnion = Foo | Biz | SomeUnion
293+
`);
294+
});
295+
283296
it('extends inputs by adding new fields', () => {
284297
const extendedSchema = extendTestSchema(`
285298
extend input SomeInput {

src/utilities/buildASTSchema.js

+18-41
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,6 @@ import type {
5050
} from '../type/definition';
5151

5252
import {
53-
assertNullableType,
5453
GraphQLScalarType,
5554
GraphQLObjectType,
5655
GraphQLInterfaceType,
@@ -89,31 +88,6 @@ export type BuildSchemaOptions = {
8988
commentDescriptions?: boolean,
9089
};
9190

92-
function buildWrappedType(
93-
innerType: GraphQLType,
94-
inputTypeNode: TypeNode,
95-
): GraphQLType {
96-
if (inputTypeNode.kind === Kind.LIST_TYPE) {
97-
return GraphQLList(buildWrappedType(innerType, inputTypeNode.type));
98-
}
99-
if (inputTypeNode.kind === Kind.NON_NULL_TYPE) {
100-
const wrappedType = buildWrappedType(innerType, inputTypeNode.type);
101-
return GraphQLNonNull(assertNullableType(wrappedType));
102-
}
103-
return innerType;
104-
}
105-
106-
function getNamedTypeNode(typeNode: TypeNode): NamedTypeNode {
107-
let namedType = typeNode;
108-
while (
109-
namedType.kind === Kind.LIST_TYPE ||
110-
namedType.kind === Kind.NON_NULL_TYPE
111-
) {
112-
namedType = namedType.type;
113-
}
114-
return namedType;
115-
}
116-
11791
/**
11892
* This takes the ast of a schema document produced by the parse function in
11993
* src/language/parser.js.
@@ -187,7 +161,6 @@ export function buildASTSchema(
187161
},
188162
);
189163

190-
const types = definitionBuilder.buildTypes(typeDefs);
191164
const directives = directiveDefs.map(def =>
192165
definitionBuilder.buildDirective(def),
193166
);
@@ -218,7 +191,7 @@ export function buildASTSchema(
218191
subscription: operationTypes.subscription
219192
? (definitionBuilder.buildType(operationTypes.subscription): any)
220193
: null,
221-
types,
194+
types: typeDefs.map(node => definitionBuilder.buildType(node)),
222195
directives,
223196
astNode: schemaDef,
224197
assumeValid: options && options.assumeValid,
@@ -268,12 +241,6 @@ export class ASTDefinitionBuilder {
268241
);
269242
}
270243

271-
buildTypes(
272-
nodes: $ReadOnlyArray<NamedTypeNode | TypeDefinitionNode>,
273-
): Array<GraphQLNamedType> {
274-
return nodes.map(node => this.buildType(node));
275-
}
276-
277244
buildType(node: NamedTypeNode | TypeDefinitionNode): GraphQLNamedType {
278245
const typeName = node.name.value;
279246
if (!this._cache[typeName]) {
@@ -290,8 +257,16 @@ export class ASTDefinitionBuilder {
290257
}
291258

292259
_buildWrappedType(typeNode: TypeNode): GraphQLType {
293-
const typeDef = this.buildType(getNamedTypeNode(typeNode));
294-
return buildWrappedType(typeDef, typeNode);
260+
if (typeNode.kind === Kind.LIST_TYPE) {
261+
return GraphQLList(this._buildWrappedType(typeNode.type));
262+
}
263+
if (typeNode.kind === Kind.NON_NULL_TYPE) {
264+
return GraphQLNonNull(
265+
// Note: GraphQLNonNull constructor validates this type
266+
(this._buildWrappedType(typeNode.type): any),
267+
);
268+
}
269+
return this.buildType(typeNode);
295270
}
296271

297272
buildDirective(directiveNode: DirectiveDefinitionNode): GraphQLDirective {
@@ -363,16 +338,17 @@ export class ASTDefinitionBuilder {
363338
}
364339

365340
_makeTypeDef(def: ObjectTypeDefinitionNode) {
366-
const typeName = def.name.value;
367-
const interfaces = def.interfaces;
341+
const interfaces: ?$ReadOnlyArray<NamedTypeNode> = def.interfaces;
368342
return new GraphQLObjectType({
369-
name: typeName,
343+
name: def.name.value,
370344
description: getDescription(def, this._options),
371345
fields: () => this._makeFieldDefMap(def),
372346
// Note: While this could make early assertions to get the correctly
373347
// typed values, that would throw immediately while type system
374348
// validation with validateSchema() will produce more actionable results.
375-
interfaces: interfaces ? () => (this.buildTypes(interfaces): any) : [],
349+
interfaces: interfaces
350+
? () => interfaces.map(ref => (this.buildType(ref): any))
351+
: [],
376352
astNode: def,
377353
});
378354
}
@@ -426,13 +402,14 @@ export class ASTDefinitionBuilder {
426402
}
427403

428404
_makeUnionDef(def: UnionTypeDefinitionNode) {
405+
const types: ?$ReadOnlyArray<NamedTypeNode> = def.types;
429406
return new GraphQLUnionType({
430407
name: def.name.value,
431408
description: getDescription(def, this._options),
432409
// Note: While this could make assertions to get the correctly typed
433410
// values below, that would throw immediately while type system
434411
// validation with validateSchema() will produce more actionable results.
435-
types: def.types ? (this.buildTypes(def.types): any) : [],
412+
types: types ? () => types.map(ref => (this.buildType(ref): any)) : [],
436413
astNode: def,
437414
});
438415
}

src/utilities/extendSchema.js

+17-15
Original file line numberDiff line numberDiff line change
@@ -244,7 +244,7 @@ export function extendSchema(
244244
// that any type not directly referenced by a field will get created.
245245
...objectValues(schema.getTypeMap()).map(type => extendNamedType(type)),
246246
// Do the same with new types.
247-
...astBuilder.buildTypes(objectValues(typeDefinitionMap)),
247+
...objectValues(typeDefinitionMap).map(type => astBuilder.buildType(type)),
248248
];
249249

250250
// Support both original legacy names and extended legacy names.
@@ -300,9 +300,6 @@ export function extendSchema(
300300
extendTypeCache[name] = extendEnumType(type);
301301
} else if (isInputObjectType(type)) {
302302
extendTypeCache[name] = extendInputObjectType(type);
303-
} else {
304-
// This type is not yet extendable.
305-
extendTypeCache[name] = type;
306303
}
307304
}
308305
return (extendTypeCache[name]: any);
@@ -498,7 +495,20 @@ export function extendSchema(
498495
? type.extensionASTNodes.concat(typeExtensionsMap[name])
499496
: typeExtensionsMap[name]
500497
: type.extensionASTNodes;
501-
const unionTypes = type.getTypes().map(extendNamedType);
498+
return new GraphQLUnionType({
499+
name,
500+
description: type.description,
501+
types: () => extendPossibleTypes(type),
502+
astNode: type.astNode,
503+
resolveType: type.resolveType,
504+
extensionASTNodes,
505+
});
506+
}
507+
508+
function extendPossibleTypes(
509+
type: GraphQLUnionType,
510+
): Array<GraphQLObjectType> {
511+
const possibleTypes = type.getTypes().map(extendNamedType);
502512

503513
// If there are any extensions to the union, apply those here.
504514
const extensions = typeExtensionsMap[type.name];
@@ -508,19 +518,11 @@ export function extendSchema(
508518
// Note: While this could make early assertions to get the correctly
509519
// typed values, that would throw immediately while type system
510520
// validation with validateSchema() will produce more actionable results.
511-
unionTypes.push((astBuilder.buildType(namedType): any));
521+
possibleTypes.push((astBuilder.buildType(namedType): any));
512522
}
513523
}
514524
}
515-
516-
return new GraphQLUnionType({
517-
name,
518-
description: type.description,
519-
types: unionTypes,
520-
astNode: type.astNode,
521-
resolveType: type.resolveType,
522-
extensionASTNodes,
523-
});
525+
return possibleTypes;
524526
}
525527

526528
function extendImplementedInterfaces(

0 commit comments

Comments
 (0)