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

Allow to order the middleware list #1490

Closed
2 tasks done
sylvainmouquet opened this issue Feb 9, 2022 · 4 comments
Closed
2 tasks done

Allow to order the middleware list #1490

sylvainmouquet opened this issue Feb 9, 2022 · 4 comments

Comments

@sylvainmouquet
Copy link

Checklist

  • There are no similar issues or pull requests for this yet.
  • I discussed this idea on the community chat and feedback is positive.

Is your feature related to a problem? Please describe.

    def add_middleware(self, middleware_class: type, **options: typing.Any) -> None:
        self.user_middleware.insert(0, Middleware(middleware_class, **options))
        self.middleware_stack = self.build_middleware_stack()

The adding of a new middleware is done after the first position.
There is not a method to reordered the list of middleware

Describe the solution you would like.

Add q

Describe alternatives you considered

No response

Additional context

No response

@Kludex
Copy link
Sponsor Member

Kludex commented Feb 9, 2022

The middleware order is mentioned on our docs: https://www.starlette.io/middleware/

@internetofjames
Copy link

@Kludex @tomchristie I don't think this issue should be closed, I believe you are misunderstanding what @sylvainmouquet is raising an issue to.

If we applied the same middleware via the Starlette constructor argument vs adding them in order one at a time via app.add_middleware, the behavior is different (starlette==0.23.1):

middleware = [
        Middleware(
            TrustedHostMiddleware,
            allowed_hosts=['example.com', '*.example.com'],
        ),
        Middleware(HTTPSRedirectMiddleware),
        Middleware(CORSMiddleware, allow_origins=['*'])
    ]

print("Middleware list:\n", middleware)
print()

app = Starlette(middleware=middleware)
app.user_middleware
    
print("app.user_middleware after passing list into Starlette constructor: ", app.user_middleware)
print()

app = Starlette()
for mid_type, opts in middleware:
    app.add_middleware(mid_type, **opts)
    
print("app.user_middleware after iterating through list and adding middleware via app.add_middleware: ", app.user_middleware)

Output:

Middleware list:
 [Middleware(TrustedHostMiddleware, allowed_hosts=['example.com', '*.example.com']), Middleware(HTTPSRedirectMiddleware), Middleware(CORSMiddleware, allow_origins=['*'])]

app.user_middleware after passing list into Starlette constructor:  [Middleware(TrustedHostMiddleware, allowed_hosts=['example.com', '*.example.com']), Middleware(HTTPSRedirectMiddleware), Middleware(CORSMiddleware, allow_origins=['*'])]

app.user_middleware after iterating through list and adding middleware via app.add_middleware:  [Middleware(CORSMiddleware, allow_origins=['*']), Middleware(HTTPSRedirectMiddleware), Middleware(TrustedHostMiddleware, allowed_hosts=['example.com', '*.example.com'])]

As you can see, depending on how the middleware is added, the order changes. IMO, it makes intuitive sense to go the way of the constructor, as the order the middleware is passed in is the order that starlette will evaluate it. Futhermore, FastAPI only provides users the option to add middleware via the add_middleware method. Their docs do not show an example of adding more than one middleware, and so the user will be frustrated with this unintuitive behavior if the order their middleware is applied matters.

I think the issue should be changed from "Allow to order the middleware list" to unifying the two methods so that both the constructor and the add_middleware method behave the same. Don't necessarily grant the user the ability to reorder the middleware, but simply that add_middleware should append the middleware to the end of the user_middleware list so that the method call history aligns with the order of middleware evaluation. (#1092 raised a similar issue to this behavior)

Let me know your thoughts! I could also take a stab at implementing this if you'd like.

@Kludex
Copy link
Sponsor Member

Kludex commented Dec 12, 2022

We are aware. There have been discussions to deprecate the add_middleware because of that reason. 👀

@internetofjames
Copy link

I see. Would it be better to deprecate the method vs. altering the behavior in a breaking change?

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

4 participants