feat!: Support back-tracking during routing #297
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 a recursive lookup implementation for PathTrie with a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (5 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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #297 +/- ##
==========================================
- Coverage 92.00% 91.93% -0.07%
==========================================
Files 97 97
Lines 3664 3682 +18
Branches 1881 1888 +7
==========================================
+ Hits 3371 3385 +14
- Misses 293 297 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/src/router/path_trie.dart (1)
464-465: Consider removing in a future cleanup.The preserved iterative implementation serves as a useful reference during this transition. Once the recursive implementation is validated in production, consider removing this dead code to reduce maintenance burden.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/src/router/path_trie.dart(1 hunks)test/router/path_trie_test.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.dart
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.dart: All Dart code must pass static analysis usingdart analyze --fatal-infoswith no issues
All Dart files must be formatted withdart format(CI enforcesdart format --set-exit-if-changed .)
Files:
test/router/path_trie_test.dartlib/src/router/path_trie.dart
test/**/*.dart
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
test/**/*.dart: Tests should follow the Given-When-Then pattern in descriptions (flexible structuring allowed)
Use Arrange-Act-Assert pattern within test bodies
Provide clear, descriptive test titles; prefer single responsibility per test unless related assertions improve clarity
Place tests in thetest/directory mirroring thelib/structure
Files:
test/router/path_trie_test.dart
lib/**/*.dart
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
lib/**/*.dart: Use Uint8List for request/response bodies for performance; avoid List for body payloads
Use type-safe HTTP header parsing and validation when accessing headers
Use router with trie-based matching and symbol-based path parameters (e.g.,#name,#age) for routing
Ensure WebSocket handling includes proper lifecycle management (e.g., ping/pong for connection health)
Files:
lib/src/router/path_trie.dart
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: serverpod/relic PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-09T16:21:09.310Z
Learning: Applies to lib/**/*.dart : Use router with trie-based matching and symbol-based path parameters (e.g., `#name`, `#age`) for routing
Learnt from: nielsenko
Repo: serverpod/relic PR: 186
File: lib/src/router/router.dart:60-64
Timestamp: 2025-10-03T08:29:41.568Z
Learning: In the Router class in lib/src/router/router.dart, the use() method stores transformation functions on trie nodes. These functions are composed and applied during lookup (in PathTrie.lookup() at line 405), but the original stored route values are never modified. The transformation is temporary and only affects the returned value from lookup operations.
Learnt from: nielsenko
Repo: serverpod/relic PR: 48
File: lib/src/handler/handler.dart:59-67
Timestamp: 2025-04-25T07:39:38.915Z
Learning: Nielsenko prefers using switch statements with pattern matching over if statements when working with sealed classes in Dart, as they provide exhaustiveness checking at compile time and can be more concise.
📚 Learning: 2025-10-09T16:21:09.310Z
Learnt from: CR
Repo: serverpod/relic PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-09T16:21:09.310Z
Learning: Applies to lib/**/*.dart : Use router with trie-based matching and symbol-based path parameters (e.g., `#name`, `#age`) for routing
Applied to files:
test/router/path_trie_test.dartlib/src/router/path_trie.dart
📚 Learning: 2025-05-05T14:40:00.323Z
Learnt from: nielsenko
Repo: serverpod/relic PR: 52
File: lib/src/router/router.dart:37-53
Timestamp: 2025-05-05T14:40:00.323Z
Learning: In the Router and PathTrie implementation in Dart, both static and dynamic routes consistently throw ArgumentError when attempting to add duplicate routes, despite comments suggesting dynamic routes would be overwritten with a warning.
Applied to files:
test/router/path_trie_test.dartlib/src/router/path_trie.dart
📚 Learning: 2025-10-09T16:21:09.310Z
Learnt from: CR
Repo: serverpod/relic PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-09T16:21:09.310Z
Learning: Applies to test/**/*.dart : Provide clear, descriptive test titles; prefer single responsibility per test unless related assertions improve clarity
Applied to files:
test/router/path_trie_test.dart
📚 Learning: 2025-05-05T16:06:15.941Z
Learnt from: nielsenko
Repo: serverpod/relic PR: 52
File: lib/src/router/normalized_path.dart:72-86
Timestamp: 2025-05-05T16:06:15.941Z
Learning: When writing tests for Dart classes with private constructors and factory methods, extending the class for test purposes may not be possible. The NormalizedPath class in particular uses a private constructor and factory constructor pattern.
Applied to files:
test/router/path_trie_test.dart
📚 Learning: 2025-05-09T10:11:33.427Z
Learnt from: nielsenko
Repo: serverpod/relic PR: 62
File: lib/src/router/router.dart:0-0
Timestamp: 2025-05-09T10:11:33.427Z
Learning: In the Router implementation, re-adding a route with the same path isn't currently allowed and will throw an ArgumentError from _allRoutes.add(), but handling cache invalidation or updates proactively is important for future-proofing in case route updates are later supported.
Applied to files:
test/router/path_trie_test.dart
📚 Learning: 2025-10-03T08:29:41.568Z
Learnt from: nielsenko
Repo: serverpod/relic PR: 186
File: lib/src/router/router.dart:60-64
Timestamp: 2025-10-03T08:29:41.568Z
Learning: In the Router class in lib/src/router/router.dart, the use() method stores transformation functions on trie nodes. These functions are composed and applied during lookup (in PathTrie.lookup() at line 405), but the original stored route values are never modified. The transformation is temporary and only affects the returned value from lookup operations.
Applied to files:
test/router/path_trie_test.dartlib/src/router/path_trie.dart
🔇 Additional comments (9)
lib/src/router/path_trie.dart (5)
365-367: LGTM!Clean delegation to the recursive implementation with appropriate initial parameters: root node, starting index 0, root's map function, and empty parameters map.
378-399: Verify that tail node's map is intentionally not applied in the fallback case.When the path exactly matches a prefix ending at a tail (e.g.,
/archivematching/archive/**), the fallback retrieves the value fromdynamicSegment.node(line 387) but appliescurrentMap(line 392), which doesn't includedynamicSegment.node.map.The iterative implementation has the same behavior, so this is consistent. Confirm this is intentional—if a map is set via
use()on the tail route itself, it won't be applied when matching the prefix without the tail segment.
401-416: LGTM! Clean backtracking implementation.The literal-first approach with fallback to dynamic segments is well-structured. If the literal path leads to a dead end (returns null), execution naturally falls through to attempt dynamic segment matching, implementing backtracking without explicit stack management.
418-449: LGTM!Dynamic segment handling is correct:
- Parameters correctly extract values into the params map using
Symbol(dynamicSegment.name)- Tail segments return immediately when a value exists, capturing the remaining path
- Non-tail segments (parameters/wildcards) continue the recursive search
The distinction between tail (greedy match) and parameter/wildcard (single segment match with continued traversal) is properly implemented.
454-462: LGTM!The composition order
outer(inner(v))correctly applies child node maps first, then parent maps—matching the composition logic inuse()at line 190 and the iterative implementation at lines 476-481. Based on learnings, this ensures transformation functions are properly composed during lookup.test/router/path_trie_test.dart (4)
116-134: LGTM!The skip annotation with a clear message appropriately documents that this test verifies the old (non-backtracking) behavior. The new expected behavior is covered by the test at lines 138-149 in the Backtracking group.
137-149: LGTM! Excellent test coverage for the core backtracking behavior.This test directly demonstrates the breaking change: paths that previously returned null (when a literal match led to a dead end) now backtrack and match alternative parameter routes. The test clearly documents the new expected behavior.
204-213: LGTM!Good edge case coverage. The test correctly expects null because:
- "guest" matches the
:idparameter, but:idnode has no "settings" child- Backtracking cannot switch to the literal "admin" path since we're already at "users" level
- This confirms backtracking respects the trie structure boundaries
228-316: LGTM! Comprehensive tail route backtracking coverage.These tests thoroughly cover tail route (
/**) interactions with backtracking:
- Tail as fallback when literal paths fail (lines 228-245)
- Tail combined with parameter routes (lines 247-265)
- Nested tail precedence (lines 267-286)
- Tail with intermediate literal nodes (lines 288-315)
The assertions for
matched.pathandremaining.pathcorrectly verify the path splitting behavior documented inTrieMatch.
nielsenko
force-pushed
the
back-tracking
branch
from
2d9fc82 to
94c20e7
Compare
|
This does dethrone us ever so slightly: ❯ ./benchmark/benchmark.exe run -v
Setting up benchmark data with 1000 routes...
Setup complete.
Starting benchmarks
Add;Static;x1000;Routingkit;RunTime;512.0287342085708;us
Add;Static;x1000;Spanner;RunTime;443.9005;us
Add;Static;x1000;Router;RunTime;302.7725717685557;us
Lookup;Static;x1000;Routingkit;RunTime;195.55810561120296;us
Lookup;Static;x1000;Spanner;RunTime;131.90784647629027;us
Lookup;Static;x1000;Router;RunTime;136.60510102683008;us
Add;Dynamic;x1000;Routingkit;RunTime;731.6195;us
Add;Dynamic;x1000;Spanner;RunTime;3724.7884615384614;us
Add;Dynamic;x1000;Router;RunTime;556.02675;us
Lookup;Dynamic;x1000;Routingkit;RunTime;1786.8708333333334;us
Lookup;Dynamic;x1000;Spanner;RunTime;345.2196118488253;us
Lookup;Dynamic;x1000;Router;RunTime;355.00105894811156;us
Done😄 |
SandPod
left a comment
SandPod
left a comment
There was a problem hiding this comment.
Nice quick work, couple of comments.
nielsenko
force-pushed
the
back-tracking
branch
from
a9c5506 to
37f1e31
Compare
SandPod
left a comment
SandPod
left a comment
There was a problem hiding this comment.
LGTM Given that a config option is exposed that could disable back-track and that the last tets are fixed.
nielsenko
force-pushed
the
back-tracking
branch
from
bdffc1a to
7bacdb1
Compare
|
Rebased on main after merge of #295 |
nielsenko
force-pushed
the
back-tracking
branch
from
7bacdb1 to
7f9a294
Compare
2 participants
Description
PathTrie.lookup()from iterative to recursive implementation_lookupIterative()for reference_composeMap()helper for combining map functionsRelated Issues
Pre-Launch Checklist
///), ensuring consistency with existing project documentation.Breaking Changes
Routes that previously returned
nullwhen a literal match led to a dead end will now backtrack and potentially match a parameter or tail route instead.Additional Notes
One test skipped: "literal segments are prioritized over parameters, even if it prevents a match" - this behavior is intentionally changed by backtracking.
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.