Allow env variable substitution in CLI client properties

[VillePekka]

This is draft pull request. For some reason MirthServletTest fails when I build this. But also when I build the main branch the same test fails, so something might be wrong with my computer. The functionality still works.

Testing the functionality on Linux:

1. In mirth-cli-config.properties file, define for example password as password=${env:OIE_PASSWORD}
2. Define the environment variable in commandline export OIE_PASSWORD=<password>
3. Run cli client ~/engine/server/setup$ java -jar mirth-cli-launcher.jar -c ../../command/conf/mirth-cli-config.properties

[Copilot]

Pull request overview

This pull request adds environment variable substitution support to the CLI client properties file by replacing the standard Java Properties class with Apache Commons Configuration2's Configuration interface. This allows users to specify property values like password=${env:OIE_PASSWORD} in the configuration file, which will be resolved from environment variables at runtime.

Changes:

• Replaced Properties with Apache Commons Configuration2's Configuration for loading CLI configuration
• Added new imports for Configuration2 classes
• Modified configuration loading logic to use FileBasedConfigurationBuilder instead of direct file reading

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The config variable is declared but may be null when accessed. If the ConfigurationException is caught (line 166), config remains null, but it's then used unconditionally on lines 175-178 which will cause a NullPointerException. The config variable should be checked for null before use, or the error handling flow should exit the method after handling the exception.

[Copilot]

Inconsistent indentation detected. Lines 109-113 and 120-191 use inconsistent indentation compared to the rest of the file. The opening brace and method body should align with standard Java formatting (typically 4 spaces per indent level).

[Copilot]

The FileBasedConfigurationBuilder is created with a generic type parameter that specifies FileBasedConfiguration, but then instantiated with PropertiesConfiguration.class. While this works due to inheritance, it's clearer and more type-safe to use PropertiesConfiguration as the type parameter to match the actual class being instantiated.

Suggested change

	FileBasedConfigurationBuilder<FileBasedConfiguration> builder =
	new FileBasedConfigurationBuilder<FileBasedConfiguration>(PropertiesConfiguration.class)
	FileBasedConfigurationBuilder<PropertiesConfiguration> builder =
	new FileBasedConfigurationBuilder<PropertiesConfiguration>(PropertiesConfiguration.class)

[Copilot]

Missing documentation. The new configuration loading mechanism using Apache Commons Configuration2 should be documented with comments explaining that it supports environment variable substitution (e.g., using ${env:VARIABLE_NAME} syntax). This is the key feature of this PR but there's no code comment explaining this capability to future developers.

Suggested change

	config = builder.getConfiguration();
	// config contains all properties read from the file
	// Load properties using Apache Commons Configuration2. The resulting config
	// instance supports variable interpolation, including environment variables
	// using the ${env:VARIABLE_NAME} syntax (for example ${env:MC_SERVER_URL}).
	config = builder.getConfiguration();

[Copilot]

The try-catch block formatting is inconsistent with Java conventions. The opening brace after 'try' should be on the same line (line 161), and 'catch' should be on the same line as the closing brace of the try block (line 166). Currently the formatting deviates from standard Java style.

[Copilot]

Redundant comment on line 164 states "config contains all properties read from the file" but provides no additional value beyond what's obvious from the code. Consider removing this comment or replacing it with documentation about the environment variable substitution feature that this change enables.

Suggested change

	// config contains all properties read from the file
	// Configuration values loaded here support property interpolation (including environment variable substitution).

[Copilot]

Unclear error message. The comment on line 168 says "loading of the configuration file failed" but the actual implementation only shows this error if line.hasOption("c") is true. This means if the default config file doesn't exist and the user didn't specify -c, the error is silently ignored. This behavior should be clarified in the error handling or the error message should be updated to reflect when it actually triggers.

Suggested change

	error("We could not find the file: " + line.getOptionValue("c"), null);
	System.exit(2);
	// An explicit configuration file was specified with -c; fail fast with a clear error
	error("Failed to load the configuration file specified with -c: " + line.getOptionValue("c"), cex);
	System.exit(2);
	} else {
	// No explicit config was provided; fall back to an empty configuration
	config = new PropertiesConfiguration();

[Copilot]

Inefficient comparison to empty string, check for zero length instead.

Suggested change

	if (name.equals("")) {
	if (name.length() == 0) {

[Copilot]

This check is useless. file cannot be null at this check, since new File(...) always is non-null.

Suggested change

	if (file != null && file.exists()) {
	if (file.exists()) {

[Copilot]

This check is useless. file cannot be null at this check, since new File(...) always is non-null.

Suggested change

	if (file != null) {
	try {
	PropertiesConfiguration properties = PropertiesConfigurationUtil.create(file);
	properties.clear();
	PropertiesConfigurationLayout layout = properties.getLayout();
	Map<String, ConfigurationProperty> configurationMap = client.getConfigurationMap();
	Map<String, ConfigurationProperty> sortedMap = new TreeMap<String, ConfigurationProperty>(String.CASE_INSENSITIVE_ORDER);
	sortedMap.putAll(configurationMap);
	for (Entry<String, ConfigurationProperty> entry : sortedMap.entrySet()) {
	String key = entry.getKey();
	String value = entry.getValue().getValue();
	String comment = entry.getValue().getComment();
	if (StringUtils.isNotBlank(key)) {
	properties.setProperty(key, value);
	layout.setComment(key, StringUtils.isBlank(comment) ? null : comment);
	}
	}
	PropertiesConfigurationUtil.saveTo(properties, file);
	out.println("Configuration map export complete.");
	} catch (ConfigurationException | IOException e) {
	error("Unable to export configuration map.", e);
	}
	try {
	PropertiesConfiguration properties = PropertiesConfigurationUtil.create(file);
	properties.clear();
	PropertiesConfigurationLayout layout = properties.getLayout();
	Map<String, ConfigurationProperty> configurationMap = client.getConfigurationMap();
	Map<String, ConfigurationProperty> sortedMap = new TreeMap<String, ConfigurationProperty>(String.CASE_INSENSITIVE_ORDER);
	sortedMap.putAll(configurationMap);
	for (Entry<String, ConfigurationProperty> entry : sortedMap.entrySet()) {
	String key = entry.getKey();
	String value = entry.getValue().getValue();
	String comment = entry.getValue().getComment();
	if (StringUtils.isNotBlank(key)) {
	properties.setProperty(key, value);
	layout.setComment(key, StringUtils.isBlank(comment) ? null : comment);
	}
	}
	PropertiesConfigurationUtil.saveTo(properties, file);
	out.println("Configuration map export complete.");
	} catch (ConfigurationException | IOException e) {
	error("Unable to export configuration map.", e);