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

(optional) null should be part of matchedData #568

Closed
melroy89 opened this issue Apr 18, 2018 · 12 comments · Fixed by #577
Closed

(optional) null should be part of matchedData #568

melroy89 opened this issue Apr 18, 2018 · 12 comments · Fixed by #577
Labels

Comments

@melroy89
Copy link

melroy89 commented Apr 18, 2018

Hi,

TLDR: matchedData() is not returning my body field with JSON value of null, while I try to validate the data with the check API.

I don't know if I use your validator incorrect or if it's a feature request.. but..
I use your validator for my backend app before sending the JSON object further to the database (eg. MariaDB).

I have a database field what either can be of type integer or have the value of NULL. So I tried to use your validator in this way (at least this is what I thought should work):

body('journal_bank_id').optional({nullable: true}).isInt()

However, I'm also using the filter matchedData() function in order to guarantee that my data I put in my database is fully validated first by your validator package 👍

object = matchedData(req)

Now, when I try to request an API call to the Express backend using your validator with the HTTP body:

{"journal_bank_id": null}

Expected situation: I would expect the JSON object is now validated correctly and the journal_bank_id with null value becomes part of the returned object from matchedData(). Meaning the MySQL query will set the corresponding field to NULL accordantly. So basically I would expect matchedData() returns:
{"journal_bank_id": null}

Actual situation: matchedData() is not returning journal_bank_id with value null at all. Meaning my database field can't be set to NULL because of your validator.

Do I miss something here? Should I maybe not use optional()? I couldn't find isNull().. I have no clue anymore..

Thanks in advance!

Kind regards,
Melroy van den Berg

@melroy89
Copy link
Author

Like previous version supported: .allow(null) if I'm not mistaken..

@gustavohenke
Copy link
Member

gustavohenke commented May 10, 2018

Thanks for reporting this, @Danger89. Will need to add an option to include optional data, otherwise could break people's applications.

@melroy89
Copy link
Author

Sure whatever is needed

@melroy89
Copy link
Author

melroy89 commented Jun 1, 2018

@gustavohenke
The interface says filterOptionals not includeOptionals. So what is it? Is this a typo or?

export interface MatchedDataOptions {
  filterOptionals: boolean;
  onlyValidData: boolean;
  locations: Location[];
}

@melroy89
Copy link
Author

melroy89 commented Jun 1, 2018

And waiting on a new release 👍

@gustavohenke
Copy link
Member

The interface is wrong

@JoseExposito
Copy link

First, thank you very much for the PR @gustavohenke

Any estimated release date? I'm also affetected by this issue. See #605

@gustavohenke
Copy link
Member

I have been unable to dedicate much time to this project lately, that's why I haven't released it yet.
I don't expect to add anything else to the already quite big log of changes of an eventual v5.3.0.

@JoseExposito
Copy link

Thank you very much for your answer and effort put in the project.

Looking forward for the next release and hopping you manage to spare some time. This kind of projects can be very time consuming, so let me know if you need help with the release.

@gustavohenke
Copy link
Member

Shipped on v5.3.0! 🚢

@GitStorageOne
Copy link
Contributor

GitStorageOne commented Mar 24, 2019

Current logic is not correct in my opinion.
ValidationSchema contain:

optional: {
  options: { nullable: true }
}

matchedData() removes fields with null value.
null is proper data type, that present in JSON, JS and Databases, and should not be dropped when nullable: true.

If we pass includeOptionals, we would be able to receive null values, but this adds undesired effect, because if datashape is not include optional field at all, matchedData will add it with undefined value.

Expected behavior is to pass null without includeOptionals option when nullable: true.
So, when client not specify optional field, we will not see it, as expected.
When client send null, we will get null as expected and database will accept that type.
That would be correct logic, but may break some user projects.

Current workaround is to set includeOptionals = true and manually delete all undefined fields which is not great.

@lock
Copy link

lock bot commented May 31, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators May 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants