Allow env variable substitution in CLI client properties #242
Conversation
There was a problem hiding this comment.
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
Propertieswith Apache Commons Configuration2'sConfigurationfor loading CLI configuration - Added new imports for Configuration2 classes
- Modified configuration loading logic to use
FileBasedConfigurationBuilderinstead of direct file reading
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| String server = line.getOptionValue("a", config.getString("address")); | ||
| String user = line.getOptionValue("u", config.getString("user")); | ||
| String password = line.getOptionValue("p", config.getString("password")); | ||
| String script = line.getOptionValue("s", config.getString("script")); |
There was a problem hiding this comment.
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.
| FileBasedConfigurationBuilder<FileBasedConfiguration> builder = | ||
| new FileBasedConfigurationBuilder<FileBasedConfiguration>(PropertiesConfiguration.class) |
There was a problem hiding this comment.
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.
| FileBasedConfigurationBuilder<FileBasedConfiguration> builder = | |
| new FileBasedConfigurationBuilder<FileBasedConfiguration>(PropertiesConfiguration.class) | |
| FileBasedConfigurationBuilder<PropertiesConfiguration> builder = | |
| new FileBasedConfigurationBuilder<PropertiesConfiguration>(PropertiesConfiguration.class) |
| try | ||
| { | ||
| config = builder.getConfiguration(); | ||
| // config contains all properties read from the file | ||
| } | ||
| catch(ConfigurationException cex) | ||
| { | ||
| // loading of the configuration file failed | ||
| if (line.hasOption("c")) { | ||
| error("We could not find the file: " + line.getOptionValue("c"), null); | ||
| System.exit(2); | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I wouldn't worry about these copilot suggestions that are not related to your PR.
| error("We could not find the file: " + line.getOptionValue("c"), null); | ||
| System.exit(2); |
There was a problem hiding this comment.
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.
| 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(); |
…variables in config file. Signed-off-by: Ville Ranta-aho <ville@ville-pekka.com>
Signed-off-by: Ville Ranta-aho <ville@ville-pekka.com>
Documentation by copilot. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: VillePekka <ville@ville-pekka.com> Signed-off-by: Ville Ranta-aho <ville@ville-pekka.com>
Signed-off-by: Ville Ranta-aho <ville@ville-pekka.com>
Signed-off-by: Ville Ranta-aho <ville@ville-pekka.com>
Signed-off-by: Ville Ranta-aho <ville@ville-pekka.com>
…type parameter. Signed-off-by: Ville Ranta-aho <ville@ville-pekka.com>
2 participants
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:
password=${env:OIE_PASSWORD}export OIE_PASSWORD=<password>~/engine/server/setup$ java -jar mirth-cli-launcher.jar -c ../../command/conf/mirth-cli-config.properties