Repository navigation
[ISSUE #4575]🚀Implement BoundaryType enum with serialization, deserialization, and parsing methods; add SearchOffsetRequestHeader struct with tests - #4576
Conversation
…lization, and parsing methods; add SearchOffsetRequestHeader struct with tests
|
🔊@mxsm 🚀Thanks for your contribution🎉！ 💡CodeRabbit(AI) will review your code first🔥！ Note 🚨The code review suggestions from CodeRabbit are to be used as a reference only, and the PR submitter can decide whether to make changes based on their own judgment. Ultimately, the project management personnel will conduct the final code review💥. |
rocketmq-rust-bot
added
auto merge
ready to review
waiting-review
WalkthroughThis PR enhances the BoundaryType enum with serialization, deserialization, and parsing capabilities, adding trait implementations for Serialize, Deserialize, Display, and FromStr. It introduces the SearchOffsetRequestHeader struct that incorporates BoundaryType as a field, with comprehensive test coverage for both components. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR implements the BoundaryType enum with comprehensive serialization, deserialization, and parsing capabilities, along with the SearchOffsetRequestHeader struct, both matching Java RocketMQ behavior for cross-language compatibility.
- Adds
BoundaryTypeenum with serialization/deserialization support, case-insensitive parsing, and default fallback behavior - Implements
SearchOffsetRequestHeaderstruct for search offset requests with proper field mappings - Includes extensive test coverage for both components covering serialization, deserialization, edge cases, and roundtrip scenarios
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| rocketmq-common/src/common/boundary_type.rs | Implements BoundaryType enum with Default, Clone, Copy traits, custom Serialize/Deserialize implementations matching Java format (uppercase), Display and FromStr traits, plus comprehensive test suite |
| rocketmq-remoting/src/protocol/header/search_offset_request_header.rs | Adds SearchOffsetRequestHeader struct with topic, queue_id, timestamp, and boundary_type fields, includes extensive tests for serialization, deserialization, edge cases, and boundary conditions |
| rocketmq-remoting/src/protocol/header.rs | Registers the new search_offset_request_header module in the header module hierarchy |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| impl FromStr for BoundaryType { | ||
| type Err = String; | ||
|
|
||
| fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
| // Always succeeds, returns Lower as default (matches Java behavior) | ||
| Ok(BoundaryType::get_type(s)) | ||
| } | ||
| } |
There was a problem hiding this comment.
The FromStr implementation's error type is defined as String, but the implementation always returns Ok. This creates an inconsistency where the error type suggests failure is possible but it never actually occurs. Consider using std::convert::Infallible as the error type to correctly represent that parsing never fails, matching the documented behavior that it "always succeeds, returns Lower as default".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
rocketmq-remoting/src/protocol/header/search_offset_request_header.rs (2)
23-30: Consider#[serde(default)]to allow missing fields (esp.boundaryType) without failing deserialization.
Right now, deserializing JSON that omitsboundaryTypeerrors instead of defaulting toBoundaryType::Lower(and similarly for other fields). If you want “Java-like defaulting” for absent fields too, add struct-level defaulting.#[derive(Default, Debug, Serialize, Deserialize, RequestHeaderCodecV2)] +#[serde(default)] #[serde(rename_all = "camelCase")] pub struct SearchOffsetRequestHeader { pub topic: CheetahString, pub queue_id: i32, pub timestamp: i64, pub boundary_type: BoundaryType, }
60-76: Prefer asserting onserde_json::Valueinstead ofjson.contains(...)for stability.
This avoids coupling the test to serialization formatting/details and makes failures more actionable.#[test] fn search_offset_request_header_serializes_to_json() { let header = SearchOffsetRequestHeader { topic: CheetahString::from("my_topic"), queue_id: 2, timestamp: 1702345678999, boundary_type: BoundaryType::Upper, }; - let json = serde_json::to_string(&header).unwrap(); - - // Verify camelCase field names and uppercase enum - assert!(json.contains(r#""topic":"my_topic""#)); - assert!(json.contains(r#""queueId":2"#)); - assert!(json.contains(r#""timestamp":1702345678999"#)); - assert!(json.contains(r#""boundaryType":"UPPER""#)); + let v = serde_json::to_value(&header).unwrap(); + assert_eq!(v["topic"], "my_topic"); + assert_eq!(v["queueId"], 2); + assert_eq!(v["timestamp"], 1702345678999_i64); + assert_eq!(v["boundaryType"], "UPPER"); }rocketmq-common/src/common/boundary_type.rs (2)
101-108:FromStrshould be infallible (Err = Infallible) since it never returnsErr.
Current signature implies parsing can fail, but it can’t.use std::fmt; use std::str::FromStr; +use std::convert::Infallible; impl FromStr for BoundaryType { - type Err = String; + type Err = Infallible; fn from_str(s: &str) -> Result<Self, Self::Err> { // Always succeeds, returns Lower as default (matches Java behavior) Ok(BoundaryType::get_type(s)) } }
21-25: Verify serde dependency/feature policy + consider non-allocating deserialize path.
Ifrocketmq-commonkeeps serde behind a feature flag, these unconditional serde imports/impls should becfg(feature = "serde"). Also, you can avoid allocating aStringby deserializing intoCow<'de, str>.Also applies to: 80-89
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
rocketmq-common/src/common/boundary_type.rs(1 hunks)rocketmq-remoting/src/protocol/header.rs(1 hunks)rocketmq-remoting/src/protocol/header/search_offset_request_header.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Upload results
- GitHub Check: Build & Test (macos-latest)
- GitHub Check: Build & Test (windows-latest)
- GitHub Check: Build & Test (ubuntu-latest)
- GitHub Check: Code Coverage
- GitHub Check: auto-approve
🔇 Additional comments (1)
rocketmq-remoting/src/protocol/header.rs (1)
72-73: Public module export looks right; just ensure module path/name matches the file.
Given this is a new public surface, a quick compile check will catch anymod/filename mismatch (and accidental shadowing with similarly named headers).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4576 +/- ##
==========================================
+ Coverage 30.91% 31.07% +0.16%
==========================================
Files 675 676 +1
Lines 98814 99028 +214
==========================================
+ Hits 30551 30776 +225
+ Misses 68263 68252 -11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rocketmq-rust-bot
left a comment
rocketmq-rust-bot
left a comment
There was a problem hiding this comment.
LGTM - All CI checks passed ✅
rocketmq-rust-bot
added
approved
Which Issue(s) This PR Fixes(Closes)
Fixes #4575
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.