Close #460 empty string enum

[fhewitt]

All Submissions:

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?
• Have you written unit tests?
• Have you written unit tests that cover the negative cases (i.e.: if bad data is submitted, does the library respond properly)?
• This PR is associated with an existing issue?

Closing issues

This closes #460 by removing the confusion between falsy and incorrect value. On validation we check that the value in enum is found instead of thruty, and we construct the accept list of values correctly.

Allowing empty enum is close to optional enum parameter, but it enforce the user of the API to knowingly pass the empty value instead of simply ignoring it. Moreover, the lack of support is a bug.

Potential Problems With The Approach

Don't see one, but I have very little knowledge of the code base. At least, this didn't break any unit test.

Test plan

None added. Probably should as the code base seems to confuse falsy/invalid, but don't know how.

1. Adding VALUE_0 = '' as any to testModel.ts > EnumStringValue
2. Then? I though about checking propertySchema.enum in definition.spec.ts line 263, but the value is undefined.

[dgreene1]

I gave instruction on how to write the unit tests.

[dgreene1]

@fhewitt here's a step by step instruction on how to write a unit test for this:

1. add an enum member to

   tsoa/tests/fixtures/testModel.ts

   Line 131 in 13d856e

   VALUE_1 = 'VALUE_1' as any,

So when you're done, it should look like:

export enum EnumStringValue {
  EMPTY = '',
  VALUE_1 = 'VALUE_1' as any,
  VALUE_2 = 'VALUE_2' as any,
}

2. Do the same for the number enums so we know that this PR doesn't create a regression bug. So add a 0 case for EnumNumberValue here:

   tsoa/tests/fixtures/testModel.ts

   Line 123 in 13d856e

   VALUE_1 = 2,

export enum EnumNumberValue {
  VALUE_0 = 0,
  VALUE_1 = 2,
  VALUE_2 = 5,
}

3. Update the unit tests to check the actual data that is produced from the schema. I would recommend a chai assertion like:

const expectedEnumValues = ['', 'VALUE_1', 'VALUE_2'];
expect(propertySchema.enum).to.eq(true, `for property ${propertyName}[enum]`);

3a) update the string enum tests:

• for OpenAPI 3:

  tsoa/tests/unit/swagger/schemaDetails3.spec.ts

  Line 303 in e482d3c

  enumStringValue: (propertyName, propertySchema) => {

• for Swagger 2:

  tsoa/tests/unit/swagger/definitionsGeneration/definitions.spec.ts

  Line 248 in 13d856e

  expect(propertySchema.type).to.eq(undefined, `for property ${propertyName}.type`);

3b) update the nnumber enum tests:

• for OpenAPI 3:

  tsoa/tests/unit/swagger/schemaDetails3.spec.ts

  Line 288 in 13d856e

  expect(propertySchema.type).to.eq(undefined, `for property ${propertyName}.type`);

• for Swagger 2:

  tsoa/tests/unit/swagger/definitionsGeneration/definitions.spec.ts

  Line 264 in 13d856e

  expect(propertySchema.type).to.eq(undefined, `for property ${propertyName}.type`);

[fhewitt]

Ok, so I were close, but it fail:

expect(propertySchema.enum).to.eq(expectedEnumValues, `for property ${propertyName}[enum]`);

give

Definition generation for OpenAPI 3.0.0
       for specDefault
         should set additionalProperties to false if noImplicitAdditionalProperties is set to "throw-on-extras" (when there are no dictionary or any types)
           should produce a valid schema for the enumStringArray property on TestModel for the specDefault:
     **AssertionError: for property enumStringArray[enum]: expected undefined to equal [ '', 'VALUE_1', 'VALUE_2' ]**
      at Object.enumStringArray (tests\unit\swagger\schemaDetails3.spec.ts:315:44)
      at Context.<anonymous> (tests\unit\swagger\schemaDetails3.spec.ts:498:49)```

Notice that the added test fail also on the master branch.

The object receive:

{ '$ref': '#/components/schemas/EnumStringValue',
  description: undefined,
  format: undefined,
  nullable: true
}

Template helper use the property.enums value for the function validateEnum. Not sure why this isn't generated for the test...

[WoH]

Enums create a ref type.
@fhewitt you can use getValidatedDefinition('EnumStringValue', currentSpec); to get the definition and proceed to do the checks @dgreene1 mentioned on the definition [definitions.spec.ts]

For OAPI3 just check the reference gets created.

[fhewitt]

The reference check already seems to be there. So, the latest commit should cover the unit test requirement.

Thank you both for your much appreciated explanation and instructions.

[dgreene1]

You're still missing the tests for OpenAPI 3. It's unfortunate that there isn't a getValidatedDefinition in the OpenAPI 3 test file. But you can replicate it by checking the structure of specDefault.spec.components.schemas.EnumStringValue

Thank you for the additional tests and for submitting this PR. I'd really like to see coverage for OpenAPI 3 before we merge this PR since most people will probably be using OpenAPI 3 (since Swagger 2 doesn't support the majority of features).

[fhewitt]

At the same time, a lot of unit test over the validation are present in definitions.spec.ts but missing in OpenAPI 3. I strongly suspect that it's because the unit tests over validation are good for both cases / all OpenAPI 3 versions, and therefore not required to be duplicated. I can do it (but not today, I fear) if really required.

[dgreene1]

I don’t believe that the missing tests in the OpenAPI test file are intentional. I believe they are missing because OpenAPI 3 was only added to tsoa about 3 months ago. So the test coverage was minimal because it was new. You can even see that in the PR for that file where the original author was like “I hope this works?”

So yes it would be really appreciated if you add the tests. Thank you for your patients. I know that it seems unnecessary but it will really help us maintainers and contributes. :)

[fhewitt]

Sprint delivered, back here!

You seems right, I've remade an equivalent of getValidatedDefinition and it required enough changes that it's seems to require distinct testing.

Let me know if you see something else.