hibernate / hibernate-reactive

A reactive API for Hibernate ORM, supporting non-blocking database drivers and a reactive style of interaction with the database.
https://hibernate.org/reactive
Apache License 2.0
441 stars 92 forks source link

hibernate.jdbc.time_zone unsupported on TIME columns #856

Closed pqab closed 3 years ago

pqab commented 3 years ago

Hi, we are trying to add the property <property name="hibernate.jdbc.time_zone" value="UTC"/> to our application, but we got UnsupportedOperationException on some MySQL TIME columns, may I know is there any plan to support that?

Stacktrace

java.lang.UnsupportedOperationException: null at org.hibernate.reactive.adaptor.impl.PreparedStatementAdaptor.setTime(PreparedStatementAdaptor.java:246) Suppressed: reactor.core.publisher.FluxOnAssembly$OnAssemblyException:

Environment Information

DavideD commented 3 years ago

In theory it should already work :-) https://github.com/hibernate/hibernate-reactive/issues/667

I will have a look

DavideD commented 3 years ago

Can you past an example of an entity you are using? A reproducer would be even better. You can use JBang to start an example testcase:

jbang init -t mysql-reproducer@hibernate/hibernate-reactive Issue856.java
jbang edit --open=idea --live Issue856.java

You can replace idea with your favourite IDE

DavideD commented 3 years ago

Nevermind.... I can see the exception

pqab commented 3 years ago

for more deatils, we are using LocalTime in the Entity and TIME in the database

Entity public class A { @Column(name = "time") private LocalTime time; }

Table CREATE TABLE IF NOT EXISTS A ( time TIME )

gavinking commented 3 years ago

So the issue here is that the notion of a zoned time is entirely broken and should never have been introduced in SQL (fortunately most databases don't support it). The mapping between timezones depends on the date, and so only zoned datetimes make sense.

Until this moment I had no idea about the behavior of hibernate.jdbc.time_zone, which looks pretty bad to me, at first sight, and perhaps we should change it in H6.

So I'm not sure what to do here. "Fix" it to do the same bad thing as ORM, or just ignore the setting when working with times?

Or just ignore the property altogether, and figure out how to do this using a connection property instead? (Might need a new feature in the Vert.x client, I'm not sure.)

gavinking commented 3 years ago

I'm removing the "bug" label. It's not a bug that zoned times don't work. They're broken by design.

gavinking commented 3 years ago

(This is of course partly my fault for not digging deeper when I did #676 and noticing the problem with the behavior of hibernate.jdbc.time_zone.)

gavinking commented 3 years ago

I started a discussion here.

DavideD commented 3 years ago

The mapping between timezones depends on the date, and so only zoned datetimes make sense.

Ok, but they are trying to store a LocalTime that doesn't have any timezone attached and shouldn't require any convertion. Am I missing something?

gavinking commented 3 years ago

But that's not what this property does, it seems. In ORM it attaches a timezone to the thing.

At least, it seems to me. (I have never been able to completely understand the semantics of that JDBC method.)

DavideD commented 3 years ago

"Fix" it to do the same bad thing as ORM

I think I will check what ORM does and do the same. It's probably the behaviour that users tjhat are familiar with ORM will expect anyway.

gavinking commented 3 years ago

I think I will check what ORM does and do the same.

hahahaha oh sweet summer child!

ORM calls a JDBC method which has essentially undocumented semantics.

(And probably behaves differently on every database.)

gavinking commented 3 years ago

So to recap part of the discussion with @yrodiere on Zulip:

  1. He says this stuff is all very fragile (and probably broken) in ORM to begin with, partly because all the JDBC drivers do fragile and different things.
  2. What I decided to do for handling Timestamps with hibernate.jdbc.time_zone (which tries to convert the given Timestamp to the given timezone before passing it to the Vert.x SQL client) might or might not be robust, but it certainly doesn't seem conceptually correct for a Time.

So the options are (1) just silently ignore the setting, (2) blow up when the setting is present, or (3) do something we know is broken.

I'm inclined to go with (2).

gavinking commented 3 years ago

I suppose we could keep doing what we're currently doing for datetimes, and just ignore the setting which it comes to times. But geez, that amounts to introducing a new and differently-broken interpretation of a feature which is already kinda broken. Brrr.

gavinking commented 3 years ago

@pqab why precisely do you use this setting? Could you reasonably, like, just not use it?

gavinking commented 3 years ago

@pqab why precisely do you use this setting? Could you reasonably, like, just _not_use it?

And same question for @Ngosti2000

DavideD commented 3 years ago

To be fair, I didn't expect it to be applied to objects that don't have time zone like LocalTime.

I suspect the problem is that it's a global property and if you set it because it makes sense in other cases I don't think there is a way to disable it for specific fields. Or maybe there is?

gavinking commented 3 years ago

To be fair, I didn't expect it to be applied to objects that don't have time zone like LocalTime.

Right, same. We assumed too much sanity.

gavinking commented 3 years ago

I don't think there is a way to disable it for specific fields. Or maybe there is?

Not that I know of.

pqab commented 3 years ago

we have some others DATE fields need the global property, but we expected that the TIME field should be ignored because it doesn't have time zone

DavideD commented 3 years ago

Done