Fix false cycle detection for generic types with different type arguments.

spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/index/MongoPersistentEntityIndexResolver.java

@@ -79,6 +79,7 @@
  * @author Dave Perryman
  * @author Stefan Tirea
  * @author Sangbeen Moon
+ * @author Kamil Krzywanski
  * @since 1.5
  */
 public class MongoPersistentEntityIndexResolver implements IndexResolver {
@@ -860,6 +861,7 @@ private String createMapKey(MongoPersistentProperty property) {
 		 *
 		 * @author Christoph Strobl
 		 * @author Mark Paluch
+		 * @author Kamil Krzywanski
 		 */
 		static class Path {
 
@@ -905,7 +907,7 @@ Path append(PersistentProperty<?> breadcrumb) {
 				elements.addAll(this.elements);
 				elements.add(breadcrumb);
 
-				return new Path(elements, this.elements.contains(breadcrumb));
+				return new Path(elements, containsCycleCandidate(this.elements, breadcrumb));
 			}
 
 			/**
@@ -936,7 +938,7 @@ String toCyclePath() {
 
 				for (int i = 0; i < this.elements.size(); i++) {
 
-					int index = indexOf(this.elements, this.elements.get(i), i + 1);
+					int index = indexOfCycleCandidate(this.elements, this.elements.get(i), i + 1);
 
 					if (index != -1) {
 						return toPath(this.elements.subList(i, index + 1).iterator());
@@ -946,17 +948,53 @@ String toCyclePath() {
 				return toString();
 			}
 
-			private static <T> int indexOf(List<T> haystack, T needle, int offset) {
+			/**
+			 * Cycle detection cannot rely on {@link PersistentProperty#equals(Object)} alone: that equality is based on the
+			 * reflected field and therefore collapses generic types that share the same raw owner type (for example
+			 * {@code Selection<FruitBasket>} vs {@code Selection<String>}). Comparing the property name together with the
+			 * owner's resolved {@link TypeInformation} keeps genuine recursion (same parameterized type reappearing) while
+			 * allowing the same generic type to appear with different type arguments.
+			 *
+			 * @see <a href="https://github.com/spring-projects/spring-data-mongodb/issues/5213">GH-5213</a>
+			 */
+			private static boolean containsCycleCandidate(List<PersistentProperty<?>> elements,
+					PersistentProperty<?> breadcrumb) {
+
+				for (PersistentProperty<?> element : elements) {
+					if (isSameCycleCandidate(element, breadcrumb)) {
+						return true;
+					}
+				}
+
+				return false;
+			}
+
+			private static int indexOfCycleCandidate(List<PersistentProperty<?>> haystack, PersistentProperty<?> needle,
+					int offset) {
 
 				for (int i = offset; i < haystack.size(); i++) {
-					if (haystack.get(i).equals(needle)) {
+					if (isSameCycleCandidate(haystack.get(i), needle)) {
 						return i;
 					}
 				}
 
 				return -1;
 			}
 
+			private static boolean isSameCycleCandidate(PersistentProperty<?> left, PersistentProperty<?> right) {
+
+				if (left == right) {
+					return true;
+				}
+
+				if (!ObjectUtils.nullSafeEquals(left.getName(), right.getName())) {
+					return false;
+				}
+
+				return ObjectUtils.nullSafeEquals(left.getOwner().getTypeInformation(),
+						right.getOwner().getTypeInformation());
+			}
+
 			private static String toPath(Iterator<PersistentProperty<?>> iterator) {
 
 				StringBuilder builder = new StringBuilder();

spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/index/MongoPersistentEntityIndexResolverUnitTests.java

@@ -66,6 +66,7 @@
  * @author Mark Paluch
  * @author Dave Perryman
  * @author Stefan Tirea
+ * @author Kamil Krzywanski
  */
 @RunWith(Suite.class)
 @SuiteClasses({ IndexResolutionTests.class, GeoSpatialIndexResolutionTests.class, CompoundIndexResolutionTests.class,
@@ -1235,6 +1236,27 @@ public void shouldNotDetectCycleWhenTypeIsUsedMoreThanOnce() {
 			assertThat(indexDefinitions).isEmpty();
 		}
 
+		@Test // GH-5213
+		public void shouldNotDetectCycleForGenericTypeUsedWithDifferentTypeArguments() {
+
+			List<IndexDefinitionHolder> indexDefinitions = prepareMappingContextAndResolveIndexForType(FruitShop.class);
+
+			assertThat(indexDefinitions).hasSize(1);
+			assertIndexPathAndCollection("fruitBaskets.include.fruit.include.name", "fruitShop", indexDefinitions.get(0));
+		}
+
+		@Test // GH-5213
+		public void shouldStillDetectGenuineCycleThroughGenericType() {
+
+			List<IndexDefinitionHolder> indexDefinitions = prepareMappingContextAndResolveIndexForType(
+					GenericTreeDocument.class);
+
+			// Indexes on the first occurrence of the generic type are kept; further recursion into the same
+			// parameterized type is treated as a cycle and stops.
+			assertThat(indexDefinitions).hasSize(1);
+			assertIndexPathAndCollection("children.name", "genericTreeDocument", indexDefinitions.get(0));
+		}
+
 		@Test // DATAMONGO-962
 		@SuppressWarnings({ "rawtypes", "unchecked" })
 		public void shouldCatchCyclicReferenceExceptionOnRoot() {
@@ -1671,6 +1693,43 @@ class SelfCyclingViaCollectionType {
 
 		}
 
+		/**
+		 * Reproducer for GH-5213: the same generic type appears twice with different type arguments.
+		 * That is not a structural cycle and nested {@link Indexed} fields must still be discovered.
+		 */
+		@Document
+		static class FruitShop {
+
+			Selection<FruitBasket> fruitBaskets;
+		}
+
+		static class FruitBasket {
+			Selection<IndexedItem> fruit;
+		}
+
+		static class IndexedItem {
+			@Indexed String name;
+		}
+
+		static class Selection<T> {
+			T include;
+		}
+
+		/**
+		 * Genuine cycle through a generic type: {@code GenericTreeDocument -> Box<GenericTreeDocument> -> GenericTreeDocument}.
+		 * Index resolution must still stop recursion after discovering indexes one level into the cycle.
+		 */
+		@Document
+		static class GenericTreeDocument {
+
+			Box<GenericTreeDocument> children;
+		}
+
+		static class Box<T> {
+			@Indexed String name;
+			T value;
+		}
+
 		@Document
 		@CompoundIndex(name = "c_index", def = "{ foo:1, bar:1 }")
 		class DocumentWithNamedCompoundIndex {

spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/index/PathUnitTests.java

@@ -18,14 +18,18 @@
 import static org.assertj.core.api.Assertions.*;
 import static org.mockito.Mockito.*;
 
+import java.util.Set;
+
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mockito.Mock;
 import org.mockito.Mockito;
 import org.mockito.junit.MockitoJUnitRunner;
 
+import org.springframework.data.core.TypeInformation;
 import org.springframework.data.mongodb.core.index.MongoPersistentEntityIndexResolver.CycleGuard.Path;
+import org.springframework.data.mongodb.core.mapping.MongoMappingContext;
 import org.springframework.data.mongodb.core.mapping.MongoPersistentEntity;
 import org.springframework.data.mongodb.core.mapping.MongoPersistentProperty;
 
@@ -34,6 +38,7 @@
  *
  * @author Christoph Strobl
  * @author Mark Paluch
+ * @author Kamil Krzywanski
  */
 @RunWith(MockitoJUnitRunner.Silent.class)
 public class PathUnitTests {
@@ -44,6 +49,7 @@ public class PathUnitTests {
 	@SuppressWarnings({ "rawtypes", "unchecked" })
 	public void setUp() {
 		when(entityMock.getType()).thenReturn((Class) Object.class);
+		doReturn(TypeInformation.of(Object.class)).when(entityMock).getTypeInformation();
 	}
 
 	@Test // DATAMONGO-962, DATAMONGO-1782
@@ -77,11 +83,81 @@ public void isCycleShouldReturnFalseCycleForNonEqualProperties() {
 
 		MongoPersistentProperty foo = createPersistentPropertyMock(entityMock, "foo");
 		MongoPersistentProperty bar = createPersistentPropertyMock(entityMock, "bar");
-		MongoPersistentProperty bar2 = createPersistentPropertyMock(mock(MongoPersistentEntity.class), "bar");
+
+		MongoPersistentProperty bar2 = createPersistentPropertyMock(mockOwner(TypeInformation.of(String.class)), "bar");
 
 		assertThat(Path.of(foo).append(bar).append(bar2).isCycle()).isFalse();
 	}
 
+	/**
+	 * Uses real mapping properties so equality matches production: {@code Selection.include} is backed by the same
+	 * reflected field for every parameterization of {@code Selection}, which is what triggered the GH-5213 false cycle.
+	 */
+	@Test // GH-5213
+	public void isCycleShouldReturnFalseForSamePropertyNameOnGenericOwnersWithDifferentTypeArguments() {
+
+		MongoMappingContext context = createMappingContext(FruitShop.class);
+
+		MongoPersistentProperty fruitBaskets = context.getRequiredPersistentEntity(FruitShop.class)
+				.getRequiredPersistentProperty("fruitBaskets");
+		MongoPersistentProperty includeBasket = context.getRequiredPersistentEntity(fruitBaskets.getTypeInformation())
+				.getRequiredPersistentProperty("include");
+		MongoPersistentProperty fruit = context.getRequiredPersistentEntity(includeBasket.getTypeInformation())
+				.getRequiredPersistentProperty("fruit");
+		MongoPersistentProperty includeString = context.getRequiredPersistentEntity(fruit.getTypeInformation())
+				.getRequiredPersistentProperty("include");
+
+		// Precondition of the bug: property equality collapses generic type arguments.
+		assertThat(includeBasket).isEqualTo(includeString);
+		assertThat(includeBasket.getOwner().getTypeInformation())
+				.isNotEqualTo(includeString.getOwner().getTypeInformation());
+
+		Path path = Path.of(includeBasket).append(fruit).append(includeString);
+
+		assertThat(path.isCycle()).isFalse();
+		assertThat(path.toCyclePath()).isEqualTo("");
+		assertThat(path.toString()).isEqualTo("include -> fruit -> include");
+	}
+
+	/**
+	 * Genuine recursion through the same parameterized type must still be reported as a cycle. Real mapping properties
+	 * are used so the path is built the same way index resolution walks the entity graph.
+	 */
+	@Test // GH-5213
+	public void isCycleShouldReturnTrueForSamePropertyNameOnGenericOwnersWithSameTypeArguments() {
+
+		MongoMappingContext context = createMappingContext(TreeNode.class);
+
+		MongoPersistentProperty child = context.getRequiredPersistentEntity(TreeNode.class)
+				.getRequiredPersistentProperty("child");
+		MongoPersistentProperty include = context.getRequiredPersistentEntity(child.getTypeInformation())
+				.getRequiredPersistentProperty("include");
+
+		// Re-entering Selection<TreeNode> yields the same property (equal and same TypeInformation).
+		assertThat(include.getOwner().getTypeInformation()).isEqualTo(child.getTypeInformation());
+
+		Path path = Path.of(include).append(child).append(include);
+
+		assertThat(path.isCycle()).isTrue();
+		assertThat(path.toCyclePath()).isEqualTo("include -> child -> include");
+	}
+
+	private static MongoMappingContext createMappingContext(Class<?>... types) {
+
+		MongoMappingContext context = new MongoMappingContext();
+		context.setInitialEntitySet(Set.of(types));
+		context.afterPropertiesSet();
+		return context;
+	}
+
+	@SuppressWarnings({ "rawtypes", "unchecked" })
+	private static MongoPersistentEntity<?> mockOwner(TypeInformation<?> typeInformation) {
+
+		MongoPersistentEntity owner = mock(MongoPersistentEntity.class);
+		doReturn(typeInformation).when(owner).getTypeInformation();
+		return owner;
+	}
+
 	@SuppressWarnings({ "rawtypes", "unchecked" })
 	private static MongoPersistentProperty createPersistentPropertyMock(MongoPersistentEntity owner, String fieldname) {
 
@@ -92,4 +168,20 @@ private static MongoPersistentProperty createPersistentPropertyMock(MongoPersist
 
 		return property;
 	}
+
+	static class FruitShop {
+		Selection<FruitBasket> fruitBaskets;
+	}
+
+	static class FruitBasket {
+		Selection<String> fruit;
+	}
+
+	static class Selection<T> {
+		T include;
+	}
+
+	static class TreeNode {
+		Selection<TreeNode> child;
+	}
 }