From 7ba319921dce170d637b340aa55ec3584a5533e4 Mon Sep 17 00:00:00 2001 From: Kamil Krzywanski Date: Sat, 11 Jul 2026 16:45:29 +0200 Subject: [PATCH 1/2] Fix false cycle detection for generic types with different type arguments. CycleGuard.Path used PersistentProperty.equals(), which collapses generic owners that share the same reflected field (for example Selection versus Selection). Compare property name and owner TypeInformation instead so nested indexes under distinct type arguments are still resolved. Closes #5213 Signed-off-by: Kamil Krzywanski --- .../MongoPersistentEntityIndexResolver.java | 46 +++++++++++-- ...ersistentEntityIndexResolverUnitTests.java | 59 +++++++++++++++++ .../mongodb/core/index/PathUnitTests.java | 65 ++++++++++++++++++- 3 files changed, 165 insertions(+), 5 deletions(-) diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/index/MongoPersistentEntityIndexResolver.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/index/MongoPersistentEntityIndexResolver.java index 207006bc5f..7ac7f5c11e 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/index/MongoPersistentEntityIndexResolver.java +++ b/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,10 +948,32 @@ String toCyclePath() { return toString(); } - private static int indexOf(List 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} vs {@code Selection}). 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 GH-5213 + */ + private static boolean containsCycleCandidate(List> elements, + PersistentProperty breadcrumb) { + + for (PersistentProperty element : elements) { + if (isSameCycleCandidate(element, breadcrumb)) { + return true; + } + } + + return false; + } + + private static int indexOfCycleCandidate(List> 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; } } @@ -957,6 +981,20 @@ private static int indexOf(List haystack, T needle, int offset) { 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> iterator) { StringBuilder builder = new StringBuilder(); diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/index/MongoPersistentEntityIndexResolverUnitTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/index/MongoPersistentEntityIndexResolverUnitTests.java index 4d61ceca6d..1c8d3fcd50 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/index/MongoPersistentEntityIndexResolverUnitTests.java +++ b/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 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 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 fruitBaskets; + } + + static class FruitBasket { + Selection fruit; + } + + static class IndexedItem { + @Indexed String name; + } + + static class Selection { + T include; + } + + /** + * Genuine cycle through a generic type: {@code GenericTreeDocument -> Box -> GenericTreeDocument}. + * Index resolution must still stop recursion after discovering indexes one level into the cycle. + */ + @Document + static class GenericTreeDocument { + + Box children; + } + + static class Box { + @Indexed String name; + T value; + } + @Document @CompoundIndex(name = "c_index", def = "{ foo:1, bar:1 }") class DocumentWithNamedCompoundIndex { diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/index/PathUnitTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/index/PathUnitTests.java index 7b1ab0be46..fd07e5facc 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/index/PathUnitTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/index/PathUnitTests.java @@ -25,6 +25,8 @@ import org.mockito.Mockito; import org.mockito.junit.MockitoJUnitRunner; +import org.springframework.core.ResolvableType; +import org.springframework.data.core.TypeInformation; import org.springframework.data.mongodb.core.index.MongoPersistentEntityIndexResolver.CycleGuard.Path; import org.springframework.data.mongodb.core.mapping.MongoPersistentEntity; import org.springframework.data.mongodb.core.mapping.MongoPersistentProperty; @@ -34,6 +36,7 @@ * * @author Christoph Strobl * @author Mark Paluch + * @author Kamil Krzywanski */ @RunWith(MockitoJUnitRunner.Silent.class) public class PathUnitTests { @@ -44,6 +47,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 +81,58 @@ 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(); } + @Test // GH-5213 + public void isCycleShouldReturnFalseForSamePropertyNameOnGenericOwnersWithDifferentTypeArguments() { + + TypeInformation selectionOfBasket = parameterized(Selection.class, FruitBasket.class); + TypeInformation selectionOfString = parameterized(Selection.class, String.class); + + MongoPersistentProperty includeBasket = createPersistentPropertyMock(mockOwner(selectionOfBasket), "include"); + MongoPersistentProperty fruit = createPersistentPropertyMock(mockOwner(TypeInformation.of(FruitBasket.class)), + "fruit"); + MongoPersistentProperty includeString = createPersistentPropertyMock(mockOwner(selectionOfString), "include"); + + Path path = Path.of(includeBasket).append(fruit).append(includeString); + + assertThat(path.isCycle()).isFalse(); + assertThat(path.toCyclePath()).isEqualTo(""); + assertThat(path.toString()).isEqualTo("include -> fruit -> include"); + } + + @Test // GH-5213 + public void isCycleShouldReturnTrueForSamePropertyNameOnGenericOwnersWithSameTypeArguments() { + + TypeInformation selectionOfNode = parameterized(Selection.class, TreeNode.class); + + MongoPersistentProperty includeOne = createPersistentPropertyMock(mockOwner(selectionOfNode), "include"); + MongoPersistentProperty child = createPersistentPropertyMock(mockOwner(TypeInformation.of(TreeNode.class)), + "child"); + MongoPersistentProperty includeTwo = createPersistentPropertyMock(mockOwner(selectionOfNode), "include"); + + Path path = Path.of(includeOne).append(child).append(includeTwo); + + assertThat(path.isCycle()).isTrue(); + assertThat(path.toCyclePath()).isEqualTo("include -> child -> include"); + } + + private static TypeInformation parameterized(Class rawType, Class typeArgument) { + return TypeInformation.of(ResolvableType.forClassWithGenerics(rawType, typeArgument)); + } + + @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 +143,16 @@ private static MongoPersistentProperty createPersistentPropertyMock(MongoPersist return property; } + + static class Selection { + T include; + } + + static class FruitBasket { + Selection fruit; + } + + static class TreeNode { + Selection child; + } } From f5fb41b5af03a8132e901c266bb123a6f26e227d Mon Sep 17 00:00:00 2001 From: Kamil Krzywanski Date: Sat, 11 Jul 2026 16:55:34 +0200 Subject: [PATCH 2/2] Use real mapping properties in Path GH-5213 tests. Mock PersistentProperty instances do not share field-based equality, so the previous path unit tests were false-green without the fix. Build the cycle path from MongoMappingContext properties so equals collapses Selection.include across type arguments the same way production does. Signed-off-by: Kamil Krzywanski --- .../mongodb/core/index/PathUnitTests.java | 63 ++++++++++++++----- 1 file changed, 46 insertions(+), 17 deletions(-) diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/index/PathUnitTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/index/PathUnitTests.java index fd07e5facc..02aa78e425 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/index/PathUnitTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/index/PathUnitTests.java @@ -18,6 +18,8 @@ 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; @@ -25,9 +27,9 @@ import org.mockito.Mockito; import org.mockito.junit.MockitoJUnitRunner; -import org.springframework.core.ResolvableType; 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; @@ -87,16 +89,28 @@ public void isCycleShouldReturnFalseCycleForNonEqualProperties() { 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() { - TypeInformation selectionOfBasket = parameterized(Selection.class, FruitBasket.class); - TypeInformation selectionOfString = parameterized(Selection.class, String.class); + 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"); - MongoPersistentProperty includeBasket = createPersistentPropertyMock(mockOwner(selectionOfBasket), "include"); - MongoPersistentProperty fruit = createPersistentPropertyMock(mockOwner(TypeInformation.of(FruitBasket.class)), - "fruit"); - MongoPersistentProperty includeString = createPersistentPropertyMock(mockOwner(selectionOfString), "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); @@ -105,24 +119,35 @@ public void isCycleShouldReturnFalseForSamePropertyNameOnGenericOwnersWithDiffer 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() { - TypeInformation selectionOfNode = parameterized(Selection.class, TreeNode.class); + MongoMappingContext context = createMappingContext(TreeNode.class); + + MongoPersistentProperty child = context.getRequiredPersistentEntity(TreeNode.class) + .getRequiredPersistentProperty("child"); + MongoPersistentProperty include = context.getRequiredPersistentEntity(child.getTypeInformation()) + .getRequiredPersistentProperty("include"); - MongoPersistentProperty includeOne = createPersistentPropertyMock(mockOwner(selectionOfNode), "include"); - MongoPersistentProperty child = createPersistentPropertyMock(mockOwner(TypeInformation.of(TreeNode.class)), - "child"); - MongoPersistentProperty includeTwo = createPersistentPropertyMock(mockOwner(selectionOfNode), "include"); + // Re-entering Selection yields the same property (equal and same TypeInformation). + assertThat(include.getOwner().getTypeInformation()).isEqualTo(child.getTypeInformation()); - Path path = Path.of(includeOne).append(child).append(includeTwo); + Path path = Path.of(include).append(child).append(include); assertThat(path.isCycle()).isTrue(); assertThat(path.toCyclePath()).isEqualTo("include -> child -> include"); } - private static TypeInformation parameterized(Class rawType, Class typeArgument) { - return TypeInformation.of(ResolvableType.forClassWithGenerics(rawType, typeArgument)); + private static MongoMappingContext createMappingContext(Class... types) { + + MongoMappingContext context = new MongoMappingContext(); + context.setInitialEntitySet(Set.of(types)); + context.afterPropertiesSet(); + return context; } @SuppressWarnings({ "rawtypes", "unchecked" }) @@ -144,14 +169,18 @@ private static MongoPersistentProperty createPersistentPropertyMock(MongoPersist return property; } - static class Selection { - T include; + static class FruitShop { + Selection fruitBaskets; } static class FruitBasket { Selection fruit; } + static class Selection { + T include; + } + static class TreeNode { Selection child; }