Unify entity type detection.

Unified the calculations made for entityTypeInformation and entityTypes in AbstractPersistentProperty. This avoids both calculations getting out of sync. Also we avoid premature calculation abortions if SimpleTypeHolder.isSimpleType(…) returns true for the raw property type. The latter has caused issues for collection properties in Spring Data KeyValue which considers everything in java.util a simple type.

Related ticket: #2390.

Author: odrotbohm

src/main/java/org/springframework/data/mapping/model/AbstractPersistentProperty.java

@@ -18,19 +18,17 @@
 import java.lang.reflect.Field;
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
-import java.util.Collection;
 import java.util.Collections;
-import java.util.LinkedHashSet;
+import java.util.HashSet;
 import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
-import java.util.function.Supplier;
+import java.util.stream.Collectors;
 
 import org.springframework.data.mapping.Association;
 import org.springframework.data.mapping.PersistentEntity;
 import org.springframework.data.mapping.PersistentProperty;
 import org.springframework.data.util.Lazy;
-import org.springframework.data.util.NullableWrapperConverters;
 import org.springframework.data.util.ReflectionUtils;
 import org.springframework.data.util.TypeInformation;
 import org.springframework.lang.Nullable;
@@ -64,11 +62,10 @@ public abstract class AbstractPersistentProperty<P extends PersistentProperty<P>
 	private final Property property;
 	private final Lazy<Integer> hashCode;
 	private final Lazy<Boolean> usePropertyAccess;
-	private final Lazy<TypeInformation<?>> entityTypeInformation;
+	private final Lazy<Set<TypeInformation<?>>> entityTypeInformation;
 
 	private final Lazy<Boolean> isAssociation;
 	private final Lazy<TypeInformation<?>> associationTargetType;
-	private final Lazy<Collection<TypeInformation<?>>> entityTypes;
 
 	private final Method getter;
 	private final Method setter;
@@ -100,10 +97,7 @@ public AbstractPersistentProperty(Property property, PersistentEntity<?, P> owne
 						.map(TypeInformation::getComponentType) //
 						.orElse(null));
 
-		this.entityTypeInformation = Lazy.of(() -> Optional.ofNullable(getAssociationOrActualType())
-				.filter(it -> !simpleTypeHolder.isSimpleType(it.getType())) //
-				.filter(it -> !it.isCollectionLike()) //
-				.filter(it -> !it.isMap()).orElse(null));
+		this.entityTypeInformation = Lazy.of(() -> detectEntityTypes(simpleTypeHolder));
 
 		this.getter = property.getGetter().orElse(null);
 		this.setter = property.getSetter().orElse(null);
@@ -115,45 +109,6 @@ public AbstractPersistentProperty(Property property, PersistentEntity<?, P> owne
 		} else {
 			this.immutable = false;
 		}
-
-		this.entityTypes = Lazy.of(() -> collectEntityTypes(simpleTypeHolder, information, new LinkedHashSet<>()));
-	}
-
-	protected Set<TypeInformation<?>> collectEntityTypes(SimpleTypeHolder simpleTypeHolder,
-			@Nullable TypeInformation<?> typeInformation, Set<TypeInformation<?>> entityTypes) {
-
-		if (typeInformation == null || entityTypes.contains(typeInformation)
-				|| simpleTypeHolder.isSimpleType(typeInformation.getType())) {
-			return entityTypes;
-		}
-
-		if (typeInformation.isMap()) {
-
-			collectEntityTypes(simpleTypeHolder, typeInformation.getComponentType(), entityTypes);
-			collectEntityTypes(simpleTypeHolder, typeInformation.getMapValueType(), entityTypes);
-			return entityTypes;
-		}
-
-		if (typeInformation.isCollectionLike()) {
-
-			collectEntityTypes(simpleTypeHolder, typeInformation.getComponentType(), entityTypes);
-			return entityTypes;
-		}
-
-		if (NullableWrapperConverters.supports(typeInformation.getType())) {
-
-			collectEntityTypes(simpleTypeHolder, typeInformation.getActualType(), entityTypes);
-			return entityTypes;
-		}
-
-		if (ASSOCIATION_TYPE != null && ASSOCIATION_TYPE.isAssignableFrom(typeInformation.getType())) {
-
-			entityTypes.add(getAssociationOrActualType());
-			return entityTypes;
-		}
-
-		entityTypes.add(typeInformation);
-		return entityTypes;
 	}
 
 	protected abstract Association<P> createAssociation();
