Closed techouse closed 1 month ago
[!IMPORTANT]
Review skipped
Auto reviews are limited to specific labels.
Labels to auto review (1)
* coderabbitPlease check the settings in the CodeRabbit UI or the
.coderabbit.yaml
file in this repository. To trigger a single review, invoke the@coderabbitai review
command.You can disable this status message by setting the
reviews.review_status
tofalse
in the CodeRabbit configuration file.
The changes enhance the parseValues
function by adding logic to decode URL-encoded square brackets, improving the handling of arrays within query parameters. Corresponding test cases were added to ensure the correct parsing of these structures, including various combinations of strings and empty values in nested arrays.
Files | Change Summary |
---|---|
lib/parse.js |
Added code to replace %5B with [ and %5D with ] in cleanStr for better handling of URL-encoded arrays. |
test/parse.js |
Introduced new test cases for parsing URL-encoded brackets with arrays of arrays, covering different string and empty value combinations. |
[!TIP]
Early access features: enabled
We are currently testing the following features in early access: - **OpenAI `gpt-4o` model for code reviews and chat**: OpenAI claims that this model is better at understanding and generating code than the previous models. We seek your feedback over the next few weeks before making it generally available. Note: - You can enable or disable early access features from the CodeRabbit UI or by updating the CodeRabbit configuration file. - Please join our [Discord Community](https://discord.com/invite/GsXnASn26c) to provide feedback and report issues. - OSS projects are always opted into early access features.
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
@coderabbitai review
Can you elaborate a bit? Which test case in your OP is incorrect on the default branch?
Which test case in your OP is incorrect on the default branch?
So the test that exists and passes in v6.12.1 branch is this https://github.com/ljharb/qs/blob/29dda211e8b02654f60975bdb703bcc73a8ee409/test/parse.js#L565
However, there is no test for an encoded query, i.e. something like the test I've added
t.test('parses url-encoded brackets holds array of arrays when having two parts of strings with comma as array divider', function (st) {
st.deepEqual(qs.parse('foo%5B%5D=1,2,3&foo%5B%5D=4,5,6', { comma: true }), { foo: [['1', '2', '3'], ['4', '5', '6']] });
st.deepEqual(qs.parse('foo%5B%5D=1,2,3&foo%5B%5D=', { comma: true }), { foo: [['1', '2', '3'], ''] });
st.deepEqual(qs.parse('foo%5B%5D=1,2,3&foo%5B%5D=,', { comma: true }), { foo: [['1', '2', '3'], ['', '']] });
st.deepEqual(qs.parse('foo%5B%5D=1,2,3&foo%5B%5D=a', { comma: true }), { foo: [['1', '2', '3'], 'a'] });
st.end();
});
This means that something like https://test.local/example?foo[]=1,2,3&foo[]=4,5,6
would parse OK, however, https://test.local/example?foo%5B%5D=1,2,3&foo%5B%5D=4,5,6
would fail. To make it work with v6.12.1 one would have to run the query through decodeURIComponent
first before feeding it to qs.decode
.
Why would you be using square brackets in keys while also using commas? Brackets and commas are mutually exclusive for array encoding.
Why would you be using square brackets in keys while also using commas?
The aforementioned test https://github.com/ljharb/qs/blob/29dda211e8b02654f60975bdb703bcc73a8ee409/test/parse.js#L565
t.test('parses brackets holds array of arrays when having two parts of strings with comma as array divider', function (st) {
st.deepEqual(qs.parse('foo[]=1,2,3&foo[]=4,5,6', { comma: true }), { foo: [['1', '2', '3'], ['4', '5', '6']] });
st.deepEqual(qs.parse('foo[]=1,2,3&foo[]=', { comma: true }), { foo: [['1', '2', '3'], ''] });
st.deepEqual(qs.parse('foo[]=1,2,3&foo[]=,', { comma: true }), { foo: [['1', '2', '3'], ['', '']] });
st.deepEqual(qs.parse('foo[]=1,2,3&foo[]=a', { comma: true }), { foo: [['1', '2', '3'], 'a'] });
st.end();
});
predates this PR by 5 years and was added in https://github.com/ljharb/qs/commit/f884e2d6274c51ad8455e1339a0ad9b12bd63e06 to fix https://github.com/ljharb/qs/issues/327. This PR just fixes it for URL-encoded query strings.
Brackets and commas are mutually exclusive for array encoding.
Sure, but this deals with decoding them, and since there's been a test for it for 5+ years, I assume it's used somewhere. 🤷
Welp, fair enough. Thanks, I’ll give this a thorough review in the next day or three.
@coderabbitai configuration
Welp, fair enough. Thanks, I’ll give this a thorough review in the next day or three.
@ljharb ping 🤓
I noticed that url-encoded square brackets, i.e.
%5B
and%5D
, do not get correctly parsed in this caseThis PR fixes this by replacing
%5B
or%5b
with[
and%5D
or%5d
with]
before splitting the string into delimited parts insideparseQueryStringValues
.I've added the test mentioned above into into the parse test suite.