-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
♻️ Refactor include_router
to mount sub-routers
#4794
base: master
Are you sure you want to change the base?
Conversation
🚀 Deployed on https://6256ed6ab06520389d671ffe--fastapi.netlify.app |
📝 Docs preview for commit edcae91 at: https://6256e9ac98ad6f36c315ea45--fastapi.netlify.app |
📝 Docs preview for commit 1c46aa2 at: https://6256edaac0ece33c275e6164--fastapi.netlify.app |
I like this very much! 👍 Could this also reduce the impact the performance issue |
I love the detailed documentation of the PR. |
This was approved by Tom: encode/starlette#1286. I didn't think we should follow that path, so we are inclined to encode/starlette#1464. |
Thanks Kludex! 🚀 |
This is amazing, thank you for detailing your thought process @tiangolo ! One thing I'm thinking about is: can we push state management up to the root level (maybe the With regards to building the Pydantic model: it's not hard to introspect into routes and traverse the routing tree with a visitor-like pattern. There's still some issues with this pattern though, namely:
|
Semi-related #5343. Calling |
encode/starlette#1649 merged in Starlette, jfyk |
This is a work in progress. 🚧
Description
Currently when a FastAPI app or a router uses
router.include_router(some_sub_router)
each of the routes insome_sub_router
is re-created inrouter
, so it is recreated in the parent router.The intention of this PR is to mount sub-routers using Starlette's regular mount.
This would allow several things:
.include_router()
requires calling it after all the routes are added, at the end of the file, or at another file after importing it (but this is still not possible in this PR).Alternative implementations and problems
There are several (too many) particular corner cases and exotic use cases that are currently supported by FastAPI, which makes this refactor/implementation non-trivial when trying to preserve compatibility for everything currently supported.
For that, I considered several approaches, but most of them have drawbacks and caveats.
General Caveats
Adding dependencies to a router means that it could require additional data from a request, e.g. more headers, or additional body fields. And when generating the OpenAPI for a specific route, it would need to have access to all that to generate the schema properly.
At the same time, ideally, the body should be validated/parsed by pydantic once, in a single point, and collect all the validation errors. So the body field should be generated once with all the fields, it wouldn't be great if the router with its dependencies has some body field and/or some additional request requirements, validates them, returns an early error, but doesn't include any other validation errors from the actual route.
Right now, for each route, a pydantic field is created for each route with a body. It's a pydantic field and not a pydantic model because a field can also contain a
List[X]
(a JSON array), not only JSON objects. But that field is created once when the route is created.All this is complicated by the fact that in pure Starlette each route doesn't have knowledge of its router, and in each router it doesn't have knowledge of any mount, or parent router. This information is (or could be) available at runtime from the ASGI scope, per request, but it would be ideal to build/compile the pydantic model once at the beginning, when starting the app, not every time there's a request.
Features to support
The idea is to be able to support the features described above. But on top of that, ideally, to support currently supported corner-case features, like:
APIRouter(prefix="/items")
).router.include_router(sub_router, prefix="/items")
)./items
that has a path/items
(instead of only supporting/items/
with the trailing slash)Note: this particular feature that is currently supported makes using the current Starlette mounts quite problematic, and that's where the current failing tests in this PR are.
/items
and/v2/items
:/items/
, one with a dependency and one without. This would not be supported by standard Starlette mounting.Possible implementations and parts of them
Recreate each route
Recreate each route and add it to the current router when using
router.include_router()
instead of mounting, this is the current behavior.Problems
Mount each router
Mount the router, add an attribute to each route with the
.router
it belongs to. And add an attribute to each router with the.parent_router
. That would allow finding the the parent dependencies and other metadata for each route.Problems
APIRouter(prefix="/items")
can't have a route/items
, it can only have/items/
.Call dependencies in router
One of the objectives is to be able to have dependencies per router that could be run for anything within that router's path prefix, even if there's no matching route, e.g. for logging. An option would be to call them in the router, assuming the router is mounted and called as an ASGI app (contrary to the current behavior of only copying the router's routes).
Problems
By default this wouldn't support extracting body parameters from those dependencies to add them to a single Pydantic body per router.
Mount router, copy router and routes, add default router
Mount the router with standard Starlette mount, this way each router could have it's own middleware and dependencies and execute them.
But the dependencies are needed per route to extract any additional data for pydantic fields/models per route, so it would still add the dependencies to each route, and create a default route to run dependencies and middleware when there's no match.
Each router has a
.parent_router
attribute pointing to the parent router, also a method.setup()
that reads the information that was set and the parent router's information to set its internal attributes. Then it also has a.copy()
method that creates a new instance of the same class with the same attributes as the current router.The copy allows setting a new parent router in the new instance and calling
.setup()
again, so that the new router can have the new parent dependencies, metadata, etc. but without overriding the old router.Each route would also have a
.router
attribute,.setup()
and.copy()
method.Problems
Mount router, copy routers/routes, custom mount
This is everything above plus an internal new custom
APIMount
extending Starlette's one, with a bunch of extra logic to support root paths under a prefix without a trailing slash.This, plus everything above, is mostly what this PR implements.
Problems
"/items"
and path""
to handle/items
) and redirecting the path with trailing slash to non-trailing slash:/items/
->/items
.Details of this PR
The way the current PR implements all this is by having the extra attributes
.router
(for routes).parent_router
(for rotuers) and the methods.setup()
,.copy()
, and having several new attributes like._router_dependencies
to store the dependencies added on instantiation, apart from the ones including the dependencies of the parent router an the chain of parent routers that are set on the normal.dependencies
attribute, and those are now set when calling.setup()
.This allows calling
.copy()
on a router or route to get a new instance with all the same attributes, and then set the new parent router, and then call.setup()
again to set any new dependencies, metadata, etc.Details and Problems
This PR is too big, the code is too complex to my taste. There's too much logic setting the private attributes to then recompute the final ones with
.setup()
.And because
.setup()
is called every time a router is included, in the end it will be called many times, although this is only on instantiation, at startup.The whole point of storing private attributes and then setting up the rest right after/during the instantiation is to have those values cached instead of computing them live. But this means there's a lof of "caches" in attributes/memory in the code, and that's always cumbersome to handle. One of the main points of this refactor is to better support subclasses of routers and routes, but all the extra logic to store private attributes and then
.setup()
on instantiation or after including them in other routers and the logic to.copy()
routers and routes, and then setting the new.router
or.parent_router
would make it more delicate and complex to implement those subclasses.This PR also has a lot of custom code to support the corner cases. For example, there's a custom
APIMount
with quite some logic duplicated from Starlette just to support routers with a prefix and a root route without a prefix (e.g.APIRouter(prefix="/items")
and a route inside that handles/items
, instead of/items/
).Questions
There's one existing feature that conflicts with a desired one (maybe more). Currently it is possible to include multiple routers under the same path (e.g. one with more restricted dependencies). But one of the objectives is to support middleware and dependencies per router that are executed even when there's no matching route (e.g. for logging, CORS). In this case, imagine there's:
Here we would probably want the routes in both routers to be included (as is currently the case).
But if there's a request for
/items
that doesn't match any path, should the dependencynew_auth
be executed?If both routers define different CORS middlewares, which one should be applied?
I'm also wondering if I can make the implementation simpler, probably without all the private attributes and copies, maybe computing the data when needed is not that expensive or maybe I can do all that in a simpler and safer way, I'm gonna try that now.