From 0156eb790ff94de9f08976ef90817dd555d4a9a3 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Thu, 21 Oct 2021 08:45:29 +0200 Subject: [PATCH 1/2] Prepare issue branch. --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index d0ddbb58ea..0d8867beb6 100644 --- a/pom.xml +++ b/pom.xml @@ -5,7 +5,7 @@ org.springframework.data spring-data-commons - 2.6.0-SNAPSHOT + 2.6.0-GH-2485-SNAPSHOT Spring Data Core From 335b62a2dfd6abf24e4fe3306b0c627a4b5e18f4 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Thu, 21 Oct 2021 16:00:25 +0200 Subject: [PATCH 2/2] Create PersistentEntities only for user class in case of proxy instance. This commit makes sure to only create a PersistentEntity for an actual user type. Therefore ClassTypeInformation now considers the given type and not only the user one for both equals and hashcode. This makes sure we can distinguish TypeInformation for a proxy from the one of a user class. The AbstractMapping context will take care of unpacking proxied types and registering the created entity for both the user as well as the proxy type information. Prior to this commit build time generated proxy instances would have been able to pollute the context depending on the order they had been served by the initial entity set. --- .../context/AbstractMappingContext.java | 16 +- .../data/util/ClassTypeInformation.java | 23 +++ .../data/util/TypeInformation.java | 23 +++ .../AbstractMappingContextUnitTests.java | 160 ++++++++++++++++++ .../util/ClassTypeInformationUnitTests.java | 136 +++++++++++++++ 5 files changed, 357 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/springframework/data/mapping/context/AbstractMappingContext.java b/src/main/java/org/springframework/data/mapping/context/AbstractMappingContext.java index a04306336c..83ed7b9d60 100644 --- a/src/main/java/org/springframework/data/mapping/context/AbstractMappingContext.java +++ b/src/main/java/org/springframework/data/mapping/context/AbstractMappingContext.java @@ -360,6 +360,17 @@ protected Optional addPersistentEntity(TypeInformation typeInformation) { return persistentEntity; } + if(typeInformation.isProxyTypeInformation()) { + + TypeInformation userTypeInformation = typeInformation.getUserTypeInformation(); + persistentEntity = persistentEntities.get(userTypeInformation); + + if (persistentEntity != null) { + persistentEntities.put(typeInformation, persistentEntity); + return persistentEntity; + } + } + } finally { read.unlock(); } @@ -369,7 +380,10 @@ protected Optional addPersistentEntity(TypeInformation typeInformation) { try { write.lock(); - entity = doAddPersistentEntity(typeInformation); + entity = doAddPersistentEntity(typeInformation.getUserTypeInformation()); + if(typeInformation.isProxyTypeInformation()) { + persistentEntities.put(typeInformation, Optional.of(entity)); + } } catch (BeansException e) { throw new MappingException(e.getMessage(), e); diff --git a/src/main/java/org/springframework/data/util/ClassTypeInformation.java b/src/main/java/org/springframework/data/util/ClassTypeInformation.java index 98e7132bc3..953b60feed 100644 --- a/src/main/java/org/springframework/data/util/ClassTypeInformation.java +++ b/src/main/java/org/springframework/data/util/ClassTypeInformation.java @@ -32,6 +32,7 @@ import org.springframework.util.Assert; import org.springframework.util.ConcurrentReferenceHashMap; import org.springframework.util.ConcurrentReferenceHashMap.ReferenceType; +import org.springframework.util.ObjectUtils; /** * {@link TypeInformation} for a plain {@link Class}. @@ -177,4 +178,26 @@ public TypeInformation specialize(ClassTypeInformation type) { public String toString() { return type.getName(); } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + + if (!super.equals(o)) { + return false; + } + + ClassTypeInformation that = (ClassTypeInformation) o; + return ObjectUtils.nullSafeEquals(type, that.type); + } + + @Override + public int hashCode() { + + int result = super.hashCode(); + result = 31 * result + ObjectUtils.nullSafeHashCode(type); + return result; + } } diff --git a/src/main/java/org/springframework/data/util/TypeInformation.java b/src/main/java/org/springframework/data/util/TypeInformation.java index d5a2f2642b..25615c6d96 100644 --- a/src/main/java/org/springframework/data/util/TypeInformation.java +++ b/src/main/java/org/springframework/data/util/TypeInformation.java @@ -149,6 +149,29 @@ default TypeInformation getRequiredMapValueType() { */ Class getType(); + /** + * Returns the user type of the property if proxied. + * + * @return the unpacked (if proxied) type of the property. + * @see ProxyUtils#getUserClass(Class) + * @since 2.6 + */ + default TypeInformation getUserTypeInformation() { + + Class userType = ProxyUtils.getUserClass(getType()); + return userType.equals(getType()) ? this : ClassTypeInformation.from(userType); + } + + /** + * Returns if {@link #getType()} refers to a proxy or user class. + * + * @return true if type is a proxy. + * @since 2.6 + */ + default boolean isProxyTypeInformation() { + return !this.equals(getUserTypeInformation()); + } + /** * Returns a {@link ClassTypeInformation} to represent the {@link TypeInformation} of the raw type of the current * instance. diff --git a/src/test/java/org/springframework/data/mapping/context/AbstractMappingContextUnitTests.java b/src/test/java/org/springframework/data/mapping/context/AbstractMappingContextUnitTests.java index f492abf816..ca79972f5d 100755 --- a/src/test/java/org/springframework/data/mapping/context/AbstractMappingContextUnitTests.java +++ b/src/test/java/org/springframework/data/mapping/context/AbstractMappingContextUnitTests.java @@ -37,8 +37,14 @@ import java.util.TreeMap; import java.util.function.Supplier; +import org.aopalliance.aop.Advice; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.springframework.aop.Advisor; +import org.springframework.aop.SpringProxy; +import org.springframework.aop.TargetSource; +import org.springframework.aop.framework.Advised; +import org.springframework.aop.framework.AopConfigException; import org.springframework.context.ApplicationContext; import org.springframework.context.ApplicationEvent; import org.springframework.data.annotation.Id; @@ -54,6 +60,7 @@ import org.springframework.data.util.ClassTypeInformation; import org.springframework.data.util.StreamUtils; import org.springframework.data.util.TypeInformation; +import org.springframework.lang.Nullable; /** * Unit test for {@link AbstractMappingContext}. @@ -323,6 +330,40 @@ void shouldNotCreatePersistentEntityForMapButItsGenericTypeArguments() { .doesNotContain(List.class, Map.class, String.class, Integer.class); } + @Test // GH-2485 + void contextSeesUserTypeBeforeProxy() { + + SampleMappingContext context = new SampleMappingContext(); + BasicPersistentEntity persistentEntity = context.getPersistentEntity(Base.class); + persistentEntity.getTypeInformation().getType().equals(Base.class); + + assertThat(context.hasPersistentEntityFor(Base.class)).isTrue(); + assertThat(context.hasPersistentEntityFor(Base$$SpringProxy$873fa2e.class)).isFalse(); + + BasicPersistentEntity persistentEntityForProxy = context.getPersistentEntity(Base$$SpringProxy$873fa2e.class); + persistentEntityForProxy.getTypeInformation().getType().equals(Base.class); + assertThat(context.hasPersistentEntityFor(Base$$SpringProxy$873fa2e.class)).isTrue(); + + assertThat(context.getPersistentEntities()).hasSize(1); // only one distinct instance + } + + @Test // GH-2485 + void contextSeesProxyBeforeUserType() { + + SampleMappingContext context = new SampleMappingContext(); + + BasicPersistentEntity persistentEntityForProxy = context.getPersistentEntity(Base$$SpringProxy$873fa2e.class); + persistentEntityForProxy.getTypeInformation().getType().equals(Base.class); + + assertThat(context.hasPersistentEntityFor(Base$$SpringProxy$873fa2e.class)).isTrue(); + assertThat(context.hasPersistentEntityFor(Base.class)).isTrue(); + + BasicPersistentEntity persistentEntity = context.getPersistentEntity(Base.class); + persistentEntity.getTypeInformation().getType().equals(Base.class); + + assertThat(context.getPersistentEntities()).hasSize(1); // only one distinct instance + } + private static void assertHasEntityFor(Class type, SampleMappingContext context, boolean expected) { boolean found = false; @@ -505,4 +546,123 @@ class WithMap { Map mapOfKeyToPerson; } + static class Base$$SpringProxy$873fa2e extends Base implements SpringProxy, Advised { + + @Override + public boolean isFrozen() { + return false; + } + + @Override + public boolean isProxyTargetClass() { + return false; + } + + @Override + public Class[] getProxiedInterfaces() { + return new Class[0]; + } + + @Override + public boolean isInterfaceProxied(Class intf) { + return false; + } + + @Override + public void setTargetSource(TargetSource targetSource) { + + } + + @Override + public TargetSource getTargetSource() { + return null; + } + + @Override + public void setExposeProxy(boolean exposeProxy) { + + } + + @Override + public boolean isExposeProxy() { + return false; + } + + @Override + public void setPreFiltered(boolean preFiltered) { + + } + + @Override + public boolean isPreFiltered() { + return false; + } + + @Override + public Advisor[] getAdvisors() { + return new Advisor[0]; + } + + @Override + public void addAdvisor(Advisor advisor) throws AopConfigException { + + } + + @Override + public void addAdvisor(int pos, Advisor advisor) throws AopConfigException { + + } + + @Override + public boolean removeAdvisor(Advisor advisor) { + return false; + } + + @Override + public void removeAdvisor(int index) throws AopConfigException { + + } + + @Override + public int indexOf(Advisor advisor) { + return 0; + } + + @Override + public boolean replaceAdvisor(Advisor a, Advisor b) throws AopConfigException { + return false; + } + + @Override + public void addAdvice(Advice advice) throws AopConfigException { + + } + + @Override + public void addAdvice(int pos, Advice advice) throws AopConfigException { + + } + + @Override + public boolean removeAdvice(Advice advice) { + return false; + } + + @Override + public int indexOf(Advice advice) { + return 0; + } + + @Override + public String toProxyConfigString() { + return null; + } + + @Nullable + @Override + public Class getTargetClass() { + return null; + } + } + } diff --git a/src/test/java/org/springframework/data/util/ClassTypeInformationUnitTests.java b/src/test/java/org/springframework/data/util/ClassTypeInformationUnitTests.java index a327cf28a9..f8487b7780 100755 --- a/src/test/java/org/springframework/data/util/ClassTypeInformationUnitTests.java +++ b/src/test/java/org/springframework/data/util/ClassTypeInformationUnitTests.java @@ -28,8 +28,16 @@ import java.util.Set; import java.util.SortedMap; +import org.aopalliance.aop.Advice; import org.junit.jupiter.api.Test; +import org.springframework.aop.Advisor; +import org.springframework.aop.SpringProxy; +import org.springframework.aop.TargetSource; +import org.springframework.aop.framework.Advised; +import org.springframework.aop.framework.AopConfigException; +import org.springframework.aop.framework.ProxyFactory; import org.springframework.data.mapping.Person; +import org.springframework.lang.Nullable; /** * Unit tests for {@link ClassTypeInformation}. @@ -469,6 +477,15 @@ void discoversMapKeyAndValueTypeFromTypedMap() { assertThat(justMap.getMapValueType().getType()).isEqualTo(Long.class); } + @Test // GH-2485 + public void proxyTypeInformationShouldNotEqualUserClassTypeInfo () { + + ClassTypeInformation typeInfoLeaf = from(Leaf.class); + ClassTypeInformation typeInformationLeafProxy = from(Leaf$$SpringProxy$873fa2e.class); + + assertThat(typeInfoLeaf).isNotEqualTo(typeInformationLeafProxy); + } + static class StringMapContainer extends MapContainer { } @@ -718,4 +735,123 @@ static class TypeWithTypedMap { MultiValueMap longMultiValueMap; Map justMap; } + + static class Leaf$$SpringProxy$873fa2e extends Leaf implements SpringProxy, Advised { + + @Override + public boolean isFrozen() { + return false; + } + + @Override + public boolean isProxyTargetClass() { + return false; + } + + @Override + public Class[] getProxiedInterfaces() { + return new Class[0]; + } + + @Override + public boolean isInterfaceProxied(Class intf) { + return false; + } + + @Override + public void setTargetSource(TargetSource targetSource) { + + } + + @Override + public TargetSource getTargetSource() { + return null; + } + + @Override + public void setExposeProxy(boolean exposeProxy) { + + } + + @Override + public boolean isExposeProxy() { + return false; + } + + @Override + public void setPreFiltered(boolean preFiltered) { + + } + + @Override + public boolean isPreFiltered() { + return false; + } + + @Override + public Advisor[] getAdvisors() { + return new Advisor[0]; + } + + @Override + public void addAdvisor(Advisor advisor) throws AopConfigException { + + } + + @Override + public void addAdvisor(int pos, Advisor advisor) throws AopConfigException { + + } + + @Override + public boolean removeAdvisor(Advisor advisor) { + return false; + } + + @Override + public void removeAdvisor(int index) throws AopConfigException { + + } + + @Override + public int indexOf(Advisor advisor) { + return 0; + } + + @Override + public boolean replaceAdvisor(Advisor a, Advisor b) throws AopConfigException { + return false; + } + + @Override + public void addAdvice(Advice advice) throws AopConfigException { + + } + + @Override + public void addAdvice(int pos, Advice advice) throws AopConfigException { + + } + + @Override + public boolean removeAdvice(Advice advice) { + return false; + } + + @Override + public int indexOf(Advice advice) { + return 0; + } + + @Override + public String toProxyConfigString() { + return null; + } + + @Nullable + @Override + public Class getTargetClass() { + return null; + } + } }