Skip to content

HHH-18532 Date/time related JavaType's are not always properly unwrapping into java.util.Date subclasses #8833

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

HHH-18532 Date/time related JavaType's are not always properly unwrapping into java.util.Date subclasses #8833

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3e1249b
HHH-18532 Test cases checking that value is unwrapped to requested type
cigaly Aug 11, 2024
d6d7cf3
HHH-18532 Fixed two things:
cigaly Aug 28, 2024
9a87812
HHH-18532 Suggested fix to conversion of epoch time in milliseconds t…
cigaly Aug 28, 2024
670de92
HHH-18532 Using java.util.Calendar instead of java.time.* clases to c…
cigaly Apr 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions ...rnate-core/src/main/java/org/hibernate/type/descriptor/java/AbstractTemporalJavaType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
*/
package org.hibernate.type.descriptor.java;

import java.sql.Time;
import java.util.Calendar;
import java.util.Comparator;

import jakarta.persistence.TemporalType;
Expand Down Expand Up @@ -60,6 +62,18 @@ private <X> TemporalJavaType<X> forMissingPrecision(TypeConfiguration typeConfig
return (TemporalJavaType) this;
}

public static Time millisToSqlTime(long millis) {
Calendar calendar = Calendar.getInstance();
calendar.setTimeInMillis( millis );
calendar.set(Calendar.YEAR, 1970);
calendar.set(Calendar.MONTH, 0);
calendar.set(Calendar.DAY_OF_MONTH, 1);

final Time time = new Time(millis);
time.setTime( calendar.getTimeInMillis() );
return time;
}

