DATAES-568 - MappingBuilder must use the @Field annotation's name att… #281

sothawo · 2019-04-30T20:59:24Z

…ribute.

I changed the MappingBuilder to use the ElasticsearchPersistentEntity and ElasticsearchPersistentProperty classes instead of doing it's own reflection on the class and fields. Using these we can now use the getFieldname() of the property to get the customized field name.

The logic of the MappingBuilder itself was not changed besides the different field name. What need to be changed was the call to setup the MappingBuilder - which now is an object instead of a collection of static functions - therefore there are some necessary changes in the Template classes and the test setups.

You have read the Spring Data contribution guidelines.
There is a ticket in the bug tracker for the project in our JIRA.
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).

sothawo · 2019-04-30T21:01:18Z

@xhaggi , @christophstrobl , @mp911de if yould hav a look please

mp911de

Did a review pass from a code perspective and not so much from a functional one and left a few comments. That's more food for thought than real issues.

mp911de · 2019-05-03T14:44:22Z

src/main/java/org/springframework/data/elasticsearch/core/ElasticsearchRestTemplate.java

-
-			xContentBuilder = buildMapping(clazz, persistentEntity.getIndexType(), property.getFieldName(),
-					persistentEntity.getParentType());
+			final MappingBuilder mappingBuilder = new MappingBuilder(elasticsearchConverter);


Nit: No need for final variables.

mp911de · 2019-05-03T14:44:38Z

src/main/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplate.java

-
-			xContentBuilder = buildMapping(clazz, persistentEntity.getIndexType(), property.getFieldName(),
-					persistentEntity.getParentType());
+			final MappingBuilder mappingBuilder = new MappingBuilder(elasticsearchConverter);


Nit: No need for final variables.

mp911de · 2019-05-03T14:48:03Z

src/main/java/org/springframework/data/elasticsearch/core/MappingBuilder.java

 		}

