From b1de52f05aaf7b766c97e1822eab40b583ec2f75 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Thu, 15 Dec 2016 10:37:16 +0100 Subject: [PATCH] DATAMONGO-1565 - Ignore placeholder pattern in replacement values for annotated queries. We now make sure to quote single and double ticks in the replacement values before actually appending them to the query. We also replace single ticks around parameters in the actual raw annotated query by double quotes to make sure they are treated as a single string parameter. --- .../mongodb/core/convert/QueryMapper.java | 5 + .../data/mongodb/core/query/BasicQuery.java | 5 +- .../ExpressionEvaluatingParameterBinder.java | 152 ++++++++++++++---- .../mongodb/repository/PersonRepository.java | 4 +- ...eactiveStringBasedMongoQueryUnitTests.java | 2 +- .../query/StringBasedMongoQueryUnitTests.java | 105 +++++++++++- 6 files changed, 229 insertions(+), 44 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 ce31d88dc..0e1f284ee 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 @@ -23,6 +23,7 @@ import java.util.List; import java.util.Map.Entry; import java.util.Set; +import org.bson.BsonValue; import org.bson.Document; import org.bson.conversions.Bson; import org.bson.types.ObjectId; @@ -408,6 +409,10 @@ public class QueryMapper { return getMappedObject((BasicDBObject) source, entity); } + if(source instanceof BsonValue) { + return source; + } + return delegateConvertToMongoType(source, entity); } diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/query/BasicQuery.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/query/BasicQuery.java index b6a30676d..539956713 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/query/BasicQuery.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/query/BasicQuery.java @@ -63,8 +63,9 @@ public class BasicQuery extends Query { * @param fields may be {@literal null}. */ public BasicQuery(String query, String fields) { - this.queryObject = query != null ? new Document(((DBObject) JSON.parse(query)).toMap()) : null; - this.fieldsObject = fields != null ? new Document(((DBObject) JSON.parse(fields)).toMap()) : null; + + this.queryObject = query != null ? Document.parse(query) : null; + this.fieldsObject = fields != null ? Document.parse(fields) : null; } /** diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/query/ExpressionEvaluatingParameterBinder.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/query/ExpressionEvaluatingParameterBinder.java index 10c34bb39..fcab6fd76 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/query/ExpressionEvaluatingParameterBinder.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/query/ExpressionEvaluatingParameterBinder.java @@ -15,8 +15,13 @@ */ package org.springframework.data.mongodb.repository.query; -import java.util.Collections; +import java.util.ArrayList; +import java.util.LinkedHashMap; import java.util.List; +import java.util.Map; +import java.util.NoSuchElementException; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import javax.xml.bind.DatatypeConverter; @@ -94,47 +99,67 @@ class ExpressionEvaluatingParameterBinder { return input; } - boolean isCompletlyParameterizedQuery = input.matches("^\\?\\d+$"); - StringBuilder result = new StringBuilder(input); - - for (ParameterBinding binding : bindingContext.getBindings()) { + if (input.matches("^\\?\\d+$")) { + return getParameterValueForBinding(accessor, bindingContext.getParameters(), + bindingContext.getBindings().iterator().next()); + } - String parameter = binding.getParameter(); - int idx = result.indexOf(parameter); + Matcher matcher = createReplacementPattern(bindingContext.getBindings()).matcher(input); + StringBuffer buffer = new StringBuffer(); - if (idx == -1) { - continue; - } + while (matcher.find()) { + ParameterBinding binding = bindingContext.getBindingFor(extractPlaceholder(matcher.group())); String valueForBinding = getParameterValueForBinding(accessor, bindingContext.getParameters(), binding); - int start = idx; - int end = idx + parameter.length(); + // appendReplacement does not like unescaped $ sign and others, so we need to quote that stuff first + matcher.appendReplacement(buffer, Matcher.quoteReplacement(valueForBinding)); + + if (binding.isQuoted()) { + postProcessQuotedBinding(buffer, valueForBinding); + } + } + + matcher.appendTail(buffer); + return buffer.toString(); + } - // If the value to bind is an object literal we need to remove the quoting around the expression insertion point. - if (valueForBinding.startsWith("{") && !isCompletlyParameterizedQuery) { + /** + * Sanitize String binding by replacing single quoted values with double quotes which prevents potential single quotes + * contained in replacement to interfere with the Json parsing. Also take care of complex objects by removing the + * quotation entirely. + * + * @param buffer the {@link StringBuffer} to operate upon. + * @param valueForBinding the actual binding value. + */ + private void postProcessQuotedBinding(StringBuffer buffer, String valueForBinding) { - // Is the insertion point actually surrounded by quotes? - char beforeStart = result.charAt(start - 1); - char afterEnd = result.charAt(end); + int quotationMarkIndex = buffer.length() - valueForBinding.length() - 1; + char quotationMark = buffer.charAt(quotationMarkIndex); - if ((beforeStart == '\'' || beforeStart == '"') && (afterEnd == '\'' || afterEnd == '"')) { + while (quotationMark != '\'' && quotationMark != '"') { - // Skip preceding and following quote - start -= 1; - end += 1; - } + quotationMarkIndex--; + if (quotationMarkIndex < 0) { + throw new IllegalArgumentException("Could not find opening quotes for quoted parameter"); } - - result.replace(start, end, valueForBinding); + quotationMark = buffer.charAt(quotationMarkIndex); } - return result.toString(); + if (valueForBinding.startsWith("{")) { // remove quotation char before the complex object string + buffer.deleteCharAt(quotationMarkIndex); + } else { + + if (quotationMark == '\'') { + buffer.replace(quotationMarkIndex, quotationMarkIndex + 1, "\""); + } + buffer.append("\""); + } } /** * Returns the serialized value to be used for the given {@link ParameterBinding}. - * + * * @param accessor must not be {@literal null}. * @param parameters * @param binding must not be {@literal null}. @@ -148,7 +173,7 @@ class ExpressionEvaluatingParameterBinder { : accessor.getBindableValue(binding.getParameterIndex()); if (value instanceof String && binding.isQuoted()) { - return (String) value; + return ((String) value).startsWith("{") ? (String) value : ((String) value).replace("\"", "\\\""); } if (value instanceof byte[]) { @@ -156,7 +181,7 @@ class ExpressionEvaluatingParameterBinder { String base64representation = DatatypeConverter.printBase64Binary((byte[]) value); if (!binding.isQuoted()) { - return "{ '$binary' : '" + base64representation + "', '$type' : " + BSON.B_GENERAL + "}"; + return "{ '$binary' : '" + base64representation + "', '$type' : '" + BSON.B_GENERAL + "'}"; } return base64representation; @@ -167,7 +192,7 @@ class ExpressionEvaluatingParameterBinder { /** * Evaluates the given {@code expressionString}. - * + * * @param expressionString must not be {@literal null} or empty. * @param parameters must not be {@literal null}. * @param parameterValues must not be {@literal null}. @@ -181,6 +206,40 @@ class ExpressionEvaluatingParameterBinder { return expression.getValue(evaluationContext, Object.class); } + /** + * Creates a replacement {@link Pattern} for all {@link ParameterBinding#getParameter() binding parameters} including + * a potentially trailing quotation mark. + * + * @param bindings + * @return + */ + private Pattern createReplacementPattern(List bindings) { + + StringBuilder regex = new StringBuilder(); + for (ParameterBinding binding : bindings) { + regex.append("|"); + regex.append(Pattern.quote(binding.getParameter())); + regex.append("['\"]?"); // potential quotation char (as in { foo : '?0' }). + } + + return Pattern.compile(regex.substring(1)); + } + + /** + * Extract the placeholder stripping any trailing trailing quotation mark that might have resulted from the + * {@link #createReplacementPattern(List) pattern} used. + * + * @param groupName The actual {@link Matcher#group() group}. + * @return + */ + private String extractPlaceholder(String groupName) { + + if (!groupName.endsWith("'") && !groupName.endsWith("\"")) { + return groupName; + } + return groupName.substring(0, groupName.length() - 1); + } + /** * @author Christoph Strobl * @since 1.9 @@ -188,18 +247,18 @@ class ExpressionEvaluatingParameterBinder { static class BindingContext { final MongoParameters parameters; - final List bindings; + final Map bindings; /** * Creates new {@link BindingContext}. - * + * * @param parameters * @param bindings */ public BindingContext(MongoParameters parameters, List bindings) { this.parameters = parameters; - this.bindings = bindings; + this.bindings = mapBindings(bindings); } /** @@ -211,11 +270,28 @@ class ExpressionEvaluatingParameterBinder { /** * Get unmodifiable list of {@link ParameterBinding}s. - * + * * @return never {@literal null}. */ public List getBindings() { - return Collections.unmodifiableList(bindings); + return new ArrayList(bindings.values()); + } + + /** + * Get the concrete {@link ParameterBinding} for a given {@literal placeholder}. + * + * @param placeholder must not be {@literal null}. + * @return + * @throws java.util.NoSuchElementException + * @since 1.10 + */ + ParameterBinding getBindingFor(String placeholder) { + + if (!bindings.containsKey(placeholder)) { + throw new NoSuchElementException(String.format("Could not to find binding for placeholder '%s'.", placeholder)); + } + + return bindings.get(placeholder); } /** @@ -227,5 +303,13 @@ class ExpressionEvaluatingParameterBinder { return parameters; } + private static Map mapBindings(List bindings) { + + Map map = new LinkedHashMap(bindings.size(), 1); + for (ParameterBinding binding : bindings) { + map.put(binding.getParameter(), binding); + } + return map; + } } } diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/PersonRepository.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/PersonRepository.java index 849a673f1..585e72ad7 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/PersonRepository.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/PersonRepository.java @@ -119,7 +119,7 @@ public interface PersonRepository extends MongoRepository, Query */ Page findByLastnameLike(String lastname, Pageable pageable); - @Query("{ 'lastname' : { '$regex' : ?0, '$options' : ''}}") + @Query("{ 'lastname' : { '$regex' : '?0', '$options' : 'i'}}") Page findByLastnameLikeWithPageable(String lastname, Pageable pageable); /** @@ -335,7 +335,7 @@ public interface PersonRepository extends MongoRepository, Query /** * @see DATAMONGO-745 */ - @Query("{lastname:?0, address.street:{$in:?1}}") + @Query("{lastname:?0, 'address.street':{$in:?1}}") Page findByCustomQueryLastnameAndAddressStreetInList(String lastname, List streetNames, Pageable page); diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/query/ReactiveStringBasedMongoQueryUnitTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/query/ReactiveStringBasedMongoQueryUnitTests.java index d7b184f7b..a277df818 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/query/ReactiveStringBasedMongoQueryUnitTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/query/ReactiveStringBasedMongoQueryUnitTests.java @@ -240,7 +240,7 @@ public class ReactiveStringBasedMongoQueryUnitTests { org.springframework.data.mongodb.core.query.Query query = mongoQuery.createQuery(accesor); org.springframework.data.mongodb.core.query.Query reference = new BasicQuery("{'lastname' : { '$binary' : '" - + DatatypeConverter.printBase64Binary(binaryData) + "', '$type' : " + BSON.B_GENERAL + "}}"); + + DatatypeConverter.printBase64Binary(binaryData) + "', '$type' : '" + BSON.B_GENERAL + "'}}"); assertThat(query.getQueryObject().toJson(), is(reference.getQueryObject().toJson())); } 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 24cbd2ce5..441d10822 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 @@ -28,6 +28,7 @@ import javax.xml.bind.DatatypeConverter; import org.bson.BSON; import org.bson.Document; +import org.bson.conversions.Bson; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -44,6 +45,7 @@ import org.springframework.data.mongodb.core.query.BasicQuery; import org.springframework.data.mongodb.repository.Address; import org.springframework.data.mongodb.repository.Person; import org.springframework.data.mongodb.repository.Query; +import org.springframework.data.mongodb.test.util.IsBsonObject; import org.springframework.data.projection.ProjectionFactory; import org.springframework.data.projection.SpelAwareProxyProjectionFactory; import org.springframework.data.repository.Repository; @@ -52,6 +54,7 @@ import org.springframework.data.repository.query.DefaultEvaluationContextProvide import org.springframework.expression.spel.standard.SpelExpressionParser; import com.mongodb.DBRef; +import org.bson.Document; /** * Unit tests for {@link StringBasedMongoQuery}. @@ -184,7 +187,6 @@ public class StringBasedMongoQueryUnitTests { StringBasedMongoQuery mongoQuery = createQueryForMethod("findByParameterizedCriteriaAndFields", Document.class, Map.class); - org.springframework.data.mongodb.core.query.Query query = mongoQuery.createQuery(accessor); assertThat(query.getQueryObject(), @@ -280,9 +282,8 @@ public class StringBasedMongoQueryUnitTests { org.springframework.data.mongodb.core.query.Query query = mongoQuery.createQuery(parameterAccessor); - DBRef dbRef = DocumentTestUtils.getTypedValue(query.getQueryObject(), "reference", DBRef.class); - assertThat(dbRef.getId(), is((Object) "myid")); - assertThat(dbRef.getCollectionName(), is("reference")); + Document dbRef = DocumentTestUtils.getTypedValue(query.getQueryObject(), "reference", Document.class); + assertThat(dbRef, is(new Document("$ref", "reference").append("$id", "myid"))); } /** @@ -360,7 +361,7 @@ public class StringBasedMongoQueryUnitTests { org.springframework.data.mongodb.core.query.Query query = mongoQuery.createQuery(accesor); org.springframework.data.mongodb.core.query.Query reference = new BasicQuery("{'lastname' : { '$binary' : '" - + DatatypeConverter.printBase64Binary(binaryData) + "', '$type' : " + BSON.B_GENERAL + "}}"); + + DatatypeConverter.printBase64Binary(binaryData) + "', '$type' : '" + BSON.B_GENERAL + "'}}"); assertThat(query.getQueryObject().toJson(), is(reference.getQueryObject().toJson())); } @@ -376,6 +377,97 @@ public class StringBasedMongoQueryUnitTests { assertThat(mongoQuery.isExistsQuery(), is(true)); } + /** + * @see DATAMONGO-1565 + */ + @Test + public void shouldIgnorePlaceholderPatternInReplacementValue() throws Exception { + + ConvertingParameterAccessor accesor = StubParameterAccessor.getAccessor(converter, "argWith?1andText", + "nothing-special"); + + StringBasedMongoQuery mongoQuery = createQueryForMethod("findByStringWithWildcardChar", String.class, String.class); + + org.springframework.data.mongodb.core.query.Query query = mongoQuery.createQuery(accesor); + assertThat(query.getQueryObject(), + is(Document.parse("{ \"arg0\" : \"argWith?1andText\" , \"arg1\" : \"nothing-special\"}"))); + } + + /** + * @see DATAMONGO-1565 + */ + @Test + public void shouldQuoteStringReplacementCorrectly() throws Exception { + + StringBasedMongoQuery mongoQuery = createQueryForMethod("findByLastnameQuoted", String.class); + ConvertingParameterAccessor accesor = StubParameterAccessor.getAccessor(converter, "Matthews', password: 'foo"); + + org.springframework.data.mongodb.core.query.Query query = mongoQuery.createQuery(accesor); + assertThat(query.getQueryObject(), + is(not(new Document().append("lastname", "Matthews").append("password", "foo")))); + assertThat(query.getQueryObject(), is(new Document("lastname", "Matthews', password: 'foo"))); + } + + /** + * @see DATAMONGO-1565 + */ + @Test + public void shouldQuoteStringReplacementContainingQuotesCorrectly() throws Exception { + + StringBasedMongoQuery mongoQuery = createQueryForMethod("findByLastnameQuoted", String.class); + ConvertingParameterAccessor accesor = StubParameterAccessor.getAccessor(converter, "Matthews\", password: \"foo"); + + org.springframework.data.mongodb.core.query.Query query = mongoQuery.createQuery(accesor); + assertThat(query.getQueryObject(), + is(not(new Document().append("lastname", "Matthews").append("password", "foo")))); + assertThat(query.getQueryObject(), is( new Document("lastname", "Matthews\", password: \"foo"))); + } + + /** + * @see DATAMONGO-1565 + */ + @Test + public void shouldQuoteStringReplacementWithQuotationsCorrectly() throws Exception { + + StringBasedMongoQuery mongoQuery = createQueryForMethod("findByLastnameQuoted", String.class); + ConvertingParameterAccessor accesor = StubParameterAccessor.getAccessor(converter, + "\"Dave Matthews\", password: 'foo"); + + org.springframework.data.mongodb.core.query.Query query = mongoQuery.createQuery(accesor); + assertThat(query.getQueryObject(), + is(new Document("lastname", "\"Dave Matthews\", password: 'foo"))); + } + + /** + * @see DATAMONGO-1565 + */ + @Test + public void shouldQuoteComplexQueryStringCorreclty() throws Exception { + + StringBasedMongoQuery mongoQuery = createQueryForMethod("findByLastnameQuoted", String.class); + ConvertingParameterAccessor accesor = StubParameterAccessor.getAccessor(converter, "{ $ne : \"calamity\" }"); + + org.springframework.data.mongodb.core.query.Query query = mongoQuery.createQuery(accesor); + assertThat(query.getQueryObject(), + is( new Document("lastname", new Document("$ne", "calamity")))); + } + + /** + * @see DATAMONGO-1565 + */ + @Test + public void shouldQuotationInQuotedComplexQueryString() throws Exception { + + StringBasedMongoQuery mongoQuery = createQueryForMethod("findByLastnameQuoted", String.class); + ConvertingParameterAccessor accesor = StubParameterAccessor.getAccessor(converter, + "{ $ne : \"\\\"calamity\\\"\" }"); + + org.springframework.data.mongodb.core.query.Query query = mongoQuery.createQuery(accesor); + + assertThat(query.getQueryObject(), + is(new Document("lastname", new Document("$ne", "\"calamity\"")))); + } + private StringBasedMongoQuery createQueryForMethod(String name, Class... parameters) throws Exception { Method method = SampleRepository.class.getMethod(name, parameters); @@ -437,5 +529,8 @@ public class StringBasedMongoQueryUnitTests { @Query(value = "{ 'lastname' : ?0 }", exists = true) boolean existsByLastname(String lastname); + + @Query("{ 'arg0' : ?0, 'arg1' : ?1 }") + List findByStringWithWildcardChar(String arg0, String arg1); } }