DATACMNS-1259 - Fixed support for Long values in Auditables.

schauder · odrotbohm · commit c6e50d3c8f4f · 2018-02-15T14:36:59.000+01:00
Using Instant as internal data type since it's a point in time without time zone which LocalDateTime isn't. Added necessary converters. Fixed one JodaTime converter that used UTC to use SystemDefault like other similar converters. In case of a conversion failure the error message now contains the source type. Original pull request: #273.
diff --git a/src/main/java/org/springframework/data/auditing/DefaultAuditableBeanWrapperFactory.java b/src/main/java/org/springframework/data/auditing/DefaultAuditableBeanWrapperFactory.java
@@ -19,6 +19,7 @@
 import lombok.RequiredArgsConstructor;
 
 import java.lang.reflect.Field;
+import java.time.Instant;
 import java.time.LocalDateTime;
 import java.time.temporal.TemporalAccessor;
 import java.util.Calendar;
@@ -42,6 +43,7 @@
  * 
  * @author Oliver Gierke
  * @author Christoph Strobl
+ * @author Jens Schauder
  * @since 1.5
  */
 class DefaultAuditableBeanWrapperFactory implements AuditableBeanWrapperFactory {
@@ -207,8 +209,7 @@ protected Object getDateValueToSet(TemporalAccessor value, Class<?> targetType,
 				return conversionService.convert(date, targetType);
 			}
 
-			throw new IllegalArgumentException(String.format("Invalid date type for member %s! Supported types are %s.",
-					source, AnnotationAuditingMetadata.SUPPORTED_DATE_TYPES));
+			throw new IllegalArgumentException(createUnsupportedTypeErrorMessage(source));
 		}
 
 		/**
@@ -228,19 +229,24 @@ protected <T extends TemporalAccessor> Optional<T> getAsTemporalAccessor(Optiona
 					return (T) it;
 				}
 
-				Class<?> typeToConvertTo = Stream.of(target, LocalDateTime.class)//
+				Class<?> typeToConvertTo = Stream.of(target, Instant.class)//
 						.filter(type -> target.isAssignableFrom(type))//
 						.filter(type -> conversionService.canConvert(it.getClass(), type))//
 						.findFirst()
 						.orElseThrow(() -> new IllegalArgumentException(
-								String.format("Invalid date type for member %s! Supported types are %s.", source,
-										AnnotationAuditingMetadata.SUPPORTED_DATE_TYPES)));
+								createUnsupportedTypeErrorMessage(((Optional<Object>)source).orElseGet(() -> source))));
 
 				return (T) conversionService.convert(it, typeToConvertTo);
 			});
 		}
 	}
 
+	private static String createUnsupportedTypeErrorMessage(Object source) {
+
+		return String.format("Invalid date type %s for member %s! Supported types are %s.", source.getClass(), source,
+				AnnotationAuditingMetadata.SUPPORTED_DATE_TYPES);
+	}
+
 	/**
 	 * An {@link AuditableBeanWrapper} implementation that sets values on the target object using reflection.
 	 * 
diff --git a/src/main/java/org/springframework/data/convert/JodaTimeConverters.java b/src/main/java/org/springframework/data/convert/JodaTimeConverters.java
@@ -15,16 +15,19 @@
  */
 package org.springframework.data.convert;
 
+import java.time.Instant;
 import java.time.ZoneId;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Date;
 import java.util.List;
+import java.util.TimeZone;
 
 import javax.annotation.Nonnull;
 
 import org.joda.time.DateTime;
+import org.joda.time.DateTimeZone;
 import org.joda.time.LocalDate;
 import org.joda.time.LocalDateTime;
 import org.springframework.core.convert.converter.Converter;
@@ -65,6 +68,9 @@ public abstract class JodaTimeConverters {
 		converters.add(LocalDateTimeToJodaLocalDateTime.INSTANCE);
 		converters.add(LocalDateTimeToJodaDateTime.INSTANCE);
 
+		converters.add(InstantToJodaLocalDateTime.INSTANCE);
+		converters.add(JodaLocalDateTimeToInstant.INSTANCE);
+
 		converters.add(LocalDateTimeToJsr310Converter.INSTANCE);
 
 		return converters;
@@ -77,7 +83,7 @@ public enum LocalDateTimeToJsr310Converter implements Converter<LocalDateTime, j
 		@Nonnull
 		@Override
 		public java.time.LocalDateTime convert(LocalDateTime source) {
-			return java.time.LocalDateTime.ofInstant(source.toDate().toInstant(), ZoneId.of("UTC"));
+			return java.time.LocalDateTime.ofInstant(source.toDate().toInstant(), ZoneId.systemDefault());
 		}
 	}
 
@@ -158,6 +164,28 @@ public LocalDateTime convert(java.time.LocalDateTime source) {
 		}
 	}
 
+	public enum InstantToJodaLocalDateTime implements Converter<java.time.Instant, LocalDateTime> {
+
+		INSTANCE;
+
+		@Nonnull
+		@Override
+		public LocalDateTime convert(java.time.Instant source) {
+			return LocalDateTime.fromDateFields(new Date(source.toEpochMilli()));
+		}
+	}
+
+	public enum JodaLocalDateTimeToInstant implements Converter<LocalDateTime, Instant> {
+
+		INSTANCE;
+
+		@Nonnull
+		@Override
+		public Instant convert(LocalDateTime source) {
+			return Instant.ofEpochMilli(source.toDateTime().getMillis());
+		}
+	}
+
 	public enum LocalDateTimeToJodaDateTime implements Converter<java.time.LocalDateTime, DateTime> {
 
 		INSTANCE;
diff --git a/src/main/java/org/springframework/data/convert/ThreeTenBackPortConverters.java b/src/main/java/org/springframework/data/convert/ThreeTenBackPortConverters.java
@@ -36,13 +36,15 @@
 import org.threeten.bp.LocalDateTime;
 import org.threeten.bp.LocalTime;
 import org.threeten.bp.ZoneId;
+import org.threeten.bp.ZoneOffset;
 
 /**
  * Helper class to register {@link Converter} implementations for the ThreeTen Backport project in case it's present on
  * the classpath.
  * 
  * @author Oliver Gierke
  * @author Christoph Strobl
+ * @author Jens Schauder
  * @see <a href="http://www.threeten.org/threetenbp">http://www.threeten.org/threetenbp</a>
  * @since 1.10
  */
@@ -74,6 +76,8 @@ public abstract class ThreeTenBackPortConverters {
 		converters.add(ZoneIdToStringConverter.INSTANCE);
 		converters.add(StringToZoneIdConverter.INSTANCE);
 		converters.add(LocalDateTimeToJsr310LocalDateTimeConverter.INSTANCE);
+		converters.add(LocalDateTimeToJavaTimeInstantConverter.INSTANCE);
+		converters.add(JavaTimeInstantToLocalDateTimeConverter.INSTANCE);
 
 		return converters;
 	}
@@ -84,7 +88,7 @@ public static boolean supports(Class<?> type) {
 			return false;
 		}
 
-		return Arrays.<Class<?>> asList(LocalDateTime.class, LocalDate.class, LocalTime.class, Instant.class)
+		return Arrays.<Class<?>> asList(LocalDateTime.class, LocalDate.class, LocalTime.class, Instant.class, java.time.Instant.class)
 				.contains(type);
 	}
 
@@ -195,6 +199,28 @@ public Date convert(Instant source) {
 		}
 	}
 
+	public static enum LocalDateTimeToJavaTimeInstantConverter implements Converter<LocalDateTime, java.time.Instant> {
+
+		INSTANCE;
+
+		@Nonnull
+		@Override
+		public java.time.Instant convert(LocalDateTime source) {
+			return java.time.Instant.ofEpochMilli(source.atZone(ZoneOffset.systemDefault()).toInstant().toEpochMilli());
+		}
+	}
+
+	public static enum JavaTimeInstantToLocalDateTimeConverter implements Converter<java.time.Instant, LocalDateTime> {
+
+		INSTANCE;
+
+		@Nonnull
+		@Override
+		public LocalDateTime convert(java.time.Instant source) {
+			return LocalDateTime.ofInstant(Instant.ofEpochMilli(source.toEpochMilli()), ZoneOffset.systemDefault());
+		}
+	}
+
 	@WritingConverter
 	public static enum ZoneIdToStringConverter implements Converter<ZoneId, String> {
 
diff --git a/src/test/java/org/springframework/data/auditing/DefaultAuditableBeanWrapperFactoryUnitTests.java b/src/test/java/org/springframework/data/auditing/DefaultAuditableBeanWrapperFactoryUnitTests.java
@@ -18,18 +18,27 @@
 import static org.assertj.core.api.Assertions.*;
 
 import java.time.Instant;
+import java.time.LocalDateTime;
+import java.time.ZoneOffset;
 import java.time.ZonedDateTime;
+import java.time.temporal.ChronoField;
+import java.time.temporal.TemporalAccessor;
+import java.time.temporal.TemporalField;
 import java.util.Optional;
 
 import org.junit.Test;
+import org.springframework.data.annotation.CreatedDate;
+import org.springframework.data.annotation.LastModifiedDate;
 import org.springframework.data.auditing.DefaultAuditableBeanWrapperFactory.AuditableInterfaceBeanWrapper;
 import org.springframework.data.auditing.DefaultAuditableBeanWrapperFactory.ReflectionAuditingBeanWrapper;
+import org.springframework.data.domain.Auditable;
 
 /**
  * Unit tests for {@link DefaultAuditableBeanWrapperFactory}.
  * 
  * @author Oliver Gierke
  * @author Christoph Strobl
+ * @author Jens Schauder
  * @since 1.5
  */
 public class DefaultAuditableBeanWrapperFactoryUnitTests {
@@ -88,4 +97,70 @@ public void errorsWhenUnableToConvertDateViaIntermediateJavaUtilDateConversion()
 
 		assertThat(wrapper).hasValueSatisfying(it -> it.setLastModifiedDate(zonedDateTime));
 	}
+
+	@Test // DATACMNS-1259
+	public void lastModifiedDateAsLongIsAvailableViaWrapper() {
+
+		LongBasedAuditable source = new LongBasedAuditable();
+		source.dateModified = 42000L;
+
+		Optional<AuditableBeanWrapper> beanWrapper = factory.getBeanWrapperFor(source);
+		assertThat(beanWrapper).isPresent();
+
+		assertThat(beanWrapper.flatMap(AuditableBeanWrapper::getLastModifiedDate).get()) //
+				.extracting(ta -> ta.getLong(ChronoField.INSTANT_SECONDS)) //
+				.containsExactly(42L);
+	}
+
+
+	@Test // DATACMNS-1259
+	public void canSetLastModifiedDateAsInstantViaWrapperOnLongField() {
+
+		LongBasedAuditable source = new LongBasedAuditable();
+
+		Optional<AuditableBeanWrapper> beanWrapper = factory.getBeanWrapperFor(source);
+		assertThat(beanWrapper).isPresent();
+
+		beanWrapper.get().setLastModifiedDate(Instant.ofEpochMilli(42L));
+
+		assertThat(source.dateModified).isEqualTo(42L);
+	}
+
+	@Test // DATACMNS-1259
+	public void canSetLastModifiedDateAsLocalDateTimeViaWrapperOnLongField() {
+
+		LongBasedAuditable source = new LongBasedAuditable();
+
+		Optional<AuditableBeanWrapper> beanWrapper = factory.getBeanWrapperFor(source);
+		assertThat(beanWrapper).isPresent();
+
+		beanWrapper.get().setLastModifiedDate(LocalDateTime.ofInstant(Instant.ofEpochMilli(42L), ZoneOffset.systemDefault()));
+
+		assertThat(source.dateModified).isEqualTo(42L);
+	}
+
+
+	@Test // DATACMNS-1259
+	public void lastModifiedAsLocalDateTimeDateIsAvailableViaWrapperAsLocalDateTime() {
+
+		LocalDateTime now = LocalDateTime.now();
+
+		AuditedUser source = new AuditedUser();
+		source.setLastModifiedDate(now);
+
+		Optional<AuditableBeanWrapper> beanWrapper = factory.getBeanWrapperFor(source);
+		assertThat(beanWrapper).isPresent();
+
+		assertThat(beanWrapper.flatMap(AuditableBeanWrapper::getLastModifiedDate).get()) //
+				.isEqualTo(now);
+	}
+
+	public static class LongBasedAuditable {
+
+		@CreatedDate
+		public Long dateCreated;
+
+		@LastModifiedDate
+		public Long dateModified;
+	}
 }
