From 941976434241f57ce3fcdd5d4a4660fc7a62b322 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Tue, 3 Jul 2018 10:29:51 +0200 Subject: [PATCH] DATAJPA-1368 - Register ExposeRepositoryInvocationInterceptor to isolate repository method call exposure. We now ship and register ExposeRepositoryInvocationInterceptor to expose proxy method invocations without interfering with ExposeInvocationInterceptor. ExposeRepositoryInvocationInterceptor is a copy of ExposeInvocationInterceptor that holds its own instance of the invoked method and that is isolated so no other component can affect exposure of the invoked method. Interceptors in the invocation chain can invoke methods on proxied objects (e.g. proxied TransactionManager) which override the invoked proxied method as ExposeInvocationInterceptor is a singleton. We previously lost the invoked method details. Original pull request: #280. --- .../CrudMethodMetadataPostProcessor.java | 133 +++++++++++++++--- ...aPopulatingMethodInterceptorUnitTests.java | 11 +- 2 files changed, 122 insertions(+), 22 deletions(-) diff --git a/src/main/java/org/springframework/data/jpa/repository/support/CrudMethodMetadataPostProcessor.java b/src/main/java/org/springframework/data/jpa/repository/support/CrudMethodMetadataPostProcessor.java index 3efc25a10..06ac632a1 100644 --- a/src/main/java/org/springframework/data/jpa/repository/support/CrudMethodMetadataPostProcessor.java +++ b/src/main/java/org/springframework/data/jpa/repository/support/CrudMethodMetadataPostProcessor.java @@ -15,6 +15,7 @@ */ package org.springframework.data.jpa.repository.support; +import java.io.Serializable; import java.lang.reflect.Method; import java.util.Collections; import java.util.HashMap; @@ -27,10 +28,15 @@ import javax.persistence.QueryHint; import org.aopalliance.intercept.MethodInterceptor; import org.aopalliance.intercept.MethodInvocation; + +import org.springframework.aop.Advisor; import org.springframework.aop.TargetSource; import org.springframework.aop.framework.ProxyFactory; import org.springframework.aop.interceptor.ExposeInvocationInterceptor; +import org.springframework.aop.support.DefaultPointcutAdvisor; import org.springframework.beans.factory.BeanClassLoaderAware; +import org.springframework.core.NamedThreadLocal; +import org.springframework.core.PriorityOrdered; import org.springframework.core.annotation.AnnotatedElementUtils; import org.springframework.core.annotation.AnnotationUtils; import org.springframework.data.jpa.repository.EntityGraph; @@ -46,7 +52,7 @@ import org.springframework.util.ClassUtils; * {@link RepositoryProxyPostProcessor} that sets up interceptors to read metadata information from the invoked method. * This is necessary to allow redeclaration of CRUD methods in repository interfaces and configure locking information * or query hints on them. - * + * * @author Oliver Gierke * @author Thomas Darimont * @author Christoph Strobl @@ -55,7 +61,7 @@ class CrudMethodMetadataPostProcessor implements RepositoryProxyPostProcessor, B private ClassLoader classLoader = ClassUtils.getDefaultClassLoader(); - /* + /* * (non-Javadoc) * @see org.springframework.beans.factory.BeanClassLoaderAware#setBeanClassLoader(java.lang.ClassLoader) */ @@ -65,12 +71,14 @@ class CrudMethodMetadataPostProcessor implements RepositoryProxyPostProcessor, B } - /* + /* * (non-Javadoc) * @see org.springframework.data.repository.core.support.RepositoryProxyPostProcessor#postProcess(org.springframework.aop.framework.ProxyFactory, org.springframework.data.repository.core.RepositoryInformation) */ @Override public void postProcess(ProxyFactory factory, RepositoryInformation repositoryInformation) { + + factory.addAdvisor(ExposeRepositoryInvocationInterceptor.ADVISOR); factory.addAdvice(CrudMethodMetadataPopulatingMethodInterceptor.INSTANCE); } @@ -91,18 +99,18 @@ class CrudMethodMetadataPostProcessor implements RepositoryProxyPostProcessor, B /** * {@link MethodInterceptor} to build and cache {@link DefaultCrudMethodMetadata} instances for the invoked methods. * Will bind the found information to a {@link TransactionSynchronizationManager} for later lookup. - * + * * @see DefaultCrudMethodMetadata * @author Oliver Gierke * @author Thomas Darimont */ - static enum CrudMethodMetadataPopulatingMethodInterceptor implements MethodInterceptor { + enum CrudMethodMetadataPopulatingMethodInterceptor implements MethodInterceptor { INSTANCE; private final ConcurrentMap metadataCache = new ConcurrentHashMap(); - /* + /* * (non-Javadoc) * @see org.aopalliance.intercept.MethodInterceptor#invoke(org.aopalliance.intercept.MethodInvocation) */ @@ -139,7 +147,7 @@ class CrudMethodMetadataPostProcessor implements RepositoryProxyPostProcessor, B /** * Default implementation of {@link CrudMethodMetadata} that will inspect the backing method for annotations. - * + * * @author Oliver Gierke * @author Thomas Darimont */ @@ -152,7 +160,7 @@ class CrudMethodMetadataPostProcessor implements RepositoryProxyPostProcessor, B /** * Creates a new {@link DefaultCrudMethodMetadata} for the given {@link Method}. - * + * * @param method must not be {@literal null}. */ public DefaultCrudMethodMetadata(Method method) { @@ -196,7 +204,7 @@ class CrudMethodMetadataPostProcessor implements RepositoryProxyPostProcessor, B return Collections.unmodifiableMap(queryHints); } - /* + /* * (non-Javadoc) * @see org.springframework.data.jpa.repository.support.CrudMethodMetadata#getLockModeType() */ @@ -205,7 +213,7 @@ class CrudMethodMetadataPostProcessor implements RepositoryProxyPostProcessor, B return lockModeType; } - /* + /* * (non-Javadoc) * @see org.springframework.data.jpa.repository.support.CrudMethodMetadata#getQueryHints() */ @@ -214,7 +222,7 @@ class CrudMethodMetadataPostProcessor implements RepositoryProxyPostProcessor, B return queryHints; } - /* + /* * (non-Javadoc) * @see org.springframework.data.jpa.repository.support.CrudMethodMetadata#getEntityGraph() */ @@ -223,7 +231,7 @@ class CrudMethodMetadataPostProcessor implements RepositoryProxyPostProcessor, B return entityGraph; } - /* + /* * (non-Javadoc) * @see org.springframework.data.jpa.repository.support.CrudMethodMetadata#getMethod() */ @@ -235,7 +243,7 @@ class CrudMethodMetadataPostProcessor implements RepositoryProxyPostProcessor, B private static class ThreadBoundTargetSource implements TargetSource { - /* + /* * (non-Javadoc) * @see org.springframework.aop.TargetSource#getTargetClass() */ @@ -244,7 +252,7 @@ class CrudMethodMetadataPostProcessor implements RepositoryProxyPostProcessor, B return CrudMethodMetadata.class; } - /* + /* * (non-Javadoc) * @see org.springframework.aop.TargetSource#isStatic() */ @@ -253,22 +261,113 @@ class CrudMethodMetadataPostProcessor implements RepositoryProxyPostProcessor, B return false; } - /* + /* * (non-Javadoc) * @see org.springframework.aop.TargetSource#getTarget() */ @Override public Object getTarget() throws Exception { - MethodInvocation invocation = ExposeInvocationInterceptor.currentInvocation(); + MethodInvocation invocation = ExposeRepositoryInvocationInterceptor.currentInvocation(); return TransactionSynchronizationManager.getResource(invocation.getMethod()); } - /* + /* * (non-Javadoc) * @see org.springframework.aop.TargetSource#releaseTarget(java.lang.Object) */ @Override public void releaseTarget(Object target) throws Exception {} } + + /** + * Own copy of {@link ExposeInvocationInterceptor} scoped to repository proxy method usage to not conflict with + * {@link ExposeInvocationInterceptor} that might expose nested proxy calls to e.g. proxied transaction managers. + * + * @author Mark Paluch + * @since 1.11.13 + * @see ExposeInvocationInterceptor + */ + @SuppressWarnings("serial") + static class ExposeRepositoryInvocationInterceptor implements MethodInterceptor, PriorityOrdered, Serializable { + + /** + * Singleton instance of this class + */ + static final ExposeRepositoryInvocationInterceptor INSTANCE = new ExposeRepositoryInvocationInterceptor(); + + private static final ThreadLocal invocation = new NamedThreadLocal( + "Current AOP method invocation"); + + /** + * Singleton advisor for this class. Use in preference to {@code INSTANCE} when using Spring AOP, as it prevents the + * need to create a new Advisor to wrap the instance. + */ + static final Advisor ADVISOR = new DefaultPointcutAdvisor(INSTANCE) { + @Override + public String toString() { + return ExposeRepositoryInvocationInterceptor.class.getName() + ".ADVISOR"; + } + }; + + /** + * Ensures that only the canonical instance can be created. + */ + private ExposeRepositoryInvocationInterceptor() {} + + /** + * Return the AOP Alliance {@link MethodInvocation} object associated with the current invocation. + * + * @return the invocation object associated with the current invocation. + * @throws IllegalStateException if there is no AOP invocation in progress, or if the + * {@link ExposeRepositoryInvocationInterceptor} was not added to this interceptor chain. + */ + static MethodInvocation currentInvocation() throws IllegalStateException { + + MethodInvocation mi = invocation.get(); + + if (mi == null) + throw new IllegalStateException( + "No MethodInvocation found: Check that an AOP invocation is in progress, and that the " + + "ExposeRepositoryInvocationInterceptor is upfront in the interceptor chain. Specifically, note that " + + "advices with order HIGHEST_PRECEDENCE will execute before ExposeRepositoryMethodInvocationInterceptor!"); + return mi; + } + + /* + * (non-Javadoc) + * @see org.aopalliance.intercept.MethodInterceptor#invoke(org.aopalliance.intercept.MethodInvocation) + */ + @Override + public Object invoke(MethodInvocation mi) throws Throwable { + + MethodInvocation oldInvocation = invocation.get(); + invocation.set(mi); + + try { + return mi.proceed(); + } finally { + invocation.set(oldInvocation); + } + } + + /* + * (non-Javadoc) + * @see org.springframework.core.Ordered#getOrder() + */ + @Override + public int getOrder() { + return PriorityOrdered.HIGHEST_PRECEDENCE + 1; + } + + /** + * Required to support serialization. Replaces with canonical instance on deserialization, protecting Singleton + * pattern. + *

