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
16
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.
+552
−0
Open
Changes from 11 commits
Commits
Show all changes
16 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 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. | ||
|
|
||
| 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| changeKind: internal | ||
|
msyyc marked this conversation as resolved.
Outdated
|
||
| packages: | ||
| - "@azure-tools/typespec-azure-resource-manager" | ||
| --- | ||
|
|
||
| Reuse the ARM template-operation guard and compiler property lookup in the repeated-path rule. | ||
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
8 changes: 8 additions & 0 deletions
8
...es/typespec-azure-resource-manager/src/rules/arm-resource-duplicate-property.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
112 changes: 112 additions & 0 deletions
112
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,112 @@ | ||
| 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 compares authored member names in the model-valued `properties` bag | ||
| with the PUT operation's HTTP path and query parameter names. It includes | ||
| inherited members and reports on the repeated property declaration. Each matching | ||
| name is reported once per concrete PUT operation; a declaration shared by two PUT | ||
| operations can receive two diagnostics. | ||
|
|
||
| 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. | ||
|
|
||
| The check does not recurse into nested models, inspect top-level envelope | ||
| duplicates, or check PATCH bodies. Template sources are skipped, but concrete | ||
| operation aliases are checked. Provider namespace metadata is not required when | ||
| this rule is selected. The rule is available but disabled by default in the | ||
| Azure resource-manager ruleset. | ||
|
|
||
| `@encodedName("application/json", ...)` does not change an authored property | ||
| name for this check. A JSON alias alone cannot introduce or remove a diagnostic; | ||
| HTTP `@path` and `@query` name overrides do determine the parameter names. | ||
|
|
||
| ## 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. | ||
| Repeating it in `WidgetProperties` triggers this rule. | ||
|
|
||
| ```tsp | ||
| @armProviderNamespace | ||
| namespace Microsoft.Contoso; | ||
|
|
||
| model Widget is TrackedResource<WidgetProperties> { | ||
| @key("widgetName") | ||
| @segment("widgets") | ||
| @path | ||
| name: string; | ||
| } | ||
|
|
||
| model WidgetProperties { | ||
| widgetName?: string; | ||
| description?: string; | ||
| } | ||
|
|
||
| @armResourceOperations | ||
| interface Widgets { | ||
| createOrUpdate is ArmResourceCreateOrReplaceSync<Widget>; | ||
| } | ||
| ``` | ||
|
|
||
| ## ✅ Correct | ||
|
|
||
| Keep the same operation template and remove only the redundant properties-bag | ||
| member. | ||
|
|
||
| ```tsp | ||
| @armProviderNamespace | ||
| namespace Microsoft.Contoso; | ||
|
|
||
| model Widget is TrackedResource<WidgetProperties> { | ||
| @key("widgetName") | ||
| @segment("widgets") | ||
| @path | ||
| name: string; | ||
| } | ||
|
|
||
| model WidgetProperties { | ||
| description?: string; | ||
| } | ||
|
|
||
| @armResourceOperations | ||
| interface Widgets { | ||
| createOrUpdate is ArmResourceCreateOrReplaceSync<Widget>; | ||
| } | ||
| ``` | ||
|
|
||
| ## 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`. Unlike the Swagger validator, which compares | ||
| serialized JSON keys, this rule deliberately compares authored TypeSpec member | ||
| names. JSON name overrides therefore have only partial Swagger parity. | ||
| The rule checks the compiler's native operation graph, not an emitted | ||
| single-version schema. |
79 changes: 79 additions & 0 deletions
79
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,79 @@ | ||
| import type { Model } from "@typespec/compiler"; | ||
| import { | ||
| createRule, | ||
| fileRef, | ||
| getProperty, | ||
| isTemplateInstance, | ||
| paramMessage, | ||
| walkPropertiesInherited, | ||
| } from "@typespec/compiler"; | ||
| import { getHttpOperation } from "@typespec/http"; | ||
| 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 (isTemplateInstance(operation) || isTemplatedInterfaceOperation(operation)) { | ||
|
msyyc marked this conversation as resolved.
|
||
| 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>(); | ||
| for (const property of walkPropertiesInherited(propertiesModel)) { | ||
| if (!repeatedNames.has(property.name)) { | ||
| continue; | ||
| } | ||
| if (reportedNames.has(property.name)) { | ||
| continue; | ||
| } | ||
| reportedNames.add(property.name); | ||
|
|
||
| context.reportDiagnostic({ | ||
| target: property, | ||
| format: { | ||
| name: property.name, | ||
| }, | ||
| }); | ||
| } | ||
| }, | ||
| }; | ||
| }, | ||
| }); | ||
|
|
||
| function getPropertiesModel(model: Model): Model | undefined { | ||
| const property = getProperty(model, "properties"); | ||
| return property?.type.kind === "Model" ? property.type : undefined; | ||
| } | ||
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't that basically already https://github.com/Azure/typespec-azure/blob/5985744dffac74796b8dfd6e813098b91b535bae/packages/typespec-azure-resource-manager/src/rules/arm-resource-duplicate-property.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They overlap, but neither replaces the other.
arm-resource-duplicate-propertycompares the properties bag with envelope property names on recognized ARM resource models, independently of operations.no-repeated-path-infocompares authored properties-bag names with a concrete PUT operation's HTTP path/query names.For example, with envelope
nameannotated@key("widgetName"),properties.widgetNameis caught only by the new rule. Conversely,properties.identityduplicates an envelopeidentityunder the existing rule, but not the new rule when there is no URI parameter namedidentity.Added mutual documentation links and regression cases in 4bf3599 that enable both rules on the same ARM source, covering their distinct findings and the overlap when names coincide. Both rule implementations and the new rule's default-disabled ARM ruleset registration are unchanged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would like Mark Cowlishaw (@markcowl) feedback on that, some of this feels way too similar to me to split into 2 rules. It seems more of an swagger issue trying to be translated in typespec without a real meaning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yuchao Yan (@msyyc) please do not relesolve this comment until mark as given input
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, I'll leave this discussion open for Mark's input. The checks currently cover different contracts: for example, a PUT query parameter
modeduplicated only inproperties.modeis not an envelope-property duplicate. Whether to expose that through a separate rule or broaden the existing rule still needs the public-policy decision; I haven't changed either rule's ID or enablement.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mark Cowlishaw (@markcowl), could you confirm whether URI-parameter duplication should remain a separate opt-in rule or be added to
arm-resource-duplicate-property? A PUT query parametermoderepeated only inproperties.modedistinguishes the current URI check from envelope-property duplication. The implementations and enablement are unchanged; this discussion remains open for your policy input.