Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot use square brackets in key name #235

Open
alasdairhurst opened this issue Nov 8, 2017 · 7 comments
Open

Cannot use square brackets in key name #235

alasdairhurst opened this issue Nov 8, 2017 · 7 comments

Comments

@alasdairhurst
Copy link

alasdairhurst commented Nov 8, 2017

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.

@ljharb
Copy link
Owner

ljharb commented Nov 8, 2017

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.

@alasdairhurst
Copy link
Author

alasdairhurst commented Nov 9, 2017

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.

@ljharb
Copy link
Owner

ljharb commented Nov 9, 2017

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.

@alasdairhurst
Copy link
Author

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
Copy link
Author

alasdairhurst commented Nov 10, 2017

@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?

@ljharb
Copy link
Owner

ljharb commented Nov 11, 2017

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.

@joshjg
Copy link

joshjg commented Nov 17, 2017

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants