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 6da85c086..9c299125c 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 @@ -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()) { - indexInformation.addAll(resolveIndexForClass(persistentProperty.getActualType(), - persistentProperty.getFieldName(), root.getCollection(), guard)); - } + try { + if (persistentProperty.isEntity()) { + indexInformation.addAll(resolveIndexForClass(persistentProperty.getActualType(), + persistentProperty.getFieldName(), root.getCollection(), guard)); + } - IndexDefinitionHolder indexDefinitionHolder = createIndexDefinitionHolderForProperty( - persistentProperty.getFieldName(), root.getCollection(), persistentProperty); - if (indexDefinitionHolder != null) { - indexInformation.add(indexDefinitionHolder); + IndexDefinitionHolder indexDefinitionHolder = createIndexDefinitionHolderForProperty( + persistentProperty.getFieldName(), root.getCollection(), persistentProperty); + if (indexDefinitionHolder != null) { + 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> 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 paths = new ArrayList(); @@ -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.
+ * A {@link Path} with {@literal spring.data.mongodb} would be created for the property {@code Three.mongodb}. + * + *
+		 * 
+		 * @Document
+		 * class One {
+		 *   Two spring;
+		 * }
+		 * 
+		 * class Two {
+		 *   Three data;
+		 * }
+		 * 
+		 * class Three {
+		 *   String mongodb;
+		 * }
+		 * 
+		 * 
+ * + * @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 + * 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}. + * + * @param property + * @param path + * @return + */ + boolean cycles(MongoPersistentProperty property, String path) { - Pattern pattern = Pattern.compile("\\b" + Pattern.quote(property.getFieldName()) + "\\b"); - Matcher matcher = pattern.matcher(path); - - int count = 0; - while (matcher.find()) { - count++; + 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(), - dotPath); + return String.format("Found cycle for field '%s' in type '%s' for path '%s'", propertyName, + type != null ? type.getSimpleName() : "unknown", dotPath); } } 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 2eb1ff9a1..4667a6975 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 @@ -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 indexDefinitions = prepareMappingContextAndResolveIndexForType(CycleStartingInBetween.class); - assertThat(indexDefinitions, hasSize(2)); + assertThat(indexDefinitions, hasSize(1)); } /** @@ -490,9 +496,7 @@ public class MongoPersistentEntityIndexResolverUnitTests { List indexDefinitions = prepareMappingContextAndResolveIndexForType(CycleOnLevelOne.class); assertIndexPathAndCollection("reference.indexedProperty", "cycleOnLevelOne", indexDefinitions.get(0)); - assertIndexPathAndCollection("reference.cyclicReference.reference.indexedProperty", "cycleOnLevelOne", - indexDefinitions.get(1)); - assertThat(indexDefinitions, hasSize(2)); + assertThat(indexDefinitions, hasSize(1)); } /** @@ -520,6 +524,58 @@ public class MongoPersistentEntityIndexResolverUnitTests { assertThat(indexDefinitions, hasSize(1)); } + /** + * @see DATAMONGO-962 + */ + @Test + public void shouldDetectSelfCycleViaCollectionTypeCorrectly() { + + List indexDefinitions = prepareMappingContextAndResolveIndexForType(SelfCyclingViaCollectionType.class); + assertThat(indexDefinitions, IsEmptyCollection.empty()); + } + + /** + * @see DATAMONGO-962 + */ + @Test + public void shouldNotDetectCycleWhenTypeIsUsedMoreThanOnce() { + + List indexDefinitions = prepareMappingContextAndResolveIndexForType(MultipleObjectsOfSameType.class); + assertThat(indexDefinitions, IsEmptyCollection.empty()); + } + + /** + * @see DATAMONGO-962 + */ + @Test + public void shouldCatchCyclicReferenceExceptionOnRoot() { + + Document documentDummy = new Document() { + + @Override + public Class 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 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 cyclic; + + } } private static List prepareMappingContextAndResolveIndexForType(Class type) { @@ -629,4 +700,5 @@ public class MongoPersistentEntityIndexResolverUnitTests { } assertThat(holder.getCollection(), equalTo(expectedCollection)); } + } 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 new file mode 100644 index 000000000..9e88c00c1 --- /dev/null +++ b/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; + } +} diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/mapping/MongoPersistentEntityTestDummy.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/mapping/MongoPersistentEntityTestDummy.java new file mode 100644 index 000000000..8512a6edd --- /dev/null +++ b/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 + */ +public class MongoPersistentEntityTestDummy implements MongoPersistentEntity { + + private Map, Annotation> annotations = new HashMap, Annotation>(); + private Collection properties = new ArrayList(); + private String collection; + private String name; + private Class type; + + @Override + public String getName() { + return name; + } + + @Override + public PreferredConstructor 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 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 getType() { + return this.type; + } + + @Override + public Object getTypeAlias() { + return null; + } + + @Override + public TypeInformation getTypeInformation() { + return null; + } + + @Override + public void doWithProperties(PropertyHandler 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 handler) { + + } + + @Override + public void doWithAssociations(SimpleAssociationHandler handler) { + + } + + @SuppressWarnings("unchecked") + @Override + public A findAnnotation(Class 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 + */ + public static class MongoPersistentEntityDummyBuilder { + + private MongoPersistentEntityTestDummy instance; + + private MongoPersistentEntityDummyBuilder(Class type) { + this.instance = new MongoPersistentEntityTestDummy(); + this.instance.type = type; + } + + @SuppressWarnings({ "rawtypes", "unchecked" }) + public static MongoPersistentEntityDummyBuilder forClass(Class type) { + return new MongoPersistentEntityDummyBuilder(type); + } + + public MongoPersistentEntityDummyBuilder withName(String name) { + this.instance.name = name; + return this; + } + + public MongoPersistentEntityDummyBuilder and(MongoPersistentProperty property) { + this.instance.properties.add(property); + return this; + } + + public MongoPersistentEntityDummyBuilder withCollection(String collection) { + this.instance.collection = collection; + return this; + } + + public MongoPersistentEntityDummyBuilder and(Annotation annotation) { + this.instance.annotations.put(annotation.annotationType(), annotation); + return this; + } + + public MongoPersistentEntityTestDummy build() { + return this.instance; + } + + } +}