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

Add register_middleware hook #50

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

davidbrochart
Copy link
Contributor

No description provided.

@davidbrochart davidbrochart marked this pull request as draft October 18, 2021 16:07
@adriendelsalle
Copy link
Member

Thanks for opening this @davidbrochart . I'm still a bit confused about how to handle a middleware since we should not allow any plugin to register on the app directly without ensuring no collisions or intrusive declarations vs other plugins.
I'll take a look

@adriendelsalle
Copy link
Member

fastapi/fastapi#1174

@davidbrochart
Copy link
Contributor Author

I think it can be useful to have plugins register middlewares at the app-level, so that they change the entire app. This is needed e.g. in Quetz. For router-level middlewares, I think there is nothing to do in FPS, since it happens at router definition.
I'll try and add handling of app-level middleware conflicts.

Copy link
Member

@adriendelsalle adriendelsalle left a comment

Choose a reason for hiding this comment

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

Hey @davidbrochart thanks for working on this

I would say:

  • a plugin should not be allowed to register at application level something that would change any other plugins behavior (e.g. patching)
  • collisions between middlewares is not about same class used multiple times
  • the case of plugins dependencies may legitimate the fact that an upstream plugin may want to add a middleware or change the root of its downstream(s) plugin(s)
    • plugin2 depends on plugin1 and plugin-auth and would like to apply some middleware provided by plugin-auth on plugin1 routes
    • in that case we could make it possible for plugin2 to import plugin1 router(s) and inject a middleware. we still need capability to apply a middleware on specific routes, not globally for all plugins
    • collisions could be detected if 2 plugins try to apply a middleware on another one (and should be forbidden to avoid undefined behaviors)
  • an App, as a pure collection of plugins, could be also authorized modify the final app
    • the question of app of apps would raise the exact same questions mentioned in the previous bullet

TBH I think the solution to your needs is to push upstream (in fastapi and starlette) to make APIRouter capable to handle middlewares: fastapi/fastapi#1174

fps/app.py Outdated

for plugin_middleware, plugin_kwargs in middlewares:
if plugin_middleware in [
middleware.cls for middleware in app.user_middleware
Copy link
Member

Choose a reason for hiding this comment

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

I think the redefinition of a middleware is not related to having the same middleware class registered or not.
One may want to register the same middleware class for 2 different groups of routes (e.g. routers).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't we have the same situation for exception handlers?

Copy link
Member

Choose a reason for hiding this comment

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

when registering an exception handler:

  • there is no collision between exception classes or handlers (these are directly Python objects)
  • one need to import the exception (from a plugin dependency for example) to use it

So I would say no there is basically no possible interference between exception hanlders

@davidbrochart
Copy link
Contributor Author

davidbrochart commented Oct 19, 2021

TBH I think the solution to your needs is to push upstream (in fastapi and starlette) to make APIRouter capable to handle middlewares: tiangolo/fastapi#1174

It has been done here: encode/starlette#1286.
But I think an app-level middleware should be allowed too, what do you think @wolfv?

@adriendelsalle
Copy link
Member

adriendelsalle commented Oct 19, 2021

I haven't said it would not be useful ;)
Sorry for this answer but I think that the point of this project is to ensure by design that a plugin is not trying to act outer its scope.

Maybe we need to think about this at App level yes. And probably not allow multiple active Apps at the same time.

Any other thoughts about plugin systems @JohanMabille @fcollonval?

@wolfv
Copy link

wolfv commented Oct 20, 2021

I think it needs to be possible to register middleware at the global scope (even if it comes from a plugin).

But maybe the way to go is via declaration in a config file? So that you can say:

[middleware]
register_middleware = [
   my_auth_plugin.middleware,
   fast_api.gzip_middleware]

Then a per-plugin middleware is another problem that needs solving but route-specific middleware doesn't seem to be on a good track (if I interpret the fastapi / starlette responses properly).

@adriendelsalle
Copy link
Member

adriendelsalle commented Oct 20, 2021

Yeah it could be a way to go to declare the middlewares at FPS config (=the App). Thanks @wolfv for your help

@davidbrochart
Copy link
Contributor Author

Or maybe enabled_middlewares instead of register_middleware, since we already have a enabled_plugins option to enable a plugin as a whole, and plugins already register their middlewares with register_middleware.

@adriendelsalle
Copy link
Member

Just middlewares would be fine

fps/app.py Outdated
@@ -294,6 +294,7 @@ def _load_middlewares(app: FastAPI) -> None:
Config(FPSConfig).enabled_plugins
and p_name not in Config(FPSConfig).enabled_plugins
)
or p_name not in Config(FPSConfig).middlewares
Copy link
Member

Choose a reason for hiding this comment

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

the order of middlewares is not guaranteed here
I think you need to :

  • collect middlewares from enabled plugins
  • check middlewares listed in FPS config are found
  • add them on the app in the config order

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm wondering if the middlewares config should specify plugins, or middlewares inside plugins?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems natural to let each plugin specify its middlewares' order, and thus the middlewares config should rather specify plugins, but it looks like the middlewares' order we get in grouped_middlewares doesn't respect the registration order.

@masus04
Copy link

masus04 commented Mar 15, 2023

Any updates on this? Seems like a very promising change.

@davidbrochart
Copy link
Contributor Author

Actually we are going to use Asphalt instead of FPS in Jupyverse (see jupyter-server/jupyverse#277), so I'm not planning to maintain FPS anymore.

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.

4 participants