ljharb / qs

A querystring parser with nesting support
BSD 3-Clause "New" or "Revised" License
8.58k stars 731 forks source link

Cannot parse array of objects using arrayFormat = 'brackets' #215

Open franciscolourenco opened 7 years ago

franciscolourenco commented 7 years ago

The README states:

You can also create arrays of objects:

var arraysOfObjects = qs.parse('a[][b]=c');
assert.deepEqual(arraysOfObjects, { a: [{ b: 'c' }] });

However when you try to parse an array with 2 objects, an object with 2 keys is created instead:

var arraysOfObjects = qs.parse('a[][b]=c&a[][d]=e');
assert.deepEqual(arraysOfObjects, { a: [{ b: 'c' , d: 'e'}] });

This also happens with arrayFormat = 'repeat'.

If arrays without indices cannot be supported, may the allowDots option could affect arrays instead: (a.0.b=c&a.1.d=e)

ryan-sherpa commented 7 years ago

Any update here?

ryan-sherpa commented 7 years ago

@aristidesfl I hacked together a fix in a forked version

ljharb commented 7 years ago

@ryan-sherpa no update, but if you have a fix, a PR would be very welcome!

franciscolourenco commented 7 years ago

@ryan-sherpa did you add support for list of object without indices? Does is support objects with different keys? How does it work?

EDIT: According to my current understanding a[][b]=c&a[][c]=d notation cannot be reliably parsed for objects with different keys and should be replaced in the docs with a[0][b]=c&a[1][c]=d

craigbeck commented 7 years ago

just found this myself... using arraFormat: 'brackets' with an array of objects can't be round-tripped

with option specified it fails:

> qs.parse(qs.stringify({someKeys:[{name:'foo',value:'34'},{name:'bar',value:'98798'}]}, {arrayFormat:'brackets'}), {arrayFormat:'brackets'});
{ someKeys: [ { name: [Array], value: [Array] } ] }

Using default option it round-trips correctly:

> qs.parse(qs.stringify({someKeys:[{name:'foo',value:'34'},{name:'bar',value:'98798'}]}, {}), {});
{ someKeys: 
   [ { name: 'foo', value: '34' },
     { name: 'bar', value: '98798' } ] }
ljharb commented 7 years ago

@craigbeck parse does not currently support an arrayFormat option. Perhaps it should.

jchook commented 6 years ago

I think the issue with this is the parsing strategy in general...

  1. Get a list of key descriptors
  2. Get a list of values
  3. merge() them one-by-one

For a[0][b]=c&a[1][d]=e, here is what gets passed into merge:

  1. {a: [{b: 'c'}]}
  2. {a: [<empty item>, {d: 'e'}]}

For a[][b]=c&a[][d]=e, here is what gets passed into merge:

  1. {a: [{b: 'c'}]}
  2. {a: [{d: 'e'}]}

You can see in the latter example, the positional information is not available.

One solution would be to "normalize" the key descriptors, changing a[][b]=c&a[][d]=e to a[0][b]=c&a[1][d]=e before attempting to parseObject().. but with the logic needed to do that, you may as well parse the entire string.

jchook commented 6 years ago

Okay I went ahead and implemented a normalizeKeys() solution, but I am realizing the problem stems even farther than I could have supposed.

Apparently qs.parse() aims to support "semi-parsed" states such as converting objects with URI-encoded keys into deeper objects before supporting basic features like strings with empty brackets.

I have yet to find a single issue that requests or concerns qs.parse() support of object input. Would love to understand the rationale behind this design decision as it seems out of scope.


Edit

I have posted a potential solution to this problem in 5144da93b98e1e23c010ad7a24b3c977af48a8ea.

However, I am running into another problem in the tests... a[5]=b is expected to parse as { a: ['b'] }. Doesn't that seem patently incorrect? I would expect { a: [,,,,'b'] } 100%.

I can identify 11 other similar tests.

@ljharb since the improvements I want to make would be breaking changes (and since it looks like you wrote these 12 tests), I'd be interested in your feedback about how to proceed.

ljharb commented 6 years ago

It looks like that was added here - before it was even qs. @nlf, any chance you remember why that was added?

It definitely seems weird to parse anything but a query string; and I'd be comfortable making that breaking change in v7 in the absence of a justification for keeping it.

jchook commented 6 years ago

The good news is my changes support object input, in case we want to keep it.

My main question is... is there some non-obvious, important purpose of Utils.compact() in qs.parse()?

It seems to break many of my expectations for a robust and interoperable query string parser.

Examples:

a[5]=b

a[2]=b&a[]=c&a[]=d

a[2]=b&a[]=c&a[3]=d

ljharb commented 6 years ago

By default, sparse arrays are never produced, nor should they be - see https://github.com/ljharb/qs#parsing-arrays

When creating arrays with specific indices, qs will compact a sparse array to only the existing values preserving their order:

jchook commented 6 years ago

Okay, I can still fix the OP's bug, keeping the compacting and object input.

But... why not add an allowSparse option while I'm at it? Maybe with a size limit?

Compacting the arrays has its benefits, but you're also discarding information. E.g. http://tic.tac.toe/?squares[1][1]=x might show a tic-tac-toe board with the center marked X.

ljharb commented 6 years ago

Let's save allowSparse for a separate issue :-)