From 27094724dcf1a9f3932c86adff6cb58341ec2016 Mon Sep 17 00:00:00 2001 From: Peter-Josef Meisch Date: Wed, 14 Jul 2021 19:31:30 +0200 Subject: [PATCH] Use registered converters for parameters of @Query annotated methods. Original Pull Request #1867 Closes #1866 --- .../query/ElasticsearchStringQuery.java | 3 +- .../ReactiveElasticsearchStringQuery.java | 4 +- .../repository/support/StringQueryUtil.java | 15 ++-- .../ElasticsearchStringQueryUnitTestBase.java | 78 +++++++++++++++++++ .../ElasticsearchStringQueryUnitTests.java | 55 ++++++------- ...tiveElasticsearchStringQueryUnitTests.java | 54 ++++++------- 6 files changed, 138 insertions(+), 71 deletions(-) create mode 100644 src/test/java/org/springframework/data/elasticsearch/repository/query/ElasticsearchStringQueryUnitTestBase.java diff --git a/src/main/java/org/springframework/data/elasticsearch/repository/query/ElasticsearchStringQuery.java b/src/main/java/org/springframework/data/elasticsearch/repository/query/ElasticsearchStringQuery.java index 3f5e9ed83..1b359db3f 100644 --- a/src/main/java/org/springframework/data/elasticsearch/repository/query/ElasticsearchStringQuery.java +++ b/src/main/java/org/springframework/data/elasticsearch/repository/query/ElasticsearchStringQuery.java @@ -102,7 +102,8 @@ public class ElasticsearchStringQuery extends AbstractElasticsearchRepositoryQue } protected StringQuery createQuery(ParametersParameterAccessor parameterAccessor) { - String queryString = StringQueryUtil.replacePlaceholders(this.query, parameterAccessor); + String queryString = new StringQueryUtil(elasticsearchOperations.getElasticsearchConverter().getConversionService()) + .replacePlaceholders(this.query, parameterAccessor); return new StringQuery(queryString); } diff --git a/src/main/java/org/springframework/data/elasticsearch/repository/query/ReactiveElasticsearchStringQuery.java b/src/main/java/org/springframework/data/elasticsearch/repository/query/ReactiveElasticsearchStringQuery.java index 110a2d8ba..6fe4f4793 100644 --- a/src/main/java/org/springframework/data/elasticsearch/repository/query/ReactiveElasticsearchStringQuery.java +++ b/src/main/java/org/springframework/data/elasticsearch/repository/query/ReactiveElasticsearchStringQuery.java @@ -47,7 +47,9 @@ public class ReactiveElasticsearchStringQuery extends AbstractReactiveElasticsea @Override protected StringQuery createQuery(ElasticsearchParameterAccessor parameterAccessor) { - String queryString = StringQueryUtil.replacePlaceholders(this.query, parameterAccessor); + String queryString = new StringQueryUtil( + getElasticsearchOperations().getElasticsearchConverter().getConversionService()).replacePlaceholders(this.query, + parameterAccessor); return new StringQuery(queryString); } diff --git a/src/main/java/org/springframework/data/elasticsearch/repository/support/StringQueryUtil.java b/src/main/java/org/springframework/data/elasticsearch/repository/support/StringQueryUtil.java index 89e4cc921..6f1354cd7 100644 --- a/src/main/java/org/springframework/data/elasticsearch/repository/support/StringQueryUtil.java +++ b/src/main/java/org/springframework/data/elasticsearch/repository/support/StringQueryUtil.java @@ -20,7 +20,7 @@ import java.util.regex.Matcher; import java.util.regex.Pattern; import java.util.stream.Collectors; -import org.springframework.core.convert.support.GenericConversionService; +import org.springframework.core.convert.ConversionService; import org.springframework.data.repository.query.ParameterAccessor; import org.springframework.util.NumberUtils; @@ -31,11 +31,14 @@ import org.springframework.util.NumberUtils; final public class StringQueryUtil { private static final Pattern PARAMETER_PLACEHOLDER = Pattern.compile("\\?(\\d+)"); - private static final GenericConversionService conversionService = new GenericConversionService(); - private StringQueryUtil() {} + private final ConversionService conversionService; - public static String replacePlaceholders(String input, ParameterAccessor accessor) { + public StringQueryUtil(ConversionService conversionService) { + this.conversionService = conversionService; + } + + public String replacePlaceholders(String input, ParameterAccessor accessor) { Matcher matcher = PARAMETER_PLACEHOLDER.matcher(input); String result = input; @@ -48,7 +51,7 @@ final public class StringQueryUtil { return result; } - private static String getParameterWithIndex(ParameterAccessor accessor, int index) { + private String getParameterWithIndex(ParameterAccessor accessor, int index) { Object parameter = accessor.getBindableValue(index); String parameterValue = "null"; @@ -63,7 +66,7 @@ final public class StringQueryUtil { } - private static String convert(Object parameter) { + private String convert(Object parameter) { if (Collection.class.isAssignableFrom(parameter.getClass())) { Collection collectionParam = (Collection) parameter; StringBuilder sb = new StringBuilder("["); diff --git a/src/test/java/org/springframework/data/elasticsearch/repository/query/ElasticsearchStringQueryUnitTestBase.java b/src/test/java/org/springframework/data/elasticsearch/repository/query/ElasticsearchStringQueryUnitTestBase.java new file mode 100644 index 000000000..429d97a56 --- /dev/null +++ b/src/test/java/org/springframework/data/elasticsearch/repository/query/ElasticsearchStringQueryUnitTestBase.java @@ -0,0 +1,78 @@ +/* + * 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.query; + +import java.util.ArrayList; +import java.util.Collection; + +import org.springframework.core.convert.converter.Converter; +import org.springframework.data.convert.CustomConversions; +import org.springframework.data.elasticsearch.core.convert.ElasticsearchConverter; +import org.springframework.data.elasticsearch.core.convert.ElasticsearchCustomConversions; +import org.springframework.data.elasticsearch.core.convert.MappingElasticsearchConverter; +import org.springframework.data.elasticsearch.core.mapping.SimpleElasticsearchMappingContext; +import org.springframework.lang.Nullable; + +/** + * @author Peter-Josef Meisch + */ +public class ElasticsearchStringQueryUnitTestBase { + + protected ElasticsearchConverter setupConverter() { + MappingElasticsearchConverter converter = new MappingElasticsearchConverter( + new SimpleElasticsearchMappingContext()); + Collection> converters = new ArrayList<>(); + converters.add(ElasticsearchStringQueryUnitTests.CarConverter.INSTANCE); + CustomConversions customConversions = new ElasticsearchCustomConversions(converters); + converter.setConversions(customConversions); + converter.afterPropertiesSet(); + return converter; + } + + static class Car { + @Nullable private String name; + @Nullable private String model; + + @Nullable + public String getName() { + return name; + } + + public void setName(@Nullable String name) { + this.name = name; + } + + @Nullable + public String getModel() { + return model; + } + + public void setModel(@Nullable String model) { + this.model = model; + } + } + + enum CarConverter implements Converter { + INSTANCE; + + @Override + public String convert(ElasticsearchStringQueryUnitTests.Car car) { + return (car.getName() != null ? car.getName() : "null") + '-' + + (car.getModel() != null ? car.getModel() : "null"); + } + } + +} diff --git a/src/test/java/org/springframework/data/elasticsearch/repository/query/ElasticsearchStringQueryUnitTests.java b/src/test/java/org/springframework/data/elasticsearch/repository/query/ElasticsearchStringQueryUnitTests.java index 0c351db0a..d37c5707f 100644 --- a/src/test/java/org/springframework/data/elasticsearch/repository/query/ElasticsearchStringQueryUnitTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/repository/query/ElasticsearchStringQueryUnitTests.java @@ -16,6 +16,7 @@ package org.springframework.data.elasticsearch.repository.query; import static org.assertj.core.api.Assertions.*; +import static org.mockito.Mockito.*; import java.lang.reflect.Method; import java.util.ArrayList; @@ -40,9 +41,6 @@ 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; import org.springframework.data.elasticsearch.core.query.StringQuery; import org.springframework.data.projection.SpelAwareProxyProjectionFactory; import org.springframework.data.repository.Repository; @@ -55,14 +53,13 @@ import org.springframework.lang.Nullable; * @author Niklas Herder */ @ExtendWith(MockitoExtension.class) -public class ElasticsearchStringQueryUnitTests { +public class ElasticsearchStringQueryUnitTests extends ElasticsearchStringQueryUnitTestBase { @Mock ElasticsearchOperations operations; - ElasticsearchConverter converter; @BeforeEach public void setUp() { - converter = new MappingElasticsearchConverter(new SimpleElasticsearchMappingContext()); + when(operations.getElasticsearchConverter()).thenReturn(setupConverter()); } @Test // DATAES-552 @@ -141,6 +138,21 @@ public class ElasticsearchStringQueryUnitTests { return elasticsearchStringQuery.createQuery(new ElasticsearchParametersParameterAccessor(queryMethod, args)); } + @Test // #1866 + @DisplayName("should use converter on parameters") + void shouldUseConverterOnParameters() throws NoSuchMethodException { + + Car car = new Car(); + car.setName("Toyota"); + car.setModel("Prius"); + + org.springframework.data.elasticsearch.core.query.Query query = createQuery("findByCar", car); + + assertThat(query).isInstanceOf(StringQuery.class); + assertThat(((StringQuery) query).getSource()) + .isEqualTo("{ 'bool' : { 'must' : { 'term' : { 'car' : 'Toyota-Prius' } } } }"); + } + private ElasticsearchStringQuery queryForMethod(ElasticsearchQueryMethod queryMethod) { return new ElasticsearchStringQuery(queryMethod, operations, queryMethod.getAnnotatedQuery()); } @@ -149,7 +161,7 @@ public class ElasticsearchStringQueryUnitTests { Method method = SampleRepository.class.getMethod(name, parameters); return new ElasticsearchQueryMethod(method, new DefaultRepositoryMetadata(SampleRepository.class), - new SpelAwareProxyProjectionFactory(), converter.getMappingContext()); + new SpelAwareProxyProjectionFactory(), operations.getElasticsearchConverter().getMappingContext()); } private interface SampleRepository extends Repository { @@ -172,13 +184,16 @@ public class ElasticsearchStringQueryUnitTests { @Query("{\"bool\":{\"must\": [{\"match\": {\"prefix\": {\"name\" : \"?0\"}}]}}") SearchHits findByPrefix(String prefix); + + @Query("{ 'bool' : { 'must' : { 'term' : { 'car' : '?0' } } } }") + Person findByCar(Car car); } /** * @author Rizwan Idrees * @author Mohsin Husen * @author Artur Konczak - * @author Niklas Herder + * @author Niklas Herder */ @Document(indexName = "test-index-person-query-unittest") @@ -292,29 +307,6 @@ public class ElasticsearchStringQueryUnitTests { } } - static class Car { - @Nullable private String name; - @Nullable private String model; - - @Nullable - public String getName() { - return name; - } - - public void setName(@Nullable String name) { - this.name = name; - } - - @Nullable - public String getModel() { - return model; - } - - public void setModel(@Nullable String model) { - this.model = model; - } - } - static class Author { @Nullable private String id; @@ -338,5 +330,4 @@ public class ElasticsearchStringQueryUnitTests { this.name = name; } } - } diff --git a/src/test/java/org/springframework/data/elasticsearch/repository/query/ReactiveElasticsearchStringQueryUnitTests.java b/src/test/java/org/springframework/data/elasticsearch/repository/query/ReactiveElasticsearchStringQueryUnitTests.java index 9a24e0af3..aabbd80f7 100644 --- a/src/test/java/org/springframework/data/elasticsearch/repository/query/ReactiveElasticsearchStringQueryUnitTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/repository/query/ReactiveElasticsearchStringQueryUnitTests.java @@ -16,6 +16,7 @@ package org.springframework.data.elasticsearch.repository.query; import static org.assertj.core.api.Assertions.*; +import static org.mockito.Mockito.*; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; @@ -43,9 +44,6 @@ 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; import org.springframework.data.elasticsearch.core.query.StringQuery; import org.springframework.data.projection.SpelAwareProxyProjectionFactory; import org.springframework.data.repository.Repository; @@ -59,16 +57,15 @@ import org.springframework.lang.Nullable; * @author Peter-Josef Meisch */ @ExtendWith(MockitoExtension.class) -public class ReactiveElasticsearchStringQueryUnitTests { +public class ReactiveElasticsearchStringQueryUnitTests extends ElasticsearchStringQueryUnitTestBase { SpelExpressionParser PARSER = new SpelExpressionParser(); - ElasticsearchConverter converter; @Mock ReactiveElasticsearchOperations operations; @BeforeEach public void setUp() { - converter = new MappingElasticsearchConverter(new SimpleElasticsearchMappingContext()); + when(operations.getElasticsearchConverter()).thenReturn(setupConverter()); } @Test // DATAES-519 @@ -132,7 +129,22 @@ public class ReactiveElasticsearchStringQueryUnitTests { .isEqualTo("{\"bool\":{\"must\": [{\"match\": {\"prefix\": {\"name\" : \"hello \\\"Stranger\\\"\"}}]}}"); } - private org.springframework.data.elasticsearch.core.query.Query createQuery(String methodName, String... args) + @Test // #1866 + @DisplayName("should use converter on parameters") + void shouldUseConverterOnParameters() throws Exception { + + Car car = new Car(); + car.setName("Toyota"); + car.setModel("Prius"); + + org.springframework.data.elasticsearch.core.query.Query query = createQuery("findByCar", car); + + assertThat(query).isInstanceOf(StringQuery.class); + assertThat(((StringQuery) query).getSource()) + .isEqualTo("{ 'bool' : { 'must' : { 'term' : { 'car' : 'Toyota-Prius' } } } }"); + } + + private org.springframework.data.elasticsearch.core.query.Query createQuery(String methodName, Object... args) throws NoSuchMethodException { Class[] argTypes = Arrays.stream(args).map(Object::getClass).toArray(Class[]::new); @@ -152,7 +164,7 @@ public class ReactiveElasticsearchStringQueryUnitTests { Method method = SampleRepository.class.getMethod(name, parameters); return new ReactiveElasticsearchQueryMethod(method, new DefaultRepositoryMetadata(SampleRepository.class), - new SpelAwareProxyProjectionFactory(), converter.getMappingContext()); + new SpelAwareProxyProjectionFactory(), operations.getElasticsearchConverter().getMappingContext()); } private ReactiveElasticsearchStringQuery createQueryForMethod(String name, Class... parameters) throws Exception { @@ -180,6 +192,9 @@ public class ReactiveElasticsearchStringQueryUnitTests { @Query("{\"bool\":{\"must\": [{\"match\": {\"prefix\": {\"name\" : \"?0\"}}]}}") Flux> findByPrefix(String prefix); + @Query("{ 'bool' : { 'must' : { 'term' : { 'car' : '?0' } } } }") + Mono findByCar(Car car); + } /** @@ -292,29 +307,6 @@ public class ReactiveElasticsearchStringQueryUnitTests { } } - static class Car { - @Nullable private String name; - @Nullable private String model; - - @Nullable - public String getName() { - return name; - } - - public void setName(@Nullable String name) { - this.name = name; - } - - @Nullable - public String getModel() { - return model; - } - - public void setModel(@Nullable String model) { - this.model = model; - } - } - static class Author { @Nullable private String id;