Avoid writing the keystore if no changes were made #134
Conversation
Signed-off-by: Richard Ogin <rogin@users.noreply.github.com>
rogin
force-pushed
the
avoid-keystore-write
branch
from
7ad8178 to
8b1a164
Compare
tonygermano
left a comment
tonygermano
left a comment
There was a problem hiding this comment.
I haven't tested it yet, but I'm fine with this approach.
I would like to see the deprecation of ResourceUtil.closeResourceQuietly broken out to a different PR. It's not related to this keystore change, and the current state of the PR leaves a lot of references calling the deprecated method. I wonder if in some situations we would do better to change the calls to a try-with-resource block rather than using IOUtils.
Could you also please update the javadoc for the two private methods you changed to indicate the purpose of the return value?
There was a problem hiding this comment.
Pull request overview
This PR optimizes keystore management to avoid unnecessary writes to the filesystem, enabling the use of keystores on read-only filesystems. The primary change introduces a dirtiedKeystore flag that tracks whether modifications were made to the keystore during initialization, and only writes the keystore back to disk if changes occurred. Additionally, the PR replaces deprecated ResourceUtil.closeResourceQuietly() calls with IOUtils.closeQuietly() from Apache Commons IO, and includes minor code cleanup (removing unused imports and reformatting JavaDoc).
- Tracks keystore modifications with a boolean flag to prevent unnecessary writes
- Refactors
configureEncryption()andgenerateDefaultCertificate()to return boolean values indicating if the keystore was modified - Migrates from
ResourceUtil.closeResourceQuietly()toIOUtils.closeQuietly()for resource cleanup
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| server/src/com/mirth/connect/server/util/ResourceUtil.java | Adds deprecation annotation and documentation to closeResourceQuietly() method |
| server/src/com/mirth/connect/server/controllers/DefaultConfigurationController.java | Implements conditional keystore writes using dirtiedKeystore flag; refactors configureEncryption() and generateDefaultCertificate() to return boolean modification status; replaces ResourceUtil.closeResourceQuietly() with IOUtils.closeQuietly(); removes unused import and reformats JavaDoc |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fos = new FileOutputStream(keyStoreFile); | ||
| keyStore.store(fos, keyStorePassword); | ||
| // only re-write the keystore if it was changed | ||
| if(dirtiedKeystore) { |
There was a problem hiding this comment.
There should be a space after 'if' before the opening parenthesis, following standard Java formatting conventions. The line should read 'if (dirtiedKeystore)' instead of 'if(dirtiedKeystore)'.
| if(dirtiedKeystore) { | |
| if (dirtiedKeystore) { |
|
|
||
| /** | ||
| * Checks for an existing certificate to use for secure communication between the server and | ||
| * client. If no certficate exists, this will generate a new one. |
There was a problem hiding this comment.
Typo in the documentation: 'certficate' should be 'certificate'.
| * client. If no certficate exists, this will generate a new one. | |
| * client. If no certificate exists, this will generate a new one. |
| dirtiedKeystore |= configureEncryption(provider, keyStore, keyPassword); | ||
| dirtiedKeystore |= generateDefaultCertificate(provider, keyStore, keyPassword); | ||
|
|
||
| // write the keystore back to the file | ||
| fos = new FileOutputStream(keyStoreFile); | ||
| keyStore.store(fos, keyStorePassword); | ||
| // only re-write the keystore if it was changed | ||
| if(dirtiedKeystore) { | ||
| fos = new FileOutputStream(keyStoreFile); | ||
| keyStore.store(fos, keyStorePassword); | ||
| } |
There was a problem hiding this comment.
The new conditional keystore write logic (checking dirtiedKeystore flag before writing) lacks test coverage. Consider adding a test in DefaultConfigurationControllerTest to verify that the keystore is not written when no changes are made, such as by checking the file's modification time before and after calling initializeSecuritySettings with an existing, valid keystore.
2 participants
Allow for using the keystore on a readonly filesystem. Currently, a write to the keystore will always occur. This change limits writes to only when necessary.
I welcome others to test by mounting the keystore as readonly. The keystore should not have the default storepass and keypass that that will trigger a write.
I'm considering a test in
DefaultConfigurationControllerTestwhere the modification time of a keystore is checked before and after the call, but that's a lot of investigation to get it written correctly.Minor other code cleanup was also included.
Related
Original ticket - nextgenhealthcare/connect#5467
Previous PR by @jonbartels - nextgenhealthcare/connect@development...jonbartels:connect:5467-keytore-avoid-writes
Summary table in #121