Implement extractor types and update docs #79
Conversation
Reviewer's GuideThis PR implements two new extractor types (Message and ConnectionInfo), enhances SharedState extractor error handling by propagating decode errors, updates documentation with custom extractor usage and current limitations, and adds comprehensive tests for extractor functionality. Class diagram for new and updated extractor typesclassDiagram
class FromMessageRequest {
<<trait>>
+from_message_request(req: &MessageRequest, payload: &mut Payload) Result<Self, Self::Error>
+Error
}
class Message~T~ {
+Message(T)
+into_inner() T
+deref() &T
}
class ConnectionInfo {
+peer_addr: Option<SocketAddr>
+peer_addr() Option<SocketAddr>
}
class SharedState~T~ {
+deref() &T
}
class ExtractError {
+MissingState(&'static str)
+Deserialize(DecodeError)
}
FromMessageRequest <|.. Message
FromMessageRequest <|.. ConnectionInfo
FromMessageRequest <|.. SharedState
SharedState --> ExtractError
ExtractError o-- DecodeError
Message o-- T
SharedState o-- T
ConnectionInfo o-- SocketAddr
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 new extractor types and error handling improvements to the extraction framework, update documentation to explain custom extractors, and add tests for the new extractors. The extractor system now supports deserialising payloads into message types and extracting connection metadata, with enhanced error reporting for deserialisation failures. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant Extractor
participant Handler
Client->>Server: Send message (payload + metadata)
Server->>Extractor: Call FromMessageRequest for each handler param
Extractor->>Extractor: Deserialize payload (Message<T>)
Extractor->>Extractor: Extract peer address (ConnectionInfo)
Extractor->>Handler: Provide extracted params
Handler->>Server: Process with extracted data
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}
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Absence of Expected Change Pattern
- wireframe/src/extractor.rs is usually changed with: wireframe/src/middleware.rs
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
![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:
- Consider unifying the
Message<T>extractor’s error type with the newExtractError::Deserializevariant so handlers always use a single error enum rather than exposingbincode::DecodeErrordirectly. - The placeholder doc link
【F:docs/rust-binary-router-library-design.md†L842-L858】looks broken—please replace it with a valid intra-repo or external URL so readers can follow the example. - You’ve added tests for
MessageandConnectionInfo—consider adding a test for theSharedState<T>extractor (including the missing-state error path) to cover all built-in extractors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider unifying the `Message<T>` extractor’s error type with the new `ExtractError::Deserialize` variant so handlers always use a single error enum rather than exposing `bincode::DecodeError` directly.
- The placeholder doc link `【F:docs/rust-binary-router-library-design.md†L842-L858】` looks broken—please replace it with a valid intra-repo or external URL so readers can follow the example.
- You’ve added tests for `Message` and `ConnectionInfo`—consider adding a test for the `SharedState<T>` extractor (including the missing-state error path) to cover all built-in extractors.
## Individual Comments
### Comment 1
<location> `src/extractor.rs:205` </location>
<code_context>
}
+
+/// Extractor that deserializes the message payload into `T`.
+pub struct Message<T>(T);
+
+impl<T> Message<T> {
</code_context>
<issue_to_address>
The module is missing a containing item comment (`//!`).
Please add a `//!` comment at the top of the file to describe the module's purpose, as required by the review instructions.
</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 (3)
README.md(1 hunks)src/extractor.rs(4 hunks)tests/extractor.rs(1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~153-~153: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...rimental. Release builds fail to compile so the library cannot be used accidentally...
(COMMA_COMPOUND_SENTENCE_2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-test
🔇 Additional comments (8)
src/extractor.rs (5)
14-16: Well-structured imports for new functionality.The imports properly support the new extractor types and error handling enhancements.
72-77: Excellent documentation enhancement.The expanded trait documentation clearly explains how extractors integrate with handler parameters and their role in sharing common parsing logic.
146-166: Proper error handling with chaining support.The new
Deserializeerror variant correctly implements error chaining via thesource()method, allowing access to underlyingDecodeErrordetails for debugging.
204-233: Well-designed generic extractor implementation.The
Message<T>extractor properly:
- Uses the
WireMessagetrait bound for type safety- Advances the payload cursor to track consumption
- Provides both
Derefandinto_inner()for flexible access patterns- Returns specific
DecodeErrorfor precise error reporting
235-258: Clean and appropriate connection metadata extractor.The
ConnectionInfoextractor correctly usesstd::convert::Infallibleas the error type since peer address extraction cannot fail, demonstrating proper type-level guarantees.README.md (1)
109-148: Excellent documentation addition for custom extractors.The new section provides clear guidance on implementing custom extractors with a practical example. The explanation of how extractors integrate with handler parameters is particularly helpful for developers.
tests/extractor.rs (2)
11-23: Comprehensive test for Message extractor.The test properly verifies both deserialization correctness and payload advancement, ensuring the extractor behaves correctly in the extraction pipeline.
25-35: Well-focused test for ConnectionInfo extractor.The test correctly validates peer address extraction from request metadata, covering the core functionality of the connection info extractor.
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Absence of Expected Change Pattern
- wireframe/src/extractor.rs is usually changed with: wireframe/src/middleware.rs
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Absence of Expected Change Pattern
- wireframe/src/extractor.rs is usually changed with: wireframe/src/middleware.rs
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
|
@sourcery-ai review |
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/extractor.rs:202` </location>
<code_context>
}
+
+/// Extractor that deserializes the message payload into `T`.
+pub struct Message<T>(T);
+
+impl<T> Message<T> {
</code_context>
<issue_to_address>
The module is missing a containing item comment (`//!`).
Please add a `//!` comment at the top of the file to describe the purpose of this module, as required by the review instructions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- Derive Debug (and optionally Clone) for the new Message and ConnectionInfo extractors so that they’re easier to inspect in logs and tests.
- Consider renaming the ExtractError::Deserialize variant (e.g. to
InvalidPayloadorDecodeError) to more clearly reflect its purpose in the extractor context. - When reporting MissingState, use
std::any::type_name::<T>()to include the fully qualified type name for clearer error messages.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Derive Debug (and optionally Clone) for the new Message<T> and ConnectionInfo extractors so that they’re easier to inspect in logs and tests.
- Consider renaming the ExtractError::Deserialize variant (e.g. to `InvalidPayload` or `DecodeError`) to more clearly reflect its purpose in the extractor context.
- When reporting MissingState, use `std::any::type_name::<T>()` to include the fully qualified type name for clearer error messages.
## Individual Comments
### Comment 1
<location> `src/extractor.rs:202` </location>
<code_context>
}
+
+/// Extractor that deserializes the message payload into `T`.
+pub struct Message<T>(T);
+
+impl<T> Message<T> {
</code_context>
<issue_to_address>
The module is missing a containing item comment (`//!`).
Please add a `//!` comment at the top of the file to describe the purpose of this module, as required by the review instructions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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/extractor.rs:202` </location>
<code_context>
}
+
+/// Extractor that deserializes the message payload into `T`.
+pub struct Message<T>(T);
+
+impl<T> Message<T> {
</code_context>
<issue_to_address>
The module is missing a containing item comment (`//!`).
Please add a `//!` comment at the top of the file to describe the module's purpose, as required by the review instructions.
</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 review |
✅ Actions performedReview triggered.
|
…erequest` Docstrings generation was requested by @leynos. * #79 (comment) The following files were modified: * `src/extractor.rs` * `tests/extractor.rs`
|
Note Generated docstrings for this pull request at #83 |
…erequest` (#83) * 📝 Add docstrings to `codex/define-extractors-and-implement-frommessagerequest` Docstrings generation was requested by @leynos. * #79 (comment) The following files were modified: * `src/extractor.rs` * `tests/extractor.rs` * Reorder attributes after doc comments (#84) --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Leynos <leynos@troubledskies.net>
leynos
deleted the
codex/define-extractors-and-implement-frommessagerequest
branch
1 participant
Summary
Message<T>andConnectionInfoextractorsSharedState<T>error handlingTesting
make fmtmake lintmake testhttps://chatgpt.com/codex/tasks/task_e_6853d8dc92588322bce4f3c5a60cf379
Summary by Sourcery
Implement typed extractors for deserializing message payloads and exposing connection metadata, extend shared state error handling, update documentation on custom extractors and library limitations, and add tests for extractor functionality.
New Features:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
Message<T>andConnectionInfo.