diff --git a/src/execution/__tests__/variables-test.ts b/src/execution/__tests__/variables-test.ts index f21bb95032..9f98253859 100644 --- a/src/execution/__tests__/variables-test.ts +++ b/src/execution/__tests__/variables-test.ts @@ -1006,6 +1006,156 @@ describe('Execute: Handles inputs', () => { }); }); + describe('using fragment arguments', () => { + it('when there are no fragment arguments', () => { + const result = executeQuery(` + 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 = executeQuery(` + 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 = executeQuery(` + query { + ...a + } + fragment a($value: String!) on TestType { + fieldWithNullableStringInput(input: $value) + } + `); + expect(result).to.deep.equal({ + data: { + fieldWithNullableStringInput: null, + }, + }); + }); + + it('when the definition has a default and is provided', () => { + const result = executeQuery(` + 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 = executeQuery(` + query { + ...a + } + fragment a($value: String! = "B") on TestType { + fieldWithNonNullableStringInput(input: $value) + } + `); + expect(result).to.deep.equal({ + data: { + fieldWithNonNullableStringInput: '"B"', + }, + }); + }); + + it('when the definition has a non-nullable default and is provided null', () => { + const result = executeQuery(` + query { + ...a(value: null) + } + fragment a($value: String! = "B") on TestType { + fieldWithNullableStringInput(input: $value) + } + `); + expect(result).to.deep.equal({ + data: { + fieldWithNullableStringInput: 'null', + }, + }); + }); + + it('when the definition has no default and is not provided', () => { + const result = executeQuery(` + query { + ...a + } + fragment a($value: String) on TestType { + fieldWithNonNullableStringInputAndDefaultArgumentValue(input: $value) + } + `); + expect(result).to.deep.equal({ + data: { + fieldWithNonNullableStringInputAndDefaultArgumentValue: + '"Hello World"', + }, + }); + }); + + it('when the argument variable is nested in a complex type', () => { + const result = executeQuery(` + 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 = executeQuery(` + 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"]', + }, + }); + }); + }); + describe('getVariableValues: limit maximum number of coercion errors', () => { const doc = parse(` query ($input: [String!]) { diff --git a/src/execution/collectFields.ts b/src/execution/collectFields.ts index bd85f73dcc..52c1ca8d78 100644 --- a/src/execution/collectFields.ts +++ b/src/execution/collectFields.ts @@ -2,13 +2,17 @@ import { AccumulatorMap } from '../jsutils/AccumulatorMap'; import type { ObjMap } from '../jsutils/ObjMap'; import type { + ArgumentNode, FieldNode, FragmentDefinitionNode, FragmentSpreadNode, InlineFragmentNode, SelectionSetNode, + ValueNode, + VariableDefinitionNode, } from '../language/ast'; import { Kind } from '../language/kinds'; +import { visit } from '../language/visitor'; import type { GraphQLObjectType } from '../type/definition'; import { isAbstractType } from '../type/definition'; @@ -139,12 +143,18 @@ function collectFieldsImpl( ) { continue; } + const selectionSetWithAppliedArguments = + selectionSetWithFragmentArgumentsApplied( + fragment.argumentDefinitions, + selection.arguments, + fragment.selectionSet, + ); collectFieldsImpl( schema, fragments, variableValues, runtimeType, - fragment.selectionSet, + selectionSetWithAppliedArguments, fields, visitedFragmentNames, ); @@ -154,6 +164,41 @@ function collectFieldsImpl( } } +function selectionSetWithFragmentArgumentsApplied( + argumentDefinitions: Readonly> | undefined, + fragmentArguments: Readonly> | undefined, + selectionSet: SelectionSetNode, +): SelectionSetNode { + const providedArguments = new Map(); + if (fragmentArguments) { + for (const argument of fragmentArguments) { + providedArguments.set(argument.name.value, argument); + } + } + + const fragmentArgumentValues = new Map(); + if (argumentDefinitions) { + for (const argumentDef of argumentDefinitions) { + const variableName = argumentDef.variable.name.value; + const providedArg = providedArguments.get(variableName); + if (providedArg) { + // Not valid if the providedArg is null and argumentDef is non-null + fragmentArgumentValues.set(variableName, providedArg.value); + } else if (argumentDef.defaultValue) { + fragmentArgumentValues.set(variableName, argumentDef.defaultValue); + } + // If argumentDef is non-null, expect a provided arg or non-null default value. + // Otherwise just preserve the variable as-is: it will be treated as unset by the executor. + } + } + + return visit(selectionSet, { + Variable(node) { + return fragmentArgumentValues.get(node.name.value); + }, + }); +} + /** * Determines if a field should be included based on the `@include` and `@skip` * directives, where `@skip` has higher precedence than `@include`. diff --git a/src/language/__tests__/parser-test.ts b/src/language/__tests__/parser-test.ts index 651da1cab1..d511ca175c 100644 --- a/src/language/__tests__/parser-test.ts +++ b/src/language/__tests__/parser-test.ts @@ -591,13 +591,16 @@ describe('Parser', () => { expect('loc' in result).to.equal(false); }); - it('Legacy: allows parsing fragment defined variables', () => { + it('allows parsing fragment defined arguments', () => { const document = 'fragment a($v: Boolean = false) on t { f(v: $v) }'; - expect(() => - parse(document, { allowLegacyFragmentVariables: true }), - ).to.not.throw(); - expect(() => parse(document)).to.throw('Syntax Error'); + expect(() => parse(document)).to.not.throw(); + }); + + it('allows parsing fragment spread arguments', () => { + const document = 'fragment a on t { ...b(v: $v) }'; + + expect(() => parse(document)).to.not.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 1050164763..1b0c5d196c 100644 --- a/src/language/__tests__/printer-test.ts +++ b/src/language/__tests__/printer-test.ts @@ -110,34 +110,58 @@ 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 }, ); - 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 }, ); - 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) }', + ); + 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") }', + ); + 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 a15f3df425..686e5e279f 100644 --- a/src/language/__tests__/visitor-test.ts +++ b/src/language/__tests__/visitor-test.ts @@ -455,10 +455,9 @@ 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, }); const visited: Array = []; @@ -505,6 +504,49 @@ describe('Visitor', () => { ]); }); + it('visits arguments on fragment spreads', () => { + const ast = parse('fragment a on t { ...s(v: false) }', { + noLocation: 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('n', () => { const ast = parse(kitchenSinkQuery, { experimentalClientControlledNullability: true, diff --git a/src/language/ast.ts b/src/language/ast.ts index 0b47c6a599..c54c158f5f 100644 --- a/src/language/ast.ts +++ b/src/language/ast.ts @@ -227,12 +227,11 @@ export const QueryDocumentKeys: { NonNullAssertion: ['nullabilityAssertion'], ErrorBoundary: ['nullabilityAssertion'], - FragmentSpread: ['name', 'directives'], + FragmentSpread: ['name', 'arguments', 'directives'], InlineFragment: ['typeCondition', 'directives', 'selectionSet'], FragmentDefinition: [ 'name', - // Note: fragment variable definitions are deprecated and will removed in v17.0.0 - 'variableDefinitions', + 'argumentDefinitions', 'typeCondition', 'directives', 'selectionSet', @@ -428,6 +427,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,8 +443,7 @@ 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?: + readonly argumentDefinitions?: | ReadonlyArray | undefined; readonly typeCondition: NamedTypeNode; diff --git a/src/language/parser.ts b/src/language/parser.ts index d74384efd9..8542cef2b7 100644 --- a/src/language/parser.ts +++ b/src/language/parser.ts @@ -82,23 +82,6 @@ export interface ParseOptions { */ noLocation?: boolean | undefined; - /** - * @deprecated will be removed in the v17.0.0 - * - * 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. - * - * The syntax is identical to normal, query-defined variables. For example: - * - * ```graphql - * fragment A($var: Boolean = false) on T { - * ... - * } - * ``` - */ - allowLegacyFragmentVariables?: boolean | undefined; - /** * EXPERIMENTAL: * @@ -539,7 +522,7 @@ export class Parser { /** * Corresponds to both FragmentSpread and InlineFragment in the spec. * - * FragmentSpread : ... FragmentName Directives? + * FragmentSpread : ... FragmentName Arguments? Directives? * * InlineFragment : ... TypeCondition? Directives? SelectionSet */ @@ -549,9 +532,18 @@ export class Parser { const hasTypeCondition = this.expectOptionalKeyword('on'); if (!hasTypeCondition && this.peek(TokenKind.NAME)) { + const name = this.parseFragmentName(); + if (this.peek(TokenKind.PAREN_L)) { + 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), }); } @@ -565,29 +557,17 @@ 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) { - return this.node(start, { - kind: Kind.FRAGMENT_DEFINITION, - name: this.parseFragmentName(), - variableDefinitions: this.parseVariableDefinitions(), - typeCondition: (this.expectKeyword('on'), this.parseNamedType()), - directives: this.parseDirectives(false), - selectionSet: this.parseSelectionSet(), - }); - } return this.node(start, { kind: Kind.FRAGMENT_DEFINITION, name: this.parseFragmentName(), + argumentDefinitions: this.parseVariableDefinitions(), typeCondition: (this.expectKeyword('on'), this.parseNamedType()), directives: this.parseDirectives(false), selectionSet: this.parseSelectionSet(), diff --git a/src/language/printer.ts b/src/language/printer.ts index 127e6cc4b8..25bfc6d851 100644 --- a/src/language/printer.ts +++ b/src/language/printer.ts @@ -105,8 +105,15 @@ const printDocASTReducer: ASTReducer = { // Fragments FragmentSpread: { - leave: ({ name, directives }) => - '...' + name + wrap(' ', join(directives, ' ')), + leave: ({ name, arguments: args, directives }) => { + const prefix = '...' + name; + let argsLine = prefix + wrap('(', join(args, ', '), ')'); + + if (argsLine.length > MAX_LINE_LENGTH) { + argsLine = prefix + wrap('(\n', indent(join(args, '\n')), '\n)'); + } + return argsLine + wrap(' ', join(directives, ' ')); + }, }, InlineFragment: { @@ -126,13 +133,13 @@ const printDocASTReducer: ASTReducer = { leave: ({ name, typeCondition, - variableDefinitions, + argumentDefinitions, directives, selectionSet, }) => // Note: fragment variable definitions are experimental and may be changed // or removed in the future. - `fragment ${name}${wrap('(', join(variableDefinitions, ', '), ')')} ` + + `fragment ${name}${wrap('(', join(argumentDefinitions, ', '), ')')} ` + `on ${typeCondition} ${wrap('', join(directives, ' '), ' ')}` + selectionSet, }, diff --git a/src/utilities/buildASTSchema.ts b/src/utilities/buildASTSchema.ts index cc7271409e..a028878094 100644 --- a/src/utilities/buildASTSchema.ts +++ b/src/utilities/buildASTSchema.ts @@ -93,7 +93,6 @@ export function buildSchema( ): GraphQLSchema { const document = parse(source, { noLocation: options?.noLocation, - allowLegacyFragmentVariables: options?.allowLegacyFragmentVariables, }); return buildASTSchema(document, { diff --git a/src/validation/__tests__/NoFragmentArgumentUsageRule-test.ts b/src/validation/__tests__/NoFragmentArgumentUsageRule-test.ts deleted file mode 100644 index 74fbf1aa9a..0000000000 --- a/src/validation/__tests__/NoFragmentArgumentUsageRule-test.ts +++ /dev/null @@ -1,50 +0,0 @@ -import { describe, it } from 'mocha'; - -import { NoFragmentArgumentUsageRule } from '../rules/custom/NoFragmentArgumentUsageRule'; - -import { expectValidationErrors } from './harness'; - -function expectErrors(queryStr: string) { - return expectValidationErrors(NoFragmentArgumentUsageRule, queryStr); -} - -function expectValid(queryStr: string) { - expectErrors(queryStr).to.deep.equal([]); -} - -describe('Validate: Known fragment names', () => { - it('known fragment names are valid', () => { - expectValid(` - { - ...HumanFields - } - fragment HumanFields on Query { - human(id: 4) { - name - } - } - `); - }); - - it('unknown fragment names are invalid', () => { - expectErrors(` - { - ...HumanFields(x: 4) - } - fragment HumanFields($x: ID) on Query { - human(id: $x) { - name - } - } - `).to.deep.equal([ - { - message: 'Fragment arguments are not enabled.', - locations: [{ line: 3, column: 24 }], - }, - { - message: 'Fragment argument definitions are not enabled.', - locations: [{ line: 5, column: 28 }], - }, - ]); - }); -}); diff --git a/src/validation/rules/custom/NoFragmentArgumentUsageRule.ts b/src/validation/rules/custom/NoFragmentArgumentUsageRule.ts deleted file mode 100644 index 6a67d8dbd8..0000000000 --- a/src/validation/rules/custom/NoFragmentArgumentUsageRule.ts +++ /dev/null @@ -1,35 +0,0 @@ -import { GraphQLError } from '../../../error/GraphQLError'; - -import type { ASTVisitor } from '../../../language/visitor'; - -import type { ASTValidationContext } from '../../ValidationContext'; - -/** - * Fragment arguments are, by default, not allowed to be used. - */ -export function NoFragmentArgumentUsageRule( - context: ASTValidationContext, -): ASTVisitor { - return { - FragmentDefinition(node) { - if (node.variableDefinitions && node.variableDefinitions.length > 0) { - context.reportError( - new GraphQLError( - 'Fragment argument definitions are not enabled.', - node.variableDefinitions[0], - ), - ); - } - }, - FragmentSpread(node) { - if (node.arguments && node.arguments.length > 0) { - context.reportError( - new GraphQLError( - 'Fragment arguments are not enabled.', - node.arguments[0], - ), - ); - } - }, - }; -}