From 40c160e9fb0e1ff92fc954c03274adf94c3004a9 Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Wed, 12 Jan 2022 20:01:09 +0200 Subject: [PATCH] OverlappingFieldsCanBeMerged: sort argument values before comparing (#3455) --- src/utilities/__tests__/sortValueNode-test.ts | 40 ++++++++++++++++ src/utilities/findBreakingChanges.ts | 18 +------ src/utilities/sortValueNode.ts | 47 +++++++++++++++++++ .../OverlappingFieldsCanBeMergedRule-test.ts | 45 ++++++++++++++++++ .../rules/OverlappingFieldsCanBeMergedRule.ts | 7 +-- 5 files changed, 138 insertions(+), 19 deletions(-) create mode 100644 src/utilities/__tests__/sortValueNode-test.ts create mode 100644 src/utilities/sortValueNode.ts diff --git a/src/utilities/__tests__/sortValueNode-test.ts b/src/utilities/__tests__/sortValueNode-test.ts new file mode 100644 index 0000000000..5bda137d14 --- /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/utilities/findBreakingChanges.ts b/src/utilities/findBreakingChanges.ts index b82901f972..0bf0d453b4 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( diff --git a/src/utilities/sortValueNode.ts b/src/utilities/sortValueNode.ts new file mode 100644 index 0000000000..6050c9a907 --- /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, +): Array { + return fields + .map((fieldNode) => ({ + ...fieldNode, + value: sortValueNode(fieldNode.value), + })) + .sort((fieldA, fieldB) => + naturalCompare(fieldA.name.value, fieldB.name.value), + ); +} diff --git a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts index 9d864902da..46cf014e46 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(` { diff --git a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts index 5796da6f28..83f6945703 100644 --- a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts +++ b/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.