Skip to content
Open
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .chronus/changes/arm-no-repeated-path-info.md

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member Author

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-property compares the properties bag with envelope property names on recognized ARM resource models, independently of operations. no-repeated-path-info compares authored properties-bag names with a concrete PUT operation's HTTP path/query names.

For example, with envelope name annotated @key("widgetName"), properties.widgetName is caught only by the new rule. Conversely, properties.identity duplicates an envelope identity under the existing rule, but not the new rule when there is no URI parameter named identity.

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.

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member Author

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 mode duplicated only in properties.mode is 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.

Copy link
Copy Markdown
Member Author

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 parameter mode repeated only in properties.mode distinguishes the current URI check from envelope-property duplication. The implementations and enablement are unchanged; this discussion remains open for your policy input.

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.
7 changes: 7 additions & 0 deletions .chronus/changes/register-no-repeated-path-info-disabled.md
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.
7 changes: 7 additions & 0 deletions .chronus/changes/review-repeated-path-helpers.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
changeKind: internal
Comment thread
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.
1 change: 1 addition & 0 deletions packages/typespec-azure-resource-manager/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ Available ruleSets:
| [`@azure-tools/typespec-azure-resource-manager/missing-x-ms-identifiers`](https://azure.github.io/typespec-azure/docs/libraries/azure-resource-manager/rules/missing-x-ms-identifiers) | Array properties should describe their identifying properties with x-ms-identifiers. Decorate the property with @OpenAPI.extension("x-ms-identifiers", #[id-prop]) where "id-prop" is a list of the names of identifying properties in the item type. |
| [`@azure-tools/typespec-azure-resource-manager/no-response-body`](https://azure.github.io/typespec-azure/docs/libraries/azure-resource-manager/rules/no-response-body) | Check that the body is empty for 202 and 204 responses, and not empty for other success (2xx) responses. |
| [`@azure-tools/typespec-azure-resource-manager/no-query-in-post`](https://azure.github.io/typespec-azure/docs/libraries/azure-resource-manager/rules/no-query-in-post) | ARM POST operations must not declare query parameters other than api-version. |
| [`@azure-tools/typespec-azure-resource-manager/no-repeated-path-info`](https://azure.github.io/typespec-azure/docs/libraries/azure-resource-manager/rules/no-repeated-path-info) | ARM PUT request bodies must not repeat path or query parameters in the resource properties bag. |
| [`@azure-tools/typespec-azure-resource-manager/missing-operations-endpoint`](https://azure.github.io/typespec-azure/docs/libraries/azure-resource-manager/rules/missing-operations-endpoint) | Check for missing Operations interface. |
| [`@azure-tools/typespec-azure-resource-manager/patch-envelope`](https://azure.github.io/typespec-azure/docs/libraries/azure-resource-manager/rules/patch-envelope) | Patch envelope properties should match the resource properties. |
| [`@azure-tools/typespec-azure-resource-manager/arm-resource-patch`](https://azure.github.io/typespec-azure/docs/libraries/azure-resource-manager/rules/arm-resource-patch) | Validate ARM PATCH operations. |
Expand Down
2 changes: 2 additions & 0 deletions packages/typespec-azure-resource-manager/src/linter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import { noOverridePropsRule } from "./rules/no-override-props.js";
import { noQueryInCollectionRule } from "./rules/no-query-in-collection.js";
import { noQueryInPointOpRule } from "./rules/no-query-in-point-op.js";
import { noQueryInPostRule } from "./rules/no-query-in-post.js";
import { noRepeatedPathInfoRule } from "./rules/no-repeated-path-info.js";
import { noReservedResourcePropertyRule } from "./rules/no-reserved-resource-property.js";
import { deleteOperationMissingRule } from "./rules/no-resource-delete-operation.js";
import { noResponseBodyRule } from "./rules/no-response-body.js";
Expand Down Expand Up @@ -95,6 +96,7 @@ const rules = [
missingXmsIdentifiersRule,
noResponseBodyRule,
noQueryInPostRule,
noRepeatedPathInfoRule,
operationsInterfaceMissingRule,
patchEnvelopePropertiesRules,
patchOperationsRule,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
Envelope properties should not be duplicated in the `properties` model.

This rule compares properties-bag members with envelope property names on ARM
resource models, even without a PUT operation. It complements
[`no-repeated-path-info`](./no-repeated-path-info.md), which compares them with
HTTP path and query parameter names on PUT operations. For example, an envelope
`name` with `@key("widgetName")` makes `properties.widgetName` a URI duplication
only, while `properties.identity` duplicates an envelope `identity` even when
there is no URI parameter named `identity`. Neither rule replaces the other.

## Impact

- **Area:** API, SDK
Expand Down
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.
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)) {
Comment thread
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;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
import { getSourceLocation } from "@typespec/compiler";
import { expectDiagnostics } from "@typespec/compiler/testing";
Comment thread
msyyc marked this conversation as resolved.
Outdated
import { it } from "vitest";
import { Tester } from "../tester.js";

it.each(["widgetName", "identity"])(
"distinguishes envelope duplication from URI duplication with @key(%s)",
async (parameterName) => {
const [{ widgetName, identity }, diagnostics] = await Tester.compileAndDiagnose(
`
@armProviderNamespace
namespace Microsoft.Contoso;
model Widget is TrackedResource<WidgetProperties> {
@key("${parameterName}") @segment("widgets") @path name: string;
...ManagedServiceIdentityProperty;
}
model WidgetProperties {
/*widgetName*/widgetName?: string;
/*identity*/identity?: string;
}
@armResourceOperations
interface Widgets {
createOrUpdate is ArmResourceCreateOrReplaceSync<Widget>;
}
`,
{
compilerOptions: {
linterRuleSet: {
enable: {
"@azure-tools/typespec-azure-resource-manager/arm-resource-duplicate-property": true,
"@azure-tools/typespec-azure-resource-manager/no-repeated-path-info": true,
},
},
},
},
);
const envelopeLocation = getSourceLocation(identity);
const uriLocation = getSourceLocation(parameterName === "widgetName" ? widgetName : identity);
expectDiagnostics(diagnostics, [
{
code: "@azure-tools/typespec-azure-resource-manager/arm-resource-duplicate-property",
severity: "warning",
message:
'Duplicate property "identity" found in the resource envelope and resource properties. Please do not duplicate envelope properties in resource properties.',
pos: envelopeLocation.pos,
end: envelopeLocation.end,
},
{
code: "@azure-tools/typespec-azure-resource-manager/no-repeated-path-info",
severity: "warning",
message: `Request body property '${parameterName}' repeats information already carried in the path or query.`,
pos: uriLocation.pos,
end: uriLocation.end,
},
]);
},
);
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import { createLinterRuleTester } from "@typespec/compiler/testing";
import { it, vi } from "vitest";
import { noRepeatedPathInfoRule } from "../../src/rules/no-repeated-path-info.js";
Comment thread
msyyc marked this conversation as resolved.
Outdated
import { Tester } from "../tester.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("checks URI duplication without loading emitter or TCGC packages", async () => {
const tester = createLinterRuleTester(
await Tester.createInstance(),
noRepeatedPathInfoRule,
"@azure-tools/typespec-azure-resource-manager",
);
await tester
.expect(
`
@put @route("/widgets/{widgetName}")
op create(@path widgetName: string, @body body: {
properties: { widgetName?: string; }
}): void;
`,
)
.toEmitDiagnostics({
code: "@azure-tools/typespec-azure-resource-manager/no-repeated-path-info",
message:
"Request body property 'widgetName' repeats information already carried in the path or query.",
});
});
Loading
Loading