qs icon indicating copy to clipboard operation
qs copied to clipboard

Cannot use square brackets in key name

Open alasdairhurst opened this issue 6 years ago • 7 comments

I want to make a request that looks like this:

?foo[bar]=baz

Without looking into all the fancy features that QS provides, i expect the following object:

{
  "foo[bar]": "baz"
}

I see this:

{
  "foo": {
    "bar": "baz"
  }
}

I see that square brackets are used as some sort of object accessor but there seems like no documented way of escaping them or disabling this feature that I can see for this simplest of use cases.

alasdairhurst avatar Nov 08 '17 14:11 alasdairhurst

This behavior is a standard convention across the entire web.

If you want square brackets in a key name, you'd need to escape them yourself - encodeURIComponent('foo[bar]') - since servers would interpret them the way qs does anyways.

In other words, it's not a simple use case, it's an exceedingly uncommon one for decades on the web.

ljharb avatar Nov 08 '17 20:11 ljharb

Then is it an issue if I get the same behaviour no matter if the square brackets are escaped or not? I originally ran into the issue doing escaping first. (?foo%5Bbar%5D=baz)

Additionally, the node querystring library does not behave this way, Is that an issue with node?

Here's a test with qs, query-string and node's querystring

var query_string = require("query-string")
var querystring = require('querystring');
var qs = require('qs');
var results = [];
var key = 'foo[bar]';

var escapedQuery = encodeURIComponent(key) + '=baz';
var query = key + '=baz';

results.push(query_string.parse(escapedQuery));
results.push(query_string.parse(query));
results.push(querystring.parse(escapedQuery));
results.push(querystring.parse(query));
results.push(qs.parse(escapedQuery));
results.push(qs.parse(query));

