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.

This backport contains elements from DATAMONGO-1575.

Original pull request: #441.
Related ticket: DATAMONGO-1575.

Author: christophstrobl

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2015-2016 the original author or authors.
+ * Copyright 2015-2017 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -15,7 +15,9 @@
  */
 package org.springframework.data.mongodb.repository.query;
 
+import lombok.EqualsAndHashCode;
 import lombok.Value;
+import lombok.experimental.UtilityClass;
 
 import java.util.ArrayList;
 import java.util.LinkedHashMap;
@@ -37,12 +39,13 @@
 import org.springframework.util.CollectionUtils;
 import org.springframework.util.StringUtils;
 
+import com.mongodb.DBObject;
 import com.mongodb.util.JSON;
 
 /**
- * {@link ExpressionEvaluatingParameterBinder} allows to evaluate, convert and bind parameters to placholders within a
+ * {@link ExpressionEvaluatingParameterBinder} allows to evaluate, convert and bind parameters to placeholders within a
  * {@link String}.
- * 
+ *
  * @author Christoph Strobl
  * @author Thomas Darimont
  * @author Oliver Gierke
@@ -56,7 +59,7 @@ class ExpressionEvaluatingParameterBinder {
 
 	/**
 	 * Creates new {@link ExpressionEvaluatingParameterBinder}
-	 * 
+	 *
 	 * @param expressionParser must not be {@literal null}.
 	 * @param evaluationContextProvider must not be {@literal null}.
 	 */
