[ISSUE #1583]🍻Add QuerySubscriptionByConsumerRequestHeader struct #1584
Conversation
WalkthroughThe pull request introduces a new public module 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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1584 +/- ##
==========================================
+ Coverage 25.28% 25.32% +0.03%
==========================================
Files 457 458 +1
Lines 60310 60341 +31
==========================================
+ Hits 15251 15282 +31
Misses 45059 45059 ☔ View full report in Codecov by Sentry. |
![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
🧹 Outside diff range and nitpick comments (1)
rocketmq-remoting/src/protocol/header/query_subscription_by_consumer_request_header.rs (1)
24-35: Add documentation for the struct and its fields.While the implementation is correct, please add documentation comments to:
- Explain the purpose of the struct
- Document each field's purpose and constraints
- Include example usage if applicable
This will improve maintainability and help other developers understand the intended usage.
Example documentation format:
/// Represents a request header for querying consumer subscriptions /// /// This header is used to query subscription information for a specific consumer group /// and topic combination. #[derive(Clone, Debug, Serialize, Deserialize, RequestHeaderCodec)] #[serde(rename_all = "camelCase")] pub struct QuerySubscriptionByConsumerRequestHeader { /// The consumer group name to query subscriptions for #[required] pub group: CheetahString, /// The topic name to query subscriptions 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/query_subscription_by_consumer_request_header.rs(1 hunks)
🔇 Additional comments (2)
rocketmq-remoting/src/protocol/header.rs (1)
52-52: LGTM!
The new module declaration follows the established pattern and is correctly placed in alphabetical order.
rocketmq-remoting/src/protocol/header/query_subscription_by_consumer_request_header.rs (1)
24-35: Verify consistency with other request header implementations.
The implementation looks good, but let's verify it follows the same patterns as other request headers in the codebase.
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Implementation is consistent with request header patterns
The implementation follows the established patterns in the codebase:
- Uses the standard derive attributes
Clone, Debug, Serialize, Deserialize, RequestHeaderCodec - Correctly applies
#[required]attribute for mandatory fields - Properly implements
Option<TopicRequestHeader>with#[serde(flatten)], consistent with 20+ other request headers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check patterns in other request header implementations
# Check if other request headers use similar patterns
echo "Checking patterns in other request headers..."
ast-grep --pattern $'#[derive(Clone, Debug, Serialize, Deserialize, RequestHeaderCodec)]
#[serde(rename_all = "camelCase")]
pub struct $_RequestHeader {
$$$
}'
# Check usage of required attribute
echo "Checking required attribute usage..."
ast-grep --pattern '#[required]'
# Check flattened TopicRequestHeader usage
echo "Checking TopicRequestHeader usage..."
rg -A 2 'pub topic_request_header: Option<TopicRequestHeader>'
Length of output: 9598
| #[cfg(test)] | ||
| mod tests { | ||
| use cheetah_string::CheetahString; | ||
|
|
||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn query_subscription_by_consumer_request_header_serializes_correctly() { | ||
| let header = QuerySubscriptionByConsumerRequestHeader { | ||
| group: CheetahString::from_static_str("test_group"), | ||
| topic: CheetahString::from_static_str("test_topic"), | ||
| topic_request_header: None, | ||
| }; | ||
| let serialized = serde_json::to_string(&header).unwrap(); | ||
| let expected = r#"{"group":"test_group","topic":"test_topic"}"#; | ||
| assert_eq!(serialized, expected); | ||
| } | ||
|
|
||
| #[test] | ||
| fn query_subscription_by_consumer_request_header_deserializes_correctly() { | ||
| let data = r#"{"group":"test_group","topic":"test_topic"}"#; | ||
| let header: QuerySubscriptionByConsumerRequestHeader = serde_json::from_str(data).unwrap(); | ||
| assert_eq!(header.group, CheetahString::from_static_str("test_group")); | ||
| assert_eq!(header.topic, CheetahString::from_static_str("test_topic")); | ||
| assert!(!header.topic_request_header.is_none()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn query_subscription_by_consumer_request_header_handles_missing_optional_fields() { | ||
| let data = r#"{"group":"test_group","topic":"test_topic"}"#; | ||
| let header: QuerySubscriptionByConsumerRequestHeader = serde_json::from_str(data).unwrap(); | ||
| assert_eq!(header.group, CheetahString::from_static_str("test_group")); | ||
| assert_eq!(header.topic, CheetahString::from_static_str("test_topic")); | ||
| assert!(!header.topic_request_header.is_none()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn query_subscription_by_consumer_request_header_handles_invalid_data() { | ||
| let data = r#"{"group":12345,"topic":"test_topic"}"#; | ||
| let result: Result<QuerySubscriptionByConsumerRequestHeader, _> = | ||
| serde_json::from_str(data); | ||
| assert!(result.is_err()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test coverage and assertions.
The test suite has several issues that should be addressed:
- The assertions on lines 61 and 70 use a double negative which reduces readability:
-assert!(!header.topic_request_header.is_none());
+assert!(header.topic_request_header.is_none());-
The test
handles_missing_optional_fieldsis redundant withdeserializes_correctly. Consider removing it or testing different scenarios. -
Add a test case for when
topic_request_headeris present to ensure proper serialization and deserialization of the flattened field.
Example additional test:
#[test]
fn query_subscription_by_consumer_request_header_with_topic_header() {
let header = QuerySubscriptionByConsumerRequestHeader {
group: CheetahString::from_static_str("test_group"),
topic: CheetahString::from_static_str("test_topic"),
topic_request_header: Some(TopicRequestHeader {
// Add required fields here
}),
};
let serialized = serde_json::to_string(&header).unwrap();
let deserialized: QuerySubscriptionByConsumerRequestHeader =
serde_json::from_str(&serialized).unwrap();
assert_eq!(header.group, deserialized.group);
assert_eq!(header.topic, deserialized.topic);
assert!(deserialized.topic_request_header.is_some());
// Add assertions for topic_request_header fields
}
rocketmq-rust-bot
added
approved
Which Issue(s) This PR Fixes(Closes)
Fixes #1583
Brief Description
How Did You Test This Change?
Summary by CodeRabbit