From 22a252f001ef266274881d0f5d130774fa403939 Mon Sep 17 00:00:00 2001 From: Nick Guerrera Date: Tue, 19 Apr 2022 12:00:32 -0500 Subject: [PATCH 1/3] Encode OpenAPI refs and remove concept of "ref-safe" names * Correctly URI-encode OpenAPI refs when necessary. Delete broken code that attempted to address this with name mangling. * Remove all regexes from how we decide what to inline and what to ref, and how to name things. The reasoning is now done on the type objects, and not on their serialized names. * The current choices are preserved modulo obvious bugs, but I will open an issue to revisit some of them. It will be easier to change after this refactoring. * Centralize the logic in the shared openapi lib for reuse by cadl-autorest. * Fix issues with the regex-based stripping of Cadl and service namespaces by replacing that with a namespace filter callback on Checker.getTypeName. * Defend against parameter key and type name collisions with diagnostics. --- .../url-encode-ref_2022-04-20-18-22.json | 10 ++ .../url-encode-ref_2022-04-20-18-22.json | 10 ++ .../url-encode-ref_2022-04-20-18-22.json | 10 ++ packages/compiler/core/checker.ts | 46 ++++-- packages/compiler/core/index.ts | 1 + packages/openapi/src/helpers.ts | 146 ++++++++++++++++++ packages/openapi/src/index.ts | 1 + packages/openapi/src/lib.ts | 7 + packages/openapi3/src/openapi.ts | 105 ++++--------- packages/openapi3/test/test-host.ts | 5 + packages/openapi3/test/test-openapi-output.ts | 43 +++++- packages/openapi3/test/test-parameters.ts | 59 ++++++- .../output/grpc-kiosk-example/openapi.json | 2 +- .../samples/test/output/nullable/openapi.json | 14 +- .../samples/test/output/optional/openapi.json | 2 +- .../samples/test/output/petstore/openapi.json | 4 +- .../test/output/rest/petstore/openapi.json | 38 ++--- .../testserver/body-complex/openapi.json | 6 +- .../testserver/body-string/openapi.json | 4 +- .../test/output/versioning/openapi.v2.json | 2 +- 20 files changed, 379 insertions(+), 136 deletions(-) create mode 100644 common/changes/@cadl-lang/compiler/url-encode-ref_2022-04-20-18-22.json create mode 100644 common/changes/@cadl-lang/openapi/url-encode-ref_2022-04-20-18-22.json create mode 100644 common/changes/@cadl-lang/openapi3/url-encode-ref_2022-04-20-18-22.json create mode 100644 packages/openapi/src/helpers.ts diff --git a/common/changes/@cadl-lang/compiler/url-encode-ref_2022-04-20-18-22.json b/common/changes/@cadl-lang/compiler/url-encode-ref_2022-04-20-18-22.json new file mode 100644 index 00000000000..611c336d53f --- /dev/null +++ b/common/changes/@cadl-lang/compiler/url-encode-ref_2022-04-20-18-22.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@cadl-lang/compiler", + "comment": "Add option to Checker.getTypeName to filter namespaces", + "type": "minor" + } + ], + "packageName": "@cadl-lang/compiler" +} \ No newline at end of file diff --git a/common/changes/@cadl-lang/openapi/url-encode-ref_2022-04-20-18-22.json b/common/changes/@cadl-lang/openapi/url-encode-ref_2022-04-20-18-22.json new file mode 100644 index 00000000000..45aa06bea82 --- /dev/null +++ b/common/changes/@cadl-lang/openapi/url-encode-ref_2022-04-20-18-22.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@cadl-lang/openapi", + "comment": "Add shared helpers for OpenAPI 2 and 3 emit", + "type": "minor" + } + ], + "packageName": "@cadl-lang/openapi" +} \ No newline at end of file diff --git a/common/changes/@cadl-lang/openapi3/url-encode-ref_2022-04-20-18-22.json b/common/changes/@cadl-lang/openapi3/url-encode-ref_2022-04-20-18-22.json new file mode 100644 index 00000000000..c2090525748 --- /dev/null +++ b/common/changes/@cadl-lang/openapi3/url-encode-ref_2022-04-20-18-22.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@cadl-lang/openapi3", + "comment": "URI-encode refs", + "type": "patch" + } + ], + "packageName": "@cadl-lang/openapi3" +} \ No newline at end of file diff --git a/packages/compiler/core/checker.ts b/packages/compiler/core/checker.ts index 898dc913c40..ca9c08ee309 100644 --- a/packages/compiler/core/checker.ts +++ b/packages/compiler/core/checker.ts @@ -94,6 +94,10 @@ import { } from "./types.js"; import { isArray } from "./util.js"; +export interface TypeNameOptions { + namespaceFilter: (ns: NamespaceType) => boolean; +} + export interface Checker { getTypeForNode(node: Node): Type; setUsingsForFile(file: CadlScriptNode): void; @@ -107,8 +111,8 @@ export interface Checker { getLiteralType(node: NumericLiteralNode): NumericLiteralType; getLiteralType(node: BooleanLiteralNode): BooleanLiteralType; getLiteralType(node: LiteralNode): LiteralType; - getTypeName(type: Type): string; - getNamespaceString(type: NamespaceType | undefined): string; + getTypeName(type: Type, options?: TypeNameOptions): string; + getNamespaceString(type: NamespaceType | undefined, options?: TypeNameOptions): string; cloneType(type: T): T; evalProjection(node: ProjectionNode, target: Type, args: Type[]): Type; project( @@ -407,18 +411,18 @@ export function createChecker(program: Program): Checker { return errorType; } - function getTypeName(type: Type): string { + function getTypeName(type: Type, options?: TypeNameOptions): string { switch (type.kind) { case "Model": - return getModelName(type); + return getModelName(type, options); case "Enum": - return getEnumName(type); + return getEnumName(type, options); case "Union": - return type.name || type.options.map(getTypeName).join(" | "); + return type.name || type.options.map((x) => getTypeName(x, options)).join(" | "); case "UnionVariant": - return getTypeName(type.type); + return getTypeName(type.type, options); case "Array": - return getTypeName(type.elementType) + "[]"; + return getTypeName(type.elementType, options) + "[]"; case "String": case "Number": case "Boolean": @@ -428,10 +432,18 @@ export function createChecker(program: Program): Checker { return "(unnamed type)"; } - function getNamespaceString(type: NamespaceType | undefined): string { - if (!type) return ""; - const parent = type.namespace; - return parent && parent.name !== "" ? `${getNamespaceString(parent)}.${type.name}` : type.name; + function getNamespaceString(type: NamespaceType | undefined, options?: TypeNameOptions): string { + if (!type || !type.name) { + return ""; + } + + const filter = options?.namespaceFilter; + if (filter && !filter(type)) { + return ""; + } + + const parent = getNamespaceString(type.namespace, options); + return parent ? `${parent}.${type.name}` : type.name; } function getFullyQualifiedSymbolName(sym: Sym | undefined): string { @@ -442,8 +454,8 @@ export function createChecker(program: Program): Checker { : sym.name; } - function getEnumName(e: EnumType): string { - const nsName = getNamespaceString(e.namespace); + function getEnumName(e: EnumType, options: TypeNameOptions | undefined): string { + const nsName = getNamespaceString(e.namespace, options); return nsName ? `${nsName}.${e.name}` : e.name; } @@ -456,12 +468,12 @@ export function createChecker(program: Program): Checker { return node.symbol!.id!; } - function getModelName(model: ModelType) { - const nsName = getNamespaceString(model.namespace); + function getModelName(model: ModelType, options: TypeNameOptions | undefined) { + const nsName = getNamespaceString(model.namespace, options); const modelName = (nsName ? nsName + "." : "") + (model.name || "(anonymous model)"); if (model.templateArguments && model.templateArguments.length > 0) { // template instantiation - const args = model.templateArguments.map(getTypeName); + const args = model.templateArguments.map((x) => getTypeName(x, options)); return `${modelName}<${args.join(", ")}>`; } else if ((model.node as ModelStatementNode).templateParameters?.length > 0) { // template diff --git a/packages/compiler/core/index.ts b/packages/compiler/core/index.ts index 7cf17c59519..5deea98dfa5 100644 --- a/packages/compiler/core/index.ts +++ b/packages/compiler/core/index.ts @@ -3,6 +3,7 @@ export * as decorators from "../lib/decorators.js"; export * from "../lib/service.js"; export * as service from "../lib/service.js"; export * from "../server/serverlib.js"; +export * from "./checker.js"; export * from "./decorator-utils.js"; export * from "./diagnostics.js"; export * from "./library.js"; diff --git a/packages/openapi/src/helpers.ts b/packages/openapi/src/helpers.ts new file mode 100644 index 00000000000..906125f079f --- /dev/null +++ b/packages/openapi/src/helpers.ts @@ -0,0 +1,146 @@ +import { + getFriendlyName as getAssignedFriendlyName, + ModelType, + ModelTypeProperty, + Program, + Type, + TypeNameOptions, +} from "@cadl-lang/compiler"; +import { reportDiagnostic } from "./lib.js"; + +/** + * Determines whether a type will be inlined in OpenAPI rather than defined + * as a schema and referenced. + * + * All anonymous types (anonymous models, arrays, tuples, etc.) are inlined. + * + * Template instantiations are inlined unless they have a friendly name. + * + * A friendly name can be provided by the user using `@friendlyName` + * decorator, or chosen by default in simple cases. + */ +export function shouldInline(program: Program, type: Type): boolean { + if (hasFriendlyName(program, type)) { + return false; + } + + switch (type.kind) { + case "Model": + return !type.name || hasTemplateArguments(type); + case "Enum": + case "Union": + return !type.name; + default: + return true; + } +} + +/** + * Gets the name of a type to be used in OpenAPI. + * + * For inlined types: this is the Cadl-native name written to `x-cadl-name`. + * + * For non-inlined types: this is either the friendly name or the Cadl-native name. + * + * Cadl-native names are shortened to exclude root `Cadl` namespace and service + * namespace using the provided `TypeNameOptions`. + */ +export function getTypeName( + program: Program, + type: Type, + options: TypeNameOptions, + existing?: Record +): string { + const name = + getFriendlyName(program, type, options) ?? program.checker!.getTypeName(type, options); + + if (existing && existing[name] !== undefined) { + reportDiagnostic(program, { + code: "duplicate-type-name", + format: { + value: name, + }, + target: type, + }); + } + + return name; +} + +/** + * Gets the key that is used to define a parameter in OpenAPI. + */ +export function getParameterKey( + program: Program, + propery: ModelTypeProperty, + newParam: unknown, + existingParams: Record, + options: TypeNameOptions +): string { + const parent = propery.model!; + let key = getTypeName(program, parent, options); + + if (parent.properties.size > 1) { + key += `.${propery.name}`; + } + + // JSON check is workaround for https://github.com/microsoft/cadl/issues/462 + if (existingParams[key] && JSON.stringify(newParam) !== JSON.stringify(existingParams[key])) { + reportDiagnostic(program, { + code: "duplicate-type-name", + messageId: "parameter", + format: { + value: key, + }, + target: propery, + }); + } + + return key; +} + +function hasTemplateArguments(type: Type): type is ModelType & { templateArguments: Type[] } { + return type.kind === "Model" && !!type.templateArguments && type.templateArguments.length > 0; +} + +function hasFriendlyName(program: Program, type: Type): boolean { + return !!getAssignedFriendlyName(program, type) || hasDefaultFriendlyName(program, type); +} + +function getFriendlyName(program: Program, type: Type, options: TypeNameOptions): string { + return getAssignedFriendlyName(program, type) ?? getDefaultFriendlyName(program, type, options); +} + +/** + * A template instantiation has a default friendly name if none if its type + * arguments are nested template instantiations or inlined types. + */ +function hasDefaultFriendlyName( + program: Program, + type: Type +): type is ModelType & { name: string; templateArguments: Type[] } { + return ( + type.kind === "Model" && + !!type.name && + hasTemplateArguments(type) && + !type.templateArguments.some((arg) => hasTemplateArguments(arg) || shouldInline(program, arg)) + ); +} + +/** + * Gets the default friendly name of the form Type_Arg1_..._ArgN when applicable as described + * by `hasDefaultFriendlyName`. Returns undefined when not applicable. + */ +function getDefaultFriendlyName( + program: Program, + type: Type, + options: TypeNameOptions +): string | undefined { + if (!hasDefaultFriendlyName(program, type)) { + return undefined; + } + const ns = program.checker!.getNamespaceString(type.namespace, options); + const model = (ns ? ns + "." : "") + type.name; + const args = type.templateArguments.map((arg) => getTypeName(program, arg, options)); + return `${model}_${args.join("_")}`; +} diff --git a/packages/openapi/src/index.ts b/packages/openapi/src/index.ts index 5400dba1865..4df9053d5dd 100644 --- a/packages/openapi/src/index.ts +++ b/packages/openapi/src/index.ts @@ -1 +1,2 @@ export * from "./decorators.js"; +export * from "./helpers.js"; diff --git a/packages/openapi/src/lib.ts b/packages/openapi/src/lib.ts index 07ad2c429f0..f8a5da3623b 100644 --- a/packages/openapi/src/lib.ts +++ b/packages/openapi/src/lib.ts @@ -9,6 +9,13 @@ export const libDef = { default: paramMessage`OpenAPI extension must start with 'x-' but was '${"value"}'`, }, }, + "duplicate-type-name": { + severity: "error", + messages: { + default: paramMessage`Duplicate type name: '${"value"}'. Check @friendlyName decorators and overlap with types in Cadl or service namespace.`, + parameter: paramMessage`Duplicate parameter key: '${"value"}'. Check @friendlyName decorators and overlap with types in Cadl or service namespace.` + }, + }, }, } as const; export const { reportDiagnostic } = createCadlLibrary(libDef); diff --git a/packages/openapi3/src/openapi.ts b/packages/openapi3/src/openapi.ts index 32b4b741437..e3fefd79440 100644 --- a/packages/openapi3/src/openapi.ts +++ b/packages/openapi3/src/openapi.ts @@ -7,7 +7,6 @@ import { getAllTags, getDoc, getFormat, - getFriendlyName, getIntrinsicModelName, getKnownValues, getMaxLength, @@ -37,10 +36,18 @@ import { Program, resolvePath, Type, + TypeNameOptions, UnionType, UnionTypeVariant, } from "@cadl-lang/compiler"; -import { getExtensions, getExternalDocs, getOperationId } from "@cadl-lang/openapi"; +import { + getExtensions, + getExternalDocs, + getOperationId, + getParameterKey, + getTypeName, + shouldInline, +} from "@cadl-lang/openapi"; import { Discriminator, getAllRoutes, @@ -171,6 +178,14 @@ function createOAPIEmitter(program: Program, options: OpenAPIEmitterOptions) { // De-dupe the per-endpoint tags that will be added into the #/tags let tags: Set; + const typeNameOptions: TypeNameOptions = { + // shorten type names by removing Cadl and service namespace + namespaceFilter(ns) { + const name = program.checker!.getNamespaceString(ns); + return name !== "Cadl" && name !== serviceNamespace; + }, + }; + return { emitOpenAPI }; function initializeEmitter(serviceNamespaceType: NamespaceType, version?: string) { @@ -418,12 +433,9 @@ function createOAPIEmitter(program: Program, options: OpenAPIEmitterOptions) { // For literal types, we just want to emit them directly as well. return mapCadlTypeToOpenAPI(type); } - const name = getTypeNameForSchemaProperties(type); - if (!isRefSafeName(name)) { - // Schema's name is not reference-able in OpenAPI so we inline it. - // This will usually happen with instantiated/anonymous types, but could also - // happen if Cadl identifier uses characters that are problematic for OpenAPI. - // Users will have to rename / alias type to have it get ref'ed. + const name = getTypeName(program, type, typeNameOptions); + + if (shouldInline(program, type)) { const schema = getSchemaForType(type); if (schema === undefined && isErrorType(type)) { @@ -439,7 +451,7 @@ function createOAPIEmitter(program: Program, options: OpenAPIEmitterOptions) { return schema; } else { const placeholder = { - $ref: "#/components/schemas/" + name, + $ref: "#/components/schemas/" + encodeURIComponent(name), }; schemas.add(type); return placeholder; @@ -576,21 +588,26 @@ function createOAPIEmitter(program: Program, options: OpenAPIEmitterOptions) { function emitReferences() { for (const [property, param] of params) { - const key = getParameterKey(property, param); + let key = getParameterKey( + program, + property, + param, + root.components.parameters, + typeNameOptions + ); root.components.parameters[key] = { ...param }; - for (const key of Object.keys(param)) { delete param[key]; } - param["$ref"] = "#/components/parameters/" + key; + param["$ref"] = "#/components/parameters/" + encodeURIComponent(key); } for (const type of schemas) { - const name = getTypeNameForSchemaProperties(type); const schemaForType = getSchemaForType(type); if (schemaForType) { + const name = getTypeName(program, type, typeNameOptions, root.components.schemas); root.components.schemas[name] = schemaForType; } } @@ -602,30 +619,6 @@ function createOAPIEmitter(program: Program, options: OpenAPIEmitterOptions) { } } - function getParameterKey(property: ModelTypeProperty, param: any) { - const parent = property.model!; - let key = program.checker!.getTypeName(parent); - let isQualifiedParamName = false; - - if (parent.properties.size > 1) { - key += `.${property.name}`; - isQualifiedParamName = true; - } - - // Try to shorten the type name to exclude the top-level service namespace - let baseKey = getRefSafeName(key); - if (serviceNamespace && key.startsWith(serviceNamespace)) { - baseKey = key.substring(serviceNamespace.length + 1); - - // If no parameter exists with the shortened name, use it, otherwise use the fully-qualified name - if (!root.components.parameters[baseKey] || isQualifiedParamName) { - key = baseKey; - } - } - - return key; - } - function getSchemaForType(type: Type) { const builtinType = mapCadlTypeToOpenAPI(type); if (builtinType !== undefined) return builtinType; @@ -1036,30 +1029,8 @@ function createOAPIEmitter(program: Program, options: OpenAPIEmitterOptions) { return !(headerInfo || queryInfo || pathInfo || statusCodeinfo); } - function getTypeNameForSchemaProperties(type: Type) { - // If there's a friendly name for the type, use that instead - let typeName = getFriendlyName(program, type); - if (typeName) { - return typeName; - } - - // Try to shorten the type name to exclude the top-level service namespace - typeName = program!.checker!.getTypeName(type).replace(/<([\w.]+)>/, "_$1"); - - if (isRefSafeName(typeName)) { - if (serviceNamespace) { - typeName = typeName.replace(RegExp(serviceNamespace + "\\.", "g"), ""); - } - // exclude the Cadl namespace in type names - typeName = typeName.replace(/($|_)(Cadl\.)/g, "$1"); - } - - return typeName; - } - function applyIntrinsicDecorators(cadlType: ModelType | ModelTypeProperty, target: any): any { const newTarget = { ...target }; - const docStr = getDoc(program, cadlType); const isString = isStringType(program, getPropertyType(cadlType)); const isNumeric = isNumericType(program, getPropertyType(cadlType)); @@ -1139,14 +1110,6 @@ function createOAPIEmitter(program: Program, options: OpenAPIEmitterOptions) { case "Model": return mapCadlIntrinsicModelToOpenAPI(cadlType); } - // // The base model doesn't correspond to a primitive OA type, but it could - // // derive from one. Let's check. - // if (cadlType.kind === "Model" && cadlType.baseModel) { - // const baseSchema = mapCadlTypeToOpenAPI(cadlType.baseModel); - // if (baseSchema) { - // return applyIntrinsicDecorators(cadlType, baseSchema); - // } - // } } /** @@ -1205,14 +1168,6 @@ function createOAPIEmitter(program: Program, options: OpenAPIEmitterOptions) { } } -function isRefSafeName(name: string) { - return /^[A-Za-z0-9-_.]+$/.test(name); -} - -function getRefSafeName(name: string) { - return name.replace(/^[A-Za-z0-9-_.]/g, "_"); -} - function prettierOutput(output: string) { return output + "\n"; } diff --git a/packages/openapi3/test/test-host.ts b/packages/openapi3/test/test-host.ts index 5c31ec07da6..75843d84f73 100644 --- a/packages/openapi3/test/test-host.ts +++ b/packages/openapi3/test/test-host.ts @@ -24,6 +24,11 @@ function versionedOutput(path: string, version: string) { return path.replace(".json", "." + version + ".json"); } +export async function diagnoseOpenApiFor(code: string) { + const runner = await createOpenAPITestRunner(); + return await runner.diagnose(code); +} + export async function openApiFor(code: string, versions?: string[]) { const host = await createOpenAPITestHost(); const outPath = resolveVirtualPath("openapi.json"); diff --git a/packages/openapi3/test/test-openapi-output.ts b/packages/openapi3/test/test-openapi-output.ts index 8315c4901cb..eb053dd270e 100644 --- a/packages/openapi3/test/test-openapi-output.ts +++ b/packages/openapi3/test/test-openapi-output.ts @@ -1,6 +1,11 @@ import { expectDiagnostics } from "@cadl-lang/compiler/testing"; import { deepStrictEqual, ok, strictEqual } from "assert"; -import { createOpenAPITestRunner, oapiForModel, openApiFor } from "./test-host.js"; +import { + createOpenAPITestRunner, + diagnoseOpenApiFor, + oapiForModel, + openApiFor, +} from "./test-host.js"; describe("openapi3: definitions", () => { it("defines models", async () => { @@ -21,6 +26,32 @@ describe("openapi3: definitions", () => { }); }); + it("errors on duplicate model names", async () => { + const diagnostics = await diagnoseOpenApiFor( + ` + model P { + propA: string; + } + + @friendlyName("P") + model Q { + propB: string; + } + + @route("/test1") + @get + op test1(p: P): Q; + ` + ); + + expectDiagnostics(diagnostics, [ + { + code: "@cadl-lang/openapi/duplicate-type-name", + message: /type/, + }, + ]); + }); + it("doesn't define anonymous or unconnected models", async () => { const res = await oapiForModel( "{ ... Foo }", @@ -428,7 +459,7 @@ describe("openapi3: definitions", () => { name: { type: "string", nullable: true, - "x-cadl-name": "Cadl.string | Cadl.null", + "x-cadl-name": "string | null", }, }, required: ["name"], @@ -455,7 +486,7 @@ describe("openapi3: definitions", () => { format: "int32", }, nullable: true, - "x-cadl-name": "Cadl.int32[] | Cadl.null", + "x-cadl-name": "int32[] | null", }, }, required: ["name"], @@ -479,7 +510,7 @@ describe("openapi3: definitions", () => { type: "string", enum: ["cat", "dog"], nullable: true, - "x-cadl-name": "cat | dog | Cadl.null", + "x-cadl-name": "cat | dog | null", }, }, required: ["type"], @@ -539,7 +570,7 @@ describe("openapi3: definitions", () => { `); ok(openApi.components.schemas.Cat, "expected definition named Cat"); deepStrictEqual(openApi.paths["/"].post.requestBody.content["application/json"].schema, { - "x-cadl-name": "Cat | Cadl.string", + "x-cadl-name": "Cat | string", anyOf: [{ $ref: "#/components/schemas/Cat" }, { type: "string" }], }); }); @@ -599,7 +630,7 @@ describe("openapi3: definitions", () => { `); ok(openApi.components.schemas.Cat, "expected definition named Cat"); deepStrictEqual(openApi.paths["/"].get.responses["200"].content["application/json"].schema, { - "x-cadl-name": "Cat | Cadl.string", + "x-cadl-name": "Cat | string", anyOf: [{ $ref: "#/components/schemas/Cat" }, { type: "string" }], }); }); diff --git a/packages/openapi3/test/test-parameters.ts b/packages/openapi3/test/test-parameters.ts index 2467d78f2ac..08d7b44df4a 100644 --- a/packages/openapi3/test/test-parameters.ts +++ b/packages/openapi3/test/test-parameters.ts @@ -1,5 +1,6 @@ -import { deepStrictEqual, strictEqual } from "assert"; -import { openApiFor } from "./test-host.js"; +import { expectDiagnostics } from "@cadl-lang/compiler/testing"; +import { deepStrictEqual, ok, strictEqual } from "assert"; +import { diagnoseOpenApiFor, openApiFor } from "./test-host.js"; describe("openapi3: parameters", () => { it("create a query param", async () => { @@ -23,4 +24,58 @@ describe("openapi3: parameters", () => { strictEqual(res.paths["/"].get.parameters[0].description, "mydoc"); strictEqual(res.paths["/"].get.parameters[0].schema.description, undefined); }); + + it("errors on duplicate parameter keys", async () => { + const diagnostics = await diagnoseOpenApiFor( + ` + model P { + @query id: string; + } + + @friendlyName("P") + model Q { + @header id: string; + } + + @route("/test1") + op test1(...P): void; + + @route("/test2") + op test2(...Q): void; + ` + ); + + expectDiagnostics(diagnostics, [ + { + code: "@cadl-lang/openapi/duplicate-type-name", + message: /parameter/, + }, + ]); + }); + + it("encodes parameter keys in references", async () => { + const oapi = await openApiFor( + ` + model Pet extends Pet$Id { + name: string; + } + model Pet$Id { + @path + petId: string; + } + @route("/Pets") + namespace root { + @get() + op get(... Pet$Id): Pet; + } + ` + ); + + ok(oapi.paths["/Pets/{petId}"].get); + strictEqual( + oapi.paths["/Pets/{petId}"].get.parameters[0]["$ref"], + "#/components/parameters/Pet%24Id" + ); + strictEqual(oapi.components.parameters["Pet$Id"].name, "petId"); + }); }); diff --git a/packages/samples/test/output/grpc-kiosk-example/openapi.json b/packages/samples/test/output/grpc-kiosk-example/openapi.json index bd04605091c..482b0ee58a7 100644 --- a/packages/samples/test/output/grpc-kiosk-example/openapi.json +++ b/packages/samples/test/output/grpc-kiosk-example/openapi.json @@ -328,7 +328,7 @@ "type": "integer", "format": "int32" }, - "x-cadl-name": "Cadl.int32[]" + "x-cadl-name": "int32[]" } } } diff --git a/packages/samples/test/output/nullable/openapi.json b/packages/samples/test/output/nullable/openapi.json index 191f6af4e4a..8b6397576a6 100644 --- a/packages/samples/test/output/nullable/openapi.json +++ b/packages/samples/test/output/nullable/openapi.json @@ -47,7 +47,7 @@ "num" ], "nullable": true, - "x-cadl-name": "AnotherModel | Cadl.null" + "x-cadl-name": "AnotherModel | null" } } } @@ -70,7 +70,7 @@ "strOrNull": { "type": "string", "nullable": true, - "x-cadl-name": "Cadl.string | Cadl.null" + "x-cadl-name": "string | null" }, "modelOrNull": { "type": "object", @@ -84,7 +84,7 @@ "num" ], "nullable": true, - "x-cadl-name": "AnotherModel | Cadl.null" + "x-cadl-name": "AnotherModel | null" }, "literalsOrNull": { "type": "string", @@ -93,12 +93,12 @@ "two" ], "nullable": true, - "x-cadl-name": "one | two | Cadl.null" + "x-cadl-name": "one | two | null" }, "manyNullsOneString": { "type": "string", "nullable": true, - "x-cadl-name": "Cadl.null | Cadl.null | Cadl.string | Cadl.null" + "x-cadl-name": "null | null | string | null" }, "manyNullsSomeValues": { "type": "number", @@ -107,7 +107,7 @@ 100 ], "nullable": true, - "x-cadl-name": "Cadl.null | 42 | Cadl.null | 100 | Cadl.null" + "x-cadl-name": "null | 42 | null | 100 | null" }, "arr": { "type": "array", @@ -115,7 +115,7 @@ "type": "string" }, "nullable": true, - "x-cadl-name": "Cadl.string[] | Cadl.null" + "x-cadl-name": "string[] | null" } }, "required": [ diff --git a/packages/samples/test/output/optional/openapi.json b/packages/samples/test/output/optional/openapi.json index 6b61eec5b01..370e549192d 100644 --- a/packages/samples/test/output/optional/openapi.json +++ b/packages/samples/test/output/optional/openapi.json @@ -71,7 +71,7 @@ "items": { "type": "string" }, - "x-cadl-name": "Cadl.string[]", + "x-cadl-name": "string[]", "default": [ "foo", "bar" diff --git a/packages/samples/test/output/petstore/openapi.json b/packages/samples/test/output/petstore/openapi.json index 90e58761f8f..f7bbc302a25 100644 --- a/packages/samples/test/output/petstore/openapi.json +++ b/packages/samples/test/output/petstore/openapi.json @@ -231,7 +231,7 @@ "items": { "$ref": "#/components/schemas/Pet" }, - "x-cadl-name": "PetStore.Pet[]" + "x-cadl-name": "Pet[]" }, "nextLink": { "type": "string" @@ -270,7 +270,7 @@ "items": { "$ref": "#/components/schemas/Toy" }, - "x-cadl-name": "PetStore.Toy[]" + "x-cadl-name": "Toy[]" }, "nextLink": { "type": "string" diff --git a/packages/samples/test/output/rest/petstore/openapi.json b/packages/samples/test/output/rest/petstore/openapi.json index ed6ead76bdf..f55cf955dc3 100644 --- a/packages/samples/test/output/rest/petstore/openapi.json +++ b/packages/samples/test/output/rest/petstore/openapi.json @@ -92,7 +92,7 @@ } }, "description": "The template for adding optional properties.", - "x-cadl-name": "Cadl.OptionalProperties" + "x-cadl-name": "OptionalProperties>" } } } @@ -180,7 +180,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/Cadl.Rest.Resource.Page_Pet" + "$ref": "#/components/schemas/Rest.Resource.Page_Pet" } } } @@ -268,7 +268,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/Cadl.Rest.Resource.Page_Checkup" + "$ref": "#/components/schemas/Rest.Resource.Page_Checkup" } } } @@ -367,7 +367,7 @@ } }, "description": "The template for adding optional properties.", - "x-cadl-name": "Cadl.OptionalProperties" + "x-cadl-name": "OptionalProperties>" } } } @@ -432,7 +432,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/Cadl.Rest.Resource.Page_Toy" + "$ref": "#/components/schemas/Rest.Resource.Page_Toy" } } } @@ -537,7 +537,7 @@ } }, "description": "The template for adding optional properties.", - "x-cadl-name": "Cadl.OptionalProperties" + "x-cadl-name": "OptionalProperties>" } } } @@ -607,7 +607,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/Cadl.Rest.Resource.Page_Checkup" + "$ref": "#/components/schemas/Rest.Resource.Page_Checkup" } } } @@ -702,7 +702,7 @@ } }, "description": "The template for adding optional properties.", - "x-cadl-name": "Cadl.OptionalProperties" + "x-cadl-name": "OptionalProperties>" } } } @@ -790,7 +790,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/Cadl.Rest.Resource.Page_Owner" + "$ref": "#/components/schemas/Rest.Resource.Page_Owner" } } } @@ -878,7 +878,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/Cadl.Rest.Resource.Page_Checkup" + "$ref": "#/components/schemas/Rest.Resource.Page_Checkup" } } } @@ -977,7 +977,7 @@ } }, "description": "The template for adding optional properties.", - "x-cadl-name": "Cadl.OptionalProperties" + "x-cadl-name": "OptionalProperties>" } } } @@ -1072,7 +1072,7 @@ "message" ] }, - "Cadl.Rest.Resource.Page_Pet": { + "Rest.Resource.Page_Pet": { "type": "object", "properties": { "value": { @@ -1080,7 +1080,7 @@ "items": { "$ref": "#/components/schemas/Pet" }, - "x-cadl-name": "PetStore.Pet[]", + "x-cadl-name": "Pet[]", "description": "The items on this page" }, "nextLink": { @@ -1113,7 +1113,7 @@ "notes" ] }, - "Cadl.Rest.Resource.Page_Checkup": { + "Rest.Resource.Page_Checkup": { "type": "object", "properties": { "value": { @@ -1121,7 +1121,7 @@ "items": { "$ref": "#/components/schemas/Checkup" }, - "x-cadl-name": "PetStore.Checkup[]", + "x-cadl-name": "Checkup[]", "description": "The items on this page" }, "nextLink": { @@ -1176,7 +1176,7 @@ "name" ] }, - "Cadl.Rest.Resource.Page_Toy": { + "Rest.Resource.Page_Toy": { "type": "object", "properties": { "value": { @@ -1184,7 +1184,7 @@ "items": { "$ref": "#/components/schemas/Toy" }, - "x-cadl-name": "PetStore.Toy[]", + "x-cadl-name": "Toy[]", "description": "The items on this page" }, "nextLink": { @@ -1218,7 +1218,7 @@ "age" ] }, - "Cadl.Rest.Resource.Page_Owner": { + "Rest.Resource.Page_Owner": { "type": "object", "properties": { "value": { @@ -1226,7 +1226,7 @@ "items": { "$ref": "#/components/schemas/Owner" }, - "x-cadl-name": "PetStore.Owner[]", + "x-cadl-name": "Owner[]", "description": "The items on this page" }, "nextLink": { diff --git a/packages/samples/test/output/testserver/body-complex/openapi.json b/packages/samples/test/output/testserver/body-complex/openapi.json index d964b7c0dfd..30be7172c33 100644 --- a/packages/samples/test/output/testserver/body-complex/openapi.json +++ b/packages/samples/test/output/testserver/body-complex/openapi.json @@ -770,7 +770,7 @@ "type": "integer", "format": "int32", "nullable": true, - "x-cadl-name": "Cadl.int32 | Cadl.null", + "x-cadl-name": "int32 | null", "description": "Basic Id" }, "name": { @@ -894,7 +894,7 @@ "null": { "type": "string", "nullable": true, - "x-cadl-name": "Cadl.string | Cadl.null" + "x-cadl-name": "string | null" } }, "required": [ @@ -969,7 +969,7 @@ "items": { "type": "string" }, - "x-cadl-name": "Cadl.string[]" + "x-cadl-name": "string[]" } }, "required": [ diff --git a/packages/samples/test/output/testserver/body-string/openapi.json b/packages/samples/test/output/testserver/body-string/openapi.json index ef3de1e4813..088f34a8480 100644 --- a/packages/samples/test/output/testserver/body-string/openapi.json +++ b/packages/samples/test/output/testserver/body-string/openapi.json @@ -26,7 +26,7 @@ "schema": { "type": "string", "nullable": true, - "x-cadl-name": "Cadl.string | Cadl.null" + "x-cadl-name": "string | null" } } } @@ -71,7 +71,7 @@ "schema": { "type": "string", "nullable": true, - "x-cadl-name": "Cadl.string | Cadl.null" + "x-cadl-name": "string | null" } } } diff --git a/packages/samples/test/output/versioning/openapi.v2.json b/packages/samples/test/output/versioning/openapi.v2.json index ab58eea153a..b6928e3906c 100644 --- a/packages/samples/test/output/versioning/openapi.v2.json +++ b/packages/samples/test/output/versioning/openapi.v2.json @@ -122,7 +122,7 @@ "items": { "type": "string" }, - "x-cadl-name": "Cadl.string[]" + "x-cadl-name": "string[]" } }, "required": [ From 1a46b6c8220a1d3d5ac993fd95440968638bbeca Mon Sep 17 00:00:00 2001 From: Nick Guerrera Date: Wed, 20 Apr 2022 13:57:57 -0500 Subject: [PATCH 2/3] format and lint --- packages/openapi/src/lib.ts | 2 +- packages/openapi3/src/openapi.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/openapi/src/lib.ts b/packages/openapi/src/lib.ts index f8a5da3623b..05bcae81bbe 100644 --- a/packages/openapi/src/lib.ts +++ b/packages/openapi/src/lib.ts @@ -13,7 +13,7 @@ export const libDef = { severity: "error", messages: { default: paramMessage`Duplicate type name: '${"value"}'. Check @friendlyName decorators and overlap with types in Cadl or service namespace.`, - parameter: paramMessage`Duplicate parameter key: '${"value"}'. Check @friendlyName decorators and overlap with types in Cadl or service namespace.` + parameter: paramMessage`Duplicate parameter key: '${"value"}'. Check @friendlyName decorators and overlap with types in Cadl or service namespace.`, }, }, }, diff --git a/packages/openapi3/src/openapi.ts b/packages/openapi3/src/openapi.ts index e3fefd79440..2b055a5416d 100644 --- a/packages/openapi3/src/openapi.ts +++ b/packages/openapi3/src/openapi.ts @@ -588,7 +588,7 @@ function createOAPIEmitter(program: Program, options: OpenAPIEmitterOptions) { function emitReferences() { for (const [property, param] of params) { - let key = getParameterKey( + const key = getParameterKey( program, property, param, From 168fee413205dc63aad1fd804bdf08efd7ed57c3 Mon Sep 17 00:00:00 2001 From: Nick Guerrera Date: Thu, 21 Apr 2022 09:04:35 -0500 Subject: [PATCH 3/3] fix small inconsistency between two nearby lines --- packages/openapi/src/helpers.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/openapi/src/helpers.ts b/packages/openapi/src/helpers.ts index 906125f079f..2dc8228cb2a 100644 --- a/packages/openapi/src/helpers.ts +++ b/packages/openapi/src/helpers.ts @@ -54,7 +54,7 @@ export function getTypeName( const name = getFriendlyName(program, type, options) ?? program.checker!.getTypeName(type, options); - if (existing && existing[name] !== undefined) { + if (existing && existing[name]) { reportDiagnostic(program, { code: "duplicate-type-name", format: {