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

Unable to add middleware after creation: Can not add a route since the matcher is already built. #3026

Closed
JoaquimLey opened this issue Jun 23, 2024 · 10 comments

Comments

@JoaquimLey
Copy link
Contributor

JoaquimLey commented Jun 23, 2024

What version of Hono are you using?

4.4.5

What runtime/platform is your app running on?

Bun

What steps can reproduce the bug?

Bootstrap the app like this

app.use("*", async (c, next) => {
  const start = Date.now()
  await next()
  const end = Date.now()
  c.res.headers.set('X-Response-Time', `${end - start}`)
})

app.post("/email/confirm", (c) => {
  return c.text("Hello World");
});

Then during testing try to add another middleware (this is after the app is instantiated):

describe("Auth router - [POST] /email/confirm", () => {
  beforeEach(() => {
    authRouter.use("/email/confirm", async (c, next) => {
     console.log("Setting mock user in test middleware");
     const mockUser = { sub: "mock_id", email: "[email protected]" }
     c.set("user", mockUser)
    });

    it("It should return Hello world", async () => {
      const mock_access_token = "mock_access_token";
      const res = await authRouter.request(
        "/email/confirm",
        {
          method: "POST",
          headers: { "Content-Type": "application/json", Authorization: `Bearer ${mock_access_token}` },
        },
        {}
      );

      expect(res.status).toBe(200)

      const data = await res.text();
      expect(data).toEqual("Hello World");
    });
  });
});

What is the expected behavior?

