Home Original page

🐛 Fix mount subapp with APIRouter by BilalAlpaslan · Pull Request #5373 · fastapi/fastapi

@BilalAlpaslan

@BilalAlpaslan

@BilalAlpaslan

@codecov

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (cf73051) 100.00% compared to head (8fa13ba) 100.00%.
Report is 1038 commits behind head on master.

❗ Current head 8fa13ba differs from pull request most recent head 1441fea. Consider uploading reports for the commit 1441fea to get more accurate results

Additional details and impacted files 
@@            Coverage Diff            @@
##            master     #5373   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          540       541    +1     
  Lines        13969     13986   +17     
=========================================
+ Hits         13969     13986   +17

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@BilalAlpaslan BilalAlpaslan changed the title 🐛 Fix mount subapp with APIrouter 🐛 Fix mount subapp with APIRouter

Sep 11, 2022

@github-actions

iudeen

Choose a reason for hiding this comment

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

LGTM

mbroton

def mount(self, path: str, app: ASGIApp, name: Optional[str] = None) -> None:
route = routing.Mount(path, app=app, name=name)
if route.__getattribute__("path") == "":
route.__setattr__("path", "/")

Choose a reason for hiding this comment

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

These are magic methods, IMO they shouldn't be called explicitly as the inbuilt functions that use the magic methods can perform additional checks.
If there's no particular reason to use magic method directly, It would be better to use setattr/getattr.

Choose a reason for hiding this comment

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

yes, you're right. I have e three solution for that and @Kludex i need your opinion
1- solve this issue in fastAPI: override mount function and change the path
if route.path == "":
route.path = "/"
2-override Mount class and change this line: self.path = path.rstrip("/")
3-change this code on starlette
or maybe anyone has a better idea

Choose a reason for hiding this comment

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

What is the problem here? 😅

Choose a reason for hiding this comment

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

if mount asgi app on APIRouter Starlette router.Mount object return ' ' for path='/'. self.path = path.rstrip("/") cause this and fastapi raise Prefix and path cannot be both empty

Choose a reason for hiding this comment

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

@tiangolo

@Kludex

@tiangolo

It seems this was related to the ASGI root_path stuff, and handled in Starlette, right? Then I'm gonna close this one. 🤓 ☕

If you still have issues, please create a discussion with a minimal self contained example that I can copy paste and run to understand the use case better. 🤓