+ * Alternative to overriding the {@code equals} method. + */ + private Object readResolve() { + return INSTANCE; + } + } } diff --git a/src/test/java/org/springframework/data/jpa/repository/support/CrudMethodMetadataPopulatingMethodInterceptorUnitTests.java b/src/test/java/org/springframework/data/jpa/repository/support/CrudMethodMetadataPopulatingMethodInterceptorUnitTests.java index 08d618492..b84528c26 100644 --- a/src/test/java/org/springframework/data/jpa/repository/support/CrudMethodMetadataPopulatingMethodInterceptorUnitTests.java +++ b/src/test/java/org/springframework/data/jpa/repository/support/CrudMethodMetadataPopulatingMethodInterceptorUnitTests.java @@ -30,15 +30,16 @@ import org.junit.runner.RunWith; import org.mockito.Mock; import org.mockito.runners.MockitoJUnitRunner; import org.springframework.aop.framework.ProxyFactory; -import org.springframework.aop.interceptor.ExposeInvocationInterceptor; import org.springframework.data.jpa.repository.Lock; import org.springframework.data.jpa.repository.support.CrudMethodMetadataPostProcessor.CrudMethodMetadataPopulatingMethodInterceptor; +import org.springframework.data.jpa.repository.support.CrudMethodMetadataPostProcessor.ExposeRepositoryInvocationInterceptor; import org.springframework.transaction.support.TransactionSynchronizationManager; /** * Unit tests for {@link CrudMethodMetadataPopulatingMethodInterceptor}. * * @author Oliver Gierke + * @author Mark Paluch */ @RunWith(MockitoJUnitRunner.class) public class CrudMethodMetadataPopulatingMethodInterceptorUnitTests { @@ -56,8 +57,8 @@ public class CrudMethodMetadataPopulatingMethodInterceptorUnitTests { assertThat(TransactionSynchronizationManager.getResource(method), is(nullValue())); } - @Test // DATAJPA-839 - public void looksUpCrudMethodMetadataForEveryInvocation() throws Throwable { + @Test // DATAJPA-839, DATAJPA-1368 + public void looksUpCrudMethodMetadataForEveryInvocation() { CrudMethodMetadata metadata = new CrudMethodMetadataPostProcessor().getCrudMethodMetadata(); @@ -68,7 +69,7 @@ public class CrudMethodMetadataPopulatingMethodInterceptorUnitTests { private Method prepareMethodInvocation(String name) throws Throwable { Method method = Sample.class.getMethod(name); - ExposeInvocationInterceptor.INSTANCE.invoke(invocation); + ExposeRepositoryInvocationInterceptor.INSTANCE.invoke(invocation); when(invocation.getMethod()).thenReturn(method); return method; @@ -78,7 +79,7 @@ public class CrudMethodMetadataPopulatingMethodInterceptorUnitTests { ProxyFactory factory = new ProxyFactory(new Object()); factory.addInterface(Sample.class); - factory.addAdvice(ExposeInvocationInterceptor.INSTANCE); + factory.addAdvice(ExposeRepositoryInvocationInterceptor.INSTANCE); factory.addAdvice(CrudMethodMetadataPopulatingMethodInterceptor.INSTANCE); factory.addAdvice(new MethodInterceptor() {