Conversation
Reviewer's GuideThis PR implements a Transform/Service-based middleware chaining mechanism around route handlers by introducing a HandlerService abstraction, refactors WireframeApp to register and dispatch Handlers via built middleware chains using ServiceRequest/ServiceResponse, extends the Envelope API for ergonomic construction and deconstruction, and adds corresponding tests and documentation updates. Sequence diagram for middleware chain invocation on request dispatchsequenceDiagram
participant Client
participant WireframeApp
participant MiddlewareChain as "Middleware Chain"
participant HandlerService
participant RouteHandler as "Route Handler"
Client->>WireframeApp: Send request (Envelope)
WireframeApp->>MiddlewareChain: Dispatch request (ServiceRequest)
MiddlewareChain->>HandlerService: call(ServiceRequest)
HandlerService->>RouteHandler: call(&Envelope)
RouteHandler-->>HandlerService: (processes request)
HandlerService-->>MiddlewareChain: ServiceResponse
MiddlewareChain-->>WireframeApp: ServiceResponse
WireframeApp-->>Client: Send response (Envelope)
Class diagram for updated WireframeApp structureclassDiagram
class WireframeApp {
-routes: HashMap<u32, Handler>
-services: Vec<Handler>
-middleware: Vec<Box<dyn Transform<HandlerService, Output = HandlerService>>>
-frame_processor: BoxedFrameProcessor
-serializer: S
-app_data: HashMap<TypeId, Arc<dyn Any + Send + Sync>>
-on_disconnect: Option<Arc<ConnectionTeardown<C>>>
+route(id: u32, handler: Handler) -> Result<Self>
+service(handler: Handler) -> Result<Self>
+wrap(mw: Transform<HandlerService, Output = HandlerService>) -> Result<Self>
+build_chains() -> HashMap<u32, HandlerService>
+process_stream(stream, routes)
+handle_frame(stream, frame, deser_failures, routes)
}
WireframeApp --> Handler : uses
WireframeApp --> HandlerService : builds chains of
WireframeApp --> Transform : uses for middleware
WireframeApp --> Envelope : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto incremental 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 WalkthroughThe changes introduce a middleware transformation pipeline in the application framework. Route handlers are now wrapped as Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WireframeApp
participant MiddlewareChain
participant HandlerService
participant Handler
Client->>WireframeApp: Sends Envelope (frame)
WireframeApp->>MiddlewareChain: Passes ServiceRequest
MiddlewareChain->>HandlerService: Calls service
HandlerService->>Handler: Invokes handler with Envelope
Handler-->>HandlerService: Returns response
HandlerService-->>MiddlewareChain: Returns ServiceResponse
MiddlewareChain-->>WireframeApp: Returns ServiceResponse
WireframeApp->>Client: Sends Envelope (response)
Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate Unit Tests
🪧 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 (
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- Currently
build_chainsis invoked inside each new connection—consider building and storing the middleware chains once during app setup to avoid reconstructing the sameHandlerServicemap on every connection. - The
process_streammethod now requires passing in the routes map; you could encapsulate the routes and stream handling into a dedicated struct or context to simplify the method signature and reduce parameter passing. HandlerServiceuses boxedServicetrait objects for every route, incurring dynamic dispatch and heap allocations—if this is a hotspot, consider leveraging generics or type erasure at compile time to reduce overhead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Currently `build_chains` is invoked inside each new connection—consider building and storing the middleware chains once during app setup to avoid reconstructing the same `HandlerService` map on every connection.
- The `process_stream` method now requires passing in the routes map; you could encapsulate the routes and stream handling into a dedicated struct or context to simplify the method signature and reduce parameter passing.
- `HandlerService` uses boxed `Service` trait objects for every route, incurring dynamic dispatch and heap allocations—if this is a hotspot, consider leveraging generics or type erasure at compile time to reduce overhead.
## Individual Comments
### Comment 1
<location> `src/middleware.rs:263` </location>
<code_context>
+ id: self.id,
+ msg: req.into_inner(),
+ };
+ (self.handler.as_ref())(&env).await;
+ let (_, bytes) = env.into_parts();
+ Ok(ServiceResponse::new(bytes))
+ }
</code_context>
<issue_to_address>
Handler is called with a reference to Envelope, but Envelope is then consumed.
Since Envelope is consumed after being passed by reference, verify that handlers do not mutate or require ownership of Envelope. Confirm all handlers are compatible with this pattern.
</issue_to_address>
### Comment 2
<location> `docs/roadmap.md:91` </location>
<code_context>
- 866-899) and introduce a `Next` helper to build the asynchronous call
- chain. Trait definitions live in
- [`src/middleware.rs`](../src/middleware.rs#L71-L84).
+ 866-899) and introduce a `Next` helper to build the asynchronous call chain.
+ Trait definitions live in
+ [`src/middleware.rs`](../src/middleware.rs#L71-L84).
</code_context>
<issue_to_address>
This bullet point exceeds the 80 column wrapping limit for bullets/paragraphs.
Please wrap this line so that no line in the bullet point exceeds 80 columns, as per the style guide.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
docs/roadmap.md(1 hunks)src/app.rs(10 hunks)src/middleware.rs(2 hunks)tests/middleware_order.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-test
🔇 Additional comments (6)
docs/roadmap.md (1)
88-102: Documentation updates look good!The roadmap correctly reflects the completion of middleware implementation tasks, including the Transform/Service traits and the WireframeApp::wrap registration method.
src/middleware.rs (1)
219-277: Well-designed service abstraction for route handlers!The
HandlerServiceimplementation provides a clean abstraction for wrapping route handlers and applying middleware chains. The use ofInfallibleas the error type is appropriate since handlers don't return errors in the current design. The separation betweenHandlerService(public API) andRouteService(implementation detail) follows good encapsulation practices.tests/middleware_order.rs (1)
1-83: Excellent test coverage for middleware ordering!The test comprehensively verifies that middleware is applied in reverse order during request processing and forward order during response processing. The
TagMiddlewareimplementation is a clever way to track the execution order by appending tags at each stage.src/app.rs (3)
71-90: Clean type abstractions for the middleware system!The refactoring from boxed
Servicefunctions toArc<Handler>provides better ergonomics, and theMiddlewaretrait with its blanket implementation elegantly abstracts theTransform<HandlerService>constraint.
136-148: Good encapsulation of Envelope fields!Making the fields
pub(crate)with public constructor and accessor methods provides the right balance between internal access and API encapsulation.
385-395: Correct middleware chain construction!The
build_chainsmethod properly applies middleware in reverse order, ensuring that the last registered middleware runs first on requests and last on responses. The async transformation allows middleware to perform any necessary async initialisation.
|
@sourcery-ai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
leynos
force-pushed
the
codex/implement-middleware-chain-using-handlerservice
branch
from
f49a1bf to
0925f63
Compare
leynos
force-pushed
the
codex/implement-middleware-chain-using-handlerservice
branch
from
0925f63 to
044b431
Compare
1 participant
Summary
HandlerServiceto invoke route handlersTransform<HandlerService>and updateWireframeAppTesting
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_68556ded20148322a71269bd06e29654
Summary by Sourcery
Implement a middleware handler chain by introducing HandlerService and Transform-based middleware in WireframeApp, refactor handler types and dispatch flow to use ServiceRequest/ServiceResponse, enhance Envelope API, and add tests verifying middleware order.
New Features:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit