<fix>[vm]: <description#3453
Open
ZStack-Robot wants to merge 1 commit intofeature-zsv-5.0.0-zmigratefrom
Open
Conversation
MatheMatrix
force-pushed
the
sync/tao.gan/zmigrate-imagestore-ZSV-1000@@3
branch
2 times, most recently
from
ae4446d to
304917a
Compare
Walkthrough将 Ceph 备份存储中的 URL、SSH 用户名和远程路径校验逻辑从 Changes
Sequence Diagram(s)（本次变更为校验逻辑集中与调用点调整，未引入新的跨组件顺序流程，故省略序列图。） Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
MatheMatrix
force-pushed
the
sync/tao.gan/zmigrate-imagestore-ZSV-1000@@3
branch
from
304917a to
36509a3
Compare
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@utils/src/main/java/org/zstack/utils/path/RemotePathValidator.java`:
- Around line 70-85: validateUrlScheme currently throws NPE for null input, uses
upload-specific wording, and returns a scheme with original casing that can
mismatch agent expectations; fix by first null/empty-checking the url (trim and
if null/empty return a generic error like "URL [%s] is missing or empty"), then
try new URI(url) catching URISyntaxException and returning "failed to parse URL
[%s]: %s", normalize the extracted scheme to lower-case before validating
against ALLOWED_URL_SCHEMES (use scheme = scheme.toLowerCase(Locale.ROOT)), and
change any "upload URL" wording in validateUrlScheme (and ensure
extractUrlScheme returns the lower-cased scheme) so messages are generic to both
upload/download and consistent with agent-side lower-case comparisons.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f8da8936-6941-4ea1-8af7-3b9d0b40ca8d
📒 Files selected for processing (2)
plugin/ceph/src/main/java/org/zstack/storage/ceph/backup/CephBackupStorageBase.javautils/src/main/java/org/zstack/utils/path/RemotePathValidator.java
MatheMatrix
force-pushed
the
sync/tao.gan/zmigrate-imagestore-ZSV-1000@@3
branch
from
36509a3 to
f01a99e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
utils/src/main/java/org/zstack/utils/path/RemotePathValidator.java (1)
108-114: 可选优化：null 值时的错误消息格式当
username为null时，错误消息会显示"SSH username [null] is invalid...", 建议分离处理以提供更清晰的错误提示。♻️ 可选改进
public static String validateSshUsername(String username) { - if (username == null || !SSH_USERNAME_PATTERN.matcher(username).matches()) { - return String.format("SSH username [%s] is invalid, only alphanumeric characters, dots, hyphens, underscores and trailing dollar sign are allowed", - username); + if (username == null || username.isEmpty()) { + return "SSH username cannot be null or empty"; + } + if (!SSH_USERNAME_PATTERN.matcher(username).matches()) { + return String.format("SSH username [%s] is invalid, only alphanumeric characters, dots, hyphens, underscores and trailing dollar sign are allowed", + username); } return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/src/main/java/org/zstack/utils/path/RemotePathValidator.java` around lines 108 - 114, The validateSshUsername method currently embeds the literal "null" in its error message when username is null; update validateSshUsername to handle the null case separately (check username == null first) and return a clearer message like "SSH username is required" or similar, otherwise keep the existing pattern check using SSH_USERNAME_PATTERN and the current formatted message for non-null invalid values; reference validateSshUsername and SSH_USERNAME_PATTERN when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@utils/src/main/java/org/zstack/utils/path/RemotePathValidator.java`:
- Around line 108-114: The validateSshUsername method currently embeds the
literal "null" in its error message when username is null; update
validateSshUsername to handle the null case separately (check username == null
first) and return a clearer message like "SSH username is required" or similar,
otherwise keep the existing pattern check using SSH_USERNAME_PATTERN and the
current formatted message for non-null invalid values; reference
validateSshUsername and SSH_USERNAME_PATTERN when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 10d105ed-9f86-461e-ad64-3e8e75dc3d5d
📒 Files selected for processing (2)
plugin/ceph/src/main/java/org/zstack/storage/ceph/backup/CephBackupStorageBase.javautils/src/main/java/org/zstack/utils/path/RemotePathValidator.java
Resolves: ZSV-11449 Change-Id: I616767657867766b7170767463756e7777626574
MatheMatrix
force-pushed
the
sync/tao.gan/zmigrate-imagestore-ZSV-1000@@3
branch
from
f01a99e to
4c20fbc
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
utils/src/main/java/org/zstack/utils/path/RemotePathValidator.java (2)
85-85: 建议统一使用Locale.ROOT进行大小写转换。
validateUrlScheme中使用scheme.toLowerCase()而extractUrlScheme中使用scheme.toLowerCase(Locale.ROOT)，存在不一致。虽然对 ASCII 字符（http/https/ftp/sftp）不会产生实际影响，但为了一致性和避免在某些语言环境下的潜在问题（如土耳其语），建议统一使用Locale.ROOT。♻️ 建议修改
- if (!ALLOWED_URL_SCHEMES.contains(scheme.toLowerCase())) { + if (!ALLOWED_URL_SCHEMES.contains(scheme.toLowerCase(Locale.ROOT))) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/src/main/java/org/zstack/utils/path/RemotePathValidator.java` at line 85, The code uses different locale-aware lowercasing: replace the call to scheme.toLowerCase() in validateUrlScheme with scheme.toLowerCase(Locale.ROOT) so it matches extractUrlScheme and avoids locale-dependent behavior; update any comparisons against ALLOWED_URL_SCHEMES to use the normalized value (e.g., lowercased with Locale.ROOT) to ensure consistency between validateUrlScheme and extractUrlScheme.
108-114: 实现正确，null 显示可优化。验证逻辑正确。当
username为null时，错误信息会显示为"SSH username [null] is invalid"，虽然功能上没有问题，但可以考虑优化显示效果。♻️ 可选优化
public static String validateSshUsername(String username) { - if (username == null || !SSH_USERNAME_PATTERN.matcher(username).matches()) { - return String.format("SSH username [%s] is invalid, only alphanumeric characters, dots, hyphens, underscores and trailing dollar sign are allowed", - username); + if (username == null) { + return "SSH username cannot be null"; + } + if (!SSH_USERNAME_PATTERN.matcher(username).matches()) { + return String.format("SSH username [%s] is invalid, only alphanumeric characters, dots, hyphens, underscores and trailing dollar sign are allowed", username); } return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/src/main/java/org/zstack/utils/path/RemotePathValidator.java` around lines 108 - 114, The error message in validateSshUsername currently prints "null" when username is null; update the formatting to display a clearer value (e.g. empty string or placeholder) instead of the literal "null". In the validateSshUsername method, replace the current String.format usage with a safer conversion (for example using Objects.toString(username, "") or a ternary username == null ? "<empty>" : username) so the log shows a friendly placeholder rather than "null" while keeping the rest of the validation and SSH_USERNAME_PATTERN logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@utils/src/main/java/org/zstack/utils/path/RemotePathValidator.java`:
- Line 85: The code uses different locale-aware lowercasing: replace the call to
scheme.toLowerCase() in validateUrlScheme with scheme.toLowerCase(Locale.ROOT)
so it matches extractUrlScheme and avoids locale-dependent behavior; update any
comparisons against ALLOWED_URL_SCHEMES to use the normalized value (e.g.,
lowercased with Locale.ROOT) to ensure consistency between validateUrlScheme and
extractUrlScheme.
- Around line 108-114: The error message in validateSshUsername currently prints
"null" when username is null; update the formatting to display a clearer value
(e.g. empty string or placeholder) instead of the literal "null". In the
validateSshUsername method, replace the current String.format usage with a safer
conversion (for example using Objects.toString(username, "") or a ternary
username == null ? "<empty>" : username) so the log shows a friendly placeholder
rather than "null" while keeping the rest of the validation and
SSH_USERNAME_PATTERN logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f5a7ceab-818b-4312-bb4a-165c52d7d987
📒 Files selected for processing (2)
plugin/ceph/src/main/java/org/zstack/storage/ceph/backup/CephBackupStorageBase.javautils/src/main/java/org/zstack/utils/path/RemotePathValidator.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
APIImpact
Resolves: ZSV-1000
Change-Id: I616767657867766b7170767463756e7777626574
sync from gitlab !9311