-
Notifications
You must be signed in to change notification settings - Fork 3
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.
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
feat(NotEmptyValidator): improving notEmpty validator to work with empty strings, arrays and objects #98
base: master
Are you sure you want to change the base?
Conversation
…defined, null, strings, arrays, objects and anything having a numeric length Signed-off-by: Victor Antonio Barzana Crespo <viticoinf@gmail.com>
Signed-off-by: Victor Antonio Barzana Crespo <viticoinf@gmail.com>
} | ||
|
||
function hasNumericLength(value: any) { | ||
return (Array.isArray(value) || typeof value === 'string') && typeof value.length === 'number'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method name is a bit misleading. Essentially it's a check that the value is an array or string. As both those allowed types have length
the typeof value.length === 'number'
check is redundant. Also it does not count for new String("")
(see isString
function). Maybe it would be more clear to state that this validator supports strings, arrays and objects, that they are not null, undefined or empty? The old implementation, i.e. the existence of length
property, is unclear in case of an object: is { length: 0 }
empty or not?
} | ||
} | ||
|
||
function isObject(value: any) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These methods (isObject and hasNumericLength) can be optimized by combining them directly to the NotEmptyValidator: there's no need to recheck if value is not null and isArray(value)
doesn't need to run twice. These methods are also only used in that validator.
| toString | | Converts primitive values to (primitive) strings. | | ||
| notNull | | Requires non-null and non-undefined value. | | ||
| nullOrUndefined | | Requires null or undefined value. | | ||
| notEmpty | | Requires non-null, non-undefined and not empty. Works with `string`, `null`, `undefined`, `arrays`, `objects` and anything having a numeric length. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it already states "non-null, non-undefined and not empty", I think it suffice to say that it works with strings, arrays and objects.
The "anything having a numeric length" is contradictory in itself: is object with one property length=0 empty or not? I'd simply remove that thing, even though it could be considered a major change, as there are better ways of checking that some property has > 0 value.
|
||
describe('notEmpty', () => { | ||
describe('valid cases', () => { | ||
test('valid string input', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These (both valid and invalid cases separately) can be combined using test.each
: https://vitest.dev/api/#test-each
await expectViolations({}, V.notEmpty(), defaultViolations.notEmpty(ROOT, {})); | ||
}); | ||
|
||
test('validates prototyped objects with inherited properties, not own ones', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really good that you test also inherited properties!
Description of Change
In this PR, we have enhanced the capabilities of our existing
notEmpty
validator within our validation framework. This update broadens the scope of the validator to include checks for empty values in arrays, objects, strings, and to detect undefined or null values.The purpose of this enhancement is to fortify our input validation processes by ensuring that a variety of input types are not just present but also hold meaningful content. The updated
notEmpty
validator is now more versatile, able to handle different types of inputs with the same stringent criteria.Use Case Implementation
An example of where this enhanced validator is now used is in the user password validation process. The
userValidator
utilizes the updatednotEmpty
validator to confirm that the password field is filled with valid and substantial content, reinforcing the security measures in our system.Code Example
console output:
Related Issue
Motivation and Context
We have been working on an important feature in our team that requires this validator or similar to be implemented. For example, when all properties of an object are optional, but at least one of them is required and we don't know which one is, it is important to have such validation option in place, and this validator solves the problem.
How Has This Been Tested?
Added a bunch of tests in the code to check that this works as expected.
Screenshots (if appropriate):