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

Adjusted the url package to encode nested objects and arrays (to help address #5065). #8261

Merged
merged 2 commits into from Feb 8, 2017

Conversation

Projects
None yet
4 participants
@hwillson
Member

hwillson commented Jan 17, 2017

Hi all - this PR is intended to help address issue #5065. I've extracted @aldeed's proposed code from the dispatch:url-encode-params package, adding it to the url package. As mentioned in the original issue, this isn't a perfect solution since Node handles this differently, but it's definitely better than the current behaviour (and can always be extended later if/as needed). Anyways, it's a start - let the discussions begin! :-)

@aldeed

This comment has been minimized.

Contributor

aldeed commented Jan 25, 2017

Thanks @hwillson!

URL._encodeParams = function (params, prefix) {
var str = [];
for (var p in params) {
if (params.hasOwnProperty(p)) {

This comment has been minimized.

@benjamn

benjamn Jan 25, 2017

Member

I know this may seem like an edge case, but what if there's a parameter named hasOwnProperty?

This comment has been minimized.

@hwillson

hwillson Jan 26, 2017

Member

Good call @benjamn - fixing coming - thanks!

@abernix

abernix approved these changes Feb 8, 2017

@benjamn benjamn merged commit c7e8706 into meteor:devel Feb 8, 2017

3 checks passed

CLA Author has signed the Meteor CLA.
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@benjamn benjamn added this to the Release 1.4.3 milestone Feb 8, 2017

@abernix

This comment has been minimized.

Member

abernix commented Feb 9, 2017

Result of this PR

Thought I might document a possible problem others might encounter that I just found while debugging a problem with accounts-facebook. As I'll try to explain here, this may be a very small edge case for some who have (miraculously?) found that having arrays serialized into a string, separated by a comma to be desired behavior.

Getting into it... the Meteor implementation of HTTP.call (via the http package) provides the ability to provide a params object containing various data parameters which are encoded into the request, either via GET or POST.

Quick Rundown

Prior to #8261 (this PR), an HTTP.call with params of { fields: ["id", "email", "name"] } would be serialized by url as fields=id%2C%email%2Cname – a comma-delimited string, but single query string parameter.

With #8261 (this PR), HTTP.call with the same params will be serialized to: fields[]=id&fields[]=email&fields[]=name. This is in-line with PHP, Ruby and jQuery.param which also use/read bracketed array notation.

Explanation

The HTTP.call method internally calls URL._encodeParams or URL._constructUrl which are provided here by the url package.

Previously, HTTP.call used a super simple, but super error-prone serialization method for Arrays which would simply join the array elements together with commas. Actually, it didn't actually do any of that serialization itself preferring to outsource that to encodeURIComponent, a method which is intended to deal with Strings. Thus, the Array's .toString() method was being invoked, resulting in the default Array.prototype.toString() behavior of a comma-delimited list.

If anyone has attempted to pass an array in params to HTTP.call, they would have encountered a terrible failure if their array contained a comma as ["Hey, you", "over there"] would have become an element longer (Hey, you, over there).

Similarly, if an Object was passed, you'd end up with the hilarity that is: variable=[Object object].

Failure

One implementation has been found so far which was actually relying on this array-joining, which is Meteor's own accounts-facebook package which actually desires the fields result in the resulting request to be id,email,name.

Expectations

To be honest, there's no reason why accounts-facebook (or any other package, inside or outside Meteor) should have been expecting a comma-delimited result to come out of this. While http and url have tests, there were no tests in place which showed intention of making sure this .join(',') behavior work. Additionally, the documentation provides zero insinuation that this would work.

What to do?

We could revert this change as complaints were reasonably low, but the previous implementation left a lot to be desired (see Object, above). I believe we'll just need to make it very clear about this change in behavior in History.md (e.g. if you expect arrays to be serialized as comma-delimited when using `HTTP.call`, you should call `.join(',')` yourself to continue this behavior.. The accounts-facebook package can just easily be changed to do this.

Open to other suggestions too... though if you're suggesting using the query-string, I'll point out that it has a 16KB foot-print (uncompressed/unminified).

/cc @hwillson @aldeed @benjamn

@hwillson

This comment has been minimized.

Member

hwillson commented Feb 9, 2017

Thanks for the detailed breakdown @abernix! My vote goes towards keeping the change and documenting it - as you said, there's no reason why accounts-facebook or others should have been expecting this behaviour. Thanks again!

abernix added a commit that referenced this pull request Feb 10, 2017

Explicitly join the fields array to a comma-delimited string.
With #8261 merged, arrays are no longer concatenated (with commas) into a string during `HTTP.call` requests so we need to specify the exact behavior we want. Any previous behavior of this type was purely accidental.

The definition of this field in Facebook's API is that it should be a comma-separated field:

https://developers.facebook.com/docs/graph-api/using-graph-api

abernix added a commit that referenced this pull request Feb 10, 2017

Explicitly join the fields array to a comma-delimited string.
With #8261 merged, arrays are no longer concatenated (with commas) into a string during `HTTP.call` requests so we need to specify the exact behavior we want. Any previous behavior of this type was purely accidental.

The definition of this field in Facebook's API is that it should be a comma-separated field:

https://developers.facebook.com/docs/graph-api/using-graph-api

abernix added a commit that referenced this pull request Feb 10, 2017

Explicitly join the fields array to a comma-delimited string.
With #8261 merged, arrays are no longer concatenated (with commas) into a string during `HTTP.call` requests so we need to specify the exact behavior we want. Any previous behavior of this type was purely accidental.

The definition of this field in Facebook's API is that it should be a comma-separated field:

https://developers.facebook.com/docs/graph-api/using-graph-api

abernix added a commit to abernix/meteor that referenced this pull request Feb 10, 2017

Explicitly join the fields array to a comma-delimited string.
With meteor#8261 merged, arrays are no longer concatenated (with commas) into a string during `HTTP.call` requests so we need to specify the exact behavior we want. Any previous behavior of this type was purely accidental.

The definition of this field in Facebook's API is that it should be a comma-separated field:

https://developers.facebook.com/docs/graph-api/using-graph-api

abernix added a commit that referenced this pull request Feb 10, 2017

Explicitly join the fields array to a comma-delimited string.
With #8261 merged, arrays are no longer concatenated (with commas) into a string during `HTTP.call` requests so we need to specify the exact behavior we want. Any previous behavior of this type was purely accidental.

The definition of this field in Facebook's API is that it should be a comma-separated field:

https://developers.facebook.com/docs/graph-api/using-graph-api

@abernix abernix referenced this pull request Feb 13, 2017

Closed

query format fail #124

abernix added a commit that referenced this pull request Feb 13, 2017

abernix added a commit that referenced this pull request Mar 28, 2017

[backport] Explicitly join the fields array to a comma-delimited string.
This is a backport of 2c5dda1675a9e3526e3a926d3ec2b45d0fab14c6gu for the
Meteor 1.4.2.x series.

With #8261 merged, arrays are no longer concatenated (with commas) into a string during `HTTP.call` requests so we need to specify the exact behavior we want. Any previous behavior of this type was purely accidental.

The definition of this field in Facebook's API is that it should be a comma-separated field:

https://developers.facebook.com/docs/graph-api/using-graph-api
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment