-
Notifications
You must be signed in to change notification settings - Fork 880
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
Rewrite JSR 310 types to use get/setObject(). Improves accuracy. #3402
Conversation
querydsl-sql/src/main/java/com/querydsl/sql/types/JSR310InstantType.java
Outdated
Show resolved
Hide resolved
Duplicate of #3382, although this PR is more complete and the other PR has a few mistakes. Look into the failing Java 11 build though. |
After some investigation, there appear to be multiple problems that cause the tests to fail. One problem is that the existing tests for the offset and zoned types are broken. They only "succeed" right now because of the bugs I noted when I opened this PR. The tests that would have caught these bugs are right underneath the broken tests, but are marked Another problem is that some of the JDBC drivers currently used for testing are not 4.2 compliant:
|
I've opened #3403 to upgrade the JDBC driver for SQL Server. That upgrade revealed a bug in UtilDateType which is also fixed in that PR. I also have branches prepared locally to upgrade the JDBC drivers for Firebird and Oracle to latest, but those hit the same issue with UtilDateType as the SQL Server driver does. So #3403 needs to be merged first. Then I can open PRs for Firebird and Oracle, and those will also need to be merged before the tests for this PR will have any chance of passing. |
ebd14df
to
6e7c0ac
Compare
I think all the tests should pass now. Regarding H2, it apparently supports most JSR 310 types in 1.4.197, but support for TIME WITH TIME ZONE was not added until 1.4.200. Because of the issue with h2gis mentioned above, H2 cannot be upgraded right now, so I was forced to disable the test for OffsetTime on H2. Regarding Derby and DB2, their JDBC drivers appear to not be compliant with JDBC 4.2, so I have disabled the entire test for them. IMO they've had more than enough time to get into compliance, and it's not QueryDSL's job to cover up their deficiencies. Especially since it is not possible to implement the zoned/offset types correctly without support in the driver. |
It turns out Postgres has only fake support for time zones: it always converts values to UTC and stores them that way, so there is no way to determine the original zone. |
@jwgmeligmeyling Test runs are now clean, so this is ready for you to look at when you get the chance. |
This change rewrites the support for JSR 310 types to use getObject() and setObject() instead of trying to munge Timestamp into the appropriate object.
QueryDSL now requires Java 8, which includes JDBC 4.2, which added mappings for most JSR 310 types. IMO it is fine to stop supporting older JDBC drivers that do not implement the 4.2 spec.
Note that this change also fixes some bugs and drawbacks of the current approach. It lets the JDBC driver create the result object, which will probably be more efficient than going through Timestamp. Also, time zones/offsets will be what they are in the database instead of UTC. The existing code does not handle zones/offsets correctly because the 2-arg version of getObject() ignores the provided Calendar if the database has time zone info available, which means that QueryDSL returns the incorrect value in that case since it assumes the value returned by the driver is in UTC time zone.
Tests were removed because they did not provide much value before the change, and provide none afterwards.