OverlappingFieldsCanBeMerged: sort argument values before comparing (#3455)

Author: IvanGoncharov

src/utilities/__tests__/sortValueNode-test.ts

@@ -0,0 +1,40 @@
+import { expect } from 'chai';
+import { describe, it } from 'mocha';
+
+import { parseValue } from '../../language/parser';
+import { print } from '../../language/printer';
+
+import { sortValueNode } from '../sortValueNode';
+
+describe('sortValueNode', () => {
+  function expectSortedValue(source: string) {
+    return expect(print(sortValueNode(parseValue(source))));
+  }
+
+  it('do not change non-object values', () => {
+    expectSortedValue('1').to.equal('1');
+    expectSortedValue('3.14').to.equal('3.14');
+    expectSortedValue('null').to.equal('null');
+    expectSortedValue('true').to.equal('true');
+    expectSortedValue('false').to.equal('false');
+    expectSortedValue('"cba"').to.equal('"cba"');
+    expectSortedValue('"""cba"""').to.equal('"""cba"""');
+    expectSortedValue('[1, 3.14, null, false, "cba"]').to.equal(
+      '[1, 3.14, null, false, "cba"]',
+    );
+    expectSortedValue('[[1, 3.14, null, false, "cba"]]').to.equal(
+      '[[1, 3.14, null, false, "cba"]]',
+    );
+  });
+
+  it('sort input object fields', () => {
+    expectSortedValue('{ b: 2, a: 1 }').to.equal('{a: 1, b: 2}');
+    expectSortedValue('{ a: { c: 3, b: 2 } }').to.equal('{a: {b: 2, c: 3}}');
+    expectSortedValue('[{ b: 2, a: 1 }, { d: 4, c: 3}]').to.equal(
+      '[{a: 1, b: 2}, {c: 3, d: 4}]',
+    );
+    expectSortedValue(
+      '{ b: { g: 7, f: 6 }, c: 3 , a: { d: 4, e: 5 } }',
+    ).to.equal('{a: {d: 4, e: 5}, b: {f: 6, g: 7}, c: 3}');
+  });
+});

src/utilities/findBreakingChanges.ts

@@ -1,10 +1,8 @@
 import { inspect } from '../jsutils/inspect';
 import { invariant } from '../jsutils/invariant';
 import { keyMap } from '../jsutils/keyMap';
-import { naturalCompare } from '../jsutils/naturalCompare';
 
 import { print } from '../language/printer';
-import { visit } from '../language/visitor';
 
 import type {
   GraphQLEnumType,
@@ -34,6 +32,7 @@ import { isSpecifiedScalarType } from '../type/scalars';
 import type { GraphQLSchema } from '../type/schema';
 
 import { astFromValue } from './astFromValue';
+import { sortValueNode } from './sortValueNode';
 
 export enum BreakingChangeType {
   TYPE_REMOVED = 'TYPE_REMOVED',
@@ -536,20 +535,7 @@ function typeKindName(type: GraphQLNamedType): string {
 function stringifyValue(value: unknown, type: GraphQLInputType): string {
   const ast = astFromValue(value, type);
   invariant(ast != null);
-
-  const sortedAST = visit(ast, {
-    ObjectValue(objectNode) {
-      // Make a copy since sort mutates array
-      const fields = [...objectNode.fields];
-
-      fields.sort((fieldA, fieldB) =>
-        naturalCompare(fieldA.name.value, fieldB.name.value),
-      );
-      return { ...objectNode, fields };
-    },
-  });
-
-  return print(sortedAST);
+  return print(sortValueNode(ast));
 }
 
 function diff<T extends { name: string }>(

src/utilities/sortValueNode.ts

@@ -0,0 +1,47 @@
+import { naturalCompare } from '../jsutils/naturalCompare';
+
+import type { ObjectFieldNode, ValueNode } from '../language/ast';
+import { Kind } from '../language/kinds';
+
+/**
+ * Sort ValueNode.
+ *
+ * This function returns a sorted copy of the given ValueNode.
+ *
+ * @internal
+ */
+export function sortValueNode(valueNode: ValueNode): ValueNode {
+  switch (valueNode.kind) {
+    case Kind.OBJECT:
+      return {
+        ...valueNode,
+        fields: sortFields(valueNode.fields),
+      };
+    case Kind.LIST:
+      return {
+        ...valueNode,
+        values: valueNode.values.map(sortValueNode),
+      };
+    case Kind.INT:
+    case Kind.FLOAT:
+    case Kind.STRING:
+    case Kind.BOOLEAN:
+    case Kind.NULL:
+    case Kind.ENUM:
+    case Kind.VARIABLE:
+      return valueNode;
+  }
+}
+
+function sortFields(
+  fields: ReadonlyArray<ObjectFieldNode>,
+): Array<ObjectFieldNode> {
+  return fields
+    .map((fieldNode) => ({
+      ...fieldNode,
+      value: sortValueNode(fieldNode.value),
+    }))
+    .sort((fieldA, fieldB) =>
+      naturalCompare(fieldA.name.value, fieldB.name.value),
+    );
+}

src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts

@@ -236,6 +236,51 @@ describe('Validate: Overlapping fields can be merged', () => {
     `);
   });
 
+  it('allows different order of args', () => {
+    const schema = buildSchema(`
+      type Query {
+        someField(a: String, b: String): String
+      }
+    `);
+
+    // This is valid since arguments are unordered, see:
+    // https://spec.graphql.org/draft/#sec-Language.Arguments.Arguments-are-unordered
+    expectValidWithSchema(
+      schema,
+      `
+        {
+          someField(a: null, b: null)
+          someField(b: null, a: null)
+        }
+      `,
+    );
+  });
+
+  it('allows different order of input object fields in arg values', () => {
+    const schema = buildSchema(`
+      input SomeInput {
+        a: String
+        b: String
+      }
+
+      type Query {
+        someField(arg: SomeInput): String
+      }
+    `);
+
+    // This is valid since input object fields are unordered, see:
+    // https://spec.graphql.org/draft/#sec-Input-Object-Values.Input-object-fields-are-unordered
+    expectValidWithSchema(
+      schema,
+      `
+        {
+          someField(arg: { a: null, b: null })
+          someField(arg: { b: null, a: null })
+        }
+      `,
+    );
+  });
+
   it('encounters conflict in fragments', () => {
     expectErrors(`
       {

src/validation/rules/OverlappingFieldsCanBeMergedRule.ts

@@ -29,6 +29,7 @@ import {
   isObjectType,
 } from '../../type/definition';
 
+import { sortValueNode } from '../../utilities/sortValueNode';
 import { typeFromAST } from '../../utilities/typeFromAST';
 
 import type { ValidationContext } from '../ValidationContext';
@@ -652,12 +653,12 @@ function sameArguments(
     if (!argument2) {
       return false;
     }
-    return sameValue(argument1.value, argument2.value);
+    return stringifyValue(argument1.value) === stringifyValue(argument2.value);
   });
 }
 
-function sameValue(value1: ValueNode, value2: ValueNode): boolean {
-  return print(value1) === print(value2);
+function stringifyValue(value: ValueNode): string {
+  return print(sortValueNode(value));
 }
 
 // Two types conflict if both types could not apply to a value simultaneously.