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

feat: Add route-level middleware #1286

Closed
wants to merge 49 commits into from

Conversation

adriangb
Copy link
Member

@adriangb adriangb commented Sep 13, 2021

In case you reconsider #685 (comment) @tomchristie

I think this would be a useful feature. I often see middleware parameters that are "a regex pattern to match routes" because the middleware has to determine what routes to interact with before routing is done. Here and here are two examples. This also closes fastapi/fastapi#1174

Aside from being a pain to build some complex regex, there are performance implications: imagine (in the absurd scenario) that you have 100 endpoints and 100 middleware, but each middleware applies only to 1 endpoint. Now you have 99 middleware executing and doing regex matching when they could have just been bypassed to begin with if they executed after routing.

That said, re-using the existing Middleware construct for this use has it's cons. For example, it is obvious that the middleware cannot modify the path. I can't think of any other major limitations, but I there may be others.

@alex-oleshkevich
Copy link
Member

A more common use case of that is to apply a specific middleware to /api, /admin, and / scopes. Admin may require session, API wants token authentication, and landing page needs none of them.

@alex-oleshkevich
Copy link
Member

alex-oleshkevich commented Sep 18, 2021

May I also ask you to add middleware to Mount and Host classes? That would be awesome!

@adriangb
Copy link
Member Author

May I also ask you to add middleware to Mount and Host classes? That would be awesome!

I think that'd be cool, and should be doable, but maybe let's leave it for another PR, depending on the outcome of this one? I'd like to get an initial review / buy in from the Starlette team before making any more changes

@lorthirk
Copy link

Sorry to jump in, but there's any news about this? It would be useful in my projects

@adriangb
Copy link
Member Author

Sorry to jump in, but there's any news about this? It would be useful in my projects

It is waiting for review, there is no ETA. But, if you really needed to, you could just wrap the class in your app to add the functionality given that .app is a public attribute

@tomchristie
Copy link
Member

Okay, ya know what - it's a pretty nice simple implementation, and even though I'm not super keen on route-based middlewares (just because constraints are not a bad thing) I think we could go with this.

Here's what I'd like to see in order to merge this...

  • Let's get the same thing added to Mount as well. Having middleware on mounts kinda makes more sense to middleware on routes to me.
  • Let's get docs included on the PR as well.

After that then I reckon it's a thumbs up from me. Good stuff!

@adriangb
Copy link
Member Author

Hey @tomchristie, thank you for looking at this!

I added the same implementation into Mount and repeated the test for that as well.

What are your thoughts on the other route types? I reckon this can also be added to WebSocketRoute and Host, but maybe it's better to wait until someone actually requests that feature.

I added a blurb in the docs, it's relatively short but I think it gets the point across.

docs/middleware.md Outdated Show resolved Hide resolved
adriangb and others added 2 commits November 11, 2021 12:59
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
@adriangb
Copy link
Member Author

@tomchristie looks like I was able to implement your requested changes and get all tests passing.

I just wanted to highlight this question before we move forward with this:

What are your thoughts on the other route types? I reckon this can also be added to WebSocketRoute and Host, but maybe it's better to wait until someone actually requests that feature.

Also, like we were discussing w/ @Kludex on Gitter, I'm open to being added as a maintainer so I can merge PRs after approval (given that I have several open across Starlette and httpx).

@@ -340,16 +348,22 @@ def __init__(
self.path = path.rstrip("/")
if app is not None:
self.app: ASGIApp = app
routes = getattr(self.app, "routes", [])
Copy link
Member

Choose a reason for hiding this comment

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

Is this variable used?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah good catch, it is not being used. It's leftover from a previous implementation.

Comment on lines 352 to 367
return getattr(self.app, "routes", [])
# we dynamically grab the routes so that if this is a Starlette router
# it can have routes added to it after it is mounted
return getattr(self._user_app, "routes", [])
Copy link
Member Author

Choose a reason for hiding this comment

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

@tomchristie how would you feel about just grabbing a reference to .routes in __init__ instead of doing a getattr here? I suppose that would break if the mounted router replaces its .routes with a new list, but otherwise it should be fine and it would avoid having to add this _user_app attribute.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I like it more. I went ahead and implemented it that way: e9ff903

If you prefer it as it was doing a getattr in self.routes, I'm happy to revert it

Copy link
Member

@tomchristie tomchristie left a comment

Choose a reason for hiding this comment

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

All seems pretty reasonable yup. I'm personally still not that wild about route-level middleware, but I'm happy to defer this one to the rest of the team. 🤣

Copy link
Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

There were two possible solutions proposed by Tom on the issue that motivated this PR.

  • We could potentially add some public API on the application so that it's easier to premptively resolve the request routing? Ideally we ensure that can be cached, so that the lookup only occurs once.
  • We could add the ability to have some kinds of middleware run after the request routing. (Probably not super keen on that.)

I took my time to think about the first solution, and how that would look like... I think what we can do is to add a parameter on the Middleware class:

app = Starlette(
    middleware=[
        Middleware(PotatoMiddleware, path_prefix="/potato")  # or `path_prefixes`
    ]
)

Considering the above, we stick with the middleware concept being only at the app level.

Thoughts? @alex-oleshkevich @adriangb

starlette/routing.py Outdated Show resolved Hide resolved
adriangb and others added 2 commits January 29, 2022 12:10
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
@adriangb
Copy link
Member Author

We could potentially add some public API on the application so that it's easier to preemptively resolve the request routing?

I think what we can do is to add a parameter on the Middleware class:

app = Starlette(
    middleware=[
        Middleware(PotatoMiddleware, path_prefix="/potato")  # or `path_prefixes`
    ]
)

How would that be implemented? Is Middleware doing something special with path_prefix, or is it passing it to PotatoMiddleware like it does with all other keyword arguments? What if PotatoMiddleware expects a path_prefix argument? And when does routing run? Apologies if I'm just missing something, but I don't see how this could work. It also seems like this would end up in much of the same place we are now: middleware that accepts some path_prefix_regex parameter with regexes to match against (essentially re-inventing routing inside middleware).

@adriangb
Copy link
Member Author

adriangb commented Feb 1, 2022

FYI for those subscribed to this issue, I opened #1464 which I think solves this problem more elegantly

@adriangb adriangb added the feature New feature or request label Feb 2, 2022
@Kludex
Copy link
Member

Kludex commented Apr 21, 2022

@adriangb Can we close this PR, and focus on #1464?

@adriangb
Copy link
Member Author

Yes

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

Successfully merging this pull request may close these issues.

How to use different middleware for different routes/path
5 participants