fix(ngResource): fix query url params encoding

[alediaferia]

Encoding URL Query Parameters should be "more aggressive" than the URL Segment encoding.
You can see what I mean in the added test case.

I'm currently using AngularJS 1.3.16 but I found this bug is still there in master.

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please let us know the company's name.

[alediaferia]

I signed it!

[googlebot]

CLAs look good, thanks!

[nacmacfeegle]

+1 to this fix, thanks @alediaferia

[gkalpak]

You need the '(\\W|$)' part or else you might get false positives. (Better yet '(?:\\W|$)'.)
E.g. param: 'foo' and url: '/path/foo?key=:foobar' will lead to a false positive.

[alediaferia]

I agree my regex is too broad but I'm not sure I completely get the one proposed by you.
Would something like the following be more accurate?
new RegExp("\\?((\\w+)\\=\\w+&)*\\w+=:" + param + "(\\W|$)").test(url)

[gkalpak]

No need to make it too strict either :)

IMO, new RegExp('\\?(.*):' + param + '(\\W|$)') (or new RegExp('\\?(?:.*):' + param + '(?:\\W|$)')) is fine.

[alediaferia]

So what does ?: match exactly? This is the only bit I don't understand. :)

[alediaferia]

Ooh, it's the non-capturing group! :)

[alediaferia]

I've updated the code accordingly and split my commit messages to better comply with contribution guidelines.

[gkalpak]

Nitpick: You don't need \\ before : (it's not a special character in this context).

[gkalpak]

LGTM

[davidesantangelo]

LGTM

[alediaferia]

Is there a plan for when this will go in the main line?

[alediaferia]

Bump!

[gkalpak]

I'd prefer a more descriptive testcase name (e.g. should encode & in query params unless in query param value or something).

[gkalpak]

@alediaferia, thx for the update. I left a couple of comments (mostly minor).
Feel free to ping me once you've pushed changes, so I can take another look.

[alediaferia]

@gkalpak BTW do you have any hint on why now my grunt package fails with stderr maxBuffer exceeded ?

[gkalpak]

@alediaferia, no idea :) Does it happen with the code currently on the PR ?
(If not, you can push the code somewhere so I can take a look.)

[petebacondarwin]

Sounds like something to do with the closure compiler?

[alediaferia]

@gkalpak @petebacondarwin sorry for not getting back earlier. It's happening with the current PR but it's happening with updated master as well. It may be something wrong with my npm/node configuration, but not sure. I'm not much into JavaScript :)

[gkalpak]

@alediaferia, (un)fortunately, I'm not getting any issues with grunt package 😞

[alediaferia]

I'm wondering if part of this test is a duplicate of the one preceding this test.

'should encode & in url params'

[gkalpak]

I would say close but not quite.
I would be happy to have them merged into one it block (keeping all 3 cases).

Feel free to merge them.

[alediaferia]

@gkalpak ping

[gkalpak]

I guess this has to do with your grunt package issues, but it definitely does not belong in this PR :)

[alediaferia]

Woops! sorry :)

[gkalpak]

LGTM, thx @alediaferia !

/cc @petebacondarwin

[alediaferia]

If this is ok, shall I clean up the git history squashing the latest commits and following your commit message conventions?

[petebacondarwin]

OK, LGTM. @alediaferia thanks for your work on this. If you want to squash and tidy up the commit message that would be great. Otherwise, perhaps @gkalpak could do it when merging?

[gkalpak]

Sure, I'll take care of squashing and merging !

[alediaferia]

Thank you @gkalpak @petebacondarwin

[gkalpak]

@petebacondarwin, should this be backported to 1.4.x ?

[petebacondarwin]

If you are confident it is not a breaking change.

[gkalpak]

It's one of those times that I find it hard to distinguish between a fix and a BC.

The only change introduced is when a param (:xyz) is used as the value part of a URL query key-value pair. In any other case, there is no difference. But it could still break an app (theoretically).

E.g., assuming $resource('/path?key=:val').get({val: 'foo&bar'}), the request is sent to this URL:

1.4.x: /path?key=foo&bar
1.5.x: /path?key=foo%26bar

[alediaferia]

Although I feel the "new" behaviour is the right one I unfortunately think there may be a lot of apps out there relying on a workaround for this "issue".

If you look for e.g. "angularjs ngresource query parameters" on Google there are some issues open on SO with a few workarounds that usually involve using $http directly.

Just my 2 cents.

Happy to have contributed to such a big project anyway :)

[petebacondarwin]

OK, so no backport.

[gkalpak]

@alediaferia, I couldn't find any SO questions/answers that would be broken by this "fix". Relying on a workaround isn't a problem (it won't get broken).
Do you have any specific SO question/answer in mind ?

The problem is if someone has as a URL template such as /path?key=:val and expects to set other query param via :val (e.g. /path?key=1&key2=2&key3=3). It is a weird usecase imo (but theoretically possible nontheless).

[alediaferia]

@gkalpak I cannot manage to find the question I had in mind, strange. Anyway, as you point out, relying on a workaround won't make things break. That's true. I don't have further objections.