Skip to content
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.

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

hibernate.jdbc.time_zone unsupported on TIME columns #856

Closed
pqab opened this issue Jun 14, 2021 · 21 comments · Fixed by #857
Closed

hibernate.jdbc.time_zone unsupported on TIME columns #856

pqab opened this issue Jun 14, 2021 · 21 comments · Fixed by #857
Assignees
Labels
problem A limitation or source of discomfort
Milestone

Comments

@pqab
Copy link

pqab commented Jun 14, 2021

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

  • Version: 1.0.0.CR6
@DavideD
Copy link
Member

DavideD commented Jun 14, 2021

In theory it should already work :-)
#667

I will have a look

@DavideD
Copy link
Member

DavideD commented Jun 14, 2021

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
Copy link
Member

DavideD commented Jun 14, 2021

Nevermind.... I can see the exception

@pqab
Copy link
Author

pqab commented Jun 14, 2021

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 )

@DavideD DavideD self-assigned this Jun 14, 2021
@DavideD DavideD added the bug Something isn't working label Jun 14, 2021
@DavideD DavideD added this to the 1.0.0.CR7 milestone Jun 14, 2021
@gavinking
Copy link
Member

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 gavinking added problem A limitation or source of discomfort and removed bug Something isn't working labels Jun 14, 2021
@gavinking
Copy link
Member

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

@gavinking
Copy link
Member

(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
Copy link
Member

I started a discussion here.

@DavideD
Copy link
Member

DavideD commented Jun 14, 2021

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
Copy link
Member

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
Copy link
Member

DavideD commented Jun 14, 2021

"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
Copy link
Member

gavinking commented Jun 14, 2021

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
Copy link
Member

gavinking commented Jun 14, 2021

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
Copy link
Member

gavinking commented Jun 14, 2021

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
Copy link
Member

gavinking commented Jun 14, 2021

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

@gavinking
Copy link
Member

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

And same question for @Ngosti2000

@DavideD
Copy link
Member

DavideD commented Jun 14, 2021

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
Copy link
Member

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
Copy link
Member

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

Not that I know of.

@pqab
Copy link
Author

pqab commented Jun 14, 2021

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 added a commit to DavideD/hibernate-reactive that referenced this issue Jun 14, 2021
We cannot apply a timezone to a time without a date,
this probably the best we can do without throwing
an exception.
DavideD added a commit to DavideD/hibernate-reactive that referenced this issue Jun 14, 2021
DavideD added a commit to DavideD/hibernate-reactive that referenced this issue Jun 14, 2021
DavideD added a commit to DavideD/hibernate-reactive that referenced this issue Jun 15, 2021
We cannot apply a timezone to a time without a date,
this probably the best we can do without throwing
an exception.
DavideD added a commit to DavideD/hibernate-reactive that referenced this issue Jun 15, 2021
DavideD added a commit to DavideD/hibernate-reactive that referenced this issue Jun 15, 2021
DavideD added a commit to DavideD/hibernate-reactive that referenced this issue Jun 15, 2021
DavideD added a commit to DavideD/hibernate-reactive that referenced this issue Jun 15, 2021
DavideD added a commit to DavideD/hibernate-reactive that referenced this issue Jun 15, 2021
DavideD added a commit that referenced this issue Jun 15, 2021
We cannot apply a timezone to a time without a date,
this probably the best we can do without throwing
an exception.
@DavideD
Copy link
Member

DavideD commented Jun 15, 2021

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
problem A limitation or source of discomfort
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants