Fix handling of array-of-strings parameters for @Query-annotated queries

[diamondT]

As described in the linked ticket, elasticsearch queries do not play along with array (collection) parameters. This appears to be happening for reactive repositories only.

Proposing to handle collections explicitly in ConvertingParameterAccessor and then the rest should fall into place in StringQueryUtil#convert.
Also proposing to amend the unit tests for reactive repositories (ReactiveElasticsearchStringQueryUnitTests) to use the ConvertingParameterAccessor as well.

Let me know what you think.

Thanks,

Closes #2135

[sothawo]

Sorry for the late reaction; I had two weeks vacation, and now I'm sick; but I'll have a look this week

[sothawo]

Having a closer look at this, I have to admit that it is a kind of mess - not your PR, thanks for that, but the existing code. Both the imperative and reactive part were written by different people before my time, I think this needs some consolidation.

I was just playing around a little in the ReactiveElasticsearchStringQueryUnitTests class and added these two test functions (wanted to check Integer arrays)

@Test // #THIS-SHOULD_BE_THE ACTUAL _ISSUE_NUMBER
@DisplayName("should handle array-of-strings parameters correctly")
void shouldHandleArrayOfStringsParametersCorrectly() throws Exception {
	List<String> otherNames = List.of("Wesley", "Emmett");
	org.springframework.data.elasticsearch.core.query.Query query = createQuery("findByOtherNames", otherNames);
	assertThat(query).isInstanceOf(StringQuery.class);
	assertThat(((StringQuery) query).getSource())
		.isEqualTo("{ 'bool' : { 'must' : { 'terms' : { 'otherNames' : [\"Wesley\",\"Emmett\"] } } } }");
}

@Test // #THIS-SHOULD_BE_THE ACTUAL _ISSUE_NUMBER
@DisplayName("should handle array-of-Integers parameters correctly")
void shouldHandleArrayOfIntegerParametersCorrectly() throws Exception {
	List<Integer> ages = List.of(42, 57);
	org.springframework.data.elasticsearch.core.query.Query query = createQuery("findByAges", ages);
	assertThat(query).isInstanceOf(StringQuery.class);
	assertThat(((StringQuery) query).getSource())
		.isEqualTo("{ 'bool' : { 'must' : { 'terms' : { 'ages' : [42,57] } } } }");
}

together with these repository functions:

@Query("{ 'bool' : { 'must' : { 'terms' : { 'otherNames' : ?0 } } } }")
Flux<Person> findByOtherNames(List<String> otherNames);

@Query("{ 'bool' : { 'must' : { 'terms' : { 'ages' : ?0 } } } }")
Flux<Person> findByAges(List<Integer> ages);

and a modified createQuery (taking your change and using ReactiveElasticsearchParametersParameterAccessor)

private org.springframework.data.elasticsearch.core.query.Query createQuery(String methodName, Object... args)
	throws NoSuchMethodException {

	Class<?>[] argTypes = Arrays.stream(args).map(Object::getClass)
		.map(clazz -> Collection.class.isAssignableFrom(clazz) ? List.class : clazz).toArray(Class[]::new);
	ReactiveElasticsearchQueryMethod queryMethod = getQueryMethod(methodName, argTypes);
	ReactiveElasticsearchStringQuery elasticsearchStringQuery = queryForMethod(queryMethod);

	return elasticsearchStringQuery
		.createQuery(new ReactiveElasticsearchParametersParameterAccessor(queryMethod, args));
}

I am on purpose not using the ConvertingParameterAccessor here, because the conversion of parameters (and in the case of a collection of the single collection elements) is done in org.springframework.data.elasticsearch.repository.support.StringQueryUtil#convert .

This tests then pass. I have not tried it in an integration test.

I think if we'd change the same way to build the query in ReactiveElasticsearchStringQuery this would be closer to the way it is done in the imperative part. Basically that would mean to just skip wrapping the parameterAccessor argument in org.springframework.data.elasticsearch.repository.query.AbstractReactiveElasticsearchRepositoryQuery#execute(org.springframework.data.elasticsearch.repository.query.ElasticsearchParameterAccessor) in a ConvertingParameterAccessor. We could drop the ConvertingParameterAccessor then altogether, it's not used anywhere else.

What do you think?

[diamondT]

thanks for taking the time to look into this.
if we can drop ConvertingParameterAccessor then yes i agree what you are proposing is cleaner. i assumed that the wrapper was there for a reason but - as you said - it might be simply because different people wrote different parts.

if that's ok, i can continue with dropping the accessor (should i just delete it?), copy your array-of-integers unit test and introduce an integration test for the same (already done for array-of-strings).

thanks,

[sothawo]

dropping the accessor (should i just delete it?),

yes, it has just this one use. And code that is not used should be removed.

[diamondT]

done. also fixed a typo, had the wrong ticket number...