tj / node-querystring

querystring parser for node and the browser - supporting nesting (used by Express, Connect, etc)
MIT License
455 stars 66 forks source link

Unifies multipart, json, and form-urlencoded in the bodyParser() middleware #24

Closed jackyz closed 12 years ago

jackyz commented 12 years ago

http://tjholowaychuk.com/post/12943975936/connect-1-8-0-multipart-support

Unifies multipart, json, and x-www-form-urlencoded parsing in the bodyParser() middleware is exactlly what I want. But something is missing, still.

for example:

$ curl http://127.0.0.1:3000/test.form -F content[1]=@test/clear.gif -F content[0]="text0" -F content[2]="test2" -F content[]="test3"
 {"content":["text0",null,"test2","test3"],"content[1]":{...}}

But it should be (keep same as x-www-form-urlencoded parsing does)

$ curl http://127.0.0.1:3000/test.form -F content[1]=@test/clear.gif -F content[0]="text0" -F content[2]="test2" -F content[]="test3"
 {"content":["text0",{...},"test2","test3"]}

After a half-day hacking in the code. I found the nest-name parse was in the qs module. It shoud been reuse in multipart parsing process as well.

so I refactor querystring.js(just move out functions etc.), make qs.parse can accept an object as input param.

After that, the bodyParser() middleware could reuse the nest-name parse logic in multipart, just like this:

/**
 * Parse multipart/form-data.
 *
 * TODO: make multiple support optional
 * TODO: revisit "error" flag if it's a formidable bug
 */
exports.parse['multipart/form-data'] = function(req, options, fn){
  var form = new formidable.IncomingForm
    , data = {}
    , done;

  Object.keys(options).forEach(function(key){
    form[key] = options[key];
  });

  function collect(name, val){
    if (Array.isArray(data[name])) {
      data[name].push(val);
    } else if (data[name]) {
      data[name] = [data[name], val];
    } else {
      data[name] = val;
    }
  }

  form.on('field', collect);

  form.on('file', collect);

  form.on('error', function(err){
    fn(err);
    done = true;
  });

  form.on('end', function(){
    if (done) return;
    try {
      req.body = qs.parse(data); // using the same nest-name mapping logic
      fn();
    } catch (err) {
      fn(err);
    }
  });

  form.parse(req);
};

BTW. I cannot find the latest version of bodyParser.js in github, so I just paste the code here.

tj commented 12 years ago

seems like it breaks the tests

jackyz commented 12 years ago

I missed one line, fixed, the test will happy now.

gasi commented 12 years ago

I’d love for that to be improved as well :)

gasi commented 12 years ago

@jackyz: Here’s bodyParser on GitHub: https://github.com/senchalabs/connect/blob/master/lib/middleware/bodyParser.js

jackyz commented 12 years ago

@gasi: That isn't the latest version that Connect 1.8.0 use. You can use npm install connect@1.8.0 to get v1.8.0, and check the bodyParser.js in node_modules/connect/lib/middleware dir.

tj commented 12 years ago

there's a 1.x branch

tj commented 12 years ago

no worries though if the tests pass now i can integrate that into master and backport

tj commented 12 years ago

this lib is becoming a mess haha we'll have to document it better sometime but hey it works for now so i'll merge this

gasi commented 12 years ago

YES! You two are awesome, thanks :) You accepted this pull request in the same time I wrote a workaround ;) Please ping this thread once you’ve published the new qs on npm, thanks.

tj commented 12 years ago

already done :) just going to integrate with connect and update tests etc

gasi commented 12 years ago

Tried the new multipart parsing in bodyParser and I love it. Unfortunately, existing code directly based on formidable’s IncomingForm parsing breaks since the multipart body has already been consumed by the new bodyParser. Nothing you can do about it but might be useful to document as part of the new express release.