Bugfix: app.include_router doesn't merge nested lifespans

[Lancetnik]

I am working on my own Propan library and as a part of it I implemented a custom FastAPI router with the following behavior.

from fastapi import FastAPI
from propan.fastapi import RabbitRouter

router = RabbitRouter("amqp://guest:guest@localhost:5672")
app = FastAPI()
app.include_router(router)

To connect RabbitMQ at Fast API startup I used router.on_startup events and all worked fine, but now it is deprecated. So I migrate to lifespan and now Fast API doesn't run router lifespan at the code above.

I suppose it was missed cuz the original include_router method includes all nested on_startup and on_shutdown deprecated events here, but doesn't includes a router lifespan.

So, the following code doesn't work

from contextlib import asynccontextmanager
from fastapi import FastAPI, APIRouter

@asynccontextmanager
async def router_lifespan(app):
    print('router start')
    yield
    print('router shutdown')

router = APIRouter(lifespan=router_lifespan)
app = FastAPI()
app.include_router(router)

The current PR fixes the original framework miss.
All original test and lint.sh checks works fine.

[github-actions]

📝 Docs preview for commit 860c851 at: https://647f7cdb633e285f9b5dace9--fastapi.netlify.app

[github-actions]

📝 Docs preview for commit 984e59f at: https://647f7f0b29b2f764bcaa3dd9--fastapi.netlify.app

[tiangolo]

📝 Docs preview for commit 8f26ed5 at: https://6484cca7bf187a105b276a5e--fastapi.netlify.app

[Lancetnik]

@tiangolo can you take a look here, please? It's a minor bugfix, but it's important for my project.

[tiangolo]

📝 Docs preview for commit f322c99 at: https://648c2ec07c8ac9541405829c--fastapi.netlify.app

[tiangolo]

📝 Docs preview for commit 3e2fd98 at: https://648e16b538df29410a152328--fastapi.netlify.app

[Kludex]

@tiangolo can you take a look here, please? It's a minor bugfix, but it's important for my project.

This is not minor, and ideally we'd like this to live in Starlette.

[Lancetnik]

@Kludex maybe, but how we can move it to Starlette if include_router is the FastAPI-level method? Do you suggest move the method to Starlette?

[Lancetnik]

This will require a lot of work in both FastAPI and Starlette, since none of the FastAPI methods uses super().add_route() from Starlette. Therefore, it is necessary to rewrite all the methods add_api_route, add_websocket_route, add_api_websocket_route to use the parent methods of Starlette. It will also require a consistent update of these two packages. It seems to me that this is too difficult way, but I will do it. Perhaps it makes sense to merge the current PR for now before we can make a more elegant solution.

[Lancetnik]

@Kludex, so can you check a Starlette PR?

[Lancetnik]

This is not minor, and ideally we'd like this to live in Starlette.

@Kludex this bug doesn't on Starlette side and not connected with this Issue or this Starlette PR or Issue.

It should be fixed in FastAPI cuz we have no Starlette calls inside include_router method. It doesn't use Starlette Host or Mount that can be nested at Starlette side, but copies all included router attributes to app.router directly. So, we should merge the incuded lifespan here too.

So can you review and approve this PR?

[Kludex]

Starlette should support running the lifespan of other routers... It shouldn't be here.

[Lancetnik]

Starlette should support running the lifespan of other routers... It shouldn't be here.

But there no other routers. We have the original app.router and copy all routes of included router to it. What do you want to do with it?

@Kludex just take a look at FastAPI original code here

A FastAPI application works with an only one original router (if we didn't use app.mount to mount another FastAPI application). So how we can use Starlette lifespan resolving logic here?

[ghost]

Would love the functionality to get restored/implemented, recently upgraded fastapi code that previously made use of @router.on_event("startup") inside a router object. But the switch to lifespan has broken this functionality, when the router is included into a FastAPI() object. I don't know the codebase well enough, but is the FastAPI object+ router object 's lifespan code part of Starlette?

[Lancetnik]

@neeps yes, lifespan functional is a part of Starlette, but incude_router is a FastAPI custom method and the bug is here.

[ghost]

Thank you for clarifying, I've downgraded to pre-lifespan for the time being.

[Lancetnik]

@Kludex sorry for being annoying. I know, that

Refactor routers, to re-use router instances insteaad of cloning path operations

Is in roadmap and It will fix the problem

But why we can't merge this PR for now? It just a few lines of code and it fixes the problem already without any waiting for abstract "roadmap". Working solution is better than perfect, but in the future, isn't it?

[logankaser]

If we can't fix this now, would you accept a PR to raise a NotImplementedError
when some poor fool like me tries to use the lifespan parameter on APIRouter
only to tear his application apart trying to figure out why it was not running?

[Tishka17]

Any updates on this PR? Is it going to be merged?

@tiangolo

[abdullahsych]

Any updates on this PR? @tiangolo

[Praneethvvs]

Thank you for clarifying, I've downgraded to pre-lifespan for the time being.

Hi, I'm trying to use lifespan with ApiRouter() instead of FastApi instance. It is still not working. Can you pin the latest stable version where the startup/shutdown was functional.