Skip to content
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

Conversation

lpizzinidev
Copy link

fixes #1057

  • I have added tests for what I changed.
  • This pull request is ready to merge.

@coveralls
Copy link

coveralls commented Oct 15, 2022

Coverage Status

Coverage: 100.0%. Remained the same when pulling 032dd5e on lpizzinidev:fix-default-optional into f50a1b4 on express-validator:v7-features.

Copy link
Member

@gustavohenke gustavohenke left a 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():

/**
* 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:

addFieldInstances(instances: FieldInstance[]) {
instances.forEach(instance => {
this.dataMap.set(getDataMapKey(instance.path, instance.location), { ...instance });

WDYT, @lpizzinidev?

@gustavohenke gustavohenke self-requested a review November 6, 2022 09:58
@lpizzinidev
Copy link
Author

@gustavohenke 👍 Ok, I will apply the changes.

@lpizzinidev
Copy link
Author

@gustavohenke Let me know if I understood your request correctly.

@gustavohenke gustavohenke self-requested a review November 26, 2022 05:28
Copy link
Member

@gustavohenke gustavohenke left a 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 gustavohenke added this to the v7.0.0 milestone Dec 29, 2022
@lpizzinidev
Copy link
Author

@gustavohenke
No problem! Let me know if further changes are needed 👍

@gustavohenke gustavohenke changed the base branch from master to v7-features March 11, 2023 06:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.default() sanitizer not working when chained with .optional()
4 participants