diff --git a/.changeset/clean-hairs-count.md b/.changeset/clean-hairs-count.md new file mode 100644 index 00000000000..08ed3277920 --- /dev/null +++ b/.changeset/clean-hairs-count.md @@ -0,0 +1,5 @@ +--- +'@graphql-eslint/eslint-plugin': patch +--- + +fix `no-duplicate-fields`, compare selections with fragment spreads and inline fragments diff --git a/packages/plugin/src/estree-parser/converter.ts b/packages/plugin/src/estree-parser/converter.ts index 5c3aa69302b..d9468596412 100644 --- a/packages/plugin/src/estree-parser/converter.ts +++ b/packages/plugin/src/estree-parser/converter.ts @@ -7,11 +7,10 @@ import { Kind, DocumentNode, ASTVisitor, - Location, } from 'graphql'; -import { SourceLocation, Comment } from 'estree'; -import { extractCommentsFromAst } from './utils'; -import { GraphQLESTreeNode, TypeInformation } from './estree-ast'; +import type { Comment } from 'estree'; +import { extractCommentsFromAst, convertLocation } from './utils'; +import type { GraphQLESTreeNode, TypeInformation } from './estree-ast'; export function convertToESTree(node: T, typeInfo?: TypeInfo) { const visitor: ASTVisitor = { leave: convertNode(typeInfo) }; @@ -25,32 +24,6 @@ function hasTypeField(node: T): node is T & { readonly type: return 'type' in node && Boolean(node.type); } -function convertLocation(location: Location): SourceLocation { - const { startToken, endToken, source, start, end } = location; - /* - * ESLint has 0-based column number - * https://eslint.org/docs/developer-guide/working-with-rules#contextreport - */ - const loc = { - start: { - /* - * Kind.Document has startToken: { line: 0, column: 0 }, we set line as 1 and column as 0 - */ - line: startToken.line === 0 ? 1 : startToken.line, - column: startToken.column === 0 ? 0 : startToken.column - 1, - }, - end: { - line: endToken.line, - column: endToken.column - 1, - }, - source: source.body, - }; - if (loc.start.column === loc.end.column) { - loc.end.column += end - start; - } - return loc; -} - const convertNode = (typeInfo?: TypeInfo) => (node: T, key: string | number, parent: any): GraphQLESTreeNode => { diff --git a/packages/plugin/src/estree-parser/utils.ts b/packages/plugin/src/estree-parser/utils.ts index d01357c8de1..1d896d53176 100644 --- a/packages/plugin/src/estree-parser/utils.ts +++ b/packages/plugin/src/estree-parser/utils.ts @@ -9,7 +9,7 @@ import { Source, Lexer, } from 'graphql'; -import type { Comment } from 'estree'; +import type { Comment, SourceLocation } from 'estree'; import type { AST } from 'eslint'; import { valueFromASTUntyped } from 'graphql/utilities/valueFromASTUntyped'; @@ -125,3 +125,29 @@ export function extractCommentsFromAst(loc: Location): Comment[] { } return comments; } + +export function convertLocation(location: Location): SourceLocation { + const { startToken, endToken, source, start, end } = location; + /* + * ESLint has 0-based column number + * https://eslint.org/docs/developer-guide/working-with-rules#contextreport + */ + const loc = { + start: { + /* + * Kind.Document has startToken: { line: 0, column: 0 }, we set line as 1 and column as 0 + */ + line: startToken.line === 0 ? 1 : startToken.line, + column: startToken.column === 0 ? 0 : startToken.column - 1, + }, + end: { + line: endToken.line, + column: endToken.column - 1, + }, + source: source.body, + }; + if (loc.start.column === loc.end.column) { + loc.end.column += end - start; + } + return loc; +} diff --git a/packages/plugin/src/rules/no-duplicate-fields.ts b/packages/plugin/src/rules/no-duplicate-fields.ts index 9d96dbcdfdb..fed61b36ec4 100644 --- a/packages/plugin/src/rules/no-duplicate-fields.ts +++ b/packages/plugin/src/rules/no-duplicate-fields.ts @@ -1,12 +1,17 @@ -import { Kind, NameNode } from 'graphql'; -import { GraphQLESLintRule } from '../types'; -import { GraphQLESTreeNode } from '../estree-parser'; +import { ASTNode, ASTVisitor, FragmentDefinitionNode, Kind, NameNode, visit } from 'graphql'; +import type { GraphQLESLintRule, ReportDescriptor } from '../types'; +import type { SiblingOperations } from '../sibling-operations'; +import { logger, requireSiblingsOperations } from '../utils'; +import { convertLocation, GraphQLESTreeNode } from '../estree-parser'; const RULE_ID = 'no-duplicate-fields'; +type MetaInfo = { fragmentName?: string; inlineFragmentName?: string }; + const rule: GraphQLESLintRule = { meta: { type: 'suggestion', + // eslint-disable-next-line eslint-plugin/require-meta-has-suggestions hasSuggestions: true, docs: { description: `Checks for duplicate fields in selection set, variables in operation definition, or in arguments set of a field.`, @@ -57,56 +62,118 @@ const rule: GraphQLESLintRule = { ], }, messages: { - [RULE_ID]: '{{ type }} `{{ fieldName }}` defined multiple times.', + [RULE_ID]: '{{ type }} `{{ fieldName }}` already defined{{ addition }}.', }, schema: [], }, create(context) { - function checkNode(usedFields: Set, node: GraphQLESTreeNode): void { + let siblings: SiblingOperations | null = null; + + try { + siblings = requireSiblingsOperations(RULE_ID, context); + } catch { + logger.warn( + `Rule "${RULE_ID}" works best with siblings operations loaded. For more info: https://bit.ly/graphql-eslint-operations` + ); + } + + function checkNode( + usedFields: Map, + node: GraphQLESTreeNode | NameNode, + parent = (node as any).parent, + meta: MetaInfo = {} + ): void { const fieldName = node.value; - if (usedFields.has(fieldName)) { - const { parent } = node as any; - context.report({ - node, - messageId: RULE_ID, - data: { - type: parent.type, - fieldName, - }, - suggest: [ - { - desc: `Remove \`${fieldName}\` ${parent.type.toLowerCase()}`, - fix(fixer) { - return fixer.remove(parent.type === Kind.VARIABLE ? parent.parent : parent); - }, + const field = usedFields.get(fieldName); + if (!field) { + usedFields.set(fieldName, meta); + return; + } + + let addition = ''; + if (field.fragmentName) { + addition = ` in \`${field.fragmentName}\` fragment`; + } else if (field.inlineFragmentName) { + addition = ` in \`${field.inlineFragmentName}\` inline fragment`; + } + const problem: ReportDescriptor = { + loc: 'startToken' in node.loc ? convertLocation(node.loc) : node.loc, + messageId: RULE_ID, + data: { + type: parent.kind, + fieldName, + addition, + }, + }; + + if ('type' in node) { + problem.suggest = [ + { + desc: `Remove \`${fieldName}\` ${parent.type.toLowerCase()}`, + fix(fixer) { + return fixer.remove(parent.type === Kind.VARIABLE ? parent.parent : parent); }, - ], - }); - } else { - usedFields.add(fieldName); + }, + ]; } + + context.report(problem); } return { OperationDefinition(node) { - const set = new Set(); + const set = new Map(); for (const varDef of node.variableDefinitions) { checkNode(set, varDef.variable.name); } }, Field(node) { - const set = new Set(); + const set = new Map(); for (const arg of node.arguments) { checkNode(set, arg.name); } }, - SelectionSet(node) { - const set = new Set(); - for (const selection of node.selections) { - if (selection.kind === Kind.FIELD) { - checkNode(set, selection.alias || selection.name); - } - } + SelectionSet(selectionSet) { + const set = new Map(); + + const visitor: ASTVisitor = { + Field(node, _, _2, _3, ancestors): false | void { + const parent = ancestors[ancestors.length - 2] as ASTNode; + let meta: MetaInfo; + if (parent) { + if (parent.kind === Kind.INLINE_FRAGMENT) { + meta = { inlineFragmentName: parent.typeCondition.name.value }; + } else if (parent.kind === Kind.FRAGMENT_DEFINITION) { + meta = { fragmentName: parent.name.value }; + } + } + checkNode(set, node.alias || node.name, node, meta); + if (node.selectionSet) { + // Skip visiting node if field contains selections + return false; + } + }, + FragmentSpread(spread): FragmentDefinitionNode | void { + const [fragment] = siblings.getFragment(spread.name.value); + if (fragment) { + return { + ...fragment.document, + selectionSet: { + ...fragment.document.selectionSet, + selections: fragment.document.selectionSet.selections.map(node => { + const enhancedNode = { ...node }; + if ('name' in enhancedNode) { + enhancedNode.name = { ...enhancedNode.name, loc: spread.name.loc }; + } + return enhancedNode; + }), + }, + }; + } + }, + }; + + visit(selectionSet.rawNode(), visitor); }, }; }, diff --git a/packages/plugin/src/rules/require-id-when-available.ts b/packages/plugin/src/rules/require-id-when-available.ts index 13ddfccc356..c7347e70ce6 100644 --- a/packages/plugin/src/rules/require-id-when-available.ts +++ b/packages/plugin/src/rules/require-id-when-available.ts @@ -9,6 +9,7 @@ import { visit, visitWithTypeInfo, } from 'graphql'; +import type { AST } from 'eslint'; import type * as ESTree from 'estree'; import { asArray } from '@graphql-tools/utils'; import { ARRAY_DEFAULT_OPTIONS, requireGraphQLSchemaFromContext, requireSiblingsOperations } from '../utils'; @@ -132,7 +133,7 @@ const rule: GraphQLESLintRule<[RequireIdWhenAvailableRuleConfig], true> = { if (parent.kind === Kind.FRAGMENT_DEFINITION) { checkedFragmentSpreads.add(parent.name.value); } else if (parent.kind !== Kind.INLINE_FRAGMENT) { - checkSelections(node, typeInfo.getType(), selection.loc.start, parent, checkedFragmentSpreads); + checkSelections(node, typeInfo.getType(), selection.name.loc, parent, checkedFragmentSpreads); } }, }); @@ -146,7 +147,7 @@ const rule: GraphQLESLintRule<[RequireIdWhenAvailableRuleConfig], true> = { type: GraphQLOutputType, // Fragment can be placed in separate file // Provide actual fragment spread location instead of location in fragment - loc: ESTree.Position, + loc: AST.SourceLocation | ESTree.Position, // Can't access to node.parent in GraphQL AST.Node, so pass as argument parent: any, checkedFragmentSpreads = new Set() diff --git a/packages/plugin/src/rules/selection-set-depth.ts b/packages/plugin/src/rules/selection-set-depth.ts index 8da2cbb2160..4bde10ea57d 100644 --- a/packages/plugin/src/rules/selection-set-depth.ts +++ b/packages/plugin/src/rules/selection-set-depth.ts @@ -85,9 +85,9 @@ const rule: GraphQLESLintRule<[SelectionSetDepthRuleConfig]> = { try { siblings = requireSiblingsOperations(RULE_ID, context); - } catch (e) { + } catch { logger.warn( - `Rule "${RULE_ID}" works best with siblings operations loaded. For more info: http://bit.ly/graphql-eslint-operations` + `Rule "${RULE_ID}" works best with siblings operations loaded. For more info: https://bit.ly/graphql-eslint-operations` ); } diff --git a/packages/plugin/tests/__snapshots__/examples.spec.md b/packages/plugin/tests/__snapshots__/examples.spec.md index 4376b0785b2..f124d13f8c1 100644 --- a/packages/plugin/tests/__snapshots__/examples.spec.md +++ b/packages/plugin/tests/__snapshots__/examples.spec.md @@ -185,13 +185,21 @@ Array [ message: Anonymous GraphQL operations are forbidden. Make sure to name your query!, ruleId: @graphql-eslint/no-anonymous-operations, }, + Object { + message: Field \`name\` already defined., + ruleId: @graphql-eslint/no-duplicate-fields, + }, + Object { + message: Field \`name\` already defined., + ruleId: @graphql-eslint/no-duplicate-fields, + }, ], }, Object { filePath: examples/graphql-config/operations/user.fragment.graphql, messages: Array [ Object { - message: Field \`name\` defined multiple times., + message: Field \`name\` already defined., ruleId: @graphql-eslint/no-duplicate-fields, }, ], diff --git a/packages/plugin/tests/__snapshots__/no-duplicate-fields.spec.md b/packages/plugin/tests/__snapshots__/no-duplicate-fields.spec.md index 4f28434a3d0..64d6c05af90 100644 --- a/packages/plugin/tests/__snapshots__/no-duplicate-fields.spec.md +++ b/packages/plugin/tests/__snapshots__/no-duplicate-fields.spec.md @@ -3,14 +3,14 @@ exports[` 1`] = ` ❌ Error - > 1 | query test($v: String, $t: String, $v: String) { - | ^ Variable \`v\` defined multiple times. + > 1 | query ($v: String, $t: String, $v: String) { + | ^ Variable \`v\` already defined. 2 | id 3 | } 💡 Suggestion: Remove \`v\` variable - 1 | query test($v: String, $t: String, ) { + 1 | query ($v: String, $t: String, ) { 2 | id 3 | } `; @@ -18,16 +18,16 @@ exports[` 1`] = ` exports[` 2`] = ` ❌ Error - 1 | query test { + 1 | { > 2 | users(first: 100, after: 10, filter: "test", first: 50) { - | ^^^^^ Argument \`first\` defined multiple times. + | ^^^^^ Argument \`first\` already defined. 3 | id 4 | } 5 | } 💡 Suggestion: Remove \`first\` argument - 1 | query test { + 1 | { 2 | users(first: 100, after: 10, filter: "test", ) { 3 | id 4 | } @@ -37,49 +37,114 @@ exports[` 2`] = ` exports[` 3`] = ` ❌ Error - 1 | query test { + 1 | { 2 | users { 3 | id 4 | name 5 | email > 6 | name - | ^^^^ Field \`name\` defined multiple times. + | ^^^^ Field \`name\` already defined. 7 | } 8 | } - -💡 Suggestion: Remove \`name\` field - - 1 | query test { - 2 | users { - 3 | id - 4 | name - 5 | email - 6 | - 7 | } - 8 | } `; exports[` 4`] = ` ❌ Error - 1 | query test { + 1 | { 2 | users { 3 | id 4 | name 5 | email > 6 | email: somethingElse - | ^^^^^ Field \`email\` defined multiple times. + | ^^^^^ Field \`email\` already defined. 7 | } 8 | } +`; -💡 Suggestion: Remove \`email\` field +exports[` 5`] = ` +Code + + 1 | { + 2 | users { + 3 | id + 4 | ...UserFullFields # email #email + 5 | name #3 + 6 | ...UserFields # email firstName + 7 | ... on User { + 8 | id #6 + 9 | name #7 + 10 | email #8 + 11 | } + 12 | posts { + 13 | title + 14 | content + 15 | createdAt + 16 | } + 17 | id: name #9 + 18 | } + 19 | } + +❌ Error 1/9 - 1 | query test { - 2 | users { - 3 | id - 4 | name - 5 | email - 6 | - 7 | } - 8 | } + 3 | id + > 4 | ...UserFullFields # email #email + | ^^^^^^^^^^^^^^ Field \`email\` already defined in \`User\` inline fragment. + 5 | name #3 + +❌ Error 2/9 + + 3 | id + > 4 | ...UserFullFields # email #email + | ^^^^^^^^^^^^^^ Field \`email\` already defined in \`User\` inline fragment. + 5 | name #3 + +❌ Error 3/9 + + 4 | ...UserFullFields # email #email + > 5 | name #3 + | ^^^^ Field \`name\` already defined in \`UserFullFields\` fragment. + 6 | ...UserFields # email firstName + +❌ Error 4/9 + + 5 | name #3 + > 6 | ...UserFields # email firstName + | ^^^^^^^^^^ Field \`email\` already defined in \`User\` inline fragment. + 7 | ... on User { + +❌ Error 5/9 + + 5 | name #3 + > 6 | ...UserFields # email firstName + | ^^^^^^^^^^ Field \`firstName\` already defined in \`UserFields\` fragment. + 7 | ... on User { + +❌ Error 6/9 + + 7 | ... on User { + > 8 | id #6 + | ^^ Field \`id\` already defined. + 9 | name #7 + +❌ Error 7/9 + + 8 | id #6 + > 9 | name #7 + | ^^^^ Field \`name\` already defined in \`UserFullFields\` fragment. + 10 | email #8 + +❌ Error 8/9 + + 9 | name #7 + > 10 | email #8 + | ^^^^^ Field \`email\` already defined in \`User\` inline fragment. + 11 | } + +❌ Error 9/9 + + 16 | } + > 17 | id: name #9 + | ^^ Field \`id\` already defined. + 18 | } `; diff --git a/packages/plugin/tests/__snapshots__/require-id-when-available.spec.md b/packages/plugin/tests/__snapshots__/require-id-when-available.spec.md index 6444dd086a1..a16f08236a8 100644 --- a/packages/plugin/tests/__snapshots__/require-id-when-available.spec.md +++ b/packages/plugin/tests/__snapshots__/require-id-when-available.spec.md @@ -79,7 +79,7 @@ exports[` 5`] = ` ❌ Error > 1 | { user { id ...UserFields } } - | ^ Field \`posts.id\` must be selected when it's available on a type. + | ^^^^^^^^^^ Field \`posts.id\` must be selected when it's available on a type. Include it in your selection set or add to used fragment \`UserFields\`. `; @@ -101,18 +101,18 @@ Code ❌ Error 2/4 > 1 | { user { ...UserFullFields } } - | ^ Field \`posts.id\` must be selected when it's available on a type. + | ^^^^^^^^^^^^^^ Field \`posts.id\` must be selected when it's available on a type. Include it in your selection set or add to used fragment \`UserFullFields\`. ❌ Error 3/4 > 1 | { user { ...UserFullFields } } - | ^ Field \`author.id\` must be selected when it's available on a type. + | ^^^^^^^^^^^^^^ Field \`author.id\` must be selected when it's available on a type. Include it in your selection set or add to used fragments \`UserFullFields\` or \`UserFields\`. ❌ Error 4/4 > 1 | { user { ...UserFullFields } } - | ^ Field \`authorPosts.id\` must be selected when it's available on a type. + | ^^^^^^^^^^^^^^ Field \`authorPosts.id\` must be selected when it's available on a type. Include it in your selection set or add to used fragments \`UserFullFields\` or \`UserFields\`. `; diff --git a/packages/plugin/tests/examples.spec.ts b/packages/plugin/tests/examples.spec.ts index 2d8e8db203e..e61795dc1c1 100644 --- a/packages/plugin/tests/examples.spec.ts +++ b/packages/plugin/tests/examples.spec.ts @@ -41,35 +41,35 @@ describe('Examples', () => { it('should work on `.graphql` files', () => { const cwd = join(process.cwd(), 'examples/basic'); const results = getEslintOutput(cwd); - expect(countErrors(results)).toBe(6); testSnapshot(results); + expect(countErrors(results)).toBe(6); }); it('should work on `.js` files', () => { const cwd = join(process.cwd(), 'examples/code-file'); const results = getEslintOutput(cwd); - expect(countErrors(results)).toBe(2); testSnapshot(results); + expect(countErrors(results)).toBe(2); }); it('should work with `graphql-config`', () => { const cwd = join(process.cwd(), 'examples/graphql-config'); const results = getEslintOutput(cwd); - expect(countErrors(results)).toBe(2); testSnapshot(results); + expect(countErrors(results)).toBe(4); }); it('should work with `graphql-config` on `.js` files', () => { const cwd = join(process.cwd(), 'examples/graphql-config-code-file'); const results = getEslintOutput(cwd); - expect(countErrors(results)).toBe(2); testSnapshot(results); + expect(countErrors(results)).toBe(2); }); it('should work with `eslint-plugin-prettier`', () => { const cwd = join(process.cwd(), 'examples/prettier'); const results = getEslintOutput(cwd); - expect(countErrors(results)).toBe(23); testSnapshot(results); + expect(countErrors(results)).toBe(23); }); }); diff --git a/packages/plugin/tests/no-duplicate-fields.spec.ts b/packages/plugin/tests/no-duplicate-fields.spec.ts index e21c3b2c65e..847b7364e2c 100644 --- a/packages/plugin/tests/no-duplicate-fields.spec.ts +++ b/packages/plugin/tests/no-duplicate-fields.spec.ts @@ -1,4 +1,4 @@ -import { GraphQLRuleTester } from '../src/testkit'; +import { GraphQLRuleTester } from '../src'; import rule from '../src/rules/no-duplicate-fields'; const ruleTester = new GraphQLRuleTester(); @@ -8,25 +8,25 @@ ruleTester.runGraphQLTests('no-duplicate-fields', rule, { invalid: [ { code: /* GraphQL */ ` - query test($v: String, $t: String, $v: String) { + query ($v: String, $t: String, $v: String) { id } `, - errors: [{ message: 'Variable `v` defined multiple times.' }], + errors: [{ message: 'Variable `v` already defined.' }], }, { code: /* GraphQL */ ` - query test { + { users(first: 100, after: 10, filter: "test", first: 50) { id } } `, - errors: [{ message: 'Argument `first` defined multiple times.' }], + errors: [{ message: 'Argument `first` already defined.' }], }, { code: /* GraphQL */ ` - query test { + { users { id name @@ -35,11 +35,11 @@ ruleTester.runGraphQLTests('no-duplicate-fields', rule, { } } `, - errors: [{ message: 'Field `name` defined multiple times.' }], + errors: [{ message: 'Field `name` already defined.' }], }, { code: /* GraphQL */ ` - query test { + { users { id name @@ -48,7 +48,58 @@ ruleTester.runGraphQLTests('no-duplicate-fields', rule, { } } `, - errors: [{ message: 'Field `email` defined multiple times.' }], + errors: [{ message: 'Field `email` already defined.' }], + }, + { + code: /* GraphQL */ ` + { + users { + id + ...UserFullFields # email #email + name #3 + ...UserFields # email firstName + ... on User { + id #6 + name #7 + email #8 + } + posts { + title + content + createdAt + } + id: name #9 + } + } + `, + parserOptions: { + operations: /* GraphQL */ ` + fragment UserFullFields on User { + name + ... on User { + email + } + email + ...UserFields + } + + fragment UserFields on User { + email + firstName + } + `, + }, + errors: [ + { message: 'Field `email` already defined in `User` inline fragment.' }, + { message: 'Field `email` already defined in `User` inline fragment.' }, + { message: 'Field `name` already defined in `UserFullFields` fragment.' }, + { message: 'Field `email` already defined in `User` inline fragment.' }, + { message: 'Field `firstName` already defined in `UserFields` fragment.' }, + { message: 'Field `id` already defined.' }, + { message: 'Field `name` already defined in `UserFullFields` fragment.' }, + { message: 'Field `email` already defined in `User` inline fragment.' }, + { message: 'Field `id` already defined.' }, + ], }, ], });