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

I found some more shady JDK methods (story of my life) #125

Open
hakanai opened this issue Jul 20, 2017 · 4 comments
Open

I found some more shady JDK methods (story of my life) #125

hakanai opened this issue Jul 20, 2017 · 4 comments

Comments

@hakanai
Copy link

hakanai commented Jul 20, 2017

Many databases (MS SQL Server and Derby confirmed, probably many others as well) store the date/time values in local time with no zone, so for methods which set date values without passing a time zone it's unclear which value you're going to be storing. Often you have to read the documentation for the network protocol to know what the database will use...

But it turns out that all the following methods also have a second overload taking java.util.Calendar, which of course allows passing in an explicit time zone, so maybe they should just be added to the list.

java.sql.CallableStatement#getDate(int)
java.sql.CallableStatement#getTime(int)
java.sql.CallableStatement#getTimestamp(int)

java.sql.CallableStatement#getDate(java.lang.String)
java.sql.CallableStatement#getTime(java.lang.String)
java.sql.CallableStatement#getTimestamp(java.lang.String)

java.sql.ResultSet#getDate(int)
java.sql.ResultSet#getTime(int)
java.sql.ResultSet#getTimestamp(int)

java.sql.ResultSet#getDate(java.lang.String)
java.sql.ResultSet#getTime(java.lang.String)
java.sql.ResultSet#getTimestamp(java.lang.String)

java.sql.CallableStatement#setDate(java.lang.String, java.sql.Date)
java.sql.CallableStatement#setTime(java.lang.String, java.sql.Time)
java.sql.CallableStatement#setTimestamp(java.lang.String, java.sql.Timestamp)

java.sql.PreparedStatement#setDate(int, java.sql.Date)
java.sql.PreparedStatement#setTime(int, java.sql.Time)
java.sql.PreparedStatement#setTimestamp(int, java.sql.Timestamp)

javax.sql.rowset.BaseRowSet#setDate(int, java.sql.Date)
javax.sql.rowset.BaseRowSet#setTime(int, java.sql.Time)
javax.sql.rowset.BaseRowSet#setTimestamp(int, java.sql.Timestamp)

javax.sql.rowset.BaseRowSet#setDate(java.lang.String, java.sql.Date)
javax.sql.rowset.BaseRowSet#setTime(java.lang.String, java.sql.Time)
javax.sql.rowset.BaseRowSet#setTimestamp(java.lang.String, java.sql.Timestamp)

javax.sql.RowSet#setDate(int, java.sql.Date)
javax.sql.RowSet#setTime(int, java.sql.Time)
javax.sql.RowSet#setTimestamp(int, java.sql.Timestamp)

javax.sql.RowSet#setDate(java.lang.String, java.sql.Date)
javax.sql.RowSet#setTime(java.lang.String, java.sql.Time)
javax.sql.RowSet#setTimestamp(java.lang.String, java.sql.Timestamp)
@uschindler
Copy link
Member

Hi,
I know those already - the problem is that it depends on the database if they are broken. In addition, it is much more shitty alltogether: If you call getObject(), you may get back a Date/Time/Timestamp without any ability to detect it by static code analysis.
BTW: In my own code, I always have a ThreadLocal with a GregorianCalendar(UTC) to call those methods:

public static final ThreadLocal<Calendar> UTC_CAL =
  ThreadLocal.withInitial(() -> new GregorianCalendar(TimeZone.getTimeZone(ZoneOffset.UTC), Locale.ROOT));

//...
result.getTimestamp(col, UTC_CAL.get());

The threadlocal is needed because the Calendar on its own is not thread safe (urgh).

I am not sure if we can easily add those to the general unsafe ones. Mabye add another bundled signature?

@uschindler
Copy link
Member

FYI,
the Timestamp class and their Date/Time ones have more methods that use local timezone. E.g. the ones that print with toString() but also the new ones in Java 8 that convert to java.time.Local*. It's broken altogether. IMHO, JDBC should be vanished and reimplemented!

@hakanai
Copy link
Author

hakanai commented Jul 20, 2017

The thread-local Calendar is the solution I was planning to implement too. But if I try to put that in a utility method somewhere, Calendar has one other trap, which is that it's mutable. So it would either have to hand out cloned copies, or reset some of the state each time someone asks for one.

I guess the catch is that even if our code currently calls a database where the method is safe, having used JDBC makes future developers think that it will work correctly on another database, so they might happily switch databases without noticing that it's not working correctly. (It took us over 10 years to discover this issue in the first place, in Derby, where we had incorrectly assumed that it was storing timestamps as, well, a timestamp.)

And yeah, Timestamp and its friends are weird. Timestamps in the actual database tend to be in local time, not millisecond values, but then Java implements Timestamp as only a millisecond value, forcing everyone to convert. Then they later implement conversion back to types which are local time again, and we're supposed to trust that both sides of that calculation will be using the same zone. And then java.time comes along, and the API doesn't even get updated to support them directly - you're expected to use getObject/setObject on drivers which support it, which has the problems already mentioned.

So you're right, they should burn JDBC to the ground and try again. Maybe next time they'll get it right. In the meantime I currently have all the bad stuff calling through static utility methods, but it seems easy to forget to call those, so I'm thinking I might have to wrap the entire API to discourage doing things the wrong way. :/

@uschindler
Copy link
Member

And yeah, Timestamp and its friends are weird. Timestamps in the actual database tend to be in local time, not millisecond values, but then Java implements Timestamp as only a millisecond value, forcing everyone to convert.

The worst here is: You fetch a value from the database, it is converted according to local timezone. When you then update it and store it again using a prepared statement it is converted back to a local time. The problem is: If DST changes apply, the whole process is not bidirectional! We figured out that at the time when DST changes apply (one hour in spring and one hour in autums), all your database values break because the conversion database -> long -> database is not returning the same value as before! This is the reason why I tend to always use UTC as timezone for calculations.

Yes, JDBC should use LocalDateTime, LocalDate, LocalTime instances from the java.time API. The problem with JDBC is: It's all interfaces, so you cannot change without breaking all drivers out there!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants