improve interpolation of params in ngRoute #8200

avdv · 2014-07-15T11:30:18Z

Request Type: bug

How to reproduce: 6f77610

create a route with optional parameters, eg. $routeProvider.when('/profile/:userId?', ...)
angular will add the automatic redirect with or without the trailing /, in this case '/profile/userId?/'
point your browser to the redirect http://localhost/#!/profile/0/, you'll see the encoded ? (%3F) at the end of the url: http://localhost/#!/profile/0%3F

Component(s): ngRoute

Impact: medium

Complexity: small

This issue is related to:

Detailed Description:

In line with #5746, this changes how parameters are interpolated in redirectTo strings.

First, I added a test case (it's my first in Javascript ever, so bear with me) which exhibits the failure:

        Expected '/bar/id1/subid3?/23' to equal '/bar/id1/subid3/23'.
        Error: Expected '/bar/id1/subid3?/23' to equal '/bar/id1/subid3/23'.
            at null.<anonymous> (/home/vtc/src/angular.js/test/ngRoute/routeSpec.js:826:34)
            at Object.invoke (/home/vtc/src/angular.js/src/auto/injector.js:801:17)
            at workFn (/home/vtc/src/angular.js/src/ngMock/angular-mocks.js:2187:20)
            at window.inject.angular.mock.inject (/home/vtc/src/angular.js/src/ngMock/angular-mocks.js:2159:42)
            at null.<anonymous> (/home/vtc/src/angular.js/test/ngRoute/routeSpec.js:822:7)
        Expected '/bar/id1/?/23' to equal '/bar/id1/23'.
        Error: Expected '/bar/id1/?/23' to equal '/bar/id1/23'.
            at null.<anonymous> (/home/vtc/src/angular.js/test/ngRoute/routeSpec.js:832:34)
            at Object.invoke (/home/vtc/src/angular.js/src/auto/injector.js:801:17)
            at workFn (/home/vtc/src/angular.js/src/ngMock/angular-mocks.js:2187:20)
            at window.inject.angular.mock.inject (/home/vtc/src/angular.js/src/ngMock/angular-mocks.js:2159:42)
            at null.<anonymous> (/home/vtc/src/angular.js/test/ngRoute/routeSpec.js:822:7)
        Expected '/path/foovalue/barvalue*/1' to equal '/path/foovalue/barvalue/1'.
        Error: Expected '/path/foovalue/barvalue*/1' to equal '/path/foovalue/barvalue/1'.
            at null.<anonymous> (/home/vtc/src/angular.js/test/ngRoute/routeSpec.js:837:34)
            at Object.invoke (/home/vtc/src/angular.js/src/auto/injector.js:801:17)
            at workFn (/home/vtc/src/angular.js/src/ngMock/angular-mocks.js:2187:20)
            at window.inject.angular.mock.inject (/home/vtc/src/angular.js/src/ngMock/angular-mocks.js:2159:42)
            at null.<anonymous> (/home/vtc/src/angular.js/test/ngRoute/routeSpec.js:822:7)

Alas, the fix from Pasvaz in #5746 lead to an error too:

        Expected '/bar/id1//23' to equal '/bar/id1/23'.
        Error: Expected '/bar/id1//23' to equal '/bar/id1/23'.
            at null.<anonymous> (/home/vtc/src/angular.js/test/ngRoute/routeSpec.js:832:34)
            at Object.invoke (/home/vtc/src/angular.js/src/auto/injector.js:801:17)
            at workFn (/home/vtc/src/angular.js/src/ngMock/angular-mocks.js:2187:20)
            at window.inject.angular.mock.inject (/home/vtc/src/angular.js/src/ngMock/angular-mocks.js:2159:42)
            at null.<anonymous> (/home/vtc/src/angular.js/test/ngRoute/routeSpec.js:822:7)l

Note the doubled slash, which is kind of awkward. So, a slash should be removed when the optional parameter is missing.

Now string interpolation is done similar to the generation of the regexp in route, using a replace function.

All good, tests pass.

Other Comments:

I don't know what your preference is, should I reorder the commits in order to keep the tests passing for every single commit?

mary-poppins · 2014-07-15T11:30:21Z

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

Uses the issue template (#8200)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

Narretz · 2014-07-15T14:33:55Z

How to reproduce points to a commit, is this intentional?

avdv · 2014-07-15T20:43:47Z

How to reproduce points to a commit, is this intentional?

I'm not sure what you're asking here. Are you talking about the commit reference I gave? What did you expect in place of it?

Basically, you just check out that commit as usual git checkout 6f77610 if you want to reproduce it.

caitp · 2014-07-15T20:47:26Z

@avdv I think what he's saying is, please provide some actual steps to reproduce this (IE, code to run). Posting a revision to test is helpful, but we need more than that.

edit: I see there are instructions, but a minimal code example on plnkr or heroku would be better

Narretz · 2014-07-15T20:47:55Z

Oh, that's what you mean @avdv .As a reproduction I understand a runnable example, such as in a plnkr.co or maybe even a git repo to check out.

avdv · 2014-07-15T21:20:33Z

Got it. If you want that I could probably do it tomorrow...

avdv · 2014-07-16T06:22:48Z

Here you go: http://plnkr.co/edit/gO7kz2UICfErehcFPhOu?p=preview

click on the "Test 1" link
look at the page route parameter, it's value ends with an ?
click on the "Test 2" link
look at the chapterId parameter, it's value ends with an *

loddit · 2014-07-29T02:53:34Z

I have this bug too.
how to reproduce?

create a route with optional parameters, e.g. when(/:options?)
go to a matched url ,then angular works well.
refresh this page, you can see url change at address bar. first add a tail slash , second slash become to "%3f" ,and refresh again , add one more '%3f' => /path%3f%3f

caitp · 2014-08-01T14:24:01Z

Interesting --- this might be a regression, because this is documented as a feature which should work. Surprised tests aren't broken because of it. (I haven't looked too closely at the repro yet, it could be that something is just wrong there) but so far this doesn't look unreasonable.

caitp · 2014-08-01T15:25:39Z

test/ngRoute/routeSpec.js

@@ -813,6 +813,32 @@ describe('$route', function() {
    });


+    it('should interpolate optional and long route vars in the redirected path from original path', function() {


what the heck does "long route vars" mean here?

what the heck does "long route vars" mean here?

I mean those vars with an asterisk at the end. How do you call them? Greedy route vars?

May be it's a bit confusing that the documentation calls those things "named groups", but throughout the code the term "route params" as well as "route vars" is used?

The test case for that feature calls it a "catch-all" parameter, but the documentation refers to it as "eagerly matches all characters" (whereas without the '*', we match until the next slash).

caitp · 2014-08-01T15:29:29Z

So, if you could address those comments, that would be helpful -- if someone else says this looks good (with comments addressed) I think we can merge it

avdv · 2014-08-02T11:35:29Z

I have this bug too.
how to reproduce?

create a route with optional parameters, e.g. when(/:options?)
go to a matched url ,then angular works well.
refresh this page, you can see url change at address bar. first add a tail slash , second slash become to "%3f" ,and refresh again , add one more '%3f' => /path%3f%3f

Have you tried this patch and did it fix it this problem for you?

loddit · 2014-08-04T03:56:50Z

@avdv yes, it`s fix this problem.
I pull your branch, grunt buld , and copy angular-route.js to bower_components folder of my app.

I`m not sure about this pull request. the new spec looks like about path resolution.

I just fix this bug ,to avoid route tread the "?" after ":option" as a "/.*/" https://gist.github.com/loddit/2e97ea12a32f12d7427f

caitp · 2014-08-04T04:09:59Z

test/ngRoute/routeSpec.js

+        $rootScope.$digest();
+
+        expect($location.path()).toEqual('/bar/id1/23');
+        expect($location.search()).toEqual({extraId: 'gah'});


should test that $route.current.params equal what we expect, not just $location.search() (for each of these cases).

caitp · 2014-08-04T04:15:45Z

I do expect this change to be a little bit slower than the current implementation, so it would be good to consider alternative ways of implementing it. But the test case is good stuff, just needs to be broken into smaller pieces.

avdv · 2014-08-04T20:35:16Z

@caitp Thanks for your review. I think I (hopefully) addressed all of your complains.

\edit: seems the Travis build is a bit flaky at times. Is there some way to trigger a re-run?

Make sure that :name? and :name* vars are properly interpolated in redirectUrl.

When the redirect path contains optional or special parameters the interpolation function misses to remove them. This is also the case for the auto-generated redirection where a slash is appended or removed to a route. Change the way parameters are interpolated in a string. Remove undefined optional named groups and properly prepend a slash in case the named group was starting with one. Related to angular#5746.

avdv · 2014-08-20T15:34:55Z

I did just rebase to upstream master which triggered a new Travis CI build.

Any comments on the current patch set?

vladimir-vg · 2014-10-06T09:06:11Z

This bug also causes problems when using recently added updateParams to $route: #8358

Getting %3F for /foo/:bar? like routes.

Will it be merged to 1.3.0 ?

Problem already fixed in angular/angular.js#8200 , but still doesn't merged to upstream. This commit fixes bug in bower package for deployment-only. Hope fix mentioned above will be merged soon, and this repo will be purged.

Narretz · 2014-10-22T12:21:38Z

Easier approach that's probably not working (#5746 (comment)) as-is here #5746

Narretz · 2015-02-08T18:27:55Z

This was fixed by 891acf4 back in November

avdv mentioned this pull request Jul 15, 2014

fix(ngRoute): wrong redirect to paths containing optional or special parameters #5746

Closed

avdv added cla: yes and removed cla: no labels Jul 15, 2014

Narretz added component: ngRoute labels Jul 15, 2014

Narretz added this to the Backlog milestone Jul 15, 2014

caitp reviewed Aug 1, 2014
View reviewed changes

caitp reviewed Aug 4, 2014
View reviewed changes

avdv added 2 commits August 20, 2014 17:11

test(ngRoute): add test case for special params in redirectUrl

d968ec7

Make sure that :name? and :name* vars are properly interpolated in redirectUrl.

avdv force-pushed the ngRoute-improve-interpolation branch from 9fcdb38 to 0336d57 Compare August 20, 2014 15:14

btford removed the gh: PR label Aug 20, 2014

caitp force-pushed the master branch from 591bac5 to d713ad1 Compare August 22, 2014 13:51

tbosch force-pushed the master branch from 6c27b89 to 271572c Compare August 30, 2014 00:57

tbosch force-pushed the master branch from a3d7934 to 1418383 Compare September 10, 2014 18:14

petebacondarwin force-pushed the master branch from 02dc2aa to fd2d6c0 Compare September 16, 2014 11:26

jeffbcross force-pushed the master branch 2 times, most recently from cad9560 to f294244 Compare October 2, 2014 22:09

btford force-pushed the master branch from 9011a65 to 46db47b Compare October 6, 2014 22:54

jeffbcross force-pushed the master branch 2 times, most recently from abdaab7 to 30996f8 Compare October 8, 2014 19:46

jeffbcross mentioned this pull request Oct 9, 2014

ngRoute 1.3.0 $route.updateParams issue #9471

Closed

jeffbcross force-pushed the master branch 2 times, most recently from e8dc429 to e83fab9 Compare October 10, 2014 17:37

petebacondarwin force-pushed the master branch from 4dd5a20 to 998c61c Compare October 19, 2014 10:57

Narretz modified the milestones: 1.3.x, Backlog Oct 22, 2014

Narretz mentioned this pull request Oct 22, 2014

$routeProvider - optional named group + trailing slash #9742

Closed

gkalpak mentioned this pull request Oct 31, 2014

ngRoute interpolate route with optional parameters #9819

Closed

gkalpak mentioned this pull request Nov 10, 2014

fix($route): fix redirection with optional/eager params #9827

Closed

Narretz self-assigned this Jan 20, 2015

Narretz closed this Feb 8, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve interpolation of params in ngRoute #8200

improve interpolation of params in ngRoute #8200

avdv commented Jul 15, 2014

mary-poppins commented Jul 15, 2014

Narretz commented Jul 15, 2014

avdv commented Jul 15, 2014

caitp commented Jul 15, 2014

Narretz commented Jul 15, 2014

avdv commented Jul 15, 2014

avdv commented Jul 16, 2014

loddit commented Jul 29, 2014

caitp commented Aug 1, 2014

caitp Aug 1, 2014

avdv Aug 2, 2014

caitp Aug 4, 2014

caitp commented Aug 1, 2014

avdv commented Aug 2, 2014

loddit commented Aug 4, 2014

caitp Aug 4, 2014

caitp commented Aug 4, 2014

avdv commented Aug 4, 2014

avdv commented Aug 20, 2014

vladimir-vg commented Oct 6, 2014

Narretz commented Oct 22, 2014

Narretz commented Feb 8, 2015

		@@ -813,6 +813,32 @@ describe('$route', function() {
		});


		it('should interpolate optional and long route vars in the redirected path from original path', function() {

improve interpolation of params in ngRoute #8200

improve interpolation of params in ngRoute #8200

Conversation

avdv commented Jul 15, 2014

mary-poppins commented Jul 15, 2014

Narretz commented Jul 15, 2014

avdv commented Jul 15, 2014

caitp commented Jul 15, 2014

Narretz commented Jul 15, 2014

avdv commented Jul 15, 2014

avdv commented Jul 16, 2014

loddit commented Jul 29, 2014

caitp commented Aug 1, 2014

caitp Aug 1, 2014

Choose a reason for hiding this comment

avdv Aug 2, 2014

Choose a reason for hiding this comment

caitp Aug 4, 2014

Choose a reason for hiding this comment

caitp commented Aug 1, 2014

avdv commented Aug 2, 2014

loddit commented Aug 4, 2014

caitp Aug 4, 2014

Choose a reason for hiding this comment

caitp commented Aug 4, 2014

avdv commented Aug 4, 2014

avdv commented Aug 20, 2014

vladimir-vg commented Oct 6, 2014

Narretz commented Oct 22, 2014

Narretz commented Feb 8, 2015