[ISSUE #1829]🌽Add BatchAckMessageRequestBody struct for rust🚀 #1832
Conversation
WalkthroughThe pull request introduces a new public module Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
rocketmq-rust-bot
added
AI review first
|
🔊@mxsm 🚀Thanks for your contribution 🎉. CodeRabbit(AI) will review your code first 🔥 |
rocketmq-rust-robot
added
feature🚀
rocketmq-rust-bot
requested review from
rocketmq-rust-bot,
rocketmq-rust-robot and
SpaceXCN
![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.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
rocketmq-remoting/src/protocol/body.rs(1 hunks)rocketmq-remoting/src/protocol/body/batch_ack_message_request_body.rs(1 hunks)
🔇 Additional comments (4)
rocketmq-remoting/src/protocol/body.rs (1)
26-27: LGTM! Module declarations are well-organized.
The new module declarations maintain consistent visibility and alphabetical ordering with existing modules.
rocketmq-remoting/src/protocol/body/batch_ack_message_request_body.rs (3)
17-28: LGTM! Well-structured struct definition with appropriate serialization.
The struct follows Rust best practices with:
- Proper use of derive macros for Debug and serialization
- Consistent field naming and types
- Appropriate JSON serialization configuration
37-58: LGTM! Comprehensive serialization test coverage.
The test effectively verifies serialization/deserialization and field value preservation.
60-68: LGTM! Good coverage of default values.
The test properly verifies initialization with empty values.
| #[test] | ||
| fn batch_ack_message_request_body_edge_case_empty_strings() { | ||
| let body = BatchAckMessageRequestBody { | ||
| broker_name: CheetahString::from(""), | ||
| acks: vec![BatchAck { | ||
| consumer_group: String::from(""), | ||
| topic: String::from(""), | ||
| retry: String::from(""), | ||
| start_offset: -1, | ||
| queue_id: -1, | ||
| revive_queue_id: -1, | ||
| pop_time: -1, | ||
| invisible_time: -1, | ||
| bit_set: BitVec::new(), | ||
| }], | ||
| }; | ||
| assert_eq!(body.broker_name, CheetahString::from("")); | ||
| assert_eq!(body.acks.len(), 1); | ||
| assert_eq!(body.acks[0].consumer_group, ""); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Based on the complete code review, I can now generate the final response:
Validation is needed for negative values in BatchAck struct
The BatchAck struct currently accepts negative values for numeric fields without any validation, as evidenced by both the struct definition and test cases. While the test batch_ack_edge_case_empty_strings shows that negative values are technically accepted, these fields represent queue positions, timestamps, and offsets which should logically be non-negative:
start_offset: Queue message offset should be non-negativequeue_id: Queue identifier should be non-negativerevive_queue_id: Queue identifier should be non-negativepop_time: Timestamp should be non-negativeinvisible_time: Duration should be non-negative
Add validation by implementing:
impl BatchAck {
pub fn validate(&self) -> Result<(), String> {
if self.start_offset < 0 {
return Err("start_offset cannot be negative".to_string());
}
if self.queue_id < 0 {
return Err("queue_id cannot be negative".to_string());
}
if self.revive_queue_id < 0 {
return Err("revive_queue_id cannot be negative".to_string());
}
if self.pop_time < 0 {
return Err("pop_time cannot be negative".to_string());
}
if self.invisible_time < 0 {
return Err("invisible_time cannot be negative".to_string());
}
Ok(())
}
}🔗 Analysis chain
Consider validating negative values in BatchAck struct.
While testing edge cases is good, the use of negative values for fields like queue_id, start_offset, etc., might indicate missing validation in the BatchAck struct.
Let's check if BatchAck implements validation for these fields:
Consider adding validation in BatchAck to ensure these fields don't accept negative values:
impl BatchAck {
pub fn validate(&self) -> Result<(), String> {
if self.start_offset < 0 {
return Err("start_offset cannot be negative".to_string());
}
if self.queue_id < 0 {
return Err("queue_id cannot be negative".to_string());
}
// ... similar validation for other numeric fields
Ok(())
}
}Let me search for more specific information about the BatchAck struct and its usage.
Let me search for the BatchAck struct definition and any validation logic in the codebase.
Let me search for the complete BatchAck struct definition to see all its fields and any related implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for BatchAck struct definition and any validation logic
ast-grep --pattern 'struct BatchAck {
$$$
}'
# Search for any validation methods or implementations for BatchAck
rg -A 5 'impl.*BatchAck'
Length of output: 564
Script:
#!/bin/bash
# Search for BatchAck struct definition
rg -A 10 "struct BatchAck"
# Search for any validation or error handling related to these fields
rg -B 2 -A 5 "queue_id|start_offset|revive_queue_id|pop_time|invisible_time" "rocketmq-remoting/src"
# Look for any error handling or validation patterns in the codebase
rg "validate|check.*valid" "rocketmq-remoting/src/protocol/body"
Length of output: 121509
Script:
#!/bin/bash
# Search for BatchAck struct definition and its implementation
rg -A 10 "pub struct BatchAck" "rocketmq-remoting/src"
# Search for any validation or error handling related to BatchAck fields
rg -B 2 -A 5 "impl.*BatchAck" "rocketmq-remoting/src"
# Look for any validation patterns in the protocol/body directory
rg "validate|check.*valid" "rocketmq-remoting/src/protocol/body"
Length of output: 2022
Script:
#!/bin/bash
# Get the complete BatchAck struct definition
cat rocketmq-remoting/src/protocol/body/batch_ack.rs
# Look for any validation patterns in the same file
rg -B 2 -A 5 "impl" "rocketmq-remoting/src/protocol/body/batch_ack.rs"
Length of output: 5881
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1832 +/- ##
==========================================
+ Coverage 28.35% 28.40% +0.05%
==========================================
Files 474 475 +1
Lines 65957 66005 +48
==========================================
+ Hits 18703 18751 +48
Misses 47254 47254 ☔ View full report in Codecov by Sentry. |
rocketmq-rust-bot
added
approved
Which Issue(s) This PR Fixes(Closes)
Fixes #1829
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
New Features
BatchAckMessageRequestBodystruct for handling batch acknowledgment requests.Bug Fixes
Documentation