Refactor fragmentation: move fragment_packet to fragment crate#436
Refactor fragmentation: move fragment_packet to fragment crate#436
Conversation
Resolves the layering violation where `fragment_packet` was defined in `app::fragment_utils` but used by the connection actor. The function now resides in its canonical location at `fragment::packet` alongside other fragmentation primitives, while preserving backward compatibility through a re-export from the original `app::fragment_utils` path. This follows the modularisation pattern established in PR #424. Closes #431 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Reviewer's GuideCentralizes packet fragmentation logic in the fragment module by introducing a new fragment::packet module, moving the fragment_packet implementation there, updating internal callers to use it, and re-exporting the function from both the fragment crate and the existing app-level helper for backward compatibility. Class diagram for centralized fragment_packet in fragment packet moduleclassDiagram
class fragment_mod {
<<module>>
}
class fragment_packet_mod {
<<module>>
+fragment_packet(fragmenter, packet) Result_vec_E_FragmentationError
}
class app_fragment_utils_mod {
<<module>>
+fragment_packet(fragmenter, packet) Result_vec_E_FragmentationError
}
class app_fragmentation_state {
<<struct>>
-fragmenter Fragmenter
+fragment(packet) Result_vec_E_FragmentationError
}
class Fragmenter {
+fragment_bytes(payload) FragmentBatch
}
class FragmentBatch {
+is_fragmented() bool
+len() usize
+into_iter() Iterator_FragmentFrame
}
class FragmentFrame {
+into_parts() FragmentHeader_payload_bytes
}
class FragmentHeader {
}
class FragmentationError {
}
class Packet {
<<trait>>
+into_parts() PacketParts
+from_parts(parts) Self
}
class PacketParts {
+new(id, correlation_id, payload) PacketParts
+id() MessageId
+correlation_id() CorrelationId
+payload() bytes
}
class MessageId {
}
class CorrelationId {
}
class payload_mod {
<<module>>
+encode_fragment_payload(header, payload) bytes_or_error
}
fragment_mod <|-- fragment_packet_mod
fragment_mod <|-- payload_mod
app_fragment_utils_mod --> fragment_packet_mod : re_exports_fragment_packet
app_fragmentation_state --> Fragmenter
app_fragmentation_state --> fragment_packet_mod : uses_fragment_packet
fragment_packet_mod --> Fragmenter
fragment_packet_mod --> Packet
fragment_packet_mod --> PacketParts
fragment_packet_mod --> FragmentationError
fragment_packet_mod --> payload_mod : encode_fragment_payload
fragment_packet_mod --> FragmentBatch
FragmentBatch --> FragmentFrame
FragmentFrame --> FragmentHeader
FragmentFrame --> payload_mod
PacketParts --> MessageId
PacketParts --> CorrelationId
Flow diagram for fragment_packet call paths after refactorflowchart LR
subgraph app_layer
app_callers[app callers using fragment_packet]
app_fragment_utils_mod[app::fragment_utils module]
app_fragmentation_state[app::FragmentationState.fragment]
end
subgraph fragment_layer
fragment_packet_mod[fragment::packet::fragment_packet]
fragment_mod_exports[fragment::mod re_export]
end
app_callers -->|existing imports| app_fragment_utils_mod
app_fragment_utils_mod -->|re_export call| fragment_packet_mod
app_fragmentation_state -->|direct call| fragment_packet_mod
fragment_packet_mod --> fragment_mod_exports
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
WalkthroughMove the transport-level fragmentation logic out of Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new
fragment::packetmodule still depends oncrate::app::{Packet, PacketParts}, so the layering violation is only partially addressed; consider introducing a fragmentation-facing trait or abstraction in the fragment layer so it no longer needs to depend onapptypes. - If
app::fragment_utils::fragment_packetis now just a compatibility shim, consider marking the re-export as#[deprecated]with guidance to migrate tocrate::fragment::fragment_packetto make the transition path explicit.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `fragment::packet` module still depends on `crate::app::{Packet, PacketParts}`, so the layering violation is only partially addressed; consider introducing a fragmentation-facing trait or abstraction in the fragment layer so it no longer needs to depend on `app` types.
- If `app::fragment_utils::fragment_packet` is now just a compatibility shim, consider marking the re-export as `#[deprecated]` with guidance to migrate to `crate::fragment::fragment_packet` to make the transition path explicit.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Addresses code review feedback on the fragment_packet refactoring:
1. Introduces `Fragmentable` trait and `FragmentParts` type in the
fragment module, removing the dependency on `crate::app::{Packet,
PacketParts}`. The fragmentation layer now operates on its own
abstractions.
2. Adds a blanket implementation in the app module so all `Packet`
types automatically implement `Fragmentable`, preserving backward
compatibility.
3. Marks `app::fragment_utils::fragment_packet` as deprecated with
guidance to migrate to `crate::fragment::fragment_packet`.
This completes the layering fix: the fragment module no longer depends
on app-layer types, and the transition path for callers is explicit.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@coderabbitai Have the following now been resolved?
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3424 🏁 Script executed: Length of output: 523 🏁 Script executed: Length of output: 541
1. Layering violation eliminated: The fragment module now defines Evidence:
2. Deprecation marking added: The re-export in #[deprecated(
since = "0.3.0",
note = "use `crate::fragment::fragment_packet` instead"
)]
pub use crate::fragment::fragment_packet;The module documentation also clearly states the deprecation and migration path. This provides an explicit transition path whilst maintaining backward compatibility. 🐰✨ |
Summary
Changes
Core changes
src/fragment/packet.rsimplementingfragment_packetalong withFragmentPartsandFragmentablepub mod packetinsrc/fragment/mod.rsand exposedfragment_packet,FragmentParts, andFragmentablepubliclyFragmentPartsandFragmentablefromcrate::fragmentfor convenient accesssrc/app/envelope.rsto provide a blanket implementation: anyPacketnow implementsFragmentablesrc/app/fragmentation_state.rsto acceptE: Fragmentableand callcrate::fragment::fragment_packetinstead of app-layer pathsrc/app/fragment_utils.rsto re-export the canonicalfragment_packetfromcrate::fragment, preserving existing importsCompatibility
Behavior
Why
Test plan
cargo test◳ Generated by DevBoxer ◰
ℹ️ Tag @devboxerhub to ask questions and address PR feedback
📎 Task: https://www.devboxer.com/task/58637d28-099d-4210-84c7-3b4d55874cd5
📝 Closes #431
Summary by Sourcery
Centralize packet fragmentation logic in the fragment module while preserving existing public APIs.
Enhancements: