app.route includes middleware from previous groups

[KaelWD]

What version of Hono are you using?

4.4.6

What runtime/platform is your app running on?

Node

What steps can reproduce the bug?

const routerA = new Hono()
  .use(async (c, next) => {
    console.log('Middleware A')
    await next()
  })
  .get('/a', async c => {
    return c.text('A')
  })

const routerB = new Hono()
  .use(async (c, next) => {
    console.log('Middleware B')
    await next()
  })
  .get('/b', async c => {
    return c.text('B')
  })

const app = new Hono()
  .route('/', routerA)
  .route('/', routerB)

What is the expected behavior?

calling GET /a logs "Middleware A"
calling GET /b logs "Middleware B"

What do you see instead?

calling GET /a logs "Middleware A"
calling GET /b logs "Middleware A" "Middleware B"

Additional information

I was trying to avoid #2987 with a pattern like this instead of ContextVariableMap but appMiddleware ends up being called multiple times in later groups

function createRouter () {
  return new Hono()
    .use(appMiddleware)
}

const router = createRouter()
  .get('/test', routeMiddleware, /* handler */)

const app = new Hono()
  .route('/', router)

[ryuapp]

I think this is not a bug. If I organize your code, it will look like this:

import { Hono } from "hono";

const app = new Hono();

app.use(async (_, next) => {
  console.log("Middleware A start");
  await next();
  console.log("Middleware A end");
});
app.get("/a", (c) => {
  return c.text("A");
});

app.use(async (_, next) => {
  console.log("Middleware B start");
  await next();
  console.log("Middleware B end");
});
app.get("/b", (c) => {
  return c.text("B");
});

export default app;

Middleware is executed in the order in which it is registered, but in the case of /a, middleware registered after it will not be executed.
The execution order is written in the documentation.
https://hono.dev/docs/guides/middleware#execution-order

[KaelWD]

That doesn't say anything about .route(), I'd expect middleware to only apply to the group.

If I organize your code

Please read the additional information section of the issue, this was originally just one middleware repeated in each group for typescript reasons. I understand how it's happening, this behaviour is unintuitive though and doesn't seem useful. We can create route groups with scoped paths but they still leak middleware to the rest of the app. Expanding the example with groups this is how I expected it to work:

const router = new Hono()
  .use(async (_, next) => {
    console.log('middleware 2 start')
    await next()
    console.log('middleware 2 end')
  })
  .get('/1', (c) => {
    console.log('handler 1')
    return c.text('Hello!')
  })

app.use(async (_, next) => {
  console.log('middleware 1 start')
  await next()
  console.log('middleware 1 end')
})
app.route('/', router)
app.use(async (_, next) => {
  console.log('middleware 3 start')
  await next()
  console.log('middleware 3 end')
})
app.get('/2', (c) => {
  console.log('handler 2')
  return c.text('Hello!')
})

GET /1
middleware 1 start
  middleware 2 start
    handler 1
  middleware 2 end
middleware 1 end

GET /2
middleware 1 start
  middleware 3 start
    handler 2
  middleware 3 end
middleware 1 end

[KaelWD]

I found a feature request for something similar, I don't see why this shouldn't just be the default behaviour though: #1674

[KaelWD]

This would also solve #2249

[KaelWD]

And #2794

app
  .route('/foo/', new Hono().use(authMiddleware).route('/', PageFoo))
  .get('/protected', authMiddleware, (ctx) => ctx.text('protected', 200))

[yusukebe]

Hi @KaelWD

Thank you for the issue. This is not a bug. But as you feel, it may not be intuitive. I'll consider this issue for a while.

[KaelWD]

The types actually already work like this:

const router = new Hono()
  .use<{
    Variables: {
      test: 'override'
    }
  }>(async (c, next) => {
    c.set('test', 'override')
    await next()
  })

const app = new Hono()
  .use<{
    Variables: {
      test: 'root'
    }
  }>(async (c, next) => {
    c.set('test', 'root')
    await next()
  })
  .route('/', router)
  .get('/test', async (c) => {
    const test = c.get('test')
    //    ^? "root"
    return c.text(test) // 'override', type doesn't match
  })

[yusukebe]

Hi @KaelWD Thanks. Can you create a separate issue?

[usualoma]

Hmmm, this is a difficult one.

There is no doubt that the current behaviour is in itself a specification.
However, as the documentation recommends a similar approach, this certainly appears to be a behaviour that is certainly not expected.
Grouping without changing base

Internal specification of route()

Plugs the subApp's routing under the specified path. (Here, it does not matter whether the routing is a middleware or a handler.)

If an error handler is set for the subApp

In this case, the error handler is tied to the handler.

Given that compose is done when an error handler is set up, special handling for middleware could also be considered. However, I do not want to complicate things too much here.

Also, we want to realise things like Grouping without changing base in a simple way, so I don't want to complicate the syntax.

Realistically, there are two patterns that hit this problem

• An application with "*" is added by specifying "/".
• An application with "*” is added by specifying the same path multiple times.

One idea would be to output the following warning message.

diff --git a/src/hono-base.ts b/src/hono-base.ts
index 1ee4b10d..e9b713dc 100644
--- a/src/hono-base.ts
+++ b/src/hono-base.ts
@@ -214,6 +214,8 @@ class Hono<E extends Env = Env, S extends Schema = {}, BasePath extends string =
     path: SubPath,
     app: Hono<SubEnv, SubSchema, SubBasePath>
   ): Hono<E, MergeSchemaPath<SubSchema, MergePath<BasePath, SubPath>> & S, BasePath> {
+    const subAppPaths = ((this as any)._subAppPaths ||= new Set(['/']))
+
     const subApp = this.basePath(path)
     app.routes.map((r) => {
       let handler
@@ -225,8 +227,16 @@ class Hono<E extends Env = Env, S extends Schema = {}, BasePath extends string =
         ;(handler as any)[COMPOSED_HANDLER] = r.handler
       }
 
+      if (subAppPaths.has(path) && r.path === '*') {
+        console.warn(
+          `It is not recommended to route() applications with wildcard middleware to same path "${path}" multiple times.`
+        )
+      }
+
       subApp.addRoute(r.method, r.path, handler)
     })
+
+    subAppPaths.add(path)
     return this
   }
 

[alexgleason]

I reproduced this problem, but I found it only happens if the grouped routes contain a path. So the problem doesn't occur in this code:

const routerA = new Hono()
  .use(async (c, next) => {
    console.log('Middleware A')
    await next()
  })
  .get(async c => {
    return c.text('A')
  })

const routerB = new Hono()
  .use(async (c, next) => {
    console.log('Middleware B')
    await next()
  })
  .get(async c => {
    return c.text('B')
  })

const app = new Hono()
  .route('/a', routerA) // logs only "Middleware A"
  .route('/b', routerB) // logs only "Middleware B"

Although the behavior with paths is unexpected, I prefer to write my routes without the path. I believe this is a cleaner separation of concerns. So I'm happy.

EDIT: I see the problem with path parameters and types now.

[MiguelRipoll23]

I got into this issue as well...

I'm registering a public router, an authenticated router and a management router.

When I call a non-existing endpoint the management middleware is hit, I expect this middleware to only be hit by the registered routes from the management router.