Possible to have multiple RawContextMiddlewares?
coneybeare opened this issue · 3 comments
First off, thank you for your hard work on this package!
We use this in a few places in our ecosystem, and now we are in a situation where we likely will need a few different plugins in the context middleware. Our current implementation is a private internal package which subclasses the middleware and allows internal fastapi consumers to add it to their stack. All works well, we have had no issues with this approach.
Now we are looking to add a second RawContextMiddleware subclass in a different package, and are looking to take the same approach. But when we went to implement it, we found that only the first middleware added would work, and the second would not.
The engineer working on this got around it by pulling the plugin from one package and adding to the context middleware of the other, but this tightly couples them in ways we wish not to.
The expectation is that we should be able to do
app.add_middleware(SubclassedRawContextMiddleware1)
app.add_middleware(SubclassedRawContextMiddleware2)
and have both apply, chained, like other middlewares. Is there something in the codebase that prevents this expectation from happening?
I don't understand what is even the expectation of 2 middlewares.
The middleware is a wrapper to create a ContextVar and associate it to a request context.
The 2 middlewares would still be working on the same ContextVar object, and associating the same request.
There is no reason to subclass them.
To extend functionality such as auto-filling data from the request, you can create plugins.
Also do consider that middlewares work in chain, so the first one can create the context, and any middleware following that can assume the context has been created.
Really what are your subclasses even trying to achieve?
I'm getting vibes that you're trying to bundle too many things on those subclasses that shouldn't even be there in the first place, and keeping the RawContextMiddleware as is and providing independent composable middlewares to attach subsequently would be a more flexible approach.
Subclassing needs aside, in larger ecosystems like ours, where we have several internal packages that have different middlewares that can be added to an application like chained building blocks for different purposes, this is needed. Couldn't the addition of a second middleware simply not attempt to recreate the context if it already exists?
Couldn't the addition of a second middleware simply not attempt to recreate the context if it already exists?
why should it? the only reason people would use the middleware is to create the context.
while I guess we could add a check, I don't think it solves your core issue: your middlewares are not composeable enough.
instead of 2 subclasses duplicatedly building on top of the ContextMiddleware , it sounds very much like your stack(s) should be using 3: the original RawContextMiddleware, and the feature provided by your subclassing each as independent middlewares, that can assume the context is created already.
As you're building an internal library, I understand that would require some breaking change on the part of the downstream users to independently add each middlewares, but I very much believe this would give more flexible usage options to each of them (using only ex-subclass A, ex-subclass B, or both, or only use the starlette-context one on its own).
If really making it composable is out of the question for your use cases, may I suggest you to make your own parent class in your library that does check the presence of the context, and subclass from there.