Updates Migrate4_5_2.java to allow for log4j2 properties files not being present

[rogin]

Adds null check as property file may not exist.

To Reproduce: upgrade 4.5.1 to 4.5.2 without log4j2-cli.properties in the /conf directory

Related
nextgenhealthcare/connect#6327
Summary table in #121

[pacmano1]

Shouldn't this code write some log event somewhere on skipping?

[mgaffigan]

This doesn't seem right: the file is in the setup conf directory, and this migration seems to be the dual of 249b900. UTF-8 is not the default for appender.console.layout.charset. How are we assuming the install got to this state?

Perhaps the file should be created if it is not present? At a minimum, I would presume a warning would be called for.

[tonygermano]

I updated this PR. The original request in the NextGen repo stated that the mirth-cli had been uninstalled from the server install directory, and so the log4j2-cli.properties file was not present.

Technically, even the log4j2.properties file is not strictly required, and migration should not fail if it is missing.

This has been refactored so each file is processed in the same way and migration will progress if either is missing, logging an INFO level message when a file can't be found.

Additionally, it will now only attempt to save the file if it actually made a change to it.

[mgaffigan]

Unless I'm misreading this PR, the file is still not created by the migration.

Here are my assumptions:

• the integrity of the server deployment should be inviolable, outside of making changes to supported configuration options, and installing plugins. It's open source, so make whatever changes you would like, but the codebase assumes the shape that's produced by the installer and migrations.
• migrations should make an upgrade like a new install, carrying forward intentional non-default supported configurations

Full testing goes up with $O(2^N)$ for each configuration option. I don't see value in being able to omit log4j.properties, whether it will crash or not.

I'm not blocking, but I would suggest that the migration should match the change to the file in a new install.

[pacmano1]

This is OK by me - logging actions was my only ask.

[tonygermano]

Unless I'm misreading this PR, the file is still not created by the migration.

Here are my assumptions:

• the integrity of the server deployment should be inviolable, outside of making changes to supported configuration options, and installing plugins. It's open source, so make whatever changes you would like, but the codebase assumes the shape that's produced by the installer and migrations.
• migrations should make an upgrade like a new install, carrying forward intentional non-default supported configurations

Full testing goes up with O ( 2 N ) for each configuration option. I don't see value in being able to omit log4j.properties, whether it will crash or not.

I'm not blocking, but I would suggest that the migration should match the change to the file in a new install.

Sorry, I did not see this comment before I merged. It didn't show up in the discord notifications because it wasn't technically a review.

This migration is just to impose a new application default, which is different than the log4j2 default, if the user has not previously configured the option. I suppose you could say that deleting the log4j properties file is a user configuration choice? 😄 . The main thing was to not allow the migration to fail because the file was missing. If it was actually the user's intention to use the log4j defaults, then adding all of the application's defaults would be more of a change in behavior than the single setting that the migration is adding.

The cli is definitely not required, even though the installer installs it by default. NextGen's official docker image removes the cli application entirely, and they have always made it available as a stand-alone download, which we probably should do as well. There is no reason to install it to the same directory as the server application other than it is a strong indication that the cli version likely matches the server version when you find it there. However, since it is currently bundled and installed there by default and the cli project itself doesn't support migrations, I suppose that is why NG included that file in the server migration.