From 6b20b31ff2d92afb30ba2b56c3fe3e1c7e9110a9 Mon Sep 17 00:00:00 2001 From: jdecroock Date: Wed, 7 Aug 2024 12:56:47 +0200 Subject: [PATCH 1/6] Support printing and parsing --- src/language/__tests__/parser-test.ts | 24 +++++++- src/language/__tests__/printer-test.ts | 37 ++++++++++-- src/language/__tests__/visitor-test.ts | 48 +++++++++++++++- src/language/ast.ts | 5 +- src/language/directiveLocation.ts | 1 + src/language/parser.ts | 57 +++++++++++++------ 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 | 1 + .../__tests__/KnownDirectivesRule-test.ts | 14 ++++- src/validation/__tests__/harness.ts | 2 +- src/validation/rules/KnownDirectivesRule.ts | 9 ++- 14 files changed, 198 insertions(+), 43 deletions(-) diff --git a/src/language/__tests__/parser-test.ts b/src/language/__tests__/parser-test.ts index 87e7b92c34..c580aae303 100644 --- a/src/language/__tests__/parser-test.ts +++ b/src/language/__tests__/parser-test.ts @@ -395,15 +395,35 @@ 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', () => { const { loc } = parse('{ id }'); diff --git a/src/language/__tests__/printer-test.ts b/src/language/__tests__/printer-test.ts index 227e90dd44..fc45657166 100644 --- a/src/language/__tests__/printer-test.ts +++ b/src/language/__tests__/printer-test.ts @@ -110,10 +110,10 @@ describe('Printer: Query document', () => { `); }); - it('Legacy: prints fragment with variable directives', () => { + it('prints fragment with variable directives', () => { const queryASTWithVariableDirective = parse( 'fragment Foo($foo: TestType @test) on TestType @testDirective { id }', - { allowLegacyFragmentVariables: true }, + { experimentalFragmentArguments: true }, ); expect(print(queryASTWithVariableDirective)).to.equal(dedent` fragment Foo($foo: TestType @test) on TestType @testDirective { @@ -122,14 +122,14 @@ describe('Printer: Query document', () => { `); }); - it('Legacy: correctly prints fragment defined variables', () => { + it('correctly prints fragment defined variables', () => { const fragmentWithVariable = parse( ` fragment Foo($a: ComplexType, $b: Boolean = false) on TestType { id } `, - { allowLegacyFragmentVariables: true }, + { experimentalFragmentArguments: true }, ); expect(print(fragmentWithVariable)).to.equal(dedent` fragment Foo($a: ComplexType, $b: Boolean = false) on TestType { @@ -213,4 +213,33 @@ describe('Printer: Query document', () => { `), ); }); + + 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" + ) + } + ` + ); + }); }); diff --git a/src/language/__tests__/visitor-test.ts b/src/language/__tests__/visitor-test.ts index 9149b103e3..4b11497cea 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 variables defined in fragments', () => { const ast = parse('fragment a($v: Boolean = false) on t { f }', { noLocation: true, - allowLegacyFragmentVariables: true, + experimentalFragmentArguments: true, }); const visited: Array = []; @@ -1361,4 +1361,48 @@ 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], + ]); + }); }); diff --git a/src/language/ast.ts b/src/language/ast.ts index 29029342a1..b1e278162a 100644 --- a/src/language/ast.ts +++ b/src/language/ast.ts @@ -209,11 +209,10 @@ export const QueryDocumentKeys: { Field: ['alias', 'name', 'arguments', 'directives', 'selectionSet'], Argument: ['name', 'value'], - 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', 'typeCondition', 'directives', @@ -383,6 +382,7 @@ export interface FragmentSpreadNode { readonly kind: Kind.FRAGMENT_SPREAD; readonly loc?: Location; readonly name: NameNode; + readonly arguments?: ReadonlyArray; readonly directives?: ReadonlyArray; } @@ -398,7 +398,6 @@ export interface FragmentDefinitionNode { readonly kind: Kind.FRAGMENT_DEFINITION; readonly loc?: Location; readonly name: NameNode; - /** @deprecated variableDefinitions will be removed in v17.0.0 */ readonly variableDefinitions?: ReadonlyArray; readonly typeCondition: NamedTypeNode; readonly directives?: ReadonlyArray; diff --git a/src/language/directiveLocation.ts b/src/language/directiveLocation.ts index 5c8aeb7240..8cc407b9ab 100644 --- a/src/language/directiveLocation.ts +++ b/src/language/directiveLocation.ts @@ -23,6 +23,7 @@ enum DirectiveLocation { ENUM_VALUE = 'ENUM_VALUE', INPUT_OBJECT = 'INPUT_OBJECT', INPUT_FIELD_DEFINITION = 'INPUT_FIELD_DEFINITION', + FRAGMENT_VARIABLE_DEFINITION = 'FRAGMENT_VARIABLE_DEFINITION', } export { DirectiveLocation }; diff --git a/src/language/parser.ts b/src/language/parser.ts index eb54a0376b..045569138f 100644 --- a/src/language/parser.ts +++ b/src/language/parser.ts @@ -103,6 +103,27 @@ export interface ParseOptions { * ``` */ allowLegacyFragmentVariables?: boolean; + + /** + * EXPERIMENTAL: + * + * 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. + * + * For example: + * + * ```graphql + * { + * t { ...A(var: true) } + * } + * fragment A($var: Boolean = false) on T { + * ...B(x: $var) + * } + * ``` + */ + experimentalFragmentArguments?: boolean | undefined; } /** @@ -485,7 +506,7 @@ export class Parser { /** * Corresponds to both FragmentSpread and InlineFragment in the spec. * - * FragmentSpread : ... FragmentName Directives? + * FragmentSpread : ... FragmentName Arguments? Directives? * * InlineFragment : ... TypeCondition? Directives? SelectionSet */ @@ -495,9 +516,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), }); } @@ -511,32 +544,24 @@ 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(), + variableDefinitions: + this._options.experimentalFragmentArguments === true || this._options.allowLegacyFragmentVariables === true + ? this.parseVariableDefinitions() + : undefined, 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 e95c118d8b..476187f09e 100644 --- a/src/language/printer.ts +++ b/src/language/printer.ts @@ -57,13 +57,8 @@ const printDocASTReducer: ASTReducer = { Field: { leave({ alias, name, arguments: args, directives, selectionSet }) { const prefix = 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, join(directives, ' '), selectionSet], ' '); + return join([wrappedLineAndArgs(prefix, args), join(directives, ' '), selectionSet], ' '); }, }, @@ -72,8 +67,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: { @@ -345,3 +344,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 29994c76ed..9e77e342ca 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 2c66ca5098..c30b32e9c0 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 37af4a60f7..8ee29d892c 100644 --- a/src/utilities/__tests__/printSchema-test.ts +++ b/src/utilities/__tests__/printSchema-test.ts @@ -848,9 +848,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 eeff08e6ed..02602f5b0b 100644 --- a/src/utilities/buildASTSchema.ts +++ b/src/utilities/buildASTSchema.ts @@ -102,6 +102,7 @@ export function buildSchema( 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 4cb6e225c1..652689fd63 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 ea4840341c..de54b482da 100644 --- a/src/validation/__tests__/harness.ts +++ b/src/validation/__tests__/harness.ts @@ -126,7 +126,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 f24dbe7d28..8703e97242 100644 --- a/src/validation/rules/KnownDirectivesRule.ts +++ b/src/validation/rules/KnownDirectivesRule.ts @@ -86,8 +86,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 d82c439fe971db382848d65b83c19e14c7c2eb8f Mon Sep 17 00:00:00 2001 From: jdecroock Date: Wed, 7 Aug 2024 13:28:07 +0200 Subject: [PATCH 2/6] Support executing operations with fragment variables --- src/execution/__tests__/variables-test.ts | 298 ++++++++++++++++++ src/execution/collectFields.ts | 57 +++- src/execution/execute.ts | 61 ++-- src/execution/subscribe.ts | 14 +- src/execution/values.ts | 118 ++++++- .../rules/SingleFieldSubscriptionsRule.ts | 6 +- 6 files changed, 484 insertions(+), 70 deletions(-) diff --git a/src/execution/__tests__/variables-test.ts b/src/execution/__tests__/variables-test.ts index 3a859a0bdc..69b9e93fd9 100644 --- a/src/execution/__tests__/variables-test.ts +++ b/src/execution/__tests__/variables-test.ts @@ -60,6 +60,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 +136,10 @@ const TestType = new GraphQLObjectType({ type: TestNestedInputObject, defaultValue: 'Hello World', }), + nested: { + type: NestedType, + resolve: () => ({}), + }, list: fieldWithInputArg({ type: new GraphQLList(GraphQLString) }), nnList: fieldWithInputArg({ type: new GraphQLNonNull(new GraphQLList(GraphQLString)), @@ -154,6 +165,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 +1155,283 @@ 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?.[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?.[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 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 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]', + }, + }); + }); + }); }); diff --git a/src/execution/collectFields.ts b/src/execution/collectFields.ts index d0961bfae8..7c9d608bfe 100644 --- a/src/execution/collectFields.ts +++ b/src/execution/collectFields.ts @@ -19,7 +19,12 @@ import type { GraphQLSchema } from '../type/schema'; import { typeFromAST } from '../utilities/typeFromAST'; -import { getDirectiveValues } from './values'; +import { getArgumentValuesFromSpread, getDirectiveValues } from './values'; + +export interface FieldDetails { + node: FieldNode; + fragmentVariableValues?: ObjMap | undefined; +} /** * Given a selectionSet, collects all of the fields and returns them. @@ -36,7 +41,7 @@ export function collectFields( variableValues: { [variable: string]: unknown }, runtimeType: GraphQLObjectType, selectionSet: SelectionSetNode, -): Map> { +): Map> { const fields = new Map(); collectFieldsImpl( schema, @@ -46,6 +51,7 @@ export function collectFields( selectionSet, fields, new Set(), + undefined ); return fields; } @@ -65,24 +71,24 @@ export function collectSubfields( fragments: ObjMap, variableValues: { [variable: string]: unknown }, returnType: GraphQLObjectType, - fieldNodes: ReadonlyArray, -): Map> { - const subFieldNodes = new Map(); + fieldEntries: ReadonlyArray, +): Map> { + const subFieldEntries = new Map(); const visitedFragmentNames = new Set(); - for (const node of fieldNodes) { - if (node.selectionSet) { + for (const entry of fieldEntries) { + if (entry.node.selectionSet) { collectFieldsImpl( schema, fragments, variableValues, returnType, - node.selectionSet, - subFieldNodes, + entry.node.selectionSet, + subFieldEntries, visitedFragmentNames, ); } } - return subFieldNodes; + return subFieldEntries; } function collectFieldsImpl( @@ -91,21 +97,23 @@ function collectFieldsImpl( variableValues: { [variable: string]: unknown }, runtimeType: GraphQLObjectType, selectionSet: SelectionSetNode, - fields: Map>, + fields: Map>, visitedFragmentNames: Set, + localVariableValues?: { [variable: string]: unknown }, ): void { for (const selection of selectionSet.selections) { switch (selection.kind) { case Kind.FIELD: { - if (!shouldIncludeNode(variableValues, selection)) { + const vars = localVariableValues ?? variableValues; + if (!shouldIncludeNode(vars, selection)) { continue; } const name = getFieldEntryKey(selection); const fieldList = fields.get(name); if (fieldList !== undefined) { - fieldList.push(selection); + fieldList.push({ node: selection, fragmentVariableValues: localVariableValues ?? undefined }); } else { - fields.set(name, [selection]); + fields.set(name, [{ node: selection, fragmentVariableValues: localVariableValues ?? undefined }]); } break; } @@ -143,6 +151,26 @@ function collectFieldsImpl( ) { continue; } + + // We need to introduce a concept of shadowing: + // + // - when a fragment defines a variable that is in the parent scope but not given + // in the fragment-spread we need to look at this variable as undefined and check + // whether the definition has a defaultValue, if not remove it from the variableValues. + // - when a fragment does not define a variable we need to copy it over from the parent + // scope as that variable can still get used in spreads later on in the selectionSet. + // - when a value is passed in through the fragment-spread we need to copy over the key-value + // into our variable-values. + const fragmentVariableValues = fragment.variableDefinitions + ? getArgumentValuesFromSpread( + selection, + schema, + fragment.variableDefinitions, + variableValues, + localVariableValues, + ) + : undefined; + collectFieldsImpl( schema, fragments, @@ -151,6 +179,7 @@ function collectFieldsImpl( fragment.selectionSet, fields, visitedFragmentNames, + fragmentVariableValues ); break; } diff --git a/src/execution/execute.ts b/src/execution/execute.ts index 55c22ea9de..0a501dab0b 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -55,6 +55,7 @@ import { assertValidSchema } from '../type/validate'; import { collectFields, collectSubfields as _collectSubfields, + FieldDetails, } from './collectFields'; import { getArgumentValues, getVariableValues } from './values'; @@ -67,7 +68,7 @@ const collectSubfields = memoize3( ( exeContext: ExecutionContext, returnType: GraphQLObjectType, - fieldNodes: ReadonlyArray, + fieldNodes: ReadonlyArray, ) => _collectSubfields( exeContext.schema, @@ -402,7 +403,7 @@ function executeFieldsSerially( parentType: GraphQLObjectType, sourceValue: unknown, path: Path | undefined, - fields: Map>, + fields: Map>, ): PromiseOrValue> { return promiseReduce( fields.entries(), @@ -440,7 +441,7 @@ function executeFields( parentType: GraphQLObjectType, sourceValue: unknown, path: Path | undefined, - fields: Map>, + fields: Map>, ): PromiseOrValue> { const results = Object.create(null); let containsPromise = false; @@ -494,10 +495,10 @@ function executeField( exeContext: ExecutionContext, parentType: GraphQLObjectType, source: unknown, - fieldNodes: ReadonlyArray, + fieldEntries: ReadonlyArray, path: Path, ): PromiseOrValue { - const fieldDef = getFieldDef(exeContext.schema, parentType, fieldNodes[0]); + const fieldDef = getFieldDef(exeContext.schema, parentType, fieldEntries[0]); if (!fieldDef) { return; } @@ -508,7 +509,7 @@ function executeField( const info = buildResolveInfo( exeContext, fieldDef, - fieldNodes, + fieldEntries, parentType, path, ); @@ -519,9 +520,10 @@ function executeField( // 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, - fieldNodes[0], + fieldEntries[0].node, + fieldDef.args, exeContext.variableValues, + fieldEntries[0].fragmentVariableValues ); // The resolve function's optional third argument is a context value that @@ -534,13 +536,13 @@ function executeField( let completed; if (isPromise(result)) { completed = result.then((resolved) => - completeValue(exeContext, returnType, fieldNodes, info, path, resolved), + completeValue(exeContext, returnType, fieldEntries, info, path, resolved), ); } else { completed = completeValue( exeContext, returnType, - fieldNodes, + fieldEntries, info, path, result, @@ -551,13 +553,13 @@ function executeField( // Note: we don't rely on a `catch` method, but we do expect "thenable" // to take a second callback for the error case. return completed.then(undefined, (rawError) => { - const error = locatedError(rawError, fieldNodes, pathToArray(path)); + const error = locatedError(rawError, fieldEntries.map(entry => entry.node), pathToArray(path)); return handleFieldError(error, returnType, exeContext); }); } return completed; } catch (rawError) { - const error = locatedError(rawError, fieldNodes, pathToArray(path)); + const error = locatedError(rawError, fieldEntries.map(entry => entry.node), pathToArray(path)); return handleFieldError(error, returnType, exeContext); } } @@ -568,7 +570,7 @@ function executeField( export function buildResolveInfo( exeContext: ExecutionContext, fieldDef: GraphQLField, - fieldNodes: ReadonlyArray, + fieldEntries: ReadonlyArray, parentType: GraphQLObjectType, path: Path, ): GraphQLResolveInfo { @@ -576,7 +578,7 @@ export function buildResolveInfo( // information about the current execution state. return { fieldName: fieldDef.name, - fieldNodes, + fieldNodes: fieldEntries.map(entry => entry.node), returnType: fieldDef.type, parentType, path, @@ -629,7 +631,7 @@ function handleFieldError( function completeValue( exeContext: ExecutionContext, returnType: GraphQLOutputType, - fieldNodes: ReadonlyArray, + fieldNodes: ReadonlyArray, info: GraphQLResolveInfo, path: Path, result: unknown, @@ -720,7 +722,7 @@ function completeValue( function completeListValue( exeContext: ExecutionContext, returnType: GraphQLList, - fieldNodes: ReadonlyArray, + fieldEntries: ReadonlyArray, info: GraphQLResolveInfo, path: Path, result: unknown, @@ -746,7 +748,7 @@ function completeListValue( completeValue( exeContext, itemType, - fieldNodes, + fieldEntries, info, itemPath, resolved, @@ -756,7 +758,7 @@ function completeListValue( completedItem = completeValue( exeContext, itemType, - fieldNodes, + fieldEntries, info, itemPath, item, @@ -770,7 +772,7 @@ function completeListValue( return completedItem.then(undefined, (rawError) => { const error = locatedError( rawError, - fieldNodes, + fieldEntries.map(entry => entry.node), pathToArray(itemPath), ); return handleFieldError(error, itemType, exeContext); @@ -778,7 +780,7 @@ function completeListValue( } return completedItem; } catch (rawError) { - const error = locatedError(rawError, fieldNodes, pathToArray(itemPath)); + const error = locatedError(rawError, fieldEntries.map(entry => entry.node), pathToArray(itemPath)); return handleFieldError(error, itemType, exeContext); } }); @@ -811,7 +813,7 @@ function completeLeafValue( function completeAbstractValue( exeContext: ExecutionContext, returnType: GraphQLAbstractType, - fieldNodes: ReadonlyArray, + fieldEntries: ReadonlyArray, info: GraphQLResolveInfo, path: Path, result: unknown, @@ -820,6 +822,7 @@ function completeAbstractValue( const contextValue = exeContext.contextValue; const runtimeType = resolveTypeFn(result, contextValue, info, returnType); + const fieldNodes = fieldEntries.map(entry => entry.node) if (isPromise(runtimeType)) { return runtimeType.then((resolvedRuntimeType) => completeObjectValue( @@ -832,7 +835,7 @@ function completeAbstractValue( info, result, ), - fieldNodes, + fieldEntries, info, path, result, @@ -850,7 +853,7 @@ function completeAbstractValue( info, result, ), - fieldNodes, + fieldEntries, info, path, result, @@ -918,13 +921,13 @@ function ensureValidRuntimeType( function completeObjectValue( exeContext: ExecutionContext, returnType: GraphQLObjectType, - fieldNodes: ReadonlyArray, + fieldEntries: ReadonlyArray, info: GraphQLResolveInfo, path: Path, result: unknown, ): PromiseOrValue> { // Collect sub-fields to execute to complete this value. - const subFieldNodes = collectSubfields(exeContext, returnType, fieldNodes); + const subFieldNodes = collectSubfields(exeContext, returnType, fieldEntries); // If there is an isTypeOf predicate function, call it with the // current result. If isTypeOf returns false, then raise an error rather @@ -935,7 +938,7 @@ function completeObjectValue( if (isPromise(isTypeOf)) { return isTypeOf.then((resolvedIsTypeOf) => { if (!resolvedIsTypeOf) { - throw invalidReturnTypeError(returnType, result, fieldNodes); + throw invalidReturnTypeError(returnType, result, fieldEntries.map(entry => entry.node)); } return executeFields( exeContext, @@ -948,7 +951,7 @@ function completeObjectValue( } if (!isTypeOf) { - throw invalidReturnTypeError(returnType, result, fieldNodes); + throw invalidReturnTypeError(returnType, result, fieldEntries.map(entry => entry.node)); } } @@ -1044,9 +1047,9 @@ export const defaultFieldResolver: GraphQLFieldResolver = export function getFieldDef( schema: GraphQLSchema, parentType: GraphQLObjectType, - fieldNode: FieldNode, + entry: FieldDetails, ): Maybe> { - const fieldName = fieldNode.name.value; + const fieldName = entry.node.name.value; if ( fieldName === SchemaMetaFieldDef.name && diff --git a/src/execution/subscribe.ts b/src/execution/subscribe.ts index 8b20ec3374..7b42278a96 100644 --- a/src/execution/subscribe.ts +++ b/src/execution/subscribe.ts @@ -214,14 +214,14 @@ async function executeSubscription( rootType, operation.selectionSet, ); - const [responseName, fieldNodes] = [...rootFields.entries()][0]; - const fieldDef = getFieldDef(schema, rootType, fieldNodes[0]); + const [responseName, fieldEntries] = [...rootFields.entries()][0]; + const fieldDef = getFieldDef(schema, rootType, fieldEntries[0]); if (!fieldDef) { - const fieldName = fieldNodes[0].name.value; + const fieldName = fieldEntries[0].node.name.value; throw new GraphQLError( `The subscription field "${fieldName}" is not defined.`, - { nodes: fieldNodes }, + { nodes: fieldEntries.map(entry => entry.node) }, ); } @@ -229,7 +229,7 @@ async function executeSubscription( const info = buildResolveInfo( exeContext, fieldDef, - fieldNodes, + fieldEntries, rootType, path, ); @@ -240,7 +240,7 @@ async function executeSubscription( // Build a JS object of arguments from the field.arguments AST, using the // variables scope to fulfill any variable references. - const args = getArgumentValues(fieldDef, fieldNodes[0], variableValues); + const args = getArgumentValues(fieldEntries[0].node, fieldDef.args, variableValues); // The resolve function's optional third argument is a context value that // is provided to every resolve function within an execution. It is commonly @@ -257,6 +257,6 @@ async function executeSubscription( } return eventStream; } catch (error) { - throw locatedError(error, fieldNodes, pathToArray(path)); + throw locatedError(error, fieldEntries.map(entry => entry.node), pathToArray(path)); } } diff --git a/src/execution/values.ts b/src/execution/values.ts index d65ea9cf20..e37ea168b2 100644 --- a/src/execution/values.ts +++ b/src/execution/values.ts @@ -9,12 +9,13 @@ import { GraphQLError } from '../error/GraphQLError'; import type { DirectiveNode, FieldNode, + FragmentSpreadNode, VariableDefinitionNode, } from '../language/ast'; import { Kind } from '../language/kinds'; import { print } from '../language/printer'; -import type { GraphQLField } from '../type/definition'; +import type { GraphQLArgument, GraphQLField } from '../type/definition'; import { isInputType, isNonNullType } from '../type/definition'; import type { GraphQLDirective } from '../type/directives'; import type { GraphQLSchema } from '../type/schema'; @@ -22,6 +23,7 @@ import type { GraphQLSchema } from '../type/schema'; import { coerceInputValue } from '../utilities/coerceInputValue'; import { typeFromAST } from '../utilities/typeFromAST'; import { valueFromAST } from '../utilities/valueFromAST'; +import { valueFromASTUntyped } from '../utilities'; type CoercedVariableValues = | { errors: ReadonlyArray; coerced?: never } @@ -150,9 +152,10 @@ function coerceVariableValues( * Object prototype. */ export function getArgumentValues( - def: GraphQLField | GraphQLDirective, node: FieldNode | DirectiveNode, + argDefs: ReadonlyArray, variableValues?: Maybe>, + fragmentArgValues?: Maybe>, ): { [argument: string]: unknown } { const coercedValues: { [argument: string]: unknown } = {}; @@ -161,7 +164,7 @@ export function getArgumentValues( const argumentNodes = node.arguments ?? []; const argNodeMap = keyMap(argumentNodes, (arg) => arg.name.value); - for (const argDef of def.args) { + for (const argDef of argDefs) { const name = argDef.name; const argType = argDef.type; const argumentNode = argNodeMap[name]; @@ -180,29 +183,36 @@ export function getArgumentValues( } const valueNode = argumentNode.value; - let isNull = valueNode.kind === Kind.NULL; + let hasValue = valueNode.kind !== Kind.NULL; if (valueNode.kind === Kind.VARIABLE) { const variableName = valueNode.name.value; if ( - variableValues == null || - !hasOwnProperty(variableValues, variableName) + fragmentArgValues != null && + hasOwnProperty(fragmentArgValues, variableName) ) { - if (argDef.defaultValue !== undefined) { + hasValue = fragmentArgValues![variableName] != null; + if (!hasValue && argDef.defaultValue !== undefined) { coercedValues[name] = argDef.defaultValue; - } else if (isNonNullType(argType)) { - throw new GraphQLError( - `Argument "${name}" of required type "${inspect(argType)}" ` + - `was provided the variable "$${variableName}" which was not provided a runtime value.`, - { nodes: valueNode }, - ); + continue; } + } else if (variableValues != null && hasOwnProperty(variableValues, variableName)) { + hasValue = variableValues[variableName] != null; + } else if (argDef.defaultValue !== undefined) { + coercedValues[name] = argDef.defaultValue; + continue; + } else if (isNonNullType(argType)) { + throw new GraphQLError( + `Argument "${name}" of required type "${inspect(argType)}" ` + + `was provided the variable "$${variableName}" which was not provided a runtime value.`, + { nodes: valueNode }, + ); + } else { continue; } - isNull = variableValues[variableName] == null; } - if (isNull && isNonNullType(argType)) { + if (!hasValue && isNonNullType(argType)) { throw new GraphQLError( `Argument "${name}" of non-null type "${inspect(argType)}" ` + 'must not be null.', @@ -210,7 +220,11 @@ export function getArgumentValues( ); } - const coercedValue = valueFromAST(valueNode, argType, variableValues); + const coercedValue = valueFromAST(valueNode, argType, { + ...variableValues, + ...fragmentArgValues, + }); + if (coercedValue === undefined) { // Note: ValuesOfCorrectTypeRule validation should catch this before // execution. This is a runtime check to ensure execution does not @@ -225,6 +239,76 @@ export function getArgumentValues( return coercedValues; } +export function getArgumentValuesFromSpread( + /** NOTE: For error annotations only */ + node: FragmentSpreadNode, + schema: GraphQLSchema, + fragmentVarDefs: ReadonlyArray, + variableValues: Maybe>, + fragmentArgValues?: Maybe>, +): { [argument: string]: unknown } { + const coercedValues: { [argument: string]: unknown } = {}; + const argNodeMap = keyMap(node?.arguments || [], (arg) => arg.name.value); + + for (const varDef of fragmentVarDefs) { + const name = varDef.variable.name.value; + const argType = typeFromAST(schema, varDef.type); + const argumentNode = argNodeMap[name]; + + if (argumentNode == null) { + if (varDef.defaultValue !== undefined) { + coercedValues[name] = valueFromASTUntyped(varDef.defaultValue); + } else if (isNonNullType(argType)) { + throw new GraphQLError( + `Argument "${name}" of required type "${inspect(argType)}" ` + + 'was not provided.', + { nodes: node }, + ); + } else { + coercedValues[name] = undefined; + } + continue; + } + + const valueNode = argumentNode.value; + + let hasValue = valueNode.kind !== Kind.NULL; + if (valueNode.kind === Kind.VARIABLE) { + const variableName = valueNode.name.value; + if ( + fragmentArgValues != null && + hasOwnProperty(fragmentArgValues, variableName) + ) { + hasValue = fragmentArgValues[variableName] != null; + } else if ( + variableValues != null && + hasOwnProperty(variableValues, variableName) + ) { + hasValue = variableValues[variableName] != null; + } + } + + if (!hasValue && isNonNullType(argType)) { + throw new GraphQLError( + `Argument "${name}" of non-null type "${inspect(argType)}" ` + + 'must not be null.', + { nodes: valueNode }, + ); + } + + let coercedValue; + if (argType && isInputType(argType)) { + coercedValue = valueFromAST(valueNode, argType, { + ...variableValues, + ...fragmentArgValues, + }); + } + + coercedValues[name] = coercedValue; + } + return coercedValues; +} + /** * Prepares an object map of argument values given a directive definition * and a AST node which may contain directives. Optionally also accepts a map @@ -246,7 +330,7 @@ export function getDirectiveValues( ); if (directiveNode) { - return getArgumentValues(directiveDef, directiveNode, variableValues); + return getArgumentValues(directiveNode, directiveDef.args, variableValues); } } diff --git a/src/validation/rules/SingleFieldSubscriptionsRule.ts b/src/validation/rules/SingleFieldSubscriptionsRule.ts index 21cb1abaf6..ff9f33fec7 100644 --- a/src/validation/rules/SingleFieldSubscriptionsRule.ts +++ b/src/validation/rules/SingleFieldSubscriptionsRule.ts @@ -57,20 +57,20 @@ export function SingleFieldSubscriptionsRule( operationName != null ? `Subscription "${operationName}" must select only one top level field.` : 'Anonymous Subscription must select only one top level field.', - { nodes: extraFieldSelections }, + { nodes: extraFieldSelections.map(entry => entry.node) }, ), ); } for (const fieldNodes of fields.values()) { const field = fieldNodes[0]; - const fieldName = field.name.value; + const fieldName = field.node.name.value; if (fieldName.startsWith('__')) { context.reportError( new GraphQLError( operationName != null ? `Subscription "${operationName}" must not select an introspection top level field.` : 'Anonymous Subscription must not select an introspection top level field.', - { nodes: fieldNodes }, + { nodes: fieldNodes.map(entry => entry.node) }, ), ); } From 7819043e32f0f6f375c3ee09b9e6ded8c5f8050f Mon Sep 17 00:00:00 2001 From: jdecroock Date: Wed, 7 Aug 2024 14:25:37 +0200 Subject: [PATCH 3/6] Add typeinfo --- src/utilities/TypeInfo.ts | 79 ++++++++- src/utilities/__tests__/TypeInfo-test.ts | 212 +++++++++++++++++++++++ 2 files changed, 284 insertions(+), 7 deletions(-) diff --git a/src/utilities/TypeInfo.ts b/src/utilities/TypeInfo.ts index e72dfb01fb..979f506e74 100644 --- a/src/utilities/TypeInfo.ts +++ b/src/utilities/TypeInfo.ts @@ -1,6 +1,12 @@ import type { Maybe } from '../jsutils/Maybe'; +import type { ObjMap } from '../jsutils/ObjMap'; -import type { ASTNode, FieldNode } from '../language/ast'; +import type { + ASTNode, + FieldNode, + FragmentDefinitionNode, + FragmentSpreadNode, +} from '../language/ast'; import { isNode } from '../language/ast'; import { Kind } from '../language/kinds'; import type { ASTVisitor } from '../language/visitor'; @@ -37,6 +43,7 @@ import { import type { GraphQLSchema } from '../type/schema'; import { typeFromAST } from './typeFromAST'; +import { valueFromAST } from './valueFromAST'; /** * TypeInfo is a utility class which, given a GraphQL schema, can keep track @@ -53,6 +60,8 @@ export class TypeInfo { private _directive: Maybe; private _argument: Maybe; private _enumValue: Maybe; + private _fragmentSpread: Maybe; + private _fragmentDefinitions: ObjMap; private _getFieldDef: GetFieldDefFn; constructor( @@ -75,6 +84,8 @@ export class TypeInfo { this._directive = null; this._argument = null; this._enumValue = null; + this._fragmentSpread = null; + this._fragmentDefinitions = {}; this._getFieldDef = getFieldDefFn ?? getFieldDef; if (initialType) { if (isInputType(initialType)) { @@ -148,6 +159,17 @@ 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: { + // A document's fragment definitions are type signatures + // referenced via fragment spreads. Ensure we can use definitions + // before visiting their call sites. + for (const astNode of node.definitions) { + if (astNode.kind === Kind.FRAGMENT_DEFINITION) { + this._fragmentDefinitions[astNode.name.value] = astNode; + } + } + break; + } case Kind.SELECTION_SET: { const namedType: unknown = getNamedType(this.getType()); this._parentTypeStack.push( @@ -177,6 +199,10 @@ export class TypeInfo { this._typeStack.push(isObjectType(rootType) ? rootType : undefined); break; } + case Kind.FRAGMENT_SPREAD: { + this._fragmentSpread = node; + break; + } case Kind.INLINE_FRAGMENT: case Kind.FRAGMENT_DEFINITION: { const typeConditionAST = node.typeCondition; @@ -196,15 +222,48 @@ export class TypeInfo { case Kind.ARGUMENT: { let argDef; let argType: unknown; - const fieldOrDirective = this.getDirective() ?? this.getFieldDef(); - if (fieldOrDirective) { - argDef = fieldOrDirective.args.find( - (arg) => arg.name === node.name.value, + const directive = this.getDirective(); + const fragmentSpread = this._fragmentSpread; + const fieldDef = this.getFieldDef(); + if (directive) { + argDef = directive.args.find((arg) => arg.name === node.name.value); + } else if (fragmentSpread) { + const fragmentDef = this._fragmentDefinitions[fragmentSpread.name.value] + const fragVarDef = fragmentDef?.variableDefinitions?.find( + (varDef) => varDef.variable.name.value === node.name.value, ); - if (argDef) { - argType = argDef.type; + + if (fragVarDef) { + const fragVarType = typeFromAST(schema, fragVarDef.type); + if (isInputType(fragVarType)) { + const fragVarDefault = fragVarDef.defaultValue + ? valueFromAST(fragVarDef.defaultValue, fragVarType) + : undefined; + + const schemaArgDef: GraphQLArgument = { + name: fragVarDef.variable.name.value, + type: fragVarType, + defaultValue: fragVarDefault, + description: undefined, + deprecationReason: undefined, + extensions: {}, + astNode: { + ...fragVarDef, + kind: Kind.INPUT_VALUE_DEFINITION, + name: fragVarDef.variable.name, + }, + }; + argDef = schemaArgDef; + } } + } else if (fieldDef) { + argDef = fieldDef.args.find((arg) => arg.name === node.name.value); + } + + if (argDef) { + argType = argDef.type; } + this._argument = argDef; this._defaultValueStack.push(argDef ? argDef.defaultValue : undefined); this._inputTypeStack.push(isInputType(argType) ? argType : undefined); @@ -254,6 +313,9 @@ export class TypeInfo { leave(node: ASTNode) { switch (node.kind) { + case Kind.DOCUMENT: + this._fragmentDefinitions = {}; + break; case Kind.SELECTION_SET: this._parentTypeStack.pop(); break; @@ -264,6 +326,9 @@ export class TypeInfo { case Kind.DIRECTIVE: this._directive = null; break; + case Kind.FRAGMENT_SPREAD: + this._fragmentSpread = null; + break; case Kind.OPERATION_DEFINITION: case Kind.INLINE_FRAGMENT: case Kind.FRAGMENT_DEFINITION: diff --git a/src/utilities/__tests__/TypeInfo-test.ts b/src/utilities/__tests__/TypeInfo-test.ts index 5c04458c51..0f7ed49f20 100644 --- a/src/utilities/__tests__/TypeInfo-test.ts +++ b/src/utilities/__tests__/TypeInfo-test.ts @@ -457,4 +457,216 @@ 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) + } + 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', 'Argument', null, 'QueryRoot', 'ID!'], + ['enter', 'Name', 'x', 'QueryRoot', 'ID!'], + ['leave', 'Name', 'x', 'QueryRoot', 'ID!'], + ['enter', 'IntValue', null, 'QueryRoot', 'ID!'], + ['leave', 'IntValue', null, 'QueryRoot', 'ID!'], + ['leave', 'Argument', 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', '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', 'Argument', null, 'QueryRoot', 'ID'], + ['enter', 'Name', 'x', 'QueryRoot', 'ID'], + ['leave', 'Name', 'x', 'QueryRoot', 'ID'], + ['enter', 'NullValue', null, 'QueryRoot', 'ID'], + ['leave', 'NullValue', null, 'QueryRoot', 'ID'], + ['leave', 'Argument', 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'], + ]); + }); }); From aba2bbb07ba88cef57d0a2c290978bf4d9ee8a85 Mon Sep 17 00:00:00 2001 From: jdecroock Date: Wed, 7 Aug 2024 14:27:35 +0200 Subject: [PATCH 4/6] Lint files --- src/execution/collectFields.ts | 16 ++++-- src/execution/execute.ts | 49 ++++++++++++++----- src/execution/subscribe.ts | 14 ++++-- src/execution/values.ts | 13 +++-- src/language/__tests__/printer-test.ts | 3 +- src/language/parser.ts | 6 +-- src/language/printer.ts | 5 +- src/utilities/TypeInfo.ts | 3 +- .../rules/SingleFieldSubscriptionsRule.ts | 4 +- 9 files changed, 81 insertions(+), 32 deletions(-) diff --git a/src/execution/collectFields.ts b/src/execution/collectFields.ts index 7c9d608bfe..004a0337aa 100644 --- a/src/execution/collectFields.ts +++ b/src/execution/collectFields.ts @@ -51,7 +51,7 @@ export function collectFields( selectionSet, fields, new Set(), - undefined + undefined, ); return fields; } @@ -111,9 +111,17 @@ function collectFieldsImpl( const name = getFieldEntryKey(selection); const fieldList = fields.get(name); if (fieldList !== undefined) { - fieldList.push({ node: selection, fragmentVariableValues: localVariableValues ?? undefined }); + fieldList.push({ + node: selection, + fragmentVariableValues: localVariableValues ?? undefined, + }); } else { - fields.set(name, [{ node: selection, fragmentVariableValues: localVariableValues ?? undefined }]); + fields.set(name, [ + { + node: selection, + fragmentVariableValues: localVariableValues ?? undefined, + }, + ]); } break; } @@ -179,7 +187,7 @@ function collectFieldsImpl( fragment.selectionSet, fields, visitedFragmentNames, - fragmentVariableValues + fragmentVariableValues, ); break; } diff --git a/src/execution/execute.ts b/src/execution/execute.ts index 0a501dab0b..d11900976b 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -52,10 +52,10 @@ import { import type { GraphQLSchema } from '../type/schema'; import { assertValidSchema } from '../type/validate'; +import type { FieldDetails } from './collectFields'; import { collectFields, collectSubfields as _collectSubfields, - FieldDetails, } from './collectFields'; import { getArgumentValues, getVariableValues } from './values'; @@ -523,7 +523,7 @@ function executeField( fieldEntries[0].node, fieldDef.args, exeContext.variableValues, - fieldEntries[0].fragmentVariableValues + fieldEntries[0].fragmentVariableValues, ); // The resolve function's optional third argument is a context value that @@ -536,7 +536,14 @@ function executeField( let completed; if (isPromise(result)) { completed = result.then((resolved) => - completeValue(exeContext, returnType, fieldEntries, info, path, resolved), + completeValue( + exeContext, + returnType, + fieldEntries, + info, + path, + resolved, + ), ); } else { completed = completeValue( @@ -553,13 +560,21 @@ function executeField( // Note: we don't rely on a `catch` method, but we do expect "thenable" // to take a second callback for the error case. return completed.then(undefined, (rawError) => { - const error = locatedError(rawError, fieldEntries.map(entry => entry.node), pathToArray(path)); + const error = locatedError( + rawError, + fieldEntries.map((entry) => entry.node), + pathToArray(path), + ); return handleFieldError(error, returnType, exeContext); }); } return completed; } catch (rawError) { - const error = locatedError(rawError, fieldEntries.map(entry => entry.node), pathToArray(path)); + const error = locatedError( + rawError, + fieldEntries.map((entry) => entry.node), + pathToArray(path), + ); return handleFieldError(error, returnType, exeContext); } } @@ -578,7 +593,7 @@ export function buildResolveInfo( // information about the current execution state. return { fieldName: fieldDef.name, - fieldNodes: fieldEntries.map(entry => entry.node), + fieldNodes: fieldEntries.map((entry) => entry.node), returnType: fieldDef.type, parentType, path, @@ -772,7 +787,7 @@ function completeListValue( return completedItem.then(undefined, (rawError) => { const error = locatedError( rawError, - fieldEntries.map(entry => entry.node), + fieldEntries.map((entry) => entry.node), pathToArray(itemPath), ); return handleFieldError(error, itemType, exeContext); @@ -780,7 +795,11 @@ function completeListValue( } return completedItem; } catch (rawError) { - const error = locatedError(rawError, fieldEntries.map(entry => entry.node), pathToArray(itemPath)); + const error = locatedError( + rawError, + fieldEntries.map((entry) => entry.node), + pathToArray(itemPath), + ); return handleFieldError(error, itemType, exeContext); } }); @@ -822,7 +841,7 @@ function completeAbstractValue( const contextValue = exeContext.contextValue; const runtimeType = resolveTypeFn(result, contextValue, info, returnType); - const fieldNodes = fieldEntries.map(entry => entry.node) + const fieldNodes = fieldEntries.map((entry) => entry.node); if (isPromise(runtimeType)) { return runtimeType.then((resolvedRuntimeType) => completeObjectValue( @@ -938,7 +957,11 @@ function completeObjectValue( if (isPromise(isTypeOf)) { return isTypeOf.then((resolvedIsTypeOf) => { if (!resolvedIsTypeOf) { - throw invalidReturnTypeError(returnType, result, fieldEntries.map(entry => entry.node)); + throw invalidReturnTypeError( + returnType, + result, + fieldEntries.map((entry) => entry.node), + ); } return executeFields( exeContext, @@ -951,7 +974,11 @@ function completeObjectValue( } if (!isTypeOf) { - throw invalidReturnTypeError(returnType, result, fieldEntries.map(entry => entry.node)); + throw invalidReturnTypeError( + returnType, + result, + fieldEntries.map((entry) => entry.node), + ); } } diff --git a/src/execution/subscribe.ts b/src/execution/subscribe.ts index 7b42278a96..caf3c7617f 100644 --- a/src/execution/subscribe.ts +++ b/src/execution/subscribe.ts @@ -221,7 +221,7 @@ async function executeSubscription( const fieldName = fieldEntries[0].node.name.value; throw new GraphQLError( `The subscription field "${fieldName}" is not defined.`, - { nodes: fieldEntries.map(entry => entry.node) }, + { nodes: fieldEntries.map((entry) => entry.node) }, ); } @@ -240,7 +240,11 @@ async function executeSubscription( // Build a JS object of arguments from the field.arguments AST, using the // variables scope to fulfill any variable references. - const args = getArgumentValues(fieldEntries[0].node, fieldDef.args, variableValues); + const args = getArgumentValues( + fieldEntries[0].node, + fieldDef.args, + variableValues, + ); // The resolve function's optional third argument is a context value that // is provided to every resolve function within an execution. It is commonly @@ -257,6 +261,10 @@ async function executeSubscription( } return eventStream; } catch (error) { - throw locatedError(error, fieldEntries.map(entry => entry.node), pathToArray(path)); + throw locatedError( + error, + fieldEntries.map((entry) => entry.node), + pathToArray(path), + ); } } diff --git a/src/execution/values.ts b/src/execution/values.ts index e37ea168b2..c1050cc6ef 100644 --- a/src/execution/values.ts +++ b/src/execution/values.ts @@ -15,7 +15,7 @@ import type { import { Kind } from '../language/kinds'; import { print } from '../language/printer'; -import type { GraphQLArgument, GraphQLField } from '../type/definition'; +import type { GraphQLArgument } from '../type/definition'; import { isInputType, isNonNullType } from '../type/definition'; import type { GraphQLDirective } from '../type/directives'; import type { GraphQLSchema } from '../type/schema'; @@ -23,7 +23,7 @@ import type { GraphQLSchema } from '../type/schema'; import { coerceInputValue } from '../utilities/coerceInputValue'; import { typeFromAST } from '../utilities/typeFromAST'; import { valueFromAST } from '../utilities/valueFromAST'; -import { valueFromASTUntyped } from '../utilities'; +import { valueFromASTUntyped } from '../utilities/valueFromASTUntyped'; type CoercedVariableValues = | { errors: ReadonlyArray; coerced?: never } @@ -191,12 +191,15 @@ export function getArgumentValues( fragmentArgValues != null && hasOwnProperty(fragmentArgValues, variableName) ) { - hasValue = fragmentArgValues![variableName] != null; + hasValue = fragmentArgValues[variableName] != null; if (!hasValue && argDef.defaultValue !== undefined) { coercedValues[name] = argDef.defaultValue; continue; } - } else if (variableValues != null && hasOwnProperty(variableValues, variableName)) { + } else if ( + variableValues != null && + hasOwnProperty(variableValues, variableName) + ) { hasValue = variableValues[variableName] != null; } else if (argDef.defaultValue !== undefined) { coercedValues[name] = argDef.defaultValue; @@ -248,7 +251,7 @@ export function getArgumentValuesFromSpread( fragmentArgValues?: Maybe>, ): { [argument: string]: unknown } { const coercedValues: { [argument: string]: unknown } = {}; - const argNodeMap = keyMap(node?.arguments || [], (arg) => arg.name.value); + const argNodeMap = keyMap(node?.arguments ?? [], (arg) => arg.name.value); for (const varDef of fragmentVarDefs) { const name = varDef.variable.name.value; diff --git a/src/language/__tests__/printer-test.ts b/src/language/__tests__/printer-test.ts index fc45657166..13ad9ce6c6 100644 --- a/src/language/__tests__/printer-test.ts +++ b/src/language/__tests__/printer-test.ts @@ -239,7 +239,6 @@ describe('Printer: Query document', () => { c: "a long string extending arguments over max length" ) } - ` - ); + `); }); }); diff --git a/src/language/parser.ts b/src/language/parser.ts index 045569138f..d4504d2271 100644 --- a/src/language/parser.ts +++ b/src/language/parser.ts @@ -123,7 +123,7 @@ export interface ParseOptions { * } * ``` */ - experimentalFragmentArguments?: boolean | undefined; + experimentalFragmentArguments?: boolean | undefined; } /** @@ -555,13 +555,13 @@ export class Parser { kind: Kind.FRAGMENT_DEFINITION, name: this.parseFragmentName(), variableDefinitions: - this._options.experimentalFragmentArguments === true || this._options.allowLegacyFragmentVariables === true + this._options.experimentalFragmentArguments === true || + this._options.allowLegacyFragmentVariables === true ? this.parseVariableDefinitions() : undefined, 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 476187f09e..673d3e69fc 100644 --- a/src/language/printer.ts +++ b/src/language/printer.ts @@ -58,7 +58,10 @@ const printDocASTReducer: ASTReducer = { leave({ alias, name, arguments: args, directives, selectionSet }) { const prefix = wrap('', alias, ': ') + name; - return join([wrappedLineAndArgs(prefix, args), join(directives, ' '), selectionSet], ' '); + return join( + [wrappedLineAndArgs(prefix, args), join(directives, ' '), selectionSet], + ' ', + ); }, }, diff --git a/src/utilities/TypeInfo.ts b/src/utilities/TypeInfo.ts index 979f506e74..160a93fffb 100644 --- a/src/utilities/TypeInfo.ts +++ b/src/utilities/TypeInfo.ts @@ -228,7 +228,8 @@ export class TypeInfo { if (directive) { argDef = directive.args.find((arg) => arg.name === node.name.value); } else if (fragmentSpread) { - const fragmentDef = this._fragmentDefinitions[fragmentSpread.name.value] + const fragmentDef = + this._fragmentDefinitions[fragmentSpread.name.value]; const fragVarDef = fragmentDef?.variableDefinitions?.find( (varDef) => varDef.variable.name.value === node.name.value, ); diff --git a/src/validation/rules/SingleFieldSubscriptionsRule.ts b/src/validation/rules/SingleFieldSubscriptionsRule.ts index ff9f33fec7..a93aeeb224 100644 --- a/src/validation/rules/SingleFieldSubscriptionsRule.ts +++ b/src/validation/rules/SingleFieldSubscriptionsRule.ts @@ -57,7 +57,7 @@ export function SingleFieldSubscriptionsRule( operationName != null ? `Subscription "${operationName}" must select only one top level field.` : 'Anonymous Subscription must select only one top level field.', - { nodes: extraFieldSelections.map(entry => entry.node) }, + { nodes: extraFieldSelections.map((entry) => entry.node) }, ), ); } @@ -70,7 +70,7 @@ export function SingleFieldSubscriptionsRule( operationName != null ? `Subscription "${operationName}" must not select an introspection top level field.` : 'Anonymous Subscription must not select an introspection top level field.', - { nodes: fieldNodes.map(entry => entry.node) }, + { nodes: fieldNodes.map((entry) => entry.node) }, ), ); } From 97b9a8f032aa340c7e74bc36292aab209b3c0366 Mon Sep 17 00:00:00 2001 From: jdecroock Date: Thu, 8 Aug 2024 09:52:31 +0200 Subject: [PATCH 5/6] Implement validation --- src/validation/ValidationContext.ts | 30 +- .../NoUndefinedVariablesRule-test.ts | 63 ++++ .../NoUnusedFragmentVariablesRule-test.ts | 37 +++ .../__tests__/NoUnusedVariablesRule-test.ts | 22 ++ .../OverlappingFieldsCanBeMergedRule-test.ts | 132 ++++++++ .../ProvidedRequiredArgumentsRule-test.ts | 104 +++++++ .../__tests__/ValuesOfCorrectTypeRule-test.ts | 31 ++ .../VariablesAreInputTypesRule-test.ts | 27 ++ .../VariablesInAllowedPositionRule-test.ts | 104 +++++++ src/validation/index.ts | 3 + .../rules/NoUndefinedVariablesRule.ts | 5 +- .../rules/NoUnusedFragmentVariablesRule.ts | 40 +++ src/validation/rules/NoUnusedVariablesRule.ts | 4 +- .../rules/OverlappingFieldsCanBeMergedRule.ts | 287 ++++++++++++------ .../rules/ProvidedRequiredArgumentsRule.ts | 40 ++- .../rules/VariablesInAllowedPositionRule.ts | 4 +- src/validation/specifiedRules.ts | 3 + 17 files changed, 832 insertions(+), 104 deletions(-) create mode 100644 src/validation/__tests__/NoUnusedFragmentVariablesRule-test.ts create mode 100644 src/validation/rules/NoUnusedFragmentVariablesRule.ts diff --git a/src/validation/ValidationContext.ts b/src/validation/ValidationContext.ts index f6944a6ebd..27a3d456c0 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'; import { Kind } from '../language/kinds'; @@ -26,13 +27,15 @@ import type { import type { GraphQLDirective } from '../type/directives'; import type { GraphQLSchema } from '../type/schema'; -import { TypeInfo, visitWithTypeInfo } from '../utilities/TypeInfo'; +import type { TypeInfo } from '../utilities/TypeInfo'; +import { visitWithTypeInfo } from '../utilities/TypeInfo'; type NodeWithSelectionSet = OperationDefinitionNode | FragmentDefinitionNode; interface VariableUsage { readonly node: VariableNode; readonly type: Maybe; readonly defaultValue: Maybe; + readonly fragmentVarDef: Maybe; } /** @@ -199,16 +202,25 @@ export class ValidationContext extends ASTValidationContext { let usages = this._variableUsages.get(node); if (!usages) { const newUsages: Array = []; - const typeInfo = new TypeInfo(this._schema); + const typeInfo = this._typeInfo; + const fragmentVariableDefinitions = + node.kind === Kind.FRAGMENT_DEFINITION + ? node.variableDefinitions + : undefined; + visit( node, visitWithTypeInfo(typeInfo, { VariableDefinition: () => false, Variable(variable) { + const fragmentVarDef = fragmentVariableDefinitions?.find( + (varDef) => varDef.variable.name.value === variable.name.value, + ); newUsages.push({ node: variable, type: typeInfo.getInputType(), defaultValue: typeInfo.getDefaultValue(), + fragmentVarDef, }); }, }), @@ -233,6 +245,20 @@ export class ValidationContext extends ASTValidationContext { return usages; } + getOperationVariableUsages( + operation: OperationDefinitionNode, + ): ReadonlyArray { + let usages = this._recursiveVariableUsages.get(operation); + if (!usages) { + usages = this.getVariableUsages(operation); + for (const frag of this.getRecursivelyReferencedFragments(operation)) { + usages = usages.concat(this.getVariableUsages(frag)); + } + this._recursiveVariableUsages.set(operation, usages); + } + return usages.filter(({ fragmentVarDef }) => !fragmentVarDef); + } + getType(): Maybe { return this._typeInfo.getType(); } diff --git a/src/validation/__tests__/NoUndefinedVariablesRule-test.ts b/src/validation/__tests__/NoUndefinedVariablesRule-test.ts index e027d4a49b..ca69fe0789 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__/NoUnusedFragmentVariablesRule-test.ts b/src/validation/__tests__/NoUnusedFragmentVariablesRule-test.ts new file mode 100644 index 0000000000..d7b0060e21 --- /dev/null +++ b/src/validation/__tests__/NoUnusedFragmentVariablesRule-test.ts @@ -0,0 +1,37 @@ +import { describe, it } from 'mocha'; + +import { NoUnusedFragmentVariablesRule } from '../rules/NoUnusedFragmentVariablesRule'; + +import { expectValidationErrors } from './harness'; + +function expectErrors(queryStr: string) { + return expectValidationErrors(NoUnusedFragmentVariablesRule, queryStr); +} + +function expectValid(queryStr: string) { + expectErrors(queryStr).toDeepEqual([]); +} + +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', () => { + expectErrors(` + query Foo($b: String) { + ...FragA(a: $b) + } + fragment FragA($a: String) on Type { + field1 + } + `); + }); +}); diff --git a/src/validation/__tests__/NoUnusedVariablesRule-test.ts b/src/validation/__tests__/NoUnusedVariablesRule-test.ts index 6be63cd23d..6b95847e93 100644 --- a/src/validation/__tests__/NoUnusedVariablesRule-test.ts +++ b/src/validation/__tests__/NoUnusedVariablesRule-test.ts @@ -230,4 +230,26 @@ 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 + } + `); + }); }); diff --git a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts index 46cf014e46..8d28bb565c 100644 --- a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts +++ b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts @@ -1138,4 +1138,136 @@ 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('encounters conflict in fragment - field no args', () => { + expectErrors(` + query ($y: Int = 1) { + a(x: $y) + ...WithArgs + } + fragment WithArgs on Type { + a(x: $y) + } + `).toDeepEqual([]); + }); + + it('encounters conflict in fragment/field', () => { + expectErrors(` + query ($y: Int = 1) { + a(x: $y) + ...WithArgs(x: $y) + } + fragment WithArgs($x: Int) on Type { + a(x: $x) + } + `).toDeepEqual([]); + }); + + // This is currently not validated, should we? + 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 23a272572c..1cfedeeada 100644 --- a/src/validation/__tests__/ProvidedRequiredArgumentsRule-test.ts +++ b/src/validation/__tests__/ProvidedRequiredArgumentsRule-test.ts @@ -353,4 +353,108 @@ describe('Validate: Provided required arguments', () => { ]); }); }); + + describe('Fragment required arguments', () => { + it('ignores unknown arguments', () => { + expectValid(` + { + ...Foo(unknownArgument: true) + } + fragment Foo on Query { + dog + } + `); + }); + + // Query: should this be allowed? + // We could differentiate between required/optional (i.e. no default value) + // vs. nullable/non-nullable (i.e. no !), whereas now they are conflated. + // So today: + // $x: Int! `x:` is required and must not be null (NOT a nullable variable) + // $x: Int! = 3 `x:` is not required and must not be null (MAY BE a nullable variable) + // $x: Int `x:` is not required and may be null + // $x: Int = 3 `x:` is not required and may be null + // + // It feels weird to collapse the nullable cases but not the non-nullable ones. + // Whereas all four feel like they ought to mean something explicitly different. + // + // Potential proposal: + // $x: Int! `x:` is required and must not be null (NOT a nullable variable) + // $x: Int! = 3 `x:` is not required and must not be null (NOT a nullable variable) + // $x: Int `x:` is required and may be null + // $x: Int = 3 `x:` is not required and may be null + // + // Required then is whether there's a default value, + // and nullable is whether there's a ! + it('Missing nullable argument with default is allowed', () => { + expectValid(` + { + ...F + } + fragment F($x: Int = 3) on Query { + foo + } + `); + }); + // Above proposal: this should be an error + 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 "{ kind: "NonNullType", type: { kind: "NamedType", name: [Object], loc: [Object] }, loc: [Object] }" is required, but it was not provided.', + locations: [ + { line: 3, column: 13 }, + { line: 5, column: 22 }, + ], + }, + ]); + }); + + 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 3610fa648b..ab9da66ca5 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 7b754fd76f..226613720f 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 090f1680c4..3af5ef4e76 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 587479e351..14646ac270 100644 --- a/src/validation/index.ts +++ b/src/validation/index.ts @@ -51,6 +51,9 @@ export { PossibleFragmentSpreadsRule } from './rules/PossibleFragmentSpreadsRule // Spec Section: "Argument Optionality" export { ProvidedRequiredArgumentsRule } from './rules/ProvidedRequiredArgumentsRule'; +// Spec Section: "All Fragment Variables Used" +export { NoUnusedFragmentVariablesRule } from './rules/NoUnusedFragmentVariablesRule'; + // Spec Section: "Leaf Field Selections" export { ScalarLeafsRule } from './rules/ScalarLeafsRule'; diff --git a/src/validation/rules/NoUndefinedVariablesRule.ts b/src/validation/rules/NoUndefinedVariablesRule.ts index 3d499b5dcc..82692a3a91 100644 --- a/src/validation/rules/NoUndefinedVariablesRule.ts +++ b/src/validation/rules/NoUndefinedVariablesRule.ts @@ -25,7 +25,10 @@ export function NoUndefinedVariablesRule( leave(operation) { const usages = context.getRecursiveVariableUsages(operation); - for (const { node } of usages) { + for (const { node, fragmentVarDef } of usages) { + if (fragmentVarDef) { + continue; + } const varName = node.name.value; if (variableNameDefined[varName] !== true) { context.reportError( diff --git a/src/validation/rules/NoUnusedFragmentVariablesRule.ts b/src/validation/rules/NoUnusedFragmentVariablesRule.ts new file mode 100644 index 0000000000..9cfde886bd --- /dev/null +++ b/src/validation/rules/NoUnusedFragmentVariablesRule.ts @@ -0,0 +1,40 @@ +import { GraphQLError } from '../../error/GraphQLError'; + +import type { ASTVisitor } from '../../language/visitor'; + +import type { ValidationContext } from '../ValidationContext'; + +/** + * No unused variables + * + * A GraphQL fragment is only valid if all arguments defined by it + * are used within the same fragment. + * + * See https://spec.graphql.org/draft/#sec-All-Fragment-Variables-Used + */ +export function NoUnusedFragmentVariablesRule( + context: ValidationContext, +): ASTVisitor { + return { + 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 }, + ), + ); + } + } + }, + }; +} diff --git a/src/validation/rules/NoUnusedVariablesRule.ts b/src/validation/rules/NoUnusedVariablesRule.ts index 5083af4f28..c2f58d05b0 100644 --- a/src/validation/rules/NoUnusedVariablesRule.ts +++ b/src/validation/rules/NoUnusedVariablesRule.ts @@ -11,7 +11,7 @@ import type { ValidationContext } from '../ValidationContext'; * A GraphQL operation is only valid if all variables defined by an operation * are used, either directly or within a spread fragment. * - * See https://spec.graphql.org/draft/#sec-All-Variables-Used + * See https://spec.graphql.org/draft/#sec-All-Operation-Variables-Used */ export function NoUnusedVariablesRule(context: ValidationContext): ASTVisitor { let variableDefs: Array = []; @@ -23,7 +23,7 @@ export function NoUnusedVariablesRule(context: ValidationContext): ASTVisitor { }, leave(operation) { const variableNameUsed = Object.create(null); - const usages = context.getRecursiveVariableUsages(operation); + const usages = context.getOperationVariableUsages(operation); for (const { node } of usages) { variableNameUsed[node.name.value] = true; diff --git a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts index 4305064a6f..0ac766b95c 100644 --- a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts +++ b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts @@ -8,12 +8,14 @@ import type { DirectiveNode, FieldNode, FragmentDefinitionNode, + FragmentSpreadNode, SelectionSetNode, ValueNode, } from '../../language/ast'; import { Kind } from '../../language/kinds'; import { print } from '../../language/printer'; import type { ASTVisitor } from '../../language/visitor'; +import { visit } from '../../language/visitor'; import type { GraphQLField, @@ -67,13 +69,13 @@ export function OverlappingFieldsCanBeMergedRule( // A cache for the "field map" and list of fragment names 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, @@ -104,8 +106,22 @@ type NodeAndDef = [ ]; // Map of array of those. type NodeAndDefCollection = ObjMap>; -type FragmentNames = Array; -type FieldsAndFragmentNames = readonly [NodeAndDefCollection, FragmentNames]; +type FragmentSpreadsByName = ObjMap>; +type FieldsAndFragmentSpreads = readonly [ + NodeAndDefCollection, + FragmentSpreadsByName, +]; + +const printFragmentSpreadArguments = (fragmentSpread: FragmentSpreadNode) => { + if (!fragmentSpread.arguments || fragmentSpread.arguments.length === 0) { + return fragmentSpread.name.value; + } + + const printedArguments: Array = fragmentSpread.arguments + .map(print) + .sort((a, b) => a.localeCompare(b)); + return fragmentSpread.name.value + '(' + printedArguments.join(',') + ')'; +}; /** * Algorithm: @@ -167,56 +183,64 @@ 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, fragmentSpreadMap] = getFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, parentType, selectionSet, ); - // (A) Find find all conflicts "within" the fields of this selection set. + // (A) First find all conflicts "within" the fields and fragment-spreads of this selection set. // Note: this is the *only place* `collectConflictsWithin` is called. collectConflictsWithin( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, fieldMap, ); - if (fragmentNames.length !== 0) { + const allFragmentSpreads = []; + for (const [, fragmentSpreads] of Object.entries(fragmentSpreadMap)) { + allFragmentSpreads.push(...fragmentSpreads); + } + + if (allFragmentSpreads.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++) { + for (let i = 0; i < allFragmentSpreads.length; i++) { collectConflictsBetweenFieldsAndFragment( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, false, fieldMap, - fragmentNames[i], + allFragmentSpreads[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 // item in that same list (except for itself). - for (let j = i + 1; j < fragmentNames.length; j++) { + for (let j = i + 1; j < allFragmentSpreads.length; j++) { collectConflictsBetweenFragments( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, false, - fragmentNames[i], - fragmentNames[j], + allFragmentSpreads[i], + allFragmentSpreads[j], ); } } @@ -229,22 +253,27 @@ function findConflictsWithinSelectionSet( function collectConflictsBetweenFieldsAndFragment( context: ValidationContext, conflicts: Array, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, fieldMap: NodeAndDefCollection, - fragmentName: string, + fragmentSpread: FragmentSpreadNode, ): void { + const fragmentName = fragmentSpread.name.value; const fragment = context.getFragment(fragmentName); if (!fragment) { return; } const [fieldMap2, referencedFragmentNames] = - getReferencedFieldsAndFragmentNames( + getReferencedFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, fragment, + fragmentSpread, ); // Do not compare a fragment's fieldMap to itself. @@ -257,7 +286,7 @@ function collectConflictsBetweenFieldsAndFragment( collectConflictsBetween( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, fieldMap, @@ -266,7 +295,9 @@ function collectConflictsBetweenFieldsAndFragment( // (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) { + for (const [referencedFragmentName, [spread]] of Object.entries( + referencedFragmentNames, + )) { // Memoize so two fragments are not compared for conflicts more than once. if ( comparedFragmentPairs.has( @@ -286,11 +317,11 @@ function collectConflictsBetweenFieldsAndFragment( collectConflictsBetweenFieldsAndFragment( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, fieldMap, - referencedFragmentName, + spread, ); } } @@ -300,28 +331,49 @@ function collectConflictsBetweenFieldsAndFragment( function collectConflictsBetweenFragments( context: ValidationContext, conflicts: Array, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, - fragmentName1: string, - fragmentName2: string, + fragmentSpread1: FragmentSpreadNode, + fragmentSpread2: FragmentSpreadNode, ): void { - // No need to compare a fragment to itself. - if (fragmentName1 === fragmentName2) { + const fragmentName1 = fragmentSpread1.name.value; + const fragmentName2 = fragmentSpread2.name.value; + if ( + fragmentName1 === fragmentName2 && + !sameArguments(fragmentSpread1, fragmentSpread2) + ) { + context.reportError( + new GraphQLError( + `Spreads "${fragmentName1}" conflict because ${printFragmentSpreadArguments( + fragmentSpread1, + )} and ${printFragmentSpreadArguments( + fragmentSpread2, + )} have different fragment arguments.`, + { nodes: [fragmentSpread1, fragmentSpread2] }, + ), + ); return; } - // Memoize so two fragments are not compared for conflicts more than once. + // No need to compare a fragment to itself. if ( - comparedFragmentPairs.has( - fragmentName1, - fragmentName2, - areMutuallyExclusive, - ) + fragmentName1 === fragmentName2 && + sameArguments(fragmentSpread1, fragmentSpread2) ) { return; } - comparedFragmentPairs.add(fragmentName1, fragmentName2, areMutuallyExclusive); + + const fragKey1 = printFragmentSpreadArguments(fragmentSpread1); + const fragKey2 = printFragmentSpreadArguments(fragmentSpread2); + // Memoize so two fragments are not compared for conflicts more than once. + if (comparedFragmentPairs.has(fragKey1, fragKey2, areMutuallyExclusive)) { + return; + } + comparedFragmentPairs.add(fragKey1, fragKey2, areMutuallyExclusive); const fragment1 = context.getFragment(fragmentName1); const fragment2 = context.getFragment(fragmentName2); @@ -329,17 +381,19 @@ function collectConflictsBetweenFragments( return; } - const [fieldMap1, referencedFragmentNames1] = - getReferencedFieldsAndFragmentNames( + const [fieldMap1, referencedFragmentSpreads1] = + getReferencedFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, fragment1, + fragmentSpread1, ); - const [fieldMap2, referencedFragmentNames2] = - getReferencedFieldsAndFragmentNames( + const [fieldMap2, referencedFragmentSpreads2] = + getReferencedFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, fragment2, + fragmentSpread2, ); // (F) First, collect all conflicts between these two collections of fields @@ -347,7 +401,7 @@ function collectConflictsBetweenFragments( collectConflictsBetween( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, fieldMap1, @@ -356,29 +410,33 @@ function collectConflictsBetweenFragments( // (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 Object.values( + 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 Object.values( + referencedFragmentSpreads1, + )) { collectConflictsBetweenFragments( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, - referencedFragmentName1, - fragmentName2, + referencedFragmentSpread1, + fragmentSpread2, ); } } @@ -388,7 +446,10 @@ 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, @@ -398,15 +459,15 @@ function findConflictsBetweenSubSelectionSets( ): Array { const conflicts: Array = []; - const [fieldMap1, fragmentNames1] = getFieldsAndFragmentNames( + const [fieldMap1, fragmentSpreadsByName1] = getFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, parentType1, selectionSet1, ); - const [fieldMap2, fragmentNames2] = getFieldsAndFragmentNames( + const [fieldMap2, fragmentSpreadsByName2] = getFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, parentType2, selectionSet2, ); @@ -415,7 +476,7 @@ function findConflictsBetweenSubSelectionSets( collectConflictsBetween( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, fieldMap1, @@ -424,45 +485,45 @@ function findConflictsBetweenSubSelectionSets( // (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 Object.values(fragmentSpreadsByName2)) { 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 Object.values(fragmentSpreadsByName1)) { 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) { + for (const [fragmentSpread1] of Object.values(fragmentSpreadsByName1)) { + for (const [fragmentSpread2] of Object.values(fragmentSpreadsByName2)) { collectConflictsBetweenFragments( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, - fragmentName1, - fragmentName2, + fragmentSpread1, + fragmentSpread2, ); } } @@ -473,7 +534,10 @@ function findConflictsBetweenSubSelectionSets( function collectConflictsWithin( context: ValidationContext, conflicts: Array, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, comparedFragmentPairs: PairSet, fieldMap: NodeAndDefCollection, ): void { @@ -490,7 +554,7 @@ 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, @@ -514,7 +578,10 @@ function collectConflictsWithin( function collectConflictsBetween( context: ValidationContext, conflicts: Array, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, comparedFragmentPairs: PairSet, parentFieldsAreMutuallyExclusive: boolean, fieldMap1: NodeAndDefCollection, @@ -532,7 +599,7 @@ function collectConflictsBetween( for (const field2 of fields2) { const conflict = findConflict( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, parentFieldsAreMutuallyExclusive, responseName, @@ -552,7 +619,10 @@ function collectConflictsBetween( // comparing their sub-fields. function findConflict( context: ValidationContext, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, comparedFragmentPairs: PairSet, parentFieldsAreMutuallyExclusive: boolean, responseName: string, @@ -623,7 +693,7 @@ function findConflict( if (selectionSet1 && selectionSet2) { const conflicts = findConflictsBetweenSubSelectionSets( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, getNamedType(type1), @@ -636,8 +706,8 @@ function findConflict( } function sameArguments( - node1: FieldNode | DirectiveNode, - node2: FieldNode | DirectiveNode, + node1: FieldNode | DirectiveNode | FragmentSpreadNode, + node2: FieldNode | DirectiveNode | FragmentSpreadNode, ): boolean { const args1 = node1.arguments; const args2 = node2.arguments; @@ -704,49 +774,70 @@ 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 // 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); +): FieldsAndFragmentSpreads { + const cached = cachedFieldsAndFragmentSpreads.get(selectionSet); if (cached) { return cached; } const nodeAndDefs: NodeAndDefCollection = Object.create(null); - const fragmentNames: ObjMap = Object.create(null); + const fragmentSpreadsByName: ObjMap> = + Object.create(null); _collectFieldsAndFragmentNames( context, parentType, selectionSet, nodeAndDefs, - fragmentNames, + fragmentSpreadsByName, ); - const result = [nodeAndDefs, Object.keys(fragmentNames)] as const; - cachedFieldsAndFragmentNames.set(selectionSet, result); + const result = [nodeAndDefs, fragmentSpreadsByName] as const; + 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( +function getReferencedFieldsAndFragmentSpreads( context: ValidationContext, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, fragment: FragmentDefinitionNode, + fragmentSpread: FragmentSpreadNode, ) { + const args = fragmentSpread.arguments; + const fragmentSelectionSet = visit(fragment.selectionSet, { + Variable: (node) => { + const name = node.name.value; + const argNode = args?.find((arg) => arg.name.value === name); + if (argNode) { + return argNode.value; + } + + return node; + }, + }); + // Short-circuit building a type from the node if possible. - const cached = cachedFieldsAndFragmentNames.get(fragment.selectionSet); + const cached = cachedFieldsAndFragmentSpreads.get(fragmentSelectionSet); if (cached) { return cached; } const fragmentType = typeFromAST(context.getSchema(), fragment.typeCondition); - return getFieldsAndFragmentNames( + return getFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, fragmentType, - fragment.selectionSet, + fragmentSelectionSet, ); } @@ -755,7 +846,7 @@ function _collectFieldsAndFragmentNames( parentType: Maybe, selectionSet: SelectionSetNode, nodeAndDefs: NodeAndDefCollection, - fragmentNames: ObjMap, + fragmentSpreadsByName: ObjMap>, ): void { for (const selection of selectionSet.selections) { switch (selection.kind) { @@ -774,9 +865,15 @@ function _collectFieldsAndFragmentNames( nodeAndDefs[responseName].push([parentType, selection, fieldDef]); break; } - case Kind.FRAGMENT_SPREAD: - fragmentNames[selection.name.value] = true; + case Kind.FRAGMENT_SPREAD: { + const existing = fragmentSpreadsByName[selection.name.value]; + if (existing) { + existing.push(selection); + } else { + fragmentSpreadsByName[selection.name.value] = [selection]; + } break; + } case Kind.INLINE_FRAGMENT: { const typeCondition = selection.typeCondition; const inlineFragmentType = typeCondition @@ -787,7 +884,7 @@ function _collectFieldsAndFragmentNames( inlineFragmentType, selection.selectionSet, nodeAndDefs, - fragmentNames, + fragmentSpreadsByName, ); break; } diff --git a/src/validation/rules/ProvidedRequiredArgumentsRule.ts b/src/validation/rules/ProvidedRequiredArgumentsRule.ts index b111dcee1b..7a7e8ff153 100644 --- a/src/validation/rules/ProvidedRequiredArgumentsRule.ts +++ b/src/validation/rules/ProvidedRequiredArgumentsRule.ts @@ -4,7 +4,10 @@ import type { ObjMap } from '../../jsutils/ObjMap'; import { GraphQLError } from '../../error/GraphQLError'; -import type { InputValueDefinitionNode } from '../../language/ast'; +import type { + InputValueDefinitionNode, + VariableDefinitionNode, +} from '../../language/ast'; import { Kind } from '../../language/kinds'; import { print } from '../../language/printer'; import type { ASTVisitor } from '../../language/visitor'; @@ -56,6 +59,37 @@ export function ProvidedRequiredArgumentsRule( } }, }, + FragmentSpread: { + // Validate on leave to allow for directive errors to appear first. + leave(spreadNode) { + const fragmentDef = context.getFragment(spreadNode.name.value); + if (!fragmentDef) { + 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), + ); + // FIXME: https://github.com/graphql/graphql-js/issues/2203 + /* c8 ignore next */ + for (const varDef of fragmentDef.variableDefinitions ?? []) { + if ( + !providedArgs.has(varDef.variable.name.value) && + isRequiredArgumentNode(varDef) + ) { + const argTypeStr = inspect(varDef.type); + context.reportError( + new GraphQLError( + `Fragment "${spreadNode.name.value}" argument "${varDef.variable.name.value}" of type "${argTypeStr}" is required, but it was not provided.`, + { nodes: [spreadNode, varDef] }, + ), + ); + } + } + }, + }, }; } @@ -122,6 +156,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 a0b7e991a6..1a3c0cff02 100644 --- a/src/validation/rules/VariablesInAllowedPositionRule.ts +++ b/src/validation/rules/VariablesInAllowedPositionRule.ts @@ -36,9 +36,9 @@ export function VariablesInAllowedPositionRule( leave(operation) { const usages = context.getRecursiveVariableUsages(operation); - for (const { node, type, defaultValue } of usages) { + for (const { node, type, defaultValue, fragmentVarDef } of usages) { const varName = node.name.value; - const varDef = varDefMap[varName]; + const varDef = fragmentVarDef ?? varDefMap[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 c312c9839c..d1e50c5acf 100644 --- a/src/validation/specifiedRules.ts +++ b/src/validation/specifiedRules.ts @@ -27,6 +27,8 @@ import { NoFragmentCyclesRule } from './rules/NoFragmentCyclesRule'; import { NoUndefinedVariablesRule } from './rules/NoUndefinedVariablesRule'; // Spec Section: "Fragments must be used" import { NoUnusedFragmentsRule } from './rules/NoUnusedFragmentsRule'; +// Spec Section: "All Fragment Variables Used" +import { NoUnusedFragmentVariablesRule } from './rules/NoUnusedFragmentVariablesRule'; // Spec Section: "All Variables Used" import { NoUnusedVariablesRule } from './rules/NoUnusedVariablesRule'; // Spec Section: "Field Selection Merging" @@ -100,6 +102,7 @@ export const specifiedRules: ReadonlyArray = Object.freeze([ NoUndefinedVariablesRule, NoUnusedVariablesRule, KnownDirectivesRule, + NoUnusedFragmentVariablesRule, UniqueDirectivesPerLocationRule, KnownArgumentNamesRule, UniqueArgumentNamesRule, From d47e25eddeb94a4282adaf1451c463e24818b990 Mon Sep 17 00:00:00 2001 From: jdecroock Date: Sat, 7 Sep 2024 09:52:17 +0200 Subject: [PATCH 6/6] Apply changes from v17 branch Co-authored-by: Yaacov Rydzinski --- src/execution/__tests__/variables-test.ts | 69 +++- src/execution/collectFields.ts | 80 +++-- src/execution/execute.ts | 34 +- src/execution/subscribe.ts | 2 +- src/execution/values.ts | 179 +++------- 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 + src/type/definition.ts | 5 +- src/utilities/TypeInfo.ts | 142 ++++---- src/utilities/__tests__/TypeInfo-test.ts | 11 +- src/utilities/getVariableSignature.ts | 46 +++ src/utilities/valueFromAST.ts | 39 ++- src/validation/ValidationContext.ts | 74 ++-- .../__tests__/KnownArgumentNamesRule-test.ts | 50 +++ .../NoUnusedFragmentVariablesRule-test.ts | 37 -- .../__tests__/NoUnusedVariablesRule-test.ts | 18 +- .../OverlappingFieldsCanBeMergedRule-test.ts | 121 ++++++- .../ProvidedRequiredArgumentsRule-test.ts | 30 +- src/validation/index.ts | 3 - .../rules/KnownArgumentNamesRule.ts | 29 +- .../rules/NoUndefinedVariablesRule.ts | 4 +- .../rules/NoUnusedFragmentVariablesRule.ts | 40 --- src/validation/rules/NoUnusedVariablesRule.ts | 74 ++-- .../rules/OverlappingFieldsCanBeMergedRule.ts | 326 +++++++++++------- .../rules/ProvidedRequiredArgumentsRule.ts | 29 +- .../rules/SingleFieldSubscriptionsRule.ts | 10 +- .../rules/VariablesInAllowedPositionRule.ts | 13 +- src/validation/specifiedRules.ts | 3 - 33 files changed, 936 insertions(+), 573 deletions(-) create mode 100644 src/utilities/getVariableSignature.ts delete mode 100644 src/validation/__tests__/NoUnusedFragmentVariablesRule-test.ts delete mode 100644 src/validation/rules/NoUnusedFragmentVariablesRule.ts diff --git a/src/execution/__tests__/variables-test.ts b/src/execution/__tests__/variables-test.ts index 69b9e93fd9..04aaf5971b 100644 --- a/src/execution/__tests__/variables-test.ts +++ b/src/execution/__tests__/variables-test.ts @@ -1201,7 +1201,7 @@ describe('Execute: Handles inputs', () => { expect(result).to.have.property('errors'); expect(result.errors).to.have.length(1); - expect(result.errors?.[0]?.message).to.match( + expect(result.errors?.[0].message).to.match( /Argument "value" of required type "String!"/, ); }); @@ -1269,7 +1269,7 @@ describe('Execute: Handles inputs', () => { expect(result).to.have.property('errors'); expect(result.errors).to.have.length(1); - expect(result.errors?.[0]?.message).to.match( + expect(result.errors?.[0].message).to.match( /Argument "value" of non-null type "String!"/, ); }); @@ -1307,6 +1307,22 @@ describe('Execute: Handles inputs', () => { }); }); + 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") { @@ -1411,6 +1427,27 @@ describe('Execute: Handles inputs', () => { }); }); + 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") { @@ -1433,5 +1470,33 @@ describe('Execute: Handles inputs', () => { }, }); }); + + 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 argument passed to a directive on a nested field', () => { + const result = executeQueryWithFragmentArguments(` + query { + ...a(value: true) + } + fragment a($value: Boolean!) on TestType { + nested { echo(input: "echo") @skip(if: $value) } + } + `); + expect(result).to.deep.equal({ + data: { nested: {} }, + }); + }); }); }); diff --git a/src/execution/collectFields.ts b/src/execution/collectFields.ts index 004a0337aa..04fd1fa510 100644 --- a/src/execution/collectFields.ts +++ b/src/execution/collectFields.ts @@ -17,13 +17,24 @@ import { } from '../type/directives'; import type { GraphQLSchema } from '../type/schema'; +import type { GraphQLVariableSignature } from '../utilities/getVariableSignature'; import { typeFromAST } from '../utilities/typeFromAST'; -import { getArgumentValuesFromSpread, getDirectiveValues } from './values'; +import { experimentalGetArgumentValues, getDirectiveValues } from './values'; + +export interface FragmentVariables { + signatures: ObjMap; + values: ObjMap; +} export interface FieldDetails { node: FieldNode; - fragmentVariableValues?: ObjMap | undefined; + fragmentVariables?: FragmentVariables | undefined; +} + +export interface FragmentDetails { + definition: FragmentDefinitionNode; + variableSignatures?: ObjMap | undefined; } /** @@ -37,7 +48,7 @@ export interface FieldDetails { */ export function collectFields( schema: GraphQLSchema, - fragments: ObjMap, + fragments: ObjMap, variableValues: { [variable: string]: unknown }, runtimeType: GraphQLObjectType, selectionSet: SelectionSetNode, @@ -68,7 +79,7 @@ export function collectFields( */ export function collectSubfields( schema: GraphQLSchema, - fragments: ObjMap, + fragments: ObjMap, variableValues: { [variable: string]: unknown }, returnType: GraphQLObjectType, fieldEntries: ReadonlyArray, @@ -85,6 +96,7 @@ export function collectSubfields( entry.node.selectionSet, subFieldEntries, visitedFragmentNames, + entry.fragmentVariables, ); } } @@ -93,19 +105,18 @@ export function collectSubfields( function collectFieldsImpl( schema: GraphQLSchema, - fragments: ObjMap, + fragments: ObjMap, variableValues: { [variable: string]: unknown }, runtimeType: GraphQLObjectType, selectionSet: SelectionSetNode, fields: Map>, visitedFragmentNames: Set, - localVariableValues?: { [variable: string]: unknown }, + fragmentVariables?: FragmentVariables, ): void { for (const selection of selectionSet.selections) { switch (selection.kind) { case Kind.FIELD: { - const vars = localVariableValues ?? variableValues; - if (!shouldIncludeNode(vars, selection)) { + if (!shouldIncludeNode(selection, variableValues, fragmentVariables)) { continue; } const name = getFieldEntryKey(selection); @@ -113,13 +124,13 @@ function collectFieldsImpl( if (fieldList !== undefined) { fieldList.push({ node: selection, - fragmentVariableValues: localVariableValues ?? undefined, + fragmentVariables, }); } else { fields.set(name, [ { node: selection, - fragmentVariableValues: localVariableValues ?? undefined, + fragmentVariables, }, ]); } @@ -127,7 +138,7 @@ function collectFieldsImpl( } case Kind.INLINE_FRAGMENT: { if ( - !shouldIncludeNode(variableValues, selection) || + !shouldIncludeNode(selection, variableValues, fragmentVariables) || !doesFragmentConditionMatch(schema, selection, runtimeType) ) { continue; @@ -140,6 +151,7 @@ function collectFieldsImpl( selection.selectionSet, fields, visitedFragmentNames, + fragmentVariables, ); break; } @@ -147,7 +159,7 @@ function collectFieldsImpl( const fragName = selection.name.value; if ( visitedFragmentNames.has(fragName) || - !shouldIncludeNode(variableValues, selection) + !shouldIncludeNode(selection, variableValues, fragmentVariables) ) { continue; } @@ -155,39 +167,34 @@ function collectFieldsImpl( const fragment = fragments[fragName]; if ( !fragment || - !doesFragmentConditionMatch(schema, fragment, runtimeType) + !doesFragmentConditionMatch(schema, fragment.definition, runtimeType) ) { continue; } - // We need to introduce a concept of shadowing: - // - // - when a fragment defines a variable that is in the parent scope but not given - // in the fragment-spread we need to look at this variable as undefined and check - // whether the definition has a defaultValue, if not remove it from the variableValues. - // - when a fragment does not define a variable we need to copy it over from the parent - // scope as that variable can still get used in spreads later on in the selectionSet. - // - when a value is passed in through the fragment-spread we need to copy over the key-value - // into our variable-values. - const fragmentVariableValues = fragment.variableDefinitions - ? getArgumentValuesFromSpread( + const fragmentVariableSignatures = fragment.variableSignatures; + let newFragmentVariables: FragmentVariables | undefined; + if (fragmentVariableSignatures) { + newFragmentVariables = { + signatures: fragmentVariableSignatures, + values: experimentalGetArgumentValues( selection, - schema, - fragment.variableDefinitions, + Object.values(fragmentVariableSignatures), variableValues, - localVariableValues, - ) - : undefined; + fragmentVariables, + ), + }; + } collectFieldsImpl( schema, fragments, variableValues, runtimeType, - fragment.selectionSet, + fragment.definition.selectionSet, fields, visitedFragmentNames, - fragmentVariableValues, + newFragmentVariables, ); break; } @@ -200,10 +207,16 @@ function collectFieldsImpl( * 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; } @@ -212,6 +225,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 d11900976b..7b5288c388 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -4,6 +4,7 @@ import { invariant } from '../jsutils/invariant'; import { isIterableObject } from '../jsutils/isIterableObject'; import { isObjectLike } from '../jsutils/isObjectLike'; import { isPromise } from '../jsutils/isPromise'; +import { mapValue } from '../jsutils/mapValue'; import type { Maybe } from '../jsutils/Maybe'; import { memoize3 } from '../jsutils/memoize3'; import type { ObjMap } from '../jsutils/ObjMap'; @@ -20,7 +21,6 @@ import { locatedError } from '../error/locatedError'; import type { DocumentNode, FieldNode, - FragmentDefinitionNode, OperationDefinitionNode, } from '../language/ast'; import { OperationTypeNode } from '../language/ast'; @@ -52,12 +52,14 @@ import { import type { GraphQLSchema } from '../type/schema'; import { assertValidSchema } from '../type/validate'; -import type { FieldDetails } from './collectFields'; +import { getVariableSignature } from '../utilities/getVariableSignature'; + +import type { FieldDetails, FragmentDetails } from './collectFields'; import { collectFields, collectSubfields as _collectSubfields, } from './collectFields'; -import { getArgumentValues, getVariableValues } from './values'; +import { experimentalGetArgumentValues, getVariableValues } from './values'; /** * A memoized collection of relevant subfields with regard to the return @@ -107,7 +109,7 @@ const collectSubfields = memoize3( */ export interface ExecutionContext { schema: GraphQLSchema; - fragments: ObjMap; + fragments: ObjMap; rootValue: unknown; contextValue: unknown; operation: OperationDefinitionNode; @@ -290,7 +292,7 @@ export function buildExecutionContext( } = args; 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: @@ -307,9 +309,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 } @@ -519,11 +530,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( + const args = experimentalGetArgumentValues( fieldEntries[0].node, fieldDef.args, exeContext.variableValues, - fieldEntries[0].fragmentVariableValues, + fieldEntries[0].fragmentVariables, ); // The resolve function's optional third argument is a context value that @@ -598,7 +609,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, diff --git a/src/execution/subscribe.ts b/src/execution/subscribe.ts index caf3c7617f..bc799e9585 100644 --- a/src/execution/subscribe.ts +++ b/src/execution/subscribe.ts @@ -241,8 +241,8 @@ async function executeSubscription( // Build a JS object of arguments from the field.arguments AST, using the // variables scope to fulfill any variable references. const args = getArgumentValues( + fieldDef, fieldEntries[0].node, - fieldDef.args, variableValues, ); diff --git a/src/execution/values.ts b/src/execution/values.ts index c1050cc6ef..7500619c6c 100644 --- a/src/execution/values.ts +++ b/src/execution/values.ts @@ -1,5 +1,4 @@ import { inspect } from '../jsutils/inspect'; -import { keyMap } from '../jsutils/keyMap'; import type { Maybe } from '../jsutils/Maybe'; import type { ObjMap } from '../jsutils/ObjMap'; import { printPathArray } from '../jsutils/printPathArray'; @@ -15,15 +14,17 @@ import type { import { Kind } from '../language/kinds'; import { print } from '../language/printer'; -import type { GraphQLArgument } from '../type/definition'; -import { isInputType, isNonNullType } from '../type/definition'; +import type { GraphQLArgument, GraphQLField } from '../type/definition'; +import { isNonNullType } from '../type/definition'; import type { GraphQLDirective } from '../type/directives'; import type { GraphQLSchema } from '../type/schema'; import { coerceInputValue } from '../utilities/coerceInputValue'; -import { typeFromAST } from '../utilities/typeFromAST'; +import type { GraphQLVariableSignature } from '../utilities/getVariableSignature'; +import { getVariableSignature } from '../utilities/getVariableSignature'; import { valueFromAST } from '../utilities/valueFromAST'; -import { valueFromASTUntyped } from '../utilities/valueFromASTUntyped'; + +import type { FragmentVariables } from './collectFields'; type CoercedVariableValues = | { errors: ReadonlyArray; coerced?: never } @@ -79,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 (!hasOwnProperty(inputs, varName)) { if (varDefNode.defaultValue) { - coercedValues[varName] = valueFromAST(varDefNode.defaultValue, varType); + coercedValues[varName] = varSignature.defaultValue; } else if (isNonNullType(varType)) { const varTypeStr = inspect(varType); onError( @@ -151,25 +144,25 @@ function coerceVariableValues( * exposed to user code. Care should be taken to not pull values from the * Object prototype. */ -export function getArgumentValues( - node: FieldNode | DirectiveNode, - argDefs: ReadonlyArray, - variableValues?: Maybe>, - fragmentArgValues?: Maybe>, +export function experimentalGetArgumentValues( + node: FieldNode | DirectiveNode | FragmentSpreadNode, + argDefs: ReadonlyArray, + variableValues: Maybe>, + fragmentVariables?: Maybe, ): { [argument: string]: unknown } { const coercedValues: { [argument: string]: unknown } = {}; // FIXME: https://github.com/graphql/graphql-js/issues/2203 /* c8 ignore next */ const argumentNodes = node.arguments ?? []; - const argNodeMap = keyMap(argumentNodes, (arg) => arg.name.value); + const argNodeMap = new Map(argumentNodes.map((arg) => [arg.name.value, arg])); for (const argDef of argDefs) { const name = argDef.name; const argType = argDef.type; - const argumentNode = argNodeMap[name]; + const argumentNode = argNodeMap.get(name); - if (!argumentNode) { + if (argumentNode == null) { if (argDef.defaultValue !== undefined) { coercedValues[name] = argDef.defaultValue; } else if (isNonNullType(argType)) { @@ -183,39 +176,32 @@ export function getArgumentValues( } const valueNode = argumentNode.value; - let hasValue = valueNode.kind !== Kind.NULL; + let isNull = valueNode.kind === Kind.NULL; if (valueNode.kind === Kind.VARIABLE) { const variableName = valueNode.name.value; + const scopedVariableValues = fragmentVariables?.signatures[variableName] + ? fragmentVariables.values + : variableValues; if ( - fragmentArgValues != null && - hasOwnProperty(fragmentArgValues, variableName) + scopedVariableValues == null || + !hasOwnProperty(scopedVariableValues, variableName) ) { - hasValue = fragmentArgValues[variableName] != null; - if (!hasValue && argDef.defaultValue !== undefined) { + if (argDef.defaultValue !== undefined) { coercedValues[name] = argDef.defaultValue; - continue; + } else if (isNonNullType(argType)) { + throw new GraphQLError( + `Argument "${name}" of required type "${inspect(argType)}" ` + + `was provided the variable "$${variableName}" which was not provided a runtime value.`, + { nodes: valueNode }, + ); } - } else if ( - variableValues != null && - hasOwnProperty(variableValues, variableName) - ) { - hasValue = variableValues[variableName] != null; - } else if (argDef.defaultValue !== undefined) { - coercedValues[name] = argDef.defaultValue; - continue; - } else if (isNonNullType(argType)) { - throw new GraphQLError( - `Argument "${name}" of required type "${inspect(argType)}" ` + - `was provided the variable "$${variableName}" which was not provided a runtime value.`, - { nodes: valueNode }, - ); - } else { continue; } + isNull = scopedVariableValues[variableName] == null; } - if (!hasValue && isNonNullType(argType)) { + if (isNull && isNonNullType(argType)) { throw new GraphQLError( `Argument "${name}" of non-null type "${inspect(argType)}" ` + 'must not be null.', @@ -223,11 +209,12 @@ export function getArgumentValues( ); } - const coercedValue = valueFromAST(valueNode, argType, { - ...variableValues, - ...fragmentArgValues, - }); - + 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 @@ -242,76 +229,6 @@ export function getArgumentValues( return coercedValues; } -export function getArgumentValuesFromSpread( - /** NOTE: For error annotations only */ - node: FragmentSpreadNode, - schema: GraphQLSchema, - fragmentVarDefs: ReadonlyArray, - variableValues: Maybe>, - fragmentArgValues?: Maybe>, -): { [argument: string]: unknown } { - const coercedValues: { [argument: string]: unknown } = {}; - const argNodeMap = keyMap(node?.arguments ?? [], (arg) => arg.name.value); - - for (const varDef of fragmentVarDefs) { - const name = varDef.variable.name.value; - const argType = typeFromAST(schema, varDef.type); - const argumentNode = argNodeMap[name]; - - if (argumentNode == null) { - if (varDef.defaultValue !== undefined) { - coercedValues[name] = valueFromASTUntyped(varDef.defaultValue); - } else if (isNonNullType(argType)) { - throw new GraphQLError( - `Argument "${name}" of required type "${inspect(argType)}" ` + - 'was not provided.', - { nodes: node }, - ); - } else { - coercedValues[name] = undefined; - } - continue; - } - - const valueNode = argumentNode.value; - - let hasValue = valueNode.kind !== Kind.NULL; - if (valueNode.kind === Kind.VARIABLE) { - const variableName = valueNode.name.value; - if ( - fragmentArgValues != null && - hasOwnProperty(fragmentArgValues, variableName) - ) { - hasValue = fragmentArgValues[variableName] != null; - } else if ( - variableValues != null && - hasOwnProperty(variableValues, variableName) - ) { - hasValue = variableValues[variableName] != null; - } - } - - if (!hasValue && isNonNullType(argType)) { - throw new GraphQLError( - `Argument "${name}" of non-null type "${inspect(argType)}" ` + - 'must not be null.', - { nodes: valueNode }, - ); - } - - let coercedValue; - if (argType && isInputType(argType)) { - coercedValue = valueFromAST(valueNode, argType, { - ...variableValues, - ...fragmentArgValues, - }); - } - - coercedValues[name] = coercedValue; - } - return coercedValues; -} - /** * Prepares an object map of argument values given a directive definition * and a AST node which may contain directives. Optionally also accepts a map @@ -323,17 +240,31 @@ export function getArgumentValuesFromSpread( * exposed to user code. Care should be taken to not pull values from the * Object prototype. */ +export function getArgumentValues( + def: GraphQLField | GraphQLDirective, + node: FieldNode | DirectiveNode, + variableValues?: Maybe>, +): { [argument: string]: unknown } { + return experimentalGetArgumentValues(node, def.args, variableValues); +} + export function getDirectiveValues( directiveDef: GraphQLDirective, node: { readonly directives?: ReadonlyArray }, variableValues?: Maybe>, + fragmentVariables?: Maybe, ): undefined | { [argument: string]: unknown } { const directiveNode = node.directives?.find( (directive) => directive.name.value === directiveDef.name, ); if (directiveNode) { - return getArgumentValues(directiveNode, directiveDef.args, variableValues); + return experimentalGetArgumentValues( + directiveNode, + directiveDef.args, + variableValues, + fragmentVariables, + ); } } diff --git a/src/index.ts b/src/index.ts index 877939d879..ae2f4215ac 100644 --- a/src/index.ts +++ b/src/index.ts @@ -263,6 +263,7 @@ export type { SelectionNode, FieldNode, ArgumentNode, + FragmentArgumentNode, ConstArgumentNode, FragmentSpreadNode, InlineFragmentNode, diff --git a/src/language/__tests__/visitor-test.ts b/src/language/__tests__/visitor-test.ts index 4b11497cea..8dab3026e7 100644 --- a/src/language/__tests__/visitor-test.ts +++ b/src/language/__tests__/visitor-test.ts @@ -1393,12 +1393,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 b1e278162a..f6d1bfbcb3 100644 --- a/src/language/ast.ts +++ b/src/language/ast.ts @@ -145,6 +145,7 @@ export type ASTNode = | SelectionSetNode | FieldNode | ArgumentNode + | FragmentArgumentNode | FragmentSpreadNode | InlineFragmentNode | FragmentDefinitionNode @@ -208,6 +209,7 @@ export const QueryDocumentKeys: { SelectionSet: ['selections'], Field: ['alias', 'name', 'arguments', 'directives', 'selectionSet'], Argument: ['name', 'value'], + FragmentArgument: ['name', 'value'], FragmentSpread: ['name', 'arguments', 'directives'], InlineFragment: ['typeCondition', 'directives', 'selectionSet'], @@ -382,7 +384,7 @@ export interface FragmentSpreadNode { readonly kind: Kind.FRAGMENT_SPREAD; readonly loc?: Location; readonly name: NameNode; - readonly arguments?: ReadonlyArray; + readonly arguments?: ReadonlyArray; readonly directives?: ReadonlyArray; } @@ -501,6 +503,13 @@ export interface ConstObjectFieldNode { readonly value: ConstValueNode; } +export interface FragmentArgumentNode { + readonly kind: Kind.FRAGMENT_ARGUMENT; + readonly loc?: Location | undefined; + readonly name: NameNode; + readonly value: ValueNode; +} + /** Directives */ export interface DirectiveNode { diff --git a/src/language/index.ts b/src/language/index.ts index ec4d195e1a..79b0dc2f20 100644 --- a/src/language/index.ts +++ b/src/language/index.ts @@ -43,6 +43,7 @@ export type { SelectionNode, FieldNode, ArgumentNode, + FragmentArgumentNode /* for experimental fragment arguments */, ConstArgumentNode, FragmentSpreadNode, InlineFragmentNode, diff --git a/src/language/kinds.ts b/src/language/kinds.ts index cd05f66a3b..8e6e258801 100644 --- a/src/language/kinds.ts +++ b/src/language/kinds.ts @@ -12,6 +12,7 @@ enum Kind { SELECTION_SET = 'SelectionSet', FIELD = 'Field', ARGUMENT = 'Argument', + FRAGMENT_ARGUMENT = 'FragmentArgument', /** Fragments */ FRAGMENT_SPREAD = 'FragmentSpread', diff --git a/src/language/parser.ts b/src/language/parser.ts index d4504d2271..10dbbb04d3 100644 --- a/src/language/parser.ts +++ b/src/language/parser.ts @@ -23,6 +23,7 @@ import type { FieldDefinitionNode, FieldNode, FloatValueNode, + FragmentArgumentNode, FragmentDefinitionNode, FragmentSpreadNode, InlineFragmentNode, @@ -524,7 +525,7 @@ export class Parser { return this.node(start, { kind: Kind.FRAGMENT_SPREAD, name, - arguments: this.parseArguments(false), + arguments: this.parseFragmentArguments(), directives: this.parseDirectives(false), }); } @@ -542,6 +543,25 @@ export class Parser { }); } + /* experimental */ + parseFragmentArguments(): Array { + const item = this.parseFragmentArgument; + return this.optionalMany(TokenKind.PAREN_L, item, TokenKind.PAREN_R); + } + + /* 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), + }); + } + /** * FragmentDefinition : * - fragment FragmentName VariableDefinitions? on TypeCondition Directives? SelectionSet diff --git a/src/language/printer.ts b/src/language/printer.ts index 673d3e69fc..3c89f004e0 100644 --- a/src/language/printer.ts +++ b/src/language/printer.ts @@ -66,6 +66,7 @@ const printDocASTReducer: ASTReducer = { }, Argument: { leave: ({ name, value }) => name + ': ' + value }, + FragmentArgument: { leave: ({ name, value }) => name + ': ' + value }, // Fragments diff --git a/src/type/definition.ts b/src/type/definition.ts index 7eaac560dc..a4f36e0ce7 100644 --- a/src/type/definition.ts +++ b/src/type/definition.ts @@ -40,6 +40,7 @@ import type { import { Kind } from '../language/kinds'; import { print } from '../language/printer'; +import type { GraphQLVariableSignature } from '../utilities/getVariableSignature'; import { valueFromASTUntyped } from '../utilities/valueFromASTUntyped'; import { assertEnumValueName, assertName } from './assertName'; @@ -1069,7 +1070,9 @@ export interface GraphQLArgument { astNode: Maybe; } -export function isRequiredArgument(arg: GraphQLArgument): boolean { +export function isRequiredArgument( + arg: GraphQLArgument | GraphQLVariableSignature, +): boolean { return isNonNullType(arg.type) && arg.defaultValue === undefined; } diff --git a/src/utilities/TypeInfo.ts b/src/utilities/TypeInfo.ts index 160a93fffb..55887a0ddf 100644 --- a/src/utilities/TypeInfo.ts +++ b/src/utilities/TypeInfo.ts @@ -3,9 +3,10 @@ import type { ObjMap } from '../jsutils/ObjMap'; import type { ASTNode, + DocumentNode, FieldNode, FragmentDefinitionNode, - FragmentSpreadNode, + VariableDefinitionNode, } from '../language/ast'; import { isNode } from '../language/ast'; import { Kind } from '../language/kinds'; @@ -43,8 +44,11 @@ import { import type { GraphQLSchema } from '../type/schema'; import { typeFromAST } from './typeFromAST'; -import { valueFromAST } from './valueFromAST'; +export interface FragmentSignature { + readonly definition: FragmentDefinitionNode; + readonly variableDefinitions: ObjMap; +} /** * 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 @@ -60,8 +64,12 @@ export class TypeInfo { private _directive: Maybe; private _argument: Maybe; private _enumValue: Maybe; - private _fragmentSpread: Maybe; - private _fragmentDefinitions: ObjMap; + private _fragmentSignaturesByName: ( + fragmentName: string, + ) => Maybe; + + private _fragmentSignature: Maybe; + private _fragmentArgument: Maybe; private _getFieldDef: GetFieldDefFn; constructor( @@ -73,7 +81,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 = []; @@ -84,8 +95,9 @@ export class TypeInfo { this._directive = null; this._argument = null; this._enumValue = null; - this._fragmentSpread = null; - this._fragmentDefinitions = {}; + this._fragmentSignaturesByName = fragmentSignatures ?? (() => null); + this._fragmentSignature = null; + this._fragmentArgument = null; this._getFieldDef = getFieldDefFn ?? getFieldDef; if (initialType) { if (isInputType(initialType)) { @@ -148,6 +160,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; } @@ -160,14 +186,9 @@ export class TypeInfo { // which occurs before guarantees of schema and document validity. switch (node.kind) { case Kind.DOCUMENT: { - // A document's fragment definitions are type signatures - // referenced via fragment spreads. Ensure we can use definitions - // before visiting their call sites. - for (const astNode of node.definitions) { - if (astNode.kind === Kind.FRAGMENT_DEFINITION) { - this._fragmentDefinitions[astNode.name.value] = astNode; - } - } + const fragmentSignatures = getFragmentSignatures(node); + this._fragmentSignaturesByName = (fragmentName: string) => + fragmentSignatures[fragmentName]; break; } case Kind.SELECTION_SET: { @@ -200,7 +221,9 @@ export class TypeInfo { break; } case Kind.FRAGMENT_SPREAD: { - this._fragmentSpread = node; + this._fragmentSignature = this.getFragmentSignatureByName()( + node.name.value, + ); break; } case Kind.INLINE_FRAGMENT: @@ -219,52 +242,29 @@ export class TypeInfo { ); break; } + case Kind.FRAGMENT_ARGUMENT: { + const fragmentSignature = this.getFragmentSignature(); + const argDef = fragmentSignature?.variableDefinitions[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.ARGUMENT: { let argDef; let argType: unknown; - const directive = this.getDirective(); - const fragmentSpread = this._fragmentSpread; - const fieldDef = this.getFieldDef(); - if (directive) { - argDef = directive.args.find((arg) => arg.name === node.name.value); - } else if (fragmentSpread) { - const fragmentDef = - this._fragmentDefinitions[fragmentSpread.name.value]; - const fragVarDef = fragmentDef?.variableDefinitions?.find( - (varDef) => varDef.variable.name.value === node.name.value, + const fieldOrDirective = this.getDirective() ?? this.getFieldDef(); + if (fieldOrDirective) { + argDef = fieldOrDirective.args.find( + (arg) => arg.name === node.name.value, ); - - if (fragVarDef) { - const fragVarType = typeFromAST(schema, fragVarDef.type); - if (isInputType(fragVarType)) { - const fragVarDefault = fragVarDef.defaultValue - ? valueFromAST(fragVarDef.defaultValue, fragVarType) - : undefined; - - const schemaArgDef: GraphQLArgument = { - name: fragVarDef.variable.name.value, - type: fragVarType, - defaultValue: fragVarDefault, - description: undefined, - deprecationReason: undefined, - extensions: {}, - astNode: { - ...fragVarDef, - kind: Kind.INPUT_VALUE_DEFINITION, - name: fragVarDef.variable.name, - }, - }; - argDef = schemaArgDef; - } + if (argDef) { + argType = argDef.type; } - } else if (fieldDef) { - argDef = fieldDef.args.find((arg) => arg.name === node.name.value); - } - - if (argDef) { - argType = argDef.type; } - this._argument = argDef; this._defaultValueStack.push(argDef ? argDef.defaultValue : undefined); this._inputTypeStack.push(isInputType(argType) ? argType : undefined); @@ -315,7 +315,8 @@ export class TypeInfo { leave(node: ASTNode) { switch (node.kind) { case Kind.DOCUMENT: - this._fragmentDefinitions = {}; + this._fragmentSignaturesByName = /* c8 ignore start */ () => + null /* c8 ignore end */; break; case Kind.SELECTION_SET: this._parentTypeStack.pop(); @@ -328,7 +329,7 @@ export class TypeInfo { this._directive = null; break; case Kind.FRAGMENT_SPREAD: - this._fragmentSpread = null; + this._fragmentSignature = null; break; case Kind.OPERATION_DEFINITION: case Kind.INLINE_FRAGMENT: @@ -338,6 +339,12 @@ export class TypeInfo { case Kind.VARIABLE_DEFINITION: this._inputTypeStack.pop(); break; + case Kind.FRAGMENT_ARGUMENT: { + this._fragmentArgument = null; + this._defaultValueStack.pop(); + this._inputTypeStack.pop(); + break; + } case Kind.ARGUMENT: this._argument = null; this._defaultValueStack.pop(); @@ -391,6 +398,25 @@ function getFieldDef( } } +function getFragmentSignatures( + document: DocumentNode, +): ObjMap { + const fragmentSignatures = Object.create(null); + for (const definition of document.definitions) { + if (definition.kind === Kind.FRAGMENT_DEFINITION) { + const variableDefinitions = Object.create(null); + if (definition.variableDefinitions) { + for (const varDef of definition.variableDefinitions) { + variableDefinitions[varDef.variable.name.value] = varDef; + } + } + const signature = { definition, variableDefinitions }; + fragmentSignatures[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 0f7ed49f20..e2a7b6110f 100644 --- a/src/utilities/__tests__/TypeInfo-test.ts +++ b/src/utilities/__tests__/TypeInfo-test.ts @@ -68,6 +68,9 @@ describe('TypeInfo', () => { expect(typeInfo.getDirective()).to.equal(null); expect(typeInfo.getArgument()).to.equal(null); expect(typeInfo.getEnumValue()).to.equal(null); + expect(typeInfo.getFragmentSignature()).to.equal(null); + expect(typeInfo.getFragmentSignatureByName()('')).to.equal(null); + expect(typeInfo.getFragmentArgument()).to.equal(null); }); }); @@ -511,12 +514,12 @@ describe('visitWithTypeInfo', () => { ['enter', 'FragmentSpread', null, 'QueryRoot', 'undefined'], ['enter', 'Name', 'Foo', 'QueryRoot', 'undefined'], ['leave', 'Name', 'Foo', 'QueryRoot', 'undefined'], - ['enter', 'Argument', null, 'QueryRoot', 'ID!'], + ['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', 'Argument', null, 'QueryRoot', 'ID!'], + ['leave', 'FragmentArgument', null, 'QueryRoot', 'ID!'], ['leave', 'FragmentSpread', null, 'QueryRoot', 'undefined'], ['leave', 'SelectionSet', null, 'QueryRoot', 'undefined'], ['leave', 'OperationDefinition', null, 'QueryRoot', 'undefined'], @@ -617,12 +620,12 @@ describe('visitWithTypeInfo', () => { ['enter', 'FragmentSpread', null, 'QueryRoot', 'undefined'], ['enter', 'Name', 'Foo', 'QueryRoot', 'undefined'], ['leave', 'Name', 'Foo', 'QueryRoot', 'undefined'], - ['enter', 'Argument', null, 'QueryRoot', 'ID'], + ['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', 'Argument', null, 'QueryRoot', 'ID'], + ['leave', 'FragmentArgument', null, 'QueryRoot', 'ID'], ['leave', 'FragmentSpread', null, 'QueryRoot', 'undefined'], ['leave', 'SelectionSet', null, 'QueryRoot', 'undefined'], ['leave', 'OperationDefinition', null, 'QueryRoot', 'undefined'], diff --git a/src/utilities/getVariableSignature.ts b/src/utilities/getVariableSignature.ts new file mode 100644 index 0000000000..362e39314f --- /dev/null +++ b/src/utilities/getVariableSignature.ts @@ -0,0 +1,46 @@ +import { GraphQLError } from '../error/GraphQLError'; + +import type { VariableDefinitionNode } from '../language/ast'; +import { print } from '../language/printer'; + +import { isInputType } from '../type/definition'; +import type { GraphQLInputType, GraphQLSchema } from '../type/index'; + +import { typeFromAST } from './typeFromAST'; +import { valueFromAST } from './valueFromAST'; + +/** + * 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/utilities/valueFromAST.ts b/src/utilities/valueFromAST.ts index 2e6cc1c613..bdc964c175 100644 --- a/src/utilities/valueFromAST.ts +++ b/src/utilities/valueFromAST.ts @@ -39,6 +39,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. @@ -48,11 +49,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. } @@ -66,7 +68,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) { @@ -79,7 +81,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)) { @@ -87,7 +89,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. } @@ -96,7 +103,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. } @@ -111,7 +123,10 @@ export function valueFromAST( const fieldNodes = keyMap(valueNode.fields, (field) => field.name.value); for (const field of Object.values(type.getFields())) { const fieldNode = fieldNodes[field.name]; - if (!fieldNode || isMissingVariable(fieldNode.value, variables)) { + if ( + !fieldNode || + isMissingVariable(fieldNode.value, variables, fragmentVariables) + ) { if (field.defaultValue !== undefined) { coercedObj[field.name] = field.defaultValue; } else if (isNonNullType(field.type)) { @@ -119,7 +134,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. } @@ -165,9 +185,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/ValidationContext.ts b/src/validation/ValidationContext.ts index 27a3d456c0..a96fe5fb97 100644 --- a/src/validation/ValidationContext.ts +++ b/src/validation/ValidationContext.ts @@ -27,15 +27,15 @@ import type { import type { GraphQLDirective } from '../type/directives'; import type { GraphQLSchema } from '../type/schema'; -import type { TypeInfo } from '../utilities/TypeInfo'; -import { visitWithTypeInfo } from '../utilities/TypeInfo'; +import type { FragmentSignature } from '../utilities/TypeInfo'; +import { TypeInfo, visitWithTypeInfo } from '../utilities/TypeInfo'; type NodeWithSelectionSet = OperationDefinitionNode | FragmentDefinitionNode; interface VariableUsage { readonly node: VariableNode; readonly type: Maybe; readonly defaultValue: Maybe; - readonly fragmentVarDef: Maybe; + readonly fragmentVariableDefinition: Maybe; } /** @@ -202,26 +202,42 @@ export class ValidationContext extends ASTValidationContext { let usages = this._variableUsages.get(node); if (!usages) { const newUsages: Array = []; - const typeInfo = this._typeInfo; - const fragmentVariableDefinitions = - node.kind === Kind.FRAGMENT_DEFINITION - ? node.variableDefinitions - : undefined; + 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) { - const fragmentVarDef = fragmentVariableDefinitions?.find( - (varDef) => varDef.variable.name.value === variable.name.value, - ); - newUsages.push({ - node: variable, - type: typeInfo.getInputType(), - defaultValue: typeInfo.getDefaultValue(), - fragmentVarDef, - }); + let fragmentVariableDefinition; + if (fragmentDefinition) { + const fragmentSignature = typeInfo.getFragmentSignatureByName()( + fragmentDefinition.name.value, + ); + + fragmentVariableDefinition = + fragmentSignature?.variableDefinitions[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, + }); + } }, }), ); @@ -245,20 +261,6 @@ export class ValidationContext extends ASTValidationContext { return usages; } - getOperationVariableUsages( - operation: OperationDefinitionNode, - ): ReadonlyArray { - let usages = this._recursiveVariableUsages.get(operation); - if (!usages) { - usages = this.getVariableUsages(operation); - for (const frag of this.getRecursivelyReferencedFragments(operation)) { - usages = usages.concat(this.getVariableUsages(frag)); - } - this._recursiveVariableUsages.set(operation, usages); - } - return usages.filter(({ fragmentVarDef }) => !fragmentVarDef); - } - getType(): Maybe { return this._typeInfo.getType(); } @@ -287,6 +289,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 4ce5fd190f..854a75ad66 100644 --- a/src/validation/__tests__/KnownArgumentNamesRule-test.ts +++ b/src/validation/__tests__/KnownArgumentNamesRule-test.ts @@ -97,6 +97,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(` { @@ -145,6 +158,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__/NoUnusedFragmentVariablesRule-test.ts b/src/validation/__tests__/NoUnusedFragmentVariablesRule-test.ts deleted file mode 100644 index d7b0060e21..0000000000 --- a/src/validation/__tests__/NoUnusedFragmentVariablesRule-test.ts +++ /dev/null @@ -1,37 +0,0 @@ -import { describe, it } from 'mocha'; - -import { NoUnusedFragmentVariablesRule } from '../rules/NoUnusedFragmentVariablesRule'; - -import { expectValidationErrors } from './harness'; - -function expectErrors(queryStr: string) { - return expectValidationErrors(NoUnusedFragmentVariablesRule, queryStr); -} - -function expectValid(queryStr: string) { - expectErrors(queryStr).toDeepEqual([]); -} - -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', () => { - expectErrors(` - query Foo($b: String) { - ...FragA(a: $b) - } - fragment FragA($a: String) on Type { - field1 - } - `); - }); -}); diff --git a/src/validation/__tests__/NoUnusedVariablesRule-test.ts b/src/validation/__tests__/NoUnusedVariablesRule-test.ts index 6b95847e93..dcb94add26 100644 --- a/src/validation/__tests__/NoUnusedVariablesRule-test.ts +++ b/src/validation/__tests__/NoUnusedVariablesRule-test.ts @@ -248,8 +248,24 @@ describe('Validate: No unused variables', () => { ...FragA(a: $b) } fragment FragA($a: String) on Type { - field1 + 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 8d28bb565c..99580e417e 100644 --- a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts +++ b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts @@ -1177,20 +1177,20 @@ describe('Validate: Overlapping fields can be merged', () => { ]); }); - it('encounters conflict in fragment - field no args', () => { - expectErrors(` + it('allows operations with overlapping fields with arguments using identical operation variables', () => { + expectValid(` query ($y: Int = 1) { a(x: $y) - ...WithArgs + ...WithArgs(x: 1) } - fragment WithArgs on Type { + fragment WithArgs($x: Int = 1) on Type { a(x: $y) } - `).toDeepEqual([]); + `); }); - it('encounters conflict in fragment/field', () => { - expectErrors(` + 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) @@ -1198,10 +1198,113 @@ describe('Validate: Overlapping fields can be merged', () => { fragment WithArgs($x: Int) on Type { a(x: $x) } - `).toDeepEqual([]); + `); + }); + + 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}) + } + `); }); - // This is currently not validated, should we? it('encounters nested field conflict in fragments that could otherwise merge', () => { expectErrors(` query ValidDifferingFragmentArgs($command1: DogCommand, $command2: DogCommand) { diff --git a/src/validation/__tests__/ProvidedRequiredArgumentsRule-test.ts b/src/validation/__tests__/ProvidedRequiredArgumentsRule-test.ts index 1cfedeeada..a5f1d235f5 100644 --- a/src/validation/__tests__/ProvidedRequiredArgumentsRule-test.ts +++ b/src/validation/__tests__/ProvidedRequiredArgumentsRule-test.ts @@ -366,26 +366,6 @@ describe('Validate: Provided required arguments', () => { `); }); - // Query: should this be allowed? - // We could differentiate between required/optional (i.e. no default value) - // vs. nullable/non-nullable (i.e. no !), whereas now they are conflated. - // So today: - // $x: Int! `x:` is required and must not be null (NOT a nullable variable) - // $x: Int! = 3 `x:` is not required and must not be null (MAY BE a nullable variable) - // $x: Int `x:` is not required and may be null - // $x: Int = 3 `x:` is not required and may be null - // - // It feels weird to collapse the nullable cases but not the non-nullable ones. - // Whereas all four feel like they ought to mean something explicitly different. - // - // Potential proposal: - // $x: Int! `x:` is required and must not be null (NOT a nullable variable) - // $x: Int! = 3 `x:` is not required and must not be null (NOT a nullable variable) - // $x: Int `x:` is required and may be null - // $x: Int = 3 `x:` is not required and may be null - // - // Required then is whether there's a default value, - // and nullable is whether there's a ! it('Missing nullable argument with default is allowed', () => { expectValid(` { @@ -396,7 +376,7 @@ describe('Validate: Provided required arguments', () => { } `); }); - // Above proposal: this should be an error + it('Missing nullable argument is allowed', () => { expectValid(` { @@ -407,6 +387,7 @@ describe('Validate: Provided required arguments', () => { } `); }); + it('Missing non-nullable argument with default is allowed', () => { expectValid(` { @@ -429,11 +410,8 @@ describe('Validate: Provided required arguments', () => { `).toDeepEqual([ { message: - 'Fragment "F" argument "x" of type "{ kind: "NonNullType", type: { kind: "NamedType", name: [Object], loc: [Object] }, loc: [Object] }" is required, but it was not provided.', - locations: [ - { line: 3, column: 13 }, - { line: 5, column: 22 }, - ], + 'Fragment "F" argument "x" of type "Int!" is required, but it was not provided.', + locations: [{ line: 3, column: 13 }], }, ]); }); diff --git a/src/validation/index.ts b/src/validation/index.ts index 14646ac270..587479e351 100644 --- a/src/validation/index.ts +++ b/src/validation/index.ts @@ -51,9 +51,6 @@ export { PossibleFragmentSpreadsRule } from './rules/PossibleFragmentSpreadsRule // Spec Section: "Argument Optionality" export { ProvidedRequiredArgumentsRule } from './rules/ProvidedRequiredArgumentsRule'; -// Spec Section: "All Fragment Variables Used" -export { NoUnusedFragmentVariablesRule } from './rules/NoUnusedFragmentVariablesRule'; - // Spec Section: "Leaf Field Selections" export { ScalarLeafsRule } from './rules/ScalarLeafsRule'; diff --git a/src/validation/rules/KnownArgumentNamesRule.ts b/src/validation/rules/KnownArgumentNamesRule.ts index 332b21c1ca..373bb6981b 100644 --- a/src/validation/rules/KnownArgumentNamesRule.ts +++ b/src/validation/rules/KnownArgumentNamesRule.ts @@ -26,6 +26,29 @@ 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[argNode.name.value]; + if (!varDef) { + const argName = argNode.name.value; + const suggestions = suggestionList( + argName, + Array.from( + Object.values(fragmentSignature.variableDefinitions), + ).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 +56,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 82692a3a91..c91ede1e84 100644 --- a/src/validation/rules/NoUndefinedVariablesRule.ts +++ b/src/validation/rules/NoUndefinedVariablesRule.ts @@ -25,8 +25,8 @@ export function NoUndefinedVariablesRule( leave(operation) { const usages = context.getRecursiveVariableUsages(operation); - for (const { node, fragmentVarDef } of usages) { - if (fragmentVarDef) { + for (const { node, fragmentVariableDefinition } of usages) { + if (fragmentVariableDefinition) { continue; } const varName = node.name.value; diff --git a/src/validation/rules/NoUnusedFragmentVariablesRule.ts b/src/validation/rules/NoUnusedFragmentVariablesRule.ts deleted file mode 100644 index 9cfde886bd..0000000000 --- a/src/validation/rules/NoUnusedFragmentVariablesRule.ts +++ /dev/null @@ -1,40 +0,0 @@ -import { GraphQLError } from '../../error/GraphQLError'; - -import type { ASTVisitor } from '../../language/visitor'; - -import type { ValidationContext } from '../ValidationContext'; - -/** - * No unused variables - * - * A GraphQL fragment is only valid if all arguments defined by it - * are used within the same fragment. - * - * See https://spec.graphql.org/draft/#sec-All-Fragment-Variables-Used - */ -export function NoUnusedFragmentVariablesRule( - context: ValidationContext, -): ASTVisitor { - return { - 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 }, - ), - ); - } - } - }, - }; -} diff --git a/src/validation/rules/NoUnusedVariablesRule.ts b/src/validation/rules/NoUnusedVariablesRule.ts index c2f58d05b0..fb67b35ebf 100644 --- a/src/validation/rules/NoUnusedVariablesRule.ts +++ b/src/validation/rules/NoUnusedVariablesRule.ts @@ -1,6 +1,5 @@ import { GraphQLError } from '../../error/GraphQLError'; -import type { VariableDefinitionNode } from '../../language/ast'; import type { ASTVisitor } from '../../language/visitor'; import type { ValidationContext } from '../ValidationContext'; @@ -11,41 +10,56 @@ import type { ValidationContext } from '../ValidationContext'; * A GraphQL operation is only valid if all variables defined by an operation * are used, either directly or within a spread fragment. * - * See https://spec.graphql.org/draft/#sec-All-Operation-Variables-Used + * See https://spec.graphql.org/draft/#sec-All-Variables-Used */ export function NoUnusedVariablesRule(context: ValidationContext): ASTVisitor { - let variableDefs: Array = []; - return { - OperationDefinition: { - enter() { - variableDefs = []; - }, - leave(operation) { - const variableNameUsed = Object.create(null); - const usages = context.getOperationVariableUsages(operation); - - for (const { node } of usages) { - variableNameUsed[node.name.value] = true; + 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); + } + } - for (const variableDef of variableDefs) { - const variableName = variableDef.variable.name.value; - if (variableNameUsed[variableName] !== true) { - context.reportError( - new GraphQLError( - operation.name - ? `Variable "$${variableName}" is never used in operation "${operation.name.value}".` - : `Variable "$${variableName}" is never used.`, - { nodes: variableDef }, - ), - ); - } + // 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 (!operationVariableNameUsed.has(variableName)) { + context.reportError( + new GraphQLError( + operation.name + ? `Variable "$${variableName}" is never used in operation "${operation.name.value}".` + : `Variable "$${variableName}" is never used.`, + { nodes: variableDef }, + ), + ); } - }, - }, - VariableDefinition(def) { - variableDefs.push(def); + } }, }; } diff --git a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts index 0ac766b95c..51b5408e58 100644 --- a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts +++ b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts @@ -5,8 +5,9 @@ import type { ObjMap } from '../../jsutils/ObjMap'; import { GraphQLError } from '../../error/GraphQLError'; import type { - DirectiveNode, + ArgumentNode, FieldNode, + FragmentArgumentNode, FragmentDefinitionNode, FragmentSpreadNode, SelectionSetNode, @@ -15,7 +16,6 @@ import type { import { Kind } from '../../language/kinds'; import { print } from '../../language/printer'; import type { ASTVisitor } from '../../language/visitor'; -import { visit } from '../../language/visitor'; import type { GraphQLField, @@ -66,7 +66,7 @@ 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 cachedFieldsAndFragmentSpreads = new Map(); @@ -106,23 +106,17 @@ type NodeAndDef = [ ]; // Map of array of those. type NodeAndDefCollection = ObjMap>; -type FragmentSpreadsByName = ObjMap>; +interface FragmentSpread { + key: string; + node: FragmentSpreadNode; + varMap: Map | undefined; +} +type FragmentSpreads = ReadonlyArray; type FieldsAndFragmentSpreads = readonly [ NodeAndDefCollection, - FragmentSpreadsByName, + FragmentSpreads, ]; -const printFragmentSpreadArguments = (fragmentSpread: FragmentSpreadNode) => { - if (!fragmentSpread.arguments || fragmentSpread.arguments.length === 0) { - return fragmentSpread.name.value; - } - - const printedArguments: Array = fragmentSpread.arguments - .map(print) - .sort((a, b) => a.localeCompare(b)); - return fragmentSpread.name.value + '(' + printedArguments.join(',') + ')'; -}; - /** * Algorithm: * @@ -193,11 +187,12 @@ function findConflictsWithinSelectionSet( ): Array { const conflicts: Array = []; - const [fieldMap, fragmentSpreadMap] = getFieldsAndFragmentSpreads( + const [fieldMap, fragmentSpreads] = getFieldsAndFragmentSpreads( context, cachedFieldsAndFragmentSpreads, parentType, selectionSet, + undefined, ); // (A) First find all conflicts "within" the fields and fragment-spreads of this selection set. @@ -210,15 +205,10 @@ function findConflictsWithinSelectionSet( fieldMap, ); - const allFragmentSpreads = []; - for (const [, fragmentSpreads] of Object.entries(fragmentSpreadMap)) { - allFragmentSpreads.push(...fragmentSpreads); - } - - if (allFragmentSpreads.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 < allFragmentSpreads.length; i++) { + for (let i = 0; i < fragmentSpreads.length; i++) { collectConflictsBetweenFieldsAndFragment( context, conflicts, @@ -226,21 +216,21 @@ function findConflictsWithinSelectionSet( comparedFragmentPairs, false, fieldMap, - allFragmentSpreads[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 // item in that same list (except for itself). - for (let j = i + 1; j < allFragmentSpreads.length; j++) { + for (let j = i + 1; j < fragmentSpreads.length; j++) { collectConflictsBetweenFragments( context, conflicts, cachedFieldsAndFragmentSpreads, comparedFragmentPairs, false, - allFragmentSpreads[i], - allFragmentSpreads[j], + fragmentSpreads[i], + fragmentSpreads[j], ); } } @@ -260,9 +250,9 @@ function collectConflictsBetweenFieldsAndFragment( comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, fieldMap: NodeAndDefCollection, - fragmentSpread: FragmentSpreadNode, + fragmentSpread: FragmentSpread, ): void { - const fragmentName = fragmentSpread.name.value; + const fragmentName = fragmentSpread.node.name.value; const fragment = context.getFragment(fragmentName); if (!fragment) { return; @@ -273,7 +263,7 @@ function collectConflictsBetweenFieldsAndFragment( context, cachedFieldsAndFragmentSpreads, fragment, - fragmentSpread, + fragmentSpread.varMap, ); // Do not compare a fragment's fieldMap to itself. @@ -290,27 +280,29 @@ function collectConflictsBetweenFieldsAndFragment( 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, [spread]] of Object.entries( + for (const referencedFragmentSpread of Object.values( referencedFragmentNames, )) { // 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, ); @@ -321,7 +313,7 @@ function collectConflictsBetweenFieldsAndFragment( comparedFragmentPairs, areMutuallyExclusive, fieldMap, - spread, + referencedFragmentSpread, ); } } @@ -337,46 +329,50 @@ function collectConflictsBetweenFragments( >, comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, - fragmentSpread1: FragmentSpreadNode, - fragmentSpread2: FragmentSpreadNode, + fragmentSpread1: FragmentSpread, + fragmentSpread2: FragmentSpread, ): void { - const fragmentName1 = fragmentSpread1.name.value; - const fragmentName2 = fragmentSpread2.name.value; - if ( - fragmentName1 === fragmentName2 && - !sameArguments(fragmentSpread1, fragmentSpread2) - ) { - context.reportError( - new GraphQLError( - `Spreads "${fragmentName1}" conflict because ${printFragmentSpreadArguments( - fragmentSpread1, - )} and ${printFragmentSpreadArguments( - fragmentSpread2, - )} have different fragment arguments.`, - { nodes: [fragmentSpread1, fragmentSpread2] }, - ), - ); + // No need to compare a fragment to itself. + if (fragmentSpread1.key === fragmentSpread2.key) { return; } - // No need to compare a fragment to itself. - if ( - fragmentName1 === fragmentName2 && - sameArguments(fragmentSpread1, fragmentSpread2) - ) { - 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; + } } - const fragKey1 = printFragmentSpreadArguments(fragmentSpread1); - const fragKey2 = printFragmentSpreadArguments(fragmentSpread2); - // Memoize so two fragments are not compared for conflicts more than once. - if (comparedFragmentPairs.has(fragKey1, fragKey2, areMutuallyExclusive)) { + if ( + comparedFragmentPairs.has( + fragmentSpread1.key, + fragmentSpread2.key, + areMutuallyExclusive, + ) + ) { return; } - comparedFragmentPairs.add(fragKey1, fragKey2, 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; } @@ -386,14 +382,14 @@ function collectConflictsBetweenFragments( context, cachedFieldsAndFragmentSpreads, fragment1, - fragmentSpread1, + fragmentSpread1.varMap, ); const [fieldMap2, referencedFragmentSpreads2] = getReferencedFieldsAndFragmentSpreads( context, cachedFieldsAndFragmentSpreads, fragment2, - fragmentSpread2, + fragmentSpread2.varMap, ); // (F) First, collect all conflicts between these two collections of fields @@ -405,14 +401,14 @@ function collectConflictsBetweenFragments( 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 [referencedFragmentSpread2] of Object.values( - referencedFragmentSpreads2, - )) { + for (const referencedFragmentSpread2 of referencedFragmentSpreads2) { collectConflictsBetweenFragments( context, conflicts, @@ -426,9 +422,7 @@ function collectConflictsBetweenFragments( // (G) Then collect conflicts between the second fragment and any nested // fragments spread in the first fragment. - for (const [referencedFragmentSpread1] of Object.values( - referencedFragmentSpreads1, - )) { + for (const referencedFragmentSpread1 of referencedFragmentSpreads1) { collectConflictsBetweenFragments( context, conflicts, @@ -454,22 +448,26 @@ function findConflictsBetweenSubSelectionSets( areMutuallyExclusive: boolean, parentType1: Maybe, selectionSet1: SelectionSetNode, + varMap1: Map | undefined, parentType2: Maybe, selectionSet2: SelectionSetNode, + varMap2: Map | undefined, ): Array { const conflicts: Array = []; - const [fieldMap1, fragmentSpreadsByName1] = getFieldsAndFragmentSpreads( + const [fieldMap1, fragmentSpreads1] = getFieldsAndFragmentSpreads( context, cachedFieldsAndFragmentSpreads, parentType1, selectionSet1, + varMap1, ); - const [fieldMap2, fragmentSpreadsByName2] = getFieldsAndFragmentSpreads( + const [fieldMap2, fragmentSpreads2] = getFieldsAndFragmentSpreads( context, cachedFieldsAndFragmentSpreads, parentType2, selectionSet2, + varMap2, ); // (H) First, collect all conflicts between these two collections of field. @@ -480,12 +478,14 @@ function findConflictsBetweenSubSelectionSets( 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 [fragmentSpread2] of Object.values(fragmentSpreadsByName2)) { + for (const fragmentSpread2 of fragmentSpreads2) { collectConflictsBetweenFieldsAndFragment( context, conflicts, @@ -499,7 +499,7 @@ function findConflictsBetweenSubSelectionSets( // (I) Then collect conflicts between the second collection of fields and // those referenced by each fragment name associated with the first. - for (const [fragmentSpread1] of Object.values(fragmentSpreadsByName1)) { + for (const fragmentSpread1 of fragmentSpreads1) { collectConflictsBetweenFieldsAndFragment( context, conflicts, @@ -511,11 +511,11 @@ function findConflictsBetweenSubSelectionSets( ); } - // (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 [fragmentSpread1] of Object.values(fragmentSpreadsByName1)) { - for (const [fragmentSpread2] of Object.values(fragmentSpreadsByName2)) { + // (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, @@ -559,7 +559,9 @@ function collectConflictsWithin( false, // within one collection is never mutually exclusive responseName, fields[i], + undefined, fields[j], + undefined, ); if (conflict) { conflicts.push(conflict); @@ -585,7 +587,9 @@ function collectConflictsBetween( 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 @@ -604,7 +608,9 @@ function collectConflictsBetween( parentFieldsAreMutuallyExclusive, responseName, field1, + varMap1, field2, + varMap2, ); if (conflict) { conflicts.push(conflict); @@ -627,7 +633,9 @@ function findConflict( 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; @@ -659,7 +667,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], @@ -685,7 +693,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; @@ -698,20 +706,21 @@ function findConflict( areMutuallyExclusive, getNamedType(type1), selectionSet1, + varMap1, getNamedType(type2), selectionSet2, + varMap2, ); return subfieldConflicts(conflicts, responseName, node1, node2); } } -function sameArguments( - node1: FieldNode | DirectiveNode | FragmentSpreadNode, - node2: FieldNode | DirectiveNode | FragmentSpreadNode, +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; } @@ -726,9 +735,17 @@ function sameArguments( /* c8 ignore next */ } - 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; @@ -738,6 +755,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)); } @@ -782,22 +827,26 @@ function getFieldsAndFragmentSpreads( >, parentType: Maybe, selectionSet: SelectionSetNode, + varMap: Map | undefined, ): FieldsAndFragmentSpreads { const cached = cachedFieldsAndFragmentSpreads.get(selectionSet); if (cached) { return cached; } const nodeAndDefs: NodeAndDefCollection = Object.create(null); - const fragmentSpreadsByName: ObjMap> = - Object.create(null); + const fragmentSpreads: ObjMap = Object.create(null); _collectFieldsAndFragmentNames( context, parentType, selectionSet, nodeAndDefs, - fragmentSpreadsByName, + fragmentSpreads, + varMap, ); - const result = [nodeAndDefs, fragmentSpreadsByName] as const; + const result: FieldsAndFragmentSpreads = [ + nodeAndDefs, + Array.from(Object.values(fragmentSpreads)), + ]; cachedFieldsAndFragmentSpreads.set(selectionSet, result); return result; } @@ -811,23 +860,9 @@ function getReferencedFieldsAndFragmentSpreads( FieldsAndFragmentSpreads >, fragment: FragmentDefinitionNode, - fragmentSpread: FragmentSpreadNode, + varMap: Map | undefined, ) { - const args = fragmentSpread.arguments; - const fragmentSelectionSet = visit(fragment.selectionSet, { - Variable: (node) => { - const name = node.name.value; - const argNode = args?.find((arg) => arg.name.value === name); - if (argNode) { - return argNode.value; - } - - return node; - }, - }); - - // Short-circuit building a type from the node if possible. - const cached = cachedFieldsAndFragmentSpreads.get(fragmentSelectionSet); + const cached = cachedFieldsAndFragmentSpreads.get(fragment.selectionSet); if (cached) { return cached; } @@ -837,7 +872,8 @@ function getReferencedFieldsAndFragmentSpreads( context, cachedFieldsAndFragmentSpreads, fragmentType, - fragmentSelectionSet, + fragment.selectionSet, + varMap, ); } @@ -846,7 +882,8 @@ function _collectFieldsAndFragmentNames( parentType: Maybe, selectionSet: SelectionSetNode, nodeAndDefs: NodeAndDefCollection, - fragmentSpreadsByName: ObjMap>, + fragmentSpreads: ObjMap, + varMap: Map | undefined, ): void { for (const selection of selectionSet.selections) { switch (selection.kind) { @@ -866,12 +903,8 @@ function _collectFieldsAndFragmentNames( break; } case Kind.FRAGMENT_SPREAD: { - const existing = fragmentSpreadsByName[selection.name.value]; - if (existing) { - existing.push(selection); - } else { - fragmentSpreadsByName[selection.name.value] = [selection]; - } + const fragmentSpread = getFragmentSpread(context, selection, varMap); + fragmentSpreads[fragmentSpread.key] = fragmentSpread; break; } case Kind.INLINE_FRAGMENT: { @@ -884,7 +917,8 @@ function _collectFieldsAndFragmentNames( inlineFragmentType, selection.selectionSet, nodeAndDefs, - fragmentSpreadsByName, + fragmentSpreads, + varMap, ); break; } @@ -892,6 +926,50 @@ 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 Object.entries( + 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 7a7e8ff153..e6a425e970 100644 --- a/src/validation/rules/ProvidedRequiredArgumentsRule.ts +++ b/src/validation/rules/ProvidedRequiredArgumentsRule.ts @@ -16,6 +16,8 @@ import type { GraphQLArgument } from '../../type/definition'; import { isRequiredArgument, isType } from '../../type/definition'; import { specifiedDirectives } from '../../type/directives'; +import { typeFromAST } from '../../utilities/typeFromAST'; + import type { SDLValidationContext, ValidationContext, @@ -40,7 +42,6 @@ export function ProvidedRequiredArgumentsRule( if (!fieldDef) { return false; } - const providedArgs = new Set( // FIXME: https://github.com/graphql/graphql-js/issues/2203 /* c8 ignore next */ @@ -60,10 +61,10 @@ export function ProvidedRequiredArgumentsRule( }, }, FragmentSpread: { - // Validate on leave to allow for directive errors to appear first. + // Validate on leave to allow for deeper errors to appear first. leave(spreadNode) { - const fragmentDef = context.getFragment(spreadNode.name.value); - if (!fragmentDef) { + const fragmentSignature = context.getFragmentSignature(); + if (!fragmentSignature) { return false; } @@ -72,18 +73,22 @@ export function ProvidedRequiredArgumentsRule( /* c8 ignore next */ spreadNode.arguments?.map((arg) => arg.name.value), ); - // FIXME: https://github.com/graphql/graphql-js/issues/2203 - /* c8 ignore next */ - for (const varDef of fragmentDef.variableDefinitions ?? []) { + for (const [varName, variableDefinition] of Object.entries( + fragmentSignature.variableDefinitions, + )) { if ( - !providedArgs.has(varDef.variable.name.value) && - isRequiredArgumentNode(varDef) + !providedArgs.has(varName) && + isRequiredArgumentNode(variableDefinition) ) { - const argTypeStr = inspect(varDef.type); + const type = typeFromAST( + context.getSchema(), + variableDefinition.type, + ); + const argTypeStr = inspect(type); context.reportError( new GraphQLError( - `Fragment "${spreadNode.name.value}" argument "${varDef.variable.name.value}" of type "${argTypeStr}" is required, but it was not provided.`, - { nodes: [spreadNode, varDef] }, + `Fragment "${spreadNode.name.value}" argument "${varName}" of type "${argTypeStr}" is required, but it was not provided.`, + { nodes: spreadNode }, ), ); } diff --git a/src/validation/rules/SingleFieldSubscriptionsRule.ts b/src/validation/rules/SingleFieldSubscriptionsRule.ts index a93aeeb224..d8e40e4499 100644 --- a/src/validation/rules/SingleFieldSubscriptionsRule.ts +++ b/src/validation/rules/SingleFieldSubscriptionsRule.ts @@ -2,13 +2,11 @@ import type { ObjMap } from '../../jsutils/ObjMap'; import { GraphQLError } from '../../error/GraphQLError'; -import type { - FragmentDefinitionNode, - OperationDefinitionNode, -} from '../../language/ast'; +import type { OperationDefinitionNode } from '../../language/ast'; import { Kind } from '../../language/kinds'; import type { ASTVisitor } from '../../language/visitor'; +import type { FragmentDetails } from '../../execution/collectFields'; import { collectFields } from '../../execution/collectFields'; import type { ValidationContext } from '../ValidationContext'; @@ -35,10 +33,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 fields = collectFields( diff --git a/src/validation/rules/VariablesInAllowedPositionRule.ts b/src/validation/rules/VariablesInAllowedPositionRule.ts index 1a3c0cff02..0b616d2aa9 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, fragmentVarDef } of usages) { + for (const { + node, + type, + defaultValue, + fragmentVariableDefinition, + } of usages) { const varName = node.name.value; - const varDef = fragmentVarDef ?? varDefMap[varName]; + let varDef = fragmentVariableDefinition; + if (!varDef) { + varDef = varDefMap[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 d1e50c5acf..c312c9839c 100644 --- a/src/validation/specifiedRules.ts +++ b/src/validation/specifiedRules.ts @@ -27,8 +27,6 @@ import { NoFragmentCyclesRule } from './rules/NoFragmentCyclesRule'; import { NoUndefinedVariablesRule } from './rules/NoUndefinedVariablesRule'; // Spec Section: "Fragments must be used" import { NoUnusedFragmentsRule } from './rules/NoUnusedFragmentsRule'; -// Spec Section: "All Fragment Variables Used" -import { NoUnusedFragmentVariablesRule } from './rules/NoUnusedFragmentVariablesRule'; // Spec Section: "All Variables Used" import { NoUnusedVariablesRule } from './rules/NoUnusedVariablesRule'; // Spec Section: "Field Selection Merging" @@ -102,7 +100,6 @@ export const specifiedRules: ReadonlyArray = Object.freeze([ NoUndefinedVariablesRule, NoUnusedVariablesRule, KnownDirectivesRule, - NoUnusedFragmentVariablesRule, UniqueDirectivesPerLocationRule, KnownArgumentNamesRule, UniqueArgumentNamesRule,