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

Feature: allow including afaststream.broker.router.BrokerRouter() in a StreamRouter #1168

Closed
strangemonad opened this issue Jan 24, 2024 · 2 comments · Fixed by #1747
Closed
Labels
Core Issues related to core FastStream functionality and affects to all brokers enhancement New feature or request good first issue Good for newcomers

Comments

@strangemonad
Copy link

Is your feature request related to a problem? Please describe.

Consider the following scenario: You have an existing module that's based on a BrokerRouter (e.g. a NatsRouter) and it only has subscribers and publishers. You want to be able to re-use that module in plain FastStream apps and other apps (e.g. a FastAPI app). This means that at the top-level, you need to do something like

app_router = faststream.nats.fastapi.NatsRouter() # Any subclass of StreamRouter
app_router.include_router(shared_broker_routes.router) # This raises because `BrokerRouter.routes` doesn't exist.
app_router.include_router(shared_api_routes.router)
app = FastAPI()
# ...

Describe the solution you'd like
It would be nice to not have to change existing BrokerRouter routes

a few options might be

  • change StreamRouter.include_router to conditionally call super().include_router()
  • change BrokerRouter to expose an empty routes property (this would work for Nats + FastAPI / Starlette but I'm not sure how universal that would be.
  • Attempt to unify the concepts of BrokerRouter handlers and and APIRouter routes

Describe alternatives you've considered
I currently can't directly share the code, I have to force everything to be a fastapi StreamRouter.

@strangemonad strangemonad added the enhancement New feature or request label Jan 24, 2024
@Lancetnik
Copy link
Member

Thanks for the Idea!

But, I am not sure, that is required due Stream Routers' can include each over. But it is a great way to reuse your regular FastStream routers in Fast API application... I need to think and take a look on implementation. Added to backlog, but it is still feature to discuss.

@Lancetnik Lancetnik added the Core Issues related to core FastStream functionality and affects to all brokers label May 16, 2024
@Lancetnik Lancetnik added the good first issue Good for newcomers label Aug 27, 2024
@Lancetnik
Copy link
Member

It should be easy-to-do with 0.5.19 FastAPI integration structure

github-merge-queue bot pushed a commit that referenced this issue Aug 30, 2024
)

* feat (#1168): allow include regular router to FastAPI integration

* docs: generate API References

* docs: update FastAPI integration to use BrokerRouter

* tests: add FastAPI include regular router testcase

---------

Co-authored-by: Lancetnik <Lancetnik@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core Issues related to core FastStream functionality and affects to all brokers enhancement New feature or request good first issue Good for newcomers
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants