Repository navigation
[Swagger Linter Migration] PatchInOperationName -> use-update-for-patch #5665
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
base: main
Are you sure you want to change the base?
Changes from all commits
3e7122b
5d131d4
2af91d5
5a9015b
fee3ceb
90e1e7d
ed35661
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| changeKind: internal | ||
| packages: | ||
| - "@azure-tools/typespec-azure-rulesets" | ||
| --- | ||
|
|
||
| Register the TCGC `use-update-for-patch` lint rule as disabled in the client SDK ruleset. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| changeKind: internal | ||
| packages: | ||
| - "@azure-tools/typespec-client-generator-core" | ||
| --- | ||
|
|
||
| Share package-private SDK verb-prefix traversal between PATCH and PUT naming rules. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| changeKind: feature | ||
| packages: | ||
| - "@azure-tools/typespec-client-generator-core" | ||
| --- | ||
|
|
||
| Add the opt-in `use-update-for-patch` lint rule to require common SDK method names for ARM PATCH operations to start with `update`, honoring common client name overrides. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| Use an SDK method name beginning with `update` for ARM PATCH operations, such as | ||
| `update`, `updateTags`, or `updateWidget`. This applies even when the operation uses | ||
| a standard ARM patch template, independently of its response status codes. | ||
|
|
||
| The rule checks the common TCGC SDK method name case-insensitively on concrete HTTP | ||
| endpoints. When enabled, it does not require `@armProviderNamespace`; select it for | ||
| ARM APIs, not data-plane APIs with different naming guidance. It is disabled by | ||
| default in the `client-sdk` ruleset. | ||
|
|
||
| Name resolution uses TCGC's `getLibraryName`, including an unscoped `@clientName` | ||
| override, `exact` client names, and the usual `@friendlyName` fallback. | ||
| Emitter-scoped overrides and OpenAPI `@operationId` values do not define the common | ||
| SDK method name and are ignored. | ||
|
|
||
| The source program is checked, including operations removed in later versions. | ||
| Historical names supplied through `@renamedFrom` are not reconstructed. | ||
|
|
||
| #### ❌ Incorrect | ||
|
|
||
| ```typespec | ||
| @armResourceOperations | ||
| interface Widgets { | ||
| modify is ArmResourcePatchSync<Widget, Properties = WidgetProperties>; | ||
| } | ||
| ``` | ||
|
|
||
| #### ✅ Correct | ||
|
|
||
| ```typespec | ||
| @armResourceOperations | ||
| interface Widgets { | ||
| update is ArmResourcePatchSync<Widget, Properties = WidgetProperties>; | ||
| } | ||
| ``` | ||
|
|
||
| These examples assume an ARM service, a tracked resource named `Widget`, and its | ||
| property model `WidgetProperties`. | ||
|
|
||
| ## Impact | ||
|
|
||
| - **Area:** SDK | ||
|
|
||
| Consistent update-method naming makes partial updates easier to discover in | ||
| generated clients. Renaming a method after an SDK has shipped may introduce a | ||
| breaking change to its public API. Review compatibility before changing an | ||
| existing SDK method name. | ||
|
|
||
| An unscoped `@clientName` can provide the correct SDK name without renaming the | ||
| TypeSpec operation: | ||
|
|
||
| ```typespec | ||
| @armResourceOperations | ||
| interface Widgets { | ||
| @Azure.ClientGenerator.Core.clientName("updateWidget") | ||
| modify is ArmResourcePatchSync<Widget, Properties = WidgetProperties>; | ||
| } | ||
| ``` | ||
|
|
||
| Import `@azure-tools/typespec-client-generator-core` to use `@clientName`. | ||
|
|
||
| ## Suppression | ||
|
|
||
| Suppression is acceptable when preserving an already-shipped SDK method name | ||
| requires an exception approved by the API reviewer. For new operations, fix the | ||
| SDK method name instead, either by renaming the operation or using `@clientName`. | ||
|
|
||
| ```typespec | ||
| @armResourceOperations | ||
| interface Widgets { | ||
| #suppress "@azure-tools/typespec-client-generator-core/use-update-for-patch" "Preserve the already-shipped SDK method name." | ||
| modify is ArmResourcePatchSync<Widget, Properties = WidgetProperties>; | ||
| } | ||
| ``` | ||
|
|
||
| ## LintDiff Equivalent | ||
|
|
||
| This rule enforces the ARM SDK method-naming intent of | ||
| [`PatchInOperationName`](https://github.com/Azure/azure-openapi-validator/blob/aec54e95e52338d0afa911a92958541d5624c105/docs/patch-in-operation-name.md). | ||
| It follows TCGC's `use-create-for-put` common SDK naming pattern rather than | ||
| reproducing Swagger `operationId` group-name exemptions or checking only authored | ||
| TypeSpec names. Common SDK name overrides intentionally affect this rule; no | ||
| emitter or version projection is used. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| import { createRule, fileRef, paramMessage } from "@typespec/compiler"; | ||
| import { createTCGCContext } from "../context.js"; | ||
| import { getOperationsWithIncorrectVerbPrefix } from "./verb-prefix.js"; | ||
|
|
||
| export const useUpdateForPatchRule = createRule({ | ||
| name: "use-update-for-patch", | ||
| description: "ARM PATCH SDK method names should use 'update' as the verb prefix.", | ||
| severity: "warning", | ||
| url: "https://azure.github.io/typespec-azure/docs/libraries/typespec-client-generator-core/rules/use-update-for-patch", | ||
| docs: fileRef.fromPackageRoot("src/rules/use-update-for-patch.md"), | ||
| messages: { | ||
| default: paramMessage`PATCH SDK method name '${"operationName"}' should start with 'update'. Note: If you have already shipped an SDK on top of this spec, fixing this warning may introduce a breaking change.`, | ||
| }, | ||
| create(context) { | ||
| const tcgcContext = createTCGCContext( | ||
| context.program, | ||
| "@azure-tools/typespec-client-generator-core", | ||
| { mutateNamespace: false }, | ||
| ); | ||
|
|
||
| return { | ||
| root: () => { | ||
| for (const { operation, operationName } of getOperationsWithIncorrectVerbPrefix( | ||
| tcgcContext, | ||
| "patch", | ||
| "update", | ||
| )) { | ||
| context.reportDiagnostic({ | ||
| target: operation, | ||
| format: { operationName }, | ||
| }); | ||
| } | ||
| }, | ||
| }; | ||
| }, | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| import type { Operation } from "@typespec/compiler"; | ||
| import { getAllHttpServices, type HttpVerb } from "@typespec/http"; | ||
| import type { TCGCContext } from "../interfaces.js"; | ||
| import { AllScopes } from "../internal-utils.js"; | ||
| import { getLibraryName } from "../public-utils.js"; | ||
|
|
||
| export function* getOperationsWithIncorrectVerbPrefix( | ||
| context: TCGCContext, | ||
| verb: HttpVerb, | ||
| prefix: string, | ||
| ): Iterable<{ operation: Operation; operationName: string }> { | ||
| const [services] = getAllHttpServices(context.program); | ||
| for (const service of services) { | ||
| for (const endpoint of service.operations) { | ||
| if (endpoint.verb !== verb) continue; | ||
| const operationName = getLibraryName(context, endpoint.operation, AllScopes); | ||
| if (!operationName.toLowerCase().startsWith(prefix)) { | ||
| yield { operation: endpoint.operation, operationName }; | ||
| } | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| import { createLinterRuleTester } from "@typespec/compiler/testing"; | ||
| import { describe, it, vi } from "vitest"; | ||
| import { useCreateForPutRule } from "../../src/rules/use-create-for-put.js"; | ||
| import { useUpdateForPatchRule } from "../../src/rules/use-update-for-patch.js"; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remove these extra per-rule behavior checks from this new isolation suite. PUT and PATCH already have their own canonical Copilot-generated comment. |
||
| import { ArmTester } from "../tester.js"; | ||
|
|
||
| vi.mock("@azure-tools/typespec-autorest", () => { | ||
| throw new Error("Native lint must not load the AutoRest emitter."); | ||
| }); | ||
|
|
||
| describe("native SDK naming linter package integration", () => { | ||
| for (const [rule, verb, prefix] of [ | ||
| [useCreateForPutRule, "put", "create"], | ||
| [useUpdateForPatchRule, "patch", "update"], | ||
| ] as const) { | ||
| it(`runs ${rule.name} without loading the AutoRest emitter`, async () => { | ||
| const tester = createLinterRuleTester( | ||
| await ArmTester.createInstance(), | ||
| rule, | ||
| "@azure-tools/typespec-client-generator-core", | ||
| ); | ||
| await tester | ||
| .expect( | ||
| ` | ||
| @service namespace Service; | ||
| @${verb} @route("/invalid") op modify(): string; | ||
| @clientName("${prefix}Widget") | ||
| @${verb} @route("/valid") op change(): string; | ||
| `, | ||
| ) | ||
| .toEmitDiagnostics([ | ||
| { | ||
| code: `@azure-tools/typespec-client-generator-core/${rule.name}`, | ||
| severity: "warning", | ||
| target: "modify", | ||
| message: `${verb.toUpperCase()} SDK method name 'modify' should start with '${prefix}'. Note: If you have already shipped an SDK on top of this spec, fixing this warning may introduce a breaking change.`, | ||
| }, | ||
| ]); | ||
| }); | ||
| } | ||
| }); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remove the redundant top-level Copilot-generated comment. |
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.
Please remove this redundant internal changeset. This PR already has a non-internal entry for
@azure-tools/typespec-client-generator-core:.chronus/changes/tcgc-use-update-for-patch.md(feature).Review-driven cleanup, test changes, helper reuse, formatting, and syncing with main do not need extra internal entries just to satisfy Chronus. Keep the final shipped behavior in the existing feature/fix entry instead of accumulating unpublished review-history noise. Preserve separate registration entries for packages that have no non-internal entry.
Same feedback as PR 5502.
Copilot-generated comment.