feat: implement GetUserSubCommand (#5662) #6041
Merged
+505
−53
Conversation
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
rocketmq-rust-bot
requested review from
TeslaRustor,
mxsm and
rocketmq-rust-bot
Collaborator
|
🔊@magogosora 🚀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💥. |
Contributor
WalkthroughAdds a GetUser admin flow: broker-side GetUserRequestHandler and wiring, remoting header, MQ client API and admin impl support returning Option, CLI GetUser subcommand, and a Target abstraction for broker/cluster selection. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as GetUserSubCommand
participant AdminExt as MQAdminExt
participant AdminImpl as DefaultMQAdminExtImpl
participant ClientAPI as MQClientAPIImpl
participant Remoting as RemotingClient
participant Broker as AdminBrokerProcessor
participant Handler as GetUserRequestHandler
CLI->>AdminExt: get_user(broker_addr, username)
AdminExt->>AdminImpl: get_user(broker_addr, username)
AdminImpl->>ClientAPI: get_user(broker_addr, username, timeout)
ClientAPI->>Remoting: send AuthGetUser (with GetUserRequestHeader)
Remoting->>Broker: deliver AuthGetUser
Broker->>Handler: get_user(channel, ctx, request_code, request)
Handler-->>Broker: RemotingCommand (response)
Broker-->>Remoting: response
Remoting-->>ClientAPI: response
ClientAPI->>AdminImpl: Option<UserInfo>
AdminImpl-->>AdminExt: Option<UserInfo]
AdminExt-->>CLI: Option<UserInfo]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 5
🤖 Fix all issues with AI agents
In
`@rocketmq-broker/src/processor/admin_broker_processor/get_user_request_handler.rs`:
- Around line 1-9: This file is missing the Apache 2.0 license header; add the
standard Apache 2.0 copyright header (starting with "// Copyright 2023 The
RocketMQ Rust Authors" and the full license boilerplate) at the very top of the
file, above the first use statements (e.g. above "use
crate::broker_runtime::BrokerRuntimeInner;") so the header appears before any
code or imports in get_user_request_handler.rs; ensure the header matches the
repository's existing files exactly.
- Around line 20-34: Update the commented decode line in the get_user function
to reference the correct request header type: replace the commented use of
ListUsersRequestHeader with GetUserRequestHeader so it reads the intended call
to request.decode_command_custom_header::<GetUserRequestHeader>()?; locate this
in the get_user method where the commented request_header line appears and
adjust the type name only.
In `@rocketmq-client/src/implementation/mq_client_api_impl.rs`:
- Around line 313-349: In get_user, when matching ResponseCode::Success and
response.get_body() is None you currently call mq_client_err! with
response.code() (a success code); change this to return a proper non-success
error instead of reporting a success code — e.g. construct the error with a
meaningful failure code (use ResponseCode::SystemError or another appropriate
non-success enum variant) and a clear message like "missing response body for
user '<username>'", or alternatively mirror list_users and return an explicit
"not found"/empty result type; update the error construction site (the closure
passed to ok_or_else) so mq_client_err! is called with the chosen non-success
code and descriptive remark before attempting UserInfo::decode.
In
`@rocketmq-tools/rocketmq-admin/rocketmq-admin-core/src/commands/auth_commands.rs`:
- Around line 77-82: Update the CLI help text for the GetUser command so it
matches the issue description: change the about string on the GetUser command
attribute (the enum variant GetUser in auth_commands.rs with #[command(name =
"getUser", ...)]) from "Get user." to "Get user from cluster" (keep long_about
as-is).
In
`@rocketmq-tools/rocketmq-admin/rocketmq-admin-core/src/commands/auth_commands/get_user_sub_command.rs`:
- Around line 98-101: The cluster-target branch (Target::ClusterName) calls
print_users(&user_info) without first printing the header, causing inconsistent
output; update the Target::ClusterName handling in get_user_sub_command.rs to
call print_header() before print_users(&user_info) (mirror what the
broker-target branch does with print_header() and print_user()), and make the
same change at the other similar cluster-handling site later in the file so both
occurrences print the header before printing users.
🧹 Nitpick comments (1)
rocketmq-tools/rocketmq-admin/rocketmq-admin-core/src/commands/target.rs (1)
45-76: Consider adding edge case tests for normalization.The tests cover the main scenarios well. Consider adding tests for edge cases handled by
normalize:
- Whitespace-only cluster name:
Target::new(&Some(" ".into()), &None)should error- Empty string broker address:
Target::new(&None, &Some("".into()))should error
rocketmq-broker/src/processor/admin_broker_processor/get_user_request_handler.rs
Show resolved
Hide resolved
rocketmq-broker/src/processor/admin_broker_processor/get_user_request_handler.rs
Show resolved
Hide resolved
rocketmq-tools/rocketmq-admin/rocketmq-admin-core/src/commands/auth_commands.rs
Show resolved
Hide resolved
...-tools/rocketmq-admin/rocketmq-admin-core/src/commands/auth_commands/get_user_sub_command.rs
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6041 +/- ##
==========================================
- Coverage 40.54% 40.50% -0.04%
==========================================
Files 881 884 +3
Lines 122549 122760 +211
==========================================
+ Hits 49682 49726 +44
- Misses 72867 73034 +167 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rocketmq-rust-bot
previously approved these changes
Feb 3, 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 ✅
magogosora
force-pushed
the
feature/issue-5662-implement-get-user-sub-command
branch
from
5931778 to
ad28f48
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rocketmq-broker/src/processor/admin_broker_processor/list_users_request_handler.rs (1)
34-47:⚠️ Potential issue | 🟠 MajorAuthListUsers still always fails (SystemError).
This handler returns a hard-coded SystemError, so any AuthListUsers request will fail even though the client/admin APIs exist. If list-users is intended to work, please implement the retrieval via the authentication metadata manager and return a Success response with an encoded body. If it is intentionally unsupported, consider routing to the “not supported” response instead of SystemError for consistency.
rocketmq-tools/rocketmq-admin/rocketmq-admin-core/src/commands/auth_commands/copy_users_sub_command.rs (1)
182-193:⚠️ Potential issue | 🟠 MajorHandle Ok(None) when deciding create vs update.
With the new Option return, Ok(None) means the user doesn’t exist on the target; the currentis_err()check routes those cases to update, which will likely fail or silently skip creation.🔧 Suggested fix
- let copy_result = if target_user_result.is_err() { - default_mqadmin_ext - .create_user_with_user_info(target_broker.into(), user_info.clone()) - .await - } else { - default_mqadmin_ext - .update_user_with_user_info(target_broker.into(), user_info.clone()) - .await - }; + let copy_result = match target_user_result { + Ok(Some(_)) => { + default_mqadmin_ext + .update_user_with_user_info(target_broker.into(), user_info.clone()) + .await + } + Ok(None) | Err(_) => { + default_mqadmin_ext + .create_user_with_user_info(target_broker.into(), user_info.clone()) + .await + } + };
rocketmq-rust-bot
approved these changes
Feb 3, 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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
Improvements
Bug Fixes