Unify conversion services for value replacement when querying elasticsearch.

Original Pull Request #2834
Closes #2833

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

@@ -15,6 +15,7 @@
  */
 package org.springframework.data.elasticsearch.repository.query;
 
+import org.springframework.core.convert.ConversionService;
 import org.springframework.data.elasticsearch.core.ElasticsearchOperations;
 import org.springframework.data.elasticsearch.core.query.BaseQuery;
 import org.springframework.data.elasticsearch.core.query.StringQuery;
@@ -64,11 +65,12 @@ public class ElasticsearchStringQuery extends AbstractElasticsearchRepositoryQue
 	}
 
 	protected BaseQuery createQuery(ElasticsearchParametersParameterAccessor parameterAccessor) {
-		String queryString = new StringQueryUtil(elasticsearchOperations.getElasticsearchConverter().getConversionService())
+		ConversionService conversionService = elasticsearchOperations.getElasticsearchConverter().getConversionService();
+		String queryString = new StringQueryUtil(conversionService)
 				.replacePlaceholders(this.queryString, parameterAccessor);
 
 		QueryStringSpELEvaluator evaluator = new QueryStringSpELEvaluator(queryString, parameterAccessor, queryMethod,
-				evaluationContextProvider);
+				evaluationContextProvider, conversionService);
 		var query = new StringQuery(evaluator.evaluate());
 		query.addSort(parameterAccessor.getSort());
 		return query;

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

@@ -15,6 +15,7 @@
  */
 package org.springframework.data.elasticsearch.repository.query;
 
+import org.springframework.core.convert.ConversionService;
 import org.springframework.data.elasticsearch.core.ReactiveElasticsearchOperations;
 import org.springframework.data.elasticsearch.core.query.BaseQuery;
 import org.springframework.data.elasticsearch.core.query.StringQuery;
@@ -56,8 +57,10 @@ public class ReactiveElasticsearchStringQuery extends AbstractReactiveElasticsea
 				getElasticsearchOperations().getElasticsearchConverter().getConversionService())
 				.replacePlaceholders(this.query, parameterAccessor);
 
+		ConversionService conversionService = getElasticsearchOperations().getElasticsearchConverter()
+				.getConversionService();
 		QueryStringSpELEvaluator evaluator = new QueryStringSpELEvaluator(queryString, parameterAccessor, queryMethod,
-				evaluationContextProvider);
+				evaluationContextProvider, conversionService);
 		return new StringQuery(evaluator.evaluate());
 	}
 

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

@@ -15,18 +15,19 @@
  */
 package org.springframework.data.elasticsearch.repository.support;
 
-import java.util.Collection;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
-import java.util.stream.Collectors;
 
 import org.springframework.core.convert.ConversionService;
