fix(asgi): resolve path template for routers added with include_router - #907
adityaanikam wants to merge 1 commit into
Conversation
CagriYonca
left a comment
There was a problem hiding this comment.
A few comments, then it looks good
| @@ -55,7 +71,9 @@ def _collect_kvs(self, scope: Dict[str, Any], span: "InstanaSpan") -> None: | |||
There was a problem hiding this comment.
Hi @adityaanikam,
Thanks for working on this bug!
I would suggest using the public helper iter_route_contexts in fastapi.routing introduced in FastAPI ≥ 0.137.2 that flattens the _IncludedRouter tree into matchable route context objects, each carrying the full templated path. For FastAPI 0.137.0–0.137.1 (which lack iter_route_contexts but still have _IncludedRouter), effective_route_contexts() on the router node serves as a fallback. For FastAPI < 0.137, routes are already flat and need no special handling.
This would avoid relying on _match(), which is a private Starlette method with no stability guarantee — a future Starlette or FastAPI release could rename it, change its return type, or remove it entirely, breaking path_tpl collection silently for all requests without any warning at startup or runtime.
For more details, refer the official documentation
cfdf4c8 to
a4aa388
Compare
|
Thanks both, I reworked it along both reviews.
I ran the FastAPI tests against 0.136.1, 0.137.0, 0.137.1 and 0.141.1, and the included-router cases fail on the unmodified code. |
|
Hello again @adityaanikam , could you rebase your code to the main? CI tests will pass, then we can merge it. Thanks a lot for your efforts! |
FastAPI 0.137 stores routers added with include_router() in a lazy wrapper that has no path attribute. _collect_kvs read route.path unconditionally, so it raised AttributeError, which was swallowed and logged at debug level, and http.path_tpl was never set for those endpoints. Flatten the routes with FastAPI's public iter_route_contexts() (0.137.2 and later), or with effective_route_contexts() on the wrapper for 0.137.0 and 0.137.1, so every route carries its fully prefixed path. Older FastAPI versions and Starlette keep their routes flat and are handled as before. Fixes instana#906 Signed-off-by: adityaanikam <adityanikam9502@gmail.com>
a4aa388 to
bc03d4b
Compare
CagriYonca
left a comment
There was a problem hiding this comment.
Thanks for the changes, it looks good to me now. Let's wait until @GSVarsha also reviews, then we can merge.
Fixes #906
With FastAPI 0.137 and later, routers added with
include_router()are kept in a lazy_IncludedRouterwrapper that has nopathattribute.InstanaASGIMiddleware._collect_kvsreadroute.pathfor the matched route, so it raisedAttributeErrorfor every request to those endpoints. The exception is swallowed and logged at debug level, andhttp.path_tplwas never set, so those endpoints lost their path template.This flattens the app's routes before matching, with a new
_iter_routeshelper. It uses FastAPI's publiciter_route_contexts()(0.137.2 and later), falls back toeffective_route_contexts()on the wrapper for 0.137.0 and 0.137.1, and leaves older FastAPI versions and plain Starlette apps unchanged, since their routes are already flat. Each flattened route carries its full templated path, including every router prefix, so routers nested several levels deep are covered. The private_match()is no longer used.Matchanditer_route_contextsare imported at the top of the module behindImportErrorguards, so Starlette and FastAPI stay optional andinstana.middlewarestill imports without them.Testing: added
test_path_templates_with_included_routerswith a single level and a nestedinclude_router()route, andtest_iter_routes_without_iter_route_contextsfor the fallback branches. The two router cases fail on the unmodified code (nopath_tplon the span) and pass with the change. Rantests/frameworks/test_fastapi.pyagainst FastAPI 0.136.1, 0.137.0, 0.137.1 and 0.141.1 (14 passed each), andtest_fastapi_middleware.py,test_starlette.pyandtest_starlette_middleware.pywith 0.141.1 (12 passed).ruff checkis clean on the changed files, andruff format --checkonasgi.pyandtest_fastapi.py.