perf: skip validation on default value #1708
Conversation
performance improvement: if the provided value is the default value, no need to validate
|
Are these test correct? In both cases, set default value as string with type integer? maybe |
They should be, I think since we get them as strings Code, we probably never coerce them to the target data type when serializing the spec. |
cesco69
changed the title
cesco69
changed the title
| @@ -325,7 +325,7 @@ describe('ValidationService', () => { | |||
| describe('Param validate', () => { | |||
| it('should apply defaults for optional properties', () => { | |||
| const value = undefined; | |||
| const propertySchema: TsoaRoute.PropertySchema = { dataType: 'integer', default: '666', required: false, validators: {} }; | |||
| const propertySchema: TsoaRoute.PropertySchema = { dataType: 'integer', default: 666, required: false, validators: {} }; | |||
There was a problem hiding this comment.
Can you revert these changes? At least while we don't check/fix when creating the metadata for validation.
There was a problem hiding this comment.
If revert it, test fails... it's ok?
There was a problem hiding this comment.
The problem then is that this is the schema with the default being a string is what we would generate "in the real world".
There was a problem hiding this comment.
ah ok, so this PR can't work!
| @@ -53,7 +53,10 @@ export class ValidationService { | |||
| return value; | |||
| } | |||
| } | |||
|
|
|||
| // performance improvement: if the provided value is the default value, no need to validate | |||
| if( property.default === value ){ | |||
There was a problem hiding this comment.
| if( property.default === value ){ | |
| if(property.default === value){ |
|
Nitpicks, otherwise LGTM |
WoH
force-pushed
the
master
branch
2 times, most recently
from
30cc104 to
82b61ec
Compare
performance improvement: if the provided value is the default value, no need to validate