Repository: graphql/graphql-js
Pull request #3454 (2022-01-11 13:30:51)

feat: allow providing an object to the GraphQLError constructor
When using the `GraphQL` error within my schema code I usually only use the `message` and `extensions` parameters. However, this requires passing 4 undefined values to the constructor.

```ts
throw new GraphQLError("Oops.", undefined, undefined, undefined, undefined, { code: "SERVER_GOES_BRRT" })
```

This API allows writing this code, which feels less verbose to me.

```ts
throw new GraphQLError({
  message: "Oops.",
  extensions: { code: "SERVER_GOES_BRRT" },
})
```

Given that the `execute` and `subscribe` function signatures also evolved into a single object, this change feels natural to me. Please let me know what you think.

An alternative API might be the following, where the message is ALWAYS the first argument.

```ts
throw new GraphQLError("Oops.", {
  extensions: { code: "SERVER_GOES_BRRT" },
})
```


Patch:
diff --git a/src/error/GraphQLError.ts b/src/error/GraphQLError.ts
index 6d6677fa..c859f59e 100644
--- a/src/error/GraphQLError.ts
+++ b/src/error/GraphQLError.ts
@@ -20,6 +20,41 @@ export interface GraphQLErrorExtensions {
   [attributeName: string]: unknown;
 }
 
+export interface GraphQLErrorArgs {
+  nodes?: ReadonlyArray<ASTNode> | ASTNode | null;
+  source?: Maybe<Source>;
+  positions?: Maybe<ReadonlyArray<number>>;
+  path?: Maybe<ReadonlyArray<string | number>>;
+  originalError?: Maybe<Error & { readonly extensions?: unknown }>;
+  extensions?: Maybe<GraphQLErrorExtensions>;
+}
+
+type BackwardsCompatibleArgs =
+  | [args?: GraphQLErrorArgs]
+  | [
+      nodes?: GraphQLErrorArgs['nodes'],
+      source?: GraphQLErrorArgs['source'],
+      positions?: GraphQLErrorArgs['positions'],
+      path?: GraphQLErrorArgs['path'],
+      originalError?: GraphQLErrorArgs['originalError'],
+      extensions?: GraphQLErrorArgs['extensions'],
+    ];
+
+function toNormalizedArgs(args: BackwardsCompatibleArgs): GraphQLErrorArgs {
+  const firstArg = args[0];
+  if (firstArg == null || 'kind' in firstArg || 'length' in firstArg) {
+    return {
+      nodes: firstArg,
+      source: args[1],
+      positions: args[2],
+      path: args[3],
+      originalError: args[4],
+      extensions: args[5],
+    };
+  }
+  return firstArg;
+}
+
 /**
  * A GraphQLError describes an Error found during the parse, validate, or
  * execute phases of performing a GraphQL operation. In addition to a message
@@ -76,6 +111,9 @@ export class GraphQLError extends Error {
    */
   readonly extensions: GraphQLErrorExtensions;
 
+  /**
+   * @deprecated Please use the `GraphQLErrorArgs` constructor overload instead.
+   */
   constructor(
     message: string,
     nodes?: ReadonlyArray<ASTNode> | ASTNode | null,
@@ -84,7 +122,11 @@ export class GraphQLError extends Error {
     path?: Maybe<ReadonlyArray<string | number>>,
     originalError?: Maybe<Error & { readonly extensions?: unknown }>,
     extensions?: Maybe<GraphQLErrorExtensions>,
-  ) {
+  );
+  constructor(message: string, args?: GraphQLErrorArgs);
+  constructor(message: string, ...rawArgs: BackwardsCompatibleArgs) {
+    const { nodes, source, positions, path, originalError, extensions } =
+      toNormalizedArgs(rawArgs);
     super(message);
 
     this.name = 'GraphQLError';
diff --git a/src/utilities/findBreakingChanges.ts b/src/utilities/findBreakingChanges.ts
index b82901f9..0bf0d453 100644
--- a/src/utilities/findBreakingChanges.ts
+++ b/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 }>(
diff --git a/src/utilities/sortValueNode.ts b/src/utilities/sortValueNode.ts
new file mode 100644
index 00000000..6050c9a9
--- /dev/null
+++ b/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),
+    );
+}
diff --git a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts
index 5796da6f..a7d2b257 100644
--- a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts
+++ b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts
@@ -5,11 +5,10 @@ import type { ObjMap } from '../../jsutils/ObjMap';
 import { GraphQLError } from '../../error/GraphQLError';
 
 import type {
-  ArgumentNode,
   FieldNode,
   FragmentDefinitionNode,
+  ObjectValueNode,
   SelectionSetNode,
-  ValueNode,
 } from '../../language/ast';
 import { Kind } from '../../language/kinds';
 import { print } from '../../language/printer';
