feat!: Router now supports verb directly #65
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 📝 WalkthroughWalkthroughThe changes introduce HTTP method-aware routing to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant Handler
Client->>Router: add(Method.get, "/foo", handlerA)
Router->>Router: Store handlerA for GET on "/foo"
Client->>Router: add(Method.post, "/foo", handlerB)
Router->>Router: Store handlerB for POST on "/foo"
Client->>Router: lookup(Method.get, "/foo")
Router->>Router: Find handlerA for GET on "/foo"
Router-->>Client: handlerA
Client->>Router: lookup(Method.post, "/foo")
Router->>Router: Find handlerB for POST on "/foo"
Router-->>Client: handlerB
Client->>Router: lookup(Method.put, "/foo")
Router->>Router: No handler for PUT on "/foo"
Router-->>Client: null
Assessment against linked issues
Possibly related PRs
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #65 +/- ##
==========================================
+ Coverage 80.29% 80.54% +0.25%
==========================================
Files 78 78
Lines 2304 2339 +35
Branches 1300 1321 +21
==========================================
+ Hits 1850 1884 +34
- Misses 454 455 +1 ☔ 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: 1
🧹 Nitpick comments (2)
benchmark/benchmark.dart (1)
236-238: Typo in class name (DynamicLookupSpanneBenchmark)The missing “r” makes the spelling inconsistent with the other benchmark classes and with the package name. It is only cosmetic, but keeping names consistent improves discoverability (e.g., for IDE search).
-class DynamicLookupSpanneBenchmark extends RouterBenchmark { - DynamicLookupSpanneBenchmark() : super('Spanner Lookup Dynamic x$routeCount'); +class DynamicLookupSpannerBenchmark extends RouterBenchmark { + DynamicLookupSpannerBenchmark() + : super('Spanner Lookup Dynamic x$routeCount');Remember to update the instantiation in
driver():- DynamicLookupSpanneBenchmark().report(); + DynamicLookupSpannerBenchmark().report();lib/src/router/router.dart (1)
26-31: Provide more helpful error details when a verb is already registered
ArgumentError('$method already registered')gives no information about where the duplication happened. Including the offending path (or at least clarifying the message) makes debugging far easier.- throw ArgumentError('$method already registered'); + throw ArgumentError( + 'A route for HTTP method $method is already registered on this path');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
benchmark/benchmark.dart(10 hunks)lib/src/router/path_trie.dart(3 hunks)lib/src/router/router.dart(3 hunks)pubspec.yaml(2 hunks)test/router/router_methods_test.dart(1 hunks)test/router/router_test.dart(23 hunks)
🧰 Additional context used
🧠 Learnings (1)
test/router/router_test.dart (1)
Learnt from: nielsenko
PR: serverpod/relic#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.
🔇 Additional comments (13)
pubspec.yaml (2)
11-11: Updated collection package dependencyThe
collectionpackage has been updated to version^1.18.0from a previous version, which is likely required for compatibility with the new router method-aware functionality.
26-26: Added spanner router package for benchmarkingThe addition of the
spannerpackage as a development dependency is appropriate for the benchmark classes that were introduced to compare routing performance.test/router/router_test.dart (2)
26-27: Router API updated to support HTTP method awarenessThe existing test suite has been properly updated to use the new method-aware API, consistently replacing
router.add(path, value)withrouter.get(path, value)androuter.lookup(path)withrouter.lookup(Method.get, path). The test logic remains the same, ensuring backward compatibility while adapting to the new HTTP method-aware routing.Also applies to: 33-34, 40-41, 47-48, 55-56, 63-64, 82-83, 89-90, 96-97, 114-115, 121-122, 140-141, 147-148, 168-169, 175-176, 196-197, 203-204, 230-231, 244-245, 268-269, 285-286, 292-293, 300-301, 319-320, 326-327, 354-355, 361-362, 368-369, 389-390, 396-397, 418-419, 425-426, 446-447, 453-454, 476-477, 483-484, 500-501, 507-508, 523-524, 530-531, 546-547, 562-563, 583-605, 627-628, 634-636, 639-641, 645-647, 651-652, 668-670
572-578: Consistent HTTP method awareness in tests with various path patternsThe updated test case maintains proper coverage for different path variations (with/without leading/trailing slashes) and dynamic routes, ensuring the method-aware routing works correctly with all path configurations.
lib/src/router/path_trie.dart (3)
12-12: Simplified LookupResult constructorThe
isDynamicparameter has been removed from theLookupResultconstructor, streamlining the implementation. This simplification is appropriate as the dynamic route indication is no longer necessary or tracked in the returned lookup results.
83-107: Added new method for in-place updates to the PathTrieThe new
addOrUpdateInPlacemethod is a well-designed addition that facilitates updating trie node values based on their current state. This is essential for implementing HTTP method-aware routing where each node might store multiple handlers (one per HTTP method).The implementation properly reuses the existing
_buildmethod to ensure parameter conflict checks are preserved, and the documentation clearly explains the method's purpose and behavior.
275-275: Updated lookup method return statementThe return statement has been correctly updated to match the simplified
LookupResultconstructor, no longer including theisDynamicflag.test/router/router_methods_test.dart (5)
1-53: Well-structured method-specific routing testsThis comprehensive test suite for HTTP method-specific routing is well-designed. The use of parameterized testing elegantly verifies that all HTTP methods (GET, POST, PUT, etc.) work correctly with the router, avoiding code duplication while ensuring thorough coverage.
63-96: Thorough testing of the 'any' method registrationThe tests for the
router.any()method thoroughly verify that a handler registered with this method is correctly returned for all HTTP methods. The separate test that explicitly checks all methods in a loop provides complete coverage of this important functionality.
98-164: Comprehensive conflict detection testsThe conflict detection tests are thorough and verify multiple important scenarios:
- Conflicts between same-method registrations (GET + GET)
- Conflicts using different registration approaches (
router.get()vsrouter.add())- Conflicts between specific methods and
any()registrations- Absence of conflicts between different paths with different methods
These tests ensure that route registration conflicts are properly detected and reported.
166-226: Method handling with parameterized pathsThese tests effectively verify that method-specific routing works correctly with parameterized paths, ensuring that:
- Parameters are correctly extracted for specific HTTP methods
- Different methods can be registered for the same parameterized path
- Lookups with non-matching methods return null
- The
any()method works with parameterized pathsThis is a critical aspect of the router functionality and is well-tested.
228-254: Edge case testing for unregistered paths/methodsThe tests properly verify the behavior of the router when looking up unregistered paths or methods:
- Empty router returns null for any path/method
- Lookup of non-existent path returns null
- Lookup with wrong method returns null
These tests ensure the router behaves correctly in edge cases and doesn't return incorrect handlers.
lib/src/router/router.dart (1)
167-170:any()may throw unexpectedly when called repeatedly
any()iterates overMethod.valuesand callsadd()for each.
If the same path is later registered again (e.g., another module re-exports routes),add()will throw, but the exception originates deep insideany(). Consider either:
- Documenting that
any()must be called only once per (path, value) pair, or- Adding a fast-fail check (e.g.,
containsRoute) before the loop.This will help users avoid surprising runtime errors.
SandPod
left a comment
SandPod
left a comment
There was a problem hiding this comment.
Changes look good!
Relative performance is looking good as well!
Description
Router class now require method(/verb) on add and lookup. Hence, handlers can now defer to the router to ensure they are invoked with correct method.
The extra cost is kept to a constant factor (O(1)) for both speed and space.
Related Issues
Pre-Launch Checklist
Please ensure that your PR meets the following requirements before submitting:
///), ensuring consistency with existing project documentation.Breaking Changes
Public interface of
Router<T>changedBenchmark results
2,4 GHz 8-Core Intel Core i9
Summary by CodeRabbit
spannerrouter package.spannerpackage and updatingcollectionto a newer version.