Repository: lukeautry/tsoa (ts)

Issue:
Enums with strings and numbers should not be supported
<!--- Provide a general summary of the issue in the Title above -->

The goal is to be more strict in v3 so we can clarify to users why they're getting a bad swagger document when they used mixed enums (i.e. enums with strings and numbers).

See this comment for more information: https://github.com/lukeautry/tsoa/pull/493/files#r330696346

## Sorting

* **I'm submitting a ...**
  - [ ] bug report
  - [X] feature request
  - [ ] support request

* I confirm that I
  - [X] used the [search](https://github.com/lukeautry/tsoa/search?type=Issues) to make sure that a similar issue hasn't already been submit

## Expected Behavior
<!--- Tell us what should happen -->
<!--- If this is related to swagger/OpenAPI files, then please include swagger.yaml that you wish tsoa had generated  -->
<!--- If this is related to a TypeScript issue, then include the way that you expected to be able to write the code  -->
in the v2 branch, if you pass in an enum with strings and numbers then it tries to build a swagger object, but it shouldn't
```ts
export enum MixedStringAndNumberEnum {
  one = 1,
  two = 'two',
  three = 3,
  four = 'four',
}
```
Due to requirements to not create breaking changes within v2, we had no choice but to build some schema, but it's wrong:
```yaml
paths:
  /items:
    get:
      parameters:
        - in: query
          name: sort
          description: Sort order
          schema:
            type: string // <-- If swagger were to support it, this should be string | number
            enum: [1, two, 3, four]
```
Ideally it would throw an error.

## Possible Solution
<!--- Not obligatory, but suggest a fix/reason for the bug, -->
* Throw an error that clarifies that swagger/ OpenAPI does not support enums with strings and numbers

## Steps to Reproduce
<!--- Provide a link to a live example, or an unambiguous set of steps to -->
<!--- reproduce this bug. Include code to reproduce, if relevant -->
1. see the unit test labeled `"should warn if an enum is mixed with numbers and strings"`
2. that ^ is the unit test that should be changed to be `"should throw an error if an enum is mixed with numbers and strings"`

## Breaking change?
<!--- if this is a breaking change, then please describe how it breaks the existing libraries inputs/outputs -->
<!--- if this is a breaking change, then discuss how we can mitigate that -->
yes, that's why I'm adding it the the `V3` github project kanban board

Pull request:
Fix TS Enum generation
- Support mixed Enums in OpenAPI 3
- Throw when using mixed Enums in Swagger
- Deprecate enum initializer Expressions, will be ignored
- Change numeric enums to type 'number' instead of 'integer' !

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing](https://github.com/lukeautry/tsoa/tree/master/docs/CONTRIBUTING.md) document?
* [x] Have you checked to ensure there aren't other open [Pull Requests](https://github.com/lukeautry/tsoa/pulls) for the same update/change?
* [x] Have you adapted unit tests?
* [x] Have you adapted unit tests that cover the negative cases (i.e.: if bad data is submitted, does the library respond properly)?
* [x] This PR is associated with an existing issue?

**Closing issues**

Closes #501, #571

### If this is a new feature submission:

* [x] Has the issue had a maintainer respond to the issue and clarify that the feature is something that aligns with the [goals](https://github.com/lukeautry/tsoa#goal) and [philosophy](https://github.com/lukeautry/tsoa#philosophy) of the project?

**Potential Problems With The Approach**

Breaking change, needs to be documented

Patch:
diff --git a/src/metadataGeneration/typeResolver.ts b/src/metadataGeneration/typeResolver.ts
index 31dde95b..808b0b97 100644
--- a/src/metadataGeneration/typeResolver.ts
+++ b/src/metadataGeneration/typeResolver.ts
@@ -352,29 +352,12 @@ export class TypeResolver {
 
     const enumDeclaration = enumNodes[0] as ts.EnumDeclaration;
 
-    const typeChecker = this.current.typeChecker;
-    function getEnumValue(member: any) {
-      const constantValue = typeChecker.getConstantValue(member);
-      if (constantValue != null) {
-        return constantValue;
-      }
-      const initializer = member.initializer;
-      if (initializer) {
-        if (initializer.expression) {
-          return initializer.expression.text;
-        }
-        return initializer.text;
-      }
-      return;
-    }
+    const isNotUndefined = <T>(item: T): item is Exclude<T, undefined> => {
+      return item === undefined ? false : true;
+    };
+
+    const enums = enumDeclaration.members.map(this.current.typeChecker.getConstantValue).filter(isNotUndefined);
 
-    const enums = enumDeclaration.members.map((member: any, index) => {
-      const enumValue = getEnumValue(member);
-      if (enumValue !== 0 && enumValue !== '' && !enumValue) {
-        return String(index);
-      }
-      return enumValue;
-    });
     return {
       dataType: 'refEnum',
       description: this.getNodeDescription(enumDeclaration),
diff --git a/src/swagger/specGenerator.ts b/src/swagger/specGenerator.ts
index 3d98f446..6f5e26b0 100644
--- a/src/swagger/specGenerator.ts
+++ b/src/swagger/specGenerator.ts
@@ -191,7 +191,7 @@ export abstract class SpecGenerator {
     };
   }
 
-  private determineTypesUsedInEnum(anEnum: Array<string | number>) {
+  protected determineTypesUsedInEnum(anEnum: Array<string | number>) {
     const typesUsedInEnum = anEnum.reduce((theSet, curr) => {
       const typeUsed = typeof curr;
       theSet.add(typeUsed);
@@ -201,35 +201,6 @@ export abstract class SpecGenerator {
     return typesUsedInEnum;
   }
 
-  protected decideEnumType(anEnum: Array<string | number>, nameOfEnum: string): 'string' | 'integer' {
-    const typesUsedInEnum = this.determineTypesUsedInEnum(anEnum);
-
-    const badEnumErrorMessage = () => {
-      const valuesDelimited = Array.from(typesUsedInEnum).join(',');
-      return `Enums can only have string or number values, but enum ${nameOfEnum} had ${valuesDelimited}`;
-    };
-
-    let enumTypeForSwagger: 'string' | 'integer';
-    if (typesUsedInEnum.has('string') && typesUsedInEnum.has('number')) {
-      if (typesUsedInEnum.size !== 2) {
-        throw new Error(badEnumErrorMessage());
-      }
-      // TODO: It would be great to throw here, but without knowing how our users have been using tsoa, we can't make a breaking change until version 3.0
-      // tslint:disable-next-line: no-console
-      console.warn(`Swagger/OpenAPI does not support enums with both strings and numbers but found that for enum ${nameOfEnum}`);
-      const weHaveToPickOneSinceWeCantThrow = 'string';
-      enumTypeForSwagger = weHaveToPickOneSinceWeCantThrow;
-      // End of TODO
-    } else if (typesUsedInEnum.has('string') && typesUsedInEnum.size === 1) {
-      enumTypeForSwagger = 'string';
-    } else if (typesUsedInEnum.has('number') && typesUsedInEnum.size === 1) {
-      enumTypeForSwagger = 'integer';
-    } else {
-      throw new Error(badEnumErrorMessage());
-    }
-    return enumTypeForSwagger;
-  }
-
   protected getSwaggerTypeForEnumType(enumType: Tsoa.EnumType): Swagger.Schema {
     return { type: 'string', enum: enumType.enums.map(member => String(member)) };
   }
diff --git a/src/swagger/specGenerator2.ts b/src/swagger/specGenerator2.ts
index a56e9faf..69a7a6d8 100644
--- a/src/swagger/specGenerator2.ts
+++ b/src/swagger/specGenerator2.ts
@@ -372,4 +372,23 @@ export class SpecGenerator2 extends SpecGenerator {
   protected getSwaggerTypeForReferenceType(referenceType: Tsoa.ReferenceType): Swagger.BaseSchema {
     return { $ref: `#/definitions/${referenceType.refName}` };
   }
+
+  private decideEnumType(anEnum: Array<string | number>, nameOfEnum: string): 'string' | 'number' {
+    const typesUsedInEnum = this.determineTypesUsedInEnum(anEnum);
+
+    const badEnumErrorMessage = () => {
+      const valuesDelimited = Array.from(typesUsedInEnum).join(',');
+      return `Enums can only have string or number values, but enum ${nameOfEnum} had ${valuesDelimited}`;
+    };
+
+    let enumTypeForSwagger: 'string' | 'number';
+    if (typesUsedInEnum.has('string') && typesUsedInEnum.size === 1) {
+      enumTypeForSwagger = 'string';
+    } else if (typesUsedInEnum.has('number') && typesUsedInEnum.size === 1) {
+      enumTypeForSwagger = 'number';
+    } else {
+      throw new Error(badEnumErrorMessage());
+    }
+    return enumTypeForSwagger;
+  }
 }
diff --git a/src/swagger/specGenerator3.ts b/src/swagger/specGenerator3.ts
index f797dd32..f7078400 100644
--- a/src/swagger/specGenerator3.ts
+++ b/src/swagger/specGenerator3.ts
@@ -126,7 +126,7 @@ export class SpecGenerator3 extends SpecGenerator {
   }
 
   private buildSchema() {
-    const schema: { [name: string]: Swagger.Schema } = {};
+    const schema: { [name: string]: Swagger.Schema3 } = {};
     Object.keys(this.metadata.referenceTypeMap).map(typeName => {
       const referenceType = this.metadata.referenceTypeMap[typeName];
 
@@ -151,11 +151,29 @@ export class SpecGenerator3 extends SpecGenerator {
           schema[referenceType.refName].example = referenceType.example;
         }
       } else if (referenceType.dataType === 'refEnum') {
-        schema[referenceType.refName] = {
-          description: referenceType.description,
-          enum: referenceType.enums,
-          type: this.decideEnumType(referenceType.enums, referenceType.refName),
-        };
+        const enumTypes = this.determineTypesUsedInEnum(referenceType.enums);
+
+        if (enumTypes.size === 1) {
+          schema[referenceType.refName] = {
+            description: referenceType.description,
+            enum: referenceType.enums,
+            type: enumTypes.has('string') ? 'string' : 'number',
+          };
+        } else {
+          schema[referenceType.refName] = {
+            description: referenceType.description,
+            oneOf: [
+              {
+                type: 'number',
+                enum: referenceType.enums.filter(e => typeof e === 'number'),
+              },
+              {
+                type: 'string',
+                enum: referenceType.enums.filter(e => typeof e === 'string'),
+              },
+            ],
+          };
+        }
       } else if (referenceType.dataType === 'refAlias') {
         const swaggerType = this.getSwaggerType(referenceType.type);
         const validators = Object.keys(referenceType.validators)
diff --git a/src/swagger/swagger.ts b/src/swagger/swagger.ts
index 83d442f1..2bfc4b89 100644
--- a/src/swagger/swagger.ts
+++ b/src/swagger/swagger.ts
@@ -43,7 +43,7 @@ export namespace Swagger {
     parameters?: { [name: string]: Parameter };
     requestBodies?: { [name: string]: any };
     responses?: { [name: string]: Response };
-    schemas?: { [name: string]: Schema };
+    schemas?: { [name: string]: Schema | Schema3 };
     securitySchemes?: { [name: string]: Security };
   }
 
@@ -225,7 +225,8 @@ export namespace Swagger {
     items?: BaseSchema;
   }
 
-  export interface Schema3 extends Schema {
+  export interface Schema3 extends Omit<Schema, 'type'> {
+    type?: DataType;
     nullable?: boolean;
     oneOf?: BaseSchema[];
     allOf?: BaseSchema[];


Test patch:
diff --git a/tests/fixtures/testModel.ts b/tests/fixtures/testModel.ts
index d8c8b730..dd2e30df 100644
--- a/tests/fixtures/testModel.ts
+++ b/tests/fixtures/testModel.ts
@@ -212,9 +212,9 @@ export enum EnumStringNumberValue {
  * EnumStringValue.
  */
 export enum EnumStringValue {
-  EMPTY = '' as any,
-  VALUE_1 = 'VALUE_1' as any,
-  VALUE_2 = 'VALUE_2' as any,
+  EMPTY = '',
+  VALUE_1 = 'VALUE_1',
+  VALUE_2 = 'VALUE_2',
 }
 
 // shortened from StringLiteral to make the tslint enforced
@@ -480,8 +480,8 @@ export interface ValidateMapStringToAny {
  * Gender msg
  */
 export enum Gender {
-  MALE = 'MALE' as any,
-  FEMALE = 'FEMALE' as any,
+  MALE = 'MALE',
+  FEMALE = 'FEMALE',
 }
 
 export interface ErrorResponseModel {
diff --git a/tests/unit/swagger/definitionsGeneration/definitions.spec.ts b/tests/unit/swagger/definitionsGeneration/definitions.spec.ts
index b241fbff..3511bc00 100644
--- a/tests/unit/swagger/definitionsGeneration/definitions.spec.ts
+++ b/tests/unit/swagger/definitionsGeneration/definitions.spec.ts
@@ -251,7 +251,7 @@ describe('Definition generation', () => {
             expect(propertySchema).to.not.haveOwnProperty('additionalProperties', `for property ${propertyName}`);
 
             const validatedDefinition = getValidatedDefinition('EnumNumberValue', currentSpec);
-            expect(validatedDefinition.type).to.eq('integer');
+            expect(validatedDefinition.type).to.eq('number');
             const expectedEnumValues = [0, 2, 5];
             expect(validatedDefinition.enum).to.eql(expectedEnumValues, `for property ${propertyName}[enum]`);
           },
diff --git a/tests/unit/swagger/schemaDetails.spec.ts b/tests/unit/swagger/schemaDetails.spec.ts
index dacc6ba1..7db95603 100644
--- a/tests/unit/swagger/schemaDetails.spec.ts
+++ b/tests/unit/swagger/schemaDetails.spec.ts
@@ -5,6 +5,8 @@ import { expect } from 'chai';
 import { MetadataGenerator } from '../../../src/metadataGeneration/metadataGenerator';
 import { SpecGenerator2 } from '../../../src/swagger/specGenerator2';
 import { getDefaultOptions } from '../../fixtures/defaultOptions';
+import { Tsoa } from '../../../src/metadataGeneration/tsoa';
+import { SwaggerConfig } from '../../../src';
 
 describe('Schema details generation', () => {
   const metadata = new MetadataGenerator('./tests/fixtures/controllers/getController.ts').Generate();
@@ -72,4 +74,55 @@ describe('Schema details generation', () => {
       });
     });
   });
+
+  describe('illegal input values', () => {
+    it('should not allow an enum that has anything other than string | number', () => {
+      // Arrange
+      const schemaName = 'tooManyTypesEnum';
+      const metadataForEnums: Tsoa.Metadata = {
+        controllers: [],
+        referenceTypeMap: {
+          [schemaName]: {
+            refName: schemaName,
+            dataType: 'refEnum',
+            enums: [1, 'two', 3, 'four', ({} as unknown) as number],
+          },
+        },
+      };
+      const swaggerConfig: SwaggerConfig = {
+        outputDirectory: 'mockOutputDirectory',
+        entryFile: 'mockEntryFile',
+      };
+
+      // Act
+      let errToTest: Error | null = null;
+      try {
+        new SpecGenerator2(metadataForEnums, swaggerConfig).GetSpec();
+      } catch (err) {
+        errToTest = err;
+      }
+
+      // Assert
+      expect(errToTest!.message).to.eq(`Enums can only have string or number values, but enum ${schemaName} had number,string,object`);
+    });
+
+    it('should throw if an enum is mixed with numbers and strings', () => {
+      const swaggerConfig: SwaggerConfig = {
+        outputDirectory: 'mockOutputDirectory',
+        entryFile: 'mockEntryFile',
+      };
+      const mixedEnumMetadata = new MetadataGenerator('./tests/fixtures/controllers/mixedEnumController.ts').Generate();
+
+      // Act
+      let errToTest: Error | null = null;
+      try {
+        new SpecGenerator2(mixedEnumMetadata, swaggerConfig).GetSpec();
+      } catch (err) {
+        errToTest = err;
+      }
+
+      // Assert
+      expect(errToTest!.message).to.eq(`Enums can only have string or number values, but enum MixedStringAndNumberEnum had number,string`);
+    });
+  });
 });
diff --git a/tests/unit/swagger/schemaDetails3.spec.ts b/tests/unit/swagger/schemaDetails3.spec.ts
index b542954e..9c2ef89a 100644
--- a/tests/unit/swagger/schemaDetails3.spec.ts
+++ b/tests/unit/swagger/schemaDetails3.spec.ts
@@ -1,15 +1,12 @@
 import { expect } from 'chai';
 import 'mocha';
-import sinon = require('sinon');
 import { SwaggerConfig } from '../../../src/config';
 import { MetadataGenerator } from '../../../src/metadataGeneration/metadataGenerator';
-import { Tsoa } from '../../../src/metadataGeneration/tsoa';
 import { SpecGenerator3 } from '../../../src/swagger/specGenerator3';
 import { Swagger } from '../../../src/swagger/swagger';
 import { getDefaultOptions } from '../../fixtures/defaultOptions';
 import { TestModel } from '../../fixtures/duplicateTestModel';
-
-import assert = require('assert');
+import { Tsoa } from '../../../src/metadataGeneration/tsoa';
 
 describe('Definition generation for OpenAPI 3.0.0', () => {
   const metadata = new MetadataGenerator('./tests/fixtures/controllers/getController.ts').Generate();
@@ -445,7 +442,7 @@ describe('Definition generation for OpenAPI 3.0.0', () => {
             expect(propertySchema).to.not.haveOwnProperty('additionalProperties', `for property ${propertyName}`);
 
             const schema = getComponentSchema('EnumNumberValue', currentSpec);
-            expect(schema.type).to.eq('integer');
+            expect(schema.type).to.eq('number');
             expect(schema.enum).to.eql([0, 2, 5]);
           },
           enumStringNumberValue: (propertyName, propertySchema) => {
@@ -1059,8 +1056,8 @@ describe('Definition generation for OpenAPI 3.0.0', () => {
     });
   });
 
-  describe('illegal input values', () => {
-    it('should not allow an enum that has anything other than string | number', () => {
+  describe('Mixed Enums', () => {
+    it('should combine to metaschema', () => {
       // Arrange
       const schemaName = 'tooManyTypesEnum';
       const metadataForEnums: Tsoa.Metadata = {
@@ -1069,7 +1066,7 @@ describe('Definition generation for OpenAPI 3.0.0', () => {
           [schemaName]: {
             refName: schemaName,
             dataType: 'refEnum',
-            enums: [1, 'two', 3, 'four', ({} as unknown) as number],
+            enums: [1, 'two', 3, 'four'],
           },
         },
       };
@@ -1079,27 +1076,13 @@ describe('Definition generation for OpenAPI 3.0.0', () => {
       };
 
       // Act
-      let errToTest: Error | null = null;
-      try {
-        new SpecGenerator3(metadataForEnums, swaggerConfig).GetSpec();
-      } catch (err) {
-        errToTest = err;
-      }
-
-      // Assert
-      expect(errToTest!.message).to.eq(`Enums can only have string or number values, but enum ${schemaName} had number,string,object`);
-    });
-
-    it('should warn if an enum is mixed with numbers and strings', () => {
-      // Arrange
-      const spyOfConsoleWarn = sinon.spy(console, 'warn');
-      const mixedEnumMetadata = new MetadataGenerator('./tests/fixtures/controllers/mixedEnumController.ts').Generate();
-
-      // Act
-      new SpecGenerator3(mixedEnumMetadata, defaultOptions).GetSpec();
+      const spec = new SpecGenerator3(metadataForEnums, swaggerConfig).GetSpec();
 
       // Assert
-      assert(spyOfConsoleWarn.calledWithExactly('Swagger/OpenAPI does not support enums with both strings and numbers but found that for enum MixedStringAndNumberEnum'));
+      expect(getComponentSchema(schemaName, { specName: 'specDefault', spec })).to.deep.eq({
+        description: undefined,
+        oneOf: [{ type: 'number', enum: [1, 3] }, { type: 'string', enum: ['two', 'four'] }],
+      });
     });
   });
 });