@@ -29,6 +28,7 @@ import {
   isObjectType,
 } from '../../type/definition';
 
+import { sortValueNode } from '../../utilities/sortValueNode';
 import { typeFromAST } from '../../utilities/typeFromAST';
 
 import type { ValidationContext } from '../ValidationContext';
@@ -587,12 +587,8 @@ function findConflict(
       ];
     }
 
-    // FIXME https://github.com/graphql/graphql-js/issues/2203
-    const args1 = /* c8 ignore next */ node1.arguments ?? [];
-    const args2 = /* c8 ignore next */ node2.arguments ?? [];
-
     // Two field calls must have the same arguments.
-    if (!sameArguments(args1, args2)) {
+    if (stringifyArguments(node1) !== stringifyArguments(node2)) {
       return [
         [responseName, 'they have differing arguments'],
         [node1],
@@ -638,26 +634,19 @@ function findConflict(
   }
 }
 
-function sameArguments(
-  arguments1: ReadonlyArray<ArgumentNode>,
-  arguments2: ReadonlyArray<ArgumentNode>,
-): boolean {
-  if (arguments1.length !== arguments2.length) {
-    return false;
-  }
-  return arguments1.every((argument1) => {
-    const argument2 = arguments2.find(
-      (argument) => argument.name.value === argument1.name.value,
-    );
-    if (!argument2) {
-      return false;
-    }
-    return sameValue(argument1.value, argument2.value);
-  });
-}
-
-function sameValue(value1: ValueNode, value2: ValueNode): boolean {
-  return print(value1) === print(value2);
+function stringifyArguments(fieldNode: FieldNode): 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));
 }
 
 // Two types conflict if both types could not apply to a value simultaneously.


Test patch:
diff --git a/src/error/__tests__/GraphQLError-test.ts b/src/error/__tests__/GraphQLError-test.ts
index 21e1ab1f..60f35e9b 100644
--- a/src/error/__tests__/GraphQLError-test.ts
+++ b/src/error/__tests__/GraphQLError-test.ts
@@ -353,4 +353,22 @@ describe('toJSON', () => {
       extensions: { foo: 'bar' },
     });
   });
+
+  it('can be created with the alternative object argument', () => {
+    const error = new GraphQLError('msg', {
+      nodes: [operationNode],
+      source,
+      positions: [6],
+      path: ['path', 2, 'a'],
+      originalError: new Error('I like turtles'),
+      extensions: { hee: 'I like turtles' },
+    });
+
+    expect(error.toJSON()).to.deep.equal({
+      message: 'msg',
+      locations: [{ column: 5, line: 2 }],
+      path: ['path', 2, 'a'],
+      extensions: { hee: 'I like turtles' },
+    });
+  });
 });
diff --git a/src/language/__tests__/blockString-test.ts b/src/language/__tests__/blockString-test.ts
index a37858d3..21fa49b0 100644
--- a/src/language/__tests__/blockString-test.ts
+++ b/src/language/__tests__/blockString-test.ts
@@ -20,7 +20,7 @@ describe('dedentBlockStringLines', () => {
     expectDedent(['']).to.deep.equal([]);
   });
 
-  it('do not dedent first line', () => {
+  it('does not dedent first line', () => {
     expectDedent(['  a']).to.deep.equal(['  a']);
     expectDedent([' a', '  b']).to.deep.equal([' a', 'b']);
   });
@@ -162,12 +162,7 @@ describe('isPrintableAsBlockString', () => {
     expectNonPrintable(' \t');
   });
 
-  it('rejects strings with non-printable character', () => {
-    expectNonPrintable('\x00');
-    expectNonPrintable('a\x00b');
-  });
-
-  it('rejects strings with non-printable character', () => {
+  it('rejects strings with non-printable characters', () => {
     expectNonPrintable('\x00');
     expectNonPrintable('a\x00b');
   });
@@ -194,7 +189,7 @@ describe('isPrintableAsBlockString', () => {
     expectNonPrintable('\n\na');
   });
 
-  it('rejects strings with leading empty lines', () => {
+  it('rejects strings with trailing empty lines', () => {
     expectNonPrintable('a\n');
     expectNonPrintable('a\n ');
     expectNonPrintable('a\n\t');
@@ -217,7 +212,7 @@ describe('printBlockString', () => {
     };
   }
 
-  it('do not escape characters', () => {
+  it('does not escape characters', () => {
     const str = '" \\ / \b \f \n \r \t';
     expectBlockString(str).toEqual({
       readable: '"""\n' + str + '\n"""',
diff --git a/src/utilities/__tests__/sortValueNode-test.ts b/src/utilities/__tests__/sortValueNode-test.ts
new file mode 100644
index 00000000..5bda137d
--- /dev/null
+++ b/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}');
+  });
+});
diff --git a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts
index 9d864902..46cf014e 100644
--- a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts
+++ b/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(`
       {