console.log(results)
[
  { "foo[bar]": "baz" },
  { "foo[bar]": "baz" },
  { "foo[bar]": "baz" },
  { "foo[bar]": "baz" },
  { "foo": { "bar": { "baz" } },
  { "foo": { "bar": { "baz" } }
]

As you can see, only QS handles square brackets that way. It doesn't include the square brackets as part of the key if they are escaped as you are saying.

I also tested in C# with HttpUtility.ParseQueryString("foo%5Bbar%5D=baz"); and HttpUtility.ParseQueryString("foo[bar]=baz"); The first key returned from the resulting collection was foo[bar] both times. https://msdn.microsoft.com/en-us/library/ms150046(v=vs.110).aspx

I tested in PHP:

<?php
$str = "foo%5Bbar%5D=baz";

echo parse_str($str, $output);
echo $output["foo"]["bar"];

$str2 = "foo[bar]=baz";

echo parse_str($str2, $output2);
echo $output2["foo"]["bar"];

This output baz both times, meaning that encoding the square brackets did nothing, but it was consistent with the QS functionality.

Test in python using urllib:

import urllib.parse

print(urllib.parse.urlparse('http://localhost?foo[bar]=baz').query)
print(urllib.parse.parse_qs('foo[bar]=baz'));
print(urllib.parse.parse_qsl('foo[bar]=baz'))
print(dict(urllib.parse.parse_qsl('foo[bar]=baz')))

print(urllib.parse.urlparse('http://localhost?foo%5Bbar%5D=baz').query)
print(urllib.parse.parse_qs('foo%5Bbar%5D=baz'));
print(urllib.parse.parse_qsl('foo%5Bbar%5D=baz'))
print(dict(urllib.parse.parse_qsl('foo%5Bbar%5D=baz')))

with the following results:

foo[bar]=baz
{'foo[bar]': ['baz']}
[('foo[bar]', 'baz')]
{'foo[bar]': 'baz'}
foo%5Bbar%5D=baz
{'foo[bar]': ['baz']}
[('foo[bar]', 'baz')]
{'foo[bar]': 'baz'}

As you can see, other than QS, I have only found one example of the behaviour that QS shows. Others do not do this. Additionally in every test I have done, encoding the square brackets has no effect on the result.

alasdairhurst avatar Nov 09 '17 10:11 alasdairhurst

Thanks, this information changes things slightly.

Without a doubt, this should be true (take =~= here as a magical operator that compares objects by contents):

qs.stringify({
  "foo[bar]": "baz"
}) === 'foo%5Bbar%5D=baz'; // true

qs.stringify({
  foo: {
    bar: "baz",
  },
}) === 'foo[bar]=baz'; // true

qs.parse('foo[bar]=baz') =~= {
  foo: {
    bar: 'baz'
  },
}; // true

qs.parse('foo%5Bbar%5D=baz') =~= {
  'foo[bar]': 'baz',
}; // true

If any of these four fail, then there is decidedly a bug in qs.

A PR to add these test cases, and fix, is welcome - I'll take a look at it in a few days otherwise.

ljharb avatar Nov 09 '17 23:11 ljharb

Great! I'll push a PR when i get a chance. The only thing that i'm worried about is if people are currently encoding their square brackets and expecting the current behaviour. Seems like this would count as a breaking change when it goes for release.

alasdairhurst avatar Nov 10 '17 09:11 alasdairhurst

@ljharb So i've added and ran a few tests including ones in addition to your proposed tests, and all the ones where the square brackets are part of the key do fail.

It uncovers the additional issue that { 'foo[bar]': 'baz' } and { foo: { bar: 'baz' } } both stringify to 'foo%5Bbar%5D=baz'.

In one way it is expected since square brackets are URL encodable, but it's something to be aware of going forward.

Edit: In fact, a large amount of tests are going to be affected by the change.

Previously mentioned object stringification: qs.stringify({ a: { b: 'c', d: null } }, { skipNulls: true }) currently === 'a%5Bb%5D=c' and would change to 'a[b]=c'

Array value stringification: qs.stringify({ a: ['b', 'c', 'd'] }); currently === 'a%5B0%5D=b&a%5B1%5D=c&a%5B2%5D=d' and would change to 'a[]=b&a[]=c&a[]=d'

A huge difference shows when parsing the previous values: qs.parse('a[]=b&a[]=c&a[]=d') currently === { a: ['b', 'c', 'd'] } qs.parse('a%5B0%5D=b&a%5B1%5D=c&a%5B2%5D=d') currently === { a: ['b', 'c', 'd'] } as well, but would become === { "a[]": 'd' } if the change was made (assuming keys get overwritten by the last occurance)

This all seems to rely on the "encode" option not being passed in and defaulting to true. What would be the expected behavior with { encode: false }? qs.stringify({ "foo[bar]": "baz" }, { encode: false}); === 'foo[bar]=baz;'? it didn't get encoded, but now it means something else :S

I don't know how many people are encoding their square bracketed queries but I'm guessing a lot? This is likely to mess a lot of people up if they don't pay attention to changelogs :)

Part of me feels that putting something behind an option would be the right thing to do here to stop it breaking people, but the other part feels like the option should be to enable the legacy functionality of ignoring if the square brackets are escaped or not.

The downside of this is that a big user of qs is express, and express doesn't make it easy to change the config passed to qs, so they're gonna pick what they think is best for everyone which most likely is the current limitation which is disappointing in a way.

Thoughts?

alasdairhurst avatar Nov 10 '17 19:11 alasdairhurst

Hmm, this is definitely complex.

Let me think more about it.

It might be better to change the behavior only when an option is passed, so as to avoid breaking changes - even if that means express users can't access that behavior.

ljharb avatar Nov 11 '17 06:11 ljharb

@ljharb Ran into a similar issue recently - I need to be able to include keys which I have no control over, which unfortunately may include square brackets/dots. I was wondering if you are aware of a good way to encode/decode them in this case, so as not to be interpreted by qs as nested properties but rather a flat key-value pair.

joshjg avatar Nov 17 '17 22:11 joshjg