search

ljharb / qs

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

Encode comma values more consistently #463

Closed bakkerthehacker closed 1 year ago

bakkerthehacker commented 1 year ago

Fixes #459 but only for encodeValuesOnly: true

This PR includes tests for arrays with comma values and different settings.

The behaviour for encodeValuesOnly has been adjusted to encode values before they are joined together, as an example it changes stringifying { a: [',', '', 'c,d'] } from a=,,,c,d to a=%2C,,c%2Cd.

The behaviour of the other 2 encoding setups (encode: false) and (encode: true with encodeValuesOnly: false) have been left unmodified. These formats do seem less useful for arrayFormat: 'comma', and tricky to get right in all situations.

However, there is additional functionality in stringify that isnt really tested, but is related to these options for the types: 'string' 'number' 'boolean' 'symbol' 'bigint' 'buffer'. This inconsistent additional functionality also avoids encoding commas when they appear in these types.

I have included additional tests for these situations, where a comma appears in a string outside an array. I have also removed the custom functionality in this PR and these types now encode comma values normally. If this change goes too far, it can be removed, but it seems odd to have the comma version of the arrayFormat affect non-array types just because they contain a comma.

bakkerthehacker commented 1 year ago

This looks good overall, but there's a test failing - "array with multiple items with a comma inside". Does that mean this is a breaking change, or is it a bug in the PR?

There were already tests for 2 of the 12 test cases that I added here. Both the existing test cases use arrayFormat: comma which is being adjusted in this PR. One of the 2 tests has a double encode (comma -> %2C -> %25%2C) which seems wrong, and it is affected by the change.

ljharb commented 1 year ago

Given that qs.parse('a=c%25%2Cd%2Ce') is { a: 'c%,d,e' } and qs.parse('a=c%2Cd%2Ce') is { a: 'c,d,e' }, this is certainly an improvement, but i think the ideal end point is where things can round trip.

I'll consider this semver-patch since for comma arrayFormat users it's clearly broken atm.

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and no project coverage change

Comparison is base (20820fa) 99.79% compared to head (4c4b23d) 99.79%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #463 +/- ## ======================================= Coverage 99.79% 99.79% ======================================= Files 8 8 Lines 1459 1487 +28 Branches 177 176 -1 ======================================= + Hits 1456 1484 +28 Misses 3 3 ``` | [Impacted Files](https://codecov.io/gh/ljharb/qs/pull/463?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jordan+Harband) | Coverage Δ | | |---|---|---| | [lib/stringify.js](https://codecov.io/gh/ljharb/qs/pull/463?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jordan+Harband#diff-bGliL3N0cmluZ2lmeS5qcw==) | `99.35% <100.00%> (-0.02%)` | :arrow_down: | | [test/stringify.js](https://codecov.io/gh/ljharb/qs/pull/463?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jordan+Harband#diff-dGVzdC9zdHJpbmdpZnkuanM=) | `99.78% <100.00%> (+0.01%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jordan+Harband). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jordan+Harband)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.