fix(rest): improve route sorting to group by path and verb

loopbackio · Nov 5, 2018 · ce31bf7 · ce31bf7
1 parent 17adc7a
commit ce31bf7
Show file tree

Hide file tree

Showing 3 changed files with 29 additions and 39 deletions.
diff --git a/packages/rest/src/router/route-sort.ts b/packages/rest/src/router/route-sort.ts
@@ -28,41 +28,30 @@ export function compareRoute(
   route1: Pick<RouteEntry, 'verb' | 'path'>,
   route2: Pick<RouteEntry, 'verb' | 'path'>,
 ): number {
-  // First check verb
-  const verb1 = HTTP_VERBS[route1.verb.toLowerCase()] || HTTP_VERBS.get;
-  const verb2 = HTTP_VERBS[route2.verb.toLowerCase()] || HTTP_VERBS.get;
-  if (verb1 !== verb2) return verb1 - verb2;
-
-  // Then check the path tokens
+  // First check the path tokens
   const path1 = route1.path.replace(/{([^}]*)}(\/|$)/g, ':$1$2');
   const path2 = route2.path.replace(/{([^}]*)}(\/|$)/g, ':$1$2');
   const tokensForPath1: pathToRegExp.Token[] = parse(path1);
   const tokensForPath2: pathToRegExp.Token[] = parse(path2);
 
-  // Longer path comes before shorter path
-  if (tokensForPath1.length < tokensForPath2.length) {
-    return 1;
-  } else if (tokensForPath1.length > tokensForPath2.length) {
-    return -1;
-  }
+  const length =
+    tokensForPath1.length > tokensForPath2.length
+      ? tokensForPath1.length
+      : tokensForPath2.length;
 
-  // Now check token by token
-  for (let i = 0; i < tokensForPath1.length; i++) {
+  for (let i = 0; i < length; i++) {
     const token1 = tokensForPath1[i];
     const token2 = tokensForPath2[i];
-    if (typeof token1 === 'string' && typeof token2 === 'string') {
-      if (token1 < token2) return -1;
-      else if (token1 > token2) return 1;
-    } else if (typeof token1 === 'string' && typeof token2 === 'object') {
-      // token 1 is a literal while token 2 is a parameter
-      return -1;
-    } else if (typeof token1 === 'object' && typeof token2 === 'string') {
-      // token 1 is a parameter while token 2 is a literal
-      return 1;
-    } else {
-      // Both token are parameters. Treat as equal weight.
-    }
+    if (token1 === token2) continue;
+    if (token1 === undefined) return 1;
+    if (token2 === undefined) return -1;
+    if (token1 < token2) return -1;
+    if (token1 > token2) return 1;
   }
+  // Then check verb
+  const verb1 = HTTP_VERBS[route1.verb.toLowerCase()] || HTTP_VERBS.get;
+  const verb2 = HTTP_VERBS[route2.verb.toLowerCase()] || HTTP_VERBS.get;
+  if (verb1 !== verb2) return verb1 - verb2;
   return 0;
 }
 
@@ -77,7 +66,8 @@ function parse(path: string) {
       // The string can be /orders/count
       tokens.push(...p.split('/').filter(Boolean));
     } else {
-      tokens.push(p);
+      // Use `{}` for wildcard as they are larger than any other ascii chars
+      tokens.push(`{}`);
     }
   });
   return tokens;

diff --git a/packages/rest/test/unit/router/route-sort.unit.ts b/packages/rest/test/unit/router/route-sort.unit.ts
@@ -14,15 +14,15 @@ describe('route sorter', () => {
       compareRoute(a[1], b[1]),
     );
     expect(sortedEndpoints).to.eql([
-      ['create', {verb: 'post', path: '/orders'}],
+      ['count', {verb: 'get', path: '/orders/count'}],
+      ['exists', {verb: 'get', path: '/orders/{id}/exists'}],
       ['replaceById', {verb: 'put', path: '/orders/{id}'}],
       ['updateById', {verb: 'patch', path: '/orders/{id}'}],
-      ['updateAll', {verb: 'patch', path: '/orders'}],
-      ['exists', {verb: 'get', path: '/orders/{id}/exists'}],
-      ['count', {verb: 'get', path: '/orders/count'}],
       ['findById', {verb: 'get', path: '/orders/{id}'}],
-      ['findAll', {verb: 'get', path: '/orders'}],
       ['deleteById', {verb: 'delete', path: '/orders/{id}'}],
+      ['create', {verb: 'post', path: '/orders'}],
+      ['updateAll', {verb: 'patch', path: '/orders'}],
+      ['findAll', {verb: 'get', path: '/orders'}],
       ['deleteAll', {verb: 'delete', path: '/orders'}],
     ]);
   });

diff --git a/packages/rest/test/unit/router/trie-router.unit.ts b/packages/rest/test/unit/router/trie-router.unit.ts
@@ -56,19 +56,19 @@ describe('trie router', () => {
 
   it('lists routes by order', () => {
     expect(router.list().map(getVerbAndPath)).to.eql([
-      {verb: 'post', path: '/orders'},
+      {verb: 'get', path: '/orders/count'},
+      {verb: 'get', path: '/orders/{id}/exists'},
       {verb: 'put', path: '/orders/{id}'},
       {verb: 'patch', path: '/orders/{id}'},
+      {verb: 'get', path: '/orders/{id}'},
+      {verb: 'delete', path: '/orders/{id}'},
+      {verb: 'post', path: '/orders'},
       {verb: 'patch', path: '/orders'},
-      {verb: 'get', path: '/orders/{id}/exists'},
+      {verb: 'get', path: '/orders'},
+      {verb: 'delete', path: '/orders'},
       {verb: 'get', path: '/users/{userId}/orders'},
       {verb: 'get', path: '/users/{id}/products'},
-      {verb: 'get', path: '/orders/count'},
-      {verb: 'get', path: '/orders/{id}'},
       {verb: 'get', path: '/users/{id}'},
-      {verb: 'get', path: '/orders'},
-      {verb: 'delete', path: '/orders/{id}'},
-      {verb: 'delete', path: '/orders'},
     ]);
   });