<feature>[sdk]: expose volumeUuids on PrimaryStorageMigrateVolumeAction#4022
Open
MatheMatrix wants to merge 1 commit into
Open
<feature>[sdk]: expose volumeUuids on PrimaryStorageMigrateVolumeAction#4022MatheMatrix wants to merge 1 commit into
MatheMatrix wants to merge 1 commit into
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本 PR 将单卷迁移参数设为可选并新增批量迁移相关 SDK 类与数据结构，增加测试辅助方法，并提交 ZSPHER-244 的 API 设计权衡文档。 Changes卷迁参数与设计文档
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@sdk/src/main/java/org/zstack/sdk/PrimaryStorageMigrateVolumeAction.java`:
- Around line 28-29: The REST path for PrimaryStorageMigrateVolumeAction still
uses the path placeholder {volumeUuid} while the field volumeUuid was made
optional, causing a contract mismatch when callers supply only volumeUuids (or
omit volumeUuid); either make volumeUuid required again or remove the path
placeholder and accept the UUIDs from the request body (e.g., use volumeUuids or
volumeUuid in the JSON payload). Update the action/route handling to
consistently derive the target UUID(s) from the same source (volumeUuid OR
volumeUuids) and adjust the controller/route definition to no longer require
{volumeUuid} in the path if you choose the body approach; also apply the same
fix to the other occurrences referenced around the volumeUuid/volumeUuids fields
(lines ~97-100).
- Around line 28-33: In PrimaryStorageMigrateVolumeAction, enforce a one-of
constraint between volumeUuid and volumeUuids so requests cannot have both empty
or both set: add validation that at least one of the fields is provided and
ideally only one is set; also tighten volumeUuids' Param annotation by adding
nonempty = true and nullElements = false to reject empty lists and null entries;
implement this check in the action's validate()/checkParameters() path (or
equivalent pre-request validation) and return a clear validation error when the
one-of rule or list constraints are violated.
🪄 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: 17bd9c02-a62e-4396-82ab-3e66b21f8fdb
📒 Files selected for processing (1)
sdk/src/main/java/org/zstack/sdk/PrimaryStorageMigrateVolumeAction.java
Comment on lines
+28
to
+33
| @Param(required = false, nonempty = false, nullElements = false, emptyString = true, noTrim = false) | ||
| public java.lang.String volumeUuid; | ||
|
|
||
| @Param(required = false) | ||
| public java.util.List<String> volumeUuids; | ||
|
|
There was a problem hiding this comment.
缺少 volumeUuid / volumeUuids 的入参互斥或至少其一约束
当前两个字段都为可选，且没有本地校验，允许“两个都不传”或“两个都传”的无效状态进入请求层。建议增加 one-of 校验（至少一个且最好只允许一个），并对 volumeUuids 增加 nonempty/nullElements 约束，避免空列表和空元素下发。
可参考的最小校验补丁
public Result call() {
+ validateVolumeParams();
ApiResult res = ZSClient.call(this);
return makeResult(res);
}
public void call(final Completion<Result> completion) {
+ validateVolumeParams();
ZSClient.call(this, new InternalCompletion() {
`@Override`
public void complete(ApiResult res) {
completion.complete(makeResult(res));
}
});
}
+
+private void validateVolumeParams() {
+ boolean hasSingle = volumeUuid != null && !volumeUuid.isEmpty();
+ boolean hasBatch = volumeUuids != null && !volumeUuids.isEmpty();
+ if (!hasSingle && !hasBatch) {
+ throw new ApiException("either volumeUuid or volumeUuids must be provided");
+ }
+ if (hasSingle && hasBatch) {
+ throw new ApiException("volumeUuid and volumeUuids cannot be provided together");
+ }
+}🤖 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/PrimaryStorageMigrateVolumeAction.java`
around lines 28 - 33, In PrimaryStorageMigrateVolumeAction, enforce a one-of
constraint between volumeUuid and volumeUuids so requests cannot have both empty
or both set: add validation that at least one of the fields is provided and
ideally only one is set; also tighten volumeUuids' Param annotation by adding
nonempty = true and nullElements = false to reject empty lists and null entries;
implement this check in the action's validate()/checkParameters() path (or
equivalent pre-request validation) and return a clear validation error when the
one-of rule or list constraints are violated.
MatheMatrix
force-pushed
the
sync/tao.gan/ZSV-12280@@2
branch
2 times, most recently
from
d48241a to
9f3c68d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/ZSPHER-244-api-design-tradeoff.md`:
- Around line 3-4: Update the two Jira links referencing ZSPHER-244 and
ZSV-12280 to use HTTPS instead of HTTP; locate the strings
"http://jira.zstack.io/browse/ZSPHER-244" and
"http://jira.zstack.io/browse/ZSV-12280" in the document and replace their
schemes with "https://".
🪄 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: 7d1d3223-3f3f-4964-9d21-d33021ee5352
📒 Files selected for processing (3)
docs/ZSPHER-244-api-design-tradeoff.mdsdk/src/main/java/org/zstack/sdk/PrimaryStorageMigrateVolumeAction.javatestlib/src/main/java/org/zstack/testlib/ApiHelper.groovy
Comment on lines
+3
to
+4
| > 关联工单：[ZSPHER-244](http://jira.zstack.io/browse/ZSPHER-244) — 更改数据存储支持数据盘指定不同目标 | ||
| > 关联前置：[ZSV-12280](http://jira.zstack.io/browse/ZSV-12280) — SharedBlock 多数据盘热迁移 |
There was a problem hiding this comment.
将 Jira 链接统一为 HTTPS。
当前文档引用使用了 http://，建议改为 https:// 以避免跳转或安全告警。
建议修改
-> 关联工单：[ZSPHER-244](http://jira.zstack.io/browse/ZSPHER-244) — 更改数据存储支持数据盘指定不同目标
-> 关联前置：[ZSV-12280](http://jira.zstack.io/browse/ZSV-12280) — SharedBlock 多数据盘热迁移
+> 关联工单：[ZSPHER-244](https://jira.zstack.io/browse/ZSPHER-244) — 更改数据存储支持数据盘指定不同目标
+> 关联前置：[ZSV-12280](https://jira.zstack.io/browse/ZSV-12280) — SharedBlock 多数据盘热迁移📝 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
| > 关联工单：[ZSPHER-244](http://jira.zstack.io/browse/ZSPHER-244) — 更改数据存储支持数据盘指定不同目标 | |
| > 关联前置：[ZSV-12280](http://jira.zstack.io/browse/ZSV-12280) — SharedBlock 多数据盘热迁移 | |
| > 关联工单：[ZSPHER-244](https://jira.zstack.io/browse/ZSPHER-244) — 更改数据存储支持数据盘指定不同目标 | |
| > 关联前置：[ZSV-12280](https://jira.zstack.io/browse/ZSV-12280) — SharedBlock 多数据盘热迁移 |
🤖 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 `@docs/ZSPHER-244-api-design-tradeoff.md` around lines 3 - 4, Update the two
Jira links referencing ZSPHER-244 and ZSV-12280 to use HTTPS instead of HTTP;
locate the strings "http://jira.zstack.io/browse/ZSPHER-244" and
"http://jira.zstack.io/browse/ZSV-12280" in the document and replace their
schemes with "https://".
9f3c68d to
522614f
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
sdk/src/main/java/org/zstack/sdk/PrimaryStorageMigrateVolumeAction.java (2)
28-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick win缺少
volumeUuid与volumeUuids的互斥校验与非空约束当前两个字段均为可选，且 SDK 层未实施校验，允许以下无效状态：
- 两者均未传：请求无迁移目标
- 两者均传：语义不明确
volumeUuids为空列表或包含 null 元素：缺少nonempty和nullElements约束建议在
call()方法调用ZSClient.call()前增加本地校验，确保：
- 至少提供
volumeUuid或volumeUuids其中之一- 两者不能同时提供（互斥约束）
- 为
volumeUuids添加nonempty = true, nullElements = false约束参考校验实现
public Result call() { + validateVolumeParams(); ApiResult res = ZSClient.call(this); return makeResult(res); } public void call(final Completion<Result> completion) { + validateVolumeParams(); ZSClient.call(this, new InternalCompletion() { `@Override` public void complete(ApiResult res) { completion.complete(makeResult(res)); } }); } + +private void validateVolumeParams() { + boolean hasSingle = volumeUuid != null && !volumeUuid.isEmpty(); + boolean hasBatch = volumeUuids != null && !volumeUuids.isEmpty(); + if (!hasSingle && !hasBatch) { + throw new ApiException("必须提供 volumeUuid 或 volumeUuids 其中之一"); + } + if (hasSingle && hasBatch) { + throw new ApiException("volumeUuid 和 volumeUuids 不能同时提供"); + } +}并在 Line 31 处调整注解：
-@Param(required = false) +@Param(required = false, nonempty = true, nullElements = false) public java.util.List<String> volumeUuids;🤖 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/PrimaryStorageMigrateVolumeAction.java` around lines 28 - 33, Add local validation in PrimaryStorageMigrateVolumeAction.call() before invoking ZSClient.call(): ensure exactly one of volumeUuid or volumeUuids is provided (if both null/empty or both set, throw an IllegalArgumentException or set error in result), and treat volumeUuids as required non-empty with no null elements (reject empty list or lists containing nulls). Also update the field annotation for volumeUuids to include nonempty = true and nullElements = false to match the runtime check; reference the fields volumeUuid and volumeUuids and the call() method and ZSClient.call() when implementing the checks.
97-100:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftREST 路径占位符与可选
volumeUuid字段存在契约冲突当前 REST 路径定义为
/primary-storage/volumes/{volumeUuid}/actions，强依赖路径变量{volumeUuid}，但 Line 28 已将volumeUuid字段改为可选。当调用方仅提供volumeUuids进行批量迁移时，路径变量无法解析，导致：
- URL 构造失败或生成无效路径
- 批量迁移场景无法使用该 API
建议采用以下方案之一：
- 保持
volumeUuid必填，volumeUuids仅作为批量扩展字段（与单卷迁移共用路径）- 移除路径中的
{volumeUuid}占位符，将卷标识完全移至请求体（需评估向后兼容性）- 新增独立的批量迁移 Action（如
PrimaryStorageMigrateBatchVolumesAction），保持当前 Action 单卷语义根据设计权衡文档（
docs/ZSPHER-244-api-design-tradeoff.md）推荐方案 B（新增独立 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 `@sdk/src/main/java/org/zstack/sdk/PrimaryStorageMigrateVolumeAction.java` around lines 97 - 100, The REST path in PrimaryStorageMigrateVolumeAction (getRestInfo) includes a required {volumeUuid} path variable while the volumeUuid field is now optional, causing invalid URL construction for batch requests; update getRestInfo to remove the path placeholder (e.g. change info.path from "/primary-storage/volumes/{volumeUuid}/actions" to a path that does not require a single-volume UUID such as "/primary-storage/volumes/actions"), and ensure the action consumes volume identifiers from the request body (volumeUuid or volumeUuids) accordingly; alternatively, if you prefer single- vs batch- semantics, create a separate PrimaryStorageMigrateBatchVolumesAction and keep the original getRestInfo path for single-volume migration—pick one approach and make the path, getRestInfo, and request-body fields consistent.docs/ZSPHER-244-api-design-tradeoff.md (1)
3-4:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win将 Jira 链接统一改为 HTTPS。
这两处仍在使用
http://，建议改成https://以避免跳转与安全告警。建议修改
-> 关联工单：[ZSPHER-244](http://jira.zstack.io/browse/ZSPHER-244) — 更改数据存储支持数据盘指定不同目标 -> 关联前置：[ZSV-12280](http://jira.zstack.io/browse/ZSV-12280) — SharedBlock 多数据盘热迁移 +> 关联工单：[ZSPHER-244](https://jira.zstack.io/browse/ZSPHER-244) — 更改数据存储支持数据盘指定不同目标 +> 关联前置：[ZSV-12280](https://jira.zstack.io/browse/ZSV-12280) — SharedBlock 多数据盘热迁移🤖 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 `@docs/ZSPHER-244-api-design-tradeoff.md` around lines 3 - 4, Update the two Jira links that currently use "http://" to use "https://" instead: change the ZSPHER-244 link and the ZSV-12280 link in the text (references to ZSPHER-244 and ZSV-12280) so both URLs begin with "https://" to avoid redirects and security warnings.
🤖 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.
Duplicate comments:
In `@docs/ZSPHER-244-api-design-tradeoff.md`:
- Around line 3-4: Update the two Jira links that currently use "http://" to use
"https://" instead: change the ZSPHER-244 link and the ZSV-12280 link in the
text (references to ZSPHER-244 and ZSV-12280) so both URLs begin with "https://"
to avoid redirects and security warnings.
In `@sdk/src/main/java/org/zstack/sdk/PrimaryStorageMigrateVolumeAction.java`:
- Around line 28-33: Add local validation in
PrimaryStorageMigrateVolumeAction.call() before invoking ZSClient.call(): ensure
exactly one of volumeUuid or volumeUuids is provided (if both null/empty or both
set, throw an IllegalArgumentException or set error in result), and treat
volumeUuids as required non-empty with no null elements (reject empty list or
lists containing nulls). Also update the field annotation for volumeUuids to
include nonempty = true and nullElements = false to match the runtime check;
reference the fields volumeUuid and volumeUuids and the call() method and
ZSClient.call() when implementing the checks.
- Around line 97-100: The REST path in PrimaryStorageMigrateVolumeAction
(getRestInfo) includes a required {volumeUuid} path variable while the
volumeUuid field is now optional, causing invalid URL construction for batch
requests; update getRestInfo to remove the path placeholder (e.g. change
info.path from "/primary-storage/volumes/{volumeUuid}/actions" to a path that
does not require a single-volume UUID such as
"/primary-storage/volumes/actions"), and ensure the action consumes volume
identifiers from the request body (volumeUuid or volumeUuids) accordingly;
alternatively, if you prefer single- vs batch- semantics, create a separate
PrimaryStorageMigrateBatchVolumesAction and keep the original getRestInfo path
for single-volume migration—pick one approach and make the path, getRestInfo,
and request-body fields consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: aef2bb2d-3174-47e5-93d5-0c1641a5b57a
📒 Files selected for processing (3)
docs/ZSPHER-244-api-design-tradeoff.mdsdk/src/main/java/org/zstack/sdk/PrimaryStorageMigrateVolumeAction.javatestlib/src/main/java/org/zstack/testlib/ApiHelper.groovy
MatheMatrix
force-pushed
the
sync/tao.gan/ZSV-12280@@2
branch
from
522614f to
02293db
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
docs/ZSPHER-244-api-design-tradeoff.md (1)
3-4:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win将 Jira 链接统一为 HTTPS
当前文档引用使用了
http://，建议改为https://以避免跳转或安全告警。🔧 建议修改
-> 关联工单：[ZSPHER-244](http://jira.zstack.io/browse/ZSPHER-244) — 更改数据存储支持数据盘指定不同目标 -> 关联前置：[ZSV-12280](http://jira.zstack.io/browse/ZSV-12280) — SharedBlock 多数据盘热迁移 +> 关联工单：[ZSPHER-244](https://jira.zstack.io/browse/ZSPHER-244) — 更改数据存储支持数据盘指定不同目标 +> 关联前置：[ZSV-12280](https://jira.zstack.io/browse/ZSV-12280) — SharedBlock 多数据盘热迁移🤖 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 `@docs/ZSPHER-244-api-design-tradeoff.md` around lines 3 - 4, Update the Jira links in the document to use HTTPS instead of HTTP: replace occurrences like "http://jira.zstack.io/browse/ZSPHER-244" and "http://jira.zstack.io/browse/ZSV-12280" with "https://..." so all Jira references in docs/ZSPHER-244-api-design-tradeoff.md use secure HTTPS URLs and avoid redirects or security warnings.sdk/src/main/java/org/zstack/sdk/PrimaryStorageMigrateVolumeAction.java (2)
28-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
volumeUuid和volumeUuids缺少互斥校验和列表约束当前两个字段均为可选且无本地校验，允许以下无效状态：
- 两个都不传 → 无法确定迁移目标卷
- 两个都传 → 语义冲突
volumeUuids为空列表或包含 null 元素 → 运行时错误建议：
- 增加 one-of 校验（至少提供一个，最好互斥）
- 为
volumeUuids添加约束：@Param(required = false, nonempty = true, nullElements = false)💡 参考实现
在
call()方法调用前增加校验：public Result call() { + validateVolumeParams(); ApiResult res = ZSClient.call(this); return makeResult(res); } public void call(final Completion<Result> completion) { + validateVolumeParams(); ZSClient.call(this, new InternalCompletion() { `@Override` public void complete(ApiResult res) { completion.complete(makeResult(res)); } }); } + +private void validateVolumeParams() { + boolean hasSingle = volumeUuid != null && !volumeUuid.isEmpty(); + boolean hasBatch = volumeUuids != null && !volumeUuids.isEmpty(); + if (!hasSingle && !hasBatch) { + throw new ApiException("必须提供 volumeUuid 或 volumeUuids 之一"); + } + if (hasSingle && hasBatch) { + throw new ApiException("volumeUuid 和 volumeUuids 不能同时提供"); + } +}同时调整
volumeUuids的参数注解：-@Param(required = false) +@Param(required = false, nonempty = true, nullElements = false) public java.util.List<String> volumeUuids;🤖 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/PrimaryStorageMigrateVolumeAction.java` around lines 28 - 33, Add mutual-exclusion and list-content validation for the volume selection parameters in PrimaryStorageMigrateVolumeAction: update the volumeUuids field annotation to `@Param`(required = false, nonempty = true, nullElements = false) and implement a pre-call check inside the class (e.g., at the start of call()/validate() helper) that enforces exactly one of volumeUuid or volumeUuids is provided (reject if both absent or both present) and that volumeUuids is non-empty and contains no nulls; throw an IllegalArgumentException or populate the action error consistently when validation fails.
97-105:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftREST 路径占位符与可选参数发生契约冲突
第 100 行的 REST 路径仍使用
{volumeUuid}占位符，但第 28 行已将volumeUuid改为可选。当调用方仅提供volumeUuids（或两者都不提供）时，URL 无法正确构造，导致：
- 路径解析失败
- 请求无法发送
- 迁移功能不可用
建议方案（二选一）：
- 保持
volumeUuid为必填（required = true），在批量场景要求调用方同时提供volumeUuid和volumeUuids- 调整 REST 路径为不依赖占位符的形式（如
/primary-storage/volume-migrations/actions），所有卷标识通过请求体传递从架构角度，根据配套设计文档（
ZSPHER-244-api-design-tradeoff.md）的分析，方案 B（新增独立 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 `@sdk/src/main/java/org/zstack/sdk/PrimaryStorageMigrateVolumeAction.java` around lines 97 - 105, The REST path in PrimaryStorageMigrateVolumeAction.getRestInfo uses the {volumeUuid} placeholder while the volumeUuid parameter was made optional, causing URL construction failures; either make the volumeUuid field required again (set required = true on the volumeUuid parameter) so callers must supply it alongside volumeUuids, or change getRestInfo to use a non-placeholder path (for example replace "/primary-storage/volumes/{volumeUuid}/actions" with a generic endpoint like "/primary-storage/volume-migrations/actions") and ensure all volume identifiers (volumeUuid/volumeUuids) are passed in the request body; update the corresponding callers/tests to match the chosen approach.
🧹 Nitpick comments (1)
sdk/src/main/java/org/zstack/sdk/PrimaryStorageMigrateVolumeAction.java (1)
28-33: 🏗️ Heavy lift实现方案与设计文档推荐不一致
当前实现采用"扩展现有 API"方式（在
PrimaryStorageMigrateVolumeAction上添加volumeUuids字段），但配套的设计文档ZSPHER-244-api-design-tradeoff.md明确推荐"方案 B：新增独立 API"，并列出方案 A 的多个缺陷：
- 互斥字段爆炸（volumeUuid / volumeUuids / 未来的 volumeMigrationSpecs）
- 语义模糊（单盘/多盘、冷迁移/热迁移混用）
- 路径占位符冲突（本次 PR 已显现）
- 审计/事件 inventory 形状不一致
建议：
- 如果本次 PR 只是 Phase 1（先暴露 SDK 字段），请在 commit message 或文档中明确说明后续演进计划
- 如果设计决策尚未最终确定，建议在合并前与 PD/架构组达成一致，避免后续推倒重来
- 考虑参考文档中的建议，评估是否直接采用方案 B（新增
PrimaryStorageMigrateDataVolumesAction）🤖 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/PrimaryStorageMigrateVolumeAction.java` around lines 28 - 33, The current change adds a new field volumeUuids to PrimaryStorageMigrateVolumeAction, which diverges from the design doc ZSPHER-244-api-design-tradeoff.md that recommends adding a separate API; either clearly mark this PR as a Phase 1/temporary SDK-only change and add a commit message and/or comment documenting the planned follow-ups, or revert the field addition and implement the recommended approach by creating a new API/action (e.g., PrimaryStorageMigrateDataVolumesAction) instead; coordinate the final choice with PD/architecture and update related docs to avoid the mutex-field, semantic, path-placeholder and inventory shape issues (reference fields: PrimaryStorageMigrateVolumeAction.volumeUuid, PrimaryStorageMigrateVolumeAction.volumeUuids, and the suggested PrimaryStorageMigrateDataVolumesAction).
🤖 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.
Duplicate comments:
In `@docs/ZSPHER-244-api-design-tradeoff.md`:
- Around line 3-4: Update the Jira links in the document to use HTTPS instead of
HTTP: replace occurrences like "http://jira.zstack.io/browse/ZSPHER-244" and
"http://jira.zstack.io/browse/ZSV-12280" with "https://..." so all Jira
references in docs/ZSPHER-244-api-design-tradeoff.md use secure HTTPS URLs and
avoid redirects or security warnings.
In `@sdk/src/main/java/org/zstack/sdk/PrimaryStorageMigrateVolumeAction.java`:
- Around line 28-33: Add mutual-exclusion and list-content validation for the
volume selection parameters in PrimaryStorageMigrateVolumeAction: update the
volumeUuids field annotation to `@Param`(required = false, nonempty = true,
nullElements = false) and implement a pre-call check inside the class (e.g., at
the start of call()/validate() helper) that enforces exactly one of volumeUuid
or volumeUuids is provided (reject if both absent or both present) and that
volumeUuids is non-empty and contains no nulls; throw an
IllegalArgumentException or populate the action error consistently when
validation fails.
- Around line 97-105: The REST path in
PrimaryStorageMigrateVolumeAction.getRestInfo uses the {volumeUuid} placeholder
while the volumeUuid parameter was made optional, causing URL construction
failures; either make the volumeUuid field required again (set required = true
on the volumeUuid parameter) so callers must supply it alongside volumeUuids, or
change getRestInfo to use a non-placeholder path (for example replace
"/primary-storage/volumes/{volumeUuid}/actions" with a generic endpoint like
"/primary-storage/volume-migrations/actions") and ensure all volume identifiers
(volumeUuid/volumeUuids) are passed in the request body; update the
corresponding callers/tests to match the chosen approach.
---
Nitpick comments:
In `@sdk/src/main/java/org/zstack/sdk/PrimaryStorageMigrateVolumeAction.java`:
- Around line 28-33: The current change adds a new field volumeUuids to
PrimaryStorageMigrateVolumeAction, which diverges from the design doc
ZSPHER-244-api-design-tradeoff.md that recommends adding a separate API; either
clearly mark this PR as a Phase 1/temporary SDK-only change and add a commit
message and/or comment documenting the planned follow-ups, or revert the field
addition and implement the recommended approach by creating a new API/action
(e.g., PrimaryStorageMigrateDataVolumesAction) instead; coordinate the final
choice with PD/architecture and update related docs to avoid the mutex-field,
semantic, path-placeholder and inventory shape issues (reference fields:
PrimaryStorageMigrateVolumeAction.volumeUuid,
PrimaryStorageMigrateVolumeAction.volumeUuids, and the suggested
PrimaryStorageMigrateDataVolumesAction).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 132f9e0d-324d-40cc-b76d-45f6ed125129
📒 Files selected for processing (2)
docs/ZSPHER-244-api-design-tradeoff.mdsdk/src/main/java/org/zstack/sdk/PrimaryStorageMigrateVolumeAction.java
MatheMatrix
force-pushed
the
sync/tao.gan/ZSV-12280@@2
branch
from
02293db to
a97c804
Compare
Resolves: ZSV-12280 Change-Id: Ie3fb7043450a35f6833eb937d4fd157d7fe4cbde
MatheMatrix
force-pushed
the
sync/tao.gan/ZSV-12280@@2
branch
from
a97c804 to
c9c3a16
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
sdk/src/main/java/org/zstack/sdk/PrimaryStorageMigrateVolumeAction.java (3)
28-29:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy lift路径占位符与可选参数的契约冲突仍然存在
尽管先前评论标记为已解决，但当前代码中
volumeUuid已改为可选（required = false），而 REST 路径仍为/primary-storage/volumes/{volumeUuid}/actions，强依赖路径变量{volumeUuid}。当调用方仅提供volumeUuids或两者均未提供时，URL 构造可能失败或行为不确定。建议：
- 保持
volumeUuid必填并将批量场景迁移至独立 API（如已新增的PrimaryStorageMigrateDataVolumesAction）- 或调整路径为不依赖路径变量的形式（如
/primary-storage/volumes/actions），通过请求体传递 UUIDAlso applies to: 100-100
🤖 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/PrimaryStorageMigrateVolumeAction.java` around lines 28 - 29, PrimaryStorageMigrateVolumeAction currently declares volumeUuid as optional while the REST path still requires the path variable /primary-storage/volumes/{volumeUuid}/actions; fix by either making volumeUuid required again (set required = true on the volumeUuid field in PrimaryStorageMigrateVolumeAction) and keep single-volume path semantics, or change the API path to a non-path-variable form (e.g., /primary-storage/volumes/actions) and accept volumeUuids in the request body or route callers to the batch-specific PrimaryStorageMigrateDataVolumesAction; update the action declaration and API routing consistently so path variables and parameter requirements do not conflict.
28-33:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win缺少 volumeUuid 与 volumeUuids 的互斥与至少其一约束
当前两个字段均为可选且无客户端校验，允许"两者皆空"或"两者皆传"的无效状态进入请求层。建议在
call()方法中增加校验逻辑：
- 至少提供
volumeUuid或volumeUuids其中之一- 理想情况下两者互斥，仅允许一种方式
参考校验实现
public Result call() { + validateVolumeParams(); ApiResult res = ZSClient.call(this); return makeResult(res); } public void call(final Completion<Result> completion) { + validateVolumeParams(); ZSClient.call(this, new InternalCompletion() { `@Override` public void complete(ApiResult res) { completion.complete(makeResult(res)); } }); } + +private void validateVolumeParams() { + boolean hasSingle = volumeUuid != null && !volumeUuid.isEmpty(); + boolean hasBatch = volumeUuids != null && !volumeUuids.isEmpty(); + if (!hasSingle && !hasBatch) { + throw new ApiException("必须提供 volumeUuid 或 volumeUuids 其中之一"); + } + if (hasSingle && hasBatch) { + throw new ApiException("volumeUuid 与 volumeUuids 不能同时提供"); + } +}Also applies to: 75-87
🤖 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/PrimaryStorageMigrateVolumeAction.java` around lines 28 - 33, PrimaryStorageMigrateVolumeAction currently allows both volumeUuid and volumeUuids to be empty or both set; add a validation in the call() method of PrimaryStorageMigrateVolumeAction to ensure exactly one of these is provided: if both are null/empty or both non-empty, throw an IllegalArgumentException (or set ApiResult with proper error) with a clear message. Check the fields volumeUuid and volumeUuids (and apply the same validation to the other occurrence referenced around lines 75-87) before proceeding with the request execution so the client cannot submit an invalid combination.
31-32: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win为 volumeUuids 增加列表完整性约束
volumeUuids的@Param注解较为宽松，应添加nonempty = true和nullElements = false约束，防止空列表或包含 null 元素的列表下发至后端导致运行时异常。♻️ 建议修复
- `@Param`(required = false) + `@Param`(required = false, nonempty = true, nullElements = false, emptyString = true, noTrim = false) public java.util.List<String> volumeUuids;🤖 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/PrimaryStorageMigrateVolumeAction.java` around lines 31 - 32, 为字段 volumeUuids（类 PrimaryStorageMigrateVolumeAction、类型 java.util.List<String>）的 `@Param` 注解添加完整性约束：在现有 `@Param`(...) 中设置 nonempty = true 和 nullElements = false，确保不允许空列表或包含 null 元素的列表下发到后端，从而避免运行时异常。
🧹 Nitpick comments (3)
sdk/src/main/java/org/zstack/sdk/PrimaryStorageMigrateVolumeAction.java (1)
38-38: ⚡ Quick win为 systemTags 和 userTags 添加泛型类型参数
与
PrimaryStorageMigrateDataVolumesAction中相同问题，systemTags和userTags应明确为List<String>以增强类型安全性并符合项目惯用模式。♻️ 建议修复
`@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;Also applies to: 41-41
🤖 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/PrimaryStorageMigrateVolumeAction.java` at line 38, 在 PrimaryStorageMigrateVolumeAction 类中为字段 systemTags 和 userTags 添加泛型参数，将它们从 raw java.util.List 改为 java.util.List<String>（与 PrimaryStorageMigrateDataVolumesAction 保持一致），以提高类型安全性并符合项目习惯；确认相应的导入仍存在并且不破坏序列化/反序列化行为。sdk/src/main/java/org/zstack/sdk/PrimaryStorageMigrateDataVolumesAction.java (2)
35-35: ⚡ Quick win为 systemTags 和 userTags 添加泛型类型参数
systemTags和userTags均使用原始List类型，应明确指定元素类型（通常为List<String>）以符合 ZStack SDK 惯用模式并提升类型安全性。♻️ 建议修复
`@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;Also applies to: 38-38
🤖 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/PrimaryStorageMigrateDataVolumesAction.java` at line 35, The fields using raw List should be parameterized: update the declarations of systemTags and userTags in PrimaryStorageMigrateDataVolumesAction to use a generic type (e.g. java.util.List<String>) instead of raw java.util.List to match SDK conventions and improve type safety; ensure any related getters/setters and usages in the class (and other occurrences of these fields) are updated to the same generic type.
5-5: ⚡ Quick win移除冗余的同包导入语句
当前类已在
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 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/PrimaryStorageMigrateDataVolumesAction.java` at line 5, 当前类 PrimaryStorageMigrateDataVolumesAction 位于 org.zstack.sdk 包内，不需要导入同包的通配符 import org.zstack.sdk.*；请删除该冗余导入语句（import org.zstack.sdk.*）以提升可读性并避免无用代码，保留其它必需的显式导入不受影响。
🤖 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
`@sdk/src/main/java/org/zstack/sdk/PrimaryStorageMigrateDataVolumesAction.java`:
- Line 32: The field volumeMigrationSpecs in
PrimaryStorageMigrateDataVolumesAction is declared as a raw List; change its
declaration to use a generic type List<VolumeMigrationSpec> to restore
compile-time type safety and eliminate raw-type warnings—update any places that
construct or iterate this list (constructors, setters/getters, or JSON
serialization usages) to use the VolumeMigrationSpec type as well so all usages
compile cleanly.
---
Duplicate comments:
In `@sdk/src/main/java/org/zstack/sdk/PrimaryStorageMigrateVolumeAction.java`:
- Around line 28-29: PrimaryStorageMigrateVolumeAction currently declares
volumeUuid as optional while the REST path still requires the path variable
/primary-storage/volumes/{volumeUuid}/actions; fix by either making volumeUuid
required again (set required = true on the volumeUuid field in
PrimaryStorageMigrateVolumeAction) and keep single-volume path semantics, or
change the API path to a non-path-variable form (e.g.,
/primary-storage/volumes/actions) and accept volumeUuids in the request body or
route callers to the batch-specific PrimaryStorageMigrateDataVolumesAction;
update the action declaration and API routing consistently so path variables and
parameter requirements do not conflict.
- Around line 28-33: PrimaryStorageMigrateVolumeAction currently allows both
volumeUuid and volumeUuids to be empty or both set; add a validation in the
call() method of PrimaryStorageMigrateVolumeAction to ensure exactly one of
these is provided: if both are null/empty or both non-empty, throw an
IllegalArgumentException (or set ApiResult with proper error) with a clear
message. Check the fields volumeUuid and volumeUuids (and apply the same
validation to the other occurrence referenced around lines 75-87) before
proceeding with the request execution so the client cannot submit an invalid
combination.
- Around line 31-32: 为字段 volumeUuids（类 PrimaryStorageMigrateVolumeAction、类型
java.util.List<String>）的 `@Param` 注解添加完整性约束：在现有 `@Param`(...) 中设置 nonempty = true 和
nullElements = false，确保不允许空列表或包含 null 元素的列表下发到后端，从而避免运行时异常。
---
Nitpick comments:
In
`@sdk/src/main/java/org/zstack/sdk/PrimaryStorageMigrateDataVolumesAction.java`:
- Line 35: The fields using raw List should be parameterized: update the
declarations of systemTags and userTags in
PrimaryStorageMigrateDataVolumesAction to use a generic type (e.g.
java.util.List<String>) instead of raw java.util.List to match SDK conventions
and improve type safety; ensure any related getters/setters and usages in the
class (and other occurrences of these fields) are updated to the same generic
type.
- Line 5: 当前类 PrimaryStorageMigrateDataVolumesAction 位于 org.zstack.sdk
包内，不需要导入同包的通配符 import org.zstack.sdk.*；请删除该冗余导入语句（import
org.zstack.sdk.*）以提升可读性并避免无用代码，保留其它必需的显式导入不受影响。
In `@sdk/src/main/java/org/zstack/sdk/PrimaryStorageMigrateVolumeAction.java`:
- Line 38: 在 PrimaryStorageMigrateVolumeAction 类中为字段 systemTags 和 userTags
添加泛型参数，将它们从 raw java.util.List 改为 java.util.List<String>（与
PrimaryStorageMigrateDataVolumesAction
保持一致），以提高类型安全性并符合项目习惯；确认相应的导入仍存在并且不破坏序列化/反序列化行为。
🪄 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: 5b64f473-3adc-469c-8f94-ea196ff661d1
📒 Files selected for processing (6)
docs/ZSPHER-244-api-design-tradeoff.mdsdk/src/main/java/org/zstack/sdk/PrimaryStorageMigrateDataVolumesAction.javasdk/src/main/java/org/zstack/sdk/PrimaryStorageMigrateDataVolumesResult.javasdk/src/main/java/org/zstack/sdk/PrimaryStorageMigrateVolumeAction.javasdk/src/main/java/org/zstack/sdk/VolumeMigrationSpec.javatestlib/src/main/java/org/zstack/testlib/ApiHelper.groovy
✅ Files skipped from review due to trivial changes (2)
- sdk/src/main/java/org/zstack/sdk/VolumeMigrationSpec.java
- sdk/src/main/java/org/zstack/sdk/PrimaryStorageMigrateDataVolumesResult.java
| public java.lang.String vmInstanceUuid; | ||
|
|
||
| @Param(required = true, nonempty = true, nullElements = false, emptyString = true, noTrim = false) | ||
| public java.util.List volumeMigrationSpecs; |
There was a problem hiding this comment.
使用泛型类型参数以增强类型安全
volumeMigrationSpecs 声明为原始 List 类型，缺失泛型参数会导致编译器警告并丧失编译期类型检查。根据 PR 上下文，该字段应为 List<VolumeMigrationSpec>，明确元素类型可避免运行时类型转换错误。
🔧 建议修复
`@Param`(required = true, nonempty = true, nullElements = false, emptyString = true, noTrim = false)
- public java.util.List volumeMigrationSpecs;
+ public java.util.List<VolumeMigrationSpec> volumeMigrationSpecs;🤖 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/PrimaryStorageMigrateDataVolumesAction.java`
at line 32, The field volumeMigrationSpecs in
PrimaryStorageMigrateDataVolumesAction is declared as a raw List; change its
declaration to use a generic type List<VolumeMigrationSpec> to restore
compile-time type safety and eliminate raw-type warnings—update any places that
construct or iterate this list (constructors, setters/getters, or JSON
serialization usages) to use the VolumeMigrationSpec type as well so all usages
compile cleanly.
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
2 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.
Resolves: ZSV-12280
Change-Id: Ie3fb7043450a35f6833eb937d4fd157d7fe4cbde
sync from gitlab !9917