Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#255] Fixing 'liquibase' domain issues against Core 2.5.5+. #256

Merged
merged 5 commits into from
Nov 9, 2023

Conversation

mseaton
Copy link
Collaborator

@mseaton mseaton commented Nov 4, 2023

The genesis of this is trying to use the liquibase domain on an OpenMRS 2.5.x instance to load a liquibase file that contains "sqlFile" changesets which refer to sql files at a relative path, which fails due to an NPE.

This is something I had noticed a long time ago, and we worked around in our main PIH EMR distribution by a) patching core, and b) using a custom liquibase loader at startup. You can see that code here, which has been used successfully in that distribution for years. This also addresses an issue that I brought up on the core ticket which provided the necessary fixes in 2.5.x and beyond, where we discovered an issue with the previous use of absolute file paths in the liquibase changelog. Basically, the use of absolute paths made the changesets non-portable, so migrating (say) from /home/tomcat7/.OpenMRS to /home/tomcat8/.OpenMRS and then restarting your sever would result in all of the changes in the liquibase file getting re-executed, whether they already had been or not, since the change in file would be interpreted by liquibase as a totally new set of changesets. This could have very bad consequences if, for example, there are data migration or data cleanup changesets that were intended to run only once and never again and don't have sufficient preconditions on them.

This PR aims to solve these issues for implementations that have reached OpenMRS 2.5 (although this will only work for OpenMRS 2.5.5+ due to where the core patch was installed).

@mks-d / @Ruhanga / @ibacher / @rbuisson let me know what you think.

@mseaton mseaton changed the title Issue 255 #255 Liquibase loading fails when trying to execute sqlFiles relative to changset Nov 4, 2023
@mseaton mseaton changed the title #255 Liquibase loading fails when trying to execute sqlFiles relative to changset #255 Liquibase loading fails in OpenMRS 2.5.x+ Nov 4, 2023
@mks-d mks-d changed the title #255 Liquibase loading fails in OpenMRS 2.5.x+ [#255] Fixing 'liquibase' domain issues against Core 2.5.x+. Nov 6, 2023
@mks-d mks-d changed the title [#255] Fixing 'liquibase' domain issues against Core 2.5.x+. [#255] Fixing 'liquibase' domain issues against Core 2.5.5+. Nov 6, 2023
@mks-d mks-d removed request for rbuisson and mks-d November 6, 2023 12:58
@mseaton
Copy link
Collaborator Author

mseaton commented Nov 8, 2023

@ibacher / @Ruhanga - thoughts on this PR?

Copy link
Member

@ibacher ibacher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a good idea to me!

pom.xml Outdated Show resolved Hide resolved
@mseaton mseaton merged commit 65e4661 into mekomsolutions:master Nov 9, 2023
2 checks passed
@mseaton mseaton deleted the ISSUE-255 branch November 9, 2023 03:19
@ibacher
Copy link
Member

ibacher commented Mar 1, 2024

@mseaton So, I'm running into issues with this change. Specifically, the relativeFilePath doesn't seem to work at all. The code in the PIH EMR seems to work because here you're still passing an absolute path, but in this PR, we're only passing a relative path and this doesn't seem to work. What am I missing?

@mseaton
Copy link
Collaborator Author

mseaton commented Mar 1, 2024

because here you're still passing an absolute path

That isn't an absolute path @ibacher . It's a path relative to the application data directory, specifically "configuration/pih/liquibase/xxx.xml"

If it is helpful @ibacher , we are successfully using the Iniz liquibase domain in our Rwanda distribution here.

@ibacher
Copy link
Member

ibacher commented Mar 1, 2024

@mseaton Yeah, I discovered a different bug in core that needs fixing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Liquibase loading fails when trying to execute sqlFiles relative to changset
2 participants