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..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,7 +27,9 @@ 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 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 fruitBaskets; + } + + static class FruitBasket { + Selection fruit; + } + + static class Selection { + T include; + } + + static class TreeNode { + Selection child; + } }