Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Handle partially parsed objects (w/ querystrings as keys) #27

Open
wants to merge 2 commits into from

2 participants

@kainosnoema

tl;dr

  • { 'foo[bar]': { qux: 0 }, 'foo': { baz: 1 } } => { foo: { bar: { qux: 0 }, baz: 1 } }
  • previously this would work in some cases, this fix handles most cases properly
  • allows for handling of pre-parsed multipart forms from felixge/node-formidable
  • only added a few lines plus tests, all tests pass

Why?

For multipart forms, particularly from node-formidable. This particular use case is important:

We can use mimetypes to partially parse data from node-formidable parts, before using qs to reassemble

// Raw POST body:
//
// --xxxxxxxx
// Content-Disposition: form-data; name="foo"
// Content-Type: application/json
// 
// {"bar": "baz"}
// --xxxxxxxx
// Content-Disposition: form-data; name="foo[qux]"
// Content-Type: image/jpeg
// 
// (... image data ...)
// --xxxxxxxx--

// Simplified example:

> console.log(qs.parse({
>   foo: JSON.parse("{\"bar\": \"baz\"}")
> , 'foo[qux]': [object Object] // JPEG data in Formidable.File object
> }));
>
{
  foo: {
    bar: "baz",
    qux: [object Object] // JPEG data in Formidable.File object
  }
}

Does we really need this?

Yes. The RestKit iOS framework (among others) can send multipart forms containing both application/json and attachment (ie. image/jpeg) data. Properly merging the parts back into a single params object is crucial. The ExpressJS body-parser got us most of the way there—although we did have to override handlePart() so we could parse the JSON based on the mimetype.

The final piece of the puzzle is getting qs to reassemble the parts from the querystring paths as expected. As I mentioned in the commit message, qs was doing this properly in some cases, but not all (depending on part order from node-formidable, which is apparently non-deterministic). This fix should handle most cases as expected.

