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

fix($resource): params should expand array values properly #1921

Closed
wants to merge 1 commit into from

Conversation

marknadig
Copy link
Contributor

Today, calling e.g. var R = $resource('/Path/:a'); R.get({a: 'foo', bar: ['baz1', 'baz2']}); results in a query
string like "/Path/doh?bar=baz1,baz2" which is undesirable. This commit enhances resource to use
$http to encode any non-url parameters resulting in a query string like "/Path/doh?bar=baz1&bar=baz2".

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 resource service.

Today, calling e.g.  var R = $resource('/Path/:a'); R.get({a: 'foo', bar: ['baz1', 'baz2']}); results in a query
string like "/Path/doh?bar=baz1,baz2" which is undesirable. This commit enhances resource to use
$http to encode any non-url parameters resulting in a query string like "/Path/doh?bar=baz1&bar=baz2".

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 resource service.
@marknadig
Copy link
Contributor Author

@IgorMinar there is a secnario in resourceSpec 'should not encode @ in url params' that I commented out. Because resource now defers to $http to encode any non-url params, and $http doesn't have the encodeUriSegment logic in it. So, my question is: should the special encodeUriSegment logic be in $http, or is the special encoding not really needed, or am I missing some other aspect?

@marknadig
Copy link
Contributor Author

related PR #1640

@vojtajina
Copy link
Contributor

With regards to "should not encode @", yep this should be tested on $http level (but no reason to removing the test for $resource, we can have them both): https://github.com/angular/angular.js/blob/master/src/ng/http.js#L810

@marknadig
Copy link
Contributor Author

@vojtajina thanks for the reply. Since the problem w/ http encoding "@" exists already in $http - would you be ok with merging this PR in and creating a sep issue to resolve that in $http?

@vojtajina
Copy link
Contributor

I merged your PR as 2a21234 (with subtle tweaks) and submitted one more commit (fixing the $http to not encode these special characters) - 288b69a.

Thanks for help!

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

Successfully merging this pull request may close these issues.

2 participants