@@ -73,7 +76,7 @@ public ExpressionEvaluatingParameterBinder(SpelExpressionParser expressionParser
 	/**
 	 * 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}.
@@ -90,7 +93,7 @@ public String bind(String raw, MongoParameterAccessor accessor, BindingContext b
 
 	/**
 	 * 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}.
@@ -110,16 +113,23 @@ private String replacePlaceholders(String input, MongoParameterAccessor accessor
 		Matcher matcher = createReplacementPattern(bindingContext.getBindings()).matcher(input);
 		StringBuffer buffer = new StringBuffer();
 
+		int parameterIndex = 0;
 		while (matcher.find()) {
 
-			ParameterBinding binding = bindingContext.getBindingFor(extractPlaceholder(matcher.group()));
+			Placeholder placeholder = extractPlaceholder(parameterIndex++, matcher);
+			ParameterBinding binding = bindingContext.getBindingFor(placeholder);
 			String valueForBinding = getParameterValueForBinding(accessor, bindingContext.getParameters(), binding);
 
 			// appendReplacement does not like unescaped $ sign and others, so we need to quote that stuff first
 			matcher.appendReplacement(buffer, Matcher.quoteReplacement(valueForBinding));
+			if (StringUtils.hasText(placeholder.getSuffix())) {
+				buffer.append(placeholder.getSuffix());
+			}
 
-			if (binding.isQuoted()) {
-				postProcessQuotedBinding(buffer, valueForBinding);
+			if (placeholder.isQuoted()) {
+				postProcessQuotedBinding(buffer, valueForBinding,
+						!binding.isExpression() ? accessor.getBindableValue(binding.getParameterIndex()) : null,
+						binding.isExpression());
 			}
 		}
 
@@ -134,8 +144,10 @@ private String replacePlaceholders(String input, MongoParameterAccessor accessor
 	 *
 	 * @param buffer the {@link StringBuffer} to operate upon.
 	 * @param valueForBinding the actual binding value.
+	 * @param raw the raw binding value
+	 * @param isExpression {@literal true} if the binding value results from a SpEL expression.
 	 */
-	private void postProcessQuotedBinding(StringBuffer buffer, String valueForBinding) {
+	private void postProcessQuotedBinding(StringBuffer buffer, String valueForBinding, Object raw, boolean isExpression) {
 
 		int quotationMarkIndex = buffer.length() - valueForBinding.length() - 1;
 		char quotationMark = buffer.charAt(quotationMarkIndex);
@@ -151,7 +163,8 @@ private void postProcessQuotedBinding(StringBuffer buffer, String valueForBindin
 			quotationMark = buffer.charAt(quotationMarkIndex);
 		}
 
-		if (valueForBinding.startsWith("{")) { // remove quotation char before the complex object string
+		// remove quotation char before the complex object string
+		if (valueForBinding.startsWith("{") && (raw instanceof DBObject || isExpression)) {
 
 			buffer.deleteCharAt(quotationMarkIndex);
 
@@ -181,7 +194,12 @@ private String getParameterValueForBinding(MongoParameterAccessor accessor, Mong
 				: accessor.getBindableValue(binding.getParameterIndex());
 
 		if (value instanceof String && binding.isQuoted()) {
-			return ((String) value).startsWith("{") ? (String) value : ((String) value).replace("\"", "\\\"");
+
+			if (binding.isExpression() && ((String) value).startsWith("{")) {
+				return (String) value;
+			}
+
+			return QuotedString.unquote(JSON.serialize(value));
 		}
 
 		if (value instanceof byte[]) {
@@ -228,8 +246,9 @@ private Pattern createReplacementPattern(List<ParameterBinding> bindings) {
 		for (ParameterBinding binding : bindings) {
 
 			regex.append("|");
-			regex.append(Pattern.quote(binding.getParameter()));
-			regex.append("['\"]?"); // potential quotation char (as in { foo : '?0' }).
+			regex.append("(" + Pattern.quote(binding.getParameter()) + ")");
+			regex.append("([\\w.]*");
+			regex.append("(\\W?['\"]|\\w*')?)");
 		}
 
 		return Pattern.compile(regex.substring(1));
@@ -239,14 +258,40 @@ private Pattern createReplacementPattern(List<ParameterBinding> bindings) {
 	 * 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}.
+	 * @param parameterIndex The actual parameter index.
+	 * @param matcher The actual {@link Matcher}.
 	 * @return
 	 */
-	private Placeholder extractPlaceholder(String groupName) {
+	private Placeholder extractPlaceholder(int parameterIndex, Matcher matcher) {
+
+		String rawPlaceholder = matcher.group(parameterIndex * 3 + 1);
+		String suffix = matcher.group(parameterIndex * 3 + 2);
+
+		if (!StringUtils.hasText(rawPlaceholder)) {
 
-		return !groupName.endsWith("'") && !groupName.endsWith("\"") ? //
-				Placeholder.of(groupName, false) : //
-				Placeholder.of(groupName.substring(0, groupName.length() - 1), true);
+			rawPlaceholder = matcher.group();
+			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 = rawPlaceholder.substring(0,
+						rawPlaceholder.length() - (StringUtils.hasText(suffix) ? suffix.length() : 1));
+			}
+		}
+
+		if (StringUtils.hasText(suffix)) {
+
+			boolean quoted = QuotedString.endsWithQuote(suffix);
+
+			return Placeholder.of(parameterIndex, rawPlaceholder, quoted,
+					quoted ? QuotedString.unquoteSuffix(suffix) : suffix);
+		}
+		return Placeholder.of(parameterIndex, rawPlaceholder, false, null);
 	}
 
 	/**
@@ -317,8 +362,9 @@ private static Map<Placeholder, ParameterBinding> mapBindings(List<ParameterBind
 
 			Map<Placeholder, ParameterBinding> map = new LinkedHashMap<Placeholder, ParameterBinding>(bindings.size(), 1);
 
+			int parameterIndex = 0;
 			for (ParameterBinding binding : bindings) {
-				map.put(Placeholder.of(binding.getParameter(), binding.isQuoted()), binding);
+				map.put(Placeholder.of(parameterIndex++, binding.getParameter(), binding.isQuoted(), null), binding);
 			}
 
 			return map;
@@ -332,18 +378,60 @@ private static Map<Placeholder, ParameterBinding> mapBindings(List<ParameterBind
 	 * @since 1.9
 	 */
 	@Value(staticConstructor = "of")
+	@EqualsAndHashCode(exclude = { "quoted", "suffix" })
 	static class Placeholder {
 
+		private int parameterIndex;
 		private final String parameter;
 		private final boolean quoted;
+		private final String suffix;
 
 		/*
 		 * (non-Javadoc)
 		 * @see java.lang.Object#toString()
 		 */
 		@Override
 		public String toString() {
-			return quoted ? String.format("'%s'", parameter) : parameter;
+			return quoted ? String.format("'%s'", parameter + (suffix != null ? suffix : ""))
+					: parameter + (suffix != null ? suffix : "");
+		}
+
+	}
+
+	/**
+	 * Utility to handle quoted strings using single/double quotes.
+	 *
+	 * @author Mark Paluch
+	 */
+	@UtilityClass
+	static class QuotedString {
+
+		/**
+		 * @param string
+		 * @return {@literal true} if {@literal string} ends with a single/double quote.
+		 */
+		static boolean endsWithQuote(String string) {
+			return string.endsWith("'") || string.endsWith("\"");
+		}
+
+		/**
+		 * Remove trailing quoting from {@literal quoted}.
+		 *
+		 * @param quoted
+		 * @return {@literal quoted} with removed quotes.
+		 */
+		public static String unquoteSuffix(String quoted) {
+			return quoted.substring(0, quoted.length() - 1);
+		}
+
+		/**
+		 * Remove leading and trailing quoting from {@literal quoted}.
+		 *
+		 * @param quoted
+		 * @return {@literal quoted} with removed quotes.
+		 */
+		public static String unquote(String quoted) {
+			return quoted.substring(1, quoted.length() - 1);
 		}
 	}
 }

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2011-2015 the original author or authors.
+ * Copyright 2011-2017 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -38,10 +38,11 @@
 
 /**
  * Query to use a plain JSON String to create the {@link Query} to actually execute.
- * 
+ *
  * @author Oliver Gierke
  * @author Christoph Strobl
  * @author Thomas Darimont
+ * @author Mark Paluch
  */
 public class StringBasedMongoQuery extends AbstractMongoQuery {
 
@@ -59,7 +60,7 @@ public class StringBasedMongoQuery extends AbstractMongoQuery {
 
 	/**
 	 * Creates a new {@link StringBasedMongoQuery} for the given {@link MongoQueryMethod} and {@link MongoOperations}.
-	 * 
+	 *
 	 * @param method must not be {@literal null}.
 	 * @param mongoOperations must not be {@literal null}.
 	 * @param expressionParser must not be {@literal null}.
@@ -126,7 +127,7 @@ protected Query createQuery(ConvertingParameterAccessor accessor) {
 		return query;
 	}
 
-	/* 
+	/*
 	 * (non-Javadoc)
 	 * @see org.springframework.data.mongodb.repository.query.AbstractMongoQuery#isCountQuery()
 	 */
@@ -146,10 +147,10 @@ protected boolean isDeleteQuery() {
 
 	/**
 	 * A parser that extracts the parameter bindings from a given query string.
-	 * 
+	 *
 	 * @author Thomas Darimont
 	 */
-	private static enum ParameterBindingParser {
+	private enum ParameterBindingParser {
 
 		INSTANCE;
 
@@ -169,7 +170,7 @@ private static enum ParameterBindingParser {
 		/**
 		 * Returns a list of {@link ParameterBinding}s found in the given {@code input} or an
 		 * {@link Collections#emptyList()}.
-		 * 
+		 *
 		 * @param input can be {@literal null} or empty.
 		 * @param bindings must not be {@literal null}.
 		 * @return
@@ -256,15 +257,15 @@ private static void collectParameterReferencesIntoBindings(List<ParameterBinding
 
 			} else if (value instanceof Pattern) {
 
-				String string = ((Pattern) value).toString().trim();
+				String string = value.toString().trim();
 				Matcher valueMatcher = PARSEABLE_BINDING_PATTERN.matcher(string);
 
 				while (valueMatcher.find()) {
 
 					int paramIndex = Integer.parseInt(valueMatcher.group(PARAMETER_INDEX_GROUP));
 
 					/*
-					 * The pattern is used as a direct parameter replacement, e.g. 'field': ?1, 
+					 * The pattern is used as a direct parameter replacement, e.g. 'field': ?1,
 					 * therefore we treat it as not quoted to remain backwards compatible.
 					 */
 					boolean quoted = !string.equals(PARAMETER_PREFIX + paramIndex);
@@ -297,8 +298,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));
 			}
@@ -315,7 +315,7 @@ private static int getIndexOfExpressionParameter(String input, int position) {
 
 	/**
 	 * A generic parameter binding with name or position information.
-	 * 
+	 *
 	 * @author Thomas Darimont
 	 */
 	static class ParameterBinding {
@@ -326,7 +326,7 @@ static class ParameterBinding {
 
 		/**
 		 * Creates a new {@link ParameterBinding} with the given {@code parameterIndex} and {@code quoted} information.
-		 * 
+		 *
 		 * @param parameterIndex
 		 * @param quoted whether or not the parameter is already quoted.
 		 */