-
-
Notifications
You must be signed in to change notification settings - Fork 616
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
fix(default): not working when chained with .optional()
#1182
fix(default): not working when chained with .optional()
#1182
Conversation
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.
Since default()
now has an effect beyond just being a sanitizer, I think it should probably be in ContextHandler
, next to optional()
:
express-validator/src/chain/context-handler.ts
Lines 45 to 53 in 02b69d4
/** | |
* Marks the field(s) of the validation chain as optional. | |
* By default, only fields with an `undefined` value are considered optional and will be ignored | |
* when validating. | |
* | |
* @param options an object of options to customize the behavior of optional. | |
* @returns the current validation chain | |
*/ | |
optional(options?: Partial<Optional> | true): Chain; |
Then, when a context is having field instances selected, the default value is applied:
express-validator/src/context.ts
Lines 75 to 77 in 02b69d4
addFieldInstances(instances: FieldInstance[]) { | |
instances.forEach(instance => { | |
this.dataMap.set(getDataMapKey(instance.path, instance.location), { ...instance }); |
WDYT, @lpizzinidev?
@gustavohenke 👍 Ok, I will apply the changes. |
c83324a
to
ff7d232
Compare
@gustavohenke Let me know if I understood your request correctly. |
1d2fe1d
to
14a75de
Compare
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.
Hey @lpizzinidev, it just occurred to me that this is a breaking change.
Reason:
default
, as a sanitizer, can be placed at any point in the chain, and it only changes the result from that point onwards, which is pretty much the issue with #1057.
Example
Say that there's a requirement that a fruit value is provided, and it must be at least 5 chars long:
// A sanitizer that changes a fruit to `null` if it's value is `apple`
const sanitizer = (fruit) => fruit === 'apple' ? null : fruit;
check('fruit').customSanitizer(sanitizer).default('banana').isLength({ min: 5 })
check('fruit').default('banana').customSanitizer(sanitizer).isLength({ min: 5 })
In the first chain above, if you don't pass a value to fruit
or if you pass apple
, the result will be the same - and the validation will pass.
In the second example, if you pass a value of apple
, users will be puzzled as it's 5 chars in length, but it still fails -- because the default of banana
is applied too early.
I still think that your change is a good one, but it will have to be part of v7 (see #990).
There might be more changes needed here though -- but rest assured I can implement them later 😄
@gustavohenke |
14a75de
to
032dd5e
Compare
45d649b
to
ec46dad
Compare
fixes #1057