-
Notifications
You must be signed in to change notification settings - Fork 436
Description
Summary
The codebase uses two different patterns to access the application's root path:
request.scope.get("root_path", "")- ~80 occurrencessettings.app_root_path- ~15 occurrences
While both approaches currently return the same value (FastAPI sets scope["root_path"] from its root_path parameter in __call__), the codebase should use a consistent pattern.
Related PR
This issue was identified during the review of #1547, which fixed the root redirect in main.py to use settings.app_root_path instead of request.scope.get("root_path", "").
Technical Analysis
Why both work the same:
FastAPI's __call__ method (in fastapi/applications.py:1131-1134) sets the scope's root_path:
async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None:
if self.root_path:
scope["root_path"] = self.root_path
await super().__call__(scope, receive, send)Since mcpgateway initializes FastAPI with root_path=settings.app_root_path (in main.py:617), both patterns return the same value.
Why standardization matters:
- Consistency: Reduces cognitive load when reading/maintaining code
- Clarity:
settings.app_root_pathis explicit about the source - Future-proofing: Doesn't depend on FastAPI's internal behavior
- Testability:
settings.app_root_pathcan be easily mocked without request context
Files Affected
Using request.scope.get("root_path", "") (~80 occurrences):
mcpgateway/admin.py- 75 occurrences (redirects, template contexts)mcpgateway/utils/verify_credentials.py- 3 occurrencesmcpgateway/routers/sso.py- 1 occurrencemcpgateway/routers/oauth_router.py- 1 occurrence
Already using settings.app_root_path (~15 occurrences):
mcpgateway/main.py- Fixed in fix: root redirect uses app_root_path with trailing slash #1547mcpgateway/admin.py- Some template contexts (lines 2646, 2718, 2766, etc.)- Various other locations
Additional Finding: Missing Trailing Slashes
Some redirects use /admin instead of /admin/, which causes an unnecessary 307 redirect when FastAPI normalizes the trailing slash. Example from admin.py:2764:
return RedirectResponse(url=f"{root_path}/admin", status_code=303)Should be:
return RedirectResponse(url=f"{root_path}/admin/", status_code=303)Proposed Solution
- Replace all
request.scope.get("root_path", "")withsettings.app_root_path - Add trailing slashes to redirect URLs where appropriate
- Consider creating a helper function for consistent redirect URL generation
Acceptance Criteria
- All occurrences of
request.scope.get("root_path", "")replaced withsettings.app_root_path - Redirect URLs use consistent trailing slash pattern
- All existing tests pass
- No functional changes to behavior