fix: metrics matching routes with path params first#2202
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideReorders Actix-web route registrations for PURL and vulnerability endpoints so that routes with path parameters are registered after more specific routes, ensuring metrics and routing match the intended handlers. Flow diagram for updated Actix route matching orderflowchart TD
subgraph PurlEndpoints
A["Incoming request /purl/recommend"] --> B{"Match route /purl/recommend?"}
B -- "yes" --> C["Dispatch to handler recommend"]
B -- "no" --> D{"Match route /purl/{id}?"}
D -- "yes" --> E["Dispatch to handler get"]
D -- "no" --> F["Return 404 or continue other routes"]
end
subgraph VulnerabilityEndpoints
G["Incoming request /vuln/analyze"] --> H{"Match route /vuln/analyze?"}
H -- "yes" --> I["Dispatch to handler analyze"]
H -- "no" --> J{"Match route /vuln/analyze_v3?"}
J -- "yes" --> K["Dispatch to handler analyze_v3"]
J -- "no" --> L{"Match route /vuln/{id}?"}
L -- "yes" --> M["Dispatch to handler get"]
L -- "no" --> N["Return 404 or continue other routes"]
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Because the behavior now depends on the registration order of the routes, consider adding an inline comment near the
.service(...)chain in both modules to explain whygetmust come after the more specific endpoints to prevent regressions from future refactoring. - To make the route ordering requirement harder to accidentally break, you might extract the
.service(...)chains into small helper functions (e.g.,register_specific_routesandregister_catch_all_routes) so that the distinction between specific and parameterized routes is more explicit.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Because the behavior now depends on the registration order of the routes, consider adding an inline comment near the `.service(...)` chain in both modules to explain why `get` must come after the more specific endpoints to prevent regressions from future refactoring.
- To make the route ordering requirement harder to accidentally break, you might extract the `.service(...)` chains into small helper functions (e.g., `register_specific_routes` and `register_catch_all_routes`) so that the distinction between specific and parameterized routes is more explicit.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Ruben Romero Montes <rromerom@redhat.com>
aa55af2 to
4a2cacd
Compare
|
If approved I guess this is something that needs to be included in the 2.2.1 release? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2202 +/- ##
=======================================
Coverage 68.64% 68.65%
=======================================
Files 379 379
Lines 21449 21449
Branches 21449 21449
=======================================
+ Hits 14723 14725 +2
+ Misses 5858 5851 -7
- Partials 868 873 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@ruromero I think the AI is right, and we should add a source code comment about this |
done |
Signed-off-by: Ruben Romero Montes <rromerom@redhat.com>
a464c29 to
ef67eab
Compare
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release/0.4.z
git worktree add -d .worktree/backport-2202-to-release/0.4.z origin/release/0.4.z
cd .worktree/backport-2202-to-release/0.4.z
git switch --create backport-2202-to-release/0.4.z
git cherry-pick -x 809b6d465b91aad2d171a985885cc3807efdcf92 693278b24af4ea601219abb5ddac7049540e1f11 |
Fix #2201
Summary by Sourcery
Bug Fixes: