Testing OffsetDateTimeHandler which retains Offset from UTC #1368

raupachz · 2018-10-10T07:55:03Z

PR for the issue in #1081.

Changes OffsetDateTimeHandler now uses getObject and setObject. This retains the offset. Before that we used setTimestamp which looses the offset.

This branch in my fork is over a year old. Not sure if everything is done well. Looking for feedback.

harawata · 2018-10-11T11:37:22Z

I wrote a JDBC test that does the following.

Inserting OffsetDateTime.of(2018, 10, 8, 11, 22, 33, 123456789, ZoneOffset.ofHoursMinutes(10, 20)) [5] using PreparedStatement#setObject(int, Object).
Select the inserted timestamp value with ResultSet#getObject(int, Class).

DB	Driver	Column type	Returned value	c.f.
HSQLDB 2.4.1	2.4.1	TIMESTAMP WITH TIME ZONE	2018-10-08T11:22:33.123456+10:20
PostgreSQL 10.4	42.2.4	TIMESTAMP WITH TIME ZONE	2018-10-08T01:02:33.123457Z	[6]
Oracle 12c 12.2.0.1.0	12.2.0.1.0	TIMESTAMP WITH TIME ZONE	2018-10-08T11:22:33.123457+10:20
MySQL 5.7.22	8.0.11	-	-	[1]
DB2 for LUW 11.1	4.24.92	-	-	[2]
MS SQL Server 2017 (Express Edition) 14.00.1000	7.1.4.0 (jre11-preview)	DATETIMEOFFSET	2018-10-08T11:22:33.123456800+10:20	[3]
SAP ASE 16	jconn4.jar	-	-	[4]
H2 1.4.197	1.4.197	TIMESTAMP WITH TIME ZONE	2018-10-08T11:22:33.123457+10:20
Maria DB 10.3.8	2.3.0	TIMESTAMP(6) or DATETIME(6)	2018-10-08T10:02:33.123456+09:00	[5]

[1] "The types OffsetDateTime and OffsetTime are partially supported via conversion to *CHAR types as MySQL doesn’t provide support for temporal data containing time zone information and, thus, doesn’t support the new TIME_WITH_TIMEZONE and TIMESTAMP_WITH_TIMEZONE JDBC types as well." link
getObject() internally calls OffsetDateTime.parse(), but there seems to be no corresponding OffsetDateTime to text conversion for setObject().
[2] "The data type TIMESTAMP WITH TIME ZONE is a DB2 for zOS feature, not a Db2 LUW" link
[3] ResultSet#getObject(int, Class) does not support OffsetDateTime (yet). With Microsoft SQL Server Management Studio, the inserted data is shown as '2018-10-08 12:42:33.1234568 +10:20' which is incorrect. I reported it as a bug. [EDIT] These issues were resolved in 7.1.4.
[4] "The datetime and smalldatetime datatypes, however, do not store time zone information and ASE is entirely ignorant of the concepts of time zones and daylight savings time." link
[5] Offset is not stored. On insertion, OffsetDateTime is converted to local datetime of system's default timezone. On retrieval, the offset of system's default timezone is added to the stored local datetime. This means that you must use the same default timezone for inserting and selecting OffsetDateTime. Otherwise, you'll get an unexpected result. '+09:00' is my system's time zone.
[6] Offset is not stored. The data is stored as UTC.

In case anyone is interested, the app is on my Gist.
https://gist.github.com/harawata/29762e8216ff68b089a582b6831595b9

harawata

Thank you for the PR, @raupachz and I'm sorry about the delay!
I now feel confident enough to proceed with merging this PR.
Could you check my reviews?

If you are busy, please just let me know and I may be able to add commits to this PR myself.

harawata · 2018-10-11T07:07:16Z

...apache/ibatis/submitted/usesjava8/timestamp_with_timezone/OffsetDateTimeTypeHandlerTest.java

+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+package org.apache.ibatis.submitted.usesjava8.timestamp_with_timezone;


Please move the files to org.apache.ibatis.submitted.timestamp_with_timezone.

We no longer use usesjava8 sub package because Java 8 will be the minimum required version in MyBatis 3.5.0.

harawata · 2018-10-11T07:07:34Z

...apache/ibatis/submitted/usesjava8/timestamp_with_timezone/OffsetDateTimeTypeHandlerTest.java

