From 3918bb534d5f1cf5579664420103c2a58e5ae511 Mon Sep 17 00:00:00 2001 From: Jovi De Croock Date: Fri, 26 Jan 2024 10:09:41 +0100 Subject: [PATCH 1/5] Add support for parsing fragment arguments Co-authored-by: mjmahone --- src/language/__tests__/parser-test.ts | 25 ++++++++-- src/language/__tests__/printer-test.ts | 44 +++++++++++++---- src/language/__tests__/visitor-test.ts | 48 ++++++++++++++++++- src/language/ast.ts | 4 +- src/language/directiveLocation.ts | 1 + src/language/parser.ts | 41 ++++++++++------ src/language/printer.ts | 27 +++++++---- src/type/__tests__/introspection-test.ts | 5 ++ src/type/introspection.ts | 6 ++- src/utilities/__tests__/printSchema-test.ts | 5 +- src/utilities/buildASTSchema.ts | 2 +- .../__tests__/KnownDirectivesRule-test.ts | 14 ++++-- src/validation/__tests__/harness.ts | 2 +- src/validation/rules/KnownDirectivesRule.ts | 9 +++- 14 files changed, 187 insertions(+), 46 deletions(-) diff --git a/src/language/__tests__/parser-test.ts b/src/language/__tests__/parser-test.ts index 34e9dff4b9..215e743be7 100644 --- a/src/language/__tests__/parser-test.ts +++ b/src/language/__tests__/parser-test.ts @@ -607,13 +607,32 @@ describe('Parser', () => { expect('loc' in result).to.equal(false); }); - it('Legacy: allows parsing fragment defined variables', () => { + it('allows parsing fragment defined variables', () => { const document = 'fragment a($v: Boolean = false) on t { f(v: $v) }'; expect(() => - parse(document, { allowLegacyFragmentVariables: true }), + parse(document, { experimentalFragmentArguments: true }), ).to.not.throw(); - expect(() => parse(document)).to.throw('Syntax Error'); + }); + + it('disallows parsing fragment defined variables without experimental flag', () => { + const document = 'fragment a($v: Boolean = false) on t { f(v: $v) }'; + + expect(() => parse(document)).to.throw(); + }); + + it('allows parsing fragment spread arguments', () => { + const document = 'fragment a on t { ...b(v: $v) }'; + + expect(() => + parse(document, { experimentalFragmentArguments: true }), + ).to.not.throw(); + }); + + it('disallows parsing fragment spread arguments without experimental flag', () => { + const document = 'fragment a on t { ...b(v: $v) }'; + + expect(() => parse(document)).to.throw(); }); it('contains location that can be Object.toStringified, JSON.stringified, or jsutils.inspected', () => { diff --git a/src/language/__tests__/printer-test.ts b/src/language/__tests__/printer-test.ts index 7eea508458..8669b64ec7 100644 --- a/src/language/__tests__/printer-test.ts +++ b/src/language/__tests__/printer-test.ts @@ -168,34 +168,62 @@ describe('Printer: Query document', () => { `); }); - it('Legacy: prints fragment with variable directives', () => { - const queryASTWithVariableDirective = parse( + it('prints fragment with argument definition directives', () => { + const fragmentWithArgumentDefinitionDirective = parse( 'fragment Foo($foo: TestType @test) on TestType @testDirective { id }', - { allowLegacyFragmentVariables: true }, + { experimentalFragmentArguments: true }, ); - expect(print(queryASTWithVariableDirective)).to.equal(dedent` + expect(print(fragmentWithArgumentDefinitionDirective)).to.equal(dedent` fragment Foo($foo: TestType @test) on TestType @testDirective { id } `); }); - it('Legacy: correctly prints fragment defined variables', () => { - const fragmentWithVariable = parse( + it('correctly prints fragment defined arguments', () => { + const fragmentWithArgumentDefinition = parse( ` fragment Foo($a: ComplexType, $b: Boolean = false) on TestType { id } `, - { allowLegacyFragmentVariables: true }, + { experimentalFragmentArguments: true }, ); - expect(print(fragmentWithVariable)).to.equal(dedent` + expect(print(fragmentWithArgumentDefinition)).to.equal(dedent` fragment Foo($a: ComplexType, $b: Boolean = false) on TestType { id } `); }); + it('prints fragment spread with arguments', () => { + const fragmentSpreadWithArguments = parse( + 'fragment Foo on TestType { ...Bar(a: {x: $x}, b: true) }', + { experimentalFragmentArguments: true }, + ); + expect(print(fragmentSpreadWithArguments)).to.equal(dedent` + fragment Foo on TestType { + ...Bar(a: { x: $x }, b: true) + } + `); + }); + + it('prints fragment spread with multi-line arguments', () => { + const fragmentSpreadWithArguments = parse( + 'fragment Foo on TestType { ...Bar(a: {x: $x, y: $y, z: $z, xy: $xy}, b: true, c: "a long string extending arguments over max length") }', + { experimentalFragmentArguments: true }, + ); + expect(print(fragmentSpreadWithArguments)).to.equal(dedent` + fragment Foo on TestType { + ...Bar( + a: { x: $x, y: $y, z: $z, xy: $xy } + b: true + c: "a long string extending arguments over max length" + ) + } + `); + }); + it('prints kitchen sink without altering ast', () => { const ast = parse(kitchenSinkQuery, { noLocation: true, diff --git a/src/language/__tests__/visitor-test.ts b/src/language/__tests__/visitor-test.ts index c70cb783d5..ab44b1a518 100644 --- a/src/language/__tests__/visitor-test.ts +++ b/src/language/__tests__/visitor-test.ts @@ -455,10 +455,10 @@ describe('Visitor', () => { ]); }); - it('Legacy: visits variables defined in fragments', () => { + it('visits arguments defined on fragments', () => { const ast = parse('fragment a($v: Boolean = false) on t { f }', { noLocation: true, - allowLegacyFragmentVariables: true, + experimentalFragmentArguments: true, }); const visited: Array = []; @@ -505,6 +505,50 @@ describe('Visitor', () => { ]); }); + it('visits arguments on fragment spreads', () => { + const ast = parse('fragment a on t { ...s(v: false) }', { + noLocation: true, + experimentalFragmentArguments: true, + }); + const visited: Array = []; + + visit(ast, { + enter(node) { + checkVisitorFnArgs(ast, arguments); + visited.push(['enter', node.kind, getValue(node)]); + }, + leave(node) { + checkVisitorFnArgs(ast, arguments); + visited.push(['leave', node.kind, getValue(node)]); + }, + }); + + expect(visited).to.deep.equal([ + ['enter', 'Document', undefined], + ['enter', 'FragmentDefinition', undefined], + ['enter', 'Name', 'a'], + ['leave', 'Name', 'a'], + ['enter', 'NamedType', undefined], + ['enter', 'Name', 't'], + ['leave', 'Name', 't'], + ['leave', 'NamedType', undefined], + ['enter', 'SelectionSet', undefined], + ['enter', 'FragmentSpread', undefined], + ['enter', 'Name', 's'], + ['leave', 'Name', 's'], + ['enter', 'Argument', { kind: 'BooleanValue', value: false }], + ['enter', 'Name', 'v'], + ['leave', 'Name', 'v'], + ['enter', 'BooleanValue', false], + ['leave', 'BooleanValue', false], + ['leave', 'Argument', { kind: 'BooleanValue', value: false }], + ['leave', 'FragmentSpread', undefined], + ['leave', 'SelectionSet', undefined], + ['leave', 'FragmentDefinition', undefined], + ['leave', 'Document', undefined], + ]); + }); + it('properly visits the kitchen sink query', () => { const ast = parse(kitchenSinkQuery, { experimentalClientControlledNullability: true, diff --git a/src/language/ast.ts b/src/language/ast.ts index 22a7cc253c..c42866119d 100644 --- a/src/language/ast.ts +++ b/src/language/ast.ts @@ -227,7 +227,7 @@ export const QueryDocumentKeys: { NonNullAssertion: ['nullabilityAssertion'], ErrorBoundary: ['nullabilityAssertion'], - FragmentSpread: ['name', 'directives'], + FragmentSpread: ['name', 'arguments', 'directives'], InlineFragment: ['typeCondition', 'directives', 'selectionSet'], FragmentDefinition: [ 'name', @@ -428,6 +428,7 @@ export interface FragmentSpreadNode { readonly kind: Kind.FRAGMENT_SPREAD; readonly loc?: Location | undefined; readonly name: NameNode; + readonly arguments?: ReadonlyArray | undefined; readonly directives?: ReadonlyArray | undefined; } @@ -443,7 +444,6 @@ export interface FragmentDefinitionNode { readonly kind: Kind.FRAGMENT_DEFINITION; readonly loc?: Location | undefined; readonly name: NameNode; - /** @deprecated variableDefinitions will be removed in v17.0.0 */ readonly variableDefinitions?: | ReadonlyArray | undefined; diff --git a/src/language/directiveLocation.ts b/src/language/directiveLocation.ts index b10214d47f..568de6bba1 100644 --- a/src/language/directiveLocation.ts +++ b/src/language/directiveLocation.ts @@ -23,4 +23,5 @@ export enum DirectiveLocation { ENUM_VALUE = 'ENUM_VALUE', INPUT_OBJECT = 'INPUT_OBJECT', INPUT_FIELD_DEFINITION = 'INPUT_FIELD_DEFINITION', + FRAGMENT_VARIABLE_DEFINITION = 'FRAGMENT_VARIABLE_DEFINITION', } diff --git a/src/language/parser.ts b/src/language/parser.ts index cd9345f6dd..d6aae4a01c 100644 --- a/src/language/parser.ts +++ b/src/language/parser.ts @@ -92,21 +92,25 @@ export interface ParseOptions { maxTokens?: number | undefined; /** - * @deprecated will be removed in the v17.0.0 + * EXPERIMENTAL: * - * If enabled, the parser will understand and parse variable definitions - * contained in a fragment definition. They'll be represented in the - * `variableDefinitions` field of the FragmentDefinitionNode. + * If enabled, the parser will understand and parse fragment variable definitions + * and arguments on fragment spreads. Fragment variable definitions will be represented + * in the `variableDefinitions` field of the FragmentDefinitionNode. + * Fragment spread arguments will be represented in the `arguments` field of FragmentSpreadNode. * - * The syntax is identical to normal, query-defined variables. For example: + * For example: * * ```graphql + * { + * t { ...A(var: true) } + * } * fragment A($var: Boolean = false) on T { - * ... + * ...B(x: $var) * } * ``` */ - allowLegacyFragmentVariables?: boolean | undefined; + experimentalFragmentArguments?: boolean | undefined; /** * EXPERIMENTAL: @@ -550,7 +554,7 @@ export class Parser { /** * Corresponds to both FragmentSpread and InlineFragment in the spec. * - * FragmentSpread : ... FragmentName Directives? + * FragmentSpread : ... FragmentName Arguments? Directives? * * InlineFragment : ... TypeCondition? Directives? SelectionSet */ @@ -560,9 +564,21 @@ export class Parser { const hasTypeCondition = this.expectOptionalKeyword('on'); if (!hasTypeCondition && this.peek(TokenKind.NAME)) { + const name = this.parseFragmentName(); + if ( + this.peek(TokenKind.PAREN_L) && + this._options.experimentalFragmentArguments + ) { + return this.node(start, { + kind: Kind.FRAGMENT_SPREAD, + name, + arguments: this.parseArguments(false), + directives: this.parseDirectives(false), + }); + } return this.node(start, { kind: Kind.FRAGMENT_SPREAD, - name: this.parseFragmentName(), + name, directives: this.parseDirectives(false), }); } @@ -576,17 +592,14 @@ export class Parser { /** * FragmentDefinition : - * - fragment FragmentName on TypeCondition Directives? SelectionSet + * - fragment FragmentName VariableDefinitions? on TypeCondition Directives? SelectionSet * * TypeCondition : NamedType */ parseFragmentDefinition(): FragmentDefinitionNode { const start = this._lexer.token; this.expectKeyword('fragment'); - // Legacy support for defining variables within fragments changes - // the grammar of FragmentDefinition: - // - fragment FragmentName VariableDefinitions? on TypeCondition Directives? SelectionSet - if (this._options.allowLegacyFragmentVariables === true) { + if (this._options.experimentalFragmentArguments === true) { return this.node(start, { kind: Kind.FRAGMENT_DEFINITION, name: this.parseFragmentName(), diff --git a/src/language/printer.ts b/src/language/printer.ts index 28bb25da17..bd82672735 100644 --- a/src/language/printer.ts +++ b/src/language/printer.ts @@ -64,14 +64,9 @@ const printDocASTReducer: ASTReducer = { selectionSet, }) { const prefix = join([wrap('', alias, ': '), name], ''); - let argsLine = prefix + wrap('(', join(args, ', '), ')'); - - if (argsLine.length > MAX_LINE_LENGTH) { - argsLine = prefix + wrap('(\n', indent(join(args, '\n')), '\n)'); - } return join([ - argsLine, + wrappedLineAndArgs(prefix, args), // Note: Client Controlled Nullability is experimental and may be // changed or removed in the future. nullabilityAssertion, @@ -105,8 +100,12 @@ const printDocASTReducer: ASTReducer = { // Fragments FragmentSpread: { - leave: ({ name, directives }) => - '...' + name + wrap(' ', join(directives, ' ')), + leave: ({ name, arguments: args, directives }) => { + const prefix = '...' + name; + return ( + wrappedLineAndArgs(prefix, args) + wrap(' ', join(directives, ' ')) + ); + }, }, InlineFragment: { @@ -392,3 +391,15 @@ function hasMultilineItems(maybeArray: Maybe>): boolean { /* c8 ignore next */ return maybeArray?.some((str) => str.includes('\n')) ?? false; } + +function wrappedLineAndArgs( + prefix: string, + args: ReadonlyArray | undefined, +): string { + let argsLine = prefix + wrap('(', join(args, ', '), ')'); + + if (argsLine.length > MAX_LINE_LENGTH) { + argsLine = prefix + wrap('(\n', indent(join(args, '\n')), '\n)'); + } + return argsLine; +} diff --git a/src/type/__tests__/introspection-test.ts b/src/type/__tests__/introspection-test.ts index e6e5d0943e..160c2ea245 100644 --- a/src/type/__tests__/introspection-test.ts +++ b/src/type/__tests__/introspection-test.ts @@ -859,6 +859,11 @@ describe('Introspection', () => { isDeprecated: false, deprecationReason: null, }, + { + name: 'FRAGMENT_VARIABLE_DEFINITION', + isDeprecated: false, + deprecationReason: null, + }, { name: 'SCHEMA', isDeprecated: false, diff --git a/src/type/introspection.ts b/src/type/introspection.ts index 5f3c9c1fa5..6d5c29dd90 100644 --- a/src/type/introspection.ts +++ b/src/type/introspection.ts @@ -155,7 +155,11 @@ export const __DirectiveLocation: GraphQLEnumType = new GraphQLEnumType({ }, VARIABLE_DEFINITION: { value: DirectiveLocation.VARIABLE_DEFINITION, - description: 'Location adjacent to a variable definition.', + description: 'Location adjacent to an operation variable definition.', + }, + FRAGMENT_VARIABLE_DEFINITION: { + value: DirectiveLocation.FRAGMENT_VARIABLE_DEFINITION, + description: 'Location adjacent to a fragment variable definition.', }, SCHEMA: { value: DirectiveLocation.SCHEMA, diff --git a/src/utilities/__tests__/printSchema-test.ts b/src/utilities/__tests__/printSchema-test.ts index 78f793b183..ac9002e388 100644 --- a/src/utilities/__tests__/printSchema-test.ts +++ b/src/utilities/__tests__/printSchema-test.ts @@ -971,9 +971,12 @@ describe('Type System Printer', () => { """Location adjacent to an inline fragment.""" INLINE_FRAGMENT - """Location adjacent to a variable definition.""" + """Location adjacent to an operation variable definition.""" VARIABLE_DEFINITION + """Location adjacent to a fragment variable definition.""" + FRAGMENT_VARIABLE_DEFINITION + """Location adjacent to a schema definition.""" SCHEMA diff --git a/src/utilities/buildASTSchema.ts b/src/utilities/buildASTSchema.ts index 09fd7cd7dd..5be0b6e421 100644 --- a/src/utilities/buildASTSchema.ts +++ b/src/utilities/buildASTSchema.ts @@ -93,7 +93,7 @@ export function buildSchema( ): GraphQLSchema { const document = parse(source, { noLocation: options?.noLocation, - allowLegacyFragmentVariables: options?.allowLegacyFragmentVariables, + experimentalFragmentArguments: options?.experimentalFragmentArguments, }); return buildASTSchema(document, { diff --git a/src/validation/__tests__/KnownDirectivesRule-test.ts b/src/validation/__tests__/KnownDirectivesRule-test.ts index a3bbc198da..2cc8c44ede 100644 --- a/src/validation/__tests__/KnownDirectivesRule-test.ts +++ b/src/validation/__tests__/KnownDirectivesRule-test.ts @@ -44,6 +44,7 @@ const schemaWithDirectives = buildSchema(` directive @onFragmentSpread on FRAGMENT_SPREAD directive @onInlineFragment on INLINE_FRAGMENT directive @onVariableDefinition on VARIABLE_DEFINITION + directive @onFragmentVariableDefinition on FRAGMENT_VARIABLE_DEFINITION `); const schemaWithSDLDirectives = buildSchema(` @@ -150,7 +151,9 @@ describe('Validate: Known directives', () => { someField @onField } - fragment Frag on Human @onFragmentDefinition { + fragment Frag( + $arg: Int @onFragmentVariableDefinition + ) on Human @onFragmentDefinition { name @onField } `); @@ -175,7 +178,7 @@ describe('Validate: Known directives', () => { someField @onQuery } - fragment Frag on Human @onQuery { + fragment Frag($arg: Int @onVariableDefinition) on Human @onQuery { name @onQuery } `).toDeepEqual([ @@ -219,9 +222,14 @@ describe('Validate: Known directives', () => { message: 'Directive "@onQuery" may not be used on FIELD.', locations: [{ column: 19, line: 16 }], }, + { + message: + 'Directive "@onVariableDefinition" may not be used on FRAGMENT_VARIABLE_DEFINITION.', + locations: [{ column: 31, line: 19 }], + }, { message: 'Directive "@onQuery" may not be used on FRAGMENT_DEFINITION.', - locations: [{ column: 30, line: 19 }], + locations: [{ column: 63, line: 19 }], }, { message: 'Directive "@onQuery" may not be used on FIELD.', diff --git a/src/validation/__tests__/harness.ts b/src/validation/__tests__/harness.ts index b7710ff9d9..0db861f45b 100644 --- a/src/validation/__tests__/harness.ts +++ b/src/validation/__tests__/harness.ts @@ -129,7 +129,7 @@ export function expectValidationErrorsWithSchema( rule: ValidationRule, queryStr: string, ): any { - const doc = parse(queryStr); + const doc = parse(queryStr, { experimentalFragmentArguments: true }); const errors = validate(schema, doc, [rule]); return expectJSON(errors); } diff --git a/src/validation/rules/KnownDirectivesRule.ts b/src/validation/rules/KnownDirectivesRule.ts index a2c7ec81eb..9a1e87d029 100644 --- a/src/validation/rules/KnownDirectivesRule.ts +++ b/src/validation/rules/KnownDirectivesRule.ts @@ -89,8 +89,13 @@ function getDirectiveLocationForASTPath( return DirectiveLocation.INLINE_FRAGMENT; case Kind.FRAGMENT_DEFINITION: return DirectiveLocation.FRAGMENT_DEFINITION; - case Kind.VARIABLE_DEFINITION: - return DirectiveLocation.VARIABLE_DEFINITION; + case Kind.VARIABLE_DEFINITION: { + const parentNode = ancestors[ancestors.length - 3]; + invariant('kind' in parentNode); + return parentNode.kind === Kind.OPERATION_DEFINITION + ? DirectiveLocation.VARIABLE_DEFINITION + : DirectiveLocation.FRAGMENT_VARIABLE_DEFINITION; + } case Kind.SCHEMA_DEFINITION: case Kind.SCHEMA_EXTENSION: return DirectiveLocation.SCHEMA; From c3f01bdb8ff62184581e54a3abc1ae5c19c10ce5 Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Thu, 29 Aug 2024 20:51:01 +0300 Subject: [PATCH 2/5] use a different Kind for Fragment Arguments (#13) at least for now! --- src/index.ts | 1 + src/language/__tests__/visitor-test.ts | 4 ++-- src/language/ast.ts | 11 ++++++++++- src/language/index.ts | 1 + src/language/kinds.ts | 1 + src/language/parser.ts | 22 +++++++++++++++++++++- src/language/printer.ts | 1 + 7 files changed, 37 insertions(+), 4 deletions(-) diff --git a/src/index.ts b/src/index.ts index fb67b4c75e..3d82bffdda 100644 --- a/src/index.ts +++ b/src/index.ts @@ -264,6 +264,7 @@ export type { SelectionNode, FieldNode, ArgumentNode, + FragmentArgumentNode, NullabilityAssertionNode, NonNullAssertionNode, ErrorBoundaryNode, diff --git a/src/language/__tests__/visitor-test.ts b/src/language/__tests__/visitor-test.ts index ab44b1a518..73b46d6830 100644 --- a/src/language/__tests__/visitor-test.ts +++ b/src/language/__tests__/visitor-test.ts @@ -536,12 +536,12 @@ describe('Visitor', () => { ['enter', 'FragmentSpread', undefined], ['enter', 'Name', 's'], ['leave', 'Name', 's'], - ['enter', 'Argument', { kind: 'BooleanValue', value: false }], + ['enter', 'FragmentArgument', { kind: 'BooleanValue', value: false }], ['enter', 'Name', 'v'], ['leave', 'Name', 'v'], ['enter', 'BooleanValue', false], ['leave', 'BooleanValue', false], - ['leave', 'Argument', { kind: 'BooleanValue', value: false }], + ['leave', 'FragmentArgument', { kind: 'BooleanValue', value: false }], ['leave', 'FragmentSpread', undefined], ['leave', 'SelectionSet', undefined], ['leave', 'FragmentDefinition', undefined], diff --git a/src/language/ast.ts b/src/language/ast.ts index c42866119d..8eb8eb132b 100644 --- a/src/language/ast.ts +++ b/src/language/ast.ts @@ -146,6 +146,7 @@ export type ASTNode = | SelectionSetNode | FieldNode | ArgumentNode + | FragmentArgumentNode | FragmentSpreadNode | InlineFragmentNode | FragmentDefinitionNode @@ -221,6 +222,7 @@ export const QueryDocumentKeys: { 'nullabilityAssertion', ], Argument: ['name', 'value'], + FragmentArgument: ['name', 'value'], // Note: Client Controlled Nullability is experimental and may be changed // or removed in the future. ListNullabilityOperator: ['nullabilityAssertion'], @@ -422,13 +424,20 @@ export interface ConstArgumentNode { readonly value: ConstValueNode; } +export interface FragmentArgumentNode { + readonly kind: Kind.FRAGMENT_ARGUMENT; + readonly loc?: Location | undefined; + readonly name: NameNode; + readonly value: ValueNode; +} + /** Fragments */ export interface FragmentSpreadNode { readonly kind: Kind.FRAGMENT_SPREAD; readonly loc?: Location | undefined; readonly name: NameNode; - readonly arguments?: ReadonlyArray | undefined; + readonly arguments?: ReadonlyArray | undefined; readonly directives?: ReadonlyArray | undefined; } diff --git a/src/language/index.ts b/src/language/index.ts index fc87a566cc..2899f47c64 100644 --- a/src/language/index.ts +++ b/src/language/index.ts @@ -44,6 +44,7 @@ export type { ErrorBoundaryNode, ListNullabilityOperatorNode, ArgumentNode, + FragmentArgumentNode /* for experimental fragment arguments */, ConstArgumentNode, FragmentSpreadNode, InlineFragmentNode, diff --git a/src/language/kinds.ts b/src/language/kinds.ts index d606c12cbe..f3d8fdc3c5 100644 --- a/src/language/kinds.ts +++ b/src/language/kinds.ts @@ -12,6 +12,7 @@ export enum Kind { SELECTION_SET = 'SelectionSet', FIELD = 'Field', ARGUMENT = 'Argument', + FRAGMENT_ARGUMENT = 'FragmentArgument', /** Nullability Modifiers */ LIST_NULLABILITY_OPERATOR = 'ListNullabilityOperator', diff --git a/src/language/parser.ts b/src/language/parser.ts index d6aae4a01c..0bda2b0c8f 100644 --- a/src/language/parser.ts +++ b/src/language/parser.ts @@ -24,6 +24,7 @@ import type { FieldDefinitionNode, FieldNode, FloatValueNode, + FragmentArgumentNode, FragmentDefinitionNode, FragmentSpreadNode, InlineFragmentNode, @@ -528,6 +529,12 @@ export class Parser { return this.optionalMany(TokenKind.PAREN_L, item, TokenKind.PAREN_R); } + /* experimental */ + parseFragmentArguments(): Array { + const item = this.parseFragmentArgument; + return this.optionalMany(TokenKind.PAREN_L, item, TokenKind.PAREN_R); + } + /** * Argument[Const] : Name : Value[?Const] */ @@ -549,6 +556,19 @@ export class Parser { return this.parseArgument(true); } + /* experimental */ + parseFragmentArgument(): FragmentArgumentNode { + const start = this._lexer.token; + const name = this.parseName(); + + this.expectToken(TokenKind.COLON); + return this.node(start, { + kind: Kind.FRAGMENT_ARGUMENT, + name, + value: this.parseValueLiteral(false), + }); + } + // Implements the parsing rules in the Fragments section. /** @@ -572,7 +592,7 @@ export class Parser { return this.node(start, { kind: Kind.FRAGMENT_SPREAD, name, - arguments: this.parseArguments(false), + arguments: this.parseFragmentArguments(), directives: this.parseDirectives(false), }); } diff --git a/src/language/printer.ts b/src/language/printer.ts index bd82672735..9889e41bd8 100644 --- a/src/language/printer.ts +++ b/src/language/printer.ts @@ -76,6 +76,7 @@ const printDocASTReducer: ASTReducer = { }, }, Argument: { leave: ({ name, value }) => name + ': ' + value }, + FragmentArgument: { leave: ({ name, value }) => name + ': ' + value }, // Nullability Modifiers From f9a1a693d9ff3dddeb438030b961541b7ea6e314 Mon Sep 17 00:00:00 2001 From: Jovi De Croock Date: Wed, 4 Sep 2024 07:52:53 +0200 Subject: [PATCH 3/5] feat: execution for fragment arguments syntax (#2) * implement execution for fragment arguments syntax Co-authored-by: mjmahone * add directive test (#9) * add directive test * add failing test add additional nested fragment test (#8) Correct test and lint stuff suggestions for execution (#11) * introduce internal getVariableSignature utility now extracted also to graphql-js PR, see https://github.com/graphql/graphql-js/pull/4175 * execution suggestions fixes execution to always use fragment variable when has the same name as an operation variable previously, we were allowing an operation variable to be used if the fragment variable was not provided, and the field had no default. Now, we still use the fragment variable, and so the value is null. this now correct logic allows us to significantly reduce the diff from main adds additional test * move getVariableSignature to execution as it cannot be used by validation, which must collect all errors rather than fail with invalid type for signature --------- Co-authored-by: mjmahone Co-authored-by: Yaacov Rydzinski --- src/execution/__tests__/variables-test.ts | 403 +++++++++++++++++- src/execution/collectFields.ts | 78 +++- src/execution/execute.ts | 34 +- src/execution/getVariableSignature.ts | 46 ++ src/execution/values.ts | 63 ++- src/utilities/valueFromAST.ts | 39 +- .../rules/SingleFieldSubscriptionsRule.ts | 15 +- 7 files changed, 615 insertions(+), 63 deletions(-) create mode 100644 src/execution/getVariableSignature.ts diff --git a/src/execution/__tests__/variables-test.ts b/src/execution/__tests__/variables-test.ts index 6e4b39e810..ac810c4eeb 100644 --- a/src/execution/__tests__/variables-test.ts +++ b/src/execution/__tests__/variables-test.ts @@ -7,6 +7,7 @@ import { inspect } from '../../jsutils/inspect.js'; import { GraphQLError } from '../../error/GraphQLError.js'; +import { DirectiveLocation } from '../../language/directiveLocation.js'; import { Kind } from '../../language/kinds.js'; import { parse } from '../../language/parser.js'; @@ -22,10 +23,14 @@ import { GraphQLObjectType, GraphQLScalarType, } from '../../type/definition.js'; -import { GraphQLString } from '../../type/scalars.js'; +import { + GraphQLDirective, + GraphQLIncludeDirective, +} from '../../type/directives.js'; +import { GraphQLBoolean, GraphQLString } from '../../type/scalars.js'; import { GraphQLSchema } from '../../type/schema.js'; -import { executeSync } from '../execute.js'; +import { executeSync, experimentalExecuteIncrementally } from '../execute.js'; import { getVariableValues } from '../values.js'; const TestFaultyScalarGraphQLError = new GraphQLError( @@ -59,6 +64,13 @@ const TestComplexScalar = new GraphQLScalarType({ }, }); +const NestedType: GraphQLObjectType = new GraphQLObjectType({ + name: 'NestedType', + fields: { + echo: fieldWithInputArg({ type: GraphQLString }), + }, +}); + const TestInputObject = new GraphQLInputObjectType({ name: 'TestInputObject', fields: { @@ -129,6 +141,10 @@ const TestType = new GraphQLObjectType({ defaultValue: 'Hello World', }), list: fieldWithInputArg({ type: new GraphQLList(GraphQLString) }), + nested: { + type: NestedType, + resolve: () => ({}), + }, nnList: fieldWithInputArg({ type: new GraphQLNonNull(new GraphQLList(GraphQLString)), }), @@ -143,7 +159,30 @@ const TestType = new GraphQLObjectType({ }, }); -const schema = new GraphQLSchema({ query: TestType }); +const schema = new GraphQLSchema({ + query: TestType, + directives: [ + new GraphQLDirective({ + name: 'skip', + description: + 'Directs the executor to skip this field or fragment when the `if` argument is true.', + locations: [ + DirectiveLocation.FIELD, + DirectiveLocation.FRAGMENT_SPREAD, + DirectiveLocation.INLINE_FRAGMENT, + ], + args: { + if: { + type: new GraphQLNonNull(GraphQLBoolean), + description: 'Skipped when true.', + // default values will override operation variables in the setting of defined fragment variables that are not provided + defaultValue: true, + }, + }, + }), + GraphQLIncludeDirective, + ], +}); function executeQuery( query: string, @@ -153,6 +192,14 @@ function executeQuery( return executeSync({ schema, document, variableValues }); } +function executeQueryWithFragmentArguments( + query: string, + variableValues?: { [variable: string]: unknown }, +) { + const document = parse(query, { experimentalFragmentArguments: true }); + return executeSync({ schema, document, variableValues }); +} + describe('Execute: Handles inputs', () => { describe('Handles objects and nullability', () => { describe('using inline structs', () => { @@ -1136,4 +1183,354 @@ describe('Execute: Handles inputs', () => { }); }); }); + + describe('using fragment arguments', () => { + it('when there are no fragment arguments', () => { + const result = executeQueryWithFragmentArguments(` + query { + ...a + } + fragment a on TestType { + fieldWithNonNullableStringInput(input: "A") + } + `); + expect(result).to.deep.equal({ + data: { + fieldWithNonNullableStringInput: '"A"', + }, + }); + }); + + it('when a value is required and provided', () => { + const result = executeQueryWithFragmentArguments(` + query { + ...a(value: "A") + } + fragment a($value: String!) on TestType { + fieldWithNonNullableStringInput(input: $value) + } + `); + expect(result).to.deep.equal({ + data: { + fieldWithNonNullableStringInput: '"A"', + }, + }); + }); + + it('when a value is required and not provided', () => { + const result = executeQueryWithFragmentArguments(` + query { + ...a + } + fragment a($value: String!) on TestType { + fieldWithNullableStringInput(input: $value) + } + `); + + expect(result).to.have.property('errors'); + expect(result.errors).to.have.length(1); + expect(result.errors?.at(0)?.message).to.match( + /Argument "value" of required type "String!"/, + ); + }); + + it('when the definition has a default and is provided', () => { + const result = executeQueryWithFragmentArguments(` + query { + ...a(value: "A") + } + fragment a($value: String! = "B") on TestType { + fieldWithNonNullableStringInput(input: $value) + } + `); + expect(result).to.deep.equal({ + data: { + fieldWithNonNullableStringInput: '"A"', + }, + }); + }); + + it('when the definition has a default and is not provided', () => { + const result = executeQueryWithFragmentArguments(` + query { + ...a + } + fragment a($value: String! = "B") on TestType { + fieldWithNonNullableStringInput(input: $value) + } + `); + expect(result).to.deep.equal({ + data: { + fieldWithNonNullableStringInput: '"B"', + }, + }); + }); + + it('when a definition has a default, is not provided, and spreads another fragment', () => { + const result = executeQueryWithFragmentArguments(` + query { + ...a + } + fragment a($a: String! = "B") on TestType { + ...b(b: $a) + } + fragment b($b: String!) on TestType { + fieldWithNonNullableStringInput(input: $b) + } + `); + expect(result).to.deep.equal({ + data: { + fieldWithNonNullableStringInput: '"B"', + }, + }); + }); + + it('when the definition has a non-nullable default and is provided null', () => { + const result = executeQueryWithFragmentArguments(` + query { + ...a(value: null) + } + fragment a($value: String! = "B") on TestType { + fieldWithNullableStringInput(input: $value) + } + `); + + expect(result).to.have.property('errors'); + expect(result.errors).to.have.length(1); + expect(result.errors?.at(0)?.message).to.match( + /Argument "value" of non-null type "String!"/, + ); + }); + + it('when the definition has no default and is not provided', () => { + const result = executeQueryWithFragmentArguments(` + query { + ...a + } + fragment a($value: String) on TestType { + fieldWithNonNullableStringInputAndDefaultArgumentValue(input: $value) + } + `); + expect(result).to.deep.equal({ + data: { + fieldWithNonNullableStringInputAndDefaultArgumentValue: + '"Hello World"', + }, + }); + }); + + it('when an argument is shadowed by an operation variable', () => { + const result = executeQueryWithFragmentArguments(` + query($x: String! = "A") { + ...a(x: "B") + } + fragment a($x: String) on TestType { + fieldWithNullableStringInput(input: $x) + } + `); + expect(result).to.deep.equal({ + data: { + fieldWithNullableStringInput: '"B"', + }, + }); + }); + + it('when a nullable argument without a field default is not provided and shadowed by an operation variable', () => { + const result = executeQueryWithFragmentArguments(` + query($x: String = "A") { + ...a + } + fragment a($x: String) on TestType { + fieldWithNullableStringInput(input: $x) + } + `); + expect(result).to.deep.equal({ + data: { + fieldWithNullableStringInput: null, + }, + }); + }); + + it('when a nullable argument with a field default is not provided and shadowed by an operation variable', () => { + const result = executeQueryWithFragmentArguments(` + query($x: String = "A") { + ...a + } + fragment a($x: String) on TestType { + fieldWithNonNullableStringInputAndDefaultArgumentValue(input: $x) + } + `); + expect(result).to.deep.equal({ + data: { + fieldWithNonNullableStringInputAndDefaultArgumentValue: + '"Hello World"', + }, + }); + }); + + it('when a fragment-variable is shadowed by an intermediate fragment-spread but defined in the operation-variables', () => { + const result = executeQueryWithFragmentArguments(` + query($x: String = "A") { + ...a + } + fragment a($x: String) on TestType { + ...b + } + + fragment b on TestType { + fieldWithNullableStringInput(input: $x) + } + `); + expect(result).to.deep.equal({ + data: { + fieldWithNullableStringInput: '"A"', + }, + }); + }); + + it('when a fragment is used with different args', () => { + const result = executeQueryWithFragmentArguments(` + query($x: String = "Hello") { + a: nested { + ...a(x: "a") + } + b: nested { + ...a(x: "b", b: true) + } + hello: nested { + ...a(x: $x) + } + } + fragment a($x: String, $b: Boolean = false) on NestedType { + a: echo(input: $x) @skip(if: $b) + b: echo(input: $x) @include(if: $b) + } + `); + expect(result).to.deep.equal({ + data: { + a: { + a: '"a"', + }, + b: { + b: '"b"', + }, + hello: { + a: '"Hello"', + }, + }, + }); + }); + + it('when the argument variable is nested in a complex type', () => { + const result = executeQueryWithFragmentArguments(` + query { + ...a(value: "C") + } + fragment a($value: String) on TestType { + list(input: ["A", "B", $value, "D"]) + } + `); + expect(result).to.deep.equal({ + data: { + list: '["A", "B", "C", "D"]', + }, + }); + }); + + it('when argument variables are used recursively', () => { + const result = executeQueryWithFragmentArguments(` + query { + ...a(aValue: "C") + } + fragment a($aValue: String) on TestType { + ...b(bValue: $aValue) + } + fragment b($bValue: String) on TestType { + list(input: ["A", "B", $bValue, "D"]) + } + `); + expect(result).to.deep.equal({ + data: { + list: '["A", "B", "C", "D"]', + }, + }); + }); + + it('when argument variables with the same name are used directly and recursively', () => { + const result = executeQueryWithFragmentArguments(` + query { + ...a(value: "A") + } + fragment a($value: String!) on TestType { + ...b(value: "B") + fieldInFragmentA: fieldWithNonNullableStringInput(input: $value) + } + fragment b($value: String!) on TestType { + fieldInFragmentB: fieldWithNonNullableStringInput(input: $value) + } + `); + expect(result).to.deep.equal({ + data: { + fieldInFragmentA: '"A"', + fieldInFragmentB: '"B"', + }, + }); + }); + + it('when argument passed in as list', () => { + const result = executeQueryWithFragmentArguments(` + query Q($opValue: String = "op") { + ...a(aValue: "A") + } + fragment a($aValue: String, $bValue: String) on TestType { + ...b(aValue: [$aValue, "B"], bValue: [$bValue, $opValue]) + } + fragment b($aValue: [String], $bValue: [String], $cValue: String) on TestType { + aList: list(input: $aValue) + bList: list(input: $bValue) + cList: list(input: [$cValue]) + } + `); + expect(result).to.deep.equal({ + data: { + aList: '["A", "B"]', + bList: '[null, "op"]', + cList: '[null]', + }, + }); + }); + + it('when argument passed to a directive', () => { + const result = executeQueryWithFragmentArguments(` + query { + ...a(value: true) + } + fragment a($value: Boolean!) on TestType { + fieldWithNonNullableStringInput @skip(if: $value) + } + `); + expect(result).to.deep.equal({ + data: {}, + }); + }); + + it('when a nullable argument to a directive with a field default is not provided and shadowed by an operation variable', () => { + // this test uses the @defer directive and incremental delivery because the `if` argument for skip/include have no field defaults + const document = parse( + ` + query($shouldDefer: Boolean = false) { + ...a + } + fragment a($shouldDefer: Boolean) on TestType { + ... @defer(if: $shouldDefer) { + fieldWithDefaultArgumentValue + } + } + `, + { experimentalFragmentArguments: true }, + ); + const result = experimentalExecuteIncrementally({ schema, document }); + expect(result).to.include.keys('initialResult', 'subsequentResults'); + }); + }); }); diff --git a/src/execution/collectFields.ts b/src/execution/collectFields.ts index d411ff3f77..94dd2b50bd 100644 --- a/src/execution/collectFields.ts +++ b/src/execution/collectFields.ts @@ -24,26 +24,39 @@ import type { GraphQLSchema } from '../type/schema.js'; import { typeFromAST } from '../utilities/typeFromAST.js'; -import { getDirectiveValues } from './values.js'; +import type { GraphQLVariableSignature } from './getVariableSignature.js'; +import { experimentalGetArgumentValues, getDirectiveValues } from './values.js'; export interface DeferUsage { label: string | undefined; parentDeferUsage: DeferUsage | undefined; } +export interface FragmentVariables { + signatures: ObjMap; + values: ObjMap; +} + export interface FieldDetails { node: FieldNode; - deferUsage: DeferUsage | undefined; + deferUsage?: DeferUsage | undefined; + fragmentVariables?: FragmentVariables | undefined; } export type FieldGroup = ReadonlyArray; export type GroupedFieldSet = ReadonlyMap; +export interface FragmentDetails { + definition: FragmentDefinitionNode; + variableSignatures?: ObjMap | undefined; +} + interface CollectFieldsContext { schema: GraphQLSchema; - fragments: ObjMap; + fragments: ObjMap; variableValues: { [variable: string]: unknown }; + fragmentVariableValues?: FragmentVariables; operation: OperationDefinitionNode; runtimeType: GraphQLObjectType; visitedFragmentNames: Set; @@ -60,7 +73,7 @@ interface CollectFieldsContext { */ export function collectFields( schema: GraphQLSchema, - fragments: ObjMap, + fragments: ObjMap, variableValues: { [variable: string]: unknown }, runtimeType: GraphQLObjectType, operation: OperationDefinitionNode, @@ -101,7 +114,7 @@ export function collectFields( // eslint-disable-next-line max-params export function collectSubfields( schema: GraphQLSchema, - fragments: ObjMap, + fragments: ObjMap, variableValues: { [variable: string]: unknown }, operation: OperationDefinitionNode, returnType: GraphQLObjectType, @@ -140,12 +153,14 @@ export function collectSubfields( }; } +// eslint-disable-next-line max-params function collectFieldsImpl( context: CollectFieldsContext, selectionSet: SelectionSetNode, groupedFieldSet: AccumulatorMap, newDeferUsages: Array, deferUsage?: DeferUsage, + fragmentVariables?: FragmentVariables, ): void { const { schema, @@ -159,18 +174,19 @@ function collectFieldsImpl( for (const selection of selectionSet.selections) { switch (selection.kind) { case Kind.FIELD: { - if (!shouldIncludeNode(variableValues, selection)) { + if (!shouldIncludeNode(selection, variableValues, fragmentVariables)) { continue; } groupedFieldSet.add(getFieldEntryKey(selection), { node: selection, deferUsage, + fragmentVariables, }); break; } case Kind.INLINE_FRAGMENT: { if ( - !shouldIncludeNode(variableValues, selection) || + !shouldIncludeNode(selection, variableValues, fragmentVariables) || !doesFragmentConditionMatch(schema, selection, runtimeType) ) { continue; @@ -179,6 +195,7 @@ function collectFieldsImpl( const newDeferUsage = getDeferUsage( operation, variableValues, + fragmentVariables, selection, deferUsage, ); @@ -190,6 +207,7 @@ function collectFieldsImpl( groupedFieldSet, newDeferUsages, deferUsage, + fragmentVariables, ); } else { newDeferUsages.push(newDeferUsage); @@ -199,6 +217,7 @@ function collectFieldsImpl( groupedFieldSet, newDeferUsages, newDeferUsage, + fragmentVariables, ); } @@ -210,6 +229,7 @@ function collectFieldsImpl( const newDeferUsage = getDeferUsage( operation, variableValues, + fragmentVariables, selection, deferUsage, ); @@ -217,7 +237,7 @@ function collectFieldsImpl( if ( !newDeferUsage && (visitedFragmentNames.has(fragName) || - !shouldIncludeNode(variableValues, selection)) + !shouldIncludeNode(selection, variableValues, fragmentVariables)) ) { continue; } @@ -225,27 +245,44 @@ function collectFieldsImpl( const fragment = fragments[fragName]; if ( fragment == null || - !doesFragmentConditionMatch(schema, fragment, runtimeType) + !doesFragmentConditionMatch(schema, fragment.definition, runtimeType) ) { continue; } + + const fragmentVariableSignatures = fragment.variableSignatures; + let newFragmentVariables: FragmentVariables | undefined; + if (fragmentVariableSignatures) { + newFragmentVariables = { + signatures: fragmentVariableSignatures, + values: experimentalGetArgumentValues( + selection, + Object.values(fragmentVariableSignatures), + variableValues, + fragmentVariables, + ), + }; + } + if (!newDeferUsage) { visitedFragmentNames.add(fragName); collectFieldsImpl( context, - fragment.selectionSet, + fragment.definition.selectionSet, groupedFieldSet, newDeferUsages, deferUsage, + newFragmentVariables, ); } else { newDeferUsages.push(newDeferUsage); collectFieldsImpl( context, - fragment.selectionSet, + fragment.definition.selectionSet, groupedFieldSet, newDeferUsages, newDeferUsage, + newFragmentVariables, ); } break; @@ -262,10 +299,16 @@ function collectFieldsImpl( function getDeferUsage( operation: OperationDefinitionNode, variableValues: { [variable: string]: unknown }, + fragmentVariables: FragmentVariables | undefined, node: FragmentSpreadNode | InlineFragmentNode, parentDeferUsage: DeferUsage | undefined, ): DeferUsage | undefined { - const defer = getDirectiveValues(GraphQLDeferDirective, node, variableValues); + const defer = getDirectiveValues( + GraphQLDeferDirective, + node, + variableValues, + fragmentVariables, + ); if (!defer) { return; @@ -291,10 +334,16 @@ function getDeferUsage( * directives, where `@skip` has higher precedence than `@include`. */ function shouldIncludeNode( - variableValues: { [variable: string]: unknown }, node: FragmentSpreadNode | FieldNode | InlineFragmentNode, + variableValues: { [variable: string]: unknown }, + fragmentVariables: FragmentVariables | undefined, ): boolean { - const skip = getDirectiveValues(GraphQLSkipDirective, node, variableValues); + const skip = getDirectiveValues( + GraphQLSkipDirective, + node, + variableValues, + fragmentVariables, + ); if (skip?.if === true) { return false; } @@ -303,6 +352,7 @@ function shouldIncludeNode( GraphQLIncludeDirective, node, variableValues, + fragmentVariables, ); if (include?.if === false) { return false; diff --git a/src/execution/execute.ts b/src/execution/execute.ts index 99582b828d..14798f96f0 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -5,6 +5,7 @@ import { isAsyncIterable } from '../jsutils/isAsyncIterable.js'; import { isIterableObject } from '../jsutils/isIterableObject.js'; import { isObjectLike } from '../jsutils/isObjectLike.js'; import { isPromise } from '../jsutils/isPromise.js'; +import { mapValue } from '../jsutils/mapValue.js'; import type { Maybe } from '../jsutils/Maybe.js'; import { memoize3 } from '../jsutils/memoize3.js'; import type { ObjMap } from '../jsutils/ObjMap.js'; @@ -20,7 +21,6 @@ import { locatedError } from '../error/locatedError.js'; import type { DocumentNode, FieldNode, - FragmentDefinitionNode, OperationDefinitionNode, } from '../language/ast.js'; import { OperationTypeNode } from '../language/ast.js'; @@ -53,12 +53,14 @@ import { buildExecutionPlan } from './buildExecutionPlan.js'; import type { DeferUsage, FieldGroup, + FragmentDetails, GroupedFieldSet, } from './collectFields.js'; import { collectFields, collectSubfields as _collectSubfields, } from './collectFields.js'; +import { getVariableSignature } from './getVariableSignature.js'; import { buildIncrementalResponse } from './IncrementalPublisher.js'; import { mapAsyncIterable } from './mapAsyncIterable.js'; import type { @@ -74,6 +76,7 @@ import type { } from './types.js'; import { DeferredFragmentRecord } from './types.js'; import { + experimentalGetArgumentValues, getArgumentValues, getDirectiveValues, getVariableValues, @@ -132,7 +135,7 @@ const collectSubfields = memoize3( */ export interface ExecutionContext { schema: GraphQLSchema; - fragments: ObjMap; + fragments: ObjMap; rootValue: unknown; contextValue: unknown; operation: OperationDefinitionNode; @@ -445,7 +448,7 @@ export function buildExecutionContext( assertValidSchema(schema); let operation: OperationDefinitionNode | undefined; - const fragments: ObjMap = Object.create(null); + const fragments: ObjMap = Object.create(null); for (const definition of document.definitions) { switch (definition.kind) { case Kind.OPERATION_DEFINITION: @@ -462,9 +465,18 @@ export function buildExecutionContext( operation = definition; } break; - case Kind.FRAGMENT_DEFINITION: - fragments[definition.name.value] = definition; + case Kind.FRAGMENT_DEFINITION: { + let variableSignatures; + if (definition.variableDefinitions) { + variableSignatures = Object.create(null); + for (const varDef of definition.variableDefinitions) { + const signature = getVariableSignature(schema, varDef); + variableSignatures[signature.name] = signature; + } + } + fragments[definition.name.value] = { definition, variableSignatures }; break; + } default: // ignore non-executable definitions } @@ -720,10 +732,11 @@ function executeField( // Build a JS object of arguments from the field.arguments AST, using the // variables scope to fulfill any variable references. // TODO: find a way to memoize, in case this field is within a List type. - const args = getArgumentValues( - fieldDef, + const args = experimentalGetArgumentValues( fieldGroup[0].node, + fieldDef.args, exeContext.variableValues, + fieldGroup[0].fragmentVariables, ); // The resolve function's optional third argument is a context value that @@ -806,7 +819,10 @@ export function buildResolveInfo( parentType, path, schema: exeContext.schema, - fragments: exeContext.fragments, + fragments: mapValue( + exeContext.fragments, + (fragment) => fragment.definition, + ), rootValue: exeContext.rootValue, operation: exeContext.operation, variableValues: exeContext.variableValues, @@ -1029,6 +1045,7 @@ function getStreamUsage( GraphQLStreamDirective, fieldGroup[0].node, exeContext.variableValues, + fieldGroup[0].fragmentVariables, ); if (!stream) { @@ -1057,6 +1074,7 @@ function getStreamUsage( const streamedFieldGroup: FieldGroup = fieldGroup.map((fieldDetails) => ({ node: fieldDetails.node, deferUsage: undefined, + fragmentVariables: fieldDetails.fragmentVariables, })); const streamUsage = { diff --git a/src/execution/getVariableSignature.ts b/src/execution/getVariableSignature.ts new file mode 100644 index 0000000000..984b816c9b --- /dev/null +++ b/src/execution/getVariableSignature.ts @@ -0,0 +1,46 @@ +import { GraphQLError } from '../error/GraphQLError.js'; + +import type { VariableDefinitionNode } from '../language/ast.js'; +import { print } from '../language/printer.js'; + +import { isInputType } from '../type/definition.js'; +import type { GraphQLInputType, GraphQLSchema } from '../type/index.js'; + +import { typeFromAST } from '../utilities/typeFromAST.js'; +import { valueFromAST } from '../utilities/valueFromAST.js'; + +/** + * A GraphQLVariableSignature is required to coerce a variable value. + * + * Designed to have comparable interface to GraphQLArgument so that + * getArgumentValues() can be reused for fragment arguments. + * */ +export interface GraphQLVariableSignature { + name: string; + type: GraphQLInputType; + defaultValue: unknown; +} + +export function getVariableSignature( + schema: GraphQLSchema, + varDefNode: VariableDefinitionNode, +): GraphQLVariableSignature | GraphQLError { + const varName = varDefNode.variable.name.value; + const varType = typeFromAST(schema, varDefNode.type); + + if (!isInputType(varType)) { + // Must use input types for variables. This should be caught during + // validation, however is checked again here for safety. + const varTypeStr = print(varDefNode.type); + return new GraphQLError( + `Variable "$${varName}" expected value of type "${varTypeStr}" which cannot be used as an input type.`, + { nodes: varDefNode.type }, + ); + } + + return { + name: varName, + type: varType, + defaultValue: valueFromAST(varDefNode.defaultValue, varType), + }; +} diff --git a/src/execution/values.ts b/src/execution/values.ts index 5511911c78..e3fa23066a 100644 --- a/src/execution/values.ts +++ b/src/execution/values.ts @@ -8,20 +8,24 @@ import { GraphQLError } from '../error/GraphQLError.js'; import type { DirectiveNode, FieldNode, + FragmentSpreadNode, VariableDefinitionNode, } from '../language/ast.js'; import { Kind } from '../language/kinds.js'; import { print } from '../language/printer.js'; -import type { GraphQLField } from '../type/definition.js'; -import { isInputType, isNonNullType } from '../type/definition.js'; +import type { GraphQLArgument, GraphQLField } from '../type/definition.js'; +import { isNonNullType } from '../type/definition.js'; import type { GraphQLDirective } from '../type/directives.js'; import type { GraphQLSchema } from '../type/schema.js'; import { coerceInputValue } from '../utilities/coerceInputValue.js'; -import { typeFromAST } from '../utilities/typeFromAST.js'; import { valueFromAST } from '../utilities/valueFromAST.js'; +import type { FragmentVariables } from './collectFields.js'; +import type { GraphQLVariableSignature } from './getVariableSignature.js'; +import { getVariableSignature } from './getVariableSignature.js'; + type CoercedVariableValues = | { errors: ReadonlyArray; coerced?: never } | { coerced: { [variable: string]: unknown }; errors?: never }; @@ -76,24 +80,16 @@ function coerceVariableValues( ): { [variable: string]: unknown } { const coercedValues: { [variable: string]: unknown } = {}; for (const varDefNode of varDefNodes) { - const varName = varDefNode.variable.name.value; - const varType = typeFromAST(schema, varDefNode.type); - if (!isInputType(varType)) { - // Must use input types for variables. This should be caught during - // validation, however is checked again here for safety. - const varTypeStr = print(varDefNode.type); - onError( - new GraphQLError( - `Variable "$${varName}" expected value of type "${varTypeStr}" which cannot be used as an input type.`, - { nodes: varDefNode.type }, - ), - ); + const varSignature = getVariableSignature(schema, varDefNode); + if (varSignature instanceof GraphQLError) { + onError(varSignature); continue; } + const { name: varName, type: varType } = varSignature; if (!Object.hasOwn(inputs, varName)) { if (varDefNode.defaultValue) { - coercedValues[varName] = valueFromAST(varDefNode.defaultValue, varType); + coercedValues[varName] = varSignature.defaultValue; } else if (isNonNullType(varType)) { const varTypeStr = inspect(varType); onError( @@ -152,6 +148,15 @@ export function getArgumentValues( def: GraphQLField | GraphQLDirective, node: FieldNode | DirectiveNode, variableValues?: Maybe>, +): { [argument: string]: unknown } { + return experimentalGetArgumentValues(node, def.args, variableValues); +} + +export function experimentalGetArgumentValues( + node: FieldNode | DirectiveNode | FragmentSpreadNode, + argDefs: ReadonlyArray, + variableValues: Maybe>, + fragmentVariables?: Maybe, ): { [argument: string]: unknown } { const coercedValues: { [argument: string]: unknown } = {}; @@ -160,7 +165,7 @@ export function getArgumentValues( const argumentNodes = node.arguments ?? []; const argNodeMap = new Map(argumentNodes.map((arg) => [arg.name.value, arg])); - for (const argDef of def.args) { + for (const argDef of argDefs) { const name = argDef.name; const argType = argDef.type; const argumentNode = argNodeMap.get(name); @@ -183,9 +188,12 @@ export function getArgumentValues( if (valueNode.kind === Kind.VARIABLE) { const variableName = valueNode.name.value; + const scopedVariableValues = fragmentVariables?.signatures[variableName] + ? fragmentVariables.values + : variableValues; if ( - variableValues == null || - !Object.hasOwn(variableValues, variableName) + scopedVariableValues == null || + !Object.hasOwn(scopedVariableValues, variableName) ) { if (argDef.defaultValue !== undefined) { coercedValues[name] = argDef.defaultValue; @@ -198,7 +206,7 @@ export function getArgumentValues( } continue; } - isNull = variableValues[variableName] == null; + isNull = scopedVariableValues[variableName] == null; } if (isNull && isNonNullType(argType)) { @@ -209,7 +217,12 @@ export function getArgumentValues( ); } - const coercedValue = valueFromAST(valueNode, argType, variableValues); + const coercedValue = valueFromAST( + valueNode, + argType, + variableValues, + fragmentVariables?.values, + ); if (coercedValue === undefined) { // Note: ValuesOfCorrectTypeRule validation should catch this before // execution. This is a runtime check to ensure execution does not @@ -239,12 +252,18 @@ export function getDirectiveValues( directiveDef: GraphQLDirective, node: { readonly directives?: ReadonlyArray | undefined }, variableValues?: Maybe>, + fragmentVariables?: Maybe, ): undefined | { [argument: string]: unknown } { const directiveNode = node.directives?.find( (directive) => directive.name.value === directiveDef.name, ); if (directiveNode) { - return getArgumentValues(directiveDef, directiveNode, variableValues); + return experimentalGetArgumentValues( + directiveNode, + directiveDef.args, + variableValues, + fragmentVariables, + ); } } diff --git a/src/utilities/valueFromAST.ts b/src/utilities/valueFromAST.ts index 5e0d3c517e..3aec3f272f 100644 --- a/src/utilities/valueFromAST.ts +++ b/src/utilities/valueFromAST.ts @@ -38,6 +38,7 @@ export function valueFromAST( valueNode: Maybe, type: GraphQLInputType, variables?: Maybe>, + fragmentVariables?: Maybe>, ): unknown { if (!valueNode) { // When there is no node, then there is also no value. @@ -47,11 +48,12 @@ export function valueFromAST( if (valueNode.kind === Kind.VARIABLE) { const variableName = valueNode.name.value; - if (variables == null || variables[variableName] === undefined) { + const variableValue = + fragmentVariables?.[variableName] ?? variables?.[variableName]; + if (variableValue === undefined) { // No valid return value. return; } - const variableValue = variables[variableName]; if (variableValue === null && isNonNullType(type)) { return; // Invalid: intentionally return no value. } @@ -65,7 +67,7 @@ export function valueFromAST( if (valueNode.kind === Kind.NULL) { return; // Invalid: intentionally return no value. } - return valueFromAST(valueNode, type.ofType, variables); + return valueFromAST(valueNode, type.ofType, variables, fragmentVariables); } if (valueNode.kind === Kind.NULL) { @@ -78,7 +80,7 @@ export function valueFromAST( if (valueNode.kind === Kind.LIST) { const coercedValues = []; for (const itemNode of valueNode.values) { - if (isMissingVariable(itemNode, variables)) { + if (isMissingVariable(itemNode, variables, fragmentVariables)) { // If an array contains a missing variable, it is either coerced to // null or if the item type is non-null, it considered invalid. if (isNonNullType(itemType)) { @@ -86,7 +88,12 @@ export function valueFromAST( } coercedValues.push(null); } else { - const itemValue = valueFromAST(itemNode, itemType, variables); + const itemValue = valueFromAST( + itemNode, + itemType, + variables, + fragmentVariables, + ); if (itemValue === undefined) { return; // Invalid: intentionally return no value. } @@ -95,7 +102,12 @@ export function valueFromAST( } return coercedValues; } - const coercedValue = valueFromAST(valueNode, itemType, variables); + const coercedValue = valueFromAST( + valueNode, + itemType, + variables, + fragmentVariables, + ); if (coercedValue === undefined) { return; // Invalid: intentionally return no value. } @@ -112,7 +124,10 @@ export function valueFromAST( ); for (const field of Object.values(type.getFields())) { const fieldNode = fieldNodes.get(field.name); - if (fieldNode == null || isMissingVariable(fieldNode.value, variables)) { + if ( + fieldNode == null || + isMissingVariable(fieldNode.value, variables, fragmentVariables) + ) { if (field.defaultValue !== undefined) { coercedObj[field.name] = field.defaultValue; } else if (isNonNullType(field.type)) { @@ -120,7 +135,12 @@ export function valueFromAST( } continue; } - const fieldValue = valueFromAST(fieldNode.value, field.type, variables); + const fieldValue = valueFromAST( + fieldNode.value, + field.type, + variables, + fragmentVariables, + ); if (fieldValue === undefined) { return; // Invalid: intentionally return no value. } @@ -166,9 +186,12 @@ export function valueFromAST( function isMissingVariable( valueNode: ValueNode, variables: Maybe>, + fragmentVariables: Maybe>, ): boolean { return ( valueNode.kind === Kind.VARIABLE && + (fragmentVariables == null || + fragmentVariables[valueNode.name.value] === undefined) && (variables == null || variables[valueNode.name.value] === undefined) ); } diff --git a/src/validation/rules/SingleFieldSubscriptionsRule.ts b/src/validation/rules/SingleFieldSubscriptionsRule.ts index 700bc0bda7..dfefc6cdd8 100644 --- a/src/validation/rules/SingleFieldSubscriptionsRule.ts +++ b/src/validation/rules/SingleFieldSubscriptionsRule.ts @@ -2,15 +2,14 @@ import type { ObjMap } from '../../jsutils/ObjMap.js'; import { GraphQLError } from '../../error/GraphQLError.js'; -import type { - FieldNode, - FragmentDefinitionNode, - OperationDefinitionNode, -} from '../../language/ast.js'; +import type { FieldNode, OperationDefinitionNode } from '../../language/ast.js'; import { Kind } from '../../language/kinds.js'; import type { ASTVisitor } from '../../language/visitor.js'; -import type { FieldGroup } from '../../execution/collectFields.js'; +import type { + FieldGroup, + FragmentDetails, +} from '../../execution/collectFields.js'; import { collectFields } from '../../execution/collectFields.js'; import type { ValidationContext } from '../ValidationContext.js'; @@ -41,10 +40,10 @@ export function SingleFieldSubscriptionsRule( [variable: string]: any; } = Object.create(null); const document = context.getDocument(); - const fragments: ObjMap = Object.create(null); + const fragments: ObjMap = Object.create(null); for (const definition of document.definitions) { if (definition.kind === Kind.FRAGMENT_DEFINITION) { - fragments[definition.name.value] = definition; + fragments[definition.name.value] = { definition }; } } const { groupedFieldSet } = collectFields( From a5627c0a0fa45ea3eafa41b687b51b8089aa518c Mon Sep 17 00:00:00 2001 From: Jovi De Croock Date: Wed, 4 Sep 2024 07:53:02 +0200 Subject: [PATCH 4/5] TypeInfo & Validation for fragment-arguments (#4) * Add typeinfo functionality as a run-up to supporting the new validation rules Co-authored-by: mjmahone * Fragment arguments validation (#5) * Fragment args validation * add experimental substitution * validation suggestions (#12) * validation suggestions * remove OperationSignature from ValidationContext only used in one rule, does not need to be on context remove dependency on getVariableSignature move FRAGMENT_ARGUMENT below ARGUMENT fragment arguments do not have location defaults only variable defaults => so getDefaultValue(), which returns location defaults for use with the allowedVariableUsage helper, should never be called by getVariableUsages with respect to fragment arguments fragment arguments therefore need not add anything to the default value stack reduce diff from main these diffs crept in before Kind.FRAGMENT_ARGUMENT was separated out --------- Co-authored-by: Yaacov Rydzinski * OverlappingFieldsCanBeMergedRule suggestions (#15) --------- Co-authored-by: mjmahone Co-authored-by: Yaacov Rydzinski --- src/utilities/TypeInfo.ts | 98 +++- src/utilities/__tests__/TypeInfo-test.ts | 220 +++++++++ src/validation/ValidationContext.ts | 49 +- .../__tests__/KnownArgumentNamesRule-test.ts | 50 +++ .../NoUndefinedVariablesRule-test.ts | 63 +++ .../NoUnusedFragmentArgumentsRule-test.ts | 42 ++ .../__tests__/NoUnusedVariablesRule-test.ts | 38 ++ .../OverlappingFieldsCanBeMergedRule-test.ts | 235 ++++++++++ .../ProvidedRequiredArgumentsRule-test.ts | 79 ++++ .../__tests__/ValuesOfCorrectTypeRule-test.ts | 31 ++ .../VariablesAreInputTypesRule-test.ts | 27 ++ .../VariablesInAllowedPositionRule-test.ts | 104 +++++ src/validation/index.ts | 3 + .../rules/KnownArgumentNamesRule.ts | 30 +- .../rules/NoUndefinedVariablesRule.ts | 5 +- .../rules/NoUnusedFragmentArgumentsRule.ts | 90 ++++ src/validation/rules/NoUnusedVariablesRule.ts | 32 +- .../rules/OverlappingFieldsCanBeMergedRule.ts | 418 +++++++++++++----- .../rules/ProvidedRequiredArgumentsRule.ts | 47 +- .../rules/VariablesInAllowedPositionRule.ts | 13 +- src/validation/specifiedRules.ts | 3 + 21 files changed, 1539 insertions(+), 138 deletions(-) create mode 100644 src/validation/__tests__/NoUnusedFragmentArgumentsRule-test.ts create mode 100644 src/validation/rules/NoUnusedFragmentArgumentsRule.ts diff --git a/src/utilities/TypeInfo.ts b/src/utilities/TypeInfo.ts index ffcc964b4a..2fd4765c30 100644 --- a/src/utilities/TypeInfo.ts +++ b/src/utilities/TypeInfo.ts @@ -1,6 +1,12 @@ import type { Maybe } from '../jsutils/Maybe.js'; -import type { ASTNode, FieldNode } from '../language/ast.js'; +import type { + ASTNode, + DocumentNode, + FieldNode, + FragmentDefinitionNode, + VariableDefinitionNode, +} from '../language/ast.js'; import { isNode } from '../language/ast.js'; import { Kind } from '../language/kinds.js'; import type { ASTVisitor } from '../language/visitor.js'; @@ -32,6 +38,11 @@ import type { GraphQLSchema } from '../type/schema.js'; import { typeFromAST } from './typeFromAST.js'; +export interface FragmentSignature { + readonly definition: FragmentDefinitionNode; + readonly variableDefinitions: Map; +} + /** * TypeInfo is a utility class which, given a GraphQL schema, can keep track * of the current field and type definitions at any point in a GraphQL document @@ -47,6 +58,12 @@ export class TypeInfo { private _directive: Maybe; private _argument: Maybe; private _enumValue: Maybe; + private _fragmentSignaturesByName: ( + fragmentName: string, + ) => Maybe; + + private _fragmentSignature: Maybe; + private _fragmentArgument: Maybe; private _getFieldDef: GetFieldDefFn; constructor( @@ -58,7 +75,10 @@ export class TypeInfo { initialType?: Maybe, /** @deprecated will be removed in 17.0.0 */ - getFieldDefFn?: GetFieldDefFn, + getFieldDefFn?: Maybe, + fragmentSignatures?: Maybe< + (fragmentName: string) => Maybe + >, ) { this._schema = schema; this._typeStack = []; @@ -69,6 +89,9 @@ export class TypeInfo { this._directive = null; this._argument = null; this._enumValue = null; + this._fragmentSignaturesByName = fragmentSignatures ?? (() => null); + this._fragmentSignature = null; + this._fragmentArgument = null; this._getFieldDef = getFieldDefFn ?? getFieldDef; if (initialType) { if (isInputType(initialType)) { @@ -119,6 +142,20 @@ export class TypeInfo { return this._argument; } + getFragmentSignature(): Maybe { + return this._fragmentSignature; + } + + getFragmentSignatureByName(): ( + fragmentName: string, + ) => Maybe { + return this._fragmentSignaturesByName; + } + + getFragmentArgument(): Maybe { + return this._fragmentArgument; + } + getEnumValue(): Maybe { return this._enumValue; } @@ -130,6 +167,12 @@ export class TypeInfo { // checked before continuing since TypeInfo is used as part of validation // which occurs before guarantees of schema and document validity. switch (node.kind) { + case Kind.DOCUMENT: { + const fragmentSignatures = getFragmentSignatures(node); + this._fragmentSignaturesByName = (fragmentName: string) => + fragmentSignatures.get(fragmentName); + break; + } case Kind.SELECTION_SET: { const namedType: unknown = getNamedType(this.getType()); this._parentTypeStack.push( @@ -159,6 +202,12 @@ export class TypeInfo { this._typeStack.push(isObjectType(rootType) ? rootType : undefined); break; } + case Kind.FRAGMENT_SPREAD: { + this._fragmentSignature = this.getFragmentSignatureByName()( + node.name.value, + ); + break; + } case Kind.INLINE_FRAGMENT: case Kind.FRAGMENT_DEFINITION: { const typeConditionAST = node.typeCondition; @@ -192,6 +241,19 @@ export class TypeInfo { this._inputTypeStack.push(isInputType(argType) ? argType : undefined); break; } + case Kind.FRAGMENT_ARGUMENT: { + const fragmentSignature = this.getFragmentSignature(); + const argDef = fragmentSignature?.variableDefinitions.get( + node.name.value, + ); + this._fragmentArgument = argDef; + let argType: unknown; + if (argDef) { + argType = typeFromAST(this._schema, argDef.type); + } + this._inputTypeStack.push(isInputType(argType) ? argType : undefined); + break; + } case Kind.LIST: { const listType: unknown = getNullableType(this.getInputType()); const itemType: unknown = isListType(listType) @@ -236,6 +298,10 @@ export class TypeInfo { leave(node: ASTNode) { switch (node.kind) { + case Kind.DOCUMENT: + this._fragmentSignaturesByName = /* c8 ignore start */ () => + null /* c8 ignore end */; + break; case Kind.SELECTION_SET: this._parentTypeStack.pop(); break; @@ -246,6 +312,9 @@ export class TypeInfo { case Kind.DIRECTIVE: this._directive = null; break; + case Kind.FRAGMENT_SPREAD: + this._fragmentSignature = null; + break; case Kind.OPERATION_DEFINITION: case Kind.INLINE_FRAGMENT: case Kind.FRAGMENT_DEFINITION: @@ -259,6 +328,12 @@ export class TypeInfo { this._defaultValueStack.pop(); this._inputTypeStack.pop(); break; + case Kind.FRAGMENT_ARGUMENT: { + this._fragmentArgument = null; + this._defaultValueStack.pop(); + this._inputTypeStack.pop(); + break; + } case Kind.LIST: case Kind.OBJECT_FIELD: this._defaultValueStack.pop(); @@ -287,6 +362,25 @@ function getFieldDef( return schema.getField(parentType, fieldNode.name.value); } +function getFragmentSignatures( + document: DocumentNode, +): Map { + const fragmentSignatures = new Map(); + for (const definition of document.definitions) { + if (definition.kind === Kind.FRAGMENT_DEFINITION) { + const variableDefinitions = new Map(); + if (definition.variableDefinitions) { + for (const varDef of definition.variableDefinitions) { + variableDefinitions.set(varDef.variable.name.value, varDef); + } + } + const signature = { definition, variableDefinitions }; + fragmentSignatures.set(definition.name.value, signature); + } + } + return fragmentSignatures; +} + /** * Creates a new visitor instance which maintains a provided TypeInfo instance * along with visiting visitor. diff --git a/src/utilities/__tests__/TypeInfo-test.ts b/src/utilities/__tests__/TypeInfo-test.ts index 971316f8b4..cc129287c3 100644 --- a/src/utilities/__tests__/TypeInfo-test.ts +++ b/src/utilities/__tests__/TypeInfo-test.ts @@ -68,6 +68,9 @@ describe('TypeInfo', () => { expect(typeInfo.getFieldDef()).to.equal(undefined); expect(typeInfo.getDefaultValue()).to.equal(undefined); expect(typeInfo.getDirective()).to.equal(null); + expect(typeInfo.getFragmentSignature()).to.equal(null); + expect(typeInfo.getFragmentSignatureByName()('')).to.equal(null); + expect(typeInfo.getFragmentArgument()).to.equal(null); expect(typeInfo.getArgument()).to.equal(null); expect(typeInfo.getEnumValue()).to.equal(null); }); @@ -515,4 +518,221 @@ describe('visitWithTypeInfo', () => { ['leave', 'SelectionSet', null, 'Human', 'Human'], ]); }); + + it('supports traversals of fragment arguments', () => { + const typeInfo = new TypeInfo(testSchema); + + const ast = parse( + ` + query { + ...Foo(x: 4) + ...Bar + } + fragment Foo( + $x: ID! + ) on QueryRoot { + human(id: $x) { name } + } + `, + { experimentalFragmentArguments: true }, + ); + + const visited: Array = []; + visit( + ast, + visitWithTypeInfo(typeInfo, { + enter(node) { + const type = typeInfo.getType(); + const inputType = typeInfo.getInputType(); + visited.push([ + 'enter', + node.kind, + node.kind === 'Name' ? node.value : null, + String(type), + String(inputType), + ]); + }, + leave(node) { + const type = typeInfo.getType(); + const inputType = typeInfo.getInputType(); + visited.push([ + 'leave', + node.kind, + node.kind === 'Name' ? node.value : null, + String(type), + String(inputType), + ]); + }, + }), + ); + + expect(visited).to.deep.equal([ + ['enter', 'Document', null, 'undefined', 'undefined'], + ['enter', 'OperationDefinition', null, 'QueryRoot', 'undefined'], + ['enter', 'SelectionSet', null, 'QueryRoot', 'undefined'], + ['enter', 'FragmentSpread', null, 'QueryRoot', 'undefined'], + ['enter', 'Name', 'Foo', 'QueryRoot', 'undefined'], + ['leave', 'Name', 'Foo', 'QueryRoot', 'undefined'], + ['enter', 'FragmentArgument', null, 'QueryRoot', 'ID!'], + ['enter', 'Name', 'x', 'QueryRoot', 'ID!'], + ['leave', 'Name', 'x', 'QueryRoot', 'ID!'], + ['enter', 'IntValue', null, 'QueryRoot', 'ID!'], + ['leave', 'IntValue', null, 'QueryRoot', 'ID!'], + ['leave', 'FragmentArgument', null, 'QueryRoot', 'ID!'], + ['leave', 'FragmentSpread', null, 'QueryRoot', 'undefined'], + ['enter', 'FragmentSpread', null, 'QueryRoot', 'undefined'], + ['enter', 'Name', 'Bar', 'QueryRoot', 'undefined'], + ['leave', 'Name', 'Bar', 'QueryRoot', 'undefined'], + ['leave', 'FragmentSpread', null, 'QueryRoot', 'undefined'], + ['leave', 'SelectionSet', null, 'QueryRoot', 'undefined'], + ['leave', 'OperationDefinition', null, 'QueryRoot', 'undefined'], + ['enter', 'FragmentDefinition', null, 'QueryRoot', 'undefined'], + ['enter', 'Name', 'Foo', 'QueryRoot', 'undefined'], + ['leave', 'Name', 'Foo', 'QueryRoot', 'undefined'], + ['enter', 'VariableDefinition', null, 'QueryRoot', 'ID!'], + ['enter', 'Variable', null, 'QueryRoot', 'ID!'], + ['enter', 'Name', 'x', 'QueryRoot', 'ID!'], + ['leave', 'Name', 'x', 'QueryRoot', 'ID!'], + ['leave', 'Variable', null, 'QueryRoot', 'ID!'], + ['enter', 'NonNullType', null, 'QueryRoot', 'ID!'], + ['enter', 'NamedType', null, 'QueryRoot', 'ID!'], + ['enter', 'Name', 'ID', 'QueryRoot', 'ID!'], + ['leave', 'Name', 'ID', 'QueryRoot', 'ID!'], + ['leave', 'NamedType', null, 'QueryRoot', 'ID!'], + ['leave', 'NonNullType', null, 'QueryRoot', 'ID!'], + ['leave', 'VariableDefinition', null, 'QueryRoot', 'ID!'], + ['enter', 'NamedType', null, 'QueryRoot', 'undefined'], + ['enter', 'Name', 'QueryRoot', 'QueryRoot', 'undefined'], + ['leave', 'Name', 'QueryRoot', 'QueryRoot', 'undefined'], + ['leave', 'NamedType', null, 'QueryRoot', 'undefined'], + ['enter', 'SelectionSet', null, 'QueryRoot', 'undefined'], + ['enter', 'Field', null, 'Human', 'undefined'], + ['enter', 'Name', 'human', 'Human', 'undefined'], + ['leave', 'Name', 'human', 'Human', 'undefined'], + ['enter', 'Argument', null, 'Human', 'ID'], + ['enter', 'Name', 'id', 'Human', 'ID'], + ['leave', 'Name', 'id', 'Human', 'ID'], + ['enter', 'Variable', null, 'Human', 'ID'], + ['enter', 'Name', 'x', 'Human', 'ID'], + ['leave', 'Name', 'x', 'Human', 'ID'], + ['leave', 'Variable', null, 'Human', 'ID'], + ['leave', 'Argument', null, 'Human', 'ID'], + ['enter', 'SelectionSet', null, 'Human', 'undefined'], + ['enter', 'Field', null, 'String', 'undefined'], + ['enter', 'Name', 'name', 'String', 'undefined'], + ['leave', 'Name', 'name', 'String', 'undefined'], + ['leave', 'Field', null, 'String', 'undefined'], + ['leave', 'SelectionSet', null, 'Human', 'undefined'], + ['leave', 'Field', null, 'Human', 'undefined'], + ['leave', 'SelectionSet', null, 'QueryRoot', 'undefined'], + ['leave', 'FragmentDefinition', null, 'QueryRoot', 'undefined'], + ['leave', 'Document', null, 'undefined', 'undefined'], + ]); + }); + + it('supports traversals of fragment arguments with default-value', () => { + const typeInfo = new TypeInfo(testSchema); + + const ast = parse( + ` + query { + ...Foo(x: null) + } + fragment Foo( + $x: ID = 4 + ) on QueryRoot { + human(id: $x) { name } + } + `, + { experimentalFragmentArguments: true }, + ); + + const visited: Array = []; + visit( + ast, + visitWithTypeInfo(typeInfo, { + enter(node) { + const type = typeInfo.getType(); + const inputType = typeInfo.getInputType(); + visited.push([ + 'enter', + node.kind, + node.kind === 'Name' ? node.value : null, + String(type), + String(inputType), + ]); + }, + leave(node) { + const type = typeInfo.getType(); + const inputType = typeInfo.getInputType(); + visited.push([ + 'leave', + node.kind, + node.kind === 'Name' ? node.value : null, + String(type), + String(inputType), + ]); + }, + }), + ); + + expect(visited).to.deep.equal([ + ['enter', 'Document', null, 'undefined', 'undefined'], + ['enter', 'OperationDefinition', null, 'QueryRoot', 'undefined'], + ['enter', 'SelectionSet', null, 'QueryRoot', 'undefined'], + ['enter', 'FragmentSpread', null, 'QueryRoot', 'undefined'], + ['enter', 'Name', 'Foo', 'QueryRoot', 'undefined'], + ['leave', 'Name', 'Foo', 'QueryRoot', 'undefined'], + ['enter', 'FragmentArgument', null, 'QueryRoot', 'ID'], + ['enter', 'Name', 'x', 'QueryRoot', 'ID'], + ['leave', 'Name', 'x', 'QueryRoot', 'ID'], + ['enter', 'NullValue', null, 'QueryRoot', 'ID'], + ['leave', 'NullValue', null, 'QueryRoot', 'ID'], + ['leave', 'FragmentArgument', null, 'QueryRoot', 'ID'], + ['leave', 'FragmentSpread', null, 'QueryRoot', 'undefined'], + ['leave', 'SelectionSet', null, 'QueryRoot', 'undefined'], + ['leave', 'OperationDefinition', null, 'QueryRoot', 'undefined'], + ['enter', 'FragmentDefinition', null, 'QueryRoot', 'undefined'], + ['enter', 'Name', 'Foo', 'QueryRoot', 'undefined'], + ['leave', 'Name', 'Foo', 'QueryRoot', 'undefined'], + ['enter', 'VariableDefinition', null, 'QueryRoot', 'ID'], + ['enter', 'Variable', null, 'QueryRoot', 'ID'], + ['enter', 'Name', 'x', 'QueryRoot', 'ID'], + ['leave', 'Name', 'x', 'QueryRoot', 'ID'], + ['leave', 'Variable', null, 'QueryRoot', 'ID'], + ['enter', 'NamedType', null, 'QueryRoot', 'ID'], + ['enter', 'Name', 'ID', 'QueryRoot', 'ID'], + ['leave', 'Name', 'ID', 'QueryRoot', 'ID'], + ['leave', 'NamedType', null, 'QueryRoot', 'ID'], + ['enter', 'IntValue', null, 'QueryRoot', 'ID'], + ['leave', 'IntValue', null, 'QueryRoot', 'ID'], + ['leave', 'VariableDefinition', null, 'QueryRoot', 'ID'], + ['enter', 'NamedType', null, 'QueryRoot', 'undefined'], + ['enter', 'Name', 'QueryRoot', 'QueryRoot', 'undefined'], + ['leave', 'Name', 'QueryRoot', 'QueryRoot', 'undefined'], + ['leave', 'NamedType', null, 'QueryRoot', 'undefined'], + ['enter', 'SelectionSet', null, 'QueryRoot', 'undefined'], + ['enter', 'Field', null, 'Human', 'undefined'], + ['enter', 'Name', 'human', 'Human', 'undefined'], + ['leave', 'Name', 'human', 'Human', 'undefined'], + ['enter', 'Argument', null, 'Human', 'ID'], + ['enter', 'Name', 'id', 'Human', 'ID'], + ['leave', 'Name', 'id', 'Human', 'ID'], + ['enter', 'Variable', null, 'Human', 'ID'], + ['enter', 'Name', 'x', 'Human', 'ID'], + ['leave', 'Name', 'x', 'Human', 'ID'], + ['leave', 'Variable', null, 'Human', 'ID'], + ['leave', 'Argument', null, 'Human', 'ID'], + ['enter', 'SelectionSet', null, 'Human', 'undefined'], + ['enter', 'Field', null, 'String', 'undefined'], + ['enter', 'Name', 'name', 'String', 'undefined'], + ['leave', 'Name', 'name', 'String', 'undefined'], + ['leave', 'Field', null, 'String', 'undefined'], + ['leave', 'SelectionSet', null, 'Human', 'undefined'], + ['leave', 'Field', null, 'Human', 'undefined'], + ['leave', 'SelectionSet', null, 'QueryRoot', 'undefined'], + ['leave', 'FragmentDefinition', null, 'QueryRoot', 'undefined'], + ['leave', 'Document', null, 'undefined', 'undefined'], + ]); + }); }); diff --git a/src/validation/ValidationContext.ts b/src/validation/ValidationContext.ts index b0c5524fa7..7f881fc369 100644 --- a/src/validation/ValidationContext.ts +++ b/src/validation/ValidationContext.ts @@ -9,6 +9,7 @@ import type { FragmentSpreadNode, OperationDefinitionNode, SelectionSetNode, + VariableDefinitionNode, VariableNode, } from '../language/ast.js'; import { Kind } from '../language/kinds.js'; @@ -26,6 +27,7 @@ import type { import type { GraphQLDirective } from '../type/directives.js'; import type { GraphQLSchema } from '../type/schema.js'; +import type { FragmentSignature } from '../utilities/TypeInfo.js'; import { TypeInfo, visitWithTypeInfo } from '../utilities/TypeInfo.js'; type NodeWithSelectionSet = OperationDefinitionNode | FragmentDefinitionNode; @@ -33,6 +35,7 @@ interface VariableUsage { readonly node: VariableNode; readonly type: Maybe; readonly defaultValue: Maybe; + readonly fragmentVariableDefinition: Maybe; } /** @@ -199,17 +202,41 @@ export class ValidationContext extends ASTValidationContext { let usages = this._variableUsages.get(node); if (!usages) { const newUsages: Array = []; - const typeInfo = new TypeInfo(this._schema); + const typeInfo = new TypeInfo( + this._schema, + undefined, + undefined, + this._typeInfo.getFragmentSignatureByName(), + ); + const fragmentDefinition = + node.kind === Kind.FRAGMENT_DEFINITION ? node : undefined; visit( node, visitWithTypeInfo(typeInfo, { VariableDefinition: () => false, Variable(variable) { - newUsages.push({ - node: variable, - type: typeInfo.getInputType(), - defaultValue: typeInfo.getDefaultValue(), - }); + let fragmentVariableDefinition; + if (fragmentDefinition) { + const fragmentSignature = typeInfo.getFragmentSignatureByName()( + fragmentDefinition.name.value, + ); + + fragmentVariableDefinition = + fragmentSignature?.variableDefinitions.get(variable.name.value); + newUsages.push({ + node: variable, + type: typeInfo.getInputType(), + defaultValue: undefined, // fragment variables have a variable default but no location default, which is what this default value represents + fragmentVariableDefinition, + }); + } else { + newUsages.push({ + node: variable, + type: typeInfo.getInputType(), + defaultValue: typeInfo.getDefaultValue(), + fragmentVariableDefinition: undefined, + }); + } }, }), ); @@ -261,6 +288,16 @@ export class ValidationContext extends ASTValidationContext { return this._typeInfo.getArgument(); } + getFragmentSignature(): Maybe { + return this._typeInfo.getFragmentSignature(); + } + + getFragmentSignatureByName(): ( + fragmentName: string, + ) => Maybe { + return this._typeInfo.getFragmentSignatureByName(); + } + getEnumValue(): Maybe { return this._typeInfo.getEnumValue(); } diff --git a/src/validation/__tests__/KnownArgumentNamesRule-test.ts b/src/validation/__tests__/KnownArgumentNamesRule-test.ts index 0fcffeca2c..28e3b564cb 100644 --- a/src/validation/__tests__/KnownArgumentNamesRule-test.ts +++ b/src/validation/__tests__/KnownArgumentNamesRule-test.ts @@ -100,6 +100,19 @@ describe('Validate: Known argument names', () => { `); }); + it('fragment args are known', () => { + expectValid(` + { + dog { + ...withArg(dogCommand: SIT) + } + } + fragment withArg($dogCommand: DogCommand) on Dog { + doesKnowCommand(dogCommand: $dogCommand) + } + `); + }); + it('field args are invalid', () => { expectErrors(` { @@ -148,6 +161,43 @@ describe('Validate: Known argument names', () => { ]); }); + it('arg passed to fragment without arg is reported', () => { + expectErrors(` + { + dog { + ...withoutArg(unknown: true) + } + } + fragment withoutArg on Dog { + doesKnowCommand + } + `).toDeepEqual([ + { + message: 'Unknown argument "unknown" on fragment "withoutArg".', + locations: [{ line: 4, column: 25 }], + }, + ]); + }); + + it('misspelled fragment args are reported', () => { + expectErrors(` + { + dog { + ...withArg(command: SIT) + } + } + fragment withArg($dogCommand: DogCommand) on Dog { + doesKnowCommand(dogCommand: $dogCommand) + } + `).toDeepEqual([ + { + message: + 'Unknown argument "command" on fragment "withArg". Did you mean "dogCommand"?', + locations: [{ line: 4, column: 22 }], + }, + ]); + }); + it('invalid arg name', () => { expectErrors(` fragment invalidArgName on Dog { diff --git a/src/validation/__tests__/NoUndefinedVariablesRule-test.ts b/src/validation/__tests__/NoUndefinedVariablesRule-test.ts index c6ed758cad..9d53ea8c2e 100644 --- a/src/validation/__tests__/NoUndefinedVariablesRule-test.ts +++ b/src/validation/__tests__/NoUndefinedVariablesRule-test.ts @@ -404,4 +404,67 @@ describe('Validate: No undefined variables', () => { }, ]); }); + + it('fragment defined arguments are not undefined variables', () => { + expectValid(` + query Foo { + ...FragA + } + fragment FragA($a: String) on Type { + field1(a: $a) + } + `); + }); + + it('defined variables used as fragment arguments are not undefined variables', () => { + expectValid(` + query Foo($b: String) { + ...FragA(a: $b) + } + fragment FragA($a: String) on Type { + field1 + } + `); + }); + + it('variables used as fragment arguments may be undefined variables', () => { + expectErrors(` + query Foo { + ...FragA(a: $a) + } + fragment FragA($a: String) on Type { + field1 + } + `).toDeepEqual([ + { + message: 'Variable "$a" is not defined by operation "Foo".', + locations: [ + { line: 3, column: 21 }, + { line: 2, column: 7 }, + ], + }, + ]); + }); + + it('variables shadowed by parent fragment arguments are still undefined variables', () => { + expectErrors(` + query Foo { + ...FragA + } + fragment FragA($a: String) on Type { + ...FragB + } + fragment FragB on Type { + field1(a: $a) + } + `).toDeepEqual([ + { + message: 'Variable "$a" is not defined by operation "Foo".', + locations: [ + { line: 9, column: 19 }, + { line: 2, column: 7 }, + ], + }, + ]); + }); }); diff --git a/src/validation/__tests__/NoUnusedFragmentArgumentsRule-test.ts b/src/validation/__tests__/NoUnusedFragmentArgumentsRule-test.ts new file mode 100644 index 0000000000..b5b6fe392d --- /dev/null +++ b/src/validation/__tests__/NoUnusedFragmentArgumentsRule-test.ts @@ -0,0 +1,42 @@ +import { describe, it } from 'mocha'; + +import { NoUnusedFragmentArgumentsRule } from '../rules/NoUnusedFragmentArgumentsRule.js'; + +import { expectValidationErrors } from './harness.js'; + +function expectErrors(queryStr: string) { + return expectValidationErrors(NoUnusedFragmentArgumentsRule, queryStr); +} + +function expectValid(queryStr: string) { + expectErrors(queryStr).toDeepEqual([]); +} + +describe('Validate: No unused fragment arguments', () => { + it('allows used fragment arguments', () => { + expectValid(` + query Foo { + ...FragA(a: "value") + } + fragment FragA($a: String) on Type { + field1(a: $a) + } + `); + }); + + it('reports errors with unused fragment arguments', () => { + expectErrors(` + query Foo($b: String) { + ...FragA + } + fragment FragA($a: String) on Type { + field1(a: $a) + } + `).toDeepEqual([ + { + message: 'Fragment argument "a" is not used.', + locations: [{ line: 5, column: 22 }], + }, + ]); + }); +}); diff --git a/src/validation/__tests__/NoUnusedVariablesRule-test.ts b/src/validation/__tests__/NoUnusedVariablesRule-test.ts index 47dac39c99..49076361be 100644 --- a/src/validation/__tests__/NoUnusedVariablesRule-test.ts +++ b/src/validation/__tests__/NoUnusedVariablesRule-test.ts @@ -230,4 +230,42 @@ describe('Validate: No unused variables', () => { }, ]); }); + + it('fragment defined arguments are not unused variables', () => { + expectValid(` + query Foo { + ...FragA + } + fragment FragA($a: String) on Type { + field1(a: $a) + } + `); + }); + + it('defined variables used as fragment arguments are not unused variables', () => { + expectValid(` + query Foo($b: String) { + ...FragA(a: $b) + } + fragment FragA($a: String) on Type { + field1(a: $a) + } + `); + }); + + it('unused fragment variables are reported', () => { + expectErrors(` + query Foo { + ...FragA(a: "value") + } + fragment FragA($a: String) on Type { + field1 + } + `).toDeepEqual([ + { + message: 'Variable "$a" is never used in fragment "FragA".', + locations: [{ line: 5, column: 22 }], + }, + ]); + }); }); diff --git a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts index ecb56a15cf..7d7c54f37b 100644 --- a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts +++ b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts @@ -1264,4 +1264,239 @@ describe('Validate: Overlapping fields can be merged', () => { }, ]); }); + + describe('fragment arguments must produce fields that can be merged', () => { + it('allows conflicting spreads at different depths', () => { + expectValid(` + query ValidDifferingFragmentArgs($command1: DogCommand, $command2: DogCommand) { + dog { + ...DoesKnowCommand(command: $command1) + mother { + ...DoesKnowCommand(command: $command2) + } + } + } + fragment DoesKnowCommand($command: DogCommand) on Dog { + doesKnowCommand(dogCommand: $command) + } + `); + }); + + it('encounters conflict in fragments', () => { + expectErrors(` + { + ...WithArgs(x: 3) + ...WithArgs(x: 4) + } + fragment WithArgs($x: Int) on Type { + a(x: $x) + } + `).toDeepEqual([ + { + message: + 'Spreads "WithArgs" conflict because WithArgs(x: 3) and WithArgs(x: 4) have different fragment arguments.', + locations: [ + { line: 3, column: 11 }, + { line: 4, column: 11 }, + ], + }, + ]); + }); + + it('allows operations with overlapping fields with arguments using identical operation variables', () => { + expectValid(` + query ($y: Int = 1) { + a(x: $y) + ...WithArgs(x: 1) + } + fragment WithArgs($x: Int = 1) on Type { + a(x: $y) + } + `); + }); + + it('allows operations with overlapping fields with identical variable arguments passed via fragment arguments', () => { + expectValid(` + query ($y: Int = 1) { + a(x: $y) + ...WithArgs(x: $y) + } + fragment WithArgs($x: Int) on Type { + a(x: $x) + } + `); + }); + + it('allows operations with overlapping fields with identical variable arguments passed via nested fragment arguments', () => { + expectValid(` + query ($z: Int = 1) { + a(x: $z) + ...WithArgs(y: $z) + } + fragment WithArgs($y: Int) on Type { + ...NestedWithArgs(x: $y) + } + fragment NestedWithArgs($x: Int) on Type { + a(x: $x) + } + `); + }); + + it('allows operations with overlapping fields with identical arguments via fragment variable defaults', () => { + expectValid(` + query { + a(x: 1) + ...WithArgs + } + fragment WithArgs($x: Int = 1) on Type { + a(x: $x) + } + `); + }); + + it('raises errors with overlapping fields with arguments that conflict via operation variables even with defaults and fragment variable defaults', () => { + expectErrors(` + query ($y: Int = 1) { + a(x: $y) + ...WithArgs + } + fragment WithArgs($x: Int = 1) on Type { + a(x: $x) + } + `).toDeepEqual([ + { + message: + 'Fields "a" conflict because they have differing arguments. Use different aliases on the fields to fetch both if this was intentional.', + locations: [ + { line: 3, column: 11 }, + { line: 7, column: 11 }, + ], + }, + ]); + }); + + it('allows operations with overlapping list fields with identical variable arguments passed via fragment arguments', () => { + expectValid(` + query Query($stringListVarY: [String]) { + complicatedArgs { + stringListArgField(stringListArg: $stringListVarY) + ...WithArgs(stringListVarX: $stringListVarY) + } + } + fragment WithArgs($stringListVarX: [String]) on Type { + stringListArgField(stringListArg: $stringListVarX) + } + `); + }); + + it('allows operations with overlapping list fields with identical variable arguments in item position passed via fragment arguments', () => { + expectValid(` + query Query($stringListVarY: [String]) { + complicatedArgs { + stringListArgField(stringListArg: [$stringListVarY]) + ...WithArgs(stringListVarX: $stringListVarY) + } + } + fragment WithArgs($stringListVarX: [String]) on Type { + stringListArgField(stringListArg: [$stringListVarX]) + } + `); + }); + + it('allows operations with overlapping input object fields with identical variable arguments passed via fragment arguments', () => { + expectValid(` + query Query($complexVarY: ComplexInput) { + complicatedArgs { + complexArgField(complexArg: $complexVarY) + ...WithArgs(complexVarX: $complexVarY) + } + } + fragment WithArgs($complexVarX: ComplexInput) on Type { + complexArgField(complexArg: $complexVarX) + } + `); + }); + + it('allows operations with overlapping input object fields with identical variable arguments in field position passed via fragment arguments', () => { + expectValid(` + query Query($boolVarY: Boolean) { + complicatedArgs { + complexArgField(complexArg: {requiredArg: $boolVarY}) + ...WithArgs(boolVarX: $boolVarY) + } + } + fragment WithArgs($boolVarX: Boolean) on Type { + complexArgField(complexArg: {requiredArg: $boolVarX}) + } + `); + }); + + it('encounters nested field conflict in fragments that could otherwise merge', () => { + expectErrors(` + query ValidDifferingFragmentArgs($command1: DogCommand, $command2: DogCommand) { + dog { + ...DoesKnowCommandNested(command: $command1) + mother { + ...DoesKnowCommandNested(command: $command2) + } + } + } + fragment DoesKnowCommandNested($command: DogCommand) on Dog { + doesKnowCommand(dogCommand: $command) + mother { + doesKnowCommand(dogCommand: $command) + } + } + `).toDeepEqual([ + { + message: + 'Fields "mother" conflict because subfields "doesKnowCommand" conflict because they have differing arguments. Use different aliases on the fields to fetch both if this was intentional.', + locations: [ + { line: 5, column: 13 }, + { line: 13, column: 13 }, + { line: 12, column: 11 }, + { line: 11, column: 11 }, + ], + }, + ]); + }); + + it('encounters nested conflict in fragments', () => { + expectErrors(` + { + connection { + edges { + ...WithArgs(x: 3) + } + } + ...Connection + } + fragment Connection on Type { + connection { + edges { + ...WithArgs(x: 4) + } + } + } + fragment WithArgs($x: Int) on Type { + a(x: $x) + } + `).toDeepEqual([ + { + message: + 'Spreads "WithArgs" conflict because WithArgs(x: 3) and WithArgs(x: 4) have different fragment arguments.', + locations: [ + { + column: 15, + line: 5, + }, + { + column: 15, + line: 13, + }, + ], + }, + ]); + }); + }); }); diff --git a/src/validation/__tests__/ProvidedRequiredArgumentsRule-test.ts b/src/validation/__tests__/ProvidedRequiredArgumentsRule-test.ts index 6f0d223c15..1b7ec38524 100644 --- a/src/validation/__tests__/ProvidedRequiredArgumentsRule-test.ts +++ b/src/validation/__tests__/ProvidedRequiredArgumentsRule-test.ts @@ -356,4 +356,83 @@ describe('Validate: Provided required arguments', () => { ]); }); }); + + describe('Fragment required arguments', () => { + it('ignores unknown arguments', () => { + expectValid(` + { + ...Foo(unknownArgument: true) + } + fragment Foo on Query { + dog + } + `); + }); + + it('Missing nullable argument with default is allowed', () => { + expectValid(` + { + ...F + } + fragment F($x: Int = 3) on Query { + foo + } + `); + }); + it('Missing nullable argument is allowed', () => { + expectValid(` + { + ...F + } + fragment F($x: Int) on Query { + foo + } + `); + }); + it('Missing non-nullable argument with default is allowed', () => { + expectValid(` + { + ...F + } + fragment F($x: Int! = 3) on Query { + foo + } + `); + }); + it('Missing non-nullable argument is not allowed', () => { + expectErrors(` + { + ...F + } + fragment F($x: Int!) on Query { + foo + } + `).toDeepEqual([ + { + message: + 'Fragment "F" argument "x" of type "Int!" is required, but it was not provided.', + locations: [{ line: 3, column: 13 }], + }, + ]); + }); + + it('Supplies required variables', () => { + expectValid(` + { + ...F(x: 3) + } + fragment F($x: Int!) on Query { + foo + } + `); + }); + + it('Skips missing fragments', () => { + expectValid(` + { + ...Missing(x: 3) + } + `); + }); + }); }); diff --git a/src/validation/__tests__/ValuesOfCorrectTypeRule-test.ts b/src/validation/__tests__/ValuesOfCorrectTypeRule-test.ts index c36ebb6992..e2ebb3d356 100644 --- a/src/validation/__tests__/ValuesOfCorrectTypeRule-test.ts +++ b/src/validation/__tests__/ValuesOfCorrectTypeRule-test.ts @@ -1284,4 +1284,35 @@ describe('Validate: Values of correct type', () => { ]); }); }); + + describe('Fragment argument values', () => { + it('list variables with invalid item', () => { + expectErrors(` + fragment InvalidItem($a: [String] = ["one", 2]) on Query { + dog { name } + } + `).toDeepEqual([ + { + message: 'String cannot represent a non string value: 2', + locations: [{ line: 2, column: 53 }], + }, + ]); + }); + + it('fragment spread with invalid argument value', () => { + expectErrors(` + fragment GivesString on Query { + ...ExpectsInt(a: "three") + } + fragment ExpectsInt($a: Int) on Query { + dog { name } + } + `).toDeepEqual([ + { + message: 'Int cannot represent non-integer value: "three"', + locations: [{ line: 3, column: 28 }], + }, + ]); + }); + }); }); diff --git a/src/validation/__tests__/VariablesAreInputTypesRule-test.ts b/src/validation/__tests__/VariablesAreInputTypesRule-test.ts index 8027a35826..eddd202df8 100644 --- a/src/validation/__tests__/VariablesAreInputTypesRule-test.ts +++ b/src/validation/__tests__/VariablesAreInputTypesRule-test.ts @@ -18,6 +18,9 @@ describe('Validate: Variables are input types', () => { query Foo($a: Unknown, $b: [[Unknown!]]!) { field(a: $a, b: $b) } + fragment Bar($a: Unknown, $b: [[Unknown!]]!) on Query { + field(a: $a, b: $b) + } `); }); @@ -26,6 +29,9 @@ describe('Validate: Variables are input types', () => { query Foo($a: String, $b: [Boolean!]!, $c: ComplexInput) { field(a: $a, b: $b, c: $c) } + fragment Bar($a: String, $b: [Boolean!]!, $c: ComplexInput) on Query { + field(a: $a, b: $b, c: $c) + } `); }); @@ -49,4 +55,25 @@ describe('Validate: Variables are input types', () => { }, ]); }); + + it('output types on fragment arguments are invalid', () => { + expectErrors(` + fragment Bar($a: Dog, $b: [[CatOrDog!]]!, $c: Pet) on Query { + field(a: $a, b: $b, c: $c) + } + `).toDeepEqual([ + { + locations: [{ line: 2, column: 24 }], + message: 'Variable "$a" cannot be non-input type "Dog".', + }, + { + locations: [{ line: 2, column: 33 }], + message: 'Variable "$b" cannot be non-input type "[[CatOrDog!]]!".', + }, + { + locations: [{ line: 2, column: 53 }], + message: 'Variable "$c" cannot be non-input type "Pet".', + }, + ]); + }); }); diff --git a/src/validation/__tests__/VariablesInAllowedPositionRule-test.ts b/src/validation/__tests__/VariablesInAllowedPositionRule-test.ts index 16467741bb..d0da8f5305 100644 --- a/src/validation/__tests__/VariablesInAllowedPositionRule-test.ts +++ b/src/validation/__tests__/VariablesInAllowedPositionRule-test.ts @@ -356,5 +356,109 @@ describe('Validate: Variables are in allowed positions', () => { dog @include(if: $boolVar) }`); }); + + it('undefined in directive with default value with option', () => { + expectValid(` + { + dog @include(if: $x) + }`); + }); + }); + + describe('Fragment arguments are validated', () => { + it('Boolean => Boolean', () => { + expectValid(` + query Query($booleanArg: Boolean) + { + complicatedArgs { + ...A(b: $booleanArg) + } + } + fragment A($b: Boolean) on ComplicatedArgs { + booleanArgField(booleanArg: $b) + } + `); + }); + + it('Boolean => Boolean with default value', () => { + expectValid(` + query Query($booleanArg: Boolean) + { + complicatedArgs { + ...A(b: $booleanArg) + } + } + fragment A($b: Boolean = true) on ComplicatedArgs { + booleanArgField(booleanArg: $b) + } + `); + }); + + it('Boolean => Boolean!', () => { + expectErrors(` + query Query($ab: Boolean) + { + complicatedArgs { + ...A(b: $ab) + } + } + fragment A($b: Boolean!) on ComplicatedArgs { + booleanArgField(booleanArg: $b) + } + `).toDeepEqual([ + { + message: + 'Variable "$ab" of type "Boolean" used in position expecting type "Boolean!".', + locations: [ + { line: 2, column: 21 }, + { line: 5, column: 21 }, + ], + }, + ]); + }); + + it('Int => Int! fails when variable provides null default value', () => { + expectErrors(` + query Query($intVar: Int = null) { + complicatedArgs { + ...A(i: $intVar) + } + } + fragment A($i: Int!) on ComplicatedArgs { + nonNullIntArgField(nonNullIntArg: $i) + } + `).toDeepEqual([ + { + message: + 'Variable "$intVar" of type "Int" used in position expecting type "Int!".', + locations: [ + { line: 2, column: 21 }, + { line: 4, column: 21 }, + ], + }, + ]); + }); + + it('Int fragment arg => Int! field arg fails even when shadowed by Int! variable', () => { + expectErrors(` + query Query($intVar: Int!) { + complicatedArgs { + ...A(i: $intVar) + } + } + fragment A($intVar: Int) on ComplicatedArgs { + nonNullIntArgField(nonNullIntArg: $intVar) + } + `).toDeepEqual([ + { + message: + 'Variable "$intVar" of type "Int" used in position expecting type "Int!".', + locations: [ + { line: 7, column: 20 }, + { line: 8, column: 45 }, + ], + }, + ]); + }); }); }); diff --git a/src/validation/index.ts b/src/validation/index.ts index dbe8e57dc0..95de4408db 100644 --- a/src/validation/index.ts +++ b/src/validation/index.ts @@ -51,6 +51,9 @@ export { NoUnusedFragmentsRule } from './rules/NoUnusedFragmentsRule.js'; // Spec Section: "All Variables Used" export { NoUnusedVariablesRule } from './rules/NoUnusedVariablesRule.js'; +// Spec Section: "All Fragment Arguments Used" +export { NoUnusedFragmentArgumentsRule } from './rules/NoUnusedFragmentArgumentsRule.js'; + // Spec Section: "Field Selection Merging" export { OverlappingFieldsCanBeMergedRule } from './rules/OverlappingFieldsCanBeMergedRule.js'; diff --git a/src/validation/rules/KnownArgumentNamesRule.ts b/src/validation/rules/KnownArgumentNamesRule.ts index 2f09348778..b3171258ec 100644 --- a/src/validation/rules/KnownArgumentNamesRule.ts +++ b/src/validation/rules/KnownArgumentNamesRule.ts @@ -26,6 +26,30 @@ export function KnownArgumentNamesRule(context: ValidationContext): ASTVisitor { return { // eslint-disable-next-line new-cap ...KnownArgumentNamesOnDirectivesRule(context), + FragmentArgument(argNode) { + const fragmentSignature = context.getFragmentSignature(); + if (fragmentSignature) { + const varDef = fragmentSignature.variableDefinitions.get( + argNode.name.value, + ); + if (!varDef) { + const argName = argNode.name.value; + const suggestions = suggestionList( + argName, + Array.from(fragmentSignature.variableDefinitions.values()).map( + (varSignature) => varSignature.variable.name.value, + ), + ); + context.reportError( + new GraphQLError( + `Unknown argument "${argName}" on fragment "${fragmentSignature.definition.name.value}".` + + didYouMean(suggestions), + { nodes: argNode }, + ), + ); + } + } + }, Argument(argNode) { const argDef = context.getArgument(); const fieldDef = context.getFieldDef(); @@ -33,8 +57,10 @@ export function KnownArgumentNamesRule(context: ValidationContext): ASTVisitor { if (!argDef && fieldDef && parentType) { const argName = argNode.name.value; - const knownArgsNames = fieldDef.args.map((arg) => arg.name); - const suggestions = suggestionList(argName, knownArgsNames); + const suggestions = suggestionList( + argName, + fieldDef.args.map((arg) => arg.name), + ); context.reportError( new GraphQLError( `Unknown argument "${argName}" on field "${parentType.name}.${fieldDef.name}".` + diff --git a/src/validation/rules/NoUndefinedVariablesRule.ts b/src/validation/rules/NoUndefinedVariablesRule.ts index d1672ecd0b..ae010cab8e 100644 --- a/src/validation/rules/NoUndefinedVariablesRule.ts +++ b/src/validation/rules/NoUndefinedVariablesRule.ts @@ -22,7 +22,10 @@ export function NoUndefinedVariablesRule( ); const usages = context.getRecursiveVariableUsages(operation); - for (const { node } of usages) { + for (const { node, fragmentVariableDefinition } of usages) { + if (fragmentVariableDefinition) { + continue; + } const varName = node.name.value; if (!variableNameDefined.has(varName)) { context.reportError( diff --git a/src/validation/rules/NoUnusedFragmentArgumentsRule.ts b/src/validation/rules/NoUnusedFragmentArgumentsRule.ts new file mode 100644 index 0000000000..d73f847116 --- /dev/null +++ b/src/validation/rules/NoUnusedFragmentArgumentsRule.ts @@ -0,0 +1,90 @@ +import { GraphQLError } from '../../error/GraphQLError.js'; + +import type { + FragmentDefinitionNode, + SelectionSetNode, +} from '../../language/ast.js'; +import type { ASTVisitor } from '../../language/visitor.js'; + +import type { ValidationContext } from '../ValidationContext.js'; + +/** + * No unused fragment arguments + * + * A GraphQL document is only valid if each argument defined by a fragment + * definition within the document is used by at least one fragment + * spread within the document. + * + * See https://spec.graphql.org/draft/#sec-Fragment-Arguments-Must-Be-Used + */ +export function NoUnusedFragmentArgumentsRule( + context: ValidationContext, +): ASTVisitor { + const fragmentArgumentNamesUsed = new Map< + FragmentDefinitionNode, + Set + >(); + return { + OperationDefinition(operation) { + addUsedArguments( + context, + fragmentArgumentNamesUsed, + operation.selectionSet, + ); + }, + FragmentDefinition(fragment) { + addUsedArguments( + context, + fragmentArgumentNamesUsed, + fragment.selectionSet, + ); + }, + Document: { + leave(document) { + for (const definition of document.definitions) { + if (definition.kind === 'FragmentDefinition') { + const argumentsUsedForFragment = + fragmentArgumentNamesUsed.get(definition); + const variableDefinitions = definition.variableDefinitions; + if (variableDefinitions) { + for (const variableDefinition of variableDefinitions) { + const argumentName = variableDefinition.variable.name.value; + if (!argumentsUsedForFragment?.has(argumentName)) { + context.reportError( + new GraphQLError( + `Fragment argument "${argumentName}" is not used.`, + { nodes: variableDefinition }, + ), + ); + } + } + } + } + } + }, + }, + }; +} + +function addUsedArguments( + context: ValidationContext, + fragmentArgumentNamesUsed: Map>, + selectionSetNode: SelectionSetNode, +): void { + const operationSpreads = context.getFragmentSpreads(selectionSetNode); + for (const spread of operationSpreads) { + const fragment = context.getFragment(spread.name.value); + if (fragment) { + let argumentsUsedForFragment = fragmentArgumentNamesUsed.get(fragment); + if (!argumentsUsedForFragment) { + argumentsUsedForFragment = new Set(); + fragmentArgumentNamesUsed.set(fragment, argumentsUsedForFragment); + } + if (spread.arguments) { + for (const argument of spread.arguments) { + argumentsUsedForFragment.add(argument.name.value); + } + } + } + } +} diff --git a/src/validation/rules/NoUnusedVariablesRule.ts b/src/validation/rules/NoUnusedVariablesRule.ts index 7a0660cce0..e55e1c16ed 100644 --- a/src/validation/rules/NoUnusedVariablesRule.ts +++ b/src/validation/rules/NoUnusedVariablesRule.ts @@ -14,18 +14,42 @@ import type { ValidationContext } from '../ValidationContext.js'; */ export function NoUnusedVariablesRule(context: ValidationContext): ASTVisitor { return { - OperationDefinition(operation) { - const usages = context.getRecursiveVariableUsages(operation); - const variableNameUsed = new Set( + FragmentDefinition(fragment) { + const usages = context.getVariableUsages(fragment); + const argumentNameUsed = new Set( usages.map(({ node }) => node.name.value), ); + // FIXME: https://github.com/graphql/graphql-js/issues/2203 + /* c8 ignore next */ + const variableDefinitions = fragment.variableDefinitions ?? []; + for (const varDef of variableDefinitions) { + const argName = varDef.variable.name.value; + if (!argumentNameUsed.has(argName)) { + context.reportError( + new GraphQLError( + `Variable "$${argName}" is never used in fragment "${fragment.name.value}".`, + { nodes: varDef }, + ), + ); + } + } + }, + OperationDefinition(operation) { + const usages = context.getRecursiveVariableUsages(operation); + const operationVariableNameUsed = new Set(); + for (const { node, fragmentVariableDefinition } of usages) { + const varName = node.name.value; + if (!fragmentVariableDefinition) { + operationVariableNameUsed.add(varName); + } + } // FIXME: https://github.com/graphql/graphql-js/issues/2203 /* c8 ignore next */ const variableDefinitions = operation.variableDefinitions ?? []; for (const variableDef of variableDefinitions) { const variableName = variableDef.variable.name.value; - if (!variableNameUsed.has(variableName)) { + if (!operationVariableNameUsed.has(variableName)) { context.reportError( new GraphQLError( operation.name diff --git a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts index 851f7ea625..dddb66c07d 100644 --- a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts +++ b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts @@ -4,9 +4,12 @@ import type { Maybe } from '../../jsutils/Maybe.js'; import { GraphQLError } from '../../error/GraphQLError.js'; import type { + ArgumentNode, DirectiveNode, FieldNode, + FragmentArgumentNode, FragmentDefinitionNode, + FragmentSpreadNode, SelectionSetNode, ValueNode, } from '../../language/ast.js'; @@ -67,16 +70,16 @@ export function OverlappingFieldsCanBeMergedRule( // dramatically improve the performance of this validator. const comparedFragmentPairs = new PairSet(); - // A cache for the "field map" and list of fragment names found in any given + // A cache for the "field map" and list of fragment spreads found in any given // selection set. Selection sets may be asked for this information multiple // times, so this improves the performance of this validator. - const cachedFieldsAndFragmentNames = new Map(); + const cachedFieldsAndFragmentSpreads = new Map(); return { SelectionSet(selectionSet) { const conflicts = findConflictsWithinSelectionSet( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, context.getParentType(), selectionSet, @@ -107,8 +110,16 @@ type NodeAndDef = [ ]; // Map of array of those. type NodeAndDefCollection = Map>; -type FragmentNames = ReadonlyArray; -type FieldsAndFragmentNames = readonly [NodeAndDefCollection, FragmentNames]; +interface FragmentSpread { + key: string; + node: FragmentSpreadNode; + varMap: Map | undefined; +} +type FragmentSpreads = ReadonlyArray; +type FieldsAndFragmentSpreads = readonly [ + NodeAndDefCollection, + FragmentSpreads, +]; /** * Algorithm: @@ -170,56 +181,60 @@ type FieldsAndFragmentNames = readonly [NodeAndDefCollection, FragmentNames]; // GraphQL Document. function findConflictsWithinSelectionSet( context: ValidationContext, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, comparedFragmentPairs: PairSet, parentType: Maybe, selectionSet: SelectionSetNode, ): Array { const conflicts: Array = []; - const [fieldMap, fragmentNames] = getFieldsAndFragmentNames( + const [fieldMap, fragmentSpreads] = getFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, parentType, selectionSet, + undefined, ); - // (A) Find find all conflicts "within" the fields of this selection set. + // (A) Find find all conflicts "within" the fields and f of this selection set. // Note: this is the *only place* `collectConflictsWithin` is called. collectConflictsWithin( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, fieldMap, ); - if (fragmentNames.length !== 0) { + if (fragmentSpreads.length !== 0) { // (B) Then collect conflicts between these fields and those represented by - // each spread fragment name found. - for (let i = 0; i < fragmentNames.length; i++) { + // each spread found. + for (let i = 0; i < fragmentSpreads.length; i++) { collectConflictsBetweenFieldsAndFragment( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, false, fieldMap, - fragmentNames[i], + fragmentSpreads[i], ); // (C) Then compare this fragment with all other fragments found in this // selection set to collect conflicts between fragments spread together. - // This compares each item in the list of fragment names to every other + // This compares each item in the list of fragment spreads to every other // item in that same list (except for itself). - for (let j = i + 1; j < fragmentNames.length; j++) { + for (let j = i + 1; j < fragmentSpreads.length; j++) { collectConflictsBetweenFragments( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, false, - fragmentNames[i], - fragmentNames[j], + fragmentSpreads[i], + fragmentSpreads[j], ); } } @@ -232,22 +247,26 @@ function findConflictsWithinSelectionSet( function collectConflictsBetweenFieldsAndFragment( context: ValidationContext, conflicts: Array, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, fieldMap: NodeAndDefCollection, - fragmentName: string, + fragmentSpread: FragmentSpread, ): void { - const fragment = context.getFragment(fragmentName); + const fragment = context.getFragment(fragmentSpread.node.name.value); if (!fragment) { return; } - const [fieldMap2, referencedFragmentNames] = - getReferencedFieldsAndFragmentNames( + const [fieldMap2, referencedFragmentSpreads] = + getReferencedFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, fragment, + fragmentSpread.varMap, ); // Do not compare a fragment's fieldMap to itself. @@ -260,40 +279,42 @@ function collectConflictsBetweenFieldsAndFragment( collectConflictsBetween( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, fieldMap, + undefined, fieldMap2, + fragmentSpread.varMap, ); // (E) Then collect any conflicts between the provided collection of fields - // and any fragment names found in the given fragment. - for (const referencedFragmentName of referencedFragmentNames) { + // and any fragment spreads found in the given fragment. + for (const referencedFragmentSpread of referencedFragmentSpreads) { // Memoize so two fragments are not compared for conflicts more than once. if ( comparedFragmentPairs.has( - referencedFragmentName, - fragmentName, + referencedFragmentSpread.key, + fragmentSpread.key, areMutuallyExclusive, ) ) { continue; } comparedFragmentPairs.add( - referencedFragmentName, - fragmentName, + referencedFragmentSpread.key, + fragmentSpread.key, areMutuallyExclusive, ); collectConflictsBetweenFieldsAndFragment( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, fieldMap, - referencedFragmentName, + referencedFragmentSpread, ); } } @@ -303,46 +324,74 @@ function collectConflictsBetweenFieldsAndFragment( function collectConflictsBetweenFragments( context: ValidationContext, conflicts: Array, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, - fragmentName1: string, - fragmentName2: string, + fragmentSpread1: FragmentSpread, + fragmentSpread2: FragmentSpread, ): void { // No need to compare a fragment to itself. - if (fragmentName1 === fragmentName2) { + if (fragmentSpread1.key === fragmentSpread2.key) { return; } + if (fragmentSpread1.node.name.value === fragmentSpread2.node.name.value) { + if ( + !sameArguments( + fragmentSpread1.node.arguments, + fragmentSpread1.varMap, + fragmentSpread2.node.arguments, + fragmentSpread2.varMap, + ) + ) { + context.reportError( + new GraphQLError( + `Spreads "${fragmentSpread1.node.name.value}" conflict because ${fragmentSpread1.key} and ${fragmentSpread2.key} have different fragment arguments.`, + { nodes: [fragmentSpread1.node, fragmentSpread2.node] }, + ), + ); + return; + } + } + // Memoize so two fragments are not compared for conflicts more than once. if ( comparedFragmentPairs.has( - fragmentName1, - fragmentName2, + fragmentSpread1.key, + fragmentSpread2.key, areMutuallyExclusive, ) ) { return; } - comparedFragmentPairs.add(fragmentName1, fragmentName2, areMutuallyExclusive); + comparedFragmentPairs.add( + fragmentSpread1.key, + fragmentSpread2.key, + areMutuallyExclusive, + ); - const fragment1 = context.getFragment(fragmentName1); - const fragment2 = context.getFragment(fragmentName2); + const fragment1 = context.getFragment(fragmentSpread1.node.name.value); + const fragment2 = context.getFragment(fragmentSpread2.node.name.value); if (!fragment1 || !fragment2) { return; } - const [fieldMap1, referencedFragmentNames1] = - getReferencedFieldsAndFragmentNames( + const [fieldMap1, referencedFragmentSpreads1] = + getReferencedFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, fragment1, + fragmentSpread1.varMap, ); - const [fieldMap2, referencedFragmentNames2] = - getReferencedFieldsAndFragmentNames( + const [fieldMap2, referencedFragmentSpreads2] = + getReferencedFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, fragment2, + fragmentSpread2.varMap, ); // (F) First, collect all conflicts between these two collections of fields @@ -350,38 +399,40 @@ function collectConflictsBetweenFragments( collectConflictsBetween( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, fieldMap1, + fragmentSpread1.varMap, fieldMap2, + fragmentSpread2.varMap, ); // (G) Then collect conflicts between the first fragment and any nested // fragments spread in the second fragment. - for (const referencedFragmentName2 of referencedFragmentNames2) { + for (const referencedFragmentSpread2 of referencedFragmentSpreads2) { collectConflictsBetweenFragments( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, - fragmentName1, - referencedFragmentName2, + fragmentSpread1, + referencedFragmentSpread2, ); } // (G) Then collect conflicts between the second fragment and any nested // fragments spread in the first fragment. - for (const referencedFragmentName1 of referencedFragmentNames1) { + for (const referencedFragmentSpread1 of referencedFragmentSpreads1) { collectConflictsBetweenFragments( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, - referencedFragmentName1, - fragmentName2, + referencedFragmentSpread1, + fragmentSpread2, ); } } @@ -391,81 +442,90 @@ function collectConflictsBetweenFragments( // between the sub-fields of two overlapping fields. function findConflictsBetweenSubSelectionSets( context: ValidationContext, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, parentType1: Maybe, selectionSet1: SelectionSetNode, + varMap1: Map | undefined, parentType2: Maybe, selectionSet2: SelectionSetNode, + varMap2: Map | undefined, ): Array { const conflicts: Array = []; - const [fieldMap1, fragmentNames1] = getFieldsAndFragmentNames( + const [fieldMap1, fragmentSpreads1] = getFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, parentType1, selectionSet1, + varMap1, ); - const [fieldMap2, fragmentNames2] = getFieldsAndFragmentNames( + const [fieldMap2, fragmentSpreads2] = getFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, parentType2, selectionSet2, + varMap2, ); // (H) First, collect all conflicts between these two collections of field. collectConflictsBetween( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, fieldMap1, + varMap1, fieldMap2, + varMap2, ); // (I) Then collect conflicts between the first collection of fields and // those referenced by each fragment name associated with the second. - for (const fragmentName2 of fragmentNames2) { + for (const fragmentSpread2 of fragmentSpreads2) { collectConflictsBetweenFieldsAndFragment( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, fieldMap1, - fragmentName2, + fragmentSpread2, ); } // (I) Then collect conflicts between the second collection of fields and // those referenced by each fragment name associated with the first. - for (const fragmentName1 of fragmentNames1) { + for (const fragmentSpread1 of fragmentSpreads1) { collectConflictsBetweenFieldsAndFragment( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, fieldMap2, - fragmentName1, + fragmentSpread1, ); } - // (J) Also collect conflicts between any fragment names by the first and - // fragment names by the second. This compares each item in the first set of - // names to each item in the second set of names. - for (const fragmentName1 of fragmentNames1) { - for (const fragmentName2 of fragmentNames2) { + // (J) Also collect conflicts between any fragment spreads by the first and + // fragment spreads by the second. This compares each item in the first set of + // spreads to each item in the second set of spreads. + for (const fragmentSpread1 of fragmentSpreads1) { + for (const fragmentSpread2 of fragmentSpreads2) { collectConflictsBetweenFragments( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, - fragmentName1, - fragmentName2, + fragmentSpread1, + fragmentSpread2, ); } } @@ -476,7 +536,10 @@ function findConflictsBetweenSubSelectionSets( function collectConflictsWithin( context: ValidationContext, conflicts: Array, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, comparedFragmentPairs: PairSet, fieldMap: NodeAndDefCollection, ): void { @@ -493,12 +556,14 @@ function collectConflictsWithin( for (let j = i + 1; j < fields.length; j++) { const conflict = findConflict( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, false, // within one collection is never mutually exclusive responseName, fields[i], + undefined, fields[j], + undefined, ); if (conflict) { conflicts.push(conflict); @@ -517,11 +582,16 @@ function collectConflictsWithin( function collectConflictsBetween( context: ValidationContext, conflicts: Array, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, comparedFragmentPairs: PairSet, parentFieldsAreMutuallyExclusive: boolean, fieldMap1: NodeAndDefCollection, + varMap1: Map | undefined, fieldMap2: NodeAndDefCollection, + varMap2: Map | undefined, ): void { // A field map is a keyed collection, where each key represents a response // name and the value at that key is a list of all fields which provide that @@ -535,12 +605,14 @@ function collectConflictsBetween( for (const field2 of fields2) { const conflict = findConflict( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, parentFieldsAreMutuallyExclusive, responseName, field1, + varMap1, field2, + varMap2, ); if (conflict) { conflicts.push(conflict); @@ -555,12 +627,17 @@ function collectConflictsBetween( // comparing their sub-fields. function findConflict( context: ValidationContext, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, comparedFragmentPairs: PairSet, parentFieldsAreMutuallyExclusive: boolean, responseName: string, field1: NodeAndDef, + varMap1: Map | undefined, field2: NodeAndDef, + varMap2: Map | undefined, ): Maybe { const [parentType1, node1, def1] = field1; const [parentType2, node2, def2] = field2; @@ -592,7 +669,7 @@ function findConflict( } // Two field calls must have the same arguments. - if (!sameArguments(node1, node2)) { + if (!sameArguments(node1.arguments, varMap1, node2.arguments, varMap2)) { return [ [responseName, 'they have differing arguments'], [node1], @@ -604,7 +681,7 @@ function findConflict( // FIXME https://github.com/graphql/graphql-js/issues/2203 const directives1 = /* c8 ignore next */ node1.directives ?? []; const directives2 = /* c8 ignore next */ node2.directives ?? []; - if (!sameStreams(directives1, directives2)) { + if (!sameStreams(directives1, varMap1, directives2, varMap2)) { return [ [responseName, 'they have differing stream directives'], [node1], @@ -629,7 +706,7 @@ function findConflict( ]; } - // Collect and compare sub-fields. Use the same "visited fragment names" list + // Collect and compare sub-fields. Use the same "visited fragment spreads" list // for both collections so fields in a fragment reference are never // compared to themselves. const selectionSet1 = node1.selectionSet; @@ -637,25 +714,26 @@ function findConflict( if (selectionSet1 && selectionSet2) { const conflicts = findConflictsBetweenSubSelectionSets( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, getNamedType(type1), selectionSet1, + varMap1, getNamedType(type2), selectionSet2, + varMap2, ); return subfieldConflicts(conflicts, responseName, node1, node2); } } -function sameArguments( - node1: FieldNode | DirectiveNode, - node2: FieldNode | DirectiveNode, +function sameArguments( + args1: ReadonlyArray | undefined, + varMap1: Map | undefined, + args2: ReadonlyArray | undefined, + varMap2: Map | undefined, ): boolean { - const args1 = node1.arguments; - const args2 = node2.arguments; - if (args1 === undefined || args1.length === 0) { return args2 === undefined || args2.length === 0; } @@ -667,9 +745,17 @@ function sameArguments( return false; } - const values2 = new Map(args2.map(({ name, value }) => [name.value, value])); + const values2 = new Map( + args2.map(({ name, value }) => [ + name.value, + varMap2 === undefined ? value : replaceFragmentVariables(value, varMap2), + ]), + ); return args1.every((arg1) => { - const value1 = arg1.value; + let value1 = arg1.value; + if (varMap1) { + value1 = replaceFragmentVariables(value1, varMap1); + } const value2 = values2.get(arg1.name.value); if (value2 === undefined) { return false; @@ -679,6 +765,34 @@ function sameArguments( }); } +function replaceFragmentVariables( + valueNode: ValueNode, + varMap: ReadonlyMap, +): ValueNode { + switch (valueNode.kind) { + case Kind.VARIABLE: + return varMap.get(valueNode.name.value) ?? valueNode; + case Kind.LIST: + return { + ...valueNode, + values: valueNode.values.map((node) => + replaceFragmentVariables(node, varMap), + ), + }; + case Kind.OBJECT: + return { + ...valueNode, + fields: valueNode.fields.map((field) => ({ + ...field, + value: replaceFragmentVariables(field.value, varMap), + })), + }; + default: { + return valueNode; + } + } +} + function stringifyValue(value: ValueNode): string | null { return print(sortValueNode(value)); } @@ -691,7 +805,9 @@ function getStreamDirective( function sameStreams( directives1: ReadonlyArray, + varMap1: Map | undefined, directives2: ReadonlyArray, + varMap2: Map | undefined, ): boolean { const stream1 = getStreamDirective(directives1); const stream2 = getStreamDirective(directives2); @@ -700,7 +816,12 @@ function sameStreams( return true; } else if (stream1 && stream2) { // check if both fields have equivalent streams - return sameArguments(stream1, stream2); + return sameArguments( + stream1.arguments, + varMap1, + stream2.arguments, + varMap2, + ); } // fields have a mix of stream and no stream return false; @@ -736,60 +857,74 @@ function doTypesConflict( } // Given a selection set, return the collection of fields (a mapping of response -// name to field nodes and definitions) as well as a list of fragment names +// name to field nodes and definitions) as well as a list of fragment spreads // referenced via fragment spreads. -function getFieldsAndFragmentNames( +function getFieldsAndFragmentSpreads( context: ValidationContext, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, parentType: Maybe, selectionSet: SelectionSetNode, -): FieldsAndFragmentNames { - const cached = cachedFieldsAndFragmentNames.get(selectionSet); + varMap: Map | undefined, +): FieldsAndFragmentSpreads { + const cached = cachedFieldsAndFragmentSpreads.get(selectionSet); if (cached) { return cached; } const nodeAndDefs: NodeAndDefCollection = new Map(); - const fragmentNames = new Set(); - _collectFieldsAndFragmentNames( + const fragmentSpreads = new Map(); + _collectFieldsAndFragmentSpreads( context, parentType, selectionSet, nodeAndDefs, - fragmentNames, + fragmentSpreads, + varMap, ); - const result = [nodeAndDefs, [...fragmentNames]] as const; - cachedFieldsAndFragmentNames.set(selectionSet, result); + const result: FieldsAndFragmentSpreads = [ + nodeAndDefs, + Array.from(fragmentSpreads.values()), + ]; + cachedFieldsAndFragmentSpreads.set(selectionSet, result); return result; } // Given a reference to a fragment, return the represented collection of fields -// as well as a list of nested fragment names referenced via fragment spreads. -function getReferencedFieldsAndFragmentNames( +// as well as a list of nested fragment spreads referenced via fragment spreads. +function getReferencedFieldsAndFragmentSpreads( context: ValidationContext, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, fragment: FragmentDefinitionNode, + varMap: Map | undefined, ) { // Short-circuit building a type from the node if possible. - const cached = cachedFieldsAndFragmentNames.get(fragment.selectionSet); + const cached = cachedFieldsAndFragmentSpreads.get(fragment.selectionSet); if (cached) { return cached; } const fragmentType = typeFromAST(context.getSchema(), fragment.typeCondition); - return getFieldsAndFragmentNames( + return getFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, fragmentType, fragment.selectionSet, + varMap, ); } -function _collectFieldsAndFragmentNames( +function _collectFieldsAndFragmentSpreads( context: ValidationContext, parentType: Maybe, selectionSet: SelectionSetNode, nodeAndDefs: NodeAndDefCollection, - fragmentNames: Set, + fragmentSpreads: Map, + varMap: Map | undefined, ): void { for (const selection of selectionSet.selections) { switch (selection.kind) { @@ -811,20 +946,23 @@ function _collectFieldsAndFragmentNames( nodeAndDefsList.push([parentType, selection, fieldDef]); break; } - case Kind.FRAGMENT_SPREAD: - fragmentNames.add(selection.name.value); + case Kind.FRAGMENT_SPREAD: { + const fragmentSpread = getFragmentSpread(context, selection, varMap); + fragmentSpreads.set(fragmentSpread.key, fragmentSpread); break; + } case Kind.INLINE_FRAGMENT: { const typeCondition = selection.typeCondition; const inlineFragmentType = typeCondition ? typeFromAST(context.getSchema(), typeCondition) : parentType; - _collectFieldsAndFragmentNames( + _collectFieldsAndFragmentSpreads( context, inlineFragmentType, selection.selectionSet, nodeAndDefs, - fragmentNames, + fragmentSpreads, + varMap, ); break; } @@ -832,6 +970,48 @@ function _collectFieldsAndFragmentNames( } } +function getFragmentSpread( + context: ValidationContext, + fragmentSpreadNode: FragmentSpreadNode, + varMap: Map | undefined, +): FragmentSpread { + let key = ''; + const newVarMap = new Map(); + const fragmentSignature = context.getFragmentSignatureByName()( + fragmentSpreadNode.name.value, + ); + const argMap = new Map(); + if (fragmentSpreadNode.arguments) { + for (const arg of fragmentSpreadNode.arguments) { + argMap.set(arg.name.value, arg.value); + } + } + if (fragmentSignature?.variableDefinitions) { + key += fragmentSpreadNode.name.value + '('; + for (const [varName, variable] of fragmentSignature.variableDefinitions) { + const value = argMap.get(varName); + if (value) { + key += varName + ': ' + print(sortValueNode(value)); + } + const arg = argMap.get(varName); + if (arg !== undefined) { + newVarMap.set( + varName, + varMap !== undefined ? replaceFragmentVariables(arg, varMap) : arg, + ); + } else if (variable.defaultValue) { + newVarMap.set(varName, variable.defaultValue); + } + } + key += ')'; + } + return { + key, + node: fragmentSpreadNode, + varMap: newVarMap.size > 0 ? newVarMap : undefined, + }; +} + // Given a series of Conflicts which occurred between two sub-fields, generate // a single Conflict. function subfieldConflicts( diff --git a/src/validation/rules/ProvidedRequiredArgumentsRule.ts b/src/validation/rules/ProvidedRequiredArgumentsRule.ts index 72d104b852..f9114198cd 100644 --- a/src/validation/rules/ProvidedRequiredArgumentsRule.ts +++ b/src/validation/rules/ProvidedRequiredArgumentsRule.ts @@ -2,7 +2,10 @@ import { inspect } from '../../jsutils/inspect.js'; import { GraphQLError } from '../../error/GraphQLError.js'; -import type { InputValueDefinitionNode } from '../../language/ast.js'; +import type { + InputValueDefinitionNode, + VariableDefinitionNode, +} from '../../language/ast.js'; import { Kind } from '../../language/kinds.js'; import { print } from '../../language/printer.js'; import type { ASTVisitor } from '../../language/visitor.js'; @@ -11,6 +14,8 @@ import type { GraphQLArgument } from '../../type/definition.js'; import { isRequiredArgument, isType } from '../../type/definition.js'; import { specifiedDirectives } from '../../type/directives.js'; +import { typeFromAST } from '../../utilities/typeFromAST.js'; + import type { SDLValidationContext, ValidationContext, @@ -54,6 +59,42 @@ export function ProvidedRequiredArgumentsRule( } }, }, + FragmentSpread: { + // Validate on leave to allow for deeper errors to appear first. + leave(spreadNode) { + const fragmentSignature = context.getFragmentSignature(); + if (!fragmentSignature) { + return false; + } + + const providedArgs = new Set( + // FIXME: https://github.com/graphql/graphql-js/issues/2203 + /* c8 ignore next */ + spreadNode.arguments?.map((arg) => arg.name.value), + ); + for (const [ + varName, + variableDefinition, + ] of fragmentSignature.variableDefinitions) { + if ( + !providedArgs.has(varName) && + isRequiredArgumentNode(variableDefinition) + ) { + const type = typeFromAST( + context.getSchema(), + variableDefinition.type, + ); + const argTypeStr = inspect(type); + context.reportError( + new GraphQLError( + `Fragment "${spreadNode.name.value}" argument "${varName}" of type "${argTypeStr}" is required, but it was not provided.`, + { nodes: spreadNode }, + ), + ); + } + } + }, + }, }; } @@ -127,6 +168,8 @@ export function ProvidedRequiredArgumentsOnDirectivesRule( }; } -function isRequiredArgumentNode(arg: InputValueDefinitionNode): boolean { +function isRequiredArgumentNode( + arg: InputValueDefinitionNode | VariableDefinitionNode, +): boolean { return arg.type.kind === Kind.NON_NULL_TYPE && arg.defaultValue == null; } diff --git a/src/validation/rules/VariablesInAllowedPositionRule.ts b/src/validation/rules/VariablesInAllowedPositionRule.ts index e662ba443c..e68a595c4f 100644 --- a/src/validation/rules/VariablesInAllowedPositionRule.ts +++ b/src/validation/rules/VariablesInAllowedPositionRule.ts @@ -36,9 +36,18 @@ export function VariablesInAllowedPositionRule( leave(operation) { const usages = context.getRecursiveVariableUsages(operation); - for (const { node, type, defaultValue } of usages) { + for (const { + node, + type, + defaultValue, + fragmentVariableDefinition, + } of usages) { const varName = node.name.value; - const varDef = varDefMap.get(varName); + + let varDef = fragmentVariableDefinition; + if (!varDef) { + varDef = varDefMap.get(varName); + } if (varDef && type) { // A var type is allowed if it is the same or more strict (e.g. is // a subtype of) than the expected type. It can be more strict if diff --git a/src/validation/specifiedRules.ts b/src/validation/specifiedRules.ts index 9884bea8b9..cd97c84c5a 100644 --- a/src/validation/specifiedRules.ts +++ b/src/validation/specifiedRules.ts @@ -31,6 +31,8 @@ import { MaxIntrospectionDepthRule } from './rules/MaxIntrospectionDepthRule.js' import { NoFragmentCyclesRule } from './rules/NoFragmentCyclesRule.js'; // Spec Section: "All Variable Used Defined" import { NoUndefinedVariablesRule } from './rules/NoUndefinedVariablesRule.js'; +// Spec Section: "All Fragment Arguments Used" +import { NoUnusedFragmentArgumentsRule } from './rules/NoUnusedFragmentArgumentsRule.js'; // Spec Section: "Fragments must be used" import { NoUnusedFragmentsRule } from './rules/NoUnusedFragmentsRule.js'; // Spec Section: "All Variables Used" @@ -107,6 +109,7 @@ export const specifiedRules: ReadonlyArray = Object.freeze([ UniqueVariableNamesRule, NoUndefinedVariablesRule, NoUnusedVariablesRule, + NoUnusedFragmentArgumentsRule, KnownDirectivesRule, UniqueDirectivesPerLocationRule, DeferStreamDirectiveOnRootFieldRule, From 5a1613524e627c1d6e303ab33e909c2fee1a66d0 Mon Sep 17 00:00:00 2001 From: jdecroock Date: Wed, 4 Sep 2024 18:18:29 +0200 Subject: [PATCH 5/5] Remove valiation rule --- .../NoUnusedFragmentArgumentsRule-test.ts | 42 --------- src/validation/index.ts | 3 - .../rules/NoUnusedFragmentArgumentsRule.ts | 90 ------------------- src/validation/specifiedRules.ts | 3 - 4 files changed, 138 deletions(-) delete mode 100644 src/validation/__tests__/NoUnusedFragmentArgumentsRule-test.ts delete mode 100644 src/validation/rules/NoUnusedFragmentArgumentsRule.ts diff --git a/src/validation/__tests__/NoUnusedFragmentArgumentsRule-test.ts b/src/validation/__tests__/NoUnusedFragmentArgumentsRule-test.ts deleted file mode 100644 index b5b6fe392d..0000000000 --- a/src/validation/__tests__/NoUnusedFragmentArgumentsRule-test.ts +++ /dev/null @@ -1,42 +0,0 @@ -import { describe, it } from 'mocha'; - -import { NoUnusedFragmentArgumentsRule } from '../rules/NoUnusedFragmentArgumentsRule.js'; - -import { expectValidationErrors } from './harness.js'; - -function expectErrors(queryStr: string) { - return expectValidationErrors(NoUnusedFragmentArgumentsRule, queryStr); -} - -function expectValid(queryStr: string) { - expectErrors(queryStr).toDeepEqual([]); -} - -describe('Validate: No unused fragment arguments', () => { - it('allows used fragment arguments', () => { - expectValid(` - query Foo { - ...FragA(a: "value") - } - fragment FragA($a: String) on Type { - field1(a: $a) - } - `); - }); - - it('reports errors with unused fragment arguments', () => { - expectErrors(` - query Foo($b: String) { - ...FragA - } - fragment FragA($a: String) on Type { - field1(a: $a) - } - `).toDeepEqual([ - { - message: 'Fragment argument "a" is not used.', - locations: [{ line: 5, column: 22 }], - }, - ]); - }); -}); diff --git a/src/validation/index.ts b/src/validation/index.ts index 95de4408db..dbe8e57dc0 100644 --- a/src/validation/index.ts +++ b/src/validation/index.ts @@ -51,9 +51,6 @@ export { NoUnusedFragmentsRule } from './rules/NoUnusedFragmentsRule.js'; // Spec Section: "All Variables Used" export { NoUnusedVariablesRule } from './rules/NoUnusedVariablesRule.js'; -// Spec Section: "All Fragment Arguments Used" -export { NoUnusedFragmentArgumentsRule } from './rules/NoUnusedFragmentArgumentsRule.js'; - // Spec Section: "Field Selection Merging" export { OverlappingFieldsCanBeMergedRule } from './rules/OverlappingFieldsCanBeMergedRule.js'; diff --git a/src/validation/rules/NoUnusedFragmentArgumentsRule.ts b/src/validation/rules/NoUnusedFragmentArgumentsRule.ts deleted file mode 100644 index d73f847116..0000000000 --- a/src/validation/rules/NoUnusedFragmentArgumentsRule.ts +++ /dev/null @@ -1,90 +0,0 @@ -import { GraphQLError } from '../../error/GraphQLError.js'; - -import type { - FragmentDefinitionNode, - SelectionSetNode, -} from '../../language/ast.js'; -import type { ASTVisitor } from '../../language/visitor.js'; - -import type { ValidationContext } from '../ValidationContext.js'; - -/** - * No unused fragment arguments - * - * A GraphQL document is only valid if each argument defined by a fragment - * definition within the document is used by at least one fragment - * spread within the document. - * - * See https://spec.graphql.org/draft/#sec-Fragment-Arguments-Must-Be-Used - */ -export function NoUnusedFragmentArgumentsRule( - context: ValidationContext, -): ASTVisitor { - const fragmentArgumentNamesUsed = new Map< - FragmentDefinitionNode, - Set - >(); - return { - OperationDefinition(operation) { - addUsedArguments( - context, - fragmentArgumentNamesUsed, - operation.selectionSet, - ); - }, - FragmentDefinition(fragment) { - addUsedArguments( - context, - fragmentArgumentNamesUsed, - fragment.selectionSet, - ); - }, - Document: { - leave(document) { - for (const definition of document.definitions) { - if (definition.kind === 'FragmentDefinition') { - const argumentsUsedForFragment = - fragmentArgumentNamesUsed.get(definition); - const variableDefinitions = definition.variableDefinitions; - if (variableDefinitions) { - for (const variableDefinition of variableDefinitions) { - const argumentName = variableDefinition.variable.name.value; - if (!argumentsUsedForFragment?.has(argumentName)) { - context.reportError( - new GraphQLError( - `Fragment argument "${argumentName}" is not used.`, - { nodes: variableDefinition }, - ), - ); - } - } - } - } - } - }, - }, - }; -} - -function addUsedArguments( - context: ValidationContext, - fragmentArgumentNamesUsed: Map>, - selectionSetNode: SelectionSetNode, -): void { - const operationSpreads = context.getFragmentSpreads(selectionSetNode); - for (const spread of operationSpreads) { - const fragment = context.getFragment(spread.name.value); - if (fragment) { - let argumentsUsedForFragment = fragmentArgumentNamesUsed.get(fragment); - if (!argumentsUsedForFragment) { - argumentsUsedForFragment = new Set(); - fragmentArgumentNamesUsed.set(fragment, argumentsUsedForFragment); - } - if (spread.arguments) { - for (const argument of spread.arguments) { - argumentsUsedForFragment.add(argument.name.value); - } - } - } - } -} diff --git a/src/validation/specifiedRules.ts b/src/validation/specifiedRules.ts index cd97c84c5a..9884bea8b9 100644 --- a/src/validation/specifiedRules.ts +++ b/src/validation/specifiedRules.ts @@ -31,8 +31,6 @@ import { MaxIntrospectionDepthRule } from './rules/MaxIntrospectionDepthRule.js' import { NoFragmentCyclesRule } from './rules/NoFragmentCyclesRule.js'; // Spec Section: "All Variable Used Defined" import { NoUndefinedVariablesRule } from './rules/NoUndefinedVariablesRule.js'; -// Spec Section: "All Fragment Arguments Used" -import { NoUnusedFragmentArgumentsRule } from './rules/NoUnusedFragmentArgumentsRule.js'; // Spec Section: "Fragments must be used" import { NoUnusedFragmentsRule } from './rules/NoUnusedFragmentsRule.js'; // Spec Section: "All Variables Used" @@ -109,7 +107,6 @@ export const specifiedRules: ReadonlyArray = Object.freeze([ UniqueVariableNamesRule, NoUndefinedVariablesRule, NoUnusedVariablesRule, - NoUnusedFragmentArgumentsRule, KnownDirectivesRule, UniqueDirectivesPerLocationRule, DeferStreamDirectiveOnRootFieldRule,