@kainosnoema kainosnoema Handle partially parsed objects (w/ querystrings as keys)
- previously this would work in some cases, this fix handles most cases properly
- { 'foo[bar]': { qux: 0 }, 'foo': { baz: 1 } } => { foo: { bar: { qux: 0 }, baz: 1 } }
- allows for handling of pre-parsed multipart forms from felixge/node-formidable
583a95b
@tj tj commented on the diff
lib/querystring.js
@@ -232,10 +224,13 @@ function stringifyObject(obj, prefix) {
function set(obj, key, val) {
var v = obj[key];
- if (undefined === v) {
+ if ('undefined' == typeof v) {
@tj Owner
tj added a note

why do we need this? where's v coming from that we need typeof

@tj Owner
tj added a note

oh it's right above haha, we dont need typeof then

Just consistency really, and because undefined can be redefined in Javascript, meaning the safest way to check for undefined is with typeof. It's a habit of mine...

EDIT: I knew this, but just tried it... blows my mind:

> typeof undefined
'undefined'
> {}['foo'] === undefined
true
> undefined = 'foo';
'foo'
> typeof undefined
'string'
> {}['foo'] === undefined
false

EDIT 2: Apparently this behavior was eliminated in newer versions of V8? The above was in Node v0.4.8, v0.6.2 doesn't allow it... thank goodness.

@tj Owner
tj added a note

ah gotcha, personally I avoid defensive programming whenever possible to delegate the issue. plus there are a ton of retarded things you can do in js, people just shouldn't do them haha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/querystring.js
((6 lines not shown))
obj[key] = val;
} else if (Array.isArray(v)) {
v.push(val);
+ } else if (('object' == typeof v) && ('[object Object]' == toString.call(val))) {
+ // partially parsed object
+ parseObject(val, v);

btw, putting this recursive call inside set() seems a very odd to me given that its not actually setting anything. The only reason is to keep things a little cleaner since you'd otherwise have to check and handle this case in both the parse() and merge() functions. That said, if you have a better idea I'll be glad to make changes.

@tj Owner
tj added a note

meh yeah it's, the whole lib is pretty obscure, it's a strange thing to parse, as long as we have strong test cases we should be fine

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

Ok, so I've found a weird edge case that I'd like to get your feedback on before you do anything with this:

Current behavior:

> console.log(qs.parse({ 'foo[items]': [], foo: { items: ['bar'] } }))
{ "foo": { "items": [ [ "bar" ] ] } }

> console.log(qs.parse({ foo: { items: ['bar'] }, 'foo[items]': [] }))
{ "foo": { "items": [ "bar", [] ] } }

Oddly enough, we actually ran into this situation with RestKit (starting to hate it). We were expecting a consistent output, regardless of order, but unfortunately there isn't one when you use the "rules" of querystrings: that repeated keys are pushed together into arrays. The output above actually makes sense given the rules—its just not what we expected or wanted.

One possible solution would be to use concat() instead of push() under the set() function, since with non-Array objects, concat() behaves similarly to push() (so we keep existing behavior for normal querystrings—all tests still pass). At least the result is consistent regardless of order, but again, this isn't standard querystring behavior:

Using concat() instead of push():

> console.log(qs.parse({ 'foo[items]': [], foo: { items: ['bar'] } }))
{ "foo": { "items": [ "bar" ] } }

> console.log(qs.parse({ foo: { items: ['bar'] }, 'foo[items]': [] }))
{ "foo": { "items": [ "bar" ] } }

To be honest, I don't really like either option—this whole thing seems really weird.

Thoughts?

@tj
Owner
tj commented

my vote is on the second I think

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 17, 2011
  1. @kainosnoema

    Handle partially parsed objects (w/ querystrings as keys)

    kainosnoema authored
    - previously this would work in some cases, this fix handles most cases properly
    - { 'foo[bar]': { qux: 0 }, 'foo': { baz: 1 } } => { foo: { bar: { qux: 0 }, baz: 1 } }
    - allows for handling of pre-parsed multipart forms from felixge/node-formidable
Commits on Dec 19, 2011
  1. @kainosnoema
This page is out of date. Refresh to see the latest.
Showing with 31 additions and 13 deletions.
  1. +8 −13 lib/querystring.js
  2. +23 −0 test/parse.js
View
21 lib/querystring.js
@@ -35,15 +35,7 @@ function parse(parts, parent, key, val) {
var part = parts.shift();
// end
if (!part) {
- if (Array.isArray(parent[key])) {
- parent[key].push(val);
- } else if ('object' == typeof parent[key]) {
- parent[key] = val;
- } else if ('undefined' == typeof parent[key]) {
- parent[key] = val;
- } else {
- parent[key] = [parent[key], val];
- }
+ set(parent, key, val);
// array
} else {
var obj = parent[key] = parent[key] || [];
@@ -95,8 +87,8 @@ function merge(parent, key, val){
* Parse the given obj.
*/
-function parseObject(obj){
- var ret = { base: {} };
+function parseObject(obj, base){
+ var ret = { base: base || {} };
Object.keys(obj).forEach(function(name){
merge(ret, name, obj[name]);
});
@@ -232,10 +224,13 @@ function stringifyObject(obj, prefix) {
function set(obj, key, val) {
var v = obj[key];
- if (undefined === v) {
+ if ('undefined' == typeof v) {
@tj Owner
tj added a note

why do we need this? where's v coming from that we need typeof

@tj Owner
tj added a note

oh it's right above haha, we dont need typeof then

Just consistency really, and because undefined can be redefined in Javascript, meaning the safest way to check for undefined is with typeof. It's a habit of mine...

EDIT: I knew this, but just tried it... blows my mind:

> typeof undefined
'undefined'
> {}['foo'] === undefined
true
> undefined = 'foo';
'foo'
> typeof undefined
'string'
> {}['foo'] === undefined
false

EDIT 2: Apparently this behavior was eliminated in newer versions of V8? The above was in Node v0.4.8, v0.6.2 doesn't allow it... thank goodness.

@tj Owner
tj added a note

ah gotcha, personally I avoid defensive programming whenever possible to delegate the issue. plus there are a ton of retarded things you can do in js, people just shouldn't do them haha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
obj[key] = val;
} else if (Array.isArray(v)) {
- v.push(val);
+ obj[key] = v.concat(val);
+ } else if (('object' == typeof v) && ('[object Object]' == toString.call(val))) {
+ // partially parsed object
+ parseObject(val, v);
} else {
obj[key] = [v, val];
}
View
23 test/parse.js
@@ -139,6 +139,29 @@ module.exports = {
'test malformed uri': function(){
qs.parse('{%:%}').should.eql({ '{%:%}': '' });
qs.parse('foo=%:%}').should.eql({ 'foo': '%:%}' });
+ },
+
+ 'test partially parsed objects': function(){
+ qs.parse({ 'foo[0]': 'bar', 'foo[1]': 'baz' })
+ .should.eql({ foo: ['bar', 'baz'] });
+
+ qs.parse({ 'foo[items]': [], foo: { items: ['bar'] } })
+ .should.eql({ foo: { items: ['bar'] } })
+
+ qs.parse({ foo: { items: ['bar'] }, 'foo[items]': [] })
+ .should.eql({ foo: { items: ['bar'] } })
+
+ qs.parse({ 'foo[base64]': 'RAWR', 'foo[64base]': 'RAWR' })
+ .should.eql({ foo: { base64: 'RAWR', '64base': 'RAWR' } });
+
+ qs.parse({ 'user[name][first]': ['tj', 'TJ'] })
+ .should.eql({ user: { name: { first: ['tj', 'TJ'] } } });
+
+ qs.parse({ 'user[name]': { first: 'tj' }, 'user': { name: { last: 'holowaychuk' } } })
+ .should.eql({ user: { name: { first: 'tj', last: 'holowaychuk' } } });
+
+ qs.parse({ 'user': { name: { last: 'holowaychuk' } }, 'user[name]': { first: 'tj' } })
+ .should.eql({ user: { name: { last: 'holowaychuk', first: 'tj' } } });
}
// 'test complex': function(){
Something went wrong with that request. Please try again.