Skip to content

GH-1123 - Detect and warn about events configured for externalization published outside transaction context #1456

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

GH-1123 - Detect and warn about events configured for externalization published outside transaction context #1456

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions ...in/java/org/springframework/modulith/events/config/EventPublicationAutoConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import org.springframework.context.annotation.Import;
import org.springframework.context.annotation.Role;
import org.springframework.core.env.Environment;
import org.springframework.modulith.events.EventExternalizationConfiguration;
import org.springframework.modulith.events.config.EventPublicationAutoConfiguration.AsyncEnablingConfiguration;
import org.springframework.modulith.events.core.DefaultEventPublicationRegistry;
import org.springframework.modulith.events.core.EventPublicationRegistry;
Expand Down Expand Up @@ -74,9 +75,11 @@ DefaultEventPublicationRegistry eventPublicationRegistry(EventPublicationReposit
@Role(BeanDefinition.ROLE_INFRASTRUCTURE)
@ConditionalOnBean(EventPublicationRegistry.class)
static PersistentApplicationEventMulticaster applicationEventMulticaster(
ObjectFactory<EventPublicationRegistry> eventPublicationRegistry, ObjectFactory<Environment> environment) {
ObjectFactory<EventPublicationRegistry> eventPublicationRegistry, ObjectFactory<Environment> environment,
ObjectProvider<EventExternalizationConfiguration> externalizationConfiguration) {

return EventPublicationConfiguration.applicationEventMulticaster(eventPublicationRegistry, environment);
return EventPublicationConfiguration.applicationEventMulticaster(eventPublicationRegistry, environment,
externalizationConfiguration);
}

@Bean
Expand Down
7 changes: 5 additions & 2 deletions ...c/main/java/org/springframework/modulith/events/config/EventPublicationConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import org.springframework.context.annotation.Configuration;
import org.springframework.context.annotation.Role;
import org.springframework.core.env.Environment;
import org.springframework.modulith.events.EventExternalizationConfiguration;
import org.springframework.modulith.events.core.DefaultEventPublicationRegistry;
import org.springframework.modulith.events.core.EventPublicationRegistry;
import org.springframework.modulith.events.core.EventPublicationRepository;
Expand Down Expand Up @@ -51,10 +52,12 @@ DefaultEventPublicationRegistry eventPublicationRegistry(EventPublicationReposit
@Bean
@Role(BeanDefinition.ROLE_INFRASTRUCTURE)
static PersistentApplicationEventMulticaster applicationEventMulticaster(
ObjectFactory<EventPublicationRegistry> eventPublicationRegistry, ObjectFactory<Environment> environment) {
ObjectFactory<EventPublicationRegistry> eventPublicationRegistry, ObjectFactory<Environment> environment,
ObjectProvider<EventExternalizationConfiguration> externalizationConfiguration) {

return new PersistentApplicationEventMulticaster(() -> eventPublicationRegistry.getObject(),
() -> environment.getObject());
() -> environment.getObject(),
() -> externalizationConfiguration.getIfAvailable(EventExternalizationConfiguration::disabled));
}

@Bean
Expand Down
54 changes: 51 additions & 3 deletions ...va/org/springframework/modulith/events/support/PersistentApplicationEventMulticaster.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import org.springframework.core.ResolvableType;
import org.springframework.core.annotation.AnnotationAwareOrderComparator;
import org.springframework.core.env.Environment;
import org.springframework.modulith.events.EventExternalizationConfiguration;
import org.springframework.modulith.events.EventPublication;
import org.springframework.modulith.events.FailedEventPublications;
import org.springframework.modulith.events.IncompleteEventPublications;
Expand All @@ -47,6 +48,7 @@
import org.springframework.modulith.events.core.TargetEventPublication;
import org.springframework.transaction.event.TransactionPhase;
import org.springframework.transaction.event.TransactionalApplicationListener;
import org.springframework.transaction.support.TransactionSynchronizationManager;
import org.springframework.util.Assert;

/**
Expand All @@ -58,6 +60,7 @@
* for incomplete publications.
*
* @author Oliver Drotbohm
* @author Yunho Jung
* @see CompletionRegisteringAdvisor
*/
public class PersistentApplicationEventMulticaster extends AbstractApplicationEventMulticaster
Expand All @@ -70,21 +73,26 @@ public class PersistentApplicationEventMulticaster extends AbstractApplicationEv

private final @NonNull Supplier<EventPublicationRegistry> registry;
private final @NonNull Supplier<Environment> environment;
private final @NonNull Supplier<EventExternalizationConfiguration> externalizationConfiguration;

/**
* Creates a new {@link PersistentApplicationEventMulticaster} for the given {@link EventPublicationRegistry}.
*
* @param registry must not be {@literal null}.
* @param environment must not be {@literal null}.
* @param externalizationConfiguration must not be {@literal null}.
*/
public PersistentApplicationEventMulticaster(Supplier<EventPublicationRegistry> registry,
Supplier<Environment> environment) {
Supplier<Environment> environment,
Supplier<EventExternalizationConfiguration> externalizationConfiguration) {

Assert.notNull(registry, "EventPublicationRegistry must not be null!");
Assert.notNull(environment, "Environment must not be null!");
Assert.notNull(externalizationConfiguration, "EventExternalizationConfiguration must not be null!");

this.registry = registry;
this.environment = environment;
this.externalizationConfiguration = externalizationConfiguration;
}

/*
Expand All @@ -111,8 +119,13 @@ public void multicastEvent(ApplicationEvent event, @Nullable ResolvableType even
return;
}

new TransactionalEventListeners(listeners)
.ifPresent(it -> storePublications(it, getEventToPersist(event)));
var eventToPersist = getEventToPersist(event);
var transactionalListeners = new TransactionalEventListeners(listeners);

// Detect events configured for externalization published outside transaction context
detectEventPublishedOutsideTransaction(transactionalListeners, eventToPersist);

transactionalListeners.ifPresent(it -> storePublications(it, eventToPersist));

for (ApplicationListener listener : listeners) {
listener.onApplicationEvent(event);
Expand Down Expand Up @@ -273,6 +286,41 @@ private static boolean invokeShouldHandle(ApplicationListener<?> candidate, Appl
return true;
}

/**
* Detects if an event selected for externalization is published outside a transaction context.
* If detected, logs a warning message to help developers identify the problem.
*
* @param transactionalListeners the transactional event listeners
* @param event the event being published
*/
private void detectEventPublishedOutsideTransaction(TransactionalEventListeners transactionalListeners,
Object event) {

// Transaction is active, no problem
if (TransactionSynchronizationManager.isActualTransactionActive()) {
return;
}

// No transactional listeners, nothing to check
if (!transactionalListeners.hasListeners()) {
return;
}

// Check if the event is configured for externalization
var config = externalizationConfiguration.get();
if (!config.supports(event)) {
return;
}

// Issue a warning log hinting at the problem
LOGGER.warn(
"Event {} is configured for externalization but published outside a transaction context. "
+ "Event externalization requires a transactional context to work properly. "
+ "The event will not be persisted to the event publication registry and externalization will not be triggered. "
+ "Consider publishing this event from a @Transactional method.",
event.getClass().getName());
}

/**
* First-class collection to work with transactional event listeners, i.e. {@link ApplicationListener} instances that
* implement {@link TransactionalApplicationListener}.
Expand Down
4 changes: 3 additions & 1 deletion ...ringframework/modulith/events/support/PersistentApplicationEventMulticasterUnitTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import org.springframework.core.ResolvableType;
import org.springframework.core.env.MapPropertySource;
import org.springframework.core.env.StandardEnvironment;
import org.springframework.modulith.events.EventExternalizationConfiguration;
import org.springframework.modulith.events.core.EventPublicationRegistry;
import org.springframework.stereotype.Component;
import org.springframework.transaction.event.TransactionPhase;
Expand All @@ -51,7 +52,8 @@ class PersistentApplicationEventMulticasterUnitTests {

@BeforeEach
void setUp() {
this.multicaster = new PersistentApplicationEventMulticaster(() -> registry, () -> environment);
this.multicaster = new PersistentApplicationEventMulticaster(() -> registry, () -> environment,
EventExternalizationConfiguration::disabled);
}

@Test // GH-240, GH-251
Expand Down