From a4497bcf8a6315a586046ab7efd18f39e79e154c Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Wed, 15 Oct 2014 10:13:53 +0200 Subject: [PATCH] DATAMONGO-1070 - Fixed a few glitches in DBRef binding for repository query methods. The QueryMapping for derived repository queries pointing to the identifier of the referenced document. We now reduce the query field's key from reference.id to reference so that the generated DBRef is applied correctly and also take care that the id's are potentially converted to ObjectIds. This is mainly achieved by using the AssociationConverter pulled up from UpdateMapper in ObjectMapper.getMappedKey(). MongoQueryCreator now refrains from translating the field keys as that will fail the QueryMapper to correctly detect id properties. Fixed DBRef handling for StringBasedMongoQuery which previously didn't parse the DBRef instance created after JSON parsing for placeholders. --- .../mongodb/core/convert/QueryMapper.java | 54 +++++++++++++++++-- .../mongodb/core/convert/UpdateMapper.java | 41 -------------- .../repository/query/MongoQueryCreator.java | 8 +-- .../query/StringBasedMongoQuery.java | 32 ++++++++--- .../core/convert/QueryMapperUnitTests.java | 16 ++++++ .../query/MongoQueryCreatorUnitTests.java | 19 ++----- .../query/StringBasedMongoQueryUnitTests.java | 23 +++++++- 7 files changed, 118 insertions(+), 75 deletions(-) diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/QueryMapper.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/QueryMapper.java index 536f97c87..0ac69a768 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/QueryMapper.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/QueryMapper.java @@ -334,7 +334,8 @@ public class QueryMapper { } MongoPersistentEntity entity = documentField.getPropertyEntity(); - return entity.hasIdProperty() && entity.getIdProperty().getActualType().isAssignableFrom(type); + return entity.hasIdProperty() + && (type.equals(DBRef.class) || entity.getIdProperty().getActualType().isAssignableFrom(type)); } /** @@ -382,10 +383,16 @@ public class QueryMapper { */ protected Object convertAssociation(Object source, MongoPersistentProperty property) { - if (property == null || source == null || source instanceof DBRef || source instanceof DBObject) { + if (property == null || source == null || source instanceof DBObject) { return source; } + if (source instanceof DBRef) { + + DBRef ref = (DBRef) source; + return new DBRef(ref.getDB(), ref.getRef(), convertId(ref.getId())); + } + if (source instanceof Iterable) { BasicDBList result = new BasicDBList(); for (Object element : (Iterable) source) { @@ -785,7 +792,8 @@ public class QueryMapper { */ @Override public String getMappedKey() { - return path == null ? name : path.toDotPath(getPropertyConverter()); + return path == null ? name : path.toDotPath(isAssociation() ? new AssociationConverter(getAssociation()) + : getPropertyConverter()); } protected PersistentPropertyPath getPath() { @@ -838,4 +846,44 @@ public class QueryMapper { return PropertyToFieldNameConverter.INSTANCE; } } + + /** + * Converter to skip all properties after an association property was rendered. + * + * @author Oliver Gierke + */ + protected static class AssociationConverter implements Converter { + + private final MongoPersistentProperty property; + private boolean associationFound; + + /** + * Creates a new {@link AssociationConverter} for the given {@link Association}. + * + * @param association must not be {@literal null}. + */ + public AssociationConverter(Association association) { + + Assert.notNull(association, "Association must not be null!"); + this.property = association.getInverse(); + } + + /* + * (non-Javadoc) + * @see org.springframework.core.convert.converter.Converter#convert(java.lang.Object) + */ + @Override + public String convert(MongoPersistentProperty source) { + + if (associationFound) { + return null; + } + + if (property.equals(source)) { + associationFound = true; + } + + return source.getFieldName(); + } + } } diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/UpdateMapper.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/UpdateMapper.java index 6e6e0fb51..fcb31852c 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/UpdateMapper.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/UpdateMapper.java @@ -20,7 +20,6 @@ import java.util.Iterator; import java.util.Map.Entry; import org.springframework.core.convert.converter.Converter; -import org.springframework.data.mapping.Association; import org.springframework.data.mapping.context.MappingContext; import org.springframework.data.mongodb.core.mapping.MongoPersistentEntity; import org.springframework.data.mongodb.core.mapping.MongoPersistentProperty; @@ -197,46 +196,6 @@ public class UpdateMapper extends QueryMapper { return isAssociation() ? new AssociationConverter(getAssociation()) : new UpdatePropertyConverter(key); } - /** - * Converter to skip all properties after an association property was rendered. - * - * @author Oliver Gierke - */ - private static class AssociationConverter implements Converter { - - private final MongoPersistentProperty property; - private boolean associationFound; - - /** - * Creates a new {@link AssociationConverter} for the given {@link Association}. - * - * @param association must not be {@literal null}. - */ - public AssociationConverter(Association association) { - - Assert.notNull(association, "Association must not be null!"); - this.property = association.getInverse(); - } - - /* - * (non-Javadoc) - * @see org.springframework.core.convert.converter.Converter#convert(java.lang.Object) - */ - @Override - public String convert(MongoPersistentProperty source) { - - if (associationFound) { - return null; - } - - if (property.equals(source)) { - associationFound = true; - } - - return source.getFieldName(); - } - } - /** * Special {@link Converter} for {@link MongoPersistentProperty} instances that will concatenate the {@literal $} * contained in the source update key. diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/query/MongoQueryCreator.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/query/MongoQueryCreator.java index ead1d642e..16b2af801 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/query/MongoQueryCreator.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/query/MongoQueryCreator.java @@ -102,9 +102,7 @@ class MongoQueryCreator extends AbstractQueryCreator { PersistentPropertyPath path = context.getPersistentPropertyPath(part.getProperty()); MongoPersistentProperty property = path.getLeafProperty(); - Criteria criteria = from(part, property, - where(path.toDotPath(MongoPersistentProperty.PropertyToFieldNameConverter.INSTANCE)), - (PotentiallyConvertingIterator) iterator); + Criteria criteria = from(part, property, where(path.toDotPath()), (PotentiallyConvertingIterator) iterator); return criteria; } @@ -123,9 +121,7 @@ class MongoQueryCreator extends AbstractQueryCreator { PersistentPropertyPath path = context.getPersistentPropertyPath(part.getProperty()); MongoPersistentProperty property = path.getLeafProperty(); - return from(part, property, - base.and(path.toDotPath(MongoPersistentProperty.PropertyToFieldNameConverter.INSTANCE)), - (PotentiallyConvertingIterator) iterator); + return from(part, property, base.and(path.toDotPath()), (PotentiallyConvertingIterator) iterator); } /* diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/query/StringBasedMongoQuery.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/query/StringBasedMongoQuery.java index de17a672a..c5b445e4e 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/query/StringBasedMongoQuery.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/query/StringBasedMongoQuery.java @@ -29,6 +29,7 @@ import org.springframework.data.mongodb.core.query.Query; import org.springframework.util.StringUtils; import com.mongodb.DBObject; +import com.mongodb.DBRef; import com.mongodb.util.JSON; /** @@ -199,6 +200,7 @@ public class StringBasedMongoQuery extends AbstractMongoQuery { * {@link Collections#emptyList()}. * * @param input + * @param conversionService must not be {@literal null}. * @return */ public List parseParameterBindingsFrom(String input) { @@ -229,14 +231,7 @@ public class StringBasedMongoQuery extends AbstractMongoQuery { if (value instanceof String) { String string = ((String) value).trim(); - - Matcher valueMatcher = PARSEABLE_BINDING_PATTERN.matcher(string); - while (valueMatcher.find()) { - int paramIndex = Integer.parseInt(valueMatcher.group(PARAMETER_INDEX_GROUP)); - boolean quoted = (string.startsWith("'") && string.endsWith("'")) - || (string.startsWith("\"") && string.endsWith("\"")); - bindings.add(new ParameterBinding(paramIndex, quoted)); - } + potentiallyAddBinding(string, bindings); } else if (value instanceof Pattern) { @@ -255,6 +250,13 @@ public class StringBasedMongoQuery extends AbstractMongoQuery { bindings.add(new ParameterBinding(paramIndex, quoted)); } + } else if (value instanceof DBRef) { + + DBRef dbref = (DBRef) value; + + potentiallyAddBinding(dbref.getRef(), bindings); + potentiallyAddBinding(dbref.getId().toString(), bindings); + } else if (value instanceof DBObject) { DBObject dbo = (DBObject) value; @@ -264,6 +266,20 @@ public class StringBasedMongoQuery extends AbstractMongoQuery { } } } + + private void potentiallyAddBinding(String source, List bindings) { + + Matcher valueMatcher = PARSEABLE_BINDING_PATTERN.matcher(source); + + while (valueMatcher.find()) { + + int paramIndex = Integer.parseInt(valueMatcher.group(PARAMETER_INDEX_GROUP)); + boolean quoted = (source.startsWith("'") && source.endsWith("'")) + || (source.startsWith("\"") && source.endsWith("\"")); + + bindings.add(new ParameterBinding(paramIndex, quoted)); + } + } } /** diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/QueryMapperUnitTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/QueryMapperUnitTests.java index e8431fdff..1fa54024e 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/QueryMapperUnitTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/QueryMapperUnitTests.java @@ -658,6 +658,22 @@ public class QueryMapperUnitTests { assertThat(dbo, equalTo(new BasicDBObjectBuilder().add("_id", 1).get())); } + /** + * @see DATAMONGO-1070 + */ + @Test + public void mapsIdReferenceToDBRefCorrectly() { + + ObjectId id = new ObjectId(); + + DBObject query = new BasicDBObject("reference.id", new com.mongodb.DBRef(null, "reference", id.toString())); + DBObject result = mapper.getMappedObject(query, context.getPersistentEntity(WithDBRef.class)); + + assertThat(result.containsField("reference"), is(true)); + com.mongodb.DBRef reference = getTypedValue(result, "reference", com.mongodb.DBRef.class); + assertThat(reference.getId(), is(instanceOf(ObjectId.class))); + } + @Document public class Foo { @Id private ObjectId id; diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/query/MongoQueryCreatorUnitTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/query/MongoQueryCreatorUnitTests.java index 2e1767eac..a6e90fd05 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/query/MongoQueryCreatorUnitTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/query/MongoQueryCreatorUnitTests.java @@ -167,19 +167,6 @@ public class MongoQueryCreatorUnitTests { assertThat(creator.createQuery(), is(reference)); } - /** - * @see DATAMONGO-291 - */ - @Test - public void honoursMappingInformationForPropertyPaths() { - - PartTree partTree = new PartTree("findByUsername", User.class); - - MongoQueryCreator creator = new MongoQueryCreator(partTree, getAccessor(converter, "Oliver"), context); - Query reference = query(where("foo").is("Oliver")); - assertThat(creator.createQuery(), is(reference)); - } - /** * @see DATAMONGO-338 */ @@ -268,7 +255,7 @@ public class MongoQueryCreatorUnitTests { MongoQueryCreator creator = new MongoQueryCreator(tree, getAccessor(converter, "Matt"), context); Query query = creator.createQuery(); - assertThat(query, is(query(where("foo").regex("^Matt")))); + assertThat(query, is(query(where("username").regex("^Matt")))); } /** @@ -281,7 +268,7 @@ public class MongoQueryCreatorUnitTests { MongoQueryCreator creator = new MongoQueryCreator(tree, getAccessor(converter, "ews"), context); Query query = creator.createQuery(); - assertThat(query, is(query(where("foo").regex("ews$")))); + assertThat(query, is(query(where("username").regex("ews$")))); } /** @@ -294,7 +281,7 @@ public class MongoQueryCreatorUnitTests { MongoQueryCreator creator = new MongoQueryCreator(tree, getAccessor(converter, "thew"), context); Query query = creator.createQuery(); - assertThat(query, is(query(where("foo").regex(".*thew.*")))); + assertThat(query, is(query(where("username").regex(".*thew.*")))); } private void assertBindsDistanceToQuery(Point point, Distance distance, Query reference) throws Exception { diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/query/StringBasedMongoQueryUnitTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/query/StringBasedMongoQueryUnitTests.java index 8c48a4c22..8eb1e814c 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/query/StringBasedMongoQueryUnitTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/query/StringBasedMongoQueryUnitTests.java @@ -29,6 +29,7 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.Mock; import org.mockito.runners.MockitoJUnitRunner; +import org.springframework.data.mongodb.core.DBObjectTestUtils; import org.springframework.data.mongodb.core.MongoOperations; import org.springframework.data.mongodb.core.convert.DbRefResolver; import org.springframework.data.mongodb.core.convert.DefaultMongoTypeMapper; @@ -43,6 +44,7 @@ import org.springframework.data.repository.core.RepositoryMetadata; import com.mongodb.BasicDBObject; import com.mongodb.DBObject; +import com.mongodb.DBRef; /** * Unit tests for {@link StringBasedMongoQuery}. @@ -255,6 +257,22 @@ public class StringBasedMongoQueryUnitTests { assertThat(query.getQueryObject(), is(reference.getQueryObject())); } + /** + * @see DATAMONGO-1070 + */ + @Test + public void parsesDbRefDeclarationsCorrectly() throws Exception { + + StringBasedMongoQuery mongoQuery = createQueryForMethod("methodWithManuallyDefinedDbRef", String.class); + ConvertingParameterAccessor parameterAccessor = StubParameterAccessor.getAccessor(converter, "myid"); + + org.springframework.data.mongodb.core.query.Query query = mongoQuery.createQuery(parameterAccessor); + + DBRef dbRef = DBObjectTestUtils.getTypedValue(query.getQueryObject(), "reference", DBRef.class); + assertThat(dbRef.getId(), is((Object) "myid")); + assertThat(dbRef.getRef(), is("reference")); + } + private StringBasedMongoQuery createQueryForMethod(String name, Class... parameters) throws Exception { Method method = SampleRepository.class.getMethod(name, parameters); @@ -291,7 +309,10 @@ public class StringBasedMongoQueryUnitTests { @Query("{'title': { $regex : '^?0', $options : 'i'}}") List findByTitleBeginsWithExplicitQuoting(String title); - @Query(value = "{$where: 'return this.date.getUTCMonth() == ?2 && this.date.getUTCDay() == ?3;'}") + @Query("{$where: 'return this.date.getUTCMonth() == ?2 && this.date.getUTCDay() == ?3;'}") List findByQueryWithParametersInExpression(int param1, int param2, int param3, int param4); + + @Query("{ 'reference' : { $ref : 'reference', $id : ?0 }}") + Object methodWithManuallyDefinedDbRef(String id); } }