From af02e673037ddaf3b9da326be4a79440410b3654 Mon Sep 17 00:00:00 2001 From: Anna Bocharova Date: Thu, 16 May 2024 00:19:46 +0200 Subject: [PATCH] Header params are missing or duplicated (#1769) Fixes #1768 1. Body depiction does not exclude header params (only path params); 2. Params depiction only depicts header params when `query` is enabled as input source. --- CHANGELOG.md | 10 + src/documentation-helpers.ts | 101 +++++----- src/documentation.ts | 10 +- .../documentation-helpers.spec.ts.snap | 8 +- .../__snapshots__/documentation.spec.ts.snap | 175 +++++++++++++++++- tests/unit/documentation-helpers.spec.ts | 2 +- tests/unit/documentation.spec.ts | 49 ++--- 7 files changed, 281 insertions(+), 74 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ad39157d..f84680429 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,16 @@ ## Version 19 +### v19.1.1 + +- Fixed a bug on duplicated or missing request header parameters in the generated Documentation: + - The issue corresponds to the "Headers as input source" opt-in feature; + - When `query` was not listed in the input sources: + - Headers used to be missing in the documented request parameters. + - When `body` was listed along with `query` in the input sources: + - Headers used to be duplicated into the documented request body. + - The issue was found and reported by [@boarush](https://github.com/boarush). + ### v19.1.0 - Feature: customizable handling rules for your branded schemas in Documentation and Integration: diff --git a/src/documentation-helpers.ts b/src/documentation-helpers.ts index cd90b450c..c20b7c457 100644 --- a/src/documentation-helpers.ts +++ b/src/documentation-helpers.ts @@ -3,6 +3,7 @@ import { ExamplesObject, MediaTypeObject, OAuthFlowObject, + ParameterLocation, ParameterObject, ReferenceObject, RequestBodyObject, @@ -686,7 +687,7 @@ export const depictRequestParams = ({ description = `${method.toUpperCase()} ${path} Parameter`, }: Omit & { inputSources: InputSource[]; -}): ParameterObject[] => { +}) => { const { shape } = extractObjectSchema( schema, new DocumentationError({ @@ -704,39 +705,50 @@ export const depictRequestParams = ({ areParamsEnabled && pathParams.includes(name); const isHeaderParam = (name: string) => areHeadersEnabled && isCustomHeader(name); - return Object.keys(shape) - .filter((name) => isQueryEnabled || isPathParam(name)) - .map((name) => { - const depicted = walkSchema(shape[name], { - rules: { ...brandHandling, ...depicters }, - onEach, - onMissing, - ctx: { - isResponse: false, - serializer, - getRef, - makeRef, - path, - method, - }, - }); - const result = - composition === "components" - ? makeRef(makeCleanId(description, name), depicted) - : depicted; - return { - name, - in: isPathParam(name) - ? "path" - : isHeaderParam(name) - ? "header" - : "query", - required: !shape[name].isOptional(), - description: depicted.description || description, - schema: result, - examples: depictParamExamples(schema, name), - }; + + const parameters = Object.keys(shape) + .map<{ name: string; location?: ParameterLocation }>((name) => ({ + name, + location: isPathParam(name) + ? "path" + : isHeaderParam(name) + ? "header" + : isQueryEnabled + ? "query" + : undefined, + })) + .filter( + (parameter): parameter is Required => + parameter.location !== undefined, + ); + + return parameters.map(({ name, location }) => { + const depicted = walkSchema(shape[name], { + rules: { ...brandHandling, ...depicters }, + onEach, + onMissing, + ctx: { + isResponse: false, + serializer, + getRef, + makeRef, + path, + method, + }, }); + const result = + composition === "components" + ? makeRef(makeCleanId(description, name), depicted) + : depicted; + return { + name, + in: location, + required: !shape[name].isOptional(), + description: depicted.description || description, + schema: result, + examples: depictParamExamples(schema, name), + }; + }); }; export const depicters: HandlingRules< @@ -828,29 +840,29 @@ export const onMissing: SchemaHandler< export const excludeParamsFromDepiction = ( depicted: SchemaObject | ReferenceObject, - pathParams: string[], + names: string[], ): SchemaObject | ReferenceObject => { if (isReferenceObject(depicted)) { return depicted; } const copy = { ...depicted }; if (copy.properties) { - copy.properties = omit(pathParams, copy.properties); + copy.properties = omit(names, copy.properties); } if (copy.examples) { - copy.examples = copy.examples.map((entry) => omit(pathParams, entry)); + copy.examples = copy.examples.map((entry) => omit(names, entry)); } if (copy.required) { - copy.required = copy.required.filter((name) => !pathParams.includes(name)); + copy.required = copy.required.filter((name) => !names.includes(name)); } if (copy.allOf) { copy.allOf = copy.allOf.map((entry) => - excludeParamsFromDepiction(entry, pathParams), + excludeParamsFromDepiction(entry, names), ); } if (copy.oneOf) { copy.oneOf = copy.oneOf.map((entry) => - excludeParamsFromDepiction(entry, pathParams), + excludeParamsFromDepiction(entry, names), ); } return copy; @@ -1010,6 +1022,7 @@ export const depictSecurityRefs = ( return depictSecurityRefs({ or: [container] }); }; +// @todo depictBody export const depictRequest = ({ method, path, @@ -1020,9 +1033,11 @@ export const depictRequest = ({ makeRef, composition, brandHandling, + paramNames, description = `${method.toUpperCase()} ${path} Request body`, -}: ReqResDepictHelperCommonProps): RequestBodyObject => { - const pathParams = getRoutePathParams(path); +}: ReqResDepictHelperCommonProps & { + paramNames: string[]; +}): RequestBodyObject => { const bodyDepiction = excludeExamplesFromDepiction( excludeParamsFromDepiction( walkSchema(schema, { @@ -1038,7 +1053,7 @@ export const depictRequest = ({ method, }, }), - pathParams, + paramNames, ), ); const media: MediaTypeObject = { @@ -1046,7 +1061,7 @@ export const depictRequest = ({ composition === "components" ? makeRef(makeCleanId(description), bodyDepiction) : bodyDepiction, - examples: depictExamples(schema, false, pathParams), + examples: depictExamples(schema, false, paramNames), }; return { description, content: fromPairs(xprod(mimeTypes, [media])) }; }; diff --git a/src/documentation.ts b/src/documentation.ts index 8423e48f7..9559b90da 100644 --- a/src/documentation.ts +++ b/src/documentation.ts @@ -7,6 +7,7 @@ import { SecuritySchemeObject, SecuritySchemeType, } from "openapi3-ts/oas31"; +import { pluck } from "ramda"; import { z } from "zod"; import { DocumentationError } from "./errors"; import { @@ -157,7 +158,7 @@ export class Documentation extends OpenApiBuilder { _method, ) => { const method = _method as Method; - const commonParams = { + const commons = { path, method, endpoint, @@ -185,7 +186,7 @@ export class Documentation extends OpenApiBuilder { ); const depictedParams = depictRequestParams({ - ...commonParams, + ...commons, inputSources, schema: endpoint.getSchema("input"), description: descriptions?.requestParameter?.call(null, { @@ -201,7 +202,7 @@ export class Documentation extends OpenApiBuilder { for (const { mimeTypes, schema, statusCodes } of apiResponses) { for (const statusCode of statusCodes) { responses[statusCode] = depictResponse({ - ...commonParams, + ...commons, variant, schema, mimeTypes, @@ -221,7 +222,8 @@ export class Documentation extends OpenApiBuilder { const requestBody = inputSources.includes("body") ? depictRequest({ - ...commonParams, + ...commons, + paramNames: pluck("name", depictedParams), schema: endpoint.getSchema("input"), mimeTypes: endpoint.getMimeTypes("input"), description: descriptions?.requestBody?.call(null, { diff --git a/tests/unit/__snapshots__/documentation-helpers.spec.ts.snap b/tests/unit/__snapshots__/documentation-helpers.spec.ts.snap index 1531efb9c..fef4c2f7d 100644 --- a/tests/unit/__snapshots__/documentation-helpers.spec.ts.snap +++ b/tests/unit/__snapshots__/documentation-helpers.spec.ts.snap @@ -1317,7 +1317,7 @@ exports[`Documentation helpers > excludeParamsFromDepiction() > should handle th } `; -exports[`Documentation helpers > excludeParamsFromDepiction() > should omit specified path params 0 1`] = ` +exports[`Documentation helpers > excludeParamsFromDepiction() > should omit specified params 0 1`] = ` { "properties": { "b": { @@ -1331,7 +1331,7 @@ exports[`Documentation helpers > excludeParamsFromDepiction() > should omit spec } `; -exports[`Documentation helpers > excludeParamsFromDepiction() > should omit specified path params 1 1`] = ` +exports[`Documentation helpers > excludeParamsFromDepiction() > should omit specified params 1 1`] = ` { "oneOf": [ { @@ -1354,7 +1354,7 @@ exports[`Documentation helpers > excludeParamsFromDepiction() > should omit spec } `; -exports[`Documentation helpers > excludeParamsFromDepiction() > should omit specified path params 2 1`] = ` +exports[`Documentation helpers > excludeParamsFromDepiction() > should omit specified params 2 1`] = ` { "properties": { "b": { @@ -1368,7 +1368,7 @@ exports[`Documentation helpers > excludeParamsFromDepiction() > should omit spec } `; -exports[`Documentation helpers > excludeParamsFromDepiction() > should omit specified path params 3 1`] = ` +exports[`Documentation helpers > excludeParamsFromDepiction() > should omit specified params 3 1`] = ` { "allOf": [ { diff --git a/tests/unit/__snapshots__/documentation.spec.ts.snap b/tests/unit/__snapshots__/documentation.spec.ts.snap index 0a5a0e419..7c6b5eff5 100644 --- a/tests/unit/__snapshots__/documentation.spec.ts.snap +++ b/tests/unit/__snapshots__/documentation.spec.ts.snap @@ -3683,7 +3683,7 @@ servers: " `; -exports[`Documentation > Feature 1180: Headers opt-in params > should describe x- inputs as header params 1`] = ` +exports[`Documentation > Feature 1180: Headers opt-in params > should describe x- inputs as header params in get request 1`] = ` "openapi: 3.1.0 info: title: Testing headers params @@ -3763,6 +3763,179 @@ servers: " `; +exports[`Documentation > Feature 1180: Headers opt-in params > should describe x- inputs as header params in post request 1`] = ` +"openapi: 3.1.0 +info: + title: Testing headers params + version: 3.4.5 +paths: + /v1/test: + post: + operationId: PostV1Test + parameters: + - name: id + in: query + required: true + description: POST /v1/test Parameter + schema: + type: string + - name: x-request-id + in: header + required: true + description: POST /v1/test Parameter + schema: + type: string + requestBody: + description: POST /v1/test Request body + content: + application/json: + schema: + type: object + properties: {} + required: [] + responses: + "200": + description: POST /v1/test Positive response + content: + application/json: + schema: + type: object + properties: + status: + type: string + const: success + data: + type: object + required: + - status + - data + "400": + description: POST /v1/test Negative response + content: + application/json: + schema: + type: object + properties: + status: + type: string + const: error + error: + type: object + properties: + message: + type: string + required: + - message + required: + - status + - error + examples: + example1: + value: + status: error + error: + message: Sample error message +components: + schemas: {} + responses: {} + parameters: {} + examples: {} + requestBodies: {} + headers: {} + securitySchemes: {} + links: {} + callbacks: {} +tags: [] +servers: + - url: https://example.com +" +`; + +exports[`Documentation > Feature 1180: Headers opt-in params > should describe x- inputs as header params in put request 1`] = ` +"openapi: 3.1.0 +info: + title: Testing headers params + version: 3.4.5 +paths: + /v1/test: + put: + operationId: PutV1Test + parameters: + - name: x-request-id + in: header + required: true + description: PUT /v1/test Parameter + schema: + type: string + requestBody: + description: PUT /v1/test Request body + content: + application/json: + schema: + type: object + properties: + id: + type: string + required: + - id + responses: + "200": + description: PUT /v1/test Positive response + content: + application/json: + schema: + type: object + properties: + status: + type: string + const: success + data: + type: object + required: + - status + - data + "400": + description: PUT /v1/test Negative response + content: + application/json: + schema: + type: object + properties: + status: + type: string + const: error + error: + type: object + properties: + message: + type: string + required: + - message + required: + - status + - error + examples: + example1: + value: + status: error + error: + message: Sample error message +components: + schemas: {} + responses: {} + parameters: {} + examples: {} + requestBodies: {} + headers: {} + securitySchemes: {} + links: {} + callbacks: {} +tags: [] +servers: + - url: https://example.com +" +`; + exports[`Documentation > Issue #98 > Should describe non-empty array 1`] = ` "openapi: 3.1.0 info: diff --git a/tests/unit/documentation-helpers.spec.ts b/tests/unit/documentation-helpers.spec.ts index 0c85f9acc..592a5017b 100644 --- a/tests/unit/documentation-helpers.spec.ts +++ b/tests/unit/documentation-helpers.spec.ts @@ -193,7 +193,7 @@ describe("Documentation helpers", () => { z .record(z.literal("a"), z.string()) .and(z.record(z.string(), z.string())), - ])("should omit specified path params %#", (schema) => { + ])("should omit specified params %#", (schema) => { const depicted = walkSchema(schema, { ctx: requestCtx, rules: depicters, diff --git a/tests/unit/documentation.spec.ts b/tests/unit/documentation.spec.ts index c5153ed6e..86370a432 100644 --- a/tests/unit/documentation.spec.ts +++ b/tests/unit/documentation.spec.ts @@ -981,31 +981,38 @@ describe("Documentation", () => { describe("Feature 1180: Headers opt-in params", () => { const specificConfig = createConfig({ ...sampleConfig, - inputSources: { get: ["query", "params", "headers"] }, + inputSources: { + get: ["query", "params", "headers"], + post: ["body", "query", "params", "headers"], + put: ["body", "headers"], // query is not enabled + }, }); - test("should describe x- inputs as header params", () => { - const spec = new Documentation({ - config: specificConfig, - routing: { - v1: { - test: defaultEndpointsFactory.build({ - method: "get", - input: z.object({ - id: z.string(), - "x-request-id": z.string(), + test.each(["get", "post", "put"] as const)( + "should describe x- inputs as header params in %s request", + (method) => { + const spec = new Documentation({ + config: specificConfig, + routing: { + v1: { + test: defaultEndpointsFactory.build({ + method, + input: z.object({ + id: z.string(), + "x-request-id": z.string(), + }), + output: z.object({}), + handler: vi.fn(), }), - output: z.object({}), - handler: vi.fn(), - }), + }, }, - }, - version: "3.4.5", - title: "Testing headers params", - serverUrl: "https://example.com", - }).getSpecAsYaml(); - expect(spec).toMatchSnapshot(); - }); + version: "3.4.5", + title: "Testing headers params", + serverUrl: "https://example.com", + }).getSpecAsYaml(); + expect(spec).toMatchSnapshot(); + }, + ); }); describe("Feature #1431: Multiple schemas for different status codes", () => {