diff --git a/src/main/java/org/springframework/data/elasticsearch/core/convert/MappingConversionException.java b/src/main/java/org/springframework/data/elasticsearch/core/convert/MappingConversionException.java new file mode 100644 index 000000000..7f6cd7b59 --- /dev/null +++ b/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; + } +} diff --git a/src/main/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverter.java b/src/main/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverter.java index c827b5e6d..68808a711 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverter.java +++ b/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 readEntity(ElasticsearchPersistentEntity entity, Map 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 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 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); } diff --git a/src/test/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverterUnitTests.java b/src/test/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverterUnitTests.java index fc3191495..aacf044c8 100644 --- a/src/test/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverterUnitTests.java +++ b/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 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> getIntegerRangeList() { public void setIntegerRangeList(List> 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 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;