From 244fbae0ce0def024fd0e6c748e1bae55ddee90e Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Thu, 26 Jun 2014 09:08:26 +0200 Subject: [PATCH] DATAMONGO-962 - Cycle guard should respect full path. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../MongoPersistentEntityIndexResolver.java | 79 +++++-- ...ersistentEntityIndexResolverUnitTests.java | 80 ++++++- .../mongodb/core/index/PathUnitTests.java | 92 ++++++++ .../MongoPersistentEntityTestDummy.java | 212 ++++++++++++++++++ 4 files changed, 435 insertions(+), 28 deletions(-) create mode 100644 spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/index/PathUnitTests.java create mode 100644 spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/mapping/MongoPersistentEntityTestDummy.java 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; + } + + } +}