fix: allow parsing of empty keys in objects #433

Coobaha · 2022-03-05T05:08:43Z

Resolves #432

to add an option to both parse and stringify
to have tests, for both parse and stringify, for at least:
- repeat keys with all array formats
- bracketed keys with all array formats
- a lone = (and =& and &= and &=&)
- ❌ deep array/object case

If we are happy with this implementation and want to proceed:

to document in the readme
split tests before merge to parse/stringify files
update https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/qs (also allowSparse is missing there and maybe other types)

Other:

Stringify works without an option, so it is potentially a patch but altering parse behaviour only via option seems a safe bet to not accidentally break ppl code, and can be considered as a default behaviour in the next major version?

lib/parse.js

Coobaha · 2022-03-05T05:25:11Z

test/parse.js

+        st.deepEqual(qs.stringify({ '': { '': [2, 3] } }, { encode: false }), '[][0]=2&[][1]=3');
+        st.deepEqual(qs.stringify({ '': { '': [2, 3], a: 2 } }, { encode: false }), '[][0]=2&[][1]=3&[a]=2');
+        st.deepEqual(qs.parse('[][0]=2&[][1]=3', { allowEmptyKeys: true }), { '': { '': ['2', '3'] } });
+        st.deepEqual(qs.parse('[][0]=2&[][1]=3&[a]=2', { allowEmptyKeys: true }), { '': { '': ['2', '3'], a: '2' } });


This case fails, but this is ambiguous string which is treated as { : [ '2', '3' ] } and stringify <-> parse is not producing the same results

--- operator: deepEqual expected: |- { : { : [ '2', '3' ] } } actual: |- { : [ '2', '3' ] }

operator: deepEqual expected: |- { : { : [ '2', '3' ], a: '2' } } actual: |- { : { 0: '2', 1: '3', a: '2' } }

it's ok to mark this case as a TODO for now; there's an existing issue about a nested array/object combo

ljharb · 2022-03-05T05:36:41Z

There's unlikely to be a major version any time soon; breaking changes just aren't worth it. Configuring it via option is the best bet for now.

ljharb

At the very least, let's add stringify tests using all of the outputs in the parse tests as input.

lib/parse.js

ljharb · 2022-03-05T05:39:03Z

test/parse.js

