DATAES-924 - Conversion of properties of collections of Temporal values fails.

Original PR: #519

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

@@ -16,18 +16,10 @@
 package org.springframework.data.elasticsearch.core.convert;
 
 import java.time.temporal.TemporalAccessor;
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.Collection;
-import java.util.Collections;
-import java.util.Iterator;
-import java.util.LinkedHashMap;
-import java.util.List;
-import java.util.Map;
+import java.util.*;
 import java.util.Map.Entry;
-import java.util.Optional;
-import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.stream.Collectors;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -281,8 +273,8 @@ public class MappingElasticsearchConverter
 
 		Class<R> rawType = targetType.getType();
 
-		if (property.hasPropertyConverter() && String.class.isAssignableFrom(source.getClass())) {
-			source = property.getPropertyConverter().read((String) source);
+		if (property.hasPropertyConverter()) {
+			source = propertyConverterRead(property, source);
 		} else if (TemporalAccessor.class.isAssignableFrom(property.getType())
 				&& !conversions.hasCustomReadTarget(source.getClass(), rawType)) {
 
@@ -310,6 +302,32 @@ public class MappingElasticsearchConverter
 		return (R) readSimpleValue(source, targetType);
 	}
 
+	private Object propertyConverterRead(ElasticsearchPersistentProperty property, Object source) {
+		ElasticsearchPersistentPropertyConverter propertyConverter = Objects
+				.requireNonNull(property.getPropertyConverter());
+
+		if (source instanceof String[]) {
+			// convert to a List
+			source = Arrays.asList((String[]) source);
+		}
+
+		if (source instanceof List) {
+			source = ((List<?>) source).stream().map(it -> convertOnRead(propertyConverter, it)).collect(Collectors.toList());
+		} else if (source instanceof Set) {
+			source = ((Set<?>) source).stream().map(it -> convertOnRead(propertyConverter, it)).collect(Collectors.toSet());
+		} else {
+			source = convertOnRead(propertyConverter, source);
+		}
+		return source;
+	}
+
+	private Object convertOnRead(ElasticsearchPersistentPropertyConverter propertyConverter, Object source) {
+		if (String.class.isAssignableFrom(source.getClass())) {
+			source = propertyConverter.read((String) source);
+		}
+		return source;
+	}
+
 	@SuppressWarnings("unchecked")
 	@Nullable
 	private <R> R readCollectionValue(@Nullable List<?> source, ElasticsearchPersistentProperty property,
@@ -506,9 +524,8 @@ public class MappingElasticsearchConverter
 			}
 
 			if (property.hasPropertyConverter()) {
-				ElasticsearchPersistentPropertyConverter propertyConverter = property.getPropertyConverter();
-				value = propertyConverter.write(value);
-			} else if (TemporalAccessor.class.isAssignableFrom(property.getType())
+				value = propertyConverterWrite(property, value);
+			} else if (TemporalAccessor.class.isAssignableFrom(property.getActualType())
 					&& !conversions.hasCustomWriteTarget(value.getClass())) {
 
 				// log at most 5 times
@@ -535,6 +552,20 @@ public class MappingElasticsearchConverter
 		}
 	}
 
