From d8c41c2583bf5b5c17bf281715c5ae856a0e6ef6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Nicoll?= Date: Tue, 26 Nov 2024 19:56:32 +0100 Subject: [PATCH] Defensively unwrap CacheConnectionFactory This commit refines the optimization introduced in gh-39816 to only unwrap our own caching connection factory. The more advanced unwrap algorithm is still available, but opt-in only. Unwrapping more aggressively may break use cases where the wrapped ConnectionFactory is required, i.e. for transactional purposes. Closes gh-43277 --- .../jms/JmsAnnotationDrivenConfiguration.java | 2 +- .../reference/pages/messaging/jms.adoc | 2 + .../MyJmsConfiguration.java | 2 +- .../receiving/custom/MyJmsConfiguration.java | 2 +- .../MyJmsConfiguration.kt | 2 +- .../receiving/custom/MyJmsConfiguration.kt | 2 +- .../boot/jms/ConnectionFactoryUnwrapper.java | 17 +++ .../jms/ConnectionFactoryUnwrapperTests.java | 144 ++++++++++++------ 8 files changed, 122 insertions(+), 51 deletions(-) diff --git a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jms/JmsAnnotationDrivenConfiguration.java b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jms/JmsAnnotationDrivenConfiguration.java index 6de5bc8fcf..cea5d85401 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jms/JmsAnnotationDrivenConfiguration.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jms/JmsAnnotationDrivenConfiguration.java @@ -90,7 +90,7 @@ class JmsAnnotationDrivenConfiguration { DefaultJmsListenerContainerFactory jmsListenerContainerFactory( DefaultJmsListenerContainerFactoryConfigurer configurer, ConnectionFactory connectionFactory) { DefaultJmsListenerContainerFactory factory = new DefaultJmsListenerContainerFactory(); - configurer.configure(factory, ConnectionFactoryUnwrapper.unwrap(connectionFactory)); + configurer.configure(factory, ConnectionFactoryUnwrapper.unwrapCaching(connectionFactory)); return factory; } diff --git a/spring-boot-project/spring-boot-docs/src/docs/antora/modules/reference/pages/messaging/jms.adoc b/spring-boot-project/spring-boot-docs/src/docs/antora/modules/reference/pages/messaging/jms.adoc index 78d985c968..52cc56b34e 100644 --- a/spring-boot-project/spring-boot-docs/src/docs/antora/modules/reference/pages/messaging/jms.adoc +++ b/spring-boot-project/spring-boot-docs/src/docs/antora/modules/reference/pages/messaging/jms.adoc @@ -165,6 +165,8 @@ In most scenarios, message listener containers should be configured against the This way each listener container has its own connection and this gives full responsibility to it in terms of local recovery. The auto-configuration uses javadoc:org.springframework.boot.jms.ConnectionFactoryUnwrapper[] to unwrap the native connection factory from the auto-configured one. +NOTE: The auto-configuration only unwraps `CachedConnectionFactory`. + By default, the default factory is transactional. If you run in an infrastructure where a javadoc:org.springframework.transaction.jta.JtaTransactionManager[] is present, it is associated to the listener container by default. If not, the `sessionTransacted` flag is enabled. diff --git a/spring-boot-project/spring-boot-docs/src/main/java/org/springframework/boot/docs/howto/messaging/disabletransactedjmssession/MyJmsConfiguration.java b/spring-boot-project/spring-boot-docs/src/main/java/org/springframework/boot/docs/howto/messaging/disabletransactedjmssession/MyJmsConfiguration.java index cedf98bea6..c033190a73 100644 --- a/spring-boot-project/spring-boot-docs/src/main/java/org/springframework/boot/docs/howto/messaging/disabletransactedjmssession/MyJmsConfiguration.java +++ b/spring-boot-project/spring-boot-docs/src/main/java/org/springframework/boot/docs/howto/messaging/disabletransactedjmssession/MyJmsConfiguration.java @@ -31,7 +31,7 @@ public class MyJmsConfiguration { public DefaultJmsListenerContainerFactory jmsListenerContainerFactory(ConnectionFactory connectionFactory, DefaultJmsListenerContainerFactoryConfigurer configurer) { DefaultJmsListenerContainerFactory listenerFactory = new DefaultJmsListenerContainerFactory(); - configurer.configure(listenerFactory, ConnectionFactoryUnwrapper.unwrap(connectionFactory)); + configurer.configure(listenerFactory, ConnectionFactoryUnwrapper.unwrapCaching(connectionFactory)); listenerFactory.setTransactionManager(null); listenerFactory.setSessionTransacted(false); return listenerFactory; diff --git a/spring-boot-project/spring-boot-docs/src/main/java/org/springframework/boot/docs/messaging/jms/receiving/custom/MyJmsConfiguration.java b/spring-boot-project/spring-boot-docs/src/main/java/org/springframework/boot/docs/messaging/jms/receiving/custom/MyJmsConfiguration.java index ac9b38cde5..7261056cb2 100644 --- a/spring-boot-project/spring-boot-docs/src/main/java/org/springframework/boot/docs/messaging/jms/receiving/custom/MyJmsConfiguration.java +++ b/spring-boot-project/spring-boot-docs/src/main/java/org/springframework/boot/docs/messaging/jms/receiving/custom/MyJmsConfiguration.java @@ -31,7 +31,7 @@ public class MyJmsConfiguration { public DefaultJmsListenerContainerFactory myFactory(DefaultJmsListenerContainerFactoryConfigurer configurer, ConnectionFactory connectionFactory) { DefaultJmsListenerContainerFactory factory = new DefaultJmsListenerContainerFactory(); - configurer.configure(factory, ConnectionFactoryUnwrapper.unwrap(connectionFactory)); + configurer.configure(factory, ConnectionFactoryUnwrapper.unwrapCaching(connectionFactory)); factory.setMessageConverter(new MyMessageConverter()); return factory; } diff --git a/spring-boot-project/spring-boot-docs/src/main/kotlin/org/springframework/boot/docs/howto/messaging/disabletransactedjmssession/MyJmsConfiguration.kt b/spring-boot-project/spring-boot-docs/src/main/kotlin/org/springframework/boot/docs/howto/messaging/disabletransactedjmssession/MyJmsConfiguration.kt index 39b2f19444..d1ee891f79 100644 --- a/spring-boot-project/spring-boot-docs/src/main/kotlin/org/springframework/boot/docs/howto/messaging/disabletransactedjmssession/MyJmsConfiguration.kt +++ b/spring-boot-project/spring-boot-docs/src/main/kotlin/org/springframework/boot/docs/howto/messaging/disabletransactedjmssession/MyJmsConfiguration.kt @@ -31,7 +31,7 @@ class MyJmsConfiguration { fun jmsListenerContainerFactory(connectionFactory: ConnectionFactory?, configurer: DefaultJmsListenerContainerFactoryConfigurer): DefaultJmsListenerContainerFactory { val listenerFactory = DefaultJmsListenerContainerFactory() - configurer.configure(listenerFactory, ConnectionFactoryUnwrapper.unwrap(connectionFactory)) + configurer.configure(listenerFactory, ConnectionFactoryUnwrapper.unwrapCaching(connectionFactory)) listenerFactory.setTransactionManager(null) listenerFactory.setSessionTransacted(false) return listenerFactory diff --git a/spring-boot-project/spring-boot-docs/src/main/kotlin/org/springframework/boot/docs/messaging/jms/receiving/custom/MyJmsConfiguration.kt b/spring-boot-project/spring-boot-docs/src/main/kotlin/org/springframework/boot/docs/messaging/jms/receiving/custom/MyJmsConfiguration.kt index d788b97d99..6228ceea13 100644 --- a/spring-boot-project/spring-boot-docs/src/main/kotlin/org/springframework/boot/docs/messaging/jms/receiving/custom/MyJmsConfiguration.kt +++ b/spring-boot-project/spring-boot-docs/src/main/kotlin/org/springframework/boot/docs/messaging/jms/receiving/custom/MyJmsConfiguration.kt @@ -30,7 +30,7 @@ class MyJmsConfiguration { fun myFactory(configurer: DefaultJmsListenerContainerFactoryConfigurer, connectionFactory: ConnectionFactory): DefaultJmsListenerContainerFactory { val factory = DefaultJmsListenerContainerFactory() - configurer.configure(factory, ConnectionFactoryUnwrapper.unwrap(connectionFactory)) + configurer.configure(factory, ConnectionFactoryUnwrapper.unwrapCaching(connectionFactory)) factory.setMessageConverter(MyMessageConverter()) return factory } diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/jms/ConnectionFactoryUnwrapper.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/jms/ConnectionFactoryUnwrapper.java index 681d70e532..ec98a93b21 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/jms/ConnectionFactoryUnwrapper.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/jms/ConnectionFactoryUnwrapper.java @@ -33,6 +33,23 @@ public final class ConnectionFactoryUnwrapper { private ConnectionFactoryUnwrapper() { } + /** + * Return the native {@link ConnectionFactory} by unwrapping ot from a + * {@link CachingConnectionFactory}. Return the given {@link ConnectionFactory} if no + * {@link CachingConnectionFactory} wrapper has been detected. + * @param connectionFactory a connection factory + * @return the native connection factory that a {@link CachingConnectionFactory} + * wraps, if any + * @since 3.4.1 + */ + public static ConnectionFactory unwrapCaching(ConnectionFactory connectionFactory) { + if (connectionFactory instanceof CachingConnectionFactory cachingConnectionFactory) { + ConnectionFactory unwrapedConnectionFactory = cachingConnectionFactory.getTargetConnectionFactory(); + return (unwrapedConnectionFactory != null) ? unwrapCaching(unwrapedConnectionFactory) : connectionFactory; + } + return connectionFactory; + } + /** * Return the native {@link ConnectionFactory} by unwrapping it from a cache or pool * connection factory. Return the given {@link ConnectionFactory} if no caching diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/jms/ConnectionFactoryUnwrapperTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/jms/ConnectionFactoryUnwrapperTests.java index 993860a2ac..5a65cfde8f 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/jms/ConnectionFactoryUnwrapperTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/jms/ConnectionFactoryUnwrapperTests.java @@ -17,6 +17,7 @@ package org.springframework.boot.jms; import jakarta.jms.ConnectionFactory; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.messaginghub.pooled.jms.JmsPoolConnectionFactory; @@ -35,59 +36,110 @@ import static org.mockito.Mockito.mock; */ class ConnectionFactoryUnwrapperTests { - @Test - void unwrapWithSingleConnectionFactory() { - ConnectionFactory connectionFactory = new SingleConnectionFactory(); - assertThat(ConnectionFactoryUnwrapper.unwrap(connectionFactory)).isSameAs(connectionFactory); + @Nested + class UnwrapCaching { + + @Test + void unwrapWithSingleConnectionFactory() { + ConnectionFactory connectionFactory = new SingleConnectionFactory(); + assertThat(unwrapCaching(connectionFactory)).isSameAs(connectionFactory); + } + + @Test + void unwrapWithConnectionFactory() { + ConnectionFactory connectionFactory = mock(ConnectionFactory.class); + assertThat(unwrapCaching(connectionFactory)).isSameAs(connectionFactory); + } + + @Test + void unwrapWithCachingConnectionFactory() { + ConnectionFactory connectionFactory = mock(ConnectionFactory.class); + assertThat(unwrapCaching(new CachingConnectionFactory(connectionFactory))).isSameAs(connectionFactory); + } + + @Test + void unwrapWithNestedCachingConnectionFactories() { + ConnectionFactory connectionFactory = mock(ConnectionFactory.class); + CachingConnectionFactory firstCachingConnectionFactory = new CachingConnectionFactory(connectionFactory); + CachingConnectionFactory secondCachingConnectionFactory = new CachingConnectionFactory( + firstCachingConnectionFactory); + assertThat(unwrapCaching(secondCachingConnectionFactory)).isSameAs(connectionFactory); + } + + @Test + void unwrapWithJmsPoolConnectionFactory() { + ConnectionFactory connectionFactory = mock(ConnectionFactory.class); + JmsPoolConnectionFactory poolConnectionFactory = new JmsPoolConnectionFactory(); + poolConnectionFactory.setConnectionFactory(connectionFactory); + assertThat(unwrapCaching(poolConnectionFactory)).isSameAs(poolConnectionFactory); + } + + private ConnectionFactory unwrapCaching(ConnectionFactory connectionFactory) { + return ConnectionFactoryUnwrapper.unwrapCaching(connectionFactory); + } + } - @Test - void unwrapWithConnectionFactory() { - ConnectionFactory connectionFactory = mock(ConnectionFactory.class); - assertThat(ConnectionFactoryUnwrapper.unwrap(connectionFactory)).isSameAs(connectionFactory); - } + @Nested + class Unwrap { - @Test - void unwrapWithCachingConnectionFactory() { - ConnectionFactory connectionFactory = mock(ConnectionFactory.class); - assertThat(ConnectionFactoryUnwrapper.unwrap(new CachingConnectionFactory(connectionFactory))) - .isSameAs(connectionFactory); - } + @Test + void unwrapWithSingleConnectionFactory() { + ConnectionFactory connectionFactory = new SingleConnectionFactory(); + assertThat(unwrap(connectionFactory)).isSameAs(connectionFactory); + } - @Test - void unwrapWithNestedCachingConnectionFactories() { - ConnectionFactory connectionFactory = mock(ConnectionFactory.class); - CachingConnectionFactory firstCachingConnectionFactory = new CachingConnectionFactory(connectionFactory); - CachingConnectionFactory secondCachingConnectionFactory = new CachingConnectionFactory( - firstCachingConnectionFactory); - assertThat(ConnectionFactoryUnwrapper.unwrap(secondCachingConnectionFactory)).isSameAs(connectionFactory); - } + @Test + void unwrapWithConnectionFactory() { + ConnectionFactory connectionFactory = mock(ConnectionFactory.class); + assertThat(unwrap(connectionFactory)).isSameAs(connectionFactory); + } - @Test - void unwrapWithJmsPoolConnectionFactory() { - ConnectionFactory connectionFactory = mock(ConnectionFactory.class); - JmsPoolConnectionFactory poolConnectionFactory = new JmsPoolConnectionFactory(); - poolConnectionFactory.setConnectionFactory(connectionFactory); - assertThat(ConnectionFactoryUnwrapper.unwrap(poolConnectionFactory)).isSameAs(connectionFactory); - } + @Test + void unwrapWithCachingConnectionFactory() { + ConnectionFactory connectionFactory = mock(ConnectionFactory.class); + assertThat(unwrap(new CachingConnectionFactory(connectionFactory))).isSameAs(connectionFactory); + } - @Test - void unwrapWithNestedJmsPoolConnectionFactories() { - ConnectionFactory connectionFactory = mock(ConnectionFactory.class); - JmsPoolConnectionFactory firstPooledConnectionFactory = new JmsPoolConnectionFactory(); - firstPooledConnectionFactory.setConnectionFactory(connectionFactory); - JmsPoolConnectionFactory secondPooledConnectionFactory = new JmsPoolConnectionFactory(); - secondPooledConnectionFactory.setConnectionFactory(firstPooledConnectionFactory); - assertThat(ConnectionFactoryUnwrapper.unwrap(secondPooledConnectionFactory)).isSameAs(connectionFactory); - } + @Test + void unwrapWithNestedCachingConnectionFactories() { + ConnectionFactory connectionFactory = mock(ConnectionFactory.class); + CachingConnectionFactory firstCachingConnectionFactory = new CachingConnectionFactory(connectionFactory); + CachingConnectionFactory secondCachingConnectionFactory = new CachingConnectionFactory( + firstCachingConnectionFactory); + assertThat(unwrap(secondCachingConnectionFactory)).isSameAs(connectionFactory); + } + + @Test + void unwrapWithJmsPoolConnectionFactory() { + ConnectionFactory connectionFactory = mock(ConnectionFactory.class); + JmsPoolConnectionFactory poolConnectionFactory = new JmsPoolConnectionFactory(); + poolConnectionFactory.setConnectionFactory(connectionFactory); + assertThat(unwrap(poolConnectionFactory)).isSameAs(connectionFactory); + } + + @Test + void unwrapWithNestedJmsPoolConnectionFactories() { + ConnectionFactory connectionFactory = mock(ConnectionFactory.class); + JmsPoolConnectionFactory firstPooledConnectionFactory = new JmsPoolConnectionFactory(); + firstPooledConnectionFactory.setConnectionFactory(connectionFactory); + JmsPoolConnectionFactory secondPooledConnectionFactory = new JmsPoolConnectionFactory(); + secondPooledConnectionFactory.setConnectionFactory(firstPooledConnectionFactory); + assertThat(unwrap(secondPooledConnectionFactory)).isSameAs(connectionFactory); + } + + @Test + @ClassPathExclusions("pooled-jms-*") + void unwrapWithoutJmsPoolOnClasspath() { + assertThat(ClassUtils.isPresent("org.messaginghub.pooled.jms.JmsPoolConnectionFactory", null)).isFalse(); + ConnectionFactory connectionFactory = mock(ConnectionFactory.class); + assertThat(unwrap(new CachingConnectionFactory(connectionFactory))).isSameAs(connectionFactory); + } + + private ConnectionFactory unwrap(ConnectionFactory connectionFactory) { + return ConnectionFactoryUnwrapper.unwrap(connectionFactory); + } - @Test - @ClassPathExclusions("pooled-jms-*") - void unwrapWithoutJmsPoolOnClasspath() { - assertThat(ClassUtils.isPresent("org.messaginghub.pooled.jms.JmsPoolConnectionFactory", null)).isFalse(); - ConnectionFactory connectionFactory = mock(ConnectionFactory.class); - assertThat(ConnectionFactoryUnwrapper.unwrap(new CachingConnectionFactory(connectionFactory))) - .isSameAs(connectionFactory); } }