From e29840c1d7e855dbb267c16f74a9e6aed4626f82 Mon Sep 17 00:00:00 2001 From: Aaron Moat <2937187+AaronMoat@users.noreply.github.com> Date: Thu, 24 Aug 2023 21:34:51 +1000 Subject: [PATCH 1/4] Fix performance degradation --- .../rules/OverlappingFieldsCanBeMergedRule.ts | 40 +++++++++++-------- 1 file changed, 24 insertions(+), 16 deletions(-) diff --git a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts index 185648fe29..11ad8bd991 100644 --- a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts +++ b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts @@ -4,11 +4,12 @@ import type { Maybe } from '../../jsutils/Maybe.js'; import { GraphQLError } from '../../error/GraphQLError.js'; import type { + ArgumentNode, DirectiveNode, FieldNode, FragmentDefinitionNode, - ObjectValueNode, SelectionSetNode, + ValueNode, } from '../../language/ast.js'; import { Kind } from '../../language/kinds.js'; import { print } from '../../language/printer.js'; @@ -592,7 +593,7 @@ function findConflict( } // Two field calls must have the same arguments. - if (stringifyArguments(node1) !== stringifyArguments(node2)) { + if (!sameArguments(node1.arguments, node2.arguments)) { return [ [responseName, 'they have differing arguments'], [node1], @@ -649,19 +650,26 @@ 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( + arguments1: ReadonlyArray = [], + arguments2: ReadonlyArray = [], +): 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 stringifyValue(argument1.value) === stringifyValue(argument2.value); + }); +} + +function stringifyValue(value: ValueNode): string { + return print(sortValueNode(value)); } function getStreamDirective( @@ -681,7 +689,7 @@ function sameStreams( return true; } else if (stream1 && stream2) { // check if both fields have equivalent streams - return stringifyArguments(stream1) === stringifyArguments(stream2); + return sameArguments(stream1.arguments, stream2.arguments); } // fields have a mix of stream and no stream return false; From 8d962e8d427812e0a43a3e51a4fb811805767bc6 Mon Sep 17 00:00:00 2001 From: AaronMoat Date: Fri, 1 Sep 2023 21:41:27 +1000 Subject: [PATCH 2/4] Add benchmark --- benchmark/repeated-fields-benchmark.js | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 benchmark/repeated-fields-benchmark.js diff --git a/benchmark/repeated-fields-benchmark.js b/benchmark/repeated-fields-benchmark.js new file mode 100644 index 0000000000..812757a99e --- /dev/null +++ b/benchmark/repeated-fields-benchmark.js @@ -0,0 +1,18 @@ +import { graphqlSync } from 'graphql/graphql.js'; +import { buildSchema } from 'graphql/utilities/buildASTSchema.js'; + +import { bigSchemaSDL } from './fixtures.js'; + +const schema = buildSchema(bigSchemaSDL, { assumeValid: true }); +const query = `{ ${'__typename '.repeat(2000)}}`; + +export const benchmark = { + name: 'Many repeated fields', + count: 50, + measure() { + graphqlSync({ + schema, + source: query, + }); + }, +}; From 70ff0edbbbb71810bf1ac340ed37247c2783dcf9 Mon Sep 17 00:00:00 2001 From: AaronMoat Date: Sat, 2 Sep 2023 16:14:19 +1000 Subject: [PATCH 3/4] Reduce benchmark footprint --- benchmark/repeated-fields-benchmark.js | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/benchmark/repeated-fields-benchmark.js b/benchmark/repeated-fields-benchmark.js index 812757a99e..b416986214 100644 --- a/benchmark/repeated-fields-benchmark.js +++ b/benchmark/repeated-fields-benchmark.js @@ -1,18 +1,13 @@ import { graphqlSync } from 'graphql/graphql.js'; import { buildSchema } from 'graphql/utilities/buildASTSchema.js'; -import { bigSchemaSDL } from './fixtures.js'; - -const schema = buildSchema(bigSchemaSDL, { assumeValid: true }); -const query = `{ ${'__typename '.repeat(2000)}}`; +const schema = buildSchema('type Query { hello: String! }'); +const source = `{ ${'hello '.repeat(250)}}`; export const benchmark = { name: 'Many repeated fields', - count: 50, + count: 5, measure() { - graphqlSync({ - schema, - source: query, - }); + graphqlSync({ schema, source }); }, }; From 2991112867dbe25937c330806864e133951a57e7 Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Tue, 5 Sep 2023 21:21:28 +0300 Subject: [PATCH 4/4] review changes --- .../OverlappingFieldsCanBeMergedRule-test.ts | 18 +++++++++ .../rules/OverlappingFieldsCanBeMergedRule.ts | 37 ++++++++++++------- 2 files changed, 42 insertions(+), 13 deletions(-) diff --git a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts index 52c2deb1a0..ecb56a15cf 100644 --- 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 index 11ad8bd991..851f7ea625 100644 --- a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts +++ b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts @@ -4,7 +4,6 @@ import type { Maybe } from '../../jsutils/Maybe.js'; import { GraphQLError } from '../../error/GraphQLError.js'; import type { - ArgumentNode, DirectiveNode, FieldNode, FragmentDefinitionNode, @@ -593,7 +592,7 @@ function findConflict( } // Two field calls must have the same arguments. - if (!sameArguments(node1.arguments, node2.arguments)) { + if (!sameArguments(node1, node2)) { return [ [responseName, 'they have differing arguments'], [node1], @@ -651,24 +650,36 @@ function findConflict( } function sameArguments( - arguments1: ReadonlyArray = [], - arguments2: ReadonlyArray = [], + node1: FieldNode | DirectiveNode, + node2: FieldNode | DirectiveNode, ): boolean { - if (arguments1?.length !== arguments2?.length) { + 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; } - return arguments1.every((argument1) => { - const argument2 = arguments2.find( - (argument) => argument.name.value === argument1.name.value, - ); - if (!argument2) { + + 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(argument1.value) === stringifyValue(argument2.value); + + return stringifyValue(value1) === stringifyValue(value2); }); } -function stringifyValue(value: ValueNode): string { +function stringifyValue(value: ValueNode): string | null { return print(sortValueNode(value)); } @@ -689,7 +700,7 @@ function sameStreams( return true; } else if (stream1 && stream2) { // check if both fields have equivalent streams - return sameArguments(stream1.arguments, stream2.arguments); + return sameArguments(stream1, stream2); } // fields have a mix of stream and no stream return false;