-
Notifications
You must be signed in to change notification settings - Fork 12
feat(trie): Add support for setting up mappings of values on lookup trie.use(path, map)
#185
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughIntroduces per-subpath value mapping to PathTrie with a new use(...) API and composed map application during lookup. Tightens generic bounds to T extends Object across Router, PathTrie, and routing middleware. Adds tests validating mapping behavior across path patterns. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Router
participant PathTrie
participant Node as Trie Node(s)
Client->>Router: handle(request)
Router->>PathTrie: lookup(normalizedPath)
rect rgba(217,235,255,0.4)
note right of PathTrie: Traverse segments (literal/param/wildcard)
loop for each segment
PathTrie->>Node: descend(segment)
Node-->>PathTrie: child node + optional mapper
note right of PathTrie: Compose mapping if present
end
end
PathTrie-->>Router: TrieMatch(value [, composedMap])
alt composedMap present
note right of Router: Apply composedMap to value
end
Router-->>Client: route match (final value)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks and finishing touches and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #185 +/- ##
==========================================
+ Coverage 91.79% 91.84% +0.04%
==========================================
Files 85 85
Lines 3133 3150 +17
Branches 1620 1627 +7
==========================================
+ Hits 2876 2893 +17
Misses 257 257 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
ad3d327 to
8fea006
Compare
…trie.use(path, map)`
8fea006 to
6debe0a
Compare
|
The test coverage could be improved, but mostly for some error situations, such as setting up mappings on the same node twice, or attaching nodes to each other with conflicting mappings. |
SandPod
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes looks good!
Have one question regarding the order of operations. Would be good to get your thoughts on that.
|
Changed semantics slightly in 24462c2. You are now allowed to call Also extended test coverage a bit. |
|
Found a bug in |
…cumulate by composing with existing function)
|
Changed semantics in 7e6e34a to allow |
SandPod
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👍
Description
Adds
PathTrie.use()method to apply transformation functions to values on lookup. When a mapping is registered at a path, it transforms the value at that path and all descendant paths during lookup. Multiple mappings along a path compose from leaf to root.Key changes:
mapfield to_TrieNodeto store transformation functionsuse()method to register mappings at specific pathslookup()to compose and apply mappings during traversalattach()for overlapping mapsThe higher purpose for this change is to allow for setup of middleware at various points in the routing tree. PRs to update
Router<T>as well as exposing inServerpodwill follow.Related Issues
Pre-Launch Checklist
Breaking Changes
Summary by CodeRabbit