OffsetDateTimeTypeHandler or ZonedDateTimeTypeHandler loses time zone information

[kazuki43zoo]

Hello,
I would assume that people using your implementation of a "ZonedDateTimeTypeHandler" expect it to convert dates with timezone information in their database to EQUIVALENT Java objects, which it does not (really).

It merely converts them to objects that, technically point to the same moment in time, however with the zone or offset information completely lost. All the ZonedDateTime or OffsetDateTime objects that the handlers create always refer to the system default time zone, never to the one of the actual entry!

Instead of only using the getTimestamp() method of the ResultSet (which can only return a java.sql.Timestamp with the zone information already lost) it would be cool if you could at least offer an alternative implementation that tries to retain the offset/timezone information by using the getString() method instead.

An example for the OffsetDateTimeTypeHandler could be:

@Override
public OffsetDateTime getNullableResult(ResultSet rs, String columnName) throws SQLException {
  String dateString = rs.getString(columnName)
  if(dateString == null) {
    return null;
  }
  OffsetDateTime offsetDateTime = OffsetDateTime.parse(dateString, DateTimeFormatter.ISO_OFFSET_DATE_TIME);
  return offsetDateTime;
}

I found the idea here. Of course, this would assume that the String representation of the databases for those "timestamp with time zone" datatypes comply with the ISO format. Instead of guessing users could hand over the format-mask to a method that converts to the String representation of that column in the select statement. E.g. in Oracle that could be:
SELECT to_char(table0.MY_DATE_COLUMN, 'YYYY-MM-DD"T"HH24:MI:SSFFTZH:TZM') as MY_DATE_COLUMN). Such a remark could be added in the class documentation of the type handler, for example.

Even if you do not want to add such handlers, you should at least mention in your documentation (class documentation and README.MD) that the handlers you provide do NOT retain the information of the original timezone or the original offset but do always convert them to the system's default.

Cheers!

Note

This issue moved from mybatis/typehandlers-jsr310#39.

[raupachz]

I agree. These cases are not well covered at the moment. Not sure if TypeHandlers support parameters, but I like the idea you proposed.

JDBC 4.2 (Java 8 only) does support TIME_WITH_TIMEZONE and TIMESTAMP_WITH_TIMEZONE JDBC Types. It maps them to java.time.OffsetTime and java.time.OffsetDateTime. Maybe that is something to look into as well.

There is no support for java.time.ZonedDateTime which is unfortunate but reasonable. Timezones can change (dropping daylight savings time for example) explicit offsets do not. Maybe it was a bad idea to provide a type handler for java.time.ZonedDateTime to begin with.

AFAIK if you need to support clients in different timezones there is no way around storing all time related data types in UTC and have the clients do the proper conversion to their local time.

Edit
Did some playing around. HSQLDB 2.4 has a TIMESTAMP_WITH_TIMEZONE data type. If we use setObject instead of 'setTimestamp' in the OffsetDateTimeHandler the offset is preserved.

Could create a proper pull request but I would like to get some more feedback on this issue.

Reference: https://github.com/raupachz/mybatis-3/tree/ISSUE-1081

[Treppenhouse]

Sorry for the late reply but here is some feedback from me:

I think the OffsetDateTimeTypeHandler and the ZonedDateTimeTypeHandler are definitely broken. They silently will map your TIMESTAMP result to a OffsetDateTime object but simply apply the offset of the current system's default. That might completely change the data and there is not even a warning about this!!

Also, the current JUnit test "covering" this type handler does not have a fixed date that is being converted but simply uses the current date (and time zone) from the computer that is executing the JUnit test. Of course it will work then!
The test input should be completely fixed (like a fixed point in time) and not being machine-dependent. If we changed this, the test would fail.

So at the very least we should:

1. update the documentation of these type handlers to explicitly state its shortcomings and "warn" the users any information about time zone offsets is lost if they use this type handler.
2. provide alternative implementations that do retain the offset information and perhaps not put them in the TypeHandlerRegistry but let the users chose them explicitly and maybe replace the default type handler later.

Here are the two alternative implementations I would suggest:

1. The one that makes use of JDBC 4.2 and uses getObject() / setObject() as you suggested it.
2. The one that uses the "workaround" of formatting the column to a ISO String first, then retrieving it via getString() and parsing it eventually in the type handler back to a OffsetDateTime object (It is the one propsed in the initial issue text and copied from the original issue of the jsr-310 project where I published it). I am currently using that in production and it works fine.

What do you think? I can create a pull request for the second one and maybe you for the first one?

[raupachz]

I agree with OffsetDateTimeTypeHandler and the ZonedDateTimeTypeHandler being broken.

