fix(ngMock/$httpBackend): correctly ignore query params in {expect,when}Route

Previously, a route definition such as
`$httpBackend.whenRoute('GET', '/route/:id')` matched against a URL with
query params, for example `/route/1?q=foo`, would incorrectly include
the query params in `id`: `{id: '1?q=foo', q: 'foo'}`.

This commit fixes it, so that the extracted `params` will now be:
`{id: '1', q: 'foo'}`.

Fixes #14173

Closes #16589

Author: thorn0

angularFiles.js

@@ -131,6 +131,7 @@ var angularFiles = {
     ],
     'ngRoute': [
       'src/shallowCopy.js',
+      'src/routeToRegExp.js',
       'src/ngRoute/route.js',
       'src/ngRoute/routeParams.js',
       'src/ngRoute/directive/ngView.js'
@@ -140,6 +141,7 @@ var angularFiles = {
       'src/ngSanitize/filter/linky.js'
     ],
     'ngMock': [
+      'src/routeToRegExp.js',
       'src/ngMock/angular-mocks.js',
       'src/ngMock/browserTrigger.js'
     ],

src/ngMock/angular-mocks.js

@@ -1,5 +1,7 @@
 'use strict';
 
+/* global routeToRegExp: false */
+
 /**
  * @ngdoc object
  * @name angular.mock
@@ -1282,7 +1284,7 @@ angular.mock.dump = function(object) {
  * ## Matching route requests
  *
  * For extra convenience, `whenRoute` and `expectRoute` shortcuts are available. These methods offer colon
- * delimited matching of the url path, ignoring the query string. This allows declarations
+ * delimited matching of the url path, ignoring the query string and trailing slashes. This allows declarations
  * similar to how application routes are configured with `$routeProvider`. Because these methods convert
  * the definition url to regex, declaration order is important. Combined with query parameter parsing,
  * the following is possible:
@@ -1481,8 +1483,9 @@ function createHttpBackendMock($rootScope, $timeout, $delegate, $browser) {
    *      ```
    *    – The respond method takes a set of static data to be returned or a function that can
    *    return an array containing response status (number), response data (Array|Object|string),
-   *    response headers (Object), and the text for the status (string). The respond method returns
-   *    the `requestHandler` object for possible overrides.
+   *    response headers (Object), HTTP status text (string), and XMLHttpRequest status (string:
+   *    `complete`, `error`, `timeout` or `abort`). The respond method returns the `requestHandler`
+   *    object for possible overrides.
    */
   $httpBackend.when = function(method, url, data, headers, keys) {
 
@@ -1663,40 +1666,10 @@ function createHttpBackendMock($rootScope, $timeout, $delegate, $browser) {
    * See {@link ngMock.$httpBackend#when `when`} for more info.
    */
   $httpBackend.whenRoute = function(method, url) {
-    var pathObj = parseRoute(url);
+    var pathObj = routeToRegExp(url, {caseInsensitiveMatch: true, ignoreTrailingSlashes: true});
     return $httpBackend.when(method, pathObj.regexp, undefined, undefined, pathObj.keys);
   };
 
-  function parseRoute(url) {
-    var ret = {
-      regexp: url
-    },
-    keys = ret.keys = [];
-
-    if (!url || !angular.isString(url)) return ret;
-
-    url = url
-      .replace(/([().])/g, '\\$1')
-      .replace(/(\/)?:(\w+)([?*])?/g, function(_, slash, key, option) {
-        var optional = option === '?' ? option : null;
-        var star = option === '*' ? option : null;
-        keys.push({ name: key, optional: !!optional });
-        slash = slash || '';
-        return ''
-          + (optional ? '' : slash)
-          + '(?:'
-          + (optional ? slash : '')
-          + (star && '(.+?)' || '([^/]+)')
-          + (optional || '')
-          + ')'
-          + (optional || '');
-      })
-      .replace(/([/$*])/g, '\\$1');
-
-    ret.regexp = new RegExp('^' + url, 'i');
-    return ret;
-  }
-
   /**
    * @ngdoc method
    * @name $httpBackend#expect
@@ -1717,14 +1690,15 @@ function createHttpBackendMock($rootScope, $timeout, $delegate, $browser) {
    *  order to change how a matched request is handled.
    *
    *  - respond –
-   *    ```
-   *    { function([status,] data[, headers, statusText])
-   *    | function(function(method, url, data, headers, params)}
-   *    ```
+   *      ```js
+   *      {function([status,] data[, headers, statusText])
+   *      | function(function(method, url, data, headers, params)}
+   *      ```
    *    – The respond method takes a set of static data to be returned or a function that can
    *    return an array containing response status (number), response data (Array|Object|string),
-   *    response headers (Object), and the text for the status (string). The respond method returns
-   *    the `requestHandler` object for possible overrides.
+   *    response headers (Object), HTTP status text (string), and XMLHttpRequest status (string:
+   *    `complete`, `error`, `timeout` or `abort`). The respond method returns the `requestHandler`
+   *    object for possible overrides.
    */
   $httpBackend.expect = function(method, url, data, headers, keys) {
 
@@ -1876,7 +1850,7 @@ function createHttpBackendMock($rootScope, $timeout, $delegate, $browser) {
    * See {@link ngMock.$httpBackend#expect `expect`} for more info.
    */
   $httpBackend.expectRoute = function(method, url) {
-    var pathObj = parseRoute(url);
+    var pathObj = routeToRegExp(url, {caseInsensitiveMatch: true, ignoreTrailingSlashes: true});
     return $httpBackend.expect(method, pathObj.regexp, undefined, undefined, pathObj.keys);
   };
 

src/ngRoute/route.js

@@ -1,5 +1,6 @@
 'use strict';
 
+/* global routeToRegExp: false */
 /* global shallowCopy: false */
 
 // `isArray` and `isObject` are necessary for `shallowCopy()` (included via `src/shallowCopy.js`).
@@ -224,7 +225,7 @@ function $RouteProvider() {
     }
     routes[path] = angular.extend(
       routeCopy,
-      path && pathRegExp(path, routeCopy)
+      path && routeToRegExp(path, routeCopy)
     );
 
     // create redirection for trailing slashes
@@ -235,7 +236,7 @@ function $RouteProvider() {
 
       routes[redirectPath] = angular.extend(
         {redirectTo: path},
-        pathRegExp(redirectPath, routeCopy)
+        routeToRegExp(redirectPath, routeCopy)
       );
     }
 
@@ -253,47 +254,6 @@ function $RouteProvider() {
    */
   this.caseInsensitiveMatch = false;
 
-   /**
-    * @param path {string} path
-    * @param opts {Object} options
-    * @return {?Object}
-    *
-    * @description
-    * Normalizes the given path, returning a regular expression
-    * and the original path.
-    *
-    * Inspired by pathRexp in visionmedia/express/lib/utils.js.
-    */
-  function pathRegExp(path, opts) {
-    var insensitive = opts.caseInsensitiveMatch,
-        ret = {
-          originalPath: path,
-          regexp: path
-        },
-        keys = ret.keys = [];
-
-    path = path
-      .replace(/([().])/g, '\\$1')
-      .replace(/(\/)?:(\w+)(\*\?|[?*])?/g, function(_, slash, key, option) {
-        var optional = (option === '?' || option === '*?') ? '?' : null;
-        var star = (option === '*' || option === '*?') ? '*' : null;
-        keys.push({ name: key, optional: !!optional });
-        slash = slash || '';
-        return ''
-          + (optional ? '' : slash)
-          + '(?:'
-          + (optional ? slash : '')
-          + (star && '(.+?)' || '([^/]+)')
-          + (optional || '')
-          + ')'
-          + (optional || '');
-      })
-      .replace(/([/$*])/g, '\\$1');
-
-    ret.regexp = new RegExp('^' + path + '$', insensitive ? 'i' : '');
-    return ret;
-  }
-
   /**
    * @ngdoc method
    * @name $routeProvider#otherwise

src/routeToRegExp.js

@@ -0,0 +1,46 @@
+'use strict';
+
+/* global routeToRegExp: true */
+
+/**
+ * @param path {string} path
+ * @param opts {Object} options
+ * @return {?Object}
+ *
+ * @description
+ * Normalizes the given path, returning a regular expression
+ * and the original path.
+ *
+ * Inspired by pathRexp in visionmedia/express/lib/utils.js.
+ */
+function routeToRegExp(path, opts) {
+  var keys = [];
+
+  var pattern = path
+    .replace(/([().])/g, '\\$1')
+    .replace(/(\/)?:(\w+)(\*\?|[?*])?/g, function(_, slash, key, option) {
+      var optional = option === '?' || option === '*?';
+      var star = option === '*' || option === '*?';
+      keys.push({ name: key, optional: optional });
+      slash = slash || '';
+      return (
+        (optional ? '(?:' + slash : slash + '(?:') +
+        (star ? '([^?#]+?)' : '([^/?#]+)') +
+        (optional ? '?)?' : ')')
+      );
+    })
+    .replace(/([/$*])/g, '\\$1');
+
+  if (opts.ignoreTrailingSlashes) {
+    pattern = pattern.replace(/\/+$/, '') + '/*';
+  }
+
+  return {
+    originalPath: path,
+    keys: keys,
+    regexp: new RegExp(
+      '^' + pattern + '(?:[?#]|$)',
+      opts.caseInsensitiveMatch ? 'i' : ''
+    )
+  };
+}

test/ngMock/angular-mocksSpec.js

@@ -1941,12 +1941,36 @@ describe('ngMock', function() {
             expect(callback).toHaveBeenCalledOnceWith(200, 'path', '', '', 'complete');
           }
         );
-        they('should ignore query param when matching in ' + routeShortcut + ' $prop method', methods,
+        they('should ignore query params when matching in ' + routeShortcut + ' $prop method', methods,
           function() {
-            hb[routeShortcut](this, '/route/:id').respond('path');
-            hb(this, '/route/123?q=str&foo=bar', undefined, callback);
-            hb.flush();
-            expect(callback).toHaveBeenCalledOnceWith(200, 'path', '', '', 'complete');
+            angular.forEach([
+              {route: '/route1/:id', url: '/route1/Alpha', expectedParams: {id: 'Alpha'}},
+              {route: '/route2/:id', url: '/route2/Bravo/?', expectedParams: {id: 'Bravo'}},
+              {route: '/route3/:id', url: '/route3/Charlie?q=str&foo=bar', expectedParams: {id: 'Charlie', q: 'str', foo: 'bar'}},
+              {route: '/:x/route4', url: '/Delta/route4?q=str&foo=bar', expectedParams: {x: 'Delta', q: 'str', foo: 'bar'}},
+              {route: '/route5/:id*', url: '/route5/Echo/456?q=str&foo=bar', expectedParams: {id: 'Echo/456', q: 'str', foo: 'bar'}},
+              {route: '/route6/:id*', url: '/route6/Foxtrot/456/?q=str&foo=bar', expectedParams: {id: 'Foxtrot/456', q: 'str', foo: 'bar'}},
+              {route: '/route7/:id*', url: '/route7/Golf/456//?q=str&foo=bar', expectedParams: {id: 'Golf/456', q: 'str', foo: 'bar'}},
+              {route: '/:x*/route8', url: '/Hotel/123/456/route8/?q=str&foo=bar', expectedParams: {x: 'Hotel/123/456', q: 'str', foo: 'bar'}},
+              {route: '/:x*/route9/:id', url: '/India/456/route9/0?q=str&foo=bar', expectedParams: {x: 'India/456', id: '0', q: 'str', foo: 'bar'}},
+              {route: '/route10', url: '/route10?q=Juliet&foo=bar', expectedParams: {q: 'Juliet', foo: 'bar'}},
+              {route: '/route11', url: '/route11///?q=Kilo', expectedParams: {q: 'Kilo'}},
+              {route: '/route12', url: '/route12///', expectedParams: {}}
+            ], function(testDataEntry) {
+              callback.calls.reset();
+              var paramsSpy = jasmine.createSpy('params');
+              hb[routeShortcut](this, testDataEntry.route).respond(
+                function(method, url, data, headers, params) {
+                  paramsSpy(params);
+                  // status, response, headers, statusText, xhrStatus
+                  return [200, 'path', { 'x-header': 'foo' }, 'OK', 'complete'];
+                }
+              );
+              hb(this, testDataEntry.url, undefined, callback);
+              hb.flush();
+              expect(callback).toHaveBeenCalledOnceWith(200, 'path', 'x-header: foo', 'OK', 'complete');
+              expect(paramsSpy).toHaveBeenCalledOnceWith(testDataEntry.expectedParams);
+            });
           }
         );
       });