<fix>[conf]: Modify the name of the properties file #3178
Conversation
Walkthrough引入 Changes
Sequence Diagram(s)（无） Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 诗
🚥 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
📜 Recent review detailsConfiguration 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 (4)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🔍 Remote MCP AtlassianMCPAdditional context relevant to reviewing this PR
Sources:
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: 4
🤖 Fix all issues with AI agents
In @core/src/main/java/org/zstack/core/config/AppConfig.java:
- Around line 41-60: Remove the redundant bootstrap loader by deleting
Platform.loadPropertiesFileNameFromBootstrap() and any calls to it, and ensure
Platform's static initialization uses AppConfig.getPropertiesFileName()
exclusively; specifically, keep AppConfig.loadPropertiesFileNameFromConfig()
(invoked via AppConfig.getPropertiesFileName()), remove the duplicate method in
Platform, and update Platform's static block to reference
AppConfig.getPropertiesFileName() if it does not already, eliminating dead code.
- Around line 48-53: The code in AppConfig that reads the propertiesFile node
assigns fileName = nodes.item(0).getTextContent().trim() without checking for an
empty string; add a check after trimming to verify fileName.isEmpty() (or
length()==0) and handle it (e.g., skip returning it and continue searching or
log a warning and return null) instead of proceeding to use an empty filename;
update the block around nodes, fileName and the System.out.println call to only
print/return when fileName is non-empty.
- Around line 44-46: AppConfig currently creates a DocumentBuilderFactory and
parses appConfigFile without disabling XML external entity handling; update the
DocumentBuilderFactory configuration before calling factory.newDocumentBuilder()
to mitigate XXE: enable XMLConstants.FEATURE_SECURE_PROCESSING, set
"http://apache.org/xml/features/disallow-doctype-decl" to true, set
"http://xml.org/sax/features/external-general-entities" and
"http://xml.org/sax/features/external-parameter-entities" to false, set
XIncludeAware to false and setAttribute for
"http://javax.xml.XMLConstants/property/accessExternalDTD" and
"http://javax.xml.XMLConstants/property/accessExternalSchema" to an empty
string; then create the DocumentBuilder and call builder.parse(appConfigFile) as
before.
In @core/src/main/java/org/zstack/core/Platform.java:
- Around line 280-303: The method loadPropertiesFileNameFromBootstrap() is dead,
duplicates AppConfig.loadPropertiesFileNameFromConfig(), contains an XXE risk,
and logs the wrong filename; remove the entire method and any unused imports it
brought in, update any references (the static initialization already uses
AppConfig.getPropertiesFileName()), and ensure no callers remain; also remove or
correct the misleading logger text ("bootstrap.xml") if duplicated elsewhere.
🧹 Nitpick comments (4)
core/src/main/java/org/zstack/core/config/AppConfig.java (2)
51-56: 应使用日志框架而非System.out/System.err当前使用
System.out.println和System.err.println进行日志输出，这与代码库中其他地方使用CLogger的做法不一致，且不利于日志级别控制和生产环境的日志管理。🔎 建议使用 CLogger
+import org.zstack.utils.Utils; +import org.zstack.utils.logging.CLogger; + public class AppConfig { + private static final CLogger logger = Utils.getLogger(AppConfig.class); private static final String DEFAULT_PROPERTIES_FILE = "zstack.properties"; private static volatile String propertiesFileName = null;if (nodes.getLength() > 0) { String fileName = nodes.item(0).getTextContent().trim(); - System.out.println("[AppConfig] Using properties file: " + fileName); + logger.info(String.format("Using properties file: %s", fileName)); return fileName; } } catch (Exception e) { - System.err.println("[AppConfig] Failed to load app_config.xml, using default: " + DEFAULT_PROPERTIES_FILE); - e.printStackTrace(); + logger.warn(String.format("Failed to load app_config.xml, using default: %s", DEFAULT_PROPERTIES_FILE), e); }
62-67:reset()方法的线程安全性说明
reset()方法用于测试目的，但在并发场景下调用可能导致竞态条件。建议在方法注释中明确说明此方法仅应在单线程测试环境中使用。🔎 建议增强注释
/** * Reset cached value (mainly for testing) + * WARNING: This method is NOT thread-safe and should only be called in single-threaded test environments */ public static void reset() { + synchronized (AppConfig.class) { propertiesFileName = null; + } }test/src/test/java/org/zstack/test/DBUtil.java (1)
41-41: 错误信息中仍硬编码 "zstack.properties"错误信息引用了硬编码的 "zstack.properties"，但实际加载的属性文件可能是其他名称。建议使用动态获取的文件名或通用描述。
🔎 建议修改
if (user == null) { - throw new CloudRuntimeException("cannot find DB user in zstack.properties, please set either DB.user or DbFacadeDataSource.user"); + throw new CloudRuntimeException(String.format("cannot find DB user in %s, please set either DB.user or DbFacadeDataSource.user", AppConfig.getPropertiesFileName())); }Line 52 同样需要修改：
if (password == null) { - throw new CloudRuntimeException("cannot find DB user in zstack.properties, please set either DB.password or DbFacadeDataSource.password"); + throw new CloudRuntimeException(String.format("cannot find DB password in %s, please set either DB.password or DbFacadeDataSource.password", AppConfig.getPropertiesFileName())); }core/src/main/java/org/zstack/core/Platform.java (1)
56-59: 可能存在未使用的导入如果移除
loadPropertiesFileNameFromBootstrap()方法（该方法与AppConfig功能重复），则这些 XML 解析相关的导入将不再需要。🔎 如果移除冗余方法，请同时删除以下导入
-import javax.xml.parsers.DocumentBuilder; -import javax.xml.parsers.DocumentBuilderFactory; -import org.w3c.dom.Document; -import org.w3c.dom.NodeList;
📜 Review details
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 (4)
conf/app_config.xmlis excluded by!**/*.xmlconf/zstack.xmlis excluded by!**/*.xmlconf/zstackSimulator.xmlis excluded by!**/*.xmltest/src/test/resources/zstack-template.xmlis excluded by!**/*.xml
📒 Files selected for processing (3)
core/src/main/java/org/zstack/core/Platform.javacore/src/main/java/org/zstack/core/config/AppConfig.javatest/src/test/java/org/zstack/test/DBUtil.java
🧰 Additional context used
📓 Path-based instructions (2)
**/*.*
⚙️ CodeRabbit configuration file
**/*.*: - 代码里不应当有有中文，包括报错、注释等都应当使用正确的、无拼写错误的英文来写
Files:
core/src/main/java/org/zstack/core/config/AppConfig.javatest/src/test/java/org/zstack/test/DBUtil.javacore/src/main/java/org/zstack/core/Platform.java
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: ## 1. API 设计要求
- API 命名：
- API 名称必须唯一，不能重复。
- API 消息类需要继承
APIMessage；其返回类必须继承APIReply或APIEvent，并在注释中用@RestResponse进行标注。- API 消息上必须添加注解
@RestRequest，并满足如下规范：
path:
- 针对资源使用复数形式。
- 当 path 中引用消息类变量时，使用
{variableName}格式。- HTTP 方法对应：
- 查询操作 →
HttpMethod.GET- 更新操作 →
HttpMethod.PUT- 创建操作 →
HttpMethod.POST- 删除操作 →
HttpMethod.DELETE- API 类需要实现
__example__方法以便生成 API 文档，并确保生成对应的 Groovy API Template 与 API Markdown 文件。
2. 命名与格式规范
类名：
- 使用 UpperCamelCase 风格。
- 特殊情况：
- VO/AO/EO 类型类除外。
- 抽象类采用
Abstract或Base前缀/后缀。- 异常类应以
Exception结尾。- 测试类需要以
Test或Case结尾。方法名、参数名、成员变量和局部变量：
- 使用 lowerCamelCase 风格。
常量命名：
- 全部大写，使用下划线分隔单词。
- 要求表达清楚，避免使用含糊或不准确的名称。
包名：
- 统一使用小写，使用点分隔符，每个部分应是一个具有自然语义的英文单词（参考 Spring 框架的结构）。
命名细节：
- 避免在父子类或同一代码块中出现相同名字的成员或局部变量，防止混淆。
- 命名缩写：
- 不允许使用不必要的缩写，如：
AbsSchedulerJob、condi、Fu等。应使用完整单词提升可读性。
3. 编写自解释代码
意图表达：
- 避免使用布尔型参数造成含义不明确。例如：
- 对于
stopAgent(boolean ignoreError)，建议拆分为不同函数（如stopAgentIgnoreError()），或使用枚举表达操作类型。- 命名应尽量用完整的单词组合表达意图，并在名称中体现数据类型或用途（例如在常量与变量名称中，将类型词放在末尾）。
- 避免使用魔法值（Magic Value）：
直接使用未经定义的数值或字符串（如 if (status == 5)）应替换为枚举或常量。
示例：
// 错误示例：魔法值
if (user.getStatus() == 5) { ... }
// 正确示例：常量或枚举
public static final int STATUS_ACTIVE = 5;
if (user.getStatus() == STATUS_ACTIVE) { ... }
// 或使用枚举
enum UserStatus { ACTIVE, INACTIVE }
注释：
- 代码应尽量做到自解释，对少于两行的说明可以直接写在代码中。
- 对于较长的注释，需要仔细校对并随代码更新，确保内容正确。
- 接口方法不应有多余的修饰符（例如
public），且必须配有有效的 Javadoc 注释。
4. 流程控制和结构优化
if...else 的使用：
- 应尽量减少 if...else 结构的使用，建议：
- 限制嵌套层级最多为两层，且内层不应再出现
else分支。- 尽早返回（Early Return），将条件判断中的处理逻辑提前结束或抽成独立方法。
- 使用 Java Stream 或 Lambda 表达式代替冗长的循环与条件判断。
条件判断：
- if 条件表达不宜过长或过于复杂，必要时可以将条件抽成 boolean 变量描述。
代码块长度：
...
Files:
core/src/main/java/org/zstack/core/config/AppConfig.javatest/src/test/java/org/zstack/test/DBUtil.javacore/src/main/java/org/zstack/core/Platform.java
🧬 Code graph analysis (2)
test/src/test/java/org/zstack/test/DBUtil.java (1)
core/src/main/java/org/zstack/core/config/AppConfig.java (1)
AppConfig(16-68)
core/src/main/java/org/zstack/core/Platform.java (2)
core/src/main/java/org/zstack/core/config/AppConfig.java (1)
AppConfig(16-68)utils/src/main/java/org/zstack/utils/path/PathUtil.java (1)
PathUtil(20-468)
🔍 Remote MCP AtlassianMCP
Based on the search results, I now have important context about this PR. Let me provide a comprehensive summary:
Additional Context Found
Issue Background (ZSTAC-80219)
The Jira issue ZSTAC-80219 is an improvement task titled "#NexaVM 提供Cloud5.4.2的Nexavm定制OEM版本，只留英文，明显的中文信息去除，自带三个月测试许可-后端" (NexaVM providing a customized OEM version with English-only content and removal of obvious Chinese information, including a 3-month testing license for the backend).
Key Details
- Status: Open
- Priority: P2
- Type: Improvement
- Assignee: haoyu.ding
Related Context
The issue track shows that multiple related PRs have been merged across different repositories on branch fix-80219@@3 and fix-80219@@2, including changes to faulttolerance, zstackbuild, core, zstackctl, ansible, and v2v components, with titles like "use oem config" and "rename oemname to appname".
Purpose of This PR
This PR (#3178) is part of a broader effort to support OEM customization for NexaVM, specifically to allow properties file configuration to be dynamically loaded from a bootstrap configuration rather than being hard-coded. The changes enable:
- AppConfig - A new centralized configuration reader that loads the properties file name from
app_config.xml - Platform.java - Modified to use the dynamically loaded properties file name instead of hard-coded
zstack.properties - DBUtil.java - Updated to use the new AppConfig mechanism
This aligns with the broader initiative referenced in the Jira issue to create a customized OEM version where configuration can be externalized and modified without code changes.,,
🔇 Additional comments (2)
test/src/test/java/org/zstack/test/DBUtil.java (1)
5-5: LGTM!使用
AppConfig.getPropertiesFileName()替代硬编码的属性文件名，与集中配置管理方案保持一致。Also applies to: 32-32
core/src/main/java/org/zstack/core/Platform.java (1)
521-528: LGTM!正确使用
AppConfig.getPropertiesFileName()获取配置文件名，并设置系统属性供 Spring 等组件使用。实现符合 OEM 定制化需求。
| private static String loadPropertiesFileNameFromConfig() { | ||
| try { | ||
| File appConfigFile = PathUtil.findFileOnClassPath("app_config.xml", true); | ||
| DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); | ||
| DocumentBuilder builder = factory.newDocumentBuilder(); | ||
| Document doc = builder.parse(appConfigFile); | ||
|
|
||
| NodeList nodes = doc.getElementsByTagName("propertiesFile"); | ||
| if (nodes.getLength() > 0) { | ||
| String fileName = nodes.item(0).getTextContent().trim(); | ||
| System.out.println("[AppConfig] Using properties file: " + fileName); | ||
| return fileName; | ||
| } | ||
| } catch (Exception e) { | ||
| System.err.println("[AppConfig] Failed to load app_config.xml, using default: " + DEFAULT_PROPERTIES_FILE); | ||
| e.printStackTrace(); | ||
| } | ||
|
|
||
| return DEFAULT_PROPERTIES_FILE; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
代码重复：loadPropertiesFileNameFromConfig() 与 Platform.loadPropertiesFileNameFromBootstrap() 逻辑几乎完全一致
Platform.java 中的 loadPropertiesFileNameFromBootstrap() 方法（Lines 285-303）与此方法功能重复。既然已经创建了 AppConfig 作为集中配置读取器，Platform.java 应该直接调用 AppConfig.getPropertiesFileName() 而不是再实现一遍相同的逻辑。
当前 Platform.java 已经在 static block 中调用了 AppConfig.getPropertiesFileName()，但仍保留了 loadPropertiesFileNameFromBootstrap() 方法，这是死代码。
🔎 建议：移除 Platform.java 中的冗余方法
在 Platform.java 中删除 loadPropertiesFileNameFromBootstrap() 方法（Lines 280-303），因为该功能已由 AppConfig 提供。
🤖 Prompt for AI Agents
In @core/src/main/java/org/zstack/core/config/AppConfig.java around lines 41 -
60, Remove the redundant bootstrap loader by deleting
Platform.loadPropertiesFileNameFromBootstrap() and any calls to it, and ensure
Platform's static initialization uses AppConfig.getPropertiesFileName()
exclusively; specifically, keep AppConfig.loadPropertiesFileNameFromConfig()
(invoked via AppConfig.getPropertiesFileName()), remove the duplicate method in
Platform, and update Platform's static block to reference
AppConfig.getPropertiesFileName() if it does not already, eliminating dead code.
| DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); | ||
| DocumentBuilder builder = factory.newDocumentBuilder(); | ||
| Document doc = builder.parse(appConfigFile); |
There was a problem hiding this comment.
潜在的 XXE（XML 外部实体注入）漏洞
DocumentBuilderFactory 在解析 XML 时未禁用外部实体处理，可能存在 XXE 漏洞风险。虽然 app_config.xml 是内部配置文件，但建议遵循安全最佳实践。
🔎 建议添加 XXE 防护
try {
File appConfigFile = PathUtil.findFileOnClassPath("app_config.xml", true);
DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
+ // Disable XXE
+ factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
+ factory.setFeature("http://xml.org/sax/features/external-general-entities", false);
+ factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
DocumentBuilder builder = factory.newDocumentBuilder();
Document doc = builder.parse(appConfigFile);🤖 Prompt for AI Agents
In @core/src/main/java/org/zstack/core/config/AppConfig.java around lines 44 -
46, AppConfig currently creates a DocumentBuilderFactory and parses
appConfigFile without disabling XML external entity handling; update the
DocumentBuilderFactory configuration before calling factory.newDocumentBuilder()
to mitigate XXE: enable XMLConstants.FEATURE_SECURE_PROCESSING, set
"http://apache.org/xml/features/disallow-doctype-decl" to true, set
"http://xml.org/sax/features/external-general-entities" and
"http://xml.org/sax/features/external-parameter-entities" to false, set
XIncludeAware to false and setAttribute for
"http://javax.xml.XMLConstants/property/accessExternalDTD" and
"http://javax.xml.XMLConstants/property/accessExternalSchema" to an empty
string; then create the DocumentBuilder and call builder.parse(appConfigFile) as
before.
| NodeList nodes = doc.getElementsByTagName("propertiesFile"); | ||
| if (nodes.getLength() > 0) { | ||
| String fileName = nodes.item(0).getTextContent().trim(); | ||
| System.out.println("[AppConfig] Using properties file: " + fileName); | ||
| return fileName; | ||
| } |
There was a problem hiding this comment.
缺少空字符串校验
当 propertiesFile 节点存在但内容为空时，trim() 后会返回空字符串，这可能导致后续加载属性文件失败。
🔎 建议添加空字符串校验
NodeList nodes = doc.getElementsByTagName("propertiesFile");
if (nodes.getLength() > 0) {
String fileName = nodes.item(0).getTextContent().trim();
- System.out.println("[AppConfig] Using properties file: " + fileName);
- return fileName;
+ if (!fileName.isEmpty()) {
+ System.out.println("[AppConfig] Using properties file: " + fileName);
+ return fileName;
+ }
}🤖 Prompt for AI Agents
In @core/src/main/java/org/zstack/core/config/AppConfig.java around lines 48 -
53, The code in AppConfig that reads the propertiesFile node assigns fileName =
nodes.item(0).getTextContent().trim() without checking for an empty string; add
a check after trimming to verify fileName.isEmpty() (or length()==0) and handle
it (e.g., skip returning it and continue searching or log a warning and return
null) instead of proceeding to use an empty filename; update the block around
nodes, fileName and the System.out.println call to only print/return when
fileName is non-empty.
| /** | ||
| * Load properties file name from bootstrap configuration | ||
| * The bootstrap.xml will be replaced by build script for OEM customization | ||
| * @return properties file name (e.g., zstack.properties, myapp.properties) | ||
| */ | ||
| private static String loadPropertiesFileNameFromBootstrap() { | ||
| try { | ||
| File bootstrapFile = PathUtil.findFileOnClassPath("app_config.xml", true); | ||
| DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); | ||
| DocumentBuilder builder = factory.newDocumentBuilder(); | ||
| Document doc = builder.parse(bootstrapFile); | ||
|
|
||
| NodeList nodes = doc.getElementsByTagName("propertiesFile"); | ||
| if (nodes.getLength() > 0) { | ||
| String fileName = nodes.item(0).getTextContent().trim(); | ||
| logger.info(String.format("Loading properties from bootstrap config: %s", fileName)); | ||
| return fileName; | ||
| } | ||
| } catch (Exception e) { | ||
| logger.warn("Failed to load bootstrap.xml, using default zstack.properties", e); | ||
| } | ||
|
|
||
| return "zstack.properties"; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
死代码：loadPropertiesFileNameFromBootstrap() 方法未被使用
此方法从未被调用，因为 static block（Line 523）已直接使用 AppConfig.getPropertiesFileName()。此外：
- Line 299 的日志消息提及 "bootstrap.xml"，但代码实际读取的是 "app_config.xml"，存在不一致
- 此方法与
AppConfig.loadPropertiesFileNameFromConfig()逻辑重复 - 同样存在 XXE 漏洞
建议删除此方法及相关导入。
🔎 建议删除整个方法
- /**
- * Load properties file name from bootstrap configuration
- * The bootstrap.xml will be replaced by build script for OEM customization
- * @return properties file name (e.g., zstack.properties, myapp.properties)
- */
- private static String loadPropertiesFileNameFromBootstrap() {
- try {
- File bootstrapFile = PathUtil.findFileOnClassPath("app_config.xml", true);
- DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
- DocumentBuilder builder = factory.newDocumentBuilder();
- Document doc = builder.parse(bootstrapFile);
-
- NodeList nodes = doc.getElementsByTagName("propertiesFile");
- if (nodes.getLength() > 0) {
- String fileName = nodes.item(0).getTextContent().trim();
- logger.info(String.format("Loading properties from bootstrap config: %s", fileName));
- return fileName;
- }
- } catch (Exception e) {
- logger.warn("Failed to load bootstrap.xml, using default zstack.properties", e);
- }
-
- return "zstack.properties";
- }
-🤖 Prompt for AI Agents
In @core/src/main/java/org/zstack/core/Platform.java around lines 280 - 303, The
method loadPropertiesFileNameFromBootstrap() is dead, duplicates
AppConfig.loadPropertiesFileNameFromConfig(), contains an XXE risk, and logs the
wrong filename; remove the entire method and any unused imports it brought in,
update any references (the static initialization already uses
AppConfig.getPropertiesFileName()), and ensure no callers remain; also remove or
correct the misleading logger text ("bootstrap.xml") if duplicated elsewhere.
Resolves: ZSTAC-80219 Change-Id: I7a62637177726f6e61746e7763667278627a7a76
MatheMatrix
force-pushed
the
sync/haoyu.ding/fix-80219@@3
branch
from
cc54741 to
71f975b
Compare
Resolves: ZSTAC-80219
Change-Id: I7a62637177726f6e61746e7763667278627a7a76
sync from gitlab !8995