protected <X> TemporalJavaType<X> forTimestampPrecision(TypeConfiguration typeConfiguration) {
throw new UnsupportedOperationException(
this + " as `jakarta.persistence.TemporalType.TIMESTAMP` not supported"
Expand Down
2 changes: 1 addition & 1 deletion hibernate-core/src/main/java/org/hibernate/type/descriptor/java/CalendarDateJavaType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ public <X> X unwrap(Calendar value, Class<X> type, WrapperOptions options) {
return (X) new java.sql.Date( value.getTimeInMillis() );
}
if ( java.sql.Time.class.isAssignableFrom( type ) ) {
return (X) new java.sql.Time( value.getTimeInMillis() % 86_400_000 );
return (X) millisToSqlTime( value.getTimeInMillis() );
}
if ( java.sql.Timestamp.class.isAssignableFrom( type ) ) {
return (X) new java.sql.Timestamp( value.getTimeInMillis() );
Expand Down
2 changes: 1 addition & 1 deletion hibernate-core/src/main/java/org/hibernate/type/descriptor/java/CalendarJavaType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ public <X> X unwrap(Calendar value, Class<X> type, WrapperOptions options) {
return (X) new java.sql.Date( value.getTimeInMillis() );
}
if ( java.sql.Time.class.isAssignableFrom( type ) ) {
return (X) new java.sql.Time( value.getTimeInMillis() % 86_400_000 );
return (X) millisToSqlTime( value.getTimeInMillis() );
}
if ( java.sql.Timestamp.class.isAssignableFrom( type ) ) {
return (X) new java.sql.Timestamp( value.getTimeInMillis() );
Expand Down
2 changes: 1 addition & 1 deletion hibernate-core/src/main/java/org/hibernate/type/descriptor/java/CalendarTimeJavaType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ public <X> X unwrap(Calendar value, Class<X> type, WrapperOptions options) {
return (X) new java.sql.Date( value.getTimeInMillis() );
}
if ( java.sql.Time.class.isAssignableFrom( type ) ) {
return (X) new java.sql.Time( value.getTimeInMillis() % 86_400_000 );
return (X) millisToSqlTime( value.getTimeInMillis() );
}
if ( java.sql.Timestamp.class.isAssignableFrom( type ) ) {
return (X) new java.sql.Timestamp( value.getTimeInMillis() );
Expand Down
2 changes: 1 addition & 1 deletion hibernate-core/src/main/java/org/hibernate/type/descriptor/java/DateJavaType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ public <X> X unwrap(Date value, Class<X> type, WrapperOptions options) {
if ( java.sql.Time.class.isAssignableFrom( type ) ) {
final java.sql.Time rtn = value instanceof java.sql.Time
? ( java.sql.Time ) value
: new java.sql.Time( value.getTime() % 86_400_000 );
: millisToSqlTime( value.getTime() );
return (X) rtn;
}
if ( java.sql.Timestamp.class.isAssignableFrom( type ) ) {
Expand Down
2 changes: 1 addition & 1 deletion hibernate-core/src/main/java/org/hibernate/type/descriptor/java/InstantJavaType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ public <X> X unwrap(Instant instant, Class<X> type, WrapperOptions options) {
}

if ( java.sql.Time.class.isAssignableFrom( type ) ) {
return (X) new java.sql.Time( instant.toEpochMilli() % 86_400_000 );
return (X) millisToSqlTime( instant.toEpochMilli() );
}

if ( Date.class.isAssignableFrom( type ) ) {
Expand Down
16 changes: 8 additions & 8 deletions hibernate-core/src/main/java/org/hibernate/type/descriptor/java/JdbcDateJavaType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,14 @@ public Object unwrap(Date value, Class type, WrapperOptions options) {
return unwrapSqlDate( value );
}

if ( java.sql.Timestamp.class.isAssignableFrom( type ) ) {
return new java.sql.Timestamp( unwrapDateEpoch( value ) );
}

if ( java.sql.Time.class.isAssignableFrom( type ) ) {
throw new IllegalArgumentException( "Illegal attempt to treat `java.sql.Date` as `java.sql.Time`" );
}

if ( java.util.Date.class.isAssignableFrom( type ) ) {
return value;
}
Expand All @@ -141,14 +149,6 @@ public Object unwrap(Date value, Class type, WrapperOptions options) {
return cal;
}

if ( java.sql.Timestamp.class.isAssignableFrom( type ) ) {
return new java.sql.Timestamp( unwrapDateEpoch( value ) );
}

if ( java.sql.Time.class.isAssignableFrom( type ) ) {
throw new IllegalArgumentException( "Illegal attempt to treat `java.sql.Date` as `java.sql.Time`" );
}

throw unknownUnwrap( type );
}

Expand Down
30 changes: 12 additions & 18 deletions hibernate-core/src/main/java/org/hibernate/type/descriptor/java/JdbcTimeJavaType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ public Object unwrap(Date value, Class type, WrapperOptions options) {
if ( LocalTime.class.isAssignableFrom( type ) ) {
final Time time = value instanceof java.sql.Time
? ( (java.sql.Time) value )
: new java.sql.Time( value.getTime() % 86_400_000 );
: millisToSqlTime( value.getTime() );
final LocalTime localTime = time.toLocalTime();
long millis = time.getTime() % 1000;
if ( millis == 0 ) {
Expand All @@ -135,9 +135,15 @@ public Object unwrap(Date value, Class type, WrapperOptions options) {
}

if ( Time.class.isAssignableFrom( type ) ) {
return value instanceof Time
? value
: new Time( value.getTime() % 86_400_000 );
return millisToSqlTime( value.getTime() );
}

if ( java.sql.Timestamp.class.isAssignableFrom( type ) ) {
return new java.sql.Timestamp( value.getTime() );
}

if ( java.sql.Date.class.isAssignableFrom( type ) ) {
throw new IllegalArgumentException( "Illegal attempt to treat `java.sql.Time` as `java.sql.Date`" );
}

if ( Date.class.isAssignableFrom( type ) ) {
Expand All @@ -158,14 +164,6 @@ public Object unwrap(Date value, Class type, WrapperOptions options) {
return cal;
}

if ( java.sql.Timestamp.class.isAssignableFrom( type ) ) {
return new java.sql.Timestamp( value.getTime() );
}

if ( java.sql.Date.class.isAssignableFrom( type ) ) {
throw new IllegalArgumentException( "Illegal attempt to treat `java.sql.Time` as `java.sql.Date`" );
}

throw unknownUnwrap( type );
}

Expand All @@ -175,12 +173,8 @@ public Date wrap(Object value, WrapperOptions options) {
return null;
}

if ( value instanceof Time time ) {
Copy link
Member

@beikov beikov May 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why you removed this?

Copy link
Contributor Author

@cigaly cigaly May 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request is that output Time should represent local time on January 1st 1970, but it input can be on any date, for example:

jshell> var t = new Time(System.currentTimeMillis())
t ==> 22:24:43

jshell> new Date(t.getTime())
$6 ==> Thu May 15 22:24:43 CEST 2025

This the reason why we have to treat it like any other java.util.Date subclass

jshell> var tt = T.millisToSqlTime(t.getTime())
tt ==> 22:24:43

This is now java.sql.Time instance on January 1st 1970

jshell> new Date(tt.getTime())
$11 ==> Thu Jan 01 22:24:43 CET 1970

Copy link
Member

@beikov beikov May 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get that, it's just unfortunate that we would have to create copies every time we bind a java.sql.Time to a prepared statement even if the value already is correctly at the epoch date.

Copy link
Contributor Author

@cigaly cigaly May 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is true. One possibility to prevent this is to return time if it is already at epoch date, and make new copy if not. Something like

		if ( value instanceof Time time ) {
			Date date = new Date( time.getTime() );
			if ( date.getYear() == 70 && date.getMonth() == 0 && date.getDate() == 1 ) {
				return time;
			}
		}

Alternatively java.util.Calendar can be used, but I guess that this is 'cheaper' to create java.util.Date instance than java.util.Calendar

		if ( value instanceof Time time ) {
			Calendar calendar = Calendar.getInstance();
			calendar.setTime( time );
			if ( calendar.get( Calendar.YEAR ) == 1970
				 && calendar.get( Calendar.MONTH ) == Calendar.JANUARY
				 && calendar.get( Calendar.DAY_OF_MONTH ) == 1 ) {
				return time;
			}
		}

Copy link
Member

@beikov beikov May 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm missing something, but java.util.Date#getTime() specifies

Returns the number of milliseconds since January 1, 1970, 00:00:00 GMT represented by this Date object.

So AFAIU, using the modulo trick that we have right now should work just fine. Please help me understand what I am missing. According so java docs the code should be correct.

Copy link
Member

@beikov beikov May 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getHours() is deprecated and the Java doc also explains that it returns the TZ dependent value.

Copy link
Contributor Author

@cigaly cigaly May 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct, that (and all other getXXX methods, with exception of getTime) were deprecated starting from JDK 1.1. Additional reason why java.time classes should be used.

Anyway, I see that you are using Time.valueOf(LocalTIme) (seven out of eight times in hibernate-core), so here is one example ...

jshell> var newYork = Time.valueOf(LocalTime.of(23, 0))
newYork ==> 23:00:00

jshell> newYork.getTime()
$20 ==> 100800000

jshell> Time.valueOf(LocalTime.of(12, 0))
$21 ==> 12:00:00

jshell> $21.getTime()
$22 ==> 61200000

Copy link
Member

@beikov beikov May 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, I see that you are using Time.valueOf(LocalTIme) (seven out of eight times in hibernate-core), so here is one example ...

In fact, java.sql.Time#valueOf(java.time.LocalTime) will interpret the LocalTime as being a time in the time zone of the JVM, which is IMO natural given the way java.sql.Time#getTime() is specified. So to compute the milliseconds for that time since the epoch, it will have to add the time zone offset for the 1st January 1970, which is GMT-5, so it will have to add 18000000 milliseconds to get a GMT value.

I don't know where java.sql.Time#valueOf(java.time.LocalTime) is used in our code, but I agree that we have to be careful not to use that method if the LocalTime value comes from the database, because AFAIU that value would have no time zone association.

Copy link
Contributor Author

@cigaly cigaly May 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know where java.sql.Time#valueOf(java.time.LocalTime) is used in our code, but I agree that we have to be careful not to use that method if the LocalTime value comes from the database, because AFAIU that value would have no time zone association.

org.hibernate.query.hql.internal.SemanticQueryBuilder#sqlTimeLiteralFrom
org.hibernate.type.descriptor.java.JdbcTimeJavaType#wrap
org.hibernate.type.descriptor.java.JdbcTimeJavaType#fromString
org.hibernate.type.descriptor.java.JdbcTimeJavaType#fromEncodedString
org.hibernate.type.descriptor.java.LocalTimeJavaType#unwrap
org.hibernate.type.descriptor.java.OffsetTimeJavaType#unwrap
`org.hibernate.type.descriptor.jdbc.TimeUtcAsJdbcTimeJdbcType#getBinder

Anyway as Yoggi Berra used to say, it's like déjà vu all over again, so I suggest that this PR be closed.

Copy link
Member

@beikov beikov May 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to discourage you on your quest to fix time zone related issues, but we have to be careful to fully understand real problems before doing anything.
I'm sure there is a bug hiding somewhere, but it would be best if we first start off with a real use case that fails.

return time;
}

if ( value instanceof Date date ) {
return new Time( date.getTime() % 86_400_000 );
return millisToSqlTime( date.getTime() );
}

if ( value instanceof LocalTime localTime ) {
Expand All @@ -197,7 +191,7 @@ public Date wrap(Object value, WrapperOptions options) {
}

if ( value instanceof Calendar calendar ) {
return new Time( calendar.getTimeInMillis() % 86_400_000 );
return millisToSqlTime( calendar.getTimeInMillis() );
}

throw unknownWrap( value.getClass() );
Expand Down
24 changes: 12 additions & 12 deletions hibernate-core/src/main/java/org/hibernate/type/descriptor/java/JdbcTimestampJavaType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,18 @@ public Object unwrap(Date value, Class type, WrapperOptions options) {
: new Timestamp( value.getTime() );
}

if ( java.sql.Date.class.isAssignableFrom( type ) ) {
return value instanceof java.sql.Date
? ( java.sql.Date ) value
: new java.sql.Date( value.getTime() );
}

if ( java.sql.Time.class.isAssignableFrom( type ) ) {
return value instanceof java.sql.Time
? ( java.sql.Time ) value
: millisToSqlTime( value.getTime() );
}

if ( Date.class.isAssignableFrom( type ) ) {
return value;
}
Expand All @@ -146,18 +158,6 @@ public Object unwrap(Date value, Class type, WrapperOptions options) {
return value.getTime();
}

if ( java.sql.Date.class.isAssignableFrom( type ) ) {
return value instanceof java.sql.Date
? ( java.sql.Date ) value
: new java.sql.Date( value.getTime() );
}

if ( java.sql.Time.class.isAssignableFrom( type ) ) {
return value instanceof java.sql.Time
? ( java.sql.Time ) value
: new java.sql.Time( value.getTime() % 86_400_000 );
}

throw unknownUnwrap( type );
}

Expand Down
4 changes: 2 additions & 2 deletions hibernate-core/src/main/java/org/hibernate/type/descriptor/java/LocalDateTimeJavaType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,12 +97,12 @@ public <X> X unwrap(LocalDateTime value, Class<X> type, WrapperOptions options)

if ( java.sql.Date.class.isAssignableFrom( type ) ) {
Instant instant = value.atZone( ZoneId.systemDefault() ).toInstant();
return (X) java.sql.Date.from( instant );
return (X) new java.sql.Date( instant.toEpochMilli() );
}

if ( java.sql.Time.class.isAssignableFrom( type ) ) {
Instant instant = value.atZone( ZoneId.systemDefault() ).toInstant();
return (X) java.sql.Time.from( instant );
return (X) millisToSqlTime( instant.toEpochMilli() );
}

if ( Date.class.isAssignableFrom( type ) ) {
Expand Down
4 changes: 2 additions & 2 deletions hibernate-core/src/main/java/org/hibernate/type/descriptor/java/OffsetDateTimeJavaType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,11 +158,11 @@ public <X> X unwrap(OffsetDateTime offsetDateTime, Class<X> type, WrapperOptions
}

if ( java.sql.Date.class.isAssignableFrom( type ) ) {
return (X) java.sql.Date.from( offsetDateTime.toInstant() );
return (X) new java.sql.Date( offsetDateTime.toInstant().toEpochMilli() );
}

if ( java.sql.Time.class.isAssignableFrom( type ) ) {
return (X) java.sql.Time.from( offsetDateTime.toInstant() );
return (X) millisToSqlTime( offsetDateTime.toInstant().toEpochMilli() );
}

if ( Date.class.isAssignableFrom( type ) ) {
Expand Down
4 changes: 4 additions & 0 deletions hibernate-core/src/main/java/org/hibernate/type/descriptor/java/OffsetTimeJavaType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,10 @@ public <X> X unwrap(OffsetTime offsetTime, Class<X> type, WrapperOptions options
return (X) Long.valueOf( instant.toEpochMilli() );
}

if ( java.sql.Date.class.isAssignableFrom( type ) ) {
throw new IllegalArgumentException( "Illegal attempt to treat `java.time.OffsetTime` as `java.sql.Date`" );
}

if ( Date.class.isAssignableFrom( type ) ) {
return (X) Date.from( instant );
}
Expand Down
4 changes: 2 additions & 2 deletions hibernate-core/src/main/java/org/hibernate/type/descriptor/java/ZonedDateTimeJavaType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,11 +120,11 @@ public <X> X unwrap(ZonedDateTime zonedDateTime, Class<X> type, WrapperOptions o
}

if ( java.sql.Date.class.isAssignableFrom( type ) ) {
return (X) java.sql.Date.from( zonedDateTime.toInstant() );
return (X) new java.sql.Date( zonedDateTime.toInstant().toEpochMilli() );
}

if ( java.sql.Time.class.isAssignableFrom( type ) ) {
return (X) java.sql.Time.from( zonedDateTime.toInstant() );
return (X) millisToSqlTime( zonedDateTime.toInstant().toEpochMilli() );
}

if ( Date.class.isAssignableFrom( type ) ) {
Expand Down
Loading