diff --git a/src/test/java/org/springframework/data/auditing/MappingAuditableBeanWrapperFactoryUnitTests.java b/src/test/java/org/springframework/data/auditing/MappingAuditableBeanWrapperFactoryUnitTests.java
@@ -20,13 +20,16 @@
 
 import java.time.Instant;
 import java.time.LocalDateTime;
+import java.time.ZoneOffset;
+import java.time.temporal.ChronoField;
 import java.time.temporal.TemporalAccessor;
 import java.util.Calendar;
 import java.util.Collections;
 import java.util.Date;
 import java.util.GregorianCalendar;
 import java.util.Optional;
 
+import org.assertj.core.api.AbstractLongAssert;
 import org.junit.Before;
 import org.junit.Test;
 import org.springframework.data.annotation.CreatedBy;
@@ -45,6 +48,7 @@
  * Unit tests for {@link MappingAuditableBeanWrapperFactory}.
  * 
  * @author Oliver Gierke
+ * @author Jens Schauder
  * @since 1.8
  */
 public class MappingAuditableBeanWrapperFactoryUnitTests {
@@ -169,14 +173,42 @@ public void exposesInstantAsModificationDate() {
 		assertThat(wrapper.flatMap(it -> it.getLastModifiedDate())).hasValue(sample.modified);
 	}
 
+	@Test // DATACMNS-1259
+	public void exposesLongAsModificationDate() {
+
+		Long reference = new Date().getTime();
+
+		assertLastModificationDate(reference, Instant.ofEpochMilli(reference));
+	}
+
 	private void assertLastModificationDate(Object source, TemporalAccessor expected) {
 
 		Sample sample = new Sample();
 		sample.lastModifiedDate = source;
 
 		Optional<AuditableBeanWrapper> wrapper = factory.getBeanWrapperFor(sample);
 
-		assertThat(wrapper.flatMap(it -> it.getLastModifiedDate())).hasValue(expected);
+		assertThat(wrapper.flatMap(it -> it.getLastModifiedDate())).hasValueSatisfying(ta -> {
+			compareTemporalAccessors(expected, ta);
+		});
+	}
+
+	private AbstractLongAssert<?> compareTemporalAccessors(TemporalAccessor expected, TemporalAccessor actual) {
+
+		long actualSeconds = getInstantSeconds(actual);
+		long expectedSeconds = getInstantSeconds(expected);
+
+		return assertThat(actualSeconds).describedAs("Difference is %s", actualSeconds - expectedSeconds)
+				.isEqualTo(expectedSeconds);
+	}
+
+	private long getInstantSeconds(TemporalAccessor actual) {
+
+		if (actual instanceof LocalDateTime) {
+			return getInstantSeconds(((LocalDateTime) actual).atZone(ZoneOffset.systemDefault()));
+		}
+
+		return actual.getLong(ChronoField.INSTANT_SECONDS);
 	}
 
 	static class Sample {
