From 3250409d3f0e466c588b3b4e9ccb3117668fbc99 Mon Sep 17 00:00:00 2001 From: Dereck Tu Date: Mon, 2 Feb 2026 10:32:57 -0500 Subject: [PATCH] fix: resolve name collisions Ticket: DX-2854 This commit resolves name collisions by appending a collision number to the component name --- packages/openapi-generator/src/cli.ts | 35 ++++- packages/openapi-generator/src/index.ts | 2 +- packages/openapi-generator/src/openapi.ts | 87 +++++++---- packages/openapi-generator/src/project.ts | 22 ++- .../test/externalModule.test.ts | 138 +++++++++++++++++- .../test/sample-types/nameCollision.ts | 9 ++ .../node_modules/@test/pkg-a/package.json | 5 + .../node_modules/@test/pkg-a/src/index.ts | 8 + .../node_modules/@test/pkg-b/package.json | 5 + .../node_modules/@test/pkg-b/src/index.ts | 7 + 10 files changed, 277 insertions(+), 41 deletions(-) create mode 100644 packages/openapi-generator/test/sample-types/nameCollision.ts create mode 100644 packages/openapi-generator/test/sample-types/node_modules/@test/pkg-a/package.json create mode 100644 packages/openapi-generator/test/sample-types/node_modules/@test/pkg-a/src/index.ts create mode 100644 packages/openapi-generator/test/sample-types/node_modules/@test/pkg-b/package.json create mode 100644 packages/openapi-generator/test/sample-types/node_modules/@test/pkg-b/src/index.ts diff --git a/packages/openapi-generator/src/cli.ts b/packages/openapi-generator/src/cli.ts index a1b64ab2..a51f8b77 100644 --- a/packages/openapi-generator/src/cli.ts +++ b/packages/openapi-generator/src/cli.ts @@ -157,13 +157,42 @@ const app = command({ ...Object.values(route.response), ]; }); + const componentLocations: Record = {}; + const componentCollisionCounters: Record = {}; + const componentNameMapping: Record> = {}; + + // Helper to generate unique component names (name, name1, name2, ...) + const getUniqueComponentName = (name: string): string => { + if (components[name] === undefined) { + return name; + } + const counter = (componentCollisionCounters[name] ?? 0) + 1; + componentCollisionCounters[name] = counter; + return `${name}${counter}`; + }; + let schema: Schema | undefined; while (((schema = queue.pop()), schema !== undefined)) { const refs = getRefs(schema, project.getTypes()); for (const ref of refs) { - if (components[ref.name] !== undefined) { + if ( + components[ref.name] !== undefined && + componentLocations[ref.name] === ref.location + ) { continue; } + + const componentName = + components[ref.name] !== undefined + ? getUniqueComponentName(ref.name) + : ref.name; + + // Track the mapping: location -> originalName -> finalComponentName + if (componentNameMapping[ref.location] === undefined) { + componentNameMapping[ref.location] = {}; + } + componentNameMapping[ref.location]![ref.name] = componentName; + const sourceFile = project.get(ref.location); if (sourceFile === undefined) { logError(`Could not find '${ref.name}' from '${ref.location}'`); @@ -217,7 +246,8 @@ const app = command({ codecE.right.comment = comment; } - components[ref.name] = codecE.right; + components[componentName] = codecE.right; + componentLocations[componentName] = ref.location; queue.push(codecE.right); } } @@ -231,6 +261,7 @@ const app = command({ servers, apiSpec, components, + componentNameMapping, ); console.log(JSON.stringify(openapi, null, 2)); diff --git a/packages/openapi-generator/src/index.ts b/packages/openapi-generator/src/index.ts index aa1a4b01..5104d4a9 100644 --- a/packages/openapi-generator/src/index.ts +++ b/packages/openapi-generator/src/index.ts @@ -1,7 +1,7 @@ export { parseApiSpec, parseApiSpecComment } from './apiSpec'; export { parseCodecInitializer, parsePlainInitializer } from './codec'; export { parseCommentBlock, type JSDoc } from './jsdoc'; -export { convertRoutesToOpenAPI } from './openapi'; +export { convertRoutesToOpenAPI, type ComponentNameMapping } from './openapi'; export { optimize } from './optimize'; export { Project } from './project'; export { getRefs } from './ref'; diff --git a/packages/openapi-generator/src/openapi.ts b/packages/openapi-generator/src/openapi.ts index 5c01eeff..c1c2e08b 100644 --- a/packages/openapi-generator/src/openapi.ts +++ b/packages/openapi-generator/src/openapi.ts @@ -7,8 +7,11 @@ import type { Route } from './route'; import type { Schema } from './ir'; import { Block } from 'comment-parser'; +export type ComponentNameMapping = Record>; + export function schemaToOpenAPI( schema: Schema, + componentNameMapping?: ComponentNameMapping, ): OpenAPIV3.SchemaObject | OpenAPIV3.ReferenceObject | undefined { schema = optimize(schema); @@ -59,16 +62,19 @@ export function schemaToOpenAPI( // Or should we just conflate explicit null and undefined properties? return { nullable: true, enum: [] }; case 'ref': + // Resolve the component name using the mapping (handles collisions) + const resolvedName = + componentNameMapping?.[schema.location]?.[schema.name] ?? schema.name; // if defaultOpenAPIObject is empty, no need to wrap the $ref in an allOf array if (Object.keys(defaultOpenAPIObject).length === 0) { - return { $ref: `#/components/schemas/${schema.name}` }; + return { $ref: `#/components/schemas/${resolvedName}` }; } return { - allOf: [{ $ref: `#/components/schemas/${schema.name}` }], + allOf: [{ $ref: `#/components/schemas/${resolvedName}` }], ...defaultOpenAPIObject, }; case 'array': - const innerSchema = schemaToOpenAPI(schema.items); + const innerSchema = schemaToOpenAPI(schema.items, componentNameMapping); if (innerSchema === undefined) { return undefined; } @@ -110,7 +116,7 @@ export function schemaToOpenAPI( ...defaultOpenAPIObject, properties: Object.entries(schema.properties).reduce( (acc, [name, prop]) => { - const innerSchema = schemaToOpenAPI(prop); + const innerSchema = schemaToOpenAPI(prop, componentNameMapping); if (innerSchema === undefined) { return acc; } @@ -124,7 +130,7 @@ export function schemaToOpenAPI( case 'intersection': return { allOf: schema.schemas.flatMap((s) => { - const innerSchema = schemaToOpenAPI(s); + const innerSchema = schemaToOpenAPI(s, componentNameMapping); if (innerSchema === undefined) { return []; } @@ -149,11 +155,14 @@ export function schemaToOpenAPI( const isOptional = schema.schemas.length >= 2 && undefinedSchema && nonUndefinedSchema; if (isOptional) { - return schemaToOpenAPI({ - ...nonUndefinedSchema, - comment: schema.comment, - ...(nullSchema ? { nullable: true } : {}), - }); + return schemaToOpenAPI( + { + ...nonUndefinedSchema, + comment: schema.comment, + ...(nullSchema ? { nullable: true } : {}), + }, + componentNameMapping, + ); } // This is an edge case for something like this -> t.union([WellDefinedCodec, t.unknown]) @@ -164,18 +173,24 @@ export function schemaToOpenAPI( const nonUnknownSchemas = schema.schemas.filter((s) => s.type !== 'any'); if (nonUnknownSchemas.length === 1 && nonUnknownSchemas[0] !== undefined) { - return schemaToOpenAPI({ - ...nonUnknownSchemas[0], - comment: schema.comment, - ...(nullSchema ? { nullable: true } : {}), - }); + return schemaToOpenAPI( + { + ...nonUnknownSchemas[0], + comment: schema.comment, + ...(nullSchema ? { nullable: true } : {}), + }, + componentNameMapping, + ); } else if (nonUnknownSchemas.length > 1) { - return schemaToOpenAPI({ - type: 'union', - schemas: nonUnknownSchemas, - comment: schema.comment, - ...(nullSchema ? { nullable: true } : {}), - }); + return schemaToOpenAPI( + { + type: 'union', + schemas: nonUnknownSchemas, + comment: schema.comment, + ...(nullSchema ? { nullable: true } : {}), + }, + componentNameMapping, + ); } } @@ -184,7 +199,7 @@ export function schemaToOpenAPI( nullable = true; continue; } - const innerSchema = schemaToOpenAPI(s); + const innerSchema = schemaToOpenAPI(s, componentNameMapping); if (innerSchema !== undefined) { oneOf.push(innerSchema); } @@ -215,11 +230,17 @@ export function schemaToOpenAPI( return { ...(nullable ? { nullable } : {}), oneOf, ...defaultOpenAPIObject }; } case 'record': - const additionalProperties = schemaToOpenAPI(schema.codomain); + const additionalProperties = schemaToOpenAPI( + schema.codomain, + componentNameMapping, + ); if (additionalProperties === undefined) return undefined; if (schema.domain !== undefined) { - const keys = schemaToOpenAPI(schema.domain) as OpenAPIV3.SchemaObject; + const keys = schemaToOpenAPI( + schema.domain, + componentNameMapping, + ) as OpenAPIV3.SchemaObject; if (keys.type === 'string' && keys.enum !== undefined) { const properties = keys.enum.reduce((acc, key) => { return { ...acc, [key]: additionalProperties }; @@ -333,7 +354,10 @@ export function schemaToOpenAPI( return openAPIObject; } -function routeToOpenAPI(route: Route): [string, string, OpenAPIV3.OperationObject] { +function routeToOpenAPI( + route: Route, + componentNameMapping?: ComponentNameMapping, +): [string, string, OpenAPIV3.OperationObject] { const jsdoc = route.comment !== undefined ? parseCommentBlock(route.comment) : {}; const operationId = jsdoc.tags?.operationId; const tag = jsdoc.tags?.tag ?? ''; @@ -367,7 +391,9 @@ function routeToOpenAPI(route: Route): [string, string, OpenAPIV3.OperationObjec : { requestBody: { content: { - 'application/json': { schema: schemaToOpenAPI(route.body) }, + 'application/json': { + schema: schemaToOpenAPI(route.body, componentNameMapping), + }, }, }, }; @@ -387,7 +413,7 @@ function routeToOpenAPI(route: Route): [string, string, OpenAPIV3.OperationObjec : {}), parameters: route.parameters.map((p) => { // Array types not allowed here - const schema = schemaToOpenAPI(p.schema); + const schema = schemaToOpenAPI(p.schema, componentNameMapping); if (schema && 'description' in schema) { delete schema.description; @@ -420,7 +446,7 @@ function routeToOpenAPI(route: Route): [string, string, OpenAPIV3.OperationObjec description, content: { 'application/json': { - schema: schemaToOpenAPI(response), + schema: schemaToOpenAPI(response, componentNameMapping), ...(example !== undefined ? { example } : undefined), }, }, @@ -436,10 +462,11 @@ export function convertRoutesToOpenAPI( servers: OpenAPIV3.ServerObject[], routes: Route[], schemas: Record, + componentNameMapping?: ComponentNameMapping, ): OpenAPIV3.Document { const paths = routes.reduce( (acc, route) => { - const [path, method, pathItem] = routeToOpenAPI(route); + const [path, method, pathItem] = routeToOpenAPI(route, componentNameMapping); let pathObject = acc[path] ?? {}; pathObject[method] = pathItem; return { ...acc, [path]: pathObject }; @@ -449,7 +476,7 @@ export function convertRoutesToOpenAPI( const openapiSchemas = Object.entries(schemas).reduce( (acc, [name, schema]) => { - const openapiSchema = schemaToOpenAPI(schema); + const openapiSchema = schemaToOpenAPI(schema, componentNameMapping); if (openapiSchema === undefined) { return acc; } else if ('$ref' in openapiSchema) { diff --git a/packages/openapi-generator/src/project.ts b/packages/openapi-generator/src/project.ts index 2aff1aed..1075690b 100644 --- a/packages/openapi-generator/src/project.ts +++ b/packages/openapi-generator/src/project.ts @@ -16,6 +16,7 @@ export class Project { private processedFiles: Record; private pendingFiles: Set; private types: Record; + private typeCollisionCounters: Record; private visitedPackages: Set; constructor(files: Record = {}, knownImports = KNOWN_IMPORTS) { @@ -23,6 +24,7 @@ export class Project { this.pendingFiles = new Set(); this.knownImports = knownImports; this.types = {}; + this.typeCollisionCounters = {}; this.visitedPackages = new Set(); } @@ -30,12 +32,23 @@ export class Project { this.processedFiles[path] = sourceFile; this.pendingFiles.delete(path); - // Update types mapping + // Update types mapping with collision handling for (const exp of sourceFile.symbols.exports) { - this.types[exp.exportedName] = path; + const name = this.getUniqueTypeName(exp.exportedName); + this.types[name] = path; } } + private getUniqueTypeName(name: string): string { + if (this.types[name] === undefined) { + return name; + } + + const counter = (this.typeCollisionCounters[name] ?? 0) + 1; + this.typeCollisionCounters[name] = counter; + return `${name}${counter}`; + } + get(path: string): SourceFile | undefined { return this.processedFiles[path]; } @@ -62,11 +75,6 @@ export class Project { continue; } - // map types to their file path - for (const exp of sourceFile.symbols.exports) { - this.types[exp.exportedName] = path; - } - this.add(path, sourceFile); // Process imports diff --git a/packages/openapi-generator/test/externalModule.test.ts b/packages/openapi-generator/test/externalModule.test.ts index 86805b92..de81427d 100644 --- a/packages/openapi-generator/test/externalModule.test.ts +++ b/packages/openapi-generator/test/externalModule.test.ts @@ -3,7 +3,14 @@ import assert from 'node:assert/strict'; import test from 'node:test'; import * as p from 'node:path'; -import { parsePlainInitializer, Project, type Schema } from '../src'; +import { + parsePlainInitializer, + Project, + type Schema, + type Route, + convertRoutesToOpenAPI, + type ComponentNameMapping, +} from '../src'; import { KNOWN_IMPORTS } from '../src/knownImports'; import { stripStacktraceOfErrors } from '../src/error'; @@ -199,3 +206,132 @@ testCase( {}, {}, ); + +test('parses types from external packages correctly', async () => { + const project = new Project({}, KNOWN_IMPORTS); + const entryPointPath = p.resolve('test/sample-types/nameCollision.ts'); + await project.parseEntryPoint(entryPointPath); + + const types = project.getTypes(); + + const pkgAPath = p.resolve('test/sample-types/node_modules/@test/pkg-a/src/index.ts'); + const pkgBPath = p.resolve('test/sample-types/node_modules/@test/pkg-b/src/index.ts'); + + // SharedType should map to pkg-a + assert.equal(types['SharedType'], pkgAPath); + + // SharedTypeCodec should map to pkg-b + assert.equal(types['SharedTypeCodec'], pkgBPath); + + // Verify both files were parsed correctly + const pkgAFile = project.get(pkgAPath); + const pkgBFile = project.get(pkgBPath); + + assert.notEqual(pkgAFile, undefined, 'pkg-a file should be parsed'); + assert.notEqual(pkgBFile, undefined, 'pkg-b file should be parsed'); +}); + +test('generates correct $ref for collided component names in OpenAPI output', async () => { + // This test verifies that when two different schemas have the same name but different + // locations, the OpenAPI output correctly references the renamed components. + // We simulate this by creating mock components with the same name from different "locations" + + const pkgAPath = '/mock/pkg-a/index.ts'; + const pkgBPath = '/mock/pkg-b/index.ts'; + + // Create routes that reference "SharedType" from two different locations + const routes: Route[] = [ + { + path: '/route-a', + method: 'GET', + parameters: [], + response: { + 200: { + type: 'ref', + name: 'SharedType', + location: pkgAPath, + }, + }, + }, + { + path: '/route-b', + method: 'GET', + parameters: [], + response: { + 200: { + type: 'ref', + name: 'SharedType', + location: pkgBPath, + }, + }, + }, + ]; + + // Simulate the component collection with collision handling + // First SharedType comes from pkgA, second from pkgB gets renamed to SharedType1 + const components: Record = { + SharedType: { + type: 'string', + enum: ['a', 'b', 'c'], + }, + SharedType1: { + type: 'string', + enum: ['x', 'y', 'z'], + }, + }; + + // Build the component name mapping: location -> originalName -> finalComponentName + const componentNameMapping: ComponentNameMapping = { + [pkgAPath]: { + SharedType: 'SharedType', + }, + [pkgBPath]: { + SharedType: 'SharedType1', + }, + }; + + // Convert to OpenAPI with the component name mapping + const openapi = convertRoutesToOpenAPI( + { title: 'Test', version: '1.0.0' }, + [], + routes, + components, + componentNameMapping, + ); + + // Verify the $ref values in the OpenAPI output + const routeAResponse = openapi.paths['/route-a']?.get?.responses?.['200']; + const routeBResponse = openapi.paths['/route-b']?.get?.responses?.['200']; + + assert.ok(routeAResponse, 'Route A should have a 200 response'); + assert.ok(routeBResponse, 'Route B should have a 200 response'); + + // Get the schema from the responses + const routeASchema = (routeAResponse as any).content?.['application/json']?.schema; + const routeBSchema = (routeBResponse as any).content?.['application/json']?.schema; + + assert.ok(routeASchema, 'Route A should have a schema'); + assert.ok(routeBSchema, 'Route B should have a schema'); + + // Verify $ref values point to the correct component + assert.equal( + routeASchema.$ref, + '#/components/schemas/SharedType', + 'Route A should reference SharedType (from pkg-a)', + ); + assert.equal( + routeBSchema.$ref, + '#/components/schemas/SharedType1', + 'Route B should reference SharedType1 (renamed from pkg-b)', + ); + + // Verify the components are correctly included + assert.ok( + openapi.components?.schemas?.['SharedType'], + 'SharedType component should exist', + ); + assert.ok( + openapi.components?.schemas?.['SharedType1'], + 'SharedType1 component should exist', + ); +}); diff --git a/packages/openapi-generator/test/sample-types/nameCollision.ts b/packages/openapi-generator/test/sample-types/nameCollision.ts new file mode 100644 index 00000000..da609d22 --- /dev/null +++ b/packages/openapi-generator/test/sample-types/nameCollision.ts @@ -0,0 +1,9 @@ +import * as t from 'io-ts'; +import { SharedType } from '@test/pkg-a'; +import { SharedTypeCodec } from '@test/pkg-b'; + +// This file uses SharedType from pkg-a and SharedTypeCodec from pkg-b +export const MyCodec = t.type({ + stateFromA: SharedType, + otherState: SharedTypeCodec, +}); diff --git a/packages/openapi-generator/test/sample-types/node_modules/@test/pkg-a/package.json b/packages/openapi-generator/test/sample-types/node_modules/@test/pkg-a/package.json new file mode 100644 index 00000000..ec04e337 --- /dev/null +++ b/packages/openapi-generator/test/sample-types/node_modules/@test/pkg-a/package.json @@ -0,0 +1,5 @@ +{ + "name": "@test/pkg-a", + "version": "0.0.1", + "types": "src/index.ts" +} diff --git a/packages/openapi-generator/test/sample-types/node_modules/@test/pkg-a/src/index.ts b/packages/openapi-generator/test/sample-types/node_modules/@test/pkg-a/src/index.ts new file mode 100644 index 00000000..164b4d91 --- /dev/null +++ b/packages/openapi-generator/test/sample-types/node_modules/@test/pkg-a/src/index.ts @@ -0,0 +1,8 @@ +import * as t from 'io-ts' + +export const SharedType = t.keyof({ + initialized: 1, + pendingSignature: 2, + signed: 3, + rejected: 4, +}); diff --git a/packages/openapi-generator/test/sample-types/node_modules/@test/pkg-b/package.json b/packages/openapi-generator/test/sample-types/node_modules/@test/pkg-b/package.json new file mode 100644 index 00000000..8048d99b --- /dev/null +++ b/packages/openapi-generator/test/sample-types/node_modules/@test/pkg-b/package.json @@ -0,0 +1,5 @@ +{ + "name": "@test/pkg-b", + "version": "0.0.1", + "types": "src/index.ts" +} diff --git a/packages/openapi-generator/test/sample-types/node_modules/@test/pkg-b/src/index.ts b/packages/openapi-generator/test/sample-types/node_modules/@test/pkg-b/src/index.ts new file mode 100644 index 00000000..f5293122 --- /dev/null +++ b/packages/openapi-generator/test/sample-types/node_modules/@test/pkg-b/src/index.ts @@ -0,0 +1,7 @@ +import * as t from 'io-ts' + +// This package exports SharedTypeCodec for testing +export const SharedTypeCodec = t.keyof({ + typeA: 1, + typeB: 2, +});