Introduce MappingConversionException.

Original Pull Request #2882
Closes #2879

Author: sothawo

src/main/java/org/springframework/data/elasticsearch/core/convert/MappingConversionException.java

@@ -0,0 +1,40 @@
+/*
+ * Copyright 2024 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.springframework.data.elasticsearch.core.convert;
+
+import org.springframework.lang.Nullable;
+
+/**
+ * @since 5.3
+ * @author Peter-Josef Meisch
+ */
+public class MappingConversionException extends RuntimeException {
+	private final String documentId;
+
+	public MappingConversionException(@Nullable String documentId, Throwable cause) {
+		super(cause);
+		this.documentId = documentId != null ? documentId : "\"null\"";
+	}
+
+	public String getDocumentId() {
+		return documentId;
+	}
+
+	@Override
+	public String getMessage() {
+		return "Conversion exception when converting document id " + documentId;
+	}
+}

src/main/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverter.java

@@ -65,6 +65,8 @@
 import org.springframework.util.CollectionUtils;
 import org.springframework.util.ObjectUtils;
 
+import javax.print.Doc;
+
 /**
  * Elasticsearch specific {@link org.springframework.data.convert.EntityConverter} implementation based on domain type
  * {@link ElasticsearchPersistentEntity metadata}.
@@ -333,46 +335,52 @@ private <R> R readEntity(ElasticsearchPersistentEntity<?> entity, Map<String, Ob
 				return instance;
 			}
 
+			Document document = (source instanceof Document) ? (Document) source : null;
+
 			ElasticsearchPropertyValueProvider valueProvider = new ElasticsearchPropertyValueProvider(accessor, evaluator);
-			R result = readProperties(targetEntity, instance, valueProvider);
-
-			if (source instanceof Document document) {
-				if (document.hasId()) {
-					ElasticsearchPersistentProperty idProperty = targetEntity.getIdProperty();
-					PersistentPropertyAccessor<R> propertyAccessor = new ConvertingPropertyAccessor<>(
-							targetEntity.getPropertyAccessor(result), conversionService);
-					// Only deal with String because ES generated Ids are strings !
-					if (idProperty != null && idProperty.isReadable() && idProperty.getType().isAssignableFrom(String.class)) {
-						propertyAccessor.setProperty(idProperty, document.getId());
+			try {
+				R result = readProperties(targetEntity, instance, valueProvider);
+
+				if (document != null) {
+					if (document.hasId()) {
+						ElasticsearchPersistentProperty idProperty = targetEntity.getIdProperty();
+						PersistentPropertyAccessor<R> propertyAccessor = new ConvertingPropertyAccessor<>(
+								targetEntity.getPropertyAccessor(result), conversionService);
+						// Only deal with String because ES generated Ids are strings !
+						if (idProperty != null && idProperty.isReadable() && idProperty.getType().isAssignableFrom(String.class)) {
+							propertyAccessor.setProperty(idProperty, document.getId());
+						}
 					}
-				}
 
-				if (document.hasVersion()) {
-					long version = document.getVersion();
-					ElasticsearchPersistentProperty versionProperty = targetEntity.getVersionProperty();
-					// Only deal with Long because ES versions are longs !
-					if (versionProperty != null && versionProperty.getType().isAssignableFrom(Long.class)) {
-						// check that a version was actually returned in the response, -1 would indicate that
-						// a search didn't request the version ids in the response, which would be an issue
-						Assert.isTrue(version != -1, "Version in response is -1");
-						targetEntity.getPropertyAccessor(result).setProperty(versionProperty, version);
+					if (document.hasVersion()) {
+						long version = document.getVersion();
+						ElasticsearchPersistentProperty versionProperty = targetEntity.getVersionProperty();
+						// Only deal with Long because ES versions are longs !
+						if (versionProperty != null && versionProperty.getType().isAssignableFrom(Long.class)) {
+							// check that a version was actually returned in the response, -1 would indicate that
+							// a search didn't request the version ids in the response, which would be an issue
+							Assert.isTrue(version != -1, "Version in response is -1");
+							targetEntity.getPropertyAccessor(result).setProperty(versionProperty, version);
+						}
 					}
-				}
 
-				if (targetEntity.hasSeqNoPrimaryTermProperty() && document.hasSeqNo() && document.hasPrimaryTerm()) {
-					if (isAssignedSeqNo(document.getSeqNo()) && isAssignedPrimaryTerm(document.getPrimaryTerm())) {
-						SeqNoPrimaryTerm seqNoPrimaryTerm = new SeqNoPrimaryTerm(document.getSeqNo(), document.getPrimaryTerm());
-						ElasticsearchPersistentProperty property = targetEntity.getRequiredSeqNoPrimaryTermProperty();
-						targetEntity.getPropertyAccessor(result).setProperty(property, seqNoPrimaryTerm);
+					if (targetEntity.hasSeqNoPrimaryTermProperty() && document.hasSeqNo() && document.hasPrimaryTerm()) {
+						if (isAssignedSeqNo(document.getSeqNo()) && isAssignedPrimaryTerm(document.getPrimaryTerm())) {
+							SeqNoPrimaryTerm seqNoPrimaryTerm = new SeqNoPrimaryTerm(document.getSeqNo(), document.getPrimaryTerm());
+							ElasticsearchPersistentProperty property = targetEntity.getRequiredSeqNoPrimaryTermProperty();
+							targetEntity.getPropertyAccessor(result).setProperty(property, seqNoPrimaryTerm);
+						}
 					}
 				}
-			}
 
-			if (source instanceof SearchDocument searchDocument) {
-				populateScriptFields(targetEntity, result, searchDocument);
+				if (source instanceof SearchDocument searchDocument) {
+					populateScriptFields(targetEntity, result, searchDocument);
+				}
+				return result;
+			} catch (ConversionException e) {
+				String documentId = (document != null && document.hasId()) ? document.getId() : null;
+				throw new MappingConversionException(documentId, e);
 			}
-
-			return result;
 		}
 
 		private ParameterValueProvider<ElasticsearchPersistentProperty> getParameterProvider(
@@ -510,11 +518,9 @@ private Object propertyConverterRead(ElasticsearchPersistentProperty property, O
 			}
 
 			if (source instanceof List<?> list) {
-				source = list.stream().map(it -> convertOnRead(propertyValueConverter, it))
-						.collect(Collectors.toList());
+				source = list.stream().map(it -> convertOnRead(propertyValueConverter, it)).collect(Collectors.toList());
 			} else if (source instanceof Set<?> set) {
-				source = set.stream().map(it -> convertOnRead(propertyValueConverter, it))
-						.collect(Collectors.toSet());
+				source = set.stream().map(it -> convertOnRead(propertyValueConverter, it)).collect(Collectors.toSet());
 			} else {
 				source = convertOnRead(propertyValueConverter, source);
 			}

src/test/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverterUnitTests.java

@@ -228,6 +228,13 @@ public void init() {
 				"org.springframework.data.elasticsearch.core.convert.MappingElasticsearchConverterUnitTests$Notification");
 	}
 
+	private Map<String, Object> writeToMap(Object source) {
+
+		Document sink = Document.create();
+		mappingElasticsearchConverter.write(source, sink);
+		return sink;
+	}
+
 	@Test
 	public void shouldFailToInitializeGivenMappingContextIsNull() {
 
@@ -993,7 +1000,7 @@ class RangeTests {
 				  "nullRange": null,
 				  "integerRangeList": [
 				  	{
-				  	  "gte": "2", 
+				  	  "gte": "2",
 				  	  "lte": "5"
 				  	}
 				  ]
@@ -1178,11 +1185,13 @@ public List<Range<Integer>> getIntegerRangeList() {
 			public void setIntegerRangeList(List<Range<Integer>> integerRangeList) {
 				this.integerRangeList = integerRangeList;
 			}
+
 		}
 	}
 
 	@Nested
 	class GeoJsonUnitTests {
+
 		private GeoJsonEntity entity;
 
 		@BeforeEach
@@ -1476,6 +1485,7 @@ void shouldReadGeoJsonProperties() {
 
 			assertThat(entity).isEqualTo(mapped);
 		}
+
 	}
 
 	@Test // #1454
@@ -1942,13 +1952,6 @@ void shouldReadAnObjectArrayIntoASetPropertyImmutable() {
 		assertThat(names).containsExactlyInAnyOrder("child1", "child2");
 	}
 
-	private Map<String, Object> writeToMap(Object source) {
-
-		Document sink = Document.create();
-		mappingElasticsearchConverter.write(source, sink);
-		return sink;
-	}
-
 	@Test // #2364
 	@DisplayName("should not write id property to document source if configured so")
 	void shouldNotWriteIdPropertyToDocumentSourceIfConfiguredSo() throws JSONException {
@@ -2078,6 +2081,25 @@ void shouldMapPropertyPathToFieldNames() {
 		assertThat(mappedNames).isEqualTo("level-one.level-two.key-word");
 	}
 
+	@Test // #2879
+	@DisplayName("should throw MappingConversionException with document id on reading error")
+	void shouldThrowMappingConversionExceptionWithDocumentIdOnReadingError() {
+
+		@Language("JSON")
+		String json = """
+				{
+				  "birth-date": "this-is-not-a-local-date"
+				}""";
+
+		Document document = Document.parse(json);
+		document.setId("42");
+
+		assertThatThrownBy(() -> {
+			mappingElasticsearchConverter.read(Person.class, document);
+		}).isInstanceOf(MappingConversionException.class).hasFieldOrPropertyWithValue("documentId", "42")
+				.hasCauseInstanceOf(ConversionException.class);
+	}
+
 	// region entities
 	public static class Sample {
 		@Nullable public @ReadOnlyProperty String readOnly;