oakserver / oak

A middleware framework for handling HTTP with Deno, Node, Bun and Cloudflare Workers 🐿️ 🦕
https://oakserver.org
MIT License
5.09k stars 231 forks source link

Middlewares leaking to sibling router when using nested router #415

Open tjosepo opened 2 years ago

tjosepo commented 2 years ago

Tested on Oak 9.0.1 and Deno 1.14.0.

When two routers are used by a common router, the middlewares from the first registered router get applied to the second registered router.

Consider this example:

import { Application, Router } from "https://deno.land/x/oak@v9.0.1/mod.ts";

// Router A
const routerA = new Router()
  .use(async (ctx, next) => {
    console.log("Used router A");
    await next();
  })
  .get("/a", (ctx) => {
    ctx.response.body = "Hello from A!";
  });

// Router B
const routerB = new Router()
  .use(async (ctx, next) => {
    console.log("Used router B");
    await next();
  })
  .get("/b", (ctx) => {
    ctx.response.body = "Hello from B!";
  });

// Register both routers to another router
const routes = new Router()
  .use(routerA.routes())
  .use(routerB.routes())
  .routes();

console.log("Listening on http://localhost:8080/a and http://localhost:8080/b");
await new Application().use(routes).listen({ port: 8080 });

Expected behavior:

Actual behavior:

I'm not sure if this is a known issue and if there are already plans to address this. If not, I can offer to help with the code🙂

KyleJune commented 2 years ago

This is expected behavior. On the application, routers are used without a path, meaning it will traverse each of them. Your request to /b traverses router A and doesn't find a match, then continues to using router B where it finds a match. If you add a path arg to your use calls from the main router, it will make it so routerA and routerB are only traversed when they match /a or /b. Then update your router a to use "/" as the path instead of "/a" since it is the main path down the /a route.

tjosepo commented 2 years ago

@KyleJune Thank you for your response.

Unfortunately, your explanation is only valid when using an intermediate router.

I need to emphasize the nested router part of this issue. When Router A and Router B are used directly by the Application instance, the outcome is different. Middlewares are encapsulated if you register a router directly to the Application instance, but not if you register them to a shared intermediate router.

I made a demo where I register Router A and Router B directly to the Application instead of to an intermediate router: https://dash.deno.com/playground/oak-issue-415

Expected & Actual behavior when not using a nested router:

I might be wrong, but this seems like too much of an inconsistency to be expected.

KyleJune commented 2 years ago

I agree there is an inconsistency but I still think I'm correct in what the actual behavior should be. I think the application use function should be changed to work like the router use function when no path is specified. I created a separate issue for fixing the inconsistency.

https://github.com/oakserver/oak/issues/436

The example in your last comment was shared incorrectly, I do not have permission to view it. I believe there is a share button at the top for making it so that anyone can view it.

tjosepo commented 2 years ago

Thanks for letting me know about the example. It should be public now.