search

mekomsolutions / openmrs-module-initializer

The OpenMRS Initializer module is an API-only module that processes the content of the configuration folder when it is found inside OpenMRS' application data directory.
MIT License
23 stars 79 forks source link

OCL loader should throw an exception upon failure #263

Closed mseaton closed 8 months ago

mseaton commented 8 months ago

The current implementation ocl domain simply retrieves an instance of the OCL importer, and executes the importer.run(zip) method on the zip files contained within the configuration domain folder.

However, when this method fails, it does not throw an exception, but rather saves all of the details of the import including any errors, if any, in the database within an Import record. As a result, the current implementation of the OCL Loader does not throw any exceptions if the OCL loading fails.

The goal of this ticket is to fix this behavior and to ensure the OCL domain throws an Exception if loading of a zip file fails.

@mogoodrich / @ibacher / @mks-d FYI

mseaton commented 8 months ago

I have issued a PR here for this:

https://github.com/mekomsolutions/openmrs-module-initializer/pull/264

@mogoodrich / @ibacher / @rbuisson thoughts?

ibacher commented 8 months ago

So, yeah, I think this is generally probably the right thing to do. We probably need to clean-up the metadata in the RefApp before merging a fix for this in, though.

mseaton commented 8 months ago

We probably need to clean-up the metadata in the RefApp before merging a fix for this in, though

That may not be necessary @ibacher . I wonder if you could test that locally with this PR applied. From what I recall with Iniz, it is (a bit to my chagrin) configured to log errors and continue processing by default. So for implementations that use Iniz and don't know any better, if metadata fails to load on startup the system will still continue to start as if nothing had gone wrong, and errors will only appear down the road during usage if something is amiss.

A while back, we added a configuration setting to allow implementations to change this behavior, and in our PIH distribution, we have this behavior set to throw exceptions and fail hard at that time. See https://github.com/mekomsolutions/openmrs-module-initializer/blob/master/readme/rtprops.md#4-initializerstartupload-optional

mseaton commented 8 months ago

So as a follow-up @ibacher , I was installing a local version of the refapp to test out an unrelated ticket, and I ran the install with the latest version of Iniz that includes these changes, and it behaves as expected - there are errors in the logs due to one currently failing OCL zip, but all of the other OCL zips process subsequently, as do the other domains, and startup proceeds to completion. Personally, I think this is a mistake for most implementations, but this PR should not adversely impact the refapp.