I was hoping to have the "test middleware" added to this instance so I could manipulate what the user object is (I decode the JWT and set it c.set("user", decodedUser)

But during testing I want to have full control of what is on my context, this includes the data that was decoded from the mock jwt.

What do you see instead?

An error is thrown:

 7 |   constructor(init) {
 8 |     Object.assign(this, init);
 9 |   }
10 |   add(method, path, handler) {
11 |     if (!this.routes) {
12 |       throw new Error(MESSAGE_MATCHER_IS_ALREADY_BUILT);
                 ^
error: Can not add a route since the matcher is already built.
      at add (***/node_modules/hono/dist/router/smart-router/router.js:12:13)
      at addRoute (***/node_modules/hono/dist/hono-base.js:160:17)
      at ***/node_modules/hono/dist/hono-base.js:69:14
      at forEach (:1:11)
      at ***/node_modules/hono/dist/hono-base.js:68:16
      at ***/src/***/***/***/router.test.ts:157:16

Additional information

No response

@JoaquimLey JoaquimLey added the bug label Jun 23, 2024
@yusukebe
Copy link
Member

Hi @JoaquimLey

It's not a bug. As the error message, you can not add routes after handling a response. So, instantiate Hono in beforeEach.

@yusukebe yusukebe removed the bug label Jun 23, 2024
@JoaquimLey
Copy link
Contributor Author

JoaquimLey commented Jun 23, 2024

Yes I was also unsure if that was the right template for the issue.

I am, the app already exists (or else I wouldn’t be able to add the middleware, all my tests run after the hono app is created.

In a nutshell I’m looking for a way to inject “mock data” into the context in my tests.

Let’s say that the bearer middleware injects the decoded user, how can I verify that the right data (eg user.id) was passed down to another layer (use case, service, repository) from the router?

@yusukebe
Copy link
Member

@JoaquimLey

Thank you for you explanation. Understood.

@usualoma Do you have any thoughts?

@usualoma
Copy link
Member

Hi @JoaquimLey

Is it possible to rewrite your test as follows?

describe("Auth router - [POST] /email/confirm", () => {
  let authRouter;
  beforeEach(() => {
    authRouter = new MyApp();
    authRouter.use("/email/confirm", async (c, next) => {
     console.log("Setting mock user in test middleware");
     const mockUser = { sub: "mock_id", email: "[email protected]" }
     c.set("user", mockUser)
    });
  });

  it("It should return Hello world", async () => {
    const mock_access_token = "mock_access_token";
    const res = await authRouter.request(
      "/email/confirm",
      {
        method: "POST",
        headers: { "Content-Type": "application/json", Authorization: `Bearer ${mock_access_token}` },
      },
      {}
    );
    expect(res.status).toBe(200)
    const data = await res.text();
    expect(data).toEqual("Hello World");
  });
});

@JoaquimLey
Copy link
Contributor Author

JoaquimLey commented Jun 24, 2024

Hello @usualoma

Not sure how I would create the app new MyApp() can you show me an example?

Here's how it is working right now:

// auth.router.ts

const authRouter = new Hono<{ Bindings: Bindings }>();
authRouter.use("*", authMiddleware); // Injects service
authRouter.onError((error, c) => {
  // TODO: Hookup an actual error logger.
  console.error("Error on auth router ", error);

  if (error instanceof HTTPException) {
    return error.getResponse();
  }

  return c.json({ message: "Server error, please try again later\\." }, 500);
});

////////////
// Routes //
////////////

authRouter.post("/email/confirm", async (c) => {

Maybe I should've been more clear but his is being imported by the "root" hono app, I'm following the pattern of adding controllers (https://hono.dev/docs/guides/best-practices#building-a-larger-application):

// index.ts

import authRouter from "@/services/auth/auth.router";

(...)

const app = new Hono<{ Bindings: Env }>().basePath(API_VERSION);

(...)

app.route("auth", authRouter);

EDIT:

After trying to figure this out for some time now I'm wondering if we should have a test-util that injects/manipulates the context.
Either I'm doing something wrong in the way I'm creating my router/app (very possible) or this is going to be a very common use case.

@usualoma
Copy link
Member

Hi @JoaquimLey
Please try the following approach.

import authRouter from "@/services/auth/auth.router";

describe("Auth router - [POST] /email/confirm", () => {
  let app;
  beforeEach(() => {
    app = new Hono();
    app.route("/email/*", authRouter);
    app.use("/email/confirm", async (c, next) => {
     console.log("Setting mock user in test middleware");
     const mockUser = { sub: "mock_id", email: "[email protected]" }
     c.set("user", mockUser)
    });
  });

  it("It should return Hello world", async () => {
    const mock_access_token = "mock_access_token";
    const res = await app.request(
      "/email/confirm",
      {
        method: "POST",
        headers: { "Content-Type": "application/json", Authorization: `Bearer ${mock_access_token}` },
      },
      {}
    );
    expect(res.status).toBe(200)
    const data = await res.text();
    expect(data).toEqual("Hello World");
  });
});

@JoaquimLey
Copy link
Contributor Author

JoaquimLey commented Jun 25, 2024

Hello @usualoma I was able to get it working!
Thank you so much 🎉

Your example above is also a better overall setup, that way it'll further isolate the router tests since it won't have to deal with any potential middleware(s) from the root Hono app.

I had to make some small changes though:

describe("Auth router - [POST] /email/confirm", () => {
  let app: Hono;
  beforeEach(() => {
    app = new Hono();

    // IMPORTANT: Add the middleware before app.route
    // Added "*" for the sake of simplicity, we can still set specific routes.
    app.use("*", async (c, next) => {
      const mockUser = { sub: "mock_id", email: "[email protected]" };
      c.set("jwt", mockUser);
      await next();
    });

    app.route("auth", authRouter);
  });

  it("It should return Hello world", async () => {
    const mock_access_token = "mock_access_token";
    const res = await app.request(
      "/auth/email/confirm",
      {
        method: "POST",
        headers: { "Content-Type": "application/json", Authorization: `Bearer ${mock_access_token}` },
      },
      {}
    );

    expect(res.status).toBe(200);

    const data = await res.text();
    expect(data).toEqual("Hello World");
  });
});

I think the most relevant part is adding the middleware before any routing.

I'm wondering if it would be possible to write a small test util to wrap/abstract all this setup, the biggest potential issue I see is requiring the consumer to use the resuling "app" object, but maybe we could do something along the lines of a builder pattern for the test util, maybe expand on the testClient - https://hono.dev/docs/helpers/testing

If this makes sense I'm happy to try and take a look or just write a small package that can do this, regardless I would suggest adding this example in the docs, once I get the time I'll create a PR that you can either use or at least get some inspiration from.

PS: Feel free to close this issue, ty for your time and helping out!

@JoaquimLey
Copy link
Contributor Author

JoaquimLey commented Jun 25, 2024

Adding this WIP PR:

@yusukebe
Copy link
Member

@usualoma Thank you!

@JoaquimLey As you said, I'll close this issue now and review your PR for the website if it's ready. Thanks!

@JoaquimLey
Copy link
Contributor Author

@yusukebe PR is ready for review:

cc @usualoma

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

No branches or pull requests

3 participants