HHH-15679 proposed fix to OffsetTime handling

The idea is: convert all OffsetTimes to the system offset before sending them on

Author: gavinking

hibernate-core/src/main/java/org/hibernate/type/descriptor/java/OffsetTimeJavaType.java

@@ -11,10 +11,10 @@
 import java.sql.Types;
 import java.time.Instant;
 import java.time.LocalDate;
+import java.time.LocalTime;
 import java.time.OffsetDateTime;
 import java.time.OffsetTime;
 import java.time.ZoneOffset;
-import java.time.ZonedDateTime;
 import java.time.format.DateTimeFormatter;
 import java.util.Calendar;
 import java.util.Date;
@@ -76,31 +76,46 @@ public <X> X unwrap(OffsetTime offsetTime, Class<X> type, WrapperOptions options
 			return null;
 		}
 
+		// for java.time types, we assume that the JDBC timezone, if any, is ignored
+		// (since PS.setObject() doesn't support passing a timezone)
+
 		if ( OffsetTime.class.isAssignableFrom( type ) ) {
 			return (X) offsetTime;
 		}
 
+		if ( LocalTime.class.isAssignableFrom( type ) ) {
+			return (X) offsetTime.withOffsetSameInstant( getCurrentSystemOffset() ).toLocalTime();
+		}
+
+		// 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 ) ) {
-			return (X) Time.valueOf( offsetTime.toLocalTime() );
+			return (X) Time.valueOf( jdbcOffsetTime.toLocalTime() );
 		}
 
-		final ZonedDateTime zonedDateTime = offsetTime.atDate( LocalDate.of( 1970, 1, 1 ) ).toZonedDateTime();
+		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( offsetDateTime.toInstant() ), but this won't always work.
-			 * 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.
+			 * 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( zonedDateTime.toLocalDateTime() );
+			return (X) Timestamp.valueOf( jdbcOffsetDateTime.toLocalDateTime() );
 		}
 
 		if ( Calendar.class.isAssignableFrom( type ) ) {
-			return (X) GregorianCalendar.from( zonedDateTime );
+			return (X) GregorianCalendar.from( jdbcOffsetDateTime.toZonedDateTime() );
 		}
 
-		final Instant instant = zonedDateTime.toInstant();
+		// 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() );
@@ -119,10 +134,17 @@ public <X> OffsetTime wrap(X value, WrapperOptions options) {
 			return null;
 		}
 
+		// for java.time types, we assume that the JDBC timezone, if any, is ignored
+		// (since PS.setObject() doesn't support passing a timezone)
+
 		if (value instanceof OffsetTime) {
 			return (OffsetTime) value;
 		}
 
+		if (value instanceof LocalTime) {
+			return ((LocalTime) value).atOffset( getCurrentSystemOffset() );
+		}
+
 		/*
 		 * 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()).
@@ -137,30 +159,39 @@ public <X> OffsetTime wrap(X value, WrapperOptions options) {
 		 * 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.
 		 */
