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

Per-route middleware #399

Merged
merged 4 commits into from
Feb 9, 2020
Merged

Per-route middleware #399

merged 4 commits into from
Feb 9, 2020

Conversation

tirr-c
Copy link
Collaborator

@tirr-c tirr-c commented Feb 1, 2020

This PR adds per-route middleware support. Current design is as follows:

  • Route::middleware applies given middleware to the route.
  • Following Route::method (and Route::all) will apply middleware given before.
  • Middleware list for the route can be reset using Route::reset_middleware.
  • When Route::at is called, per-route middleware applied to the parent will be applied to the child route by default. This can be reset using .reset_middleware().

Feel free to comment if there is better design!


Closes #320.

@yoshuawuyts
Copy link
Member

Subroutes returned by Route::at won't have any middleware applied.

I would expect the following to work:

let mut app = tide::new();
app.middleware(middleware_a());

app.at("/").get(|req| "hi"); // middleware "a" applies here

app.at("/foo")
    .middleware(middleware_b())
    .get(|req| "hello");  // middleware "a" + b" apply here

app.listen("localhost:8080").await?;

This is also so that top-level middleware such as logging, cookie handling and others will always be able to run. Being able to define per-route, and per-app middleware is generally desirable. Does it work like this?

@yoshuawuyts
Copy link
Member

yoshuawuyts commented Feb 1, 2020

Also the following:

let mut outer = tide::new();
outer.middleware(middleware_a());
outer.at("/foo").get(|req| "hello");  // middleware "a" applies here

let mut inner = tide::new();
inner.middleware(middleware_b());
inner.at("/").get(|req| "hi"); // middleware "a" + "b" apply here

outer.nest(inner);
outer.listen("localhost:8080").await?;

@tirr-c
Copy link
Collaborator Author

tirr-c commented Feb 1, 2020

@yoshuawuyts Ah, I meant per-route middleware is reset -- yeah, per-app middleware will remain applied. I'll add some tests to cover those!

@yoshuawuyts
Copy link
Member

yoshuawuyts commented Feb 1, 2020

@tirr-c Can you share an example of what you mean? I don't have a clear understanding of the rules quite yet.

@tirr-c
Copy link
Collaborator Author

tirr-c commented Feb 1, 2020

@yoshuawuyts I meant like this:

let mut app = tide::new();
app.middleware(MiddlewareGlobal); // This is app-global and won't be reset
app.at("/a")
    .middleware(MiddlewareA) // This is per-route and subject to be reset
    .get(echo_path) // Global + A
    .at("/b") // /a/b, A is removed here
    .middleware(MiddlewareB)
    .get(echo_path); // Global + B

(and you mentioned the wrong person 😅)

@yoshuawuyts
Copy link
Member

Lol oops, yeah typing from the mobile app.

Hmm, yeah I think I would expect that to work still, even if I probably wouldn't write it like that myself. Route "b" is nested under "a", so hierarchically I'd expect all of the middleware from "a" to apply to "b".

@tirr-c
Copy link
Collaborator Author

tirr-c commented Feb 1, 2020

@yoshuawuyts That's the alternative approach I wrote. If you feel it's better, I'll change the behavior!

@yoshuawuyts
Copy link
Member

Yess, thank you — that'd be great!

@tirr-c
Copy link
Collaborator Author

tirr-c commented Feb 1, 2020

@yoshuawuyts Ooh I misunderstood your first code! It's quite tough to apply middleware of the parent if the subroute is created without using Route::at, e.g. calling app.at("/parent") and app.at("/parent/child") separately. I'm going to make subroutes middleware work only when it's called like app.at("/parent").at("/child"). Is it okay to do so?

@yoshuawuyts
Copy link
Member

I think that's ok! As long as the global middleware still works. I think using at as a means of nesting works well!

@tirr-c
Copy link
Collaborator Author

tirr-c commented Feb 2, 2020

@yoshuawuyts Done! Tests are added to ensure what works and what doesn't.

Copy link
Member

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is 💯

Thanks so much!!

@yoshuawuyts yoshuawuyts merged commit 589abaf into http-rs:master Feb 9, 2020
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

Successfully merging this pull request may close these issues.

Support "scoped" middlewares
2 participants