[ISSUE #6589]📝Refactor MessageTrait documentation for improved clarity and consistency#6590
Merged
rocketmq-rust-bot merged 1 commit intomainfrom Mar 1, 2026
Merged
[ISSUE #6589]📝Refactor MessageTrait documentation for improved clarity and consistency#6590rocketmq-rust-bot merged 1 commit intomainfrom
rocketmq-rust-bot merged 1 commit intomainfrom
Conversation
…y and consistency
rocketmq-rust-bot
requested review from
SpaceXCN,
TeslaRustor and
rocketmq-rust-bot
Collaborator
|
🔊@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
AI review first
rocketmq-rust-robot
added
the
documentation📝
Contributor
WalkthroughThe changes introduce a message versioning system with V2 support, expand MessageConst with new property and timer engine constants, strengthen property validation with a reserved property set, and extend MessageTrait with additional accessor/mutator methods and convenience utilities. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rocketmq-common/src/common/message.rs (1)
356-365:⚠️ Potential issue | 🟡 MinorV1 topic length silently truncates values greater than 255.
When using V1,
topic_length as u8will truncate any value above 255. If a topic name exceeds 255 bytes, this will corrupt the encoded length without warning.Consider adding a debug assertion or validation to catch oversized topic names during development:
💡 Suggested defensive check
MessageVersion::V1 => { + debug_assert!(topic_length <= 255, "V1 topic length cannot exceed 255 bytes"); buffer.push(topic_length as u8) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rocketmq-common/src/common/message.rs` around lines 356 - 365, put_topic_length currently truncates topic_length for MessageVersion::V1 by casting to u8; add a defensive check to catch oversized topic names during development by inserting a debug assertion in put_topic_length (e.g. in the MessageVersion::V1 arm use debug_assert!(topic_length <= 0xFF, "topic_length {} exceeds V1 max 255", topic_length)) so oversized lengths are detected early; reference: function put_topic_length and enum variant MessageVersion::V1.
🧹 Nitpick comments (2)
rocketmq-common/src/common/message.rs (2)
55-90: Documentation and trait implementation look good overall.The
put_user_propertyvalidation logic is well-structured. Minor note: the error message on line 80 mentions "blank string" butis_empty()only checks for zero-length strings, not whitespace-only strings. If the intent is to also reject whitespace-only input, consider usingtrim().is_empty(). Otherwise, adjust the message to say "empty" instead of "blank."💡 Optional: Align message with behavior
Either update the message:
- "The name or value of property can not be null or blank string!".to_string(), + "The name or value of property cannot be empty!".to_string(),Or extend the check to include whitespace-only strings:
- if name.is_empty() || value.is_empty() { + if name.trim().is_empty() || value.trim().is_empty() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rocketmq-common/src/common/message.rs` around lines 55 - 90, The validation in put_user_property currently uses name.is_empty() and value.is_empty() but the error text mentions "blank string" — update the validation to reject whitespace-only inputs by using name.trim().is_empty() and value.trim().is_empty() (keep the same RocketMQError::InvalidProperty path and message), and leave the follow-up reserved-name check (STRING_HASH_SET.contains(name.as_str())) and the call to put_property(name, value) unchanged.
367-382: Simplifyis_v1()andis_v2()methods.These methods can be written more concisely using direct enum comparison.
♻️ Simplified implementation
pub fn is_v1(&self) -> bool { - match self { - MessageVersion::V1 => true, - MessageVersion::V2 => false, - } + matches!(self, MessageVersion::V1) } pub fn is_v2(&self) -> bool { - match self { - MessageVersion::V1 => false, - MessageVersion::V2 => true, - } + matches!(self, MessageVersion::V2) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rocketmq-common/src/common/message.rs` around lines 367 - 382, The is_v1() and is_v2() implementations on the MessageVersion enum are verbose; replace the match arms with direct enum comparisons: change MessageVersion::is_v1 to return self == &MessageVersion::V1 and MessageVersion::is_v2 to return self == &MessageVersion::V2 (or use *self == MessageVersion::V1 / V2 if using Copy) so the methods are concise and idiomatic; update the functions named is_v1 and is_v2 on the MessageVersion type accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rocketmq-common/src/common/message.rs`:
- Around line 339-345: The get_topic_length function may cast a negative i16 to
usize for MessageVersion::V2 (buffer.get_i16()), causing huge lengths; update
MessageVersion::V2 handling in get_topic_length to either read an unsigned value
(use buffer.get_u16() semantics) or explicitly validate the i16 (check if < 0
and return an error or 0/handle appropriately) before converting to usize so
negative values cannot wrap into large sizes.
- Around line 347-354: get_topic_length_at_index currently indexes buffer
directly and can panic; change its signature to return Option<usize> and perform
bounds-checked access via slice.get(): for MessageVersion::V1 use
buffer.get(index).map(|b| *b as usize), and for MessageVersion::V2 use
buffer.get(index).and_then(|b0| buffer.get(index + 1).map(|b1| (((*b0 as usize)
<< 8) | (*b1 as usize)))); this prevents panics on short buffers and propagates
None when bytes are missing.
---
Outside diff comments:
In `@rocketmq-common/src/common/message.rs`:
- Around line 356-365: put_topic_length currently truncates topic_length for
MessageVersion::V1 by casting to u8; add a defensive check to catch oversized
topic names during development by inserting a debug assertion in
put_topic_length (e.g. in the MessageVersion::V1 arm use
debug_assert!(topic_length <= 0xFF, "topic_length {} exceeds V1 max 255",
topic_length)) so oversized lengths are detected early; reference: function
put_topic_length and enum variant MessageVersion::V1.
---
Nitpick comments:
In `@rocketmq-common/src/common/message.rs`:
- Around line 55-90: The validation in put_user_property currently uses
name.is_empty() and value.is_empty() but the error text mentions "blank string"
— update the validation to reject whitespace-only inputs by using
name.trim().is_empty() and value.trim().is_empty() (keep the same
RocketMQError::InvalidProperty path and message), and leave the follow-up
reserved-name check (STRING_HASH_SET.contains(name.as_str())) and the call to
put_property(name, value) unchanged.
- Around line 367-382: The is_v1() and is_v2() implementations on the
MessageVersion enum are verbose; replace the match arms with direct enum
comparisons: change MessageVersion::is_v1 to return self == &MessageVersion::V1
and MessageVersion::is_v2 to return self == &MessageVersion::V2 (or use *self ==
MessageVersion::V1 / V2 if using Copy) so the methods are concise and idiomatic;
update the functions named is_v1 and is_v2 on the MessageVersion type
accordingly.
Comment on lines
+339
to
345
| /// Reads and returns the topic length from the buffer, advancing the buffer position. | ||
| pub fn get_topic_length(&self, buffer: &mut Bytes) -> usize { | ||
| match self { | ||
| MessageVersion::V1 => buffer.get_u8() as usize, | ||
| MessageVersion::V2 => buffer.get_i16() as usize, | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Potential issue with negative i16 to usize cast.
On line 343, buffer.get_i16() as usize can produce incorrect results if the value is negative. A negative i16 cast to usize will wrap around to a very large value, causing incorrect topic length parsing.
🛡️ Proposed fix: use unsigned read or validate
MessageVersion::V1 => buffer.get_u8() as usize,
- MessageVersion::V2 => buffer.get_i16() as usize,
+ MessageVersion::V2 => buffer.get_u16() as usize,Alternatively, if signed is intentional, validate the value:
MessageVersion::V2 => {
let len = buffer.get_i16();
if len < 0 {
return 0; // or handle error appropriately
}
len as usize
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rocketmq-common/src/common/message.rs` around lines 339 - 345, The
get_topic_length function may cast a negative i16 to usize for
MessageVersion::V2 (buffer.get_i16()), causing huge lengths; update
MessageVersion::V2 handling in get_topic_length to either read an unsigned value
(use buffer.get_u16() semantics) or explicitly validate the i16 (check if < 0
and return an error or 0/handle appropriately) before converting to usize so
negative values cannot wrap into large sizes.
Comment on lines
+347
to
354
| /// Returns the topic length from the buffer at the specified index without advancing the | ||
| /// position. | ||
| pub fn get_topic_length_at_index(&self, buffer: &[u8], index: usize) -> usize { | ||
| match self { | ||
| MessageVersion::V1 => buffer[index] as usize, | ||
| MessageVersion::V2 => ((buffer[index] as usize) << 8) | (buffer[index + 1] as usize), | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Array access without bounds checking can panic.
get_topic_length_at_index accesses buffer[index] and buffer[index + 1] without validating that these indices are within bounds. This can cause a panic if the buffer is too short.
🛡️ Proposed fix: add bounds validation or return Result
Option 1 - Return Option<usize>:
- pub fn get_topic_length_at_index(&self, buffer: &[u8], index: usize) -> usize {
+ pub fn get_topic_length_at_index(&self, buffer: &[u8], index: usize) -> Option<usize> {
match self {
- MessageVersion::V1 => buffer[index] as usize,
+ MessageVersion::V1 => buffer.get(index).map(|&b| b as usize),
MessageVersion::V2 => {
+ if index + 1 >= buffer.len() {
+ return None;
+ }
- ((buffer[index] as usize) << 8) | (buffer[index + 1] as usize)
+ Some(((buffer[index] as usize) << 8) | (buffer[index + 1] as usize))
}
}
}Option 2 - Use .get() with a fallback if panicking is unacceptable in your use case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rocketmq-common/src/common/message.rs` around lines 347 - 354,
get_topic_length_at_index currently indexes buffer directly and can panic;
change its signature to return Option<usize> and perform bounds-checked access
via slice.get(): for MessageVersion::V1 use buffer.get(index).map(|b| *b as
usize), and for MessageVersion::V2 use buffer.get(index).and_then(|b0|
buffer.get(index + 1).map(|b1| (((*b0 as usize) << 8) | (*b1 as usize)))); this
prevents panics on short buffers and propagates None when bytes are missing.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6590 +/- ##
=======================================
Coverage 41.84% 41.84%
=======================================
Files 956 956
Lines 133557 133557
=======================================
Hits 55884 55884
Misses 77673 77673 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rocketmq-rust-bot
approved these changes
Mar 1, 2026
Collaborator
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which Issue(s) This PR Fixes(Closes)
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
Release Notes