+import org.apache.ibatis.session.SqlSession;
+import org.apache.ibatis.session.SqlSessionFactory;
+import org.apache.ibatis.session.SqlSessionFactoryBuilder;
+import org.apache.ibatis.submitted.usesjava8.use_actual_param_name.User;


Unused import.

Suggested change

import org.apache.ibatis.submitted.usesjava8.use_actual_param_name.User;

harawata · 2018-10-11T07:07:58Z

src/test/java/org/apache/ibatis/submitted/usesjava8/timestamp_with_timezone/Mapper.java

+ */
+package org.apache.ibatis.submitted.usesjava8.timestamp_with_timezone;
+
+import org.apache.ibatis.submitted.usesjava8.use_actual_param_name.*;


Unused import.

Suggested change

import org.apache.ibatis.submitted.usesjava8.use_actual_param_name.*;

harawata · 2018-10-11T07:13:35Z

src/main/java/org/apache/ibatis/type/OffsetDateTimeTypeHandler.java

  }

  @Override
  public OffsetDateTime getNullableResult(ResultSet rs, String columnName) throws SQLException {
-    Timestamp timestamp = rs.getTimestamp(columnName);
-    return getOffsetDateTime(timestamp);
+    return (OffsetDateTime) rs.getObject(columnName);


Specify the second argument of getObject() instead of cast.

Suggested change

return (OffsetDateTime) rs.getObject(columnName);

return rs.getObject(columnName, OffsetDateTime.class);

harawata · 2018-10-11T07:14:43Z

src/test/java/org/apache/ibatis/submitted/usesjava8/timestamp_with_timezone/Mapper.xml

+
+<mapper namespace="org.apache.ibatis.submitted.usesjava8.timestamp_with_timezone.Mapper">
+
+</mapper>


Empty file. Please delete it.

harawata · 2018-10-11T07:20:16Z

src/main/java/org/apache/ibatis/type/OffsetDateTimeTypeHandler.java

@@ -20,6 +20,7 @@
 import java.sql.ResultSet;
 import java.sql.SQLException;
 import java.sql.Timestamp;
+import java.sql.Types;
 import java.time.OffsetDateTime;
 import java.time.ZoneId;


Some imports are not used.

harawata · 2018-12-18T12:24:02Z

src/main/java/org/apache/ibatis/type/OffsetDateTimeTypeHandler.java

  }

  @Override
  public OffsetDateTime getNullableResult(ResultSet rs, int columnIndex) throws SQLException {
-    Timestamp timestamp = rs.getTimestamp(columnIndex);
-    return getOffsetDateTime(timestamp);
+    return (OffsetDateTime) rs.getObject(columnIndex);


Specify the second argument of getObject() instead of cast.

Suggested change

return (OffsetDateTime) rs.getObject(columnIndex);

return rs.getObject(columnIndex, OffsetDateTime.class);

harawata · 2018-12-18T12:25:32Z

src/main/java/org/apache/ibatis/type/OffsetDateTimeTypeHandler.java

  }

  @Override
  public OffsetDateTime getNullableResult(CallableStatement cs, int columnIndex) throws SQLException {
-    Timestamp timestamp = cs.getTimestamp(columnIndex);
-    return getOffsetDateTime(timestamp);
+    return (OffsetDateTime) cs.getObject(columnIndex);


Specify the second argument of getObject() instead of cast.

Suggested change

return (OffsetDateTime) cs.getObject(columnIndex);

return cs.getObject(columnIndex, OffsetDateTime.class);

harawata · 2018-12-18T12:48:26Z

...apache/ibatis/submitted/usesjava8/timestamp_with_timezone/OffsetDateTimeTypeHandlerTest.java

+    } finally {
+      sqlSession.close();
+    }
+  }  


There should be two tests with assertions at least (SELECT and INSERT).

harawata · 2019-01-07T18:24:02Z

@raupachz ,
This should be fixed in 3.5.0, so I'll start working on this tomorrow.
Please let me know if you have already started. =)

harawata · 2019-01-09T01:25:32Z

Merged. Thank you, @raupachz !

kazuki43zoo · 2019-01-09T03:52:55Z

@harawata Could you set labels?

harawata · 2019-01-09T04:00:38Z

