feat: add stream handler for pure Http Streaming#223
Open
francescovallone wants to merge 4 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds streaming response support: new RestHandler/StreamHandler types, Controller.onStream registration, propagation of a streaming flag through RouteContext, HTTP adapter streaming reply (chunked writes, flush, error handling), and response API extensions (add/flush/toggleBuffering). Changes
Sequence DiagramsequenceDiagram
participant Client
participant Controller
participant Routes as RouteRegistry
participant RouteExec as RouteExecution
participant HTTP as HTTPAdapter
participant Resp as Response
Client->>Controller: register GET /stream via onStream()
Controller->>Routes: store RestRouteHandlerSpec(streaming: true)
Note over Client,Resp: later — incoming request
Client->>RouteExec: GET /stream
RouteExec->>Routes: match route
RouteExec->>StreamHandler: call(RequestContext)
StreamHandler->>RouteExec: returns Stream<item>
RouteExec->>HTTP: send Stream as body
HTTP->>Resp: set chunked transfer, toggleBuffering(false)
loop per emitted item
StreamHandler->>HTTP: emit item
HTTP->>Resp: add/write chunk
HTTP->>Resp: flush()
end
HTTP->>Resp: close() (finally)
Resp->>Client: streamed chunks complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![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 (1)
packages/serinus/lib/src/adapters/serinus_http_server.dart (1)
164-196: Streaming implementation looks solid.The chunked transfer encoding approach is correct. A couple of observations:
Minor redundancy: Line 167 sets
content-typeheader again, but it's already set at line 161. This is harmless but could be removed.Good practices observed: Disabling buffering, proper error handling with finally block, and chunk type validation.
♻️ Optional: Remove redundant header assignment
if (bodyData is Stream) { headers[io.HttpHeaders.dateHeader] = _cachedDate; response.toggleBuffering(false); - headers[io.HttpHeaders.contentTypeHeader] = contentTypeValue; headers[io.HttpHeaders.transferEncodingHeader] = 'chunked';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/serinus/lib/src/adapters/serinus_http_server.dart` around lines 164 - 196, The code assigns the Content-Type header twice when handling a Stream response; remove the redundant headers[io.HttpHeaders.contentTypeHeader] = contentTypeValue; inside the bodyData is Stream block so the Content-Type is only set once (keep the initial assignment where headers is first populated), leaving the rest of the streaming logic (response.toggleBuffering, transfer-encoding, error handling, flushAndClose) unchanged; look for the duplicate assignment near the Stream branch that references contentTypeValue and delete it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/serinus/lib/src/core/controller.dart`:
- Around line 221-242: Add a Dart doc comment for the public method onStream
explaining its purpose and parameters (handler, route, shouldValidateMultipart)
and mention that it registers a streaming route and throws a StateError if a
duplicate route exists; mirror the style and tags from the existing on method
documentation so tools and the pipeline recognize the public API (reference
symbols: onStream, Route, RequestContext, RestRouteHandlerSpec, StreamHandler,
UuidV4).
---
Nitpick comments:
In `@packages/serinus/lib/src/adapters/serinus_http_server.dart`:
- Around line 164-196: The code assigns the Content-Type header twice when
handling a Stream response; remove the redundant
headers[io.HttpHeaders.contentTypeHeader] = contentTypeValue; inside the
bodyData is Stream block so the Content-Type is only set once (keep the initial
assignment where headers is first populated), leaving the rest of the streaming
logic (response.toggleBuffering, transfer-encoding, error handling,
flushAndClose) unchanged; look for the duplicate assignment near the Stream
branch that references contentTypeValue and delete it.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/serinus/bin/serinus.dartpackages/serinus/lib/src/adapters/serinus_http_server.dartpackages/serinus/lib/src/contexts/route_context.dartpackages/serinus/lib/src/core/controller.dartpackages/serinus/lib/src/http/internal_response.dartpackages/serinus/lib/src/routes/route_execution_context.dartpackages/serinus/lib/src/routes/routes_explorer.dartpackages/serinus/test/core/controller_test.dart
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/serinus/lib/src/core/controller.dart (2)
231-238: Consider extracting duplicate route validation logic.The route existence check is duplicated across
on,onStatic, andonStreammethods. This could be extracted into a private helper method to reduce code duplication.♻️ Optional refactor to extract helper method
+ void _ensureRouteDoesNotExist(Route route) { + final routeExists = _routes.values.any( + (r) => r.route.path == route.path && r.route.method == route.method, + ); + if (routeExists) { + throw StateError( + 'A route with the same path and method already exists. [${route.path}] [${route.method}]', + ); + } + }Then replace the duplicated blocks in
on,onStatic, andonStreamwith:_ensureRouteDoesNotExist(route);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/serinus/lib/src/core/controller.dart` around lines 231 - 238, Duplicate route-existence validation across on, onStatic, and onStream should be extracted into a private helper to remove repetition; implement a private method (e.g., _ensureRouteDoesNotExist(RouteDefinition route)) that checks _routes.values.any((r) => r.route.path == route.path && r.route.method == route.method) and throws the existing StateError with the same message when a duplicate is found, then call _ensureRouteDoesNotExist(route) from on, onStatic, and onStream instead of duplicating the check.
244-244: Minor: Add trailing comma for Dart style consistency.Adding a trailing comma after
streaming: truealigns with Dart's preferred formatting style and allows for cleaner diffs when adding future parameters.📝 Proposed fix
shouldValidateMultipart: shouldValidateMultipart, - streaming: true + streaming: true, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/serinus/lib/src/core/controller.dart` at line 244, Add a trailing comma after the "streaming: true" entry to follow Dart style conventions; locate the parameter list or map in controller.dart where "streaming: true" is set (e.g., inside the call or constructor that sets streaming) and append a comma after true, then run dartfmt/format to ensure consistent formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/serinus/lib/src/core/controller.dart`:
- Around line 231-238: Duplicate route-existence validation across on, onStatic,
and onStream should be extracted into a private helper to remove repetition;
implement a private method (e.g., _ensureRouteDoesNotExist(RouteDefinition
route)) that checks _routes.values.any((r) => r.route.path == route.path &&
r.route.method == route.method) and throws the existing StateError with the same
message when a duplicate is found, then call _ensureRouteDoesNotExist(route)
from on, onStatic, and onStream instead of duplicating the check.
- Line 244: Add a trailing comma after the "streaming: true" entry to follow
Dart style conventions; locate the parameter list or map in controller.dart
where "streaming: true" is set (e.g., inside the call or constructor that sets
streaming) and append a comma after true, then run dartfmt/format to ensure
consistent formatting.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes # (issue)
Type of change
Checklist:
Summary by CodeRabbit
New Features
Enhancements
Changes
Tests