simplify handling of datetimes and fix conversion to JDBC timezone

gavinking · gavinking · commit 849f23c9c1c6 · 2024-11-19T22:16:59.000+01:00
diff --git a/hibernate-core/src/main/java/org/hibernate/type/descriptor/WrapperOptions.java b/hibernate-core/src/main/java/org/hibernate/type/descriptor/WrapperOptions.java
@@ -9,6 +9,9 @@
 import org.hibernate.engine.spi.SessionFactoryImplementor;
 import org.hibernate.engine.spi.SharedSessionContractImplementor;
 
+import java.time.Instant;
+import java.time.ZoneId;
+import java.time.ZoneOffset;
 import java.util.TimeZone;
 
 /**
@@ -91,4 +94,53 @@ default Dialect getDialect() {
 	 * @see org.hibernate.cfg.AvailableSettings#JDBC_TIME_ZONE
 	 */
 	TimeZone getJdbcTimeZone();
+
+	/**
+	 * Get the {@link ZoneId} representing the
+	 * {@linkplain #getJdbcTimeZone JDBC time zone}, or the
+	 * {@linkplain ZoneId#systemDefault JVM default time zone id}
+	 * if no JDBC time zone was set via
+	 * {@value org.hibernate.cfg.AvailableSettings#JDBC_TIME_ZONE}.
+	 *
+	 * @apiNote Use for converting datetimes to and from the JDBC
+	 *          time zone.
+	 *
+	 * @see org.hibernate.cfg.AvailableSettings#JDBC_TIME_ZONE
+	 */
+	default ZoneId getJdbcZoneId() {
+		final TimeZone jdbcTimeZone = getJdbcTimeZone();
+		return jdbcTimeZone == null
+				? ZoneId.systemDefault()
+				: jdbcTimeZone.toZoneId();
+	}
+
+	/**
+	 * Get the current {@link ZoneOffset} for the {@link ZoneId}
+	 * representing the {@linkplain #getJdbcTimeZone JDBC time zone},
+	 * or the {@linkplain ZoneId#systemDefault JVM default time zone id}
+	 * if no JDBC time zone was set via
+	 * {@value org.hibernate.cfg.AvailableSettings#JDBC_TIME_ZONE}.
+	 *
+	 * @apiNote Use for converting <em>times</em> to and from the JDBC
+	 *          time zone. For datetimes, prefer {@link #getJdbcZoneId()}
+	 *          to apply the rules in force at the given datetime.
+	 *
+	 * @see org.hibernate.cfg.AvailableSettings#JDBC_TIME_ZONE
+	 */
+	default ZoneOffset getJdbcZoneOffset() {
+		return getJdbcZoneId().getRules().getOffset( Instant.now() );
+	}
+
+	/**
+	 * Get the current {@link ZoneOffset} for the
+	 * {@linkplain ZoneId#systemDefault JVM default time zone id}.
+	 *
+	 * @apiNote Use for converting <em>times</em> to and from the
+	 *          system time zone. For datetimes, prefer
+	 *          {@link ZoneId#systemDefault()}, to apply the rules
+	 *          in force at the given datetime.
+	 */
+	default ZoneOffset getSystemZoneOffset() {
+		return ZoneId.systemDefault().getRules().getOffset( Instant.now() );
+	}
 }
diff --git a/hibernate-core/src/main/java/org/hibernate/type/descriptor/java/InstantJavaType.java b/hibernate-core/src/main/java/org/hibernate/type/descriptor/java/InstantJavaType.java
@@ -7,7 +7,6 @@
 import java.sql.Timestamp;
 import java.time.Instant;
 import java.time.OffsetDateTime;
-import java.time.ZoneId;
 import java.time.ZoneOffset;
 import java.time.ZonedDateTime;
 import java.time.format.DateTimeFormatter;
