Skip to content

Use registered converters for parameters of @Query annotated methods. #1867

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,8 @@ public Object execute(Object[] parameters) {
}

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);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,9 @@ public ReactiveElasticsearchStringQuery(String query, ReactiveElasticsearchQuery

@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);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
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;

Expand All @@ -31,11 +31,14 @@
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;
Expand All @@ -48,7 +51,7 @@ public static String replacePlaceholders(String input, ParameterAccessor accesso
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";
Expand All @@ -63,7 +66,7 @@ private static String getParameterWithIndex(ParameterAccessor accessor, int inde

}

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("[");
Expand Down
Original file line number Diff line number Diff line change
@@ -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<Converter<?, ?>> 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<Car, String> {
INSTANCE;

@Override
public String convert(ElasticsearchStringQueryUnitTests.Car car) {
return (car.getName() != null ? car.getName() : "null") + '-'
+ (car.getModel() != null ? car.getModel() : "null");
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -40,9 +41,6 @@
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;
Expand All @@ -55,14 +53,13 @@
* @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
Expand Down Expand Up @@ -141,6 +138,21 @@ private org.springframework.data.elasticsearch.core.query.Query createQuery(Stri
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());
}
Expand All @@ -149,7 +161,7 @@ private ElasticsearchQueryMethod getQueryMethod(String name, Class<?>... paramet

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<Person, String> {
Expand All @@ -172,13 +184,16 @@ Person findWithRepeatedPlaceholder(String arg0, String arg1, String arg2, String

@Query("{\"bool\":{\"must\": [{\"match\": {\"prefix\": {\"name\" : \"?0\"}}]}}")
SearchHits<Book> 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")
Expand Down Expand Up @@ -292,29 +307,6 @@ public void setDescription(@Nullable String description) {
}
}

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;
Expand All @@ -338,5 +330,4 @@ public void setName(String name) {
this.name = name;
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -43,9 +44,6 @@
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;
Expand All @@ -59,16 +57,15 @@
* @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
Expand Down Expand Up @@ -132,7 +129,22 @@ void shouldEscapeStringsInQueryParameters() throws Exception {
.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);
Expand All @@ -152,7 +164,7 @@ private ReactiveElasticsearchQueryMethod getQueryMethod(String name, Class<?>...

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 {
Expand Down Expand Up @@ -180,6 +192,9 @@ Person findWithRepeatedPlaceholder(String arg0, String arg1, String arg2, String
@Query("{\"bool\":{\"must\": [{\"match\": {\"prefix\": {\"name\" : \"?0\"}}]}}")
Flux<SearchHit<Book>> findByPrefix(String prefix);

@Query("{ 'bool' : { 'must' : { 'term' : { 'car' : '?0' } } } }")
Mono<Person> findByCar(Car car);

}

/**
Expand Down Expand Up @@ -292,29 +307,6 @@ public void setDescription(@Nullable String description) {
}
}

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;
Expand Down