From 6c40f0bfe0c172d1f261b8dc303028b2fb584442 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 (cherry picked from commit 27094724dcf1a9f3932c86adff6cb58341ec2016) (cherry picked from commit 303438ae6398bc02f06115e752791e5ef896939f) (cherry picked from commit 626b274677100067f409a47e99581203b5717ffd) --- .../query/ElasticsearchStringQuery.java | 3 +- .../ReactiveElasticsearchStringQuery.java | 4 +- .../repository/support/StringQueryUtil.java | 58 +++++++++----- .../ElasticsearchStringQueryUnitTestBase.java | 78 +++++++++++++++++++ .../ElasticsearchStringQueryUnitTests.java | 63 ++++++--------- ...tiveElasticsearchStringQueryUnitTests.java | 69 ++++++---------- 6 files changed, 170 insertions(+), 105 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 8a70d7318..58d68a01e 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 @@ -94,7 +94,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 6cbf63191..f0de221a4 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 36061d30c..699e3266a 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,10 +20,12 @@ import java.util.regex.Matcher; import java.util.regex.Pattern; import java.util.stream.Collectors; +import org.springframework.core.convert.ConversionService; import org.springframework.core.convert.support.GenericConversionService; import org.springframework.data.elasticsearch.core.convert.DateTimeConverters; import org.springframework.data.elasticsearch.repository.query.ElasticsearchStringQuery; import org.springframework.data.repository.query.ParameterAccessor; +import org.springframework.util.Assert; import org.springframework.util.ClassUtils; import org.springframework.util.NumberUtils; @@ -34,25 +36,38 @@ 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(); - { - if (!conversionService.canConvert(java.util.Date.class, String.class)) { - conversionService.addConverter(DateTimeConverters.JavaDateConverter.INSTANCE); - } - if (ClassUtils.isPresent("org.joda.time.DateTimeZone", ElasticsearchStringQuery.class.getClassLoader())) { - if (!conversionService.canConvert(org.joda.time.ReadableInstant.class, String.class)) { - conversionService.addConverter(DateTimeConverters.JodaDateTimeConverter.INSTANCE); - } - if (!conversionService.canConvert(org.joda.time.LocalDateTime.class, String.class)) { - conversionService.addConverter(DateTimeConverters.JodaLocalDateTimeConverter.INSTANCE); - } - } - } + private final ConversionService conversionService; + private final GenericConversionService genericConversionService; - private StringQueryUtil() {} + public StringQueryUtil(ConversionService conversionService) { - public static String replacePlaceholders(String input, ParameterAccessor accessor) { + Assert.notNull(conversionService, "conversionService must not be null"); + + this.conversionService = conversionService; + genericConversionService = setupGenericConversionService(); + } + + private GenericConversionService setupGenericConversionService() { + + GenericConversionService genericConversionService = new GenericConversionService(); + + if (!genericConversionService.canConvert(java.util.Date.class, String.class)) { + genericConversionService.addConverter(DateTimeConverters.JavaDateConverter.INSTANCE); + } + + if (ClassUtils.isPresent("org.joda.time.DateTimeZone", ElasticsearchStringQuery.class.getClassLoader())) { + if (!genericConversionService.canConvert(org.joda.time.ReadableInstant.class, String.class)) { + genericConversionService.addConverter(DateTimeConverters.JodaDateTimeConverter.INSTANCE); + } + if (!genericConversionService.canConvert(org.joda.time.LocalDateTime.class, String.class)) { + genericConversionService.addConverter(DateTimeConverters.JodaLocalDateTimeConverter.INSTANCE); + } + } + return genericConversionService; + } + + public String replacePlaceholders(String input, ParameterAccessor accessor) { Matcher matcher = PARAMETER_PLACEHOLDER.matcher(input); String result = input; @@ -65,7 +80,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"; @@ -80,7 +95,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("["); @@ -101,6 +116,13 @@ final public class StringQueryUtil { if (converted != null) { parameterValue = converted; } + } else if (genericConversionService.canConvert(parameter.getClass(), String.class)) { + String converted = genericConversionService.convert(parameter, String.class); + + if (converted != null) { + parameterValue = converted; + + } } else { parameterValue = parameter.toString(); } 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 39d6a814f..71f3a9958 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 lombok.AllArgsConstructor; import lombok.Builder; @@ -46,9 +47,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; @@ -61,14 +59,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 @@ -147,6 +144,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()); } @@ -155,7 +167,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 { @@ -178,13 +190,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", replicas = 0, refreshInterval = "-1") @@ -264,38 +279,6 @@ public class ElasticsearchStringQueryUnitTests { searchAnalyzer = "standard") }) private String description; } - /** - * @author Rizwan Idrees - * @author Mohsin Husen - * @author Artur Konczak - */ - @Setter - @Getter - @NoArgsConstructor - @AllArgsConstructor - @Builder - static class Car { - - private String name; - private String model; - - public String getName() { - return name; - } - - public void setName(String name) { - this.name = name; - } - - public String getModel() { - return model; - } - - public void setModel(String model) { - this.model = model; - } - } - /** * @author Rizwan Idrees * @author Mohsin Husen 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 835cbf86f..638b373f1 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 @@ -1,5 +1,5 @@ /* - * Copyright 2019-2020 the original author or authors. + * Copyright 2019-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. @@ -16,6 +16,7 @@ package org.springframework.data.elasticsearch.repository.query; import static org.assertj.core.api.Assertions.*; +import static org.mockito.Mockito.*; import lombok.AllArgsConstructor; import lombok.Builder; @@ -48,9 +49,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; @@ -64,16 +62,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 @@ -137,7 +134,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); @@ -157,7 +169,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 { @@ -185,6 +197,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); + } /** @@ -263,42 +278,6 @@ public class ReactiveElasticsearchStringQueryUnitTests { searchAnalyzer = "standard") }) private String description; } - /** - * @author Rizwan Idrees - * @author Mohsin Husen - * @author Artur Konczak - */ - @Setter - @Getter - @NoArgsConstructor - @AllArgsConstructor - @Builder - static class Car { - - private String name; - private String model; - - public String getName() { - return name; - } - - public void setName(String name) { - this.name = name; - } - - public String getModel() { - return model; - } - - public void setModel(String model) { - this.model = model; - } - } - - /** - * @author Rizwan Idrees - * @author Mohsin Husen - */ static class Author { @Nullable private String id;