From 4bfbd61c5826963ed9ac0454f0977c9f6f82bebf Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Tue, 22 Mar 2022 14:49:01 -0700 Subject: [PATCH 1/9] Extract the response interpretation logic out of openapi3 emitter and into http library --- packages/openapi3/src/lib.ts | 4 + packages/openapi3/src/openapi.ts | 238 +++---------------------- packages/rest/src/diagnostics.ts | 24 +++ packages/rest/src/http.ts | 2 +- packages/rest/src/lib.ts | 1 + packages/rest/src/responses.ts | 294 +++++++++++++++++++++++++++++++ packages/rest/src/route.ts | 4 + 7 files changed, 351 insertions(+), 216 deletions(-) create mode 100644 packages/rest/src/responses.ts diff --git a/packages/openapi3/src/lib.ts b/packages/openapi3/src/lib.ts index 0c33eabf6cb..96da9fa3d48 100644 --- a/packages/openapi3/src/lib.ts +++ b/packages/openapi3/src/lib.ts @@ -27,24 +27,28 @@ export const libDef = { default: "Duplicate @body declarations on response type", }, }, + // TODO-REMOVE moved to rest lib "duplicate-response": { severity: "error", messages: { default: paramMessage`Multiple return types for content type ${"contentType"} and status code ${"statusCode"}`, }, }, + // TODO-REMOVE moved to rest lib "duplicate-header": { severity: "error", messages: { default: paramMessage`The header ${"header"} is defined across multiple content types`, }, }, + // TODO-REMOVE moved to rest lib "content-type-ignored": { severity: "warning", messages: { default: "content-type header ignored because return type has no body", }, }, + // TODO-REMOVE moved to rest lib "content-type-string": { severity: "error", messages: { diff --git a/packages/openapi3/src/openapi.ts b/packages/openapi3/src/openapi.ts index b1bc69043b1..bca00de2a46 100644 --- a/packages/openapi3/src/openapi.ts +++ b/packages/openapi3/src/openapi.ts @@ -31,7 +31,6 @@ import { isNumericType, isSecret, isStringType, - isVoidType, mapChildModels, ModelType, ModelTypeProperty, @@ -44,18 +43,14 @@ import { UnionTypeVariant, validateDecoratorTarget, } from "@cadl-lang/compiler"; -import { - getExtensions, - getExternalDocs, - getOperationId, - isDefaultResponse, -} from "@cadl-lang/openapi"; +import { getExtensions, getExternalDocs, getOperationId } from "@cadl-lang/openapi"; import { getAllRoutes, getDiscriminator, http, HttpOperationParameter, HttpOperationParameters, + HttpOperationResponse, OperationDetails, } from "@cadl-lang/rest"; import { getVersionRecords } from "@cadl-lang/versioning"; @@ -346,16 +341,12 @@ function createOAPIEmitter(program: Program, options: OpenAPIEmitterOptions) { emitEndpointParameters(op, op.parameters, parameters.parameters); emitRequestBody(op, op.parameters, parameters); - emitResponses(op.returnType); + emitResponses(operation.responses); } - function emitResponses(responseType: Type) { - if (responseType.kind === "Union") { - for (const option of responseType.options) { - emitResponseObject(option); - } - } else { - emitResponseObject(responseType); + function emitResponses(responses: HttpOperationResponse[]) { + for (const response of responses) { + emitResponseObject(response); } } @@ -368,222 +359,39 @@ function createOAPIEmitter(program: Program, options: OpenAPIEmitterOptions) { ); } - function emitResponseObject(responseModel: Type) { - // Get explicity defined status codes - const statusCodes = getResponseStatusCodes(responseModel); - - // Get explicitly defined content types - const contentTypes = getResponseContentTypes(responseModel); + function emitResponseObject(response: HttpOperationResponse) { + const statusCode = + response.statusCode ?? isErrorModel(program, response.type) ? "default" : "200"; + const openapiResponse = currentEndpoint.responses[statusCode] ?? { + description: response.description ?? getResponseDescriptionForStatusCode(statusCode), + }; - // Get response headers - const headers = getResponseHeaders(responseModel); + if (response.headers) { + response.headers = {}; - // Get explicitly defined body - let bodyModel = getResponseBody(responseModel); - // If there is no explicit body, it should be conjured from the return type - // if it is a primitive type or it contains more than just response metadata - if (!bodyModel) { - if (responseModel.kind === "Model") { - if (mapCadlTypeToOpenAPI(responseModel)) { - bodyModel = responseModel; - } else { - const isResponseMetadata = (p: ModelTypeProperty) => - isHeader(program, p) || isStatusCode(program, p); - const allProperties = (p: ModelType): ModelTypeProperty[] => { - return [...p.properties.values(), ...(p.baseModel ? allProperties(p.baseModel) : [])]; - }; - if (allProperties(responseModel).some((p) => !isResponseMetadata(p))) { - bodyModel = responseModel; - } - } - } else { - // body is array or possibly something else - bodyModel = responseModel; + for (const [key, value] of Object.entries(response.headers)) { + response.headers[key] = getResponseHeader(value); } } - // If there is no explicit status code, set the default - if (statusCodes.length === 0) { - statusCodes.push(getDefaultStatusCode(responseModel, bodyModel)); - } - - // If there is a body but no explicit content types, use application/json - if (bodyModel && !isVoidType(bodyModel) && contentTypes.length === 0) { - contentTypes.push("application/json"); - } - - if (!bodyModel && contentTypes.length > 0) { - reportDiagnostic(program, { - code: "content-type-ignored", - target: responseModel, - }); - } - - // Assertion: bodyModel <=> contentTypes.length > 0 - - // 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 = currentEndpoint.responses[statusCode] ?? { - description: getResponseDescription(responseModel, statusCode), - }; - - // check for duplicates - if (response.content) { - for (const contentType of contentTypes) { - if (response.content[contentType]) { - reportDiagnostic(program, { - code: "duplicate-response", - format: { statusCode, contentType }, - target: responseModel, - }); - } - } - } - - if (Object.keys(headers).length > 0) { - response.headers ??= {}; - - // OpenAPI can't represent different headers per content type. - // So we merge headers here, and report any duplicates. - // It may be possible in principle to not error for identically declared - // headers. - for (const [key, value] of Object.entries(headers)) { - if (response.headers[key]) { - reportDiagnostic(program, { - code: "duplicate-header", - format: { header: key }, - target: responseModel, - }); - continue; - } - - response.headers[key] = value; - } - } - - for (const contentType of contentTypes) { - response.content ??= {}; - const isBinary = isBinaryPayload(bodyModel!, contentType); + if (response.body !== undefined) { + for (const [contentType, bodyModel] of Object.entries(response.body)) { + openapiResponse.content = {}; + const isBinary = isBinaryPayload(bodyModel, contentType); const schema = isBinary ? { type: "string", format: "binary" } : getSchemaOrRef(bodyModel!); - response.content[contentType] = { schema }; + openapiResponse.content[contentType] = { schema }; } - currentEndpoint.responses[statusCode] = response; } + currentEndpoint.responses[statusCode] = openapiResponse; } - /** - * Return the default status code for the given response/body - * @param model representing the body - */ - function getDefaultStatusCode(responseModel: Type, bodyModel: Type | undefined) { - if (bodyModel === undefined || isVoidType(bodyModel)) { - return "204"; - } else { - return isErrorModel(program, responseModel) ? "default" : "200"; - } - } - - // Get explicity defined status codes from response Model - // Return is an array of strings, possibly empty, which indicates no explicitly defined status codes. - // We do not check for duplicates here -- that will be done by the caller. - function getResponseStatusCodes(responseModel: Type): string[] { - const codes: string[] = []; - if (responseModel.kind === "Model") { - if (responseModel.baseModel) { - codes.push(...getResponseStatusCodes(responseModel.baseModel)); - } - for (const prop of responseModel.properties.values()) { - if (isStatusCode(program, prop)) { - codes.push(...getStatusCodes(program, prop)); - } - } - } - if (isDefaultResponse(program, responseModel)) { - if (codes.length > 0) { - reportDiagnostic(program, { - code: "status-code-in-default-response", - target: responseModel, - }); - } else { - codes.push("default"); - } - } - return codes; - } - - function getResponseDescription(responseModel: Type, statusCode: string) { - const desc = getDoc(program, responseModel); - if (desc) { - return desc; - } - + function getResponseDescriptionForStatusCode(statusCode: string) { if (statusCode === "default") { return "An unexpected error response"; } return getStatusCodeDescription(statusCode) ?? "unknown"; } - // Get explicity defined content-types from response Model - // Return is an array of strings, possibly empty, which indicates no explicitly defined content-type. - // We do not check for duplicates here -- that will be done by the caller. - function getResponseContentTypes(responseModel: Type): string[] { - const contentTypes: string[] = []; - if (responseModel.kind === "Model") { - if (responseModel.baseModel) { - contentTypes.push(...getResponseContentTypes(responseModel.baseModel)); - } - for (const prop of responseModel.properties.values()) { - if (isHeader(program, prop) && getHeaderFieldName(program, prop) === "content-type") { - contentTypes.push(...getContentTypes(prop)); - } - } - } - return contentTypes; - } - - // Get response headers from response Model - function getResponseHeaders(responseModel: Type) { - if (responseModel.kind === "Model") { - const responseHeaders: any = responseModel.baseModel - ? getResponseHeaders(responseModel.baseModel) - : {}; - for (const prop of responseModel.properties.values()) { - const headerName = getHeaderFieldName(program, prop); - if (isHeader(program, prop) && headerName !== "content-type") { - responseHeaders[headerName] = getResponseHeader(prop); - } - } - return responseHeaders; - } - return {}; - } - - // Get explicity defined response body from response Model - // Search inheritance chain and error on any duplicates found - function getResponseBody(responseModel: Type): Type | undefined { - if (responseModel.kind === "Model") { - const getAllBodyProps = (m: ModelType): ModelTypeProperty[] => { - const bodyProps = [...m.properties.values()].filter((t) => isBody(program, t)); - if (m.baseModel) { - bodyProps.push(...getAllBodyProps(m.baseModel)); - } - return bodyProps; - }; - const bodyProps = getAllBodyProps(responseModel); - if (bodyProps.length > 0) { - // Report all but first body as duplicate - for (const prop of bodyProps.slice(1)) { - reportDiagnostic(program, { code: "duplicate-body", target: prop }); - } - return bodyProps[0].type; - } - } - return undefined; - } - function getResponseHeader(prop: ModelTypeProperty) { const header: any = {}; populateParameter(header, prop, undefined); diff --git a/packages/rest/src/diagnostics.ts b/packages/rest/src/diagnostics.ts index c3425af7ab1..6cf0cce1b17 100644 --- a/packages/rest/src/diagnostics.ts +++ b/packages/rest/src/diagnostics.ts @@ -95,6 +95,30 @@ const libDefinition = { value: "statusCode value must be a three digit code between 100 and 599", }, }, + "content-type-string": { + severity: "error", + messages: { + default: "contentType parameter must be a string literal or union of string literals", + }, + }, + "duplicate-response": { + severity: "error", + messages: { + default: paramMessage`Multiple return types for content type ${"contentType"} and status code ${"statusCode"}`, + }, + }, + "duplicate-header": { + severity: "error", + messages: { + default: paramMessage`The header ${"header"} is defined across multiple content types`, + }, + }, + "content-type-ignored": { + severity: "warning", + messages: { + default: "content-type header ignored because return type has no body", + }, + }, }, } as const; diff --git a/packages/rest/src/http.ts b/packages/rest/src/http.ts index abcd4d95c4e..c2882b344c6 100644 --- a/packages/rest/src/http.ts +++ b/packages/rest/src/http.ts @@ -150,7 +150,7 @@ export function isStatusCode(program: Program, entity: Type) { return program.stateMap(statusCodeKey).has(entity); } -export function getStatusCodes(program: Program, entity: Type) { +export function getStatusCodes(program: Program, entity: Type): string[] { return program.stateMap(statusCodeKey).get(entity); } diff --git a/packages/rest/src/lib.ts b/packages/rest/src/lib.ts index d854de9710a..c1169037ff7 100644 --- a/packages/rest/src/lib.ts +++ b/packages/rest/src/lib.ts @@ -1,6 +1,7 @@ export * as http from "./http.js"; export * from "./resource.js"; export * as resource from "./resource.js"; +export * from "./responses.js"; export * from "./rest.js"; export * as rest from "./rest.js"; export * from "./route.js"; diff --git a/packages/rest/src/responses.ts b/packages/rest/src/responses.ts new file mode 100644 index 00000000000..574e3244b3a --- /dev/null +++ b/packages/rest/src/responses.ts @@ -0,0 +1,294 @@ +import { + getDoc, + isIntrinsic, + isVoidType, + ModelType, + ModelTypeProperty, + OperationType, + Program, + Type, +} from "@cadl-lang/compiler"; +import { reportDiagnostic } from "./diagnostics.js"; +import { + getHeaderFieldName, + getStatusCodeDescription, + getStatusCodes, + isBody, + isHeader, + isStatusCode, +} from "./http.js"; + +export interface HttpOperationResponse { + statusCode: string | undefined; + type: Type; + description?: string; + headers?: Record; + body?: Record; +} + +const NoStatusCode = Symbol("NoStatusCode"); + +/** + * Get the responses for a given operation. + */ +export function getResponsesForOperation( + program: Program, + operation: OperationType +): HttpOperationResponse[] { + const responseType = operation.returnType; + const responses: Record = {}; + if (responseType.kind === "Union") { + for (const option of responseType.options) { + processResponseType(program, responses, option); + } + } else { + processResponseType(program, responses, responseType); + } + + const result = Object.values(responses); + if (responses[NoStatusCode]) { + result.push(responses[NoStatusCode]); + } + return result; +} + +function processResponseType( + program: Program, + responses: Record, + responseModel: Type +) { + // Get explicity defined status codes + const statusCodes: Array = getResponseStatusCodes( + program, + responseModel + ); + + // Get explicitly defined content types + const contentTypes = getResponseContentTypes(program, responseModel); + + // Get response headers + const headers = getResponseHeaders(program, responseModel); + + // Get explicitly defined body + let bodyModel = getResponseBody(program, responseModel); + // If there is no explicit body, it should be conjured from the return type + // if it is a primitive type or it contains more than just response metadata + if (!bodyModel) { + if (responseModel.kind === "Model") { + if (isIntrinsic(program, responseModel)) { + bodyModel = responseModel; + } else { + const isResponseMetadata = (p: ModelTypeProperty) => + isHeader(program, p) || isStatusCode(program, p); + const allProperties = (p: ModelType): ModelTypeProperty[] => { + return [...p.properties.values(), ...(p.baseModel ? allProperties(p.baseModel) : [])]; + }; + if (allProperties(responseModel).some((p) => !isResponseMetadata(p))) { + bodyModel = responseModel; + } + } + } else { + // body is array or possibly something else + bodyModel = responseModel; + } + } + + // If there is no explicit status code, check if it should be 204 + if (statusCodes.length === 0) { + if (bodyModel === undefined || isVoidType(bodyModel)) { + statusCodes.push("204"); + } else { + statusCodes.push(NoStatusCode); + } + } + + // If there is a body but no explicit content types, use application/json + if (bodyModel && !isVoidType(bodyModel) && contentTypes.length === 0) { + contentTypes.push("application/json"); + } + + // Assertion: bodyModel <=> contentTypes.length > 0 + + // 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[statusCode] ?? { + statusCode: statusCode === NoStatusCode ? undefined : statusCode, + type: responseModel, + description: getResponseDescription(program, responseModel, statusCode), + }; + + // check for duplicates + if (response.body) { + for (const contentType of contentTypes) { + if (response.body[contentType]) { + reportDiagnostic(program, { + code: "duplicate-response", + format: { statusCode: statusCode.toString(), contentType }, + target: responseModel, + }); + } + } + } + + if (Object.keys(headers).length > 0) { + response.headers ??= {}; + + // OpenAPI can't represent different headers per content type. + // So we merge headers here, and report any duplicates. + // It may be possible in principle to not error for identically declared + // headers. + for (const [key, value] of Object.entries(headers)) { + if (response.headers[key]) { + reportDiagnostic(program, { + code: "duplicate-header", + format: { header: key }, + target: responseModel, + }); + continue; + } + + response.headers[key] = value; + } + } + + if (bodyModel !== undefined) { + for (const contentType of contentTypes) { + response.body ??= {}; + response.body[contentType] = bodyModel; + } + } else if (contentTypes.length > 0) { + reportDiagnostic(program, { + code: "content-type-ignored", + target: responseModel, + }); + } + responses[statusCode] = response; + } +} + +/** + * Get explicity defined status codes from response Model + * Return is an array of strings, possibly empty, which indicates no explicitly defined status codes. + * We do not check for duplicates here -- that will be done by the caller. + */ +function getResponseStatusCodes(program: Program, responseModel: Type): string[] { + const codes: string[] = []; + if (responseModel.kind === "Model") { + if (responseModel.baseModel) { + codes.push(...getResponseStatusCodes(program, responseModel.baseModel)); + } + for (const prop of responseModel.properties.values()) { + if (isStatusCode(program, prop)) { + codes.push(...getStatusCodes(program, prop)); + } + } + } + return codes; +} + +/** + * Get explicity defined content-types from response Model + * Return is an array of strings, possibly empty, which indicates no explicitly defined content-type. + * We do not check for duplicates here -- that will be done by the caller. + */ +function getResponseContentTypes(program: Program, responseModel: Type): string[] { + const contentTypes: string[] = []; + if (responseModel.kind === "Model") { + if (responseModel.baseModel) { + contentTypes.push(...getResponseContentTypes(program, responseModel.baseModel)); + } + for (const prop of responseModel.properties.values()) { + if (isHeader(program, prop) && getHeaderFieldName(program, prop) === "content-type") { + contentTypes.push(...getContentTypes(program, prop)); + } + } + } + return contentTypes; +} + +function getContentTypes(program: Program, param: ModelTypeProperty): string[] { + if (param.type.kind === "String") { + return [param.type.value]; + } else if (param.type.kind === "Union") { + const contentTypes = []; + for (const option of param.type.options) { + if (option.kind === "String") { + contentTypes.push(option.value); + } else { + reportDiagnostic(program, { + code: "content-type-string", + target: param, + }); + continue; + } + } + + return contentTypes; + } + + reportDiagnostic(program, { code: "content-type-string", target: param }); + + return []; +} + +/** + * Get response headers from response Model + */ +function getResponseHeaders( + program: Program, + responseModel: Type +): Record { + if (responseModel.kind === "Model") { + const responseHeaders: any = responseModel.baseModel + ? getResponseHeaders(program, responseModel.baseModel) + : {}; + for (const prop of responseModel.properties.values()) { + const headerName = getHeaderFieldName(program, prop); + if (isHeader(program, prop) && headerName !== "content-type") { + responseHeaders[headerName] = prop; + } + } + return responseHeaders; + } + return {}; +} + +function getResponseBody(program: Program, responseModel: Type): Type | undefined { + if (responseModel.kind === "Model") { + const getAllBodyProps = (m: ModelType): ModelTypeProperty[] => { + const bodyProps = [...m.properties.values()].filter((t) => isBody(program, t)); + if (m.baseModel) { + bodyProps.push(...getAllBodyProps(m.baseModel)); + } + return bodyProps; + }; + const bodyProps = getAllBodyProps(responseModel); + if (bodyProps.length > 0) { + // Report all but first body as duplicate + for (const prop of bodyProps.slice(1)) { + reportDiagnostic(program, { code: "duplicate-body", target: prop }); + } + return bodyProps[0].type; + } + } + return undefined; +} + +function getResponseDescription( + program: Program, + responseModel: Type, + statusCode: string | typeof NoStatusCode +) { + if (statusCode === NoStatusCode) { + return undefined; + } + const desc = getDoc(program, responseModel); + if (desc) { + return desc; + } + + return getStatusCodeDescription(statusCode) ?? "unknown"; +} diff --git a/packages/rest/src/route.ts b/packages/rest/src/route.ts index a1ca379f005..14c16766764 100644 --- a/packages/rest/src/route.ts +++ b/packages/rest/src/route.ts @@ -19,6 +19,7 @@ import { HttpVerb, isBody, } from "./http.js"; +import { getResponsesForOperation, HttpOperationResponse } from "./responses.js"; import { getAction, getResourceOperation, getSegment } from "./rest.js"; export type OperationContainer = NamespaceType | InterfaceType; @@ -41,6 +42,7 @@ export interface OperationDetails { groupName: string; container: OperationContainer; parameters: HttpOperationParameters; + responses: HttpOperationResponse[]; operation: OperationType; } @@ -347,6 +349,7 @@ function buildRoutes( const route = getPathForOperation(program, op, parentFragments); const verb = getVerbForOperation(program, op, route.parameters); + const responses = getResponsesForOperation(program, op); operations.push({ path: route.path, pathFragment: route.pathFragment, @@ -355,6 +358,7 @@ function buildRoutes( groupName: container.name, parameters: route.parameters, operation: op, + responses, }); } From 8e735c7af2461dba5db3ec9c4f291b1f24eb8d9c Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Tue, 22 Mar 2022 14:52:13 -0700 Subject: [PATCH 2/9] Changelog --- .../http-common-responses_2022-03-22-21-51.json | 10 ++++++++++ .../rest/http-common-responses_2022-03-22-21-51.json | 10 ++++++++++ 2 files changed, 20 insertions(+) create mode 100644 common/changes/@cadl-lang/openapi3/http-common-responses_2022-03-22-21-51.json create mode 100644 common/changes/@cadl-lang/rest/http-common-responses_2022-03-22-21-51.json diff --git a/common/changes/@cadl-lang/openapi3/http-common-responses_2022-03-22-21-51.json b/common/changes/@cadl-lang/openapi3/http-common-responses_2022-03-22-21-51.json new file mode 100644 index 00000000000..2d093c740d0 --- /dev/null +++ b/common/changes/@cadl-lang/openapi3/http-common-responses_2022-03-22-21-51.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@cadl-lang/openapi3", + "comment": "Moved http response interpretation to @cadl-lang/rest library.", + "type": "minor" + } + ], + "packageName": "@cadl-lang/openapi3" +} diff --git a/common/changes/@cadl-lang/rest/http-common-responses_2022-03-22-21-51.json b/common/changes/@cadl-lang/rest/http-common-responses_2022-03-22-21-51.json new file mode 100644 index 00000000000..839b6cfc69d --- /dev/null +++ b/common/changes/@cadl-lang/rest/http-common-responses_2022-03-22-21-51.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@cadl-lang/rest", + "comment": "Add logic to interpret the http responses.", + "type": "minor" + } + ], + "packageName": "@cadl-lang/rest" +} From 92eb584530ec7682e4ed515f69b56da4801faa83 Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Tue, 22 Mar 2022 16:18:15 -0700 Subject: [PATCH 3/9] Some fixes to tests --- packages/openapi3/src/lib.ts | 28 ----------------- packages/openapi3/src/openapi.ts | 10 +++--- packages/openapi3/test/test-return-types.ts | 33 +------------------ packages/rest/src/responses.ts | 3 -- packages/rest/test/test-responses.ts | 35 +++++++++++++++++++++ 5 files changed, 41 insertions(+), 68 deletions(-) create mode 100644 packages/rest/test/test-responses.ts diff --git a/packages/openapi3/src/lib.ts b/packages/openapi3/src/lib.ts index 96da9fa3d48..03dda9e780f 100644 --- a/packages/openapi3/src/lib.ts +++ b/packages/openapi3/src/lib.ts @@ -21,34 +21,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", - }, - }, - // TODO-REMOVE moved to rest lib - "duplicate-response": { - severity: "error", - messages: { - default: paramMessage`Multiple return types for content type ${"contentType"} and status code ${"statusCode"}`, - }, - }, - // TODO-REMOVE moved to rest lib - "duplicate-header": { - severity: "error", - messages: { - default: paramMessage`The header ${"header"} is defined across multiple content types`, - }, - }, - // TODO-REMOVE moved to rest lib - "content-type-ignored": { - severity: "warning", - messages: { - default: "content-type header ignored because return type has no body", - }, - }, - // TODO-REMOVE moved to rest lib "content-type-string": { severity: "error", messages: { diff --git a/packages/openapi3/src/openapi.ts b/packages/openapi3/src/openapi.ts index bca00de2a46..60794925361 100644 --- a/packages/openapi3/src/openapi.ts +++ b/packages/openapi3/src/openapi.ts @@ -359,24 +359,24 @@ function createOAPIEmitter(program: Program, options: OpenAPIEmitterOptions) { ); } - function emitResponseObject(response: HttpOperationResponse) { + function emitResponseObject(response: Readonly) { const statusCode = - response.statusCode ?? isErrorModel(program, response.type) ? "default" : "200"; + response.statusCode ?? (isErrorModel(program, response.type) ? "default" : "200"); const openapiResponse = currentEndpoint.responses[statusCode] ?? { description: response.description ?? getResponseDescriptionForStatusCode(statusCode), }; if (response.headers) { - response.headers = {}; + openapiResponse.headers = {}; for (const [key, value] of Object.entries(response.headers)) { - response.headers[key] = getResponseHeader(value); + openapiResponse.headers[key] = getResponseHeader(value); } } if (response.body !== undefined) { + openapiResponse.content = {}; for (const [contentType, bodyModel] of Object.entries(response.body)) { - openapiResponse.content = {}; const isBinary = isBinaryPayload(bodyModel, contentType); const schema = isBinary ? { type: "string", format: "binary" } : getSchemaOrRef(bodyModel!); openapiResponse.content[contentType] = { schema }; diff --git a/packages/openapi3/test/test-return-types.ts b/packages/openapi3/test/test-return-types.ts index 87ef7c341b5..58f7cb8b381 100644 --- a/packages/openapi3/test/test-return-types.ts +++ b/packages/openapi3/test/test-return-types.ts @@ -338,37 +338,6 @@ describe("openapi3: return types", () => { ok(diagnostics[2].message.includes("must be a numeric or string literal")); }); - it("issues diagnostics for invalid content types", async () => { - const diagnostics = await checkFor( - ` - model Foo { - foo: string; - } - - model TextPlain { - 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 }; - } - ` - ); - expectDiagnostics(diagnostics, [ - { code: "@cadl-lang/openapi3/content-type-string" }, - { code: "@cadl-lang/openapi3/content-type-string" }, - { code: "@cadl-lang/openapi3/content-type-string" }, - ]); - }); - it("defines responses with primitive types", async () => { const res = await openApiFor( ` @@ -489,7 +458,7 @@ describe("openapi3: return types", () => { }); }); - it("defaults status code to default when model has @error decorator and explicit body", async () => { + it.only("defaults status code to default when model has @error decorator and explicit body", async () => { const res = await openApiFor( ` @error diff --git a/packages/rest/src/responses.ts b/packages/rest/src/responses.ts index 574e3244b3a..6c694a55a51 100644 --- a/packages/rest/src/responses.ts +++ b/packages/rest/src/responses.ts @@ -107,10 +107,7 @@ function processResponseType( contentTypes.push("application/json"); } - // Assertion: bodyModel <=> contentTypes.length > 0 - // 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. diff --git a/packages/rest/test/test-responses.ts b/packages/rest/test/test-responses.ts new file mode 100644 index 00000000000..fdfc9d0aef1 --- /dev/null +++ b/packages/rest/test/test-responses.ts @@ -0,0 +1,35 @@ +import { expectDiagnostics } from "@cadl-lang/compiler/testing"; +import { compileOperations } from "./test-host.js"; + +describe("cadl: rest: responses", () => { + it("issues diagnostics for invalid content types", async () => { + const [_, diagnostics] = await compileOperations( + ` + model Foo { + foo: string; + } + + model TextPlain { + 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 }; + } + ` + ); + expectDiagnostics(diagnostics, [ + { code: "@cadl-lang/rest/content-type-string" }, + { code: "@cadl-lang/rest/content-type-string" }, + { code: "@cadl-lang/rest/content-type-string" }, + ]); + }); +}); From adaa2ce6ff470acead82119cf5a7eb9fe9a49a9e Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Tue, 22 Mar 2022 16:53:27 -0700 Subject: [PATCH 4/9] Add * status code type --- packages/openapi/src/decorators.ts | 6 +-- packages/openapi3/src/openapi.ts | 16 +++++--- packages/openapi3/test/test-return-types.ts | 44 +-------------------- packages/rest/src/http.ts | 12 +++++- packages/rest/src/responses.ts | 35 ++++++---------- packages/rest/test/test-responses.ts | 41 +++++++++++++++++++ 6 files changed, 77 insertions(+), 77 deletions(-) diff --git a/packages/openapi/src/decorators.ts b/packages/openapi/src/decorators.ts index 979475a4f03..9edd510a3c1 100644 --- a/packages/openapi/src/decorators.ts +++ b/packages/openapi/src/decorators.ts @@ -5,6 +5,7 @@ import { validateDecoratorParamType, validateDecoratorTarget, } from "@cadl-lang/compiler"; +import { http } from "@cadl-lang/rest"; import { reportDiagnostic } from "./lib.js"; const operationIdsKey = Symbol(); @@ -64,13 +65,10 @@ export function $defaultResponse({ program }: DecoratorContext, entity: Type) { if (!validateDecoratorTarget(program, entity, "@defaultResponse", "Model")) { return; } + http.setStatusCode(program, entity, ["*"]); program.stateSet(defaultResponseKey).add(entity); } -export function isDefaultResponse(program: Program, entity: Type): boolean { - return program.stateSet(defaultResponseKey).has(entity); -} - export interface ExternalDocs { url: string; description?: string; diff --git a/packages/openapi3/src/openapi.ts b/packages/openapi3/src/openapi.ts index 60794925361..9f17393f167 100644 --- a/packages/openapi3/src/openapi.ts +++ b/packages/openapi3/src/openapi.ts @@ -25,7 +25,6 @@ import { getServiceVersion, getSummary, getVisibility, - isErrorModel, isErrorType, isIntrinsic, isNumericType, @@ -60,10 +59,7 @@ const { getHeaderFieldName, getPathParamName, getQueryParamName, - isBody, - isHeader, isStatusCode, - getStatusCodes, getStatusCodeDescription, } = http; @@ -359,9 +355,17 @@ function createOAPIEmitter(program: Program, options: OpenAPIEmitterOptions) { ); } + function getOpenAPIStatuscode(response: HttpOperationResponse): string { + switch (response.statusCode) { + case "*": + return "default"; + default: + return response.statusCode; + } + } + function emitResponseObject(response: Readonly) { - const statusCode = - response.statusCode ?? (isErrorModel(program, response.type) ? "default" : "200"); + const statusCode = getOpenAPIStatuscode(response); const openapiResponse = currentEndpoint.responses[statusCode] ?? { description: response.description ?? getResponseDescriptionForStatusCode(statusCode), }; diff --git a/packages/openapi3/test/test-return-types.ts b/packages/openapi3/test/test-return-types.ts index 58f7cb8b381..ac323d4d4a7 100644 --- a/packages/openapi3/test/test-return-types.ts +++ b/packages/openapi3/test/test-return-types.ts @@ -262,48 +262,6 @@ describe("openapi3: return types", () => { ok(res.paths["/"].get.responses["200"].content["text/csv"]); }); - it("issues diagnostics for duplicate body decorator", async () => { - const diagnostics = await checkFor( - ` - model Foo { - foo: string; - } - model Bar { - bar: string; - } - @route("/") - namespace root { - @get - op read(): { @body body1: Foo, @body body2: Bar }; - } - ` - ); - expectDiagnostics(diagnostics, [{ code: "@cadl-lang/openapi3/duplicate-body" }]); - }); - - it("issues diagnostics for return type with duplicate status code", async () => { - const diagnostics = await checkFor( - ` - model Foo { - foo: string; - } - model Error { - code: string; - } - @route("/") - namespace root { - @get - op read(): Foo | Error; - } - ` - ); - expectDiagnostics(diagnostics, [{ code: "@cadl-lang/openapi3/duplicate-response" }]); - strictEqual( - diagnostics[0].message, - "Multiple return types for content type application/json and status code 200" - ); - }); - it("issues diagnostics for invalid status codes", async () => { const diagnostics = await checkFor( ` @@ -458,7 +416,7 @@ describe("openapi3: return types", () => { }); }); - it.only("defaults status code to default when model has @error decorator and explicit body", async () => { + it("defaults status code to default when model has @error decorator and explicit body", async () => { const res = await openApiFor( ` @error diff --git a/packages/rest/src/http.ts b/packages/rest/src/http.ts index c2882b344c6..ac3fdbda44f 100644 --- a/packages/rest/src/http.ts +++ b/packages/rest/src/http.ts @@ -1,5 +1,7 @@ import { DecoratorContext, + ModelType, + ModelTypeProperty, Program, setDecoratorNamespace, Type, @@ -128,6 +130,14 @@ export function $statusCode({ program }: DecoratorContext, entity: Type) { } else { reportDiagnostic(program, { code: "status-code-invalid", target: entity }); } + setStatusCode(program, entity, codes); +} + +export function setStatusCode( + program: Program, + entity: ModelType | ModelTypeProperty, + codes: string[] +) { program.stateMap(statusCodeKey).set(entity, codes); } @@ -151,7 +161,7 @@ export function isStatusCode(program: Program, entity: Type) { } export function getStatusCodes(program: Program, entity: Type): string[] { - return program.stateMap(statusCodeKey).get(entity); + return program.stateMap(statusCodeKey).get(entity) ?? []; } // Note: these descriptions come from https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html diff --git a/packages/rest/src/responses.ts b/packages/rest/src/responses.ts index 6c694a55a51..3b5248a1e9e 100644 --- a/packages/rest/src/responses.ts +++ b/packages/rest/src/responses.ts @@ -1,5 +1,6 @@ import { getDoc, + isErrorModel, isIntrinsic, isVoidType, ModelType, @@ -18,16 +19,15 @@ import { isStatusCode, } from "./http.js"; +export type StatusCode = `${number}` | "*"; export interface HttpOperationResponse { - statusCode: string | undefined; + statusCode: StatusCode; type: Type; description?: string; headers?: Record; body?: Record; } -const NoStatusCode = Symbol("NoStatusCode"); - /** * Get the responses for a given operation. */ @@ -45,23 +45,16 @@ export function getResponsesForOperation( processResponseType(program, responses, responseType); } - const result = Object.values(responses); - if (responses[NoStatusCode]) { - result.push(responses[NoStatusCode]); - } - return result; + return Object.values(responses); } function processResponseType( program: Program, - responses: Record, + responses: Record, responseModel: Type ) { // Get explicity defined status codes - const statusCodes: Array = getResponseStatusCodes( - program, - responseModel - ); + const statusCodes: Array = getResponseStatusCodes(program, responseModel); // Get explicitly defined content types const contentTypes = getResponseContentTypes(program, responseModel); @@ -97,8 +90,10 @@ function processResponseType( if (statusCodes.length === 0) { if (bodyModel === undefined || isVoidType(bodyModel)) { statusCodes.push("204"); + } else if (isErrorModel(program, responseModel)) { + statusCodes.push("*"); } else { - statusCodes.push(NoStatusCode); + statusCodes.push("200"); } } @@ -112,7 +107,7 @@ function processResponseType( // the first model for this statusCode/content type pair carries the // description for the endpoint. This could probably be improved. const response: HttpOperationResponse = responses[statusCode] ?? { - statusCode: statusCode === NoStatusCode ? undefined : statusCode, + statusCode: statusCode, type: responseModel, description: getResponseDescription(program, responseModel, statusCode), }; @@ -177,6 +172,7 @@ function getResponseStatusCodes(program: Program, responseModel: Type): string[] if (responseModel.baseModel) { codes.push(...getResponseStatusCodes(program, responseModel.baseModel)); } + codes.push(...getStatusCodes(program, responseModel)); for (const prop of responseModel.properties.values()) { if (isStatusCode(program, prop)) { codes.push(...getStatusCodes(program, prop)); @@ -274,14 +270,7 @@ function getResponseBody(program: Program, responseModel: Type): Type | undefine return undefined; } -function getResponseDescription( - program: Program, - responseModel: Type, - statusCode: string | typeof NoStatusCode -) { - if (statusCode === NoStatusCode) { - return undefined; - } +function getResponseDescription(program: Program, responseModel: Type, statusCode: string) { const desc = getDoc(program, responseModel); if (desc) { return desc; diff --git a/packages/rest/test/test-responses.ts b/packages/rest/test/test-responses.ts index fdfc9d0aef1..918883e8975 100644 --- a/packages/rest/test/test-responses.ts +++ b/packages/rest/test/test-responses.ts @@ -2,6 +2,47 @@ import { expectDiagnostics } from "@cadl-lang/compiler/testing"; import { compileOperations } from "./test-host.js"; describe("cadl: rest: responses", () => { + it("issues 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 }; + } + ` + ); + expectDiagnostics(diagnostics, [{ code: "@cadl-lang/openapi3/duplicate-body" }]); + }); + + it("issues diagnostics for return type with duplicate status code", async () => { + const [_, diagnostics] = await compileOperations( + ` + model Foo { + foo: string; + } + model Error { + code: string; + } + @route("/") + namespace root { + @get + op read(): Foo | Error; + } + ` + ); + expectDiagnostics(diagnostics, { + code: "@cadl-lang/openapi3/duplicate-response", + message: "Multiple return types for content type application/json and status code 200", + }); + }); + it("issues diagnostics for invalid content types", async () => { const [_, diagnostics] = await compileOperations( ` From dbe92c0116cb9575e6583b06f090ccef22f20cff Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Tue, 22 Mar 2022 17:20:38 -0700 Subject: [PATCH 5/9] Handle headers per model --- packages/openapi3/src/lib.ts | 12 +++++++ packages/openapi3/src/openapi.ts | 38 +++++++++++++------- packages/rest/src/diagnostics.ts | 6 ---- packages/rest/src/responses.ts | 54 +++++++++++----------------- packages/rest/test/test-responses.ts | 4 +-- 5 files changed, 61 insertions(+), 53 deletions(-) diff --git a/packages/openapi3/src/lib.ts b/packages/openapi3/src/lib.ts index 03dda9e780f..08c124b4df5 100644 --- a/packages/openapi3/src/lib.ts +++ b/packages/openapi3/src/lib.ts @@ -21,6 +21,18 @@ 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: { + default: paramMessage`The header ${"header"} is defined across multiple content types`, + }, + }, "content-type-string": { severity: "error", messages: { diff --git a/packages/openapi3/src/openapi.ts b/packages/openapi3/src/openapi.ts index 9f17393f167..5fb8825a21a 100644 --- a/packages/openapi3/src/openapi.ts +++ b/packages/openapi3/src/openapi.ts @@ -370,22 +370,36 @@ function createOAPIEmitter(program: Program, options: OpenAPIEmitterOptions) { description: response.description ?? getResponseDescriptionForStatusCode(statusCode), }; - if (response.headers) { - openapiResponse.headers = {}; - - for (const [key, value] of Object.entries(response.headers)) { - openapiResponse.headers[key] = getResponseHeader(value); + for (const data of response.responses) { + if (data.headers) { + openapiResponse.headers ??= {}; + // OpenAPI can't represent different headers per content type. + // So we merge headers here, and report any duplicates. + // It may be possible in principle to not error for identically declared + // headers. + for (const [key, value] of Object.entries(data.headers)) { + if (openapiResponse.headers[key]) { + reportDiagnostic(program, { + code: "duplicate-header", + format: { header: key }, + target: response.type, + }); + continue; + } + openapiResponse.headers[key] = getResponseHeader(value); + } } - } - if (response.body !== undefined) { - openapiResponse.content = {}; - for (const [contentType, bodyModel] of Object.entries(response.body)) { - const isBinary = isBinaryPayload(bodyModel, contentType); - const schema = isBinary ? { type: "string", format: "binary" } : getSchemaOrRef(bodyModel!); - openapiResponse.content[contentType] = { schema }; + if (data.body !== undefined) { + openapiResponse.content ??= {}; + const isBinary = isBinaryPayload(data.body.type, data.body.contentType); + const schema = isBinary + ? { type: "string", format: "binary" } + : getSchemaOrRef(data.body.type); + openapiResponse.content[data.body.contentType] = { schema }; } } + currentEndpoint.responses[statusCode] = openapiResponse; } diff --git a/packages/rest/src/diagnostics.ts b/packages/rest/src/diagnostics.ts index 6cf0cce1b17..48aab30c83e 100644 --- a/packages/rest/src/diagnostics.ts +++ b/packages/rest/src/diagnostics.ts @@ -107,12 +107,6 @@ const libDefinition = { default: paramMessage`Multiple return types for content type ${"contentType"} and status code ${"statusCode"}`, }, }, - "duplicate-header": { - severity: "error", - messages: { - default: paramMessage`The header ${"header"} is defined across multiple content types`, - }, - }, "content-type-ignored": { severity: "warning", messages: { diff --git a/packages/rest/src/responses.ts b/packages/rest/src/responses.ts index 3b5248a1e9e..1bb74c2d467 100644 --- a/packages/rest/src/responses.ts +++ b/packages/rest/src/responses.ts @@ -24,8 +24,17 @@ export interface HttpOperationResponse { statusCode: StatusCode; type: Type; description?: string; + responses: HttpOperationResponseContent[]; +} + +export interface HttpOperationResponseContent { headers?: Record; - body?: Record; + body?: HttpOperationBody; +} + +export interface HttpOperationBody { + contentType: string; + type: Type; } /** @@ -110,52 +119,31 @@ function processResponseType( statusCode: statusCode, type: responseModel, description: getResponseDescription(program, responseModel, statusCode), + responses: [], }; // check for duplicates - if (response.body) { - for (const contentType of contentTypes) { - if (response.body[contentType]) { - reportDiagnostic(program, { - code: "duplicate-response", - format: { statusCode: statusCode.toString(), contentType }, - target: responseModel, - }); - } - } - } - - if (Object.keys(headers).length > 0) { - response.headers ??= {}; - - // OpenAPI can't represent different headers per content type. - // So we merge headers here, and report any duplicates. - // It may be possible in principle to not error for identically declared - // headers. - for (const [key, value] of Object.entries(headers)) { - if (response.headers[key]) { - reportDiagnostic(program, { - code: "duplicate-header", - format: { header: key }, - target: responseModel, - }); - continue; - } - - response.headers[key] = value; + for (const contentType of contentTypes) { + if (response.responses.find((x) => x.body?.contentType === contentType)) { + reportDiagnostic(program, { + code: "duplicate-response", + format: { statusCode: statusCode.toString(), contentType }, + target: responseModel, + }); } } if (bodyModel !== undefined) { for (const contentType of contentTypes) { - response.body ??= {}; - response.body[contentType] = bodyModel; + response.responses.push({ body: { contentType, type: bodyModel }, headers }); } } else if (contentTypes.length > 0) { reportDiagnostic(program, { code: "content-type-ignored", target: responseModel, }); + } else { + response.responses = [{ headers }]; } responses[statusCode] = response; } diff --git a/packages/rest/test/test-responses.ts b/packages/rest/test/test-responses.ts index 918883e8975..a689dfbb36d 100644 --- a/packages/rest/test/test-responses.ts +++ b/packages/rest/test/test-responses.ts @@ -18,7 +18,7 @@ describe("cadl: rest: responses", () => { } ` ); - expectDiagnostics(diagnostics, [{ code: "@cadl-lang/openapi3/duplicate-body" }]); + expectDiagnostics(diagnostics, [{ code: "@cadl-lang/rest/duplicate-body" }]); }); it("issues diagnostics for return type with duplicate status code", async () => { @@ -38,7 +38,7 @@ describe("cadl: rest: responses", () => { ` ); expectDiagnostics(diagnostics, { - code: "@cadl-lang/openapi3/duplicate-response", + code: "@cadl-lang/rest/duplicate-response", message: "Multiple return types for content type application/json and status code 200", }); }); From da58bfe61924d01d42f3dfd80b038ceb0f48a168 Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Tue, 22 Mar 2022 17:22:56 -0700 Subject: [PATCH 6/9] move getContentTypes logic completly out --- packages/openapi3/src/lib.ts | 6 ------ packages/openapi3/src/openapi.ts | 28 ++-------------------------- packages/rest/src/responses.ts | 2 +- 3 files changed, 3 insertions(+), 33 deletions(-) diff --git a/packages/openapi3/src/lib.ts b/packages/openapi3/src/lib.ts index 08c124b4df5..e4007bf88fc 100644 --- a/packages/openapi3/src/lib.ts +++ b/packages/openapi3/src/lib.ts @@ -33,12 +33,6 @@ export const libDef = { default: paramMessage`The header ${"header"} is defined across multiple content types`, }, }, - "content-type-string": { - severity: "error", - messages: { - default: "contentType parameter must be a string literal or union of string literals", - }, - }, "status-code-in-default-response": { severity: "error", messages: { diff --git a/packages/openapi3/src/openapi.ts b/packages/openapi3/src/openapi.ts index 5fb8825a21a..4318e567ee4 100644 --- a/packages/openapi3/src/openapi.ts +++ b/packages/openapi3/src/openapi.ts @@ -45,6 +45,7 @@ import { import { getExtensions, getExternalDocs, getOperationId } from "@cadl-lang/openapi"; import { getAllRoutes, + getContentTypes, getDiscriminator, http, HttpOperationParameter, @@ -554,7 +555,7 @@ function createOAPIEmitter(program: Program, options: OpenAPIEmitterOptions) { (p) => p.type === "header" && p.name === "content-type" ); const contentTypes = contentTypeParam - ? getContentTypes(contentTypeParam.param) + ? getContentTypes(program, contentTypeParam.param) : ["application/json"]; for (const contentType of contentTypes) { const isBinary = isBinaryPayload(bodyType, contentType); @@ -568,31 +569,6 @@ function createOAPIEmitter(program: Program, options: OpenAPIEmitterOptions) { currentEndpoint.requestBody = requestBody; } - function getContentTypes(param: ModelTypeProperty): string[] { - if (param.type.kind === "String") { - return [param.type.value]; - } else if (param.type.kind === "Union") { - const contentTypes = []; - for (const option of param.type.options) { - if (option.kind === "String") { - contentTypes.push(option.value); - } else { - reportDiagnostic(program, { - code: "content-type-string", - target: param, - }); - continue; - } - } - - return contentTypes; - } - - reportDiagnostic(program, { code: "content-type-string", target: param }); - - return []; - } - function emitParameter(parent: ModelType | undefined, param: ModelTypeProperty, kind: string) { const ph = getParamPlaceholder(parent, param); currentEndpoint.parameters.push(ph); diff --git a/packages/rest/src/responses.ts b/packages/rest/src/responses.ts index 1bb74c2d467..113fda214e3 100644 --- a/packages/rest/src/responses.ts +++ b/packages/rest/src/responses.ts @@ -190,7 +190,7 @@ function getResponseContentTypes(program: Program, responseModel: Type): string[ return contentTypes; } -function getContentTypes(program: Program, param: ModelTypeProperty): string[] { +export function getContentTypes(program: Program, param: ModelTypeProperty): string[] { if (param.type.kind === "String") { return [param.type.value]; } else if (param.type.kind === "Union") { From f29114bd14d4cb37b4bb908a7b5769aee694496e Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Tue, 22 Mar 2022 17:42:20 -0700 Subject: [PATCH 7/9] Fix samples --- packages/openapi/src/decorators.ts | 4 ++ packages/openapi3/src/openapi.ts | 2 +- packages/rest/src/responses.ts | 17 ++++++- packages/samples/tags/tagged-operations.cadl | 8 ++-- .../samples/test/output/tags/openapi.json | 48 +++++++++++++++---- .../testserver/body-boolean/openapi.json | 3 -- 6 files changed, 64 insertions(+), 18 deletions(-) diff --git a/packages/openapi/src/decorators.ts b/packages/openapi/src/decorators.ts index 9edd510a3c1..601b08274bc 100644 --- a/packages/openapi/src/decorators.ts +++ b/packages/openapi/src/decorators.ts @@ -69,6 +69,10 @@ export function $defaultResponse({ program }: DecoratorContext, entity: Type) { program.stateSet(defaultResponseKey).add(entity); } +export function isDefaultResponse(program: Program, entity: Type): boolean { + return program.stateSet(defaultResponseKey).has(entity); +} + export interface ExternalDocs { url: string; description?: string; diff --git a/packages/openapi3/src/openapi.ts b/packages/openapi3/src/openapi.ts index 4318e567ee4..87d48ec7a1b 100644 --- a/packages/openapi3/src/openapi.ts +++ b/packages/openapi3/src/openapi.ts @@ -372,7 +372,7 @@ function createOAPIEmitter(program: Program, options: OpenAPIEmitterOptions) { }; for (const data of response.responses) { - if (data.headers) { + if (data.headers && Object.keys(data.headers).length > 0) { openapiResponse.headers ??= {}; // OpenAPI can't represent different headers per content type. // So we merge headers here, and report any duplicates. diff --git a/packages/rest/src/responses.ts b/packages/rest/src/responses.ts index 113fda214e3..5595c187f81 100644 --- a/packages/rest/src/responses.ts +++ b/packages/rest/src/responses.ts @@ -1,5 +1,6 @@ import { getDoc, + getIntrinsicModelName, isErrorModel, isIntrinsic, isVoidType, @@ -48,6 +49,10 @@ export function getResponsesForOperation( const responses: Record = {}; if (responseType.kind === "Union") { for (const option of responseType.options) { + if (isNullType(program, option)) { + // TODO how should we treat this? + continue; + } processResponseType(program, responses, option); } } else { @@ -57,6 +62,10 @@ export function getResponsesForOperation( return Object.values(responses); } +function isNullType(program: Program, type: Type): boolean { + return isIntrinsic(program, type) && getIntrinsicModelName(program, type) === "null"; +} + function processResponseType( program: Program, responses: Record, @@ -258,11 +267,15 @@ function getResponseBody(program: Program, responseModel: Type): Type | undefine return undefined; } -function getResponseDescription(program: Program, responseModel: Type, statusCode: string) { +function getResponseDescription( + program: Program, + responseModel: Type, + statusCode: string +): string | undefined { const desc = getDoc(program, responseModel); if (desc) { return desc; } - return getStatusCodeDescription(statusCode) ?? "unknown"; + return getStatusCodeDescription(statusCode); } diff --git a/packages/samples/tags/tagged-operations.cadl b/packages/samples/tags/tagged-operations.cadl index e8b22b41827..1ea4af3764d 100644 --- a/packages/samples/tags/tagged-operations.cadl +++ b/packages/samples/tags/tagged-operations.cadl @@ -8,13 +8,13 @@ namespace Foo { @tag("tag1") @doc("includes namespace tag") @get - op read(@path id: int32): null; + op read(@path id: int32): void; @tag("tag1") @tag("tag2") @doc("includes namespace tag and two operations tags") @post - op create(@path id: int32): null; + op create(@path id: int32): void; } @route("/bar") @@ -26,7 +26,7 @@ namespace Bar { @doc("one operation tag") @tag("tag3") @post - op create(@path id: int32): null; + op create(@path id: int32): void; } @tag("outer") @@ -38,7 +38,7 @@ namespace NestedOuter { namespace NestedMoreInner { @tag("innerOp") @post - op createOther(@path id: int32): null; + op createOther(@path id: int32): void; } } } diff --git a/packages/samples/test/output/tags/openapi.json b/packages/samples/test/output/tags/openapi.json index 16c259c8717..ee225596c82 100644 --- a/packages/samples/test/output/tags/openapi.json +++ b/packages/samples/test/output/tags/openapi.json @@ -47,8 +47,15 @@ } ], "responses": { - "204": { - "description": "No Content" + "200": { + "description": "Ok", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Cadl.null" + } + } + } } }, "tags": [ @@ -71,8 +78,15 @@ } ], "responses": { - "204": { - "description": "No Content" + "200": { + "description": "Ok", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Cadl.null" + } + } + } } }, "tags": [ @@ -121,8 +135,15 @@ } ], "responses": { - "204": { - "description": "No Content" + "200": { + "description": "Ok", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Cadl.null" + } + } + } } }, "tags": [ @@ -145,8 +166,15 @@ } ], "responses": { - "204": { - "description": "No Content" + "200": { + "description": "Ok", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Cadl.null" + } + } + } } }, "tags": [ @@ -160,6 +188,10 @@ }, "components": { "schemas": { + "Cadl.null": { + "type": "object", + "properties": {} + }, "Resp": { "type": "object", "properties": { diff --git a/packages/samples/test/output/testserver/body-boolean/openapi.json b/packages/samples/test/output/testserver/body-boolean/openapi.json index 4ffd36236bd..5b2249d8365 100644 --- a/packages/samples/test/output/testserver/body-boolean/openapi.json +++ b/packages/samples/test/output/testserver/body-boolean/openapi.json @@ -157,9 +157,6 @@ } } }, - "204": { - "description": "No Content" - }, "default": { "description": "Error", "content": { From a9b3a58062ab724263092b69b757a013898fd5ed Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Tue, 22 Mar 2022 17:52:24 -0700 Subject: [PATCH 8/9] fix --- packages/rest/src/responses.ts | 2 +- .../samples/test/output/tags/openapi.json | 48 ++++--------------- 2 files changed, 9 insertions(+), 41 deletions(-) diff --git a/packages/rest/src/responses.ts b/packages/rest/src/responses.ts index 5595c187f81..517a9788448 100644 --- a/packages/rest/src/responses.ts +++ b/packages/rest/src/responses.ts @@ -50,7 +50,7 @@ export function getResponsesForOperation( if (responseType.kind === "Union") { for (const option of responseType.options) { if (isNullType(program, option)) { - // TODO how should we treat this? + // TODO how should we treat this? https://github.com/microsoft/cadl/issues/356 continue; } processResponseType(program, responses, option); diff --git a/packages/samples/test/output/tags/openapi.json b/packages/samples/test/output/tags/openapi.json index ee225596c82..16c259c8717 100644 --- a/packages/samples/test/output/tags/openapi.json +++ b/packages/samples/test/output/tags/openapi.json @@ -47,15 +47,8 @@ } ], "responses": { - "200": { - "description": "Ok", - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/Cadl.null" - } - } - } + "204": { + "description": "No Content" } }, "tags": [ @@ -78,15 +71,8 @@ } ], "responses": { - "200": { - "description": "Ok", - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/Cadl.null" - } - } - } + "204": { + "description": "No Content" } }, "tags": [ @@ -135,15 +121,8 @@ } ], "responses": { - "200": { - "description": "Ok", - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/Cadl.null" - } - } - } + "204": { + "description": "No Content" } }, "tags": [ @@ -166,15 +145,8 @@ } ], "responses": { - "200": { - "description": "Ok", - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/Cadl.null" - } - } - } + "204": { + "description": "No Content" } }, "tags": [ @@ -188,10 +160,6 @@ }, "components": { "schemas": { - "Cadl.null": { - "type": "object", - "properties": {} - }, "Resp": { "type": "object", "properties": { From e93350bbf456e4a4375c8a4940f08d8d8908d06b Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Tue, 22 Mar 2022 18:35:43 -0700 Subject: [PATCH 9/9] Changelog --- .../http-common-responses_2022-03-23-01-35.json | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 common/changes/@cadl-lang/openapi/http-common-responses_2022-03-23-01-35.json diff --git a/common/changes/@cadl-lang/openapi/http-common-responses_2022-03-23-01-35.json b/common/changes/@cadl-lang/openapi/http-common-responses_2022-03-23-01-35.json new file mode 100644 index 00000000000..adee6879d9d --- /dev/null +++ b/common/changes/@cadl-lang/openapi/http-common-responses_2022-03-23-01-35.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@cadl-lang/openapi", + "comment": "`@defaultResponse` set status code for model", + "type": "minor" + } + ], + "packageName": "@cadl-lang/openapi" +} \ No newline at end of file