-
-
Notifications
You must be signed in to change notification settings - Fork 616
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: validate entire array and not single elements #1280
base: fedeci/fix-property-selection
Are you sure you want to change the base?
Conversation
…dcard is not used
Hi,
FYI: even the AI (ChatGPT) understood that a standard behavior should not include the use of that workaround on all fields, |
I know this is a problem and I want to solve it as fast as possible, on a minor version possibly, even if that requires a breaking change. p.s. ChatGPT is trained on data of |
Hi, I discovered this problem by accident in 2019, after I had mistakenly duplicated the parameters in the query string. Then I realized that the issue was even more widespread, and obviously it crashed the entire logic after the validation. If this was a wanted behavior, it is not obvious to me, nor to the AI. |
@@ -34,7 +34,8 @@ | |||
"prepublishOnly": "tsc", | |||
"postpublish": "npm run docs:publish", | |||
"test": "jest", | |||
"lint": "eslint --ignore-path .gitignore 'src/**/*.ts' && prettier -c ." | |||
"lint": "eslint --ignore-path .gitignore 'src/**/*.ts' && prettier -c .", | |||
"prettier": "prettier -w ." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit 🤓
"prettier": "prettier -w ." | |
"lint:fix": "prettier -w ." |
Then the equivalent command from eslint can be added too, if you like.
@@ -183,4 +183,19 @@ describe('High level tests', () => { | |||
const result = await body('foo.*.nop').exists().run(req); | |||
expect(result.isEmpty()).toEqual(true); | |||
}); | |||
it('should error array if no wildcard is used', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar feeling to #1279 (comment).
Description
To be merged after #1279
Closes #1230
Closes #1243
Related #755 (comment)
Maybe it is not the best time for doing this since we just released v7, but I changed my mind about forcing the entire array to comply with the validator.
We provide wildcards which are a great way to validate individual array items.
This PR is born because there are a lot of cases where validating individual items if not requested by the user leads to unexpected results.
Otherwise we can: add a param to body(''), check('')... to explicitly enable/disable array, or keep the current approach.
@gustavohenke wdyt?
To-do list