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

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<IndexDefinitionHolder> indexInformation = new ArrayList<MongoPersistentEntityIndexResolver.IndexDefinitionHolder>();
+		final List<IndexDefinitionHolder> indexInformation = new ArrayList<IndexDefinitionHolder>();
 		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<IndexDefinitionHolder> resolveIndexForClass(final TypeInformation<?> type, final String path,
-			final String collection, final CycleGuard guard) {
+	private List<IndexDefinitionHolder> resolveIndexForClass(final TypeInformation<?> type, final String dotPath,
+			final Path path, final String collection, final CycleGuard guard) {
 
 		MongoPersistentEntity<?> entity = mappingContext.getPersistentEntity(type);
 
-		final List<IndexDefinitionHolder> indexInformation = new ArrayList<MongoPersistentEntityIndexResolver.IndexDefinitionHolder>();
-		indexInformation.addAll(potentiallyCreateCompoundIndexDefinitions(path, collection, entity));
+		final List<IndexDefinitionHolder> indexInformation = new ArrayList<IndexDefinitionHolder>();
+		indexInformation.addAll(potentiallyCreateCompoundIndexDefinitions(dotPath, collection, entity));
 
 		entity.doWithProperties(new PropertyHandler<MongoPersistentProperty>() {
 
 			@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<MongoPersistentProperty>() {
 
 			@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<IndexDefinitionHolder> createCompoundIndexDefinitions(String dotPath, String fallbackCollection,
 			MongoPersistentEntity<?> entity) {
 
-		List<IndexDefinitionHolder> indexDefinitions = new ArrayList<MongoPersistentEntityIndexResolver.IndexDefinitionHolder>();
+		List<IndexDefinitionHolder> indexDefinitions = new ArrayList<IndexDefinitionHolder>();
 		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<String, List<Path>> propertyTypeMap;
-
-		CycleGuard() {
-			this.propertyTypeMap = new LinkedHashMap<String, List<Path>>();
-		}
+		private final Set<String> seenProperties = new HashSet<String>();
 
 		/**
+		 * 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<Path> 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<Path> paths = new ArrayList<Path>();
-				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. <br />
+		 * Path defines the full property path from the document root. <br />
 		 * A {@link Path} with {@literal spring.data.mongodb} would be created for the property {@code Three.mongodb}.
 		 *
 		 * <pre>
@@ -549,39 +550,113 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver {
 		 * </pre>
 		 *
 		 * @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.<PersistentProperty<?>> emptyList(), false);
 
-			Path(MongoPersistentProperty property, String path) {
+			private final List<PersistentProperty<?>> 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.<PersistentProperty<?>> 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<PersistentProperty<?>> elements = new ArrayList<PersistentProperty<?>>(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 <T> int indexOf(List<T> 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<PersistentProperty<?>> 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();
 			}
 		}
 	}

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<IndexDefinitionHolder> 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<GenericEntityWrapper<DocumentWithNamedIndex>> listWithGeneircTypeElement;
 		}
-
 	}
 
 	private static List<IndexDefinitionHolder> prepareMappingContextAndResolveIndexForType(Class<?> type) {

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;
 	}
 }