diff --git a/.chronus/changes/wanl-fix-diag-1-2025-1-7-19-3-44.md b/.chronus/changes/wanl-fix-diag-1-2025-1-7-19-3-44.md new file mode 100644 index 0000000000..8231524dc8 --- /dev/null +++ b/.chronus/changes/wanl-fix-diag-1-2025-1-7-19-3-44.md @@ -0,0 +1,7 @@ +--- +changeKind: fix +packages: + - "@typespec/openapi3" +--- + +Fix: `@typespec/openapi3/invalid-component-fixed-field-key` show on incorrect target diff --git a/packages/openapi3/src/lib.ts b/packages/openapi3/src/lib.ts index b80fd5498a..601e4cfd49 100644 --- a/packages/openapi3/src/lib.ts +++ b/packages/openapi3/src/lib.ts @@ -316,7 +316,7 @@ export const libDef = { }, }, "invalid-component-fixed-field-key": { - severity: "error", + severity: "warning", messages: { default: paramMessage`Invalid key '${"value"}' used in a fixed field of the Component object. Only alphanumerics, dot (.), hyphen (-), and underscore (_) characters are allowed in keys.`, }, diff --git a/packages/openapi3/src/openapi.ts b/packages/openapi3/src/openapi.ts index f182fd40c9..1b519d20d3 100644 --- a/packages/openapi3/src/openapi.ts +++ b/packages/openapi3/src/openapi.ts @@ -47,7 +47,6 @@ import { unsafe_mutateSubgraphWithNamespace, unsafe_MutatorWithNamespace, } from "@typespec/compiler/experimental"; -import {} from "@typespec/compiler/utils"; import { AuthenticationOptionReference, AuthenticationReference, @@ -117,6 +116,7 @@ import { } from "./types.js"; import { deepEquals, + ensureValidComponentFixedFieldKey, getDefaultValue, isBytesKeptRaw, isSharedHttpOperation, @@ -681,7 +681,6 @@ function createOAPIEmitter( } } } - return [root, diagnostics.diagnostics]; } catch (err) { if (err instanceof ErrorTypeFoundError) { @@ -1567,21 +1566,6 @@ function createOAPIEmitter( } } - function validateComponentFixedFieldKey(type: Type, name: string) { - const pattern = /^[a-zA-Z0-9.\-_]+$/; - if (!pattern.test(name)) { - program.reportDiagnostic( - createDiagnostic({ - code: "invalid-component-fixed-field-key", - format: { - value: name, - }, - target: type, - }), - ); - } - } - function emitParameters() { for (const [property, param] of params) { const key = getParameterKey( @@ -1591,14 +1575,12 @@ function createOAPIEmitter( root.components!.parameters!, typeNameOptions, ); - validateComponentFixedFieldKey(property, key); - - root.components!.parameters![key] = { ...param }; + const validKey = ensureValidComponentFixedFieldKey(program, property, key); + root.components!.parameters![validKey] = { ...param }; for (const key of Object.keys(param)) { delete param[key]; } - - param.$ref = "#/components/parameters/" + encodeURIComponent(key); + param.$ref = "#/components/parameters/" + encodeURIComponent(validKey); } } @@ -1616,8 +1598,6 @@ function createOAPIEmitter( const schemas = root.components!.schemas!; const declarations = files[0].globalScope.declarations; for (const declaration of declarations) { - validateComponentFixedFieldKey(serviceNamespace, declaration.name); - schemas[declaration.name] = declaration.value as any; } } diff --git a/packages/openapi3/src/schema-emitter.ts b/packages/openapi3/src/schema-emitter.ts index 90583fc356..0f8a6dc25d 100644 --- a/packages/openapi3/src/schema-emitter.ts +++ b/packages/openapi3/src/schema-emitter.ts @@ -76,7 +76,13 @@ import { OpenAPI3SchemaProperty, OpenAPISchema3_1, } from "./types.js"; -import { getDefaultValue, includeDerivedModel, isBytesKeptRaw, isStdType } from "./util.js"; +import { + ensureValidComponentFixedFieldKey, + getDefaultValue, + includeDerivedModel, + isBytesKeptRaw, + isStdType, +} from "./util.js"; import { VisibilityUsageTracker } from "./visibility-usage.js"; import { XmlModule } from "./xml-module.js"; @@ -230,6 +236,7 @@ export class OpenAPI3SchemaEmitterBase< const baseName = getOpenAPITypeName(program, model, this.#typeNameOptions()); const isMultipart = this.getContentType().startsWith("multipart/"); const name = isMultipart ? baseName + "MultiPart" : baseName; + return this.#createDeclaration(model, name, this.applyConstraints(model, schema as any)); } @@ -501,6 +508,7 @@ export class OpenAPI3SchemaEmitterBase< enumDeclaration(en: Enum, name: string): EmitterOutput { const baseName = getOpenAPITypeName(this.emitter.getProgram(), en, this.#typeNameOptions()); + return this.#createDeclaration(en, baseName, new ObjectBuilder(this.enumSchema(en))); } @@ -805,6 +813,11 @@ export class OpenAPI3SchemaEmitterBase< } #createDeclaration(type: Type, name: string, schema: ObjectBuilder) { + const skipNameValidation = type.kind === "Model" && type.templateMapper !== undefined; + if (!skipNameValidation) { + name = ensureValidComponentFixedFieldKey(this.emitter.getProgram(), type, name); + } + const refUrl = getRef(this.emitter.getProgram(), type); if (refUrl) { return { @@ -835,6 +848,7 @@ export class OpenAPI3SchemaEmitterBase< fullName, Object.fromEntries(decl.scope.declarations.map((x) => [x.name, true])), ); + return decl; } diff --git a/packages/openapi3/src/util.ts b/packages/openapi3/src/util.ts index 929be66094..90bb5ddbde 100644 --- a/packages/openapi3/src/util.ts +++ b/packages/openapi3/src/util.ts @@ -14,6 +14,7 @@ import { Value, } from "@typespec/compiler"; import { HttpOperation } from "@typespec/http"; +import { createDiagnostic } from "./lib.js"; /** * Checks if two objects are deeply equal. * @@ -158,3 +159,33 @@ export function getDefaultValue( export function isBytesKeptRaw(program: Program, type: Type) { return type.kind === "Scalar" && type.name === "bytes" && getEncode(program, type) === undefined; } + +export function ensureValidComponentFixedFieldKey( + program: Program, + type: Type, + oldKey: string, +): string { + if (isValidComponentFixedFieldKey(oldKey)) return oldKey; + reportInvalidKey(program, type, oldKey); + return createValidKey(oldKey); +} + +function isValidComponentFixedFieldKey(key: string) { + const validPattern = /^[a-zA-Z0-9.\-_]+$/; + return validPattern.test(key); +} + +function reportInvalidKey(program: Program, type: Type, key: string) { + const diagnostic = createDiagnostic({ + code: "invalid-component-fixed-field-key", + format: { + value: key, + }, + target: type, + }); + return program.reportDiagnostic(diagnostic); +} + +function createValidKey(invalidKey: string): string { + return invalidKey.replace(/[^a-zA-Z0-9.\-_]/g, "_"); +} diff --git a/packages/openapi3/test/component.test.ts b/packages/openapi3/test/component.test.ts new file mode 100644 index 0000000000..c347d5bfd8 --- /dev/null +++ b/packages/openapi3/test/component.test.ts @@ -0,0 +1,284 @@ +import { expectDiagnostics } from "@typespec/compiler/testing"; +import { describe, expect, it } from "vitest"; +import { OpenAPI3Document } from "../src/types.js"; +import { worksFor } from "./works-for.js"; + +interface DiagnosticCheck { + expectedDiagInvalidKey: string; + expectedDeclKey: string; + expectedPrefix: "#/components/schemas/" | "#/components/parameters/"; + kind: "Model" | "ModelProperty" | "Union"; +} + +interface Case { + title: string; + code: string; + diagChecks: DiagnosticCheck[]; + refChecks?: (doc: any) => void; +} + +const testCases: Case[] = [ + { + title: "Basic model case", + code: ` + @service + namespace Ns1Valid { + model \`foo-/invalid*\td\` {} + op f(p: \`foo-/invalid*\td\`): void; + }`, + + diagChecks: [ + { + expectedDiagInvalidKey: "foo-/invalid*\td", + expectedDeclKey: "foo-_invalid__d", + expectedPrefix: "#/components/schemas/", + kind: "Model", + }, + ], + refChecks: (doc) => { + expect( + doc.paths["/"].post.requestBody.content["application/json"].schema.properties.p.$ref, + ).toBe("#/components/schemas/foo-_invalid__d"); + }, + }, + { + title: "Basic parameter case", + code: ` + @service + namespace Ns { + model Zoo { + @query + \`para/invalid\`: string; + b: string; + } + op f(...Zoo): string; + }`, + diagChecks: [ + { + expectedDiagInvalidKey: "Zoo.para/invalid", + expectedDeclKey: "Zoo.para_invalid", + expectedPrefix: "#/components/parameters/", + kind: "ModelProperty", + }, + ], + refChecks: (doc) => { + expect(doc.paths["/"].post.parameters[0].$ref).toBe( + "#/components/parameters/Zoo.para_invalid", + ); + }, + }, + { + title: "Nested model case", + code: ` + @service + namespace Ns1Valid { + model \`Nested/Model\` { + a: string; + } + model MMM { + b: \`Nested/Model\`; + } + op f(p: MMM): void; + }`, + diagChecks: [ + { + expectedDiagInvalidKey: "Nested/Model", + expectedDeclKey: "Nested_Model", + expectedPrefix: "#/components/schemas/", + kind: "Model", + }, + ], + }, + { + title: "Nested parameter case", + code: ` + @service + namespace NS { + model \`Nested/Model\` { + a: string; + } + model MMM { + @query \`b/b\`: \`Nested/Model\`; + d: string; + } + op f(...MMM): void; + }`, + diagChecks: [ + { + expectedDiagInvalidKey: "Nested/Model", + expectedDeclKey: "Nested_Model", + expectedPrefix: "#/components/schemas/", + kind: "Model", + }, + { + expectedDiagInvalidKey: "MMM.b/b", + expectedDeclKey: "MMM.b_b", + expectedPrefix: "#/components/parameters/", + kind: "ModelProperty", + }, + ], + refChecks: (doc) => { + expect(doc.components.parameters["MMM.b_b"].schema.$ref).toBe( + "#/components/schemas/Nested_Model", + ); + expect(doc.paths["/"].post.parameters[0].$ref).toBe("#/components/parameters/MMM.b_b"); + }, + }, + { + title: "Basic discriminated case", + code: ` + @service + namespace NS { + @discriminated() + union \`Pe/t\` { + cat: \`C/at\`, + dog: \`Do/g\`, + } + + model \`C/at\` { + kind: "cat"; + meow: boolean; + } + model \`Do/g\` { + kind: "dog"; + bark: boolean; + } + + op f(p: \`Pe/t\`): void; + }`, + diagChecks: [ + { + expectedDiagInvalidKey: "C/at", + expectedDeclKey: "C_at", + expectedPrefix: "#/components/schemas/", + kind: "Model", + }, + { + expectedDiagInvalidKey: "Pe/tCat", + expectedDeclKey: "Pe_tCat", + expectedPrefix: "#/components/schemas/", + kind: "Model", + }, + { + expectedDiagInvalidKey: "Do/g", + expectedDeclKey: "Do_g", + expectedPrefix: "#/components/schemas/", + kind: "Model", + }, + { + expectedDiagInvalidKey: "Pe/tDog", + expectedDeclKey: "Pe_tDog", + expectedPrefix: "#/components/schemas/", + kind: "Model", + }, + { + expectedDiagInvalidKey: "Pe/t", + expectedDeclKey: "Pe_t", + expectedPrefix: "#/components/schemas/", + kind: "Union", + }, + ], + }, + { + title: "Basic extend case", + code: ` + @service + namespace NS { + @discriminator("kind") + model \`Pe/t\`{ kind: string } + model \`C*at\` extends \`Pe/t\` {kind: "cat", meow: boolean} + model \`D*og\` extends \`Pe/t\` {kind: "dog", bark: boolean} + op f(p: \`Pe/t\`): void; + }`, + diagChecks: [ + { + expectedDiagInvalidKey: "C*at", + expectedDeclKey: "C_at", + expectedPrefix: "#/components/schemas/", + kind: "Model", + }, + { + expectedDiagInvalidKey: "D*og", + expectedDeclKey: "D_og", + expectedPrefix: "#/components/schemas/", + kind: "Model", + }, + { + expectedDiagInvalidKey: "Pe/t", + expectedDeclKey: "Pe_t", + expectedPrefix: "#/components/schemas/", + kind: "Model", + }, + ], + }, + { + title: "Basic generic case", + code: ` + @service + namespace NS { + model Foo {x: T;} + model \`x/x/x\` {a: string} + op read(x: \`x/x/x\`): Foo; + }`, + diagChecks: [ + { + expectedDiagInvalidKey: "x/x/x", + expectedDeclKey: "x_x_x", + expectedPrefix: "#/components/schemas/", + kind: "Model", + }, + ], + refChecks: (doc) => { + expect( + doc.paths["/"].post.requestBody.content["application/json"].schema.properties.x.$ref, + ).toBe("#/components/schemas/x_x_x"); + }, + }, +]; + +worksFor(["3.0.0", "3.1.0"], async (specHelpers) => { + describe("Invalid component key", () => { + it.each(testCases)( + "$title should report diagnostics and replace by valid key", + async (c: Case) => { + const [doc, diag] = await specHelpers.emitOpenApiWithDiagnostics(c.code); + + // check diagnostics + expectDiagnostics( + diag, + c.diagChecks.map((d) => createExpectedDiagnostic(d.expectedDiagInvalidKey)), + ); + for (const [i, d] of c.diagChecks.entries()) { + const target = diag[i].target as any; + expect(target).toHaveProperty("kind"); + expect(target.kind).toBe(d.kind); + // check generated doc + const componentField = getComponentField(doc, d.expectedPrefix); + expect(componentField).toBeDefined(); + expect(componentField).toHaveProperty(d.expectedDeclKey); + } + // check ref + if (c.refChecks) c.refChecks(doc); + }, + ); + }); +}); + +function getComponentField( + doc: OpenAPI3Document, + kind: "#/components/schemas/" | "#/components/parameters/", +) { + switch (kind) { + case "#/components/schemas/": + return doc.components?.schemas; + case "#/components/parameters/": + return doc.components?.parameters; + } +} + +function createExpectedDiagnostic(key: string) { + return { + code: "@typespec/openapi3/invalid-component-fixed-field-key", + message: `Invalid key '${key}' used in a fixed field of the Component object. Only alphanumerics, dot (.), hyphen (-), and underscore (_) characters are allowed in keys.`, + }; +} diff --git a/packages/openapi3/test/models.test.ts b/packages/openapi3/test/models.test.ts index 7de9cb731f..fd921303fd 100644 --- a/packages/openapi3/test/models.test.ts +++ b/packages/openapi3/test/models.test.ts @@ -1,3 +1,4 @@ +import { DiagnosticTarget } from "@typespec/compiler"; import { expectDiagnostics } from "@typespec/compiler/testing"; import { deepStrictEqual, ok, strictEqual } from "assert"; import { describe, expect, it } from "vitest"; @@ -123,6 +124,13 @@ worksFor(["3.0.0", "3.1.0"], ({ diagnoseOpenApiFor, oapiForModel, openApiFor }) code: "@typespec/openapi3/invalid-component-fixed-field-key", }, ]); + diagnostics.forEach((d) => { + const diagnosticTarget = d.target as DiagnosticTarget; + strictEqual( + diagnosticTarget && "kind" in diagnosticTarget && diagnosticTarget.kind === "Model", + true, + ); + }); }); });