From 9d8e4839e65ffdeec183bc7e52f1e909de01da86 Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Tue, 20 Feb 2024 14:27:38 -0800 Subject: [PATCH 01/26] Add new decorators --- packages/http/lib/http-decorators.tsp | 30 ++++++++++++++++++++++++++- packages/http/src/decorators.ts | 16 ++++++++++++++ packages/http/src/lib.ts | 2 ++ 3 files changed, 47 insertions(+), 1 deletion(-) diff --git a/packages/http/lib/http-decorators.tsp b/packages/http/lib/http-decorators.tsp index b73c8ba534..64b7c23f62 100644 --- a/packages/http/lib/http-decorators.tsp +++ b/packages/http/lib/http-decorators.tsp @@ -83,7 +83,10 @@ extern dec query(target: ModelProperty, queryNameOrOptions?: string | QueryOptio extern dec path(target: ModelProperty, paramName?: valueof string); /** - * Explicitly specify that this property is to be set as the body + * Explicitly specify that this property type will be exactly the HTTP body. + * + * This means that any properties under `@body` cannot be marked as headers, query parameters, or path parameters. + * If wanting to change the resolution of the body but still mix parameters, use `@bodyRoot`. * * @example * @@ -94,6 +97,31 @@ extern dec path(target: ModelProperty, paramName?: valueof string); */ extern dec body(target: ModelProperty); +/** + * Specify that the body resolution should be resolved from that property. + * By default the body is resolved by including all properties in the operation request/response that are not metadata. + * This allows to nest the body in a property while still allowing to use headers, query parameters, and path parameters in the same model. + * + * @example + * + * ```typespec + * op upload(@bodyRoot user: {name: string, @header id: string}): void; + * op download(): {@bodyRoot user: {name: string, @header id: string}}; + * ``` + */ +extern dec bodyRoot(target: ModelProperty); +/** + * Specify that this property shouldn't be included in the HTTP body. + * This can be useful when bundling metadata together that would result in an empty property to be included in the body. + * + * @example + * + * ```typespec + * op upload(name: string, @bodyIgnore headers: {@header id: string}): void; + * ``` + */ +extern dec bodyIgnore(target: ModelProperty); + /** * Specify the status code for this response. Property type must be a status code integer or a union of status code integer. * diff --git a/packages/http/src/decorators.ts b/packages/http/src/decorators.ts index 4777f05130..90ac87a897 100644 --- a/packages/http/src/decorators.ts +++ b/packages/http/src/decorators.ts @@ -170,10 +170,26 @@ export function $body(context: DecoratorContext, entity: ModelProperty) { context.program.stateSet(HttpStateKeys.body).add(entity); } +export function $bodyRoot(context: DecoratorContext, entity: ModelProperty) { + context.program.stateSet(HttpStateKeys.bodyRoot).add(entity); +} + +export function $bodyIgnore(context: DecoratorContext, entity: ModelProperty) { + context.program.stateSet(HttpStateKeys.bodyIgnore).add(entity); +} + export function isBody(program: Program, entity: Type): boolean { return program.stateSet(HttpStateKeys.body).has(entity); } +export function isBodyRoot(program: Program, entity: ModelProperty): boolean { + return program.stateSet(HttpStateKeys.bodyRoot).has(entity); +} + +export function isBodyIgnore(program: Program, entity: ModelProperty): boolean { + return program.stateSet(HttpStateKeys.bodyIgnore).has(entity); +} + export function $statusCode(context: DecoratorContext, entity: ModelProperty) { context.program.stateSet(HttpStateKeys.statusCode).add(entity); diff --git a/packages/http/src/lib.ts b/packages/http/src/lib.ts index 0c5dbc6f41..9393c308a3 100644 --- a/packages/http/src/lib.ts +++ b/packages/http/src/lib.ts @@ -136,6 +136,8 @@ export const $lib = createTypeSpecLibrary({ query: { description: "State for the @query decorator" }, path: { description: "State for the @path decorator" }, body: { description: "State for the @body decorator" }, + bodyRoot: { description: "State for the @bodyRoot decorator" }, + bodyIgnore: { description: "State for the @bodyIgnore decorator" }, statusCode: { description: "State for the @statusCode decorator" }, verbs: { description: "State for the verb decorators (@get, @post, @put, etc.)" }, servers: { description: "State for the @server decorator" }, From 9b12f773295c229b4d8075e4d28c684739bc5c23 Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Tue, 20 Feb 2024 15:12:35 -0800 Subject: [PATCH 02/26] add basic tests --- packages/http/src/responses.ts | 3 +- packages/http/test/http-decorators.test.ts | 64 ++++++++++++++++++++++ 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/packages/http/src/responses.ts b/packages/http/src/responses.ts index 7015a50f8c..c90ab3a028 100644 --- a/packages/http/src/responses.ts +++ b/packages/http/src/responses.ts @@ -22,6 +22,7 @@ import { getStatusCodeDescription, getStatusCodesWithDiagnostics, isBody, + isBodyRoot, isHeader, isStatusCode, } from "./decorators.js"; @@ -241,7 +242,7 @@ function getResponseBody( // look for explicit body let bodyProperty: ModelProperty | undefined; for (const property of metadata) { - if (isBody(program, property)) { + if (isBody(program, property) || isBodyRoot(program, property)) { if (bodyProperty) { diagnostics.add(createDiagnostic({ code: "duplicate-body", target: property })); } else { diff --git a/packages/http/test/http-decorators.test.ts b/packages/http/test/http-decorators.test.ts index 8aafa986b6..c4d400e6ef 100644 --- a/packages/http/test/http-decorators.test.ts +++ b/packages/http/test/http-decorators.test.ts @@ -18,6 +18,8 @@ import { getStatusCodes, includeInapplicableMetadataInPayload, isBody, + isBodyIgnore, + isBodyRoot, isHeader, isPathParam, isQueryParam, @@ -403,6 +405,68 @@ describe("http: decorators", () => { }); }); + describe("@bodyRoot", () => { + it("emit diagnostics when @body is not used on model property", async () => { + const diagnostics = await runner.diagnose(` + @bodyRoot op test(): string; + + @bodyRoot model Foo {} + `); + + expectDiagnostics(diagnostics, [ + { + code: "decorator-wrong-target", + message: + "Cannot apply @bodyRoot decorator to test since it is not assignable to ModelProperty", + }, + { + code: "decorator-wrong-target", + message: + "Cannot apply @bodyRoot decorator to Foo since it is not assignable to ModelProperty", + }, + ]); + }); + + it("set the body root with @bodyRoot", async () => { + const { body } = (await runner.compile(` + @post op test(@test @bodyRoot body: string): string; + `)) as { body: ModelProperty }; + + ok(isBodyRoot(runner.program, body)); + }); + }); + + describe("@bodyIgnore", () => { + it("emit diagnostics when @body is not used on model property", async () => { + const diagnostics = await runner.diagnose(` + @bodyIgnore op test(): string; + + @bodyIgnore model Foo {} + `); + + expectDiagnostics(diagnostics, [ + { + code: "decorator-wrong-target", + message: + "Cannot apply @bodyIgnore decorator to test since it is not assignable to ModelProperty", + }, + { + code: "decorator-wrong-target", + message: + "Cannot apply @bodyIgnore decorator to Foo since it is not assignable to ModelProperty", + }, + ]); + }); + + it("isBodyIgnore returns true on property decorated", async () => { + const { body } = await runner.compile(` + @post op test(@test @bodyIgnore body: string): string; + `); + + ok(isBodyIgnore(runner.program, body as ModelProperty)); + }); + }); + describe("@statusCode", () => { it("emit diagnostics when @statusCode is not used on model property", async () => { const diagnostics = await runner.diagnose(` From d2690b5c0d1251397dd799bb2268c90cd265bf2c Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Tue, 20 Feb 2024 20:11:43 -0800 Subject: [PATCH 03/26] Handle response --- packages/http/src/body.ts | 45 ++++++++ packages/http/src/lib.ts | 6 + packages/http/src/metadata.ts | 3 +- packages/http/src/responses.ts | 22 +++- packages/http/test/responses.test.ts | 163 +++++++++++++++------------ packages/openapi3/src/lib.ts | 6 - 6 files changed, 163 insertions(+), 82 deletions(-) create mode 100644 packages/http/src/body.ts diff --git a/packages/http/src/body.ts b/packages/http/src/body.ts new file mode 100644 index 0000000000..4b1e13d5ef --- /dev/null +++ b/packages/http/src/body.ts @@ -0,0 +1,45 @@ +import { + Diagnostic, + ModelProperty, + Program, + createDiagnosticCollector, + navigateType, +} from "@typespec/compiler"; +import { isHeader, isPathParam, isQueryParam, isStatusCode } from "./decorators.js"; +import { createDiagnostic } from "./lib.js"; + +/** Validate a property marked with `@body` */ +export function validateBodyProperty( + program: Program, + property: ModelProperty +): [boolean, readonly Diagnostic[]] { + const diagnostics = createDiagnosticCollector(); + navigateType( + property.type, + { + modelProperty: (prop) => { + const kind = isHeader(program, prop) + ? "header" + : isQueryParam(program, prop) + ? "query" + : isPathParam(program, prop) + ? "path" + : isStatusCode(program, prop) + ? "statusCode" + : undefined; + + if (kind) { + diagnostics.add( + createDiagnostic({ + code: "metadata-ignored", + format: { kind }, + target: prop, + }) + ); + } + }, + }, + {} + ); + return diagnostics.wrap(diagnostics.diagnostics.length === 0); +} diff --git a/packages/http/src/lib.ts b/packages/http/src/lib.ts index 9393c308a3..5e5bf99ae1 100644 --- a/packages/http/src/lib.ts +++ b/packages/http/src/lib.ts @@ -87,6 +87,12 @@ export const $lib = createTypeSpecLibrary({ default: "`Content-Type` header ignored because there is no body.", }, }, + "metadata-ignored": { + severity: "warning", + messages: { + default: paramMessage`${"kind"} property will be ignored as it is inside of a @body property. Use @bodyRoot instead if wanting to mix.`, + }, + }, "no-service-found": { severity: "warning", messages: { diff --git a/packages/http/src/metadata.ts b/packages/http/src/metadata.ts index a022e12431..8aff5ed97c 100644 --- a/packages/http/src/metadata.ts +++ b/packages/http/src/metadata.ts @@ -17,6 +17,7 @@ import { import { includeInapplicableMetadataInPayload, isBody, + isBodyRoot, isHeader, isPathParam, isQueryParam, @@ -343,7 +344,7 @@ function isApplicableMetadataCore( return false; // no metadata is applicable to collection items } - if (treatBodyAsMetadata && isBody(program, property)) { + if (treatBodyAsMetadata && (isBody(program, property) || isBodyRoot(program, property))) { return true; } diff --git a/packages/http/src/responses.ts b/packages/http/src/responses.ts index c90ab3a028..dd8bc6076a 100644 --- a/packages/http/src/responses.ts +++ b/packages/http/src/responses.ts @@ -2,6 +2,7 @@ import { createDiagnosticCollector, Diagnostic, DiagnosticCollector, + DuplicateTracker, getDoc, getErrorsDoc, getReturnsDoc, @@ -16,6 +17,7 @@ import { Type, walkPropertiesInherited, } from "@typespec/compiler"; +import { validateBodyProperty } from "./body.js"; import { getContentTypes, isContentTypeHeader } from "./content-types.js"; import { getHeaderFieldName, @@ -239,17 +241,31 @@ function getResponseBody( return responseType; } + const duplicateTracker = new DuplicateTracker(); + // look for explicit body let bodyProperty: ModelProperty | undefined; for (const property of metadata) { if (isBody(program, property) || isBodyRoot(program, property)) { - if (bodyProperty) { - diagnostics.add(createDiagnostic({ code: "duplicate-body", target: property })); - } else { + duplicateTracker.track("body", property); + if (bodyProperty === undefined) { bodyProperty = property; + if (isBody(program, property)) { + diagnostics.pipe(validateBodyProperty(program, property)); + } } } } + for (const [_, items] of duplicateTracker.entries()) { + for (const prop of items) { + diagnostics.add( + createDiagnostic({ + code: "duplicate-body", + target: prop, + }) + ); + } + } if (bodyProperty) { return bodyProperty.type; } diff --git a/packages/http/test/responses.test.ts b/packages/http/test/responses.test.ts index 3632746f52..c8c2f8ea49 100644 --- a/packages/http/test/responses.test.ts +++ b/packages/http/test/responses.test.ts @@ -4,29 +4,54 @@ import { deepStrictEqual, ok, strictEqual } from "assert"; import { describe, it } from "vitest"; import { compileOperations, getOperationsWithServiceNamespace } from "./test-host.js"; -describe("http: responses", () => { - it("issues diagnostics for duplicate body decorator", async () => { +describe("body resolution", () => { + it("emit diagnostics for duplicate @body decorator", async () => { const [_, diagnostics] = await compileOperations( - ` - model Foo { - foo: string; - } - model Bar { - bar: string; - } - @route("/") - namespace root { - @get - op read(): { @body body1: Foo, @body body2: Bar }; - } - ` + `op read(): { @body body1: string, @body body2: int32 };` + ); + expectDiagnostics(diagnostics, [ + { code: "@typespec/http/duplicate-body" }, + { code: "@typespec/http/duplicate-body" }, + ]); + }); + + it("emit diagnostics for duplicate @bodyRoot decorator", async () => { + const [_, diagnostics] = await compileOperations( + `op read(): { @bodyRoot body1: string, @bodyRoot body2: int32 };` ); - expectDiagnostics(diagnostics, [{ code: "@typespec/http/duplicate-body" }]); + expectDiagnostics(diagnostics, [ + { code: "@typespec/http/duplicate-body" }, + { code: "@typespec/http/duplicate-body" }, + ]); }); - it("issues diagnostics for invalid content types", async () => { + it("emit diagnostics for using @body and @bodyRoute decorator", async () => { const [_, diagnostics] = await compileOperations( - ` + `op read(): { @bodyRoot body1: string, @body body2: int32 };` + ); + expectDiagnostics(diagnostics, [ + { code: "@typespec/http/duplicate-body" }, + { code: "@typespec/http/duplicate-body" }, + ]); + }); + + describe("emit diagnostics when using metadata decorator in @body", () => { + it.each([ + ["@header", "id: string"], + ["@query", "id: string"], + ["@statusCode", "_: 200"], + ])("%s", async (dec, prop) => { + const [_, diagnostics] = await compileOperations( + `op read(): { @body explicit: {${dec} ${prop}, other: string} };` + ); + expectDiagnostics(diagnostics, { code: "@typespec/http/metadata-ignored" }); + }); + }); +}); + +it("issues diagnostics for invalid content types", async () => { + const [_, diagnostics] = await compileOperations( + ` model Foo { foo: string; } @@ -35,58 +60,53 @@ describe("http: responses", () => { contentType: "text/plain"; } - namespace root { - @route("/test1") - @get - op test1(): { @header contentType: string, @body body: Foo }; - @route("/test2") - @get - op test2(): { @header contentType: 42, @body body: Foo }; - @route("/test3") - @get - op test3(): { @header contentType: "application/json" | TextPlain, @body body: Foo }; - } + @route("/test1") + @get + op test1(): { @header contentType: string, @body body: Foo }; + @route("/test2") + @get + op test2(): { @header contentType: 42, @body body: Foo }; + @route("/test3") + @get + op test3(): { @header contentType: "application/json" | TextPlain, @body body: Foo }; ` - ); - expectDiagnostics(diagnostics, [ - { code: "@typespec/http/content-type-string" }, - { code: "@typespec/http/content-type-string" }, - { code: "@typespec/http/content-type-string" }, - ]); - }); + ); + expectDiagnostics(diagnostics, [ + { code: "@typespec/http/content-type-string" }, + { code: "@typespec/http/content-type-string" }, + { code: "@typespec/http/content-type-string" }, + ]); +}); - it("supports any casing for string literal 'Content-Type' header properties.", async () => { - const [routes, diagnostics] = await getOperationsWithServiceNamespace( - ` +it("supports any casing for string literal 'Content-Type' header properties.", async () => { + const [routes, diagnostics] = await getOperationsWithServiceNamespace( + ` model Foo {} - @test - namespace Test { - @route("/test1") - @get - op test1(): { @header "content-Type": "text/html", @body body: Foo }; + @route("/test1") + @get + op test1(): { @header "content-Type": "text/html", @body body: Foo }; - @route("/test2") - @get - op test2(): { @header "CONTENT-type": "text/plain", @body body: Foo }; + @route("/test2") + @get + op test2(): { @header "CONTENT-type": "text/plain", @body body: Foo }; - @route("/test3") - @get - op test3(): { @header "content-type": "application/json", @body body: Foo }; - } + @route("/test3") + @get + op test3(): { @header "content-type": "application/json", @body body: Foo }; ` - ); - expectDiagnosticEmpty(diagnostics); - strictEqual(routes.length, 3); - deepStrictEqual(routes[0].responses[0].responses[0].body?.contentTypes, ["text/html"]); - deepStrictEqual(routes[1].responses[0].responses[0].body?.contentTypes, ["text/plain"]); - deepStrictEqual(routes[2].responses[0].responses[0].body?.contentTypes, ["application/json"]); - }); + ); + expectDiagnosticEmpty(diagnostics); + strictEqual(routes.length, 3); + deepStrictEqual(routes[0].responses[0].responses[0].body?.contentTypes, ["text/html"]); + deepStrictEqual(routes[1].responses[0].responses[0].body?.contentTypes, ["text/plain"]); + deepStrictEqual(routes[2].responses[0].responses[0].body?.contentTypes, ["application/json"]); +}); - // Regression test for https://github.com/microsoft/typespec/issues/328 - it("empty response model becomes body if it has children", async () => { - const [routes, diagnostics] = await getOperationsWithServiceNamespace( - ` +// Regression test for https://github.com/microsoft/typespec/issues/328 +it("empty response model becomes body if it has children", async () => { + const [routes, diagnostics] = await getOperationsWithServiceNamespace( + ` @route("/") op read(): A; model A {} @@ -102,14 +122,13 @@ describe("http: responses", () => { } ` - ); - expectDiagnosticEmpty(diagnostics); - strictEqual(routes.length, 1); - const responses = routes[0].responses; - strictEqual(responses.length, 1); - const response = responses[0]; - const body = response.responses[0].body; - ok(body); - strictEqual((body.type as Model).name, "A"); - }); + ); + expectDiagnosticEmpty(diagnostics); + strictEqual(routes.length, 1); + const responses = routes[0].responses; + strictEqual(responses.length, 1); + const response = responses[0]; + const body = response.responses[0].body; + ok(body); + strictEqual((body.type as Model).name, "A"); }); diff --git a/packages/openapi3/src/lib.ts b/packages/openapi3/src/lib.ts index 45b3ccffc5..900346a8e4 100644 --- a/packages/openapi3/src/lib.ts +++ b/packages/openapi3/src/lib.ts @@ -161,12 +161,6 @@ export const libDef = { default: `OpenAPI does not allow paths containing a query string.`, }, }, - "duplicate-body": { - severity: "error", - messages: { - default: "Duplicate @body declarations on response type", - }, - }, "duplicate-header": { severity: "error", messages: { From 35d876f6330921a18c22c8b10249c18eeb0f2978 Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Wed, 21 Feb 2024 08:57:37 -0800 Subject: [PATCH 04/26] progress --- packages/http/src/responses.ts | 30 ++++++--- packages/http/test/responses.test.ts | 3 +- packages/openapi3/test/return-types.test.ts | 73 +++++---------------- 3 files changed, 40 insertions(+), 66 deletions(-) diff --git a/packages/http/src/responses.ts b/packages/http/src/responses.ts index dd8bc6076a..ef0a0739e8 100644 --- a/packages/http/src/responses.ts +++ b/packages/http/src/responses.ts @@ -3,6 +3,7 @@ import { Diagnostic, DiagnosticCollector, DuplicateTracker, + getDiscriminator, getDoc, getErrorsDoc, getReturnsDoc, @@ -29,7 +30,7 @@ import { isStatusCode, } from "./decorators.js"; import { createDiagnostic, HttpStateKeys, reportDiagnostic } from "./lib.js"; -import { gatherMetadata, isApplicableMetadata, Visibility } from "./metadata.js"; +import { gatherMetadata, isApplicableMetadata, isVisible, Visibility } from "./metadata.js"; import { HttpOperationResponse, HttpStatusCodes, HttpStatusCodesEntry } from "./types.js"; /** @@ -109,11 +110,14 @@ function processResponseType( // If there is no explicit status code, check if it should be 204 if (statusCodes.length === 0) { - if (bodyType === undefined || isVoidType(bodyType)) { - bodyType = undefined; - statusCodes.push(204); - } else if (isErrorModel(program, responseType)) { + if (isErrorModel(program, responseType)) { statusCodes.push("*"); + } else if (isVoidType(responseType)) { + bodyType = undefined; + statusCodes.push(204); // Only special case for 204 is op test(): void; + } else if (bodyType === undefined || isVoidType(bodyType)) { + bodyType = undefined; + statusCodes.push(200); } else { statusCodes.push(200); } @@ -270,14 +274,20 @@ function getResponseBody( return bodyProperty.type; } - // Without an explicit body, response type is response model itself if - // there it has at least one non-metadata property, if it is an empty object or if it has derived - // models - if (responseType.derivedModels.length > 0 || responseType.properties.size === 0) { + // Special case for legacy purposes if the return type is an empty model with only @discriminator("xyz") + // Then we still want to return that object as it technically always has a body with that implicit property. + if (responseType.derivedModels.length > 0 && getDiscriminator(program, responseType)) { return responseType; } + if (responseType.indexer) { + return responseType; + } + for (const property of walkPropertiesInherited(responseType)) { - if (!isApplicableMetadata(program, property, Visibility.Read)) { + if ( + !isApplicableMetadata(program, property, Visibility.Read) && + isVisible(program, property, Visibility.Read) + ) { return responseType; } } diff --git a/packages/http/test/responses.test.ts b/packages/http/test/responses.test.ts index c8c2f8ea49..89d3390c32 100644 --- a/packages/http/test/responses.test.ts +++ b/packages/http/test/responses.test.ts @@ -107,8 +107,9 @@ it("supports any casing for string literal 'Content-Type' header properties.", a it("empty response model becomes body if it has children", async () => { const [routes, diagnostics] = await getOperationsWithServiceNamespace( ` - @route("/") op read(): A; + op read(): A; + @discriminator("foo") model A {} model B extends A { diff --git a/packages/openapi3/test/return-types.test.ts b/packages/openapi3/test/return-types.test.ts index 09cd6f3d93..c84ae14d77 100644 --- a/packages/openapi3/test/return-types.test.ts +++ b/packages/openapi3/test/return-types.test.ts @@ -89,9 +89,9 @@ describe("openapi3: return types", () => { @put op create(): {@header eTag: string}; ` ); - ok(res.paths["/"].put.responses["204"]); - ok(res.paths["/"].put.responses["204"].headers["e-tag"]); - strictEqual(res.paths["/"].put.responses["204"].headers["e-tag"].required, true); + ok(res.paths["/"].put.responses["200"]); + ok(res.paths["/"].put.responses["200"].headers["e-tag"]); + strictEqual(res.paths["/"].put.responses["200"].headers["e-tag"].required, true); }); it("optional response header are marked required: false", async () => { @@ -100,9 +100,9 @@ describe("openapi3: return types", () => { @put op create(): {@header eTag?: string}; ` ); - ok(res.paths["/"].put.responses["204"]); - ok(res.paths["/"].put.responses["204"].headers["e-tag"]); - strictEqual(res.paths["/"].put.responses["204"].headers["e-tag"].required, false); + ok(res.paths["/"].put.responses["200"]); + ok(res.paths["/"].put.responses["200"].headers["e-tag"]); + strictEqual(res.paths["/"].put.responses["200"].headers["e-tag"].required, false); }); it("defines responses with headers and status codes in base model", async () => { @@ -258,40 +258,14 @@ describe("openapi3: return types", () => { ); }); - it("return type with no properties should be 200 with empty object as type", async () => { - const res = await openApiFor( - ` - @get op test(): {}; - ` + describe("response model resolving to no property in the body produce no body", () => { + it.each(["{}", "{@header prop: string}", `{@visibility("none") prop: string}`])( + "%s", + async (body) => { + const res = await openApiFor(`op test(): ${body};`); + strictEqual(res.paths["/"].get.responses["200"].content, undefined); + } ); - - const responses = res.paths["/"].get.responses; - ok(responses["200"]); - deepStrictEqual(responses["200"].content, { - "application/json": { - schema: { - type: "object", - }, - }, - }); - }); - - it("{} return type should produce 200 ", async () => { - const res = await openApiFor( - ` - @get op test(): {}; - ` - ); - - const responses = res.paths["/"].get.responses; - ok(responses["200"]); - deepStrictEqual(responses["200"].content, { - "application/json": { - schema: { - type: "object", - }, - }, - }); }); it("produce additionalProperties schema if response is Record", async () => { @@ -321,20 +295,9 @@ describe("openapi3: return types", () => { `); const responses = res.paths["/"].get.responses; - ok(responses["204"]); - ok(responses["204"].content === undefined, "response should have no content"); - ok(responses["200"] === undefined); - }); - - it("defaults status code to 204 when implicit body has no content", async () => { - const res = await openApiFor(` - @delete op delete(): { @header date: string }; - `); - const responses = res.paths["/"].delete.responses; - ok(responses["200"] === undefined); - ok(responses["204"]); - ok(responses["204"].headers["date"]); - ok(responses["204"].content === undefined); + ok(responses["200"]); + ok(responses["200"].content === undefined, "response should have no content"); + ok(responses["204"] === undefined); }); it("defaults status code to default when model has @error decorator", async () => { @@ -453,9 +416,9 @@ describe("openapi3: return types", () => { ok(res.paths["/"].get.responses["204"]); }); - it("defaults to 204 no content with void @body", async () => { + it("defaults to 200 no content with void @body", async () => { const res = await openApiFor(`@get op read(): {@body body: void};`); - ok(res.paths["/"].get.responses["204"]); + ok(res.paths["/"].get.responses["200"]); }); describe("multiple content types", () => { From f3e53f23a3581b6504fcb992e6ebd39689203f28 Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Wed, 21 Feb 2024 12:15:04 -0800 Subject: [PATCH 05/26] Differ if in explicit body --- packages/http/src/metadata.ts | 17 +++++-- packages/http/src/parameters.ts | 1 + packages/http/src/responses.ts | 52 +++++++++++++-------- packages/http/src/types.ts | 3 ++ packages/openapi3/src/openapi.ts | 28 ++++++++--- packages/openapi3/src/schema-emitter.ts | 9 +++- packages/openapi3/test/return-types.test.ts | 44 ++++++++++++----- 7 files changed, 111 insertions(+), 43 deletions(-) diff --git a/packages/http/src/metadata.ts b/packages/http/src/metadata.ts index 8aff5ed97c..94a52e9941 100644 --- a/packages/http/src/metadata.ts +++ b/packages/http/src/metadata.ts @@ -265,6 +265,9 @@ export function gatherMetadata( if (isApplicableMetadataOrBody(program, property, visibility, isMetadataCallback)) { metadata.set(property.name, property); rootMapOut?.set(property, root); + if (isBody(program, property)) { + continue; // We ignore any properties under `@body` + } } if ( @@ -394,7 +397,11 @@ export interface MetadataInfo { * payload and not applicable metadata {@link isApplicableMetadata} or * filtered out by the given visibility. */ - isPayloadProperty(property: ModelProperty, visibility: Visibility): boolean; + isPayloadProperty( + property: ModelProperty, + visibility: Visibility, + inExplicitBody?: boolean + ): boolean; /** * Determines if the given property is optional in the request or @@ -560,12 +567,14 @@ export function createMetadataInfo(program: Program, options?: MetadataInfoOptio function isPayloadProperty( property: ModelProperty, visibility: Visibility, + inExplicitBody?: boolean, keepShareableProperties?: boolean ): boolean { if ( - isEmptied(property.type, visibility) || - isApplicableMetadata(program, property, visibility) || - (isMetadata(program, property) && !includeInapplicableMetadataInPayload(program, property)) + !inExplicitBody && + (isEmptied(property.type, visibility) || + isApplicableMetadata(program, property, visibility) || + (isMetadata(program, property) && !includeInapplicableMetadataInPayload(program, property))) ) { return false; } diff --git a/packages/http/src/parameters.ts b/packages/http/src/parameters.ts index 675a1319ea..2e2bb0cb64 100644 --- a/packages/http/src/parameters.ts +++ b/packages/http/src/parameters.ts @@ -209,6 +209,7 @@ function computeHttpOperationBody( const body: HttpOperationRequestBody = { type: bodyType, + isExplicit: false, parameter: bodyProperty, contentTypes, }; diff --git a/packages/http/src/responses.ts b/packages/http/src/responses.ts index ef0a0739e8..cebd31be83 100644 --- a/packages/http/src/responses.ts +++ b/packages/http/src/responses.ts @@ -106,17 +106,17 @@ function processResponseType( const headers = getResponseHeaders(program, metadata); // Get body - let bodyType = getResponseBody(program, diagnostics, responseType, metadata); + let resolvedBody = getResponseBody(program, diagnostics, responseType, metadata); // If there is no explicit status code, check if it should be 204 if (statusCodes.length === 0) { if (isErrorModel(program, responseType)) { statusCodes.push("*"); } else if (isVoidType(responseType)) { - bodyType = undefined; + resolvedBody = undefined; statusCodes.push(204); // Only special case for 204 is op test(): void; - } else if (bodyType === undefined || isVoidType(bodyType)) { - bodyType = undefined; + } else if (resolvedBody === undefined || isVoidType(resolvedBody.type)) { + resolvedBody = undefined; statusCodes.push(200); } else { statusCodes.push(200); @@ -124,7 +124,7 @@ function processResponseType( } // If there is a body but no explicit content types, use application/json - if (bodyType && contentTypes.length === 0) { + if (resolvedBody && contentTypes.length === 0) { contentTypes.push("application/json"); } @@ -136,12 +136,18 @@ function processResponseType( statusCode: typeof statusCode === "object" ? "*" : (String(statusCode) as any), statusCodes: statusCode, type: responseType, - description: getResponseDescription(program, operation, responseType, statusCode, bodyType), + description: getResponseDescription( + program, + operation, + responseType, + statusCode, + resolvedBody?.type + ), responses: [], }; - if (bodyType !== undefined) { - response.responses.push({ body: { contentTypes: contentTypes, type: bodyType }, headers }); + if (resolvedBody !== undefined) { + response.responses.push({ body: { contentTypes: contentTypes, ...resolvedBody }, headers }); } else if (contentTypes.length > 0) { diagnostics.add( createDiagnostic({ @@ -234,27 +240,33 @@ function getResponseHeaders( return responseHeaders; } +interface ResolvedBody { + readonly type: Type; + readonly isExplicit: boolean; +} function getResponseBody( program: Program, diagnostics: DiagnosticCollector, responseType: Type, metadata: Set -): Type | undefined { +): ResolvedBody | undefined { // non-model or intrinsic/array model -> response body is response type if (responseType.kind !== "Model" || isArrayModelType(program, responseType)) { - return responseType; + return { type: responseType, isExplicit: false }; } const duplicateTracker = new DuplicateTracker(); // look for explicit body - let bodyProperty: ModelProperty | undefined; + let resolvedBody: ResolvedBody | undefined; for (const property of metadata) { - if (isBody(program, property) || isBodyRoot(program, property)) { + const isBodyVal = isBody(program, property); + const isBodyRootVal = isBodyRoot(program, property); + if (isBodyVal || isBodyRootVal) { duplicateTracker.track("body", property); - if (bodyProperty === undefined) { - bodyProperty = property; - if (isBody(program, property)) { + if (resolvedBody === undefined) { + resolvedBody = { type: property.type, isExplicit: isBodyVal }; + if (isBodyVal) { diagnostics.pipe(validateBodyProperty(program, property)); } } @@ -270,17 +282,17 @@ function getResponseBody( ); } } - if (bodyProperty) { - return bodyProperty.type; + if (resolvedBody) { + return resolvedBody; } // Special case for legacy purposes if the return type is an empty model with only @discriminator("xyz") // Then we still want to return that object as it technically always has a body with that implicit property. if (responseType.derivedModels.length > 0 && getDiscriminator(program, responseType)) { - return responseType; + return { type: responseType, isExplicit: false }; } if (responseType.indexer) { - return responseType; + return { type: responseType, isExplicit: false }; } for (const property of walkPropertiesInherited(responseType)) { @@ -288,7 +300,7 @@ function getResponseBody( !isApplicableMetadata(program, property, Visibility.Read) && isVisible(program, property, Visibility.Read) ) { - return responseType; + return { type: responseType, isExplicit: false }; } } diff --git a/packages/http/src/types.ts b/packages/http/src/types.ts index c6dbfcb48f..9ffbe9e8d4 100644 --- a/packages/http/src/types.ts +++ b/packages/http/src/types.ts @@ -382,6 +382,9 @@ export interface HttpOperationBody { * Type of the operation body. */ type: Type; + + /** If the body was explicitly set with `@body`. */ + readonly isExplicit: boolean; } export interface HttpStatusCodeRange { diff --git a/packages/openapi3/src/openapi.ts b/packages/openapi3/src/openapi.ts index 8f85bc6134..17028d1572 100644 --- a/packages/openapi3/src/openapi.ts +++ b/packages/openapi3/src/openapi.ts @@ -873,7 +873,7 @@ function createOAPIEmitter( const isBinary = isBinaryPayload(data.body.type, contentType); const schema = isBinary ? { type: "string", format: "binary" } - : getSchemaForBody(data.body.type, Visibility.Read, undefined); + : getSchemaForBody(data.body.type, Visibility.Read, data.body.isExplicit, undefined); if (schemaMap.has(contentType)) { schemaMap.get(contentType)!.push(schema); } else { @@ -906,9 +906,10 @@ function createOAPIEmitter( function callSchemaEmitter( type: Type, visibility: Visibility, + isInExplicitBody?: boolean, contentType?: string ): Refable { - const result = emitTypeWithSchemaEmitter(type, visibility, contentType); + const result = emitTypeWithSchemaEmitter(type, visibility, isInExplicitBody, contentType); switch (result.kind) { case "code": @@ -928,7 +929,7 @@ function createOAPIEmitter( } function getSchemaValue(type: Type, visibility: Visibility, contentType: string): OpenAPI3Schema { - const result = emitTypeWithSchemaEmitter(type, visibility, contentType); + const result = emitTypeWithSchemaEmitter(type, visibility, false, contentType); switch (result.kind) { case "code": @@ -949,6 +950,7 @@ function createOAPIEmitter( function emitTypeWithSchemaEmitter( type: Type, visibility: Visibility, + isInExplicitBody?: boolean, contentType?: string ): EmitEntity { if (!metadataInfo.isTransformed(type, visibility)) { @@ -956,17 +958,28 @@ function createOAPIEmitter( } contentType = contentType === "application/json" ? undefined : contentType; return schemaEmitter.emitType(type, { - referenceContext: { visibility, serviceNamespaceName: serviceNamespaceName, contentType }, + referenceContext: { + visibility, + serviceNamespaceName: serviceNamespaceName, + isInExplicitBody, + contentType, + }, }) as any; } function getSchemaForBody( type: Type, visibility: Visibility, + isExplicit: boolean, multipart: string | undefined ): any { const effectiveType = metadataInfo.getEffectivePayloadType(type, visibility); - return callSchemaEmitter(effectiveType, visibility, multipart ?? "application/json"); + return callSchemaEmitter( + effectiveType, + visibility, + isExplicit, + multipart ?? "application/json" + ); } function getParamPlaceholder(property: ModelProperty) { @@ -1044,6 +1057,7 @@ function createOAPIEmitter( : getSchemaForBody( body.type, visibility, + body.isExplicit, contentType.startsWith("multipart/") ? contentType : undefined ); if (schemaMap.has(contentType)) { @@ -1086,6 +1100,7 @@ function createOAPIEmitter( : getSchemaForBody( body.type, visibility, + body.isExplicit, contentType.startsWith("multipart/") ? contentType : undefined ); const contentEntry: any = { @@ -1402,7 +1417,7 @@ function createOAPIEmitter( const values = getKnownValues(program, typespecType as any); if (values) { return { - oneOf: [newTarget, callSchemaEmitter(values, Visibility.Read, "application/json")], + oneOf: [newTarget, callSchemaEmitter(values, Visibility.Read, false, "application/json")], }; } @@ -1421,6 +1436,7 @@ function createOAPIEmitter( const newType = callSchemaEmitter( encodeData.type, Visibility.Read, + false, "application/json" ) as OpenAPI3Schema; newTarget.type = newType.type; diff --git a/packages/openapi3/src/schema-emitter.ts b/packages/openapi3/src/schema-emitter.ts index 6456d8b589..84b4c0fd20 100644 --- a/packages/openapi3/src/schema-emitter.ts +++ b/packages/openapi3/src/schema-emitter.ts @@ -197,6 +197,10 @@ export class OpenAPI3SchemaEmitter extends TypeEmitter< return this.emitter.getContext().visibility ?? Visibility.Read; } + #isInExplicitBody(): boolean { + return this.emitter.getContext().isInExplicitBody; + } + #getContentType(): string { return this.emitter.getContext().contentType ?? "application/json"; } @@ -263,7 +267,8 @@ export class OpenAPI3SchemaEmitter extends TypeEmitter< // If the property has a type of 'never', don't include it in the schema continue; } - if (!this.#metadataInfo.isPayloadProperty(prop, visibility)) { + + if (!this.#metadataInfo.isPayloadProperty(prop, visibility, this.#isInExplicitBody())) { continue; } @@ -292,7 +297,7 @@ export class OpenAPI3SchemaEmitter extends TypeEmitter< // If the property has a type of 'never', don't include it in the schema continue; } - if (!this.#metadataInfo.isPayloadProperty(prop, visibility)) { + if (!this.#metadataInfo.isPayloadProperty(prop, visibility, this.#isInExplicitBody())) { continue; } const result = this.emitter.emitModelProperty(prop); diff --git a/packages/openapi3/test/return-types.test.ts b/packages/openapi3/test/return-types.test.ts index c84ae14d77..29fcf21832 100644 --- a/packages/openapi3/test/return-types.test.ts +++ b/packages/openapi3/test/return-types.test.ts @@ -1,6 +1,6 @@ import { expectDiagnosticEmpty, expectDiagnostics } from "@typespec/compiler/testing"; import { deepStrictEqual, ok, strictEqual } from "assert"; -import { describe, it } from "vitest"; +import { describe, expect, it } from "vitest"; import { checkFor, openApiFor } from "./test-host.js"; describe("openapi3: return types", () => { @@ -258,16 +258,6 @@ describe("openapi3: return types", () => { ); }); - describe("response model resolving to no property in the body produce no body", () => { - it.each(["{}", "{@header prop: string}", `{@visibility("none") prop: string}`])( - "%s", - async (body) => { - const res = await openApiFor(`op test(): ${body};`); - strictEqual(res.paths["/"].get.responses["200"].content, undefined); - } - ); - }); - it("produce additionalProperties schema if response is Record", async () => { const res = await openApiFor( ` @@ -421,6 +411,38 @@ describe("openapi3: return types", () => { ok(res.paths["/"].get.responses["200"]); }); + it("using @body ignore any metadata property underneath", async () => { + const res = await openApiFor(`@get op read(): { + @body body: { + #suppress "@typespec/http/metadata-ignored" + @header header: string, + #suppress "@typespec/http/metadata-ignored" + @query query: string, + #suppress "@typespec/http/metadata-ignored" + @statusCode code: 201, + } + };`); + expect(res.paths["/"].get.responses["200"].content["application/json"].schema).toEqual({ + type: "object", + properties: { + header: { type: "string" }, + query: { type: "string" }, + code: { type: "number", enum: [201] }, + }, + required: ["header", "query", "code"], + }); + }); + + describe("response model resolving to no property in the body produce no body", () => { + it.each(["{}", "{@header prop: string}", `{@visibility("none") prop: string}`])( + "%s", + async (body) => { + const res = await openApiFor(`op test(): ${body};`); + strictEqual(res.paths["/"].get.responses["200"].content, undefined); + } + ); + }); + describe("multiple content types", () => { it("handles multiple content types for the same status code", async () => { const res = await openApiFor( From 5339b348e61cfa394106cd054366735f6610726c Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Wed, 21 Feb 2024 13:16:51 -0800 Subject: [PATCH 06/26] fix test --- packages/http/src/metadata.ts | 6 +++--- packages/openapi3/src/openapi.ts | 1 + packages/openapi3/src/schema-emitter.ts | 2 ++ packages/openapi3/src/visibility-usage.ts | 3 +++ packages/rest/lib/resource.tsp | 14 +++++++------- 5 files changed, 16 insertions(+), 10 deletions(-) diff --git a/packages/http/src/metadata.ts b/packages/http/src/metadata.ts index 94a52e9941..3670dda538 100644 --- a/packages/http/src/metadata.ts +++ b/packages/http/src/metadata.ts @@ -537,8 +537,8 @@ export function createMetadataInfo(program: Program, options?: MetadataInfoOptio return true; } return ( - isPayloadProperty(property, visibility, /* keep shared */ true) !== - isPayloadProperty(property, canonicalVisibility, /*keep shared*/ true) + isPayloadProperty(property, visibility, undefined, /* keep shared */ true) !== + isPayloadProperty(property, canonicalVisibility, undefined, /*keep shared*/ true) ); } @@ -547,7 +547,7 @@ export function createMetadataInfo(program: Program, options?: MetadataInfoOptio return false; } for (const property of model.properties.values()) { - if (isPayloadProperty(property, visibility, /* keep shared */ true)) { + if (isPayloadProperty(property, visibility, undefined, /* keep shared */ true)) { return false; } } diff --git a/packages/openapi3/src/openapi.ts b/packages/openapi3/src/openapi.ts index 17028d1572..f591970b03 100644 --- a/packages/openapi3/src/openapi.ts +++ b/packages/openapi3/src/openapi.ts @@ -957,6 +957,7 @@ function createOAPIEmitter( visibility = Visibility.Read; } contentType = contentType === "application/json" ? undefined : contentType; + console.log("IS in ", (type as any).name, visibility, isInExplicitBody); return schemaEmitter.emitType(type, { referenceContext: { visibility, diff --git a/packages/openapi3/src/schema-emitter.ts b/packages/openapi3/src/schema-emitter.ts index 84b4c0fd20..2ee1640267 100644 --- a/packages/openapi3/src/schema-emitter.ts +++ b/packages/openapi3/src/schema-emitter.ts @@ -831,10 +831,12 @@ export class OpenAPI3SchemaEmitter extends TypeEmitter< } const usage = this.#visibilityUsage.getUsage(type); + const shouldAddSuffix = usage !== undefined && usage.size > 1; const visibility = this.#getVisibilityContext(); const fullName = name + (shouldAddSuffix ? getVisibilitySuffix(visibility, Visibility.Read) : ""); + console.log("Create model decl", name, this.#getVisibilityContext(), shouldAddSuffix, fullName); const decl = this.emitter.result.declaration(fullName, schema); diff --git a/packages/openapi3/src/visibility-usage.ts b/packages/openapi3/src/visibility-usage.ts index 6aa62b4e7d..07895eea17 100644 --- a/packages/openapi3/src/visibility-usage.ts +++ b/packages/openapi3/src/visibility-usage.ts @@ -172,6 +172,9 @@ function navigateReferencedTypes( callback(type, usage); navigateIterable(type.properties, usage, callback, visited); navigateIterable(type.derivedModels, usage, callback, visited); + if (type.baseModel) { + navigateReferencedTypes(type.baseModel, usage, callback, visited); + } if (type.indexer) { if (type.indexer.key.name === "integer") { navigateReferencedTypes(type.indexer.value, usage | Visibility.Item, callback, visited); diff --git a/packages/rest/lib/resource.tsp b/packages/rest/lib/resource.tsp index 7f3da7cf19..25f87ebd03 100644 --- a/packages/rest/lib/resource.tsp +++ b/packages/rest/lib/resource.tsp @@ -83,7 +83,7 @@ interface ResourceRead { @doc("Resource create operation completed successfully.") model ResourceCreatedResponse { ...CreatedResponse; - @body body: Resource; + @bodyRoot body: Resource; } /** @@ -104,7 +104,7 @@ interface ResourceCreateOrReplace { @createsOrReplacesResource(Resource) createOrReplace( ...ResourceParameters, - @body resource: ResourceCreateModel, + @bodyRoot resource: ResourceCreateModel, ): Resource | ResourceCreatedResponse | Error; } @@ -135,7 +135,7 @@ interface ResourceCreateOrUpdate { @createsOrUpdatesResource(Resource) createOrUpdate( ...ResourceParameters, - @body resource: ResourceCreateOrUpdateModel, + @bodyRoot resource: ResourceCreateOrUpdateModel, ): Resource | ResourceCreatedResponse | Error; } @@ -166,7 +166,7 @@ interface ResourceCreate { @createsResource(Resource) create( ...ResourceCollectionParameters, - @body resource: ResourceCreateModel, + @bodyRoot resource: ResourceCreateModel, ): Resource | ResourceCreatedResponse | Error; } @@ -190,7 +190,7 @@ interface ResourceUpdate { @updatesResource(Resource) update( ...ResourceParameters, - @body properties: ResourceCreateOrUpdateModel, + @bodyRoot properties: ResourceCreateOrUpdateModel, ): Resource | Error; } @@ -400,7 +400,7 @@ interface ExtensionResourceCreateOrUpdate, ...ResourceParameters, - @body resource: ResourceCreateOrUpdateModel, + @bodyRoot resource: ResourceCreateOrUpdateModel, ): Extension | ResourceCreatedResponse | Error; } @@ -423,7 +423,7 @@ interface ExtensionResourceCreate, - @body resource: ResourceCreateModel, + @bodyRoot resource: ResourceCreateModel, ): Extension | ResourceCreatedResponse | Error; } From 9682059b44a816f060727b032910f11733916f53 Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Wed, 21 Feb 2024 13:17:01 -0800 Subject: [PATCH 07/26] remove console --- packages/openapi3/src/schema-emitter.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/openapi3/src/schema-emitter.ts b/packages/openapi3/src/schema-emitter.ts index 2ee1640267..ca4fa8ed4c 100644 --- a/packages/openapi3/src/schema-emitter.ts +++ b/packages/openapi3/src/schema-emitter.ts @@ -836,7 +836,6 @@ export class OpenAPI3SchemaEmitter extends TypeEmitter< const visibility = this.#getVisibilityContext(); const fullName = name + (shouldAddSuffix ? getVisibilitySuffix(visibility, Visibility.Read) : ""); - console.log("Create model decl", name, this.#getVisibilityContext(), shouldAddSuffix, fullName); const decl = this.emitter.result.declaration(fullName, schema); From ac8306487706658bb757384b67214f518d801bff Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Wed, 21 Feb 2024 13:47:39 -0800 Subject: [PATCH 08/26] Progress --- packages/http/src/parameters.ts | 55 ++++--- packages/http/src/responses.ts | 6 +- packages/http/test/http-decorators.test.ts | 2 +- packages/http/test/parameters.test.ts | 176 +++++++++++++++++++++ packages/http/test/routes.test.ts | 168 -------------------- packages/openapi3/test/parameters.test.ts | 32 ++++ 6 files changed, 243 insertions(+), 196 deletions(-) create mode 100644 packages/http/test/parameters.test.ts diff --git a/packages/http/src/parameters.ts b/packages/http/src/parameters.ts index 2e2bb0cb64..f0ed6b0c27 100644 --- a/packages/http/src/parameters.ts +++ b/packages/http/src/parameters.ts @@ -7,6 +7,7 @@ import { Program, Type, } from "@typespec/compiler"; +import { validateBodyProperty } from "./body.js"; import { getContentTypes, isContentTypeHeader } from "./content-types.js"; import { getHeaderFieldOptions, @@ -14,9 +15,10 @@ import { getPathParamOptions, getQueryParamOptions, isBody, + isBodyRoot, } from "./decorators.js"; import { createDiagnostic } from "./lib.js"; -import { gatherMetadata, isMetadata, resolveRequestVisibility } from "./metadata.js"; +import { gatherMetadata, isMetadata, isVisible, resolveRequestVisibility } from "./metadata.js"; import { HttpOperation, HttpOperationParameter, @@ -76,8 +78,7 @@ function getOperationParametersForVerb( } const parameters: HttpOperationParameter[] = []; - let bodyType: Type | undefined; - let bodyParameter: ModelProperty | undefined; + let resolvedBody: ResolvedRequestBody | undefined; let contentTypes: string[] | undefined; for (const param of metadata) { @@ -86,12 +87,13 @@ function getOperationParametersForVerb( getPathParamOptions(program, param) ?? (isImplicitPathParam(param) && { type: "path", name: param.name }); const headerOptions = getHeaderFieldOptions(program, param); - const bodyParam = isBody(program, param); + const isBodyVal = isBody(program, param); + const isBodyRootVal = isBodyRoot(program, param); const defined = [ ["query", queryOptions], ["path", pathOptions], ["header", headerOptions], - ["body", bodyParam], + ["body", isBodyVal || isBodyRootVal], ].filter((x) => !!x[1]); if (defined.length >= 2) { diagnostics.add( @@ -130,26 +132,30 @@ function getOperationParametersForVerb( ...headerOptions, param, }); - } else if (bodyParam) { - if (bodyType === undefined) { - bodyParameter = param; - bodyType = param.type; + } else if (isBodyVal || isBodyRootVal) { + if (resolvedBody === undefined) { + if (isBodyVal) { + diagnostics.pipe(validateBodyProperty(program, param)); + } + resolvedBody = { parameter: param, type: param.type, isExplicit: isBodyVal }; } else { diagnostics.add(createDiagnostic({ code: "duplicate-body", target: param })); } } } - const bodyRoot = bodyParameter ? rootPropertyMap.get(bodyParameter) : undefined; + const bodyRoot = resolvedBody?.parameter + ? rootPropertyMap.get(resolvedBody.parameter) + : undefined; const unannotatedProperties = filterModelProperties( program, operation.parameters, - (p) => !metadata.has(p) && p !== bodyRoot + (p) => !metadata.has(p) && p !== bodyRoot && isVisible(program, p, visibility) ); if (unannotatedProperties.properties.size > 0) { - if (bodyType === undefined) { - bodyType = unannotatedProperties; + if (resolvedBody === undefined) { + resolvedBody = { type: unannotatedProperties, isExplicit: false }; } else { diagnostics.add( createDiagnostic({ @@ -160,9 +166,7 @@ function getOperationParametersForVerb( ); } } - const body = diagnostics.pipe( - computeHttpOperationBody(operation, bodyType, bodyParameter, contentTypes) - ); + const body = diagnostics.pipe(computeHttpOperationBody(operation, resolvedBody, contentTypes)); return diagnostics.wrap({ parameters, @@ -177,15 +181,20 @@ function getOperationParametersForVerb( }); } +interface ResolvedRequestBody { + readonly type: Type; + readonly isExplicit: boolean; + readonly parameter?: ModelProperty; +} + function computeHttpOperationBody( operation: Operation, - bodyType: Type | undefined, - bodyProperty: ModelProperty | undefined, + resolvedBody: ResolvedRequestBody | undefined, contentTypes: string[] | undefined ): [HttpOperationRequestBody | undefined, readonly Diagnostic[]] { contentTypes ??= []; const diagnostics: Diagnostic[] = []; - if (bodyType === undefined) { + if (resolvedBody === undefined) { if (contentTypes.length > 0) { diagnostics.push( createDiagnostic({ @@ -197,20 +206,18 @@ function computeHttpOperationBody( return [undefined, diagnostics]; } - if (contentTypes.includes("multipart/form-data") && bodyType.kind !== "Model") { + if (contentTypes.includes("multipart/form-data") && resolvedBody.type.kind !== "Model") { diagnostics.push( createDiagnostic({ code: "multipart-model", - target: bodyProperty ?? operation.parameters, + target: resolvedBody.parameter ?? operation.parameters, }) ); return [undefined, diagnostics]; } const body: HttpOperationRequestBody = { - type: bodyType, - isExplicit: false, - parameter: bodyProperty, + ...resolvedBody, contentTypes, }; return [body, diagnostics]; diff --git a/packages/http/src/responses.ts b/packages/http/src/responses.ts index cebd31be83..c1e08a0661 100644 --- a/packages/http/src/responses.ts +++ b/packages/http/src/responses.ts @@ -240,7 +240,7 @@ function getResponseHeaders( return responseHeaders; } -interface ResolvedBody { +interface ResolvedResponseBody { readonly type: Type; readonly isExplicit: boolean; } @@ -249,7 +249,7 @@ function getResponseBody( diagnostics: DiagnosticCollector, responseType: Type, metadata: Set -): ResolvedBody | undefined { +): ResolvedResponseBody | undefined { // non-model or intrinsic/array model -> response body is response type if (responseType.kind !== "Model" || isArrayModelType(program, responseType)) { return { type: responseType, isExplicit: false }; @@ -258,7 +258,7 @@ function getResponseBody( const duplicateTracker = new DuplicateTracker(); // look for explicit body - let resolvedBody: ResolvedBody | undefined; + let resolvedBody: ResolvedResponseBody | undefined; for (const property of metadata) { const isBodyVal = isBody(program, property); const isBodyRootVal = isBodyRoot(program, property); diff --git a/packages/http/test/http-decorators.test.ts b/packages/http/test/http-decorators.test.ts index c4d400e6ef..817750ffc3 100644 --- a/packages/http/test/http-decorators.test.ts +++ b/packages/http/test/http-decorators.test.ts @@ -513,7 +513,7 @@ describe("http: decorators", () => { ` model CustomUnauthorizedResponse { @statusCode _: 401; - @body body: UnauthorizedResponse; + @bodyRoot body: UnauthorizedResponse; } model Pet { diff --git a/packages/http/test/parameters.test.ts b/packages/http/test/parameters.test.ts new file mode 100644 index 0000000000..cff3fa0187 --- /dev/null +++ b/packages/http/test/parameters.test.ts @@ -0,0 +1,176 @@ +import { expectDiagnosticEmpty, expectDiagnostics } from "@typespec/compiler/testing"; +import { deepStrictEqual } from "assert"; +import { describe, it } from "vitest"; +import { compileOperations } from "./test-host.js"; + +it("emit diagnostic for parameters with multiple http request annotations", async () => { + const [_, diagnostics] = await compileOperations(` + @get op get(@body body: string, @path @query multiParam: string): string; + `); + + expectDiagnostics(diagnostics, { + code: "@typespec/http/operation-param-duplicate-type", + message: "Param multiParam has multiple types: [query, path]", + }); +}); + +it("emit diagnostic when there is an unannotated parameter and a @body param", async () => { + const [_, diagnostics] = await compileOperations(` + @get op get(param1: string, @body param2: string): string; + `); + + expectDiagnostics(diagnostics, { + code: "@typespec/http/duplicate-body", + message: + "Operation has a @body and an unannotated parameter. There can only be one representing the body", + }); +}); + +it("emit diagnostic when there are multiple @body param", async () => { + const [_, diagnostics] = await compileOperations(` + @get op get(@query select: string, @body param1: string, @body param2: string): string; + `); + + expectDiagnostics(diagnostics, { + code: "@typespec/http/duplicate-body", + message: "Operation has multiple @body parameters declared", + }); +}); + +it("emit error if using multipart/form-data contentType parameter with a body not being a model", async () => { + const [_, diagnostics] = await compileOperations(` + @get op get(@header contentType: "multipart/form-data", @body body: string | int32): string; + `); + + expectDiagnostics(diagnostics, { + code: "@typespec/http/multipart-model", + message: "Multipart request body must be a model.", + }); +}); + +it("emit warning if using contentType parameter without a body", async () => { + const [_, diagnostics] = await compileOperations(` + + @get op get(@header contentType: "image/png"): string; + `); + + expectDiagnostics(diagnostics, { + code: "@typespec/http/content-type-ignored", + message: "`Content-Type` header ignored because there is no body.", + }); +}); + +it("resolve body when defined with @body", async () => { + const [routes, diagnostics] = await compileOperations(` + @get op get(@query select: string, @body bodyParam: string): string; + `); + + expectDiagnosticEmpty(diagnostics); + deepStrictEqual(routes, [ + { + verb: "get", + path: "/", + params: { params: [{ type: "query", name: "select" }], body: "bodyParam" }, + }, + ]); +}); + +it("resolves single unannotated parameter as request body", async () => { + const [routes, diagnostics] = await compileOperations(` + @get op get(@query select: string, unannotatedBodyParam: string): string; + `); + + expectDiagnosticEmpty(diagnostics); + deepStrictEqual(routes, [ + { + verb: "get", + path: "/", + params: { + params: [{ type: "query", name: "select" }], + body: ["unannotatedBodyParam"], + }, + }, + ]); +}); + +it("resolves multiple unannotated parameters as request body", async () => { + const [routes, diagnostics] = await compileOperations(` + @get op get( + @query select: string, + unannotatedBodyParam1: string, + unannotatedBodyParam2: string): string; + `); + + expectDiagnosticEmpty(diagnostics); + deepStrictEqual(routes, [ + { + verb: "get", + path: "/", + params: { + params: [{ type: "query", name: "select" }], + body: ["unannotatedBodyParam1", "unannotatedBodyParam2"], + }, + }, + ]); +}); + +it("resolves unannotated path parameters that are included in the route path", async () => { + const [routes, diagnostics] = await compileOperations(` + @route("/test/{name}/sub/{foo}") + @get op get( + name: string, + foo: string + ): string; + + @route("/nested/{name}") + namespace A { + @route("sub") + namespace B { + @route("{bar}") + @get op get( + name: string, + bar: string + ): string; + } + } + `); + + expectDiagnosticEmpty(diagnostics); + deepStrictEqual(routes, [ + { + verb: "get", + path: "/test/{name}/sub/{foo}", + params: { + params: [ + { type: "path", name: "name" }, + { type: "path", name: "foo" }, + ], + body: undefined, + }, + }, + { + verb: "get", + path: "/nested/{name}/sub/{bar}", + params: { + params: [ + { type: "path", name: "name" }, + { type: "path", name: "bar" }, + ], + body: undefined, + }, + }, + ]); +}); + +describe("emit diagnostics when using metadata decorator in @body", () => { + it.each([ + ["@header", "id: string"], + ["@query", "id: string"], + ["@statusCode", "_: 200"], + ])("%s", async (dec, prop) => { + const [_, diagnostics] = await compileOperations( + `op read(@body explicit: {${dec} ${prop}, other: string}): void;` + ); + expectDiagnostics(diagnostics, { code: "@typespec/http/metadata-ignored" }); + }); +}); diff --git a/packages/http/test/routes.test.ts b/packages/http/test/routes.test.ts index 3f1bc0a41d..055c05688f 100644 --- a/packages/http/test/routes.test.ts +++ b/packages/http/test/routes.test.ts @@ -310,174 +310,6 @@ describe("http: routes", () => { strictEqual(diagnostics[1].message, `Duplicate operation "get2" routed at "get /test".`); }); - describe("operation parameters", () => { - it("emit diagnostic for parameters with multiple http request annotations", async () => { - const [_, diagnostics] = await compileOperations(` - @route("/test") - @get op get(@body body: string, @path @query multiParam: string): string; - `); - - expectDiagnostics(diagnostics, { - code: "@typespec/http/operation-param-duplicate-type", - message: "Param multiParam has multiple types: [query, path]", - }); - }); - - it("emit diagnostic when there is an unannotated parameter and a @body param", async () => { - const [_, diagnostics] = await compileOperations(` - @route("/test") - @get op get(param1: string, @body param2: string): string; - `); - - expectDiagnostics(diagnostics, { - code: "@typespec/http/duplicate-body", - message: - "Operation has a @body and an unannotated parameter. There can only be one representing the body", - }); - }); - - it("emit diagnostic when there are multiple @body param", async () => { - const [_, diagnostics] = await compileOperations(` - @route("/test") - @get op get(@query select: string, @body param1: string, @body param2: string): string; - `); - - expectDiagnostics(diagnostics, { - code: "@typespec/http/duplicate-body", - message: "Operation has multiple @body parameters declared", - }); - }); - - it("emit error if using multipart/form-data contentType parameter with a body not being a model", async () => { - const [_, diagnostics] = await compileOperations(` - @route("/test") - @get op get(@header contentType: "multipart/form-data", @body body: string | int32): string; - `); - - expectDiagnostics(diagnostics, { - code: "@typespec/http/multipart-model", - message: "Multipart request body must be a model.", - }); - }); - - it("emit warning if using contentType parameter without a body", async () => { - const [_, diagnostics] = await compileOperations(` - @route("/test") - @get op get(@header contentType: "image/png"): string; - `); - - expectDiagnostics(diagnostics, { - code: "@typespec/http/content-type-ignored", - message: "`Content-Type` header ignored because there is no body.", - }); - }); - - it("resolve body when defined with @body", async () => { - const [routes, diagnostics] = await compileOperations(` - @route("/test") - @get op get(@query select: string, @body bodyParam: string): string; - `); - - expectDiagnosticEmpty(diagnostics); - deepStrictEqual(routes, [ - { - verb: "get", - path: "/test", - params: { params: [{ type: "query", name: "select" }], body: "bodyParam" }, - }, - ]); - }); - - it("resolves single unannotated parameter as request body", async () => { - const [routes, diagnostics] = await compileOperations(` - @route("/test") - @get op get(@query select: string, unannotatedBodyParam: string): string; - `); - - expectDiagnosticEmpty(diagnostics); - deepStrictEqual(routes, [ - { - verb: "get", - path: "/test", - params: { - params: [{ type: "query", name: "select" }], - body: ["unannotatedBodyParam"], - }, - }, - ]); - }); - - it("resolves multiple unannotated parameters as request body", async () => { - const [routes, diagnostics] = await compileOperations(` - @route("/test") - @get op get( - @query select: string, - unannotatedBodyParam1: string, - unannotatedBodyParam2: string): string; - `); - - expectDiagnosticEmpty(diagnostics); - deepStrictEqual(routes, [ - { - verb: "get", - path: "/test", - params: { - params: [{ type: "query", name: "select" }], - body: ["unannotatedBodyParam1", "unannotatedBodyParam2"], - }, - }, - ]); - }); - - it("resolves unannotated path parameters that are included in the route path", async () => { - const [routes, diagnostics] = await compileOperations(` - @route("/test/{name}/sub/{foo}") - @get op get( - name: string, - foo: string - ): string; - - @route("/nested/{name}") - namespace A { - @route("sub") - namespace B { - @route("{bar}") - @get op get( - name: string, - bar: string - ): string; - } - } - `); - - expectDiagnosticEmpty(diagnostics); - deepStrictEqual(routes, [ - { - verb: "get", - path: "/test/{name}/sub/{foo}", - params: { - params: [ - { type: "path", name: "name" }, - { type: "path", name: "foo" }, - ], - body: undefined, - }, - }, - { - verb: "get", - path: "/nested/{name}/sub/{bar}", - params: { - params: [ - { type: "path", name: "name" }, - { type: "path", name: "bar" }, - ], - body: undefined, - }, - }, - ]); - }); - }); - describe("double @route", () => { it("emit diagnostic if specifying route twice on operation", async () => { const [_, diagnostics] = await compileOperations(` diff --git a/packages/openapi3/test/parameters.test.ts b/packages/openapi3/test/parameters.test.ts index 0d16300ce5..5c764fd58f 100644 --- a/packages/openapi3/test/parameters.test.ts +++ b/packages/openapi3/test/parameters.test.ts @@ -306,6 +306,38 @@ describe("openapi3: parameters", () => { strictEqual(res.paths["/"].post.requestBody, undefined); }); + it("using @body ignore any metadata property underneath", async () => { + const res = await openApiFor(`@get op read( + @body body: { + #suppress "@typespec/http/metadata-ignored" + @header header: string, + #suppress "@typespec/http/metadata-ignored" + @query query: string, + #suppress "@typespec/http/metadata-ignored" + @statusCode code: 201, + } + ): void;`); + expect(res.paths["/"].get.requestBody.content["application/json"].schema).toEqual({ + type: "object", + properties: { + header: { type: "string" }, + query: { type: "string" }, + code: { type: "number", enum: [201] }, + }, + required: ["header", "query", "code"], + }); + }); + + describe("request parameters resolving to no property in the body produce no body", () => { + it.each(["()", "(@header prop: string)", `(@visibility("none") prop: string)`])( + "%s", + async (params) => { + const res = await openApiFor(`op test${params}: void;`); + strictEqual(res.paths["/"].get.requestBody, undefined); + } + ); + }); + describe("content type parameter", () => { it("header named with 'Content-Type' gets resolved as content type for operation.", async () => { const res = await openApiFor( From 494d85d54c5c52c7f5cef61c7d2c82bcc277cf99 Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Wed, 21 Feb 2024 15:18:50 -0800 Subject: [PATCH 09/26] Use common logic to resolve body in request and response --- packages/http/src/body.ts | 104 +++++++++++++++++++++++- packages/http/src/parameters.ts | 57 +++---------- packages/http/src/responses.ts | 101 +++++------------------ packages/http/test/parameters.test.ts | 31 +++++-- packages/http/test/responses.test.ts | 16 ++++ packages/openapi3/src/openapi.ts | 1 - packages/openapi3/test/metadata.test.ts | 5 +- 7 files changed, 180 insertions(+), 135 deletions(-) diff --git a/packages/http/src/body.ts b/packages/http/src/body.ts index 4b1e13d5ef..5e3b8577b8 100644 --- a/packages/http/src/body.ts +++ b/packages/http/src/body.ts @@ -1,15 +1,115 @@ import { Diagnostic, + DuplicateTracker, ModelProperty, Program, + Type, createDiagnosticCollector, + filterModelProperties, + getDiscriminator, + isArrayModelType, navigateType, } from "@typespec/compiler"; -import { isHeader, isPathParam, isQueryParam, isStatusCode } from "./decorators.js"; +import { + isBody, + isBodyRoot, + isHeader, + isPathParam, + isQueryParam, + isStatusCode, +} from "./decorators.js"; import { createDiagnostic } from "./lib.js"; +import { Visibility, isVisible } from "./metadata.js"; + +export interface ResolvedBody { + readonly type: Type; + /** `true` if the body was specified with `@body` */ + readonly isExplicit: boolean; + /** If body is defined with `@body` or `@bodyRoot` this is the property */ + readonly property?: ModelProperty; +} + +export function resolveBody( + program: Program, + requestOrResponseType: Type, + metadata: Set, + rootPropertyMap: Map, + visibility: Visibility +): [ResolvedBody | undefined, readonly Diagnostic[]] { + const diagnostics = createDiagnosticCollector(); + // non-model or intrinsic/array model -> response body is response type + if (requestOrResponseType.kind !== "Model" || isArrayModelType(program, requestOrResponseType)) { + return diagnostics.wrap({ type: requestOrResponseType, isExplicit: false }); + } + + const duplicateTracker = new DuplicateTracker(); + + // look for explicit body + let resolvedBody: ResolvedBody | undefined; + for (const property of metadata) { + const isBodyVal = isBody(program, property); + const isBodyRootVal = isBodyRoot(program, property); + if (isBodyVal || isBodyRootVal) { + duplicateTracker.track("body", property); + if (resolvedBody === undefined) { + resolvedBody = { type: property.type, isExplicit: isBodyVal, property }; + if (isBodyVal) { + diagnostics.pipe(validateBodyProperty(program, property)); + } + } + } + } + for (const [_, items] of duplicateTracker.entries()) { + for (const prop of items) { + diagnostics.add( + createDiagnostic({ + code: "duplicate-body", + target: prop, + }) + ); + } + } + if (resolvedBody === undefined) { + // Special case for legacy purposes if the return type is an empty model with only @discriminator("xyz") + // Then we still want to return that object as it technically always has a body with that implicit property. + if ( + requestOrResponseType.derivedModels.length > 0 && + getDiscriminator(program, requestOrResponseType) + ) { + return diagnostics.wrap({ type: requestOrResponseType, isExplicit: false }); + } + if (requestOrResponseType.indexer) { + return diagnostics.wrap({ type: requestOrResponseType, isExplicit: false }); + } + } + + const bodyRoot = resolvedBody?.property ? rootPropertyMap.get(resolvedBody.property) : undefined; + + const unannotatedProperties = filterModelProperties( + program, + requestOrResponseType, + (p) => !metadata.has(p) && p !== bodyRoot && isVisible(program, p, visibility) + ); + + if (unannotatedProperties.properties.size > 0) { + if (resolvedBody === undefined) { + return diagnostics.wrap({ type: unannotatedProperties, isExplicit: false }); + } else { + diagnostics.add( + createDiagnostic({ + code: "duplicate-body", + messageId: "bodyAndUnannotated", + target: requestOrResponseType, + }) + ); + } + } + + return diagnostics.wrap(resolvedBody); +} /** Validate a property marked with `@body` */ -export function validateBodyProperty( +function validateBodyProperty( program: Program, property: ModelProperty ): [boolean, readonly Diagnostic[]] { diff --git a/packages/http/src/parameters.ts b/packages/http/src/parameters.ts index f0ed6b0c27..8a6c78aee5 100644 --- a/packages/http/src/parameters.ts +++ b/packages/http/src/parameters.ts @@ -1,13 +1,11 @@ import { createDiagnosticCollector, Diagnostic, - filterModelProperties, ModelProperty, Operation, Program, - Type, } from "@typespec/compiler"; -import { validateBodyProperty } from "./body.js"; +import { resolveBody, ResolvedBody } from "./body.js"; import { getContentTypes, isContentTypeHeader } from "./content-types.js"; import { getHeaderFieldOptions, @@ -18,7 +16,7 @@ import { isBodyRoot, } from "./decorators.js"; import { createDiagnostic } from "./lib.js"; -import { gatherMetadata, isMetadata, isVisible, resolveRequestVisibility } from "./metadata.js"; +import { gatherMetadata, isMetadata, resolveRequestVisibility } from "./metadata.js"; import { HttpOperation, HttpOperationParameter, @@ -78,7 +76,9 @@ function getOperationParametersForVerb( } const parameters: HttpOperationParameter[] = []; - let resolvedBody: ResolvedRequestBody | undefined; + const resolvedBody = diagnostics.pipe( + resolveBody(program, operation.parameters, metadata, rootPropertyMap, visibility) + ); let contentTypes: string[] | undefined; for (const param of metadata) { @@ -132,40 +132,9 @@ function getOperationParametersForVerb( ...headerOptions, param, }); - } else if (isBodyVal || isBodyRootVal) { - if (resolvedBody === undefined) { - if (isBodyVal) { - diagnostics.pipe(validateBodyProperty(program, param)); - } - resolvedBody = { parameter: param, type: param.type, isExplicit: isBodyVal }; - } else { - diagnostics.add(createDiagnostic({ code: "duplicate-body", target: param })); - } } } - const bodyRoot = resolvedBody?.parameter - ? rootPropertyMap.get(resolvedBody.parameter) - : undefined; - const unannotatedProperties = filterModelProperties( - program, - operation.parameters, - (p) => !metadata.has(p) && p !== bodyRoot && isVisible(program, p, visibility) - ); - - if (unannotatedProperties.properties.size > 0) { - if (resolvedBody === undefined) { - resolvedBody = { type: unannotatedProperties, isExplicit: false }; - } else { - diagnostics.add( - createDiagnostic({ - code: "duplicate-body", - messageId: "bodyAndUnannotated", - target: operation, - }) - ); - } - } const body = diagnostics.pipe(computeHttpOperationBody(operation, resolvedBody, contentTypes)); return diagnostics.wrap({ @@ -181,15 +150,9 @@ function getOperationParametersForVerb( }); } -interface ResolvedRequestBody { - readonly type: Type; - readonly isExplicit: boolean; - readonly parameter?: ModelProperty; -} - function computeHttpOperationBody( operation: Operation, - resolvedBody: ResolvedRequestBody | undefined, + resolvedBody: ResolvedBody | undefined, contentTypes: string[] | undefined ): [HttpOperationRequestBody | undefined, readonly Diagnostic[]] { contentTypes ??= []; @@ -210,15 +173,19 @@ function computeHttpOperationBody( diagnostics.push( createDiagnostic({ code: "multipart-model", - target: resolvedBody.parameter ?? operation.parameters, + target: resolvedBody.property ?? operation.parameters, }) ); return [undefined, diagnostics]; } const body: HttpOperationRequestBody = { - ...resolvedBody, + type: resolvedBody.type, + isExplicit: resolvedBody.isExplicit, contentTypes, }; + if (resolvedBody.property) { + body.parameter = resolvedBody.property; + } return [body, diagnostics]; } diff --git a/packages/http/src/responses.ts b/packages/http/src/responses.ts index c1e08a0661..18a05cf735 100644 --- a/packages/http/src/responses.ts +++ b/packages/http/src/responses.ts @@ -2,12 +2,9 @@ import { createDiagnosticCollector, Diagnostic, DiagnosticCollector, - DuplicateTracker, - getDiscriminator, getDoc, getErrorsDoc, getReturnsDoc, - isArrayModelType, isErrorModel, isNullType, isVoidType, @@ -16,21 +13,18 @@ import { Operation, Program, Type, - walkPropertiesInherited, } from "@typespec/compiler"; -import { validateBodyProperty } from "./body.js"; +import { resolveBody } from "./body.js"; import { getContentTypes, isContentTypeHeader } from "./content-types.js"; import { getHeaderFieldName, getStatusCodeDescription, getStatusCodesWithDiagnostics, - isBody, - isBodyRoot, isHeader, isStatusCode, } from "./decorators.js"; import { createDiagnostic, HttpStateKeys, reportDiagnostic } from "./lib.js"; -import { gatherMetadata, isApplicableMetadata, isVisible, Visibility } from "./metadata.js"; +import { gatherMetadata, Visibility } from "./metadata.js"; import { HttpOperationResponse, HttpStatusCodes, HttpStatusCodesEntry } from "./types.js"; /** @@ -92,7 +86,15 @@ function processResponseType( responses: ResponseIndex, responseType: Type ) { - const metadata = gatherMetadata(program, diagnostics, responseType, Visibility.Read); + const rootPropertyMap = new Map(); + const metadata = gatherMetadata( + program, + diagnostics, + responseType, + Visibility.Read, + undefined, + rootPropertyMap + ); // Get explicity defined status codes const statusCodes: HttpStatusCodes = diagnostics.pipe( @@ -106,7 +108,9 @@ function processResponseType( const headers = getResponseHeaders(program, metadata); // Get body - let resolvedBody = getResponseBody(program, diagnostics, responseType, metadata); + let resolvedBody = diagnostics.pipe( + resolveBody(program, responseType, metadata, rootPropertyMap, Visibility.Read) + ); // If there is no explicit status code, check if it should be 204 if (statusCodes.length === 0) { @@ -147,7 +151,14 @@ function processResponseType( }; if (resolvedBody !== undefined) { - response.responses.push({ body: { contentTypes: contentTypes, ...resolvedBody }, headers }); + response.responses.push({ + body: { + contentTypes: contentTypes, + type: resolvedBody.type, + isExplicit: resolvedBody.isExplicit, + }, + headers, + }); } else if (contentTypes.length > 0) { diagnostics.add( createDiagnostic({ @@ -240,74 +251,6 @@ function getResponseHeaders( return responseHeaders; } -interface ResolvedResponseBody { - readonly type: Type; - readonly isExplicit: boolean; -} -function getResponseBody( - program: Program, - diagnostics: DiagnosticCollector, - responseType: Type, - metadata: Set -): ResolvedResponseBody | undefined { - // non-model or intrinsic/array model -> response body is response type - if (responseType.kind !== "Model" || isArrayModelType(program, responseType)) { - return { type: responseType, isExplicit: false }; - } - - const duplicateTracker = new DuplicateTracker(); - - // look for explicit body - let resolvedBody: ResolvedResponseBody | undefined; - for (const property of metadata) { - const isBodyVal = isBody(program, property); - const isBodyRootVal = isBodyRoot(program, property); - if (isBodyVal || isBodyRootVal) { - duplicateTracker.track("body", property); - if (resolvedBody === undefined) { - resolvedBody = { type: property.type, isExplicit: isBodyVal }; - if (isBodyVal) { - diagnostics.pipe(validateBodyProperty(program, property)); - } - } - } - } - for (const [_, items] of duplicateTracker.entries()) { - for (const prop of items) { - diagnostics.add( - createDiagnostic({ - code: "duplicate-body", - target: prop, - }) - ); - } - } - if (resolvedBody) { - return resolvedBody; - } - - // Special case for legacy purposes if the return type is an empty model with only @discriminator("xyz") - // Then we still want to return that object as it technically always has a body with that implicit property. - if (responseType.derivedModels.length > 0 && getDiscriminator(program, responseType)) { - return { type: responseType, isExplicit: false }; - } - if (responseType.indexer) { - return { type: responseType, isExplicit: false }; - } - - for (const property of walkPropertiesInherited(responseType)) { - if ( - !isApplicableMetadata(program, property, Visibility.Read) && - isVisible(program, property, Visibility.Read) - ) { - return { type: responseType, isExplicit: false }; - } - } - - // Otherwise, there is no body - return undefined; -} - function getResponseDescription( program: Program, operation: Operation, diff --git a/packages/http/test/parameters.test.ts b/packages/http/test/parameters.test.ts index cff3fa0187..9ccb6ab3d7 100644 --- a/packages/http/test/parameters.test.ts +++ b/packages/http/test/parameters.test.ts @@ -1,7 +1,7 @@ import { expectDiagnosticEmpty, expectDiagnostics } from "@typespec/compiler/testing"; import { deepStrictEqual } from "assert"; import { describe, it } from "vitest"; -import { compileOperations } from "./test-host.js"; +import { compileOperations, getRoutesFor } from "./test-host.js"; it("emit diagnostic for parameters with multiple http request annotations", async () => { const [_, diagnostics] = await compileOperations(` @@ -14,6 +14,21 @@ it("emit diagnostic for parameters with multiple http request annotations", asyn }); }); +it("allows a deeply nested @body", async () => { + const routes = await getRoutesFor(` + op get(data: {nested: { @body param2: string }}): string; + `); + + deepStrictEqual(routes, [{ verb: "post", params: [], path: "/" }]); +}); +it("allows a deeply nested @bodyRoot", async () => { + const routes = await getRoutesFor(` + op get(data: {nested: { @bodyRoot param2: string }}): string; + `); + + deepStrictEqual(routes, [{ verb: "post", params: [], path: "/" }]); +}); + it("emit diagnostic when there is an unannotated parameter and a @body param", async () => { const [_, diagnostics] = await compileOperations(` @get op get(param1: string, @body param2: string): string; @@ -31,10 +46,16 @@ it("emit diagnostic when there are multiple @body param", async () => { @get op get(@query select: string, @body param1: string, @body param2: string): string; `); - expectDiagnostics(diagnostics, { - code: "@typespec/http/duplicate-body", - message: "Operation has multiple @body parameters declared", - }); + expectDiagnostics(diagnostics, [ + { + code: "@typespec/http/duplicate-body", + message: "Operation has multiple @body parameters declared", + }, + { + code: "@typespec/http/duplicate-body", + message: "Operation has multiple @body parameters declared", + }, + ]); }); it("emit error if using multipart/form-data contentType parameter with a body not being a model", async () => { diff --git a/packages/http/test/responses.test.ts b/packages/http/test/responses.test.ts index 89d3390c32..c8e7505a79 100644 --- a/packages/http/test/responses.test.ts +++ b/packages/http/test/responses.test.ts @@ -35,6 +35,22 @@ describe("body resolution", () => { ]); }); + it("allows a deeply nested @body", async () => { + const [_, diagnostics] = await compileOperations(` + op get(): {data: {nested: { @body param2: string }}}; + `); + + expectDiagnosticEmpty(diagnostics); + }); + + it("allows a deeply nested @bodyRoot", async () => { + const [_, diagnostics] = await compileOperations(` + op get(): {data: {nested: { @bodyRoot param2: string }}}; + `); + + expectDiagnosticEmpty(diagnostics); + }); + describe("emit diagnostics when using metadata decorator in @body", () => { it.each([ ["@header", "id: string"], diff --git a/packages/openapi3/src/openapi.ts b/packages/openapi3/src/openapi.ts index f591970b03..17028d1572 100644 --- a/packages/openapi3/src/openapi.ts +++ b/packages/openapi3/src/openapi.ts @@ -957,7 +957,6 @@ function createOAPIEmitter( visibility = Visibility.Read; } contentType = contentType === "application/json" ? undefined : contentType; - console.log("IS in ", (type as any).name, visibility, isInExplicitBody); return schemaEmitter.emitType(type, { referenceContext: { visibility, diff --git a/packages/openapi3/test/metadata.test.ts b/packages/openapi3/test/metadata.test.ts index 68e592ec15..0f99b18f07 100644 --- a/packages/openapi3/test/metadata.test.ts +++ b/packages/openapi3/test/metadata.test.ts @@ -612,7 +612,7 @@ describe("openapi3: metadata", () => { @header h: string; } @route("/single") @get op single(...Parameters): string; - @route("/batch") @get op batch(...Body): string; + @route("/batch") @get op batch(@bodyRoot _: Parameters[]): string; ` ); deepStrictEqual(res.paths, { @@ -643,7 +643,6 @@ describe("openapi3: metadata", () => { }, }, requestBody: { - description: "The body type of the operation request or response.", required: true, content: { "application/json": { @@ -766,7 +765,7 @@ describe("openapi3: metadata", () => { @path p: string; @header h: string; } - @route("/batch") @post op batch(@body body?: Parameters[]): string; + @route("/batch") @post op batch(@bodyRoot body?: Parameters[]): string; ` ); deepStrictEqual(res.paths, { From 96f488a74d3001535fa1bcb8a9330d17b2ef811f Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Wed, 21 Feb 2024 15:37:38 -0800 Subject: [PATCH 10/26] Handle body ignore --- packages/http/src/metadata.ts | 4 ++- packages/openapi3/test/parameters.test.ts | 35 +++++++++++++++++++++ packages/openapi3/test/return-types.test.ts | 35 +++++++++++++++++++++ 3 files changed, 73 insertions(+), 1 deletion(-) diff --git a/packages/http/src/metadata.ts b/packages/http/src/metadata.ts index 3670dda538..17919d5a63 100644 --- a/packages/http/src/metadata.ts +++ b/packages/http/src/metadata.ts @@ -17,6 +17,7 @@ import { import { includeInapplicableMetadataInPayload, isBody, + isBodyIgnore, isBodyRoot, isHeader, isPathParam, @@ -382,6 +383,7 @@ export interface MetadataInfo { * * When the type of a property is emptied by visibility, the property * itself is also removed. + * @deprecated This produce inconsistent behaviors and should be avoided. */ isEmptied(type: Type | undefined, visibility: Visibility): boolean; @@ -572,7 +574,7 @@ export function createMetadataInfo(program: Program, options?: MetadataInfoOptio ): boolean { if ( !inExplicitBody && - (isEmptied(property.type, visibility) || + (isBodyIgnore(program, property) || isApplicableMetadata(program, property, visibility) || (isMetadata(program, property) && !includeInapplicableMetadataInPayload(program, property))) ) { diff --git a/packages/openapi3/test/parameters.test.ts b/packages/openapi3/test/parameters.test.ts index 5c764fd58f..4da9eec046 100644 --- a/packages/openapi3/test/parameters.test.ts +++ b/packages/openapi3/test/parameters.test.ts @@ -338,6 +338,41 @@ describe("openapi3: parameters", () => { ); }); + it("property in body with only metadata properties should still be included", async () => { + const res = await openApiFor(`op read( + headers: { + @header header1: string; + @header header2: string; + }; + name: string; + ): void;`); + expect(res.paths["/"].post.requestBody.content["application/json"].schema).toEqual({ + type: "object", + properties: { + headers: { type: "object" }, + name: { type: "string" }, + }, + required: ["headers", "name"], + }); + }); + + it("property in body with only metadata properties and @bodyIgnore should not be included", async () => { + const res = await openApiFor(`op read( + @bodyIgnore headers: { + @header header1: string; + @header header2: string; + }; + name: string; + ): void;`); + expect(res.paths["/"].post.requestBody.content["application/json"].schema).toEqual({ + type: "object", + properties: { + name: { type: "string" }, + }, + required: ["name"], + }); + }); + describe("content type parameter", () => { it("header named with 'Content-Type' gets resolved as content type for operation.", async () => { const res = await openApiFor( diff --git a/packages/openapi3/test/return-types.test.ts b/packages/openapi3/test/return-types.test.ts index 29fcf21832..00f4a0add5 100644 --- a/packages/openapi3/test/return-types.test.ts +++ b/packages/openapi3/test/return-types.test.ts @@ -443,6 +443,41 @@ describe("openapi3: return types", () => { ); }); + it("property in body with only metadata properties should still be included", async () => { + const res = await openApiFor(`op read(): { + headers: { + @header header1: string; + @header header2: string; + }; + name: string; + };`); + expect(res.paths["/"].get.responses["200"].content["application/json"].schema).toEqual({ + type: "object", + properties: { + headers: { type: "object" }, + name: { type: "string" }, + }, + required: ["headers", "name"], + }); + }); + + it("property in body with only metadata properties and @bodyIgnore should not be included", async () => { + const res = await openApiFor(`op read(): { + @bodyIgnore headers: { + @header header1: string; + @header header2: string; + }; + name: string; + };`); + expect(res.paths["/"].get.responses["200"].content["application/json"].schema).toEqual({ + type: "object", + properties: { + name: { type: "string" }, + }, + required: ["name"], + }); + }); + describe("multiple content types", () => { it("handles multiple content types for the same status code", async () => { const res = await openApiFor( From 25d99ccd1bf2a80f19da76dfa2fe5d6d0dfd0978 Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Wed, 21 Feb 2024 16:50:21 -0800 Subject: [PATCH 11/26] update emptied test --- packages/openapi3/test/metadata.test.ts | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/packages/openapi3/test/metadata.test.ts b/packages/openapi3/test/metadata.test.ts index 0f99b18f07..e9d6d1a54d 100644 --- a/packages/openapi3/test/metadata.test.ts +++ b/packages/openapi3/test/metadata.test.ts @@ -925,7 +925,7 @@ describe("openapi3: metadata", () => { }); }); - it("supports nested metadata and removes emptied properties", async () => { + it("supports nested metadata", async () => { const res = await openApiFor( ` model Pet { @@ -1015,15 +1015,33 @@ describe("openapi3: metadata", () => { PetCreate: { type: "object", properties: { + headers: { + type: "object", + properties: { + moreHeaders: { + type: "object", + }, + }, + required: ["moreHeaders"], + }, name: { type: "string", }, }, - required: ["name"], + required: ["headers", "name"], }, Pet: { type: "object", properties: { + headers: { + type: "object", + properties: { + moreHeaders: { + type: "object", + }, + }, + required: ["moreHeaders"], + }, id: { type: "string", }, @@ -1031,7 +1049,7 @@ describe("openapi3: metadata", () => { type: "string", }, }, - required: ["id", "name"], + required: ["headers", "id", "name"], }, }); }); From 19dd91bbd028baac709988971a407152803e56e3 Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Wed, 21 Feb 2024 17:50:16 -0800 Subject: [PATCH 12/26] Fix --- packages/http/src/body.ts | 5 +--- packages/http/src/decorators.ts | 17 +++++++++++-- packages/http/src/responses.ts | 3 +-- packages/http/src/types.ts | 11 ++++++-- packages/openapi3/src/openapi.ts | 25 +++++++++++++------ packages/openapi3/src/schema-emitter.ts | 12 ++++++--- packages/openapi3/test/return-types.test.ts | 18 +++++++++++++ .../@typespec/openapi3/openapi.yaml | 2 +- 8 files changed, 71 insertions(+), 22 deletions(-) diff --git a/packages/http/src/body.ts b/packages/http/src/body.ts index 5e3b8577b8..6b2ae6e874 100644 --- a/packages/http/src/body.ts +++ b/packages/http/src/body.ts @@ -53,9 +53,6 @@ export function resolveBody( duplicateTracker.track("body", property); if (resolvedBody === undefined) { resolvedBody = { type: property.type, isExplicit: isBodyVal, property }; - if (isBodyVal) { - diagnostics.pipe(validateBodyProperty(program, property)); - } } } } @@ -109,7 +106,7 @@ export function resolveBody( } /** Validate a property marked with `@body` */ -function validateBodyProperty( +export function validateBodyProperty( program: Program, property: ModelProperty ): [boolean, readonly Diagnostic[]] { diff --git a/packages/http/src/decorators.ts b/packages/http/src/decorators.ts index 90ac87a897..0819c4bc4a 100644 --- a/packages/http/src/decorators.ts +++ b/packages/http/src/decorators.ts @@ -21,6 +21,7 @@ import { validateDecoratorTarget, validateDecoratorUniqueOnNode, } from "@typespec/compiler"; +import { validateBodyProperty } from "./body.js"; import { HttpStateKeys, createDiagnostic, reportDiagnostic } from "./lib.js"; import { setRoute, setSharedRoute } from "./route.js"; import { getStatusCodesFromType } from "./status-codes.js"; @@ -167,7 +168,11 @@ export function isPathParam(program: Program, entity: Type) { } export function $body(context: DecoratorContext, entity: ModelProperty) { - context.program.stateSet(HttpStateKeys.body).add(entity); + const [valid, diagnostics] = validateBodyProperty(context.program, entity); + if (!valid) { + context.program.reportDiagnostics(diagnostics); + } + context.program.stateMap(HttpStateKeys.body).set(entity, { valid }); } export function $bodyRoot(context: DecoratorContext, entity: ModelProperty) { @@ -179,7 +184,15 @@ export function $bodyIgnore(context: DecoratorContext, entity: ModelProperty) { } export function isBody(program: Program, entity: Type): boolean { - return program.stateSet(HttpStateKeys.body).has(entity); + return program.stateMap(HttpStateKeys.body).has(entity); +} + +/** + * Return if a property marked with `@body` is valid(Doesn't contain any metadata property). + * Returns `false` if it contains a metadata property. + */ +export function isExplicitBodyValid(program: Program, entity: ModelProperty): boolean { + return program.stateMap(HttpStateKeys.body).get(entity).valid; } export function isBodyRoot(program: Program, entity: ModelProperty): boolean { diff --git a/packages/http/src/responses.ts b/packages/http/src/responses.ts index 18a05cf735..37f04a78c3 100644 --- a/packages/http/src/responses.ts +++ b/packages/http/src/responses.ts @@ -154,8 +154,7 @@ function processResponseType( response.responses.push({ body: { contentTypes: contentTypes, - type: resolvedBody.type, - isExplicit: resolvedBody.isExplicit, + ...resolvedBody, }, headers, }); diff --git a/packages/http/src/types.ts b/packages/http/src/types.ts index 9ffbe9e8d4..36d0b5aa20 100644 --- a/packages/http/src/types.ts +++ b/packages/http/src/types.ts @@ -259,11 +259,18 @@ export type HttpOperationParameter = ( */ export interface HttpOperationRequestBody extends HttpOperationBody { /** - * If the body was explicitly set as a property. Correspond to the property with `@body` + * If the body was explicitly set as a property. Correspond to the property with `@body` or `@bodyRoot` */ parameter?: ModelProperty; } +export interface HttpOperationResponseBody extends HttpOperationBody { + /** + * If the body was explicitly set as a property. Correspond to the property with `@body` or `@bodyRoot` + */ + readonly property?: ModelProperty; +} + export interface HttpOperationParameters { parameters: HttpOperationParameter[]; @@ -369,7 +376,7 @@ export interface HttpOperationResponse { export interface HttpOperationResponseContent { headers?: Record; - body?: HttpOperationBody; + body?: HttpOperationResponseBody; } export interface HttpOperationBody { diff --git a/packages/openapi3/src/openapi.ts b/packages/openapi3/src/openapi.ts index 17028d1572..db397763e1 100644 --- a/packages/openapi3/src/openapi.ts +++ b/packages/openapi3/src/openapi.ts @@ -64,6 +64,7 @@ import { HttpStatusCodesEntry, HttpVerb, isContentTypeHeader, + isExplicitBodyValid, isOverloadSameEndpoint, MetadataInfo, QueryParameterOptions, @@ -873,7 +874,12 @@ function createOAPIEmitter( const isBinary = isBinaryPayload(data.body.type, contentType); const schema = isBinary ? { type: "string", format: "binary" } - : getSchemaForBody(data.body.type, Visibility.Read, data.body.isExplicit, undefined); + : getSchemaForBody( + data.body.type, + Visibility.Read, + data.body.isExplicit && !isExplicitBodyValid(program, data.body.property!), + undefined + ); if (schemaMap.has(contentType)) { schemaMap.get(contentType)!.push(schema); } else { @@ -906,10 +912,15 @@ function createOAPIEmitter( function callSchemaEmitter( type: Type, visibility: Visibility, - isInExplicitBody?: boolean, + ignoreMetadataAnnotations?: boolean, contentType?: string ): Refable { - const result = emitTypeWithSchemaEmitter(type, visibility, isInExplicitBody, contentType); + const result = emitTypeWithSchemaEmitter( + type, + visibility, + ignoreMetadataAnnotations, + contentType + ); switch (result.kind) { case "code": @@ -950,7 +961,7 @@ function createOAPIEmitter( function emitTypeWithSchemaEmitter( type: Type, visibility: Visibility, - isInExplicitBody?: boolean, + ignoreMetadataAnnotations?: boolean, contentType?: string ): EmitEntity { if (!metadataInfo.isTransformed(type, visibility)) { @@ -961,7 +972,7 @@ function createOAPIEmitter( referenceContext: { visibility, serviceNamespaceName: serviceNamespaceName, - isInExplicitBody, + ignoreMetadataAnnotations: ignoreMetadataAnnotations ?? false, contentType, }, }) as any; @@ -1057,7 +1068,7 @@ function createOAPIEmitter( : getSchemaForBody( body.type, visibility, - body.isExplicit, + body.isExplicit && !isExplicitBodyValid(program, body.parameter!), contentType.startsWith("multipart/") ? contentType : undefined ); if (schemaMap.has(contentType)) { @@ -1100,7 +1111,7 @@ function createOAPIEmitter( : getSchemaForBody( body.type, visibility, - body.isExplicit, + body.isExplicit && !isExplicitBodyValid(program, body.parameter!), contentType.startsWith("multipart/") ? contentType : undefined ); const contentEntry: any = { diff --git a/packages/openapi3/src/schema-emitter.ts b/packages/openapi3/src/schema-emitter.ts index ca4fa8ed4c..e5d6857e8e 100644 --- a/packages/openapi3/src/schema-emitter.ts +++ b/packages/openapi3/src/schema-emitter.ts @@ -197,8 +197,8 @@ export class OpenAPI3SchemaEmitter extends TypeEmitter< return this.emitter.getContext().visibility ?? Visibility.Read; } - #isInExplicitBody(): boolean { - return this.emitter.getContext().isInExplicitBody; + #ignoreMetadataAnnotations(): boolean { + return this.emitter.getContext().ignoreMetadataAnnotations; } #getContentType(): string { @@ -268,7 +268,9 @@ export class OpenAPI3SchemaEmitter extends TypeEmitter< continue; } - if (!this.#metadataInfo.isPayloadProperty(prop, visibility, this.#isInExplicitBody())) { + if ( + !this.#metadataInfo.isPayloadProperty(prop, visibility, this.#ignoreMetadataAnnotations()) + ) { continue; } @@ -297,7 +299,9 @@ export class OpenAPI3SchemaEmitter extends TypeEmitter< // If the property has a type of 'never', don't include it in the schema continue; } - if (!this.#metadataInfo.isPayloadProperty(prop, visibility, this.#isInExplicitBody())) { + if ( + !this.#metadataInfo.isPayloadProperty(prop, visibility, this.#ignoreMetadataAnnotations()) + ) { continue; } const result = this.emitter.emitModelProperty(prop); diff --git a/packages/openapi3/test/return-types.test.ts b/packages/openapi3/test/return-types.test.ts index 00f4a0add5..11ad9c854c 100644 --- a/packages/openapi3/test/return-types.test.ts +++ b/packages/openapi3/test/return-types.test.ts @@ -4,6 +4,24 @@ import { describe, expect, it } from "vitest"; import { checkFor, openApiFor } from "./test-host.js"; describe("openapi3: return types", () => { + it("model used with @body and without shouldn't conflict if it contains no metadata", async () => { + const res = await openApiFor( + ` + model Foo { + name: string; + } + @route("c1") op c1(): Foo; + @route("c2") op c2(): {@body _: Foo}; + ` + ); + deepStrictEqual(res.paths["/c1"].get.responses["200"].content["application/json"].schema, { + $ref: "#/components/schemas/Foo", + }); + deepStrictEqual(res.paths["/c2"].get.responses["200"].content["application/json"].schema, { + $ref: "#/components/schemas/Foo", + }); + }); + it("defines responses with response headers", async () => { const res = await openApiFor( ` diff --git a/packages/samples/test/output/documentation/@typespec/openapi3/openapi.yaml b/packages/samples/test/output/documentation/@typespec/openapi3/openapi.yaml index 1b62c51d42..599b027809 100644 --- a/packages/samples/test/output/documentation/@typespec/openapi3/openapi.yaml +++ b/packages/samples/test/output/documentation/@typespec/openapi3/openapi.yaml @@ -73,7 +73,7 @@ paths: schema: $ref: '#/components/schemas/Resp' '404': - description: The server cannot find the requested resource. + description: Not found content: application/json: schema: From 8ea4f2b2412e32ad941075c7e9e1d03ca85d167f Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Wed, 21 Feb 2024 18:57:49 -0800 Subject: [PATCH 13/26] docs regen --- docs/libraries/http/reference/decorators.md | 68 +++++++++++++++++++- docs/libraries/http/reference/index.mdx | 2 + packages/http/README.md | 70 ++++++++++++++++++++- 3 files changed, 138 insertions(+), 2 deletions(-) diff --git a/docs/libraries/http/reference/decorators.md b/docs/libraries/http/reference/decorators.md index 29d75b1c02..8349142a76 100644 --- a/docs/libraries/http/reference/decorators.md +++ b/docs/libraries/http/reference/decorators.md @@ -10,7 +10,10 @@ toc_max_heading_level: 3 ### `@body` {#@TypeSpec.Http.body} -Explicitly specify that this property is to be set as the body +Explicitly specify that this property type will be exactly the HTTP body. + +This means that any properties under `@body` cannot be marked as headers, query parameters, or path parameters. +If wanting to change the resolution of the body but still mix parameters, use `@bodyRoot`. ```typespec @TypeSpec.Http.body @@ -33,6 +36,69 @@ op download(): { }; ``` +### `@bodyIgnore` {#@TypeSpec.Http.bodyIgnore} + +Specify that this property shouldn't be included in the HTTP body. +This can be useful when bundling metadata together that would result in an empty property to be included in the body. + +```typespec +@TypeSpec.Http.bodyIgnore +``` + +#### Target + +`ModelProperty` + +#### Parameters + +None + +#### Examples + +```typespec +op upload( + name: string, + @bodyIgnore headers: { + @header id: string; + }, +): void; +``` + +### `@bodyRoot` {#@TypeSpec.Http.bodyRoot} + +Specify that the body resolution should be resolved from that property. +By default the body is resolved by including all properties in the operation request/response that are not metadata. +This allows to nest the body in a property while still allowing to use headers, query parameters, and path parameters in the same model. + +```typespec +@TypeSpec.Http.bodyRoot +``` + +#### Target + +`ModelProperty` + +#### Parameters + +None + +#### Examples + +```typespec +op upload( + @bodyRoot user: { + name: string; + @header id: string; + }, +): void; +op download(): { + @bodyRoot user: { + name: string; + @header id: string; + }; +}; +``` + ### `@delete` {#@TypeSpec.Http.delete} Specify the HTTP verb for the target operation to be `DELETE`. diff --git a/docs/libraries/http/reference/index.mdx b/docs/libraries/http/reference/index.mdx index 9c6cf9c172..f910e62a29 100644 --- a/docs/libraries/http/reference/index.mdx +++ b/docs/libraries/http/reference/index.mdx @@ -36,6 +36,8 @@ npm install --save-peer @typespec/http ### Decorators - [`@body`](./decorators.md#@TypeSpec.Http.body) +- [`@bodyIgnore`](./decorators.md#@TypeSpec.Http.bodyIgnore) +- [`@bodyRoot`](./decorators.md#@TypeSpec.Http.bodyRoot) - [`@delete`](./decorators.md#@TypeSpec.Http.delete) - [`@get`](./decorators.md#@TypeSpec.Http.get) - [`@head`](./decorators.md#@TypeSpec.Http.head) diff --git a/packages/http/README.md b/packages/http/README.md index b35b6e2ea3..1d70e93c66 100644 --- a/packages/http/README.md +++ b/packages/http/README.md @@ -37,6 +37,8 @@ Available ruleSets: ### TypeSpec.Http - [`@body`](#@body) +- [`@bodyIgnore`](#@bodyignore) +- [`@bodyRoot`](#@bodyroot) - [`@delete`](#@delete) - [`@get`](#@get) - [`@head`](#@head) @@ -55,7 +57,10 @@ Available ruleSets: #### `@body` -Explicitly specify that this property is to be set as the body +Explicitly specify that this property type will be exactly the HTTP body. + +This means that any properties under `@body` cannot be marked as headers, query parameters, or path parameters. +If wanting to change the resolution of the body but still mix parameters, use `@bodyRoot`. ```typespec @TypeSpec.Http.body @@ -78,6 +83,69 @@ op download(): { }; ``` +#### `@bodyIgnore` + +Specify that this property shouldn't be included in the HTTP body. +This can be useful when bundling metadata together that would result in an empty property to be included in the body. + +```typespec +@TypeSpec.Http.bodyIgnore +``` + +##### Target + +`ModelProperty` + +##### Parameters + +None + +##### Examples + +```typespec +op upload( + name: string, + @bodyIgnore headers: { + @header id: string; + }, +): void; +``` + +#### `@bodyRoot` + +Specify that the body resolution should be resolved from that property. +By default the body is resolved by including all properties in the operation request/response that are not metadata. +This allows to nest the body in a property while still allowing to use headers, query parameters, and path parameters in the same model. + +```typespec +@TypeSpec.Http.bodyRoot +``` + +##### Target + +`ModelProperty` + +##### Parameters + +None + +##### Examples + +```typespec +op upload( + @bodyRoot user: { + name: string; + @header id: string; + }, +): void; +op download(): { + @bodyRoot user: { + name: string; + @header id: string; + }; +}; +``` + #### `@delete` Specify the HTTP verb for the target operation to be `DELETE`. From 937353ad55a8822294c5ec50d38932336dd00424 Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Thu, 22 Feb 2024 09:28:54 -0800 Subject: [PATCH 14/26] fix --- packages/http/src/body.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/http/src/body.ts b/packages/http/src/body.ts index 6b2ae6e874..6a4c97de44 100644 --- a/packages/http/src/body.ts +++ b/packages/http/src/body.ts @@ -67,6 +67,11 @@ export function resolveBody( } } if (resolvedBody === undefined) { + // Special case if the model as a parent model then we'll return an empty object as this is assumed to be a nominal type. + // Special Case if the model has an indexer then it means it can return props so cannot be void. + if (requestOrResponseType.baseModel || requestOrResponseType.indexer) { + return diagnostics.wrap({ type: requestOrResponseType, isExplicit: false }); + } // Special case for legacy purposes if the return type is an empty model with only @discriminator("xyz") // Then we still want to return that object as it technically always has a body with that implicit property. if ( @@ -75,9 +80,6 @@ export function resolveBody( ) { return diagnostics.wrap({ type: requestOrResponseType, isExplicit: false }); } - if (requestOrResponseType.indexer) { - return diagnostics.wrap({ type: requestOrResponseType, isExplicit: false }); - } } const bodyRoot = resolvedBody?.property ? rootPropertyMap.get(resolvedBody.property) : undefined; From e23e9f3fe7c221eb8f36eadc2b4fa1fd104d50e4 Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Thu, 22 Feb 2024 11:26:45 -0800 Subject: [PATCH 15/26] fix --- packages/openapi3/tsconfig.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/openapi3/tsconfig.json b/packages/openapi3/tsconfig.json index 58a4b12168..f57833aa3d 100644 --- a/packages/openapi3/tsconfig.json +++ b/packages/openapi3/tsconfig.json @@ -2,7 +2,7 @@ "extends": "../../tsconfig.base.json", "references": [ { "path": "../compiler/tsconfig.json" }, - { "path": "../rest/tsconfig.json" }, + { "path": "../http/tsconfig.json" }, { "path": "../openapi/tsconfig.json" } ], "compilerOptions": { From 141010518ebb7fd7e95857c334e196ddb41a4900 Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Thu, 22 Feb 2024 16:20:26 -0800 Subject: [PATCH 16/26] fix issue --- packages/http/src/metadata.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/http/src/metadata.ts b/packages/http/src/metadata.ts index 17919d5a63..daa0e86d2e 100644 --- a/packages/http/src/metadata.ts +++ b/packages/http/src/metadata.ts @@ -592,7 +592,7 @@ export function createMetadataInfo(program: Program, options?: MetadataInfoOptio // For OpenAPI emit, for example, this means that we won't put a // readOnly: true property into a specialized schema for a non-read // visibility. - keepShareableProperties ||= visibility === canonicalVisibility; + keepShareableProperties ??= visibility === canonicalVisibility; return !!(keepShareableProperties && options?.canShareProperty?.(property)); } @@ -606,7 +606,7 @@ export function createMetadataInfo(program: Program, options?: MetadataInfoOptio function getEffectivePayloadType(type: Type, visibility: Visibility): Type { if (type.kind === "Model" && !type.name) { const effective = getEffectiveModelType(program, type, (p) => - isPayloadProperty(p, visibility) + isPayloadProperty(p, visibility, undefined, /* keep shared */ false) ); if (effective.name) { return effective; From fe18ddf750c26f74b0d586a4abd6891bf34ed7b7 Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Fri, 23 Feb 2024 07:53:46 -0800 Subject: [PATCH 17/26] revert doc change --- packages/http/src/responses.ts | 8 ++++---- .../output/documentation/@typespec/openapi3/openapi.yaml | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/http/src/responses.ts b/packages/http/src/responses.ts index 37f04a78c3..e12c7a86fc 100644 --- a/packages/http/src/responses.ts +++ b/packages/http/src/responses.ts @@ -14,7 +14,7 @@ import { Program, Type, } from "@typespec/compiler"; -import { resolveBody } from "./body.js"; +import { resolveBody, ResolvedBody } from "./body.js"; import { getContentTypes, isContentTypeHeader } from "./content-types.js"; import { getHeaderFieldName, @@ -145,7 +145,7 @@ function processResponseType( operation, responseType, statusCode, - resolvedBody?.type + resolvedBody ), responses: [], }; @@ -255,7 +255,7 @@ function getResponseDescription( operation: Operation, responseType: Type, statusCode: HttpStatusCodes[number], - bodyType: Type | undefined + body: ResolvedBody | undefined ): string | undefined { // NOTE: If the response type is an envelope and not the same as the body // type, then use its @doc as the response description. However, if the @@ -264,7 +264,7 @@ function getResponseDescription( // as the response description. This allows more freedom to change how // TypeSpec is expressed in semantically equivalent ways without causing // the output to change unnecessarily. - if (responseType !== bodyType) { + if (body === undefined || body.property) { const desc = getDoc(program, responseType); if (desc) { return desc; diff --git a/packages/samples/test/output/documentation/@typespec/openapi3/openapi.yaml b/packages/samples/test/output/documentation/@typespec/openapi3/openapi.yaml index 599b027809..1b62c51d42 100644 --- a/packages/samples/test/output/documentation/@typespec/openapi3/openapi.yaml +++ b/packages/samples/test/output/documentation/@typespec/openapi3/openapi.yaml @@ -73,7 +73,7 @@ paths: schema: $ref: '#/components/schemas/Resp' '404': - description: Not found + description: The server cannot find the requested resource. content: application/json: schema: From 7353bb857d6d124635133ad5c7562a1c83fdff3a Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Wed, 28 Feb 2024 13:49:53 -0800 Subject: [PATCH 18/26] Reduce breaking changes --- packages/http/src/body.ts | 50 +++++++++++++++++++++++++------- packages/http/src/decorators.ts | 17 ++--------- packages/http/src/parameters.ts | 3 +- packages/http/src/responses.ts | 2 +- packages/http/src/types.ts | 3 ++ packages/openapi3/src/openapi.ts | 11 ++++--- 6 files changed, 53 insertions(+), 33 deletions(-) diff --git a/packages/http/src/body.ts b/packages/http/src/body.ts index 6a4c97de44..462396bc4e 100644 --- a/packages/http/src/body.ts +++ b/packages/http/src/body.ts @@ -25,6 +25,8 @@ export interface ResolvedBody { readonly type: Type; /** `true` if the body was specified with `@body` */ readonly isExplicit: boolean; + /** If the body original model contained property annotated with metadata properties. */ + readonly containsMetadataAnnotations: boolean; /** If body is defined with `@body` or `@bodyRoot` this is the property */ readonly property?: ModelProperty; } @@ -34,12 +36,17 @@ export function resolveBody( requestOrResponseType: Type, metadata: Set, rootPropertyMap: Map, - visibility: Visibility + visibility: Visibility, + usedIn: "request" | "response" ): [ResolvedBody | undefined, readonly Diagnostic[]] { const diagnostics = createDiagnosticCollector(); // non-model or intrinsic/array model -> response body is response type if (requestOrResponseType.kind !== "Model" || isArrayModelType(program, requestOrResponseType)) { - return diagnostics.wrap({ type: requestOrResponseType, isExplicit: false }); + return diagnostics.wrap({ + type: requestOrResponseType, + isExplicit: false, + containsMetadataAnnotations: false, + }); } const duplicateTracker = new DuplicateTracker(); @@ -51,8 +58,18 @@ export function resolveBody( const isBodyRootVal = isBodyRoot(program, property); if (isBodyVal || isBodyRootVal) { duplicateTracker.track("body", property); + let containsMetadataAnnotations = false; + if (isBodyVal) { + const valid = diagnostics.pipe(validateBodyProperty(program, property, usedIn)); + containsMetadataAnnotations = !valid; + } if (resolvedBody === undefined) { - resolvedBody = { type: property.type, isExplicit: isBodyVal, property }; + resolvedBody = { + type: property.type, + isExplicit: isBodyVal, + containsMetadataAnnotations, + property, + }; } } } @@ -70,7 +87,11 @@ export function resolveBody( // Special case if the model as a parent model then we'll return an empty object as this is assumed to be a nominal type. // Special Case if the model has an indexer then it means it can return props so cannot be void. if (requestOrResponseType.baseModel || requestOrResponseType.indexer) { - return diagnostics.wrap({ type: requestOrResponseType, isExplicit: false }); + return diagnostics.wrap({ + type: requestOrResponseType, + isExplicit: false, + containsMetadataAnnotations: false, + }); } // Special case for legacy purposes if the return type is an empty model with only @discriminator("xyz") // Then we still want to return that object as it technically always has a body with that implicit property. @@ -78,7 +99,11 @@ export function resolveBody( requestOrResponseType.derivedModels.length > 0 && getDiscriminator(program, requestOrResponseType) ) { - return diagnostics.wrap({ type: requestOrResponseType, isExplicit: false }); + return diagnostics.wrap({ + type: requestOrResponseType, + isExplicit: false, + containsMetadataAnnotations: false, + }); } } @@ -92,7 +117,11 @@ export function resolveBody( if (unannotatedProperties.properties.size > 0) { if (resolvedBody === undefined) { - return diagnostics.wrap({ type: unannotatedProperties, isExplicit: false }); + return diagnostics.wrap({ + type: unannotatedProperties, + isExplicit: false, + containsMetadataAnnotations: false, + }); } else { diagnostics.add( createDiagnostic({ @@ -110,7 +139,8 @@ export function resolveBody( /** Validate a property marked with `@body` */ export function validateBodyProperty( program: Program, - property: ModelProperty + property: ModelProperty, + usedIn: "request" | "response" ): [boolean, readonly Diagnostic[]] { const diagnostics = createDiagnosticCollector(); navigateType( @@ -119,11 +149,11 @@ export function validateBodyProperty( modelProperty: (prop) => { const kind = isHeader(program, prop) ? "header" - : isQueryParam(program, prop) + : usedIn === "request" && isQueryParam(program, prop) ? "query" - : isPathParam(program, prop) + : usedIn === "request" && isPathParam(program, prop) ? "path" - : isStatusCode(program, prop) + : usedIn === "response" && isStatusCode(program, prop) ? "statusCode" : undefined; diff --git a/packages/http/src/decorators.ts b/packages/http/src/decorators.ts index 2010b688b9..1dd3e46a6e 100644 --- a/packages/http/src/decorators.ts +++ b/packages/http/src/decorators.ts @@ -22,7 +22,6 @@ import { validateDecoratorTarget, validateDecoratorUniqueOnNode, } from "@typespec/compiler"; -import { validateBodyProperty } from "./body.js"; import { HttpStateKeys, createDiagnostic, reportDiagnostic } from "./lib.js"; import { setRoute, setSharedRoute } from "./route.js"; import { getStatusCodesFromType } from "./status-codes.js"; @@ -169,11 +168,7 @@ export function isPathParam(program: Program, entity: Type) { } export function $body(context: DecoratorContext, entity: ModelProperty) { - const [valid, diagnostics] = validateBodyProperty(context.program, entity); - if (!valid) { - context.program.reportDiagnostics(diagnostics); - } - context.program.stateMap(HttpStateKeys.body).set(entity, { valid }); + context.program.stateSet(HttpStateKeys.body).add(entity); } export function $bodyRoot(context: DecoratorContext, entity: ModelProperty) { @@ -185,15 +180,7 @@ export function $bodyIgnore(context: DecoratorContext, entity: ModelProperty) { } export function isBody(program: Program, entity: Type): boolean { - return program.stateMap(HttpStateKeys.body).has(entity); -} - -/** - * Return if a property marked with `@body` is valid(Doesn't contain any metadata property). - * Returns `false` if it contains a metadata property. - */ -export function isExplicitBodyValid(program: Program, entity: ModelProperty): boolean { - return program.stateMap(HttpStateKeys.body).get(entity).valid; + return program.stateSet(HttpStateKeys.body).has(entity); } export function isBodyRoot(program: Program, entity: ModelProperty): boolean { diff --git a/packages/http/src/parameters.ts b/packages/http/src/parameters.ts index 8a6c78aee5..1b26aa9ace 100644 --- a/packages/http/src/parameters.ts +++ b/packages/http/src/parameters.ts @@ -77,7 +77,7 @@ function getOperationParametersForVerb( const parameters: HttpOperationParameter[] = []; const resolvedBody = diagnostics.pipe( - resolveBody(program, operation.parameters, metadata, rootPropertyMap, visibility) + resolveBody(program, operation.parameters, metadata, rootPropertyMap, visibility, "request") ); let contentTypes: string[] | undefined; @@ -182,6 +182,7 @@ function computeHttpOperationBody( const body: HttpOperationRequestBody = { type: resolvedBody.type, isExplicit: resolvedBody.isExplicit, + containsMetadataAnnotations: resolvedBody.containsMetadataAnnotations, contentTypes, }; if (resolvedBody.property) { diff --git a/packages/http/src/responses.ts b/packages/http/src/responses.ts index e12c7a86fc..37d16402c8 100644 --- a/packages/http/src/responses.ts +++ b/packages/http/src/responses.ts @@ -109,7 +109,7 @@ function processResponseType( // Get body let resolvedBody = diagnostics.pipe( - resolveBody(program, responseType, metadata, rootPropertyMap, Visibility.Read) + resolveBody(program, responseType, metadata, rootPropertyMap, Visibility.Read, "response") ); // If there is no explicit status code, check if it should be 204 diff --git a/packages/http/src/types.ts b/packages/http/src/types.ts index 8cec734b2b..9942255f06 100644 --- a/packages/http/src/types.ts +++ b/packages/http/src/types.ts @@ -411,6 +411,9 @@ export interface HttpOperationBody { /** If the body was explicitly set with `@body`. */ readonly isExplicit: boolean; + + /** If the body contains metadata annotations to ignore. For example `@header`. */ + readonly containsMetadataAnnotations: boolean; } export interface HttpStatusCodeRange { diff --git a/packages/openapi3/src/openapi.ts b/packages/openapi3/src/openapi.ts index f54417626c..1e361d8d09 100644 --- a/packages/openapi3/src/openapi.ts +++ b/packages/openapi3/src/openapi.ts @@ -70,7 +70,6 @@ import { HttpStatusCodesEntry, HttpVerb, isContentTypeHeader, - isExplicitBodyValid, isOverloadSameEndpoint, MetadataInfo, QueryParameterOptions, @@ -1020,7 +1019,7 @@ function createOAPIEmitter( : getSchemaForBody( data.body.type, Visibility.Read, - data.body.isExplicit && !isExplicitBodyValid(program, data.body.property!), + data.body.isExplicit && data.body.containsMetadataAnnotations, undefined ); if (schemaMap.has(contentType)) { @@ -1128,14 +1127,14 @@ function createOAPIEmitter( function getSchemaForBody( type: Type, visibility: Visibility, - isExplicit: boolean, + ignoreMetadataAnnotations: boolean, multipart: string | undefined ): any { const effectiveType = metadataInfo.getEffectivePayloadType(type, visibility); return callSchemaEmitter( effectiveType, visibility, - isExplicit, + ignoreMetadataAnnotations, multipart ?? "application/json" ); } @@ -1215,7 +1214,7 @@ function createOAPIEmitter( : getSchemaForBody( body.type, visibility, - body.isExplicit && !isExplicitBodyValid(program, body.parameter!), + body.isExplicit, contentType.startsWith("multipart/") ? contentType : undefined ); if (schemaMap.has(contentType)) { @@ -1258,7 +1257,7 @@ function createOAPIEmitter( : getSchemaForBody( body.type, visibility, - body.isExplicit && !isExplicitBodyValid(program, body.parameter!), + body.isExplicit && body.containsMetadataAnnotations, contentType.startsWith("multipart/") ? contentType : undefined ); const contentEntry: any = { From 87fc5f32fafa7b1166ead2f3d0b06beb59881fa7 Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Wed, 28 Feb 2024 13:54:13 -0800 Subject: [PATCH 19/26] Tweak --- packages/http/test/parameters.test.ts | 7 ++++++- packages/http/test/responses.test.ts | 8 +++++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/packages/http/test/parameters.test.ts b/packages/http/test/parameters.test.ts index 9ccb6ab3d7..24fcd345c7 100644 --- a/packages/http/test/parameters.test.ts +++ b/packages/http/test/parameters.test.ts @@ -187,7 +187,7 @@ describe("emit diagnostics when using metadata decorator in @body", () => { it.each([ ["@header", "id: string"], ["@query", "id: string"], - ["@statusCode", "_: 200"], + ["@path", "id: string"], ])("%s", async (dec, prop) => { const [_, diagnostics] = await compileOperations( `op read(@body explicit: {${dec} ${prop}, other: string}): void;` @@ -195,3 +195,8 @@ describe("emit diagnostics when using metadata decorator in @body", () => { expectDiagnostics(diagnostics, { code: "@typespec/http/metadata-ignored" }); }); }); + +it("doesn't emit diagnostic if the metadata is not applicable in the request", async () => { + const [_, diagnostics] = await compileOperations(`op read(@statusCode id: 200): { };`); + expectDiagnosticEmpty(diagnostics); +}); diff --git a/packages/http/test/responses.test.ts b/packages/http/test/responses.test.ts index c8e7505a79..bd830ae776 100644 --- a/packages/http/test/responses.test.ts +++ b/packages/http/test/responses.test.ts @@ -54,7 +54,6 @@ describe("body resolution", () => { describe("emit diagnostics when using metadata decorator in @body", () => { it.each([ ["@header", "id: string"], - ["@query", "id: string"], ["@statusCode", "_: 200"], ])("%s", async (dec, prop) => { const [_, diagnostics] = await compileOperations( @@ -65,6 +64,13 @@ describe("body resolution", () => { }); }); +it("doesn't emit diagnostic if the metadata is not applicable in the response", async () => { + const [_, diagnostics] = await compileOperations( + `op read(): { @body explicit: {@path id: string} };` + ); + expectDiagnosticEmpty(diagnostics); +}); + it("issues diagnostics for invalid content types", async () => { const [_, diagnostics] = await compileOperations( ` From 859efebd08a61b65f5be40007e228a2a3d789b81 Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Tue, 2 Apr 2024 08:16:47 -0700 Subject: [PATCH 20/26] Create body-consitency-2024-2-27-18-35-44.md --- .../body-consitency-2024-2-27-18-35-44.md | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 .chronus/changes/body-consitency-2024-2-27-18-35-44.md diff --git a/.chronus/changes/body-consitency-2024-2-27-18-35-44.md b/.chronus/changes/body-consitency-2024-2-27-18-35-44.md new file mode 100644 index 0000000000..f2f65e02e0 --- /dev/null +++ b/.chronus/changes/body-consitency-2024-2-27-18-35-44.md @@ -0,0 +1,22 @@ +--- +# Change versionKind to one of: internal, fix, dependencies, feature, deprecation, breaking +changeKind: breaking +packages: + - "@typespec/http" +--- + +`@body` means this is the body + + This change makes it that using `@body` will mean exactly this is the body and everything underneath will be included, including metadata properties. It will log a warning explaining that. + + ```tsp + op a1(): {@body _: {@header foo: string, other: string} }; + ^ warning header in a body, it will not be included as a header. + ``` + + Solution use `@bodyRoot` as the goal is only to change where to resolve the body from. + + ```tsp + op a1(): {@bodyRoot _: {@header foo: string, other: string} }; + ``` + From e9ef1852709ab45b4de8e6b707da862d49d16c65 Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Tue, 2 Apr 2024 08:17:16 -0700 Subject: [PATCH 21/26] Create body-consitency-2024-2-27-18-35-44-1.md --- .../body-consitency-2024-2-27-18-35-44-1.md | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 .chronus/changes/body-consitency-2024-2-27-18-35-44-1.md diff --git a/.chronus/changes/body-consitency-2024-2-27-18-35-44-1.md b/.chronus/changes/body-consitency-2024-2-27-18-35-44-1.md new file mode 100644 index 0000000000..c4598f7a4d --- /dev/null +++ b/.chronus/changes/body-consitency-2024-2-27-18-35-44-1.md @@ -0,0 +1,23 @@ +--- +# Change versionKind to one of: internal, fix, dependencies, feature, deprecation, breaking +changeKind: breaking +packages: + - "@typespec/http" +--- + +Empty model after removing metadata and visibility property result in void always + This means the following case have changed from returning `{}` to no body + + ```tsp + op b1(): {}; + op b2(): {@visibility("none") prop: string}; + op b3(): {@added(Versions.v2) prop: string}; + ``` + + Workaround: Use explicit `@body` + + ```tsp + op b1(): {@body _: {}}; + op b2(): {@body _: {@visibility("none") prop: string}}; + op b3(): {@body _: {@added(Versions.v2) prop: string}}; + ``` From 38dd669264f93a2da0123e1c31fe3ebd44a8ca64 Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Tue, 2 Apr 2024 08:17:37 -0700 Subject: [PATCH 22/26] Create body-consitency-2024-2-27-18-35-44-2.md --- .../body-consitency-2024-2-27-18-35-44-2.md | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 .chronus/changes/body-consitency-2024-2-27-18-35-44-2.md diff --git a/.chronus/changes/body-consitency-2024-2-27-18-35-44-2.md b/.chronus/changes/body-consitency-2024-2-27-18-35-44-2.md new file mode 100644 index 0000000000..cb08389af9 --- /dev/null +++ b/.chronus/changes/body-consitency-2024-2-27-18-35-44-2.md @@ -0,0 +1,20 @@ +--- +# Change versionKind to one of: internal, fix, dependencies, feature, deprecation, breaking +changeKind: breaking +packages: + - "@typespec/http" + - "@typespec/openapi3" + - "@typespec/rest" +--- + +Status code always 200 except if response is explicitly `void` + + ```tsp + op c1(): {@header foo: string}; // status code 200 (used to be 204) + ``` + + Solution: Add explicit `@statusCode` + ```tsp + op c1(): {@header foo: string, @statusCode _: 204}; + op c1(): {@header foo: string, ...NoContent}; // or spread common model + ``` From d6be3d7a657f4a53434c5b57727f12107de12074 Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Tue, 2 Apr 2024 08:19:56 -0700 Subject: [PATCH 23/26] Update body-consitency-2024-2-27-18-35-44-2.md --- .chronus/changes/body-consitency-2024-2-27-18-35-44-2.md | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/.chronus/changes/body-consitency-2024-2-27-18-35-44-2.md b/.chronus/changes/body-consitency-2024-2-27-18-35-44-2.md index cb08389af9..aa980aab9e 100644 --- a/.chronus/changes/body-consitency-2024-2-27-18-35-44-2.md +++ b/.chronus/changes/body-consitency-2024-2-27-18-35-44-2.md @@ -3,11 +3,9 @@ changeKind: breaking packages: - "@typespec/http" - - "@typespec/openapi3" - - "@typespec/rest" --- -Status code always 200 except if response is explicitly `void` +Implicit status code always 200 except if response is explicitly `void` ```tsp op c1(): {@header foo: string}; // status code 200 (used to be 204) From 14b7d6bcfc4d43de6c21cf9be16d02cbaf0a675f Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Tue, 2 Apr 2024 08:21:35 -0700 Subject: [PATCH 24/26] Create body-consitency-2024-3-2-15-21-9.md --- .../body-consitency-2024-3-2-15-21-9.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 .chronus/changes/body-consitency-2024-3-2-15-21-9.md diff --git a/.chronus/changes/body-consitency-2024-3-2-15-21-9.md b/.chronus/changes/body-consitency-2024-3-2-15-21-9.md new file mode 100644 index 0000000000..9d8ffa9114 --- /dev/null +++ b/.chronus/changes/body-consitency-2024-3-2-15-21-9.md @@ -0,0 +1,18 @@ +--- +# Change versionKind to one of: internal, fix, dependencies, feature, deprecation, breaking +changeKind: breaking +packages: + - "@typespec/http" +--- + +Properties are not automatically omitted if everything was removed from metadata or visibility + + ```tsp + op d1(): {headers: {@header foo: string}}; // body will be {headers: {}} + ``` + + Solution: use `@bodyIgnore` + + ```tsp + op d1(): {@bodyIgnore headers: {@header foo: string}}; // body will be {headers: {}} + ``` From a90480f0f1636e78398022b15acd46ae2fb92ac2 Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Tue, 2 Apr 2024 08:21:59 -0700 Subject: [PATCH 25/26] Create body-consitency-2024-3-2-15-21-10.md --- .chronus/changes/body-consitency-2024-3-2-15-21-10.md | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 .chronus/changes/body-consitency-2024-3-2-15-21-10.md diff --git a/.chronus/changes/body-consitency-2024-3-2-15-21-10.md b/.chronus/changes/body-consitency-2024-3-2-15-21-10.md new file mode 100644 index 0000000000..1a4c8cb7fc --- /dev/null +++ b/.chronus/changes/body-consitency-2024-3-2-15-21-10.md @@ -0,0 +1,9 @@ +--- +# Change versionKind to one of: internal, fix, dependencies, feature, deprecation, breaking +changeKind: feature +packages: + - "@typespec/openapi3" + - "@typespec/rest" +--- + +Add supoort for new `@bodyRoot` and `@body` distinction From 9ece16a357340968e5359cc4edcf2aaff775b5d1 Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Mon, 15 Apr 2024 15:22:12 -0700 Subject: [PATCH 26/26] Update packages/http/src/metadata.ts Co-authored-by: Mark Cowlishaw --- packages/http/src/metadata.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/http/src/metadata.ts b/packages/http/src/metadata.ts index daa0e86d2e..2108d89a23 100644 --- a/packages/http/src/metadata.ts +++ b/packages/http/src/metadata.ts @@ -383,7 +383,7 @@ export interface MetadataInfo { * * When the type of a property is emptied by visibility, the property * itself is also removed. - * @deprecated This produce inconsistent behaviors and should be avoided. + * @deprecated This produces inconsistent behaviors and should be avoided. */ isEmptied(type: Type | undefined, visibility: Visibility): boolean;