From 72a0a5623ac963fedaa57ee0914a26a669cf3b72 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Fri, 15 Sep 2017 13:16:46 +0200 Subject: [PATCH] DATAMONGO-1782 - Detect type cycles using PersistentProperty paths. We now rely on PersistentProperty paths to detect cycles between types. Cycles are detected when building up the path object and traversing PersistentProperty stops after the cycle was hit for the second time to generated indexes for at least one hierarchy level. Previously, we used String-based property dot paths and compared whether paths to a particular property was already found by a substring search which caused false positives if a property was reachable via multiple paths. Original Pull Request: #500 --- .../MongoPersistentEntityIndexResolver.java | 207 ++++++++++++------ ...ersistentEntityIndexResolverUnitTests.java | 37 +++- .../mongodb/core/index/PathUnitTests.java | 42 ++-- 3 files changed, 189 insertions(+), 97 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 2df055472..666cdf02f 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 @@ -15,12 +15,17 @@ */ package org.springframework.data.mongodb.core.index; +import lombok.AccessLevel; +import lombok.EqualsAndHashCode; +import lombok.RequiredArgsConstructor; + import java.util.ArrayList; import java.util.Collection; import java.util.Collections; -import java.util.LinkedHashMap; +import java.util.HashSet; +import java.util.Iterator; import java.util.List; -import java.util.Map; +import java.util.Set; import java.util.concurrent.TimeUnit; import org.slf4j.Logger; @@ -29,9 +34,11 @@ import org.springframework.dao.InvalidDataAccessApiUsageException; import org.springframework.data.domain.Sort; import org.springframework.data.mapping.Association; import org.springframework.data.mapping.AssociationHandler; +import org.springframework.data.mapping.PersistentProperty; import org.springframework.data.mapping.PropertyHandler; import org.springframework.data.mapping.model.MappingException; import org.springframework.data.mongodb.core.index.Index.Duplicates; +import org.springframework.data.mongodb.core.index.MongoPersistentEntityIndexResolver.CycleGuard.Path; import org.springframework.data.mongodb.core.index.MongoPersistentEntityIndexResolver.TextIndexIncludeOptions.IncludeStrategy; import org.springframework.data.mongodb.core.index.TextIndexDefinition.TextIndexDefinitionBuilder; import org.springframework.data.mongodb.core.index.TextIndexDefinition.TextIndexedFieldSpec; @@ -41,6 +48,7 @@ import org.springframework.data.mongodb.core.mapping.MongoPersistentEntity; import org.springframework.data.mongodb.core.mapping.MongoPersistentProperty; import org.springframework.data.util.TypeInformation; import org.springframework.util.Assert; +import org.springframework.util.ClassUtils; import org.springframework.util.StringUtils; import com.mongodb.BasicDBObject; @@ -57,6 +65,7 @@ import com.mongodb.util.JSON; * @author Christoph Strobl * @author Thomas Darimont * @author Martin Macko + * @author Mark Paluch * @since 1.5 */ public class MongoPersistentEntityIndexResolver implements IndexResolver { @@ -99,7 +108,7 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver { Document document = root.findAnnotation(Document.class); Assert.notNull(document, "Given entity is not collection root."); - final List indexInformation = new ArrayList(); + final List indexInformation = new ArrayList(); indexInformation.addAll(potentiallyCreateCompoundIndexDefinitions("", root.getCollection(), root)); indexInformation.addAll(potentiallyCreateTextIndexDefinition(root)); @@ -113,7 +122,7 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver { try { if (persistentProperty.isEntity()) { indexInformation.addAll(resolveIndexForClass(persistentProperty.getTypeInformation().getActualType(), - persistentProperty.getFieldName(), root.getCollection(), guard)); + persistentProperty.getFieldName(), Path.of(persistentProperty), root.getCollection(), guard)); } IndexDefinitionHolder indexDefinitionHolder = createIndexDefinitionHolderForProperty( @@ -136,31 +145,35 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver { * Recursively resolve and inspect properties of given {@literal type} for indexes to be created. * * @param type - * @param path The {@literal "dot} path. + * @param dotPath The {@literal "dot} path. + * @param path {@link PersistentProperty} path for cycle detection. * @param collection + * @param guard * @return List of {@link IndexDefinitionHolder} representing indexes for given type and its referenced property * types. Will never be {@code null}. */ - private List resolveIndexForClass(final TypeInformation type, final String path, - final String collection, final CycleGuard guard) { + private List resolveIndexForClass(final TypeInformation type, final String dotPath, + final Path path, final String collection, final CycleGuard guard) { MongoPersistentEntity entity = mappingContext.getPersistentEntity(type); - final List indexInformation = new ArrayList(); - indexInformation.addAll(potentiallyCreateCompoundIndexDefinitions(path, collection, entity)); + final List indexInformation = new ArrayList(); + indexInformation.addAll(potentiallyCreateCompoundIndexDefinitions(dotPath, collection, entity)); entity.doWithProperties(new PropertyHandler() { @Override public void doWithPersistentProperty(MongoPersistentProperty persistentProperty) { - String propertyDotPath = (StringUtils.hasText(path) ? path + "." : "") + persistentProperty.getFieldName(); - guard.protect(persistentProperty, path); + String propertyDotPath = (StringUtils.hasText(dotPath) ? dotPath + "." : "") + + persistentProperty.getFieldName(); + Path propertyPath = path.append(persistentProperty); + guard.protect(persistentProperty, propertyPath); if (persistentProperty.isEntity()) { try { indexInformation.addAll(resolveIndexForClass(persistentProperty.getTypeInformation().getActualType(), - propertyDotPath, collection, guard)); + propertyDotPath, propertyPath, collection, guard)); } catch (CyclicPropertyReferenceException e) { LOGGER.info(e.getMessage()); } @@ -174,7 +187,7 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver { } }); - indexInformation.addAll(resolveIndexesForDbrefs(path, collection, entity)); + indexInformation.addAll(resolveIndexesForDbrefs(dotPath, collection, entity)); return indexInformation; } @@ -212,8 +225,8 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver { } try { - appendTextIndexInformation("", indexDefinitionBuilder, root, new TextIndexIncludeOptions(IncludeStrategy.DEFAULT), - new CycleGuard()); + appendTextIndexInformation("", Path.empty(), indexDefinitionBuilder, root, + new TextIndexIncludeOptions(IncludeStrategy.DEFAULT), new CycleGuard()); } catch (CyclicPropertyReferenceException e) { LOGGER.info(e.getMessage()); } @@ -229,15 +242,16 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver { } - private void appendTextIndexInformation(final String dotPath, final TextIndexDefinitionBuilder indexDefinitionBuilder, - final MongoPersistentEntity entity, final TextIndexIncludeOptions includeOptions, final CycleGuard guard) { + private void appendTextIndexInformation(final String dotPath, final Path path, + final TextIndexDefinitionBuilder indexDefinitionBuilder, final MongoPersistentEntity entity, + final TextIndexIncludeOptions includeOptions, final CycleGuard guard) { entity.doWithProperties(new PropertyHandler() { @Override public void doWithPersistentProperty(MongoPersistentProperty persistentProperty) { - guard.protect(persistentProperty, dotPath); + guard.protect(persistentProperty, path); if (persistentProperty.isExplicitLanguageProperty() && !StringUtils.hasText(dotPath)) { indexDefinitionBuilder.withLanguageOverride(persistentProperty.getFieldName()); @@ -250,6 +264,8 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver { String propertyDotPath = (StringUtils.hasText(dotPath) ? dotPath + "." : "") + persistentProperty.getFieldName(); + Path propertyPath = path.append(persistentProperty); + Float weight = indexed != null ? indexed.weight() : (includeOptions.getParentFieldSpec() != null ? includeOptions.getParentFieldSpec().getWeight() : 1.0F); @@ -262,7 +278,7 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver { } try { - appendTextIndexInformation(propertyDotPath, indexDefinitionBuilder, + appendTextIndexInformation(propertyDotPath, propertyPath, indexDefinitionBuilder, mappingContext.getPersistentEntity(persistentProperty.getActualType()), optionsForNestedType, guard); } catch (CyclicPropertyReferenceException e) { LOGGER.info(e.getMessage()); @@ -291,7 +307,7 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver { protected List createCompoundIndexDefinitions(String dotPath, String fallbackCollection, MongoPersistentEntity entity) { - List indexDefinitions = new ArrayList(); + List indexDefinitions = new ArrayList(); CompoundIndexes indexes = entity.findAnnotation(CompoundIndexes.class); if (indexes != null) { @@ -482,53 +498,38 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver { * to detect potential cycles within the references. * * @author Christoph Strobl + * @author Mark Paluch */ static class CycleGuard { - private final Map> propertyTypeMap; - - CycleGuard() { - this.propertyTypeMap = new LinkedHashMap>(); - } + private final Set seenProperties = new HashSet(); /** + * Detect a cycle in a property path if the property was seen at least once. + * * @param property The property to inspect - * @param path The path under which the property can be reached. + * @param path The type path under which the property can be reached. * @throws CyclicPropertyReferenceException in case a potential cycle is detected. - * @see Path#cycles(MongoPersistentProperty, String) + * @see Path#isCycle() */ - void protect(MongoPersistentProperty property, String path) throws CyclicPropertyReferenceException { + void protect(MongoPersistentProperty property, Path path) throws CyclicPropertyReferenceException { String propertyTypeKey = createMapKey(property); - if (propertyTypeMap.containsKey(propertyTypeKey)) { + if (!seenProperties.add(propertyTypeKey)) { - List paths = propertyTypeMap.get(propertyTypeKey); - - for (Path existingPath : paths) { - - if (existingPath.cycles(property, path) && property.isEntity()) { - paths.add(new Path(property, path)); - - throw new CyclicPropertyReferenceException(property.getFieldName(), property.getOwner().getType(), - existingPath.getPath()); - } + if (path.isCycle()) { + throw new CyclicPropertyReferenceException(property.getFieldName(), property.getOwner().getType(), + path.toCyclePath()); } - - paths.add(new Path(property, path)); - } else { - - ArrayList paths = new ArrayList(); - paths.add(new Path(property, path)); - propertyTypeMap.put(propertyTypeKey, paths); } } private String createMapKey(MongoPersistentProperty property) { - return property.getOwner().getType().getSimpleName() + ":" + property.getFieldName(); + return ClassUtils.getShortName(property.getOwner().getType()) + ":" + property.getFieldName(); } /** - * Path defines the property and its full path from the document root.
+ * Path defines the full property path from the document root.
* A {@link Path} with {@literal spring.data.mongodb} would be created for the property {@code Three.mongodb}. * * 
@@ -549,39 +550,113 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver {
 		 *
* * @author Christoph Strobl + * @author Mark Paluch */ + @RequiredArgsConstructor(access = AccessLevel.PRIVATE) + @EqualsAndHashCode static class Path { - private final MongoPersistentProperty property; - private final String path; + private static final Path EMPTY = new Path(Collections.> emptyList(), false); - Path(MongoPersistentProperty property, String path) { + private final List> elements; + private final boolean cycle; - this.property = property; - this.path = path; + /** + * @return an empty {@link Path}. + * @since 1.10.8 + */ + static Path empty() { + return EMPTY; } - public String getPath() { - return path; + /** + * Creates a new {@link Path} from the initial {@link PersistentProperty}. + * + * @param initial must not be {@literal null}. + * @return the new {@link Path}. + * @since 1.10.8 + */ + static Path of(PersistentProperty initial) { + return new Path(Collections.> singletonList(initial), false); } /** - * Checks whether the given property is owned by the same entity and if it has been already visited by a subset of - * the current path. Given {@literal foo.bar.bar} cycles if {@literal foo.bar} has already been visited and - * {@code class Bar} contains a property of type {@code Bar}. The previously mentioned path would not cycle if - * {@code class Bar} contained a property of type {@code SomeEntity} named {@literal bar}. + * Creates a new {@link Path} by appending a {@link PersistentProperty breadcrumb} to the path. * - * @param property - * @param path - * @return + * @param breadcrumb must not be {@literal null}. + * @return the new {@link Path}. + * @since 1.10.8 + */ + Path append(PersistentProperty breadcrumb) { + + List> elements = new ArrayList>(this.elements.size() + 1); + elements.addAll(this.elements); + elements.add(breadcrumb); + + return new Path(elements, this.elements.contains(breadcrumb)); + } + + /** + * @return {@literal true} if a cycle was detected. + * @since 1.10.8 */ - boolean cycles(MongoPersistentProperty property, String path) { + public boolean isCycle() { + return cycle; + } - if (!property.getOwner().equals(this.property.getOwner())) { - return false; + /* + * (non-Javadoc) + * @see java.lang.Object#toString() + */ + @Override + public String toString() { + return this.elements.isEmpty() ? "(empty)" : toPath(this.elements.iterator()); + } + + /** + * Returns the cycle path truncated to the first discovered cycle. The result for the path + * {@literal foo.bar.baz.bar} is {@literal bar -> baz -> bar}. + * + * @return the cycle path truncated to the first discovered cycle. + * @since 1.10.8 + */ + String toCyclePath() { + + for (int i = 0; i < this.elements.size(); i++) { + + int index = indexOf(this.elements, this.elements.get(i), i + 1); + + if (index != -1) { + return toPath(this.elements.subList(i, index + 1).iterator()); + } + } + + return toString(); + } + + private static int indexOf(List haystack, T needle, int offset) { + + for (int i = offset; i < haystack.size(); i++) { + if (haystack.get(i).equals(needle)) { + return i; + } + } + + return -1; + } + + private static String toPath(Iterator> iterator) { + + StringBuilder builder = new StringBuilder(); + while (iterator.hasNext()) { + + builder.append(iterator.next().getName()); + if (iterator.hasNext()) { + builder.append(" -> "); + } } - return path.equals(this.path) || path.contains(this.path + ".") || path.contains("." + this.path); + return builder.toString(); } } } 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 08bb3391d..d61fe0413 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 @@ -65,7 +65,7 @@ public class MongoPersistentEntityIndexResolverUnitTests { /** * Test resolution of {@link Indexed}. - * + * * @author Christoph Strobl */ public static class IndexResolutionTests { @@ -310,7 +310,7 @@ public class MongoPersistentEntityIndexResolverUnitTests { /** * Test resolution of {@link GeoSpatialIndexed}. - * + * * @author Christoph Strobl */ public static class GeoSpatialIndexResolutionTests { @@ -423,7 +423,7 @@ public class MongoPersistentEntityIndexResolverUnitTests { /** * Test resolution of {@link CompoundIndexes}. - * + * * @author Christoph Strobl */ public static class CompoundIndexResolutionTests { @@ -914,6 +914,17 @@ public class MongoPersistentEntityIndexResolverUnitTests { .resolveIndexForEntity(selfCyclingEntity); } + @Test // DATAMONGO-1782 + public void shouldAllowMultiplePathsToDeeplyType() { + + List indexDefinitions = prepareMappingContextAndResolveIndexForType( + NoCycleManyPathsToDeepValueObject.class); + + assertIndexPathAndCollection("l3.valueObject.value", "rules", indexDefinitions.get(0)); + assertIndexPathAndCollection("l2.l3.valueObject.value", "rules", indexDefinitions.get(1)); + assertThat(indexDefinitions, hasSize(2)); + } + @Test // DATAMONGO-1025 public void shouldUsePathIndexAsIndexNameForDocumentsHavingNamedNestedCompoundIndexFixedOnCollection() { @@ -1071,6 +1082,25 @@ public class MongoPersistentEntityIndexResolverUnitTests { @Indexed String foo; } + @Document(collection = "rules") + static class NoCycleManyPathsToDeepValueObject { + + private NoCycleLevel3 l3; + private NoCycleLevel2 l2; + } + + static class NoCycleLevel2 { + private NoCycleLevel3 l3; + } + + static class NoCycleLevel3 { + private ValueObject valueObject; + } + + static class ValueObject { + @Indexed private String value; + } + @Document static class SimilarityHolingBean { @@ -1187,7 +1217,6 @@ public class MongoPersistentEntityIndexResolverUnitTests { static class EntityWithGenericTypeWrapperAsElement { List> listWithGeneircTypeElement; } - } private static List prepareMappingContextAndResolveIndexForType(Class type) { 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 52308193a..e7241ae91 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 @@ -15,7 +15,7 @@ */ package org.springframework.data.mongodb.core.index; -import static org.hamcrest.CoreMatchers.*; +import static org.hamcrest.Matchers.*; import static org.junit.Assert.*; import static org.mockito.Mockito.*; @@ -31,8 +31,9 @@ import org.springframework.data.mongodb.core.mapping.MongoPersistentProperty; /** * Unit tests for {@link Path}. - * + * * @author Christoph Strobl + * @author Mark Paluch */ @RunWith(MockitoJUnitRunner.class) public class PathUnitTests { @@ -45,39 +46,26 @@ public class PathUnitTests { when(entityMock.getType()).thenReturn((Class) Object.class); } - @Test // DATAMONGO-962 - public void shouldIdentifyCycleForOwnerOfSameTypeAndMatchingPath() { - - MongoPersistentProperty property = createPersistentPropertyMock(entityMock, "foo"); - assertThat(new Path(property, "foo.bar").cycles(property, "foo.bar.bar"), is(true)); - } - - @Test // DATAMONGO-962 - @SuppressWarnings("rawtypes") - public void shouldAllowMatchingPathForDifferentOwners() { - - MongoPersistentProperty existing = createPersistentPropertyMock(entityMock, "foo"); + @Test // DATAMONGO-962, DATAMONGO-1782 + public void shouldIdentifyCycle() { - MongoPersistentEntity entityOfDifferentType = Mockito.mock(MongoPersistentEntity.class); - when(entityOfDifferentType.getType()).thenReturn(String.class); - MongoPersistentProperty toBeVerified = createPersistentPropertyMock(entityOfDifferentType, "foo"); + MongoPersistentProperty foo = createPersistentPropertyMock(entityMock, "foo"); + MongoPersistentProperty bar = createPersistentPropertyMock(entityMock, "bar"); - assertThat(new Path(existing, "foo.bar").cycles(toBeVerified, "foo.bar.bar"), is(false)); - } - - @Test // DATAMONGO-962 - public void shouldAllowEqaulPropertiesOnDifferentPaths() { - - MongoPersistentProperty property = createPersistentPropertyMock(entityMock, "foo"); - assertThat(new Path(property, "foo.bar").cycles(property, "foo2.bar.bar"), is(false)); + assertThat(Path.of(foo).append(bar).isCycle(), is(false)); + assertThat(Path.of(foo).append(bar).append(bar).isCycle(), is(true)); + assertThat(Path.of(foo).append(bar).append(bar).toCyclePath(), is(equalTo("bar -> bar"))); + assertThat(Path.of(foo).append(bar).append(bar).toString(), is(equalTo("foo -> bar -> bar"))); } @SuppressWarnings({ "rawtypes", "unchecked" }) - private MongoPersistentProperty createPersistentPropertyMock(MongoPersistentEntity owner, String fieldname) { + private static MongoPersistentProperty createPersistentPropertyMock(MongoPersistentEntity owner, String fieldname) { MongoPersistentProperty property = Mockito.mock(MongoPersistentProperty.class); + when(property.getOwner()).thenReturn(owner); - when(property.getFieldName()).thenReturn(fieldname); + when(property.getName()).thenReturn(fieldname); + return property; } }