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.

Author: christophstrobl

spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/query/ExpressionEvaluatingParameterBinder.java

@@ -126,7 +126,7 @@ private String replacePlaceholders(String input, MongoParameterAccessor accessor
 				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 @@ private Pattern createReplacementPattern(List<ParameterBinding> bindings) {
 
 			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 @@ private Placeholder extractPlaceholder(int parameterIndex, Matcher matcher) {
 
 		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 @@ private Placeholder extractPlaceholder(int parameterIndex, Matcher matcher) {
 				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

@@ -339,8 +339,7 @@ private static void potentiallyAddBinding(String source, List<ParameterBinding>
 			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

@@ -437,6 +437,78 @@ public void shouldReplaceParametersInInQuotedExpressionOfNestedQueryOperator() t
 		assertThat(query.getQueryObject(), is((DBObject) new BasicDBObject("lastname", Pattern.compile("^(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((DBObject) new BasicDBObject().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((DBObject) new BasicDBObject().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((DBObject) new BasicDBObject().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((DBObject) new BasicDBObject().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((DBObject) JSON.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((DBObject) JSON.parse("{ 'lastname' : { '$regex' : '^(calamity|John regalia|regalia)'} }")));
+	}
+
 	private StringBasedMongoQuery createQueryForMethod(String name, Class<?>... parameters) throws Exception {
 
 		Method method = SampleRepository.class.getMethod(name, parameters);
@@ -460,6 +532,9 @@ private interface SampleRepository extends Repository<Person, Long> {
 		@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);
 
@@ -510,5 +585,20 @@ private interface SampleRepository extends Repository<Person, Long> {
 
 		@Query("{ 'arg0' : ?0 }")
 		List<Person> findByWithBsonArgument(DBObject 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);
 	}
 }