From 9c1f753f1773ad780f10ffd2112a6dfedd394835 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Mon, 5 May 2014 11:52:08 +0200 Subject: [PATCH] DATAMONGO-929 - Use property path for keys of Indexed and CompoundIndex. Index creation failed for @Indexed and @CompoundIndex as the resolved dotPath was not used for creation. We now not only resolve the dotPath but also use it within the key for index definition. In case of a nested compound index the key definition is enhanced by the provided path. When leaving the key definition empty for nested compound index we'll create an index for the whole nested document. Trying to create a compound index on root level not providing key information leads to InvalidDataApiUsageException. Original pull request: #179. --- .../mongodb/core/index/CompoundIndex.java | 5 +- .../MongoPersistentEntityIndexResolver.java | 104 +++++++++++++----- ...ersistentEntityIndexResolverUnitTests.java | 70 +++++++++++- 3 files changed, 145 insertions(+), 34 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 fc480bf3e..a3cdf34af 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 @@ -36,11 +36,12 @@ public @interface CompoundIndex { /** * The actual index definition in JSON format. The keys of the JSON document are the fields to be indexed, the values - * define the index direction (1 for ascending, -1 for descending). + * define the index direction (1 for ascending, -1 for descending).
+ * If left empty on nested document, the whole document will be indexed. * * @return */ - String def(); + String def() default ""; /** * It does not actually make sense to use that attribute as the direction has to be defined in the {@link #def()} 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 1e51bad66..e50f14761 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,6 +21,7 @@ import java.util.List; import java.util.concurrent.TimeUnit; import org.springframework.core.annotation.AnnotationUtils; +import org.springframework.dao.InvalidDataAccessApiUsageException; import org.springframework.data.domain.Sort; import org.springframework.data.mapping.PropertyHandler; import org.springframework.data.mongodb.core.index.Index.Duplicates; @@ -31,6 +32,8 @@ import org.springframework.data.mongodb.core.mapping.MongoPersistentProperty; import org.springframework.util.Assert; import org.springframework.util.StringUtils; +import com.mongodb.BasicDBObject; +import com.mongodb.BasicDBObjectBuilder; import com.mongodb.DBObject; import com.mongodb.util.JSON; @@ -158,7 +161,8 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver { private List potentiallyCreateCompoundIndexDefinitions(String dotPath, String collection, Class type) { - if (AnnotationUtils.findAnnotation(type, CompoundIndexes.class) == null) { + if (AnnotationUtils.findAnnotation(type, CompoundIndexes.class) == null + && AnnotationUtils.findAnnotation(type, CompoundIndex.class) == null) { return Collections.emptyList(); } @@ -176,38 +180,77 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver { protected List createCompoundIndexDefinitions(String dotPath, String fallbackCollection, Class type) { + List indexDefinitions = new ArrayList(); CompoundIndexes indexes = AnnotationUtils.findAnnotation(type, CompoundIndexes.class); - List indexDefinitions = new ArrayList( - indexes.value().length); - for (CompoundIndex index : indexes.value()) { + if (indexes != null) { + for (CompoundIndex index : indexes.value()) { + indexDefinitions.add(createCompoundIndexDefinition(dotPath, fallbackCollection, index)); + } + } - String path = StringUtils.hasText(index.name()) ? index.name() : dotPath; - String collection = StringUtils.hasText(index.collection()) ? index.collection() : fallbackCollection; + CompoundIndex index = AnnotationUtils.findAnnotation(type, CompoundIndex.class); - CompoundIndexDefinition indexDefinition = new CompoundIndexDefinition((DBObject) JSON.parse(index.def())); - if (!index.useGeneratedName()) { - indexDefinition.named(index.name()); - } - if (index.unique()) { - indexDefinition.unique(index.dropDups() ? Duplicates.DROP : Duplicates.RETAIN); - } - if (index.sparse()) { - indexDefinition.sparse(); - } - if (index.background()) { - indexDefinition.background(); - } - if (index.expireAfterSeconds() >= 0) { - indexDefinition.expire(index.expireAfterSeconds(), TimeUnit.SECONDS); - } - - indexDefinitions.add(new IndexDefinitionHolder(path, indexDefinition, collection)); + if (index != null) { + indexDefinitions.add(createCompoundIndexDefinition(dotPath, fallbackCollection, index)); } return indexDefinitions; } + protected IndexDefinitionHolder createCompoundIndexDefinition(String dotPath, String fallbackCollection, + CompoundIndex index) { + + CompoundIndexDefinition indexDefinition = new CompoundIndexDefinition(resolveCompoundIndexKeyFromStringDefinition( + dotPath, index.def())); + + if (!index.useGeneratedName()) { + indexDefinition.named(index.name()); + } + + if (index.unique()) { + indexDefinition.unique(index.dropDups() ? Duplicates.DROP : Duplicates.RETAIN); + } + + if (index.sparse()) { + indexDefinition.sparse(); + } + + if (index.background()) { + indexDefinition.background(); + } + + if (index.expireAfterSeconds() >= 0) { + indexDefinition.expire(index.expireAfterSeconds(), TimeUnit.SECONDS); + } + + String collection = StringUtils.hasText(index.collection()) ? index.collection() : fallbackCollection; + return new IndexDefinitionHolder(dotPath, indexDefinition, collection); + } + + private DBObject resolveCompoundIndexKeyFromStringDefinition(String dotPath, String keyDefinitionString) { + + if (!StringUtils.hasText(dotPath) && !StringUtils.hasText(keyDefinitionString)) { + throw new InvalidDataAccessApiUsageException("Cannot create index on root level for empty keys."); + } + + if (!StringUtils.hasText(keyDefinitionString)) { + return new BasicDBObject(dotPath, 1); + } + + DBObject dbo = (DBObject) JSON.parse(keyDefinitionString); + if (!StringUtils.hasText(dotPath)) { + return dbo; + } + + BasicDBObjectBuilder dboBuilder = new BasicDBObjectBuilder(); + + for (String key : dbo.keySet()) { + dboBuilder.add(dotPath + "." + key, dbo.get(key)); + } + return dboBuilder.get(); + } + /** * Creates {@link IndexDefinition} wrapped in {@link IndexDefinitionHolder} out of {@link Indexed} for given * {@link MongoPersistentProperty}. @@ -223,24 +266,25 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver { Indexed index = persitentProperty.findAnnotation(Indexed.class); String collection = StringUtils.hasText(index.collection()) ? index.collection() : fallbackCollection; - Index indexDefinition = new Index(); + Index indexDefinition = new Index().on(dotPath, + IndexDirection.ASCENDING.equals(index.direction()) ? Sort.Direction.ASC : Sort.Direction.DESC); if (!index.useGeneratedName()) { - indexDefinition.named(StringUtils.hasText(index.name()) ? index.name() : persitentProperty.getFieldName()); + indexDefinition.named(StringUtils.hasText(index.name()) ? index.name() : dotPath); } - indexDefinition.on(persitentProperty.getFieldName(), - IndexDirection.ASCENDING.equals(index.direction()) ? Sort.Direction.ASC : Sort.Direction.DESC); - if (index.unique()) { indexDefinition.unique(index.dropDups() ? Duplicates.DROP : Duplicates.RETAIN); } + if (index.sparse()) { indexDefinition.sparse(); } + if (index.background()) { indexDefinition.background(); } + if (index.expireAfterSeconds() >= 0) { indexDefinition.expire(index.expireAfterSeconds(), TimeUnit.SECONDS); } @@ -306,7 +350,7 @@ public class MongoPersistentEntityIndexResolver implements IndexResolver { } /** - * Get the {@liteal "dot"} path used to create the index. + * Get the {@literal "dot"} path used to create the index. * * @return */ 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 6fa033b7a..b8dcd9bd7 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 @@ -277,7 +277,7 @@ public class MongoPersistentEntityIndexResolverUnitTests { List indexDefinitions = prepareMappingContextAndResolveIndexForType(CompoundIndexOnLevelZero.class); assertThat(indexDefinitions, hasSize(1)); - assertIndexPathAndCollection("compound_index", "CompoundIndexOnLevelZero", indexDefinitions.get(0)); + assertIndexPathAndCollection(new String[] { "foo", "bar" }, "CompoundIndexOnLevelZero", indexDefinitions.get(0)); } /** @@ -324,7 +324,56 @@ public class MongoPersistentEntityIndexResolverUnitTests { equalTo(new BasicDBObjectBuilder().add("unique", true).add("dropDups", true).add("sparse", true) .add("background", true).add("expireAfterSeconds", 10L).get())); assertThat(indexDefinition.getIndexKeys(), equalTo(new BasicDBObjectBuilder().add("foo", 1).add("bar", -1).get())); + } + /** + * @see DATAMONGO-929 + */ + @Test + public void compoundIndexPathOnLevelOneIsResolvedCorrectly() { + + List indexDefinitions = prepareMappingContextAndResolveIndexForType(CompoundIndexOnLevelOne.class); + + assertThat(indexDefinitions, hasSize(1)); + assertIndexPathAndCollection(new String[] { "zero.foo", "zero.bar" }, "CompoundIndexOnLevelOne", + indexDefinitions.get(0)); + } + + /** + * @see DATAMONGO-929 + */ + @Test + public void emptyCompoundIndexPathOnLevelOneIsResolvedCorrectly() { + + List indexDefinitions = prepareMappingContextAndResolveIndexForType(CompoundIndexOnLevelOneWithEmptyIndexDefinition.class); + + assertThat(indexDefinitions, hasSize(1)); + assertIndexPathAndCollection(new String[] { "zero" }, "CompoundIndexOnLevelZeroWithEmptyIndexDef", + indexDefinitions.get(0)); + } + + /** + * @see DATAMONGO-929 + */ + @Test + public void singleCompoundIndexPathOnLevelZeroIsResolvedCorrectly() { + + List indexDefinitions = prepareMappingContextAndResolveIndexForType(SingleCompoundIndex.class); + + assertThat(indexDefinitions, hasSize(1)); + assertIndexPathAndCollection(new String[] { "foo", "bar" }, "CompoundIndexOnLevelZero", indexDefinitions.get(0)); + } + + @Document(collection = "CompoundIndexOnLevelOne") + static class CompoundIndexOnLevelOne { + + CompoundIndexOnLevelZero zero; + } + + @Document(collection = "CompoundIndexOnLevelZeroWithEmptyIndexDef") + static class CompoundIndexOnLevelOneWithEmptyIndexDefinition { + + CompoundIndexOnLevelZeroWithEmptyIndexDef zero; } @Document(collection = "CompoundIndexOnLevelZero") @@ -332,6 +381,15 @@ public class MongoPersistentEntityIndexResolverUnitTests { dropDups = true, expireAfterSeconds = 10, sparse = true, unique = true) }) static class CompoundIndexOnLevelZero {} + @CompoundIndexes({ @CompoundIndex(name = "compound_index", background = true, dropDups = true, + expireAfterSeconds = 10, sparse = true, unique = true) }) + static class CompoundIndexOnLevelZeroWithEmptyIndexDef {} + + @Document(collection = "CompoundIndexOnLevelZero") + @CompoundIndex(name = "compound_index", def = "{'foo': 1, 'bar': -1}", background = true, dropDups = true, + expireAfterSeconds = 10, sparse = true, unique = true) + static class SingleCompoundIndex {} + static class IndexDefinedOnSuperClass extends CompoundIndexOnLevelZero { } @@ -426,7 +484,15 @@ public class MongoPersistentEntityIndexResolverUnitTests { private static void assertIndexPathAndCollection(String expectedPath, String expectedCollection, IndexDefinitionHolder holder) { - assertThat(holder.getPath(), equalTo(expectedPath)); + assertIndexPathAndCollection(new String[] { expectedPath }, expectedCollection, holder); + } + + private static void assertIndexPathAndCollection(String[] expectedPaths, String expectedCollection, + IndexDefinitionHolder holder) { + + for (String expectedPath : expectedPaths) { + assertThat(holder.getIndexDefinition().getIndexKeys().containsField(expectedPath), equalTo(true)); + } assertThat(holder.getCollection(), equalTo(expectedCollection)); } }