-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Allow for null
and empty
parameters in the MultiField annotation.
#2960
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
Conversation
Signed-off-by: Youssef Aouichaoui <[email protected]>
I think it would be better to include these parameters in Line 181 in d079a59
boolean storeNullValue() default false;
boolean storeEmptyValue() default true; to also support inner fields Line 113 in d079a59
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking that over.
As storeEmpty and nul values are written to the index mapping I think there should be a test in the (Reacive)MappingBuilderUnitTests
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The inner field of a multi field can have a different null_value than the outer field. This should be reflected in the mapping builder test, I added the changes for that; it is already working but wasn't in the test.
I think this should be covered in the integration tests as well, using different null values for the outer and inner fields.
And no, I don't know why one would set different values, but it is possible so we need to make sure to support it.
From https://www.elastic.co/guide/en/elasticsearch/reference/current/multi-fields.html:
A multi-field doesn’t inherit any mapping options from its parent field.
src/test/java/org/springframework/data/elasticsearch/core/index/MappingBuilderUnitTests.java
Outdated
Show resolved
Hide resolved
src/test/java/org/springframework/data/elasticsearch/core/index/MappingBuilderUnitTests.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Youssef Aouichaoui <[email protected]>
I just tried to use this in a sample application, but it fails when trying to create the index with the 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. |
TIL: you cannot set a Although the documentation about null_value does not mention this. Will get back to your PR tomorrow. |
Closes #2952