-		ZoneOffset offset = OffsetDateTime.now().getOffset();
+
+		// 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) {
-			return ( (Time) value ).toLocalTime().atOffset( offset );
+			final Time time = (Time) value;
+			return time.toLocalTime().atOffset( getCurrentJdbcOffset(options) )
+					.withOffsetSameInstant( getCurrentSystemOffset() );
 		}
 
 		if (value instanceof Timestamp) {
 			final Timestamp ts = (Timestamp) value;
 			/*
 			 * Workaround for HHH-13266 (JDK-8061577).
-			 * Ideally we'd want to use OffsetDateTime.ofInstant( ts.toInstant(), ... ), but this won't always work.
-			 * 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.
+			 * 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 ts.toLocalDateTime().toLocalTime().atOffset( offset );
+			return ts.toLocalDateTime().toLocalTime().atOffset( getCurrentJdbcOffset(options) )
+					.withOffsetSameInstant( getCurrentSystemOffset() );
 		}
 
 		if (value instanceof Date) {
 			final Date date = (Date) value;
-			return OffsetTime.ofInstant( date.toInstant(), offset );
+			return OffsetTime.ofInstant( date.toInstant(), getCurrentSystemOffset() );
 		}
 
+		// for instants, we assume that the JDBC timezone, if any, is ignored
+
 		if (value instanceof Long) {
-			return OffsetTime.ofInstant( Instant.ofEpochMilli( (Long) value ), offset );
+			final long millis = (Long) value;
+			return OffsetTime.ofInstant( Instant.ofEpochMilli(millis), getCurrentSystemOffset() );
 		}
 
 		if (value instanceof Calendar) {
@@ -171,6 +202,19 @@ 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();
+		}
+		else {
+			return getCurrentSystemOffset();
+		}
+	}
+
+	private static ZoneOffset getCurrentSystemOffset() {
+		return OffsetDateTime.now().getOffset();
+	}
+
 	@Override
 	public int getDefaultSqlPrecision(Dialect dialect, JdbcType jdbcType) {
 		return 0;

hibernate-core/src/test/java/org/hibernate/orm/test/type/AbstractJavaTimeTypeTest.java

@@ -25,7 +25,6 @@
 import org.hibernate.boot.model.TypeContributions;
 import org.hibernate.cfg.AvailableSettings;
 import org.hibernate.cfg.Configuration;
-import org.hibernate.dialect.DatabaseVersion;
 import org.hibernate.dialect.Dialect;
 import org.hibernate.dialect.H2Dialect;
 import org.hibernate.service.ServiceRegistry;
@@ -125,9 +124,10 @@ public void writeThenRead() {
 			} );
 			inTransaction( session -> {
 				T read = getActualPropertyValue( session.find( getEntityType(), 1 ) );
+				T expected = getExpectedPropertyValueAfterHibernateRead();
 				assertEquals(
 						"Writing then reading a value should return the original value",
-						getExpectedPropertyValueAfterHibernateRead(), read
+						expected, read
 				);
 			} );
 		} );
@@ -152,10 +152,10 @@ public void writeThenNativeRead() {
 						try (ResultSet resultSet = statement.getResultSet()) {
 							resultSet.next();
 							Object nativeRead = getActualJdbcValue( resultSet, 1 );
+							Object expected = getExpectedJdbcValueAfterHibernateWrite();
 							assertEquals(
 									"Values written by Hibernate ORM should match the original value (same day, hour, ...)",
-									getExpectedJdbcValueAfterHibernateWrite(),
-									nativeRead
+									expected, nativeRead
 							);
 						}
 					}
@@ -184,9 +184,10 @@ public void nativeWriteThenRead() {
 			} );
 			inTransaction( session -> {
 				T read = getActualPropertyValue( session.find( getEntityType(), 1 ) );
+				T expected = getExpectedPropertyValueAfterHibernateRead();
 				assertEquals(
 						"Values written without Hibernate ORM should be read correctly by Hibernate ORM",
-						getExpectedPropertyValueAfterHibernateRead(), read
+						expected, read
 				);
 			} );
 		} );

hibernate-core/src/test/java/org/hibernate/orm/test/type/OffsetTimeTest.java

@@ -12,13 +12,15 @@
 import java.sql.Time;
 import java.sql.Timestamp;
 import java.sql.Types;
+import java.time.LocalDate;
 import java.time.OffsetDateTime;
 import java.time.OffsetTime;
 import java.time.ZoneId;
 import java.time.ZoneOffset;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
+
 import jakarta.persistence.Basic;
 import jakarta.persistence.Column;
 import jakarta.persistence.Entity;
@@ -173,11 +175,11 @@ protected OffsetTime getOriginalPropertyValue() {
 	protected OffsetTime getExpectedPropertyValueAfterHibernateRead() {
 		// For some reason, the offset is not stored, so the restored values use the offset from the default JVM timezone.
 		if ( TimeAsTimestampRemappingH2Dialect.class.equals( getRemappingDialectClass() ) ) {
-			return getOriginalPropertyValue().withOffsetSameLocal( OffsetDateTime.now().getOffset() );
+			return getOriginalPropertyValue().withOffsetSameInstant( OffsetDateTime.now().getOffset() );
 		}
 		else {
 			// When storing time as java.sql.Time, we only get second precision (not nanosecond)
-			return getOriginalPropertyValue().withNano( 0 ).withOffsetSameLocal( OffsetDateTime.now().getOffset() );
+			return getOriginalPropertyValue().withNano( 0 ).withOffsetSameInstant( OffsetDateTime.now().getOffset() );
 		}
 	}
 
@@ -189,29 +191,28 @@ protected OffsetTime getActualPropertyValue(EntityWithOffsetTime entity) {
 	@Override
 	protected void setJdbcValueForNonHibernateWrite(PreparedStatement statement, int parameterIndex)
 			throws SQLException {
+		OffsetTime offsetTime = OffsetTime.of( hour, minute, second, 0, ZoneOffset.of(offset) )
+				.withOffsetSameInstant( OffsetDateTime.now().getOffset() );
 		if ( TimeAsTimestampRemappingH2Dialect.class.equals( getRemappingDialectClass() ) ) {
 			statement.setTimestamp(
 					parameterIndex,
-					new Timestamp(
-							yearWhenPersistedWithoutHibernate - 1900,
-							monthWhenPersistedWithoutHibernate - 1,
-							dayWhenPersistedWithoutHibernate,
-							hour, minute, second, nanosecond
-					)
+					Timestamp.valueOf( offsetTime.atDate( LocalDate.EPOCH ).toLocalDateTime() )
 			);
 		}
 		else {
-			statement.setTime( parameterIndex, new Time( hour, minute, second ) );
+			statement.setTime( parameterIndex, Time.valueOf( offsetTime.toLocalTime() ) );
 		}
 	}
 
 	@Override
 	protected Object getExpectedJdbcValueAfterHibernateWrite() {
+		OffsetTime offsetTime = OffsetTime.of( hour, minute, second, 0, ZoneOffset.of(offset) )
+				.withOffsetSameInstant( OffsetDateTime.now().getOffset() );
 		if ( TimeAsTimestampRemappingH2Dialect.class.equals( getRemappingDialectClass() ) ) {
-			return new Timestamp( 1970 - 1900, 0, 1, hour, minute, second, nanosecond );
+			return Timestamp.valueOf( offsetTime.atDate( LocalDate.EPOCH ).toLocalDateTime() );
 		}
 		else {
-			return new Time( hour, minute, second );
+			return Time.valueOf( offsetTime.toLocalTime() );
 		}
 	}