From 65da72f51e235836b5ded5352cabb03c03613ed4 Mon Sep 17 00:00:00 2001 From: Travis Prescott Date: Thu, 29 Feb 2024 09:02:15 -0800 Subject: [PATCH] ARM: Add rule to allow disabling LintDiff `AvoidAdditionalProperties` (#304) Closes https://github.com/Azure/typespec-azure-pr/issues/4083 **Impact** 14 violations in REST API Specs (https://github.com/Azure/azure-rest-api-specs/pull/27926) **azure-openapi-validator** https://github.com/Azure/azure-openapi-validator/pull/667 --- ...AdditionalProperties-2024-1-23-10-13-55.md | 7 + .../reference/linter.md | 1 + .../azure-resource-manager/rules/no-record.md | 67 ++++++ .../typespec-azure-resource-manager/README.md | 1 + .../src/linter.ts | 6 + .../src/rules/arm-no-record.ts | 48 ++++ .../test/rules/arm-no-record.test.ts | 206 ++++++++++++++++++ 7 files changed, 336 insertions(+) create mode 100644 .chronus/changes/AvoidAdditionalProperties-2024-1-23-10-13-55.md create mode 100644 docs/libraries/azure-resource-manager/rules/no-record.md create mode 100644 packages/typespec-azure-resource-manager/src/rules/arm-no-record.ts create mode 100644 packages/typespec-azure-resource-manager/test/rules/arm-no-record.test.ts diff --git a/.chronus/changes/AvoidAdditionalProperties-2024-1-23-10-13-55.md b/.chronus/changes/AvoidAdditionalProperties-2024-1-23-10-13-55.md new file mode 100644 index 0000000000..3a41df0cf2 --- /dev/null +++ b/.chronus/changes/AvoidAdditionalProperties-2024-1-23-10-13-55.md @@ -0,0 +1,7 @@ +--- +changeKind: feature +packages: + - "@azure-tools/typespec-azure-resource-manager" +--- + +Add `arm-no-record` rule. diff --git a/docs/libraries/azure-resource-manager/reference/linter.md b/docs/libraries/azure-resource-manager/reference/linter.md index 89d4322c77..0f29c5088d 100644 --- a/docs/libraries/azure-resource-manager/reference/linter.md +++ b/docs/libraries/azure-resource-manager/reference/linter.md @@ -26,6 +26,7 @@ Available ruleSets: | Name | Description | | ---------------------------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------- | +| [`@azure-tools/typespec-azure-resource-manager/arm-no-record`](/libraries/azure-resource-manager/rules/no-record.md) | Don't use Record types for ARM resources. | | `@azure-tools/typespec-azure-resource-manager/arm-common-types-version` | Specify the ARM common-types version using @armCommonTypesVersion. | | `@azure-tools/typespec-azure-resource-manager/arm-resource-action-no-segment` | `@armResourceAction` should not be used with `@segment`. | | `@azure-tools/typespec-azure-resource-manager/arm-resource-duplicate-property` | Warn about duplicate properties in resources. | diff --git a/docs/libraries/azure-resource-manager/rules/no-record.md b/docs/libraries/azure-resource-manager/rules/no-record.md new file mode 100644 index 0000000000..b917f154e8 --- /dev/null +++ b/docs/libraries/azure-resource-manager/rules/no-record.md @@ -0,0 +1,67 @@ +--- +title: no-record +--- + +```text title=- Full name- +@azure-tools/typespec-azure-resource-manager/no-record +``` + +ARM requires Resource provider teams to define types explicitly. This is to ensure good customer experience in terms of the discoverability of concrete type definitions. + +#### ❌ Incorrect + +```tsp +model Address { + address: Record; + city: string; + state: string; +} +``` + +#### ✅ Correct + +```tsp +model Address { + street: string; + city: string; + state: string; + country: string; + postalCode: string; +} +``` + +#### ❌ Incorrect + +```tsp +model Address is Record; +``` + +#### ✅ Correct + +```tsp +model Address { + street: string; + city: string; + state: string; + country: string; + postalCode: string; +} +``` + +#### ❌ Incorrect + +```tsp +model Address extends Record {} +``` + +#### ✅ Correct + +```tsp +model Address { + street: string; + city: string; + state: string; + country: string; + postalCode: string; +} +``` diff --git a/packages/typespec-azure-resource-manager/README.md b/packages/typespec-azure-resource-manager/README.md index 4f1bf21ae5..49d3ee7f13 100644 --- a/packages/typespec-azure-resource-manager/README.md +++ b/packages/typespec-azure-resource-manager/README.md @@ -30,6 +30,7 @@ Available ruleSets: | Name | Description | | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------- | +| [`@azure-tools/typespec-azure-resource-manager/arm-no-record`](https://azure.github.io/typespec-azure/docs/libraries/azure-resource-manager/rules/no-record) | Don't use Record types for ARM resources. | | `@azure-tools/typespec-azure-resource-manager/arm-common-types-version` | Specify the ARM common-types version using @armCommonTypesVersion. | | `@azure-tools/typespec-azure-resource-manager/arm-resource-action-no-segment` | `@armResourceAction` should not be used with `@segment`. | | `@azure-tools/typespec-azure-resource-manager/arm-resource-duplicate-property` | Warn about duplicate properties in resources. | diff --git a/packages/typespec-azure-resource-manager/src/linter.ts b/packages/typespec-azure-resource-manager/src/linter.ts index 03b76ca9f8..6361bdcc6b 100644 --- a/packages/typespec-azure-resource-manager/src/linter.ts +++ b/packages/typespec-azure-resource-manager/src/linter.ts @@ -1,5 +1,6 @@ import { defineLinter } from "@typespec/compiler"; import { armCommonTypesVersionRule } from "./rules/arm-common-types-version.js"; +import { armNoRecordRule } from "./rules/arm-no-record.js"; import { armResourceActionNoSegmentRule } from "./rules/arm-resource-action-no-segment.js"; import { armResourceDuplicatePropertiesRule } from "./rules/arm-resource-duplicate-property.js"; import { interfacesRule } from "./rules/arm-resource-interfaces.js"; @@ -25,6 +26,7 @@ import { retryAfterRule } from "./rules/retry-after.js"; import { unsupportedTypeRule } from "./rules/unsupported-type.js"; const rules = [ + armNoRecordRule, armCommonTypesVersionRule, armResourceActionNoSegmentRule, armResourceDuplicatePropertiesRule, @@ -64,6 +66,10 @@ export const $linter = defineLinter({ // TODO: Enable this rule once azure-rest-api-specs repo is ready (issue #3839) [`@azure-tools/typespec-azure-resource-manager/${armCommonTypesVersionRule.name}`]: false, }, + disable: { + [`@azure-tools/typespec-azure-core/bad-record-type`]: + "This clashes with the ARM `no-record` rule.", + }, extends: ["@azure-tools/typespec-azure-core/all"], }, }, diff --git a/packages/typespec-azure-resource-manager/src/rules/arm-no-record.ts b/packages/typespec-azure-resource-manager/src/rules/arm-no-record.ts new file mode 100644 index 0000000000..d4cbf1ed1e --- /dev/null +++ b/packages/typespec-azure-resource-manager/src/rules/arm-no-record.ts @@ -0,0 +1,48 @@ +import { DiagnosticTarget, Model, SemanticNodeListener, createRule } from "@typespec/compiler"; +import { getArmResources } from "../resource.js"; + +export const armNoRecordRule = createRule({ + name: "arm-no-record", + severity: "warning", + description: "Don't use Record types for ARM resources.", + url: "https://azure.github.io/typespec-azure/docs/libraries/azure-resource-manager/rules/no-record", + messages: { + default: + "Model properties or operation parameters should not be of type Record. ARM requires Resource provider teams to define types explicitly.", + extends: + "Models should not extend type Record. ARM requires Resource provider teams to define types explicitly.", + is: "Models should not equate to type Record. ARM requires Resource provider teams to define types explicitly.", + }, + create(context): SemanticNodeListener { + return { + root: (_) => { + function checkModel(model: Model, target: DiagnosticTarget, kind?: "extends" | "is") { + if (model.name === "Record") { + context.reportDiagnostic({ + code: "arm-no-record", + target: target, + messageId: kind || "default", + }); + } else if (model.baseModel !== undefined) { + checkModel(model.baseModel, model, "extends"); + } else if (model.sourceModel !== undefined) { + checkModel(model.sourceModel, model, "is"); + } + if (model?.properties !== undefined) { + for (const prop of model.properties.values()) { + if (prop.type.kind === "Model") { + checkModel(prop.type, prop); + } + } + } + } + + // ensure only ARM resources and models they touch are checked + const resources = getArmResources(context.program); + for (const resource of resources) { + checkModel(resource.typespecType, resource.typespecType); + } + }, + }; + }, +}); diff --git a/packages/typespec-azure-resource-manager/test/rules/arm-no-record.test.ts b/packages/typespec-azure-resource-manager/test/rules/arm-no-record.test.ts new file mode 100644 index 0000000000..c543413846 --- /dev/null +++ b/packages/typespec-azure-resource-manager/test/rules/arm-no-record.test.ts @@ -0,0 +1,206 @@ +import { + BasicTestRunner, + LinterRuleTester, + createLinterRuleTester, +} from "@typespec/compiler/testing"; +import { beforeEach, it } from "vitest"; +import { armNoRecordRule } from "../../src/rules/arm-no-record.js"; +import { createAzureResourceManagerTestRunner } from "../test-host.js"; + +let runner: BasicTestRunner; +let tester: LinterRuleTester; + +beforeEach(async () => { + runner = await createAzureResourceManagerTestRunner(); + tester = createLinterRuleTester( + runner, + armNoRecordRule, + "@azure-tools/typespec-azure-resource-manager" + ); +}); + +const nsDef = ` +@armProviderNamespace +@useDependency(Azure.ResourceManager.Versions.v1_0_Preview_1) +namespace Microsoft.Contoso; +`; + +const resource = ` +@Azure.ResourceManager.tenantResource +model Widget is ProxyResource { + @key("widgetName") + @segment("widgets") + @path + @visibility("read") + name: string; +} +`; + +it("emits diagnostic when a model property uses Record type", async () => { + await tester + .expect( + ` + ${nsDef} + ${resource} + model WidgetProperties { + props: Record; + } + ` + ) + .toEmitDiagnostics({ + code: "@azure-tools/typespec-azure-resource-manager/arm-no-record", + message: + "Model properties or operation parameters should not be of type Record. ARM requires Resource provider teams to define types explicitly.", + }); +}); + +it("emits diagnostic when a model extends Record type", async () => { + await tester + .expect( + ` + ${nsDef} + ${resource} + model WidgetProperties extends Record {} + ` + ) + .toEmitDiagnostics({ + code: "@azure-tools/typespec-azure-resource-manager/arm-no-record", + message: + "Models should not extend type Record. ARM requires Resource provider teams to define types explicitly.", + }); +}); + +it("emits diagnostic when a model is Record type", async () => { + await tester + .expect( + ` + ${nsDef} + ${resource} + model WidgetProperties is Record; + ` + ) + .toEmitDiagnostics({ + code: "@azure-tools/typespec-azure-resource-manager/arm-no-record", + message: + "Models should not equate to type Record. ARM requires Resource provider teams to define types explicitly.", + }); +}); + +it("does not emit diagnostic when Record is used but not referenced by an ARM resource", async () => { + await tester + .expect( + ` + ${nsDef} + // should not throw because WidgetProperties is not an ARM resources and is not + // referenced by an ARM resource. + model WidgetProperties is Record; + ` + ) + .toBeValid(); +}); + +it("does not emit diagnostic when Record is used outside an ARM namespace", async () => { + await tester + .expect( + ` + namespace Test { + model Props is Record; + + @armProviderNamespace + @useDependency(Azure.ResourceManager.Versions.v1_0_Preview_1) + namespace Arm { + model WidgetProperties {}; + } + } + ` + ) + .toBeValid(); +}); + +it("emits diagnostic if an ARM Resource references a model that uses Record type", async () => { + await tester + .expect( + ` + namespace NonArm { + model Properties is Record; + + @armProviderNamespace + @useDependency(Azure.ResourceManager.Versions.v1_0_Preview_1) + namespace Arm { + ${resource} + + model WidgetProperties { + props: Properties; + } + } + } + ` + ) + .toEmitDiagnostics({ + code: "@azure-tools/typespec-azure-resource-manager/arm-no-record", + message: + "Models should not equate to type Record. ARM requires Resource provider teams to define types explicitly.", + }); +}); + +it("emits diagnostic if an ARM Resource references a subnamespace model that uses Record type", async () => { + await tester + .expect( + ` + @armProviderNamespace + @useDependency(Azure.ResourceManager.Versions.v1_0_Preview_1) + namespace Arm { + ${resource} + + model WidgetProperties { + props: Sub.Properties; + } + + namespace Sub { + model Properties is Record; + } + } + ` + ) + .toEmitDiagnostics({ + code: "@azure-tools/typespec-azure-resource-manager/arm-no-record", + message: + "Models should not equate to type Record. ARM requires Resource provider teams to define types explicitly.", + }); +}); + +it("does not emit diagnostic if ArmTagsProperty is used", async () => { + await tester + .expect( + ` + ${nsDef} + ${resource} + model WidgetProperties { + ...Foundations.ArmTagsProperty; + } + ` + ) + .toBeValid(); +}); + +it("emits a diagnostic if a deeply aliased model use Record type", async () => { + await tester + .expect( + ` + ${nsDef} + + model Foo is Bar; + model Bar is Record; + + ${resource} + model WidgetProperties { + props: Foo; + } + ` + ) + .toEmitDiagnostics({ + code: "@azure-tools/typespec-azure-resource-manager/arm-no-record", + message: + "Models should not equate to type Record. ARM requires Resource provider teams to define types explicitly.", + }); +});