Conversation
Reviewer's GuideThis PR introduces a new FrameMetadata trait for two-stage frame parsing, wires it into WireframeApp (via a parse_envelope method and a deserialize-fallback in handle_connection), extends existing serializers (e.g. BincodeSerializer) to implement the trait, updates test helpers and adds examples/tests to demonstrate header-based routing, and augments documentation accordingly. Sequence diagram for frame metadata parsing and routing in WireframeAppsequenceDiagram
participant Client
participant WireframeApp
participant Serializer
Client->>WireframeApp: Send frame bytes
WireframeApp->>Serializer: parse(frame)
alt parse succeeds
Serializer-->>WireframeApp: (Envelope, bytes_consumed)
WireframeApp->>WireframeApp: Route selection based on Envelope.id
WireframeApp->>Serializer: deserialize(frame) (if needed)
else parse fails
WireframeApp->>Serializer: deserialize(frame)
alt deserialize fails
WireframeApp-->>Client: Error/close connection
else deserialize succeeds
Serializer-->>WireframeApp: (Envelope, bytes_consumed)
WireframeApp->>WireframeApp: Route selection
end
end
Class diagram for FrameMetadata trait and serializersclassDiagram
class FrameMetadata {
<<trait>>
+parse(src: &[u8]) Result<(Frame, usize), Error>
type Frame
type Error
}
class BincodeSerializer {
+serialize()
+deserialize()
+parse(src: &[u8]) Result<(Envelope, usize), DecodeError>
}
class HeaderSerializer {
+serialize()
+deserialize()
+parse(src: &[u8]) Result<(Envelope, usize), io::Error>
}
class Envelope
FrameMetadata <|.. BincodeSerializer
FrameMetadata <|.. HeaderSerializer
BincodeSerializer --> Envelope : Frame = Envelope
HeaderSerializer --> Envelope : Frame = Envelope
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 the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WireframeApp
participant Serializer
participant ServiceHandler
Client->>WireframeApp: Send framed message
WireframeApp->>Serializer: parse(frame)
alt Parse success
Serializer-->>WireframeApp: (Envelope, bytes_consumed)
WireframeApp->>ServiceHandler: Call handler based on Envelope.id
ServiceHandler-->>WireframeApp: Response
WireframeApp->>Serializer: Serialize response
WireframeApp-->>Client: Send response frame
else Parse failure
Serializer-->>WireframeApp: Error
WireframeApp->>Serializer: deserialize(frame)
alt Deserialize success
Serializer-->>WireframeApp: Envelope
WireframeApp->>ServiceHandler: Call handler based on Envelope.id
ServiceHandler-->>WireframeApp: Response
WireframeApp->>Serializer: Serialize response
WireframeApp-->>Client: Send response frame
else Deserialize failure
WireframeApp-->>Client: Return error
end
end
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:
- Extract the
parse-then-deserializefallback logic into a standalone helper to simplify and flatten the routing loop inhandle_connection. - Introduce a type or trait alias for
S: Serializer + FrameMetadata<Frame = Envelope> + Send + Sync + 'staticin the test utilities to cut down on repetitive bounds boilerplate. - Consider moving the
FrameMetadatatrait bound off of everyhandle_*method signature and onto a consolidatedWireframeApp<S>impl or internal parse step to reduce API verbosity.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Extract the `parse`-then-`deserialize` fallback logic into a standalone helper to simplify and flatten the routing loop in `handle_connection`.
- Introduce a type or trait alias for `S: Serializer + FrameMetadata<Frame = Envelope> + Send + Sync + 'static` in the test utilities to cut down on repetitive bounds boilerplate.
- Consider moving the `FrameMetadata` trait bound off of every `handle_*` method signature and onto a consolidated `WireframeApp<S>` impl or internal parse step to reduce API verbosity.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.
|
![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 (7)
docs/frame-metadata.md(1 hunks)examples/metadata_routing.rs(1 hunks)src/app.rs(4 hunks)src/frame.rs(1 hunks)src/serializer.rs(2 hunks)tests/metadata.rs(1 hunks)tests/util.rs(4 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/frame-metadata.md
[style] ~42-~42: Would you like to use the Oxford spelling “deserialization”? The spelling ‘deserialisation’ is also correct.
Context: ...App` the metadata parser is used before deserialisation so routes can be selected as soon as th...
(OXFORD_SPELLING_Z_NOT_S)
[uncategorized] ~42-~42: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...ta parser is used before deserialisation so routes can be selected as soon as the h...
(COMMA_COMPOUND_SENTENCE_2)
🔇 Additional comments (16)
src/frame.rs (1)
192-212: Well-designed trait with clear interface and documentation.The
FrameMetadatatrait follows Rust best practices with appropriate associated types and error bounds suitable for async contexts. The documentation clearly explains its purpose for early frame header inspection.src/serializer.rs (2)
9-9: Import addition looks correct.Adding
FrameMetadatato the imports is necessary for the new trait implementation.
50-57: Solid implementation of FrameMetadata for BincodeSerializer.The implementation correctly associates the Frame type with
Envelopeand delegates parsing to the existingfrom_bytesmethod. The error type alignment is appropriate.docs/frame-metadata.md (1)
1-44: Comprehensive documentation with clear examples.The documentation effectively explains the
FrameMetadatatrait's purpose and usage. The code example demonstrates bothFrameProcessorandFrameMetadataimplementation, providing good guidance for users.examples/metadata_routing.rs (1)
53-99: Well-structured example demonstrating the FrameMetadata usage.The main function provides a clear demonstration of setting up routes, creating frames with the custom format, and handling the async communication flow. The example effectively shows how metadata-based routing works in practice.
tests/metadata.rs (2)
16-42: Excellent test design using atomic counters and panic guards.The
CountingSerializereffectively verifies the expected control flow by trackingparsemethod calls and ensuringdeserializeis not called prematurely. This validates that the framework correctly invokes metadata parsing before deserialization.
44-65: Comprehensive integration test validating metadata parsing order.The test thoroughly verifies that the
FrameMetadata::parsemethod is invoked exactly once during frame processing, confirming the intended control flow in the WireframeApp implementation.src/app.rs (5)
371-371: LGTM: Trait bound addition enables metadata parsingThe addition of the
FrameMetadata<Frame = Envelope>trait bound correctly constrains the serializer to support the new metadata parsing functionality.
409-409: LGTM: Consistent trait bound applicationThe trait bound is correctly applied to maintain consistency across the call chain.
500-500: LGTM: Trait bound enables new parsing logicThe trait bound addition supports the enhanced frame handling logic introduced below.
524-542: LGTM: Well-structured service invocation with proper error handlingThe separation of parsing logic from service invocation improves code organisation. The error handling for both service execution and response sending is comprehensive and appropriate.
502-522: ```shell
#!/bin/bashDisplay the Serializer trait and its parse/deserialize method signatures
sed -n '1,200p' src/serializer.rs
</details> <details> <summary>tests/util.rs (4)</summary> `30-39`: **LGTM: Well-structured generic constraints** The generic type parameter and trait bounds correctly enable support for serializers implementing the new `FrameMetadata` trait whilst maintaining all necessary constraints for async execution. --- `50-63`: **LGTM: Consistent generic implementation** The trait bounds and generic parameter usage are consistent with the other utility functions, maintaining a uniform API. --- `72-84`: **LGTM: Proper generic constraints maintained** The generic implementation correctly mirrors the pattern established in the other utility functions. --- `95-106`: **LGTM: Complete generic transformation** The final utility function properly implements the same generic pattern, ensuring all test utilities support the enhanced serializer requirements. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@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> `examples/metadata_routing.rs:37` </location>
<code_context>
+ type Frame = Envelope;
+ type Error = io::Error;
+
+ fn parse(&self, src: &[u8]) -> Result<(Envelope, usize), io::Error> {
+ if src.len() < 3 {
+ return Err(io::Error::new(io::ErrorKind::UnexpectedEof, "header"));
+ }
+ let id = u32::from(u16::from_be_bytes([src[0], src[1]]));
+ // The third byte carries message flags. This example intentionally
+ // ignores the flags, but a real protocol might parse and act on these
+ // bits.
+ let _ = src[2];
+ let payload = src[3..].to_vec();
+ Ok((Envelope::new(id, payload), src.len()))
+ }
+}
</code_context>
<issue_to_address>
parse returns src.len() as bytes consumed, which may not match the actual frame length.
Assuming the entire buffer is a single frame may cause issues if extra data or multiple frames are present. Please return the actual frame length or clarify the expected input.
</issue_to_address>
### Comment 2
<location> `src/app.rs:371` </location>
<code_context>
+{
+ /// Try parsing the frame using [`FrameMetadata::parse`], falling back to
+ /// full deserialization on failure.
+ fn parse_envelope(
+ &self,
+ frame: &[u8],
</code_context>
<issue_to_address>
Add a test for parse_envelope to ensure correct fallback and error handling.
The new parse_envelope method is non-trivial and handles error fallback logic. Add a unit test to verify that it correctly falls back to full deserialization when FrameMetadata::parse fails, and that it returns errors as expected.
</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.
Gates Failed
(1 file with Code Duplication)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| metadata.rs | 1 advisory rule | 10.00 → 9.39 | Suppress |
Absence of Expected Change Pattern
- wireframe/tests/util.rs is usually changed with: wireframe/tests/routes.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 Failed
(1 file with Code Duplication)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| metadata.rs | 1 advisory rule | 10.00 → 9.39 | Suppress |
Absence of Expected Change Pattern
- wireframe/tests/util.rs is usually changed with: wireframe/tests/routes.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.
| async fn falls_back_to_deserialize_after_parse_error() { | ||
| let parse_calls = Arc::new(AtomicUsize::new(0)); | ||
| let deser_calls = Arc::new(AtomicUsize::new(0)); | ||
| let serializer = FallbackSerializer(parse_calls.clone(), deser_calls.clone()); | ||
| let app = mock_wireframe_app_with_serializer(serializer); | ||
|
|
||
| let env = Envelope::new(1, vec![7]); | ||
| let bytes = BincodeSerializer.serialize(&env).unwrap(); | ||
| let mut framed = BytesMut::new(); | ||
| LengthPrefixedProcessor::default() | ||
| .encode(&bytes, &mut framed) | ||
| .unwrap(); | ||
|
|
||
| let out = run_app_with_frame(app, framed.to_vec()).await.unwrap(); | ||
| assert!(!out.is_empty()); | ||
| assert_eq!(parse_calls.load(Ordering::SeqCst), 1); | ||
| assert_eq!(deser_calls.load(Ordering::SeqCst), 1); | ||
| } |
There was a problem hiding this comment.
❌ New issue: Code Duplication
The module contains 2 functions with similar structure: falls_back_to_deserialize_after_parse_error,metadata_parser_invoked_before_deserialize
leynos
force-pushed
the
codex/define-framemetadata-trait-and-update-functionality
branch
from
45180d3 to
503e4da
Compare
There was a problem hiding this comment.
Gates Failed
(1 file with Code Duplication)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| metadata.rs | 1 advisory rule | 10.00 → 9.39 | Suppress |
Absence of Expected Change Pattern
- wireframe/tests/util.rs is usually changed with: wireframe/tests/routes.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 Failed
(1 file with Code Duplication)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| metadata.rs | 1 advisory rule | 10.00 → 9.39 | Suppress |
Absence of Expected Change Pattern
- wireframe/tests/util.rs is usually changed with: wireframe/tests/routes.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.
* Fix generics and handler frame logic * Explain envelope handling
There was a problem hiding this comment.
Gates Failed
(1 file with Code Duplication)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| metadata.rs | 1 advisory rule | 10.00 → 9.39 | Suppress |
Absence of Expected Change Pattern
- wireframe/tests/util.rs is usually changed with: wireframe/tests/routes.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.
leynos
deleted the
codex/define-framemetadata-trait-and-update-functionality
branch
1 participant
Summary
FrameMetadatatrait for early header parsingWireframeAppmetadata_routingexample showcasing header-based routingTesting
make fmtmake lintmake testcargo build --exampleshttps://chatgpt.com/codex/tasks/task_e_68574b7e82808322b3ced45205887821
Summary by Sourcery
Enable two-stage frame parsing by introducing the FrameMetadata trait and integrating it into WireframeApp for header-based routing before full deserialization.
New Features:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit