Correctly return nullable data in matchedData

[AndrewBarba]

Description

Correctly return nullable data in matchedData: #1048

To-do list

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

[gustavohenke]

Reading the issue you reported, it seems you're mostly worried with the undefined values coming along?

It's a bit tough to find a way out without a breaking change.
One thing I'm thinking is to make includeOptionals accept a string value, e.g. ignoreUndefined (or something less wordy). So it carries everything but values that were really not set.

WDYT?

[gustavohenke]

This is a breaking change, and it also doesn't make much sense:
Why would you want a field that's been marked as optional to be included when its value matches such definition, and you specified requiredOnly: true?

[AndrewBarba]

Because right now requiredOnly actually does return optional data when the value is say an Int. So by your logic - that should actually not be returned as the Int was marked as optional and you passed requiredOnly. So I could argue the current logic is already breaking that contract. What my change does is allow nulls to come through when you specifically say you want to allow nulls ie: nullable: true. Nearly identical to saying you want to allow optional Ints and having those come back when requiredOnly is true

[gustavohenke]

Not quite. Perhaps the APIs are not very good here, but this behaviour shouldn't change IMO.

The value it has must match the definition of optional. That is, one of

• undefined (default behaviour)
• null or undefined (nullable: true)
• falsy (checkFalsy: true -- I don't like this name much tbh, it's express-validator v2 inheritance)

If your integer is optional but has value of 1, then it's considered as passed in the request.

[stenneepro]

@gustavohenke

I have the same problem.
I think it's more accurate to keep keys in both request body and validation schema in matchedData return value too.

But of course, it's a breaking change.
How about add another parameter to matchedData function to keep optional keys which exist in request body?

[stenneepro]

@gustavohenke

#1273

I added a use case here.
It's causing problem for partial update.

[AndrewBarba]

I think the overall issue we are having is there is no good way to support a type like:

type Body = {
  age: number | null
}

Right now in express-validator we would attempt to do:

check('age').optional({ nullable: true }).isInt()

But this isn't really describing what we want, and I guess my PR is abusing that trying to get this to work (and I think other people are also trying to get this same behavior to work). In reality we want an API like:

check('age').isInt({ nullable: true })

This exactly describes what we're looking for. To be honest I have no clue what the current optional({ nullable: true }) is even supposed to do. As is it doesn't seem to work. Optional values simply don't come back in matchedData even if they are set correctly (pass validation).

[AndrewBarba]

Here's best example of how the current behavior is broken. You are claiming I made a breaking change by returning the null value when includeOptionals: false (which is same as requiredOnly: true). But notice in the screenshot how the first example, foo, is in fact optional, and yet its returned in the data. In the second example, foo, is the exact same definition but this time set to null, and its not returned even though I clearly marked it as nullable.

[gustavohenke]

In the second example, foo, is the exact same definition but this time set to null, and its not returned even though I clearly marked it as nullable.

Default behaviour of matchedData() is to filter out invalid values -- null doesn't pass isInt

[fedeci]

Default behaviour of matchedData() is to filter out invalid values -- null doesn't pass isInt

I agree with you if isInt() is the only validator but in this case optional({ nullable: true }) is used too, so both an int or a null value are valid data. I find that #1275 is a valid solution that could solve the problem.