TRUNK-6515: Include message broker for publishing / subscribing to external events#6205
TRUNK-6515: Include message broker for publishing / subscribing to external events#6205rkorytkowski wants to merge 1 commit into
Conversation
|
Contains one commit from unmerged PR: #6190 |
rkorytkowski
force-pushed
the
TRUNK-6515
branch
2 times, most recently
from
9290c18 to
9b9f55c
Compare
|
Updated with the already merged commit. Now just one commit in the PR. |
|
dkayiwa
left a comment
dkayiwa
left a comment
There was a problem hiding this comment.
Verdict: merge — a clean, well-tested broker-event abstraction; CI is green across JDK 8/11/17/21. Headline/scope: the changes that look like API breaks — the OutboxEventPayload -> EventPayload rename + package move, and OutboxException becoming unchecked (now extends EventException extends RuntimeException) — all land on unreleased @since 2.9.0 code (version is 2.9.0-SNAPSHOT; latest released tag is 2.8.7), so nothing external breaks.
Two non-blocking items, both inline:
api/pom.xml—spring-jms+javax.jms-apiare added as compile dependencies ofopenmrs-api, but no core code references them (git grep -i jms api/srcis empty); they look like they belong inopenmrs-module-artemis.BrokerEventListenerTest— the broker-listener wiring is asserted only at registration; nothing publishes an event end-to-end through the Spring multicaster into the adapter.
Needs action before merge: none.
| <dependency> | ||
| <groupId>org.springframework</groupId> | ||
| <artifactId>spring-jms</artifactId> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>javax.jms</groupId> | ||
| <artifactId>javax.jms-api</artifactId> | ||
| </dependency> |
There was a problem hiding this comment.
question: spring-jms and javax.jms-api are added here as compile-scope dependencies of openmrs-api, but nothing in core uses them — git grep -i jms api/src (imports, resources, XML) comes back empty. As compile-scope deps they'll be inherited transitively by every module and distribution that depends on openmrs-api, to support code that currently lives only in openmrs-module-artemis.
Is core expected to grow a JMS-based default broker in a follow-up? If not, my suggestion is to declare these in the artemis module instead (or scope them provided here), so the JMS API isn't shipped to consumers that never touch messaging.
| public void shouldBeRegisteredWithListenerFactory() { | ||
| assertThat(listenerFactory.getListeners(), contains(allOf( | ||
| hasProperty("source", equalTo("my-test-source"))))); |
There was a problem hiding this comment.
suggestion: This is the only context-level test for the broker-listener wiring, and it asserts registration only. The source/broker filtering is unit-tested in BrokerApplicationListenerMethodAdapterTest, but that test calls adapter.processEvent(...) directly with a hand-built PayloadApplicationEvent, so nothing exercises the real path: a BrokerIncomingEvent published through ApplicationEventPublisher, routed by the multicaster via BrokerEvent.getResolvableType(), into the BrokerApplicationListenerMethodAdapter that BrokerEventListenerFactory produced.
Consider publishing a BrokerIncomingEvent("payload", "my-test-source") here and asserting the @BrokerEventListener method received it, plus a non-matching source/broker that is filtered out. That would validate the factory -> adapter -> multicaster integration that the unit tests structurally can't reach.
2 participants
Description of what I changed
It is used by https://github.com/openmrs/openmrs-module-artemis
Issue I worked on
see https://issues.openmrs.org/browse/TRUNK-6515
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 --amendI 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 --amendI ran
mvn clean packageright before creating this pull request andadded 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