From 3d651b72ad818ade17f45f948560f9331420e57a Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Tue, 19 Sep 2017 09:33:10 +0200 Subject: [PATCH] DATAMONGO-1782 - Polishing. toCyclePath now returns an empty String when Path does not cycle. Also split and add tests and move code to Java8. Original Pull Request: #500 --- .../MongoPersistentEntityIndexResolver.java | 173 +++++++++--------- .../mongodb/core/index/PathUnitTests.java | 32 +++- 2 files changed, 117 insertions(+), 88 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 60a35e515..97aa92914 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 @@ -28,6 +28,7 @@ import java.util.Iterator; import java.util.List; import java.util.Set; import java.util.concurrent.TimeUnit; +import java.util.stream.Collectors; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -104,39 +105,37 @@ 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)); - final CycleGuard guard = new CycleGuard(); - - root.doWithProperties(new PropertyHandler() { - - @Override - public void doWithPersistentProperty(MongoPersistentProperty persistentProperty) { - - try { - if (persistentProperty.isEntity()) { - indexInformation.addAll(resolveIndexForClass(persistentProperty.getTypeInformation().getActualType(), - persistentProperty.getFieldName(), Path.of(persistentProperty), root.getCollection(), guard)); - } - - IndexDefinitionHolder indexDefinitionHolder = createIndexDefinitionHolderForProperty( - persistentProperty.getFieldName(), root.getCollection(), persistentProperty); - if (indexDefinitionHolder != null) { - indexInformation.add(indexDefinitionHolder); - } - } catch (CyclicPropertyReferenceException e) { - LOGGER.info(e.getMessage()); - } - } - }); + root.doWithProperties((PropertyHandler) property -> this + .potentiallyAddIndexForProperty(root, property, indexInformation, new CycleGuard())); indexInformation.addAll(resolveIndexesForDbrefs("", root.getCollection(), root)); return indexInformation; } + private void potentiallyAddIndexForProperty(MongoPersistentEntity root, MongoPersistentProperty persistentProperty, + List indexes, CycleGuard guard) { + + try { + if (persistentProperty.isEntity()) { + indexes.addAll(resolveIndexForClass(persistentProperty.getTypeInformation().getActualType(), + persistentProperty.getFieldName(), Path.of(persistentProperty), root.getCollection(), guard)); + } + + IndexDefinitionHolder indexDefinitionHolder = createIndexDefinitionHolderForProperty( + persistentProperty.getFieldName(), root.getCollection(), persistentProperty); + if (indexDefinitionHolder != null) { + indexes.add(indexDefinitionHolder); + } + } catch (CyclicPropertyReferenceException e) { + LOGGER.info(e.getMessage()); + } + } + /** * Recursively resolve and inspect properties of given {@literal type} for indexes to be created. * @@ -153,41 +152,42 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver { MongoPersistentEntity entity = mappingContext.getRequiredPersistentEntity(type); - final List indexInformation = new ArrayList(); + 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(dotPath) ? dotPath + "." : "") - + persistentProperty.getFieldName(); - Path propertyPath = path.append(persistentProperty); - guard.protect(persistentProperty, propertyPath); - - if (persistentProperty.isEntity()) { - try { - indexInformation.addAll(resolveIndexForClass(persistentProperty.getTypeInformation().getActualType(), - propertyDotPath, propertyPath, collection, guard)); - } catch (CyclicPropertyReferenceException e) { - LOGGER.info(e.getMessage()); - } - } - - IndexDefinitionHolder indexDefinitionHolder = createIndexDefinitionHolderForProperty(propertyDotPath, - collection, persistentProperty); - if (indexDefinitionHolder != null) { - indexInformation.add(indexDefinitionHolder); - } - } - }); + entity.doWithProperties((PropertyHandler) property -> this + .guradAndPotentiallyAddIndexForProperty(property, dotPath, path, collection, indexInformation, guard)); indexInformation.addAll(resolveIndexesForDbrefs(dotPath, collection, entity)); return indexInformation; } + private void guradAndPotentiallyAddIndexForProperty(MongoPersistentProperty persistentProperty, String dotPath, + Path path, String collection, List indexes, CycleGuard guard) { + + String propertyDotPath = (StringUtils.hasText(dotPath) ? dotPath + "." : "") + persistentProperty.getFieldName(); + + Path propertyPath = path.append(persistentProperty); + guard.protect(persistentProperty, propertyPath); + + if (persistentProperty.isEntity()) { + try { + indexes.addAll(resolveIndexForClass(persistentProperty.getTypeInformation().getActualType(), propertyDotPath, + propertyPath, collection, guard)); + } catch (CyclicPropertyReferenceException e) { + LOGGER.info(e.getMessage()); + } + } + + IndexDefinitionHolder indexDefinitionHolder = createIndexDefinitionHolderForProperty(propertyDotPath, collection, + persistentProperty); + + if (indexDefinitionHolder != null) { + indexes.add(indexDefinitionHolder); + } + } + @Nullable private IndexDefinitionHolder createIndexDefinitionHolderForProperty(String dotPath, String collection, MongoPersistentProperty persistentProperty) { @@ -319,13 +319,13 @@ 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) { - for (CompoundIndex index : indexes.value()) { - indexDefinitions.add(createCompoundIndexDefinition(dotPath, fallbackCollection, index, entity)); - } + indexDefinitions = Arrays.stream(indexes.value()) + .map(index -> createCompoundIndexDefinition(dotPath, fallbackCollection, index, entity)) + .collect(Collectors.toList()); } CompoundIndex index = entity.findAnnotation(CompoundIndex.class); @@ -395,6 +395,7 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver { * @param persitentProperty * @return */ + @Nullable protected IndexDefinitionHolder createIndexDefinition(String dotPath, String collection, MongoPersistentProperty persitentProperty) { @@ -439,6 +440,7 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver { * @param persistentProperty * @return */ + @Nullable protected IndexDefinitionHolder createGeoSpatialIndexDefinition(String dotPath, String collection, MongoPersistentProperty persistentProperty) { @@ -482,34 +484,33 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver { private List resolveIndexesForDbrefs(final String path, final String collection, MongoPersistentEntity entity) { - final List indexes = new ArrayList(0); - entity.doWithAssociations(new AssociationHandler() { - - @Override - public void doWithAssociation(Association association) { - - MongoPersistentProperty property = association.getInverse(); - - String propertyDotPath = (StringUtils.hasText(path) ? path + "." : "") + property.getFieldName(); - - if (property.isAnnotationPresent(GeoSpatialIndexed.class) || property.isAnnotationPresent(TextIndexed.class)) { - throw new MappingException( - String.format("Cannot create geospatial-/text- index on DBRef in collection '%s' for path '%s'.", - collection, propertyDotPath)); - } - - IndexDefinitionHolder indexDefinitionHolder = createIndexDefinitionHolderForProperty(propertyDotPath, - collection, property); - - if (indexDefinitionHolder != null) { - indexes.add(indexDefinitionHolder); - } - } - }); - + final List indexes = new ArrayList<>(0); + entity.doWithAssociations((AssociationHandler) association -> this + .resolveAndAddIndexesForAssociation(association, indexes, path, collection)); return indexes; } + private void resolveAndAddIndexesForAssociation(Association association, + List indexes, String path, String collection) { + + MongoPersistentProperty property = association.getInverse(); + + String propertyDotPath = (StringUtils.hasText(path) ? path + "." : "") + property.getFieldName(); + + if (property.isAnnotationPresent(GeoSpatialIndexed.class) || property.isAnnotationPresent(TextIndexed.class)) { + throw new MappingException( + String.format("Cannot create geospatial-/text- index on DBRef in collection '%s' for path '%s'.", collection, + propertyDotPath)); + } + + IndexDefinitionHolder indexDefinitionHolder = createIndexDefinitionHolderForProperty(propertyDotPath, collection, + property); + + if (indexDefinitionHolder != null) { + indexes.add(indexDefinitionHolder); + } + } + /** * {@link CycleGuard} holds information about properties and the paths for accessing those. This information is used * to detect potential cycles within the references. @@ -519,7 +520,7 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver { */ static class CycleGuard { - private final Set seenProperties = new HashSet(); + private final Set seenProperties = new HashSet<>(); /** * Detect a cycle in a property path if the property was seen at least once. @@ -573,7 +574,7 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver { @EqualsAndHashCode static class Path { - private static final Path EMPTY = new Path(Collections.> emptyList(), false); + private static final Path EMPTY = new Path(Collections.emptyList(), false); private final List> elements; private final boolean cycle; @@ -594,7 +595,7 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver { * @since 1.10.8 */ static Path of(PersistentProperty initial) { - return new Path(Collections.> singletonList(initial), false); + return new Path(Collections.singletonList(initial), false); } /** @@ -606,7 +607,7 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver { */ Path append(PersistentProperty breadcrumb) { - List> elements = new ArrayList>(this.elements.size() + 1); + List> elements = new ArrayList<>(this.elements.size() + 1); elements.addAll(this.elements); elements.add(breadcrumb); @@ -639,6 +640,10 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver { */ String toCyclePath() { + if (!cycle) { + return ""; + } + for (int i = 0; i < this.elements.size(); i++) { int index = indexOf(this.elements, this.elements.get(i), i + 1); 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 288fda987..f92b82676 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 @@ -52,10 +52,34 @@ public class PathUnitTests { MongoPersistentProperty foo = createPersistentPropertyMock(entityMock, "foo"); MongoPersistentProperty bar = createPersistentPropertyMock(entityMock, "bar"); - 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"))); + Path path = Path.of(foo).append(bar).append(bar); + + assertThat(path.isCycle(), is(true)); + assertThat(path.toCyclePath(), is(equalTo("bar -> bar"))); + assertThat(path.toString(), is(equalTo("foo -> bar -> bar"))); + } + + @Test // DATAMONGO-1782 + public void isCycleShouldReturnFalseWhenNoCyclePresent() { + + MongoPersistentProperty foo = createPersistentPropertyMock(entityMock, "foo"); + MongoPersistentProperty bar = createPersistentPropertyMock(entityMock, "bar"); + + Path path = Path.of(foo).append(bar); + + assertThat(path.isCycle(), is(false)); + assertThat(path.toCyclePath(), is(equalTo(""))); + assertThat(path.toString(), is(equalTo("foo -> bar"))); + } + + @Test // DATAMONGO-1782 + public void isCycleShouldReturnFalseCycleForNonEqualProperties() { + + MongoPersistentProperty foo = createPersistentPropertyMock(entityMock, "foo"); + MongoPersistentProperty bar = createPersistentPropertyMock(entityMock, "bar"); + MongoPersistentProperty bar2 = createPersistentPropertyMock(mock(MongoPersistentEntity.class), "bar"); + + assertThat(Path.of(foo).append(bar).append(bar2).isCycle(), is(false)); } @SuppressWarnings({ "rawtypes", "unchecked" })