fix!: Remove stale arguments from Request and Response class #274
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 Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughConsolidates request/response surface to use a single Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Router as RelicRouter
participant StaticHandler
participant Response
Client->>Router: GET /static/logo.png
Note right of Router: route registered via StaticHandler.injectIn
Router->>StaticHandler: invoke call()
StaticHandler->>StaticHandler: locate file, build Body
StaticHandler->>Response: create 200 Response(body, headers)
Response-->>Client: 200 OK + content
sequenceDiagram
autonumber
participant OldFlow as Old Flow
participant NewFlow as New Flow
OldFlow->>OldFlow: Request.requestedUri + handlerPath
OldFlow->>OldFlow: manual path manipulation / copyWith(path)
OldFlow->>OldFlow: per-handler routing metadata
NewFlow->>NewFlow: Request.url (single URI)
NewFlow->>NewFlow: copyWith(url)
NewFlow->>NewFlow: Router.anyOf / routeWith registers handlers
NewFlow->>NewFlow: middleware sets req.matchedPath
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 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 #274 +/- ##
==========================================
+ Coverage 91.84% 91.94% +0.09%
==========================================
Files 93 93
Lines 3667 3611 -56
Branches 1863 1846 -17
==========================================
- Hits 3368 3320 -48
+ Misses 299 291 -8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
nielsenko
force-pushed
the
response-simplify
branch
2 times, most recently
from
a9d629a to
e2d4c0c
Compare
|
@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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
test/static/alternative_root_test.dart (2)
35-49: Fix test description: File is in subdirectory, not root.The test description claims to test "a root file with space" but the file is actually located in the
files/subdirectory (/files/with%20space.txt). The description should accurately reflect this.Apply this diff to fix the description:
test( - 'Given a root file with space when accessed then it returns the file content', + 'Given a file with space in subdirectory when accessed then it returns the file content', () async {
51-65: Fix path to match test description or remove duplicate test.This test has two issues:
- The description claims to test "unencoded space" but the path uses URL-encoded
%20(identical to line 44).- The description says "root file" but the file is in the
files/subdirectory.If the intent is to test unencoded space handling, the path should use an unencoded space character:
'/files/with space.txt'. Otherwise, this test is a duplicate of the previous one (lines 35-49) and should be removed.Apply this diff if testing unencoded space is the intent:
test( - 'Given a root file with unencoded space when accessed then it returns the file content', + 'Given a file with unencoded space in subdirectory when accessed then it returns the file content', () async { final handler = StaticHandler.directory( Directory(d.sandbox), cacheControl: (_, _) => null, ).asHandler; - final response = await makeRequest(handler, '/files/with%20space.txt'); + final response = await makeRequest(handler, '/files/with space.txt'); expect(response.statusCode, HttpStatus.ok);doc/site/docs/02-reference/04-requests.md (1)
27-46: Alignurldescription with new semantics.Line 27 now says
urlis the complete original URI, but the “Request url and path” section below still describes it as a handler-relative path+query. That’s the old behavior; please update this section to clarify thatRequest.urlis the full requested URI and point users tourl.pathAndQuery(or routing middleware properties) when they want a root‑relative path+query.lib/src/context/response.dart (2)
163-176:Response.notModifiedno longer enforces content-length stripping as documented.The doc comment still says that any
content-lengthinheaderswill be removed, but the constructor now forwardsheaders ?? Headers.empty()unchanged intoResponse. That can produce 304 responses with aContent-Lengthheader, which is against HTTP expectations and contradicts the docs. Consider wrapping the headers in a.transformthat explicitly clearscontentLengthor omits the header when building a 304.
268-277: UpdatecopyWithdocumentation to match the new signature.
copyWithnow accepts aBody?andHeaders?, but the doc block above still refers toString,List<int>, andStream<List<int>>bodies. Please update the comment to describe passing aBody(created viaBody.fromString,Body.fromBytes, etc.) so it matches the actual API and avoids confusion.
🧹 Nitpick comments (5)
example/routing/request_example.dart (1)
25-29: LGTM! API usage is correct.The code correctly uses
req.matchedPathfor the matched route path andreq.urlfor the full URI, properly implementing the refactored Request API.For consistency with
request_response_example.dart(line 29), consider using "Matched path" instead of "Relative URL" in the log message, as it more precisely describes whatmatchedPathrepresents.- log('Relative URL: $matchedPath, id: $id'); + log('Matched path: $matchedPath, id: $id');test/static/cache_busting_static_handler_test.dart (1)
15-379: Router-basedStaticHandlerwiring looks correct; consider deduplicating setupMigrating from pipeline-based handlers to:
handler = (RelicRouter() ..injectAt('/static', StaticHandler.directory(...))) .asHandler;(and variants such as
'/static/', multipleinjectAtcalls, and differingmountPrefix/fileSystemRoot) keeps each test’s intent intact:
- Requests now use full paths like
/static/logo.pngand/static/logo@abc.png, which aligns with the removal ofhandlerPath.- The scenarios for root-mounted vs router-mounted handlers, alternate mount points (
/other,/cache), custom separators, and nested directories with separators are all still exercised viamakeRequest.- Using the same
StaticHandlerinstance injected at multiple prefixes in the later groups also accurately captures the “handler-level cache busting” behavior under different mount configurations.Given how many groups repeat essentially the same
RelicRouter()..injectAt('/static'|'/static/', StaticHandler.directory(...))setup and nearly identicalCacheBustingConfigconstruction, you might want to factor out a small helper (e.g.,Handler buildStaticHandler(String mount, Directory root, {CacheBustingConfig config})) to reduce duplication and make future tweaks to router injection or cache-busting config less error-prone. This is purely an ergonomics/maintenance improvement; the current tests are clear and functionally sound.test/static/test_util.dart (1)
4-19: Consider clarifying naming inmakeRequest.
reqon Line 16 actually holds the handler’s result (usually aResponse), whilerequestis theRequest. Consider renamingreqtoresultorresponseto avoid confusion between the inbound request and the handler’s result.test/message/request_test.dart (1)
6-87: Tests correctly targeturl.pathAndQuery, but description is slightly stale.The assertions using
request.url.pathAndQueryfor various URIs look correct for the new API. The description “when no url is provided…” (Line 47) still reflects the old optional-url constructor; consider rephrasing to something like “when an absolute URL is provided,url.pathAndQueryexposes the relativized path and query” for clarity.test/src/adapter/context_test.dart (1)
2-93: Tests correctly exercisecopyWith(url: ...)and token preservation.Updating these tests to use
urlincopyWithand in assertions is consistent with the new Request API, and they still verify that the token survives multiple transformations. The group descriptions still mention “withRequest”, which is slightly outdated but non‑blocking—consider renaming in a follow‑up for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
README.md(1 hunks)doc/site/docs/02-reference/04-requests.md(1 hunks)doc/site/docs/02-reference/07-middleware.md(1 hunks)example/advanced/multi_isolate.dart(1 hunks)example/context/context_example.dart(1 hunks)example/routing/request_example.dart(1 hunks)example/routing/request_response_example.dart(1 hunks)lib/relic.dart(1 hunks)lib/src/adapter/adapter.dart(1 hunks)lib/src/adapter/io/request.dart(1 hunks)lib/src/adapter/io/response.dart(1 hunks)lib/src/context/message.dart(1 hunks)lib/src/context/request.dart(4 hunks)lib/src/context/response.dart(7 hunks)lib/src/handler/cascade.dart(1 hunks)lib/src/handler/handler.dart(1 hunks)lib/src/io/static/static_handler.dart(1 hunks)lib/src/middleware/context_property.dart(1 hunks)lib/src/middleware/middleware_logger.dart(2 hunks)lib/src/middleware/routing_middleware.dart(1 hunks)lib/src/relic_server.dart(2 hunks)test/handler/cascade_test.dart(1 hunks)test/message/request_test.dart(6 hunks)test/message/response_test.dart(1 hunks)test/middleware/middleware_object_test.dart(1 hunks)test/middleware/routing_middleware_test.dart(1 hunks)test/relic_server_serve_test.dart(1 hunks)test/router/router_handler_test.dart(1 hunks)test/router/router_inject_test.dart(1 hunks)test/src/adapter/context_test.dart(3 hunks)test/src/middleware/context_property_test.dart(1 hunks)test/static/alternative_root_test.dart(5 hunks)test/static/cache_busting_static_handler_test.dart(14 hunks)test/static/test_util.dart(1 hunks)test/util/test_util.dart(2 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/router_inject_test.dartlib/src/adapter/io/request.darttest/src/middleware/context_property_test.dartlib/src/adapter/io/response.darttest/static/alternative_root_test.dartlib/src/adapter/adapter.dartlib/src/io/static/static_handler.darttest/middleware/routing_middleware_test.dartlib/src/middleware/routing_middleware.dartlib/src/relic_server.darttest/router/router_handler_test.dartlib/src/middleware/context_property.darttest/middleware/middleware_object_test.dartexample/routing/request_example.darttest/src/adapter/context_test.dartexample/context/context_example.dartlib/src/handler/handler.dartlib/relic.darttest/util/test_util.darttest/handler/cascade_test.dartexample/routing/request_response_example.dartlib/src/context/message.darttest/relic_server_serve_test.darttest/message/request_test.dartlib/src/context/request.darttest/static/cache_busting_static_handler_test.darttest/static/test_util.dartlib/src/middleware/middleware_logger.dartexample/advanced/multi_isolate.dartlib/src/handler/cascade.darttest/message/response_test.dartlib/src/context/response.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/router_inject_test.darttest/src/middleware/context_property_test.darttest/static/alternative_root_test.darttest/middleware/routing_middleware_test.darttest/router/router_handler_test.darttest/middleware/middleware_object_test.darttest/src/adapter/context_test.darttest/util/test_util.darttest/handler/cascade_test.darttest/relic_server_serve_test.darttest/message/request_test.darttest/static/cache_busting_static_handler_test.darttest/static/test_util.darttest/message/response_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/adapter/io/request.dartlib/src/adapter/io/response.dartlib/src/adapter/adapter.dartlib/src/io/static/static_handler.dartlib/src/middleware/routing_middleware.dartlib/src/relic_server.dartlib/src/middleware/context_property.dartlib/src/handler/handler.dartlib/relic.dartlib/src/context/message.dartlib/src/context/request.dartlib/src/middleware/middleware_logger.dartlib/src/handler/cascade.dartlib/src/context/response.dart
🧠 Learnings (22)
📓 Common learnings
Learnt from: nielsenko
Repo: serverpod/relic PR: 76
File: lib/src/handler/handler.dart:20-20
Timestamp: 2025-05-23T04:15:01.420Z
Learning: In the relic codebase, ResponseContext is a subtype of HandledContext, making functions with signature ResponseContext Function(RequestContext) assignable to the Handler typedef (FutureOr<HandledContext> Function(NewContext ctx)) due to covariant return types and contravariant parameter types in Dart's type system.
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: 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-09-22T08:30:14.860Z
Learnt from: nielsenko
Repo: serverpod/relic PR: 179
File: lib/src/middleware/routing_middleware.dart:42-43
Timestamp: 2025-09-22T08:30:14.860Z
Learning: In Relic's routing middleware, Uri.decodeFull(req.url.path) is used to decode incoming request paths before routing. This is correct because routes are defined with plain paths (not percent-encoded), so incoming requests need to be decoded to match the route definitions. The concern about %2F changing segmentation is not relevant since route definitions don't use percent-encoding.
Applied to files:
doc/site/docs/02-reference/07-middleware.mdREADME.mddoc/site/docs/02-reference/04-requests.mdexample/routing/request_example.dartexample/routing/request_response_example.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 lib/**/*.dart : Use router with trie-based matching and symbol-based path parameters (e.g., `#name`, `#age`) for routing
Applied to files:
doc/site/docs/02-reference/07-middleware.mdtest/router/router_inject_test.dartlib/src/adapter/io/request.dartlib/src/io/static/static_handler.darttest/middleware/routing_middleware_test.dartlib/src/middleware/routing_middleware.darttest/router/router_handler_test.dartREADME.mddoc/site/docs/02-reference/04-requests.mdexample/routing/request_example.dartexample/context/context_example.dartlib/src/handler/handler.dartexample/routing/request_response_example.darttest/static/cache_busting_static_handler_test.dart
📚 Learning: 2025-10-20T14:05:34.503Z
Learnt from: nielsenko
Repo: serverpod/relic PR: 226
File: lib/src/router/relic_app.dart:147-156
Timestamp: 2025-10-20T14:05:34.503Z
Learning: In the relic codebase, hot-reload and other development-time features in lib/src/router/relic_app.dart should fail loudly (raise exceptions) rather than catch and suppress errors, as this helps developers diagnose issues during development.
Applied to files:
test/router/router_inject_test.dartlib/src/relic_server.darttest/router/router_handler_test.darttest/middleware/middleware_object_test.darttest/src/adapter/context_test.darttest/relic_server_serve_test.darttest/static/cache_busting_static_handler_test.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/router_inject_test.darttest/src/middleware/context_property_test.darttest/static/alternative_root_test.darttest/middleware/routing_middleware_test.darttest/middleware/middleware_object_test.darttest/src/adapter/context_test.darttest/util/test_util.darttest/handler/cascade_test.darttest/relic_server_serve_test.darttest/message/request_test.dart
📚 Learning: 2025-04-24T04:14:12.943Z
Learnt from: nielsenko
Repo: serverpod/relic PR: 47
File: test/hijack/relic_hijack_test.dart:82-90
Timestamp: 2025-04-24T04:14:12.943Z
Learning: Tests within a single file in Dart's test package run sequentially, not concurrently, so global state for test resources within a file doesn't present race condition risks.
Applied to files:
test/router/router_inject_test.darttest/middleware/routing_middleware_test.darttest/middleware/middleware_object_test.darttest/src/adapter/context_test.darttest/handler/cascade_test.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 : Use Arrange-Act-Assert pattern within test bodies
Applied to files:
test/router/router_inject_test.darttest/middleware/routing_middleware_test.darttest/middleware/middleware_object_test.darttest/src/adapter/context_test.darttest/handler/cascade_test.darttest/message/request_test.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 lib/**/*.dart : Use Uint8List for request/response bodies for performance; avoid List<int> for body payloads
Applied to files:
lib/src/adapter/io/request.dartlib/src/adapter/io/response.dartlib/relic.darttest/relic_server_serve_test.dartlib/src/context/response.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 lib/**/*.dart : Use type-safe HTTP header parsing and validation when accessing headers
Applied to files:
lib/src/adapter/io/request.dartlib/src/adapter/io/response.dartlib/src/relic_server.dartlib/src/context/message.darttest/message/request_test.dart
📚 Learning: 2025-05-23T04:15:01.420Z
Learnt from: nielsenko
Repo: serverpod/relic PR: 76
File: lib/src/handler/handler.dart:20-20
Timestamp: 2025-05-23T04:15:01.420Z
Learning: In the relic codebase, ResponseContext is a subtype of HandledContext, making functions with signature ResponseContext Function(RequestContext) assignable to the Handler typedef (FutureOr<HandledContext> Function(NewContext ctx)) due to covariant return types and contravariant parameter types in Dart's type system.
Applied to files:
test/src/middleware/context_property_test.dartlib/src/adapter/io/response.darttest/router/router_handler_test.dartlib/src/middleware/context_property.dartlib/src/handler/handler.darttest/util/test_util.dartlib/src/context/request.darttest/static/test_util.dartlib/src/handler/cascade.darttest/message/response_test.dartlib/src/context/response.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 lib/**/*.dart : Ensure WebSocket handling includes proper lifecycle management (e.g., ping/pong for connection health)
Applied to files:
lib/src/adapter/io/response.dartlib/src/adapter/adapter.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:
lib/src/io/static/static_handler.dartlib/src/middleware/routing_middleware.dart
📚 Learning: 2025-05-09T07:55:38.627Z
Learnt from: nielsenko
Repo: serverpod/relic PR: 62
File: lib/src/router/router.dart:31-38
Timestamp: 2025-05-09T07:55:38.627Z
Learning: In the Relic router implementation, attaching a sub-router doesn't require clearing the parent router's static cache because it only adds new routes without affecting existing cache entries. The sub-router's cache is cleared primarily to save memory.
Applied to files:
lib/src/io/static/static_handler.darttest/static/cache_busting_static_handler_test.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 : Place tests in the `test/` directory mirroring the `lib/` structure
Applied to files:
test/middleware/routing_middleware_test.darttest/middleware/middleware_object_test.darttest/src/adapter/context_test.darttest/handler/cascade_test.darttest/relic_server_serve_test.darttest/message/request_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:
lib/src/middleware/routing_middleware.dart
📚 Learning: 2025-10-22T15:43:43.812Z
Learnt from: nielsenko
Repo: serverpod/relic PR: 216
File: test/relic_server_test.dart:44-47
Timestamp: 2025-10-22T15:43:43.812Z
Learning: In Dart, the `Uri.http` constructor's path parameter (`unencodedPath`) is optional and defaults to an empty string. Code like `Uri.http('localhost:$port')` is valid without supplying a path argument.
Applied to files:
doc/site/docs/02-reference/04-requests.mdtest/relic_server_serve_test.darttest/message/request_test.dartlib/src/context/request.dart
📚 Learning: 2025-11-04T10:24:49.925Z
Learnt from: nielsenko
Repo: serverpod/relic PR: 264
File: lib/src/context/context.dart:4-4
Timestamp: 2025-11-04T10:24:49.925Z
Learning: In Dart, cyclic imports between libraries are not a problem and are handled by the language without compile-time errors, unlike some other programming languages.
Applied to files:
lib/src/handler/handler.dart
📚 Learning: 2025-10-22T11:25:39.264Z
Learnt from: nielsenko
Repo: serverpod/relic PR: 216
File: lib/src/router/relic_app.dart:47-49
Timestamp: 2025-10-22T11:25:39.264Z
Learning: In the serverpod/relic repository, validation of the `noOfIsolates` parameter should be handled in the `RelicServer` constructor (lib/src/relic_server.dart), not in `RelicApp.run` (lib/src/router/relic_app.dart).
Applied to files:
test/relic_server_serve_test.dart
📚 Learning: 2025-06-04T15:12:15.865Z
Learnt from: nielsenko
Repo: serverpod/relic PR: 91
File: lib/src/util/util.dart:49-62
Timestamp: 2025-06-04T15:12:15.865Z
Learning: In Dart, StreamController instances created locally in methods and returned as sinks don't cause resource leaks when abandoned. The garbage collector automatically reclaims the entire object graph (sink → controller → stream → subscription) when there are no external references, as StreamControllers don't hold system resources requiring explicit cleanup.
Applied to files:
test/relic_server_serve_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/message/request_test.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 : Tests should follow the Given-When-Then pattern in descriptions (flexible structuring allowed)
Applied to files:
test/message/request_test.dart
📚 Learning: 2025-04-24T14:06:32.810Z
Learnt from: nielsenko
Repo: serverpod/relic PR: 48
File: example/example.dart:31-36
Timestamp: 2025-04-24T14:06:32.810Z
Learning: In the example code, `sleep()` is intentionally used instead of `await Future.delayed()` to simulate CPU-bound work that benefits from multiple isolates/cores. Using a blocking call demonstrates why multiple isolates are necessary, while an async approach would allow a single isolate to handle multiple requests concurrently, defeating the purpose of the multi-isolate example.
Applied to files:
example/advanced/multi_isolate.dart
🔇 Additional comments (35)
lib/src/middleware/routing_middleware.dart (1)
121-122: LGTM! Documentation enhancement aids migration.The added note clearly communicates that
matchedPathreplaces the deprecatedhandlerPathproperty, helping users understand the API evolution and migrate their code accordingly.test/static/alternative_root_test.dart (3)
28-28: LGTM: Simplified path correctly references root file.The simplified path '/root.txt' correctly accesses the root-level file created in setUp.
76-76: LGTM: Path correctly references file in subdirectory.The simplified path '/files/test.txt' correctly accesses the file created in the
files/subdirectory.
92-92: LGTM: Path correctly tests non-existent file handling.The simplified path '/not_here.txt' appropriately tests the 404 response for a non-existent file.
example/context/context_example.dart (1)
122-122: LGTM! Correct usage of the new API.The change correctly uses
req.matchedPathto access the matched route path, aligning with the PR's removal ofRequest.handlerPathand introduction of the routing middleware'smatchedPathextension.example/routing/request_response_example.dart (1)
26-30: LGTM! Excellent example of the new API.The code correctly demonstrates the refactored Request API with precise terminology. Using "Matched path" in the log message clearly communicates that
matchedPathrepresents the matched route path from the routing middleware.lib/src/adapter/io/response.dart (1)
3-21: Import switch toresult.dartis consistent with the context refactorUsing
../../context/result.dartkeepsResponseand related types available to this IO adapter, and the existingwriteHttpResponseimplementation remains correct withBody.read()returning aStream<Uint8List>. No further changes needed here.lib/src/context/message.dart (1)
1-43:part of 'result.dart'alignsMessagewith the new context modulePointing this part file at
result.dartmatches the broader context consolidation without changingMessagebehavior (headers/body,readAsString,encoding, etc.). Looks good.test/middleware/routing_middleware_test.dart (1)
2-12: Test import update toresult.dartmatches the new context layoutSwitching to
package:relic/src/context/result.dartkeepsRequestInternaland related types available, and_requestnow correctly constructsRequestwith the new(method, url, token)signature viaUri.http. The tests continue to exercise routing andpathParametersas intended.test/message/response_test.dart (1)
173-188: Encoding moved fromResponsetoBodyis consistent with the new APIPassing
encoding: latin1intoBody.fromStringand leaving theResponseconstructor unaware of encoding matches the refactor whereBodyowns encoding. The subsequentcopyWithandencodingexpectations remain valid becauseResponse.encodingstill delegates tobody.bodyType?.encoding. No issues here.lib/src/context/request.dart (1)
1-120:Requestsimplification and URL validation align well with the new API surfaceThe new shape of
Request:
- Promotes a single, authoritative
url(final Uri url;) in place ofrequestedUri/handlerPath.- Validates the URL eagerly in the private constructor via
pathSegments/queryParametersAll, plus explicit checks forisAbsoluteand an emptyfragment, which is appropriate for actual HTTP requests.- Keeps
copyWithstraightforward by allowingheaders,url, andbodyoverrides while preserving method, protocol version, and token; this matches howResponse.copyWithbehaves in the tests.The
UriExextension:extension UriEx on Uri { Uri get pathAndQuery => Uri(path: path, query: query); }provides the intended “root-relative” representation without mutating the original
Uri, and integrates cleanly with the migration guidance to userequest.url.pathAndQuerywhere a non-absolute URI is needed. If you ever find you mainly need a string form, you could optionally add a helper likeString get pathAndQueryString => pathAndQuery.toString()on top of this, but the current design is already sound.example/advanced/multi_isolate.dart (1)
47-51: LGTM!The parameter rename from
requesttoreqimproves naming consistency across the codebase. The corresponding usage update toreq.urlis correct.lib/src/adapter/adapter.dart (1)
5-5: LGTM!The import path update from
context.darttoresult.dartaligns with the documented breaking changes in this PR.lib/src/middleware/context_property.dart (1)
1-1: LGTM!The import path update correctly reflects the context module reorganization.
lib/src/adapter/io/request.dart (1)
6-6: LGTM!The import path update is correct and consistent with the broader refactoring.
test/middleware/middleware_object_test.dart (1)
2-2: LGTM!The import path update is correct for the test file and aligns with the context module reorganization.
lib/src/handler/handler.dart (1)
3-3: LGTM!The import path update correctly references the reorganized context module.
test/router/router_inject_test.dart (1)
4-4: LGTM!The import path update is correct and maintains consistency across test files.
test/src/middleware/context_property_test.dart (1)
2-2: LGTM!The import path update is correct and completes the migration to the reorganized context module structure.
test/handler/cascade_test.dart (1)
2-2: LGTM! Import path updated correctly.The import path change from
context/context.darttocontext/result.dartaligns with the repository-wide context module restructuring.doc/site/docs/02-reference/07-middleware.md (1)
180-182: LGTM! Documentation accurately reflects the new API.The warning correctly states that rewriting
request.urlin middleware attached withrouter.use(...)will not re-route the request or update routing metadata, which aligns with the PR's changes to the Request API.lib/src/handler/cascade.dart (1)
1-1: LGTM! Import path updated correctly.The import path change from
context/context.darttocontext/result.dartis consistent with the context module restructuring across the codebase.lib/src/relic_server.dart (2)
5-5: LGTM! Import path updated correctly.The import change aligns with the context module restructuring.
180-183: LGTM! Error logging updated to use the new API.The logging correctly uses
request.url.pathandrequest.url.queryinstead of the removedrequestedUriproperty, maintaining the same functionality.test/router/router_handler_test.dart (1)
2-2: LGTM! Import path updated correctly.The import path change from
context/context.darttocontext/result.dartis consistent with the repository-wide refactoring.test/relic_server_serve_test.dart (1)
74-77: LGTM! Test updated correctly for the new API.The test now uses
req.urlinstead ofreq.requestedUri, and correctly expects/foo/barwith a leading slash (asUri.pathincludes the leading slash). These changes align with the PR's API refactoring.lib/relic.dart (1)
8-8: LGTM! Public API export updated correctly.The export path change from
context/context.darttocontext/result.dartaligns with the context module restructuring, while preserving thehide RequestInternaldirective to maintain API encapsulation.test/util/test_util.dart (2)
8-8: LGTM! Import path updated correctly.The import change aligns with the context module restructuring.
30-30: LGTM! Test utility updated to use the new API.The helper function now correctly uses
req.url.pathinstead of the removedreq.requestedUri.path, maintaining the same functionality with the new API.lib/src/io/static/static_handler.dart (1)
122-126: StaticHandler.injectIn wiring looks correct.Registering
Directoryunder/**andFileunder/forGET/HEADonly matches the internal use ofremainingPathand keeps method validation centralized in_serveFile. This aligns with the router-based mounting pattern introduced in the PR.README.md (1)
96-112: Routing documentation update looks consistent.The new paragraph and example around
Router,routeWith, androuter.asHandleralign with the router‑centric model used elsewhere in the README and tests (colon path params + symbol keys). No issues from a behavior or clarity standpoint.lib/src/middleware/middleware_logger.dart (2)
14-42: logRequests refactor preserves behavior.The expanded closure form with
localLoggerkeeps the same timing, logging, and error‑handling semantics while improving readability. Usingrequest.urlin_messagecorrectly reflects the new URL API.
48-61: URL logging update matches new Request.url model.Switching
_messageto log${url.path}${_formatQuery(url.query)}viarequest.urlmaintains the previous path+query behavior now thaturlrepresents the original requested URI. This keeps log output stable across the API change.test/message/request_test.dart (1)
134-169: copyWith tests align with newurl-based Request API.The
copyWith()test correctly checks thaturlandurl.pathAndQueryare preserved when no arguments are provided, and that body streaming still works via the controller. This is a good regression guard for the simplifiedcopyWithimplementation.lib/src/context/response.dart (1)
1-1: Part directive matches the new library unit.Changing this file to
part of 'result.dart';is consistent with the context/result refactor described in the PR and keeps Response within the new public library surface.
test: Fix all tests after refactor
…e urls (pathAndQuery).
nielsenko
force-pushed
the
response-simplify
branch
from
e2d4c0c to
908af86
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull Request Overview
This PR simplifies the Request and Response classes by removing deprecated properties and parameters. The changes consolidate the URL representation into a single url property (repurposed from the old requestedUri), remove the handlerPath property in favor of extension methods, and simplify Response constructors by removing encoding and context parameters. Additionally, the core context file is renamed from context.dart to result.dart.
Key changes:
- Removed
Request.handlerPath- replaced withmatchedPathextension method from routing middleware - Renamed
Request.requestedUritoRequest.url(repurposing the oldurlfield name) - Removed
pathparameter fromRequest.copyWith() - Removed
encodingandcontextparameters from allResponseconstructors - Renamed
lib/src/context/context.darttolib/src/context/result.dart
Reviewed Changes
Copilot reviewed 35 out of 36 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/src/context/request.dart | Simplified Request class by removing handlerPath/url properties and complex validation; renamed requestedUri to url; added UriEx extension |
| lib/src/context/response.dart | Removed encoding and context parameters from all Response constructors |
| lib/src/context/result.dart | Renamed from context.dart; contains Result, Request, Response, and Message classes |
| lib/src/middleware/routing_middleware.dart | Added documentation clarifying matchedPath replaces handlerPath |
| test/static/cache_busting_static_handler_test.dart | Refactored tests to use buildStaticHandler helper and removed handlerPath parameter |
| test/static/alternative_root_test.dart | Simplified test requests by removing handlerPath parameter |
| test/message/request_test.dart | Updated tests to use new url property and removed handlerPath/url validation tests |
| example/routing/*.dart | Updated examples to use matchedPath instead of url and renamed requestedUri to url |
| doc/site/docs/02-reference/*.md | Updated documentation to reflect new URL semantics |
SandPod
left a comment
SandPod
left a comment
There was a problem hiding this comment.
One minor question, otherwise the changes looks good.
2 participants
Description
This PR refactors the
Requestclass by removing the oldurl(the name has been repurposed) andhandlerPathproperties and their associated complexity. Theurlproperty was a relative path used for routing, whilehandlerPathrepresented the path to the current handler. Both have been removed in favor of usingrequestedUri(which is now calledurl) andmatchedPath/remainingPathextension methods (ContextPropertyconvenience getters) directly throughout the codebase.Key changes:
urlandhandlerPathproperties from theRequestclass, including complex validation logic forurlandhandlerPathrelationships.requestedUriinstead ofurl.requestedUrito url.Requestconstructor andcopyWithmethodpathparameter fromcopyWithmethodcontext.darttoresult.dartencodingandcontextparameters fromResponseconstructorsStaticHandlerto implementinjectInmethodRelated Issues
None referenced.
Pre-Launch Checklist
///), ensuring consistency with existing project documentation.Breaking Changes
Breaking changes:
Request.urlrepurposed to be oldRequest.requestedUriinsteadRequest.handlerPathremoved - use routing middleware'smatchedPathpropertyRequest.copyWith(path: ...)removed.Responseconstructors no longer acceptencodingorcontextparameters.lib/src/context/context.dart→lib/src/context/result.dart(affects direct imports)Additional Notes
This should barely impact Serverpod, but it is a significant API refactoring. Users should migrate by:
request.url. Userequest.url.pathAndQueryif root relative uri is required.request.matchedPathfrom routing middleware where needed.patharguments fromrequest.copyWith()callsBody.ContextPropertyinstead ofcontextmap.Summary by CodeRabbit
Documentation
New Features
Bug Fixes / Improvements
✏️ Tip: You can customize this high-level summary in your review settings.