Repository navigation
[Swagger Linter Migration] RepeatedPathInfo -> no-repeated-path-info #5526
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Yuchao Yan (msyyc)
wants to merge
20
commits into
main
Choose a base branch
from
promote-lintdiff-repeated-path-info
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
f6e6c26
Add ARM no-repeated-path-info lint rule
msyyc 1405422
Move repeated path migration detail to LintDiff section
msyyc 5985744
Merge branch 'main' into promote-lintdiff-repeated-path-info
msyyc 4bf3599
Clarify complementary ARM property duplication rules
msyyc c68dbb5
Merge branch 'main' into promote-lintdiff-repeated-path-info
msyyc 6d13742
test(arm): focus lint coverage on rule-owned behavior
msyyc 611d044
Merge branch 'main' into promote-lintdiff-repeated-path-info
msyyc e54c9cb
test(arm): validate published repeated-path examples
msyyc cb6432b
refactor(arm): reuse repeated-path guards and semantic fixtures
msyyc a4f64f9
test(arm): remove framework-only repeated path cases
msyyc cc97acf
Merge remote-tracking branch 'origin/main' into promote-lintdiff-repe…
Copilot af9d756
test: isolate URI duplication diagnostics and package interactions
msyyc 53cc5f8
Merge branch 'main' into promote-lintdiff-repeated-path-info
msyyc a2818c3
Merge branch 'main' into promote-lintdiff-repeated-path-info
msyyc eeb9daa
test(arm): consolidate URI boundaries and package isolation
msyyc ffff37e
Merge branch 'main' into promote-lintdiff-repeated-path-info
msyyc b6ebb8d
test(arm): simplify repeated URI identity examples and coverage
msyyc 6ddd909
Check repeated path information on concrete inherited operations
msyyc 0f3139e
docs: simplify repeated path information rule guidance
msyyc 7592f2a
fix(arm): match repeated URI information by JSON name
msyyc File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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,9 @@ | ||
| --- | ||
| changeKind: feature | ||
| packages: | ||
| - "@azure-tools/typespec-azure-resource-manager" | ||
| --- | ||
|
|
||
| Add the `no-repeated-path-info` lint rule to warn when ARM PUT resource properties repeat path or query parameter names, honoring JSON-encoded property names and including concrete operations inherited from generic interfaces. | ||
|
|
||
| This complements `arm-resource-duplicate-property`, which checks duplication of resource envelope property names rather than URI parameter names. |
This file contains hidden or 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: internal | ||
| packages: | ||
| - "@azure-tools/typespec-azure-rulesets" | ||
| --- | ||
|
|
||
| Register the ARM `no-repeated-path-info` lint rule as disabled in the resource manager ruleset. |
This file contains hidden or 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 hidden or 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
29 changes: 29 additions & 0 deletions
29
packages/typespec-azure-resource-manager/src/rules/model-utils.ts
This file contains hidden or 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,29 @@ | ||
| import { | ||
| isNullType, | ||
| resolveEncodedName, | ||
| walkPropertiesInherited, | ||
| type Model, | ||
| type ModelProperty, | ||
| type Program, | ||
| type Type, | ||
| type Union, | ||
| } from "@typespec/compiler"; | ||
|
|
||
| export function getSingleNonNullMember(union: Union): Type | undefined { | ||
| let member: Type | undefined; | ||
| for (const variant of union.variants.values()) { | ||
| if (isNullType(variant.type)) continue; | ||
| if (member !== undefined) return undefined; | ||
| member = variant.type; | ||
| } | ||
| return member; | ||
| } | ||
|
|
||
| export function* walkJsonProperties( | ||
| program: Program, | ||
| model: Model, | ||
| ): Iterable<[string, ModelProperty]> { | ||
| for (const property of walkPropertiesInherited(model)) { | ||
| yield [resolveEncodedName(program, property, "application/json"), property]; | ||
| } | ||
| } |
74 changes: 74 additions & 0 deletions
74
packages/typespec-azure-resource-manager/src/rules/no-repeated-path-info.md
This file contains hidden or 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,74 @@ | ||
| ARM PUT request bodies should not repeat information already supplied in path or | ||
| query parameters. Keep resource identity in the request URI and resource-specific | ||
| data in the resource's `properties` bag, so callers do not have to supply | ||
| potentially conflicting values twice. | ||
|
|
||
| This rule complements | ||
| [`arm-resource-duplicate-property`](./arm-resource-duplicate-property.md), which | ||
| compares properties-bag members with envelope property names on ARM resource | ||
| models, independently of operations. For example, an envelope `name` with | ||
| `@key("widgetName")` makes `properties.widgetName` a URI duplication only. | ||
| Conversely, `properties.identity` duplicates an envelope `identity` only under | ||
| the envelope rule when no URI parameter is named `identity`. Both rules can | ||
| report a member when its name matches both an envelope property and a URI parameter. | ||
|
|
||
| ## Impact | ||
|
|
||
| - **Area:** API, SDK | ||
|
|
||
| Repeating URI information in resource properties creates redundant SDK inputs | ||
| and ambiguity when the request URI and payload disagree. | ||
|
|
||
| ## ❌ Incorrect | ||
|
|
||
| The standard ARM create-or-replace operation carries `widgetName` in the URI. | ||
| With `ArmResourceCreateOrReplaceSync<Widget>`, repeating that key name in | ||
| `WidgetProperties` triggers this rule. These excerpts omit unchanged provider | ||
| and operation declarations. | ||
|
|
||
| ```tsp | ||
| model Widget is TrackedResource<WidgetProperties> { | ||
| @key("widgetName") | ||
| @segment("widgets") | ||
| @path | ||
| name: string; | ||
| } | ||
|
|
||
| model WidgetProperties { | ||
| widgetName?: string; | ||
| description?: string; | ||
| } | ||
| ``` | ||
|
|
||
| ## ✅ Correct | ||
|
|
||
| Keep the resource key and operation template unchanged; remove only the redundant | ||
| properties-bag member. | ||
|
|
||
| ```tsp | ||
| model WidgetProperties { | ||
| description?: string; | ||
| } | ||
| ``` | ||
|
|
||
| ## Suppression | ||
|
|
||
| Prefer removing the redundant properties-bag member. If a published API must | ||
| retain the duplication for compatibility and an ARM reviewer has approved the | ||
| exception, suppress the warning on that member with a specific justification: | ||
|
|
||
| ```tsp | ||
| model WidgetProperties { | ||
| #suppress "@azure-tools/typespec-azure-resource-manager/no-repeated-path-info" "Retained for compatibility with the approved published API." | ||
| widgetName?: string; | ||
| } | ||
| ``` | ||
|
|
||
| ## LintDiff Equivalent | ||
|
|
||
| This rule corresponds to | ||
| [RepeatedPathInfo](https://github.com/Azure/azure-openapi-validator/blob/main/docs/repeated-path-info.md) | ||
| and ARM guideline `RPC-Put-V1-05`. Like the Swagger validator, it compares JSON | ||
| property names, including JSON name overrides, with path and query parameter | ||
| names. It checks native TypeSpec operations rather than an emitted single-version | ||
| schema. |
82 changes: 82 additions & 0 deletions
82
packages/typespec-azure-resource-manager/src/rules/no-repeated-path-info.ts
This file contains hidden or 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,82 @@ | ||
| import type { Model } from "@typespec/compiler"; | ||
| import { | ||
| createRule, | ||
| fileRef, | ||
| getProperty, | ||
| isTemplateDeclarationOrInstance, | ||
| paramMessage, | ||
| } from "@typespec/compiler"; | ||
| import { getHttpOperation } from "@typespec/http"; | ||
| import { walkJsonProperties } from "./model-utils.js"; | ||
| import { isTemplatedInterfaceOperation } from "./utils.js"; | ||
|
|
||
| export const noRepeatedPathInfoRule = createRule({ | ||
| name: "no-repeated-path-info", | ||
| docs: fileRef.fromPackageRoot("src/rules/no-repeated-path-info.md"), | ||
| description: | ||
| "ARM PUT request bodies must not repeat path or query parameters in the resource properties bag.", | ||
| severity: "warning", | ||
| url: "https://azure.github.io/typespec-azure/docs/libraries/azure-resource-manager/rules/no-repeated-path-info", | ||
| messages: { | ||
| default: paramMessage`Request body property '${"name"}' repeats information already carried in the path or query.`, | ||
| }, | ||
| create(context) { | ||
| return { | ||
| operation: (operation) => { | ||
| if ( | ||
| isTemplateDeclarationOrInstance(operation) || | ||
| isTemplatedInterfaceOperation(operation) | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| const [httpOperation] = getHttpOperation(context.program, operation); | ||
| if (httpOperation.verb !== "put") { | ||
| return; | ||
| } | ||
|
|
||
| const repeatedNames = new Set( | ||
| httpOperation.parameters.parameters | ||
| .filter((parameter) => parameter.type === "path" || parameter.type === "query") | ||
| .map((parameter) => parameter.name), | ||
| ); | ||
| if (repeatedNames.size === 0) { | ||
| return; | ||
| } | ||
|
|
||
| const body = httpOperation.parameters.body; | ||
| if (body === undefined || body.bodyKind !== "single" || body.type.kind !== "Model") { | ||
| return; | ||
| } | ||
|
|
||
| const propertiesModel = getPropertiesModel(body.type); | ||
| if (propertiesModel === undefined) { | ||
| return; | ||
| } | ||
|
|
||
| const reportedNames = new Set<string>(); | ||
|
msyyc marked this conversation as resolved.
Resolved
|
||
| for (const [name, property] of walkJsonProperties(context.program, propertiesModel)) { | ||
| if (!repeatedNames.has(name)) { | ||
| continue; | ||
| } | ||
| if (reportedNames.has(name)) { | ||
| continue; | ||
| } | ||
| reportedNames.add(name); | ||
|
|
||
| context.reportDiagnostic({ | ||
| target: property, | ||
| format: { | ||
| name, | ||
| }, | ||
| }); | ||
| } | ||
| }, | ||
| }; | ||
| }, | ||
| }); | ||
|
|
||
| function getPropertiesModel(model: Model): Model | undefined { | ||
| const property = getProperty(model, "properties"); | ||
| return property?.type.kind === "Model" ? property.type : undefined; | ||
| } | ||
13 changes: 13 additions & 0 deletions
13
packages/typespec-azure-resource-manager/test/package-isolation.test.ts
This file contains hidden or 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,13 @@ | ||
| import { expect, it, vi } from "vitest"; | ||
| import * as library from "../src/index.js"; | ||
|
|
||
| vi.mock("@azure-tools/typespec-autorest", () => { | ||
| throw new Error("Native lint must not load the AutoRest emitter."); | ||
| }); | ||
| vi.mock("@azure-tools/typespec-client-generator-core", () => { | ||
| throw new Error("Native lint must not load TCGC."); | ||
| }); | ||
|
|
||
| it("loads the ARM public entry point without emitter or TCGC dependencies", () => { | ||
| expect(library.$lib.name).toBe("@azure-tools/typespec-azure-resource-manager"); | ||
| }); |
27 changes: 27 additions & 0 deletions
27
packages/typespec-azure-resource-manager/test/rules/model-utils.test.ts
This file contains hidden or 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,27 @@ | ||
| import { t } from "@typespec/compiler/testing"; | ||
| import { expect, it } from "vitest"; | ||
| import { getSingleNonNullMember, walkJsonProperties } from "../../src/rules/model-utils.js"; | ||
| import { CompilerTester } from "../tester.js"; | ||
|
|
||
| it("preserves inheritance overrides and encoded JSON names without payload filtering", async () => { | ||
| const { | ||
| program, | ||
| Derived: model, | ||
| Nullable: nullable, | ||
| Ambiguous: ambiguous, | ||
| OnlyNull: onlyNull, | ||
| } = await CompilerTester.compile(t.code` | ||
| model Base { @encodedName("application/json", "baseName") name: string; inherited: string; } | ||
| model ${t.model("Derived")} extends Base { @encodedName("application/json", "derivedName") name: string; } | ||
| union ${t.union("Nullable")} { Derived, null } | ||
| union ${t.union("Ambiguous")} { Derived, Base, null } | ||
| union ${t.union("OnlyNull")} { null } | ||
| `); | ||
| expect([...walkJsonProperties(program, model)].map(([name]) => name)).toEqual([ | ||
| "derivedName", | ||
| "inherited", | ||
| ]); | ||
| expect(getSingleNonNullMember(nullable)).toBe(model); | ||
| expect(getSingleNonNullMember(ambiguous)).toBeUndefined(); | ||
| expect(getSingleNonNullMember(onlyNull)).toBeUndefined(); | ||
| }); |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.