From 4d84dfd092ce0510312e975d5cd57e04973fb311 Mon Sep 17 00:00:00 2001
From: David Luecke <daff@neyeon.com>
Date: Sun, 9 May 2021 09:06:22 -0700
Subject: [PATCH] fix(transport-commons): Fix route placeholder registration
 and improve radix router performance (#2336)

---
 .../transport-commons/src/routing/router.ts   | 53 +++++++++++--------
 .../test/routing/router.test.ts               | 32 ++++++++---
 2 files changed, 58 insertions(+), 27 deletions(-)

diff --git a/packages/transport-commons/src/routing/router.ts b/packages/transport-commons/src/routing/router.ts
index f05686d735..29ecf29e21 100644
--- a/packages/transport-commons/src/routing/router.ts
+++ b/packages/transport-commons/src/routing/router.ts
@@ -1,5 +1,4 @@
 import { stripSlashes } from '@feathersjs/commons';
-import { BadRequest } from '@feathersjs/errors';
 
 export interface LookupData {
   params: { [key: string]: string };
@@ -12,54 +11,66 @@ export interface LookupResult<T> extends LookupData {
 export class RouteNode<T = any> {
   data?: T;
   children: { [key: string]: RouteNode } = {};
-  placeholder?: RouteNode;
+  placeholders: RouteNode[] = [];
 
-  constructor (public name: string) {}
+  constructor (public name: string, public depth: number) {}
 
   insert (path: string[], data: T): RouteNode<T> {
-    if (path.length === 0) {
+    if (this.depth === path.length) {
+      if (this.data !== undefined) {
+        throw new Error(`Path ${path.join('/')} already exists`);
+      }
+
       this.data = data;
       return this;
     }
 
-    const [ current, ...rest ] = path;
+    const current = path[this.depth];
+    const nextDepth = this.depth + 1;
 
     if (current.startsWith(':')) {
-      const { placeholder } = this;
-      const name = current.substring(1);
+      // Insert a placeholder node like /messages/:id
+      const placeholderName = current.substring(1);
+      let placeholder = this.placeholders.find(p => p.name === placeholderName);
 
       if (!placeholder) {
-        this.placeholder = new RouteNode(name);
-      } else if(placeholder.name !== name) {
-        throw new BadRequest(`Can not add route with placeholder ':${name}' because placeholder ':${placeholder.name}' already exists`);
+        placeholder = new RouteNode(placeholderName, nextDepth);
+        this.placeholders.push(placeholder);
       }
 
-      return this.placeholder.insert(rest, data);
+      return placeholder.insert(path, data);
     }
 
-    this.children[current] = this.children[current] || new RouteNode(current);
+    const child = this.children[current] || new RouteNode(current, nextDepth);
+
+    this.children[current] = child;
 
-    return this.children[current].insert(rest, data);
+    return child.insert(path, data);
   }
 
   lookup (path: string[], info: LookupData): LookupResult<T>|null {
-    if (path.length === 0) {
-      return {
+    if (path.length === this.depth) {
+      return this.data === undefined ? null : {
         ...info,
         data: this.data
       }
     }
 
-    const [ current, ...rest ] = path;
+    const current = path[this.depth];
     const child = this.children[current];
 
     if (child) {
-      return child.lookup(rest, info);
+      return child.lookup(path, info);
     }
 
-    if (this.placeholder) {
-      info.params[this.placeholder.name] = current;
-      return this.placeholder.lookup(rest, info);
+    // This will return the first placeholder that matches early
+    for(const placeholder of this.placeholders) {
+      const result = placeholder.lookup(path, info);
+
+      if (result !== null) {
+        result.params[placeholder.name] = current;
+        return result;
+      }
     }
 
     return null;
@@ -67,7 +78,7 @@ export class RouteNode<T = any> {
 }
 
 export class Router<T> {
-  root: RouteNode<T> = new RouteNode<T>('');
+  constructor (public root: RouteNode<T> = new RouteNode<T>('', 0)) {}
 
   getPath (path: string) {
     return stripSlashes(path).split('/');
diff --git a/packages/transport-commons/test/routing/router.test.ts b/packages/transport-commons/test/routing/router.test.ts
index 48bdb81ea5..e9590ffa3e 100644
--- a/packages/transport-commons/test/routing/router.test.ts
+++ b/packages/transport-commons/test/routing/router.test.ts
@@ -40,6 +40,10 @@ describe('router', () => {
 
     const first = r.lookup('hello/there/');
 
+    assert.throws(() => r.insert('/hello/:id/you', 'two'), {
+      message: 'Path hello/:id/you already exists'
+    });
+
     assert.deepStrictEqual(first, {
       params: { id: 'there' },
       data: 'one'
@@ -65,14 +69,30 @@ describe('router', () => {
     assert.strictEqual(r.lookup('hello/yes/they/here'), null);
   });
 
-  it('errors when placeholder in a path is different', () => {
+  it('works with different placeholders in different paths (#2327)', () => {
     const r = new Router<string>();
 
-    assert.throws(() => {
-      r.insert('/hello/:id', 'one');
-      r.insert('/hello/:test/you', 'two');
-    }, {
-      message: 'Can not add route with placeholder \':test\' because placeholder \':id\' already exists'
+    r.insert('/hello/:id', 'one');
+    r.insert('/hello/:test/you', 'two');
+    r.insert('/hello/:test/:two/hi/:three', 'three');
+    r.insert('/hello/:test/:two/hi', 'four');
+
+    assert.deepStrictEqual(r.lookup('/hello/there'), {
+      params: { id: 'there' },
+      data: 'one'
+    });
+    assert.deepStrictEqual(r.lookup('/hello/there/you'), {
+      params: { test: 'there' },
+      data: 'two'
+    });
+    assert.strictEqual(r.lookup('/hello/there/bla'), null);
+    assert.deepStrictEqual(r.lookup('/hello/there/maybe/hi'), {
+      params: { test: 'there', two: 'maybe' },
+      data: 'four'
+    });
+    assert.deepStrictEqual(r.lookup('/hello/there/maybe/hi/test'), {
+      params: { three: 'test', two: 'maybe', test: 'there' },
+      data: 'three'
     });
   });
 });