In my opinion the best approach in storing both Temporals as String columns in the database: most portable and straight forward. In the TypeHandler one should use ZonedDateTime.parse() and OffsetDateTime.parse to convert from the stored characters representation to the Temporal object.

YearMonthTypeHandler use this strategy, too.

Since this change could break applications it is probably a fix for a MyBatis 3.5.x release.

[petarov]

Currently working w/h MSSQL 2017 here and I can confirm that the JDBC 4.2 getObject() / setObject() implementation works fine. In fact, I'm using a custom handler to solve the timezones problem discussed here.

I tend to agree with @Treppenhouse's comment and I'd like to add that IMHO the OffsetDateTimeTypeHandler and ZonedDateTimeTypeHandler should be revised to work with getObject() / setObject(). The old handler implementations should be renamed to something with a suffix, like Legacy, and the additional String columns implementation could be added as additional handlers.

I know this could be a breaking change, but it seems to me the most logical. Again, that being said as a humble user opinion.

---

Just for reference. New JDBC 4.2 supported types as documented in:

• PostgreSQL
• MSSQL
• MySQL

[harawata]

It might be faster to proceed step by step. :)

• OffsetDateTimeTypeHandler should be rewritten using getObject() / setObject().

Reading the comments, you all agreed on this one, I believe.
@raupachz , Could you rebase the branch and send it as a PR, please?

I'll comment on the other ideas later.

[raupachz]

Hi @harawata oh wow I almost forgot about this branch. Yes, I will rebase and send a PR. Thanks!

[harawata]

@raupachz ,
Thank you!
I'll review it as soon as I find the time.

@petarov ,
Could you post your OffsetDateTimeTypeHandler to Gist or somewhere?
I'm testing it with a simple JDBC app and although setObject() seems to be working, getObject(columnIndex, OffsetDateTime.class) results in an error.

Exception in thread "main" com.microsoft.sqlserver.jdbc.SQLServerException: The conversion to class java.time.OffsetDateTime is unsupported.
        at com.microsoft.sqlserver.jdbc@7.1.1.jre10-preview/com.microsoft.sqlserver.jdbc.SQLServerResultSet.getObject(SQLServerResultSet.java:2428)

My environment:

• SQL Server 2017 Express Edition (14.00.1000)
• mssql-jdbc 7.1.1.jre10-preview
• Column datatype is DATETIMEOFFSET
• openjdk version "11" 2018-09-25

[petarov]

@harawata @raupachz No need to provide a gist, because the exception you posted is totally real! I need to apologise for my misleading comment above. I checked my logs yesterday and I did see the same exception, but I only got to review what's going on today.

So as it turns out Microsoft's JDBC did in fact implement the setObject() call for OffsetDateTime, but they did not implement anything like that for getObject(). That returns an microsoft.sql.DateTimeOffset type instead, which I had to convert to java.time.OffsetDateTime in my custom TypeHandler. 😞

Needless to say, this defeats the purpose of a uniform setObject() / getObject() TimeTypeHandler implementation. I think this brings the topic back to @raupachz's comment about Temporals as String columns.

---

For anyone that is more interested in the topic this is what I found.

• Microsoft's getObject()
• Microsoft's OffsetDateTime conversion
• MySQL does not support OffsetDateTime at all - [1], [2], [3]
• Postgres' JDBC seems like the most complete implementation. It supports OffsetDateTime conversion - [1], [2], [3] and also getObject()

[raupachz]

@petarov Thanks for the detailed investigation! I always thought with JDBC 4.2 this should be easily done. Guess, not. Maybe, we better close the PR. Doesn't make sense to add a feature if only half of the db vendors implement the spec.

Side note: I checked the source of MariaDB Connector/J they implement getObject / setObject.

[harawata]

@petarov ,
Thank you for the clarification!
I have added my test result on #1368 .
PostgreSQL does support these methods, but the value is stored in UTC, it seems (i.e. offset is lost).
And I'm pretty sure mssql-jdbc will support ResultSet#getObject() in the future.

@raupachz ,
Good catch! I have just checked mariadb and it seems to be working with both 'DATETIME' and 'TIMESTAMP' columns.

I'm going to test PreparedStatement#setString() and ResultSet#getString() next.

[harawata]

PreparedStatement#setString("ISO formatted string") does not work at all. XD
(Note that I tested with the 'TIMESTAMP WITH TIME ZONE' columns, not the CHAR/VARCHAR columns.)

---

Anyway, getObject() / setObject() seems to work with most DBs that have TIMESTAMP WITH TIME ZONE [1], so I thought it was OK, but you guys think that OffsetDateTimeTypeHandler should map the object to text columns?

