OverlappingFieldsCanBeMergedRule: Fix performance degradation (#3958)

Co-authored-by: AaronMoat <[email protected]> Co-authored-by: Ivan Goncharov <[email protected]> Resolves #3955 (at least
graphql · Sep 5, 2023 · f94b511 · f94b511
1 parent d32b99d
commit f94b511
Show file tree

Hide file tree

Showing 3 changed files with 66 additions and 16 deletions.
diff --git a/benchmark/repeated-fields-benchmark.js b/benchmark/repeated-fields-benchmark.js
@@ -0,0 +1,13 @@
+import { graphqlSync } from 'graphql/graphql.js';
+import { buildSchema } from 'graphql/utilities/buildASTSchema.js';
+
+const schema = buildSchema('type Query { hello: String! }');
+const source = `{ ${'hello '.repeat(250)}}`;
+
+export const benchmark = {
+  name: 'Many repeated fields',
+  count: 5,
+  measure() {
+    graphqlSync({ schema, source });
+  },
+};
diff --git a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts
@@ -179,6 +179,24 @@ describe('Validate: Overlapping fields can be merged', () => {
     ]);
   });
 
+  it('different stream directive extra argument', () => {
+    expectErrors(`
+      fragment conflictingArgs on Dog {
+        name @stream(label: "streamLabel", initialCount: 1)
+        name @stream(label: "streamLabel", initialCount: 1, extraArg: true)
+      }
+    `).toDeepEqual([
+      {
+        message:
+          'Fields "name" conflict because they have differing stream directives. Use different aliases on the fields to fetch both if this was intentional.',
+        locations: [
+          { line: 3, column: 9 },
+          { line: 4, column: 9 },
+        ],
+      },
+    ]);
+  });
+
   it('mix of stream and no stream', () => {
     expectErrors(`
       fragment conflictingArgs on Dog {

diff --git a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts
@@ -7,8 +7,8 @@ import type {
   DirectiveNode,
   FieldNode,
   FragmentDefinitionNode,
-  ObjectValueNode,
   SelectionSetNode,
+  ValueNode,
 } from '../../language/ast.js';
 import { Kind } from '../../language/kinds.js';
 import { print } from '../../language/printer.js';
@@ -592,7 +592,7 @@ function findConflict(
     }
 
     // Two field calls must have the same arguments.
-    if (stringifyArguments(node1) !== stringifyArguments(node2)) {
+    if (!sameArguments(node1, node2)) {
       return [
         [responseName, 'they have differing arguments'],
         [node1],
@@ -649,19 +649,38 @@ function findConflict(
   }
 }
 
-function stringifyArguments(fieldNode: FieldNode | DirectiveNode): string {
-  // FIXME https://github.com/graphql/graphql-js/issues/2203
-  const args = /* c8 ignore next */ fieldNode.arguments ?? [];
-
-  const inputObjectWithArgs: ObjectValueNode = {
-    kind: Kind.OBJECT,
-    fields: args.map((argNode) => ({
-      kind: Kind.OBJECT_FIELD,
-      name: argNode.name,
-      value: argNode.value,
-    })),
-  };
-  return print(sortValueNode(inputObjectWithArgs));
+function sameArguments(
+  node1: FieldNode | DirectiveNode,
+  node2: FieldNode | DirectiveNode,
+): boolean {
+  const args1 = node1.arguments;
+  const args2 = node2.arguments;
+
+  if (args1 === undefined || args1.length === 0) {
+    return args2 === undefined || args2.length === 0;
+  }
+  if (args2 === undefined || args2.length === 0) {
+    return false;
+  }
+
+  if (args1.length !== args2.length) {
+    return false;
+  }
+
+  const values2 = new Map(args2.map(({ name, value }) => [name.value, value]));
+  return args1.every((arg1) => {
+    const value1 = arg1.value;
+    const value2 = values2.get(arg1.name.value);
+    if (value2 === undefined) {
+      return false;
+    }
+
+    return stringifyValue(value1) === stringifyValue(value2);
+  });
+}
+
+function stringifyValue(value: ValueNode): string | null {
+  return print(sortValueNode(value));
 }
 
 function getStreamDirective(
@@ -681,7 +700,7 @@ function sameStreams(
     return true;
   } else if (stream1 && stream2) {
     // check if both fields have equivalent streams
-    return stringifyArguments(stream1) === stringifyArguments(stream2);
+    return sameArguments(stream1, stream2);
   }
   // fields have a mix of stream and no stream
   return false;