Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a group method for applying middleware to a group of routes #1674

Open
htunnicliff opened this issue Nov 9, 2023 · 12 comments
Open

Add a group method for applying middleware to a group of routes #1674

htunnicliff opened this issue Nov 9, 2023 · 12 comments
Labels
enhancement New feature or request.

Comments

@htunnicliff
Copy link

htunnicliff commented Nov 9, 2023

What is the feature you are proposing?

I propose that the Hono instance provide a method (group or something to that effect) for applying middleware to a group of routes that don't necessarily share a base path. This would function similarly to the app.route() method, except it would omit the path argument.

Here is one example of how the syntax for this functionality might look:

const app = new Hono();

app.group(a(), few(), middleware(), (app) => {
  // ... normal `app` methods could be called here
});

Further Thoughts

Could app.route() do the same thing?

The route method can almost handle this functionality, but it necessitates that all the routes added within share the same base path. The proposed group method would not impose this restriction.

Inspiration

This feature is inspired by the Laravel framework's Route Groups middleware handler:

Route::middleware(['first', 'second'])->group(function () {
    Route::get('/', function () {
        // Uses first & second middleware...
    });
 
    Route::get('/user/profile', function () {
        // Uses first & second middleware...
    });
});
@htunnicliff htunnicliff added the enhancement New feature or request. label Nov 9, 2023
@yusukebe
Copy link
Member

@htunnicliff

I don't see the benefit of this feature with just the information provided. Adding code to hono-base.ts isn't easy, as it's a core. We need a significant reason to implement it.

@htunnicliff
Copy link
Author

Here is an example of the group method being used as I envision it:

import { Hono } from "hono";
import { cors } from "hono/cors";

const routerA = new Hono();
routerA.get("/", (c) => c.text("Hello from Router A!"));

const routerB = new Hono();
routerB.get("/", (c) => c.text("Hello from Router B!"));

const routerC = new Hono();
routerC.get("/", (c) => c.text("Hello from Router C!"));

const routerD = new Hono();
routerD.get("/", (c) => c.text("Hello from Router D!"));

// Create app
const app = new Hono();

app.group(cors(), (group) => {
  group.route("/a", routerA);
  group.route("/b", routerB);
  group.route("/c", routerC);
});

app.route("/d", routerD);

export default app;

To emulate the same behavior using Hono without that method, it would look something like this:

import { Hono } from "hono";
import { cors } from "hono/cors";

const routerA = new Hono();
routerA.use("*", cors()); // <--- CORS middleware
routerA.get("/", (c) => c.text("Hello from Router A!"));

const routerB = new Hono();
routerA.use("*", cors()); // <--- CORS middleware
routerB.get("/", (c) => c.text("Hello from Router B!"));

const routerC = new Hono();
routerA.use("*", cors()); // <--- CORS middleware
routerC.get("/", (c) => c.text("Hello from Router C!"));

const routerD = new Hono();
routerD.get("/", (c) => c.text("Hello from Router D!"));

// Create app
const app = new Hono();

app.route("/a", routerA);
app.route("/b", routerB);
app.route("/c", routerC);
app.route("/d", routerD);

export default app;

However, since routers are often moved into separate files, the main app file might look like this instead, making it harder to see which routes have which middleware:

import { Hono } from "hono";
import { routerA } from "./routerA";
import { routerB } from "./routerB";
import { routerC } from "./routerC";
import { routerD } from "./routerD";

// Create app
const app = new Hono();

app.route("/a", routerA);
app.route("/b", routerB);
app.route("/c", routerC);
app.route("/d", routerD);

export default app;

I think the group method has merit based on these examples. What do you think?

@htunnicliff
Copy link
Author

@yusukebe also, thank you for dialoguing with me on a few issues today — I really like Hono and appreciate your thoughtful responses.

@yusukebe
Copy link
Member

Hi @htunnicliff,

Hmm, I've thought about your proposal, but I don't think it's particularly beneficial.

For instance, we can already write code like this:

const app = new Hono()

app.use('/a/*', mw)
app.route('/a', routerA)
app.use('/b/*', mw)
app.route('/b', routerB)
app.use('/c/*', mw)
app.route('/c', routerC)

However, let's keep this issue open for now.

@Al00X
Copy link

Al00X commented Dec 18, 2023

@yusukebe
route groups are necessary to prevent duplication and keeping the code clean. also provides a way to add a middleware to a group of routes with a single line of code without adding it to each route separately.

things i tried to achieve this effect but didn't worked, hope there would be a way to group routes soon, since .use() is not scoped, it's difficult to do it out-of-the-box.

const groupA = new Hono();
groupA .route('/A', a);
groupA .route('/B', b);
groupA .route('/C', c);
groupA .use(Auth()); // or app.use('*', Auth()) , the first one not is not working fully, and the second one is applied to the whole routes

const groupB = new Hono();
groupB .route('/auth', auth);

app.route('/api', app.route('/', groupA).route('/', groupB))
app.route('/api', app
    .route('', app
      .route('/A', a)
      .route('/B', b)
      .route('/C', c)
      .use(Auth()) // or app.use('*', Auth())
    )
    .route('/auth', auth)
)

What would be appreciated is to implement it like this:

const groupA = new Hono();
groupA .route('/A', a);
groupA .route('/B', b);
groupA .route('/C', c);
// groupA.use(Auth()) ?, or maybe using the below code?

app.route('/api', groupA, Auth()) // or app.route('/api', groupA).use(Auth())

const groupB = new Hono();
groupB .route('/auth', auth);

app.route('/api', groupB);

Or to reduce the duplication furthermore:

const groupA = new Hono();
groupA.group({
   '/A': a,
   '/B': b,
   '/C': c,
}, Auth())

