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.

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);

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));
 			}

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<Person> findByWithBsonArgument(Document arg0);
+
+		@Query("{ 'arg0' : ?0, 'arg1' : ?1, 'arg2' : ?0 }")
+		List<Person> findByReusingPlaceholdersMultipleTimes(String arg0, String arg1);
+
+		@Query("{ 'arg0' : ?0, 'arg1' : ?1, 'arg2' : '?0' }")
+		List<Person> findByReusingPlaceholdersMultipleTimesWhenQuoted(String arg0, String arg1);
+
+		@Query("{ 'arg0' : '?0', 'arg1' : ?1, 'arg2' : '?0s' }")
+		List<Person> findByReusingPlaceholdersMultipleTimesWhenQuotedAndSomeStuffAppended(String arg0, String arg1);
+
+		@Query("{ 'arg0' : '?0', 'arg1' : '?1s' }")
+		List<Person> 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);
 	}
 }