[ISSUE #7122]🚀feat(cli): refactor ACL command implementations to use new request structures and improve error handling#7123
Merged
rocketmq-rust-bot merged 1 commit intomainfrom Apr 12, 2026
Merged
Conversation
…new request structures and improve error handling
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
Contributor
WalkthroughThis PR refactors ACL command implementations across the RocketMQ admin CLI to delegate ACL operations to a new centralized Changes
Sequence DiagramsequenceDiagram
actor User as CLI User
participant CLI as ACL Subcommand
participant Auth as AuthService
participant Admin as DefaultMQAdminExt
participant Broker as Broker RPC
User->>CLI: Execute ACL command with args
activate CLI
CLI->>CLI: Create *Request via try_new()
CLI->>Auth: call_acl_by_request_with_rpc_hook(request, hook)
activate Auth
Auth->>Admin: new + start()
activate Admin
alt Broker Target
Auth->>Admin: get_acl(broker_addr, subject)
Admin->>Broker: RPC call
Broker-->>Admin: AclInfo result
else Cluster Target
Auth->>Admin: resolve_cluster_addresses()
Admin-->>Auth: [broker_addrs]
Auth->>Admin: parallel get_acl() for each broker
Admin->>Broker: parallel RPC calls
Broker-->>Admin: AclInfo results
end
Admin->>Auth: return aggregated result
deactivate Admin
Auth->>Admin: shutdown()
Auth-->>CLI: *Result (success_data, failed_broker_addrs)
deactivate Auth
CLI->>CLI: render_*_result(result)
CLI-->>User: formatted output + warnings
deactivate CLI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rocketmq-tools/rocketmq-admin/rocketmq-admin-core/src/core/auth.rs (1)
1-1:⚠️ Potential issue | 🟡 MinorUse the repository-standard copyright year.
This header regresses from the repo convention and should stay on
2023.Based on learnings, In Rust source files (*.rs) across the rocketmq-rust repository, enforce using "Copyright 2023" as the year in the header instead of the current calendar year.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rocketmq-tools/rocketmq-admin/rocketmq-admin-core/src/core/auth.rs` at line 1, The file header in rocketmq-admin-core's auth.rs uses "Copyright 2026" which violates the repository standard; update the top-of-file copyright line in auth.rs to read "Copyright 2023" (preserving the rest of the header text and formatting) so it matches the repo-wide convention used across Rust source files in this crate.
🤖 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-tools/rocketmq-admin/rocketmq-admin-cli/src/commands/auth/get_acl_sub_command.rs`:
- Around line 56-60: In render_get_acl_result, don't treat single-broker
failures as "not found": change the early-return condition to require
result.failed_broker_addrs.is_empty() as well (i.e., only return Ok(()) and
print "No ACL..." when single_broker_target && result.acl_infos.is_empty() &&
result.failed_broker_addrs.is_empty()); if failed_broker_addrs is non-empty, do
not return early—report the broker failure(s) (include
result.failed_broker_addrs in the output or error path) so real
timeouts/rejections from the single broker are surfaced instead of being masked.
In `@rocketmq-tools/rocketmq-admin/rocketmq-admin-core/src/core/auth.rs`:
- Around line 1231-1241: The current match on
admin.get_acl(request.to_broker().clone(), subject.clone()).await treats any Err
as “ACL absent” and always calls create_acl_with_acl_info; instead inspect the
returned error from admin.get_acl and only call admin.create_acl_with_acl_info
when the error indicates a definite NotFound/missing-ACL condition, while for
transport/auth/RPC or other errors return/propagate the error into failures (do
not attempt create). Adjust the match/if handling around
admin.get_acl(...).await, using the error variant or status check to distinguish
NotFound from other errors before invoking create_acl_with_acl_info; keep the
existing update_acl_with_acl_info branch for Ok responses and ensure
non-NotFound errors are preserved in failures rather than swallowed.
- Around line 1202-1210: The loop that calls admin.get_acl currently treats any
Err as a failure; change it to detect "not found" results and record those into
skipped_subjects instead of failures while preserving successful AclInfo values
verbatim. Specifically, inside the loop in auth.rs where
admin.get_acl(request.from_broker().clone(), subject.clone()).await is matched,
map the NotFound/ACL-missing error (or the error variant/message your admin
client uses for a missing ACL) to pushing the subject into skipped_subjects
rather than pushing an AuthOperationFailure into failures; for all Ok(acl_info)
keep pushing the acl_info as-is into acl_infos, and for other error kinds
continue to push AuthOperationFailure. Apply the same change to the analogous
block referenced at lines 1226-1228. Ensure you reference admin.get_acl,
skipped_subjects, failures, AuthOperationFailure, and AclInfo when making the
edits.
- Around line 635-646: The current try_new uses
subjects.map(split_csv_values).filter(|subjects| !subjects.is_empty()) which
turns an explicitly-provided but-empty --subjects into None (meaning "copy
all"); instead, after mapping with split_csv_values in try_new, detect if
Some(vec) and vec.is_empty() and return an error (RocketMQResult::Err)
indicating an invalid/empty subjects list so consumers cannot accidentally
trigger "copy all"; update the try_new function (reference: try_new, the
subjects local variable and split_csv_values) to perform this check and return a
descriptive error rather than converting empty input to None.
In `@rocketmq-tools/rocketmq-admin/rocketmq-admin-core/tests/auth_core_models.rs`:
- Around line 1-8: The file lacks the repository's standard 2023
copyright/license header; prepend the canonical "Copyright 2023" header block to
the top of this test file (above the current imports such as AuthTarget,
CopyAclRequest, CreateAclRequest, CreateUserRequest, DeleteAclRequest,
ListAclRequest, ListUsersRequest, UpdateAclRequest) so the file matches the
project's required header convention.
- Around line 110-125: The test shows build_acl_info() produces a single ACL
entry with entry.resource containing comma-separated resources, but the admin
consumer in DefaultMQAdminExtImpl reads entry.resource as a single resource; fix
by either changing request.build_acl_info() to emit one ACL entry per individual
resource (split the input resource list into multiple entries) or update the
DefaultMQAdminExtImpl code that consumes entry.resource to split on commas and
iterate/create one outgoing ACL request per split resource before forwarding;
reference the request.build_acl_info() producer and the DefaultMQAdminExtImpl
consumer that currently uses entry.resource.
---
Outside diff comments:
In `@rocketmq-tools/rocketmq-admin/rocketmq-admin-core/src/core/auth.rs`:
- Line 1: The file header in rocketmq-admin-core's auth.rs uses "Copyright 2026"
which violates the repository standard; update the top-of-file copyright line in
auth.rs to read "Copyright 2023" (preserving the rest of the header text and
formatting) so it matches the repo-wide convention used across Rust source files
in this crate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aa4e5c95-fd24-4ebf-8d4f-e4bea58b60d1
📒 Files selected for processing (12)
rocketmq-client/src/admin/default_mq_admin_ext_impl.rsrocketmq-tools/rocketmq-admin/rocketmq-admin-cli/src/commands.rsrocketmq-tools/rocketmq-admin/rocketmq-admin-cli/src/commands/auth/copy_acl_sub_command.rsrocketmq-tools/rocketmq-admin/rocketmq-admin-cli/src/commands/auth/create_acl_sub_command.rsrocketmq-tools/rocketmq-admin/rocketmq-admin-cli/src/commands/auth/delete_acl_sub_command.rsrocketmq-tools/rocketmq-admin/rocketmq-admin-cli/src/commands/auth/get_acl_sub_command.rsrocketmq-tools/rocketmq-admin/rocketmq-admin-cli/src/commands/auth/list_acl_sub_command.rsrocketmq-tools/rocketmq-admin/rocketmq-admin-cli/src/commands/auth/update_acl_sub_command.rsrocketmq-tools/rocketmq-admin/rocketmq-admin-cli/src/commands/target.rsrocketmq-tools/rocketmq-admin/rocketmq-admin-core/src/admin/default_mq_admin_ext.rsrocketmq-tools/rocketmq-admin/rocketmq-admin-core/src/core/auth.rsrocketmq-tools/rocketmq-admin/rocketmq-admin-core/tests/auth_core_models.rs
💤 Files with no reviewable changes (2)
- rocketmq-tools/rocketmq-admin/rocketmq-admin-cli/src/commands.rs
- rocketmq-tools/rocketmq-admin/rocketmq-admin-cli/src/commands/target.rs
Comment on lines
+56
to
60
| fn render_get_acl_result(result: GetAclResult, subject: &str, single_broker_target: bool) -> RocketMQResult<()> { | ||
| if single_broker_target && result.acl_infos.is_empty() { | ||
| eprintln!("No ACL with subject {} was found", subject); | ||
| return Ok(()); | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t treat single-broker failures as “not found.”
This early return fires before failed_broker_addrs is checked. If the only broker times out or rejects the request, the command exits successfully and prints “No ACL…”, which masks a real failure. Only return the not-found case here when failed_broker_addrs is also empty.
Suggested fix
fn render_get_acl_result(result: GetAclResult, subject: &str, single_broker_target: bool) -> RocketMQResult<()> {
- if single_broker_target && result.acl_infos.is_empty() {
+ if single_broker_target
+ && result.acl_infos.is_empty()
+ && result.failed_broker_addrs.is_empty()
+ {
eprintln!("No ACL with subject {} was found", subject);
return Ok(());
}📝 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.
Suggested change
| fn render_get_acl_result(result: GetAclResult, subject: &str, single_broker_target: bool) -> RocketMQResult<()> { | |
| if single_broker_target && result.acl_infos.is_empty() { | |
| eprintln!("No ACL with subject {} was found", subject); | |
| return Ok(()); | |
| } | |
| fn render_get_acl_result(result: GetAclResult, subject: &str, single_broker_target: bool) -> RocketMQResult<()> { | |
| if single_broker_target | |
| && result.acl_infos.is_empty() | |
| && result.failed_broker_addrs.is_empty() | |
| { | |
| eprintln!("No ACL with subject {} was found", subject); | |
| return Ok(()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@rocketmq-tools/rocketmq-admin/rocketmq-admin-cli/src/commands/auth/get_acl_sub_command.rs`
around lines 56 - 60, In render_get_acl_result, don't treat single-broker
failures as "not found": change the early-return condition to require
result.failed_broker_addrs.is_empty() as well (i.e., only return Ok(()) and
print "No ACL..." when single_broker_target && result.acl_infos.is_empty() &&
result.failed_broker_addrs.is_empty()); if failed_broker_addrs is non-empty, do
not return early—report the broker failure(s) (include
result.failed_broker_addrs in the output or error path) so real
timeouts/rejections from the single broker are surfaced instead of being masked.
Comment on lines
+635
to
+646
| pub fn try_new( | ||
| from_broker: impl Into<String>, | ||
| to_broker: impl Into<String>, | ||
| subjects: Option<String>, | ||
| ) -> RocketMQResult<Self> { | ||
| let subjects = subjects.map(split_csv_values).filter(|subjects| !subjects.is_empty()); | ||
|
|
||
| Ok(Self { | ||
| from_broker: trim_required_cheetah("fromBroker", from_broker)?, | ||
| to_broker: trim_required_cheetah("toBroker", to_broker)?, | ||
| subjects, | ||
| namesrv_addr: None, |
Contributor
There was a problem hiding this comment.
Reject an empty --subjects list instead of copying everything.
If the flag is present but parses to zero subjects (--subjects "", " , "), this currently becomes None, which flips the request into “copy all ACLs”. That is a dangerous behavior change for a typoed targeted copy.
Suggested fix
pub fn try_new(
from_broker: impl Into<String>,
to_broker: impl Into<String>,
subjects: Option<String>,
) -> RocketMQResult<Self> {
- let subjects = subjects.map(split_csv_values).filter(|subjects| !subjects.is_empty());
+ let subjects = match subjects {
+ Some(subjects) => {
+ let parsed = split_csv_values(subjects);
+ if parsed.is_empty() {
+ return Err(
+ ToolsError::validation_error(
+ "subjects",
+ "subjects must contain at least one non-empty subject",
+ )
+ .into(),
+ );
+ }
+ Some(parsed)
+ }
+ None => None,
+ };
Ok(Self {
from_broker: trim_required_cheetah("fromBroker", from_broker)?,
to_broker: trim_required_cheetah("toBroker", to_broker)?,
subjects,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rocketmq-tools/rocketmq-admin/rocketmq-admin-core/src/core/auth.rs` around
lines 635 - 646, The current try_new uses
subjects.map(split_csv_values).filter(|subjects| !subjects.is_empty()) which
turns an explicitly-provided but-empty --subjects into None (meaning "copy
all"); instead, after mapping with split_csv_values in try_new, detect if
Some(vec) and vec.is_empty() and return an error (RocketMQResult::Err)
indicating an invalid/empty subjects list so consumers cannot accidentally
trigger "copy all"; update the try_new function (reference: try_new, the
subjects local variable and split_csv_values) to perform this check and return a
descriptive error rather than converting empty input to None.
Comment on lines
+1202
to
+1210
| let acl_infos = if let Some(subjects) = request.subjects() { | ||
| let mut acl_infos = Vec::new(); | ||
| for subject in subjects { | ||
| match admin.get_acl(request.from_broker().clone(), subject.clone()).await { | ||
| Ok(acl_info) => acl_infos.push(acl_info), | ||
| Err(error) => failures.push(AuthOperationFailure { | ||
| broker_addr: request.from_broker().clone(), | ||
| error: format!("get ACL {subject}: {error}"), | ||
| }), |
Contributor
There was a problem hiding this comment.
Missing source ACLs are being reported as failures, not skips.
skipped_subjects is only populated when the returned AclInfo has no subject, so a requested subject that does not exist on from_broker never reaches the skip path. That changes the command behavior and bypasses the "Could not find ACL ..." rendering that this result type already supports.
When you fix this, keep copying the fetched AclInfo as-is so source ACL fields are preserved verbatim. Based on learnings, In the copyAcls command implementation (rocketmq-client/src/admin/default_mq_admin_ext_impl.rs), when copying ACL entries, empty decision values from the source should be preserved as-is in the destination.
Also applies to: 1226-1228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rocketmq-tools/rocketmq-admin/rocketmq-admin-core/src/core/auth.rs` around
lines 1202 - 1210, The loop that calls admin.get_acl currently treats any Err as
a failure; change it to detect "not found" results and record those into
skipped_subjects instead of failures while preserving successful AclInfo values
verbatim. Specifically, inside the loop in auth.rs where
admin.get_acl(request.from_broker().clone(), subject.clone()).await is matched,
map the NotFound/ACL-missing error (or the error variant/message your admin
client uses for a missing ACL) to pushing the subject into skipped_subjects
rather than pushing an AuthOperationFailure into failures; for all Ok(acl_info)
keep pushing the acl_info as-is into acl_infos, and for other error kinds
continue to push AuthOperationFailure. Apply the same change to the analogous
block referenced at lines 1226-1228. Ensure you reference admin.get_acl,
skipped_subjects, failures, AuthOperationFailure, and AclInfo when making the
edits.
Comment on lines
+1231
to
+1241
| let copy_result = match admin.get_acl(request.to_broker().clone(), subject.clone()).await { | ||
| Ok(_) => { | ||
| admin | ||
| .update_acl_with_acl_info(request.to_broker().clone(), acl_info.clone()) | ||
| .await | ||
| } | ||
| Err(_) => { | ||
| admin | ||
| .create_acl_with_acl_info(request.to_broker().clone(), acl_info.clone()) | ||
| .await | ||
| } |
Contributor
There was a problem hiding this comment.
Do not treat every destination get_acl error as “ACL absent”.
The Err(_) => create branch also swallows transport/auth/RPC failures from the existence check. In those cases we should surface a failure, not attempt a create against a broker we just failed to query.
Only fall back to create_acl_with_acl_info for a definite not-found error; keep other errors in failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rocketmq-tools/rocketmq-admin/rocketmq-admin-core/src/core/auth.rs` around
lines 1231 - 1241, The current match on
admin.get_acl(request.to_broker().clone(), subject.clone()).await treats any Err
as “ACL absent” and always calls create_acl_with_acl_info; instead inspect the
returned error from admin.get_acl and only call admin.create_acl_with_acl_info
when the error indicates a definite NotFound/missing-ACL condition, while for
transport/auth/RPC or other errors return/propagate the error into failures (do
not attempt create). Adjust the match/if handling around
admin.get_acl(...).await, using the error variant or status check to distinguish
NotFound from other errors before invoking create_acl_with_acl_info; keep the
existing update_acl_with_acl_info branch for Ok responses and ensure
non-NotFound errors are preserved in failures rather than swallowed.
Comment on lines
1
to
+8
| use rocketmq_admin_core::core::auth::AuthTarget; | ||
| use rocketmq_admin_core::core::auth::CopyAclRequest; | ||
| use rocketmq_admin_core::core::auth::CreateAclRequest; | ||
| use rocketmq_admin_core::core::auth::CreateUserRequest; | ||
| use rocketmq_admin_core::core::auth::DeleteAclRequest; | ||
| use rocketmq_admin_core::core::auth::ListAclRequest; | ||
| use rocketmq_admin_core::core::auth::ListUsersRequest; | ||
| use rocketmq_admin_core::core::auth::UpdateAclRequest; |
Contributor
There was a problem hiding this comment.
Add the standard 2023 file header.
This test file now starts without the repository’s standard copyright/license header.
Based on learnings, Rust source files in this repository are expected to include the standard Copyright 2023 header.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rocketmq-tools/rocketmq-admin/rocketmq-admin-core/tests/auth_core_models.rs`
around lines 1 - 8, The file lacks the repository's standard 2023
copyright/license header; prepend the canonical "Copyright 2023" header block to
the top of this test file (above the current imports such as AuthTarget,
CopyAclRequest, CreateAclRequest, CreateUserRequest, DeleteAclRequest,
ListAclRequest, ListUsersRequest, UpdateAclRequest) so the file matches the
project's required header convention.
Comment on lines
+110
to
+125
| let acl_info = request.build_acl_info(); | ||
| assert_eq!( | ||
| acl_info.subject.as_ref().map(|subject| subject.as_str()), | ||
| Some("user:alice") | ||
| ); | ||
| let entries = acl_info | ||
| .policies | ||
| .as_ref() | ||
| .and_then(|policies| policies.first()) | ||
| .and_then(|policy| policy.entries.as_ref()) | ||
| .unwrap(); | ||
| let entry = entries.first().unwrap(); | ||
| assert_eq!( | ||
| entry.resource.as_ref().map(|resource| resource.as_str()), | ||
| Some("Topic:order-topic,Topic:user-topic") | ||
| ); |
Contributor
There was a problem hiding this comment.
This ACL shape collapses multiple resources into one broker request.
The new expectation here is entry.resource == "Topic:order-topic,Topic:user-topic", but the consumer in rocketmq-client/src/admin/default_mq_admin_ext_impl.rs treats entry.resource as a single resource at Line 218 and Line 262 and never splits commas. That means create/update will send one malformed resource instead of two whenever the CLI passes multiple resources.
Please either emit one ACL entry per resource from the request model or split entry.resource in the admin impl before forwarding it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rocketmq-tools/rocketmq-admin/rocketmq-admin-core/tests/auth_core_models.rs`
around lines 110 - 125, The test shows build_acl_info() produces a single ACL
entry with entry.resource containing comma-separated resources, but the admin
consumer in DefaultMQAdminExtImpl reads entry.resource as a single resource; fix
by either changing request.build_acl_info() to emit one ACL entry per individual
resource (split the input resource list into multiple entries) or update the
DefaultMQAdminExtImpl code that consumes entry.resource to split on commas and
iterate/create one outgoing ACL request per split resource before forwarding;
reference the request.build_acl_info() producer and the DefaultMQAdminExtImpl
consumer that currently uses entry.resource.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7123 +/- ##
==========================================
+ Coverage 59.29% 59.40% +0.11%
==========================================
Files 1074 1073 -1
Lines 186916 186876 -40
==========================================
+ Hits 110829 111012 +183
+ Misses 76087 75864 -223 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rocketmq-rust-bot
approved these changes
Apr 12, 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
New Features
Refactor