fix($route): correctly extract path params if path contains question mark or hash #16672

gkalpak · 2018-08-21T12:35:01Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix (and refactorings).

What is the current behavior? (You can also link to an open issue here)
The routeToRegExp() function, introduced by 840b5f0, cannot extract path params if the path contains question mark or hash. Although these characters would normally be encoded in the path, they are decoded by $location.path(), before being passed to the RegExp returned by routeToRegExp().

routeToRegExp() has to be able to deal with both encoded URL and decoded path, because it is being shared between ngRoute and ngMocks.

What is the new behavior (if this is a feature change)?
The RegExp returned by routeToRegExp() only deals with the path part and the callers are responsible for stripping off the query and hash parts (if any).

(This PR contains the code from #16670, plus some refactoring to simplify the implementation).

Does this PR introduce a breaking change?
No.

Please check if the PR fulfills these requirements

The commit message follows our guidelines
Fix/Feature: Docs have been added/updated
Fix/Feature: Tests have been added; existing tests pass

Other information:
Closes #16670.

googlebot · 2018-08-21T12:35:04Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

googlebot · 2018-08-21T12:35:04Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

Narretz · 2018-08-22T10:00:13Z

test/ngRoute/routeParamsSpec.js

    });
  });

+  it('should correctly extract path params containing hashes and/or question marks', function() {


Should we test for $location.path('/foo/bar?baz#qux');?

And is it intended that the "params" have no values, i.e ?baz=val?

Should we test if multiple params are correctly extracted?

Sure. This is not the primary focus of this test, but I'm never one to shy away from adding more testcases 😛

Narretz · 2018-08-22T10:03:08Z

src/ngMock/angular-mocks.js

    return method + ' ' + url;
  };

  this.params = function(u) {


Should we give the params here (u, m) better names since we are already refactoring?

Suggestions?

u -> url
m -> match

😄

url is taken 😁

What's the difference between url and u anyway?

url is the URL of the definition (when calling $httpBackend.when(method, url)). u is the actual URL that $http was called with and we want to verify whether it matches the definition.
(Also note, url can be a string or a RegExp or a function.)

Pretty big difference then! How about requestUrl instead of u?

I took a different approach and renamed the MockHttpExpectation() arguments to expectedXyz (see eaf7a5db1).
Let me know what you think.

…mark or hash The `routeToRegExp()` function, introduced by 840b5f0, could not extract path params if the path contained question mark or hash. Although these characters would normally be encoded in the path, they are decoded by `$location.path()`, before being passed to the RegExp returned by `routeToRegExp()`. `routeToRegExp()` has to be able to deal with both encoded URL and decoded path, because it is being shared between `ngRoute` and `ngMocks`. This commit fixes the issue, by introducing an `isUrl` option that allows creating an appropriate RegExp for each usecase.

…estion mark or hash

…teToRegExp`

…`MockHttpExpectation`

…ery/hash stripped off

…ery/hash stripped off Closes #16672

googlebot added the cla: no label Aug 21, 2018

gkalpak added this to the 1.7.x milestone Aug 21, 2018

gkalpak added type: bug component: ngRoute frequency: low needs: review severity: regression type: refactor component: ngMock labels Aug 21, 2018

gkalpak mentioned this pull request Aug 21, 2018

fix(ngRoute): extract path params containing hashes and/or question marks correctly #16670

Closed

3 tasks

gkalpak force-pushed the fix-route-path-params branch from 96d522a to 3dc24b2 Compare August 21, 2018 12:54

Narretz reviewed Aug 22, 2018

View reviewed changes

susisu and others added 8 commits August 25, 2018 15:25

fixup! fix($route): correctly extract path params if path contains qu…

afff809

…estion mark or hash

test(ngMocks): use correct method name in $httpBackend test

ff1d24e

refactor(ngRoute): do not unnecessarily return originalPath in `rou…

08c095a

…teToRegExp`

refactor(ngMocks): clean up MockHttpExpectation#params()

af75815

refactor(ngMocks): ignore query/hash when extracting path params for …

db8394b

…`MockHttpExpectation`

refactor(ngMocks): clean up MockHttpExpectation

eaf7a5d

refactor(ngMocks): simplify routeToRegExp by assuming path has qu…

9b9e25f

…ery/hash stripped off

gkalpak force-pushed the fix-route-path-params branch from fd6ff1b to 9b9e25f Compare August 25, 2018 12:29

Narretz approved these changes Aug 25, 2018

View reviewed changes

gkalpak closed this in a553735 Aug 25, 2018

gkalpak deleted the fix-route-path-params branch August 25, 2018 20:44

gkalpak added a commit that referenced this pull request Aug 25, 2018

refactor(ngMocks): simplify routeToRegExp by assuming path has qu…

2bbc7c4

…ery/hash stripped off Closes #16672

fix($route): correctly extract path params if path contains question mark or hash #16672

fix($route): correctly extract path params if path contains question mark or hash #16672

Uh oh!

Conversation

gkalpak commented Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googlebot commented Aug 21, 2018

Uh oh!

googlebot commented Aug 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gkalpak commented Aug 21, 2018 •

edited

Loading