<fix>[storage]: filter non-preferred backup storage types before sorting#3395
<fix>[storage]: filter non-preferred backup storage types before sorting#3395zstack-robot-2 wants to merge 1 commit into5.5.12from
Conversation
indexOf() returns -1 for non-preferred types, causing them to sort before preferred ones. Filter by preferBsTypes first and return error when no matching backup storage available. Use dedicated error code ORG_ZSTACK_STORAGE_ADDON_PRIMARY_10015 for the new "no matching backup storage" condition, with i18n entries in all 10 locales. Related: ZSTAC-71706 Change-Id: Ia3af38cc50e69132a1c769180792363495c1080f
总体概览外部主存储备份存储选择逻辑重构，引入专用的过滤排序方法以优先处理首选类型，并添加相应的错误处理和验证测试。同时补充新的错误代码常量。 变更清单
代码审查工作量估算🎯 2 (简单) | ⏱️ ~12 分钟 诗
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.41.0)utils/src/main/java/org/zstack/utils/clouderrorcode/CloudOperationsErrorCode.javaComment |
|
Comment from yaohua.wu: Review: MR !9243 — ZSTAC-71706
业务背景
Critical无 Warning
Suggestion
亮点
Verdict: APPROVED修复正确地解决了根因问题，方案稳健，测试充分。仅有 CRLF 行尾问题建议修复后合并。 🤖 Robot Reviewer |
![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
`@storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java`:
- Around line 527-534: The method filterAndSortByPreferredTypes is
package-private but is used from a test in a different package; make it public
(change its signature to public static List<BackupStorageVO>
filterAndSortByPreferredTypes(...)) and add a short javadoc noting it’s exposed
for tests/tools, or alternatively move the
ExternalPrimaryStorageSelectBackupStorageCase test into the same package as
ExternalPrimaryStorage (org.zstack.storage.addon.primary) so package-private
visibility is valid.
ℹ️ Review info
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
conf/i18n/globalErrorCodeMapping/global-error-de-DE.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-en_US.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-fr-FR.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-id-ID.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-ja-JP.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-ko-KR.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-ru-RU.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-th-TH.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-zh_CN.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-zh_TW.jsonis excluded by!**/*.json
📒 Files selected for processing (3)
storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.javatest/src/test/groovy/org/zstack/test/integration/storage/primary/addon/ExternalPrimaryStorageSelectBackupStorageCase.groovyutils/src/main/java/org/zstack/utils/clouderrorcode/CloudOperationsErrorCode.java
| // package-visible for testing | ||
| static List<BackupStorageVO> filterAndSortByPreferredTypes( | ||
| List<BackupStorageVO> candidates, List<String> preferBsTypes) { | ||
| return candidates.stream() | ||
| .filter(bs -> preferBsTypes.contains(bs.getType())) | ||
| .sorted(Comparator.comparingInt(o -> preferBsTypes.indexOf(o.getType()))) | ||
| .collect(Collectors.toList()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ExternalPrimaryStorage.java package + method signature =="
sed -n '1,5p;525,535p' storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java
echo
echo "== Test package + call site =="
sed -n '1,8p;60,66p' test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/ExternalPrimaryStorageSelectBackupStorageCase.groovy
echo
echo "== All call sites =="
rg -n --type=java --type=groovy 'filterAndSortByPreferredTypes\s*\('Repository: MatheMatrix/zstack
Length of output: 3335
将 filterAndSortByPreferredTypes 改为 public，或将测试类移到同包。
方法 filterAndSortByPreferredTypes 目前是包级可见（无访问修饰符），但测试类在不同包 org.zstack.test.integration.storage.primary.addon 中调用它。根据 Java 可见性规则，包级可见方法不应被不同包中的类访问。
虽然 Groovy 的运行时反射机制可能允许此类访问，但这违反了 Java 封装原则，且易导致维护问题。建议：
- 改为
public static（添加注释说明仅供测试和工具方法使用），或 - 将 ExternalPrimaryStorageSelectBackupStorageCase 测试类移至
org.zstack.storage.addon.primary.test或同包位置。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java`
around lines 527 - 534, The method filterAndSortByPreferredTypes is
package-private but is used from a test in a different package; make it public
(change its signature to public static List<BackupStorageVO>
filterAndSortByPreferredTypes(...)) and add a short javadoc noting it’s exposed
for tests/tools, or alternatively move the
ExternalPrimaryStorageSelectBackupStorageCase test into the same package as
ExternalPrimaryStorage (org.zstack.storage.addon.primary) so package-private
visibility is valid.
3 participants
ZSTAC-71706
indexOf() returns -1 for non-preferred backup storage types, causing them to sort before preferred ones. Fix: filter by preferBsTypes first, then sort. Added dedicated error code ORG_ZSTACK_STORAGE_ADDON_PRIMARY_10015 with i18n entries in all 10 locales.
Related: ZSTAC-71706
sync from gitlab !9243