[Fix] stringify: encode dots in a top-level key with a primitive value when encodeDotInKeys is set#562
Conversation
…e when encodeDotInKeys is set
encodeDotInKeys only encoded dots in object keys whose values were
themselves objects (the recursion path). A top-level key with a
primitive value took the leaf path and was emitted with its dots
unencoded, so stringify(parse(x)) lost the key structure
(e.g. { 'a.b': 'c' } produced a.b=c, which parses back to
{ a: { b: 'c' } } rather than { 'a.b': 'c' }).
Encode the key at the top-level driver, mirroring how nested keys are
already handled, so a dotted key round-trips regardless of its value
type.
ljharb
left a comment
There was a problem hiding this comment.
Looks good - just a few comments first.
| continue; | ||
| } | ||
|
|
||
| var encodedKey = options.encodeDotInKeys ? String(key).replace(/\./g, '%2E') : String(key); |
There was a problem hiding this comment.
This is the right fix in the right place. Encoding the dot to %2E here means the existing encodedPrefix re-encode (line 157) finds no literal dot left and is a no-op for top-level keys, so the object/array/nested cases stay byte-identical to before (no double-encoding). Checking encodeDotInKeys alone (not allowDots) correctly mirrors line 157 (the prefix encoder) rather than line 175 (the child-key encoder), so a top-level dotted key now encodes the same whether its value is a primitive or an object.
| { 'name.obj': 'John' }, | ||
| { encodeDotInKeys: true, allowDots: true, encodeValuesOnly: true } | ||
| ), | ||
| 'name%2Eobj=John', |
There was a problem hiding this comment.
Heads up that this encodeValuesOnly output does not round-trip: qs.parse('name%2Eobj=John', { allowDots: true, decodeDotInKeys: true }) returns { name: { obj: 'John' } }, not { 'name.obj': 'John' }. With encodeValuesOnly the key encoder is skipped, so the single %2E decodes back to a structural dot.
This is a pre-existing encodeValuesOnly + encodeDotInKeys limitation (the object-valued case behaves the same on main), not something this PR introduces. But sitting directly above the round-trip assertion, it reads as if it round-trips too. Suggest a short note here, or asserting the actual parse result, so the limitation is explicit.
| 'round-trips a dotted key with a primitive value' | ||
| ); | ||
|
|
||
| st.end(); |
There was a problem hiding this comment.
A few more cases would lock the full surface of this fix (all verified passing). The last one is the most valuable: it pins the invariant the fix restores, that a top-level key encodes identically regardless of value type.
st.equal(
qs.stringify({ 'a.b': null }, { allowDots: true, encodeDotInKeys: true, strictNullHandling: true }),
'a%252Eb',
'encodes a dotted key with a null value'
);
st.equal(
qs.stringify({ 'a.b': '1', 'c.d': '2' }, { allowDots: true, encodeDotInKeys: true }),
'a%252Eb=1&c%252Ed=2',
'encodes dots in every top-level key, not just the first'
);
st.equal(
qs.stringify({ 'a.b.c': 'x' }, { allowDots: true, encodeDotInKeys: true }),
'a%252Eb%252Ec=x',
'encodes every dot in a key'
);
var primitiveKey = qs.stringify({ 'a.b': 'c' }, { allowDots: true, encodeDotInKeys: true }).split('=')[0];
st.equal(
qs.stringify({ 'a.b': { x: 'c' } }, { allowDots: true, encodeDotInKeys: true }).indexOf(primitiveKey + '.'),
0,
'a top-level dotted key encodes identically for primitive and object values'
);
With
encodeDotInKeys: true, a key that contains a dot is supposed to survive astringify→parseround-trip. That works when the value is an object, but breaks when the value is a primitive at the top level:So the dot in the key is silently treated as structure and the original key is lost.
The dot-encoding was only applied on the recursion path (when the key's value is another object) and on nested child keys, but a top-level key with a primitive value takes the leaf path and was passed through unencoded. I moved the encoding to the top-level driver so the key is encoded the same way regardless of its value's type, matching how nested keys are already handled. The existing
encodeDotInKeystests only ever used object values, which is why this slipped through; I added a case covering a primitive value (plus a round-trip assertion).Full test suite, lint, and readme checks pass locally.