fix(checkSchema): check all locations if in is not specified.#1070
fix(checkSchema): check all locations if in is not specified.#1070hottomato-c wants to merge 1 commit into
checkSchema): check all locations if in is not specified.#1070Conversation
in is not specified.
in is not specified.checkSchema): check all locations if in is not specified.
|
Hey @nikli2009 can you add a few more tests to check if #1010 is fixed by this? |
hey man, gladly ! checking today |
Test added, could you help review and let me know if need to add more |
fedeci
left a comment
There was a problem hiding this comment.
Some nitty comments but look fine. Thanks!
/cc @gustavohenke patch or minor? It actually corrects a bug(?) but may break something🤔
|
Tests updated + commits squashed 🥪🥪 . |
gustavohenke
left a comment
There was a problem hiding this comment.
This is a breaking change.
For everybody using check() instead of the specialised functions, this can be a bit wild...
Test code:
const req = {};
check('foo').isNumeric().run(req).then(() => {
console.log(validationResult(req).array());
});Output:
[
{
value: undefined,
msg: 'Invalid value',
param: 'foo',
location: 'body'
},
{
value: undefined,
msg: 'Invalid value',
param: 'foo',
location: 'cookies'
},
{
value: undefined,
msg: 'Invalid value',
param: 'foo',
location: 'headers'
},
{
value: undefined,
msg: 'Invalid value',
param: 'foo',
location: 'params'
},
{
value: undefined,
msg: 'Invalid value',
param: 'foo',
location: 'query'
}
]Looking at #1071, it's conceptually wrong that you don't pass in to the schema, and expect a certain request location to have the value you wanted.
You should probably use matchedData() instead.
But then, a few things I'm wondering:
- would it makes sense to differentiate between "all/multiple locations" vs "any location", so to appease to those that want the convenience of
check? - should we verify if the user actually tried to pass the validated field somewhere -- e.g.
/some?queryvs/some-- and prefer handlingreq.queryinstead ofreq.body?
|
Thanks for taking time to review this PR and sharing all the detail of API design @gustavohenke
Totally agree 👍 , we can check query if query isn't empty. // when use check or checkSchema
check('anyKey').exists()
checkSchema({
'anyKey': {
exists: true
}
})
// req is simply `/helloworld`
{
body: {},
query: {},
params: {},
headers: { ... stuff },
cookies: { ...stuff }
}
// expected validation result
// Option 1: [ notInBody ]
// Option 2: [ notInBody, notInQuery, notInParams ]
// Option 3: [ notInBody, notInQuery, notInParams, notInHeaders, notInCookies ] |
3177f64 to
6e160b1
Compare
Description
When using
checkSchema, I notice that thedefaultoption doesn't work as expected when the field is inreq.queryandinis not specified. (ifinis empty, by default we should check all locations as official doc suggested hereRelated issues:
To-do list