From cac7cdef751ea217f74d5da5b5ff7995975b9e81 Mon Sep 17 00:00:00 2001 From: Jan Melcher Date: Fri, 6 Sep 2024 18:45:34 +0200 Subject: [PATCH] feat: add quick fix to add @suppress directive --- spec/model/validation/suppress/quick-fix.ts | 218 ++++++++++++++++++ src/model/change-set/change-set.ts | 2 + src/model/create-model.ts | 2 +- src/model/implementation/enum-type.ts | 2 +- src/model/implementation/field.ts | 2 +- src/model/implementation/flex-search.ts | 2 +- src/model/implementation/roles-specifier.ts | 2 +- src/model/implementation/root-entity-type.ts | 2 +- src/model/implementation/time-to-live.ts | 2 +- src/model/implementation/type-base.ts | 2 +- src/model/validation/message.ts | 69 ++---- .../validation/suppress/is-suppressed.ts | 31 +++ .../validation/suppress}/message-codes.ts | 0 src/model/validation/suppress/quick-fix.ts | 169 ++++++++++++++ src/model/validation/suppress/utils.ts | 19 ++ src/schema/graphql-base.ts | 4 +- ...-unused-non-root-object-types-validator.ts | 2 +- 17 files changed, 475 insertions(+), 55 deletions(-) create mode 100644 spec/model/validation/suppress/quick-fix.ts create mode 100644 src/model/validation/suppress/is-suppressed.ts rename src/{schema => model/validation/suppress}/message-codes.ts (100%) create mode 100644 src/model/validation/suppress/quick-fix.ts create mode 100644 src/model/validation/suppress/utils.ts diff --git a/spec/model/validation/suppress/quick-fix.ts b/spec/model/validation/suppress/quick-fix.ts new file mode 100644 index 00000000..3b669a0a --- /dev/null +++ b/spec/model/validation/suppress/quick-fix.ts @@ -0,0 +1,218 @@ +import { expectQuickFix } from '../../implementation/validation-utils'; +import gql from 'graphql-tag'; +import { Project } from '../../../../src/project/project'; + +describe('@suppress quick fix', () => { + it('generates a quick fix on type level', () => { + const project = new Project([ + gql` + type Stuff @rootEntity { + foo: String + } + # comment before type + type Child @childEntity { + # comment in type + stuff: Int + } + `.loc!.source, + ]); + expectQuickFix( + project, + 'Suppress this warning', + ` + type Stuff @rootEntity { + foo: String + } + # comment before type + type Child @childEntity @suppress(warnings: UNUSED) { + # comment in type + stuff: Int + } + `, + ); + }); + + it('generates a quick fix on enum level', () => { + const project = new Project([ + gql` + type Stuff @rootEntity { + value: myenum + } + + enum myenum { + NAME1 + NAME2 + } + `.loc!.source, + ]); + expectQuickFix( + project, + 'Suppress this warning', + ` + type Stuff @rootEntity { + value: myenum + } + + enum myenum @suppress(warnings: NAMING) { + NAME1 + NAME2 + } + `, + ); + }); + + it('generates a quick fix on enum value level', () => { + const project = new Project([ + gql` + type Stuff @rootEntity { + value: MyEnum + } + + enum MyEnum { + NAME1 + name2 + } + `.loc!.source, + ]); + expectQuickFix( + project, + 'Suppress this warning', + ` + type Stuff @rootEntity { + value: MyEnum + } + + enum MyEnum { + NAME1 + name2 @suppress(warnings: NAMING) + } + `, + ); + }); + + it('generates a quick fix on field level', () => { + const project = new Project([ + gql` + type Stuff @rootEntity { + _key: String @key # after field + key: String + } + `.loc!.source, + ]); + expectQuickFix( + project, + 'Suppress this warning', + ` + type Stuff @rootEntity { + _key: String @key @suppress(warnings: DEPRECATED) # after field + key: String + } + `, + ); + }); + + it('generates a quick fix if there already is an empty @suppress directive', () => { + const project = new Project([ + gql` + type Stuff @rootEntity { + _key: String @key @suppress + key: String + } + `.loc!.source, + ]); + expectQuickFix( + project, + 'Suppress this warning', + ` + type Stuff @rootEntity { + _key: String @key @suppress(warnings: DEPRECATED) + key: String + } + `, + ); + }); + + it('generates a quick fix if there already is a @suppress directive with a different arg', () => { + const project = new Project([ + gql` + type Stuff @rootEntity { + _key: String @key @suppress(infos: NO_TYPE_CHECKS) + key: String + } + `.loc!.source, + ]); + expectQuickFix( + project, + 'Suppress this warning', + ` + type Stuff @rootEntity { + _key: String @key @suppress(infos: NO_TYPE_CHECKS, warnings: DEPRECATED) + key: String + } + `, + ); + }); + + it('generates a quick fix if there already is a @suppress directive with a single code', () => { + const project = new Project([ + gql` + type Stuff @rootEntity { + _key: String @key @suppress(warnings: UNUSED) + key: String + } + `.loc!.source, + ]); + expectQuickFix( + project, + 'Suppress this warning', + ` + type Stuff @rootEntity { + _key: String @key @suppress(warnings: [UNUSED, DEPRECATED]) + key: String + } + `, + ); + }); + + it('generates a quick fix if there already is a @suppress directive with a single code as a list', () => { + const project = new Project([ + gql` + type Stuff @rootEntity { + _key: String @key @suppress(warnings: [UNUSED]) + key: String + } + `.loc!.source, + ]); + expectQuickFix( + project, + 'Suppress this warning', + ` + type Stuff @rootEntity { + _key: String @key @suppress(warnings: [UNUSED, DEPRECATED]) + key: String + } + `, + ); + }); + + it('generates a quick fix if there already is a @suppress directive with a multiple codes as a list', () => { + const project = new Project([ + gql` + type Stuff @rootEntity { + _key: String @key @suppress(warnings: [UNUSED, NAMING]) + key: String + } + `.loc!.source, + ]); + expectQuickFix( + project, + 'Suppress this warning', + ` + type Stuff @rootEntity { + _key: String @key @suppress(warnings: [UNUSED, NAMING, DEPRECATED]) + key: String + } + `, + ); + }); +}); diff --git a/src/model/change-set/change-set.ts b/src/model/change-set/change-set.ts index 4cdf687a..ad9204cc 100644 --- a/src/model/change-set/change-set.ts +++ b/src/model/change-set/change-set.ts @@ -15,6 +15,8 @@ export class ChangeSet { this.textChanges = changes.filter((c) => c instanceof TextChange); this.appendChanges = changes.filter((c) => c instanceof AppendChange); } + + static EMPTY = new ChangeSet([]); } /** diff --git a/src/model/create-model.ts b/src/model/create-model.ts index a4c3ace7..983330a3 100644 --- a/src/model/create-model.ts +++ b/src/model/create-model.ts @@ -124,7 +124,7 @@ import { parseI18nConfigs } from './parse-i18n'; import { parseModuleConfigs } from './parse-modules'; import { parseTTLConfigs } from './parse-ttl'; import { ValidationContext, ValidationMessage } from './validation'; -import { WarningCode } from '../schema/message-codes'; +import { WarningCode } from './validation/suppress/message-codes'; export function createModel(parsedProject: ParsedProject, options: ModelOptions = {}): Model { const validationContext = new ValidationContext(); diff --git a/src/model/implementation/enum-type.ts b/src/model/implementation/enum-type.ts index e2f88b65..33a18c5f 100644 --- a/src/model/implementation/enum-type.ts +++ b/src/model/implementation/enum-type.ts @@ -6,7 +6,7 @@ import { EnumValueLocalization } from './i18n'; import { Model } from './model'; import { TypeBase } from './type-base'; import memorize from 'memorize-decorator'; -import { WarningCode } from '../../schema/message-codes'; +import { WarningCode } from '../validation/suppress/message-codes'; export class EnumType extends TypeBase { constructor(input: EnumTypeConfig, model: Model) { diff --git a/src/model/implementation/field.ts b/src/model/implementation/field.ts index f3a3ea26..8b87d03d 100644 --- a/src/model/implementation/field.ts +++ b/src/model/implementation/field.ts @@ -55,7 +55,7 @@ import { Relation, RelationSide } from './relation'; import { RolesSpecifier } from './roles-specifier'; import { InvalidType, ObjectType, Type } from './type'; import { ValueObjectType } from './value-object-type'; -import { WarningCode } from '../../schema/message-codes'; +import { WarningCode } from '../validation/suppress/message-codes'; export interface SystemFieldConfig extends FieldConfig { readonly isSystemField?: boolean; diff --git a/src/model/implementation/flex-search.ts b/src/model/implementation/flex-search.ts index b3806ff5..73bf78b5 100644 --- a/src/model/implementation/flex-search.ts +++ b/src/model/implementation/flex-search.ts @@ -4,7 +4,7 @@ import { ModelComponent, ValidationContext } from '../validation/validation-cont import { FlexSearchPrimarySortClauseConfig } from '../config'; import { RootEntityType } from './root-entity-type'; import { Severity, ValidationMessage } from '../validation'; -import { WarningCode } from '../../schema/message-codes'; +import { WarningCode } from '../validation/suppress/message-codes'; export const IDENTITY_ANALYZER = 'identity'; export const NORM_CI_ANALYZER = 'norm_ci'; diff --git a/src/model/implementation/roles-specifier.ts b/src/model/implementation/roles-specifier.ts index 6f15e20b..2ffdd7e4 100644 --- a/src/model/implementation/roles-specifier.ts +++ b/src/model/implementation/roles-specifier.ts @@ -1,7 +1,7 @@ import { ModelComponent, ValidationContext } from '../validation/validation-context'; import { RolesSpecifierConfig } from '../config'; import { ValidationMessage } from '../validation'; -import { WarningCode } from '../../schema/message-codes'; +import { WarningCode } from '../validation/suppress/message-codes'; import { Type } from './type'; import { Field } from './field'; diff --git a/src/model/implementation/root-entity-type.ts b/src/model/implementation/root-entity-type.ts index 307a1ed1..75aa6130 100644 --- a/src/model/implementation/root-entity-type.ts +++ b/src/model/implementation/root-entity-type.ts @@ -35,7 +35,7 @@ import { RolesSpecifier } from './roles-specifier'; import { ScalarType } from './scalar-type'; import { TimeToLiveType } from './time-to-live'; import { EffectiveModuleSpecification } from './modules/effective-module-specification'; -import { WarningCode } from '../../schema/message-codes'; +import { WarningCode } from '../validation/suppress/message-codes'; export class RootEntityType extends ObjectTypeBase { private readonly permissions: PermissionsConfig & {}; diff --git a/src/model/implementation/time-to-live.ts b/src/model/implementation/time-to-live.ts index ad1163e2..f4205885 100644 --- a/src/model/implementation/time-to-live.ts +++ b/src/model/implementation/time-to-live.ts @@ -11,7 +11,7 @@ import { RootEntityType } from './root-entity-type'; import { ScalarType } from './scalar-type'; import { Type } from './type'; import { FieldPath } from './field-path'; -import { WarningCode } from '../../schema/message-codes'; +import { WarningCode } from '../validation/suppress/message-codes'; export class TimeToLiveType implements ModelComponent { readonly cascadeFields: ReadonlyArray = []; diff --git a/src/model/implementation/type-base.ts b/src/model/implementation/type-base.ts index dff9a31d..4ef86c51 100644 --- a/src/model/implementation/type-base.ts +++ b/src/model/implementation/type-base.ts @@ -11,7 +11,7 @@ import { EffectiveModuleSpecification } from './modules/effective-module-specifi import { MODULES_DIRECTIVE } from '../../schema/constants'; import { TypeModuleSpecification } from './modules/type-module-specification'; import { Type } from './type'; -import { WarningCode } from '../../schema/message-codes'; +import { WarningCode } from '../validation/suppress/message-codes'; export abstract class TypeBase implements ModelComponent { readonly name: string; diff --git a/src/model/validation/message.ts b/src/model/validation/message.ts index e4b9a1f2..c07f224c 100644 --- a/src/model/validation/message.ts +++ b/src/model/validation/message.ts @@ -1,4 +1,14 @@ -import { EnumValueDefinitionNode, FieldDefinitionNode, Kind, TypeDefinitionNode } from 'graphql'; +import { + ArgumentNode, + ASTNode, + DirectiveNode, + EnumValueDefinitionNode, + FieldDefinitionNode, + Kind, + ListValueNode, + print, + TypeDefinitionNode, +} from 'graphql'; import { SUPPRESS_COMPATIBILITY_ISSUES_ARG, SUPPRESS_DIRECTIVE, @@ -10,9 +20,12 @@ import { InfoCode, MessageCode, WarningCode, -} from '../../schema/message-codes'; +} from './suppress/message-codes'; import { LocationLike, MessageLocation } from './location'; import { QuickFix } from './quick-fix'; +import { ChangeSet, TextChange } from '../change-set/change-set'; +import { isSuppressed } from './suppress/is-suppressed'; +import { createSuppressQuickFix } from './suppress/quick-fix'; export enum Severity { ERROR = 'ERROR', @@ -70,14 +83,21 @@ export class ValidationMessage { ) { // not sure if this is the right time to do this // also does not allow us to detect superfluous directives at the moment - const isSuppressed = calculateIsSuppressed(severity, astNode, code); + const suppressed = isSuppressed(severity, astNode, code); + let quickFixes = options?.quickFixes ?? []; + if (!suppressed && astNode) { + const suppressQuickFix = createSuppressQuickFix(severity, code, astNode); + if (suppressQuickFix) { + quickFixes = [...quickFixes, suppressQuickFix]; + } + } return new ValidationMessage({ severity, code, message, location: options?.location ?? astNode, - isSuppressed, - ...options, + isSuppressed: suppressed, + quickFixes, }); } @@ -190,42 +210,3 @@ function severityToString(severity: Severity) { return 'Compatibility issue'; } } - -function calculateIsSuppressed( - severity: Severity, - location: AstNodeWithDirectives | undefined, - code: MessageCode, -) { - const suppressDirective = location?.directives?.find( - (d) => d.name.value === SUPPRESS_DIRECTIVE, - ); - if (!suppressDirective) { - return false; - } - let argName; - switch (severity) { - case Severity.COMPATIBILITY_ISSUE: - argName = SUPPRESS_COMPATIBILITY_ISSUES_ARG; - break; - case Severity.WARNING: - argName = SUPPRESS_WARNINGS_ARG; - break; - case Severity.INFO: - argName = SUPPRESS_INFOS_ARG; - break; - default: - throw new Error(`Non-suppressable severity: ${severity}`); - } - const codesArg = suppressDirective?.arguments?.find((a) => a.name.value === argName); - if (!codesArg) { - return false; - } - if (codesArg.value.kind === Kind.ENUM) { - // you can omit the [] in graphql if it's a single list entry - return codesArg.value.value === code; - } - if (codesArg.value.kind !== Kind.LIST) { - return false; - } - return codesArg.value.values.some((v) => v.kind === Kind.ENUM && v.value === code); -} diff --git a/src/model/validation/suppress/is-suppressed.ts b/src/model/validation/suppress/is-suppressed.ts new file mode 100644 index 00000000..4badf779 --- /dev/null +++ b/src/model/validation/suppress/is-suppressed.ts @@ -0,0 +1,31 @@ +import { MessageCode } from './message-codes'; +import { SUPPRESS_DIRECTIVE } from '../../../schema/constants'; +import { Kind } from 'graphql/index'; +import { AstNodeWithDirectives, Severity } from '../message'; +import { getSuppressArgName } from './utils'; + +export function isSuppressed( + severity: Severity, + location: AstNodeWithDirectives | undefined, + code: MessageCode, +) { + const suppressDirective = location?.directives?.find( + (d) => d.name.value === SUPPRESS_DIRECTIVE, + ); + if (!suppressDirective) { + return false; + } + const argName = getSuppressArgName(severity); + const codesArg = suppressDirective?.arguments?.find((a) => a.name.value === argName); + if (!codesArg) { + return false; + } + if (codesArg.value.kind === Kind.ENUM) { + // you can omit the [] in graphql if it's a single list entry + return codesArg.value.value === code; + } + if (codesArg.value.kind !== Kind.LIST) { + return false; + } + return codesArg.value.values.some((v) => v.kind === Kind.ENUM && v.value === code); +} diff --git a/src/schema/message-codes.ts b/src/model/validation/suppress/message-codes.ts similarity index 100% rename from src/schema/message-codes.ts rename to src/model/validation/suppress/message-codes.ts diff --git a/src/model/validation/suppress/quick-fix.ts b/src/model/validation/suppress/quick-fix.ts new file mode 100644 index 00000000..ac850bd8 --- /dev/null +++ b/src/model/validation/suppress/quick-fix.ts @@ -0,0 +1,169 @@ +import { ArgumentNode, ASTNode, DirectiveNode, Kind, ListValueNode, print } from 'graphql/index'; +import { MessageCode } from './message-codes'; +import { QuickFix } from '../quick-fix'; +import { SUPPRESS_DIRECTIVE } from '../../../schema/constants'; +import { MessageLocation } from '../location'; +import { ChangeSet, TextChange } from '../../change-set/change-set'; +import { AstNodeWithDirectives, Severity } from '../message'; +import { getSuppressArgName } from './utils'; + +export function createSuppressQuickFix( + severity: Severity, + code: MessageCode, + astNode: AstNodeWithDirectives | undefined, +): QuickFix | undefined { + if (!astNode || !astNode.loc) { + return undefined; + } + + let messageKind; + switch (severity) { + case Severity.WARNING: + messageKind = 'warning'; + break; + case Severity.INFO: + messageKind = 'info'; + break; + case Severity.COMPATIBILITY_ISSUE: + messageKind = 'compatibility issue'; + break; + default: + messageKind = severity.toString(); + } + + return new QuickFix({ + description: `Suppress this ${messageKind}`, + isPreferred: false, + + changeSet: () => getChangeSet(severity, code, astNode), + }); +} + +// can theoretically generate an empty change set, but only if +// - a loc is missing (unlikely, since we check if the astNode has a loc in createSuppressQuickFix) +// - the code is already suppressed (in which case createSuppressQuickFix() should not be called +// in the first place) +function getChangeSet( + severity: Severity, + code: MessageCode, + astNode: AstNodeWithDirectives, +): ChangeSet { + const argName = getSuppressArgName(severity); + const newArgumentNode: ArgumentNode = { + kind: Kind.ARGUMENT, + name: { kind: Kind.NAME, value: argName }, + value: { kind: Kind.ENUM, value: code }, + }; + const newDirectiveNode: DirectiveNode = { + kind: Kind.DIRECTIVE, + name: { + kind: Kind.NAME, + value: SUPPRESS_DIRECTIVE, + }, + arguments: [newArgumentNode], + }; + + const existingDirective = astNode.directives?.find((d) => d.name.value === SUPPRESS_DIRECTIVE); + if (!existingDirective) { + // append the @suppress() directive + return appendAfterNode(getNewDirectiveLocation(astNode), ' ' + print(newDirectiveNode)); + } + + if (!existingDirective.arguments?.length) { + // plain @suppress without any arguments - just replace the directive + return replaceNode(existingDirective, print(newDirectiveNode)); + } + + const existingArg = existingDirective.arguments?.find((d) => d.name.value === argName); + if (!existingArg) { + // @suppress() is already there but with a different argument - add our argument + return appendAfterNode(existingDirective.arguments.at(-1)!, ', ' + print(newArgumentNode)); + } + + switch (existingArg.value.kind) { + case Kind.ENUM: + if (existingArg.value.value === code) { + // already suppressed (should not happen) + return ChangeSet.EMPTY; + } + + // we need to change the single value into a list + const newArgumentValue: ListValueNode = { + kind: Kind.LIST, + values: [existingArg.value, newArgumentNode.value], + }; + return replaceNode(existingArg.value, print(newArgumentValue)); + + case Kind.LIST: + if (existingArg.value.values.some((v) => v.kind === Kind.ENUM && v.value === code)) { + // already suppressed (should not happen) + return ChangeSet.EMPTY; + } + + // append ", " after the last value + return appendAfterNode( + existingArg.value.values.at(-1)!, + ', ' + print(newArgumentNode.value), + ); + + default: + // invalid type + return ChangeSet.EMPTY; + } +} + +/** + * Finds the location where a new directive should be added + * + * @param astNode the node after which the directive should be placed, separated by a space + */ +function getNewDirectiveLocation(astNode: AstNodeWithDirectives): ASTNode | undefined { + // if there already are directives, we can just add our directive after the last one + const lastDirective = astNode.directives?.at(-1); + if (lastDirective) { + return lastDirective; + } + + // need to figure out where directives are placed in the given definition + switch (astNode.kind) { + case Kind.FIELD_DEFINITION: + case Kind.ENUM_VALUE_DEFINITION: + // directives occur at the end of the node + return astNode; + case Kind.ENUM_TYPE_DEFINITION: + // directives occur between the name and the values + return astNode.name; + case Kind.OBJECT_TYPE_DEFINITION: + // directives occur between the name (or interface list, if exists) and the fields + // we currently don't support "implements" but we might in the future + const lastInterface = astNode.interfaces?.at(-1); + if (lastInterface) { + return lastInterface; + } else { + return astNode.name; + } + default: + // the other definition types do not occur in cruddl + return undefined; + } +} + +function appendAfterNode(astNode: ASTNode | undefined, text: string): ChangeSet { + if (!astNode?.loc) { + return ChangeSet.EMPTY; + } + const appendAfterLocation = MessageLocation.fromGraphQLLocation(astNode.loc); + const changeLocation = new MessageLocation( + appendAfterLocation.source, + appendAfterLocation.end, + appendAfterLocation.end, + ); + return new ChangeSet([new TextChange(changeLocation, text)]); +} + +function replaceNode(astNode: ASTNode | undefined, text: string): ChangeSet { + if (!astNode?.loc) { + return ChangeSet.EMPTY; + } + return new ChangeSet([new TextChange(MessageLocation.fromGraphQLLocation(astNode.loc), text)]); +} diff --git a/src/model/validation/suppress/utils.ts b/src/model/validation/suppress/utils.ts new file mode 100644 index 00000000..7b9a4546 --- /dev/null +++ b/src/model/validation/suppress/utils.ts @@ -0,0 +1,19 @@ +import { + SUPPRESS_COMPATIBILITY_ISSUES_ARG, + SUPPRESS_INFOS_ARG, + SUPPRESS_WARNINGS_ARG, +} from '../../../schema/constants'; +import { Severity } from '../message'; + +export function getSuppressArgName(severity: Severity) { + switch (severity) { + case Severity.COMPATIBILITY_ISSUE: + return SUPPRESS_COMPATIBILITY_ISSUES_ARG; + case Severity.WARNING: + return SUPPRESS_WARNINGS_ARG; + case Severity.INFO: + return SUPPRESS_INFOS_ARG; + default: + throw new Error(`Non-suppressable severity: ${severity}`); + } +} diff --git a/src/schema/graphql-base.ts b/src/schema/graphql-base.ts index ef467769..f2a88ef3 100644 --- a/src/schema/graphql-base.ts +++ b/src/schema/graphql-base.ts @@ -11,7 +11,7 @@ import { INFO_CODES, MessageCodes, WARNING_CODES, -} from './message-codes'; +} from '../model/validation/suppress/message-codes'; const directivesBase: DocumentNode = gql` "Declares a type for root-level objects with ids that are stored directly in the data base" @@ -285,7 +285,7 @@ const directivesBase: DocumentNode = gql` warnings: [WarningCode!] infos: [InfoCode!] compatibilityIssues: [CompatibilityIssueCode!] - ) on OBJECT | ENUM | FIELD_DEFINITION + ) on OBJECT | ENUM | ENUM_VALUE | FIELD_DEFINITION "Codes that can be used with @suppress(warnings: [...])" enum WarningCode { diff --git a/src/schema/preparation/ast-validation-modules/no-unused-non-root-object-types-validator.ts b/src/schema/preparation/ast-validation-modules/no-unused-non-root-object-types-validator.ts index be47d0f5..2949c82b 100644 --- a/src/schema/preparation/ast-validation-modules/no-unused-non-root-object-types-validator.ts +++ b/src/schema/preparation/ast-validation-modules/no-unused-non-root-object-types-validator.ts @@ -7,7 +7,7 @@ import { getTypeNameIgnoringNonNullAndList, } from '../../schema-utils'; import { ASTValidator } from '../ast-validator'; -import { WarningCode } from '../../message-codes'; +import { WarningCode } from '../../../model/validation/suppress/message-codes'; export class NoUnusedNonRootObjectTypesValidator implements ASTValidator { validate(ast: DocumentNode): ReadonlyArray {