<feature>[sdk]: expose volumeUuids on PrimaryStorageMigrateVolumeAction#4013
Open
MatheMatrix wants to merge 1 commit into
Open
<feature>[sdk]: expose volumeUuids on PrimaryStorageMigrateVolumeAction#4013MatheMatrix wants to merge 1 commit into
MatheMatrix wants to merge 1 commit into
Conversation
WalkthroughPR 更新了 变更卷迁移操作参数支持
预估代码审查工作量🎯 2 (简单) | ⏱️ ~5 分钟 诗
🚥 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk/src/main/java/org/zstack/sdk/PrimaryStorageMigrateVolumeAction.java (1)
97-105:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftREST 路径变量与批量参数存在设计冲突
PrimaryStorageMigrateVolumeAction同时支持单个卷迁移（volumeUuid）和批量卷迁移（volumeUuids），但 REST 路径定义为/primary-storage/volumes/{volumeUuid}/actions。当调用者仅提供volumeUuids而不提供volumeUuid时，框架尝试替换路径变量{volumeUuid}会失败，抛出异常 "missing required field[volumeUuid]"，导致批量操作无法正常工作。建议方案：
- 方案 1（推荐）：为批量迁移创建独立的 API 类（如
PrimaryStorageBatchMigrateVolumesAction），使用专用路径（如/primary-storage/volumes/batch-migrate），通过请求体传递volumeUuids。- 方案 2：修改当前 API 为通用路径（如
/primary-storage/volume-migration），支持两种使用模式，所有参数通过请求体传递，无需在 URL 中包含卷 ID。- 方案 3：若仅支持批量操作，则从
volumeUuids中取第一个元素作为{volumeUuid}路径变量，并通过请求体传递完整的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 97 - 105, The getRestInfo() in PrimaryStorageMigrateVolumeAction defines a path with a required {volumeUuid}, which conflicts with batch parameter volumeUuids and causes "missing required field[volumeUuid]" when callers supply only volumeUuids; fix by implementing the recommended Solution 1: create a new action class (e.g., PrimaryStorageBatchMigrateVolumesAction) that replaces per-volume semantics, define its getRestInfo() to use a batch-specific path such as "/primary-storage/volumes/batch-migrate" (no path variables), set httpMethod to "PUT", needSession and needPoll as appropriate, and set parameterName to "primaryStorageBatchMigrateVolumes" so callers send volumeUuids in the request body; keep PrimaryStorageMigrateVolumeAction unchanged for single-volume use or deprecate it if you consolidate.
🧹 Nitpick comments (1)
sdk/src/main/java/org/zstack/sdk/PrimaryStorageMigrateVolumeAction.java (1)
31-32: ⚡ Quick win建议为 volumeUuids 参数添加更完整的验证约束
新增的
volumeUuids参数仅标记了required = false，缺少其他验证约束。建议参考volumeUuid的验证配置，补充以下约束：
nullElements = false：防止列表中包含 null 元素emptyString = true：根据业务需求确定是否允许空字符串 UUIDnonempty = true：如果使用批量迁移，列表不应为空♻️ 建议的参数验证配置
- `@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, The volumeUuids field in PrimaryStorageMigrateVolumeAction currently only has required=false and needs stronger validation like volumeUuid; update the `@Param` annotation on the volumeUuids field to include nullElements=false, nonempty=true and emptyString=true (or adjust emptyString per business rules) so the list cannot contain nulls and cannot be empty while matching the volumeUuid validation style.
🤖 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-32: The PrimaryStorageMigrateVolumeAction class now exposes
both volumeUuid and volumeUuids but lacks validation to ensure at least one is
provided; add a validation step in the action (e.g., a validate() method invoked
before request assembly or inside the existing call/execute path) that checks
fields volumeUuid and volumeUuids and throws/returns a clear parameter error if
both are null/empty, or alternatively document their
mutual-exclusion/requirement at class-level if you prefer not to validate in
code; reference the class PrimaryStorageMigrateVolumeAction and the fields
volumeUuid and volumeUuids when implementing this check.
---
Outside diff comments:
In `@sdk/src/main/java/org/zstack/sdk/PrimaryStorageMigrateVolumeAction.java`:
- Around line 97-105: The getRestInfo() in PrimaryStorageMigrateVolumeAction
defines a path with a required {volumeUuid}, which conflicts with batch
parameter volumeUuids and causes "missing required field[volumeUuid]" when
callers supply only volumeUuids; fix by implementing the recommended Solution 1:
create a new action class (e.g., PrimaryStorageBatchMigrateVolumesAction) that
replaces per-volume semantics, define its getRestInfo() to use a batch-specific
path such as "/primary-storage/volumes/batch-migrate" (no path variables), set
httpMethod to "PUT", needSession and needPoll as appropriate, and set
parameterName to "primaryStorageBatchMigrateVolumes" so callers send volumeUuids
in the request body; keep PrimaryStorageMigrateVolumeAction unchanged for
single-volume use or deprecate it if you consolidate.
---
Nitpick comments:
In `@sdk/src/main/java/org/zstack/sdk/PrimaryStorageMigrateVolumeAction.java`:
- Around line 31-32: The volumeUuids field in PrimaryStorageMigrateVolumeAction
currently only has required=false and needs stronger validation like volumeUuid;
update the `@Param` annotation on the volumeUuids field to include
nullElements=false, nonempty=true and emptyString=true (or adjust emptyString
per business rules) so the list cannot contain nulls and cannot be empty while
matching the volumeUuid validation style.
🪄 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: 1cb3fd3d-c7d2-4bcb-be2e-c36a8eca666a
📒 Files selected for processing (1)
sdk/src/main/java/org/zstack/sdk/PrimaryStorageMigrateVolumeAction.java
Comment on lines
+28
to
+32
| @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 的互斥验证逻辑
将 volumeUuid 改为可选并新增 volumeUuids 后，当前实现无法保证至少提供其中一个参数。如果两个参数都为空，迁移操作将无法执行，可能导致运行时错误或需要在服务端额外验证。
建议：
- 在 SDK Action 层面添加参数验证逻辑，确保
volumeUuid和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 - 32, The PrimaryStorageMigrateVolumeAction class now exposes
both volumeUuid and volumeUuids but lacks validation to ensure at least one is
provided; add a validation step in the action (e.g., a validate() method invoked
before request assembly or inside the existing call/execute path) that checks
fields volumeUuid and volumeUuids and throws/returns a clear parameter error if
both are null/empty, or alternatively document their
mutual-exclusion/requirement at class-level if you prefer not to validate in
code; reference the class PrimaryStorageMigrateVolumeAction and the fields
volumeUuid and volumeUuids when implementing this check.
MatheMatrix
force-pushed
the
sync/tao.gan/5.1.0-ZSV-10831@@2
branch
from
4629e57 to
a445e3d
Compare
Resolves: ZSV-10831 Change-Id: Ie3fb7043450a35f6833eb937d4fd157d7fe4cbde
MatheMatrix
force-pushed
the
sync/tao.gan/5.1.0-ZSV-10831@@2
branch
from
a445e3d to
2f46f6e
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 `@sdk/src/main/java/org/zstack/sdk/PrimaryStorageMigrateVolumeAction.java`:
- Around line 28-33: The REST path in PrimaryStorageMigrateVolumeAction still
embeds a single volumeUuid while the class now accepts both volumeUuid and
volumeUuids, causing batch calls to fail; update the getRestInfo()
implementation to use a non-instance path that does not require the {volumeUuid}
path variable (e.g. switch to a collection-level route like
/primary-storage/volumes/actions) and ensure request serialization uses
volumeUuids when provided (or alternatively split into two Actions: one that
keeps volumeUuid and its current path and one new
BatchPrimaryStorageMigrateVolumesAction that uses volumeUuids and a collection
route); modify getRestInfo() and any routing-related metadata accordingly to
make the parameter/route contract consistent with the volumeUuid and volumeUuids
fields.
🪄 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: d2634952-c314-4f7c-9c25-ee2a89fe3912
📒 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.
批量参数已放开，但当前 URL 仍硬依赖 volumeUuid，会导致批量场景不可达。
Line 28 把 volumeUuid 改为可选，Line 31 新增 volumeUuids，但 getRestInfo() 仍使用 /primary-storage/volumes/{volumeUuid}/actions。这样在仅传 volumeUuids 时，路径变量无法可靠组装，SDK 合约前后不一致。建议统一为不依赖单个 volumeUuid 的路由（或拆分单卷/批量两个 Action，保持各自路径与参数契约一致）。
🤖 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 REST path in PrimaryStorageMigrateVolumeAction still
embeds a single volumeUuid while the class now accepts both volumeUuid and
volumeUuids, causing batch calls to fail; update the getRestInfo()
implementation to use a non-instance path that does not require the {volumeUuid}
path variable (e.g. switch to a collection-level route like
/primary-storage/volumes/actions) and ensure request serialization uses
volumeUuids when provided (or alternatively split into two Actions: one that
keeps volumeUuid and its current path and one new
BatchPrimaryStorageMigrateVolumesAction that uses volumeUuids and a collection
route); modify getRestInfo() and any routing-related metadata accordingly to
make the parameter/route contract consistent with the volumeUuid and volumeUuids
fields.
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-10831
Change-Id: Ie3fb7043450a35f6833eb937d4fd157d7fe4cbde
sync from gitlab !9906