DATAMONGO-962 - Cycle guard should respect full path.

We now check on intersections of given path and existing to not only check types and contained property names but also properties full path which must not be present in already traversed paths.

Additionally we’ll now catch any CyclicPropertyReferenceExceptions on the root level to prevent cycle detection interfering with application startup.

Original pull request: #197.

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

@@ -21,8 +21,6 @@ import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.TimeUnit;
-import java.util.regex.Matcher;
-import java.util.regex.Pattern;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -103,15 +101,19 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver {
 			@Override
 			public void doWithPersistentProperty(MongoPersistentProperty persistentProperty) {
 
-				if (persistentProperty.isEntity()) {
+				try {
-					indexInformation.addAll(resolveIndexForClass(persistentProperty.getActualType(),
+					if (persistentProperty.isEntity()) {
-							persistentProperty.getFieldName(), root.getCollection(), guard));
+						indexInformation.addAll(resolveIndexForClass(persistentProperty.getActualType(),
-				}
+								persistentProperty.getFieldName(), root.getCollection(), guard));
+					}
 
-				IndexDefinitionHolder indexDefinitionHolder = createIndexDefinitionHolderForProperty(
+					IndexDefinitionHolder indexDefinitionHolder = createIndexDefinitionHolderForProperty(
-						persistentProperty.getFieldName(), root.getCollection(), persistentProperty);
+							persistentProperty.getFieldName(), root.getCollection(), persistentProperty);
-				if (indexDefinitionHolder != null) {
+					if (indexDefinitionHolder != null) {
-					indexInformation.add(indexDefinitionHolder);
+						indexInformation.add(indexDefinitionHolder);
+					}
+				} catch (CyclicPropertyReferenceException e) {
+					LOGGER.warn(e.getMessage());
 				}
 			}
 		});
