From 26f9a928378314bfb306bc4f324f3fb21d37f6d3 Mon Sep 17 00:00:00 2001 From: Tadaya Tsuyukubo Date: Sun, 18 Nov 2018 08:00:11 -0800 Subject: [PATCH 1/3] Perform best effort to retrieve DataSourceProxy Prior to this commit, `DataSourceJmxConfiguration` with tomcat `DataSource`, it can only find `DataSourceProxy` if the given `DataSource` is a direct child of it. Since it uses `instanceof`, it could not find `DataSourceProxy` if the `DataSource` is wrapped(delegated) or proxied. This is because `DataSourceProxy#unwrap()` always returns null; thus cannot use this method to directly obtain `DataSourceProxy`. In this commit, updated the check logic to perform the best effort to retrieve `DataSourceProxy`. If given `DataSource` is wrapped or proxied by spring, tries to unwrap or get target datasource recursively to find `DataSourceProxy`. See gh-15206 --- .../jdbc/DataSourceJmxConfiguration.java | 39 +++++- .../jdbc/DataSourceJmxConfigurationTests.java | 125 +++++++++++++++++- 2 files changed, 160 insertions(+), 4 deletions(-) diff --git a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jdbc/DataSourceJmxConfiguration.java b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jdbc/DataSourceJmxConfiguration.java index 9195430b8f..fc1f111433 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jdbc/DataSourceJmxConfiguration.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jdbc/DataSourceJmxConfiguration.java @@ -26,6 +26,8 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.apache.tomcat.jdbc.pool.DataSourceProxy; +import org.springframework.aop.framework.AopProxyUtils; +import org.springframework.aop.support.AopUtils; import org.springframework.beans.factory.ObjectProvider; import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; @@ -33,12 +35,14 @@ import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.boot.autoconfigure.condition.ConditionalOnSingleCandidate; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; +import org.springframework.jdbc.datasource.DelegatingDataSource; import org.springframework.jmx.export.MBeanExporter; /** * Configures DataSource related MBeans. * * @author Stephane Nicoll + * @author Tadaya Tsuyukubo */ @Configuration @ConditionalOnProperty(prefix = "spring.jmx", name = "enabled", havingValue = "true", matchIfMissing = true) @@ -89,9 +93,10 @@ class DataSourceJmxConfiguration { @Bean @ConditionalOnMissingBean(name = "dataSourceMBean") public Object dataSourceMBean(DataSource dataSource) { - if (dataSource instanceof DataSourceProxy) { + DataSourceProxy dataSourceProxy = extractDataSourceProxy(dataSource); + if (dataSourceProxy != null) { try { - return ((DataSourceProxy) dataSource).createPool().getJmxPool(); + return dataSourceProxy.createPool().getJmxPool(); } catch (SQLException ex) { logger.warn("Cannot expose DataSource to JMX (could not connect)"); @@ -100,6 +105,36 @@ class DataSourceJmxConfiguration { return null; } + /** + * Perform best effort to retrieve tomcat's {@link DataSourceProxy}. + * + * Since {@link DataSourceProxy#unwrap(Class)} always return {@code null}, it + * cannot directly retrieve {@link DataSourceProxy}. This method tries best effort + * to find {@link DataSourceProxy} if the given {@link DataSource} is wrapped or + * proxied by spring. + * @param dataSource candidate datasource + * @return found DataSourceProxy or null + */ + private DataSourceProxy extractDataSourceProxy(DataSource dataSource) { + if (dataSource instanceof DataSourceProxy) { + return (DataSourceProxy) dataSource; // found + } + else if (dataSource instanceof DelegatingDataSource) { + // check delegating target + return extractDataSourceProxy( + ((DelegatingDataSource) dataSource).getTargetDataSource()); + } + else if (AopUtils.isAopProxy(dataSource)) { + // for proxy by spring, try target(advised) instance + Object target = AopProxyUtils.getSingletonTarget(dataSource); + if (target instanceof DataSource) { + return extractDataSourceProxy((DataSource) target); + } + } + + return null; + } + } } diff --git a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/jdbc/DataSourceJmxConfigurationTests.java b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/jdbc/DataSourceJmxConfigurationTests.java index 2af9d60b45..2f20b76d90 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/jdbc/DataSourceJmxConfigurationTests.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/jdbc/DataSourceJmxConfigurationTests.java @@ -31,13 +31,16 @@ import org.apache.tomcat.jdbc.pool.DataSourceProxy; import org.apache.tomcat.jdbc.pool.jmx.ConnectionPool; import org.junit.Test; +import org.springframework.aop.framework.AopProxyUtils; import org.springframework.aop.framework.ProxyFactory; +import org.springframework.aop.support.AopUtils; import org.springframework.beans.factory.config.BeanPostProcessor; import org.springframework.boot.autoconfigure.AutoConfigurations; import org.springframework.boot.autoconfigure.jmx.JmxAutoConfiguration; import org.springframework.boot.test.context.runner.ApplicationContextRunner; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; +import org.springframework.jdbc.datasource.DelegatingDataSource; import static org.assertj.core.api.Assertions.assertThat; @@ -45,6 +48,7 @@ import static org.assertj.core.api.Assertions.assertThat; * Tests for {@link DataSourceJmxConfiguration}. * * @author Stephane Nicoll + * @author Tadaya Tsuyukubo */ public class DataSourceJmxConfigurationTests { @@ -162,6 +166,78 @@ public class DataSourceJmxConfigurationTests { }); } + @Test + public void tomcatProxiedCanExposeMBeanPool() { + this.contextRunner.withUserConfiguration(DataSourceProxyConfiguration.class) + .withPropertyValues( + "spring.datasource.type=" + DataSource.class.getName(), + "spring.datasource.jmx-enabled=true") + .run((context) -> { + assertThat(context).hasSingleBean(ConnectionPool.class); + DataSourceProxy dataSourceProxy = (DataSourceProxy) AopProxyUtils + .getSingletonTarget( + context.getBean(javax.sql.DataSource.class)); + assertThat(dataSourceProxy.createPool().getJmxPool()) + .isSameAs(context.getBean(ConnectionPool.class)); + }); + } + + @Test + public void tomcatDelegateCanExposeMBeanPool() { + this.contextRunner.withUserConfiguration(DataSourceDelegateConfiguration.class) + .withPropertyValues( + "spring.datasource.type=" + DataSource.class.getName(), + "spring.datasource.jmx-enabled=true") + .run((context) -> { + assertThat(context).hasSingleBean(ConnectionPool.class); + DataSourceProxy dataSourceProxy = (DataSourceProxy) context + .getBean(DelegatingDataSource.class).getTargetDataSource(); + assertThat(dataSourceProxy.createPool().getJmxPool()) + .isSameAs(context.getBean(ConnectionPool.class)); + }); + } + + @Test + public void tomcatProxyAndDelegateCanExposeMBeanPool() { + this.contextRunner + .withUserConfiguration(DataSourceMixWrapAndProxyConfiguration.class) + .withPropertyValues( + "spring.datasource.type=" + DataSource.class.getName(), + "spring.datasource.jmx-enabled=true") + .run((context) -> { + assertThat(context).hasSingleBean(ConnectionPool.class); + DataSourceProxy dataSourceProxy = extractTomcatDataSource( + context.getBean(javax.sql.DataSource.class)); + assertThat(dataSourceProxy.createPool().getJmxPool()) + .isSameAs(context.getBean(ConnectionPool.class)); + }); + } + + private static javax.sql.DataSource wrap(javax.sql.DataSource dataSource) { + return (javax.sql.DataSource) new ProxyFactory(dataSource).getProxy(); + } + + private static javax.sql.DataSource delegate(javax.sql.DataSource dataSource) { + return new DelegatingDataSource(dataSource); + } + + private static DataSource extractTomcatDataSource(javax.sql.DataSource dataSource) { + if (dataSource instanceof DataSource) { + return (DataSource) dataSource; + } + else if (dataSource instanceof DelegatingDataSource) { + return extractTomcatDataSource( + ((DelegatingDataSource) dataSource).getTargetDataSource()); + } + else if (AopUtils.isAopProxy(dataSource)) { + return extractTomcatDataSource( + (javax.sql.DataSource) AopProxyUtils.getSingletonTarget(dataSource)); + } + + throw new RuntimeException( + "Not proxied or delegated tomcat DataSource: " + dataSource); + } + @Configuration static class DataSourceProxyConfiguration { @@ -182,8 +258,53 @@ public class DataSourceJmxConfigurationTests { return bean; } - private static javax.sql.DataSource wrap(javax.sql.DataSource dataSource) { - return (javax.sql.DataSource) new ProxyFactory(dataSource).getProxy(); + } + + @Configuration + static class DataSourceDelegateConfiguration { + + @Bean + public static DataSourceBeanPostProcessor dataSourceBeanPostProcessor() { + return new DataSourceBeanPostProcessor() { + @Override + public Object postProcessAfterInitialization(Object bean, + String beanName) { + if (bean instanceof javax.sql.DataSource) { + return new DelegatingDataSource((javax.sql.DataSource) bean); + } + return bean; + } + }; + } + + } + + @Configuration + static class DataSourceMixWrapAndProxyConfiguration { + + @Bean + public static DataSourceBeanPostProcessor dataSourceBeanPostProcessor() { + return new DataSourceBeanPostProcessor() { + @Override + public Object postProcessAfterInitialization(Object bean, + String beanName) { + if (bean instanceof javax.sql.DataSource) { + javax.sql.DataSource dataSource = (javax.sql.DataSource) bean; + // delegate/wrap multiple times + for (int i = 0; i < 10; i++) { + if (i % 2 == 0) { + dataSource = wrap(dataSource); + } + else { + dataSource = delegate(dataSource); + } + } + + return dataSource; + } + return bean; + } + }; } } From e424dfbe15570b013f05600389ce73b4f7e6cbf9 Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Fri, 23 Nov 2018 18:34:33 +0100 Subject: [PATCH 2/3] Polish "Perform best effort to retrieve DataSourceProxy" Closes gh-15206 --- .../jdbc/DataSourceJmxConfiguration.java | 38 +------ .../jdbc/DataSourceJmxConfigurationTests.java | 93 ++-------------- .../boot/jdbc/DataSourceUnwrapper.java | 97 +++++++++++++++++ .../DataSourceUnwrapperNoSpringJdbcTests.java | 61 +++++++++++ .../boot/jdbc/DataSourceUnwrapperTests.java | 102 ++++++++++++++++++ 5 files changed, 271 insertions(+), 120 deletions(-) create mode 100644 spring-boot-project/spring-boot/src/main/java/org/springframework/boot/jdbc/DataSourceUnwrapper.java create mode 100644 spring-boot-project/spring-boot/src/test/java/org/springframework/boot/jdbc/DataSourceUnwrapperNoSpringJdbcTests.java create mode 100644 spring-boot-project/spring-boot/src/test/java/org/springframework/boot/jdbc/DataSourceUnwrapperTests.java diff --git a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jdbc/DataSourceJmxConfiguration.java b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jdbc/DataSourceJmxConfiguration.java index fc1f111433..c946d843e0 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jdbc/DataSourceJmxConfiguration.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jdbc/DataSourceJmxConfiguration.java @@ -26,23 +26,20 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.apache.tomcat.jdbc.pool.DataSourceProxy; -import org.springframework.aop.framework.AopProxyUtils; -import org.springframework.aop.support.AopUtils; import org.springframework.beans.factory.ObjectProvider; import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.boot.autoconfigure.condition.ConditionalOnSingleCandidate; +import org.springframework.boot.jdbc.DataSourceUnwrapper; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; -import org.springframework.jdbc.datasource.DelegatingDataSource; import org.springframework.jmx.export.MBeanExporter; /** * Configures DataSource related MBeans. * * @author Stephane Nicoll - * @author Tadaya Tsuyukubo */ @Configuration @ConditionalOnProperty(prefix = "spring.jmx", name = "enabled", havingValue = "true", matchIfMissing = true) @@ -93,7 +90,8 @@ class DataSourceJmxConfiguration { @Bean @ConditionalOnMissingBean(name = "dataSourceMBean") public Object dataSourceMBean(DataSource dataSource) { - DataSourceProxy dataSourceProxy = extractDataSourceProxy(dataSource); + DataSourceProxy dataSourceProxy = DataSourceUnwrapper.unwrap(dataSource, + DataSourceProxy.class); if (dataSourceProxy != null) { try { return dataSourceProxy.createPool().getJmxPool(); @@ -105,36 +103,6 @@ class DataSourceJmxConfiguration { return null; } - /** - * Perform best effort to retrieve tomcat's {@link DataSourceProxy}. - * - * Since {@link DataSourceProxy#unwrap(Class)} always return {@code null}, it - * cannot directly retrieve {@link DataSourceProxy}. This method tries best effort - * to find {@link DataSourceProxy} if the given {@link DataSource} is wrapped or - * proxied by spring. - * @param dataSource candidate datasource - * @return found DataSourceProxy or null - */ - private DataSourceProxy extractDataSourceProxy(DataSource dataSource) { - if (dataSource instanceof DataSourceProxy) { - return (DataSourceProxy) dataSource; // found - } - else if (dataSource instanceof DelegatingDataSource) { - // check delegating target - return extractDataSourceProxy( - ((DelegatingDataSource) dataSource).getTargetDataSource()); - } - else if (AopUtils.isAopProxy(dataSource)) { - // for proxy by spring, try target(advised) instance - Object target = AopProxyUtils.getSingletonTarget(dataSource); - if (target instanceof DataSource) { - return extractDataSourceProxy((DataSource) target); - } - } - - return null; - } - } } diff --git a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/jdbc/DataSourceJmxConfigurationTests.java b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/jdbc/DataSourceJmxConfigurationTests.java index 2f20b76d90..38e5870abc 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/jdbc/DataSourceJmxConfigurationTests.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/jdbc/DataSourceJmxConfigurationTests.java @@ -31,9 +31,7 @@ import org.apache.tomcat.jdbc.pool.DataSourceProxy; import org.apache.tomcat.jdbc.pool.jmx.ConnectionPool; import org.junit.Test; -import org.springframework.aop.framework.AopProxyUtils; import org.springframework.aop.framework.ProxyFactory; -import org.springframework.aop.support.AopUtils; import org.springframework.beans.factory.config.BeanPostProcessor; import org.springframework.boot.autoconfigure.AutoConfigurations; import org.springframework.boot.autoconfigure.jmx.JmxAutoConfiguration; @@ -159,6 +157,7 @@ public class DataSourceJmxConfigurationTests { this.contextRunner.withPropertyValues( "spring.datasource.type=" + DataSource.class.getName(), "spring.datasource.jmx-enabled=true").run((context) -> { + assertThat(context).hasBean("dataSourceMBean"); assertThat(context).hasSingleBean(ConnectionPool.class); assertThat(context.getBean(DataSourceProxy.class).createPool() .getJmxPool()) @@ -173,12 +172,9 @@ public class DataSourceJmxConfigurationTests { "spring.datasource.type=" + DataSource.class.getName(), "spring.datasource.jmx-enabled=true") .run((context) -> { - assertThat(context).hasSingleBean(ConnectionPool.class); - DataSourceProxy dataSourceProxy = (DataSourceProxy) AopProxyUtils - .getSingletonTarget( - context.getBean(javax.sql.DataSource.class)); - assertThat(dataSourceProxy.createPool().getJmxPool()) - .isSameAs(context.getBean(ConnectionPool.class)); + assertThat(context).hasBean("dataSourceMBean"); + assertThat(context).getBean("dataSourceMBean") + .isInstanceOf(ConnectionPool.class); }); } @@ -189,55 +185,12 @@ public class DataSourceJmxConfigurationTests { "spring.datasource.type=" + DataSource.class.getName(), "spring.datasource.jmx-enabled=true") .run((context) -> { - assertThat(context).hasSingleBean(ConnectionPool.class); - DataSourceProxy dataSourceProxy = (DataSourceProxy) context - .getBean(DelegatingDataSource.class).getTargetDataSource(); - assertThat(dataSourceProxy.createPool().getJmxPool()) - .isSameAs(context.getBean(ConnectionPool.class)); - }); - } - - @Test - public void tomcatProxyAndDelegateCanExposeMBeanPool() { - this.contextRunner - .withUserConfiguration(DataSourceMixWrapAndProxyConfiguration.class) - .withPropertyValues( - "spring.datasource.type=" + DataSource.class.getName(), - "spring.datasource.jmx-enabled=true") - .run((context) -> { - assertThat(context).hasSingleBean(ConnectionPool.class); - DataSourceProxy dataSourceProxy = extractTomcatDataSource( - context.getBean(javax.sql.DataSource.class)); - assertThat(dataSourceProxy.createPool().getJmxPool()) - .isSameAs(context.getBean(ConnectionPool.class)); + assertThat(context).hasBean("dataSourceMBean"); + assertThat(context).getBean("dataSourceMBean") + .isInstanceOf(ConnectionPool.class); }); } - private static javax.sql.DataSource wrap(javax.sql.DataSource dataSource) { - return (javax.sql.DataSource) new ProxyFactory(dataSource).getProxy(); - } - - private static javax.sql.DataSource delegate(javax.sql.DataSource dataSource) { - return new DelegatingDataSource(dataSource); - } - - private static DataSource extractTomcatDataSource(javax.sql.DataSource dataSource) { - if (dataSource instanceof DataSource) { - return (DataSource) dataSource; - } - else if (dataSource instanceof DelegatingDataSource) { - return extractTomcatDataSource( - ((DelegatingDataSource) dataSource).getTargetDataSource()); - } - else if (AopUtils.isAopProxy(dataSource)) { - return extractTomcatDataSource( - (javax.sql.DataSource) AopProxyUtils.getSingletonTarget(dataSource)); - } - - throw new RuntimeException( - "Not proxied or delegated tomcat DataSource: " + dataSource); - } - @Configuration static class DataSourceProxyConfiguration { @@ -253,7 +206,7 @@ public class DataSourceJmxConfigurationTests { @Override public Object postProcessAfterInitialization(Object bean, String beanName) { if (bean instanceof javax.sql.DataSource) { - return wrap((javax.sql.DataSource) bean); + return new ProxyFactory(bean).getProxy(); } return bean; } @@ -279,34 +232,4 @@ public class DataSourceJmxConfigurationTests { } - @Configuration - static class DataSourceMixWrapAndProxyConfiguration { - - @Bean - public static DataSourceBeanPostProcessor dataSourceBeanPostProcessor() { - return new DataSourceBeanPostProcessor() { - @Override - public Object postProcessAfterInitialization(Object bean, - String beanName) { - if (bean instanceof javax.sql.DataSource) { - javax.sql.DataSource dataSource = (javax.sql.DataSource) bean; - // delegate/wrap multiple times - for (int i = 0; i < 10; i++) { - if (i % 2 == 0) { - dataSource = wrap(dataSource); - } - else { - dataSource = delegate(dataSource); - } - } - - return dataSource; - } - return bean; - } - }; - } - - } - } diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/jdbc/DataSourceUnwrapper.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/jdbc/DataSourceUnwrapper.java new file mode 100644 index 0000000000..cdc86eed31 --- /dev/null +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/jdbc/DataSourceUnwrapper.java @@ -0,0 +1,97 @@ +/* + * Copyright 2012-2018 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.boot.jdbc; + +import java.sql.Wrapper; + +import javax.sql.DataSource; + +import org.springframework.aop.framework.AopProxyUtils; +import org.springframework.aop.support.AopUtils; +import org.springframework.jdbc.datasource.DelegatingDataSource; +import org.springframework.util.ClassUtils; + +/** + * Unwraps a {@link DataSource} that may have been proxied or wrapped in a custom + * {@link Wrapper} such as {@link DelegatingDataSource}. + * + * @author Tadaya Tsuyukubo + * @author Stephane Nicoll + * @since 2.0.7 + */ +public final class DataSourceUnwrapper { + + private static final boolean DELEGATING_DATA_SOURCE_PRESENT = ClassUtils.isPresent( + "org.springframework.jdbc.datasource.DelegatingDataSource", + DataSourceUnwrapper.class.getClassLoader()); + + private DataSourceUnwrapper() { + } + + /** + * Return an object that implements the given {@code target} type, unwrapping delegate + * or proxy if necessary. + * @param dataSource the datasource to handle + * @param target the type that the result must implement + * @param the target type + * @return an object that implements the target type or {@code null} + */ + public static T unwrap(DataSource dataSource, Class target) { + if (target.isInstance(dataSource)) { + return target.cast(dataSource); + } + T unwrapped = safeUnwrap(dataSource, target); + if (unwrapped != null) { + return unwrapped; + } + if (DELEGATING_DATA_SOURCE_PRESENT) { + DataSource targetDataSource = DelegatingDataSourceUnwrapper + .getTargetDataSource(dataSource); + if (targetDataSource != null) { + return unwrap(targetDataSource, target); + } + } + if (AopUtils.isAopProxy(dataSource)) { + Object proxyTarget = AopProxyUtils.getSingletonTarget(dataSource); + if (proxyTarget instanceof DataSource) { + return unwrap((DataSource) proxyTarget, target); + } + } + return null; + } + + private static S safeUnwrap(Wrapper wrapper, Class target) { + try { + return wrapper.unwrap(target); + } + catch (Exception ex) { + return null; + } + } + + private static class DelegatingDataSourceUnwrapper { + + private static DataSource getTargetDataSource(DataSource dataSource) { + if (dataSource instanceof DelegatingDataSource) { + return ((DelegatingDataSource) dataSource).getTargetDataSource(); + } + return null; + } + + } + +} diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/jdbc/DataSourceUnwrapperNoSpringJdbcTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/jdbc/DataSourceUnwrapperNoSpringJdbcTests.java new file mode 100644 index 0000000000..d3cdfb3b83 --- /dev/null +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/jdbc/DataSourceUnwrapperNoSpringJdbcTests.java @@ -0,0 +1,61 @@ +/* + * Copyright 2012-2018 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.boot.jdbc; + +import javax.sql.DataSource; + +import com.zaxxer.hikari.HikariDataSource; +import org.apache.tomcat.jdbc.pool.DataSourceProxy; +import org.junit.Test; +import org.junit.runner.RunWith; + +import org.springframework.aop.framework.ProxyFactory; +import org.springframework.boot.testsupport.runner.classpath.ClassPathExclusions; +import org.springframework.boot.testsupport.runner.classpath.ModifiedClassPathRunner; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Integration tests for {@link DataSourceUnwrapper} when spring-jdbc is not available. + * + * @author Stephane Nicoll + */ +@RunWith(ModifiedClassPathRunner.class) +@ClassPathExclusions("spring-jdbc-*.jar") +public class DataSourceUnwrapperNoSpringJdbcTests { + + @Test + public void unwrapWithProxy() { + DataSource dataSource = new HikariDataSource(); + DataSource actual = wrapInProxy(wrapInProxy(dataSource)); + assertThat(DataSourceUnwrapper.unwrap(actual, HikariDataSource.class)) + .isSameAs(dataSource); + } + + @Test + public void unwrapDataSourceProxy() { + org.apache.tomcat.jdbc.pool.DataSource dataSource = new org.apache.tomcat.jdbc.pool.DataSource(); + DataSource actual = wrapInProxy(wrapInProxy(dataSource)); + assertThat(DataSourceUnwrapper.unwrap(actual, DataSourceProxy.class)) + .isSameAs(dataSource); + } + + private DataSource wrapInProxy(DataSource dataSource) { + return (DataSource) new ProxyFactory(dataSource).getProxy(); + } + +} diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/jdbc/DataSourceUnwrapperTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/jdbc/DataSourceUnwrapperTests.java new file mode 100644 index 0000000000..0f3d9fb3c1 --- /dev/null +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/jdbc/DataSourceUnwrapperTests.java @@ -0,0 +1,102 @@ +/* + * Copyright 2012-2018 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.boot.jdbc; + +import javax.sql.DataSource; + +import com.zaxxer.hikari.HikariDataSource; +import org.apache.tomcat.jdbc.pool.DataSourceProxy; +import org.junit.Test; + +import org.springframework.aop.framework.ProxyFactory; +import org.springframework.jdbc.datasource.DelegatingDataSource; +import org.springframework.jdbc.datasource.SingleConnectionDataSource; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Tests for {@link DataSourceUnwrapper}. + * + * @author Stephane Nicoll + */ +public class DataSourceUnwrapperTests { + + @Test + public void unwrapWithTarget() { + DataSource dataSource = new HikariDataSource(); + assertThat(DataSourceUnwrapper.unwrap(dataSource, HikariDataSource.class)) + .isSameAs(dataSource); + } + + @Test + public void unwrapWithWrongTarget() { + DataSource dataSource = new HikariDataSource(); + assertThat( + DataSourceUnwrapper.unwrap(dataSource, SingleConnectionDataSource.class)) + .isNull(); + } + + @Test + public void unwrapWithDelegate() { + DataSource dataSource = new HikariDataSource(); + DataSource actual = wrapInDelegate(wrapInDelegate(dataSource)); + assertThat(DataSourceUnwrapper.unwrap(actual, HikariDataSource.class)) + .isSameAs(dataSource); + } + + @Test + public void unwrapWithProxy() { + DataSource dataSource = new HikariDataSource(); + DataSource actual = wrapInProxy(wrapInProxy(dataSource)); + assertThat(DataSourceUnwrapper.unwrap(actual, HikariDataSource.class)) + .isSameAs(dataSource); + } + + @Test + public void unwrapWithProxyAndDelegate() { + DataSource dataSource = new HikariDataSource(); + DataSource actual = wrapInProxy(wrapInDelegate(dataSource)); + assertThat(DataSourceUnwrapper.unwrap(actual, HikariDataSource.class)) + .isSameAs(dataSource); + } + + @Test + public void unwrapWithSeveralLevelOfWrapping() { + DataSource dataSource = new HikariDataSource(); + DataSource actual = wrapInProxy(wrapInDelegate( + wrapInDelegate((wrapInProxy(wrapInDelegate(dataSource)))))); + assertThat(DataSourceUnwrapper.unwrap(actual, HikariDataSource.class)) + .isSameAs(dataSource); + } + + @Test + public void unwrapDataSourceProxy() { + org.apache.tomcat.jdbc.pool.DataSource dataSource = new org.apache.tomcat.jdbc.pool.DataSource(); + DataSource actual = wrapInDelegate(wrapInProxy(dataSource)); + assertThat(DataSourceUnwrapper.unwrap(actual, DataSourceProxy.class)) + .isSameAs(dataSource); + } + + private DataSource wrapInProxy(DataSource dataSource) { + return (DataSource) new ProxyFactory(dataSource).getProxy(); + } + + private DataSource wrapInDelegate(DataSource dataSource) { + return new DelegatingDataSource(dataSource); + } + +} From 5f0ac46d785eacffe846fec54232e747dff877c5 Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Mon, 26 Nov 2018 10:35:10 +0100 Subject: [PATCH 3/3] Unwrap DataSource target rather than plain instanceof calls Closes gh-15227 --- .../jdbc/DataSourceJmxConfiguration.java | 12 ++-------- ...rcePoolMetadataProvidersConfiguration.java | 24 +++++++++++-------- 2 files changed, 16 insertions(+), 20 deletions(-) diff --git a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jdbc/DataSourceJmxConfiguration.java b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jdbc/DataSourceJmxConfiguration.java index c946d843e0..44ee849305 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jdbc/DataSourceJmxConfiguration.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jdbc/DataSourceJmxConfiguration.java @@ -63,22 +63,14 @@ class DataSourceJmxConfiguration { @PostConstruct public void validateMBeans() { - HikariDataSource hikariDataSource = unwrapHikariDataSource(); + HikariDataSource hikariDataSource = DataSourceUnwrapper + .unwrap(this.dataSource, HikariDataSource.class); if (hikariDataSource != null && hikariDataSource.isRegisterMbeans()) { this.mBeanExporter .ifUnique((exporter) -> exporter.addExcludedBean("dataSource")); } } - private HikariDataSource unwrapHikariDataSource() { - try { - return this.dataSource.unwrap(HikariDataSource.class); - } - catch (SQLException ex) { - return null; - } - } - } @Configuration diff --git a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jdbc/metadata/DataSourcePoolMetadataProvidersConfiguration.java b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jdbc/metadata/DataSourcePoolMetadataProvidersConfiguration.java index 88b1afec01..2ccf48de95 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jdbc/metadata/DataSourcePoolMetadataProvidersConfiguration.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jdbc/metadata/DataSourcePoolMetadataProvidersConfiguration.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2017 the original author or authors. + * Copyright 2012-2018 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -20,6 +20,7 @@ import com.zaxxer.hikari.HikariDataSource; import org.apache.commons.dbcp2.BasicDataSource; import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; +import org.springframework.boot.jdbc.DataSourceUnwrapper; import org.springframework.boot.jdbc.metadata.CommonsDbcp2DataSourcePoolMetadata; import org.springframework.boot.jdbc.metadata.DataSourcePoolMetadataProvider; import org.springframework.boot.jdbc.metadata.HikariDataSourcePoolMetadata; @@ -44,9 +45,10 @@ public class DataSourcePoolMetadataProvidersConfiguration { @Bean public DataSourcePoolMetadataProvider tomcatPoolDataSourceMetadataProvider() { return (dataSource) -> { - if (dataSource instanceof org.apache.tomcat.jdbc.pool.DataSource) { - return new TomcatDataSourcePoolMetadata( - (org.apache.tomcat.jdbc.pool.DataSource) dataSource); + org.apache.tomcat.jdbc.pool.DataSource tomcatDataSource = DataSourceUnwrapper + .unwrap(dataSource, org.apache.tomcat.jdbc.pool.DataSource.class); + if (tomcatDataSource != null) { + return new TomcatDataSourcePoolMetadata(tomcatDataSource); } return null; }; @@ -61,9 +63,10 @@ public class DataSourcePoolMetadataProvidersConfiguration { @Bean public DataSourcePoolMetadataProvider hikariPoolDataSourceMetadataProvider() { return (dataSource) -> { - if (dataSource instanceof HikariDataSource) { - return new HikariDataSourcePoolMetadata( - (HikariDataSource) dataSource); + HikariDataSource hikariDataSource = DataSourceUnwrapper.unwrap(dataSource, + HikariDataSource.class); + if (hikariDataSource != null) { + return new HikariDataSourcePoolMetadata(hikariDataSource); } return null; }; @@ -78,9 +81,10 @@ public class DataSourcePoolMetadataProvidersConfiguration { @Bean public DataSourcePoolMetadataProvider commonsDbcp2PoolDataSourceMetadataProvider() { return (dataSource) -> { - if (dataSource instanceof BasicDataSource) { - return new CommonsDbcp2DataSourcePoolMetadata( - (BasicDataSource) dataSource); + BasicDataSource dbcpDataSource = DataSourceUnwrapper.unwrap(dataSource, + BasicDataSource.class); + if (dbcpDataSource != null) { + return new CommonsDbcp2DataSourcePoolMetadata(dbcpDataSource); } return null; };