I'm actually not a big fan of that approach.
There may be cases that justify the usage, but in general, we should avoid storing date/time value in text columns, IMHO.
I would rather convert it to UTC and store it into TIMSTAMP or DATETIME columns if my DB does not support TIMESTAMP WITH TIME ZONE (think about querying the data, for example).

I'm curious about what other devs think.

[1] Not with MS SQL Server right now, but @petarov shared a decent workaround.

[petarov]

but you guys think that OffsetDateTimeTypeHandler should map the object to text columns?

I personally see this as a separate handler, something like OffsetDateTimeTypeAsStringHandler. I think the OffsetDateTimeTypeHandler should either use getObject() / setObject() as you suggest or be deprecated in order to not mislead developers that timezones save would always work.

I'm also not sure what's to be done with ZonedDateTimeTypeHandler. From what I've read so far java.time.ZoneDateTime isn't supported at all in JDBC (except with MariaDB's Connector, are there others?), but maybe it's possible to convert that to java.time.OffsetDateTime and then still use getObject() / setObject(). 🤔

Btw, big thanks for your test results in #1368. This is awesome! 🍺

[harawata]

Adding another type handler that maps OffsetDateTime to string columns sounds OK.
I would wait until someone actually requests it, though.
Please let me know if any of you is already in need of such type handler.

This is just my personal view, but type handlers for these classes (i.e. OffsetDateTime, OffsetTime and ZonedDateTime) are useful only when the database supports the corresponding data types (in that sense, they are similar to SqlXmlTypeHandler ( #1221 ) which works only with a DB that supports SQLXML data type).
And if/when these data types are supported, setObject() / getObject() are the standard (or de-facto standard?) way to access them with the current JDBC.
By this logic, I think it's right to use setObject()/getObject() in ZonedDateTimeTypeHandler or OffsetTimeTypeHandler even if they work with few (or no) DBs right now.

As this is just my personal opinion and could be impractical, I would like to wait for other committers' comment before we proceed.
Good thing about type handler is that it's fairly easy to write/use a custom type handler. :)

Btw, big thanks for your test results in #1368. This is awesome! 🍺

You're very welcome. I learned a lot from this discussion, too. =)

FYI, I noticed that there is a bug in mssql-jdbc's PreparedStatement#setObject() that occurs when the passed OffsetDateTime has a different time zone than the default one and sent a fix.
I also sent another PR that adds OffsetDateTime support to ResultSet#getObject().
Not sure when or if they're accepted, though.

[petarov]

@harawata Nice! I hope they pull it in. It would save me from using a custom handler. It's currently all over my mappers. 😉

Btw, this was my routine to convert from dto to odt.

	private OffsetDateTime toOffsetDateTime(DateTimeOffset dto) {
		if (dto != null) {
			return OffsetDateTime.ofInstant(dto.getTimestamp().toInstant(),
			        ZoneOffset.ofHours(dto.getMinutesOffset() / 60));
		}
		return null;
	}

[harawata]

That's my plan B if they don't like me modifying DateTimeOffset.
ZoneOffset.ofHoursMinutes(dto.getMinutesOffset()) might be safer as there seems to be a timezone like '+08:45' in the world. 🌎

It's currently all over my mappers.

It might not be what you meant, but instead of specifying typeHandler in each mapper, you can register the custom type handler in the global config.

[harawata]

Hi all,

#1368 has been merged.
I plan to update OffsetTimeTypeHandler in the similar manner as some drivers support the type.

@petarov ,
The latest 3.5.0-SNAPSHOT + mssql-jdbc 7.1.4 preview should work as you expect. :D

[harawata]

OffsetTimeTypeHandler #1437 and ZonedDateTimeHandler #1438 have been updated to use getObject() and setObject().
If anyone wants alternative type handlers with String conversion, please open a new issue so that we can discuss the spec in detail.
Thank you all for sharing thoughts, opinions and code!

[petarov]

@harawata I've tested the OffsetTimeTypeHandler with SNAPSHOT-3.5.0 and mssql 7.1.4.jre11-preview and so far everything runs without any issues. getObject() and setObject() seem to handle everything perfectly! Will try to do some ZonedDateTime tests a bit later.

Thanks a lot for this! 🍻

[harawata]

@petarov Thank you for verifying the fix!
It's really great that @microsoft open-sourced the driver.
Hope other vendors are considering it, too. ;D

[gsbtech]

I just upgraded to mybatis 3.5.0 and the ZonedDateTimeTypeHandler is not working.
As a mater of fact, with the previous verion, 3.4.6, I include the jar
org.mybatis:mybatis-typehandlers-jsr310
and it has been working great. Somehow, my code got rearranged, and got the typehandler class from mybatis .jar instead of from the mybatis-typehandlers .jar and even though the java package is the same, the code is different.
The mybatis-typehandlers works fine, mybatis 5.0 typehandler does not work.