Escape strings with quotes in custom query parameters. (#1793)

Original Pull Request #1793 
Closes #1790

src/main/java/org/springframework/data/elasticsearch/repository/query/ElasticsearchStringQuery.java

@@ -15,22 +15,16 @@
  */
 package org.springframework.data.elasticsearch.repository.query;
 
-import java.util.regex.Matcher;
-import java.util.regex.Pattern;
-
-import org.springframework.core.convert.support.GenericConversionService;
 import org.springframework.data.domain.PageRequest;
 import org.springframework.data.elasticsearch.core.ElasticsearchOperations;
 import org.springframework.data.elasticsearch.core.SearchHitSupport;
 import org.springframework.data.elasticsearch.core.SearchHits;
-import org.springframework.data.elasticsearch.core.convert.DateTimeConverters;
 import org.springframework.data.elasticsearch.core.mapping.IndexCoordinates;
 import org.springframework.data.elasticsearch.core.query.StringQuery;
+import org.springframework.data.elasticsearch.repository.support.StringQueryUtil;
 import org.springframework.data.repository.query.ParametersParameterAccessor;
 import org.springframework.data.util.StreamUtils;
 import org.springframework.util.Assert;
-import org.springframework.util.ClassUtils;
-import org.springframework.util.NumberUtils;
 
 /**
  * ElasticsearchStringQuery
@@ -43,11 +37,8 @@ import org.springframework.util.NumberUtils;
  */
 public class ElasticsearchStringQuery extends AbstractElasticsearchRepositoryQuery {
 
-	private static final Pattern PARAMETER_PLACEHOLDER = Pattern.compile("\\?(\\d+)");
 	private String query;
 
-	private final GenericConversionService conversionService = new GenericConversionService();
-
 	public ElasticsearchStringQuery(ElasticsearchQueryMethod queryMethod, ElasticsearchOperations elasticsearchOperations,
 			String query) {
 		super(queryMethod, elasticsearchOperations);
@@ -104,31 +95,8 @@ public class ElasticsearchStringQuery extends AbstractElasticsearchRepositoryQue
 	}
 
 	protected StringQuery createQuery(ParametersParameterAccessor parameterAccessor) {
-		String queryString = replacePlaceholders(this.query, parameterAccessor);
+		String queryString = StringQueryUtil.replacePlaceholders(this.query, parameterAccessor);
 		return new StringQuery(queryString);
 	}
 
-	private String replacePlaceholders(String input, ParametersParameterAccessor accessor) {
-
-		Matcher matcher = PARAMETER_PLACEHOLDER.matcher(input);
-		String result = input;
-		while (matcher.find()) {
-
-			String placeholder = Pattern.quote(matcher.group()) + "(?!\\d+)";
-			int index = NumberUtils.parseNumber(matcher.group(1), Integer.class);
-			result = result.replaceAll(placeholder, getParameterWithIndex(accessor, index));
-		}
-		return result;
-	}
-
-	private String getParameterWithIndex(ParametersParameterAccessor accessor, int index) {
-		Object parameter = accessor.getBindableValue(index);
-		if (parameter == null) {
-			return "null";
-		}
-		if (conversionService.canConvert(parameter.getClass(), String.class)) {
-			return conversionService.convert(parameter, String.class);
-		}
-		return parameter.toString();
-	}
 }

src/main/java/org/springframework/data/elasticsearch/repository/query/ReactiveElasticsearchStringQuery.java

@@ -15,15 +15,11 @@
  */
 package org.springframework.data.elasticsearch.repository.query;
 
-import java.util.regex.Matcher;
-import java.util.regex.Pattern;
-
 import org.springframework.data.elasticsearch.core.ReactiveElasticsearchOperations;
 import org.springframework.data.elasticsearch.core.query.StringQuery;
+import org.springframework.data.elasticsearch.repository.support.StringQueryUtil;
 import org.springframework.data.repository.query.QueryMethodEvaluationContextProvider;
 import org.springframework.expression.spel.standard.SpelExpressionParser;
-import org.springframework.util.NumberUtils;
-import org.springframework.util.ObjectUtils;
 
 /**
  * @author Christoph Strobl
@@ -32,7 +28,6 @@ import org.springframework.util.ObjectUtils;
  */
 public class ReactiveElasticsearchStringQuery extends AbstractReactiveElasticsearchRepositoryQuery {
 
-	private static final Pattern PARAMETER_PLACEHOLDER = Pattern.compile("\\?(\\d+)");
 	private final String query;
 
 	public ReactiveElasticsearchStringQuery(ReactiveElasticsearchQueryMethod queryMethod,
@@ -52,27 +47,10 @@ public class ReactiveElasticsearchStringQuery extends AbstractReactiveElasticsea
 
 	@Override
 	protected StringQuery createQuery(ElasticsearchParameterAccessor parameterAccessor) {
-		String queryString = replacePlaceholders(this.query, parameterAccessor);
+		String queryString = StringQueryUtil.replacePlaceholders(this.query, parameterAccessor);
 		return new StringQuery(queryString);
 	}
 
-	private String replacePlaceholders(String input, ElasticsearchParameterAccessor accessor) {
-
-		Matcher matcher = PARAMETER_PLACEHOLDER.matcher(input);
-		String result = input;
-		while (matcher.find()) {
-
-			String placeholder = Pattern.quote(matcher.group()) + "(?!\\d+)";
-			int index = NumberUtils.parseNumber(matcher.group(1), Integer.class);
-			result = result.replaceAll(placeholder, getParameterWithIndex(accessor, index));
-		}
-		return result;
-	}
-
-	private String getParameterWithIndex(ElasticsearchParameterAccessor accessor, int index) {
-		return ObjectUtils.nullSafeToString(accessor.getBindableValue(index));
-	}
-
 	@Override
 	boolean isCountQuery() {
 		return queryMethod.hasCountQueryAnnotation();

src/main/java/org/springframework/data/elasticsearch/repository/support/StringQueryUtil.java

@@ -0,0 +1,72 @@
+/*
+ * Copyright 2021 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.
+ * You may obtain a copy of the License at
+ *
+ *      https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.springframework.data.elasticsearch.repository.support;
+
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+import org.springframework.core.convert.support.GenericConversionService;
+import org.springframework.data.repository.query.ParameterAccessor;
+import org.springframework.util.NumberUtils;
+
+/**
+ * @author Peter-Josef Meisch
+ */
+final public class StringQueryUtil {
+
+	private static final Pattern PARAMETER_PLACEHOLDER = Pattern.compile("\\?(\\d+)");
+	private static final GenericConversionService conversionService = new GenericConversionService();
+
+	private StringQueryUtil() {}
+
+	public static String replacePlaceholders(String input, ParameterAccessor accessor) {
+
+		Matcher matcher = PARAMETER_PLACEHOLDER.matcher(input);
+		String result = input;
+		while (matcher.find()) {
+
+			String placeholder = Pattern.quote(matcher.group()) + "(?!\\d+)";
+			int index = NumberUtils.parseNumber(matcher.group(1), Integer.class);
+			result = result.replaceAll(placeholder, Matcher.quoteReplacement(getParameterWithIndex(accessor, index)));
+		}
+		return result;
+	}
+
+	private static String getParameterWithIndex(ParameterAccessor accessor, int index) {
+
+		Object parameter = accessor.getBindableValue(index);
+		String parameterValue = "null";
+
+		// noinspection ConstantConditions
+		if (parameter != null) {
+
+			if (conversionService.canConvert(parameter.getClass(), String.class)) {
+				String converted = conversionService.convert(parameter, String.class);
+
+				if (converted != null) {
+					parameterValue = converted;
+				}
+			} else {
+				parameterValue = parameter.toString();
+			}
+		}
+
+		parameterValue = parameterValue.replaceAll("\"", Matcher.quoteReplacement("\\\""));
+		return parameterValue;
+
+	}
+
+}

src/test/java/org/springframework/data/elasticsearch/repository/query/ElasticsearchStringQueryUnitTests.java

@@ -25,6 +25,7 @@ import java.util.List;
 import java.util.Map;
 
 import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.mockito.Mock;
@@ -37,6 +38,7 @@ import org.springframework.data.elasticsearch.annotations.InnerField;
 import org.springframework.data.elasticsearch.annotations.MultiField;
 import org.springframework.data.elasticsearch.annotations.Query;
 import org.springframework.data.elasticsearch.core.ElasticsearchOperations;
+import org.springframework.data.elasticsearch.core.SearchHits;
 import org.springframework.data.elasticsearch.core.convert.ElasticsearchConverter;
 import org.springframework.data.elasticsearch.core.convert.MappingElasticsearchConverter;
 import org.springframework.data.elasticsearch.core.mapping.SimpleElasticsearchMappingContext;
@@ -82,6 +84,17 @@ public class ElasticsearchStringQueryUnitTests {
 				.isEqualTo("name:(zero, eleven, one, two, three, four, five, six, seven, eight, nine, ten, eleven, zero, one)");
 	}
 
+	@Test // #1790
+	@DisplayName("should escape Strings in query parameters")
+	void shouldEscapeStringsInQueryParameters() throws Exception {
+
+		org.springframework.data.elasticsearch.core.query.Query query = createQuery("findByPrefix", "hello \"Stranger\"");
+
+		assertThat(query).isInstanceOf(StringQuery.class);
+		assertThat(((StringQuery) query).getSource())
+				.isEqualTo("{\"bool\":{\"must\": [{\"match\": {\"prefix\": {\"name\" : \"hello \\\"Stranger\\\"\"}}]}}");
+	}
+
 	private org.springframework.data.elasticsearch.core.query.Query createQuery(String methodName, String... args)
 			throws NoSuchMethodException {
 
@@ -90,7 +103,6 @@ public class ElasticsearchStringQueryUnitTests {
 		ElasticsearchStringQuery elasticsearchStringQuery = queryForMethod(queryMethod);
 		return elasticsearchStringQuery.createQuery(new ElasticsearchParametersParameterAccessor(queryMethod, args));
 	}
-
 	private ElasticsearchStringQuery queryForMethod(ElasticsearchQueryMethod queryMethod) {
 		return new ElasticsearchStringQuery(queryMethod, operations, queryMethod.getAnnotatedQuery());
 	}
@@ -110,6 +122,9 @@ public class ElasticsearchStringQueryUnitTests {
 		@Query(value = "name:(?0, ?11, ?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?0, ?1)")
 		Person findWithRepeatedPlaceholder(String arg0, String arg1, String arg2, String arg3, String arg4, String arg5,
 				String arg6, String arg7, String arg8, String arg9, String arg10, String arg11);
+
+		@Query("{\"bool\":{\"must\": [{\"match\": {\"prefix\": {\"name\" : \"?0\"}}]}}")
+		SearchHits<Book> findByPrefix(String prefix);
 	}
 
 	/**

src/test/java/org/springframework/data/elasticsearch/repository/query/ReactiveElasticsearchStringQueryUnitTests.java

@@ -29,6 +29,7 @@ import java.util.Map;
 
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Disabled;
+import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.mockito.Mock;
@@ -41,6 +42,7 @@ import org.springframework.data.elasticsearch.annotations.InnerField;
 import org.springframework.data.elasticsearch.annotations.MultiField;
 import org.springframework.data.elasticsearch.annotations.Query;
 import org.springframework.data.elasticsearch.core.ReactiveElasticsearchOperations;
+import org.springframework.data.elasticsearch.core.SearchHit;
 import org.springframework.data.elasticsearch.core.convert.ElasticsearchConverter;
 import org.springframework.data.elasticsearch.core.convert.MappingElasticsearchConverter;
 import org.springframework.data.elasticsearch.core.mapping.SimpleElasticsearchMappingContext;
@@ -119,6 +121,17 @@ public class ReactiveElasticsearchStringQueryUnitTests {
 				.isEqualTo("name:(zero, eleven, one, two, three, four, five, six, seven, eight, nine, ten, eleven, zero, one)");
 	}
 
+	@Test // #1790
+	@DisplayName("should escape Strings in query parameters")
+	void shouldEscapeStringsInQueryParameters() throws Exception {
+
+		org.springframework.data.elasticsearch.core.query.Query query = createQuery("findByPrefix", "hello \"Stranger\"");
+
+		assertThat(query).isInstanceOf(StringQuery.class);
+		assertThat(((StringQuery) query).getSource())
+				.isEqualTo("{\"bool\":{\"must\": [{\"match\": {\"prefix\": {\"name\" : \"hello \\\"Stranger\\\"\"}}]}}");
+	}
+
 	private org.springframework.data.elasticsearch.core.query.Query createQuery(String methodName, String... args)
 			throws NoSuchMethodException {
 
@@ -163,6 +176,10 @@ public class ReactiveElasticsearchStringQueryUnitTests {
 		@Query(value = "name:(?0, ?11, ?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?0, ?1)")
 		Person findWithRepeatedPlaceholder(String arg0, String arg1, String arg2, String arg3, String arg4, String arg5,
 				String arg6, String arg7, String arg8, String arg9, String arg10, String arg11);
+
+		@Query("{\"bool\":{\"must\": [{\"match\": {\"prefix\": {\"name\" : \"?0\"}}]}}")
+		Flux<SearchHit<Book>> findByPrefix(String prefix);
+
 	}
 
 	/**