-
Notifications
You must be signed in to change notification settings - Fork 44
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
ARM: Add rule to allow disabling LintDiff
AvoidAdditionalProperties (…
…#304) Closes Azure/typespec-azure-pr#4083 **Impact** 14 violations in REST API Specs (Azure/azure-rest-api-specs#27926) **azure-openapi-validator** Azure/azure-openapi-validator#667
- Loading branch information
1 parent
68fce76
commit 65da72f
Showing
7 changed files
with
336 additions
and
0 deletions.
There are no files selected for viewing
7 changes: 7 additions & 0 deletions
7
.chronus/changes/AvoidAdditionalProperties-2024-1-23-10-13-55.md
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| changeKind: feature | ||
| packages: | ||
| - "@azure-tools/typespec-azure-resource-manager" | ||
| --- | ||
|
|
||
| Add `arm-no-record` rule. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<string>; | ||
| city: string; | ||
| state: string; | ||
| } | ||
| ``` | ||
|
|
||
| #### ✅ Correct | ||
|
|
||
| ```tsp | ||
| model Address { | ||
| street: string; | ||
| city: string; | ||
| state: string; | ||
| country: string; | ||
| postalCode: string; | ||
| } | ||
| ``` | ||
|
|
||
| #### ❌ Incorrect | ||
|
|
||
| ```tsp | ||
| model Address is Record<string>; | ||
| ``` | ||
|
|
||
| #### ✅ Correct | ||
|
|
||
| ```tsp | ||
| model Address { | ||
| street: string; | ||
| city: string; | ||
| state: string; | ||
| country: string; | ||
| postalCode: string; | ||
| } | ||
| ``` | ||
|
|
||
| #### ❌ Incorrect | ||
|
|
||
| ```tsp | ||
| model Address extends Record<string> {} | ||
| ``` | ||
|
|
||
| #### ✅ Correct | ||
|
|
||
| ```tsp | ||
| model Address { | ||
| street: string; | ||
| city: string; | ||
| state: string; | ||
| country: string; | ||
| postalCode: string; | ||
| } | ||
| ``` |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
48 changes: 48 additions & 0 deletions
48
packages/typespec-azure-resource-manager/src/rules/arm-no-record.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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); | ||
| } | ||
| }, | ||
| }; | ||
| }, | ||
| }); |
206 changes: 206 additions & 0 deletions
206
packages/typespec-azure-resource-manager/test/rules/arm-no-record.test.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<WidgetProperties> { | ||
| @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<string>; | ||
| } | ||
| ` | ||
| ) | ||
| .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<string> {} | ||
| ` | ||
| ) | ||
| .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<string>; | ||
| ` | ||
| ) | ||
| .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<string>; | ||
| ` | ||
| ) | ||
| .toBeValid(); | ||
| }); | ||
|
|
||
| it("does not emit diagnostic when Record is used outside an ARM namespace", async () => { | ||
| await tester | ||
| .expect( | ||
| ` | ||
| namespace Test { | ||
| model Props is Record<unknown>; | ||
| @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<string>; | ||
| @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<string>; | ||
| } | ||
| } | ||
| ` | ||
| ) | ||
| .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<unknown>; | ||
| ${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.", | ||
| }); | ||
| }); |