@kazuki43zoo
Done!
Is the milestone OK? I'm a little bit unsure how to set milestone when there is both an issue and a PR on the same subject.

kazuki43zoo · 2019-01-09T04:47:04Z

@harawata
Thanks! As personally, I prefer that add labels and milestone to an original issue rather than PR. But there is no rule at now ... :(

I think that it is better to decide rules as a team.

harawata · 2019-01-09T15:08:18Z

I see. May I ask why you prefer issues over PRs?

I myself try to choose the one that is more informative as a milestone entry.
And a PR with enhancements, especially, contains the final spec (e.g. new option name, special notes, test cases demonstrate the behavior, etc.) and that's what I would want to see when checking milestone.
In case of a bug, the original issue (sometimes 🙃) contains useful information (e.g. stack trace, conditions to reproduce, environment, etc.), so I might choose it over the PR.

Testing OffsetDateTimeHandler which retains Offset from UTC

Testing OffsetDateTimeHandler which retains Offset from UTC

d86dce0

harawata mentioned this pull request Oct 11, 2018

OffsetDateTimeTypeHandler or ZonedDateTimeTypeHandler loses time zone information #1081

Closed

harawata requested changes Dec 19, 2018

View reviewed changes

harawata added this to the 3.5.0 milestone Jan 7, 2019

harawata self-assigned this Jan 8, 2019

harawata added 3 commits January 8, 2019 23:40

Merge branch 'master' into gh/1368

76ecc01

Applied the requested change.

0f0eafa

Forgot to update some tests.

c7b1d95

harawata approved these changes Jan 9, 2019

View reviewed changes

harawata merged commit c8f5a48 into mybatis:master Jan 9, 2019

harawata added the bug label Jan 9, 2019

harawata mentioned this pull request Jan 10, 2019

OffsetTimeTypeHandler should use setObject(), getObject() #1437

Merged

harawata removed this from the 3.5.0 milestone Jan 20, 2019

harawata mentioned this pull request Nov 23, 2019

OffsetDateTimeTypeHandler had a breaking change from 3.4.6 -> 3.5.3 #1751

Closed

pulllock pushed a commit to pulllock/mybatis-3 that referenced this pull request Oct 19, 2023

Merge pull request mybatis#1368 from raupachz/ISSUE-1081

f559f9d

Testing OffsetDateTimeHandler which retains Offset from UTC

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Testing OffsetDateTimeHandler which retains Offset from UTC #1368

Testing OffsetDateTimeHandler which retains Offset from UTC #1368

raupachz commented Oct 10, 2018

harawata commented Oct 11, 2018 •

edited

Loading

harawata left a comment

harawata Oct 11, 2018

harawata Oct 11, 2018

harawata Oct 11, 2018

harawata Oct 11, 2018

harawata Oct 11, 2018

harawata Oct 11, 2018

harawata Dec 18, 2018

harawata Dec 18, 2018

harawata Dec 18, 2018

harawata commented Jan 7, 2019

harawata commented Jan 9, 2019

kazuki43zoo commented Jan 9, 2019

harawata commented Jan 9, 2019

kazuki43zoo commented Jan 9, 2019

harawata commented Jan 9, 2019

	return (OffsetDateTime) rs.getObject(columnName);
	return rs.getObject(columnName, OffsetDateTime.class);


		<mapper namespace="org.apache.ibatis.submitted.usesjava8.timestamp_with_timezone.Mapper">

		</mapper>

	return (OffsetDateTime) rs.getObject(columnIndex);
	return rs.getObject(columnIndex, OffsetDateTime.class);

	return (OffsetDateTime) cs.getObject(columnIndex);
	return cs.getObject(columnIndex, OffsetDateTime.class);

Testing OffsetDateTimeHandler which retains Offset from UTC #1368

Testing OffsetDateTimeHandler which retains Offset from UTC #1368

Conversation

raupachz commented Oct 10, 2018

harawata commented Oct 11, 2018 • edited Loading

harawata left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harawata commented Jan 7, 2019

harawata commented Jan 9, 2019

kazuki43zoo commented Jan 9, 2019

harawata commented Jan 9, 2019

kazuki43zoo commented Jan 9, 2019

harawata commented Jan 9, 2019

harawata commented Oct 11, 2018 •

edited

Loading