diff --git a/.chronus/changes/tsp-openapi3-fix-query-params-2024-9-25-14-20-22.md b/.chronus/changes/tsp-openapi3-fix-query-params-2024-9-25-14-20-22.md new file mode 100644 index 0000000000..790cc701e0 --- /dev/null +++ b/.chronus/changes/tsp-openapi3-fix-query-params-2024-9-25-14-20-22.md @@ -0,0 +1,7 @@ +--- +changeKind: fix +packages: + - "@typespec/openapi3" +--- + +Updates tsp-openapi3 query decorator generation to use the value arguments. \ No newline at end of file diff --git a/packages/openapi3/src/cli/actions/convert/generators/generate-decorators.ts b/packages/openapi3/src/cli/actions/convert/generators/generate-decorators.ts index 08407cb28b..33c898ed8d 100644 --- a/packages/openapi3/src/cli/actions/convert/generators/generate-decorators.ts +++ b/packages/openapi3/src/cli/actions/convert/generators/generate-decorators.ts @@ -1,8 +1,10 @@ -import { TypeSpecDecorator } from "../interfaces.js"; +import { TSValue, TypeSpecDecorator } from "../interfaces.js"; function generateDecorator({ name, args }: TypeSpecDecorator): string { const hasArgs = args.length; - const stringifiedArguments = hasArgs ? `(${args.map((a) => JSON.stringify(a)).join(", ")})` : ""; + const stringifiedArguments = hasArgs + ? `(${args.map((a) => (isTSValue(a) ? a.value : JSON.stringify(a))).join(", ")})` + : ""; return `@${name}${stringifiedArguments}`; } @@ -11,3 +13,9 @@ export function generateDecorators(decorators: TypeSpecDecorator[]): string[] { const uniqueDecorators = new Set(decorators.map(generateDecorator)); return Array.from(uniqueDecorators); } + +function isTSValue(value: unknown): value is TSValue { + return Boolean( + value && typeof value === "object" && "__kind" in value && value["__kind"] === "value", + ); +} diff --git a/packages/openapi3/src/cli/actions/convert/interfaces.ts b/packages/openapi3/src/cli/actions/convert/interfaces.ts index c4e6897cea..9c67de0f8e 100644 --- a/packages/openapi3/src/cli/actions/convert/interfaces.ts +++ b/packages/openapi3/src/cli/actions/convert/interfaces.ts @@ -34,7 +34,16 @@ export interface TypeSpecServiceInfo { export interface TypeSpecDecorator { name: string; - args: (object | number | string)[]; + args: (object | number | string | TSValue)[]; +} + +export interface TSValue { + __kind: "value"; + /** + * String representation of the value. + * This will be directly substituted into the generated code. + */ + value: string; } export interface TypeSpecAugmentation extends TypeSpecDecorator { diff --git a/packages/openapi3/src/cli/actions/convert/utils/decorators.ts b/packages/openapi3/src/cli/actions/convert/utils/decorators.ts index 0d7c9654e2..c9d3a4ae15 100644 --- a/packages/openapi3/src/cli/actions/convert/utils/decorators.ts +++ b/packages/openapi3/src/cli/actions/convert/utils/decorators.ts @@ -1,6 +1,6 @@ import { ExtensionKey } from "@typespec/openapi"; import { Extensions, OpenAPI3Parameter, OpenAPI3Schema, Refable } from "../../../../types.js"; -import { TypeSpecDecorator } from "../interfaces.js"; +import { TSValue, TypeSpecDecorator } from "../interfaces.js"; const validLocations = ["header", "query", "path"]; const extensionDecoratorName = "extension"; @@ -45,35 +45,79 @@ function getLocationDecorator(parameter: OpenAPI3Parameter): TypeSpecDecorator | }; let format: string | undefined; + let decoratorArgs: TypeSpecDecorator["args"][0] | undefined; switch (parameter.in) { case "header": format = getHeaderFormat(parameter.style); break; case "query": - format = getQueryFormat(parameter.explode, parameter.style); + decoratorArgs = getQueryArgs(parameter); break; } if (format) { decorator.args.push({ format }); } + if (decoratorArgs) { + decorator.args.push(decoratorArgs); + } return decorator; } -function getQueryFormat(explode?: boolean, style?: string): string | undefined { - if (explode) { - return "form"; - } else if (style === "form") { - return "simple"; - } else if (style === "spaceDelimited") { - return "ssv"; - } else if (style === "pipeDelimited") { - return "pipes"; +function getQueryArgs(parameter: OpenAPI3Parameter): TSValue | undefined { + const queryOptions = getNormalizedQueryOptions(parameter); + if (Object.keys(queryOptions).length) { + return { + __kind: "value", + value: `#{${Object.entries(queryOptions) + .map(([key, value]) => { + return `${key}: ${JSON.stringify(value)}`; + }) + .join(", ")}}`, + }; } + return; } +type QueryOptions = { explode?: boolean; format?: string }; + +function getNormalizedQueryOptions({ + explode, + style = "", +}: { + explode?: boolean; + style?: string; +}): QueryOptions { + const queryOptions: QueryOptions = {}; + // In OpenAPI 3, default style is 'form', and explode is true when 'form' is the style + if (typeof explode !== "boolean") { + if (style === "form" || !style) { + explode = true; + } else { + explode = false; + } + } + + // Format only emits additional data if set to one of the following: + // ssv (spaceDelimited), pipes (pipeDelimited) + if (style === "spaceDelimited") { + queryOptions.format = "ssv"; + } else if (style === "pipeDelimited") { + queryOptions.format = "pipes"; + } + + // In TypeSpec, default explode is "false" + if (!explode && queryOptions.format) { + queryOptions.explode = false; + } else if (explode) { + queryOptions.explode = true; + } + + return queryOptions; +} + function getHeaderFormat(style?: string): string | undefined { return style === "simple" ? "simple" : undefined; } diff --git a/packages/openapi3/test/tsp-openapi3/output/escaped-identifiers/main.tsp b/packages/openapi3/test/tsp-openapi3/output/escaped-identifiers/main.tsp index 2189df76ec..1230f8cc0c 100644 --- a/packages/openapi3/test/tsp-openapi3/output/escaped-identifiers/main.tsp +++ b/packages/openapi3/test/tsp-openapi3/output/escaped-identifiers/main.tsp @@ -27,7 +27,7 @@ model `Escaped-Model` { model `get-thingDefaultResponse` {} @route("/{escaped-property}") @get op `get-thing`( - @query `weird@param`?: `Foo-Bar`, + @query(#{ explode: true }) `weird@param`?: `Foo-Bar`, ...Parameters.`Escaped-Model`.`escaped-property`, @bodyRoot body: `Escaped-Model`, ): `get-thingDefaultResponse`; diff --git a/packages/openapi3/test/tsp-openapi3/output/param-decorators/main.tsp b/packages/openapi3/test/tsp-openapi3/output/param-decorators/main.tsp index 8c328eaf3a..f5f0e29d20 100644 --- a/packages/openapi3/test/tsp-openapi3/output/param-decorators/main.tsp +++ b/packages/openapi3/test/tsp-openapi3/output/param-decorators/main.tsp @@ -42,7 +42,7 @@ model Operations_putThing200ApplicationJsonResponse { @minValue(0) @maxValue(10) - @query + @query(#{ explode: true }) count: int32, ): Operations_getThing200ApplicationJsonResponse; diff --git a/packages/openapi3/test/tsp-openapi3/output/petstore-sample/main.tsp b/packages/openapi3/test/tsp-openapi3/output/petstore-sample/main.tsp index 89796f955c..37239cf5da 100644 --- a/packages/openapi3/test/tsp-openapi3/output/petstore-sample/main.tsp +++ b/packages/openapi3/test/tsp-openapi3/output/petstore-sample/main.tsp @@ -89,7 +89,7 @@ op listPets( /** * How many items to return at one time (max 100) */ - @query limit?: int32, + @query(#{ explode: true }) limit?: int32, ): listPets200ApplicationJsonResponse | listPetsDefaultApplicationJsonResponse; @tag("pets") diff --git a/packages/openapi3/test/tsp-openapi3/output/petstore-swagger/main.tsp b/packages/openapi3/test/tsp-openapi3/output/petstore-swagger/main.tsp index 52be62b9a4..0458c3c151 100644 --- a/packages/openapi3/test/tsp-openapi3/output/petstore-swagger/main.tsp +++ b/packages/openapi3/test/tsp-openapi3/output/petstore-swagger/main.tsp @@ -539,10 +539,7 @@ op findPetsByStatus( /** * Status values that need to be considered for filter */ - @query({ - format: "form", - }) - status?: "available" | "pending" | "sold" = "available", + @query(#{ explode: true }) status?: "available" | "pending" | "sold" = "available", ): findPetsByStatus200ApplicationXmlResponse | findPetsByStatus200ApplicationJsonResponse | findPetsByStatus400Response; /** @@ -556,10 +553,7 @@ op findPetsByTags( /** * Tags to filter by */ - @query({ - format: "form", - }) - tags?: string[], + @query(#{ explode: true }) tags?: string[], ): findPetsByTags200ApplicationXmlResponse | findPetsByTags200ApplicationJsonResponse | findPetsByTags400Response; @tag("pet") @@ -606,12 +600,12 @@ op updatePetWithForm( /** * Name of pet that needs to be updated */ - @query name?: string, + @query(#{ explode: true }) name?: string, /** * Status of pet that needs to be updated */ - @query status?: string, + @query(#{ explode: true }) status?: string, ): updatePetWithForm405Response; @tag("pet") @@ -627,7 +621,7 @@ op uploadFile( /** * Additional Metadata */ - @query additionalMetadata?: string, + @query(#{ explode: true }) additionalMetadata?: string, @header contentType: "application/octet-stream", @bodyRoot body: bytes, @@ -722,12 +716,12 @@ op loginUser( /** * The user name for login */ - @query username?: string, + @query(#{ explode: true }) username?: string, /** * The password for login in clear text */ - @query password?: string, + @query(#{ explode: true }) password?: string, ): loginUser200ApplicationXmlResponse | loginUser200ApplicationJsonResponse | loginUser400Response; @tag("user") diff --git a/packages/openapi3/test/tsp-openapi3/parameters.test.ts b/packages/openapi3/test/tsp-openapi3/parameters.test.ts index 53656e0006..a1a6d38d36 100644 --- a/packages/openapi3/test/tsp-openapi3/parameters.test.ts +++ b/packages/openapi3/test/tsp-openapi3/parameters.test.ts @@ -1,7 +1,9 @@ -import { Numeric } from "@typespec/compiler"; +import { getDocData, Numeric } from "@typespec/compiler"; +import { expectDiagnosticEmpty, expectDiagnostics } from "@typespec/compiler/testing"; +import { ok } from "assert"; import { assert, describe, expect, it } from "vitest"; import { expectDecorators } from "./utils/expect.js"; -import { tspForOpenAPI3 } from "./utils/tsp-for-openapi3.js"; +import { compileForOpenAPI3, tspForOpenAPI3 } from "./utils/tsp-for-openapi3.js"; describe("converts top-level parameters", () => { it.each(["query", "header", "path"] as const)(`Supports location: %s`, async (location) => { @@ -159,7 +161,11 @@ describe("converts top-level parameters", () => { }); it("supports doc generation", async () => { - const serviceNamespace = await tspForOpenAPI3({ + const { + namespace: serviceNamespace, + diagnostics, + program, + } = await compileForOpenAPI3({ parameters: { Foo: { name: "foo", @@ -172,6 +178,8 @@ describe("converts top-level parameters", () => { }, }); + expectDiagnosticEmpty(diagnostics); + const parametersNamespace = serviceNamespace.namespaces.get("Parameters"); assert(parametersNamespace, "Parameters namespace not found"); @@ -192,9 +200,10 @@ describe("converts top-level parameters", () => { optional: true, type: { kind: "Scalar", name: "string" }, }); - expect(foo?.decorators.find((d) => d.definition?.name === "@query")).toBeTruthy(); - const docDecorator = foo?.decorators.find((d) => d.decorator?.name === "$docFromComment"); - expect(docDecorator?.args[1]).toMatchObject({ jsValue: "Docs for foo" }); + ok(foo); + expectDecorators(foo.decorators, [{ name: "query" }], { strict: false }); + const docData = getDocData(program, foo); + expect(docData).toMatchObject({ source: "comment", value: "Docs for foo" }); }); it("supports referenced schemas", async () => { @@ -259,7 +268,7 @@ describe("converts top-level parameters", () => { }); expectDecorators(Foo.properties.get("foo")!.decorators, [ { name: "query" }, - { name: "summary", args: [{ jsValue: "Foo Title" }] }, + { name: "summary", args: ["Foo Title"] }, ]); }); @@ -296,3 +305,244 @@ describe("converts top-level parameters", () => { }, ); }); + +describe("header", () => { + it(`sets no args if style is not set`, async () => { + const serviceNamespace = await tspForOpenAPI3({ + parameters: { + Foo: { + name: "foo", + in: "header", + schema: { + type: "string", + }, + }, + }, + }); + + const parametersNamespace = serviceNamespace.namespaces.get("Parameters"); + assert(parametersNamespace, "Parameters namespace not found"); + + const models = parametersNamespace.models; + + /* model Foo { @header foo: string, } */ + const Foo = models.get("Foo"); + assert(Foo, "Foo model not found"); + expect(Foo.properties.size).toBe(1); + const fooProperty = Foo.properties.get("foo"); + assert(fooProperty, "foo property not found"); + expectDecorators(fooProperty.decorators, [{ name: "header" }]); + }); + + it(`sets format to 'simple' when style is set to 'simple'`, async () => { + const serviceNamespace = await tspForOpenAPI3({ + parameters: { + Foo: { + name: "foo", + in: "header", + schema: { + type: "array", + items: { + type: "string", + }, + }, + style: "simple", + }, + }, + }); + + const parametersNamespace = serviceNamespace.namespaces.get("Parameters"); + assert(parametersNamespace, "Parameters namespace not found"); + + const models = parametersNamespace.models; + + /* model Foo { @header({ format: "simple" }) foo: string[], } */ + const Foo = models.get("Foo"); + assert(Foo, "Foo model not found"); + expect(Foo.properties.size).toBe(1); + const fooProperty = Foo.properties.get("foo"); + assert(fooProperty, "foo property not found"); + expectDecorators(fooProperty.decorators, [{ name: "header", args: [{ format: "simple" }] }]); + }); +}); + +describe("query", () => { + it(`sets explode: true for default OpenAPI 3 parameter`, async () => { + const serviceNamespace = await tspForOpenAPI3({ + parameters: { + Foo: { + name: "foo", + in: "query", + required: true, + schema: { + type: "array", + items: { + type: "string", + }, + }, + }, + }, + }); + + const parametersNamespace = serviceNamespace.namespaces.get("Parameters"); + assert(parametersNamespace, "Parameters namespace not found"); + + const models = parametersNamespace.models; + + /* model Foo { @query(#{ explode: true }) foo: string[], } */ + const Foo = models.get("Foo"); + assert(Foo, "Foo model not found"); + expect(Foo.properties.size).toBe(1); + const fooProperty = Foo.properties.get("foo"); + assert(fooProperty, "foo property not found"); + expectDecorators(fooProperty.decorators, [{ name: "query", args: [{ explode: true }] }]); + }); + + describe("explicit explode: false", () => { + const explode = false; + it.each(["", "form"])("sets no args when style: %s", async (style) => { + const serviceNamespace = await tspForOpenAPI3({ + parameters: { + Foo: { + name: "foo", + in: "query", + schema: { + type: "array", + items: { + type: "string", + }, + }, + style: style as any, + explode, + }, + }, + }); + + const parametersNamespace = serviceNamespace.namespaces.get("Parameters"); + assert(parametersNamespace, "Parameters namespace not found"); + + const models = parametersNamespace.models; + + /* model Foo { @query foo: string[], } */ + const Foo = models.get("Foo"); + assert(Foo, "Foo model not found"); + expect(Foo.properties.size).toBe(1); + const fooProperty = Foo.properties.get("foo"); + assert(fooProperty, "foo property not found"); + expectDecorators(fooProperty.decorators, [{ name: "query" }]); + }); + + it.each([ + { style: "spaceDelimited", format: "ssv" }, + { style: "pipeDelimited", format: "pipes" }, + ])("sets explode and format args when style: $style", async ({ style, format }) => { + const { namespace: serviceNamespace, diagnostics } = await compileForOpenAPI3({ + parameters: { + Foo: { + name: "foo", + in: "query", + schema: { + type: "array", + items: { + type: "string", + }, + }, + style: style as any, + explode, + }, + }, + }); + + // Expect a deprecated warning due to the use of format in the query args + expectDiagnostics(diagnostics, [{ code: "deprecated" }]); + + const parametersNamespace = serviceNamespace.namespaces.get("Parameters"); + assert(parametersNamespace, "Parameters namespace not found"); + + const models = parametersNamespace.models; + + /* model Foo { @query(#{explode: false, format: $format}) foo: string[], } */ + const Foo = models.get("Foo"); + assert(Foo, "Foo model not found"); + expect(Foo.properties.size).toBe(1); + const fooProperty = Foo.properties.get("foo"); + assert(fooProperty, "foo property not found"); + expectDecorators(fooProperty.decorators, [{ name: "query", args: [{ explode, format }] }]); + }); + }); + + describe("explicit explode: true", () => { + const explode = true; + it.each(["", "form"])("sets only explode in query args", async (style) => { + const serviceNamespace = await tspForOpenAPI3({ + parameters: { + Foo: { + name: "foo", + in: "query", + schema: { + type: "array", + items: { + type: "string", + }, + }, + style: style as any, + explode, + }, + }, + }); + + const parametersNamespace = serviceNamespace.namespaces.get("Parameters"); + assert(parametersNamespace, "Parameters namespace not found"); + + const models = parametersNamespace.models; + + /* model Foo { @query(#{explode: true}) foo: string[], } */ + const Foo = models.get("Foo"); + assert(Foo, "Foo model not found"); + expect(Foo.properties.size).toBe(1); + const fooProperty = Foo.properties.get("foo"); + assert(fooProperty, "foo property not found"); + expectDecorators(fooProperty.decorators, [{ name: "query", args: [{ explode }] }]); + }); + + it.each([ + { style: "spaceDelimited", format: "ssv" }, + { style: "pipeDelimited", format: "pipes" }, + ])("sets explode and format args when style: $style", async ({ style, format }) => { + const { namespace: serviceNamespace, diagnostics } = await compileForOpenAPI3({ + parameters: { + Foo: { + name: "foo", + in: "query", + schema: { + type: "array", + items: { + type: "string", + }, + }, + style: style as any, + explode, + }, + }, + }); + + // Expect a deprecated warning due to the use of format in the query args + expectDiagnostics(diagnostics, [{ code: "deprecated" }]); + + const parametersNamespace = serviceNamespace.namespaces.get("Parameters"); + assert(parametersNamespace, "Parameters namespace not found"); + + const models = parametersNamespace.models; + + /* model Foo { @query(#{explode: false, format: $format}) foo: string[], } */ + const Foo = models.get("Foo"); + assert(Foo, "Foo model not found"); + expect(Foo.properties.size).toBe(1); + const fooProperty = Foo.properties.get("foo"); + assert(fooProperty, "foo property not found"); + expectDecorators(fooProperty.decorators, [ + { name: "query", args: [{ explode: true, format }] }, + ]); + }); + }); +}); diff --git a/packages/openapi3/test/tsp-openapi3/paths.test.ts b/packages/openapi3/test/tsp-openapi3/paths.test.ts index 3d9550f765..5e0e176ee2 100644 --- a/packages/openapi3/test/tsp-openapi3/paths.test.ts +++ b/packages/openapi3/test/tsp-openapi3/paths.test.ts @@ -294,7 +294,7 @@ it("supports operation summary", async () => { /* @get @route("/") @summary("Root Get Summary") */ expectDecorators(rootGet.decorators, [ - { name: "summary", args: [{ jsValue: "Root Get Summary" }] }, + { name: "summary", args: ["Root Get Summary"] }, { name: "get" }, { name: "route", args: ["/"] }, ]); diff --git a/packages/openapi3/test/tsp-openapi3/utils/expect.ts b/packages/openapi3/test/tsp-openapi3/utils/expect.ts index 57c8dc7721..30995dbac2 100644 --- a/packages/openapi3/test/tsp-openapi3/utils/expect.ts +++ b/packages/openapi3/test/tsp-openapi3/utils/expect.ts @@ -1,4 +1,4 @@ -import { DecoratorApplication, isType, Numeric } from "@typespec/compiler"; +import { DecoratorApplication, isType, Numeric, typespecTypeToJson } from "@typespec/compiler"; import { assert, expect } from "vitest"; export interface DecoratorMatch { @@ -41,8 +41,18 @@ export function expectDecorators( } if (expectation.args) { + const actualArgs = decorator.args.map((arg: any) => { + if (isType(arg)) return arg; + if ("jsValue" in arg) { + if (isType(arg.jsValue) && arg.jsValue.kind === "Model") { + return typespecTypeToJson(arg.jsValue, arg.jsValue)[0]; + } + return arg.jsValue; + } + return arg; + }); const args = expectation.args.map(transformDecoratorArg); - expect(decorator.args).toMatchObject(args); + expect(actualArgs).toMatchObject(args); } } } @@ -50,9 +60,6 @@ export function expectDecorators( function transformDecoratorArg(arg: any) { if (isType(arg)) return arg; - if (typeof arg === "string") { - return { jsValue: arg }; - } if (typeof arg === "number") { return { jsValue: Numeric(`${arg}`) }; } diff --git a/packages/openapi3/test/tsp-openapi3/utils/tsp-for-openapi3.ts b/packages/openapi3/test/tsp-openapi3/utils/tsp-for-openapi3.ts index fe8031062f..f746fe549e 100644 --- a/packages/openapi3/test/tsp-openapi3/utils/tsp-for-openapi3.ts +++ b/packages/openapi3/test/tsp-openapi3/utils/tsp-for-openapi3.ts @@ -1,4 +1,5 @@ -import { createTestHost } from "@typespec/compiler/testing"; +import { Diagnostic, Namespace, Program } from "@typespec/compiler"; +import { createTestHost, expectDiagnosticEmpty } from "@typespec/compiler/testing"; import { HttpTestLibrary } from "@typespec/http/testing"; import { OpenAPITestLibrary } from "@typespec/openapi/testing"; import assert from "node:assert"; @@ -24,7 +25,17 @@ export interface OpenAPI3Options { paths?: Record; } -export async function tspForOpenAPI3({ parameters, paths, schemas }: OpenAPI3Options) { +export async function tspForOpenAPI3(props: OpenAPI3Options) { + const { namespace: TestService, diagnostics } = await compileForOpenAPI3(props); + expectDiagnosticEmpty(diagnostics); + return TestService; +} + +export async function compileForOpenAPI3({ parameters, paths, schemas }: OpenAPI3Options): Promise<{ + namespace: Namespace; + diagnostics: readonly Diagnostic[]; + program: Program; +}> { const openApi3Doc: OpenAPI3Document = { info: { title: "Test Service", @@ -49,11 +60,16 @@ export async function tspForOpenAPI3({ parameters, paths, schemas }: OpenAPI3Opt }); host.addTypeSpecFile("main.tsp", testableCode); - const { TestService } = await host.compile("main.tsp"); + const [types, diagnostics] = await host.compileAndDiagnose("main.tsp"); + const { TestService } = types; assert( TestService?.kind === "Namespace", `Expected TestService to be a namespace, instead got ${TestService?.kind}`, ); - return TestService; + return { + namespace: TestService, + diagnostics, + program: host.program, + }; }