From 8068e36679a358c992d76435a83f1c5a9763cdc7 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Wed, 8 Feb 2017 15:11:13 +0100 Subject: [PATCH] DATAMONGO-1603 - Fix Placeholder not replaced correctly in @Query. Fix issues when placeholders are appended with other chars eg. '?0xyz' or have been reused multiple times within the query. Additional tests and fixes for complex quoted replacements eg. in regex query. Rely on placeholder quotation indication instead of binding one. Might be misleading when placeholder is used more than once. Original pull request: #441. --- .../ExpressionEvaluatingParameterBinder.java | 29 ++++-- .../query/StringBasedMongoQuery.java | 6 +- .../query/StringBasedMongoQueryUnitTests.java | 95 ++++++++++++++++++- 3 files changed, 116 insertions(+), 14 deletions(-) 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 8e5f91252..b1e9328e5 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 @@ -45,7 +45,7 @@ import com.mongodb.util.JSON; /** * {@link ExpressionEvaluatingParameterBinder} allows to evaluate, convert and bind parameters to placeholders within a * {@link String}. - * + * * @author Christoph Strobl * @author Thomas Darimont * @author Oliver Gierke @@ -59,7 +59,7 @@ class ExpressionEvaluatingParameterBinder { /** * Creates new {@link ExpressionEvaluatingParameterBinder} - * + * * @param expressionParser must not be {@literal null}. * @param evaluationContextProvider must not be {@literal null}. */ @@ -76,7 +76,7 @@ class ExpressionEvaluatingParameterBinder { /** * Bind values provided by {@link MongoParameterAccessor} to placeholders in {@literal raw} while considering * potential conversions and parameter types. - * + * * @param raw can be {@literal null} or empty. * @param accessor must not be {@literal null}. * @param bindingContext must not be {@literal null}. @@ -93,7 +93,7 @@ class ExpressionEvaluatingParameterBinder { /** * Replaced the parameter placeholders with the actual parameter values from the given {@link ParameterBinding}s. - * + * * @param input must not be {@literal null} or empty. * @param accessor must not be {@literal null}. * @param bindingContext must not be {@literal null}. @@ -126,7 +126,7 @@ class ExpressionEvaluatingParameterBinder { buffer.append(placeholder.getSuffix()); } - if (binding.isQuoted() || placeholder.isQuoted()) { + if (placeholder.isQuoted()) { postProcessQuotedBinding(buffer, valueForBinding, !binding.isExpression() ? accessor.getBindableValue(binding.getParameterIndex()) : null, binding.isExpression()); @@ -247,7 +247,8 @@ class ExpressionEvaluatingParameterBinder { regex.append("|"); regex.append("(" + Pattern.quote(binding.getParameter()) + ")"); - regex.append("(\\W?['\"])?"); // potential quotation char (as in { foo : '?0' }). + regex.append("([\\w.]*"); + regex.append("(\\W?['\"]|\\w*')?)"); } return Pattern.compile(regex.substring(1)); @@ -265,15 +266,22 @@ class ExpressionEvaluatingParameterBinder { if (matcher.groupCount() > 1) { - String rawPlaceholder = matcher.group(parameterIndex * 2 + 1); - String suffix = matcher.group(parameterIndex * 2 + 2); + String rawPlaceholder = matcher.group(parameterIndex * 3 + 1); + String suffix = matcher.group(parameterIndex * 3 + 2); if (!StringUtils.hasText(rawPlaceholder)) { rawPlaceholder = matcher.group(); - suffix = "" + rawPlaceholder.charAt(rawPlaceholder.length() - 1); + if(rawPlaceholder.matches(".*\\d$")) { + suffix = ""; + } else { + int index = rawPlaceholder.replaceAll("[^\\?0-9]*$", "").length() - 1; + if (index > 0 && rawPlaceholder.length() > index) { + suffix = rawPlaceholder.substring(index+1); + } + } if (QuotedString.endsWithQuote(rawPlaceholder)) { - rawPlaceholder = QuotedString.unquoteSuffix(rawPlaceholder); + rawPlaceholder = rawPlaceholder.substring(0, rawPlaceholder.length() - (StringUtils.hasText(suffix) ? suffix.length() : 1)); } } @@ -284,6 +292,7 @@ class ExpressionEvaluatingParameterBinder { return Placeholder.of(parameterIndex, rawPlaceholder, quoted, quoted ? QuotedString.unquoteSuffix(suffix) : suffix); } + return Placeholder.of(parameterIndex, rawPlaceholder, false, null); } return Placeholder.of(parameterIndex, matcher.group(), false, null); 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 bb148128c..8f9a3728a 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 @@ -168,7 +168,8 @@ public class StringBasedMongoQuery extends AbstractMongoQuery { return this.isDeleteQuery; } - private static boolean hasAmbiguousProjectionFlags(boolean isCountQuery, boolean isExistsQuery, boolean isDeleteQuery) { + private static boolean hasAmbiguousProjectionFlags(boolean isCountQuery, boolean isExistsQuery, + boolean isDeleteQuery) { return countBooleanValues(isCountQuery, isExistsQuery, isDeleteQuery) > 1; } @@ -347,8 +348,7 @@ public class StringBasedMongoQuery extends AbstractMongoQuery { while (valueMatcher.find()) { int paramIndex = Integer.parseInt(valueMatcher.group(PARAMETER_INDEX_GROUP)); - boolean quoted = (source.startsWith("'") && source.endsWith("'")) - || (source.startsWith("\"") && source.endsWith("\"")); + boolean quoted = source.startsWith("'") || source.startsWith("\""); bindings.add(new ParameterBinding(paramIndex, quoted)); } 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 a66dcfeea..dee1a399e 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,9 @@ import java.util.regex.Pattern; import javax.xml.bind.DatatypeConverter; +import com.mongodb.BasicDBObject; +import com.mongodb.DBObject; +import com.mongodb.util.JSON; import org.bson.BSON; import org.bson.BsonRegularExpression; import org.bson.Document; @@ -56,7 +59,7 @@ import org.springframework.expression.spel.standard.SpelExpressionParser; /** * Unit tests for {@link StringBasedMongoQuery}. - * + * * @author Oliver Gierke * @author Christoph Strobl * @author Thomas Darimont @@ -435,6 +438,78 @@ public class StringBasedMongoQueryUnitTests { assertThat(query.getQueryObject(), is(new Document("lastname", new BsonRegularExpression("^(calamity)")))); } + @Test // DATAMONGO-1603 + public void shouldAllowReuseOfPlaceholderWithinQuery() throws Exception { + + StringBasedMongoQuery mongoQuery = createQueryForMethod("findByReusingPlaceholdersMultipleTimes", String.class, + String.class); + ConvertingParameterAccessor accessor = StubParameterAccessor.getAccessor(converter, "calamity", "regalia"); + + org.springframework.data.mongodb.core.query.Query query = mongoQuery.createQuery(accessor); + assertThat(query.getQueryObject(), is(new Document().append("arg0", "calamity") + .append("arg1", "regalia").append("arg2", "calamity"))); + } + + @Test // DATAMONGO-1603 + public void shouldAllowReuseOfQuotedPlaceholderWithinQuery() throws Exception { + + StringBasedMongoQuery mongoQuery = createQueryForMethod("findByReusingPlaceholdersMultipleTimesWhenQuoted", + String.class, String.class); + ConvertingParameterAccessor accessor = StubParameterAccessor.getAccessor(converter, "calamity", "regalia"); + + org.springframework.data.mongodb.core.query.Query query = mongoQuery.createQuery(accessor); + assertThat(query.getQueryObject(), is(new Document().append("arg0", "calamity") + .append("arg1", "regalia").append("arg2", "calamity"))); + } + + @Test // DATAMONGO-1603 + public void shouldAllowReuseOfQuotedPlaceholderWithinQueryAndIncludeSuffixCorrectly() throws Exception { + + StringBasedMongoQuery mongoQuery = createQueryForMethod( + "findByReusingPlaceholdersMultipleTimesWhenQuotedAndSomeStuffAppended", String.class, String.class); + ConvertingParameterAccessor accessor = StubParameterAccessor.getAccessor(converter, "calamity", "regalia"); + + org.springframework.data.mongodb.core.query.Query query = mongoQuery.createQuery(accessor); + assertThat(query.getQueryObject(), is(new Document().append("arg0", "calamity") + .append("arg1", "regalia").append("arg2", "calamitys"))); + } + + @Test // DATAMONGO-1603 + public void shouldAllowQuotedParameterWithSuffixAppended() throws Exception { + + StringBasedMongoQuery mongoQuery = createQueryForMethod("findByWhenQuotedAndSomeStuffAppended", String.class, + String.class); + ConvertingParameterAccessor accessor = StubParameterAccessor.getAccessor(converter, "calamity", "regalia"); + + org.springframework.data.mongodb.core.query.Query query = mongoQuery.createQuery(accessor); + assertThat(query.getQueryObject(), + is(new Document().append("arg0", "calamity").append("arg1", "regalias"))); + } + + @Test // DATAMONGO-1603 + public void shouldCaptureReplacementWithComplexSuffixCorrectly() throws Exception { + + StringBasedMongoQuery mongoQuery = createQueryForMethod("findByMultiRegex", String.class); + ConvertingParameterAccessor accessor = StubParameterAccessor.getAccessor(converter, "calamity"); + + org.springframework.data.mongodb.core.query.Query query = mongoQuery.createQuery(accessor); + + assertThat(query.getQueryObject(), is(Document.parse( + "{ \"$or\" : [ { \"firstname\" : { \"$regex\" : \".*calamity.*\" , \"$options\" : \"i\"}} , { \"lastname\" : { \"$regex\" : \".*calamityxyz.*\" , \"$options\" : \"i\"}}]}"))); + } + + @Test // DATAMONGO-1603 + public void shouldAllowPlaceholderReuseInQuotedValue() throws Exception { + + StringBasedMongoQuery mongoQuery = createQueryForMethod("findByLastnameRegex", String.class, String.class); + ConvertingParameterAccessor accessor = StubParameterAccessor.getAccessor(converter, "calamity", "regalia"); + + org.springframework.data.mongodb.core.query.Query query = mongoQuery.createQuery(accessor); + + assertThat(query.getQueryObject(), + is(Document.parse("{ 'lastname' : { '$regex' : '^(calamity|John regalia|regalia)'} }"))); + } + private StringBasedMongoQuery createQueryForMethod(String name, Class... parameters) throws Exception { Method method = SampleRepository.class.getMethod(name, parameters); @@ -458,6 +533,9 @@ public class StringBasedMongoQueryUnitTests { @Query("{ 'lastname' : { '$regex' : '^(?0)'} }") Person findByLastnameRegex(String lastname); + @Query("{'$or' : [{'firstname': {'$regex': '.*?0.*', '$options': 'i'}}, {'lastname' : {'$regex': '.*?0xyz.*', '$options': 'i'}} ]}") + Person findByMultiRegex(String arg0); + @Query("{ 'address' : ?0 }") Person findByAddress(Address address); @@ -508,5 +586,20 @@ public class StringBasedMongoQueryUnitTests { @Query("{ 'arg0' : ?0 }") List findByWithBsonArgument(Document arg0); + + @Query("{ 'arg0' : ?0, 'arg1' : ?1, 'arg2' : ?0 }") + List findByReusingPlaceholdersMultipleTimes(String arg0, String arg1); + + @Query("{ 'arg0' : ?0, 'arg1' : ?1, 'arg2' : '?0' }") + List findByReusingPlaceholdersMultipleTimesWhenQuoted(String arg0, String arg1); + + @Query("{ 'arg0' : '?0', 'arg1' : ?1, 'arg2' : '?0s' }") + List findByReusingPlaceholdersMultipleTimesWhenQuotedAndSomeStuffAppended(String arg0, String arg1); + + @Query("{ 'arg0' : '?0', 'arg1' : '?1s' }") + List findByWhenQuotedAndSomeStuffAppended(String arg0, String arg1); + + @Query("{ 'lastname' : { '$regex' : '^(?0|John ?1|?1)'} }") // use spel or some regex string this is fucking bad + Person findByLastnameRegex(String lastname, String alternative); } }