From b51cf05f90ae4cee64c43dd5319327f8d451c4f7 Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Sat, 12 Oct 2013 17:39:57 +0200 Subject: [PATCH] DATAMONGO-752 - Improved keyword detection in QueryMapper. The check for keywords in QueryMapper now selectively decides between checks for a nested keyword (DBObject) object and the check for a simple key. This allows the usage of criteria values starting with $ (e.g. { 'myvalue' : '$334' }) without the value being considered a keyword and thus erroneously triggering a potential conversion of the value. Moved more logic for a keyword into the Keyword value object. --- .../mongodb/core/convert/QueryMapper.java | 96 ++++++++++++------- .../core/convert/QueryMapperUnitTests.java | 28 ++++++ 2 files changed, 92 insertions(+), 32 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 a803bc628..2ba9b8862 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 @@ -48,7 +48,6 @@ import com.mongodb.DBRef; public class QueryMapper { private static final List DEFAULT_ID_NAMES = Arrays.asList("id", "_id"); - private static final String N_OR_PATTERN = "\\$.*or"; private final ConversionService conversionService; private final MongoConverter converter; @@ -79,7 +78,7 @@ public class QueryMapper { @SuppressWarnings("deprecation") public DBObject getMappedObject(DBObject query, MongoPersistentEntity entity) { - if (Keyword.isKeyword(query)) { + if (isNestedKeyword(query)) { return getMappedKeyword(new Keyword(query), entity); } @@ -97,7 +96,7 @@ public class QueryMapper { continue; } - if (Keyword.isKeyword(key)) { + if (isKeyword(key)) { result.putAll(getMappedKeyword(new Keyword(query, key), entity)); continue; } @@ -107,7 +106,7 @@ public class QueryMapper { Object rawValue = query.get(key); String newKey = field.getMappedKey(); - if (Keyword.isKeyword(rawValue) && !field.isIdField()) { + if (isNestedKeyword(rawValue) && !field.isIdField()) { Keyword keyword = new Keyword((DBObject) rawValue); result.put(newKey, getMappedKeyword(field, keyword)); } else { @@ -121,16 +120,16 @@ public class QueryMapper { /** * Returns the given {@link DBObject} representing a keyword by mapping the keyword's value. * - * @param query the {@link DBObject} representing a keyword (e.g. {@code $ne : … } ) + * @param keyword the {@link DBObject} representing a keyword (e.g. {@code $ne : … } ) * @param entity * @return */ - private DBObject getMappedKeyword(Keyword query, MongoPersistentEntity entity) { + private DBObject getMappedKeyword(Keyword keyword, MongoPersistentEntity entity) { // $or/$nor - if (query.key.matches(N_OR_PATTERN) || query.value instanceof Iterable) { + if (keyword.isOrOrNor() || keyword.hasIterableValue()) { - Iterable conditions = (Iterable) query.value; + Iterable conditions = keyword.getValue(); BasicDBList newConditions = new BasicDBList(); for (Object condition : conditions) { @@ -138,10 +137,10 @@ public class QueryMapper { : convertSimpleOrDBObject(condition, entity)); } - return new BasicDBObject(query.key, newConditions); + return new BasicDBObject(keyword.getKey(), newConditions); } - return new BasicDBObject(query.key, convertSimpleOrDBObject(query.value, entity)); + return new BasicDBObject(keyword.getKey(), convertSimpleOrDBObject(keyword.getValue(), entity)); } /** @@ -154,10 +153,12 @@ public class QueryMapper { private DBObject getMappedKeyword(Field property, Keyword keyword) { boolean needsAssociationConversion = property.isAssociation() && !keyword.isExists(); - Object value = needsAssociationConversion ? convertAssociation(keyword.value, property.getProperty()) - : getMappedValue(property.with(keyword.key), keyword.value); + Object value = keyword.getValue(); - return new BasicDBObject(keyword.key, value); + Object convertedValue = needsAssociationConversion ? convertAssociation(value, property.getProperty()) + : getMappedValue(property.with(keyword.getKey()), value); + + return new BasicDBObject(keyword.key, convertedValue); } /** @@ -195,7 +196,7 @@ public class QueryMapper { } } - if (Keyword.isKeyword(value)) { + if (isNestedKeyword(value)) { return getMappedKeyword(new Keyword((DBObject) value), null); } @@ -289,6 +290,39 @@ public class QueryMapper { return delegateConvertToMongoType(id, null); } + /** + * Returns whether the given {@link Object} is a keyword, i.e. if it's a {@link DBObject} with a keyword key. + * + * @param candidate + * @return + */ + protected boolean isNestedKeyword(Object candidate) { + + if (!(candidate instanceof BasicDBObject)) { + return false; + } + + BasicDBObject dbObject = (BasicDBObject) candidate; + Set keys = dbObject.keySet(); + + if (keys.size() != 1) { + return false; + } + + return isKeyword(keys.iterator().next().toString()); + } + + /** + * Returns whether the given {@link String} is a MongoDB keyword. The default implementation will check against the + * set of registered keywords returned by {@link #getKeywords()}. + * + * @param candidate + * @return + */ + protected boolean isKeyword(String candidate) { + return candidate.startsWith("$"); + } + /** * Value object to capture a query keyword representation. * @@ -296,8 +330,10 @@ public class QueryMapper { */ private static class Keyword { - String key; - Object value; + private static final String N_OR_PATTERN = "\\$.*or"; + + private final String key; + private final Object value; public Keyword(DBObject source, String key) { this.key = key; @@ -322,25 +358,21 @@ public class QueryMapper { return "$exists".equalsIgnoreCase(key); } - /** - * Returns whether the given value actually represents a keyword. If this returns {@literal true} it's safe to call - * the constructor. - * - * @param value - * @return - */ - public static boolean isKeyword(Object value) { + public boolean isOrOrNor() { + return key.matches(N_OR_PATTERN); + } - if (value instanceof String) { - return ((String) value).startsWith("$"); - } + public boolean hasIterableValue() { + return value instanceof Iterable; + } - if (!(value instanceof DBObject)) { - return false; - } + public String getKey() { + return key; + } - DBObject dbObject = (DBObject) value; - return dbObject.keySet().size() == 1 && dbObject.keySet().iterator().next().startsWith("$"); + @SuppressWarnings("unchecked") + public T getValue() { + return (T) value; } } 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 fd6c82451..372bd3975 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 @@ -438,6 +438,34 @@ public class QueryMapperUnitTests { assertThat(inClause.get(0), is(instanceOf(com.mongodb.DBRef.class))); } + /** + * @see DATAMONGO-752 + */ + @Test + public void mapsSimpleValuesStartingWith$Correctly() { + + Query query = query(where("myvalue").is("$334")); + + DBObject result = mapper.getMappedObject(query.getQueryObject(), null); + + assertThat(result.keySet(), hasSize(1)); + assertThat(result.get("myvalue"), is((Object) "$334")); + } + + /** + * @see DATAMONGO-752 + */ + @Test + public void mapsKeywordAsSimpleValuesCorrectly() { + + Query query = query(where("myvalue").is("$center")); + + DBObject result = mapper.getMappedObject(query.getQueryObject(), null); + + assertThat(result.keySet(), hasSize(1)); + assertThat(result.get("myvalue"), is((Object) "$center")); + } + class IdWrapper { Object id; }