Repository: graphql/graphql-js
Pull request #3453 (2022-01-10 17:04:21)

fix(validate): no reusing root types
Hello `graphql-js` folks 👋 While playing with schema and SDL validation, I noticed that the reference implementation does not follow the following rule from the [latest spec](https://spec.graphql.org/draft/#sec-Root-Operation-Types):

> The query, mutation, and subscription root types must all be different types if provided.

Not sure if that's an error in the spec or in the reference implementation 🤔 I went ahead and proposed a fix in here, but happy to open a PR on the spec instead if that's the preferred. This is after all a breaking change, even though I doubt that it would have a big impact. (At least I've never seen a schema that does this.)

linux-foundation-easycla[bot]: <a href="https://api.easycla.lfx.linuxfoundation.org/v2/repository-provider/github/sign/12261526/38307428/3453/#/?version=2"><img src="https://s3.amazonaws.com/cla-project-logo-prod/cla-not-signed.svg" alt="CLA Not Signed" align="left" height="28" width="328" ></a><br/><br /><ul><li><a href='https://api.easycla.lfx.linuxfoundation.org/v2/repository-provider/github/sign/12261526/38307428/3453/#/?version=2' target='_blank'>:x:</a> - Thomas Heyenbrock The commit (1aa3ed7d649097600e7e12cecbcc6e097e789b81) is not authorized under a signed CLA. <a href='https://api.easycla.lfx.linuxfoundation.org/v2/repository-provider/github/sign/12261526/38307428/3453/#/?version=2' target='_blank'>Please click here to be authorized</a>. For further assistance with EasyCLA, <a href='https://jira.linuxfoundation.org/servicedesk/customer/portal/4' target='_blank'>please submit a support request ticket</a>.</li></ul>
netlify[bot]: ### <span aria-hidden="true">👷</span> Deploy Preview for *compassionate-pike-271cb3* processing.


|  Name | Link |
|---------------------------------|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | d5ba5046d87010f2b39d4801ff732bbf201a0cf2 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/6278f3cc257b7f0008b8217a |
IvanGoncharov: @thomasheyenbrock Thanks for PR 👍 
I applied review changes in 095517abee2751c04ba04d2baee9e2cc5504821a

Patch:
diff --git a/src/jsutils/didYouMean.ts b/src/jsutils/didYouMean.ts
index 33e10a42..ef1931c7 100644
--- a/src/jsutils/didYouMean.ts
+++ b/src/jsutils/didYouMean.ts
@@ -1,3 +1,5 @@
+import { orList } from './formatList';
+
 const MAX_SUGGESTIONS = 5;
 
 /**
@@ -12,26 +14,21 @@ export function didYouMean(
   firstArg: string | ReadonlyArray<string>,
   secondArg?: ReadonlyArray<string>,
 ) {
-  const [subMessage, suggestionsArg] = secondArg
+  const [subMessage, suggestions] = secondArg
     ? [firstArg as string, secondArg]
     : [undefined, firstArg as ReadonlyArray<string>];
 
+  if (suggestions.length === 0) {
+    return '';
+  }
+
   let message = ' Did you mean ';
   if (subMessage) {
     message += subMessage + ' ';
   }
 
-  const suggestions = suggestionsArg.map((x) => `"${x}"`);
-  switch (suggestions.length) {
-    case 0:
-      return '';
-    case 1:
-      return message + suggestions[0] + '?';
-    case 2:
-      return message + suggestions[0] + ' or ' + suggestions[1] + '?';
-  }
-
-  const selected = suggestions.slice(0, MAX_SUGGESTIONS);
-  const lastItem = selected.pop();
-  return message + selected.join(', ') + ', or ' + lastItem + '?';
+  const suggestionList = orList(
+    suggestions.slice(0, MAX_SUGGESTIONS).map((x) => `"${x}"`),
+  );
+  return message + suggestionList + '?';
 }
diff --git a/src/jsutils/formatList.ts b/src/jsutils/formatList.ts
new file mode 100644
index 00000000..fe3afe53
--- /dev/null
+++ b/src/jsutils/formatList.ts
@@ -0,0 +1,30 @@
+import { invariant } from './invariant';
+
+/**
+ * Given [ A, B, C ] return 'A, B, or C'.
+ */
+export function orList(items: ReadonlyArray<string>): string {
+  return formatList('or', items);
+}
+
+/**
+ * Given [ A, B, C ] return 'A, B, and C'.
+ */
+export function andList(items: ReadonlyArray<string>): string {
+  return formatList('and', items);
+}
+
+function formatList(conjunction: string, items: ReadonlyArray<string>): string {
+  invariant(items.length !== 0);
+
+  switch (items.length) {
+    case 1:
+      return items[0];
+    case 2:
+      return items[0] + ' ' + conjunction + ' ' + items[1];
+  }
+
+  const allButLast = items.slice(0, -1);
+  const lastItem = items[items.length - 1];
+  return allButLast.join(', ') + ', ' + conjunction + ' ' + lastItem;
+}
diff --git a/src/type/validate.ts b/src/type/validate.ts
index 9ce66543..c6385d27 100644
--- a/src/type/validate.ts
+++ b/src/type/validate.ts
@@ -1,4 +1,6 @@
+import { AccumulatorMap } from '../jsutils/AccumulatorMap';
 import { capitalize } from '../jsutils/capitalize';
+import { andList } from '../jsutils/formatList';
 import { inspect } from '../jsutils/inspect';
 import type { Maybe } from '../jsutils/Maybe';
 
@@ -118,18 +120,38 @@ function validateRootTypes(context: SchemaValidationContext): void {
     context.reportError('Query root type must be provided.', schema.astNode);
   }
 
+  const rootTypesMap = new AccumulatorMap<
+    GraphQLObjectType,
+    OperationTypeNode
+  >();
   for (const operationType of Object.values(OperationTypeNode)) {
     const rootType = schema.getRootType(operationType);
 
-    if (rootType != null && !isObjectType(rootType)) {
-      const operationTypeStr = capitalize(operationType);
-      const rootTypeStr = inspect(rootType);
+    if (rootType != null) {
+      if (!isObjectType(rootType)) {
+        const operationTypeStr = capitalize(operationType);
+        const rootTypeStr = inspect(rootType);
+        context.reportError(
+          operationType === OperationTypeNode.QUERY
+            ? `${operationTypeStr} root type must be Object type, it cannot be ${rootTypeStr}.`
+            : `${operationTypeStr} root type must be Object type if provided, it cannot be ${rootTypeStr}.`,
+          getOperationTypeNode(schema, operationType) ??
+            (rootType as any).astNode,
+        );
+      } else {
+        rootTypesMap.add(rootType, operationType);
+      }
+    }
+  }
+
+  for (const [rootType, operationTypes] of rootTypesMap.entries()) {
+    if (operationTypes.length > 1) {
+      const operationList = andList(operationTypes);
       context.reportError(
-        operationType === OperationTypeNode.QUERY
-          ? `${operationTypeStr} root type must be Object type, it cannot be ${rootTypeStr}.`
-          : `${operationTypeStr} root type must be Object type if provided, it cannot be ${rootTypeStr}.`,
-        getOperationTypeNode(schema, operationType) ??
-          (rootType as any).astNode,
+        `All root types must be different, "${rootType.name}" type is used as ${operationList} root types.`,
+        operationTypes.map((operationType) =>
+          getOperationTypeNode(schema, operationType),
+        ),
       );
     }
   }


Test patch:
diff --git a/src/type/__tests__/validation-test.ts b/src/type/__tests__/validation-test.ts
index 921ea87c..af82d30f 100644
--- a/src/type/__tests__/validation-test.ts
+++ b/src/type/__tests__/validation-test.ts
@@ -433,6 +433,86 @@ describe('Type System: A Schema must have Object root types', () => {
   });
 });
 
+describe('Type System: Root types must all be different if provided', () => {
+  it('accepts a Schema with different root types', () => {
+    const schema = buildSchema(`
+      type SomeObject1 {
+        field: String
+      }
+
+      type SomeObject2 {
+        field: String
+      }
+
+      type SomeObject3 {
+        field: String
+      }
+
+      schema {
+        query: SomeObject1
+        mutation: SomeObject2
+        subscription: SomeObject3
+      }
+    `);
+    expectJSON(validateSchema(schema)).toDeepEqual([]);
+  });
+
+  it('rejects a Schema where the same type is used for multiple root types', () => {
+    const schema = buildSchema(`
+      type SomeObject {
+        field: String
+      }
+
+      type UniqueObject {
+        field: String
+      }
+
+      schema {
+        query: SomeObject
+        mutation: UniqueObject
+        subscription: SomeObject
+      }
+    `);
+
+    expectJSON(validateSchema(schema)).toDeepEqual([
+      {
+        message:
+          'All root types must be different, "SomeObject" type is used as query and subscription root types.',
+        locations: [
+          { line: 11, column: 16 },
+          { line: 13, column: 23 },
+        ],
+      },
+    ]);
+  });
+
+  it('rejects a Schema where the same type is used for all root types', () => {
+    const schema = buildSchema(`
+      type SomeObject {
+        field: String
+      }
+
+      schema {
+        query: SomeObject
+        mutation: SomeObject
+        subscription: SomeObject
+      }
+    `);
+
+    expectJSON(validateSchema(schema)).toDeepEqual([
+      {
+        message:
+          'All root types must be different, "SomeObject" type is used as query, mutation, and subscription root types.',
+        locations: [
+          { line: 7, column: 16 },
+          { line: 8, column: 19 },
+          { line: 9, column: 23 },
+        ],
+      },
+    ]);
+  });
+});
+
 describe('Type System: Objects must have fields', () => {
   it('accepts an Object type with fields object', () => {
     const schema = buildSchema(`