<feature>[identity]: add ChangeAccountType API#3519
Closed
ZStack-Robot wants to merge 1 commit into
Closed
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough新增账户类型变更端到端功能：包含 API 消息与事件、拦截校验、业务处理（含扩展点调用）、持久化与事件发布、SDK 调用类、中文 API 文档及测试辅助方法，支持在 SystemAdmin 与 Normal 间提权/降权并返回更新后的 AccountInventory。 Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant API as API Gateway / AccountManagerImpl
participant Interceptor as Interceptor/Validator
participant Service as AccountBase
participant Ext as AccountTypeChangedExtensionPoint
participant DB as Database
Client->>API: PUT /accounts/{uuid}/actions\n(APIChangeAccountTypeMsg)
API->>Interceptor: intercept(msg)
Interceptor->>DB: 查询目标账户(uuid)
DB-->>Interceptor: 返回账户记录
alt 验证失败
Interceptor-->>Client: 抛出 ApiMessageInterceptionException
else 验证通过
API->>Service: handle(APIChangeAccountTypeMsg)
Service->>Ext: preAccountTypeChange(accountUuid, oldType, newType)
Ext-->>Service: ErrorCode / null
opt no error
Service->>Ext: beforeAccountTypeChange(accountUuid, oldType, newType)
Ext-->>Service: 返回
Service->>DB: 更新并持久化账户 type
DB-->>Service: 更新确认
Service->>Ext: afterAccountTypeChange(accountUuid, newType)
Ext-->>Service: 返回
Service-->>Client: 返回 APIChangeAccountTypeEvent(inventory)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
| location "url" | ||
| type "String" | ||
| optional false | ||
| since "4.10.0" |
Collaborator
There was a problem hiding this comment.
Comment from wenhao.zhang:
5.0.0
![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
🧹 Nitpick comments (3)
sdk/src/main/java/org/zstack/sdk/ChangeAccountTypeAction.java (2)
5-5: 冗余导入
import org.zstack.sdk.*;是冗余的，因为当前类已经位于org.zstack.sdk包中。♻️ 建议移除
import java.util.HashMap; import java.util.Map; -import org.zstack.sdk.*;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/main/java/org/zstack/sdk/ChangeAccountTypeAction.java` at line 5, The file ChangeAccountTypeAction.java contains a redundant self-package wildcard import "import org.zstack.sdk.*;" which should be removed; open the ChangeAccountTypeAction class and delete that import statement so the class relies on its package scope or explicit imports only, ensuring no other code references break and the file compiles.
37-41: 建议使用泛型类型而非原始类型
systemTags和userTags字段使用了原始类型List，建议指定泛型参数以提高类型安全性。♻️ 建议修改
`@Param`(required = false) - public java.util.List systemTags; + public java.util.List<String> systemTags; `@Param`(required = false) - public java.util.List userTags; + public java.util.List<String> userTags;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/main/java/org/zstack/sdk/ChangeAccountTypeAction.java` around lines 37 - 41, The fields systemTags and userTags in ChangeAccountTypeAction are declared as raw java.util.List; update them to use a generic type (e.g., java.util.List<String>) to enforce type safety and avoid raw-type warnings, and adjust any code that reads/writes these fields to work with the chosen generic type (ensure any serialization or callers expecting raw List are updated to List<String> as well).header/src/main/java/org/zstack/header/identity/APIChangeAccountTypeEventDoc_zh_cn.groovy (1)
12-15: Event 文档中不应包含request块
APIChangeAccountTypeEvent是 API 响应事件类，其文档中不应包含request块。request块通常用于描述 API 消息（如APIChangeAccountTypeMsgDoc_zh_cn.groovy）的请求参数。建议移除或修正此
request块：♻️ 建议修改
rest { - request { - desc """变更账户类型的返回结果""" - } - response { clz APIChangeAccountTypeEvent.class🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/identity/APIChangeAccountTypeEventDoc_zh_cn.groovy` around lines 12 - 15, The event doc APIChangeAccountTypeEventDoc_zh_cn.groovy incorrectly contains a rest { request { ... } } block; remove the request block (and its desc content) so the file documents the response/event only (keep or move any response-related desc into rest { response { ... } } if needed), ensuring only response/event documentation remains in APIChangeAccountTypeEventDoc_zh_cn.groovy and request params stay in APIChangeAccountTypeMsgDoc_zh_cn.groovy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@identity/src/main/java/org/zstack/identity/AccountBase.java`:
- Around line 312-323: The extension callbacks for
AccountTypeChangedExtensionPoint are being invoked with
CollectionUtils.safeForEach which swallows exceptions allowing account.setType
and dbf.updateAndRefresh to succeed even if extensions fail; change the calls
that use
CollectionUtils.safeForEach(pluginRgty.getExtensionList(AccountTypeChangedExtensionPoint.class),
...) to a loop that invokes each ext.beforeAccountTypeChange(...) and
ext.afterAccountTypeChange(...) and lets any thrown exception propagate (or
catches and wraps/rethrows) so that failures abort the operation and trigger
transaction/request failure; ensure this applies to both the before and after
invocation surrounding account.setType and dbf.updateAndRefresh so partial state
changes cannot be committed when extensions fail.
- Around line 315-320: In AccountBase where account type is changed (the if
(msg.isPromote()) { account.setType(...) } block and the similar branch around
lines 325–349), you must call the cleanup helpers after persisting the new type:
invoke detachAccountRoles(account.getUuid()) and
revokeAccountSharedResources(account.getUuid()) for the case that
removes/changes role/shared-resource semantics (e.g., when promoting to
SystemAdmin or when demoting to Normal as appropriate), ensure calls run in the
same transactional context or catch/log exceptions so persistence and event
publishing remain consistent, and perform the cleanup before publishing the
account-change event so the system state matches the new AccountVO.type.
In `@identity/src/main/java/org/zstack/identity/AccountManagerImpl.java`:
- Around line 1003-1019: The current AccountManagerImpl rejects demotion (if
!msg.isPromote()) but AccountBase and APIChangeAccountTypeMsgDoc_zh_cn.groovy
still declare/handle demotion, causing inconsistent API behavior; update
AccountManagerImpl to handle both promote and demote paths consistently (or
remove demotion support from docs and AccountBase), by using msg.isPromote() to
branch: when promoting ensure account.getType() != AccountType.SystemAdmin and
perform promotion; when demoting ensure account.getType() ==
AccountType.SystemAdmin and perform demotion; also update or remove the doc
entry in APIChangeAccountTypeMsgDoc_zh_cn.groovy and any demotion logic in
AccountBase to match the implemented behavior so runtime, code, and
documentation are consistent.
---
Nitpick comments:
In
`@header/src/main/java/org/zstack/header/identity/APIChangeAccountTypeEventDoc_zh_cn.groovy`:
- Around line 12-15: The event doc APIChangeAccountTypeEventDoc_zh_cn.groovy
incorrectly contains a rest { request { ... } } block; remove the request block
(and its desc content) so the file documents the response/event only (keep or
move any response-related desc into rest { response { ... } } if needed),
ensuring only response/event documentation remains in
APIChangeAccountTypeEventDoc_zh_cn.groovy and request params stay in
APIChangeAccountTypeMsgDoc_zh_cn.groovy.
In `@sdk/src/main/java/org/zstack/sdk/ChangeAccountTypeAction.java`:
- Line 5: The file ChangeAccountTypeAction.java contains a redundant
self-package wildcard import "import org.zstack.sdk.*;" which should be removed;
open the ChangeAccountTypeAction class and delete that import statement so the
class relies on its package scope or explicit imports only, ensuring no other
code references break and the file compiles.
- Around line 37-41: The fields systemTags and userTags in
ChangeAccountTypeAction are declared as raw java.util.List; update them to use a
generic type (e.g., java.util.List<String>) to enforce type safety and avoid
raw-type warnings, and adjust any code that reads/writes these fields to work
with the chosen generic type (ensure any serialization or callers expecting raw
List are updated to List<String> as well).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ac619ed7-1680-4f9a-9b75-1a8093224a82
⛔ Files ignored due to path filters (1)
conf/serviceConfig/identity.xmlis excluded by!**/*.xml
📒 Files selected for processing (10)
header/src/main/java/org/zstack/header/identity/APIChangeAccountTypeEvent.javaheader/src/main/java/org/zstack/header/identity/APIChangeAccountTypeEventDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/identity/APIChangeAccountTypeMsg.javaheader/src/main/java/org/zstack/header/identity/APIChangeAccountTypeMsgDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/identity/AccountTypeChangedExtensionPoint.javaidentity/src/main/java/org/zstack/identity/AccountBase.javaidentity/src/main/java/org/zstack/identity/AccountManagerImpl.javasdk/src/main/java/org/zstack/sdk/ChangeAccountTypeAction.javasdk/src/main/java/org/zstack/sdk/ChangeAccountTypeResult.javatestlib/src/main/java/org/zstack/testlib/ApiHelper.groovy
MatheMatrix
force-pushed
the
sync/zstackio/ZSPHERE-175@@2
branch
from
c467404 to
033da13
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
header/src/main/java/org/zstack/header/identity/AccountTypeChangedExtensionPoint.java (1)
1-24: 扩展点接口设计良好，但可考虑增强语义清晰度。接口定义清晰，遵循 ZStack 扩展点模式（
before/after生命周期钩子）。Javadoc 注释完整。有一个小建议：
boolean promote参数虽然在文档中解释清楚了，但根据编码指南关于避免布尔参数导致含义不明确的建议，可以考虑使用枚举类型（如AccountTypeChangeDirection.PROMOTE/DEMOTE）来增强可读性。此外，当前方法签名为
void且未声明受检异常。如果扩展点实现需要能够中止操作（例如在beforeAccountTypeChange中进行校验失败时），可能需要考虑添加异常声明或返回值机制。♻️ 可选：使用枚举替代布尔参数
// 定义枚举 public enum AccountTypeChangeDirection { PROMOTE, // 提权：Normal -> SystemAdmin DEMOTE // 降权：SystemAdmin -> Normal } // 修改接口方法签名 void beforeAccountTypeChange(String accountUuid, AccountTypeChangeDirection direction); void afterAccountTypeChange(String accountUuid, AccountTypeChangeDirection direction);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/identity/AccountTypeChangedExtensionPoint.java` around lines 1 - 24, Replace the ambiguous boolean parameter with a clear enum and make the before-hook able to abort: introduce an enum AccountTypeChangeDirection {PROMOTE, DEMOTE} and update the AccountTypeChangedExtensionPoint method signatures from beforeAccountTypeChange(String, boolean) and afterAccountTypeChange(String, boolean) to beforeAccountTypeChange(String, AccountTypeChangeDirection) and afterAccountTypeChange(String, AccountTypeChangeDirection); also update Javadoc accordingly and make beforeAccountTypeChange either declare a checked exception (e.g., AccountTypeChangeException) or return a boolean/Result type so implementations can veto the change (ensure the implementing callers are adapted to handle the exception/return).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@header/src/main/java/org/zstack/header/identity/APIChangeAccountTypeMsg.java`:
- Around line 37-40: The getAccountUuid() implementation in
APIChangeAccountTypeMsg is returning the caller's session account UUID via
getSession().getAccountUuid(), which incorrectly attributes the message to the
invoking account; change getAccountUuid() to return the target resource's uuid
field (the message's uuid property) so the message is owned by the account being
modified (use the class's uuid getter/field instead of getSession()).
In `@sdk/src/main/java/org/zstack/sdk/ChangeAccountTypeAction.java`:
- Around line 34-35: The SDK action ChangeAccountTypeAction exposes a client
parameter detachAllBindings that does not exist on the server message
APIChangeAccountTypeMsg (which only has uuid and promote), causing a contract
mismatch; either remove the detachAllBindings field from ChangeAccountTypeAction
to match the server contract, or if the server should support it, add a
corresponding field to APIChangeAccountTypeMsg and server-side handling,
ensuring names and types match (refer to
ChangeAccountTypeAction.detachAllBindings and
APIChangeAccountTypeMsg.uuid/promote when making the change).
---
Nitpick comments:
In
`@header/src/main/java/org/zstack/header/identity/AccountTypeChangedExtensionPoint.java`:
- Around line 1-24: Replace the ambiguous boolean parameter with a clear enum
and make the before-hook able to abort: introduce an enum
AccountTypeChangeDirection {PROMOTE, DEMOTE} and update the
AccountTypeChangedExtensionPoint method signatures from
beforeAccountTypeChange(String, boolean) and afterAccountTypeChange(String,
boolean) to beforeAccountTypeChange(String, AccountTypeChangeDirection) and
afterAccountTypeChange(String, AccountTypeChangeDirection); also update Javadoc
accordingly and make beforeAccountTypeChange either declare a checked exception
(e.g., AccountTypeChangeException) or return a boolean/Result type so
implementations can veto the change (ensure the implementing callers are adapted
to handle the exception/return).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1c396220-4a69-4ea5-9032-9addcdad3d6a
⛔ Files ignored due to path filters (1)
conf/serviceConfig/identity.xmlis excluded by!**/*.xml
📒 Files selected for processing (10)
header/src/main/java/org/zstack/header/identity/APIChangeAccountTypeEvent.javaheader/src/main/java/org/zstack/header/identity/APIChangeAccountTypeEventDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/identity/APIChangeAccountTypeMsg.javaheader/src/main/java/org/zstack/header/identity/APIChangeAccountTypeMsgDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/identity/AccountTypeChangedExtensionPoint.javaidentity/src/main/java/org/zstack/identity/AccountBase.javaidentity/src/main/java/org/zstack/identity/AccountManagerImpl.javasdk/src/main/java/org/zstack/sdk/ChangeAccountTypeAction.javasdk/src/main/java/org/zstack/sdk/ChangeAccountTypeResult.javatestlib/src/main/java/org/zstack/testlib/ApiHelper.groovy
🚧 Files skipped from review as they are similar to previous changes (2)
- header/src/main/java/org/zstack/header/identity/APIChangeAccountTypeMsgDoc_zh_cn.groovy
- header/src/main/java/org/zstack/header/identity/APIChangeAccountTypeEventDoc_zh_cn.groovy
Comment on lines
+37
to
+40
| @Override | ||
| public String getAccountUuid() { | ||
| return this.getSession().getAccountUuid(); | ||
| } |
There was a problem hiding this comment.
getAccountUuid() 返回值错误，可能导致账号归属与鉴权链路偏移。
Line 39 当前返回会话账号 UUID，而不是目标资源 uuid。这会把消息归属绑定到调用者，而非被操作账号。建议直接返回 uuid。
建议修改
`@Override`
public String getAccountUuid() {
- return this.getSession().getAccountUuid();
+ return uuid;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@header/src/main/java/org/zstack/header/identity/APIChangeAccountTypeMsg.java`
around lines 37 - 40, The getAccountUuid() implementation in
APIChangeAccountTypeMsg is returning the caller's session account UUID via
getSession().getAccountUuid(), which incorrectly attributes the message to the
invoking account; change getAccountUuid() to return the target resource's uuid
field (the message's uuid property) so the message is owned by the account being
modified (use the class's uuid getter/field instead of getSession()).
033da13 to
eec4036
Compare
| throw new ApiMessageInterceptionException(argerr( | ||
| "Account[uuid:%s] is not an admin account.", msg.getUuid() | ||
| )); | ||
| } |
Collaborator
There was a problem hiding this comment.
Comment from taiyue.chen:
已更新文档，标注该功能暂不支持。
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
header/src/main/java/org/zstack/header/identity/AccountTypeChangedExtensionPoint.java (1)
11-15: 接口方法缺少 Javadoc 注释。根据编码规范，接口方法必须配有有效的 Javadoc 注释。建议为每个方法添加说明，明确其用途和参数含义。
另外，
afterAccountTypeChange方法缺少oldType参数，而beforeAccountTypeChange包含该参数。如果扩展实现需要在变更完成后根据旧类型执行某些逻辑（如审计日志），当前签名可能不足。📝 建议添加 Javadoc 并考虑补充 oldType 参数
public interface AccountTypeChangedExtensionPoint { + /** + * Pre-validation hook before account type change. + * `@param` accountUuid the target account UUID + * `@param` oldType current account type + * `@param` newType target account type + * `@return` ErrorCode if change should be blocked, null otherwise + */ ErrorCode preAccountTypeChange(String accountUuid, AccountType oldType, AccountType newType); + /** + * Called before account type is persisted. Extensions can perform cleanup here. + * `@param` accountUuid the target account UUID + * `@param` oldType current account type + * `@param` newType target account type + */ void beforeAccountTypeChange(String accountUuid, AccountType oldType, AccountType newType); + /** + * Called after account type change is committed. + * `@param` accountUuid the target account UUID + * `@param` oldType previous account type + * `@param` newType new account type + */ - void afterAccountTypeChange(String accountUuid, AccountType newType); + void afterAccountTypeChange(String accountUuid, AccountType oldType, AccountType newType); }根据编码规范: "接口方法不应有多余的修饰符（例如
public），且必须配有有效的 Javadoc 注释。"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/identity/AccountTypeChangedExtensionPoint.java` around lines 11 - 15, Add Javadoc comments for all three interface methods (preAccountTypeChange, beforeAccountTypeChange, afterAccountTypeChange) describing their purpose and each parameter; also change the signature of afterAccountTypeChange to include the oldType parameter (i.e., afterAccountTypeChange(String accountUuid, AccountType oldType, AccountType newType)) so implementations can access both old and new types, and update any implementing classes accordingly to match the new signature.sdk/src/main/java/org/zstack/sdk/ChangeAccountTypeResult.java (1)
3-3: 冗余的同包导入
AccountInventory与ChangeAccountTypeResult位于同一个包org.zstack.sdk中，无需显式导入。♻️ 建议移除冗余导入
package org.zstack.sdk; -import org.zstack.sdk.AccountInventory; - public class ChangeAccountTypeResult {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/main/java/org/zstack/sdk/ChangeAccountTypeResult.java` at line 3, Remove the redundant import of AccountInventory in ChangeAccountTypeResult: since both AccountInventory and the ChangeAccountTypeResult class are in the same package org.zstack.sdk, delete the line "import org.zstack.sdk.AccountInventory;" from the ChangeAccountTypeResult source so the class relies on package-level visibility and avoids an unnecessary import statement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@header/src/main/java/org/zstack/header/identity/APIChangeAccountTypeEventDoc_zh_cn.groovy`:
- Around line 13-15: The `request` block in
APIChangeAccountTypeEventDoc_zh_cn.groovy currently has desc set to
"变更账户类型的返回结果", which mismatches its purpose; update the `request` block's `desc`
(inside the APIChangeAccountTypeEventDoc_zh_cn definition) to accurately
describe the request, e.g. "变更账户类型的请求", by editing the `request { desc """..."""
}` entry accordingly.
In `@identity/src/main/java/org/zstack/identity/AccountManagerImpl.java`:
- Around line 1009-1013: The error message is misleading: the code checks
AccountType.SystemAdmin.toString().equals(msg.getType()) but throws "Account...
is not an admin account" which implies the account isn't currently an admin;
instead update the ApiMessageInterceptionException thrown in this if-block (the
argerr call) to clearly state the validation failure (e.g., that the operation
requires a SystemAdmin type or that demotion of a SystemAdmin is not supported)
so callers see the real reason; modify the message used in the argerr invocation
that references msg.getUuid() accordingly.
---
Nitpick comments:
In
`@header/src/main/java/org/zstack/header/identity/AccountTypeChangedExtensionPoint.java`:
- Around line 11-15: Add Javadoc comments for all three interface methods
(preAccountTypeChange, beforeAccountTypeChange, afterAccountTypeChange)
describing their purpose and each parameter; also change the signature of
afterAccountTypeChange to include the oldType parameter (i.e.,
afterAccountTypeChange(String accountUuid, AccountType oldType, AccountType
newType)) so implementations can access both old and new types, and update any
implementing classes accordingly to match the new signature.
In `@sdk/src/main/java/org/zstack/sdk/ChangeAccountTypeResult.java`:
- Line 3: Remove the redundant import of AccountInventory in
ChangeAccountTypeResult: since both AccountInventory and the
ChangeAccountTypeResult class are in the same package org.zstack.sdk, delete the
line "import org.zstack.sdk.AccountInventory;" from the ChangeAccountTypeResult
source so the class relies on package-level visibility and avoids an unnecessary
import statement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4451a129-b4f1-4ba1-8705-7e745ce686d3
⛔ Files ignored due to path filters (1)
conf/serviceConfig/identity.xmlis excluded by!**/*.xml
📒 Files selected for processing (10)
header/src/main/java/org/zstack/header/identity/APIChangeAccountTypeEvent.javaheader/src/main/java/org/zstack/header/identity/APIChangeAccountTypeEventDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/identity/APIChangeAccountTypeMsg.javaheader/src/main/java/org/zstack/header/identity/APIChangeAccountTypeMsgDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/identity/AccountTypeChangedExtensionPoint.javaidentity/src/main/java/org/zstack/identity/AccountBase.javaidentity/src/main/java/org/zstack/identity/AccountManagerImpl.javasdk/src/main/java/org/zstack/sdk/ChangeAccountTypeAction.javasdk/src/main/java/org/zstack/sdk/ChangeAccountTypeResult.javatestlib/src/main/java/org/zstack/testlib/ApiHelper.groovy
🚧 Files skipped from review as they are similar to previous changes (3)
- sdk/src/main/java/org/zstack/sdk/ChangeAccountTypeAction.java
- header/src/main/java/org/zstack/header/identity/APIChangeAccountTypeMsg.java
- identity/src/main/java/org/zstack/identity/AccountBase.java
Comment on lines
+13
to
+15
| request { | ||
| desc """变更账户类型的返回结果""" | ||
| } |
There was a problem hiding this comment.
request 描述与实际语义不符
当前 request 块的 desc 为 "变更账户类型的返回结果"，但 request 部分通常用于描述触发该事件的请求消息，而非返回结果。建议修正描述以准确反映请求的含义，例如 "变更账户类型的请求"。
📝 建议修正描述
rest {
request {
- desc """变更账户类型的返回结果"""
+ desc """变更账户类型的请求"""
}📝 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
| request { | |
| desc """变更账户类型的返回结果""" | |
| } | |
| request { | |
| desc """变更账户类型的请求""" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@header/src/main/java/org/zstack/header/identity/APIChangeAccountTypeEventDoc_zh_cn.groovy`
around lines 13 - 15, The `request` block in
APIChangeAccountTypeEventDoc_zh_cn.groovy currently has desc set to
"变更账户类型的返回结果", which mismatches its purpose; update the `request` block's `desc`
(inside the APIChangeAccountTypeEventDoc_zh_cn definition) to accurately
describe the request, e.g. "变更账户类型的请求", by editing the `request { desc """..."""
}` entry accordingly.
eec4036 to
7c4d5d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
header/src/main/java/org/zstack/header/identity/APIChangeAccountTypeMsg.java (1)
30-33:⚠️ Potential issue | 🔴 Critical
getAccountUuid()绑定了调用者账号，应改为目标账号 UUID。Line 32 当前返回 session 账号，会把这条消息归属到调用者而不是被操作账号
uuid，存在鉴权与资源归属偏移风险。建议修复
`@Override` public String getAccountUuid() { - return this.getSession().getAccountUuid(); + return this.uuid; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/identity/APIChangeAccountTypeMsg.java` around lines 30 - 33, The getAccountUuid() method in APIChangeAccountTypeMsg currently returns the caller's session account UUID (this.getSession().getAccountUuid()), which wrongly binds the message to the caller; change it to return the target account's UUID field instead (e.g., return this.accountUuid or the existing getter for the message's account UUID) so the message is attributed to the account being operated on; update APIChangeAccountTypeMsg.getAccountUuid() to reference the message's accountUuid property rather than the session.
🧹 Nitpick comments (2)
header/src/main/java/org/zstack/header/identity/AccountTypeChangedExtensionPoint.java (1)
11-15: 建议为接口方法添加 Javadoc 注释根据编码规范，接口方法需要配有有效的 Javadoc 注释。当前三个扩展点方法缺少独立的文档说明，建议补充每个方法的调用时机和预期行为：
preAccountTypeChange: 用于前置校验，返回 ErrorCode 时阻止变更beforeAccountTypeChange: 在持久化之前执行清理逻辑afterAccountTypeChange: 在持久化之后执行后置处理📝 建议添加方法级 Javadoc
public interface AccountTypeChangedExtensionPoint { + /** + * Pre-check before account type change. Return non-null ErrorCode to reject the change. + */ ErrorCode preAccountTypeChange(String accountUuid, AccountType oldType, AccountType newType); + /** + * Called before the account type change is persisted. + * Implementations can perform cleanup logic such as detaching roles and shared resources. + */ void beforeAccountTypeChange(String accountUuid, AccountType oldType, AccountType newType); + /** + * Called after the account type change has been persisted. + */ void afterAccountTypeChange(String accountUuid, AccountType newType); }As per coding guidelines: "接口方法不应有多余的修饰符（例如
public），且必须配有有效的 Javadoc 注释"。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/identity/AccountTypeChangedExtensionPoint.java` around lines 11 - 15, 三个扩展点方法缺少 Javadoc，请为 preAccountTypeChange、beforeAccountTypeChange 和 afterAccountTypeChange 分别添加方法级 Javadoc，说明每个方法的调用时机和预期行为（preAccountTypeChange：用于前置校验，若返回 ErrorCode 则阻止变更；beforeAccountTypeChange：在持久化之前执行清理逻辑；afterAccountTypeChange：在持久化之后执行后置处理），并确保注释遵循项目规范（无多余修饰符、中文或英文描述清晰、简要说明参数和返回值语义）。header/src/main/java/org/zstack/header/identity/APIChangeAccountTypeEventDoc_zh_cn.groovy (1)
8-13:success字段缺少描述
success字段的desc为空字符串，建议添加描述以提高文档完整性。📝 建议补充描述
field { name "success" - desc "" + desc "成功状态" type "boolean" since "5.0.0" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/identity/APIChangeAccountTypeEventDoc_zh_cn.groovy` around lines 8 - 13, APIChangeAccountTypeEventDoc_zh_cn.groovy 中的 field "success" 的 desc 为空，需补充描述以完善文档；请在 APIChangeAccountTypeEventDoc_zh_cn 类中找到名为 "success" 的 field 块并将 desc 字段填成简短明了的中文说明，例如“操作是否成功，true 表示成功，false 表示失败”，确保描述准确反映 boolean 含义并保留 since "5.0.0" 不变。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@header/src/main/java/org/zstack/header/identity/APIChangeAccountTypeEventDoc_zh_cn.groovy`:
- Around line 5-23: The doc block for APIChangeAccountTypeEvent is missing the
inventory field documentation; add a new field entry named "inventory"
describing it as the AccountInventory returned on success (type
"AccountInventory", clz AccountInventory.class), include a brief desc in Chinese
(e.g., "变更后账号的详细信息，操作成功时返回此字段"), and place it alongside the existing "success"
and "error" entries in APIChangeAccountTypeEventDoc_zh_cn.groovy so the event
response is fully documented.
---
Duplicate comments:
In
`@header/src/main/java/org/zstack/header/identity/APIChangeAccountTypeMsg.java`:
- Around line 30-33: The getAccountUuid() method in APIChangeAccountTypeMsg
currently returns the caller's session account UUID
(this.getSession().getAccountUuid()), which wrongly binds the message to the
caller; change it to return the target account's UUID field instead (e.g.,
return this.accountUuid or the existing getter for the message's account UUID)
so the message is attributed to the account being operated on; update
APIChangeAccountTypeMsg.getAccountUuid() to reference the message's accountUuid
property rather than the session.
---
Nitpick comments:
In
`@header/src/main/java/org/zstack/header/identity/AccountTypeChangedExtensionPoint.java`:
- Around line 11-15: 三个扩展点方法缺少 Javadoc，请为
preAccountTypeChange、beforeAccountTypeChange 和 afterAccountTypeChange 分别添加方法级
Javadoc，说明每个方法的调用时机和预期行为（preAccountTypeChange：用于前置校验，若返回 ErrorCode
则阻止变更；beforeAccountTypeChange：在持久化之前执行清理逻辑；afterAccountTypeChange：在持久化之后执行后置处理），并确保注释遵循项目规范（无多余修饰符、中文或英文描述清晰、简要说明参数和返回值语义）。
In
`@header/src/main/java/org/zstack/header/identity/APIChangeAccountTypeEventDoc_zh_cn.groovy`:
- Around line 8-13: APIChangeAccountTypeEventDoc_zh_cn.groovy 中的 field "success"
的 desc 为空，需补充描述以完善文档；请在 APIChangeAccountTypeEventDoc_zh_cn 类中找到名为 "success" 的
field 块并将 desc 字段填成简短明了的中文说明，例如“操作是否成功，true 表示成功，false 表示失败”，确保描述准确反映 boolean
含义并保留 since "5.0.0" 不变。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 387a87b0-b4d2-421d-95a7-191942c9e169
⛔ Files ignored due to path filters (1)
conf/serviceConfig/identity.xmlis excluded by!**/*.xml
📒 Files selected for processing (10)
header/src/main/java/org/zstack/header/identity/APIChangeAccountTypeEvent.javaheader/src/main/java/org/zstack/header/identity/APIChangeAccountTypeEventDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/identity/APIChangeAccountTypeMsg.javaheader/src/main/java/org/zstack/header/identity/APIChangeAccountTypeMsgDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/identity/AccountTypeChangedExtensionPoint.javaidentity/src/main/java/org/zstack/identity/AccountBase.javaidentity/src/main/java/org/zstack/identity/AccountManagerImpl.javasdk/src/main/java/org/zstack/sdk/ChangeAccountTypeAction.javasdk/src/main/java/org/zstack/sdk/ChangeAccountTypeResult.javatestlib/src/main/java/org/zstack/testlib/ApiHelper.groovy
🚧 Files skipped from review as they are similar to previous changes (3)
- identity/src/main/java/org/zstack/identity/AccountManagerImpl.java
- header/src/main/java/org/zstack/header/identity/APIChangeAccountTypeMsgDoc_zh_cn.groovy
- header/src/main/java/org/zstack/header/identity/APIChangeAccountTypeEvent.java
Comment on lines
+5
to
+23
| doc { | ||
| title "ChangeAccountType" | ||
|
|
||
| field { | ||
| name "success" | ||
| desc "" | ||
| type "boolean" | ||
| since "5.0.0" | ||
| } | ||
|
|
||
| ref { | ||
| name "error" | ||
| path "org.zstack.header.identity.APIChangeAccountTypeEvent.error" | ||
| desc "错误码，若不为null，则表示操作失败, 操作成功时该字段为null", false | ||
| type "ErrorCode" | ||
| since "5.0.0" | ||
| clz ErrorCode.class | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 查找 APIChangeAccountTypeEvent 类定义，确认是否包含 inventory 字段
ast-grep --pattern $'class APIChangeAccountTypeEvent extends $_ {
$$$
}'Repository: MatheMatrix/zstack
Length of output: 44
🏁 Script executed:
fd -type f -name "*APIChangeAccountTypeEvent.java" -o -name "*APIChangeAccountTypeEvent.groovy"Repository: MatheMatrix/zstack
Length of output: 232
🏁 Script executed:
fd -t f "APIChangeAccountTypeEvent" --exclude="*.class"Repository: MatheMatrix/zstack
Length of output: 230
🏁 Script executed:
cat -n header/src/main/java/org/zstack/header/identity/APIChangeAccountTypeEvent.javaRepository: MatheMatrix/zstack
Length of output: 1310
补充 inventory 字段的文档
APIChangeAccountTypeEvent 类中确实包含 inventory 字段（类型为 AccountInventory）。建议在文档中添加 inventory 字段的描述，以完整记录该事件的响应内容。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@header/src/main/java/org/zstack/header/identity/APIChangeAccountTypeEventDoc_zh_cn.groovy`
around lines 5 - 23, The doc block for APIChangeAccountTypeEvent is missing the
inventory field documentation; add a new field entry named "inventory"
describing it as the AccountInventory returned on success (type
"AccountInventory", clz AccountInventory.class), include a brief desc in Chinese
(e.g., "变更后账号的详细信息，操作成功时返回此字段"), and place it alongside the existing "success"
and "error" entries in APIChangeAccountTypeEventDoc_zh_cn.groovy so the event
response is fully documented.
7c4d5d4 to
196cf0c
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
header/src/main/java/org/zstack/header/identity/AccountTypeChangedExtensionPoint.java (1)
11-15: 建议为接口方法添加 Javadoc 注释作为扩展点接口，插件实现者需要清楚理解每个钩子方法的调用时机和语义差异：
preAccountTypeChange与beforeAccountTypeChange的区别是什么？（前者返回ErrorCode用于校验拦截，后者是无返回值的前置处理？）preAccountTypeChange返回null表示通过，返回ErrorCode表示拒绝变更？afterAccountTypeChange为何不需要oldType参数？根据编码规范，接口方法必须配有有效的 Javadoc 注释。
📝 建议添加方法级文档
public interface AccountTypeChangedExtensionPoint { + /** + * Pre-validation hook called before account type change. + * Return an ErrorCode to reject the change, or null to allow it. + * + * `@param` accountUuid the account UUID + * `@param` oldType current account type + * `@param` newType target account type + * `@return` ErrorCode if the change should be rejected, null otherwise + */ ErrorCode preAccountTypeChange(String accountUuid, AccountType oldType, AccountType newType); + /** + * Hook called before the account type change is persisted. + * Use this for preparatory operations that must happen before the change. + * + * `@param` accountUuid the account UUID + * `@param` oldType current account type + * `@param` newType target account type + */ void beforeAccountTypeChange(String accountUuid, AccountType oldType, AccountType newType); + /** + * Hook called after the account type change is persisted. + * Use this for cleanup or follow-up operations. + * + * `@param` accountUuid the account UUID + * `@param` newType the new account type + */ void afterAccountTypeChange(String accountUuid, AccountType newType); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/identity/AccountTypeChangedExtensionPoint.java` around lines 11 - 15, 为 AccountTypeChangedExtensionPoint 接口中三方法补充 Javadoc：在 preAccountTypeChange(String accountUuid, AccountType oldType, AccountType newType) 上说明该方法在变更前被调用并用于校验拦截，返回 null 表示通过、返回非 null 的 ErrorCode 表示拒绝并阻止变更；在 beforeAccountTypeChange(String accountUuid, AccountType oldType, AccountType newType) 上说明该方法在校验通过后、实际变更前调用用于做前置处理且不应抛出异常或阻断流程；在 afterAccountTypeChange(String accountUuid, AccountType newType) 上说明该方法在变更完成后调用、仅提供 newType 的原因（旧类型已不可变或不再必需），并注明线程/事务语义（若有）及实现者应注意的副作用。请在接口 AccountTypeChangedExtensionPoint 的三个方法上分别添加上述简洁明确的 Javadoc。sdk/src/main/java/org/zstack/sdk/ChangeAccountTypeResult.java (1)
3-3: 冗余的同包导入。
AccountInventory与ChangeAccountTypeResult同属org.zstack.sdk包，无需显式导入。建议修改
package org.zstack.sdk; -import org.zstack.sdk.AccountInventory; - public class ChangeAccountTypeResult {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/main/java/org/zstack/sdk/ChangeAccountTypeResult.java` at line 3, The import of AccountInventory is redundant because AccountInventory and ChangeAccountTypeResult are in the same package (org.zstack.sdk); remove the line importing org.zstack.sdk.AccountInventory from ChangeAccountTypeResult so the class relies on the package-level visibility instead of an unnecessary self-package import.sdk/src/main/java/org/zstack/sdk/ChangeAccountTypeAction.java (1)
3-5: 冗余的通配符导入。
org.zstack.sdk.*通配符导入冗余，建议移除或替换为具体类导入。建议修改
import java.util.HashMap; import java.util.Map; -import org.zstack.sdk.*;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/main/java/org/zstack/sdk/ChangeAccountTypeAction.java` around lines 3 - 5, The file ChangeAccountTypeAction.java contains a redundant wildcard import "org.zstack.sdk.*"; remove that wildcard and replace it with explicit imports for the specific classes used by ChangeAccountTypeAction (e.g., Result, ErrorCode, and any other types referenced within the ChangeAccountTypeAction class) so the imports are explicit and the wildcard is eliminated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@header/src/main/java/org/zstack/header/identity/AccountTypeChangedExtensionPoint.java`:
- Around line 11-15: 为 AccountTypeChangedExtensionPoint 接口中三方法补充 Javadoc：在
preAccountTypeChange(String accountUuid, AccountType oldType, AccountType
newType) 上说明该方法在变更前被调用并用于校验拦截，返回 null 表示通过、返回非 null 的 ErrorCode 表示拒绝并阻止变更；在
beforeAccountTypeChange(String accountUuid, AccountType oldType, AccountType
newType) 上说明该方法在校验通过后、实际变更前调用用于做前置处理且不应抛出异常或阻断流程；在 afterAccountTypeChange(String
accountUuid, AccountType newType) 上说明该方法在变更完成后调用、仅提供 newType
的原因（旧类型已不可变或不再必需），并注明线程/事务语义（若有）及实现者应注意的副作用。请在接口
AccountTypeChangedExtensionPoint 的三个方法上分别添加上述简洁明确的 Javadoc。
In `@sdk/src/main/java/org/zstack/sdk/ChangeAccountTypeAction.java`:
- Around line 3-5: The file ChangeAccountTypeAction.java contains a redundant
wildcard import "org.zstack.sdk.*"; remove that wildcard and replace it with
explicit imports for the specific classes used by ChangeAccountTypeAction (e.g.,
Result, ErrorCode, and any other types referenced within the
ChangeAccountTypeAction class) so the imports are explicit and the wildcard is
eliminated.
In `@sdk/src/main/java/org/zstack/sdk/ChangeAccountTypeResult.java`:
- Line 3: The import of AccountInventory is redundant because AccountInventory
and ChangeAccountTypeResult are in the same package (org.zstack.sdk); remove the
line importing org.zstack.sdk.AccountInventory from ChangeAccountTypeResult so
the class relies on the package-level visibility instead of an unnecessary
self-package import.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3eb451f8-c0ac-4fc0-a843-c6eb9339ebb2
⛔ Files ignored due to path filters (1)
conf/serviceConfig/identity.xmlis excluded by!**/*.xml
📒 Files selected for processing (10)
header/src/main/java/org/zstack/header/identity/APIChangeAccountTypeEvent.javaheader/src/main/java/org/zstack/header/identity/APIChangeAccountTypeEventDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/identity/APIChangeAccountTypeMsg.javaheader/src/main/java/org/zstack/header/identity/APIChangeAccountTypeMsgDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/identity/AccountTypeChangedExtensionPoint.javaidentity/src/main/java/org/zstack/identity/AccountBase.javaidentity/src/main/java/org/zstack/identity/AccountManagerImpl.javasdk/src/main/java/org/zstack/sdk/ChangeAccountTypeAction.javasdk/src/main/java/org/zstack/sdk/ChangeAccountTypeResult.javatestlib/src/main/java/org/zstack/testlib/ApiHelper.groovy
✅ Files skipped from review due to trivial changes (1)
- identity/src/main/java/org/zstack/identity/AccountManagerImpl.java
🚧 Files skipped from review as they are similar to previous changes (2)
- header/src/main/java/org/zstack/header/identity/APIChangeAccountTypeEventDoc_zh_cn.groovy
- identity/src/main/java/org/zstack/identity/AccountBase.java
MatheMatrix
force-pushed
the
sync/zstackio/ZSPHERE-175@@2
branch
5 times, most recently
from
20542df to
153d2dd
Compare
Collaborator
|
Comment from yaohua.wu: Review: MR !9378 — ZSV-11520MR: Upstream Freshness
🔴 Critical1. [APIChangeAccountTypeMsg.java] 缺少 同模块的 影响：
建议添加： @Action(category = AccountConstant.ACTION_CATEGORY, adminOnly = true)🟡 Warning1. [AccountTypeChangedExtensionPoint.java:14] void afterAccountTypeChange(String accountUuid, AccountType newType);
建议统一签名： void afterAccountTypeChange(String accountUuid, AccountType oldType, AccountType newType);2. [AccountBase.java:329-330] CollectionUtils.forEach(extensions, ext -> ext.afterAccountTypeChange(accountUuid, targetAccountType));
对比同文件的 CollectionUtils.safeForEach(extensions, ext -> ext.afterAccountTypeChange(accountUuid, targetAccountType));
3. [APIChangeAccountTypeMsg.java:18] @APIParam
private String type;当前 建议添加 @APIParam(validValues = {"SystemAdmin"})
private String type;后续支持更多类型时同步更新即可。 4. [AccountBase.java:316] Handler 中重复查询账户 — 且未使用
这意味着 admin 的 AccountBase 实例在操作另一个账户的数据。虽然功能正确（handler 是无状态的），但与 建议确认这是有意设计。如果需要与其他 API 保持一致的路由模式， 🟢 Suggestion1. [APIChangeAccountTypeEventDoc_zh_cn.groovy] 缺少 Event 类声明了 2. [AccountManagerImpl.java:983-1015]
3. 扩展点实现不在本 diff 中 MR 描述提到 "Auto-detach roles, user groups and shared resources on type change" 和 "Implement user group detachment logic in IAM1 module"，但本 diff 中无任何 Verdict: REVISION_REQUIRED核心阻塞项：缺少 🤖 Robot Reviewer |
- Add APIChangeAccountTypeMsg/Event classes - Support promoting normal user to admin user - Auto-detach roles, user groups and shared resources on type change - Add AccountTypeChangedExtensionPoint for extension mechanism - Implement user group detachment logic in IAM1 module APIImpact Resolves: ZSV-11520 Change-Id: I706a796d79756570746e646f7866777877677764
MatheMatrix
force-pushed
the
sync/zstackio/ZSPHERE-175@@2
branch
from
153d2dd to
a630cfc
Compare
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.
APIImpact
Resolves: ZSPHERE-175
Change-Id: I706a796d79756570746e646f7866777877677764
sync from gitlab !9378