[ISSUE #4087]♻️Clean up remaining clippy warnings #4107
Conversation
WalkthroughTest-only updates across multiple modules: assertions adjusted (is_none → is_some, presence checks via contains_key), minor refactors to test structure (flattened #[cfg(test)] module), small variable/type tweaks, and a removed cast in a timestamp call. No production code or public API signatures changed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
rocketmq-rust-bot
requested review from
SpaceXCN,
mxsm and
rocketmq-rust-bot
|
🔊@WaterWhisperer 🚀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
There was a problem hiding this comment.
Pull Request Overview
This PR addresses clippy warnings across the RocketMQ Rust codebase by replacing inefficient or non-idiomatic patterns with more appropriate alternatives. The changes focus on improving code quality by using more direct method calls and removing unnecessary code structures.
Key changes include:
- Replacing
.is_none()checks with.is_some()or.contains_key()for better readability - Removing unnecessary type casts and variable mutability
- Cleaning up duplicate test module structures
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| rocketmq-remoting/src/protocol/route/route_data_view.rs | Replace .get().is_some() with .contains_key() |
| rocketmq-remoting/src/protocol/header/*.rs | Replace !.is_none() with .is_some() in test assertions |
| rocketmq-remoting/src/protocol/filter/filter_api.rs | Simplify variable assignment and usage pattern |
| rocketmq-remoting/src/protocol/body/epoch_entry_cache.rs | Remove unnecessary ::default() calls |
| rocketmq-remoting/src/protocol.rs | Remove duplicate test module and unnecessary mut keywords |
| rocketmq-common/src/utils/util_all.rs | Remove unnecessary type casts |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| fn new_creates_instance_of_epoch_entry_cache() { | ||
| let epoch_entry_cache = | ||
| EpochEntryCache::new("cluster1", "broker1", 1, vec![EpochEntry::default()], 1); | ||
| let epoch_entry_cache = EpochEntryCache::new("cluster1", "broker1", 1, vec![EpochEntry], 1); |
There was a problem hiding this comment.
The vector initialization vec![EpochEntry] is invalid syntax. It should be vec![EpochEntry::default()] or an equivalent valid constructor call.
There was a problem hiding this comment.
default_constructed_unit_structs adds code complexity and an unnecessary function call
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4107 +/- ##
==========================================
- Coverage 26.59% 26.59% -0.01%
==========================================
Files 575 575
Lines 81361 81360 -1
==========================================
- Hits 21636 21635 -1
Misses 59725 59725 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rocketmq-rust-bot
added
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rocketmq-remoting/src/protocol/route/route_data_view.rs (1)
124-127: remove_broker_by_addr never removes the targeted entry (predicate bug).Current retain keeps entries when
value != broker_addr || key == broker_id, which also keeps the exact pair you want to remove. Correct predicate is to drop only when both match.Apply:
- self.broker_addrs - .retain(|key, value| value != broker_addr || *key == broker_id); + self.broker_addrs + .retain(|key, value| !(value == broker_addr && *key == broker_id)); + // equivalently: retain(|key, value| value != broker_addr || *key != broker_id)
🧹 Nitpick comments (1)
rocketmq-remoting/src/protocol/header/query_consume_time_span_request_header.rs (1)
70-70: Duplicate test logic.This test appears to be testing the same scenario as the previous test but with identical assertions.
Consider consolidating these duplicate test cases or ensuring they test different scenarios:
#[test] -fn query_consume_time_span_request_header_handles_missing_optional_fields() { +fn query_consume_time_span_request_header_handles_partial_data() { let data = r#"{"topic":"test_topic","group":"test_group"}"#; let header: QueryConsumeTimeSpanRequestHeader = serde_json::from_str(data).unwrap(); assert_eq!(header.topic, CheetahString::from_static_str("test_topic")); assert_eq!(header.group, CheetahString::from_static_str("test_group")); - assert!(header.topic_request_header.is_some()); + // Test with additional nested fields if needed for differentiation }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
rocketmq-common/src/utils/util_all.rs(2 hunks)rocketmq-remoting/src/protocol.rs(3 hunks)rocketmq-remoting/src/protocol/body/epoch_entry_cache.rs(3 hunks)rocketmq-remoting/src/protocol/filter/filter_api.rs(1 hunks)rocketmq-remoting/src/protocol/header/ack_message_request_header.rs(1 hunks)rocketmq-remoting/src/protocol/header/delete_subscription_group_request_header.rs(2 hunks)rocketmq-remoting/src/protocol/header/delete_topic_request_header.rs(1 hunks)rocketmq-remoting/src/protocol/header/get_consumer_running_info_request_header.rs(2 hunks)rocketmq-remoting/src/protocol/header/get_topic_stats_info_request_header.rs(1 hunks)rocketmq-remoting/src/protocol/header/query_consume_time_span_request_header.rs(2 hunks)rocketmq-remoting/src/protocol/header/query_subscription_by_consumer_request_header.rs(2 hunks)rocketmq-remoting/src/protocol/header/reply_message_request_header.rs(1 hunks)rocketmq-remoting/src/protocol/header/unregister_client_request_header.rs(1 hunks)rocketmq-remoting/src/protocol/route/route_data_view.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
rocketmq-common/src/utils/util_all.rs (1)
rocketmq-remoting/src/protocol.rs (1)
timestamp(381-383)
rocketmq-remoting/src/protocol/filter/filter_api.rs (1)
rocketmq-broker/src/filter/consumer_filter_data.rs (2)
expression_type(56-58)topic(48-50)
⏰ 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). (9)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build (windows-latest, nightly)
- GitHub Check: build (ubuntu-latest, nightly)
- GitHub Check: build (macos-latest, nightly)
- GitHub Check: build (macos-latest, stable)
- GitHub Check: build (ubuntu-latest, stable)
- GitHub Check: test
- GitHub Check: build
- GitHub Check: auto-approve
🔇 Additional comments (15)
rocketmq-remoting/src/protocol/route/route_data_view.rs (1)
274-274: Clippy-friendly presence check LGTM.Replacing
.get(&2).is_some()withcontains_key(&2)is idiomatic and clearer.rocketmq-common/src/utils/util_all.rs (1)
428-429: Redundant cast removal LGTM.Passing
timestampdirectly totimestamp_millis_optis correct (argument isi64).Also applies to: 492-493
rocketmq-remoting/src/protocol/body/epoch_entry_cache.rs (1)
75-75: Unit struct construction cleanup LGTM.Using
vec![EpochEntry]for a unit struct is clearer thanDefault.Also applies to: 89-90, 100-101
rocketmq-remoting/src/protocol/filter/filter_api.rs (1)
129-129: Test refinement LGTM.Explicit
CheetahStringandSome(...)remove ambiguity; direct equality check avoids.unwrap().Also applies to: 133-133, 140-140
rocketmq-remoting/src/protocol/header/delete_topic_request_header.rs (1)
92-92: Incorrect — keepassert!(header.topic_request_header.is_some()).The RequestHeaderCodec derive expands an
Option<Struct>with#[serde(flatten)]intoSome(<Struct as FromMap>::from(map)?).TopicRequestHeader::fromitself setsrpc_request_headertoSome(RpcRequestHeader::from(map)?), so with only TOPIC present the field will still beSome. Leave the test assertion as-is.Likely an incorrect or invalid review comment.
rocketmq-remoting/src/protocol/header/delete_subscription_group_request_header.rs (1)
63-63: Keep assert!(header.rpc_request_header.is_some()) — original comment is incorrect.Serde's #[serde(flatten)] can produce Some(inner) (with inner fields set to None/default) even when no flattened keys are present; RpcRequestHeader derives Default and its fields are Option, so the existing asserts are correct. (serde.rs)
Likely an incorrect or invalid review comment.
rocketmq-remoting/src/protocol/header/get_consumer_running_info_request_header.rs (2)
88-88: Same flattened field assertion inconsistency.This test also asserts
is_some()for the flattened optional field when deserializing minimal JSON data. This pattern appears across multiple header files and may indicate a systematic issue with how flattened optional fields are being tested.
72-72: Incorrect — the original concern doesn't match repository behaviorMultiple headers use #[serde(flatten)] pub rpc_request_header: Option and existing tests assert .is_some() (e.g. rocketmq-remoting/src/protocol/header/unregister_client_request_header.rs), so changing to is_some() is consistent with the codebase; the review's claim about a flattened optional defaulting to None is not applicable here.
Likely an incorrect or invalid review comment.
rocketmq-remoting/src/protocol/header/query_consume_time_span_request_header.rs (1)
61-61: Consistent with flattened field pattern.Same issue as the previous file - the test expects
topic_request_header.is_some()but the serialized JSON doesn't contain any fields that would populate this flattened optional struct.rocketmq-remoting/src/protocol/header/ack_message_request_header.rs (1)
87-87: Flattened field assertion appears correct here.Unlike the previous files, this assertion makes more sense since the deserialization is working with the same JSON structure that would flatten the
topic_request_headerfields.rocketmq-remoting/src/protocol/header/reply_message_request_header.rs (1)
164-164: Improved test assertion usingcontains_key.This change from
map.get("topicRequest").is_none()to!map.contains_key("topicRequest")is more idiomatic and clearer about the intent to check for field absence.rocketmq-remoting/src/protocol/header/unregister_client_request_header.rs (1)
128-128: Inconsistent with JSON data being tested.The assertion expects
rpc_request_header.is_some(), but the test JSON{"clientID":"test_client_id","consumerGroup":"test_consumer_group"}doesn't contain fields that would populate the flattenedRpcRequestHeaderstruct.rocketmq-remoting/src/protocol.rs (3)
597-649: Test structure reorganization approved.The change from a nested
#[cfg(test)] mod tests {}to top-level#[test]functions is a good cleanup that flattens the test structure and removes unnecessary nesting. The variable binding changes (removingmutwhere not needed) are also good improvements.
654-654: Minor variable mutability cleanup.Removing
mutfromdata_version2when it's immediately cloned is a good optimization.
663-663: Consistent variable mutability cleanup.Good cleanup removing unnecessary
mutdeclarations where variables are only used for cloning/reading.Also applies to: 684-684
| let header: GetTopicStatsInfoRequestHeader = 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_some()); |
There was a problem hiding this comment.
🧩 Analysis chain
Serde flatten Option: asserting Some here is likely wrong.
JSON only contains "topic". With #[serde(flatten)] Option<T>, absence of flattened fields yields None. Unless TopicRequestHeader is empty or defaults are synthesized, these should assert is_none().
Apply:
- assert!(header.topic_request_header.is_some());
+ assert!(header.topic_request_header.is_none());Run to verify the struct shape/attrs:
Also applies to: 64-64
🏁 Script executed:
#!/bin/bash
rg -n -C3 $'struct\\s+TopicRequestHeader\\b' -g '!**/target/**'
rg -n -C2 '#\\[serde\\(default\\)' -g '!**/target/**' rocketmq-remoting/srcLength of output: 235
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "Top-level files:"
ls -la | sed -n '1,120p' || true
echo; echo "Searching for struct definitions and usages of TopicRequestHeader..."
rg -n --hidden --follow --no-messages "struct TopicRequestHeader" -S || true
rg -n --hidden --follow --no-messages "TopicRequestHeader" -S || true
echo; echo "Searching for serde attributes (flatten/default) under rocketmq-remoting/src if present..."
rg -n --hidden --follow --no-messages "serde\(" rocketmq-remoting/src -S || true
echo; echo "Printing get_topic_stats_info_request_header.rs (if exists):"
FILE="rocketmq-remoting/src/protocol/header/get_topic_stats_info_request_header.rs"
if [ -f "$FILE" ]; then
sed -n '1,240p' "$FILE"
else
echo "file not found: $FILE"
fiLength of output: 62624
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "----- namesrv/topic_operation_header.rs -----"
sed -n '1,240p' rocketmq-remoting/src/protocol/header/namesrv/topic_operation_header.rs || true
echo; echo "----- rpc/topic_request_header.rs -----"
sed -n '1,240p' rocketmq-remoting/src/rpc/topic_request_header.rs || trueLength of output: 7844
Expect None for flattened optional TopicRequestHeader in tests
TopicRequestHeader (namesrv) has only optional fields (lo, rpc); #[serde(flatten)] Option will be None when the JSON contains only "topic" — update the test assertions to expect None.
File: rocketmq-remoting/src/protocol/header/get_topic_stats_info_request_header.rs — apply:
- assert!(header.topic_request_header.is_some());
+ assert!(header.topic_request_header.is_none());Also update the duplicate assertion in the other test (~line 64).
🤖 Prompt for AI Agents
In rocketmq-remoting/src/protocol/header/get_topic_stats_info_request_header.rs
around line 56 (and the duplicate assertion near line 64), change the test
assertions to expect None for the flattened optional TopicRequestHeader: replace
the current assert!(header.topic_request_header.is_some()) checks with
assertions that header.topic_request_header.is_none(), since TopicRequestHeader
contains only optional fields and will be None when the JSON contains only
"topic".
| 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()); | ||
| assert!(header.topic_request_header.is_some()); |
There was a problem hiding this comment.
🧩 Analysis chain
Serde Option expectation inverted (should be None).
Deserializing {"group":"...","topic":"..."} provides no fields for the flattened TopicRequestHeader; topic_request_header should be None unless defaults are auto-injected.
Apply:
- assert!(header.topic_request_header.is_some());
+ assert!(header.topic_request_header.is_none());Sanity-check the header type:
Also applies to: 70-70
🏁 Script executed:
#!/bin/bash
rg -n -C3 $'struct\\s+TopicRequestHeader\\b' -g '!**/target/**'Length of output: 148
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "---- Inspect file if present ----"
FILE="rocketmq-remoting/src/protocol/header/query_subscription_by_consumer_request_header.rs"
if [ -f "$FILE" ]; then
nl -ba -w3 -s": " "$FILE" | sed -n '1,200p'
else
echo "FILE NOT FOUND: $FILE"
fi
echo "---- rg search: TopicRequestHeader ----"
rg -n --hidden -uu -S 'TopicRequestHeader' || true
echo "---- rg search: struct TopicRequestHeader ----"
rg -n --hidden -uu -P 'struct\s+TopicRequestHeader\b' || true
echo "---- rg search: topic_request_header occurrences ----"
rg -n --hidden -uu -S 'topic_request_header' || trueLength of output: 98
🏁 Script executed:
#!/bin/bash
echo "PWD: $(pwd)"
FILE="rocketmq-remoting/src/protocol/header/query_subscription_by_consumer_request_header.rs"
echo "---- Inspect file if present ----"
if [ -f "$FILE" ]; then
echo "FILE FOUND: $FILE"
echo "----- lines 1-200 -----"
sed -n '1,200p' "$FILE" || true
echo "----- lines 50-80 -----"
sed -n '50,80p' "$FILE" || true
else
echo "FILE NOT FOUND: $FILE"
fi
echo "---- ripgrep search (prefer) ----"
if command -v rg >/dev/null 2>&1; then
rg -n --hidden -uu -S 'TopicRequestHeader' -g '!**/target/**' || true
rg -n --hidden -uu -P 'struct\s+TopicRequestHeader\b' -g '!**/target/**' || true
rg -n --hidden -uu -S 'topic_request_header' -g '!**/target/**' || true
rg -n --hidden -uu -S 'TopicRequest' -g '!**/target/**' || true
else
echo "rg not available; falling back to grep"
grep -RIn --exclude-dir=target -E 'TopicRequestHeader|topic_request_header|TopicRequest' . || true
fi
echo "---- show workspace Cargo.toml if present ----"
if [ -f Cargo.toml ]; then
sed -n '1,200p' Cargo.toml || true
else
echo "No Cargo.toml at repo root"
fiLength of output: 8268
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo
echo "---- list rpc dir ----"
ls -la rocketmq-remoting/src/rpc || true
echo
echo "---- list protocol/header dir ----"
ls -la rocketmq-remoting/src/protocol/header || true
echo
echo "---- grep for TopicRequestHeader across crate ----"
grep -RIn --exclude-dir=target --exclude-dir=.git 'TopicRequestHeader' rocketmq-remoting || true
echo
echo "---- show candidate files ----"
for f in \
rocketmq-remoting/src/rpc/topic_request_header.rs \
rocketmq-remoting/src/protocol/header/topic_request_header.rs \
rocketmq-remoting/src/protocol/topic_request_header.rs \
rocketmq-remoting/src/rpc/mod.rs; do
if [ -f "$f" ]; then
echo
echo "------ $f ------"
sed -n '1,240p' "$f"
else
echo
echo "MISSING: $f"
fi
doneLength of output: 30392
Expect flattened Option to be None — fix tests.
Flattening an Option yields None when no nested fields are present; the tests currently assert Some. Update the two test assertions in rocketmq-remoting/src/protocol/header/query_subscription_by_consumer_request_header.rs (deserializes_correctly and handles_missing_optional_fields).
Apply:
- assert!(header.topic_request_header.is_some());
+ assert!(header.topic_request_header.is_none());📝 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.
| assert!(header.topic_request_header.is_some()); | |
| assert!(header.topic_request_header.is_none()); |
🤖 Prompt for AI Agents
In
rocketmq-remoting/src/protocol/header/query_subscription_by_consumer_request_header.rs
around line 61, the tests assert that a flattened Option<TopicRequestHeader> is
Some, but when nested fields are absent the flattened Option should be None;
update the two test assertions in the file (deserializes_correctly and
handles_missing_optional_fields) to assert that
header.topic_request_header.is_none() instead of is_some(), and run tests to
confirm.
| broker_data.remove_broker_by_addr(1, &"127.0.0.1".into()); | ||
| //assert!(broker_data.broker_addrs.get(&1).is_none()); | ||
| assert!(broker_data.broker_addrs.get(&2).is_some()); | ||
| assert!(broker_data.broker_addrs.contains_key(&2)); | ||
| } |
There was a problem hiding this comment.
Re-enable the assertion that verifies removal (prevent regression hiding).
The commented assertion is the one that would catch the bug above. Keep the new contains_key check, but also assert broker 1 was removed.
Apply:
- broker_data.remove_broker_by_addr(1, &"127.0.0.1".into());
- //assert!(broker_data.broker_addrs.get(&1).is_none());
- assert!(broker_data.broker_addrs.contains_key(&2));
+ broker_data.remove_broker_by_addr(1, &"127.0.0.1".into());
+ assert!(broker_data.broker_addrs.get(&1).is_none());
+ assert!(broker_data.broker_addrs.contains_key(&2));📝 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.
| broker_data.remove_broker_by_addr(1, &"127.0.0.1".into()); | |
| //assert!(broker_data.broker_addrs.get(&1).is_none()); | |
| assert!(broker_data.broker_addrs.get(&2).is_some()); | |
| assert!(broker_data.broker_addrs.contains_key(&2)); | |
| } | |
| broker_data.remove_broker_by_addr(1, &"127.0.0.1".into()); | |
| assert!(broker_data.broker_addrs.get(&1).is_none()); | |
| assert!(broker_data.broker_addrs.contains_key(&2)); | |
| } |
🤖 Prompt for AI Agents
In rocketmq-remoting/src/protocol/route/route_data_view.rs around lines 272 to
275, re-enable the commented assertion that verifies broker 1 was removed after
calling broker_data.remove_broker_by_addr(1, &"127.0.0.1".into()); — restore or
add an assertion such as assert!(broker_data.broker_addrs.get(&1).is_none());
and keep the existing assert!(broker_data.broker_addrs.contains_key(&2)); so the
test checks both that broker 1 was removed and broker 2 still exists.
4 participants
Which Issue(s) This PR Fixes(Closes)
Fixes #4087
Brief Description
Clean up remaining clippy warnings
Now all clippy warnings have been cleaned up
How Did You Test This Change?
ran
cargo clippyandcargo testas mentioned inCONTRIBUTING.md, all passed and there were no warningsSummary by CodeRabbit
Tests
Chores