From 839f2e1cc915e104f62bdba7ead67b805319ab80 Mon Sep 17 00:00:00 2001 From: Harm Staack Date: Thu, 30 Jul 2026 00:28:51 +0200 Subject: [PATCH] [R-392] fix: Fix duplicated identifiers in functions.ts and index.ts (#392) Co-authored-by: Claude Code --- lib/ng-openapi-gen.ts | 54 ++++++++++++++++--- lib/operation-variant.ts | 2 +- test/duplicate-x-operation-name.json | 6 +-- test/duplicate-x-operation-name.spec.ts | 47 +++++++++++------ test/multi-tag-operations.config.json | 7 +++ test/multi-tag-operations.json | 30 +++++++++++ test/multi-tag-operations.spec.ts | 70 +++++++++++++++++++++++++ 7 files changed, 190 insertions(+), 26 deletions(-) create mode 100644 test/multi-tag-operations.config.json create mode 100644 test/multi-tag-operations.json create mode 100644 test/multi-tag-operations.spec.ts diff --git a/lib/ng-openapi-gen.ts b/lib/ng-openapi-gen.ts index cf2e3105..8f8fcc46 100644 --- a/lib/ng-openapi-gen.ts +++ b/lib/ng-openapi-gen.ts @@ -21,6 +21,7 @@ import { HandlebarsManager } from './handlebars-manager'; import { Logger } from './logger'; import { Model } from './model'; import { Operation } from './operation'; +import { OperationVariant } from './operation-variant'; import { Options } from './options'; import { Service } from './service'; import { Templates } from './templates'; @@ -102,37 +103,57 @@ export class NgOpenApiGen { const services = [...this.services.values()]; for (const service of services) { if (generateServices) { + this.checkDuplicatedMethods(service); this.write('service', service, service.fileName, 'services'); } } - // Generate each function - const functions = services.reduce((acc, service) => [ + // Generate each function. Services of a multi-tagged operation share its variants, so + // de-duplicate them (see #379) + const allFunctions: OperationVariant[] = services.reduce((acc, service) => [ ...acc, ...service.operations.reduce((opAcc, operation) => [ ...opAcc, ...operation.variants ], []) ], []); + const functions = [...new Set(allFunctions)]; - // Detect duplicates by methodName and set exportName + // Detect duplicated method names, such as the same 'x-operation-name' under different tags (see #389) const methodNameCounts = new Map(); for (const fn of functions) { const count = methodNameCounts.get(fn.methodName) || 0; methodNameCounts.set(fn.methodName, count + 1); } - // Set exportName and paramsTypeExportName, then write each function + // Set exportName and paramsTypeExportName, disambiguating duplicates by tag, then write each function + const usedExportNames = new Set(); + const writtenFiles = new Set(); for (const fn of functions) { const isDuplicate = (methodNameCounts.get(fn.methodName) || 0) > 1; - if (isDuplicate) { - const tagSuffix = upperFirst(fn.operation.tag); + const tagSuffix = isDuplicate ? upperFirst(methodName(fn.tag)) : ''; + + if (tagSuffix) { fn.exportName = fn.methodName + tagSuffix; - fn.paramsTypeExportName = fn.paramsType.replace('$Params', '') + tagSuffix + '$Params'; + fn.paramsTypeExportName = fn.paramsType.replace(/\$Params$/, '') + tagSuffix + '$Params'; } else { fn.exportName = fn.importName; fn.paramsTypeExportName = fn.paramsType; } + + // Duplicated export name or file. The tag cannot disambiguate, so fail + const file = `${fn.importPath}/${fn.importFile}`; + if (usedExportNames.has(fn.exportName)) { + throw new Error(`Multiple operations would be exported as '${fn.exportName}'. ` + + 'Make sure operation ids (or \'x-operation-name\') are unique within a tag.'); + } + if (writtenFiles.has(file)) { + throw new Error(`Multiple operations would be generated to the file '${file}.ts'. ` + + 'Make sure operation ids (or \'x-operation-name\') are unique within a tag.'); + } + usedExportNames.add(fn.exportName); + writtenFiles.add(file); + this.write('fn', fn, fn.importFile, fn.importPath); } @@ -177,6 +198,25 @@ export class NgOpenApiGen { } } + /** + * A service can't declare the same method twice. Only reachable when distinct operations + * sharing a tag also share a method name (see #392) + */ + private checkDuplicatedMethods(service: Service) { + const variantsByMethodName = new Map(); + for (const operation of service.operations) { + for (const variant of operation.variants) { + const other = variantsByMethodName.get(variant.methodName); + if (other) { + throw new Error(`Operations '${other.operation.id}' and '${variant.operation.id}' would both be generated ` + + `as method '${variant.methodName}' of the service for tag '${service.name}'. ` + + 'Make sure operation ids (or \'x-operation-name\') are unique among operations sharing a tag.'); + } + variantsByMethodName.set(variant.methodName, variant); + } + } + } + private write(template: string, model: any, baseName: string, subDir?: string) { const ts = this.setEndOfLine(this.templates.apply(template, model)); const file = path.join(this.tempDir, subDir || '.', `${baseName}.ts`); diff --git a/lib/operation-variant.ts b/lib/operation-variant.ts index 473d0a29..b9f9c620 100644 --- a/lib/operation-variant.ts +++ b/lib/operation-variant.ts @@ -149,6 +149,6 @@ To access the full response (for headers, for example), \`${this.responseMethodN } get tag() { - return this.operation.tags[0] || this.options.defaultTag || 'Api'; + return this.operation.tag; } } diff --git a/test/duplicate-x-operation-name.json b/test/duplicate-x-operation-name.json index aa484611..4e43853c 100644 --- a/test/duplicate-x-operation-name.json +++ b/test/duplicate-x-operation-name.json @@ -1,15 +1,15 @@ { "openapi": "3.0.0", "info": { - "title": "ng-openapi-gen-duplicated-operation-name", + "title": "ng-openapi-gen-duplicate-x-operation-name", "version": "1.0.0" }, "paths": { "/api/car/consumption": { "get": { - "x-controller-name": "Car", + "x-controller-name": "Car report", "x-operation-name": "getConsumption", - "tags": ["Car"], + "tags": ["Car report"], "responses": { "200": { "description": "Consumption", diff --git a/test/duplicate-x-operation-name.spec.ts b/test/duplicate-x-operation-name.spec.ts index 68580529..a30a5e71 100644 --- a/test/duplicate-x-operation-name.spec.ts +++ b/test/duplicate-x-operation-name.spec.ts @@ -9,22 +9,39 @@ const gen = new NgOpenApiGen(spec as OpenAPIObject, options as Options); gen.generate(); describe('Generation tests using duplicate-x-operation-name.json', () => { - it('index.ts should have both functions and parameters', () => { - // Read file options.output + '/index.ts' - const content = fs.readFileSync(options.output + '/index.ts', 'utf8'); - expect(content).toContain('export { getConsumption as getConsumptionCar }'); - expect(content).toContain('export type { GetConsumption$Params as GetConsumptionCar$Params }'); - expect(content).toContain('export { getConsumption as getConsumptionPlane }'); - expect(content).toContain('export type { GetConsumption$Params as GetConsumptionPlane$Params }'); - }); + for (const file of ['functions.ts', 'index.ts']) { + describe(file, () => { + const content = fs.readFileSync(`${options.output}/${file}`, 'utf8'); + + it('should disambiguate operations sharing a method name by their tag', () => { + expect(content).toContain('export { getConsumption as getConsumptionPlane }'); + expect(content).toContain('export type { GetConsumption$Params as GetConsumptionPlane$Params }'); + }); - it('functions.ts should have both functions and parameters', () => { - // Read file options.output + '/functions.ts' - const content = fs.readFileSync(options.output + '/functions.ts', 'utf8'); - expect(content).toContain('export { getConsumption as getConsumptionCar }'); - expect(content).toContain('export type { GetConsumption$Params as GetConsumptionCar$Params }'); - expect(content).toContain('export { getConsumption as getConsumptionPlane }'); - expect(content).toContain('export type { GetConsumption$Params as GetConsumptionPlane$Params }'); + it('should turn the tag into a valid identifier before using it as a suffix', () => { + // The tag `Car report` must be camelized, not appended verbatim + expect(content).toContain('export { getConsumption as getConsumptionCarReport }'); + expect(content).toContain('export type { GetConsumption$Params as GetConsumptionCarReport$Params }'); + }); + }); + } + + it('should fail generation when the method name is duplicated within a tag', () => { + // With the same tag on both operations, the tag cannot disambiguate anymore + const clashingSpec = JSON.parse(JSON.stringify(spec)) as OpenAPIObject; + (clashingSpec.paths!['/api/plane/consumption'] as any).get.tags = ['Car report']; + const clashingGen = new NgOpenApiGen(clashingSpec, { ...options, output: 'out/duplicate-x-operation-name-clash' } as Options); + expect(() => clashingGen.generate()).toThrowError(/unique within a tag/); }); + it('should fail generation when operations sharing a tag share a method name', () => { + // A shared super-category tag groups both operations into one service, which can't declare + // the same method twice + const clashingSpec = JSON.parse(JSON.stringify(spec)) as OpenAPIObject; + (clashingSpec.paths!['/api/car/consumption'] as any).get.tags.push('Report'); + (clashingSpec.paths!['/api/plane/consumption'] as any).get.tags.push('Report'); + const clashingGen = new NgOpenApiGen(clashingSpec, + { ...options, services: true, output: 'out/duplicate-x-operation-name-shared-tag' } as Options); + expect(() => clashingGen.generate()).toThrowError(/unique among operations sharing a tag/); + }); }); diff --git a/test/multi-tag-operations.config.json b/test/multi-tag-operations.config.json new file mode 100644 index 00000000..5bd92718 --- /dev/null +++ b/test/multi-tag-operations.config.json @@ -0,0 +1,7 @@ +{ + "$schema": "../ng-openapi-gen-schema.json", + "input": "test/multi-tag-operations.json", + "output": "out/multi-tag-operations", + "indexFile": true, + "services": true +} diff --git a/test/multi-tag-operations.json b/test/multi-tag-operations.json new file mode 100644 index 00000000..d47f16ed --- /dev/null +++ b/test/multi-tag-operations.json @@ -0,0 +1,30 @@ +{ + "openapi": "3.0.0", + "info": { + "title": "ng-openapi-gen-multi-tag-operations", + "version": "1.0.0" + }, + "paths": { + "/appointments/{id}": { + "delete": { + "operationId": "deleteAppointment", + "tags": ["Appointment", "Called by frontend"], + "parameters": [ + { + "name": "id", + "in": "path", + "required": true, + "schema": { + "type": "string" + } + } + ], + "responses": { + "204": { + "description": "Deleted" + } + } + } + } + } +} diff --git a/test/multi-tag-operations.spec.ts b/test/multi-tag-operations.spec.ts new file mode 100644 index 00000000..91b8de80 --- /dev/null +++ b/test/multi-tag-operations.spec.ts @@ -0,0 +1,70 @@ +import * as fs from 'fs-extra'; +import { NgOpenApiGen } from '../lib/ng-openapi-gen'; +import { OpenAPIObject } from '../lib/openapi-typings'; +import { Options } from '../lib/options'; +import options from './multi-tag-operations.config.json'; +import spec from './multi-tag-operations.json'; + +const gen = new NgOpenApiGen(spec as OpenAPIObject, options as Options); +gen.generate(); + +/** + * Collects the exported names of every re-export in an index file, so that duplicates can be + * spotted. Handles both the plain form (`export { A }`) and the aliased form + * (`export { A as B }`, where `B` is the exported name). Types and values live in separate + * namespaces, so type exports are kept apart from value exports by a prefix. + */ +function exportedNames(content: string): string[] { + const names: string[] = []; + const regex = /^export (type )?\{([^}]+)}/gm; + let match: RegExpExecArray | null; + while ((match = regex.exec(content)) !== null) { + const kind = match[1] ? 'type' : 'value'; + for (const item of match[2].split(',')) { + const parts = item.trim().split(/\s+as\s+/); + names.push(`${kind}:${parts[parts.length - 1]}`); + } + } + return names; +} + +describe('Generation tests using multi-tag-operations.json', () => { + const functionsFile = fs.readFileSync(`${gen.outDir}/functions.ts`, 'utf8'); + const indexFile = fs.readFileSync(`${gen.outDir}/index.ts`, 'utf8'); + + for (const [name, content] of [['functions.ts', functionsFile], ['index.ts', indexFile]]) { + describe(name, () => { + it('should never export the same name twice', () => { + const names = exportedNames(content); + expect(names.length).toBeGreaterThan(0); + expect(names.length).toBe(new Set(names).size); + }); + + it('should export an operation with multiple tags only once', () => { + // The operation declares both `Appointment` and `Called by frontend`, yet it is a single + // operation: exporting it once per tag would be a duplicate identifier + const occurrences = content.match(/export \{ deleteAppointment as \S+ }/g); + expect(occurrences).toEqual(['export { deleteAppointment as deleteAppointment }']); + expect(content).toContain( + 'export type { DeleteAppointment$Params as DeleteAppointment$Params } from \'./fn/appointment/delete-appointment\''); + // No tag suffix, as there is nothing to disambiguate from + expect(content).not.toContain('deleteAppointmentAppointment'); + expect(content).not.toContain('deleteAppointmentCalledByFrontend'); + }); + }); + } + + it('should write the function of a multi-tagged operation under its first tag only', () => { + expect(fs.existsSync(`${gen.outDir}/fn/appointment/delete-appointment.ts`)).toBe(true); + expect(fs.existsSync(`${gen.outDir}/fn/called-by-frontend`)).toBe(false); + }); + + it('should generate a service per tag, both reusing the single function', () => { + const appointment = fs.readFileSync(`${gen.outDir}/services/appointment.service.ts`, 'utf8'); + const calledByFrontend = fs.readFileSync(`${gen.outDir}/services/called-by-frontend.service.ts`, 'utf8'); + for (const service of [appointment, calledByFrontend]) { + expect(service).toContain('import { deleteAppointment } from \'../fn/appointment/delete-appointment\';'); + expect(service).toContain('deleteAppointment('); + } + }); +});