From d390b943122b3d874ec2b5db06b4fa4fcc10cf01 Mon Sep 17 00:00:00 2001
From: Christoph Zwerschke <cito@online.de>
Date: Wed, 4 Mar 2020 14:58:40 +0100
Subject: [PATCH] Improve error reporting for union member types

First make sure that the included type is an object type,
because that would be the more severe problem. Only then we
can be sure that the type has a name and check for duplicates.
Also, create only one error per different non-object type.
---
 src/type/__tests__/validation-test.ts | 43 ++++++++++++++++++++++++++-
 src/type/validate.ts                  | 20 ++++++++-----
 2 files changed, 55 insertions(+), 8 deletions(-)

diff --git a/src/type/__tests__/validation-test.ts b/src/type/__tests__/validation-test.ts
index 3c2fe21500..ca07a220d6 100644
--- a/src/type/__tests__/validation-test.ts
+++ b/src/type/__tests__/validation-test.ts
@@ -747,7 +747,7 @@ describe('Type System: Union types must be valid', () => {
     ]);
   });
 
-  it('rejects a Union type with non-Object members types', () => {
+  it('rejects a Union type with non-Object member types', () => {
     let schema = buildSchema(`
       type Query {
         test: BadUnion
@@ -807,6 +807,47 @@ describe('Type System: Union types must be valid', () => {
       ]);
     }
   });
+
+  it('rejects a Union type with duplicate non-Object member types', () => {
+    let schema = buildSchema(`
+      type Query {
+        test: BadUnion
+      }
+
+      type TypeA {
+        field: String
+      }
+
+      union BadUnion =
+        | Int
+        | String
+        | TypeA
+        | Int
+        | String
+    `);
+
+    schema = extendSchema(schema, parse('extend union BadUnion = Int'));
+
+    expectJSON(validateSchema(schema)).toDeepEqual([
+      {
+        message:
+          'Union type BadUnion can only include Object types, it cannot include Int.',
+        locations: [
+          { line: 11, column: 11 },
+          { line: 14, column: 11 },
+          { line: 1, column: 25 },
+        ],
+      },
+      {
+        message:
+          'Union type BadUnion can only include Object types, it cannot include String.',
+        locations: [
+          { line: 12, column: 11 },
+          { line: 15, column: 11 },
+        ],
+      },
+    ]);
+  });
 });
 
 describe('Type System: Input Objects must have fields', () => {
diff --git a/src/type/validate.ts b/src/type/validate.ts
index 3a69a8c6eb..b4e1568beb 100644
--- a/src/type/validate.ts
+++ b/src/type/validate.ts
@@ -480,7 +480,20 @@ function validateUnionMembers(
   }
 
   const includedTypeNames = new Set<string>();
+  const includedInvalidTypes = new Set<string>();
   for (const memberType of memberTypes) {
+    if (!isObjectType(memberType)) {
+      const typeString = inspect(memberType);
+      if (!includedInvalidTypes.has(typeString)) {
+        context.reportError(
+          `Union type ${union.name} can only include Object types, ` +
+            `it cannot include ${inspect(memberType)}.`,
+          getUnionMemberTypeNodes(union, String(memberType)),
+        );
+        includedInvalidTypes.add(typeString);
+      }
+      continue;
+    }
     if (includedTypeNames.has(memberType.name)) {
       context.reportError(
         `Union type ${union.name} can only include type ${memberType} once.`,
@@ -489,13 +502,6 @@ function validateUnionMembers(
       continue;
     }
     includedTypeNames.add(memberType.name);
-    if (!isObjectType(memberType)) {
-      context.reportError(
-        `Union type ${union.name} can only include Object types, ` +
-          `it cannot include ${inspect(memberType)}.`,
-        getUnionMemberTypeNodes(union, String(memberType)),
-      );
-    }
   }
 }