@@ -211,14 +166,14 @@ public TypeInformation<?> getTypeInformation() {
 	public Iterable<? extends TypeInformation<?>> getPersistentEntityTypes() {
 
 		if (isMap() || isCollectionLike()) {
-			return entityTypes.get();
+			return entityTypeInformation.get();
 		}
 
 		if (!isEntity()) {
 			return Collections.emptySet();
 		}
 
-		return entityTypes.get();
+		return entityTypeInformation.get();
 	}
 
 	/*
@@ -362,7 +317,7 @@ public boolean isArray() {
 	 */
 	@Override
 	public boolean isEntity() {
-		return !isTransient() && entityTypeInformation.getNullable() != null;
+		return !isTransient() && !entityTypeInformation.get().isEmpty();
 	}
 
 	/*
@@ -401,7 +356,11 @@ public Class<?> getMapValueType() {
 	 */
 	@Override
 	public Class<?> getActualType() {
-		return getRequiredAssociationOrActualType().getType();
+
+		TypeInformation<?> targetType = associationTargetType.getNullable();
+		TypeInformation<?> result = targetType == null ? information.getRequiredActualType() : targetType;
+
+		return result.getType();
 	}
 
 	/*
@@ -454,23 +413,40 @@ public String toString() {
 		return property.toString();
 	}
 
-	@Nullable
-	private TypeInformation<?> getAssociationOrActualType() {
-		return getAssociationTypeOr(() -> information.getActualType());
-	}
+	private Set<TypeInformation<?>> detectEntityTypes(SimpleTypeHolder simpleTypes) {
+
+		TypeInformation<?> typeToStartWith = ASSOCIATION_TYPE != null && ASSOCIATION_TYPE.isAssignableFrom(rawType)
+				? information.getComponentType()
+				: information;
+
+		Set<TypeInformation<?>> result = detectEntityTypes(typeToStartWith);
 
-	private TypeInformation<?> getRequiredAssociationOrActualType() {
-		return getAssociationTypeOr(() -> information.getRequiredActualType());
+		return result.stream()
+				.filter(it -> !simpleTypes.isSimpleType(it.getType()))
+				.filter(it -> !it.getType().equals(ASSOCIATION_TYPE))
+				.collect(Collectors.toSet());
 	}
 
-	private TypeInformation<?> getAssociationTypeOr(Supplier<TypeInformation<?>> fallback) {
+	private Set<TypeInformation<?>> detectEntityTypes(@Nullable TypeInformation<?> source) {
 
-		TypeInformation<?> result = associationTargetType.getNullable();
+		if (source == null) {
+			return Collections.emptySet();
+		}
+
+		Set<TypeInformation<?>> result = new HashSet<>();
 
-		if (result != null) {
-			return result;
+		if (source.isMap()) {
+			result.addAll(detectEntityTypes(source.getComponentType()));
+		}
+
+		TypeInformation<?> actualType = source.getActualType();
+
+		if (source.equals(actualType)) {
+			result.add(source);
+		} else {
+			result.addAll(detectEntityTypes(actualType));
 		}
 
-		return fallback.get();
+		return result;
 	}
 }

src/test/java/org/springframework/data/mapping/context/AbstractMappingContextUnitTests.java

@@ -295,6 +295,24 @@ void shouldNotCreatePersistentEntityForListButItsGenericTypeArgument() {
 				.doesNotContain(List.class, ArrayList.class);
 	}
 
+	@Test // GH-2390
+	void detectsEntityTypeEveneIfSimpleTypeHolderConsidersCollectionsSimple() {
+
+		context.setSimpleTypeHolder(new SimpleTypeHolder(Collections.emptySet(), true) {
+
+			@Override
+			public boolean isSimpleType(Class<?> type) {
+				return type.getName().startsWith("java.util.");
+			}
+		});
+
+		context.getPersistentEntity(WithNestedLists.class);
+
+		assertThat(context.getPersistentEntities()) //
+				.map(it -> (Class) it.getType()) //
+				.contains(Base.class);
+	}
+
 	@Test // GH-2390
 	void shouldNotCreatePersistentEntityForMapButItsGenericTypeArguments() {