Skip to content

Close #460 empty string enum #468

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dgreene1 merged 5 commits into from
Sep 18, 2019
Merged

Close #460 empty string enum #468

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
73bec89
Fix #465
Sep 10, 2019
fa24604
Merge branch 'master' into 460-empty-string-enum
Sep 10, 2019
0be788d
#460 Adding test for the validation part
Sep 10, 2019
8994a41
#460 Add validation test for empty string / 0 value enum
Sep 10, 2019
5d228b0
Add spec3 validation for enum schema.
Sep 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/metadataGeneration/typeResolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,8 @@ export class TypeResolver {

if (extractEnum) {
const enums = enumDeclaration.members.map((member: any, index) => {
return getEnumValue(member) || String(index);
Copy link
Collaborator

@dgreene1 dgreene1 Sep 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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,
}
  1. 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,
}
  1. 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:

3b) update the nnumber enum tests:

Copy link
Author

@fhewitt fhewitt Sep 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Collaborator

@WoH WoH Sep 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@fhewitt fhewitt Sep 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@dgreene1 dgreene1 Sep 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Author

@fhewitt fhewitt Sep 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@dgreene1 dgreene1 Sep 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)

Copy link
Author

@fhewitt fhewitt Sep 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const enumValue = getEnumValue(member);
return !!enumValue || enumValue === '' ? enumValue : String(index);
});
return {
dataType: 'refEnum',
Expand Down
2 changes: 1 addition & 1 deletion src/routeGeneration/templateHelpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ export class ValidationService {
const enumValue = members.find(member => {
return member === String(value);
});
if (!enumValue) {
if (enumValue === undefined) {
fieldErrors[parent + name] = {
message: `should be one of the following; ['${members.join(`', '`)}']`,
value,
Expand Down
2 changes: 2 additions & 0 deletions tests/fixtures/testModel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ export enum EnumIndexValue {
* EnumNumberValue.
*/
export enum EnumNumberValue {
VALUE_0 = 0,
VALUE_1 = 2,
VALUE_2 = 5,
}
Expand All @@ -128,6 +129,7 @@ export enum EnumNumberValue {
* EnumStringValue.
*/
export enum EnumStringValue {
EMPTY = '' as any,
VALUE_1 = 'VALUE_1' as any,
VALUE_2 = 'VALUE_2' as any,
}
Expand Down
8 changes: 8 additions & 0 deletions tests/unit/swagger/definitionsGeneration/definitions.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,10 @@ describe('Definition generation', () => {
expect(propertySchema.$ref).to.eq('#/definitions/EnumNumberValue', `for property ${propertyName}.$ref`);
expect(propertySchema['x-nullable']).to.eq(true, `for property ${propertyName}[x-nullable]`);
expect(propertySchema).to.not.haveOwnProperty('additionalProperties', `for property ${propertyName}`);

const validatedDefinition = getValidatedDefinition('EnumNumberValue', currentSpec);
const expectedEnumValues = ['0', '2', '5'];
expect(validatedDefinition.enum).to.eql(expectedEnumValues, `for property ${propertyName}[enum]`);
},
enumNumberArray: (propertyName, propertySchema) => {
expect(propertySchema.type).to.eq('array', `for property ${propertyName}.type`);
Expand All @@ -265,6 +269,10 @@ describe('Definition generation', () => {
expect(propertySchema).to.not.haveOwnProperty('additionalProperties', `for property ${propertyName}`);
expect(propertySchema.$ref).to.eq('#/definitions/EnumStringValue', `for property ${propertyName}.$ref`);
expect(propertySchema['x-nullable']).to.eq(true, `for property ${propertyName}[x-nullable]`);

const validatedDefinition = getValidatedDefinition('EnumStringValue', currentSpec);
const expectedEnumValues = ['', 'VALUE_1', 'VALUE_2'];
expect(validatedDefinition.enum).to.eql(expectedEnumValues, `for property ${propertyName}[enum]`);
},
enumStringArray: (propertyName, propertySchema) => {
expect(propertySchema.type).to.eq('array', `for property ${propertyName}.type`);
Expand Down
20 changes: 20 additions & 0 deletions tests/unit/swagger/schemaDetails3.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,20 @@ describe('Definition generation for OpenAPI 3.0.0', () => {
specName: 'specWithNoImplicitExtras',
};

const getComponentSchema = (name: string, chosenSpec: SpecAndName) => {
if (!chosenSpec.spec.components.schemas) {
throw new Error(`No schemas were generated for ${chosenSpec.specName}.`);
}

const schema = chosenSpec.spec.components.schemas[name];

if (!schema) {
throw new Error(`${name} should have been automatically generated in ${chosenSpec.specName}.`);
}

return schema;
};

/**
* This allows us to iterate over specs that have different options to ensure that certain behavior is consistent
*/
Expand Down Expand Up @@ -289,6 +303,9 @@ describe('Definition generation for OpenAPI 3.0.0', () => {
expect(propertySchema.$ref).to.eq('#/components/schemas/EnumNumberValue', `for property ${propertyName}.$ref`);
expect(propertySchema.nullable).to.eq(true, `for property ${propertyName}.nullable`);
expect(propertySchema).to.not.haveOwnProperty('additionalProperties', `for property ${propertyName}`);

const schema = getComponentSchema('EnumNumberValue', currentSpec);
expect(schema.enum).to.eql(['0', '2', '5']);
},
enumNumberArray: (propertyName, propertySchema) => {
expect(propertySchema.type).to.eq('array', `for property ${propertyName}.type`);
Expand All @@ -305,6 +322,9 @@ describe('Definition generation for OpenAPI 3.0.0', () => {
expect(propertySchema).to.not.haveOwnProperty('additionalProperties', `for property ${propertyName}`);
expect(propertySchema.$ref).to.eq('#/components/schemas/EnumStringValue', `for property ${propertyName}.$ref`);
expect(propertySchema.nullable).to.eq(true, `for property ${propertyName}.nullable`);

const schema = getComponentSchema('EnumStringValue', currentSpec);
expect(schema.enum).to.eql(['', 'VALUE_1', 'VALUE_2']);
},
enumStringArray: (propertyName, propertySchema) => {
expect(propertySchema.type).to.eq('array', `for property ${propertyName}.type`);
Expand Down
15 changes: 15 additions & 0 deletions tests/unit/swagger/templateHelpers.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,21 @@ describe('ValidationService', () => {
expect(result).to.equal(value);
});

it('should enum empty string value', () => {
const value = '';
const result = new ValidationService({}).validateEnum('name', value, {}, [''] as any);
expect(result).to.equal(value);
});

it('should enum null is not empty string value', () => {
const value = null;
const error: any = {};
const name = 'name';
const result = new ValidationService({}).validateEnum(name, value, error, [''] as any);
expect(result).to.equal(undefined);
expect(error[name].message).to.equal(`should be one of the following; ['']`);
});

it('should enum string value', () => {
const value = 'HELLO';
const result = new ValidationService({}).validateEnum('name', value, {}, ['HELLO'] as any);
Expand Down