Allow fields with id-property names (#1681)

Original Pull Request #1681 
Closes #1680

Author: sothawo

src/main/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentProperty.java

@@ -22,9 +22,7 @@
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
-import org.springframework.data.annotation.Id;
 import org.springframework.data.elasticsearch.annotations.DateFormat;
-import org.springframework.data.elasticsearch.annotations.Document;
 import org.springframework.data.elasticsearch.annotations.Field;
 import org.springframework.data.elasticsearch.annotations.FieldType;
 import org.springframework.data.elasticsearch.annotations.GeoPointField;
@@ -83,16 +81,8 @@ public SimpleElasticsearchPersistentProperty(Property property,
 		this.annotatedFieldName = getAnnotatedFieldName();
 		this.fieldNamingStrategy = fieldNamingStrategy == null ? PropertyNameFieldNamingStrategy.INSTANCE
 				: fieldNamingStrategy;
-		this.isId = super.isIdProperty() || SUPPORTED_ID_PROPERTY_NAMES.contains(getFieldName());
-
-		// deprecated since 4.1
-		@Deprecated
-		boolean isIdWithoutAnnotation = isId && !isAnnotationPresent(Id.class);
-		if (isIdWithoutAnnotation && owner.isAnnotationPresent(Document.class)) {
-			LOGGER.warn("Using the property name of '{}' to identify the id property is deprecated."
-					+ " Please annotate the id property with '@Id'", owner.getName() + "." + getName());
-		}
-
+		this.isId = super.isIdProperty()
+				|| (SUPPORTED_ID_PROPERTY_NAMES.contains(getFieldName()) && !hasExplicitFieldName());
 		this.isParent = isAnnotationPresent(Parent.class);
 		this.isSeqNoPrimaryTerm = SeqNoPrimaryTerm.class.isAssignableFrom(getRawType());
 
@@ -141,6 +131,10 @@ public boolean storeNullValue() {
 		return storeNullValue;
 	}
 
+	protected boolean hasExplicitFieldName() {
+		return StringUtils.hasText(getAnnotatedFieldName());
+	}
+
 	/**
 	 * Initializes an {@link ElasticsearchPersistentPropertyConverter} if this property is annotated as a Field with type
 	 * {@link FieldType#Date}, has a {@link DateFormat} set and if the type of the property is one of the Java8 temporal

src/test/java/org/springframework/data/elasticsearch/core/index/MappingContextBaseTests.java renamed to src/test/java/org/springframework/data/elasticsearch/core/MappingContextBaseTests.java

@@ -13,18 +13,19 @@
  * limitations under the License.
  */
 
-package org.springframework.data.elasticsearch.core.index;
+package org.springframework.data.elasticsearch.core;
 
 import org.springframework.data.elasticsearch.config.ElasticsearchConfigurationSupport;
 import org.springframework.data.elasticsearch.core.convert.ElasticsearchConverter;
 import org.springframework.data.elasticsearch.core.convert.MappingElasticsearchConverter;
+import org.springframework.data.elasticsearch.core.index.MappingBuilder;
 import org.springframework.data.elasticsearch.core.mapping.SimpleElasticsearchMappingContext;
 import org.springframework.data.util.Lazy;
 
 /**
  * @author Peter-Josef Meisch
  */
-abstract class MappingContextBaseTests {
+public abstract class MappingContextBaseTests {
 
 	protected final Lazy<ElasticsearchConverter> elasticsearchConverter = Lazy.of(this::setupElasticsearchConverter);
 
@@ -41,7 +42,7 @@ private SimpleElasticsearchMappingContext setupMappingContext() {
 		return mappingContext;
 	}
 
-	final MappingBuilder getMappingBuilder() {
+	final protected MappingBuilder getMappingBuilder() {
 		return new MappingBuilder(elasticsearchConverter.get());
 	}
 }

src/test/java/org/springframework/data/elasticsearch/core/index/MappingBuilderIntegrationTests.java

@@ -53,6 +53,7 @@
 import org.springframework.data.elasticsearch.annotations.*;
 import org.springframework.data.elasticsearch.core.ElasticsearchOperations;
 import org.springframework.data.elasticsearch.core.IndexOperations;
+import org.springframework.data.elasticsearch.core.MappingContextBaseTests;
 import org.springframework.data.elasticsearch.core.SearchHits;
 import org.springframework.data.elasticsearch.core.completion.Completion;
 import org.springframework.data.elasticsearch.core.geo.GeoPoint;

src/test/java/org/springframework/data/elasticsearch/core/index/MappingBuilderUnitTests.java

@@ -47,6 +47,7 @@
 import org.springframework.data.annotation.Id;
 import org.springframework.data.annotation.Transient;
 import org.springframework.data.elasticsearch.annotations.*;
+import org.springframework.data.elasticsearch.core.MappingContextBaseTests;
 import org.springframework.data.elasticsearch.core.completion.Completion;
 import org.springframework.data.elasticsearch.core.geo.GeoPoint;
 import org.springframework.data.elasticsearch.core.query.SeqNoPrimaryTerm;

src/test/java/org/springframework/data/elasticsearch/core/index/MappingParametersTest.java

@@ -11,6 +11,7 @@
 import org.springframework.data.elasticsearch.annotations.FieldType;
 import org.springframework.data.elasticsearch.annotations.InnerField;
 import org.springframework.data.elasticsearch.annotations.MultiField;
+import org.springframework.data.elasticsearch.core.MappingContextBaseTests;
 import org.springframework.data.elasticsearch.core.mapping.ElasticsearchPersistentEntity;
 import org.springframework.lang.Nullable;
 

src/test/java/org/springframework/data/elasticsearch/core/index/SimpleDynamicTemplatesMappingTests.java

@@ -26,6 +26,7 @@
 import org.springframework.data.elasticsearch.annotations.DynamicTemplates;
 import org.springframework.data.elasticsearch.annotations.Field;
 import org.springframework.data.elasticsearch.annotations.FieldType;
+import org.springframework.data.elasticsearch.core.MappingContextBaseTests;
 import org.springframework.lang.Nullable;
 
 /**

src/test/java/org/springframework/data/elasticsearch/core/index/SimpleElasticsearchDateMappingTests.java

@@ -20,16 +20,15 @@
 
 import lombok.Data;
 
-import java.io.IOException;
 import java.time.LocalDateTime;
-import java.util.Date;
 
 import org.junit.jupiter.api.Test;
 import org.springframework.data.annotation.Id;
 import org.springframework.data.elasticsearch.annotations.DateFormat;
 import org.springframework.data.elasticsearch.annotations.Document;
 import org.springframework.data.elasticsearch.annotations.Field;
 import org.springframework.data.elasticsearch.annotations.FieldType;
+import org.springframework.data.elasticsearch.core.MappingContextBaseTests;
 
 /**
  * @author Jakub Vavrik
@@ -41,8 +40,7 @@ public class SimpleElasticsearchDateMappingTests extends MappingContextBaseTests
 
 	private static final String EXPECTED_MAPPING = "{\"properties\":{\"message\":{\"store\":true,"
 			+ "\"type\":\"text\",\"index\":false,\"analyzer\":\"standard\"},\"customFormatDate\":{\"type\":\"date\",\"format\":\"dd.MM.uuuu hh:mm\"},"
-			+ "\"basicFormatDate\":{\""
-			+ "type\":\"date\",\"format\":\"basic_date\"}}}";
+			+ "\"basicFormatDate\":{\"" + "type\":\"date\",\"format\":\"basic_date\"}}}";
 
 	@Test // DATAES-568, DATAES-828
 	public void testCorrectDateMappings() {
@@ -56,8 +54,7 @@ public void testCorrectDateMappings() {
 	 * @author Jakub Vavrik
 	 */
 	@Data
-	@Document(indexName = "test-index-date-mapping-core", replicas = 0,
-			refreshInterval = "-1")
+	@Document(indexName = "test-index-date-mapping-core", replicas = 0, refreshInterval = "-1")
 	static class SampleDateMappingEntity {
 
 		@Id private String id;

src/test/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntityTests.java

@@ -17,10 +17,14 @@
 
 import static org.assertj.core.api.Assertions.*;
 
+import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Test;
 import org.springframework.data.annotation.Id;
 import org.springframework.data.annotation.Version;
+import org.springframework.data.elasticsearch.annotations.Document;
 import org.springframework.data.elasticsearch.annotations.Field;
+import org.springframework.data.elasticsearch.annotations.FieldType;
+import org.springframework.data.elasticsearch.core.MappingContextBaseTests;
 import org.springframework.data.elasticsearch.core.query.SeqNoPrimaryTerm;
 import org.springframework.data.mapping.MappingException;
 import org.springframework.data.mapping.model.Property;
@@ -38,7 +42,7 @@
  * @author Peter-Josef Meisch
  * @author Roman Puchkovskiy
  */
-public class SimpleElasticsearchPersistentEntityTests {
+public class SimpleElasticsearchPersistentEntityTests extends MappingContextBaseTests {
 
 	@Test
 	public void shouldThrowExceptionGivenVersionPropertyIsNotLong() {
@@ -135,6 +139,12 @@ void shouldNotAllowMoreThanOneSeqNoPrimaryTermProperties() {
 				.isInstanceOf(MappingException.class);
 	}
 
+	@Test // #1680
+	@DisplayName("should allow fields with id property names")
+	void shouldAllowFieldsWithIdPropertyNames() {
+		elasticsearchConverter.get().getMappingContext().getRequiredPersistentEntity(EntityWithIdNameFields.class);
+	}
+
 	private static SimpleElasticsearchPersistentProperty createProperty(SimpleElasticsearchPersistentEntity<?> entity,
 			String field) {
 
@@ -193,4 +203,11 @@ private static class EntityWithSeqNoPrimaryTerm {
 		private SeqNoPrimaryTerm seqNoPrimaryTerm;
 		private SeqNoPrimaryTerm seqNoPrimaryTerm2;
 	}
+
+	@Document(indexName = "fieldnames")
+	private static class EntityWithIdNameFields {
+		@Id private String theRealId;
+		@Field(type = FieldType.Text, name = "document") private String document;
+		@Field(name = "id") private String renamedId;
+	}
 }