diff --git a/.changeset/neat-eagles-tap.md b/.changeset/neat-eagles-tap.md index f1489e7b11f..906fead299f 100644 --- a/.changeset/neat-eagles-tap.md +++ b/.changeset/neat-eagles-tap.md @@ -1,5 +1,5 @@ --- -'@graphql-eslint/eslint-plugin': minor +'@graphql-eslint/eslint-plugin': patch --- fix: false negative case when nested fragments used in `require-id-when-available` rule diff --git a/.changeset/nice-pandas-confess.md b/.changeset/nice-pandas-confess.md new file mode 100644 index 00000000000..257887c571b --- /dev/null +++ b/.changeset/nice-pandas-confess.md @@ -0,0 +1,5 @@ +--- +'@graphql-eslint/eslint-plugin': patch +--- + +fix: check nested selections in fragments in `require-id-when-available` rule diff --git a/packages/plugin/src/rules/require-id-when-available.ts b/packages/plugin/src/rules/require-id-when-available.ts index abf6db59d55..13ddfccc356 100644 --- a/packages/plugin/src/rules/require-id-when-available.ts +++ b/packages/plugin/src/rules/require-id-when-available.ts @@ -1,7 +1,18 @@ -import { ARRAY_DEFAULT_OPTIONS, requireGraphQLSchemaFromContext, requireSiblingsOperations } from '../utils'; -import { GraphQLESLintRule, OmitRecursively } from '../types'; -import { GraphQLInterfaceType, GraphQLObjectType, Kind, SelectionSetNode } from 'graphql'; +import { + ASTNode, + GraphQLInterfaceType, + GraphQLObjectType, + GraphQLOutputType, + Kind, + SelectionSetNode, + TypeInfo, + visit, + visitWithTypeInfo, +} from 'graphql'; +import type * as ESTree from 'estree'; import { asArray } from '@graphql-tools/utils'; +import { ARRAY_DEFAULT_OPTIONS, requireGraphQLSchemaFromContext, requireSiblingsOperations } from '../utils'; +import { GraphQLESLintRule, OmitRecursively, ReportDescriptor } from '../types'; import { getBaseType, GraphQLESTreeNode } from '../estree-parser'; export type RequireIdWhenAvailableRuleConfig = { fieldName: string | string[] }; @@ -22,6 +33,7 @@ const englishJoinWords = words => new Intl.ListFormat('en-US', { type: 'disjunct const rule: GraphQLESLintRule<[RequireIdWhenAvailableRuleConfig], true> = { meta: { type: 'suggestion', + // eslint-disable-next-line eslint-plugin/require-meta-has-suggestions hasSuggestions: true, docs: { category: 'Operations', @@ -93,7 +105,7 @@ const rule: GraphQLESLintRule<[RequireIdWhenAvailableRuleConfig], true> = { }, }, create(context) { - requireGraphQLSchemaFromContext(RULE_ID, context); + const schema = requireGraphQLSchemaFromContext(RULE_ID, context); const siblings = requireSiblingsOperations(RULE_ID, context); const { fieldName = DEFAULT_ID_FIELD_NAME } = context.options[0] || {}; const idNames = asArray(fieldName); @@ -101,74 +113,124 @@ const rule: GraphQLESLintRule<[RequireIdWhenAvailableRuleConfig], true> = { // Check selections only in OperationDefinition, // skip selections of OperationDefinition and InlineFragment const selector = 'OperationDefinition SelectionSet[parent.kind!=/(^OperationDefinition|InlineFragment)$/]'; + const typeInfo = new TypeInfo(schema); - return { - [selector](node: GraphQLESTreeNode) { - const typeInfo = node.typeInfo(); - if (!typeInfo.gqlType) { - return; - } - const rawType = getBaseType(typeInfo.gqlType); - const isObjectType = rawType instanceof GraphQLObjectType; - const isInterfaceType = rawType instanceof GraphQLInterfaceType; - if (!isObjectType && !isInterfaceType) { - return; + function checkFragments(node: GraphQLESTreeNode): void { + for (const selection of node.selections) { + if (selection.kind !== Kind.FRAGMENT_SPREAD) { + continue; } - const fields = rawType.getFields(); - const hasIdFieldInType = idNames.some(name => fields[name]); - if (!hasIdFieldInType) { - return; + const [foundSpread] = siblings.getFragment(selection.name.value); + if (!foundSpread) { + continue; } - const checkedFragmentSpreads = new Set(); - const hasIdField = ({ selections }: OmitRecursively): boolean => - selections.some(selection => { - if (selection.kind === Kind.FIELD) { - return idNames.includes(selection.name.value); + const checkedFragmentSpreads = new Set(); + const visitor = visitWithTypeInfo(typeInfo, { + SelectionSet(node, key, parent: ASTNode) { + 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); } + }, + }); - if (selection.kind === Kind.INLINE_FRAGMENT) { - return hasIdField(selection.selectionSet); + visit(foundSpread.document, visitor); + } + } + + function checkSelections( + node: OmitRecursively, + type: GraphQLOutputType, + // Fragment can be placed in separate file + // Provide actual fragment spread location instead of location in fragment + loc: ESTree.Position, + // Can't access to node.parent in GraphQL AST.Node, so pass as argument + parent: any, + checkedFragmentSpreads = new Set() + ): void { + const rawType = getBaseType(type); + const isObjectType = rawType instanceof GraphQLObjectType; + const isInterfaceType = rawType instanceof GraphQLInterfaceType; + + if (!isObjectType && !isInterfaceType) { + return; + } + const fields = rawType.getFields(); + const hasIdFieldInType = idNames.some(name => fields[name]); + + if (!hasIdFieldInType) { + return; + } + + function hasIdField({ selections }: typeof node): boolean { + return selections.some(selection => { + if (selection.kind === Kind.FIELD) { + return idNames.includes(selection.name.value); + } + + if (selection.kind === Kind.INLINE_FRAGMENT) { + return hasIdField(selection.selectionSet); + } + + if (selection.kind === Kind.FRAGMENT_SPREAD) { + const [foundSpread] = siblings.getFragment(selection.name.value); + if (foundSpread) { + const fragmentSpread = foundSpread.document; + checkedFragmentSpreads.add(fragmentSpread.name.value); + return hasIdField(fragmentSpread.selectionSet); } + } + return false; + }); + } - if (selection.kind === Kind.FRAGMENT_SPREAD) { - const [foundSpread] = siblings.getFragment(selection.name.value); - if (foundSpread) { - const fragmentSpread = foundSpread.document; - checkedFragmentSpreads.add(fragmentSpread.name.value); - return hasIdField(fragmentSpread.selectionSet); - } - } - return false; - }); + const hasId = hasIdField(node); - if (hasIdField(node)) { - return; - } + checkFragments(node as GraphQLESTreeNode); - const pluralSuffix = idNames.length > 1 ? 's' : ''; - const fieldName = englishJoinWords(idNames.map(name => `\`${name}\``)); - const addition = - checkedFragmentSpreads.size === 0 - ? '' - : ` or add to used fragment${checkedFragmentSpreads.size > 1 ? 's' : ''} ${englishJoinWords( - [...checkedFragmentSpreads].map(name => `\`${name}\``) - )}`; - - context.report({ - loc: node.loc.start, - messageId: RULE_ID, - data: { - pluralSuffix, - fieldName, - addition, - }, - suggest: idNames.map(idName => ({ - desc: `Add \`${idName}\` selection`, - fix: fixer => fixer.insertTextBefore((node as any).selections[0], `${idName} `), - })), - }); + if (hasId) { + return; + } + + const pluralSuffix = idNames.length > 1 ? 's' : ''; + const fieldName = englishJoinWords(idNames.map(name => `\`${(parent.alias || parent.name).value}.${name}\``)); + + const addition = + checkedFragmentSpreads.size === 0 + ? '' + : ` or add to used fragment${checkedFragmentSpreads.size > 1 ? 's' : ''} ${englishJoinWords( + [...checkedFragmentSpreads].map(name => `\`${name}\``) + )}`; + + const problem: ReportDescriptor = { + loc, + messageId: RULE_ID, + data: { + pluralSuffix, + fieldName, + addition, + }, + }; + + // Don't provide suggestions for selections in fragments as fragment can be in a separate file + if ('type' in node) { + problem.suggest = idNames.map(idName => ({ + desc: `Add \`${idName}\` selection`, + fix: fixer => fixer.insertTextBefore((node as any).selections[0], `${idName} `), + })); + } + context.report(problem); + } + + return { + [selector](node: GraphQLESTreeNode) { + const typeInfo = node.typeInfo(); + if (typeInfo.gqlType) { + checkSelections(node, typeInfo.gqlType, node.loc.start, (node as any).parent); + } }, }; }, diff --git a/packages/plugin/src/rules/selection-set-depth.ts b/packages/plugin/src/rules/selection-set-depth.ts index 9dcce042c1c..8da2cbb2160 100644 --- a/packages/plugin/src/rules/selection-set-depth.ts +++ b/packages/plugin/src/rules/selection-set-depth.ts @@ -101,7 +101,7 @@ const rule: GraphQLESLintRule<[SelectionSetDepthRuleConfig]> = { ) { try { const rawNode = node.rawNode(); - const fragmentsInUse = siblings ? siblings.getFragmentsInUse(rawNode, true) : []; + const fragmentsInUse = siblings ? siblings.getFragmentsInUse(rawNode) : []; const document: DocumentNode = { kind: Kind.DOCUMENT, definitions: [rawNode, ...fragmentsInUse], diff --git a/packages/plugin/src/sibling-operations.ts b/packages/plugin/src/sibling-operations.ts index d72a63ccb32..7cd63479e1e 100644 --- a/packages/plugin/src/sibling-operations.ts +++ b/packages/plugin/src/sibling-operations.ts @@ -1,7 +1,6 @@ import { resolve } from 'path'; import { FragmentDefinitionNode, - FragmentSpreadNode, Kind, OperationDefinitionNode, SelectionSetNode, @@ -18,15 +17,15 @@ export type OperationSource = { filePath: string; document: OperationDefinitionN export type SiblingOperations = { available: boolean; - getOperations(): OperationSource[]; - getFragments(): FragmentSource[]; getFragment(fragmentName: string): FragmentSource[]; + getFragments(): FragmentSource[]; getFragmentByType(typeName: string): FragmentSource[]; getFragmentsInUse( baseOperation: OperationDefinitionNode | FragmentDefinitionNode | SelectionSetNode, - recursive: boolean + recursive?: boolean ): FragmentDefinitionNode[]; getOperation(operationName: string): OperationSource[]; + getOperations(): OperationSource[]; getOperationByType(operationType: OperationTypeNode): OperationSource[]; }; @@ -91,13 +90,13 @@ export function getSiblingOperations(options: ParserOptions, gqlConfig: GraphQLC return { available: false, - getFragments: noopWarn, - getOperations: noopWarn, getFragment: noopWarn, + getFragments: noopWarn, getFragmentByType: noopWarn, + getFragmentsInUse: noopWarn, getOperation: noopWarn, + getOperations: noopWarn, getOperationByType: noopWarn, - getFragmentsInUse: noopWarn, }; } @@ -113,11 +112,11 @@ export function getSiblingOperations(options: ParserOptions, gqlConfig: GraphQLC const result: FragmentSource[] = []; for (const source of siblings) { - for (const definition of source.document.definitions || []) { + for (const definition of source.document.definitions) { if (definition.kind === Kind.FRAGMENT_DEFINITION) { result.push({ filePath: source.location, - document: definition as FragmentDefinitionNode, + document: definition, }); } } @@ -134,11 +133,11 @@ export function getSiblingOperations(options: ParserOptions, gqlConfig: GraphQLC const result: OperationSource[] = []; for (const source of siblings) { - for (const definition of source.document.definitions || []) { + for (const definition of source.document.definitions) { if (definition.kind === Kind.OPERATION_DEFINITION) { result.push({ filePath: source.location, - document: definition as OperationDefinitionNode, + document: definition, }); } } @@ -152,25 +151,22 @@ export function getSiblingOperations(options: ParserOptions, gqlConfig: GraphQLC const collectFragments = ( selectable: SelectionSetNode | OperationDefinitionNode | FragmentDefinitionNode, - recursive = true, - collected: Map = new Map() + recursive, + collected = new Map() ) => { visit(selectable, { - FragmentSpread(spread: FragmentSpreadNode) { - const name = spread.name.value; - const fragmentInfo = getFragment(name); + FragmentSpread(spread) { + const fragmentName = spread.name.value; + const [fragment] = getFragment(fragmentName); - if (fragmentInfo.length === 0) { + if (!fragment) { logger.warn( - `Unable to locate fragment named "${name}", please make sure it's loaded using "parserOptions.operations"` + `Unable to locate fragment named "${fragmentName}", please make sure it's loaded using "parserOptions.operations"` ); return; } - const fragment = fragmentInfo[0]; - const alreadyVisited = collected.has(name); - - if (!alreadyVisited) { - collected.set(name, fragment.document); + if (!collected.has(fragmentName)) { + collected.set(fragmentName, fragment.document); if (recursive) { collectFragments(fragment.document, recursive, collected); } @@ -182,13 +178,13 @@ export function getSiblingOperations(options: ParserOptions, gqlConfig: GraphQLC siblingOperations = { available: true, - getFragments, - getOperations, getFragment, + getFragments, getFragmentByType: typeName => getFragments().filter(f => f.document.typeCondition?.name?.value === typeName), + getFragmentsInUse: (selectable, recursive = true) => Array.from(collectFragments(selectable, recursive).values()), getOperation: name => getOperations().filter(o => o.document.name?.value === name), + getOperations, getOperationByType: type => getOperations().filter(o => o.document.operation === type), - getFragmentsInUse: (selectable, recursive = true) => Array.from(collectFragments(selectable, recursive).values()), }; siblingOperationsCache.set(siblings, siblingOperations); diff --git a/packages/plugin/src/types.ts b/packages/plugin/src/types.ts index 1049bbf8c13..0d69e2d8a26 100644 --- a/packages/plugin/src/types.ts +++ b/packages/plugin/src/types.ts @@ -1,10 +1,11 @@ -import { Rule, AST, Linter } from 'eslint'; -import { ASTNode, GraphQLSchema } from 'graphql'; -import { GraphQLParseOptions } from '@graphql-tools/utils'; -import { IExtensions, IGraphQLProject, DocumentPointer, SchemaPointer } from 'graphql-config'; -import { GraphQLESLintRuleListener } from './testkit'; -import { GraphQLESTreeNode } from './estree-parser'; -import { SiblingOperations } from './sibling-operations'; +import type { Rule, AST, Linter } from 'eslint'; +import type * as ESTree from 'estree'; +import type { ASTNode, GraphQLSchema } from 'graphql'; +import type { IExtensions, IGraphQLProject, DocumentPointer, SchemaPointer } from 'graphql-config'; +import type { GraphQLParseOptions } from '@graphql-tools/utils'; +import type { GraphQLESLintRuleListener } from './testkit'; +import type { GraphQLESTreeNode } from './estree-parser'; +import type { SiblingOperations } from './sibling-operations'; export interface ParserOptions { schema?: SchemaPointer | Record }>; @@ -32,16 +33,15 @@ export type GraphQLESLintParseResult = Linter.ESLintParseResult & { services: ParserServices; }; +type ReportDescriptorLocation = { node: GraphQLESTreeNode } | { loc: AST.SourceLocation | ESTree.Position }; +export type ReportDescriptor = Rule.ReportDescriptorMessage & Rule.ReportDescriptorOptions & ReportDescriptorLocation; + export type GraphQLESLintRuleContext = Omit< Rule.RuleContext, 'parserServices' | 'report' | 'options' > & { options: Options; - report( - descriptor: Rule.ReportDescriptorMessage & - Rule.ReportDescriptorOptions & - ({ node: GraphQLESTreeNode } | { loc: AST.SourceLocation | { line: number; column: number } }) - ): void; + report(descriptor: ReportDescriptor): void; parserServices?: ParserServices; }; @@ -79,4 +79,7 @@ export type ValueOf = T[keyof T]; type Id = {} & { [P in keyof T]: T[P] }; // eslint-disable-next-line no-use-before-define type OmitDistributive = T extends object ? Id> : T; -export type OmitRecursively = Omit<{ [P in keyof T]: OmitDistributive }, K>; +export type OmitRecursively = Omit< + { [P in keyof T]: OmitDistributive }, + K +>; diff --git a/packages/plugin/tests/__snapshots__/examples.spec.md b/packages/plugin/tests/__snapshots__/examples.spec.md index cdce1605aa3..4376b0785b2 100644 --- a/packages/plugin/tests/__snapshots__/examples.spec.md +++ b/packages/plugin/tests/__snapshots__/examples.spec.md @@ -209,7 +209,7 @@ Array [ ruleId: @graphql-eslint/no-anonymous-operations, }, Object { - message: Field \`id\` must be selected when it's available on a type. + message: Field \`user.id\` must be selected when it's available on a type. Include it in your selection set., ruleId: @graphql-eslint/require-id-when-available, }, 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 296b2904c8c..6444dd086a1 100644 --- a/packages/plugin/tests/__snapshots__/require-id-when-available.spec.md +++ b/packages/plugin/tests/__snapshots__/require-id-when-available.spec.md @@ -4,7 +4,7 @@ exports[` 1`] = ` ❌ Error > 1 | { hasId { name } } - | ^ Field \`id\` must be selected when it's available on a type. + | ^ Field \`hasId.id\` must be selected when it's available on a type. Include it in your selection set. 💡 Suggestion: Add \`id\` selection @@ -22,7 +22,7 @@ exports[` 2`] = ` ❌ Error > 1 | { hasId { id } } - | ^ Field \`name\` must be selected when it's available on a type. + | ^ Field \`hasId.name\` must be selected when it's available on a type. Include it in your selection set. 💡 Suggestion: Add \`name\` selection @@ -43,7 +43,7 @@ exports[` 3`] = ` ❌ Error > 1 | { hasId { name } } - | ^ Fields \`id\` or \`_id\` must be selected when it's available on a type. + | ^ Fields \`hasId.id\` or \`hasId._id\` must be selected when it's available on a type. Include it in your selection set. 💡 Suggestion 1/2: Add \`id\` selection @@ -60,7 +60,7 @@ exports[` 4`] = ` 1 | query User { > 2 | user { - | ^ Field \`id\` must be selected when it's available on a type. + | ^ Field \`user.id\` must be selected when it's available on a type. Include it in your selection set or add to used fragments \`UserFullFields\`, \`UserMediumFields\`, or \`UserLightFields\`. 3 | ...UserFullFields 4 | } @@ -74,3 +74,45 @@ exports[` 4`] = ` 4 | } 5 | } `; + +exports[` 5`] = ` +❌ Error + + > 1 | { user { id ...UserFields } } + | ^ 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\`. +`; + +exports[` 6`] = ` +Code + + 1 | { user { ...UserFullFields } } + +❌ Error 1/4 + + > 1 | { user { ...UserFullFields } } + | ^ Field \`user.id\` must be selected when it's available on a type. + Include it in your selection set or add to used fragment \`UserFullFields\`. + +💡 Suggestion: Add \`id\` selection + + 1 | { user { id ...UserFullFields } } + +❌ Error 2/4 + + > 1 | { user { ...UserFullFields } } + | ^ 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. + 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. + Include it in your selection set or add to used fragments \`UserFullFields\` or \`UserFields\`. +`; diff --git a/packages/plugin/tests/require-id-when-available.spec.ts b/packages/plugin/tests/require-id-when-available.spec.ts index abd48d05f93..1769cacd0ca 100644 --- a/packages/plugin/tests/require-id-when-available.spec.ts +++ b/packages/plugin/tests/require-id-when-available.spec.ts @@ -48,6 +48,7 @@ const USER_POST_SCHEMA = /* GraphQL */ ` type Post { id: ID title: String + author: [User!]! } type Query { @@ -274,16 +275,6 @@ ruleTester.runGraphQLTests<[RequireIdWhenAvailableRuleConfig], true>('require-id }, ], invalid: [ - // TODO: Improve this - // { - // name: 'should report an error about missing "posts.id" selection', - // code: '{ user { id ...UserFields } }', - // errors: [MESSAGE_ID], - // parserOptions: { - // schema: USER_POST_SCHEMA, - // operations: 'fragment UserFields on User { posts { title } }' - // }, - // }, { ...WITH_SCHEMA, code: '{ hasId { name } }', @@ -329,5 +320,37 @@ ruleTester.runGraphQLTests<[RequireIdWhenAvailableRuleConfig], true>('require-id }, errors: [MESSAGE_ID], }, + { + name: 'should report an error about missing `posts.id` field in fragment', + code: '{ user { id ...UserFields } }', + errors: [MESSAGE_ID], + parserOptions: { + schema: USER_POST_SCHEMA, + operations: 'fragment UserFields on User { posts { title } }', + }, + }, + { + name: 'should report an error about missing `user.id`, `posts.id`, `author.id` and `authorPosts.id` selection', + code: '{ user { ...UserFullFields } }', + errors: [MESSAGE_ID, MESSAGE_ID, MESSAGE_ID, MESSAGE_ID], + parserOptions: { + schema: USER_POST_SCHEMA, + operations: /* GraphQL */ ` + fragment UserFullFields on User { + posts { + author { + ...UserFields + authorPosts: posts { + title + } + } + } + } + fragment UserFields on User { + name + } + `, + }, + }, ], });