+	private Object propertyConverterWrite(ElasticsearchPersistentProperty property, Object value) {
+		ElasticsearchPersistentPropertyConverter propertyConverter = Objects
+				.requireNonNull(property.getPropertyConverter());
+
+		if (value instanceof List) {
+			value = ((List<?>) value).stream().map(propertyConverter::write).collect(Collectors.toList());
+		} else if (value instanceof Set) {
+			value = ((Set<?>) value).stream().map(propertyConverter::write).collect(Collectors.toSet());
+		} else {
+			value = propertyConverter.write(value);
+		}
+		return value;
+	}
+
 	protected void writeProperty(ElasticsearchPersistentProperty property, Object value, MapValueAccessor sink) {
 
 		Optional<Class<?>> customWriteTarget = conversions.getCustomWriteTarget(value.getClass());

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

@@ -64,7 +64,7 @@ public class SimpleElasticsearchPersistentProperty extends
 	private final boolean isSeqNoPrimaryTerm;
 	private final @Nullable String annotatedFieldName;
 	@Nullable private ElasticsearchPersistentPropertyConverter propertyConverter;
-	private boolean storeNullValue;
+	private final boolean storeNullValue;
 
 	public SimpleElasticsearchPersistentProperty(Property property,
 			PersistentEntity<?, ElasticsearchPersistentProperty> owner, SimpleTypeHolder simpleTypeHolder) {
@@ -143,8 +143,9 @@ public class SimpleElasticsearchPersistentProperty extends
 	 */
 	private void initDateConverter() {
 		Field field = findAnnotation(Field.class);
-		boolean isTemporalAccessor = TemporalAccessor.class.isAssignableFrom(getType());
-		boolean isDate = Date.class.isAssignableFrom(getType());
+		Class<?> actualType = getActualType();
+		boolean isTemporalAccessor = TemporalAccessor.class.isAssignableFrom(actualType);
+		boolean isDate = Date.class.isAssignableFrom(actualType);
 
 		if (field != null && (field.type() == FieldType.Date || field.type() == FieldType.Date_Nanos)
 				&& (isTemporalAccessor || isDate)) {
@@ -156,44 +157,50 @@ public class SimpleElasticsearchPersistentProperty extends
 								getOwner().getType().getSimpleName() + "." + getName(), field.type().name()));
 			}
 
-			ElasticsearchDateConverter converter = null;
+			ElasticsearchDateConverter converter;
 
 			if (dateFormat == DateFormat.custom) {
 				String pattern = field.pattern();
 
-				if (StringUtils.hasLength(pattern)) {
-					converter = ElasticsearchDateConverter.of(pattern);
+				if (!StringUtils.hasLength(pattern)) {
+					throw new MappingException(
+							String.format("Property %s is annotated with FieldType.%s and a custom format but has no pattern defined",
+									getOwner().getType().getSimpleName() + "." + getName(), field.type().name()));
 				}
-			} else if (dateFormat != DateFormat.none) {
+
+				converter = ElasticsearchDateConverter.of(pattern);
+			} else {
 				converter = ElasticsearchDateConverter.of(dateFormat);
 			}
 
-			if (converter != null) {
-				ElasticsearchDateConverter dateConverter = converter;
-				propertyConverter = new ElasticsearchPersistentPropertyConverter() {
-					@Override
-					public String write(Object property) {
-						if (isTemporalAccessor) {
-							return dateConverter.format((TemporalAccessor) property);
-						} else { // must be Date
-							return dateConverter.format((Date) property);
-						}
-					}
+			propertyConverter = new ElasticsearchPersistentPropertyConverter() {
+				final ElasticsearchDateConverter dateConverter = converter;
 
-					@SuppressWarnings("unchecked")
-					@Override
-					public Object read(String s) {
-						if (isTemporalAccessor) {
-							return dateConverter.parse(s, (Class<? extends TemporalAccessor>) getType());
-						} else { // must be date
-							return dateConverter.parse(s);
-						}
+				@Override
+				public String write(Object property) {
+					if (isTemporalAccessor && TemporalAccessor.class.isAssignableFrom(property.getClass())) {
+						return dateConverter.format((TemporalAccessor) property);
+					} else if (isDate && Date.class.isAssignableFrom(property.getClass())) {
+						return dateConverter.format((Date) property);
+					} else {
+						return property.toString();
 					}
-				};
-			}
+				}
+
+				@SuppressWarnings("unchecked")
+				@Override
+				public Object read(String s) {
+					if (isTemporalAccessor) {
+						return dateConverter.parse(s, (Class<? extends TemporalAccessor>) actualType);
+					} else { // must be date
+						return dateConverter.parse(s);
+					}
+				}
+			};
 		}
 	}
 
+	@SuppressWarnings("ConstantConditions")
 	@Nullable
 	private String getAnnotatedFieldName() {
 

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

@@ -26,6 +26,7 @@ import lombok.EqualsAndHashCode;
 import lombok.Getter;
 import lombok.NoArgsConstructor;
 import lombok.RequiredArgsConstructor;
+import lombok.Setter;
 
 import java.time.LocalDate;
 import java.util.ArrayList;
@@ -617,6 +618,25 @@ public class MappingElasticsearchConverterUnitTests {
 		assertEquals(expected, json, false);
 	}
 
+	@Test // DATAES-924
+	@DisplayName("should write list of LocalDate")
+	void shouldWriteListOfLocalDate() throws JSONException {
+
+		LocalDatesEntity entity = new LocalDatesEntity();
+		entity.setId("4711");
+		entity.setDates(Arrays.asList(LocalDate.of(2020, 9, 15), LocalDate.of(2019, 5, 1)));
+		String expected = "{\n" + //
+				"  \"id\": \"4711\",\n" + //
+				"  \"dates\": [\"15.09.2020\", \"01.05.2019\"]\n" + //
+				"}\n"; //
+
+		Document document = Document.create();
+		mappingElasticsearchConverter.write(entity, document);
+		String json = document.toJson();
+
+		assertEquals(expected, json, false);
+	}
+
 	@Test // DATAES-716
 	void shouldReadLocalDate() {
 		Document document = Document.create();
@@ -633,6 +653,20 @@ public class MappingElasticsearchConverterUnitTests {
 		assertThat(person.getGender()).isEqualTo(Gender.MAN);
 	}
 
+	@Test // DATAES-924
+	@DisplayName("should read list of LocalDate")
+	void shouldReadListOfLocalDate() {
+
+		Document document = Document.create();
+		document.put("id", "4711");
+		document.put("dates", new String[] { "15.09.2020", "01.05.2019" });
+
+		LocalDatesEntity entity = mappingElasticsearchConverter.read(LocalDatesEntity.class, document);
+
+		assertThat(entity.getId()).isEqualTo("4711");
+		assertThat(entity.getDates()).hasSize(2).containsExactly(LocalDate.of(2020, 9, 15), LocalDate.of(2019, 5, 1));
+	}
+
 	@Test // DATAES-763
 	void writeEntityWithMapDataType() {
 
@@ -870,6 +904,15 @@ public class MappingElasticsearchConverterUnitTests {
 		Map<String, Inventory> inventoryMap;
 	}
 
+	@Data
+	@Getter
+	@Setter
+	static class LocalDatesEntity {
+		@Id private String id;
+		@Field(name = "dates", type = FieldType.Date, format = DateFormat.custom,
+				pattern = "dd.MM.uuuu") private List<LocalDate> dates;
+	}
+
 	enum Gender {
 
 		MAN("1"), MACHINE("0");

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

@@ -17,13 +17,17 @@ package org.springframework.data.elasticsearch.core.mapping;
 
 import static org.assertj.core.api.Assertions.*;
 
+import lombok.Data;
+
 import java.time.LocalDate;
 import java.time.LocalDateTime;
 import java.time.ZoneId;
 import java.time.ZonedDateTime;
 import java.util.Date;
 import java.util.GregorianCalendar;
+import java.util.List;
 
+import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Test;
 import org.springframework.data.elasticsearch.annotations.DateFormat;
 import org.springframework.data.elasticsearch.annotations.Field;
@@ -86,7 +90,7 @@ public class SimpleElasticsearchPersistentPropertyUnitTests {
 		assertThat(persistentProperty.getFieldName()).isEqualTo("mainfield");
 	}
 
-	@Test // DATAES-716, DATAES-792
+	@Test // DATAES-716, DATAES-792, DATAES-924
 	void shouldSetPropertyConverters() {
 		SimpleElasticsearchPersistentEntity<?> persistentEntity = context.getRequiredPersistentEntity(DatesProperty.class);
 
@@ -102,6 +106,9 @@ public class SimpleElasticsearchPersistentPropertyUnitTests {
 		assertThat(persistentProperty.hasPropertyConverter()).isTrue();
 		assertThat(persistentProperty.getPropertyConverter()).isNotNull();
 
+		persistentProperty = persistentEntity.getRequiredPersistentProperty("localDateList");
+		assertThat(persistentProperty.hasPropertyConverter()).isTrue();
+		assertThat(persistentProperty.getPropertyConverter()).isNotNull();
 	}
 
 	@Test // DATAES-716
@@ -199,6 +206,14 @@ public class SimpleElasticsearchPersistentPropertyUnitTests {
 				.withMessageContaining("date");
 	}
 
+	@Test // DATAES-924
+	@DisplayName("should require pattern for custom date format")
+	void shouldRequirePatternForCustomDateFormat() {
+		assertThatExceptionOfType(MappingException.class) //
+				.isThrownBy(() -> context.getRequiredPersistentEntity(DateFieldWithCustomFormatAndNoPattern.class)) //
+				.withMessageContaining("pattern");
+	}
+
 	static class InvalidScoreProperty {
 		@Nullable @Score String scoreProperty;
 	}
@@ -220,17 +235,27 @@ public class SimpleElasticsearchPersistentPropertyUnitTests {
 		@Nullable @Field(type = FieldType.Date, format = DateFormat.custom, pattern = "dd.MM.uuuu") LocalDate localDate;
 		@Nullable @Field(type = FieldType.Date, format = DateFormat.basic_date_time) LocalDateTime localDateTime;
 		@Nullable @Field(type = FieldType.Date, format = DateFormat.basic_date_time) Date legacyDate;
+		@Nullable @Field(type = FieldType.Date, format = DateFormat.custom,
+				pattern = "dd.MM.uuuu") List<LocalDate> localDateList;
 	}
 
+	@Data
 	static class SeqNoPrimaryTermProperty {
 		SeqNoPrimaryTerm seqNoPrimaryTerm;
 		String string;
 	}
 
+	@Data
 	static class DateFieldWithNoFormat {
 		@Field(type = FieldType.Date) LocalDateTime datetime;
 	}
 
+	@Data
+	static class DateFieldWithCustomFormatAndNoPattern {
+		@Field(type = FieldType.Date, format = DateFormat.custom, pattern = "") LocalDateTime datetime;
+	}
+
+	@Data
 	static class DateNanosFieldWithNoFormat {
 		@Field(type = FieldType.Date_Nanos) LocalDateTime datetime;
 	}