[Fix] stringify: encode keys when allowEmptyArrays is set#561
Conversation
ljharb
left a comment
There was a problem hiding this comment.
Looks good - just a few more things before merging.
|
|
||
| if (allowEmptyArrays && isArray(obj) && obj.length === 0) { | ||
| return adjustedPrefix + '[]'; | ||
| return (encoder && !encodeValuesOnly ? formatter(encoder(adjustedPrefix, defaults.encoder, charset, 'key', format)) : adjustedPrefix) + '[]'; |
There was a problem hiding this comment.
Nice fix. One small thing: the commit message says this mirrors the strictNullHandling line, but that sibling (the #554 fix just above) wraps formatter() around the whole conditional, including the raw branch. Here formatter is only on the encoded branch.
They are byte-identical for every real input, but they diverge for a key whose literal text contains a character the RFC1738 formatter rewrites (e.g. a literal %20) on the encodeValuesOnly / encode: false path: this emits a%20b[], while the normal non-empty-array path emits a+b... for the same key. Wrapping the whole conditional matches #554 exactly, stays byte-identical for every real case, and fixes that edge. Verified: 1006 tests pass and lib/stringify.js stays at 100% coverage.
| return (encoder && !encodeValuesOnly ? formatter(encoder(adjustedPrefix, defaults.encoder, charset, 'key', format)) : adjustedPrefix) + '[]'; | |
| return formatter(encoder && !encodeValuesOnly ? encoder(adjustedPrefix, defaults.encoder, charset, 'key', format) : adjustedPrefix) + '[]'; |
| ); | ||
| st.equal( | ||
| qs.stringify({ 'a b': [] }, { allowEmptyArrays: true, encodeValuesOnly: true }), | ||
| 'a b[]', |
There was a problem hiding this comment.
These encodeValuesOnly / encode: false cases assert a b[], which is byte-identical to the old buggy output, so the test can't distinguish "intentionally unencoded" from the bug. Worth making the encoded path's fidelity explicit, and adding a consistency check against the same key with a real value (that is the assertion that surfaces the formatter nuance in lib). For example, before st.end() (an empty array round-trips to [''], so assert on the key, not a full deep-equal):
st.ok(
'a b' in qs.parse(qs.stringify({ 'a b': [] }, { allowEmptyArrays: true }), { allowEmptyArrays: true }),
'the encoded key round-trips back to the original'
);
st.equal(
qs.stringify({ 'a b': ['x'] }, { allowEmptyArrays: true, arrayFormat: 'brackets' }),
'a%20b%5B%5D=x',
'the same key with a value encodes the key prefix identically'
);
When
allowEmptyArrays: trueis set, an empty-array key is emitted via an early return that concatenates the raw prefix with[], bypassing the encoder and formatter that every other key path runs through. As a result keys that require encoding are emitted unencoded (and invalid):This is the same class of bug as #554 (
strictNullHandlingnot applying the formatter to the encoded key, sibling code branch just above). The fix mirrors that one: wrap the prefix informatter(encoder(prefix, defaults.encoder, charset, 'key', format))when an encoder is present and we're not inencodeValuesOnlymode — matching the existingstrictNullHandlingline exactly.Behavior
{ 'a b': [] }→a%20b[]format: 'RFC1738': →a+b[]encodeValuesOnly: true: →a b[](key left raw, as expected)encode: false: →a b[](unchanged)foo) are unchanged, so existing tests/README stay valid.Tests
Added a regression test (
should encode the key of an empty array when allowEmptyArrays is set) covering the default, RFC1738,encodeValuesOnly, andencode: falsecases. Full suite green (941 tests).