Split arm-resource-operation lint rule into three focused rules#4808
Split arm-resource-operation lint rule into three focused rules#4808catalinaperalta with Copilot wants to merge 11 commits into
arm-resource-operation lint rule into three focused rules#4808Conversation
…issing-decorator and arm-resource-operation-missing-api-version Co-authored-by: catalinaperalta <9859037+catalinaperalta@users.noreply.github.com>
arm-resource-operation lint rule into three focused rules
commit: |
Co-authored-by: catalinaperalta <9859037+catalinaperalta@users.noreply.github.com>
…n-missing-decorator Co-authored-by: catalinaperalta <9859037+catalinaperalta@users.noreply.github.com>
⚡ Benchmark Results
Full details – comparing
|
| Metric | Baseline | Current | Change |
|---|---|---|---|
| total | 🔴 1.00s | 🔴 1.06s | +5.5% 🔴 |
| loader | 🟡 303.7ms | 🟡 327.9ms | +8.0% 🔴 |
| resolver | 🟢 39.8ms | 🟢 40.2ms | +0.9% |
| checker | 🟡 362.2ms | 🟡 380.0ms | +4.9% |
| validation | 🟢 84.5ms | 🟢 86.9ms | +2.9% |
| ↳ validation/@azure-tools/typespec-azure-core | 🟡 12.5ms | 🟡 13.2ms | +5.5% |
| ↳ validation/@typespec/http | 🟡 15.4ms | 🟡 16.1ms | +4.3% |
| ↳ validation/@typespec/rest | 🟢 1.6ms | 🟢 1.6ms | +0.4% |
| ↳ validation/@typespec/versioning | 🔴 50.9ms | 🔴 51.9ms | +2.0% |
| ↳ validation/compiler | 🟢 3.8ms | 🟢 4.0ms | +3.5% |
| linter | 🟡 213.7ms | 🟡 224.6ms | +5.1% 🔴 |
| ↳ linter/@azure-tools/typespec-azure-core/auth-required | 🟢 0.1ms | 🟢 0.1ms | +7.3% |
| ↳ linter/@azure-tools/typespec-azure-core/bad-record-type | 🟢 0.6ms | 🟢 0.6ms | +1.3% |
| ↳ linter/@azure-tools/typespec-azure-core/byos | 🟢 8.3ms | 🟢 8.6ms | +4.1% |
| ↳ linter/@azure-tools/typespec-azure-core/casing-style | 🟢 1.4ms | 🟢 1.5ms | +2.6% |
| ↳ linter/@azure-tools/typespec-azure-core/composition-over-inheritance | 🟢 0.2ms | 🟢 0.2ms | +2.4% |
| ↳ linter/@azure-tools/typespec-azure-core/documentation-required | 🟢 1.9ms | 🟢 2.0ms | +2.1% |
| ↳ linter/@azure-tools/typespec-azure-core/friendly-name | 🟢 1.4ms | 🟢 1.4ms | +3.5% |
| ↳ linter/@azure-tools/typespec-azure-core/key-visibility-required | 🟢 0.4ms | 🟢 0.4ms | +3.5% |
| ↳ linter/@azure-tools/typespec-azure-core/known-encoding | 🟢 0.5ms | 🟢 0.5ms | +8.2% |
| ↳ linter/@azure-tools/typespec-azure-core/long-running-polling-operation-required | 🟢 0.8ms | 🟢 0.8ms | +1.5% |
| ↳ linter/@azure-tools/typespec-azure-core/no-case-mismatch | 🟢 0.7ms | 🟢 0.7ms | +1.1% |
| ↳ linter/@azure-tools/typespec-azure-core/no-closed-literal-union | 🟢 1.0ms | 🟢 1.0ms | -2.0% |
| ↳ linter/@azure-tools/typespec-azure-core/no-enum | 🟢 0.2ms | 🟢 0.3ms | +3.1% |
| ↳ linter/@azure-tools/typespec-azure-core/no-error-status-codes | 🟢 0.3ms | 🟢 0.3ms | +2.3% |
| ↳ linter/@azure-tools/typespec-azure-core/no-explicit-routes-resource-ops | 🟢 0.2ms | 🟢 0.2ms | +1.8% |
| ↳ linter/@azure-tools/typespec-azure-core/no-format | 🟢 0.8ms | 🟢 0.8ms | +2.4% |
| ↳ linter/@azure-tools/typespec-azure-core/no-generic-numeric | 🟢 0.8ms | 🟢 0.9ms | +3.1% |
| ↳ linter/@azure-tools/typespec-azure-core/no-header-explode | 🔴 26.5ms | 🔴 28.2ms | +6.5% 🔴 |
| ↳ linter/@azure-tools/typespec-azure-core/no-legacy-usage | 🟢 2.0ms | 🟢 2.2ms | +5.9% |
| ↳ linter/@azure-tools/typespec-azure-core/no-multiple-discriminator | 🟢 0.2ms | 🟢 0.2ms | +7.5% |
| ↳ linter/@azure-tools/typespec-azure-core/no-nullable | 🟢 0.4ms | 🟢 0.4ms | +3.4% |
| ↳ linter/@azure-tools/typespec-azure-core/no-offsetdatetime | 🟢 1.9ms | 🟢 2.1ms | +5.7% |
| ↳ linter/@azure-tools/typespec-azure-core/no-openapi | 🟢 2.3ms | 🟢 2.4ms | +2.5% |
| ↳ linter/@azure-tools/typespec-azure-core/no-private-usage | 🟢 3.1ms | 🟢 3.2ms | +2.9% |
| ↳ linter/@azure-tools/typespec-azure-core/no-query-explode | 🔴 27.7ms | 🔴 29.5ms | +6.8% 🔴 |
| ↳ linter/@azure-tools/typespec-azure-core/no-response-body | 🔴 33.0ms | 🔴 34.8ms | +5.3% 🔴 |
| ↳ linter/@azure-tools/typespec-azure-core/no-rest-library-interfaces | 🟢 0.1ms | 🟢 0.1ms | +3.0% |
| ↳ linter/@azure-tools/typespec-azure-core/no-route-parameter-name-mismatch | 🟢 7.6ms | 🟢 8.1ms | +6.3% |
| ↳ linter/@azure-tools/typespec-azure-core/no-rpc-path-params | 🟢 0.4ms | 🟢 0.4ms | +5.1% |
| ↳ linter/@azure-tools/typespec-azure-core/no-string-discriminator | 🟢 0.1ms | 🟢 0.2ms | +6.0% |
| ↳ linter/@azure-tools/typespec-azure-core/no-unknown | 🟢 0.3ms | 🟢 0.4ms | +9.0% |
| ↳ linter/@azure-tools/typespec-azure-core/no-unnamed-union | 🟢 0.8ms | 🟢 0.8ms | +1.0% |
| ↳ linter/@azure-tools/typespec-azure-core/operation-missing-api-version | 🟢 0.4ms | 🟢 0.4ms | -0.5% |
| ↳ linter/@azure-tools/typespec-azure-core/request-body-problem | 🟢 0.5ms | 🟢 0.5ms | +0.7% |
| ↳ linter/@azure-tools/typespec-azure-core/require-versioned | 🟢 0.1ms | 🟢 0.1ms | +3.0% |
| ↳ linter/@azure-tools/typespec-azure-core/response-schema-problem | 🔴 32.5ms | 🔴 34.8ms | +7.0% 🔴 |
| ↳ linter/@azure-tools/typespec-azure-core/rpc-operation-request-body | 🟢 0.7ms | 🟢 0.7ms | +6.6% |
| ↳ linter/@azure-tools/typespec-azure-core/spread-discriminated-model | 🟢 0.5ms | 🟢 0.5ms | +1.8% |
| ↳ linter/@azure-tools/typespec-azure-core/use-standard-names | 🟢 7.5ms | 🟢 8.0ms | +5.7% |
| ↳ linter/@azure-tools/typespec-azure-core/use-standard-operations | 🟢 0.3ms | 🟢 0.3ms | +1.7% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-agent-base-type-child-resources | 🔴 21.4ms | 🔴 21.9ms | +2.0% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-agent-base-type-lifecycle-operations | 🟢 0.3ms | 🟢 0.2ms | -3.3% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-common-types-version | 🟡 12.1ms | 🟡 12.5ms | +2.9% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-custom-resource-no-key | 🟢 0.2ms | 🟢 0.2ms | +5.6% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-custom-resource-usage-discourage | 🟢 0.1ms | 🟢 0.1ms | +2.1% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-delete-operation-response-codes | 🟢 4.9ms | 🟢 4.6ms | -6.1% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-no-path-casing-conflicts | 🟡 17.0ms | 🟡 17.0ms | -0.2% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-no-record | 🟢 0.6ms | 🟢 0.6ms | +4.1% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-post-operation-response-codes | 🟢 1.4ms | 🟢 1.4ms | +3.0% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-put-operation-response-codes | 🟢 0.2ms | 🟢 0.2ms | -4.6% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-action-no-segment | 🟢 0.4ms | 🟢 0.4ms | -1.5% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-duplicate-property | 🟢 0.4ms | 🟢 0.4ms | +2.3% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-interface-requires-decorator | 🟢 0.1ms | 🟢 0.1ms | -0.9% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-invalid-action-verb | 🟢 0.2ms | 🟢 0.1ms | -13.6% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-invalid-envelope-property | 🟢 0.3ms | 🟢 0.3ms | +3.8% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-invalid-version-format | 🟢 0.2ms | 🟢 0.2ms | +0.7% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-key-invalid-chars | 🟢 0.5ms | 🟢 0.5ms | +4.5% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-name-pattern | 🟢 0.1ms | 🟢 0.1ms | +7.9% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-operation | 🟢 0.5ms | 🟢 0.2ms | -63.9% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-operation-missing-api-version | 🟢 0.0ms | 🟢 0.2ms | +100.0% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-operation-missing-decorator | 🟢 0.0ms | 🟢 0.3ms | +100.0% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-operation-response | 🟢 8.9ms | 🟢 9.2ms | +3.3% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-patch | 🟢 0.8ms | 🟢 0.9ms | +3.4% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-path-segment-invalid-chars | 🟢 0.4ms | 🟢 0.4ms | +2.5% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-provisioning-state | 🟢 0.4ms | 🟢 0.4ms | +0.6% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/beyond-nesting-levels | 🟢 0.2ms | 🟢 0.2ms | +3.8% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/empty-updateable-properties | 🟢 0.4ms | 🟢 0.4ms | +6.0% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/improper-subscription-list-operation | 🟢 0.1ms | 🟢 0.1ms | +0.1% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/lro-location-header | 🟡 19.6ms | 🔴 21.0ms | +7.5% 🔴 |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/missing-operations-endpoint | 🟢 0.1ms | 🟢 0.2ms | +5.2% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/missing-x-ms-identifiers | 🟢 1.0ms | 🟢 1.0ms | +1.3% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/no-empty-model | 🟢 0.3ms | 🟢 0.3ms | +4.3% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/no-override-props | 🟢 0.3ms | 🟢 0.3ms | +1.1% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/no-resource-delete-operation | 🟢 0.5ms | 🟢 0.5ms | +2.9% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/no-response-body | 🔴 29.8ms | 🔴 32.3ms | +8.7% 🔴 |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/patch-envelope | 🟢 0.4ms | 🟢 0.4ms | +5.4% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/resource-name | 🟢 0.4ms | 🟢 0.4ms | -0.2% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/secret-prop | 🟢 5.4ms | 🟢 5.2ms | -3.5% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/unsupported-type | 🟢 0.7ms | 🟢 0.7ms | -0.7% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/version-progression | 🟢 0.3ms | 🟢 0.3ms | -1.9% |
| ↳ linter/@azure-tools/typespec-client-generator-core/property-name-conflict | 🟢 2.3ms | 🟢 2.4ms | +3.3% |
| ↳ linter/@azure-tools/typespec-client-generator-core/require-client-suffix | 🟢 1.7ms | 🟢 1.7ms | +2.5% |
| emit | 🔴 6.01s | 🔴 6.24s | +3.9% |
| ↳ emit/@Azure-Tools | 🟢 0.0ms | 🟢 0.0ms | +0.0% |
| ↳ emit/@azure-tools/typespec-autorest | 🟢 124.4ms | 🟢 128.9ms | +3.6% |
| ↳ emit/@azure-tools/typespec-python | 🔴 2.24s | 🔴 2.33s | +4.0% |
| ↳ emit/@typespec | 🟢 0.0ms | 🟢 0.0ms | +0.0% |
| ↳ emit/@typespec/http-client-js | 🔴 566.3ms | 🔴 588.2ms | +3.9% |
| ↳ emit/@typespec/openapi3 | 🟢 106.8ms | 🟢 112.0ms | +4.8% |
| ↳ emit/@typespec/openapi3/compute | 🟢 92.8ms | 🟢 97.3ms | +4.9% |
| ↳ emit/@typespec/openapi3/write | 🟢 13.8ms | 🟢 14.4ms | +4.5% |
Averaged across 3 specs (azure-arm-resource-manager, azure-core-dataplane, azure-full).
Threshold: changes > ±5% are highlighted.
🟢 Fast · 🟡 Moderate (stages >200ms, rules >10ms) · 🔴 Slow (stages >400ms, rules >20ms)
…ules Co-authored-by: catalinaperalta <9859037+catalinaperalta@users.noreply.github.com>
|
@copilot fix the format issue reported in this pr |
Co-authored-by: catalinaperalta <9859037+catalinaperalta@users.noreply.github.com>
Fixed in the latest commit. Ran prettier on the 5 files flagged by the Format CI job: |
| - "@azure-tools/typespec-azure-rulesets" | ||
| --- | ||
|
|
||
| Split `arm-resource-operation` lint rule: add `arm-resource-operation-missing-decorator` and `arm-resource-operation-missing-api-version` as separate rules. The original rule now only checks that operations are inside an interface declaration. |
There was a problem hiding this comment.
| Split `arm-resource-operation` lint rule: add `arm-resource-operation-missing-decorator` and `arm-resource-operation-missing-api-version` as separate rules. The original rule now only checks that operations are inside an interface declaration. | |
| Split `arm-resource-operation` lint rule: add `use-operation-decorator` and `use-api-version` as separate rules. The original rule now only checks that operations are inside an interface declaration. |
There was a problem hiding this comment.
@copilot rename the arm-resource-operation-missing-decorator to use-operation-decorator and the arm-resource-operation-missing-api-version to use-api-version and update references to these rule names
There was a problem hiding this comment.
Done. Renamed arm-resource-operation-missing-decorator → use-operation-decorator and arm-resource-operation-missing-api-version → use-api-version across all source files, tests, ruleset, README, website docs, and the chronus change file. Updated the changelog text per @markcowl's suggestion.
| }, | ||
| }); | ||
|
|
||
| const resourceOperationDecorators: { [verb in HttpVerb]: string[] } = { |
There was a problem hiding this comment.
Hmm, looks like this rule was out of date, we also need to include some newer decorators:
$extensionResourceOperation, $legacyResourceOperation, $builtInResourceOperation, $legacyExtensionResourceOperation
This might remove a lot of false positives. Ideally we would check that the operationType parameter matches the operation verb for these.
There was a problem hiding this comment.
I am kind of dubious about looking for decorators rather than using an accessor as a mechanism, but we can maybe add this fix later.
There was a problem hiding this comment.
Done. Added $extensionResourceOperation, $legacyResourceOperation, $builtInResourceOperation, and $legacyExtensionResourceOperation to the resourceOperationDecorators map (shared across all relevant HTTP verbs) so operations bearing any of these private ARM decorators won't be flagged. Added a test for each decorator in a new "Private ARM resource decorators" describe block.
| ...ResourceGroupParameter; | ||
| ...ProviderNamespace<TResource>; | ||
| ...KeysOf<TResource>; | ||
| ...CommonTypes.ApiVersionParameter; |
There was a problem hiding this comment.
ApiersionParameter always goes first and is in the Azure.ResourceManager namespace here: https://github.com/Azure/typespec-azure/blob/main/packages/typespec-azure-resource-manager/lib/parameters.tsp#L13
(so ...ApiVersionParameter should be the first line
There was a problem hiding this comment.
Fixed — ...ApiVersionParameter is now the first line in the custom parameters model example, and the spread uses the Azure.ResourceManager alias (...ApiVersionParameter) instead of ...CommonTypes.ApiVersionParameter.
|
|
||
| @armResourceAction(FooResource) | ||
| @action | ||
| @post |
There was a problem hiding this comment.
Always include the post path in the @action decorator
There was a problem hiding this comment.
Fixed — updated @action to @action("myAction") in the explicit-decorator correct example.
| return { | ||
| operation: (operation: Operation) => { | ||
| if ( | ||
| !isSourceOperationResourceManagerInternal(operation) && |
There was a problem hiding this comment.
I think we don't need this check anymore, this was added before linters would scope to the project sources
There was a problem hiding this comment.
Is the linter already scoping automatically or do I need to insert another check into the rule?
| name: "arm-resource-operation", | ||
| severity: "warning", | ||
| description: "Validate ARM Resource operations.", | ||
| description: "Validate ARM Resource operations are defined inside an interface declaration.", |
There was a problem hiding this comment.
should that rule also be renamed while we are at it? feels the name is not very clear on the purpose here
There was a problem hiding this comment.
Totally agree, maybe something like resource-operation-interface-required or resource-operation-requires-interface?
| @azure-tools/typespec-azure-resource-manager/arm-resource-operation-missing-decorator | ||
| ``` | ||
|
|
||
| Validate that ARM Resource operations use the correct decorator for the HTTP verb. |
There was a problem hiding this comment.
those rules in theory would only apply for custom operations right? should we add a :::note for that?
There was a problem hiding this comment.
Added a :::note block clarifying that this rule only applies to custom operations and that operations using standard ARM resource templates automatically satisfy it.
…/rules/arm-resource-operation-missing-api-version.md Co-authored-by: Mark Cowlishaw <1054056+markcowl@users.noreply.github.com>
…version Co-authored-by: catalinaperalta <9859037+catalinaperalta@users.noreply.github.com>
…tionDecoratorRule, useApiVersionRule) Co-authored-by: catalinaperalta <9859037+catalinaperalta@users.noreply.github.com>
…fix docs Co-authored-by: catalinaperalta <9859037+catalinaperalta@users.noreply.github.com>
The single
arm-resource-operationrule was covering three unrelated checks with mixed concerns, making it hard to suppress selectively or tune severity independently.Rule split
arm-resource-operation(narrowed)arm-resource-operation-missing-decorator(new)@armResourceRead,@armResourceCreateOrUpdate, etc.)arm-resource-operation-missing-api-version(new)Azure.ResourceManager.CommonTypes.ApiVersionParameter— more specific than thetypespec-azure-coreequivalent which accepts anyapiVersionstring parameterBoth new rules are enabled in the
resource-managerruleset. All existing test coverage is preserved and split into per-rule test files.Example: decorator check