Skip to content

Commit

Permalink
[fixed] Double-encoding of query strings
Browse files Browse the repository at this point in the history
Also, don't use indices in array query string values.

Fixes #721
  • Loading branch information
mjackson committed Jan 25, 2015
1 parent cc5490e commit fe5ec39
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 3 deletions.
4 changes: 2 additions & 2 deletions modules/utils/Path.js
Expand Up @@ -144,10 +144,10 @@ var Path = {
if (existingQuery)
query = query ? merge(existingQuery, query) : existingQuery;

var queryString = query && qs.stringify(query);
var queryString = qs.stringify(query, { indices: false });

if (queryString)
return Path.withoutQuery(path) + '?' + queryString;
return Path.withoutQuery(path) + '?' + decodeURIComponent(queryString);

return path;
},
Expand Down
2 changes: 1 addition & 1 deletion modules/utils/__tests__/Path-test.js
Expand Up @@ -322,7 +322,7 @@ describe('Path.withQuery', function () {
});

it('merges two query strings', function () {
expect(Path.withQuery('/path?a=b', { c: [ 'd', 'e' ]})).toEqual('/path?a=b&c%5B0%5D=d&c%5B1%5D=e');
expect(Path.withQuery('/path?a=b', { c: [ 'd', 'e' ] })).toEqual('/path?a=b&c=d&c=e');
});
});

Expand Down

7 comments on commit fe5ec39

@ryanflorence
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't some (rails) servers want c[]=d&c[]=e? Can we make this work with qs?

@mjackson
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rpflorence hmm, googled around a bit and yes, it looks like PHP and Rails won't like c=d&c=e. But qs only supports indexed args, e.g. c[0]=d&c[1]=e which we don't want either. We probably need to submit a PR upstream on qs to fix this.

@ryanflorence
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the only time this matters is when you want to just pass along your client-side query to a server-side query (like an autocomplete component). You have a chance to build your own with the query object the router gives you, so its not an insurmountable task, but yeah, an upstream option would be great.

@jenseng
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like they're moving towards a solution that will give consistent parsing and generation of array params: ljharb/qs#46 /cc @amireh

@jenseng
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... specifically the comments at the end re arrayFormat ... now to create a PR :)

@mjackson
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jenseng
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome, thx @mjackson!

Please sign in to comment.