Fix path param warning for request handlers#1393
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR reduces false-positive path parameter warnings by detecting when handlers can access route parameters through type annotations or reserved parameter names like ChangesPath Parameter Warning Suppression
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
robyn/router.py (1)
78-83: 💤 Low valueMake path-param access detection robust to postponed (stringified) annotations
_handler_can_access_path_paramschecksparam.annotation in _PATH_PARAM_ACCESS_TYPES(i.e.,Request/PathParams). Withfrom __future__ import annotations(PEP 563),inspect.Parameter.annotationis typically the string"Request"/"PathParams", so this check fails and the logic falls back to name-based detection—meaning the “Route … declares path params … but handler … doesn't use them” warning can reappear for handlers that rely on type annotations but don’t use the reserved parameter names. This matches the existing limitation inwrapped_handler’s identity-based checks (e.g.,handler_param_type is Requestaround line 214). Resolve annotations first (e.g., viatyping.get_type_hints(handler)) before comparing toRequest/PathParamsif you want it to be robust.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@robyn/router.py` around lines 78 - 83, _handler_can_access_path_params currently checks param.annotation against _PATH_PARAM_ACCESS_TYPES but fails for postponed (string) annotations; change its signature to accept the handler callable (or obtain the handler where it's called) and use typing.get_type_hints(handler) to resolve annotations before comparing types to _PATH_PARAM_ACCESS_TYPES, then fall back to the existing name-based check (using _REQUEST_PARAM_NAMES | _PATH_PARAMS_PARAM_NAMES) if resolution fails; also catch and ignore TypeError/NameError from get_type_hints so the function degrades gracefully and keep behavior consistent with wrapped_handler's identity checks (e.g., comparisons to Request/PathParams).unit_tests/test_router_path_param_warnings.py (1)
88-103: ⚡ Quick winSolid negative case; consider locking the partial-coverage boundary.
The tests cover full suppression and full-miss, but not the in-between: a handler that names one declared param yet has no
Request/PathParamsaccess to the remaining one (e.g.,def handler(user_id): ...on/users/:user_id/posts/:post_id). Adding that case would pin down whether the warning fires for the uncoveredpost_id, preventing future regressions in the detection heuristic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@unit_tests/test_router_path_param_warnings.py` around lines 88 - 103, Add a new unit test that covers the partial-coverage case: create a handler that accepts only one of the declared path params (e.g., def handler(user_id): return "ok"), register it with add_get_route(Router(), "/users/:user_id/posts/:post_id", handler), capture warnings via caplog (set WARNING for "robyn.router") and assert a single warning is emitted that references the route and specifically mentions the uncovered "post_id" (and does not claim "user_id" is unused), using the same helpers path_param_warning_messages and naming the test like test_path_param_warning_when_one_param_unused to locate the behavior in the test suite.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@robyn/router.py`:
- Around line 78-83: _handler_can_access_path_params currently checks
param.annotation against _PATH_PARAM_ACCESS_TYPES but fails for postponed
(string) annotations; change its signature to accept the handler callable (or
obtain the handler where it's called) and use typing.get_type_hints(handler) to
resolve annotations before comparing types to _PATH_PARAM_ACCESS_TYPES, then
fall back to the existing name-based check (using _REQUEST_PARAM_NAMES |
_PATH_PARAMS_PARAM_NAMES) if resolution fails; also catch and ignore
TypeError/NameError from get_type_hints so the function degrades gracefully and
keep behavior consistent with wrapped_handler's identity checks (e.g.,
comparisons to Request/PathParams).
In `@unit_tests/test_router_path_param_warnings.py`:
- Around line 88-103: Add a new unit test that covers the partial-coverage case:
create a handler that accepts only one of the declared path params (e.g., def
handler(user_id): return "ok"), register it with add_get_route(Router(),
"/users/:user_id/posts/:post_id", handler), capture warnings via caplog (set
WARNING for "robyn.router") and assert a single warning is emitted that
references the route and specifically mentions the uncovered "post_id" (and does
not claim "user_id" is unused), using the same helpers
path_param_warning_messages and naming the test like
test_path_param_warning_when_one_param_unused to locate the behavior in the test
suite.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c1667911-04c8-4d42-8566-717632ebb6f1
📒 Files selected for processing (2)
robyn/router.pyunit_tests/test_router_path_param_warnings.py
1 participant
Description
This PR fixes #1336.
Summary
This PR updates the path-param warning heuristic so it respects Robyn's supported request access patterns. Routes with path params no longer warn when the handler can access those params through
Request,PathParams, reserved request aliases, or direct path-param arguments.The warning still fires when a route declares path params and the handler has no supported way to access them.
Tests cover direct param access, request aliases, typed
Request,path_params, typedPathParams, multi-param request access, and the real inaccessible-param warning case.PR Checklist
Please ensure that:
Pre-Commit Instructions:
Validation run locally:
python -m pytest unit_testspre-commit run --all-filesgit diff --checkSummary by CodeRabbit
Bug Fixes
Tests