fix: add RootPathMiddleware for proper static file routing with root_…#996
fix: add RootPathMiddleware for proper static file routing with root_…#996JartanFTW wants to merge 12 commits intosmithyhq:mainfrom
Conversation
|
For whatever reason this issue has disappeared even without the fix 😂 Not sure what's going on, or if this PR is necessary. |
aminalaee
left a comment
There was a problem hiding this comment.
Can you please add a test to verify it's working as expected?
From my original post:
Can you please clarify what specific testing you want done? |
|
@JartanFTW add a test which after redirecting to some page, check page url, and make sure that, root_path is in the URL |
|
@JartanFTW any updates? Could you also add support for starlette 1.0 which was recently released? You just need to change dependencies = [
"starlette==0.49.3,<0.50.0; python_version == '3.9'",
"starlette>=0.50; python_version >= '3.10'",
"wtforms >=3.1, <3.2",
"jinja2",
"python-multipart",
"sqlalchemy >= 2.0",
] |
|
It's on my todo list, just very busy at the moment and this is low priority for me. Apologies! |
…path When FastAPI/Starlette is deployed with root_path configured (e.g., behind a reverse proxy), static file requests could fail with 404 because Starlette's internal routing couldn't properly resolve paths for nested mounts. This middleware normalizes request paths by prepending root_path when it's set but not included in the request path, ensuring StaticFiles and other nested mounts work correctly. Fixes smithyhq#538
The middleware was on the inner admin Starlette app, where Mount had already appended base_url to root_path, causing double-prefix rewrites. Moving it to the outer app and scoping it to admin paths fixes routing for static files and all admin routes behind a reverse proxy.
d3f1378 to
caad855
Compare
|
Design decisions
What the middleware does not cover
Open question: Starlette pin for Python 3.9 |
Prevents the middleware from matching routes like /admin-panel when path_prefix is /admin by requiring an exact match or a / boundary.
|
Ok now I'm actually done. Let me know how it looks |
tests/test_root_path.py
Outdated
|
|
||
| session_maker = sessionmaker(bind=engine) | ||
|
|
||
| Base = declarative_base() # type: ignore |
There was a problem hiding this comment.
I know this is copied, but no need for type: ignore. I will remove others as well.
yes, totally agree. |
Allow compatible patch releases instead of exact pin, per review feedback.
Per review feedback from mmzeynalli.
…upport The ajax_lookup_url for Select2 relationship dropdowns was computed once at startup via urljoin, which never included root_path. Behind a reverse proxy, the AJAX data-url in HTML was wrong, causing 404s. Move URL generation to the Jinja2 template layer using url_for(), matching how every other URL in sqladmin is generated. The pre-computed URL on ModelView is kept as a fallback for backwards compatibility with custom templates.
|
@JartanFTW would @MaximDementyev's solution work, instead of this much change? |
…path
When FastAPI/Starlette is deployed with root_path configured (e.g., behind a reverse proxy), static file requests could fail with 404 because Starlette's internal routing couldn't properly resolve paths for nested mounts.
This middleware normalizes request paths by prepending root_path when it's set but not included in the request path, ensuring StaticFiles and other nested mounts work correctly.
Fixes #538
All pytest cases pass. I also tested that this fix works via a docker-compose deployment on JartanFTW/sqladmin/tree/test/root-path-deployment