Prevent infinite recursion on invalid unions (#1427)

* Small refactoring * buildSchema: Fix infinite loop with recursive union * extendSchema: Fix infinite loop with recursive union
graphql · Jul 26, 2018 · 5abe152 · 5abe152
1 parent 77aa6a9
commit 5abe152
Show file tree

Hide file tree

Showing 4 changed files with 60 additions and 56 deletions.
diff --git 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/__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/buildASTSchema.js b/src/utilities/buildASTSchema.js
@@ -50,7 +50,6 @@ import type {
 } from '../type/definition';
 
 import {
-  assertNullableType,
   GraphQLScalarType,
   GraphQLObjectType,
   GraphQLInterfaceType,
@@ -89,31 +88,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.
@@ -187,7 +161,6 @@ export function buildASTSchema(
     },
   );
 
-  const types = definitionBuilder.buildTypes(typeDefs);
   const directives = directiveDefs.map(def =>
     definitionBuilder.buildDirective(def),
   );
@@ -218,7 +191,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,
@@ -268,12 +241,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]) {
@@ -290,8 +257,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 {
@@ -363,16 +338,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,
     });
   }
@@ -426,13 +402,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,
     });
   }

diff --git a/src/utilities/extendSchema.js b/src/utilities/extendSchema.js
@@ -244,7 +244,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.
@@ -300,9 +300,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);
@@ -498,7 +495,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];
@@ -508,19 +518,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(