const groupB = new Hono();
groupB.group({
   '/auth': auth,
})

app.route('/api', groupA, groupB);

@yusukebe
Copy link
Member

Hi @Al00X

I understand what you want to do, but we need to change the core of Hono to make this happen. I don't want to change the core if it can be solved with such as the following idea.

;[a, b, c].map((subApp) => {
  subApp.use('*', Auth())
})

@Atmos4
Copy link

Atmos4 commented Jan 17, 2024

It's annoying that we are forced into route-based middlewares. It makes the logic of the app very centered around routes, which doesn't work well for more creative routing solutions. For example, here is how to introduce weird bugs in the app:

app.route("/", subController)

Doing this will apply the * middlewares used in subController to the main app, and all following endpoints will use this middleware. The lack of middleware encapsulation decreases code readability and locality of behavior, in addition of being a bug-prone approach that could lead to potential security problems.

Is there any change one of the above suggestions could be thought about? Something similar from use for Elysia or register for Fastify, that allows for application composition with proper middleware encapsulation.

Here would be what I am looking for - similar to how it works in route declarations like app.get()

app.route("/", middleware, <middleware2, middleware3, ... ,> subController)

The current (only?) way to do that is to put all routes you want out of the middleware before the middleware declaration, which is not super readable.

app.route("/", loginController)
app.use("*", authMiddleware)
app.route("/", mainController)

I would be happy to help, but maybe this is just too big of a framework rewrite. Let me know what you think about that.

Or I could just swallow my pride and start doing route-based middleware 😄

@shaunlee
Copy link

@htunnicliff I was looking for the same question as you, almost gave up on Hono. But I found this solution to the problem, it works well:

import { Hono } from "hono"
import { routerA } from "./routerA"
import { routerB } from "./routerB"
import { routerC } from "./routerC"
import { routerD } from "./routerD"

import { auth } from './middlewares'

const app = new Hono()

app.route("/a", routerA) // without middleware
app.route("/b", auth(), routerB) // with middleware
app.route("/c", auth(), routerC) // with middleware
app.route("/d", auth(), routerD) // with middleware

export default app

@bonadio
Copy link

bonadio commented Apr 1, 2024

@htunnicliff I was looking for the same question as you, almost gave up on Hono. But I found this solution to the problem, it works well:

import { Hono } from "hono"
import { routerA } from "./routerA"
import { routerB } from "./routerB"
import { routerC } from "./routerC"
import { routerD } from "./routerD"

import { auth } from './middlewares'

const app = new Hono()

app.route("/a", routerA) // without middleware
app.route("/b", auth(), routerB) // with middleware
app.route("/c", auth(), routerC) // with middleware
app.route("/d", auth(), routerD) // with middleware

export default app

This is not working for me, route takes only 2 parameters, what version are you using @htunnicliff ?

@shaunlee
Copy link

shaunlee commented Apr 2, 2024

@htunnicliff I was looking for the same question as you, almost gave up on Hono. But I found this solution to the problem, it works well:

import { Hono } from "hono"
import { routerA } from "./routerA"
import { routerB } from "./routerB"
import { routerC } from "./routerC"
import { routerD } from "./routerD"

import { auth } from './middlewares'

const app = new Hono()

app.route("/a", routerA) // without middleware
app.route("/b", auth(), routerB) // with middleware
app.route("/c", auth(), routerC) // with middleware
app.route("/d", auth(), routerD) // with middleware

export default app

This is not working for me, route takes only 2 parameters, what version are you using?

@bonadio "hono": "^4.1.0",

The auth function should return a middleware (c, next) => {...}

@markkkkas
Copy link

markkkkas commented Sep 9, 2024

I also believe that grouping routes makes sense. Let's consider a scenario where I have a router that can contain both public and protected (authenticated-only) routes.

const publicRouter = new Hono();
publicRouter.patch("/", (c) => c.json({ message: "I'm public!" });

const protectedRouter = new Hono().use(authMiddleware);
protectedRouter.post("/", (c) => c.json({ message: "I'm protected!" });

const appRouter = new Hono();
appRouter
  .route("/", publicRouter)
  .route("/", protectedRouter);

In this case, if I call GET /, the middleware will interrupt my request, even though there is no pattern defined for it.

Alternatively, the following approach works:

const publicRouter = new Hono();
publicRouter.patch("/", (c) => c.json({ message: "I'm public!" });

const protectedRouter = new Hono();
protectedRouter.post("/", authMiddleware, (c) => c.json({ message: "I'm protected!" });

const appRouter = new Hono();
appRouter
  .route("/", publicRouter)
  .route("/", protectedRouter);

In this scenario, the protected router no longer functions as such. However, I do not believe this approach will scale well, as it becomes very easy to forget to attach the necessary middleware, potentially exposing routes that should remain protected.

If grouping were available, I could just do something like this:

const router = new Hono();

router.group((group) => {
  group.patch("/", (c) => c.json({ message: "I'm public!" });
});

router.group(authMiddleware, (group) => {
  group.post("/", (c) => c.json({ message: "I'm protected!" });
});

@focux
Copy link

focux commented Oct 15, 2024

I ended up here trying to find a way to group routes, mostly for the use-case when you have many routers and want to support public/private routes without having to specify the path or having to make all routes public and use the auth middleware for the routes you want to make private but looks like there isn't a fix yet and the workaround of using two routers become a bit complex when you have many routers.

Edit.
This is my workaround:

export const imageRoutes = new OpenAPIHono();

const protectedRoutes = [PostGenerateRoute.route];
protectedRoutes.forEach((route) => {
    imageRoutes.use(route.path, authenticate);
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request.
Projects
None yet
Development

No branches or pull requests

8 participants