From fa121d3f4949c7b819064da5de7a2854c80c7323 Mon Sep 17 00:00:00 2001 From: Alitzel Mendez Bustillo Date: Tue, 22 Oct 2024 13:20:35 -0700 Subject: [PATCH 01/17] no-response-body rule changed for arm --- .../reference/linter.md | 2 +- .../rules/no-response-body.md | 98 +++++++++++++++++++ .../typespec-azure-resource-manager/README.md | 2 +- .../src/rules/no-response-body.ts | 43 ++++++-- .../test/rules/no-response-body.test.ts | 46 +++++++++ .../src/rulesets/resource-manager.ts | 4 +- 6 files changed, 182 insertions(+), 13 deletions(-) create mode 100644 docs/libraries/azure-resource-manager/rules/no-response-body.md diff --git a/docs/libraries/azure-resource-manager/reference/linter.md b/docs/libraries/azure-resource-manager/reference/linter.md index 7d64cf2dcb..b16f9f63e8 100644 --- a/docs/libraries/azure-resource-manager/reference/linter.md +++ b/docs/libraries/azure-resource-manager/reference/linter.md @@ -47,7 +47,7 @@ Available ruleSets: | `@azure-tools/typespec-azure-resource-manager/improper-subscription-list-operation` | Tenant and Extension resources should not define a list by subscription operation. | | [`@azure-tools/typespec-azure-resource-manager/lro-location-header`](/libraries/azure-resource-manager/rules/lro-location-header.md) | A 202 response should include a Location response header. | | [`@azure-tools/typespec-azure-resource-manager/missing-x-ms-identifiers`](/libraries/azure-resource-manager/rules/missing-x-ms-identifiers.md) | 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` | The body of 202 response should be empty. | +| [`@azure-tools/typespec-azure-resource-manager/no-response-body`](/libraries/azure-resource-manager/rules/no-response-body.md) | Check that the body is empty for 202 and 204 responses, and not empty for non-204/non-202 responses. | | `@azure-tools/typespec-azure-resource-manager/missing-operations-endpoint` | Check for missing Operations interface. | | `@azure-tools/typespec-azure-resource-manager/patch-envelope` | Patch envelope properties should match the resource properties. | | `@azure-tools/typespec-azure-resource-manager/arm-resource-patch` | Validate ARM PATCH operations. | diff --git a/docs/libraries/azure-resource-manager/rules/no-response-body.md b/docs/libraries/azure-resource-manager/rules/no-response-body.md new file mode 100644 index 0000000000..47bcf22a0d --- /dev/null +++ b/docs/libraries/azure-resource-manager/rules/no-response-body.md @@ -0,0 +1,98 @@ +--- +title: no-empty-model +--- + +```text title=- Full name- +@azure-tools/typespec-azure-resource-manager/no-response-body + +``` + +ARM response operations with status code 202 or 204 should not contain response body. Operations that are different for 202 and 204 should contain a response body. + +### For 202 and 204 status codes (response body should be empty) + +#### ❌ Incorrect + +```tsp +model Test204BodyResponse { + @statusCode statusCode: 204; + @bodyRoot body: string; +} +op walk(): Test204BodyResponse; +``` + +```json +{ + "responses": { + "204": { + "description": "There is no content to send for this request, but the headers may be useful. ", + "schema": { + "type": "string" + } + } + } +} +``` + +#### ✅ Correct + +```tsp +model Test204EmptyResponse { + @statusCode statusCode: 202; +} +op walk(): Test204EmptyResponse; +``` + +```json +{ + "responses": { + "202": { + "description": "The request has been accepted for processing, but processing has not yet completed." + } + } +} +``` + +### For operations different than 202 and 204 status codes (response body should not be empty) + +#### ❌ Incorrect + +```tsp +model Test201EmtpyResponse { + @statusCode statusCode: 201; +} +op walk(): Test201EmtpyResponse; +``` + +```json +{ + "responses": { + "201": { + "description": "The request has succeeded and a new resource has been created as a result." + } + } +} +``` + +#### ✅ Correct + +```tsp +model Test201BodyResponse { + @statusCode statusCode: 201; + @bodyRoot body: string; +} +op walk(): Test201BodyResponse; +``` + +```json +{ + "responses": { + "201": { + "description": "The request has succeeded and a new resource has been created as a result.", + "schema": { + "type": "string" + } + } + } +} +``` diff --git a/packages/typespec-azure-resource-manager/README.md b/packages/typespec-azure-resource-manager/README.md index d4c5548e62..87611cef67 100644 --- a/packages/typespec-azure-resource-manager/README.md +++ b/packages/typespec-azure-resource-manager/README.md @@ -51,7 +51,7 @@ Available ruleSets: | `@azure-tools/typespec-azure-resource-manager/improper-subscription-list-operation` | Tenant and Extension resources should not define a list by subscription operation. | | [`@azure-tools/typespec-azure-resource-manager/lro-location-header`](https://azure.github.io/typespec-azure/docs/libraries/azure-resource-manager/rules/lro-location-header) | A 202 response should include a Location response header. | | [`@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` | The body of 202 response should be empty. | +| [`@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 non-204/non-202 responses. | | `@azure-tools/typespec-azure-resource-manager/missing-operations-endpoint` | Check for missing Operations interface. | | `@azure-tools/typespec-azure-resource-manager/patch-envelope` | Patch envelope properties should match the resource properties. | | `@azure-tools/typespec-azure-resource-manager/arm-resource-patch` | Validate ARM PATCH operations. | diff --git a/packages/typespec-azure-resource-manager/src/rules/no-response-body.ts b/packages/typespec-azure-resource-manager/src/rules/no-response-body.ts index 04cf9f33e4..169ed27e7a 100644 --- a/packages/typespec-azure-resource-manager/src/rules/no-response-body.ts +++ b/packages/typespec-azure-resource-manager/src/rules/no-response-body.ts @@ -1,33 +1,56 @@ -import { Operation, createRule } from "@typespec/compiler"; -import { getResponsesForOperation } from "@typespec/http"; +import { createRule, Operation } from "@typespec/compiler"; +import { getResponsesForOperation, HttpOperationResponse } from "@typespec/http"; import { isTemplatedInterfaceOperation } from "./utils.js"; /** - * verify an operation returns 202 should not contains response body. + * verify an operation returns 202 or 204 should not contain response body. + * verify that operations that are different for 202 and 204 should contain a response body. */ export const noResponseBodyRule = createRule({ name: "no-response-body", + description: + "Check that the body is empty for 202 and 204 responses, and not empty for non-204/non-202 responses.", + url: "https://azure.github.io/typespec-azure/docs/libraries/azure-resource-manager/rules/no-response-body", severity: "warning", - description: `The body of 202 response should be empty.`, messages: { - default: `The body of 202 response should be empty.`, + default: `The body of non-204/non-202 responses should not be empty.`, + shouldBeEmpty202: `The body of 202 response should be empty.`, + shouldBeEmpty204: `The body of 204 response should be empty.`, }, create(context) { return { operation: (op: Operation) => { - if (isTemplatedInterfaceOperation(op)) { - return; - } + if (isTemplatedInterfaceOperation(op)) return; + const responses = getResponsesForOperation(context.program, op)[0].find( - (v) => v.statusCodes === 202, + (v) => v.statusCodes !== 204 && v.statusCodes !== 202, ); - if (responses && responses.responses.some((v) => v.body)) { + if (responses && !responses.responses.every((v) => v.body)) { + context.reportDiagnostic({ + target: op, + }); + } + + if (hasResponseBodyForCode(202, getResponsesForOperation(context.program, op)[0])) { context.reportDiagnostic({ target: op, + messageId: "shouldBeEmpty202", + }); + } + + if (hasResponseBodyForCode(204, getResponsesForOperation(context.program, op)[0])) { + context.reportDiagnostic({ + target: op, + messageId: "shouldBeEmpty204", }); } }, }; }, }); + +function hasResponseBodyForCode(statusCode: number, operations: HttpOperationResponse[]): boolean { + const response = operations.find((v) => v.statusCodes === statusCode); + return !!(response && response.responses.some((v) => v.body)); +} diff --git a/packages/typespec-azure-resource-manager/test/rules/no-response-body.test.ts b/packages/typespec-azure-resource-manager/test/rules/no-response-body.test.ts index 82c03c34f3..33a1262aaf 100644 --- a/packages/typespec-azure-resource-manager/test/rules/no-response-body.test.ts +++ b/packages/typespec-azure-resource-manager/test/rules/no-response-body.test.ts @@ -33,6 +33,36 @@ describe("typespec-azure-resource-manager: no response body rule", () => { .toBeValid(); }); + it("is valid if 204 responses have no body", async () => { + await tester + .expect( + ` + model TestAcceptedResponse { + @statusCode statusCode: 204; + } + op walk(): TestAcceptedResponse; + `, + ) + .toBeValid(); + }); + + it("emit warnings if a 204 response has a body", async () => { + await tester + .expect( + ` + model TestAcceptedResponse { + @statusCode statusCode: 204; + @bodyRoot body: string; + } + op walk(): TestAcceptedResponse; + `, + ) + .toEmitDiagnostics({ + code: "@azure-tools/typespec-azure-resource-manager/no-response-body", + message: `The body of 204 response should be empty.`, + }); + }); + it("emit warnings if a 202 response has a body", async () => { await tester .expect( @@ -49,4 +79,20 @@ describe("typespec-azure-resource-manager: no response body rule", () => { message: `The body of 202 response should be empty.`, }); }); + + it("emit warnings if 201 responses have no body", async () => { + await tester + .expect( + ` + model TestAcceptedResponse { + @statusCode statusCode: 201; + } + op walk(): TestAcceptedResponse; + `, + ) + .toEmitDiagnostics({ + code: "@azure-tools/typespec-azure-resource-manager/no-response-body", + message: `The body of non-204/non-202 responses should not be empty.`, + }); + }); }); diff --git a/packages/typespec-azure-rulesets/src/rulesets/resource-manager.ts b/packages/typespec-azure-rulesets/src/rulesets/resource-manager.ts index e7494f7c60..a1e4f0e5e1 100644 --- a/packages/typespec-azure-rulesets/src/rulesets/resource-manager.ts +++ b/packages/typespec-azure-rulesets/src/rulesets/resource-manager.ts @@ -20,7 +20,6 @@ export default { "@azure-tools/typespec-azure-core/no-generic-numeric": true, "@azure-tools/typespec-azure-core/no-nullable": true, "@azure-tools/typespec-azure-core/no-offsetdatetime": true, - "@azure-tools/typespec-azure-core/no-response-body": true, "@azure-tools/typespec-azure-core/no-rpc-path-params": true, "@azure-tools/typespec-azure-core/no-openapi": true, "@azure-tools/typespec-azure-core/prefer-csv-collection-format": true, @@ -41,6 +40,9 @@ export default { "@azure-tools/typespec-azure-core/friendly-name": true, "@azure-tools/typespec-azure-core/no-query-explode": true, + // Azure core not enable - Arm has its own conflicting rule + "@azure-tools/typespec-azure-core/no-response-body": false, + // Azure core not enabled - Arm has its own conflicting rule "@azure-tools/typespec-azure-core/bad-record-type": false, "@azure-tools/typespec-azure-core/use-standard-operations": false, From 7d6bcb240771d196ee2f129c8fd8f1f9ab5b21f2 Mon Sep 17 00:00:00 2001 From: Alitzel Mendez Bustillo Date: Tue, 22 Oct 2024 13:25:22 -0700 Subject: [PATCH 02/17] Add summary of changes --- .../changes/no-response-body-rule-2024-9-22-13-23-5.md | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 .chronus/changes/no-response-body-rule-2024-9-22-13-23-5.md diff --git a/.chronus/changes/no-response-body-rule-2024-9-22-13-23-5.md b/.chronus/changes/no-response-body-rule-2024-9-22-13-23-5.md new file mode 100644 index 0000000000..aa9f1bcd1d --- /dev/null +++ b/.chronus/changes/no-response-body-rule-2024-9-22-13-23-5.md @@ -0,0 +1,8 @@ +--- +changeKind: feature +packages: + - "@azure-tools/typespec-azure-resource-manager" + - "@azure-tools/typespec-azure-rulesets" +--- + +Update of arm no-response-body to have a similar behavior of the core rule, but with the difference that it will exclude also 202 response, meaning that 202 should also be empty. From acec6943c8b16969b208b871d16d4a0cd050eb9c Mon Sep 17 00:00:00 2001 From: Alitzel Mendez Bustillo Date: Thu, 24 Oct 2024 09:44:20 -0700 Subject: [PATCH 03/17] make summary changes more clear --- .chronus/changes/no-response-body-rule-2024-9-22-13-23-5.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.chronus/changes/no-response-body-rule-2024-9-22-13-23-5.md b/.chronus/changes/no-response-body-rule-2024-9-22-13-23-5.md index aa9f1bcd1d..9887b2e37a 100644 --- a/.chronus/changes/no-response-body-rule-2024-9-22-13-23-5.md +++ b/.chronus/changes/no-response-body-rule-2024-9-22-13-23-5.md @@ -5,4 +5,4 @@ packages: - "@azure-tools/typespec-azure-rulesets" --- -Update of arm no-response-body to have a similar behavior of the core rule, but with the difference that it will exclude also 202 response, meaning that 202 should also be empty. +Update the `arm no-response-body` rule to behave similarly to the core rule, but with the additional requirement that the 202 response can and should also be empty From b6d4e2ed3bd66418e06398ea1962001086d590ea Mon Sep 17 00:00:00 2001 From: Alitzel Mendez Bustillo Date: Thu, 24 Oct 2024 09:58:18 -0700 Subject: [PATCH 04/17] fix typo --- .../azure-resource-manager/rules/no-response-body.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/libraries/azure-resource-manager/rules/no-response-body.md b/docs/libraries/azure-resource-manager/rules/no-response-body.md index 47bcf22a0d..7e9f4317cb 100644 --- a/docs/libraries/azure-resource-manager/rules/no-response-body.md +++ b/docs/libraries/azure-resource-manager/rules/no-response-body.md @@ -58,10 +58,10 @@ op walk(): Test204EmptyResponse; #### ❌ Incorrect ```tsp -model Test201EmtpyResponse { +model Test201EmptyResponse { @statusCode statusCode: 201; } -op walk(): Test201EmtpyResponse; +op walk(): Test201EmptyResponse; ``` ```json From ebdb33c346851dee218d4166bf2b50287e4db753 Mon Sep 17 00:00:00 2001 From: Alitzel Mendez Bustillo Date: Thu, 24 Oct 2024 13:19:56 -0700 Subject: [PATCH 05/17] include only azure subnamespace rule --- .../src/rules/no-response-body.ts | 3 ++- packages/typespec-azure-resource-manager/src/rules/utils.ts | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/typespec-azure-resource-manager/src/rules/no-response-body.ts b/packages/typespec-azure-resource-manager/src/rules/no-response-body.ts index 169ed27e7a..70c00ac73d 100644 --- a/packages/typespec-azure-resource-manager/src/rules/no-response-body.ts +++ b/packages/typespec-azure-resource-manager/src/rules/no-response-body.ts @@ -1,7 +1,7 @@ import { createRule, Operation } from "@typespec/compiler"; import { getResponsesForOperation, HttpOperationResponse } from "@typespec/http"; -import { isTemplatedInterfaceOperation } from "./utils.js"; +import { isAzureSubNamespace, isTemplatedInterfaceOperation } from "./utils.js"; /** * verify an operation returns 202 or 204 should not contain response body. @@ -22,6 +22,7 @@ export const noResponseBodyRule = createRule({ return { operation: (op: Operation) => { if (isTemplatedInterfaceOperation(op)) return; + if (!isAzureSubNamespace(context.program, op.namespace)) return; const responses = getResponsesForOperation(context.program, op)[0].find( (v) => v.statusCodes !== 204 && v.statusCodes !== 202, diff --git a/packages/typespec-azure-resource-manager/src/rules/utils.ts b/packages/typespec-azure-resource-manager/src/rules/utils.ts index 1a933ccf8f..88b2343a8c 100644 --- a/packages/typespec-azure-resource-manager/src/rules/utils.ts +++ b/packages/typespec-azure-resource-manager/src/rules/utils.ts @@ -28,6 +28,12 @@ export function isTemplatedInterfaceOperation(target: Operation) { ); } +export function isAzureSubNamespace(program: Program, ns: Namespace | undefined): boolean { + if (!ns) return false; + const nsName = getNamespaceName(program, ns); + return nsName.startsWith("Azure."); +} + export function isTrackedResource(resourceType: Model) { const resultKind = getArmResourceKind(resourceType); return resultKind === "Tracked"; From bfaa0d79d90d03bcce3c1177bd0a1d1faa4ad5e8 Mon Sep 17 00:00:00 2001 From: Alitzel Mendez Bustillo Date: Thu, 24 Oct 2024 14:06:46 -0700 Subject: [PATCH 06/17] fix test to specify right namespace --- .../test/rules/no-response-body.test.ts | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/packages/typespec-azure-resource-manager/test/rules/no-response-body.test.ts b/packages/typespec-azure-resource-manager/test/rules/no-response-body.test.ts index 33a1262aaf..d3b397aee0 100644 --- a/packages/typespec-azure-resource-manager/test/rules/no-response-body.test.ts +++ b/packages/typespec-azure-resource-manager/test/rules/no-response-body.test.ts @@ -24,6 +24,8 @@ describe("typespec-azure-resource-manager: no response body rule", () => { await tester .expect( ` + namespace Azure.MyService; + model TestAcceptedResponse { @statusCode statusCode: 202; } @@ -37,6 +39,8 @@ describe("typespec-azure-resource-manager: no response body rule", () => { await tester .expect( ` + namespace Azure.MyService; + model TestAcceptedResponse { @statusCode statusCode: 204; } @@ -50,6 +54,8 @@ describe("typespec-azure-resource-manager: no response body rule", () => { await tester .expect( ` + namespace Azure.MyService; + model TestAcceptedResponse { @statusCode statusCode: 204; @bodyRoot body: string; @@ -67,6 +73,8 @@ describe("typespec-azure-resource-manager: no response body rule", () => { await tester .expect( ` + namespace Azure.MyService; + model TestAcceptedResponse { @statusCode statusCode: 202; @bodyRoot body: string; @@ -84,6 +92,8 @@ describe("typespec-azure-resource-manager: no response body rule", () => { await tester .expect( ` + namespace Azure.MyService; + model TestAcceptedResponse { @statusCode statusCode: 201; } From 65df2214185df6551fe612c18bd048ca35f5176e Mon Sep 17 00:00:00 2001 From: Alitzel Mendez Bustillo Date: Thu, 31 Oct 2024 14:51:38 -0700 Subject: [PATCH 07/17] Feedback: - Update documentation to reflect arm usage - Use isAzureSubNamespace from azure.core instead of adding a new one on arm - Update test to use arm examples - Names updates to --- .../reference/linter.md | 2 +- .../rules/no-response-body.md | 36 +++++++++---------- packages/typespec-azure-core/src/index.ts | 1 + .../typespec-azure-resource-manager/README.md | 2 +- .../src/rules/no-response-body.ts | 9 +++-- .../src/rules/utils.ts | 6 ---- .../test/rules/no-response-body.test.ts | 18 ++++------ 7 files changed, 29 insertions(+), 45 deletions(-) diff --git a/docs/libraries/azure-resource-manager/reference/linter.md b/docs/libraries/azure-resource-manager/reference/linter.md index 4f4e9c094d..32da9d4ccf 100644 --- a/docs/libraries/azure-resource-manager/reference/linter.md +++ b/docs/libraries/azure-resource-manager/reference/linter.md @@ -45,7 +45,7 @@ Available ruleSets: | `@azure-tools/typespec-azure-resource-manager/improper-subscription-list-operation` | Tenant and Extension resources should not define a list by subscription operation. | | [`@azure-tools/typespec-azure-resource-manager/lro-location-header`](/libraries/azure-resource-manager/rules/lro-location-header.md) | A 202 response should include a Location response header. | | [`@azure-tools/typespec-azure-resource-manager/missing-x-ms-identifiers`](/libraries/azure-resource-manager/rules/missing-x-ms-identifiers.md) | 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`](/libraries/azure-resource-manager/rules/no-response-body.md) | Check that the body is empty for 202 and 204 responses, and not empty for non-204/non-202 responses. | +| [`@azure-tools/typespec-azure-resource-manager/no-response-body`](/libraries/azure-resource-manager/rules/no-response-body.md) | 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/missing-operations-endpoint` | Check for missing Operations interface. | | `@azure-tools/typespec-azure-resource-manager/patch-envelope` | Patch envelope properties should match the resource properties. | | `@azure-tools/typespec-azure-resource-manager/arm-resource-patch` | Validate ARM PATCH operations. | diff --git a/docs/libraries/azure-resource-manager/rules/no-response-body.md b/docs/libraries/azure-resource-manager/rules/no-response-body.md index 7e9f4317cb..e32cfc95ac 100644 --- a/docs/libraries/azure-resource-manager/rules/no-response-body.md +++ b/docs/libraries/azure-resource-manager/rules/no-response-body.md @@ -14,11 +14,9 @@ ARM response operations with status code 202 or 204 should not contain response #### ❌ Incorrect ```tsp -model Test204BodyResponse { - @statusCode statusCode: 204; - @bodyRoot body: string; -} -op walk(): Test204BodyResponse; +op walk(): ArmNoContentResponse & { + @body body: string; +}; ``` ```json @@ -37,10 +35,7 @@ op walk(): Test204BodyResponse; #### ✅ Correct ```tsp -model Test204EmptyResponse { - @statusCode statusCode: 202; -} -op walk(): Test204EmptyResponse; +op walk(): ArmAcceptedResponse; ``` ```json @@ -58,10 +53,7 @@ op walk(): Test204EmptyResponse; #### ❌ Incorrect ```tsp -model Test201EmptyResponse { - @statusCode statusCode: 201; -} -op walk(): Test201EmptyResponse; +op walk(): CreatedResponse; ``` ```json @@ -77,20 +69,24 @@ op walk(): Test201EmptyResponse; #### ✅ Correct ```tsp -model Test201BodyResponse { - @statusCode statusCode: 201; - @bodyRoot body: string; -} -op walk(): Test201BodyResponse; +op walk(): ArmCreatedResponse<{ + name: string; +}>; ``` ```json { "responses": { "201": { - "description": "The request has succeeded and a new resource has been created as a result.", + "description": "Azure create operation completed successfully.", "schema": { - "type": "string" + "type": "object", + "properties": { + "name": { + "type": "string" + } + }, + "required": ["name"] } } } diff --git a/packages/typespec-azure-core/src/index.ts b/packages/typespec-azure-core/src/index.ts index 27ad0bc534..cd6d2d71e9 100644 --- a/packages/typespec-azure-core/src/index.ts +++ b/packages/typespec-azure-core/src/index.ts @@ -8,6 +8,7 @@ export { UnionEnum, getUnionAsEnum } from "./helpers/union-enums.js"; export * from "./lro-helpers.js"; export * from "./rules/prevent-rest-library.js"; export * from "./rules/use-standard-operations.js"; +export * from "./rules/utils.js"; export * from "./traits.js"; export * from "./utils.js"; diff --git a/packages/typespec-azure-resource-manager/README.md b/packages/typespec-azure-resource-manager/README.md index 87611cef67..f15a25eba5 100644 --- a/packages/typespec-azure-resource-manager/README.md +++ b/packages/typespec-azure-resource-manager/README.md @@ -51,7 +51,7 @@ Available ruleSets: | `@azure-tools/typespec-azure-resource-manager/improper-subscription-list-operation` | Tenant and Extension resources should not define a list by subscription operation. | | [`@azure-tools/typespec-azure-resource-manager/lro-location-header`](https://azure.github.io/typespec-azure/docs/libraries/azure-resource-manager/rules/lro-location-header) | A 202 response should include a Location response header. | | [`@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 non-204/non-202 responses. | +| [`@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/missing-operations-endpoint` | Check for missing Operations interface. | | `@azure-tools/typespec-azure-resource-manager/patch-envelope` | Patch envelope properties should match the resource properties. | | `@azure-tools/typespec-azure-resource-manager/arm-resource-patch` | Validate ARM PATCH operations. | diff --git a/packages/typespec-azure-resource-manager/src/rules/no-response-body.ts b/packages/typespec-azure-resource-manager/src/rules/no-response-body.ts index 70c00ac73d..e624852992 100644 --- a/packages/typespec-azure-resource-manager/src/rules/no-response-body.ts +++ b/packages/typespec-azure-resource-manager/src/rules/no-response-body.ts @@ -1,8 +1,7 @@ +import { isAzureSubNamespace } from "@azure-tools/typespec-azure-core"; import { createRule, Operation } from "@typespec/compiler"; import { getResponsesForOperation, HttpOperationResponse } from "@typespec/http"; - -import { isAzureSubNamespace, isTemplatedInterfaceOperation } from "./utils.js"; - +import { isTemplatedInterfaceOperation } from "./utils.js"; /** * verify an operation returns 202 or 204 should not contain response body. * verify that operations that are different for 202 and 204 should contain a response body. @@ -10,11 +9,11 @@ import { isAzureSubNamespace, isTemplatedInterfaceOperation } from "./utils.js"; export const noResponseBodyRule = createRule({ name: "no-response-body", description: - "Check that the body is empty for 202 and 204 responses, and not empty for non-204/non-202 responses.", + "Check that the body is empty for 202 and 204 responses, and not empty for other success (2xx) responses.", url: "https://azure.github.io/typespec-azure/docs/libraries/azure-resource-manager/rules/no-response-body", severity: "warning", messages: { - default: `The body of non-204/non-202 responses should not be empty.`, + default: `The body of responses with success (2xx) status codes other than 202 and 204 should not be empty.`, shouldBeEmpty202: `The body of 202 response should be empty.`, shouldBeEmpty204: `The body of 204 response should be empty.`, }, diff --git a/packages/typespec-azure-resource-manager/src/rules/utils.ts b/packages/typespec-azure-resource-manager/src/rules/utils.ts index 88b2343a8c..1a933ccf8f 100644 --- a/packages/typespec-azure-resource-manager/src/rules/utils.ts +++ b/packages/typespec-azure-resource-manager/src/rules/utils.ts @@ -28,12 +28,6 @@ export function isTemplatedInterfaceOperation(target: Operation) { ); } -export function isAzureSubNamespace(program: Program, ns: Namespace | undefined): boolean { - if (!ns) return false; - const nsName = getNamespaceName(program, ns); - return nsName.startsWith("Azure."); -} - export function isTrackedResource(resourceType: Model) { const resultKind = getArmResourceKind(resourceType); return resultKind === "Tracked"; diff --git a/packages/typespec-azure-resource-manager/test/rules/no-response-body.test.ts b/packages/typespec-azure-resource-manager/test/rules/no-response-body.test.ts index d3b397aee0..092860e324 100644 --- a/packages/typespec-azure-resource-manager/test/rules/no-response-body.test.ts +++ b/packages/typespec-azure-resource-manager/test/rules/no-response-body.test.ts @@ -25,7 +25,7 @@ describe("typespec-azure-resource-manager: no response body rule", () => { .expect( ` namespace Azure.MyService; - + model TestAcceptedResponse { @statusCode statusCode: 202; } @@ -55,12 +55,9 @@ describe("typespec-azure-resource-manager: no response body rule", () => { .expect( ` namespace Azure.MyService; - - model TestAcceptedResponse { - @statusCode statusCode: 204; - @bodyRoot body: string; - } - op walk(): TestAcceptedResponse; + op walk(): ArmNoContentResponse & { + @body body: string; + }; `, ) .toEmitDiagnostics({ @@ -94,15 +91,12 @@ describe("typespec-azure-resource-manager: no response body rule", () => { ` namespace Azure.MyService; - model TestAcceptedResponse { - @statusCode statusCode: 201; - } - op walk(): TestAcceptedResponse; + op walk(): CreatedResponse; `, ) .toEmitDiagnostics({ code: "@azure-tools/typespec-azure-resource-manager/no-response-body", - message: `The body of non-204/non-202 responses should not be empty.`, + message: `The body of responses with success (2xx) status codes other than 202 and 204 should not be empty.`, }); }); }); From f2aaec116bfd6b162415216b01914827df52235e Mon Sep 17 00:00:00 2001 From: Alitzel Mendez <6895254+AlitzelMendez@users.noreply.github.com> Date: Thu, 31 Oct 2024 14:53:48 -0700 Subject: [PATCH 08/17] Update docs/libraries/azure-resource-manager/rules/no-response-body.md Co-authored-by: Mark Cowlishaw --- docs/libraries/azure-resource-manager/rules/no-response-body.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/libraries/azure-resource-manager/rules/no-response-body.md b/docs/libraries/azure-resource-manager/rules/no-response-body.md index e32cfc95ac..ace14258fd 100644 --- a/docs/libraries/azure-resource-manager/rules/no-response-body.md +++ b/docs/libraries/azure-resource-manager/rules/no-response-body.md @@ -7,7 +7,7 @@ title: no-empty-model ``` -ARM response operations with status code 202 or 204 should not contain response body. Operations that are different for 202 and 204 should contain a response body. +ARM operation responses with status code 202 or 204 should not contain a response body. Operation responses with other success (2xx) status codes should contain a response body. ### For 202 and 204 status codes (response body should be empty) From ea57a0e1a617582a0d2aeec8510beefef9e1a676 Mon Sep 17 00:00:00 2001 From: Alitzel Mendez <6895254+AlitzelMendez@users.noreply.github.com> Date: Thu, 31 Oct 2024 14:53:57 -0700 Subject: [PATCH 09/17] Update docs/libraries/azure-resource-manager/rules/no-response-body.md Co-authored-by: Mark Cowlishaw --- docs/libraries/azure-resource-manager/rules/no-response-body.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/libraries/azure-resource-manager/rules/no-response-body.md b/docs/libraries/azure-resource-manager/rules/no-response-body.md index ace14258fd..a8b2b2f9b7 100644 --- a/docs/libraries/azure-resource-manager/rules/no-response-body.md +++ b/docs/libraries/azure-resource-manager/rules/no-response-body.md @@ -48,7 +48,7 @@ op walk(): ArmAcceptedResponse; } ``` -### For operations different than 202 and 204 status codes (response body should not be empty) +### For other success (2xx) response status codes (response body should not be empty) #### ❌ Incorrect From 759893a33d5b21be84bf0759916d8d0d17ae42ab Mon Sep 17 00:00:00 2001 From: Alitzel Mendez Bustillo Date: Thu, 31 Oct 2024 15:03:50 -0700 Subject: [PATCH 10/17] Add summary of changes --- .../changes/no-response-body-rule-2024-9-31-15-0-57.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .chronus/changes/no-response-body-rule-2024-9-31-15-0-57.md diff --git a/.chronus/changes/no-response-body-rule-2024-9-31-15-0-57.md b/.chronus/changes/no-response-body-rule-2024-9-31-15-0-57.md new file mode 100644 index 0000000000..0284b6c3f3 --- /dev/null +++ b/.chronus/changes/no-response-body-rule-2024-9-31-15-0-57.md @@ -0,0 +1,7 @@ +--- +changeKind: internal +packages: + - "@azure-tools/typespec-azure-core" +--- + +Added an export statement to export functions from the utils.js file located in the rules directory. From 65a0c3436f0108f4e15ce4ec9d82d59cd82c8169 Mon Sep 17 00:00:00 2001 From: Alitzel Mendez Bustillo Date: Mon, 4 Nov 2024 10:59:31 -0800 Subject: [PATCH 11/17] Feedback: Removing condition, still pending fixing samples --- .../changes/no-response-body-rule-2024-9-31-15-0-57.md | 7 ------- packages/typespec-azure-core/src/index.ts | 1 - .../src/rules/no-response-body.ts | 2 -- 3 files changed, 10 deletions(-) delete mode 100644 .chronus/changes/no-response-body-rule-2024-9-31-15-0-57.md diff --git a/.chronus/changes/no-response-body-rule-2024-9-31-15-0-57.md b/.chronus/changes/no-response-body-rule-2024-9-31-15-0-57.md deleted file mode 100644 index 0284b6c3f3..0000000000 --- a/.chronus/changes/no-response-body-rule-2024-9-31-15-0-57.md +++ /dev/null @@ -1,7 +0,0 @@ ---- -changeKind: internal -packages: - - "@azure-tools/typespec-azure-core" ---- - -Added an export statement to export functions from the utils.js file located in the rules directory. diff --git a/packages/typespec-azure-core/src/index.ts b/packages/typespec-azure-core/src/index.ts index cd6d2d71e9..27ad0bc534 100644 --- a/packages/typespec-azure-core/src/index.ts +++ b/packages/typespec-azure-core/src/index.ts @@ -8,7 +8,6 @@ export { UnionEnum, getUnionAsEnum } from "./helpers/union-enums.js"; export * from "./lro-helpers.js"; export * from "./rules/prevent-rest-library.js"; export * from "./rules/use-standard-operations.js"; -export * from "./rules/utils.js"; export * from "./traits.js"; export * from "./utils.js"; diff --git a/packages/typespec-azure-resource-manager/src/rules/no-response-body.ts b/packages/typespec-azure-resource-manager/src/rules/no-response-body.ts index e624852992..6f999e7fb4 100644 --- a/packages/typespec-azure-resource-manager/src/rules/no-response-body.ts +++ b/packages/typespec-azure-resource-manager/src/rules/no-response-body.ts @@ -1,4 +1,3 @@ -import { isAzureSubNamespace } from "@azure-tools/typespec-azure-core"; import { createRule, Operation } from "@typespec/compiler"; import { getResponsesForOperation, HttpOperationResponse } from "@typespec/http"; import { isTemplatedInterfaceOperation } from "./utils.js"; @@ -21,7 +20,6 @@ export const noResponseBodyRule = createRule({ return { operation: (op: Operation) => { if (isTemplatedInterfaceOperation(op)) return; - if (!isAzureSubNamespace(context.program, op.namespace)) return; const responses = getResponsesForOperation(context.program, op)[0].find( (v) => v.statusCodes !== 204 && v.statusCodes !== 202, From 167c8b6108f362aeb813d1de7379d0aeff4544a7 Mon Sep 17 00:00:00 2001 From: Alitzel Mendez Bustillo Date: Fri, 15 Nov 2024 10:14:25 -0800 Subject: [PATCH 12/17] rule condition should match ARM scenarios --- .../src/rules/no-response-body.ts | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/typespec-azure-resource-manager/src/rules/no-response-body.ts b/packages/typespec-azure-resource-manager/src/rules/no-response-body.ts index 6f999e7fb4..d4e669383d 100644 --- a/packages/typespec-azure-resource-manager/src/rules/no-response-body.ts +++ b/packages/typespec-azure-resource-manager/src/rules/no-response-body.ts @@ -1,5 +1,5 @@ import { createRule, Operation } from "@typespec/compiler"; -import { getResponsesForOperation, HttpOperationResponse } from "@typespec/http"; +import { getHttpOperation, getResponsesForOperation, HttpOperationResponse } from "@typespec/http"; import { isTemplatedInterfaceOperation } from "./utils.js"; /** * verify an operation returns 202 or 204 should not contain response body. @@ -19,12 +19,18 @@ export const noResponseBodyRule = createRule({ create(context) { return { operation: (op: Operation) => { - if (isTemplatedInterfaceOperation(op)) return; + const [httpOperation] = getHttpOperation(context.program, op); + if (isTemplatedInterfaceOperation(op) || httpOperation.verb === "head") return; const responses = getResponsesForOperation(context.program, op)[0].find( (v) => v.statusCodes !== 204 && v.statusCodes !== 202, ); + if (responses && !responses.responses.every((v) => v.body)) { + if (["delete", "post"].includes(httpOperation.verb) && responses.statusCodes === 200) { + return; + } + context.reportDiagnostic({ target: op, }); From 5f66a1818ce555c14ec61f8a31f9438f0bca01c7 Mon Sep 17 00:00:00 2001 From: Alitzel Mendez Bustillo Date: Fri, 15 Nov 2024 10:17:36 -0800 Subject: [PATCH 13/17] feedback --- .../test/rules/no-response-body.test.ts | 9 --------- 1 file changed, 9 deletions(-) diff --git a/packages/typespec-azure-resource-manager/test/rules/no-response-body.test.ts b/packages/typespec-azure-resource-manager/test/rules/no-response-body.test.ts index 092860e324..e93e4c8120 100644 --- a/packages/typespec-azure-resource-manager/test/rules/no-response-body.test.ts +++ b/packages/typespec-azure-resource-manager/test/rules/no-response-body.test.ts @@ -24,8 +24,6 @@ describe("typespec-azure-resource-manager: no response body rule", () => { await tester .expect( ` - namespace Azure.MyService; - model TestAcceptedResponse { @statusCode statusCode: 202; } @@ -39,8 +37,6 @@ describe("typespec-azure-resource-manager: no response body rule", () => { await tester .expect( ` - namespace Azure.MyService; - model TestAcceptedResponse { @statusCode statusCode: 204; } @@ -54,7 +50,6 @@ describe("typespec-azure-resource-manager: no response body rule", () => { await tester .expect( ` - namespace Azure.MyService; op walk(): ArmNoContentResponse & { @body body: string; }; @@ -70,8 +65,6 @@ describe("typespec-azure-resource-manager: no response body rule", () => { await tester .expect( ` - namespace Azure.MyService; - model TestAcceptedResponse { @statusCode statusCode: 202; @bodyRoot body: string; @@ -89,8 +82,6 @@ describe("typespec-azure-resource-manager: no response body rule", () => { await tester .expect( ` - namespace Azure.MyService; - op walk(): CreatedResponse; `, ) From 2a02191289abc922e269eb08d411dee665c2f196 Mon Sep 17 00:00:00 2001 From: Alitzel Mendez Bustillo Date: Mon, 25 Nov 2024 14:08:46 -0800 Subject: [PATCH 14/17] Add tests --- .../test/rules/no-response-body.test.ts | 70 +++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/packages/typespec-azure-resource-manager/test/rules/no-response-body.test.ts b/packages/typespec-azure-resource-manager/test/rules/no-response-body.test.ts index e93e4c8120..0c8c6e9a36 100644 --- a/packages/typespec-azure-resource-manager/test/rules/no-response-body.test.ts +++ b/packages/typespec-azure-resource-manager/test/rules/no-response-body.test.ts @@ -90,4 +90,74 @@ describe("typespec-azure-resource-manager: no response body rule", () => { message: `The body of responses with success (2xx) status codes other than 202 and 204 should not be empty.`, }); }); + + it("valid if a 2xx response has no body for head", async () => { + await tester + .expect( + ` + @useDependency(Azure.ResourceManager.Versions.v1_0_Preview_1) + @armProviderNamespace + namespace Microsoft.Foo; + + model Employee is TrackedResource<{}> { + @key("employeeName") @segment("employeeName") @path + name: string; + } + + @armResourceOperations + interface TestingOperations { + checkExistence is ArmResourceCheckExistence; + } + `, + ) + .toBeValid(); + }); + + it("valid if a 2xx response has no body for delete", async () => { + await tester + .expect( + ` + @useDependency(Azure.ResourceManager.Versions.v1_0_Preview_1) + @armProviderNamespace + namespace Microsoft.Foo; + + model Employee is TrackedResource<{}> { + @key("employeeName") + @segment("employeeName") + @path + name: string; + } + + @armResourceOperations + interface TestingOperations { + delete is ArmResourceDeleteSync; + } + `, + ) + .toBeValid(); + }); + + it("valid if a 2xx response has no body for post", async () => { + await tester + .expect( + ` + @useDependency(Azure.ResourceManager.Versions.v1_0_Preview_1) + @armProviderNamespace + namespace Microsoft.Foo; + + model Employee is TrackedResource<{}> { + @key("employeeName") + @segment("employeeName") + @path + name: string; + } + + @armResourceOperations + interface TestingOperations { + postEmployees is ArmProviderActionAsync; + } + `, + ) + .toBeValid(); + }); }); From 423aa9f4fee9c5ed91be7d8136ee1597824fbe2c Mon Sep 17 00:00:00 2001 From: Alitzel Mendez <6895254+AlitzelMendez@users.noreply.github.com> Date: Tue, 26 Nov 2024 11:48:49 -0800 Subject: [PATCH 15/17] Update packages/typespec-azure-resource-manager/src/rules/no-response-body.ts Co-authored-by: Mark Cowlishaw --- .../src/rules/no-response-body.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/typespec-azure-resource-manager/src/rules/no-response-body.ts b/packages/typespec-azure-resource-manager/src/rules/no-response-body.ts index d4e669383d..e8df267f72 100644 --- a/packages/typespec-azure-resource-manager/src/rules/no-response-body.ts +++ b/packages/typespec-azure-resource-manager/src/rules/no-response-body.ts @@ -2,7 +2,7 @@ import { createRule, Operation } from "@typespec/compiler"; import { getHttpOperation, getResponsesForOperation, HttpOperationResponse } from "@typespec/http"; import { isTemplatedInterfaceOperation } from "./utils.js"; /** - * verify an operation returns 202 or 204 should not contain response body. + * verify that 202 or 204 responses do not contain a response body. * verify that operations that are different for 202 and 204 should contain a response body. */ export const noResponseBodyRule = createRule({ From 0d66e23ef471ee7494c2d710fd3a8852305d1a56 Mon Sep 17 00:00:00 2001 From: Alitzel Mendez <6895254+AlitzelMendez@users.noreply.github.com> Date: Tue, 26 Nov 2024 11:48:56 -0800 Subject: [PATCH 16/17] Update packages/typespec-azure-resource-manager/src/rules/no-response-body.ts Co-authored-by: Mark Cowlishaw --- .../src/rules/no-response-body.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/typespec-azure-resource-manager/src/rules/no-response-body.ts b/packages/typespec-azure-resource-manager/src/rules/no-response-body.ts index e8df267f72..c518ef1502 100644 --- a/packages/typespec-azure-resource-manager/src/rules/no-response-body.ts +++ b/packages/typespec-azure-resource-manager/src/rules/no-response-body.ts @@ -3,7 +3,7 @@ import { getHttpOperation, getResponsesForOperation, HttpOperationResponse } fro import { isTemplatedInterfaceOperation } from "./utils.js"; /** * verify that 202 or 204 responses do not contain a response body. - * verify that operations that are different for 202 and 204 should contain a response body. + * verify that success (2XX) responses other than 202 and 204 contain a response body. */ export const noResponseBodyRule = createRule({ name: "no-response-body", From 17de8e775c12d57de20c4097bbdaa903cc4c1291 Mon Sep 17 00:00:00 2001 From: Alitzel Mendez Bustillo Date: Tue, 26 Nov 2024 11:55:22 -0800 Subject: [PATCH 17/17] Tess feedback --- .../test/rules/no-response-body.test.ts | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/packages/typespec-azure-resource-manager/test/rules/no-response-body.test.ts b/packages/typespec-azure-resource-manager/test/rules/no-response-body.test.ts index 0c8c6e9a36..957a919511 100644 --- a/packages/typespec-azure-resource-manager/test/rules/no-response-body.test.ts +++ b/packages/typespec-azure-resource-manager/test/rules/no-response-body.test.ts @@ -145,16 +145,9 @@ describe("typespec-azure-resource-manager: no response body rule", () => { @armProviderNamespace namespace Microsoft.Foo; - model Employee is TrackedResource<{}> { - @key("employeeName") - @segment("employeeName") - @path - name: string; - } - @armResourceOperations interface TestingOperations { - postEmployees is ArmProviderActionAsync; + postEmployees is ArmProviderActionAsync; } `, )