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

Possible to have multiple RawContextMiddlewares? #112

Open
coneybeare opened this issue Aug 8, 2023 · 3 comments
Open

Possible to have multiple RawContextMiddlewares? #112

coneybeare opened this issue Aug 8, 2023 · 3 comments

Comments

@coneybeare
Copy link

coneybeare commented Aug 8, 2023

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?

@hhamana
Copy link
Collaborator

hhamana commented Aug 25, 2023

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.

@coneybeare
Copy link
Author

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?

@hhamana
Copy link
Collaborator

hhamana commented Aug 25, 2023

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.

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

2 participants