<refactor>[core]: add immutable support for @GlobalProperty List fields#3501
Open
MatheMatrix wants to merge 1 commit into5.5.12from
Open
<refactor>[core]: add immutable support for @GlobalProperty List fields#3501MatheMatrix wants to merge 1 commit into5.5.12from
MatheMatrix wants to merge 1 commit into5.5.12from
Conversation
Walkthrough为全局属性加入不可变性支持：扩展 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/src/test/java/org/zstack/core/property/TestGlobalPropertyImmutableListGate.java (1)
78-86: 考虑补充 JAR classpath 扫描逻辑。当前实现仅处理
file协议的 URL，不会扫描 JAR 文件中的类。如果未来有@GlobalPropertyDefinition类打包在依赖 JAR 中，这些类将被跳过。对于当前项目内的门禁测试场景，这个限制是可以接受的，因为被测类通常位于
target/classes目录下。如果需要更完整的覆盖，可以考虑后续扩展支持jar协议。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/org/zstack/core/property/TestGlobalPropertyImmutableListGate.java` around lines 78 - 86, The current resource loop only handles "file" protocol and skips JARs; update the code in TestGlobalPropertyImmutableListGate where the resources Enumeration is iterated (the while (resources.hasMoreElements()) { ... } block) to also handle "jar" protocol URLs: when resource.getProtocol() is "jar" (or resource.toString() contains "!/"), open a JarURLConnection or new JarFile from the URL, iterate its JarEntries, filter entries by the package path (convert pkg to path with '/' suffix), for each matching .class entry derive the class name, load the Class and add it to the result just like scanDirectory does; keep existing exception handling but ensure you close the JarFile/connection resources.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@core/src/test/java/org/zstack/core/property/TestGlobalPropertyImmutableListGate.java`:
- Around line 78-86: The current resource loop only handles "file" protocol and
skips JARs; update the code in TestGlobalPropertyImmutableListGate where the
resources Enumeration is iterated (the while (resources.hasMoreElements()) { ...
} block) to also handle "jar" protocol URLs: when resource.getProtocol() is
"jar" (or resource.toString() contains "!/"), open a JarURLConnection or new
JarFile from the URL, iterate its JarEntries, filter entries by the package path
(convert pkg to path with '/' suffix), for each matching .class entry derive the
class name, load the Class and add it to the result just like scanDirectory
does; keep existing exception handling but ensure you close the
JarFile/connection resources.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: 25e471c6-5305-4c8c-a2da-63f5812ea177
📒 Files selected for processing (10)
console/src/main/java/org/zstack/console/ConsoleGlobalProperty.javacore/src/main/java/org/zstack/core/GlobalProperty.javacore/src/main/java/org/zstack/core/Platform.javacore/src/test/java/org/zstack/core/property/TestGlobalPropertyImmutableListGate.javaplugin/ceph/src/main/java/org/zstack/storage/ceph/CephGlobalProperty.javaplugin/flatNetworkProvider/src/main/java/org/zstack/network/service/flat/FlatNetworkServiceValidator.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMGlobalProperty.javaplugin/sftpBackupStorage/src/main/java/org/zstack/storage/backup/sftp/SftpBackupStorageGlobalProperty.javaplugin/virtualRouterProvider/src/main/java/org/zstack/network/service/virtualrouter/VirtualRouterGlobalProperty.javaplugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsGlobalProperty.java
Prevent accidental mutation of static List fields populated by the GlobalProperty framework. Add immutable attribute to @globalproperty annotation; when true, Platform wraps the injected List with Collections.unmodifiableList() at initialization time. Mark all 9 read-only List GlobalProperty fields as immutable=true. Add TestGlobalPropertyImmutableListGate gate test that fails if any new @globalproperty List field is added without immutable=true. Test: TestGlobalPropertyImmutableListGate (pass) Change-Id: Iaa981bce28035f710d15dd8bb1e11d5fa8508986 Co-Authored-By: Claude Opus 4.6 (1M context) <ye.zou@zstack.io>
MatheMatrix
force-pushed
the
sync/ye.zou/refactor/cleanup-orphan-vos-static-collections
branch
from
9e9981c to
5932935
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
core/src/test/java/org/zstack/core/property/TestGlobalPropertyImmutableListGate.java (2)
112-114: 捕获Throwable范围过广此处捕获
Throwable会吞掉Error类型的异常（如OutOfMemoryError、StackOverflowError），这些严重错误应当向上传播。建议改为捕获Exception或更具体的异常类型。♻️ 建议修改
- } catch (Throwable e) { + } catch (Exception e) { // skip classes that can't be loaded }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/org/zstack/core/property/TestGlobalPropertyImmutableListGate.java` around lines 112 - 114, The test currently catches Throwable in the class-loading loop which also swallows Error subclasses; update the catch to only catch Exception (or a narrower checked exception like ClassNotFoundException) in the try/catch inside TestGlobalPropertyImmutableListGate where classes are being loaded so that serious Errors (e.g., OutOfMemoryError) will propagate; keep the existing skip/ignore behavior for recoverable exceptions and ensure any logging or comments remain appropriate.
70-90: 类路径扫描仅处理file协议当前实现只扫描文件系统中的类（第 80 行），不处理 JAR 包中的类。对于单元测试场景，扫描编译输出目录是足够的，但如果将来需要扫描依赖 JAR 中的类，需要扩展此逻辑。
如果这是预期行为，建议在注释中明确说明只扫描项目编译输出目录。
📝 建议添加说明性注释
private List<Class<?>> findGlobalPropertyDefinitionClasses() { List<Class<?>> result = new ArrayList<>(); String[] basePackages = {"org.zstack"}; for (String pkg : basePackages) { String path = pkg.replace('.', '/'); try { Enumeration<URL> resources = Thread.currentThread().getContextClassLoader().getResources(path); while (resources.hasMoreElements()) { URL resource = resources.nextElement(); + // Only scan file-based classpath entries (compiled classes in target/classes). + // JAR dependencies are intentionally skipped as we only validate project sources. if ("file".equals(resource.getProtocol())) { scanDirectory(new File(resource.toURI()), pkg, result); } } } catch (Exception e) { // skip unresolvable classpath entries } } return result; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/org/zstack/core/property/TestGlobalPropertyImmutableListGate.java` around lines 70 - 90, The current findGlobalPropertyDefinitionClasses() only handles "file" protocol and therefore skips classes inside JARs; either add a clear comment above findGlobalPropertyDefinitionClasses() stating it intentionally only scans project compile-output (filesystem) or implement JAR support: detect resource.getProtocol() == "jar" (or open a JarURLConnection for "file" paths that reference JARs), iterate JarFile entries filtering by the package path, convert matching entries to class names and add them to result (extract the shared scanning logic into a new scanJar(...) method similar to scanDirectory(...) and reuse scanDirectory/scanJar from findGlobalPropertyDefinitionClasses()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@core/src/test/java/org/zstack/core/property/TestGlobalPropertyImmutableListGate.java`:
- Around line 112-114: The test currently catches Throwable in the class-loading
loop which also swallows Error subclasses; update the catch to only catch
Exception (or a narrower checked exception like ClassNotFoundException) in the
try/catch inside TestGlobalPropertyImmutableListGate where classes are being
loaded so that serious Errors (e.g., OutOfMemoryError) will propagate; keep the
existing skip/ignore behavior for recoverable exceptions and ensure any logging
or comments remain appropriate.
- Around line 70-90: The current findGlobalPropertyDefinitionClasses() only
handles "file" protocol and therefore skips classes inside JARs; either add a
clear comment above findGlobalPropertyDefinitionClasses() stating it
intentionally only scans project compile-output (filesystem) or implement JAR
support: detect resource.getProtocol() == "jar" (or open a JarURLConnection for
"file" paths that reference JARs), iterate JarFile entries filtering by the
package path, convert matching entries to class names and add them to result
(extract the shared scanning logic into a new scanJar(...) method similar to
scanDirectory(...) and reuse scanDirectory/scanJar from
findGlobalPropertyDefinitionClasses()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: 6cf18056-01f6-4b73-a816-9977f2676b6e
📒 Files selected for processing (10)
console/src/main/java/org/zstack/console/ConsoleGlobalProperty.javacore/src/main/java/org/zstack/core/GlobalProperty.javacore/src/main/java/org/zstack/core/Platform.javacore/src/test/java/org/zstack/core/property/TestGlobalPropertyImmutableListGate.javaplugin/ceph/src/main/java/org/zstack/storage/ceph/CephGlobalProperty.javaplugin/flatNetworkProvider/src/main/java/org/zstack/network/service/flat/FlatNetworkServiceValidator.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMGlobalProperty.javaplugin/sftpBackupStorage/src/main/java/org/zstack/storage/backup/sftp/SftpBackupStorageGlobalProperty.javaplugin/virtualRouterProvider/src/main/java/org/zstack/network/service/virtualrouter/VirtualRouterGlobalProperty.javaplugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsGlobalProperty.java
✅ Files skipped from review due to trivial changes (1)
- plugin/ceph/src/main/java/org/zstack/storage/ceph/CephGlobalProperty.java
🚧 Files skipped from review as they are similar to previous changes (3)
- core/src/main/java/org/zstack/core/Platform.java
- plugin/virtualRouterProvider/src/main/java/org/zstack/network/service/virtualrouter/VirtualRouterGlobalProperty.java
- console/src/main/java/org/zstack/console/ConsoleGlobalProperty.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
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.
Summary
@GlobalProperty注解新增immutable属性，List 字段标记immutable=true后框架自动 wrapCollections.unmodifiableList()FlatNetworkServiceValidator.methods改为static final+ unmodifiableListTestGlobalPropertyImmutableListGate：扫描全部@GlobalPropertyList 字段，未标immutable=true且不在白名单的直接失败CHRONY_SERVERS（ZOpsManagerImpl 运行时重赋值）、SERVER_IPS（集群成员变更）改动文件 (10)
GlobalProperty.javaboolean immutable() default falsePlatform.javaFlatNetworkServiceValidator.javastatic final+ unmodifiableListKVMGlobalProperty.javaVirtualRouterGlobalProperty.javaSftpBackupStorageGlobalProperty.javaZbsGlobalProperty.javaConsoleGlobalProperty.javaCephGlobalProperty.javaTestGlobalPropertyImmutableListGate.javaTest plan
mvn compile全量编译通过 (JDK 8)TestGlobalPropertyImmutableListGate测试通过🤖 Generated with Claude Code
sync from gitlab !9360