-		for (java.lang.reflect.Field field : fields) {
+		if (entity != null) {


Calling getRequiredPersistentEntity(…) in the calling code guarantees that entity is not null.

On a related note: We typically annotate fields/parameters/methods with @org.springframework.lang.Nullable to indicate a field/parameter can be null or whether a method can return null. You get proper IDE support for @Nullable with IntelliJ IDEA and Eclipse.

for the initial call this is true. But on the recursive call, the entity can be null, for example on processing the StockPrice entity in the MappingBuilder.shouldUseValueFromAnnotationType()test.

Wouldn't we need a package-info.java with the @org.springframework.lang.NonNullApi in it? I was thinking about that, but adding it to the packages I changed would have meant that I impose this on all the existing classes and therefore I refrained from it.

Nevertheless, I changed the nullable arguments in the MappingBuilder methods accordingly.

You're right, a package-info.java controls nullability defaults.

mp911de · 2019-05-03T14:48:40Z

src/main/java/org/springframework/data/elasticsearch/core/MappingBuilder.java

-			if (isGeoPointField) {
-				applyGeoPointFieldMapping(xContentBuilder, field);
-			}
+						final Iterator<? extends TypeInformation<?>> iterator = property.getPersistentEntityTypes().iterator();


Nit: No need for final variables.

mp911de · 2019-05-03T14:50:07Z

src/main/java/org/springframework/data/elasticsearch/core/MappingBuilder.java

-				applyGeoPointFieldMapping(xContentBuilder, field);
-			}
+						final Iterator<? extends TypeInformation<?>> iterator = property.getPersistentEntityTypes().iterator();
+						final ElasticsearchPersistentEntity<?> persistentEntity = iterator.hasNext()


Related: You can use PersistentProperty.isEntity() to check whether a property references an entity.

yes, but to get the actual entity type there seems to be no more elegant way

mp911de · 2019-05-03T14:54:36Z

src/main/java/org/springframework/data/elasticsearch/core/MappingBuilder.java

-	private static boolean isIdField(java.lang.reflect.Field field, String idFieldName) {
-		return idFieldName.equals(field.getName());
-	}
+	private boolean isInIgnoreFields(ElasticsearchPersistentProperty property, Field parentFieldAnnotation) {


Nit: Annotate nullable parameters with @Nullable.

mp911de · 2019-05-03T14:56:18Z

src/test/java/org/springframework/data/elasticsearch/core/MappingBuilderTests.java

 */
 @RunWith(SpringJUnit4ClassRunner.class)
 @ContextConfiguration("classpath:elasticsearch-template-test.xml")
-public class MappingBuilderTests {
+public class MappingBuilderTests extends MappingContextBaseTests {


How about using JSON matchers/JSON extensions to verify JSON payload? (e.g. JSONAssert)

changed all the JSON comparisons to JSONAssert, simplified some tests

mp911de · 2019-05-03T14:57:57Z

src/test/java/org/springframework/data/elasticsearch/entities/FieldNameEntity.java

+/**
+ * @author Peter-Josef Meisch
+ */
+public class FieldNameEntity {


Let's see whether we can keep these as little visible as possible. Visibility in tests often leads to usage in other tests and this caused coupling.

I agree, but I kept to the existing package structure, where all the entities are in one package, and the tests in the packages that match the classes to test. I move the FieldNameEntity as a nested class in the test itself, but that would make the test class even larger.

Or we could move the MappingBuilder, the tests and test entities to a separate package; org.springframework.data.elasticsearch.core.mapping wouldn't be appropriate as this is not the spring data mapping, but the Elasticsearch mapping definition, or do you think this package would do?

mp911de · 2019-05-03T14:59:00Z

src/test/java/org/springframework/data/elasticsearch/core/MappingContextBaseTests.java

+/**
+ * @author Peter-Josef Meisch
+ */
+public abstract class MappingContextBaseTests {


Let's keep this class package private.

mp911de · 2019-05-03T14:59:31Z

src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchRestTemplateTests.java

 */
 @RunWith(SpringJUnit4ClassRunner.class)
 @ContextConfiguration("classpath:elasticsearch-rest-template-test.xml")
 public class ElasticsearchRestTemplateTests extends ElasticsearchTemplateTests {

+	@Before
+	public void before() {


I moved this to the test base class ElasticsearchTemplateTests

mp911de · 2019-05-04T14:20:14Z

...g/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentProperty.java

+	@Override
+	public boolean isEntity() {
+
+		TypeInformation<?> typeInformation = getTypeInformation();


This is exactly what AbstractPersistentProperty.isEntity() is doing. See https://github.com/spring-projects/spring-data-commons/blob/48c9297118e18273a0a3dfe3cf2f9a8ffd8fdca7/src/main/java/org/springframework/data/mapping/model/AbstractPersistentProperty.java#L88-L91 and https://github.com/spring-projects/spring-data-commons/blob/48c9297118e18273a0a3dfe3cf2f9a8ffd8fdca7/src/main/java/org/springframework/data/mapping/model/AbstractPersistentProperty.java#L247.

My previous comment should indicate to just use isEntity() so you can get rid of the code duplication.

SimpleTypeHolder is configurable so registered converters are considered (for e.g. date/time types and custom types that are handled using a converter).

aargh, I only looked at the base implementation in AbstractPersistentProperty but forgot to check the implementation of the lazy get(); therefore I thought it was different.

…ribute.

mp911de · 2019-05-06T13:59:13Z

That's merged and polished now.

…ribute. Original pull request: #281.

Add package-info and nullability annotations to org.springframework.data.elasticsearch.core.mapping. Extract method to avoid excessive nesting. Add ticket references/convert old references to test methods. Move test models to inner classes. Use static imports for JSON Assert. Formatting. Original pull request: #281.

mp911de · 2019-05-06T14:21:26Z

After upgrading to ES 6.7, we're getting lots of test errors like:

build	06-May-2019 14:04:00		Suppressed: org.elasticsearch.client.ResponseException: method [PUT], host [http://127.0.0.1:9200], URI [/test-index-sample/_mapping/test-type?master_timeout=30s&include_type_name=true&timeout=30s], status line [HTTP/1.1 400 Bad Request]
build	06-May-2019 14:04:00	Warnings: [[types removal] Specifying types in put mapping requests is deprecated. To be compatible with 7.0, the mapping definition should not be nested under the type name, and the parameter include_type_name must be provided and set to false.]
build	06-May-2019 14:04:00	{"error":{"root_cause":[{"type":"illegal_argument_exception","reason":"Mapper for [message] conflicts with existing mapping in other types:\n[mapper [message] has different [store] values]"}],"type":"illegal_argument_exception","reason":"Mapper for [message] conflicts with existing mapping in other types:\n[mapper [message] has different [store] values]"},"status":400}

See e700ae6 and https://build.spring.io/download/SPRINGDATAES-DATAES-JOB1/build_logs/SPRINGDATAES-DATAES-JOB1-2522.log.

mp911de · 2019-05-06T14:22:10Z

Reverted master to git to 01cda35.

Add package-info and nullability annotations to org.springframework.data.elasticsearch.core.mapping. Extract method to avoid excessive nesting. Add ticket references/convert old references to test methods. Move test models to inner classes. Use static imports for JSON Assert. Formatting. Original pull request: #281.

mp911de · 2019-05-06T20:03:58Z

Seems the test issues above are just a consequence of adding tests/test reordering. I'll retry 6.7.2 upgrade tomorrow. See DATAES-576 for further details about test failures.

mp911de reviewed May 3, 2019

View reviewed changes

sothawo force-pushed the issue/DATAES-568 branch from cb9fed8 to 7c47b14 Compare May 3, 2019 19:59

mp911de reviewed May 4, 2019

View reviewed changes

sothawo force-pushed the issue/DATAES-568 branch 2 times, most recently from 384c978 to 1fcecc0 Compare May 4, 2019 16:41

DATAES-568 - MappingBuilder must use the @field annotation's name att…

12edf1f

…ribute.

sothawo force-pushed the issue/DATAES-568 branch from 1fcecc0 to 12edf1f Compare May 4, 2019 16:53

mp911de closed this May 6, 2019

mp911de pushed a commit that referenced this pull request May 6, 2019

DATAES-568 - MappingBuilder must use the @field annotation's name att…

66b77ec

…ribute. Original pull request: #281.

sothawo deleted the issue/DATAES-568 branch May 6, 2019 16:40

sothawo mentioned this pull request Oct 23, 2019

DATAES-20 #145

Closed

6 tasks

spring-projects-issues mentioned this pull request Dec 31, 2020

MappingBuilder must use the @Field annotation's name attribute [DATAES-568] #1140

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DATAES-568 - MappingBuilder must use the @Field annotation's name att… #281

DATAES-568 - MappingBuilder must use the @Field annotation's name att… #281

sothawo commented Apr 30, 2019

sothawo commented Apr 30, 2019

mp911de left a comment

mp911de May 3, 2019

sothawo May 3, 2019

mp911de May 3, 2019

sothawo May 3, 2019

mp911de May 3, 2019

sothawo May 3, 2019

mp911de May 4, 2019

mp911de May 3, 2019

sothawo May 3, 2019

mp911de May 3, 2019

sothawo May 3, 2019

mp911de May 3, 2019

sothawo May 3, 2019

mp911de May 3, 2019

sothawo May 3, 2019

mp911de May 3, 2019

sothawo May 3, 2019

mp911de May 3, 2019

sothawo May 3, 2019

mp911de May 3, 2019

sothawo May 4, 2019

mp911de May 4, 2019 •

edited

Loading

sothawo May 4, 2019

mp911de commented May 6, 2019

mp911de commented May 6, 2019

mp911de commented May 6, 2019

mp911de commented May 6, 2019

DATAES-568 - MappingBuilder must use the @Field annotation's name att… #281

DATAES-568 - MappingBuilder must use the @Field annotation's name att… #281

Conversation

sothawo commented Apr 30, 2019

sothawo commented Apr 30, 2019

mp911de left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mp911de May 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mp911de commented May 6, 2019

mp911de commented May 6, 2019

mp911de commented May 6, 2019

mp911de commented May 6, 2019

mp911de May 4, 2019 •

edited

Loading