Conversation
Reviewer's GuideIntroduces a functional middleware builder that enables creating middleware from async closures, documents its usage in the README, and cleans up the design document formatting to comply with markdownlint. Class diagram for from_fn helper functionclassDiagram
class from_fn {
+from_fn(f: F) FromFn<F>
}
from_fn --> FromFn : returns
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 WalkthroughThis update introduces a new function-based middleware abstraction in the codebase, allowing middleware to be constructed from asynchronous functions or closures. Documentation is updated to explain this mechanism, and editorial improvements are made to a design document. No changes are made to existing public APIs except for the new middleware feature. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware (FromFn)
participant Next/Service
Client->>Middleware (FromFn): send ServiceRequest
Middleware (FromFn)->>Next/Service: call next with ServiceRequest
Next/Service-->>Middleware (FromFn): return ServiceResponse
Middleware (FromFn)-->>Client: return (possibly modified) ServiceResponse
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 and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/middleware.rs:96` </location>
<code_context>
+/// The function receives a [`ServiceRequest`] and a [`Next`] reference to invoke
+/// the remaining middleware chain. It must return a [`ServiceResponse`] wrapped
+/// in a [`Result`]. The error type is the same as the wrapped service.
+pub struct FromFn<F> {
+ f: F,
+}
</code_context>
<issue_to_address>
The module is missing a containing item comment (`//!`).
Please add a module-level doc comment at the top of the file using `//!` to describe the purpose of this module.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| /// The function receives a [`ServiceRequest`] and a [`Next`] reference to invoke | ||
| /// the remaining middleware chain. It must return a [`ServiceResponse`] wrapped | ||
| /// in a [`Result`]. The error type is the same as the wrapped service. | ||
| pub struct FromFn<F> { |
There was a problem hiding this comment.
issue (review_instructions): The module is missing a containing item comment (//!).
Please add a module-level doc comment at the top of the file using //! to describe the purpose of this module.
Review instructions:
Path patterns: **/*.rs
Instructions:
All modules MUST have a containing item 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: 3
🔭 Outside diff range comments (1)
docs/rust-binary-router-library-design.md (1)
1085-1123: Fix code formatting issues in the example.The code example has several formatting and syntax issues that need correction.
- use std::time::{SystemTime, UNIX_EPOCH}; - // Define a message ID enum if not using type-based routing directly - enum MyMessageType { Echo = 1 } - // Handler function + use std::time::{SystemTime, UNIX_EPOCH}; + + // Define a message ID enum if not using type-based routing directly + enum MyMessageType { Echo = 1 } + + // Handler function async fn handle_echo(req: Message<EchoRequest>) -> WireframeResult<EchoResponse> { println!("Received echo request with payload: {}", req.payload); Ok(EchoResponse { original_payload: req.payload.clone(), - echoed_at: SystemTime::now().duration_since(UNIX_EPOCH).unwrap().as_secs(). + echoed_at: SystemTime::now().duration_since(UNIX_EPOCH).unwrap().as_secs(), }) } - #[tokio::main] async fn main() -> std::io::Result\<()> { println!("Starting echo server on 127.0.0.1:8000"); + + #[tokio::main] + async fn main() -> std::io::Result<()> { + println!("Starting echo server on 127.0.0.1:8000"); + WireframeServer::new(|| { WireframeApp::new() //.frame_processor(LengthPrefixedCodec) // Simplified @@ -1111,13 +1111,7 @@ .run() .await } - This example, even in outline, demonstrates how derive macros for messages, a - separable framing component, and a clear handler signature with extractors - (`Message<EchoRequest>`) and a return type (`WireframeResult<EchoResponse>`) - simplify server implementation. - - ``` - - ```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)docs/rust-binary-router-library-design.md(17 hunks)src/middleware.rs(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/rust-binary-router-library-design.md
[grammar] ~333-~333: The verb ‘derives’ is singular. Did you mean: “this derives” or “these derive”?
Context: ...rait in the library currently exposes these derives under a convenient Message trait, pro...
(SINGULAR_VERB_AFTER_THESE_OR_THOSE)
[uncategorized] ~769-~769: Loose punctuation mark.
Context: ...r_addr()); } ``` - SharedState<T>: Allows handlers to access shared applic...
(UNLIKELY_OPENING_PUNCTUATION)
[grammar] ~813-~813: Please add a punctuation mark at the end of paragraph.
Context: ...erate on "wireframe's" internal request and response types, which could be raw f...
(PUNCTUATION_PARAGRAPH_END)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-test
🔇 Additional comments (7)
docs/rust-binary-router-library-design.md (1)
332-334: Fix grammatical error in documentation.The static analysis tool correctly identified a grammar issue. The phrase "these derives" should be "this derives" since it refers to a single wrapper trait.
- `bincode` 11 or `postcard`.12 A minimal wrapper trait in the library currently - exposes these derives under a convenient `Message` trait, providing `to_bytes` - and `from_bytes` helpers. + `bincode` 11 or `postcard`.12 A minimal wrapper trait in the library currently + exposes this derives under a convenient `Message` trait, providing `to_bytes` + and `from_bytes` helpers.Likely an incorrect or invalid review comment.
src/middleware.rs (5)
29-49: LGTM! Well-documented constructor with proper attributes.The implementation correctly uses
#[inline],#[must_use], and provides comprehensive documentation with examples.
91-103: LGTM! Clean and simple middleware wrapper.The
FromFn<F>struct provides a clean abstraction for function-based middleware with an appropriate constructor.
105-131: Excellent documentation and API design.The
from_fnconvenience function is well-documented with a comprehensive example that demonstrates the expected function signature and usage pattern.
154-168: LGTM! Proper Transform implementation.The
Transformimplementation correctly creates theFnServicewith the appropriate service and function, enabling the middleware to be used in a middleware chain.
142-143: ```shell
#!/bin/bash
sed -n '120,180p' src/middleware.rs</details> <details> <summary>README.md (1)</summary> `109-124`: **Excellent documentation of the new middleware feature.** The middleware section provides a clear, concise explanation with a practical example using the `tracing` crate. The example demonstrates the key concepts: receiving a request, calling the next handler, and processing the response. The documentation correctly shows: - Import of `from_fn` - Async closure syntax with move semantics - Use of `next.call(req).await?` for chain progression - Proper error propagation with `?` operator - Practical logging example with `tracing::info!` </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| <!-- markdownlint-disable-next-line MD013 --> | ||
| | msg_header: &CommandHeader | msg_header.sub_type == CommandSubType::Special, handle_special_command ).message(MessageType::GenericCommand, handle_generic_command) // Fallback ``` | | ||
|
|
||
| ```` |
There was a problem hiding this comment.
Fix malformed code block and improve readability.
The code block appears to have formatting issues with incomplete backticks and inconsistent indentation.
- ````rust
- Router::new()
- .message_guarded(
- MessageType::GenericCommand,
- <!-- markdownlint-disable-next-line MD013 -->
- | msg_header: &CommandHeader | msg_header.sub_type == CommandSubType::Special, handle_special_command ).message(MessageType::GenericCommand, handle_generic_command) // Fallback ``` |
- ````
+ ```rust
+ Router::new()
+ .message_guarded(
+ MessageType::GenericCommand,
+ |msg_header: &CommandHeader| msg_header.sub_type == CommandSubType::Special,
+ handle_special_command
+ )
+ .message(MessageType::GenericCommand, handle_generic_command) // Fallback
+ ```🤖 Prompt for AI Agents
In docs/rust-binary-router-library-design.md around lines 544 to 547, the code
block is malformed with incomplete backticks and inconsistent indentation. Fix
this by properly enclosing the code block with triple backticks and specifying
the language as rust. Also, reformat the code inside to have consistent
indentation and remove any HTML comments or inline markdownlint disables,
ensuring the code is clean and readable.
| Middleware would operate on "wireframe's" internal request and | ||
|
|
There was a problem hiding this comment.
Complete the sentence fragment.
The static analysis tool identified an incomplete sentence that needs proper punctuation.
-- The `Service` trait would define the actual request/response processing logic.
- Middleware would operate on "wireframe's" internal request and
+- The `Service` trait would define the actual request/response processing logic.
+ Middleware would operate on "wireframe's" internal request and response types,
+ which could be raw frames at one level or deserialized messages at another,
+ depending on the middleware's purpose.Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 LanguageTool
[grammar] ~813-~813: Please add a punctuation mark at the end of paragraph.
Context: ...erate on "wireframe's" internal request and response types, which could be raw f...
(PUNCTUATION_PARAGRAPH_END)
🤖 Prompt for AI Agents
In docs/rust-binary-router-library-design.md around lines 813 to 814, the
sentence fragment "Middleware would operate on 'wireframe's' internal request
and" is incomplete. Complete the sentence by adding the missing part or
punctuation to form a full, clear sentence.
| F: for<'a> Fn(ServiceRequest, Next<'a, S>) -> Fut + Send + Sync + Clone, | ||
| Fut: std::future::Future<Output = Result<ServiceResponse, S::Error>> + Send, | ||
| { | ||
| type Error = S::Error; | ||
|
|
||
| async fn call(&self, req: ServiceRequest) -> Result<ServiceResponse, Self::Error> { | ||
| let next = Next::new(&self.service); | ||
| (self.f.clone())(req, next).await | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Review the cloning strategy for performance implications.
The implementation clones the function F on every call, which could have performance implications for complex closures or when the middleware captures large amounts of data.
Consider whether the cloning is necessary or if there's a more efficient approach:
async fn call(&self, req: ServiceRequest) -> Result<ServiceResponse, Self::Error> {
let next = Next::new(&self.service);
// Consider if cloning can be avoided or moved to construction time
(self.f.clone())(req, next).await
}The current approach is correct for the typical use case of simple function pointers or lightweight closures, but document this behaviour if it's intentional.
🤖 Prompt for AI Agents
In src/middleware.rs around lines 142 to 151, the call method clones the
function F on every invocation, which may impact performance if F is a complex
closure or captures large data. To fix this, evaluate if cloning can be avoided
by either storing F in a way that allows borrowing instead of cloning or by
moving the clone operation to the middleware construction phase rather than per
call. If cloning is necessary, add documentation explaining this design choice
and its implications for performance.
Fixes broken code fences and syntax in the illustrative server example.
1 participant
Summary
middleware::from_fnto build middleware from async closuresTesting
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_68544bc89cfc8322b34b196ee3bb3888
Summary by Sourcery
Introduce support for functional middleware by implementing a
from_fnbuilder and document its usage, alongside minor markdown lint fixes in project docs.New Features:
middleware::from_fnhelper to enable building middleware from async functions or closuresEnhancements:
FromFn,FnService, and integration withTransformandServicetraits to support functional middlewareDocumentation:
from_fnmiddleware helper and usage example in the READMEChores:
Summary by CodeRabbit
New Features
Documentation