TRUNK-6730 : Add Application Event Publishers for Module action events by sudhanshu-raj · Pull Request #6384 · openmrs/openmrs-core

sudhanshu-raj · 2026-07-24T10:22:35Z

Description of what I changed

This is publishing the module action events load, stop, start or unload of modules, and for auditing the module events in the audit log web module and related to this PR.

Issue I worked on

see https://issues.openmrs.org/browse/TRUNK-6730

Checklist: I completed these to help reviewers :)

My IDE is configured to follow the code style of this project.

No? Unsure? -> configure your IDE, format the code and add the changes with git add . && git commit --amend
I have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)

No? -> write tests and add them to this commit git add . && git commit --amend
I ran ./mvnw clean package right before creating this pull request and
added all formatting changes to my commit.

No? -> execute above command
All new and existing tests passed.

No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.

No? Unsure? -> execute command git pull --rebase upstream master

…ess param

… for it

…h exception

sudhanshu-raj · 2026-07-30T20:45:15Z

Hi @dkayiwa @wikumChamith @ibacher @ManojLL , please review the changes.

dkayiwa · 2026-07-31T14:07:05Z

+
+}
+
+enum ModuleEventType {


ModuleEventType is package private, so nothing outside org.openmrs.module can name it. If this merges as is, the discriminator on the event is unusable by the very modules the event exists for. A consumer that writes the obvious thing:

ModuleEventType type = event.getActionType();

fails to compile with ModuleEventType is not public in org.openmrs.module; cannot be accessed from outside package (I compiled this class on its own and then a consumer in another package to check). The auditlogweb PR this pairs with is already living with the consequence: it reaches the value as String.valueOf(moduleActionEvent.getActionType()) and keeps its own copy of the enum in auditlogweb/api/utils/ModuleEventType, complete with a fromName(String) parser and an UNKNOWN bucket, to get a typed value back. Every future consumer will have to do the same, and they will all drift apart. A public getter whose return type is inaccessible is also just a broken signature.

Could you make the enum public? That moves it into its own ModuleEventType.java anyway, since javac will not take a public top level type in a file named after something else. While you are in there, ModuleActionEvent and the enum are new public API with no javadoc at all, and this branch is 2.7.10-SNAPSHOT, so they want a short class comment and @since 2.7.10. The @since 2.7 on the private publishModuleEvents helper can go, since @since on a private method does not tell anyone anything.

agree, fixed it

dkayiwa · 2026-07-31T14:07:05Z

+		boolean isModuleLoaded = true;

-		Module oldModule = getLoadedModulesMap().get(module.getModuleId());
-		if (oldModule != null) {
-			int versionComparison = ModuleUtil.compareVersion(oldModule.getVersion(), module.getVersion());
-			if (versionComparison < 0) {
-				// if oldModule version is lower, unload it and use the new
-				unloadModule(oldModule);
-			} else if (versionComparison == 0) {
-				if (replaceIfExists) {
-					// if the versions are the same and we're told to replaceIfExists, use the new
+		try {
+			Module oldModule = getLoadedModulesMap().get(module.getModuleId());
+			if (oldModule != null) {
+				int versionComparison = ModuleUtil.compareVersion(oldModule.getVersion(), module.getVersion());
+				if (versionComparison < 0) {
+					// if oldModule version is lower, unload it and use the new
 					unloadModule(oldModule);
+				} else if (versionComparison == 0) {
+					if (replaceIfExists) {
+						// if the versions are the same and we're told to replaceIfExists, use the new
+						unloadModule(oldModule);
+					} else {
+						isModuleLoaded = false;
+						// if the versions are equal and we're not told to replaceIfExists, jump out of here in a bad way
+						throw new ModuleException("A module with the same id and version already exists", module.getModuleId());
+					}
 				} else {
-					// if the versions are equal and we're not told to replaceIfExists, jump out of here in a bad way
-					throw new ModuleException("A module with the same id and version already exists", module.getModuleId());
+					isModuleLoaded = false;
+					// if the older (already loaded) module is newer, keep that original one that was loaded. return that one.
+					return oldModule;
 				}
-			} else {
-				// if the older (already loaded) module is newer, keep that original one that was loaded. return that one.
-				return oldModule;
 			}
+
+			getLoadedModulesMap().put(module.getModuleId(), module);


isModuleLoaded starts out true and only becomes false on the two paths that bail out deliberately, so anything else thrown inside this try leaves it true and the finally publishes a successful load. If this merges as is, an audit log fed by these events records MODULE_LOAD / success for a load that threw and loaded nothing, and because nothing blows up there is no way to notice from the logs.

I reproduced it on your branch. Load a module, mark it mandatory, then load the same version again with replaceIfExists true. unloadModule(oldModule) throws MandatoryModuleException, the old module stays in the loaded map, the new one never gets put there, loadModule propagates the exception to its caller, and the listener still receives:

Test1 Module:MODULE_STOP:false, Test1 Module:MODULE_UNLOAD:false, Test1 Module:MODULE_LOAD:true

Flipping the default so the flag is only raised once the module is actually in the map fixes it, and the two explicit false assignments fall out:

Suggested change

boolean isModuleLoaded = true;

Module oldModule = getLoadedModulesMap().get(module.getModuleId());

if (oldModule != null) {

int versionComparison = ModuleUtil.compareVersion(oldModule.getVersion(), module.getVersion());

if (versionComparison < 0) {

// if oldModule version is lower, unload it and use the new

unloadModule(oldModule);

} else if (versionComparison == 0) {

if (replaceIfExists) {

// if the versions are the same and we're told to replaceIfExists, use the new

try {

Module oldModule = getLoadedModulesMap().get(module.getModuleId());

if (oldModule != null) {

int versionComparison = ModuleUtil.compareVersion(oldModule.getVersion(), module.getVersion());

if (versionComparison < 0) {

// if oldModule version is lower, unload it and use the new

unloadModule(oldModule);

} else if (versionComparison == 0) {

if (replaceIfExists) {

// if the versions are the same and we're told to replaceIfExists, use the new

unloadModule(oldModule);

} else {

isModuleLoaded = false;

// if the versions are equal and we're not told to replaceIfExists, jump out of here in a bad way

throw new ModuleException("A module with the same id and version already exists", module.getModuleId());

}

} else {

// if the versions are equal and we're not told to replaceIfExists, jump out of here in a bad way

throw new ModuleException("A module with the same id and version already exists", module.getModuleId());

isModuleLoaded = false;

// if the older (already loaded) module is newer, keep that original one that was loaded. return that one.

return oldModule;

}

} else {

// if the older (already loaded) module is newer, keep that original one that was loaded. return that one.

return oldModule;

}

getLoadedModulesMap().put(module.getModuleId(), module);

boolean isModuleLoaded = false;

try {

Module oldModule = getLoadedModulesMap().get(module.getModuleId());

if (oldModule != null) {

int versionComparison = ModuleUtil.compareVersion(oldModule.getVersion(), module.getVersion());

if (versionComparison < 0) {

// if oldModule version is lower, unload it and use the new

unloadModule(oldModule);

} else if (versionComparison == 0) {

if (replaceIfExists) {

// if the versions are the same and we're told to replaceIfExists, use the new

unloadModule(oldModule);

} else {

// if the versions are equal and we're not told to replaceIfExists, jump out of here in a bad way

throw new ModuleException("A module with the same id and version already exists", module.getModuleId());

}

} else {

// if the older (already loaded) module is newer, keep that original one that was loaded. return that one.

return oldModule;

}

}

getLoadedModulesMap().put(module.getModuleId(), module);

isModuleLoaded = true;

Worth a test as well, since none of the four new load tests exercise a throw coming out of unloadModule.

flipped the success var value and added test cases too

dkayiwa · 2026-07-31T14:07:05Z

 		List<Module> dependentModulesStopped = new ArrayList<>();
-
-		if (mod != null) {
-
-			if (!ModuleFactory.isModuleStarted(mod)) {
-				return dependentModulesStopped;
-			}
-
-			try {
-				// if extends BaseModuleActivator
-				if (mod.getModuleActivator() != null) {
-					mod.getModuleActivator().willStop();
+		boolean isStoppedSuccess = true;
+		try {
+			if (mod != null) {
+
+				if (!ModuleFactory.isModuleStarted(mod)) {
+					return dependentModulesStopped;
 				}


The early return for a module that is not running sits inside the try, so the finally still fires, and at that point isStoppedSuccess && !isModuleStarted(mod) evaluates to true. If this merges as is, the audit log gets a successful MODULE_STOP entry for a module that was never running.

The path that makes this matter is not somebody stopping a module twice, it is a failed start. When startModuleInternal throws before it reaches getStartedModulesMap().put(...) (a require_version mismatch against the running core is the everyday case) its catch block calls stopModule(module, false, true) to undo the startup, that call returns right here, and the listener sees:

Test1 Module:MODULE_STOP:true, Test1 Module:MODULE_START:false

I ran both that case and a plain stopModule on a loaded but never started module, and each produced a successful stop event. Someone auditing a server later cannot tell those apart from a real stop.

Hoisting the guard above the try fixes it:

Suggested change

List<Module> dependentModulesStopped = new ArrayList<>();

if (mod != null) {

if (!ModuleFactory.isModuleStarted(mod)) {

return dependentModulesStopped;

}

try {

// if extends BaseModuleActivator

if (mod.getModuleActivator() != null) {

mod.getModuleActivator().willStop();

boolean isStoppedSuccess = true;

try {

if (mod != null) {

if (!ModuleFactory.isModuleStarted(mod)) {

return dependentModulesStopped;

}

List<Module> dependentModulesStopped = new ArrayList<>();

if (mod == null || !ModuleFactory.isModuleStarted(mod)) {

return dependentModulesStopped;

}

boolean isStoppedSuccess = true;

try {

if (mod != null) {

After that the if (mod != null) wrapper is redundant, and so is the mod != null check in the finally, so both can go and the body loses a level of indentation.

One thing you might do while you are in here: wrapping the existing body in place is what forced the ~150 lines of re-indentation that make up most of this diff, and it rewrites the blame for the method. Pulling the old body into a private doStopModule(...) and leaving stopModule as the guard plus a try/finally around that call would keep the actual change visible, and it should also clear the five java:S1141 "extract this nested try block" findings Sonar raised on this file.

It's good edge case, fixed and added test case too

dkayiwa · 2026-07-31T14:07:05Z

+				if (!deleted) {
+					file.deleteOnExit();
+					isEventSuccess = false;
+					log.warn("Could not delete " + file.getAbsolutePath());
+				}


Not being able to delete the .omod file does not mean the unload failed. By the time we reach this branch the module has been stopped, removed from the loaded modules map and its class loader disposed, and deleteOnExit() has just been registered so the leftover file goes away at JVM exit. The surrounding code has always treated a failed delete as expected and recoverable, which is exactly why that fallback is there.

If this merges as is, a completed unload is recorded as a failed one. I forced file.delete() to return false on your branch and got:

Test1 Module:MODULE_STOP:true, Test1 Module:MODULE_UNLOAD:false

with the module genuinely gone from getLoadedModules(). Whoever reads that audit entry later concludes a module failed to unload when it did unload, and the only real symptom, a stale file in the module repository, is not what the entry says.

I would drop the flag here and let the existing warn carry the undeleted file:

Suggested change

if (!deleted) {

file.deleteOnExit();

isEventSuccess = false;

log.warn("Could not delete " + file.getAbsolutePath());

}

if (!deleted) {

file.deleteOnExit();

log.warn("Could not delete " + file.getAbsolutePath());

}

I guessed it same, anyway fixed

dkayiwa · 2026-07-31T14:07:05Z

 */
 public class ModuleFactory {

+	private static Logger logger = LoggerFactory.getLogger(ModuleFactory.class);


This logger is never read. The class declares log ten lines further down and all 51 logging calls in the file, including the new publishModuleEvents, go through that one. It is also the only non final logger field here. Worth deleting.

My bad, didn't see !

dkayiwa · 2026-07-31T14:07:05Z

+
+	private boolean isSuccess;
+
+	public ModuleActionEvent(Object source, ModuleEventType eventType, String moduleName, boolean isSuccess) {


Should this carry the module id alongside the display name? Module.getName() is the <name> from config.xml, a human label that can change between versions, whereas moduleId is the stable identifier the rest of the module API keys on (getModuleById, the <moduleId>.started global property, ModuleConstants.CORE_MODULES). An audit row that only says "Test1 Module" is awkward to join back to anything, and for load and start events the module version would be worth having too.

Since this is brand new API, adding moduleId (and version) to the constructor now is a lot cheaper than after 2.7.10 ships with the current shape.

True the module name itself can't tell more about itself, so version is needed and module id also can be useful for the easy API queries, added both.

dkayiwa · 2026-07-31T14:07:05Z

 	@Override
 	public void setApplicationContext(ApplicationContext applicationContext) {
 		this.applicationContext = applicationContext;
+		ModuleFactory.setApplicationEventPublisher(applicationContext);


ServiceContext already holds this context and hands it out through getApplicationContext(), so ModuleFactory can read it when it publishes rather than keeping a second copy in a static field:

ApplicationEventPublisher publisher = ServiceContext.getInstance().getApplicationContext(); if (publisher != null) { publisher.publishEvent(new ModuleActionEvent(ModuleFactory.class, eventType, moduleName, isSuccess)); }

That takes out this line and the whole change to this file, drops the static field, and keeps setApplicationEventPublisher off ModuleFactory's public surface. As written, any caller can hand ModuleFactory a different publisher or a null one and quietly switch module auditing off, which is an odd thing to expose on the class this feature is meant to make auditable. It also avoids ServiceContext picking up its first dependency on org.openmrs.module, which is the awkward direction given ModuleFactory already depends on org.openmrs.api.context.

I tried it on your branch: with this file reverted and publishModuleEvents reading the publisher off ServiceContext, all 19 ModuleFactoryTest tests pass, so the timing works out the same. Both routes are fed by this same callback. Not a blocker, but I think it is the better shape.

Ahh new learning for me, thnx

dkayiwa · 2026-07-31T14:07:05Z

+			DatabaseUtil.loadDatabaseDriver(props.getProperty(CONNECTION_URL), props.getProperty(CONNECTION_DRIVER_CLASS,
+			    null));


This file has nothing to do with TRUNK-6730. The two changes in it are this line wrap and a tab added to an otherwise blank line near the top, which look like an IDE formatter pass on a file that happened to be open. Reverting it keeps the PR inside the api module.

…param and added test cases for it

…ed unused logger

…hange

wikumChamith · 2026-08-03T05:59:50Z

 import org.openmrs.logic.LogicService;
 import org.openmrs.messagesource.MessageSourceService;
 import org.openmrs.messagesource.impl.DefaultMessageSourceServiceImpl;
+import org.openmrs.module.ModuleFactory;


Do we need this import?

wikumChamith · 2026-08-03T06:40:09Z

+		boolean isStoppedSuccess = true;
+		try {
+			dependentModulesStopped = doStopModule(mod, skipOverStartedProperty, isFailedStartup);
+		} catch(ModuleMustStartException ex) {


What will happen if we get an another type of exception here?

Pivoted to more general exception now, I thought this module exception could be only reason for failure event

… action

wikumChamith

@sudhanshu-raj this PR contains a lot of formatting changes, and that's making it harder to review. Can we revert those changes?

sudhanshu-raj · 2026-08-04T14:05:20Z

@sudhanshu-raj this PR contains a lot of formatting changes, and that's making it harder to review. Can we revert those changes?

Not sure why getting such view here, I tried now some changes but seems not that helpful, any suggestions ? Though if view from the IntelliJ compare with branch feature its not that cluttered and it's get very clear the new changes.

wikumChamith · 2026-08-06T09:04:30Z

+		this.isSuccess = isSuccess;
+	}
+
+	public ModuleEventType getActionType() {


shouldn't this be getEventType()?

wikumChamith · 2026-08-06T09:05:05Z

+	private ModuleEventType eventType;
+
+	private String moduleId;
+
+	private String moduleName;
+
+	private String moduleVersion;
+
+	private boolean isSuccess;


I think we can make these final.

wikumChamith · 2026-08-06T09:09:31Z

What's gonna happen if this parser fails due to a corruption? Don't we need to log that?

I added the tracker for this so when ever the module file parsing will fail, we will mark with fail event and will extract the module name and version(whatever is possible) from the module filename.

wikumChamith · 2026-08-06T09:11:54Z

+ * 
+ * @since 2.7.10
+ */
+public class ModuleActionEvent extends ApplicationEvent {


What if we add a failureReason field? That way audit log can say why something failed.

…me for event type

…n ModuleUtil

wikumChamith · 2026-08-10T10:18:25Z

@sudhanshu-raj what do you think about the two issues SonarCloud is pointing at?

sudhanshu-raj · 2026-08-10T11:55:34Z

@sudhanshu-raj what do you think about the two issues SonarCloud is pointing at?

One is for the cognitive complexity for which it saying to re-factor the whole method to make easy to understand but this is not new method I just added new name to it and another issue is to simplify the regular expression for the module file name pattern due to lazy quantifiers which says it can take increase runtime incase we are matching this regex against the large string but in this case string is just the module name which should not go in that way and thing is this lazy quantifier (*?) is needed to extract the proper module name and version format.

wikumChamith · 2026-08-10T16:53:56Z

+			   if(module == null) {
+				   fileName = ModuleUtil.getModuleNameAndVersionFromFileName(moduleFile.getName());
+				   isModuleLoaded = false;
+				   String[] parts = fileName.split(":");


getModuleNameAndVersionFromFileName returns null for an empty filename (e.g. new File("/")), so this split can NPE inside the finally, replacing the real ModuleException and publishing no event at all. Please null-guard the result and wrap this whole block in a try/catch so event metadata can never outrank the actual failure.

wikumChamith · 2026-08-10T16:54:49Z

+				   module = new Module(name, null, null, null, null, version, null);
+			   }
+			   publishModuleEvents(ModuleEventType.MODULE_LOAD, module, isModuleLoaded, failureReason);


The activator is assigned to module and returned to the caller, breaking the documented "returns null on error" contract. loadModules() would hold onto it and fail much later somewhere confusing. Can we keep the synthetic module local to the event publish and keep the return value honest?

Fixed, not tweaking the original return module .

…module

sonarqubecloud · 2026-08-10T19:51:03Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

sudhanshu-raj changed the base branch from master to 2.7.x July 24, 2026 10:26

sudhanshu-raj mentioned this pull request Jul 24, 2026

AUDIT-54: Implement Administration Events Logging openmrs/openmrs-module-auditlogweb#57

Open

5 tasks

sudhanshu-raj added 4 commits July 27, 2026 01:40

TRUNK-6730: Publishing application events for module action

7f9a350

TRUNK-6730:Created helper method for event publish and optimized succ…

a7ca86f

…ess param

TRUNK-6730:Fixed null pointer exceptions

e56014e

TRUNK-6730:Clearing noisy logs and back to log level warn

7efb5f1

sudhanshu-raj force-pushed the TRUNK-6730 branch from dce4039 to 7efb5f1 Compare July 26, 2026 20:13

sudhanshu-raj added 3 commits July 30, 2026 22:42

TRUNK-6730:Added test cases for module event publish

f996fbf

TRUNK-6730:Moved the stop module event publish and tweaked test cases…

9b202dc

… for it

TRUNK-6730:Replaced mock static with actual lines which can throw suc…

aadaa4d

…h exception

sudhanshu-raj marked this pull request as ready for review July 30, 2026 20:42

dkayiwa reviewed Jul 31, 2026

View reviewed changes

sudhanshu-raj added 6 commits July 31, 2026 23:20

TRUNK-6730:Made the ModuleEventType public and added short comments

1ed0e46

TRUNK-6730:Fixed load and stop module event publish abnormal success …

d0299d9

…param and added test cases for it

TRUNK-6730:Modified unload module event success false param and remov…

d7dc76f

…ed unused logger

TRUNK-6730:Removed setApplicationEventPublisher and undo formatting c…

14baa3f

…hange

TRUNK-6730:Fixed formatting changes

50a717d

TRUNK-6730:Added module id and version the module event data

186e76a

sudhanshu-raj requested a review from dkayiwa August 1, 2026 18:18

wikumChamith reviewed Aug 3, 2026

View reviewed changes

TRUNK-6730:Pivoted to more general Exception for success module event…

c97e1c7

… action

sudhanshu-raj requested a review from wikumChamith August 3, 2026 18:26

wikumChamith reviewed Aug 4, 2026

View reviewed changes

TRUNK-6730:Formatting changes for module factory

d33a5ad

wikumChamith reviewed Aug 6, 2026

View reviewed changes

sudhanshu-raj added 3 commits August 6, 2026 17:36

TRUNK-6730:Made module action event fields final and change setter na…

00f5cb5

…me for event type

TRUNK-6730: Added tracker for module file parsing also

28bae5e

TRUNK-6730:Added test cases for getModuleNameAndVersionFromFileName i…

fe0929f

…n ModuleUtil

sudhanshu-raj added 2 commits August 7, 2026 00:53

TRUNK-6730:Optimized the regex pattern for module file name

051dfd0

TRUNK-6730:Added failure reason for the module events

2b597cd

sudhanshu-raj requested a review from wikumChamith August 7, 2026 07:53

wikumChamith reviewed Aug 10, 2026

View reviewed changes

TRUNK-6730: Added null check for fileName and returning the original …

f9f7ffe

…module


		private boolean isSuccess;

		public ModuleActionEvent(Object source, ModuleEventType eventType, String moduleName, boolean isSuccess) {

		DatabaseUtil.loadDatabaseDriver(props.getProperty(CONNECTION_URL), props.getProperty(CONNECTION_DRIVER_CLASS,
		null));

Uh oh!

Conversation

sudhanshu-raj commented Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of what I changed

Issue I worked on

Checklist: I completed these to help reviewers :)

Uh oh!

sudhanshu-raj commented Jul 30, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wikumChamith left a comment

Choose a reason for hiding this comment

Uh oh!

sudhanshu-raj commented Aug 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wikumChamith commented Aug 10, 2026

Uh oh!

sudhanshu-raj commented Aug 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

sudhanshu-raj commented Jul 24, 2026 •

edited

Loading