Comma-delimited array parsing fails when individual items contain commas

[lionel-rowe]

Comma-delimited array parsing fails when running something like this:

Qs.parse('things=a%2C%20b,c%2C%20d', {
  comma: true
});

Expected: { things: ['a, b', 'c, d'] }
Actual: { things: ['a', ' b', 'c', ' d'] }

Per my understanding of RFC 3986, a percent-encoded comma should not be treated the same as a comma literal. Comma literal is a reserved character used for delimiting parameters and parameter values, but a percent-encoded comma is not.

My use case needs to deal with arbitrary user input within values, potentially including commas.

I guess this can be fixed with a custom decoder function, but it seems like something that should work by default when comma is true.

As my use case is otherwise fairly simple and I don't need lots of custom features, I figured I'd just roll my own query string parser/serializer. I'm linking to this strictly for reference and wouldn't recommend anyone to use it in production in its current state - it probably diverges from the RFC in a million other ways and fails for lots of edge cases that aren't relevant to my specific use case.

Related: #237 (but that deals with stringify, whereas this is about parse).

[ljharb]

Setting comma does not set RFC3986; try { comma: true, format: 'RFC3986' }.

[ljharb]

actually on a second look, that's supposed to be the default.

In that case, that sounds like a bug. a PR would be welcome.

[lionel-rowe]

FWIW, adding the format: 'RFC3986' option produces exactly the same result for this input (i.e. { things: ['a', ' b', 'c', ' d'] }), so I don't think the problem is an incorrect default. As far as I can tell, the format option only affects whether + is used in place of %20, and even then that only affects serialization and not parsing.

[daggerjames]

it's a bug, I'll fix it later

[daggerjames]

@ljharb

When I try to fix this issue, it has a lot of confusion.
When I implemented comma, I haven't considered the situation that there is an edge case that a comma is inside the string.

> qs.stringify({things: ['a, b', 'c, d']}, {arrayFormat: 'comma'})
'things=a%2C%20b%2Cc%2C%20d'
> qs.stringify({things: ['a, b', 'c, d']}, {arrayFormat: 'comma', encode: false})
'things=a, b,c, d'   // current

What is expected after we stringify that object?

[ljharb]

@daggerjames i would expect things=a%2C b,c%2C d, i think?

[Egorrr]

Hello, any news regarding this issue? Really hope it will be fixed

[quentinus95]

@ljharb A pull request has been opened here: #336 What is missing to get it merged?

[ljharb]

@quentinus95 #336 (comment) - it's a breaking change currently, and @Om4ar hasn't updated the PR to make it not be one.

If anyone else has an update for the PR, post a link to a branch on it (NOT a new PR) and i'll pull it in.

[quentinus95]

@ljharb here you go: https://github.com/quentinus95/qs/tree/comma-perecent-parsing

[quentinus95]

@ljharb this ticket should be kept open, the merged PR does not fix the issue. Please see my comment on the PR.
An url encoded comma should not be used as a separator to create an array. Otherwise it prevents having data with url encoded commas in the query parameters values.

As I wrote, if you have any suggestion to fix the issue without doing a large refactoring, I'll be happy to write the implementation.

Thanks!