Skip to content

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

Merged
merged 2 commits into from
Aug 19, 2024

Conversation

youssef3wi
Copy link
Contributor

  • You have read the Spring Data contribution guidelines.
  • There is a ticket in the bug tracker for the project in our issue tracker. Add the issue number to the Closes #issue-number line below
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.
  • You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

Closes #2952

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 10, 2024
@youssef3wi
Copy link
Contributor Author

I think it would be better to include these parameters in

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

to also support inner fields

storeEmptyValue = isField ? getRequiredAnnotation(Field.class).storeEmptyValue() : true;

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

Copy link
Collaborator

@sothawo sothawo left a 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.

@youssef3wi youssef3wi requested a review from sothawo August 14, 2024 10:39
Copy link
Collaborator

@sothawo sothawo left a 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.

Signed-off-by: Youssef Aouichaoui <[email protected]>
@sothawo
Copy link
Collaborator

sothawo commented Aug 18, 2024

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
Copy link
Collaborator

sothawo commented Aug 18, 2024

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.

@sothawo sothawo merged commit 9149c1b into spring-projects:main Aug 19, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support storeNullValue and storeEmptyValue for property that is annotated with @MultiField annotation
3 participants