spring-projects / spring-data-elasticsearch

Provide support to increase developer productivity in Java when using Elasticsearch. Uses familiar Spring concepts such as a template classes for core API usage and lightweight repository style data access.
https://spring.io/projects/spring-data-elasticsearch/
Apache License 2.0
2.9k stars 1.33k forks source link

Allow for `null` and `empty` parameters in the MultiField annotation. #2960

Closed youssef3wi closed 4 weeks ago

youssef3wi commented 1 month ago

Closes #2952

youssef3wi commented 1 month ago

I think it would be better to include these parameters in https://github.com/spring-projects/spring-data-elasticsearch/blob/d079a59cb4568a206764f522bb3fd4ca44af59c5/src/main/java/org/springframework/data/elasticsearch/annotations/InnerField.java#L181

boolean storeNullValue() default false;
boolean storeEmptyValue() default true;

to also support inner fields

https://github.com/spring-projects/spring-data-elasticsearch/blob/d079a59cb4568a206764f522bb3fd4ca44af59c5/src/main/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentProperty.java#L113

storeNullValue = isField ? getRequiredAnnotation(Field.class).storeNullValue() 
: isMultiField && (getRequiredAnnotation(MultiField.class).mainField().storeNullValue() 
|| Arrays.stream(getRequiredAnnotation(MultiField.class).otherFields())
.anyMatch(InnerField::storeNullValue));

storeEmptyValue = isField ? getRequiredAnnotation(Field.class).storeEmptyValue() 
: !isMultiField || getRequiredAnnotation(MultiField.class).mainField().storeEmptyValue() 
|| Arrays.stream(getRequiredAnnotation(MultiField.class).otherFields())
.anyMatch(InnerField::storeEmptyValue);
sothawo commented 1 month ago

I just tried to use this in a sample application, but it fails when trying to create the index with the null_value mapping in https://github.com/spring-projects/spring-data-elasticsearch/blob/main/src/main/java/org/springframework/data/elasticsearch/client/elc/RequestConverter.java#L258.

Might be an error on our side, or an error in the Elasticsearch client. Won't have any more time today to check.

We should have an integration test for this that creates an index with the mapping, stores a value and retrieves it.

sothawo commented 4 weeks ago

TIL: you cannot set a null_value on a field of type text. This fails on Elasticsearch level, not only in the Elasticsearch client library.

Although the documentation about null_value does not mention this.

Will get back to your PR tomorrow.