+
+test('parse/stringify empty key WIP', function (t) {
+    t.test('parses an object with empty string key', function (st) {
+        st.deepEqual(qs.parse('=a', { allowEmptyKeys: true }), { '': 'a' });


for each of these test inputs, let's test both allowEmptyKeys true, false, and absent

Done, please let me know what will be the correct place for this tests? They are testing both parse and stringify and current tests are split by this methods. To me it feels more reasonable to test them together in this case.

ljharb · 2022-03-05T05:40:30Z

test/parse.js

+        st.deepEqual(qs.stringify({ '': { '': [2, 3] } }, { encode: false }), '[][0]=2&[][1]=3');
+        st.deepEqual(qs.stringify({ '': { '': [2, 3], a: 2 } }, { encode: false }), '[][0]=2&[][1]=3&[a]=2');
+        st.deepEqual(qs.parse('[][0]=2&[][1]=3', { allowEmptyKeys: true }), { '': { '': ['2', '3'] } });
+        st.deepEqual(qs.parse('[][0]=2&[][1]=3&[a]=2', { allowEmptyKeys: true }), { '': { '': ['2', '3'], a: '2' } });


it's ok to mark this case as a TODO for now; there's an existing issue about a nested array/object combo

ljharb · 2022-03-06T20:54:16Z

test/parse.js

+            st.deepEqual(qs.stringify(parsed, { encode: false }), testCase.stringifyOutput);
+            st.deepEqual(qs.parse(testCase.stringifyOutput, { allowEmptyKeys: true }), testCase.withEmptyKeys);


i'd prefer to keep stringify tests in the stringify tests file; it's fine if the test cases are in a separate file both test files require.

Hey, sorry for the delay :) i've extracted test cases into own file and and separated stringify/parse parts accordingly

codecov · 2022-05-15T16:40:00Z

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (a5609c7) 99.80% compared to head (5e70a6a) 99.80%.

❗ Current head 5e70a6a differs from pull request most recent head c077991. Consider uploading reports for the commit c077991 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #433   +/-   ##
=======================================
  Coverage   99.80%   99.80%           
=======================================
  Files           9        9           
  Lines        1518     1531   +13     
  Branches      177      179    +2     
=======================================
+ Hits         1515     1528   +13     
  Misses          3        3

Impacted Files	Coverage Δ
test/empty-keys-cases.js	`100.00% <ø> (ø)`
lib/parse.js	`100.00% <100.00%> (ø)`
test/parse.js	`99.80% <100.00%> (+<0.01%)`	⬆️
test/stringify.js	`99.79% <100.00%> (+<0.01%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

ljharb

Looking great!

ljharb · 2022-05-15T16:40:17Z

test/parse.js

+            qs.parse('[][0]=2&[][1]=3', { allowEmptyKeys: true }),
+            { '': { '': ['2', '3'] } },
+            'array/object conversion',
+            { skip: 'TODO: figure out what this should do' }
+        );
+
+        st.deepEqual(
+            qs.parse('[][0]=2&[][1]=3&[a]=2', { allowEmptyKeys: true }),
+            { '': { '': ['2', '3'], a: '2' } },
+            'array/object conversion',
+            { skip: 'TODO: figure out what this should do' }


let's discuss these TODOs. what are your thoughts?

Since we parse empty keys I assume output should be equal to key-full strings?

If we use a[b][0]=2&a[b][1]=3 the output object is { a: { b: [ '2', '3' ] } }

when we replace a and b with empty strings in this object: { '': { '': [ '2', '3' ] } }

so query string equivalent should be [][0]=2&[][1]=3 which is <EMPTY_KEY>[<EMPTY_KEY>][0]=2

Current output for [][0]=2&[][1]=3 without empty keys flag is { 0: '2', 1: '3' }

ljharb · 2022-05-15T16:40:52Z

test/stringify.js

+test('stringifies empty keys', function (t) {
+    emptyTestCases.forEach(function (testCase) {
+        t.test('stringifies an object with empty string key with ' + testCase.input, function (st) {
+            st.deepEqual(qs.stringify(testCase.withEmptyKeys, { encode: false }), testCase.stringifyOutput);


can we also add a test case where the input is a stringified noEmptyKeys?

ljharb · 2023-05-11T06:25:59Z

test/empty-keys-cases.js

+        { input: '&', withEmptyKeys: {}, stringifyOutput: '', noEmptyKeys: {}, stringifyOutputNoEmpty: '' },
+        { input: '&&', withEmptyKeys: {}, stringifyOutput: '', noEmptyKeys: {}, stringifyOutputNoEmpty: '' },
+        { input: '&=', withEmptyKeys: { '': '' }, stringifyOutput: '=', noEmptyKeys: {}, stringifyOutputNoEmpty: '' },
+        { input: '&=&', withEmptyKeys: { '': '' }, stringifyOutput: '=', noEmptyKeys: {}, stringifyOutputNoEmpty: '' },
+        { input: '&=&=', withEmptyKeys: { '': ['', ''] }, stringifyOutput: '[0]=&[1]=', noEmptyKeys: {}, stringifyOutputNoEmpty: '' },
+        { input: '&=&=&', withEmptyKeys: { '': ['', ''] }, stringifyOutput: '[0]=&[1]=', noEmptyKeys: {}, stringifyOutputNoEmpty: '' },


it makes me very nervous to be modifying test cases for a bug fix. is there any chance for a default algorithm to infer stringifyOutputNoEmpty rather than needing to add it to each case?

🤷 I think explicit cases are better here. I assume you thought that we should default to '' if you want a smaller diff, then it also makes to do the same for stringifyOutput and withEmptyKeys, which will be hard to maintain.

Explicit cases are great, it’s just that the best way to be sure there’s no breaking changes is to not have a diff on existing test cases :-)

[Fix] parse: allow parsing of empty keys in objects

95c6c3e

Coobaha commented Mar 5, 2022

View reviewed changes

lib/parse.js Show resolved Hide resolved

Coobaha mentioned this pull request Mar 5, 2022

qs.parse omitting empty string keys #432

Open

Coobaha force-pushed the empty-keys-fix branch from 4445e76 to 9589c69 Compare March 5, 2022 05:24

Coobaha commented Mar 5, 2022

View reviewed changes

ljharb reviewed Mar 5, 2022

View reviewed changes

ljharb reviewed Mar 6, 2022

View reviewed changes

ljharb force-pushed the empty-keys-fix branch from cb5ef70 to 1d5ccca Compare May 15, 2022 16:38

ljharb reviewed May 15, 2022

View reviewed changes

[Tests] add test cases for stringify with empty keys

c077991

ljharb force-pushed the empty-keys-fix branch 2 times, most recently from 5e70a6a to c077991 Compare May 11, 2023 06:24

ljharb reviewed May 11, 2023

View reviewed changes

ljharb marked this pull request as draft May 11, 2023 06:26

ljharb force-pushed the main branch from a5371f6 to c8a269f Compare February 29, 2024 23:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: allow parsing of empty keys in objects #433

fix: allow parsing of empty keys in objects #433

Coobaha commented Mar 5, 2022 •

edited

Loading

Coobaha Mar 5, 2022 •

edited

Loading

ljharb Mar 5, 2022

ljharb commented Mar 5, 2022

ljharb left a comment

ljharb Mar 5, 2022

Coobaha Mar 6, 2022

ljharb Mar 5, 2022

ljharb Mar 6, 2022

Coobaha May 15, 2022

codecov bot commented May 15, 2022 •

edited

Loading

ljharb left a comment

ljharb May 15, 2022

Coobaha May 21, 2022 •

edited

Loading

ljharb May 15, 2022

ljharb May 11, 2023

Coobaha May 11, 2023 •

edited

Loading

ljharb May 11, 2023

		st.deepEqual(qs.stringify(parsed, { encode: false }), testCase.stringifyOutput);
		st.deepEqual(qs.parse(testCase.stringifyOutput, { allowEmptyKeys: true }), testCase.withEmptyKeys);

fix: allow parsing of empty keys in objects #433

Are you sure you want to change the base?

fix: allow parsing of empty keys in objects #433

Conversation

Coobaha commented Mar 5, 2022 • edited Loading

Coobaha Mar 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb commented Mar 5, 2022

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 15, 2022 • edited Loading

Codecov Report

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Coobaha May 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Coobaha May 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Coobaha commented Mar 5, 2022 •

edited

Loading

Coobaha Mar 5, 2022 •

edited

Loading

codecov bot commented May 15, 2022 •

edited

Loading

Coobaha May 21, 2022 •

edited

Loading

Coobaha May 11, 2023 •

edited

Loading