@@ -350,7 +352,7 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver {
 	 * 
 	 * @author Christoph Strobl
 	 */
-	private static class CycleGuard {
+	static class CycleGuard {
 
 		private final Map<String, List<Path>> propertyTypeMap;
 
@@ -362,6 +364,7 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver {
 		 * @param property The property to inspect
 		 * @param path The path under which the property can be reached.
 		 * @throws CyclicPropertyReferenceException in case a potential cycle is detected.
+		 * @see Path#cycles(MongoPersistentProperty, String)
 		 */
 		void protect(MongoPersistentProperty property, String path) throws CyclicPropertyReferenceException {
 
@@ -372,7 +375,7 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver {
 
 				for (Path existingPath : paths) {
 
-					if (existingPath.cycles(property)) {
+					if (existingPath.cycles(property, path)) {
 						paths.add(new Path(property, path));
 						throw new CyclicPropertyReferenceException(property.getFieldName(), property.getOwner().getType(),
 								existingPath.getPath());
@@ -380,7 +383,6 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver {
 				}
 
 				paths.add(new Path(property, path));
-
 			} else {
 
 				ArrayList<Path> paths = new ArrayList<Path>();
@@ -393,7 +395,30 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver {
 			return property.getOwner().getType().getSimpleName() + ":" + property.getFieldName();
 		}
 
-		private static class Path {
+		/**
+		 * Path defines the property and its full path from the document root. <br />
+		 * A {@link Path} with {@literal spring.data.mongodb} would be created for the property {@code Three.mongodb}.
+		 * 
+		 * <pre>
+		 * <code>
+		 * &#64;Document
+		 * class One {
+		 *   Two spring;
+		 * }
+		 * 
+		 * class Two {
+		 *   Three data;
+		 * }
+		 * 
+		 * class Three {
+		 *   String mongodb;
+		 * }
+		 * </code>
+		 * </pre>
+		 * 
+		 * @author Christoph Strobl
+		 */
+		static class Path {
 
 			private final MongoPersistentProperty property;
 			private final String path;
@@ -408,17 +433,23 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver {
 				return path;
 			}
 
-			boolean cycles(MongoPersistentProperty property) {
+			/**
-
+			 * Checks whether the given property is owned by the same entity and if it has been already visited by a subset of
-				Pattern pattern = Pattern.compile("\\b" + Pattern.quote(property.getFieldName()) + "\\b");
+			 * the current path. Given {@literal foo.bar.bar} cycles if {@literal foo.bar} has already been visited and
-				Matcher matcher = pattern.matcher(path);
+			 * {@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}.
-				int count = 0;
+			 * 
-				while (matcher.find()) {
+			 * @param property
-					count++;
+			 * @param path
+			 * @return
+			 */
+			boolean cycles(MongoPersistentProperty property, String path) {
+
+				if (!property.getOwner().equals(this.property.getOwner())) {
+					return false;
 				}
 
-				return count >= 1 && property.getOwner().getType().equals(this.property.getOwner().getType());
+				return path.contains(this.path);
 			}
 		}
 	}
@@ -448,8 +479,8 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver {
 		 */
 		@Override
 		public String getMessage() {
-			return String.format("Found cycle for field '%s' in type '%s' for path '%s'", propertyName, type.getSimpleName(),
+			return String.format("Found cycle for field '%s' in type '%s' for path '%s'", propertyName,
-					dotPath);
+					type != null ? type.getSimpleName() : "unknown", dotPath);
 		}
 	}
 

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

@@ -20,10 +20,13 @@ import static org.hamcrest.collection.IsEmptyCollection.*;
 import static org.hamcrest.core.IsEqual.*;
 import static org.hamcrest.core.IsInstanceOf.*;
 import static org.junit.Assert.*;
+import static org.mockito.Mockito.*;
 
+import java.lang.annotation.Annotation;
 import java.util.Collections;
 import java.util.List;
 
+import org.hamcrest.collection.IsEmptyCollection;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.Suite;
@@ -38,6 +41,9 @@ import org.springframework.data.mongodb.core.mapping.DBRef;
 import org.springframework.data.mongodb.core.mapping.Document;
 import org.springframework.data.mongodb.core.mapping.Field;
 import org.springframework.data.mongodb.core.mapping.MongoMappingContext;
+import org.springframework.data.mongodb.core.mapping.MongoPersistentEntity;
+import org.springframework.data.mongodb.core.mapping.MongoPersistentEntityTestDummy.MongoPersistentEntityDummyBuilder;
+import org.springframework.data.mongodb.core.mapping.MongoPersistentProperty;
 
 import com.mongodb.BasicDBObjectBuilder;
 
@@ -467,7 +473,7 @@ public class MongoPersistentEntityIndexResolverUnitTests {
 		public void shouldNotRunIntoStackOverflow() {
 
 			List<IndexDefinitionHolder> indexDefinitions = prepareMappingContextAndResolveIndexForType(CycleStartingInBetween.class);
-			assertThat(indexDefinitions, hasSize(2));
+			assertThat(indexDefinitions, hasSize(1));
 		}
 
 		/**
@@ -490,9 +496,7 @@ public class MongoPersistentEntityIndexResolverUnitTests {
 
 			List<IndexDefinitionHolder> indexDefinitions = prepareMappingContextAndResolveIndexForType(CycleOnLevelOne.class);
 			assertIndexPathAndCollection("reference.indexedProperty", "cycleOnLevelOne", indexDefinitions.get(0));
-			assertIndexPathAndCollection("reference.cyclicReference.reference.indexedProperty", "cycleOnLevelOne",
+			assertThat(indexDefinitions, hasSize(1));
-					indexDefinitions.get(1));
-			assertThat(indexDefinitions, hasSize(2));
 		}
 
 		/**
@@ -520,6 +524,58 @@ public class MongoPersistentEntityIndexResolverUnitTests {
 			assertThat(indexDefinitions, hasSize(1));
 		}
 
+		/**
+		 * @see DATAMONGO-962
+		 */
+		@Test
+		public void shouldDetectSelfCycleViaCollectionTypeCorrectly() {
+
+			List<IndexDefinitionHolder> indexDefinitions = prepareMappingContextAndResolveIndexForType(SelfCyclingViaCollectionType.class);
+			assertThat(indexDefinitions, IsEmptyCollection.empty());
+		}
+
+		/**
+		 * @see DATAMONGO-962
+		 */
+		@Test
+		public void shouldNotDetectCycleWhenTypeIsUsedMoreThanOnce() {
+
+			List<IndexDefinitionHolder> indexDefinitions = prepareMappingContextAndResolveIndexForType(MultipleObjectsOfSameType.class);
+			assertThat(indexDefinitions, IsEmptyCollection.empty());
+		}
+
+		/**
+		 * @see DATAMONGO-962
+		 */
+		@Test
+		public void shouldCatchCyclicReferenceExceptionOnRoot() {
+
+			Document documentDummy = new Document() {
+
+				@Override
+				public Class<? extends Annotation> annotationType() {
+					return Document.class;
+				}
+
+				@Override
+				public String collection() {
+					return null;
+				}
+			};
+
+			MongoPersistentProperty propertyMock = mock(MongoPersistentProperty.class);
+			when(propertyMock.isEntity()).thenReturn(true);
+			when(propertyMock.getActualType()).thenThrow(
+					new MongoPersistentEntityIndexResolver.CyclicPropertyReferenceException("foo", Object.class, "bar"));
+
+			MongoPersistentEntity<SelfCyclingViaCollectionType> dummy = MongoPersistentEntityDummyBuilder
+					.forClass(SelfCyclingViaCollectionType.class).withCollection("foo").and(propertyMock)
+					.and(documentDummy).build();
+
+			new MongoPersistentEntityIndexResolver(prepareMappingContext(SelfCyclingViaCollectionType.class))
+					.resolveIndexForEntity(dummy);
+		}
+
 		@Document
 		static class MixedIndexRoot {
 
@@ -597,6 +653,21 @@ public class MongoPersistentEntityIndexResolverUnitTests {
 		static class SimilaritySibling {
 			@Field("similarity") private String similarThoughNotEqualNamedProperty;
 		}
+
+		@Document
+		static class MultipleObjectsOfSameType {
+
+			SelfCyclingViaCollectionType cycleOne;
+
+			SelfCyclingViaCollectionType cycleTwo;
+		}
+
+		@Document
+		static class SelfCyclingViaCollectionType {
+
+			List<SelfCyclingViaCollectionType> cyclic;
+
+		}
 	}
 
 	private static List<IndexDefinitionHolder> prepareMappingContextAndResolveIndexForType(Class<?> type) {
@@ -629,4 +700,5 @@ public class MongoPersistentEntityIndexResolverUnitTests {
 		}
 		assertThat(holder.getCollection(), equalTo(expectedCollection));
 	}
+
 }

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

@@ -0,0 +1,92 @@
+/*
+ * Copyright 2014 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.springframework.data.mongodb.core.index;
+
+import static org.hamcrest.CoreMatchers.*;
+import static org.junit.Assert.*;
+import static org.mockito.Mockito.*;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.runners.MockitoJUnitRunner;
+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;
+
+/**
+ * Unit tests for {@link Path}.
+ * 
+ * @author Christoph Strobl
+ */
+@RunWith(MockitoJUnitRunner.class)
+public class PathUnitTests {
+
+	@Mock MongoPersistentEntity<?> entityMock;
+
+	@Before
+	@SuppressWarnings({ "rawtypes", "unchecked" })
+	public void setUp() {
+		when(entityMock.getType()).thenReturn((Class) Object.class);
+	}
+
+	/**
+	 * @see DATAMONGO-962
+	 */
+	@Test
+	public void shouldIdentifyCycleForOwnerOfSameTypeAndMatchingPath() {
+
+		MongoPersistentProperty property = createPersistentPropertyMock(entityMock, "foo");
+		assertThat(new Path(property, "foo.bar").cycles(property, "foo.bar.bar"), is(true));
+	}
+
+	/**
+	 * @see DATAMONGO-962
+	 */
+	@Test
+	@SuppressWarnings("rawtypes")
+	public void shouldAllowMatchingPathForDifferentOwners() {
+
+		MongoPersistentProperty existing = createPersistentPropertyMock(entityMock, "foo");
+
+		MongoPersistentEntity entityOfDifferentType = Mockito.mock(MongoPersistentEntity.class);
+		when(entityOfDifferentType.getType()).thenReturn(String.class);
+		MongoPersistentProperty toBeVerified = createPersistentPropertyMock(entityOfDifferentType, "foo");
+
+		assertThat(new Path(existing, "foo.bar").cycles(toBeVerified, "foo.bar.bar"), is(false));
+	}
+
+	/**
+	 * @see DATAMONGO-962
+	 */
+	@Test
+	public void shouldAllowEqaulPropertiesOnDifferentPaths() {
+
+		MongoPersistentProperty property = createPersistentPropertyMock(entityMock, "foo");
+		assertThat(new Path(property, "foo.bar").cycles(property, "foo2.bar.bar"), is(false));
+	}
+
+	@SuppressWarnings({ "rawtypes", "unchecked" })
+	private MongoPersistentProperty createPersistentPropertyMock(MongoPersistentEntity owner, String fieldname) {
+
+		MongoPersistentProperty property = Mockito.mock(MongoPersistentProperty.class);
+		when(property.getOwner()).thenReturn(owner);
+		when(property.getFieldName()).thenReturn(fieldname);
+		return property;
+	}
+}

spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/mapping/MongoPersistentEntityTestDummy.java

@@ -0,0 +1,212 @@
+/*
+ * Copyright 2014 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.springframework.data.mongodb.core.mapping;
+
+import java.lang.annotation.Annotation;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Map;
+
+import org.springframework.data.annotation.Id;
+import org.springframework.data.annotation.Version;
+import org.springframework.data.mapping.AssociationHandler;
+import org.springframework.data.mapping.PersistentProperty;
+import org.springframework.data.mapping.PreferredConstructor;
+import org.springframework.data.mapping.PropertyHandler;
+import org.springframework.data.mapping.SimpleAssociationHandler;
+import org.springframework.data.mapping.SimplePropertyHandler;
+import org.springframework.data.util.TypeInformation;
+
+/**
+ * Trivial dummy implementation of {@link MongoPersistentEntity} to be used in tests.
+ * 
+ * @author Christoph Strobl
+ * @param <T>
+ */
+public class MongoPersistentEntityTestDummy<T> implements MongoPersistentEntity<T> {
+
+	private Map<Class<?>, Annotation> annotations = new HashMap<Class<?>, Annotation>();
+	private Collection<MongoPersistentProperty> properties = new ArrayList<MongoPersistentProperty>();
+	private String collection;
+	private String name;
+	private Class<T> type;
+
+	@Override
+	public String getName() {
+		return name;
+	}
+
+	@Override
+	public PreferredConstructor<T, MongoPersistentProperty> getPersistenceConstructor() {
+		return null;
+	}
+
+	@Override
+	public boolean isConstructorArgument(PersistentProperty<?> property) {
+		return false;
+	}
+
+	@Override
+	public boolean isIdProperty(PersistentProperty<?> property) {
+		return property != null ? property.isIdProperty() : false;
+	}
+
+	@Override
+	public boolean isVersionProperty(PersistentProperty<?> property) {
+		return property != null ? property.isIdProperty() : false;
+	}
+
+	@Override
+	public MongoPersistentProperty getIdProperty() {
+		return getPersistentProperty(Id.class);
+	}
+
+	@Override
+	public MongoPersistentProperty getVersionProperty() {
+		return getPersistentProperty(Version.class);
+	}
+
+	@Override
+	public MongoPersistentProperty getPersistentProperty(String name) {
+
+		for (MongoPersistentProperty p : this.properties) {
+			if (p.getName().equals(name)) {
+				return p;
+			}
+		}
+		return null;
+	}
+
+	@Override
+	public MongoPersistentProperty getPersistentProperty(Class<? extends Annotation> annotationType) {
+
+		for (MongoPersistentProperty p : this.properties) {
+			if (p.isAnnotationPresent(annotationType)) {
+				return p;
+			}
+		}
+		return null;
+	}
+
+	@Override
+	public boolean hasIdProperty() {
+		return false;
+	}
+
+	@Override
+	public boolean hasVersionProperty() {
+		return getVersionProperty() != null;
+	}
+
+	@Override
+	public Class<T> getType() {
+		return this.type;
+	}
+
+	@Override
+	public Object getTypeAlias() {
+		return null;
+	}
+
+	@Override
+	public TypeInformation<T> getTypeInformation() {
+		return null;
+	}
+
+	@Override
+	public void doWithProperties(PropertyHandler<MongoPersistentProperty> handler) {
+
+		for (MongoPersistentProperty p : this.properties) {
+			handler.doWithPersistentProperty(p);
+		}
+	}
+
+	@Override
+	public void doWithProperties(SimplePropertyHandler handler) {
+
+		for (MongoPersistentProperty p : this.properties) {
+			handler.doWithPersistentProperty(p);
+		}
+	}
+
+	@Override
+	public void doWithAssociations(AssociationHandler<MongoPersistentProperty> handler) {
+
+	}
+
+	@Override
+	public void doWithAssociations(SimpleAssociationHandler handler) {
+
+	}
+
+	@SuppressWarnings("unchecked")
+	@Override
+	public <A extends Annotation> A findAnnotation(Class<A> annotationType) {
+		return (A) this.annotations.get(annotationType);
+	}
+
+	@Override
+	public String getCollection() {
+		return this.collection;
+	}
+
+	/**
+	 * Simple builder to create {@link MongoPersistentEntityTestDummy} with defined properties.
+	 * 
+	 * @author Christoph Strobl
+	 * @param <T>
+	 */
+	public static class MongoPersistentEntityDummyBuilder<T> {
+
+		private MongoPersistentEntityTestDummy<T> instance;
+
+		private MongoPersistentEntityDummyBuilder(Class<T> type) {
+			this.instance = new MongoPersistentEntityTestDummy<T>();
+			this.instance.type = type;
+		}
+
+		@SuppressWarnings({ "rawtypes", "unchecked" })
+		public static <T> MongoPersistentEntityDummyBuilder<T> forClass(Class<T> type) {
+			return new MongoPersistentEntityDummyBuilder(type);
+		}
+
+		public MongoPersistentEntityDummyBuilder<T> withName(String name) {
+			this.instance.name = name;
+			return this;
+		}
+
+		public MongoPersistentEntityDummyBuilder<T> and(MongoPersistentProperty property) {
+			this.instance.properties.add(property);
+			return this;
+		}
+
+		public MongoPersistentEntityDummyBuilder<T> withCollection(String collection) {
+			this.instance.collection = collection;
+			return this;
+		}
+
+		public MongoPersistentEntityDummyBuilder<T> and(Annotation annotation) {
+			this.instance.annotations.put(annotation.annotationType(), annotation);
+			return this;
+		}
+
+		public MongoPersistentEntityTestDummy<T> build() {
+			return this.instance;
+		}
+
+	}
+}