@@ -102,23 +101,10 @@ public <X> X unwrap(Instant instant, Class<X> type, WrapperOptions options) {
 		}
 
 		if ( Timestamp.class.isAssignableFrom( type ) ) {
-			/*
-			 * This works around two bugs:
-			 * - HHH-13266 (JDK-8061577): around and before 1900,
-			 * the number of milliseconds since the epoch does not mean the same thing
-			 * for java.util and java.time, so conversion must be done using the year, month, day, hour, etc.
-			 * - HHH-13379 (JDK-4312621): after 1908 (approximately),
-			 * Daylight Saving Time introduces ambiguity in the year/month/day/hour/etc representation once a year
-			 * (on DST end), so conversion must be done using the number of milliseconds since the epoch.
-			 * - around 1905, both methods are equally valid, so we don't really care which one is used.
-			 */
-			ZonedDateTime zonedDateTime = instant.atZone( ZoneId.systemDefault() );
-			if ( zonedDateTime.getYear() < 1905 ) {
-				return (X) Timestamp.valueOf( zonedDateTime.toLocalDateTime() );
-			}
-			else {
-				return (X) Timestamp.from( instant );
-			}
+			// NOTE: do not use Timestamp.from(Instant) because that returns a Timestamp in UTC
+			final ZonedDateTime dateTime =
+					instant.atZone( options.getJdbcZoneId() ); // convert to the JDBC timezone
+			return (X) Timestamp.valueOf( dateTime.toLocalDateTime() );
 		}
 
 		if ( java.sql.Date.class.isAssignableFrom( type ) ) {
@@ -155,22 +141,10 @@ public <X> Instant wrap(X value, WrapperOptions options) {
 		}
 
 		if ( value instanceof Timestamp timestamp ) {
-			/*
-			 * This works around two bugs:
-			 * - HHH-13266 (JDK-8061577): around and before 1900,
-			 * the number of milliseconds since the epoch does not mean the same thing
-			 * for java.util and java.time, so conversion must be done using the year, month, day, hour, etc.
-			 * - HHH-13379 (JDK-4312621): after 1908 (approximately),
-			 * Daylight Saving Time introduces ambiguity in the year/month/day/hour/etc representation once a year
-			 * (on DST end), so conversion must be done using the number of milliseconds since the epoch.
-			 * - around 1905, both methods are equally valid, so we don't really care which one is used.
-			 */
-			if ( timestamp.getYear() < 5 ) { // Timestamp year 0 is 1900
-				return timestamp.toLocalDateTime().atZone( ZoneId.systemDefault() ).toInstant();
-			}
-			else {
-				return timestamp.toInstant();
-			}
+			// NOTE: do not use Timestamp.getInstant() because that assumes the Timestamp is in UTC
+			return timestamp.toLocalDateTime()
+					.atZone( options.getJdbcZoneId() ) // the Timestamp is in the JDBC timezone
+					.toInstant(); // return the instant (always in UTC)
 		}
 
 		if ( value instanceof Long longValue ) {
diff --git a/hibernate-core/src/main/java/org/hibernate/type/descriptor/java/OffsetDateTimeJavaType.java b/hibernate-core/src/main/java/org/hibernate/type/descriptor/java/OffsetDateTimeJavaType.java
@@ -137,24 +137,9 @@ public <X> X unwrap(OffsetDateTime offsetDateTime, Class<X> type, WrapperOptions
 		}
 
 		if ( Timestamp.class.isAssignableFrom( type ) ) {
-			/*
-			 * This works around two bugs:
-			 * - HHH-13266 (JDK-8061577): around and before 1900,
-			 * the number of milliseconds since the epoch does not mean the same thing
-			 * for java.util and java.time, so conversion must be done using the year, month, day, hour, etc.
-			 * - HHH-13379 (JDK-4312621): after 1908 (approximately),
-			 * Daylight Saving Time introduces ambiguity in the year/month/day/hour/etc representation once a year
-			 * (on DST end), so conversion must be done using the number of milliseconds since the epoch.
-			 * - around 1905, both methods are equally valid, so we don't really care which one is used.
-			 */
-			if ( offsetDateTime.getYear() < 1905 ) {
-				return (X) Timestamp.valueOf(
-						offsetDateTime.atZoneSameInstant( ZoneId.systemDefault() ).toLocalDateTime()
-				);
-			}
-			else {
-				return (X) Timestamp.from( offsetDateTime.toInstant() );
-			}
+			final ZonedDateTime dateTime =
+					offsetDateTime.atZoneSameInstant( options.getJdbcZoneId() ); // convert to the JDBC timezone
+			return (X) Timestamp.valueOf( dateTime.toLocalDateTime() );
 		}
 
 		if ( java.sql.Date.class.isAssignableFrom( type ) ) {
@@ -195,22 +180,10 @@ public <X> OffsetDateTime wrap(X value, WrapperOptions options) {
 		}
 
 		if (value instanceof Timestamp timestamp) {
-			/*
-			 * This works around two bugs:
-			 * - HHH-13266 (JDK-8061577): around and before 1900,
-			 * the number of milliseconds since the epoch does not mean the same thing
-			 * for java.util and java.time, so conversion must be done using the year, month, day, hour, etc.
-			 * - HHH-13379 (JDK-4312621): after 1908 (approximately),
-			 * Daylight Saving Time introduces ambiguity in the year/month/day/hour/etc representation once a year
-			 * (on DST end), so conversion must be done using the number of milliseconds since the epoch.
-			 * - around 1905, both methods are equally valid, so we don't really care which one is used.
-			 */
-			if ( timestamp.getYear() < 5 ) { // Timestamp year 0 is 1900
-				return timestamp.toLocalDateTime().atZone( ZoneId.systemDefault() ).toOffsetDateTime();
-			}
-			else {
-				return OffsetDateTime.ofInstant( timestamp.toInstant(), ZoneId.systemDefault() );
-			}
+			return timestamp.toLocalDateTime()
+					.atZone( options.getJdbcZoneId() ) // the Timestamp is in the JDBC timezone
+					.withZoneSameInstant( ZoneId.systemDefault() ) // convert back to the VM timezone
+					.toOffsetDateTime(); // return the corresponding OffsetDateTime
 		}
 
 		if (value instanceof Date date) {
diff --git a/hibernate-core/src/main/java/org/hibernate/type/descriptor/java/OffsetTimeJavaType.java b/hibernate-core/src/main/java/org/hibernate/type/descriptor/java/OffsetTimeJavaType.java
@@ -11,7 +11,7 @@
 import java.time.LocalTime;
 import java.time.OffsetDateTime;
 import java.time.OffsetTime;
-import java.time.ZoneOffset;
+import java.time.ZoneId;
 import java.time.format.DateTimeFormatter;
 import java.time.temporal.ChronoField;
 import java.util.Calendar;
@@ -92,59 +92,71 @@ public <X> X unwrap(OffsetTime offsetTime, Class<X> type, WrapperOptions options
 		}
 
 		if ( LocalTime.class.isAssignableFrom( type ) ) {
-			return (X) offsetTime.withOffsetSameInstant( getCurrentSystemOffset() ).toLocalTime();
+			return (X) offsetTime.withOffsetSameInstant( options.getSystemZoneOffset() ).toLocalTime();
 		}
 
 		if ( OffsetDateTime.class.isAssignableFrom( type ) ) {
-			return (X) offsetTime.atDate( LocalDate.EPOCH );
+			return (X) offsetDateTimeAtEpoch( offsetTime );
 		}
 
 		// for legacy types, we assume that the JDBC timezone is passed to JDBC
 		// (since PS.setTime() and friends do accept a timezone passed as a Calendar)
 
-		final OffsetTime jdbcOffsetTime = offsetTime.withOffsetSameInstant( getCurrentJdbcOffset(options) );
 
 		if ( Time.class.isAssignableFrom( type ) ) {
+			// Use ZoneOffset rather than ZoneId in the conversion,
+			// since the offset for a zone varies over time, but
+			// a Time does not have an attached Date.
+			final OffsetTime jdbcOffsetTime =
+					offsetTime.withOffsetSameInstant( options.getJdbcZoneOffset() ); // convert to the JDBC timezone
 			final Time time = Time.valueOf( jdbcOffsetTime.toLocalTime() );
-			if ( jdbcOffsetTime.getNano() == 0 ) {
-				return (X) time;
-			}
-			// Preserve milliseconds, which java.sql.Time supports
-			return (X) new Time( time.getTime() + DateTimeUtils.roundToPrecision( jdbcOffsetTime.getNano(), 3 ) / 1000000 );
+			// Time.valueOf() throws away milliseconds
+			return (X) withMillis( jdbcOffsetTime, time );
 		}
 
-		final OffsetDateTime jdbcOffsetDateTime = jdbcOffsetTime.atDate( LocalDate.EPOCH );
-
 		if ( Timestamp.class.isAssignableFrom( type ) ) {
-			/*
-			 * Workaround for HHH-13266 (JDK-8061577).
-			 * Ideally we'd want to use Timestamp.from( jdbcOffsetDateTime.toInstant() ),
-			 * but this won't always work since Timestamp.from() assumes the number of
-			 * milliseconds since the epoch means the same thing in Timestamp and Instant,
-			 * but it doesn't, in particular before 1900.
-			 */
-			return (X) Timestamp.valueOf( jdbcOffsetDateTime.toLocalDateTime() );
+			final OffsetTime jdbcOffsetTime =
+					offsetTime.withOffsetSameInstant( options.getJdbcZoneOffset() ); // convert to the JDBC timezone
+			return (X) Timestamp.valueOf( offsetDateTimeAtEpoch( jdbcOffsetTime ).toLocalDateTime() );
 		}
 
 		if ( Calendar.class.isAssignableFrom( type ) ) {
-			return (X) GregorianCalendar.from( jdbcOffsetDateTime.toZonedDateTime() );
+			return (X) GregorianCalendar.from( offsetDateTimeAtEpoch( offsetTime ).toZonedDateTime() );
 		}
 
 		// for instants, we assume that the JDBC timezone, if any, is ignored
 
-		final Instant instant = offsetTime.atDate( LocalDate.EPOCH ).toInstant();
-
 		if ( Long.class.isAssignableFrom( type ) ) {
-			return (X) Long.valueOf( instant.toEpochMilli() );
+			return (X) Long.valueOf( instantAtEpoch( offsetTime ).toEpochMilli() );
 		}
 
 		if ( Date.class.isAssignableFrom( type ) ) {
-			return (X) Date.from( instant );
+			return (X) Date.from( instantAtEpoch( offsetTime ) );
 		}
 
 		throw unknownUnwrap( type );
 	}
 
+	private static Time withMillis(OffsetTime jdbcOffsetTime, Time time) {
+		final int nanos = jdbcOffsetTime.getNano();
+		if ( nanos == 0 ) {
+			return time;
+		}
+		else {
+			// Preserve milliseconds, which java.sql.Time supports
+			final long millis = DateTimeUtils.roundToPrecision( nanos, 3 ) / 1_000_000;
+			return new Time( time.getTime() + millis );
+		}
+	}
+
+	private static OffsetDateTime offsetDateTimeAtEpoch(OffsetTime jdbcOffsetTime) {
+		return jdbcOffsetTime.atDate( LocalDate.EPOCH );
+	}
+
+	private static Instant instantAtEpoch(OffsetTime offsetTime) {
+		return offsetDateTimeAtEpoch( offsetTime ).toInstant();
+	}
+
 	@Override
 	public <X> OffsetTime wrap(X value, WrapperOptions options) {
 		if ( value == null ) {
@@ -159,67 +171,39 @@ public <X> OffsetTime wrap(X value, WrapperOptions options) {
 		}
 
 		if (value instanceof LocalTime localTime) {
-			return localTime.atOffset( getCurrentSystemOffset() );
+			return localTime.atOffset( options.getSystemZoneOffset() );
 		}
 
 		if ( value instanceof OffsetDateTime offsetDateTime) {
 			return offsetDateTime.toOffsetTime();
 		}
 
-		/*
-		 * Also, in order to fix HHH-13357, and to be consistent with the conversion to Time (see above),
-		 * we set the offset to the current offset of the JVM (OffsetDateTime.now().getOffset()).
-		 * This is different from setting the *zone* to the current *zone* of the JVM (ZoneId.systemDefault()),
-		 * since a zone has a varying offset over time,
-		 * thus the zone might have a different offset for the given timezone than it has for the current date/time.
-		 * For example, if the timestamp represents 1970-01-01TXX:YY,
-		 * and the JVM is set to use Europe/Paris as a timezone, and the current time is 2019-04-16-08:53,
-		 * then applying the JVM timezone to the timestamp would result in the offset +01:00,
-		 * but applying the JVM offset would result in the offset +02:00, since DST is in effect at 2019-04-16-08:53.
-		 *
-		 * Of course none of this would be a problem if we just stored the offset in the database,
-		 * but I guess there are historical reasons that explain why we don't.
-		 */
-
-		// for legacy types, we assume that the JDBC timezone is passed to JDBC
-		// (since PS.setTime() and friends do accept a timezone passed as a Calendar)
-
 		if (value instanceof Time time) {
-			final OffsetTime offsetTime = time.toLocalTime()
-					.atOffset( getCurrentJdbcOffset( options) )
-					.withOffsetSameInstant( getCurrentSystemOffset() );
-			long millis = time.getTime() % 1000;
-			if ( millis == 0 ) {
-				return offsetTime;
-			}
-			if ( millis < 0 ) {
-				// The milliseconds for a Time could be negative,
-				// which usually means the time is in a different time zone
-				millis += 1_000L;
-			}
-			return offsetTime.with( ChronoField.NANO_OF_SECOND, millis * 1_000_000L );
+			// Use ZoneOffset rather than ZoneId in the conversion,
+			// since the offset for a zone varies over time, but
+			// a Time does not have an attached Date.
+			final OffsetTime offsetTime =
+					time.toLocalTime().atOffset( options.getJdbcZoneOffset() ) // the Timestamp is in the current JDBC timezone offset
+							.withOffsetSameInstant( options.getSystemZoneOffset() ); // convert back to the VM timezone
+			// Time.toLocalTime() strips off nanos
+			return withNanos( time, offsetTime );
 		}
 
 		if (value instanceof Timestamp timestamp) {
-			/*
-			 * Workaround for HHH-13266 (JDK-8061577).
-			 * Ideally we'd want to use OffsetDateTime.ofInstant( ts.toInstant(), ... ),
-			 * but this won't always work since ts.toInstant() assumes the number of
-			 * milliseconds since the epoch means the same thing in Timestamp and Instant,
-			 * but it doesn't, in particular before 1900.
-			 */
-			return timestamp.toLocalDateTime().toLocalTime().atOffset( getCurrentJdbcOffset(options) )
-					.withOffsetSameInstant( getCurrentSystemOffset() );
+			return timestamp.toLocalDateTime()
+					.atZone( options.getJdbcZoneId() ) // the Timestamp is in the JDBC timezone
+					.withZoneSameInstant( ZoneId.systemDefault() ) // convert back to the VM timezone
+					.toOffsetDateTime().toOffsetTime(); // return the time part
 		}
 
 		if (value instanceof Date date) {
-			return OffsetTime.ofInstant( date.toInstant(), getCurrentSystemOffset() );
+			return OffsetTime.ofInstant( date.toInstant(), options.getSystemZoneOffset() );
 		}
 
 		// for instants, we assume that the JDBC timezone, if any, is ignored
 
 		if (value instanceof Long millis) {
-			return OffsetTime.ofInstant( Instant.ofEpochMilli(millis), getCurrentSystemOffset() );
+			return OffsetTime.ofInstant( Instant.ofEpochMilli(millis), options.getSystemZoneOffset() );
 		}
 
 		if (value instanceof Calendar calendar) {
@@ -229,17 +213,21 @@ public <X> OffsetTime wrap(X value, WrapperOptions options) {
 		throw unknownWrap( value.getClass() );
 	}
 
-	private static ZoneOffset getCurrentJdbcOffset(WrapperOptions options) {
-		if (  options.getJdbcTimeZone() != null ) {
-			return OffsetDateTime.now().atZoneSameInstant( options.getJdbcTimeZone().toZoneId() ).getOffset();
+	private static OffsetTime withNanos(Time time, OffsetTime offsetTime) {
+		final long millis = time.getTime() % 1000;
+		final long nanos;
+		if ( millis == 0 ) {
+			return offsetTime;
+		}
+		else if ( millis < 0 ) {
+			// The milliseconds for a Time could be negative,
+			// which usually means the time is in a different time zone
+			nanos = (millis + 1_000L) * 1_000_000L;
 		}
 		else {
-			return getCurrentSystemOffset();
+			nanos = millis * 1_000_000L;
 		}
-	}
-
-	private static ZoneOffset getCurrentSystemOffset() {
-		return OffsetDateTime.now().getOffset();
+		return offsetTime.with( ChronoField.NANO_OF_SECOND, nanos );
 	}
 
 	@Override
diff --git a/hibernate-core/src/main/java/org/hibernate/type/descriptor/java/ZonedDateTimeJavaType.java b/hibernate-core/src/main/java/org/hibernate/type/descriptor/java/ZonedDateTimeJavaType.java
