Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

Commit

Permalink
fix($http): config.param should expand array values properly
Browse files Browse the repository at this point in the history
Today, calling e.g. $http(url, { params: { a: [1,2,3] } }) results in a query
string like "?a=%5B1%2C2%2C3%5D" which is undesirable. This commit enhances
buildURL to createa query string like "?a=1&a=2&a=3".

BREAKING CHANGE: if the server relied on the buggy behavior then either the
backend should be fixed or a simple serialization of the array should be done
on the client before calling the $http service.

Closes #1363
  • Loading branch information
tdavis authored and IgorMinar committed Nov 24, 2012
1 parent 610927d commit 79af2ba
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 4 deletions.
13 changes: 9 additions & 4 deletions src/ng/http.js
Original file line number Diff line number Diff line change
Expand Up @@ -758,10 +758,15 @@ function $HttpProvider() {
var parts = [];
forEachSorted(params, function(value, key) {
if (value == null || value == undefined) return;
if (isObject(value)) {
value = toJson(value);
}
parts.push(encodeURIComponent(key) + '=' + encodeURIComponent(value));
if (!isArray(value)) value = [value];

forEach(value, function(v) {
if (isObject(v)) {
v = toJson(v);
}
parts.push(encodeURIComponent(key) + '=' +
encodeURIComponent(v));
});
});
return url + ((url.indexOf('?') == -1) ? '?' : '&') + parts.join('&');
}
Expand Down
6 changes: 6 additions & 0 deletions test/ng/httpSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,12 @@ describe('$http', function() {
$httpBackend.expect('GET', '/url?a=1&b=%7B%22c%22%3A3%7D').respond('');
$http({url: '/url', params: {a:1, b:{c:3}}, method: 'GET'});
}));


it('should expand arrays in params map', inject(function($httpBackend, $http) {
$httpBackend.expect('GET', '/url?a=1&a=2&a=3').respond('');
$http({url: '/url', params: {a: [1,2,3]}, method: 'GET'});
}));
});


Expand Down

5 comments on commit 79af2ba

@fcoury
Copy link

@fcoury fcoury commented on 79af2ba Dec 30, 2012

Choose a reason for hiding this comment

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

Why did you decided not to follow jQuery's $.param logic for this parsing? Backends like Rails expect keys multiple values to end with [] to indicate it's an array and jQuery $.param does it correctly.

Better yet, did you consider exposing an overridable default in $httpProvider.defaults, in a way that the user of the library could override this behavior depending on how the backend expects it to be handled? Would that be a good addition?

@tdavis
Copy link
Contributor Author

@tdavis tdavis commented on 79af2ba Jan 18, 2013

Choose a reason for hiding this comment

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

Why did you decided not to follow jQuery's $.param logic for this parsing? Backends like Rails expect keys multiple values to end with [] to indicate it's an array and jQuery $.param does it correctly.

Largely because the spec already says I can have multiple query arguments with the same name and those should result in multiple values downstream. Proprietary extensions on top of that to make query argument parsing easier or more obvious as to the value type or whatever is a bad default. If you want a[] as your array (in this example) then simply pass { 'a[]': [1, 2, 3] }.

@fcoury
Copy link

@fcoury fcoury commented on 79af2ba Jan 22, 2013

Choose a reason for hiding this comment

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

Thanks for the explanation @tdavis

@kirel
Copy link

@kirel kirel commented on 79af2ba Apr 30, 2013

Choose a reason for hiding this comment

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

Better yet, did you consider exposing an overridable default in $httpProvider.defaults, in a way that the user of the library could override > this behavior depending on how the backend expects it to be handled? Would that be a good addition?

This is what I am looking for. Anything underway? Otherwise I'll have a try as soon as I find the time.

@kencaron
Copy link

Choose a reason for hiding this comment

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

Sorry if this is the wrong place to ask, but I was just curious when (roughly) this might make it's way into the stable release? New to angular and just as unfamiliar with the dev cycle. I tried this on stable 1.0.7 and this fix didn't seem to be implemented. Testing on 1.1.5 however, worked like a charm.

Please sign in to comment.