+import org.springframework.data.elasticsearch.core.convert.ConversionException;
+import org.springframework.data.elasticsearch.repository.support.value.ElasticsearchQueryValueConversionService;
 import org.springframework.data.repository.query.ParameterAccessor;
 import org.springframework.util.NumberUtils;
 
 /**
  * @author Peter-Josef Meisch
  * @author Niklas Herder
+ * @author Haibo Liu
  */
 final public class StringQueryUtil {
 
@@ -35,7 +36,7 @@ final public class StringQueryUtil {
 	private final ConversionService conversionService;
 
 	public StringQueryUtil(ConversionService conversionService) {
-		this.conversionService = conversionService;
+		this.conversionService = ElasticsearchQueryValueConversionService.getInstance(conversionService);
 	}
 
 	public String replacePlaceholders(String input, ParameterAccessor accessor) {
@@ -46,7 +47,7 @@ final public class StringQueryUtil {
 
 			String placeholder = Pattern.quote(matcher.group()) + "(?!\\d+)";
 			int index = NumberUtils.parseNumber(matcher.group(1), Integer.class);
-			String replacement = Matcher.quoteReplacement(getParameterWithIndex(accessor, index));
+			String replacement = Matcher.quoteReplacement(getParameterWithIndex(accessor, index, input));
 			result = result.replaceAll(placeholder, replacement);
 			// need to escape backslashes that are not escapes for quotes so that they are sent as double-backslashes
 			// to Elasticsearch
@@ -55,47 +56,16 @@ final public class StringQueryUtil {
 		return result;
 	}
 
-	private String getParameterWithIndex(ParameterAccessor accessor, int index) {
+	private String getParameterWithIndex(ParameterAccessor accessor, int index, String input) {
 
 		Object parameter = accessor.getBindableValue(index);
-		String parameterValue = "null";
-
-		if (parameter != null) {
-			parameterValue = convert(parameter);
-		}
-
-		return parameterValue;
-
-	}
-
-	private String convert(Object parameter) {
-		if (Collection.class.isAssignableFrom(parameter.getClass())) {
-			Collection<?> collectionParam = (Collection<?>) parameter;
-			StringBuilder sb = new StringBuilder("[");
-			sb.append(collectionParam.stream().map(o -> {
-				if (o instanceof String) {
-					return "\"" + convert(o) + "\"";
-				} else {
-					return convert(o);
-				}
-			}).collect(Collectors.joining(",")));
-			sb.append("]");
-			return sb.toString();
-		} else {
-			String parameterValue = "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;
+		String value =  conversionService.convert(parameter, String.class);
+		if (value == null) {
+			throw new ConversionException(String.format(
+					"Parameter value can't be null for placeholder at index '%s' in query '%s' when querying elasticsearch",
+					index, input));
 		}
+		return value;
 	}
 
 }

src/main/java/org/springframework/data/elasticsearch/repository/support/spel/ElasticsearchValueSpELConversionService.java

@@ -1,42 +0,0 @@
-/*
- * Copyright 2024 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.spel;
-
-import org.springframework.core.convert.ConversionService;
-import org.springframework.core.convert.converter.ConverterRegistry;
-import org.springframework.core.convert.support.DefaultConversionService;
-import org.springframework.data.util.Lazy;
-
-/**
- * To supply a {@link ConversionService} with custom converters to handle SpEL values in elasticsearch query.
- *
- * @since 5.3
- * @author Haibo Liu
- */
-public class ElasticsearchValueSpELConversionService {
-
-	public static final Lazy<ConversionService> CONVERSION_SERVICE_LAZY = Lazy.of(
-			ElasticsearchValueSpELConversionService::buildSpELConversionService);
-
-	private static ConversionService buildSpELConversionService() {
-		// register elasticsearch custom type converter for conversion service
-		ConversionService conversionService = new DefaultConversionService();
-		ConverterRegistry converterRegistry = (ConverterRegistry) conversionService;
-		converterRegistry.addConverter(new ElasticsearchCollectionValueToStringConverter(conversionService));
-		converterRegistry.addConverter(new ElasticsearchStringValueToStringConverter());
-		return conversionService;
-	}
-}

src/main/java/org/springframework/data/elasticsearch/repository/support/spel/QueryStringSpELEvaluator.java

@@ -18,8 +18,12 @@ package org.springframework.data.elasticsearch.repository.support.spel;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
 
+import org.springframework.core.convert.ConversionService;
 import org.springframework.data.elasticsearch.core.convert.ConversionException;
 import org.springframework.data.elasticsearch.repository.query.ElasticsearchParametersParameterAccessor;
+import org.springframework.data.elasticsearch.repository.support.value.ElasticsearchQueryValueConversionService;
+import org.springframework.data.elasticsearch.repository.support.value.ElasticsearchCollectionValueToStringConverter;
+import org.springframework.data.elasticsearch.repository.support.value.ElasticsearchStringValueToStringConverter;
 import org.springframework.data.repository.query.QueryMethod;
 import org.springframework.data.repository.query.QueryMethodEvaluationContextProvider;
 import org.springframework.expression.EvaluationContext;
@@ -53,13 +57,14 @@ public class QueryStringSpELEvaluator {
 	private final TypeConverter elasticsearchSpELTypeConverter;
 
 	public QueryStringSpELEvaluator(String queryString, ElasticsearchParametersParameterAccessor parameterAccessor,
-			QueryMethod queryMethod, QueryMethodEvaluationContextProvider evaluationContextProvider) {
+			QueryMethod queryMethod, QueryMethodEvaluationContextProvider evaluationContextProvider,
+			ConversionService conversionService) {
 		this.queryString = queryString;
 		this.parameterAccessor = parameterAccessor;
 		this.queryMethod = queryMethod;
 		this.evaluationContextProvider = evaluationContextProvider;
 		this.elasticsearchSpELTypeConverter = new StandardTypeConverter(
-				ElasticsearchValueSpELConversionService.CONVERSION_SERVICE_LAZY);
+				ElasticsearchQueryValueConversionService.getInstance(conversionService));
 	}
 
 	/**

src/main/java/org/springframework/data/elasticsearch/repository/support/value/ElasticsearchCollectionValueToStringConverter.java

@@ -13,7 +13,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-package org.springframework.data.elasticsearch.repository.support.spel;
+package org.springframework.data.elasticsearch.repository.support.value;
 
 import java.util.Collection;
 import java.util.Collections;

src/main/java/org/springframework/data/elasticsearch/repository/support/value/ElasticsearchQueryValueConversionService.java

@@ -0,0 +1,94 @@
+/*
+ * Copyright 2024 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.value;
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+
+import org.springframework.core.convert.ConversionService;
+import org.springframework.core.convert.TypeDescriptor;
+import org.springframework.core.convert.converter.Converter;
+import org.springframework.core.convert.support.GenericConversionService;
+import org.springframework.lang.Nullable;
+import org.springframework.util.Assert;
+
+/**
+ * A {@link ConversionService} using custom converters to handle query values in elasticsearch query. If the value to be
+ * converted beyond the scope of custom converters, it'll delegate to the {@link #delegate delegated conversion service}.
+ * <p>
+ * This is a better solution for converting query values in elasticsearch query, because it has all the capability the
+ * {@link #delegate delegated conversion service} has, especially for user-registered {@link Converter}s.
+ *
+ * @since 5.3
+ * @author Haibo Liu
+ */
+public class ElasticsearchQueryValueConversionService implements ConversionService {
+
+	private static final Map<ConversionService, ElasticsearchQueryValueConversionService> CACHE = new ConcurrentHashMap<>();
+
+	private final GenericConversionService valueConversionService = new GenericConversionService();
+
+	private final ConversionService delegate;
+
+	private ElasticsearchQueryValueConversionService(ConversionService delegate) {
+		Assert.notNull(delegate, "delegated ConversionService must not be null");
+		this.delegate = delegate;
+
+		// register elasticsearch custom type converters for conversion service
+		valueConversionService.addConverter(new ElasticsearchCollectionValueToStringConverter(this));
+		valueConversionService.addConverter(new ElasticsearchStringValueToStringConverter());
+	}
+
+	/**
+	 * Get a {@link ElasticsearchQueryValueConversionService} with this conversion service as delegated.
+	 *
+	 * @param conversionService conversion service as delegated
+	 * @return a conversion service having the capability to convert query values in elasticsearch query
+	 */
+	public static ElasticsearchQueryValueConversionService getInstance(ConversionService conversionService) {
+		return CACHE.computeIfAbsent(conversionService, ElasticsearchQueryValueConversionService::new);
+	}
+
+	@Override
+	public boolean canConvert(@Nullable Class<?> sourceType, Class<?> targetType) {
+		Assert.notNull(targetType, "Target type to convert to cannot be null");
+		return canConvert(TypeDescriptor.valueOf(sourceType), TypeDescriptor.valueOf(targetType));
+	}
+
+	@Override
+	public boolean canConvert(@Nullable TypeDescriptor sourceType, TypeDescriptor targetType) {
+		return valueConversionService.canConvert(sourceType, targetType)
+				|| delegate.canConvert(sourceType, targetType);
+	}
+
+	@SuppressWarnings("unchecked")
+	@Override
+	@Nullable
+	public <T> T convert(@Nullable Object source, Class<T> targetType) {
+		Assert.notNull(targetType, "Target type to convert to cannot be null");
+		return (T) convert(source, TypeDescriptor.forObject(source), TypeDescriptor.valueOf(targetType));
+	}
+
+	@Override
+	@Nullable
+	public Object convert(@Nullable Object source, @Nullable TypeDescriptor sourceType, TypeDescriptor targetType) {
+		if (valueConversionService.canConvert(sourceType, targetType)) {
+			return valueConversionService.convert(source, sourceType, targetType);
+		} else {
+			return delegate.convert(source, sourceType, targetType);
+		}
+	}
+}

src/main/java/org/springframework/data/elasticsearch/repository/support/value/ElasticsearchStringValueToStringConverter.java

@@ -13,7 +13,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-package org.springframework.data.elasticsearch.repository.support.spel;
+package org.springframework.data.elasticsearch.repository.support.value;
 
 import java.util.Collections;
 import java.util.Set;

src/test/java/org/springframework/data/elasticsearch/repositories/custommethod/CustomMethodRepositoryELCIntegrationTests.java

@@ -15,9 +15,12 @@
  */
 package org.springframework.data.elasticsearch.repositories.custommethod;
 
+import java.util.List;
+
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
 import org.springframework.context.annotation.Import;
+import org.springframework.data.elasticsearch.core.convert.ElasticsearchCustomConversions;
 import org.springframework.data.elasticsearch.junit.jupiter.ElasticsearchTemplateConfiguration;
 import org.springframework.data.elasticsearch.repository.config.EnableElasticsearchRepositories;
 import org.springframework.data.elasticsearch.utils.IndexNameProvider;
@@ -49,5 +52,11 @@ public class CustomMethodRepositoryELCIntegrationTests extends CustomMethodRepos
 		QueryParameter queryParameter() {
 			return new QueryParameter("abc");
 		}
+
+		@Bean
+		public ElasticsearchCustomConversions elasticsearchCustomConversions() {
+			return new ElasticsearchCustomConversions(List.of(SamplePropertyToStringConverter.INSTANCE,
+					StringToSamplePropertyConverter.INSTANCE));
+		}
 	}
 }

src/test/java/org/springframework/data/elasticsearch/repositories/custommethod/CustomMethodRepositoryIntegrationTests.java

@@ -33,6 +33,7 @@ import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Test;
 import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.core.convert.converter.Converter;
 import org.springframework.data.annotation.Id;
 import org.springframework.data.annotation.Version;
 import org.springframework.data.domain.Page;
@@ -66,6 +67,7 @@ import org.springframework.data.geo.Metrics;
 import org.springframework.data.geo.Point;
 import org.springframework.data.repository.query.Param;
 import org.springframework.lang.Nullable;
+import org.springframework.util.Assert;
 
 /**
  * @author Rizwan Idrees
@@ -1524,6 +1526,19 @@ public abstract class CustomMethodRepositoryIntegrationTests {
 		assertThat(searchHits.getTotalHits()).isEqualTo(20);
 	}
 
+	@Test
+	void shouldRaiseExceptionForNullStringQuery() {
+		List<SampleEntity> entities = createSampleEntities("abc", 20);
+		repository.saveAll(entities);
+
+		// when
+		ConversionException thrown = assertThrows(ConversionException.class, () -> repository.queryByString(null));
+
+		assertThat(thrown.getMessage())
+				.contains("Parameter value can't be null for placeholder at index '0' in query",
+						"when querying elasticsearch");
+	}
+
 	@Test
 	void shouldReturnSearchHitsForStringQuerySpEL() {
 		List<SampleEntity> entities = createSampleEntities("abc", 20);
@@ -1653,6 +1668,32 @@ public abstract class CustomMethodRepositoryIntegrationTests {
 		assertThat(searchHits.getTotalHits()).isEqualTo(20);
 	}
 
+	@Test
+	void shouldReturnSearchHitsBySampleProperty() {
+		List<SampleEntity> entities = createSampleEntities("abc", 20);
+		entities.forEach(e -> e.setSampleProperty(new SampleProperty("hello", "world")));
+		repository.saveAll(entities);
+
+		SampleProperty sampleProperty = new SampleProperty("hello", "world");
+		// when
+		SearchHits<SampleEntity> searchHits = repository.queryBySampleProperty(sampleProperty);
+
+		assertThat(searchHits.getTotalHits()).isEqualTo(20);
+	}
+
+	@Test
+	void shouldReturnSearchHitsWithCustomConversionSpEL() {
+		List<SampleEntity> entities = createSampleEntities("abc", 20);
+		entities.forEach(e -> e.setSampleProperty(new SampleProperty("hello", "world")));
+		repository.saveAll(entities);
+
+		SampleProperty sampleProperty = new SampleProperty("hello", "world");
+		// when
+		SearchHits<SampleEntity> searchHits = repository.queryBySamplePropertySpEL(sampleProperty);
+
+		assertThat(searchHits.getTotalHits()).isEqualTo(20);
+	}
+
 	@Test // DATAES-372
 	void shouldReturnHighlightsOnAnnotatedMethod() {
 		List<SampleEntity> entities = createSampleEntities("abc", 2);
@@ -2067,9 +2108,13 @@ public abstract class CustomMethodRepositoryIntegrationTests {
 		@Highlight(fields = { @HighlightField(name = "type") })
 		SearchHits<SampleEntity> queryByType(String type);
 
+		/**
+		 * The parameter is annotated with {@link Nullable} deliberately to test that our placeholder parameter will
+		 * not accept a null parameter as query value.
+		 */
 		@Query("{\"bool\": {\"must\": [{\"term\": {\"type\": \"?0\"}}]}}")
 		@Highlight(fields = { @HighlightField(name = "type") })
-		SearchHits<SampleEntity> queryByString(String type);
+		SearchHits<SampleEntity> queryByString(@Nullable String type);
 
 		/**
 		 * The parameter is annotated with {@link Nullable} deliberately to test that our elasticsearch SpEL converters will
@@ -2170,6 +2215,36 @@ public abstract class CustomMethodRepositoryIntegrationTests {
 		SearchHits<SampleEntity> queryByParameterPropertyCollectionSpELWithParamAnnotation(
 				@Param("e") Collection<QueryParameter> parameters);
 
+		@Query("""
+				{
+				  "bool":{
+				    "must":[
+				      {
+				        "term":{
+				          "sample_property": "?0"
+				        }
+				      }
+				    ]
+				  }
+				}
+				""")
+		SearchHits<SampleEntity> queryBySampleProperty(SampleProperty sampleProperty);
+
+		@Query("""
+				{
+				  "bool":{
+				    "must":[
+				      {
+				        "term":{
+				          "sample_property": "#{#sampleProperty}"
+				        }
+				      }
+				    ]
+				  }
+				}
+				""")
+		SearchHits<SampleEntity> queryBySamplePropertySpEL(SampleProperty sampleProperty);
+
 		@Query("""
 				{
 				  "bool":{
@@ -2276,6 +2351,9 @@ public abstract class CustomMethodRepositoryIntegrationTests {
 		@Field(name = "custom_field_name", type = Text)
 		@Nullable private String customFieldNameMessage;
 
+		@Field(name = "sample_property", type = Keyword)
+		@Nullable private SampleProperty sampleProperty;
+
 		@Nullable
 		public String getId() {
 			return id;
@@ -2312,6 +2390,15 @@ public abstract class CustomMethodRepositoryIntegrationTests {
 			this.customFieldNameMessage = customFieldNameMessage;
 		}
 
+		@Nullable
+		public SampleProperty getSampleProperty() {
+			return sampleProperty;
+		}
+
+		public void setSampleProperty(@Nullable SampleProperty sampleProperty) {
+			this.sampleProperty = sampleProperty;
+		}
+
 		@Nullable
 		public String getKeyword() {
 			return keyword;
@@ -2355,4 +2442,53 @@ public abstract class CustomMethodRepositoryIntegrationTests {
 			this.version = version;
 		}
 	}
+
+	static class SampleProperty {
+		@Nullable
+		private String first;
+		@Nullable private String last;
+
+		SampleProperty(@Nullable String first, @Nullable String last) {
+			this.first = first;
+			this.last = last;
+		}
+
+		@Nullable
+		public String getFirst() {
+			return first;
+		}
+
+		public void setFirst(@Nullable String first) {
+			this.first = first;
+		}
+
+		@Nullable
+		public String getLast() {
+			return last;
+		}
+
+		public void setLast(@Nullable String last) {
+			this.last = last;
+		}
+	}
+
+	enum SamplePropertyToStringConverter implements Converter<SampleProperty, String> {
+		INSTANCE;
+
+		@Override
+		public String convert(SampleProperty sampleProperty) {
+			return sampleProperty.getFirst() + '-' + sampleProperty.getLast();
+		}
+	}
+
+	enum StringToSamplePropertyConverter implements Converter<String, SampleProperty> {
+		INSTANCE;
+
+		@Override
+		public SampleProperty convert(String string) {
+			String[] splits = string.split("-");
+			Assert.isTrue(splits.length == 2, "only 1 '-' should be in sample property value");
+			return new SampleProperty(splits[0], splits[1]);
+		}
+	}
 }