Skip to content

Nested field support for sanitize() #12

Closed
wants to merge 5 commits into from

7 participants

@SirensOfTitan

As per this request: #11

I needed the functionality and wrote a quick fix to add support.

@ctavan
Owner
ctavan commented Aug 3, 2012

Thanks for the pull request!

Could you please rebase against the current master? Also, I think for such a feature we should have a test case. Can you provide one?

@dhodgin
dhodgin commented Sep 14, 2012

I need this feature pretty bad. I'm willing to take the nested.js tests for assert and either add to them or make a nestedsanatize.js test case if you can provide some info on how to run the tests. I'm on windows and have a make environment but when i run 'make test' i just get:
$ make test
for F in test/.js; do echo "$F: "; node $F; done;
test/basic.js:
/bin/sh: node: command not found
test/mapped.js:
/bin/sh: node: command not found
test/nested.js:
/bin/sh: node: command not found
test/regex.js:
/bin/sh: node: command not found
make: *
* [test] Error 127

if i can get the tests to run id make one for this so it can land and i can use it.

@SirensOfTitan

I'll merge upstream and write a test for this this weekend (sorry, this was low priority). There was actually a small bug in my current commit that I discovered while using the module, so I'll include that fix in the merge as well.

@dhodgin
dhodgin commented Sep 14, 2012

ok if you do that I will test it in my app to give you a +1 for @ctavan to accept this patch

@ctavan
Owner
ctavan commented Sep 14, 2012

As I've said, I'm happy to merge the patch as soon as it is rebased against the current master and contains a test case.

@SirensOfTitan

Okay. The most recent commit has been re-based against the current master and includes a test case. That should do it.

@ctavan ctavan commented on the diff Sep 17, 2012
lib/express_validator.js
@@ -30,18 +30,40 @@ var validator = new Validator();
var expressValidator = function(req, res, next) {
req.updateParam = function(name, value) {
- // route params like /user/:id
- if (this.params && this.params.hasOwnProperty(name) &&
- undefined !== this.params[name]) {
- return this.params[name] = value;
- }
- // query string params
- if (undefined !== this.query[name]) {
- return this.query[name] = value;
- }
- // request body params via connect.bodyParser
- if (this.body && undefined !== this.body[name]) {
- return this.body[name] = value;
+ var updateList;
+
+ if (name.length > 1) {
@ctavan
Owner
ctavan added a note Sep 17, 2012

I'm a little bit concerned about this statement: Notice that also strings have a length property, so 'abc'.length === 3.

I suppose you want to check whether name is an array?

Then rather do

if (Array.isArray(name)) {
...

The input to updateParam (name) will always be an Array, actually. I duplicated some of your code in the validate function so that all fields are represented as arrays.

This code could be further generalized, but I figured it would sacrifice readability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ctavan ctavan and 1 other commented on an outdated diff Sep 17, 2012
lib/express_validator.js
- }
- // query string params
- if (undefined !== this.query[name]) {
- return this.query[name] = value;
- }
- // request body params via connect.bodyParser
- if (this.body && undefined !== this.body[name]) {
- return this.body[name] = value;
+ var updateList;
+
+ if (name.length > 1) {
+ if (typeof this.params[name[0]] !== 'undefined') {
+ updateList = this.params;
+ } else if (typeof this.body[name[0]] !== 'undefined') {
+ updateList = this.body;
+ }
@ctavan
Owner
ctavan added a note Sep 17, 2012

you are missing this.query here. Also I would prefer checking

if (this.params.hasOwnProperty(name[0])) {
...

I can certainly change this. But the reason I omit querystrings is because (as far as I know) there is no standard for arrays as querystrings.

@ctavan
Owner
ctavan added a note Sep 17, 2012

OK, that's a valid point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ctavan
Owner
ctavan commented Sep 17, 2012

Hi. Thanks for the patch. I still have some concerns though. Sorry that I'm currently not able to fix these myself.

@SirensOfTitan

Okay. I have updated the check on the params, body variables to mimic that of the original code. In addition, I have provided explanations for any decisions that may have looked odd.

@ctavan ctavan commented on the diff Jun 5, 2013
lib/express_validator.js
+ if (name.length > 1) {
+ if (this.params && this.params.hasOwnProperty(name[0])) {
+ updateList = this.params;
+ } else if (this.body && undefined !== this.body[name[0]]) {
+ updateList = this.body;
+ }
+
+ if (typeof updateList !== 'undefined') {
+ name.map(function(item, index) {
+ if (index == (name.length - 1)) {
+ return updateList[item] = value;
+ } else {
+ updateList = updateList[item];
+ }
+ });
+ }
@ctavan
Owner
ctavan added a note Jun 5, 2013

I wonder what this code-blcok does. @SirensOfTitan can you explain it to me?

Why do you use map when you're not using the result of the map. Wouldn't forEach be more appropriate?

Also what does updateList = updateList[item] do?

@SirensOfTitan
SirensOfTitan added a note Jun 7, 2013

It essentially does something very similar to line 48 of the current master. While that code block extracts the the value from params, this code block updates the value in params after that value was sanitized.

The reason map is used is wholly to mimic the current coding style of the project, insofar that the exact same thing is done on line on line 48 of the current master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tateman66

Should we expect a 0.4.2 any day now with #12 in it.

👍

@ctavan
Owner
ctavan commented Jun 7, 2013

@tateman66 this pull request is currently not in a mergeable state and I still don't fully understand what the code is intended to do. I'm sorry I won't be able to fix this myself but if you can improve the patch I'm very happy to merge the feature!

@tateman66

I'm kinda like you, don't have time to create only to consume the wizardry of others. :|

Trying to thing of a meaningful way to use the library to run sanitize on deep nested posted JSON objects as opposed to just properties on body.

Any ideas?

@arb
Collaborator
arb commented Jul 18, 2013

This PR is over a year old... at this point I suggest we close it.

@ctavan
Owner
ctavan commented Jul 19, 2013

@zero21xxx I agree. Feel free to close pull requests if you think it's reasonable.

@arb arb closed this Jul 19, 2013
@mkoryak
mkoryak commented Jul 21, 2013

Just came here to report this as a bug and found this.

Would you be interested in a new pull request if I were to fix this?

@arb
Collaborator
arb commented Jul 22, 2013

Now that I've been thinking about it, it almost seems like the sanitize aspect of this module should be spun out into it's own module and completely dropped from this one...

Any thoughts on this @ctavan?

@ctavan
Owner
ctavan commented Jul 23, 2013

@zero21xxx I guess that's something that should rather be decided in the node-validator module. IMO this module should just give access to all the features available in node-validator.

@arb
Collaborator
arb commented Jul 23, 2013

Agreed. Probably just include a "window" or something to give access directly to the node-validator.

@mkoryak
mkoryak commented Jul 23, 2013

So should I submit a pull request for this feature or are you thinking that sanitize should be used directly on a string via node-validator?

@arb
Collaborator
arb commented Jul 23, 2013

Check if the bug exists in the newer version of node-validator, and if so, make the change and PR there. I think that is the right way to go... @ctavan?

@mkoryak
mkoryak commented Jul 23, 2013

node-validator works only with strings. Nested request field support is wholly within purview of this lib

@maxbeatty maxbeatty referenced this pull request Nov 4, 2013
Closed

Sanitize nested data #58

@nickpoorman

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.