From ec1a6b5edd1eb719e9538cf19451e2f45fda16fd Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Thu, 21 Aug 2014 08:45:34 +0200 Subject: [PATCH] DATAMONGO-1025 - Fix creation of nested named index. We new prefix explicitly named indexes on nested types (eg. for embedded properties) with the path pointing to the property. This avoids errors having equally named index definitions on different paths pointing to the same type within one collection. Along the way we harmonized index naming for geospatial index definitions where only the properties field name was taken into account where it should have been the full property path. Original pull request: #219. --- .../mongodb/core/index/CompoundIndex.java | 38 ++++++- .../mongodb/core/index/GeoSpatialIndexed.java | 36 ++++++- .../data/mongodb/core/index/Indexed.java | 36 ++++++- .../MongoPersistentEntityIndexResolver.java | 50 ++++++--- ...PersistentEntityIndexCreatorUnitTests.java | 4 +- ...ersistentEntityIndexResolverUnitTests.java | 102 ++++++++++++++++++ 6 files changed, 244 insertions(+), 22 deletions(-) diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/index/CompoundIndex.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/index/CompoundIndex.java index 55fee13fa..eb31e7a96 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/index/CompoundIndex.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/index/CompoundIndex.java @@ -73,7 +73,43 @@ public @interface CompoundIndex { boolean dropDups() default false; /** - * The name of the index to be created. + * The name of the index to be created.
+ *
+ * The name will only be applied as is when defined on root level. For usage on nested or embedded structures the + * provided name will be prefixed with the path leading to the entity.
+ *
+ * The structure below + * + * 
+	 * 
+	 * @Document
+	 * class Root {
+	 *   Hybrid hybrid;
+	 *   Nested nested;
+	 * }
+	 * 
+	 * @Document
+	 * @CompoundIndex(name = "compound_index", def = "{'h1': 1, 'h2': 1}")
+	 * class Hybrid {
+	 *   String h1, h2;
+	 * }
+	 * 
+	 * @CompoundIndex(name = "compound_index", def = "{'n1': 1, 'n2': 1}")
+	 * class Nested {
+	 *   String n1, n2;
+	 * }
+	 * 
+	 *
+ * + * resolves in the following index structures + * + * 
+	 * 
+	 * db.root.ensureIndex( { hybrid.h1: 1, hybrid.h2: 1 } , { name: "hybrid.compound_index" } )
+	 * db.root.ensureIndex( { nested.n1: 1, nested.n2: 1 } , { name: "nested.compound_index" } )
+	 * db.hybrid.ensureIndex( { h1: 1, h2: 1 } , { name: "compound_index" } )
+	 * 
+	 *
* * @return */ diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/index/GeoSpatialIndexed.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/index/GeoSpatialIndexed.java index 3bf128c14..7b4c09c79 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/index/GeoSpatialIndexed.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/index/GeoSpatialIndexed.java @@ -32,7 +32,41 @@ import java.lang.annotation.Target; public @interface GeoSpatialIndexed { /** - * Name of the property in the document that contains the [x, y] or radial coordinates to index. + * Index name.
+ *
+ * The name will only be applied as is when defined on root level. For usage on nested or embedded structures the + * provided name will be prefixed with the path leading to the entity.
+ *
+ * The structure below + * + * 
+	 * 
+	 * @Document
+	 * class Root {
+	 *   Hybrid hybrid;
+	 *   Nested nested;
+	 * }
+	 * 
+	 * @Document
+	 * class Hybrid {
+	 *   @GeoSpatialIndexed(name="index") Point h1;
+	 * }
+	 * 
+	 * class Nested {
+	 *   @GeoSpatialIndexed(name="index") Point n1;
+	 * }
+	 * 
+	 *
+ * + * resolves in the following index structures + * + * 
+	 * 
+	 * db.root.ensureIndex( { hybrid.h1: "2d" } , { name: "hybrid.index" } )
+	 * db.root.ensureIndex( { nested.n1: "2d" } , { name: "nested.index" } )
+	 * db.hybrid.ensureIndex( { h1: "2d" } , { name: "index" } )
+	 * 
+	 *
* * @return */ diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/index/Indexed.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/index/Indexed.java index 73aaeb15e..a907d4770 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/index/Indexed.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/index/Indexed.java @@ -58,7 +58,41 @@ public @interface Indexed { boolean dropDups() default false; /** - * Index name. + * Index name.
+ *
+ * The name will only be applied as is when defined on root level. For usage on nested or embedded structures the + * provided name will be prefixed with the path leading to the entity.
+ *
+ * The structure below + * + * 
+	 * 
+	 * @Document
+	 * class Root {
+	 *   Hybrid hybrid;
+	 *   Nested nested;
+	 * }
+	 * 
+	 * @Document
+	 * class Hybrid {
+	 *   @Indexed(name="index") String h1;
+	 * }
+	 * 
+	 * class Nested {
+	 *   @Indexed(name="index") String n1;
+	 * }
+	 * 
+	 *
+ * + * resolves in the following index structures + * + * 
+	 * 
+	 * db.root.ensureIndex( { hybrid.h1: 1 } , { name: "hybrid.index" } )
+	 * db.root.ensureIndex( { nested.n1: 1 } , { name: "nested.index" } )
+	 * db.hybrid.ensureIndex( { h1: 1} , { name: "index" } )
+	 * 
+	 *
* * @return */ 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 d51089a35..947a56b9a 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 @@ -25,7 +25,6 @@ import java.util.concurrent.TimeUnit; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import org.springframework.core.annotation.AnnotationUtils; import org.springframework.dao.InvalidDataAccessApiUsageException; import org.springframework.data.domain.Sort; import org.springframework.data.mapping.PropertyHandler; @@ -96,7 +95,7 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver { Assert.notNull(document, "Given entity is not collection root."); final List indexInformation = new ArrayList(); - indexInformation.addAll(potentiallyCreateCompoundIndexDefinitions("", root.getCollection(), root.getType())); + indexInformation.addAll(potentiallyCreateCompoundIndexDefinitions("", root.getCollection(), root)); indexInformation.addAll(potentiallyCreateTextIndexDefinition(root)); final CycleGuard guard = new CycleGuard(); @@ -138,10 +137,11 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver { private List resolveIndexForClass(final Class type, final String path, final String collection, final CycleGuard guard) { + MongoPersistentEntity entity = mappingContext.getPersistentEntity(type); + final List indexInformation = new ArrayList(); - indexInformation.addAll(potentiallyCreateCompoundIndexDefinitions(path, collection, type)); + indexInformation.addAll(potentiallyCreateCompoundIndexDefinitions(path, collection, entity)); - MongoPersistentEntity entity = mappingContext.getPersistentEntity(type); entity.doWithProperties(new PropertyHandler() { @Override @@ -183,14 +183,13 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver { } private List potentiallyCreateCompoundIndexDefinitions(String dotPath, String collection, - Class type) { + MongoPersistentEntity entity) { - if (AnnotationUtils.findAnnotation(type, CompoundIndexes.class) == null - && AnnotationUtils.findAnnotation(type, CompoundIndex.class) == null) { + if (entity.findAnnotation(CompoundIndexes.class) == null && entity.findAnnotation(CompoundIndex.class) == null) { return Collections.emptyList(); } - return createCompoundIndexDefinitions(dotPath, collection, type); + return createCompoundIndexDefinitions(dotPath, collection, entity); } private Collection potentiallyCreateTextIndexDefinition(MongoPersistentEntity root) { @@ -278,21 +277,21 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver { * @return */ protected List createCompoundIndexDefinitions(String dotPath, String fallbackCollection, - Class type) { + MongoPersistentEntity entity) { List indexDefinitions = new ArrayList(); - CompoundIndexes indexes = AnnotationUtils.findAnnotation(type, CompoundIndexes.class); + CompoundIndexes indexes = entity.findAnnotation(CompoundIndexes.class); if (indexes != null) { for (CompoundIndex index : indexes.value()) { - indexDefinitions.add(createCompoundIndexDefinition(dotPath, fallbackCollection, index)); + indexDefinitions.add(createCompoundIndexDefinition(dotPath, fallbackCollection, index, entity)); } } - CompoundIndex index = AnnotationUtils.findAnnotation(type, CompoundIndex.class); + CompoundIndex index = entity.findAnnotation(CompoundIndex.class); if (index != null) { - indexDefinitions.add(createCompoundIndexDefinition(dotPath, fallbackCollection, index)); + indexDefinitions.add(createCompoundIndexDefinition(dotPath, fallbackCollection, index, entity)); } return indexDefinitions; @@ -300,13 +299,13 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver { @SuppressWarnings("deprecation") protected IndexDefinitionHolder createCompoundIndexDefinition(String dotPath, String fallbackCollection, - CompoundIndex index) { + CompoundIndex index, MongoPersistentEntity entity) { CompoundIndexDefinition indexDefinition = new CompoundIndexDefinition(resolveCompoundIndexKeyFromStringDefinition( dotPath, index.def())); if (!index.useGeneratedName()) { - indexDefinition.named(index.name()); + indexDefinition.named(pathAwareIndexName(index.name(), dotPath, null)); } if (index.unique()) { @@ -377,7 +376,7 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver { IndexDirection.ASCENDING.equals(index.direction()) ? Sort.Direction.ASC : Sort.Direction.DESC); if (!index.useGeneratedName()) { - indexDefinition.named(StringUtils.hasText(index.name()) ? index.name() : dotPath); + indexDefinition.named(pathAwareIndexName(index.name(), dotPath, persitentProperty)); } if (index.unique()) { @@ -419,7 +418,7 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver { indexDefinition.withMin(index.min()).withMax(index.max()); if (!index.useGeneratedName()) { - indexDefinition.named(StringUtils.hasText(index.name()) ? index.name() : persistentProperty.getName()); + indexDefinition.named(pathAwareIndexName(index.name(), dotPath, persistentProperty)); } indexDefinition.typed(index.type()).withBucketSize(index.bucketSize()).withAdditionalField(index.additionalField()); @@ -427,6 +426,23 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver { return new IndexDefinitionHolder(dotPath, indexDefinition, collection); } + private String pathAwareIndexName(String indexName, String dotPath, MongoPersistentProperty property) { + + String nameToUse = StringUtils.hasText(indexName) ? indexName : ""; + + if (!StringUtils.hasText(dotPath) || (property != null && dotPath.equals(property.getFieldName()))) { + return StringUtils.hasText(nameToUse) ? nameToUse : dotPath; + } + + if (StringUtils.hasText(dotPath)) { + + nameToUse = StringUtils.hasText(nameToUse) ? (property != null ? dotPath.replace("." + property.getFieldName(), + "") : dotPath) + "." + nameToUse : dotPath; + } + return nameToUse; + + } + /** * {@link CycleGuard} holds information about properties and the paths for accessing those. This information is used * to detect potential cycles within the references. diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/index/MongoPersistentEntityIndexCreatorUnitTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/index/MongoPersistentEntityIndexCreatorUnitTests.java index 8887f9bde..815dd775d 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/index/MongoPersistentEntityIndexCreatorUnitTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/index/MongoPersistentEntityIndexCreatorUnitTests.java @@ -163,8 +163,8 @@ public class MongoPersistentEntityIndexCreatorUnitTests { new MongoPersistentEntityIndexCreator(mappingContext, factory); assertThat(keysCaptor.getValue(), equalTo(new BasicDBObjectBuilder().add("company.address.location", "2d").get())); - assertThat(optionsCaptor.getValue(), equalTo(new BasicDBObjectBuilder().add("name", "location").add("min", -180) - .add("max", 180).add("bits", 26).get())); + assertThat(optionsCaptor.getValue(), equalTo(new BasicDBObjectBuilder().add("name", "company.address.location") + .add("min", -180).add("max", 180).add("bits", 26).get())); } /** 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 c05aec1df..2af355571 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 @@ -744,6 +744,60 @@ public class MongoPersistentEntityIndexResolverUnitTests { .resolveIndexForEntity(dummy); } + /** + * @see DATAMONGO-1025 + */ + @Test + public void shouldUsePathIndexAsIndexNameForDocumentsHavingNamedNestedCompoundIndexFixedOnCollection() { + + List indexDefinitions = prepareMappingContextAndResolveIndexForType(DocumentWithNestedDocumentHavingNamedCompoundIndex.class); + assertThat((String) indexDefinitions.get(0).getIndexOptions().get("name"), + equalTo("propertyOfTypeHavingNamedCompoundIndex.c_index")); + } + + /** + * @see DATAMONGO-1025 + */ + @Test + public void shouldUseIndexNameForNestedTypesWithNamedCompoundIndexDefinition() { + + List indexDefinitions = prepareMappingContextAndResolveIndexForType(DocumentWithNestedTypeHavingNamedCompoundIndex.class); + assertThat((String) indexDefinitions.get(0).getIndexOptions().get("name"), + equalTo("propertyOfTypeHavingNamedCompoundIndex.c_index")); + } + + /** + * @see DATAMONGO-1025 + */ + @Test + public void shouldUsePathIndexAsIndexNameForDocumentsHavingNamedNestedIndexFixedOnCollection() { + + List indexDefinitions = prepareMappingContextAndResolveIndexForType(DocumentWithNestedDocumentHavingNamedIndex.class); + assertThat((String) indexDefinitions.get(0).getIndexOptions().get("name"), + equalTo("propertyOfTypeHavingNamedIndex.property_index")); + } + + /** + * @see DATAMONGO-1025 + */ + @Test + public void shouldUseIndexNameForNestedTypesWithNamedIndexDefinition() { + + List indexDefinitions = prepareMappingContextAndResolveIndexForType(DocumentWithNestedTypeHavingNamedIndex.class); + assertThat((String) indexDefinitions.get(0).getIndexOptions().get("name"), + equalTo("propertyOfTypeHavingNamedIndex.property_index")); + } + + /** + * @see DATAMONGO-1025 + */ + @Test + public void shouldUseIndexNameOnRootLevel() { + + List indexDefinitions = prepareMappingContextAndResolveIndexForType(DocumentWithNamedIndex.class); + assertThat((String) indexDefinitions.get(0).getIndexOptions().get("name"), equalTo("property_index")); + } + @Document static class MixedIndexRoot { @@ -836,6 +890,54 @@ public class MongoPersistentEntityIndexResolverUnitTests { List cyclic; } + + @Document + @CompoundIndex(name = "c_index", def = "{ foo:1, bar:1 }") + static class DocumentWithNamedCompoundIndex { + + String property; + } + + @Document + static class DocumentWithNamedIndex { + + @Indexed(name = "property_index") String property; + } + + static class TypeWithNamedIndex { + + @Indexed(name = "property_index") String property; + } + + @Document + static class DocumentWithNestedDocumentHavingNamedCompoundIndex { + + DocumentWithNamedCompoundIndex propertyOfTypeHavingNamedCompoundIndex; + } + + @CompoundIndex(name = "c_index", def = "{ foo:1, bar:1 }") + static class TypeWithNamedCompoundIndex { + String property; + } + + @Document + static class DocumentWithNestedTypeHavingNamedCompoundIndex { + + TypeWithNamedCompoundIndex propertyOfTypeHavingNamedCompoundIndex; + } + + @Document + static class DocumentWithNestedDocumentHavingNamedIndex { + + DocumentWithNamedIndex propertyOfTypeHavingNamedIndex; + } + + @Document + static class DocumentWithNestedTypeHavingNamedIndex { + + TypeWithNamedIndex propertyOfTypeHavingNamedIndex; + } + } private static List prepareMappingContextAndResolveIndexForType(Class type) {