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

Implicit exists() or not? #399

Closed
ijohn opened this issue Aug 30, 2017 · 5 comments
Closed

Implicit exists() or not? #399

ijohn opened this issue Aug 30, 2017 · 5 comments
Labels

Comments

@ijohn
Copy link

ijohn commented Aug 30, 2017

I am in the process of migrating my express-validator 3.2.1 app to the new v4 API (4.1.0). In order to make my question easier, I already setup a test app.

I am using Node.js 8.4.0 with npm 5.3.0 BTW.

Please run the following:

git clone git@github.com:ijohn/test-validator.git
cd test-validator
npm i
npm start

Test using curl:

curl http://localhost:3000/\?email\=one%40example.com

The following is the result:

{"email":"one@example.com"}

What I expected:

{"errors":{"name":{"location":"query","param":"name","msg":"must be ASCII"}}}

Question is, why does
query('email').isEmail().withMessage('must be valid') gets an implicit exists() while query('name').isAscii().withMessage('must be ASCII') doesn't?

@gustavohenke
Copy link
Member

This seems to be a bug, actually.

See the relevant lines in runner.js:
https://github.com/ctavan/express-validator/blob/8e3de682703b48d64629a5469b43309081b6bfe2/check/runner.js#L8-L10

Since validator only accepts strings, values passed to the standard validators get converted to string.
What means isAscii is being called with "undefined" (a string) internally; therefore, valid value.

This behaviour must be changed (perhaps use the same as v3), and possibly include a comprehensive integration test suite, testing each validator function 🛠 👨‍🏭

@ijohn
Copy link
Author

ijohn commented Aug 31, 2017

Ah! So the reason isEmail() seems to have implicit exists(), is because undefined is not a valid email address.

Thus for 4.1.0, we have to add exists() if it's not optional().

@gustavohenke
Copy link
Member

Yes, but please don't take this for granted. It's a bug.

@gustavohenke
Copy link
Member

Fix published in v4.1.1.

@lock
Copy link

lock bot commented Jun 1, 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 Jun 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants