[R-392] fix: Fix duplicated identifiers in functions.ts and index.ts (#392) Co-authored-by: Claude Code

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<string, number>();
       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<string>();
+      const writtenFiles = new Set<string>();
       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<string, OperationVariant>();
+    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`);

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;
   }
 }

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",

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/);
+  });
 });

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
+}

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"
+          }
+        }
+      }
+    }
+  }
+}

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(');
+    }
+  });
+});