[ISSUE #1565]🍻Add GetTopicStatsInfoRequestHeader struct 💡 #1576
Conversation
WalkthroughThe changes introduce a new module and a corresponding struct for handling request headers in the RocketMQ protocol. Specifically, a module named Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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
|
|
🔊@mxsm 🚀Thanks for your contribution 🎉. CodeRabbit(AI) will review your code first 🔥 |
rocketmq-rust-bot
added
AI review first
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
rocketmq-remoting/src/protocol/header/get_topic_stats_info_request_header.rs (1)
24-32: Add documentation for the struct and its fieldsWhile the struct definition is technically correct, it would benefit from documentation explaining:
- The purpose of this request header
- The meaning and usage of each field
- Any constraints or requirements for the fields
Add documentation using rustdoc format:
+/// Request header for getting topic statistics information. +/// This header is used in the RocketMQ protocol to request statistical data about a specific topic. #[derive(Clone, Debug, Serialize, Deserialize, Default, RequestHeaderCodec)] #[serde(rename_all = "camelCase")] pub struct QueryConsumeTimeSpanRequestHeader { + /// The name of the topic to get statistics for. #[required] pub topic: CheetahString, + /// Additional topic-specific request parameters. #[serde(flatten)] pub topic_request_header: Option<TopicRequestHeader>, }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
rocketmq-remoting/src/protocol/header.rs(1 hunks)rocketmq-remoting/src/protocol/header/get_topic_stats_info_request_header.rs(1 hunks)
🔇 Additional comments (1)
rocketmq-remoting/src/protocol/header.rs (1)
38-38: LGTM! Module declaration follows project conventions.
The new module declaration is correctly placed and follows the existing pattern of organizing protocol headers.
| #[cfg(test)] | ||
| mod tests { | ||
| use cheetah_string::CheetahString; | ||
|
|
||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn query_consume_time_span_request_header_serializes_correctly() { | ||
| let header = QueryConsumeTimeSpanRequestHeader { | ||
| topic: CheetahString::from_static_str("test_topic"), | ||
| topic_request_header: None, | ||
| }; | ||
| let serialized = serde_json::to_string(&header).unwrap(); | ||
| let expected = r#"{"topic":"test_topic"}"#; | ||
| assert_eq!(serialized, expected); | ||
| } | ||
|
|
||
| #[test] | ||
| fn query_consume_time_span_request_header_deserializes_correctly() { | ||
| let data = r#"{"topic":"test_topic"}"#; | ||
| let header: QueryConsumeTimeSpanRequestHeader = serde_json::from_str(data).unwrap(); | ||
| assert_eq!(header.topic, CheetahString::from_static_str("test_topic")); | ||
| assert!(!header.topic_request_header.is_none()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn query_consume_time_span_request_header_handles_missing_optional_fields() { | ||
| let data = r#"{"topic":"test_topic"}"#; | ||
| let header: QueryConsumeTimeSpanRequestHeader = serde_json::from_str(data).unwrap(); | ||
| assert_eq!(header.topic, CheetahString::from_static_str("test_topic")); | ||
| assert!(!header.topic_request_header.is_none()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn query_consume_time_span_request_header_handles_invalid_data() { | ||
| let data = r#"{"topic":12345}"#; | ||
| let result: Result<QueryConsumeTimeSpanRequestHeader, _> = serde_json::from_str(data); | ||
| assert!(result.is_err()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix incorrect test assertions and improve test coverage
There are several issues in the test implementation:
- Lines 56 and 64 have incorrect assertions:
assert!(!header.topic_request_header.is_none()); // This asserts the field is Some, but we expect None- The test cases don't fully cover the struct's functionality, missing tests for:
- Case with
topic_request_headerpresent - Serialization with
topic_request_header
- Case with
Here's how to fix and improve the tests:
#[test]
fn query_consume_time_span_request_header_deserializes_correctly() {
let data = r#"{"topic":"test_topic"}"#;
let header: QueryConsumeTimeSpanRequestHeader = serde_json::from_str(data).unwrap();
assert_eq!(header.topic, CheetahString::from_static_str("test_topic"));
- assert!(!header.topic_request_header.is_none());
+ assert!(header.topic_request_header.is_none());
}
#[test]
fn query_consume_time_span_request_header_handles_missing_optional_fields() {
let data = r#"{"topic":"test_topic"}"#;
let header: QueryConsumeTimeSpanRequestHeader = serde_json::from_str(data).unwrap();
assert_eq!(header.topic, CheetahString::from_static_str("test_topic"));
- assert!(!header.topic_request_header.is_none());
+ assert!(header.topic_request_header.is_none());
}
+#[test]
+fn query_consume_time_span_request_header_with_topic_request_header() {
+ let header = QueryConsumeTimeSpanRequestHeader {
+ topic: CheetahString::from_static_str("test_topic"),
+ topic_request_header: Some(TopicRequestHeader::default()),
+ };
+ let serialized = serde_json::to_string(&header).unwrap();
+ let deserialized: QueryConsumeTimeSpanRequestHeader = serde_json::from_str(&serialized).unwrap();
+ assert_eq!(deserialized.topic, CheetahString::from_static_str("test_topic"));
+ assert!(deserialized.topic_request_header.is_some());
+}| #[derive(Clone, Debug, Serialize, Deserialize, Default, RequestHeaderCodec)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub struct QueryConsumeTimeSpanRequestHeader { | ||
| #[required] | ||
| pub topic: CheetahString, | ||
|
|
||
| #[serde(flatten)] | ||
| pub topic_request_header: Option<TopicRequestHeader>, | ||
| } |
There was a problem hiding this comment.
Struct name doesn't match PR objective
The PR and issue #1565 specify adding a GetTopicStatsInfoRequestHeader struct, but the implementation shows QueryConsumeTimeSpanRequestHeader. This appears to be implementing a different feature than what was intended.
Please rename the struct to match the intended feature:
-pub struct QueryConsumeTimeSpanRequestHeader {
+pub struct GetTopicStatsInfoRequestHeader {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[derive(Clone, Debug, Serialize, Deserialize, Default, RequestHeaderCodec)] | |
| #[serde(rename_all = "camelCase")] | |
| pub struct QueryConsumeTimeSpanRequestHeader { | |
| #[required] | |
| pub topic: CheetahString, | |
| #[serde(flatten)] | |
| pub topic_request_header: Option<TopicRequestHeader>, | |
| } | |
| #[derive(Clone, Debug, Serialize, Deserialize, Default, RequestHeaderCodec)] | |
| #[serde(rename_all = "camelCase")] | |
| pub struct GetTopicStatsInfoRequestHeader { | |
| #[required] | |
| pub topic: CheetahString, | |
| #[serde(flatten)] | |
| pub topic_request_header: Option<TopicRequestHeader>, | |
| } |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1576 +/- ##
==========================================
+ Coverage 25.25% 25.28% +0.03%
==========================================
Files 456 457 +1
Lines 60283 60310 +27
==========================================
+ Hits 15224 15251 +27
Misses 45059 45059 ☔ View full report in Codecov by Sentry. |
rocketmq-rust-bot
added
approved
Which Issue(s) This PR Fixes(Closes)
Fixes #1565
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
New Features
Tests