improvement: raise exception when a route has duplicate params

Author: thetutlage

src/Router/Store.ts

@@ -143,6 +143,32 @@ export class Store {
       this.matchDomain = this.matchDomainReal
     }
 
+    /*
+     * Generate tokens for the given route and push to the list
+     * of tokens
+     */
+    const tokens = matchit.parse(route.pattern, route.matchers)
+    const collectedParams: Set<string> = new Set()
+
+    /**
+     * Avoiding duplicate route params
+     */
+    for (let token of tokens) {
+      if ([1, 3].includes(token.type)) {
+        if (collectedParams.has(token.val)) {
+          throw new Exception(
+            `Duplicate route param "${token.val}" in route ${route.pattern}`,
+            500,
+            'E_DUPLICATE_ROUTE',
+          )
+        } else {
+          collectedParams.add(token.val)
+        }
+      }
+    }
+
+    collectedParams.clear()
+
     route.methods.forEach((method) => {
       const methodRoutes = this.getMethodRoutes(route.domain || 'root', method)
 
@@ -159,11 +185,7 @@ export class Store {
         )
       }
 
-      /*
-       * Generate tokens for the given route and push to the list
-       * of tokens
-       */
-      methodRoutes.tokens.push(matchit.parse(route.pattern, route.matchers))
+      methodRoutes.tokens.push(tokens)
 
       /*
        * Store reference to the route, so that we can return it to the user, when

test/store.spec.ts

@@ -117,6 +117,40 @@ test.group('Store | add', () => {
     assert.throw(fn, 'Duplicate route `GET:/`')
   })
 
+  test('raise error when two params have the same name', (assert) => {
+    async function handler () {}
+    const route = {
+      pattern: '/:id/:id',
+      methods: ['GET'],
+      handler: handler,
+      matchers: {},
+      meta: {},
+      domain: 'foo.com',
+      middleware: [],
+    }
+
+    const store = new Store()
+    const fn = () => store.add(route)
+    assert.throw(fn, 'E_DUPLICATE_ROUTE: Duplicate route param "id" in route /:id/:id')
+  })
+
+  test('allow static path name same as the param name', (assert) => {
+    async function handler () {}
+    const route = {
+      pattern: '/id/:id',
+      methods: ['GET'],
+      handler: handler,
+      matchers: {},
+      meta: {},
+      domain: 'foo.com',
+      middleware: [],
+    }
+
+    const store = new Store()
+    const fn = () => store.add(route)
+    assert.doesNotThrow(fn)
+  })
+
   test('work fine when pattern is same but method is different', (assert) => {
     async function handler () {}
     const route = {
@@ -250,6 +284,71 @@ test.group('Store | add', () => {
       },
     })
   })
+
+  test('add route for multiple methods', (assert) => {
+    async function handler () {}
+
+    const store = new Store()
+    store.add({
+      pattern: '/:id',
+      methods: ['GET', 'POST'],
+      handler: handler,
+      matchers: {},
+      meta: {},
+      middleware: [],
+    })
+
+    assert.deepEqual(store.tree, {
+      tokens: [[{
+        old: 'root',
+        type: 0,
+        val: 'root',
+        end: '',
+      }]],
+      domains: {
+        'root': {
+          'GET': {
+            tokens: [[
+              {
+                old: '/:id',
+                type: 1,
+                val: 'id',
+                end: '',
+                matcher: undefined,
+              },
+            ]],
+            routes: {
+              '/:id': {
+                pattern: '/:id',
+                meta: {},
+                handler,
+                middleware: [],
+              },
+            },
+          },
+          'POST': {
+            tokens: [[
+              {
+                old: '/:id',
+                type: 1,
+                val: 'id',
+                end: '',
+                matcher: undefined,
+              },
+            ]],
+            routes: {
+              '/:id': {
+                pattern: '/:id',
+                meta: {},
+                handler,
+                middleware: [],
+              },
+            },
+          },
+        },
+      },
+    })
+  })
 })
 
 test.group('Store | match', () => {