<feature>[core]: Add external service configuration APIs#4200
<feature>[core]: Add external service configuration APIs#4200zstack-robot-2 wants to merge 1 commit into
Conversation
|
Warning Review limit reached
More reviews will be available in 43 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (41)
Walkthrough该 PR 在 ZStack 中引入外部服务配置管理:新增数据库模型与 JPA/Inventory 映射、REST API 消息/事件、跨管理节点的下发与回滚逻辑、SDK 客户端支持及测试辅助方法。 ChangesExternal Service Configuration Management
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (9)
header/src/main/java/org/zstack/header/core/external/service/APIUpdateExternalServiceConfigurationMsg.java (1)
43-46: 💤 Low value可简化审计方法实现
Line 45 中将
msg参数强制转换为APIUpdateExternalServiceConfigurationMsg是不必要的。由于audit()方法在当前类中定义,可以直接使用this.getUuid()或在方法签名处明确参数类型,提高代码可读性。可选的重构方案
`@Override` public Result audit(APIMessage msg, APIEvent rsp) { - return new APIAuditor.Result(((APIUpdateExternalServiceConfigurationMsg)msg).getUuid(), ExternalServiceConfigurationVO.class); + return new APIAuditor.Result(this.uuid, ExternalServiceConfigurationVO.class); }注:此重构假设
audit()被调用时this指向正确的消息实例。如果框架要求必须从msg参数中提取,则当前实现是正确的。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@header/src/main/java/org/zstack/header/core/external/service/APIUpdateExternalServiceConfigurationMsg.java` around lines 43 - 46, Replace the unnecessary cast inside APIUpdateExternalServiceConfigurationMsg.audit(): instead of casting the msg parameter to APIUpdateExternalServiceConfigurationMsg, directly use this.getUuid() when constructing the APIAuditor.Result (or change the audit method signature to accept APIUpdateExternalServiceConfigurationMsg if framework allows) so the code in audit() becomes new APIAuditor.Result(this.getUuid(), ExternalServiceConfigurationVO.class) and removes the redundant ((APIUpdateExternalServiceConfigurationMsg) msg) cast.header/src/main/java/org/zstack/header/core/external/service/APIDeleteExternalServiceConfigurationEvent.java (1)
28-32: ⚡ Quick win建议在 example() 中填充 applyResults 字段以完善 API 文档。
当前示例方法仅设置了
success=true,但未填充applyResults字段。为了生成更完整的 API 文档,建议添加示例数据展示该字段的结构。♻️ 建议的改进
public static APIDeleteExternalServiceConfigurationEvent __example__() { APIDeleteExternalServiceConfigurationEvent event = new APIDeleteExternalServiceConfigurationEvent(); event.setSuccess(true); + + ApplyExternalConfigurationResult result = new ApplyExternalConfigurationResult(); + result.setManagementNodeUuid(uuid()); + result.setSuccess(true); + event.setApplyResults(Collections.singletonList(result)); + return event; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@header/src/main/java/org/zstack/header/core/external/service/APIDeleteExternalServiceConfigurationEvent.java` around lines 28 - 32, The __example__() in APIDeleteExternalServiceConfigurationEvent currently only sets success=true; update the APIDeleteExternalServiceConfigurationEvent.__example__ method to also populate the applyResults field with a representative example (e.g., a list/array containing one or more ApplyResult-like objects or maps showing status, resourceId, message) so the generated API docs show the structure; locate APIDeleteExternalServiceConfigurationEvent.__example__ and add a sample applyResults value consistent with the ApplyResult type used elsewhere in the codebase.sdk/src/main/java/org/zstack/sdk/external/service/ExternalServiceConfigurationInventory.java (1)
55-61: ⚡ Quick win建议为 List 字段添加泛型类型参数
字段
applyResults声明为原始类型java.util.List,缺少泛型类型参数。根据领域模型,应指定为List<ApplyExternalConfigurationResult>,以提升类型安全性。♻️ 建议的修复
- public java.util.List applyResults; - public void setApplyResults(java.util.List applyResults) { + public java.util.List<ApplyExternalConfigurationResult> applyResults; + public void setApplyResults(java.util.List<ApplyExternalConfigurationResult> applyResults) { this.applyResults = applyResults; } - public java.util.List getApplyResults() { + public java.util.List<ApplyExternalConfigurationResult> getApplyResults() { return this.applyResults; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/src/main/java/org/zstack/sdk/external/service/ExternalServiceConfigurationInventory.java` around lines 55 - 61, 将 ExternalServiceConfigurationInventory 中的原始类型字段 applyResults 及其访问器改为使用泛型 List<ApplyExternalConfigurationResult>:把字段声明、setApplyResults 和 getApplyResults 的参数/返回类型从 java.util.List 改为 java.util.List<ApplyExternalConfigurationResult>,并在必要时导入或使用全限定名 ApplyExternalConfigurationResult,以恢复类型安全并更新签名一致性。sdk/src/main/java/org/zstack/sdk/external/service/QueryExternalServiceConfigurationResult.java (1)
6-12: ⚡ Quick win建议为 List 字段添加泛型类型参数
字段
inventories声明为原始类型java.util.List,缺少泛型类型参数,降低了类型安全性。建议指定为List<ExternalServiceConfigurationInventory>,以便在编译期捕获类型错误。♻️ 建议的修复
- public java.util.List inventories; - public void setInventories(java.util.List inventories) { + public java.util.List<ExternalServiceConfigurationInventory> inventories; + public void setInventories(java.util.List<ExternalServiceConfigurationInventory> inventories) { this.inventories = inventories; } - public java.util.List getInventories() { + public java.util.List<ExternalServiceConfigurationInventory> getInventories() { return this.inventories; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/src/main/java/org/zstack/sdk/external/service/QueryExternalServiceConfigurationResult.java` around lines 6 - 12, 将原始类型的字段与访问器改为使用泛型:把 QueryExternalServiceConfigurationResult 类中的字段 inventories 及其 getter/setter (inventories, setInventories, getInventories) 的类型从 raw java.util.List 改为 java.util.List<ExternalServiceConfigurationInventory>,并在类顶部引入或使用该类型的完整限定名;同时更新 setInventories 参数类型和 getInventories 返回类型以匹配泛型签名,确保编译通过并恢复类型安全。sdk/src/main/java/org/zstack/sdk/external/service/GetExternalServicesResult.java (1)
6-12: ⚡ Quick win建议为 List 字段添加泛型类型参数
字段
inventories声明为原始类型java.util.List,缺少泛型类型参数。应指定为List<ExternalServiceInventory>以提升类型安全性。♻️ 建议的修复
- public java.util.List inventories; - public void setInventories(java.util.List inventories) { + public java.util.List<ExternalServiceInventory> inventories; + public void setInventories(java.util.List<ExternalServiceInventory> inventories) { this.inventories = inventories; } - public java.util.List getInventories() { + public java.util.List<ExternalServiceInventory> getInventories() { return this.inventories; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/src/main/java/org/zstack/sdk/external/service/GetExternalServicesResult.java` around lines 6 - 12, 字段 inventories 在类 GetExternalServicesResult 中被声明为原始类型 java.util.List,缺乏泛型。将字段类型、getter 和 setter 的签名从 java.util.List 改为 java.util.List<ExternalServiceInventory>(并导入或使用全限定名 ExternalServiceInventory),同时更新 setInventories(List<ExternalServiceInventory> inventories) 和 getInventories() 返回 List<ExternalServiceInventory>,并修正所有引用该字段/方法的调用点以匹配泛型类型以保证类型安全。sdk/src/main/java/org/zstack/sdk/external/service/ApplyExternalConfigurationResult.java (3)
7-29: ⚡ Quick win建议将字段设为 private 以符合封装原则。
当前所有字段(
managementNodeUuid、errorCode、success)均声明为public,虽然已提供 getter/setter 方法,但这违反了 Java 的封装最佳实践。建议将字段改为private,仅通过 getter/setter 访问。注:如果 SDK 层框架对序列化有特殊要求(例如某些框架要求 public 字段),则可保持当前设计。
♻️ 建议的封装改进
- public java.lang.String managementNodeUuid; + private java.lang.String managementNodeUuid; public void setManagementNodeUuid(java.lang.String managementNodeUuid) { this.managementNodeUuid = managementNodeUuid; } public java.lang.String getManagementNodeUuid() { return this.managementNodeUuid; } - public ErrorCode errorCode; + private ErrorCode errorCode; public void setErrorCode(ErrorCode errorCode) { this.errorCode = errorCode; } public ErrorCode getErrorCode() { return this.errorCode; } - public boolean success; + private boolean success; public void setSuccess(boolean success) { this.success = success; } public boolean getSuccess() { return this.success; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/src/main/java/org/zstack/sdk/external/service/ApplyExternalConfigurationResult.java` around lines 7 - 29, Make the fields in ApplyExternalConfigurationResult private: change managementNodeUuid, errorCode, and success from public to private and keep their existing setManagementNodeUuid/getManagementNodeUuid, setErrorCode/getErrorCode, and setSuccess/getSuccess methods as the only accessors; if the SDK serialization layer requires public fields, add the appropriate annotations or explicit serializer support instead of leaving fields public.
5-5: ⚡ Quick win建议为 SDK 类添加 Javadoc 文档。
作为面向外部开发者的 SDK 类,
ApplyExternalConfigurationResult缺少类级别和成员级别的 Javadoc 注释。建议添加文档说明类的用途、各字段的含义以及使用场景,以提升 SDK 的可用性和可维护性。📝 建议的 Javadoc 示例
+/** + * 外部配置应用结果,用于封装在管理节点上应用外部服务配置的执行结果。 + */ public class ApplyExternalConfigurationResult { + /** + * 管理节点 UUID + */ public java.lang.String managementNodeUuid; + + /** + * 错误码,仅在应用失败时设置 + */ public ErrorCode errorCode; + + /** + * 应用是否成功 + */ public boolean success;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/src/main/java/org/zstack/sdk/external/service/ApplyExternalConfigurationResult.java` at line 5, Add Javadoc comments for the SDK class ApplyExternalConfigurationResult: add a class-level Javadoc describing the purpose and typical usage of ApplyExternalConfigurationResult, and add Javadoc for each member (fields and public getters/setters) describing what each field represents, its possible values and any nullability/constraints; ensure the comments follow standard Javadoc format (/** ... */) and include examples or `@see/`@since tags where helpful to guide external developers.
27-27: ⚡ Quick win建议 boolean 类型的 getter 使用
is前缀。根据 Java Bean 命名规范,boolean 类型字段的 getter 方法应使用
is前缀而非get前缀。建议将getSuccess()重命名为isSuccess(),以符合惯例并提升代码可读性。♻️ 建议的命名改进
- public boolean getSuccess() { + public boolean isSuccess() { return this.success; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/src/main/java/org/zstack/sdk/external/service/ApplyExternalConfigurationResult.java` at line 27, Rename the boolean getter getSuccess() in class ApplyExternalConfigurationResult to isSuccess() to follow Java Bean conventions: add a new method public boolean isSuccess() that returns the same field as getSuccess(), update any usages (calls to getSuccess()) to call isSuccess(), and optionally deprecate or remove getSuccess() after updating references; ensure the method signature and access remain identical aside from the name so serialization/beans tools recognize the property.core/src/main/java/org/zstack/core/externalservice/ExternalServiceManagerImpl.java (1)
257-291: 💤 Low value简化
syncKey的确定逻辑。Line 260 在
vo为null时回退到msg.getUuid()作为同步键,但随后在 Line 296-299 的更新逻辑中,如果vo为null会直接失败。因此,这个回退逻辑实际上不会产生有意义的行为,反而可能造成混淆。建议直接在 ChainTask 外部查询
vo并提前失败,或在确定同步键时假设vo一定存在(因为后续逻辑已依赖此前提)。可选的简化方案
private void handle(APIUpdateExternalServiceConfigurationMsg msg){ APIUpdateExternalServiceConfigurationEvent event = new APIUpdateExternalServiceConfigurationEvent(msg.getId()); ExternalServiceConfigurationVO vo = dbf.findByUuid(msg.getUuid(), ExternalServiceConfigurationVO.class); - final String syncKey = vo != null ? vo.getServiceType() : msg.getUuid(); + if (vo == null) { + event.setError(operr("unable to find external service configuration with uuid [%s]", msg.getUuid())); + bus.publish(event); + return; + } + final String syncKey = vo.getServiceType(); thdf.chainSubmit(new ChainTask(msg) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/main/java/org/zstack/core/externalservice/ExternalServiceManagerImpl.java` around lines 257 - 291, In handle(APIUpdateExternalServiceConfigurationMsg msg) simplify syncKey logic by resolving ExternalServiceConfigurationVO vo before creating the ChainTask and failing early if vo is null; compute syncKey = vo.getServiceType() (no fallback to msg.getUuid()), and only then submit the ChainTask that calls updateExternalServiceConfiguration(msg, event, ...). This removes the misleading fallback, aligns with updateExternalServiceConfiguration's expectations, and preserves the existing ChainTask/getSyncSignature and getName usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/src/main/java/org/zstack/core/externalservice/ExternalService.java`:
- Around line 20-22: 为 ExternalService 接口中的 getServiceType() 方法添加 Javadoc
注释,说明该方法用于返回服务类型标识(service type),并解释默认实现会调用 getName()
作为返回值以保持与服务名称一致;在注释中还可指出实现类可覆盖此方法以提供不同的类型值。请在 ExternalService 接口的
getServiceType() 方法上添加这段文档,引用方法名 getServiceType() 和 getName() 以便定位。
- Around line 24-26: Add a Javadoc comment to the ExternalService interface
method externalConfig(String serviceType) that documents the method's purpose
(hook for applying external configuration for the given service type), explains
the parameter serviceType (the identifier/name of the external configuration
scope), and clarifies the default no-op implementation semantics (services that
do not support external configuration may leave the method unimplemented).
Mention that implementers should override externalConfig to apply configuration
and that the default is intentionally empty to indicate unsupported external
configuration.
In
`@header/src/main/java/org/zstack/header/core/external/service/APIAddExternalServiceConfigurationMsg.java`:
- Line 16: Update the REST path strings for the external service configuration
APIs to use plural resource names: change the path value from
"/external/service/configuration" to "/external/service/configurations" in
APIAddExternalServiceConfigurationMsg and make the same change in
APIUpdateExternalServiceConfigurationMsg and any other related classes that
declare the same path constant/annotation; search for the exact string
"/external/service/configuration" and replace with
"/external/service/configurations" in the annotations or path variables so all
API endpoints are consistent with the plural naming convention.
- Around line 54-57: The audit method
APIAddExternalServiceConfigurationMsg.audit currently calls
evt.getInventory().getUuid() directly which can NPE if inventory is null; modify
the success branch to guard against null by checking rsp.isSuccess() &&
evt.getInventory() != null and use evt.getInventory().getUuid() only when
non-null, otherwise return an empty string (or a safe placeholder) in the Result
for ExternalServiceConfigurationVO; update references in
APIAddExternalServiceConfigurationMsg.audit and ensure the cast to
APIAddExternalServiceConfigurationEvent (evt) remains unchanged.
In
`@header/src/main/java/org/zstack/header/core/external/service/APIDeleteExternalServiceConfigurationEventDoc_zh_cn.groovy`:
- Around line 5-23: The API doc is missing the applyResults field from
APIDeleteExternalServiceConfigurationEvent; add a field block in
APIDeleteExternalServiceConfigurationEventDoc_zh_cn.groovy that documents the
applyResults property (name "applyResults"), describes it as the list of
per-configuration ApplyExternalConfigurationResult entries returned by the
operation, specifies the type as "List<ApplyExternalConfigurationResult>" (or
type "List" with a ref to ApplyExternalConfigurationResult), and include
since/version info and class reference to ApplyExternalConfigurationResult to
match how other fields (e.g., error) are documented.
In
`@header/src/main/java/org/zstack/header/core/external/service/APIDeleteExternalServiceConfigurationMsg.java`:
- Around line 15-19: Update the REST resource path strings to use plural
"configurations" instead of singular "configuration": change the `@RestRequest`
path in APIDeleteExternalServiceConfigurationMsg from
"/external/service/configuration/{uuid}" to
"/external/service/configurations/{uuid}" and make the equivalent pluralization
change in other related API message classes such as
APIQueryExternalServiceConfigurationMsg and any other classes referencing
"/external/service/configuration" so all request annotations and any consumers
use "/external/service/configurations".
In
`@header/src/main/java/org/zstack/header/core/external/service/APIUpdateExternalServiceConfigurationEventDoc_zh_cn.groovy`:
- Around line 18-23: The field block for name "success" in
APIUpdateExternalServiceConfigurationEventDoc_zh_cn.groovy has an empty desc;
update the desc value to a meaningful Chinese description (for example "操作是否成功")
to match the style used in
APIAddExternalServiceConfigurationEventDoc_zh_cn.groovy, ensuring the field {
name "success" desc "操作是否成功" type "boolean" since "5.1.0" } contains a non-empty
description.
- Around line 10-17: 将 ref 中的 inventory 描述从 desc "null" 替换为有意义的中文描述;找到
APIUpdateExternalServiceConfigurationEventDoc_zh_cn.groovy 中 ref 名为 "inventory"
的定义(path
"org.zstack.header.core.external.service.APIUpdateExternalServiceConfigurationEvent.inventory"、clz
ExternalServiceConfigurationInventory.class),将 desc 字段改为类似 "外部服务配置详情"
或其他准确中文描述以与 APIAddExternalServiceConfigurationEventDoc_zh_cn.groovy
的风格保持一致并提供可读文档。
In
`@header/src/main/java/org/zstack/header/core/external/service/APIUpdateExternalServiceConfigurationMsg.java`:
- Line 15: Update the REST path in APIUpdateExternalServiceConfigurationMsg to
use the plural resource name to match conventions and
APIAddExternalServiceConfigurationMsg: change the path value from
"/external/service/configuration/{uuid}" to
"/external/service/configurations/{uuid}" in the class-level annotation or field
that defines the path.
In
`@header/src/main/java/org/zstack/header/core/external/service/ExternalServiceConfigurationInventoryDoc_zh_cn.groovy`:
- Around line 22-26: The inventory currently exposes the "configuration" field
in plain text (see ExternalServiceConfigurationInventoryDoc_zh_cn.groovy) while
the persistence entity ExternalServiceConfigurationVO stores it as a plain
String; fix by implementing encryption-at-rest for the configuration field in
the persistence layer (add AES encryption/decryption hooks in
ExternalServiceConfigurationVO's DAO/persistence methods) and ensure
API/serialization masks or omits sensitive subfields when returning
ExternalServiceConfigurationInventory (replace password-like values with fixed
mask like "******" or strip them entirely in the serializer/mapper used to
produce the inventory); update create/update code paths to encrypt before save
and decrypt only for authorized internal use, and add unit tests to verify
stored data is encrypted and API responses never return raw credentials.
In
`@header/src/main/java/org/zstack/header/core/external/service/ExternalServiceConfigurationVO.java`:
- Around line 23-28: The configuration field in ExternalServiceConfigurationVO
currently has no column type/length and may be mapped to VARCHAR(255); update
the entity to store larger structured data by annotating the configuration field
with either `@Lob` or `@Column`(columnDefinition = "TEXT"/"MEDIUMTEXT") to avoid
truncation, and add sensible length constraints to serviceType and description
(e.g., `@Column`(length = 512)) so their columns are explicitly sized; modify the
annotations on the fields serviceType, configuration, and description in class
ExternalServiceConfigurationVO accordingly.
In
`@sdk/src/main/java/org/zstack/sdk/external/service/QueryExternalServiceConfigurationAction.java`:
- Around line 65-73: Update the REST path to use the plural form: change the
path string in QueryExternalServiceConfigurationAction.getRestInfo() from
"/external/service/configuration" to "/external/service/configurations"; also
update the `@RestRequest` annotation on APIQueryExternalServiceConfigurationMsg
(and any related message/handler classes) to use
"/external/service/configurations" and propagate the change to API
documentation/templates so all references remain consistent.
In `@testlib/src/main/java/org/zstack/testlib/ApiHelper.groovy`:
- Around line 15436-15537: The four methods addExternalServiceConfiguration,
queryExternalServiceConfiguration, updateExternalServiceConfiguration and
deleteExternalServiceConfiguration duplicate session setup, apipath tracking and
error handling; extract that shared logic into a private helper (e.g.,
executeExternalServiceAction(action, Closure postConfig = null)) which sets
action.sessionId = Test.currentEnvSpec?.session?.uuid, applies an optional
postConfig (used by query to transform a.conditions via a.conditions =
a.conditions.collect{it.toString()}), handles apipath/apiId via ApiPathTracker
and Test.apiPaths, calls errorOut(action.call()) and returns the result;
refactor each public method to construct the specific org.zstack.sdk.*Action,
delegate the closure, then call executeExternalServiceAction with any per-action
post-processing.
- Around line 15436-15537: The four helper methods
addExternalServiceConfiguration, queryExternalServiceConfiguration,
updateExternalServiceConfiguration, and deleteExternalServiceConfiguration
reference action classes under org.zstack.sdk.* which no longer exist; update
both the `@DelegatesTo` annotations and the instantiations (new ...Action()) to
use the correct package org.zstack.sdk.external.service (e.g.
org.zstack.sdk.external.service.AddExternalServiceConfigurationAction etc.), and
also add corresponding old→new mappings for these four
*ExternalServiceConfigurationAction classes in SourceClassMap.java so
compilation won't fail with ClassNotFoundException.
---
Nitpick comments:
In
`@core/src/main/java/org/zstack/core/externalservice/ExternalServiceManagerImpl.java`:
- Around line 257-291: In handle(APIUpdateExternalServiceConfigurationMsg msg)
simplify syncKey logic by resolving ExternalServiceConfigurationVO vo before
creating the ChainTask and failing early if vo is null; compute syncKey =
vo.getServiceType() (no fallback to msg.getUuid()), and only then submit the
ChainTask that calls updateExternalServiceConfiguration(msg, event, ...). This
removes the misleading fallback, aligns with
updateExternalServiceConfiguration's expectations, and preserves the existing
ChainTask/getSyncSignature and getName usage.
In
`@header/src/main/java/org/zstack/header/core/external/service/APIDeleteExternalServiceConfigurationEvent.java`:
- Around line 28-32: The __example__() in
APIDeleteExternalServiceConfigurationEvent currently only sets success=true;
update the APIDeleteExternalServiceConfigurationEvent.__example__ method to also
populate the applyResults field with a representative example (e.g., a
list/array containing one or more ApplyResult-like objects or maps showing
status, resourceId, message) so the generated API docs show the structure;
locate APIDeleteExternalServiceConfigurationEvent.__example__ and add a sample
applyResults value consistent with the ApplyResult type used elsewhere in the
codebase.
In
`@header/src/main/java/org/zstack/header/core/external/service/APIUpdateExternalServiceConfigurationMsg.java`:
- Around line 43-46: Replace the unnecessary cast inside
APIUpdateExternalServiceConfigurationMsg.audit(): instead of casting the msg
parameter to APIUpdateExternalServiceConfigurationMsg, directly use
this.getUuid() when constructing the APIAuditor.Result (or change the audit
method signature to accept APIUpdateExternalServiceConfigurationMsg if framework
allows) so the code in audit() becomes new APIAuditor.Result(this.getUuid(),
ExternalServiceConfigurationVO.class) and removes the redundant
((APIUpdateExternalServiceConfigurationMsg) msg) cast.
In
`@sdk/src/main/java/org/zstack/sdk/external/service/ApplyExternalConfigurationResult.java`:
- Around line 7-29: Make the fields in ApplyExternalConfigurationResult private:
change managementNodeUuid, errorCode, and success from public to private and
keep their existing setManagementNodeUuid/getManagementNodeUuid,
setErrorCode/getErrorCode, and setSuccess/getSuccess methods as the only
accessors; if the SDK serialization layer requires public fields, add the
appropriate annotations or explicit serializer support instead of leaving fields
public.
- Line 5: Add Javadoc comments for the SDK class
ApplyExternalConfigurationResult: add a class-level Javadoc describing the
purpose and typical usage of ApplyExternalConfigurationResult, and add Javadoc
for each member (fields and public getters/setters) describing what each field
represents, its possible values and any nullability/constraints; ensure the
comments follow standard Javadoc format (/** ... */) and include examples or
`@see/`@since tags where helpful to guide external developers.
- Line 27: Rename the boolean getter getSuccess() in class
ApplyExternalConfigurationResult to isSuccess() to follow Java Bean conventions:
add a new method public boolean isSuccess() that returns the same field as
getSuccess(), update any usages (calls to getSuccess()) to call isSuccess(), and
optionally deprecate or remove getSuccess() after updating references; ensure
the method signature and access remain identical aside from the name so
serialization/beans tools recognize the property.
In
`@sdk/src/main/java/org/zstack/sdk/external/service/ExternalServiceConfigurationInventory.java`:
- Around line 55-61: 将 ExternalServiceConfigurationInventory 中的原始类型字段
applyResults 及其访问器改为使用泛型
List<ApplyExternalConfigurationResult>:把字段声明、setApplyResults 和 getApplyResults
的参数/返回类型从 java.util.List 改为
java.util.List<ApplyExternalConfigurationResult>,并在必要时导入或使用全限定名
ApplyExternalConfigurationResult,以恢复类型安全并更新签名一致性。
In
`@sdk/src/main/java/org/zstack/sdk/external/service/GetExternalServicesResult.java`:
- Around line 6-12: 字段 inventories 在类 GetExternalServicesResult 中被声明为原始类型
java.util.List,缺乏泛型。将字段类型、getter 和 setter 的签名从 java.util.List 改为
java.util.List<ExternalServiceInventory>(并导入或使用全限定名
ExternalServiceInventory),同时更新 setInventories(List<ExternalServiceInventory>
inventories) 和 getInventories() 返回
List<ExternalServiceInventory>,并修正所有引用该字段/方法的调用点以匹配泛型类型以保证类型安全。
In
`@sdk/src/main/java/org/zstack/sdk/external/service/QueryExternalServiceConfigurationResult.java`:
- Around line 6-12: 将原始类型的字段与访问器改为使用泛型:把 QueryExternalServiceConfigurationResult
类中的字段 inventories 及其 getter/setter (inventories, setInventories, getInventories)
的类型从 raw java.util.List 改为
java.util.List<ExternalServiceConfigurationInventory>,并在类顶部引入或使用该类型的完整限定名;同时更新
setInventories 参数类型和 getInventories 返回类型以匹配泛型签名,确保编译通过并恢复类型安全。
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f1eb6f88-956a-4d11-8d55-b3bc94df06da
⛔ Files ignored due to path filters (2)
conf/persistence.xmlis excluded by!**/*.xmlconf/serviceConfig/externalService.xmlis excluded by!**/*.xml
📒 Files selected for processing (47)
conf/db/zsv/V5.1.0__schema.sqlcore/src/main/java/org/zstack/core/externalservice/ExternalService.javacore/src/main/java/org/zstack/core/externalservice/ExternalServiceManagerImpl.javaheader/src/main/java/org/zstack/header/core/external/service/APIAddExternalServiceConfigurationEvent.javaheader/src/main/java/org/zstack/header/core/external/service/APIAddExternalServiceConfigurationEventDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/core/external/service/APIAddExternalServiceConfigurationMsg.javaheader/src/main/java/org/zstack/header/core/external/service/APIAddExternalServiceConfigurationMsgDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/core/external/service/APIDeleteExternalServiceConfigurationEvent.javaheader/src/main/java/org/zstack/header/core/external/service/APIDeleteExternalServiceConfigurationEventDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/core/external/service/APIDeleteExternalServiceConfigurationMsg.javaheader/src/main/java/org/zstack/header/core/external/service/APIDeleteExternalServiceConfigurationMsgDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/core/external/service/APIQueryExternalServiceConfigurationMsg.javaheader/src/main/java/org/zstack/header/core/external/service/APIQueryExternalServiceConfigurationMsgDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/core/external/service/APIQueryExternalServiceConfigurationReply.javaheader/src/main/java/org/zstack/header/core/external/service/APIQueryExternalServiceConfigurationReplyDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/core/external/service/APIUpdateExternalServiceConfigurationEvent.javaheader/src/main/java/org/zstack/header/core/external/service/APIUpdateExternalServiceConfigurationEventDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/core/external/service/APIUpdateExternalServiceConfigurationMsg.javaheader/src/main/java/org/zstack/header/core/external/service/APIUpdateExternalServiceConfigurationMsgDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/core/external/service/ApplyExternalConfigurationResult.javaheader/src/main/java/org/zstack/header/core/external/service/ApplyExternalServiceConfigurationMsg.javaheader/src/main/java/org/zstack/header/core/external/service/ApplyExternalServiceConfigurationReply.javaheader/src/main/java/org/zstack/header/core/external/service/ExternalServiceConfigurationInventory.javaheader/src/main/java/org/zstack/header/core/external/service/ExternalServiceConfigurationInventoryDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/core/external/service/ExternalServiceConfigurationVO.javaheader/src/main/java/org/zstack/header/core/external/service/ExternalServiceConfigurationVO_.javaheader/src/main/java/org/zstack/header/core/external/service/ExternalServiceInventory.javaheader/src/main/java/org/zstack/header/core/external/service/PackageInfo.javasdk/src/main/java/org/zstack/sdk/ExternalServiceCapabilitiesBuilder.javasdk/src/main/java/org/zstack/sdk/SourceClassMap.javasdk/src/main/java/org/zstack/sdk/external/service/AddExternalServiceConfigurationAction.javasdk/src/main/java/org/zstack/sdk/external/service/AddExternalServiceConfigurationResult.javasdk/src/main/java/org/zstack/sdk/external/service/ApplyExternalConfigurationResult.javasdk/src/main/java/org/zstack/sdk/external/service/DeleteExternalServiceConfigurationAction.javasdk/src/main/java/org/zstack/sdk/external/service/DeleteExternalServiceConfigurationResult.javasdk/src/main/java/org/zstack/sdk/external/service/ExternalServiceCapabilities.javasdk/src/main/java/org/zstack/sdk/external/service/ExternalServiceConfigurationInventory.javasdk/src/main/java/org/zstack/sdk/external/service/ExternalServiceInventory.javasdk/src/main/java/org/zstack/sdk/external/service/GetExternalServicesAction.javasdk/src/main/java/org/zstack/sdk/external/service/GetExternalServicesResult.javasdk/src/main/java/org/zstack/sdk/external/service/QueryExternalServiceConfigurationAction.javasdk/src/main/java/org/zstack/sdk/external/service/QueryExternalServiceConfigurationResult.javasdk/src/main/java/org/zstack/sdk/external/service/ReloadExternalServiceAction.javasdk/src/main/java/org/zstack/sdk/external/service/ReloadExternalServiceResult.javasdk/src/main/java/org/zstack/sdk/external/service/UpdateExternalServiceConfigurationAction.javasdk/src/main/java/org/zstack/sdk/external/service/UpdateExternalServiceConfigurationResult.javatestlib/src/main/java/org/zstack/testlib/ApiHelper.groovy
| default String getServiceType() { | ||
| return getName(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
为接口方法添加 Javadoc 注释。
根据编码规范,接口方法必须配有有效的 Javadoc 注释。请为 getServiceType() 添加文档说明,解释其用途以及默认实现返回 getName() 的行为。
建议的文档格式
+/**
+ * 获取外部服务的类型标识。
+ * 默认实现返回服务名称,子类可根据需要覆盖此方法。
+ *
+ * `@return` 服务类型
+ */
default String getServiceType() {
return getName();
}📝 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.
| default String getServiceType() { | |
| return getName(); | |
| } | |
| /** | |
| * 获取外部服务的类型标识。 | |
| * 默认实现返回服务名称,子类可根据需要覆盖此方法。 | |
| * | |
| * `@return` 服务类型 | |
| */ | |
| default String getServiceType() { | |
| return getName(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/src/main/java/org/zstack/core/externalservice/ExternalService.java`
around lines 20 - 22, 为 ExternalService 接口中的 getServiceType() 方法添加 Javadoc
注释,说明该方法用于返回服务类型标识(service type),并解释默认实现会调用 getName()
作为返回值以保持与服务名称一致;在注释中还可指出实现类可覆盖此方法以提供不同的类型值。请在 ExternalService 接口的
getServiceType() 方法上添加这段文档,引用方法名 getServiceType() 和 getName() 以便定位。
Source: Coding guidelines
| default void externalConfig(String serviceType) { | ||
| // no-op by default | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
为接口方法添加 Javadoc 注释。
根据编码规范,接口方法必须配有有效的 Javadoc 注释。请为 externalConfig(String serviceType) 添加文档,说明其用途、参数含义以及默认空实现的语义(即不支持外部配置的服务类型可保持空实现)。
建议的文档格式
+/**
+ * 应用外部服务配置。
+ * 默认实现为空操作。不支持外部配置的服务类型可保持此默认行为。
+ *
+ * `@param` serviceType 服务类型
+ */
default void externalConfig(String serviceType) {
// no-op by default
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/src/main/java/org/zstack/core/externalservice/ExternalService.java`
around lines 24 - 26, Add a Javadoc comment to the ExternalService interface
method externalConfig(String serviceType) that documents the method's purpose
(hook for applying external configuration for the given service type), explains
the parameter serviceType (the identifier/name of the external configuration
scope), and clarifies the default no-op implementation semantics (services that
do not support external configuration may leave the method unimplemented).
Mention that implementers should override externalConfig to apply configuration
and that the default is intentionally empty to indicate unsupported external
configuration.
Sources: Coding guidelines, Learnings
| public Result audit(APIMessage msg, APIEvent rsp) { | ||
| APIAddExternalServiceConfigurationEvent evt = (APIAddExternalServiceConfigurationEvent) rsp; | ||
| return new Result(rsp.isSuccess() ? evt.getInventory().getUuid(): "", ExternalServiceConfigurationVO.class); | ||
| } |
There was a problem hiding this comment.
审计方法中存在潜在的空指针风险
Line 56 在成功时直接调用 evt.getInventory().getUuid(),如果 inventory 为 null 将导致 NullPointerException。虽然从上下文代码来看,inventory 应该在成功时已经设置,但添加防御性检查可以提高代码的健壮性。
建议的防御性修正
`@Override`
public Result audit(APIMessage msg, APIEvent rsp) {
APIAddExternalServiceConfigurationEvent evt = (APIAddExternalServiceConfigurationEvent) rsp;
- return new Result(rsp.isSuccess() ? evt.getInventory().getUuid(): "", ExternalServiceConfigurationVO.class);
+ String auditUuid = "";
+ if (rsp.isSuccess() && evt.getInventory() != null) {
+ auditUuid = evt.getInventory().getUuid();
+ }
+ return new Result(auditUuid, ExternalServiceConfigurationVO.class);
}📝 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.
| public Result audit(APIMessage msg, APIEvent rsp) { | |
| APIAddExternalServiceConfigurationEvent evt = (APIAddExternalServiceConfigurationEvent) rsp; | |
| return new Result(rsp.isSuccess() ? evt.getInventory().getUuid(): "", ExternalServiceConfigurationVO.class); | |
| } | |
| public Result audit(APIMessage msg, APIEvent rsp) { | |
| APIAddExternalServiceConfigurationEvent evt = (APIAddExternalServiceConfigurationEvent) rsp; | |
| String auditUuid = ""; | |
| if (rsp.isSuccess() && evt.getInventory() != null) { | |
| auditUuid = evt.getInventory().getUuid(); | |
| } | |
| return new Result(auditUuid, ExternalServiceConfigurationVO.class); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@header/src/main/java/org/zstack/header/core/external/service/APIAddExternalServiceConfigurationMsg.java`
around lines 54 - 57, The audit method
APIAddExternalServiceConfigurationMsg.audit currently calls
evt.getInventory().getUuid() directly which can NPE if inventory is null; modify
the success branch to guard against null by checking rsp.isSuccess() &&
evt.getInventory() != null and use evt.getInventory().getUuid() only when
non-null, otherwise return an empty string (or a safe placeholder) in the Result
for ExternalServiceConfigurationVO; update references in
APIAddExternalServiceConfigurationMsg.audit and ensure the cast to
APIAddExternalServiceConfigurationEvent (evt) remains unchanged.
| doc { | ||
|
|
||
| title "删除外部服务配置返回" | ||
|
|
||
| field { | ||
| name "success" | ||
| desc "操作是否成功" | ||
| type "boolean" | ||
| since "5.1.0" | ||
| } | ||
| ref { | ||
| name "error" | ||
| path "org.zstack.header.core.external.service.APIDeleteExternalServiceConfigurationEvent.error" | ||
| desc "错误码,若不为null,则表示操作失败, 操作成功时该字段为null",false | ||
| type "ErrorCode" | ||
| since "5.1.0" | ||
| clz ErrorCode.class | ||
| } | ||
| } |
There was a problem hiding this comment.
缺少 applyResults 字段的文档定义。
APIDeleteExternalServiceConfigurationEvent 类中包含 applyResults 字段(类型为 List<ApplyExternalConfigurationResult>),但当前文档中未对其进行说明。这将导致 API 文档不完整。
📝 建议添加缺失的字段文档
ref {
name "error"
path "org.zstack.header.core.external.service.APIDeleteExternalServiceConfigurationEvent.error"
desc "错误码,若不为null,则表示操作失败, 操作成功时该字段为null",false
type "ErrorCode"
since "5.1.0"
clz ErrorCode.class
}
+ ref {
+ name "applyResults"
+ path "org.zstack.header.core.external.service.APIDeleteExternalServiceConfigurationEvent.applyResults"
+ desc "各管理节点应用配置的结果列表"
+ type "List"
+ since "5.1.0"
+ clz ApplyExternalConfigurationResult.class
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@header/src/main/java/org/zstack/header/core/external/service/APIDeleteExternalServiceConfigurationEventDoc_zh_cn.groovy`
around lines 5 - 23, The API doc is missing the applyResults field from
APIDeleteExternalServiceConfigurationEvent; add a field block in
APIDeleteExternalServiceConfigurationEventDoc_zh_cn.groovy that documents the
applyResults property (name "applyResults"), describes it as the list of
per-configuration ApplyExternalConfigurationResult entries returned by the
operation, specifies the type as "List<ApplyExternalConfigurationResult>" (or
type "List" with a ref to ApplyExternalConfigurationResult), and include
since/version info and class reference to ApplyExternalConfigurationResult to
match how other fields (e.g., error) are documented.
| name "configuration" | ||
| desc "外部服务配置, 使用 json 格式。该字段按原文返回已保存配置;若包含 remote_write.basic_auth.password 等凭据,查询结果会返回明文,请仅通过管理员级接口使用" | ||
| type "String" | ||
| since "5.1.0" | ||
| } |
There was a problem hiding this comment.
严重安全隐患:明文返回敏感凭据
文档明确说明配置字段可能包含密码等敏感凭据,并且查询结果会以明文形式返回。这构成严重的安全漏洞:
- 明文暴露风险:密码以明文形式通过 API 返回,可能被日志记录、缓存或在传输过程中被拦截
- 存储安全:查看数据库 schema(
conf/db/zsv/V5.1.0__schema.sql第 141 行)和实体类(ExternalServiceConfigurationVO.java),configuration字段为普通text/String类型,未见加密处理 - 不足的防护:仅依赖"管理员级接口"限制访问不足以保护敏感凭据,即使是管理员日志、审计记录、浏览器缓存等也会暴露明文密码
建议修复方案:
- 对敏感凭据字段进行加密存储(如使用 AES 加密)
- API 返回时对密码类字段进行脱敏处理(返回
******或完全排除) - 或者提供单独的安全凭据管理机制,将敏感字段与配置分离
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@header/src/main/java/org/zstack/header/core/external/service/ExternalServiceConfigurationInventoryDoc_zh_cn.groovy`
around lines 22 - 26, The inventory currently exposes the "configuration" field
in plain text (see ExternalServiceConfigurationInventoryDoc_zh_cn.groovy) while
the persistence entity ExternalServiceConfigurationVO stores it as a plain
String; fix by implementing encryption-at-rest for the configuration field in
the persistence layer (add AES encryption/decryption hooks in
ExternalServiceConfigurationVO's DAO/persistence methods) and ensure
API/serialization masks or omits sensitive subfields when returning
ExternalServiceConfigurationInventory (replace password-like values with fixed
mask like "******" or strip them entirely in the serializer/mapper used to
produce the inventory); update create/update code paths to encrypt before save
and decrypt only for authorized internal use, and add unit tests to verify
stored data is encrypted and API responses never return raw credentials.
| @Column | ||
| private String serviceType; | ||
| @Column | ||
| private String configuration; | ||
| @Column | ||
| private String description; |
There was a problem hiding this comment.
为 configuration 字段指定适当的列类型定义。
configuration 字段未指定长度约束或列类型,默认情况下 JPA 提供者可能将其映射为 VARCHAR(255)。如果该字段用于存储 JSON、XML 或其他结构化配置数据(这在外部服务配置场景中很常见),则可能导致数据截断。
建议:
- 如果配置数据可能较大,使用
@Lob注解或明确指定columnDefinition = "TEXT"或"MEDIUMTEXT"。 - 为
serviceType和description字段也建议添加合理的长度约束,例如@Column(length = 512)。
建议的修复
`@Column`
private String serviceType;
-@Column
+@Column(columnDefinition = "TEXT")
private String configuration;
`@Column`
private String description;或使用 @Lob:
-@Column
+@Lob
+@Column
private String configuration;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@header/src/main/java/org/zstack/header/core/external/service/ExternalServiceConfigurationVO.java`
around lines 23 - 28, The configuration field in ExternalServiceConfigurationVO
currently has no column type/length and may be mapped to VARCHAR(255); update
the entity to store larger structured data by annotating the configuration field
with either `@Lob` or `@Column`(columnDefinition = "TEXT"/"MEDIUMTEXT") to avoid
truncation, and add sensible length constraints to serviceType and description
(e.g., `@Column`(length = 512)) so their columns are explicitly sized; modify the
annotations on the fields serviceType, configuration, and description in class
ExternalServiceConfigurationVO accordingly.
37655fe to
869020c
Compare
|
Comment from yaohua.wu: Review: MR !10157 — ZSV-12395Background
关联 MR
🔴 Critical
🟡 Warning
🟢 Suggestion
✅ 做得好
Coverage
Verdict: REVISION_REQUIRED1 × Critical(@SDKPackage 包路径错配,阻断 testlib + premium 编译)+ 3 × Warning。框架设计整体合理,修复 Critical 与鉴权/脱敏后可推进(注意当前为 Draft)。 🤖 Robot Reviewer |
92ccca1 to
81d29ed
Compare
- Add ExternalServiceConfigurationVO schema, inventory, query API, and add/update/delete APIs - Apply configuration changes to matching external services across management nodes - Expose serviceType in external service inventory for configuration routing - Generate SDK actions/results and testlib helpers for external service configuration operations - Register persistence and external service API metadata DBImpact APIImpact Resolves: ZSV-12395 Change-Id: I6a6b76626770736871716563676b6a7a74736966
81d29ed to
4447258
Compare
DBImpact
APIImpact
Resolves: ZSV-12395
Change-Id: I6a6b76626770736871716563676b6a7a74736966
sync from gitlab !10157