diff --git a/.chronus/changes/improve-response-descriptions-same-status-code-2026-8-9-2-8-16.md b/.chronus/changes/improve-response-descriptions-same-status-code-2026-8-9-2-8-16.md new file mode 100644 index 00000000000..f61f827e5d0 --- /dev/null +++ b/.chronus/changes/improve-response-descriptions-same-status-code-2026-8-9-2-8-16.md @@ -0,0 +1,20 @@ +--- +changeKind: fix +packages: + - "@typespec/http" + - "@typespec/openapi3" +--- + +Improve how response descriptions are chosen when multiple responses share a status code. When an operation has multiple responses with the same HTTP status code, all responses sharing that code are considered when computing the description associated with that code instead of just one. + +```tsp +@doc("A cat.") +model Cat { @statusCode _: 200, meow: boolean } + +@doc("A dog.") +model Dog { @statusCode _: 200, bark: boolean } + +// The response description defaults to "The request has succeeded." +// because the doc comments for the variants conflict. +op read(): Cat | Dog; +``` diff --git a/packages/http/src/responses.ts b/packages/http/src/responses.ts index dfc2f920df7..ccfc0816683 100644 --- a/packages/http/src/responses.ts +++ b/packages/http/src/responses.ts @@ -26,7 +26,13 @@ import type { HttpProperty } from "./http-property.js"; import { HttpStateKeys, reportDiagnostic } from "./lib.js"; import { Visibility } from "./metadata.js"; import { HttpPayloadDisposition, resolveHttpPayload } from "./payload.js"; -import type { HttpOperationResponse, HttpStatusCodes, HttpStatusCodesEntry } from "./types.js"; +import type { + HttpOperationResponse, + HttpOperationResponseContent, + HttpPayloadBody, + HttpStatusCodes, + HttpStatusCodesEntry, +} from "./types.js"; /** * Get the responses for a given operation. @@ -36,16 +42,49 @@ export function getResponsesForOperation( operation: Operation, ): [HttpOperationResponse[], readonly Diagnostic[]] { const diagnostics = createDiagnosticCollector(); - const responses = new ResponseIndex(); // Resolve union variants into concrete response types, grouping plain body variants // (no HTTP metadata) into a single union type. const variants = resolveResponseVariants(program, operation.returnType); + const processedResponses: ProcessedResponseType[] = []; for (const { type, description } of variants) { - processResponseType(program, diagnostics, operation, responses, type, description); + processedResponses.push( + processResponseType(program, diagnostics, operation, type, description), + ); + } + + const responsesByStatus = new ResponseIndex(); + for (const response of processedResponses) { + for (const statusCode of response.statusCodes) { + responsesByStatus.add(statusCode, response); + } + } + + const responses: HttpOperationResponse[] = []; + for (const [statusCode, responseGroup] of responsesByStatus.entries()) { + const responseContents: HttpOperationResponseContent[] = responseGroup.map((response) => { + const content: HttpOperationResponseContent = { + headers: response.headers, + properties: response.properties, + }; + if (response.body) { + content.body = response.body; + } + return content; + }); + + responses.push({ + statusCodes: statusCode, + // It would be more accurate to express the response type as a union of all variant types. + // However, downstream code written since we first decided to use only the first variant's + // type may rely on us to continue doing so. For now, keep this behavior. + type: responseGroup[0].type, + description: getResponsesDescription(program, operation, statusCode, responseGroup), + responses: responseContents, + }); } - return diagnostics.wrap(responses.values()); + return diagnostics.wrap(responses); } interface ResolvedResponseVariant { @@ -110,37 +149,57 @@ function resolveResponseVariants( * Class keeping an index of all the response by status code */ class ResponseIndex { - readonly #index = new Map(); + readonly #index = new Map(); - public get(statusCode: HttpStatusCodesEntry): HttpOperationResponse | undefined { - return this.#index.get(this.#indexKey(statusCode)); - } - - public set(statusCode: HttpStatusCodesEntry, response: HttpOperationResponse): void { - this.#index.set(this.#indexKey(statusCode), response); + public add(statusCode: HttpStatusCodesEntry, response: ProcessedResponseType): void { + const indexKey = this.#indexKey(statusCode); + if (this.#index.has(indexKey)) { + this.#index.get(indexKey)!.push(response); + return; + } + this.#index.set(indexKey, [response]); } - public values(): HttpOperationResponse[] { - return [...this.#index.values()]; + public *entries(): MapIterator<[HttpStatusCodesEntry, ProcessedResponseType[]]> { + for (const [indexKey, responses] of this.#index.entries()) { + let parsedStatusCodes: HttpStatusCodesEntry; + if (indexKey === "*") { + parsedStatusCodes = "*"; + } else if (indexKey.includes(",")) { + const [start, end] = indexKey.split(","); + parsedStatusCodes = { start: Number(start), end: Number(end) }; + } else { + parsedStatusCodes = Number(indexKey); + } + yield [parsedStatusCodes, responses]; + } } #indexKey(statusCode: HttpStatusCodesEntry) { if (typeof statusCode === "number" || statusCode === "*") { return String(statusCode); } else { - return `${statusCode.start}-${statusCode.end}`; + return `${statusCode.start},${statusCode.end}`; } } } +interface ProcessedResponseType { + statusCodes: HttpStatusCodes; + type: Type; + parentDescription?: string; + body?: HttpPayloadBody; + headers: Record; + properties: HttpProperty[]; +} + function processResponseType( program: Program, diagnostics: DiagnosticCollector, operation: Operation, - responses: ResponseIndex, responseType: Type, - parentDescription?: string, -) { + parentDescription: string | undefined, +): ProcessedResponseType { // Get body const verb = getOperationVerb(program, operation); let { body: resolvedBody, metadata } = diagnostics.pipe( @@ -171,35 +230,14 @@ function processResponseType( } } - // Put them into currentEndpoint.responses - for (const statusCode of statusCodes) { - // the first model for this statusCode/content type pair carries the - // description for the endpoint. This could probably be improved. - const response: HttpOperationResponse = responses.get(statusCode) ?? { - statusCodes: statusCode, - type: responseType, - description: getResponseDescription( - program, - operation, - responseType, - statusCode, - metadata, - parentDescription, - ), - responses: [], - }; - - if (resolvedBody !== undefined) { - response.responses.push({ - body: resolvedBody, - headers, - properties: metadata, - }); - } else { - response.responses.push({ headers, properties: metadata }); - } - responses.set(statusCode, response); - } + return { + statusCodes: statusCodes, + type: responseType, + parentDescription, + body: resolvedBody, + headers: headers, + properties: metadata, + }; } /** @@ -290,39 +328,53 @@ function isPlainResponseBody(program: Program, type: Type): boolean { return !result || !result.metadata.some((p) => p.kind !== "bodyProperty"); } -function getResponseDescription( +function getResponsesDescription( program: Program, operation: Operation, - responseType: Type, statusCode: HttpStatusCodes[number], - metadata: HttpProperty[], - parentDescription?: string, -): string | undefined { - // If a parent union provided a description, use that first - if (parentDescription) { - return parentDescription; + variants: ProcessedResponseType[], +) { + if (variants.length <= 0) { + return getStatusCodeDescription(statusCode); } - // 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 - // response type is the same as the body type, then use the default status - // code description and don't duplicate the schema description of the body - // 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 (isResponseEnvelope(metadata)) { - const desc = getDoc(program, responseType); - if (desc) { - return desc; + function getSingleResponseDescription(variant: ProcessedResponseType): string | undefined { + if (variant.parentDescription) { + return variant.parentDescription; + } + + // NOTE: If the response type includes response envelope metadata (e.g. @statusCode, @header), + // then use its @doc as the response description. Plain body types intentionally fall back to + // the status-code/operation-level descriptions to avoid duplicating the schema description as + // the response description. + if (isResponseEnvelope(variant.properties)) { + const desc = getDoc(program, variant.type); + if (desc) return desc; } + + return undefined; } - const desc = isErrorModel(program, responseType) - ? getErrorsDoc(program, operation) - : getReturnsDoc(program, operation); - if (desc) { - return desc; + const firstDesc = getSingleResponseDescription(variants[0]); + if (firstDesc && variants.every((v) => getSingleResponseDescription(v) === firstDesc)) { + return firstDesc; } - return getStatusCodeDescription(statusCode); + let hasError = false, + hasSuccess = false; + for (const variant of variants) { + if (isErrorModel(program, variant.type)) { + hasError = true; + } else { + hasSuccess = true; + } + } + + let desc: string | undefined; + if (hasSuccess && !hasError) { + desc = getReturnsDoc(program, operation); + } else if (hasError && !hasSuccess) { + desc = getErrorsDoc(program, operation); + } + return desc || getStatusCodeDescription(statusCode); } diff --git a/packages/http/test/response-descriptions.test.ts b/packages/http/test/response-descriptions.test.ts index 78acfd71252..1c44ff96380 100644 --- a/packages/http/test/response-descriptions.test.ts +++ b/packages/http/test/response-descriptions.test.ts @@ -79,3 +79,177 @@ it("@doc on response model set response doc if model is an evelope with @statusC ); strictEqual(op.responses[0].description, "Explicit doc"); }); + +it("uses union @doc when all responses sharing a status code came from that union", async () => { + const op = await getHttpOp(` + @doc("A cat or a dog.") + union Pet { cat: Cat, dog: Dog } + + model Cat { @statusCode _: 200, meow: boolean } + model Dog { @statusCode _: 200, bark: boolean } + + op read(): Pet; + `); + strictEqual(op.responses[0].description, "A cat or a dog."); +}); + +it("uses union @doc over operation @returnsDoc", async () => { + const op = await getHttpOp(` + @doc("A cat or a dog.") + union Pet { cat: Cat, dog: Dog } + + model Cat { @statusCode _: 200, meow: boolean } + model Dog { @statusCode _: 200, bark: boolean } + + @returnsDoc("A pet.") + op read(): Pet; + `); + strictEqual(op.responses[0].description, "A cat or a dog."); +}); + +it("uses shared @doc among all responses sharing a status code", async () => { + const op = await getHttpOp(` + @doc("A pet.") + model Cat { @statusCode _: 200, meow: boolean } + + @doc("A pet.") + model Dog { @statusCode _: 200, bark: boolean } + + op read(): Cat | Dog; + `); + strictEqual(op.responses[0].description, "A pet."); +}); + +it("uses default description when the @doc of all responses sharing a status code disagree", async () => { + const op = await getHttpOp(` + @doc("A cat.") + model Cat { @statusCode _: 200, meow: boolean } + + @doc("A dog.") + model Dog { @statusCode _: 200, bark: boolean } + + op read(): Cat | Dog; + `); + strictEqual(op.responses[0].description, "The request has succeeded."); +}); + +it("uses @returnsDoc when the @doc of all success responses sharing a status code disagree", async () => { + const op = await getHttpOp(` + @doc("A cat.") + model Cat { @statusCode _: 200, meow: boolean } + + @doc("A dog.") + model Dog { @statusCode _: 200, bark: boolean } + + @returnsDoc("A pet.") + @errorsDoc("Something went wrong.") + op read(): Cat | Dog; + `); + strictEqual(op.responses[0].description, "A pet."); +}); + +it("uses @errorsDoc when the @doc of all @error responses sharing a status code disagree", async () => { + const op = await getHttpOp(` + @doc("Error A.") + @error model ErrorA { @statusCode _: 400, codeA: string } + + @doc("Error B.") + @error model ErrorB { @statusCode _: 400, codeB: string } + + @returnsDoc("Success.") + @errorsDoc("Something went wrong.") + op read(): ErrorA | ErrorB; + `); + strictEqual(op.responses[0].description, "Something went wrong."); +}); + +it("uses @returnsDoc when every response sharing a status code is a non-error model", async () => { + const op = await getHttpOp(` + union Pet { cat: Cat, dog: Dog } + + model Cat { @statusCode _: 200, meow: boolean } + model Dog { @statusCode _: 200, bark: boolean } + + @returnsDoc("A pet.") + @errorsDoc("Something went wrong.") + op read(): Pet; + `); + strictEqual(op.responses[0].description, "A pet."); +}); + +it("uses @errorsDoc when every response sharing a status code is an @error model", async () => { + const op = await getHttpOp(` + @error model ErrorA { @statusCode _: 400, codeA: string } + @error model ErrorB { @statusCode _: 400, codeB: string } + + @returnsDoc("Success.") + @errorsDoc("Something went wrong.") + op read(): ErrorA | ErrorB; + `); + strictEqual(op.responses[0].description, "Something went wrong."); +}); + +it("uses default description when responses sharing a status code mix success and error models", async () => { + const op = await getHttpOp(` + @error model Error { @statusCode _: 200; message: string } + model Pet { @statusCode _: 200 } + + @returnsDoc("Success.") + @errorsDoc("Something went wrong.") + op read(): Pet | Error; + `); + strictEqual(op.responses[0].description, "The request has succeeded."); +}); + +it("uses shared @doc for responses sharing a status code even when one comes from a union @doc and another from its own @doc", async () => { + const op = await getHttpOp(` + @doc("Success.") + union Pet { cat: Cat } + + model Cat { @statusCode _: 200, meow: boolean } + + @doc("Success.") + model Extra { @statusCode _: 200, extra: string } + + op read(): Pet | Extra; + `); + strictEqual(op.responses[0].description, "Success."); +}); + +it("uses shared @doc among all responses sharing a status code range", async () => { + const op = await getHttpOp(` + @doc("A pet.") + model Cat { + @statusCode @minValue(200) @maxValue(299) _: int32; + meow: boolean; + } + + @doc("A pet.") + model Dog { + @statusCode @minValue(200) @maxValue(299) _: int32; + bark: boolean; + } + + op read(): Cat | Dog; + `); + strictEqual(op.responses[0].description, "A pet."); +}); + +it("uses default description when responses sharing a status code range have different descriptions", async () => { + const op = await getHttpOp(` + @doc("A cat.") + model Cat { + @statusCode @minValue(200) @maxValue(299) _: int32; + meow: boolean; + } + + @doc("A dog.") + model Dog { + @statusCode @minValue(200) @maxValue(299) _: int32; + bark: boolean; + } + + op read(): Cat | Dog; + `); + strictEqual(op.responses[0].description, "Successful"); +}); diff --git a/packages/openapi3/test/response-descriptions.test.ts b/packages/openapi3/test/response-descriptions.test.ts index bbfa05180a2..a8c5aaf4615 100644 --- a/packages/openapi3/test/response-descriptions.test.ts +++ b/packages/openapi3/test/response-descriptions.test.ts @@ -62,31 +62,6 @@ worksFor(supportedVersions, ({ openApiFor }) => { strictEqual(res.paths["/"].get.responses["default"].description, "Generic error"); }); - it("uses first model's description when multiple models have same status code", async () => { - const res = await openApiFor( - ` - @doc("Foo") model Foo { @statusCode _: 409 } - @doc("Bar") model Bar { @statusCode _: 409 } - op read(): { @statusCode _: 200, content: string } | Foo | Bar; - `, - ); - strictEqual(res.paths["/"].get.responses["200"].description, "The request has succeeded."); - strictEqual(res.paths["/"].get.responses["409"].description, "Foo"); - }); - - it("expands named union in return type and uses first variant's description", async () => { - const res = await openApiFor( - ` - @doc("Foo") model Foo { @statusCode _: 409 } - @doc("Bar") model Bar { @statusCode _: 409 } - union Conflict { Foo: Foo; Bar: Bar }; - op read(): { @statusCode _: 200, content: string } | Conflict; - `, - ); - strictEqual(res.paths["/"].get.responses["200"].description, "The request has succeeded."); - strictEqual(res.paths["/"].get.responses["409"].description, "Foo"); - }); - it("uses union variant descriptions", async () => { const res = await openApiFor( ` @@ -155,4 +130,165 @@ worksFor(supportedVersions, ({ openApiFor }) => { strictEqual(res.paths["/"].get.responses["401"].description, "Inner authentication errors"); strictEqual(res.paths["/"].get.responses["403"].description, "All error responses"); }); + + it("uses union @doc over operation @returnsDoc", async () => { + const res = await openApiFor(` + @doc("A cat or a dog.") + union Pet { cat: Cat, dog: Dog } + + model Cat { @statusCode _: 200, meow: boolean } + model Dog { @statusCode _: 200, bark: boolean } + + @returnsDoc("A pet.") + op read(): Pet; + `); + strictEqual(res.paths["/"].get.responses["200"].description, "A cat or a dog."); + }); + + it("uses shared @doc among all responses sharing a status code", async () => { + const res = await openApiFor(` + @doc("A pet.") + model Cat { @statusCode _: 200, meow: boolean } + + @doc("A pet.") + model Dog { @statusCode _: 200, bark: boolean } + + op read(): Cat | Dog; + `); + strictEqual(res.paths["/"].get.responses["200"].description, "A pet."); + }); + + it("uses default description when the @doc of all responses sharing a status code disagree", async () => { + const res = await openApiFor(` + @doc("A cat.") + model Cat { @statusCode _: 200, meow: boolean } + + @doc("A dog.") + model Dog { @statusCode _: 200, bark: boolean } + + op read(): Cat | Dog; + `); + strictEqual(res.paths["/"].get.responses["200"].description, "The request has succeeded."); + }); + + it("uses @returnsDoc when the @doc of all success responses sharing a status code disagree", async () => { + const res = await openApiFor(` + @doc("A cat.") + model Cat { @statusCode _: 200, meow: boolean } + + @doc("A dog.") + model Dog { @statusCode _: 200, bark: boolean } + + @returnsDoc("A pet.") + @errorsDoc("Something went wrong.") + op read(): Cat | Dog; + `); + strictEqual(res.paths["/"].get.responses["200"].description, "A pet."); + }); + + it("uses @errorsDoc when the @doc of all @error responses sharing a status code disagree", async () => { + const res = await openApiFor(` + @doc("Error A.") + @error model ErrorA { @statusCode _: 400, codeA: string } + + @doc("Error B.") + @error model ErrorB { @statusCode _: 400, codeB: string } + + @returnsDoc("Success.") + @errorsDoc("Something went wrong.") + op read(): ErrorA | ErrorB; + `); + strictEqual(res.paths["/"].get.responses["400"].description, "Something went wrong."); + }); + + it("uses @returnsDoc when every response sharing a status code is a non-error model", async () => { + const res = await openApiFor(` + union Pet { cat: Cat, dog: Dog } + + model Cat { @statusCode _: 200, meow: boolean } + model Dog { @statusCode _: 200, bark: boolean } + + @returnsDoc("A pet.") + @errorsDoc("Something went wrong.") + op read(): Pet; + `); + strictEqual(res.paths["/"].get.responses["200"].description, "A pet."); + }); + + it("uses @errorsDoc when every response sharing a status code is an @error model", async () => { + const res = await openApiFor(` + @error model ErrorA { @statusCode _: 400, codeA: string } + @error model ErrorB { @statusCode _: 400, codeB: string } + + @returnsDoc("Success.") + @errorsDoc("Something went wrong.") + op read(): ErrorA | ErrorB; + `); + strictEqual(res.paths["/"].get.responses["400"].description, "Something went wrong."); + }); + + it("uses default description when responses sharing a status code mix success and error models", async () => { + const res = await openApiFor(` + @error model Error { @statusCode _: 200; message: string } + model Pet { @statusCode _: 200 } + + @returnsDoc("Success.") + @errorsDoc("Something went wrong.") + op read(): Pet | Error; + `); + strictEqual(res.paths["/"].get.responses["200"].description, "The request has succeeded."); + }); + + it("uses shared @doc for responses sharing a status code even when one comes from a union @doc and another from its own @doc", async () => { + const res = await openApiFor(` + @doc("Success.") + union Pet { cat: Cat } + + model Cat { @statusCode _: 200, meow: boolean } + + @doc("Success.") + model Extra { @statusCode _: 200, extra: string } + + op read(): Pet | Extra; + `); + strictEqual(res.paths["/"].get.responses["200"].description, "Success."); + }); + + it("uses shared @doc among all responses sharing a status code range", async () => { + const res = await openApiFor(` + @doc("A pet.") + model Cat { + @statusCode @minValue(200) @maxValue(299) _: int32; + meow: boolean; + } + + @doc("A pet.") + model Dog { + @statusCode @minValue(200) @maxValue(299) _: int32; + bark: boolean; + } + + op read(): Cat | Dog; + `); + strictEqual(res.paths["/"].get.responses["2XX"].description, "A pet."); + }); + + it("uses default description when responses sharing a status code range have different descriptions", async () => { + const res = await openApiFor(` + @doc("A cat.") + model Cat { + @statusCode @minValue(200) @maxValue(299) _: int32; + meow: boolean; + } + + @doc("A dog.") + model Dog { + @statusCode @minValue(200) @maxValue(299) _: int32; + bark: boolean; + } + + op read(): Cat | Dog; + `); + strictEqual(res.paths["/"].get.responses["2XX"].description, "Successful"); + }); });