Skip to content

Allow to customize the mapped type name for @InnerField and @Field annotations #2950

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Aug 6, 2024
Merged

Allow to customize the mapped type name for @InnerField and @Field annotations #2950

merged 2 commits into from
Aug 6, 2024

Conversation

reta
Copy link
Contributor

@reta reta commented Jul 31, 2024

In Spring Data OpenSearch [1], which is built on top of Spring Data Elasticsearch, we are running into cases where both projects differ in incompatible ways. One of such cases is supported mapping field types (baked by org.springframework.data.elasticsearch.annotations.FieldType), for example:

Flattened("flattened"), //

would have to become

Flat_Object("flat_oblect"), //

for OpenSearch.

This pull request adds optional mappedName attribute to @InnerField and @Field annotations (and model classes respectively), to address the possible divergence, for example:

@Field(type = FieldType.Flattened, mappedName = "flat_oblect") String flattenedField;

It would require a bit more work for OpenSearch users but not the Elasticsearch ones. The change would be non-breaking (defaults to "" and FieldType::getMappedType) and work seamlessly.

  • 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 #2942

@reta
Allow to customize the mapped type name for @innerfield and @field an…
be9a602 
…notations

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 31, 2024
reta
reta commented Jul 31, 2024
View reviewed changes
...t/java/org/springframework/data/elasticsearch/core/index/MappingBuilderIntegrationTests.java
@@ -77,6 +78,12 @@ void cleanup() {
operations.indexOps(IndexCoordinates.of(indexNameProvider.getPrefix() + "*")).delete();
}

@Test
public void shouldSupportAllTypes() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is not strictly related but EntityWithAllTypes was not used in tests at all, fixing that

@reta
Copy link
Contributor Author

reta commented Aug 2, 2024

@sothawo would really appreciate a review when you have some time, thank you

@sothawo
Copy link
Collaborator

sothawo commented Aug 2, 2024

tomorrow, been busy this week 😁

sothawo
sothawo requested changes Aug 3, 2024
View reviewed changes
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 the PR, I just think that the mappedName should be called mappedTypeName so that it will not be confused with the field name, that is mapped as well from the Java property name to the Field.name attribute

src/main/java/org/springframework/data/elasticsearch/annotations/Field.java Outdated
@@ -240,4 +241,11 @@
* @since 5.1
*/
boolean storeEmptyValue() default true;

/**
* overrides the mapping field type which otherwise will be taken from corresponding {@link FieldType}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* overrides the mapping field type which otherwise will be taken from corresponding {@link FieldType}
* overrides the field type in the mapping which otherwise will be taken from corresponding {@link FieldType}

Seems a little clearer for me

src/main/java/org/springframework/data/elasticsearch/annotations/InnerField.java Outdated
@@ -171,4 +172,11 @@
* @since 5.4
*/
KnnIndexOptions[] knnIndexOptions() default {};

/**
* overrides the mapping field type which otherwise will be taken from corresponding {@link FieldType}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

src/main/java/org/springframework/data/elasticsearch/core/index/MappingBuilder.java Outdated
@@ -210,7 +212,7 @@ private void writeTypeHintMapping(ObjectNode propertiesNode) throws IOException
}

private void mapEntity(ObjectNode objectNode, @Nullable ElasticsearchPersistentEntity<?> entity,
boolean isRootObject, String nestedObjectFieldName, boolean nestedOrObjectField, FieldType fieldType,
boolean isRootObject, String nestedObjectFieldName, boolean nestedOrObjectField, FieldType fieldType, String mappedName,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
boolean isRootObject, String nestedObjectFieldName, boolean nestedOrObjectField, FieldType fieldType, String mappedName,
boolean isRootObject, String nestedObjectFieldName, boolean nestedOrObjectField, FieldType fieldType, String fieldTypemappedName,

might be mistaken as as mapped name of the entity

src/main/java/org/springframework/data/elasticsearch/annotations/Field.java Outdated
*
* @since 5.4
*/
String mappedName() default "";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
String mappedName() default "";
String mappedTypeName() default "";

to not confuse with the field name

@reta
Address code review comments
8ae857f 
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
@reta
Copy link
Contributor Author

reta commented Aug 5, 2024

Thanks for the PR, I just think that the mappedName should be called mappedTypeName so that it will not be confused with the field name, that is mapped as well from the Java property name to the Field.name attribute

Thanks a lot for the review @sothawo , comments addressed! Thanks again!

@sothawo sothawo merged commit 06de217 into spring-projects:main Aug 6, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sothawo sothawo sothawo requested changes

Assignees
No one assigned
Labels
status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEATURE PROPOSAL] Allow to customize the mapped type for @InnerField and @Field annotations
3 participants
@reta @sothawo @spring-projects-issues