Support for java.time #37

graynk · 2018-12-11T10:39:15Z

Accompanies j256/ormlite-core#159 in closing j256/ormlite-core#154

…upporting time with timezone

…to JDBC 4.2 Not sure about Db2 and Netezza.

noordawod · 2018-12-11T10:41:31Z

LGTM, and backward-compatible I would say (untested).

j256 · 2018-12-11T15:32:13Z

Thanks for this. How is backwards compatibility achieved? Does this force ORMLite to Java 8?

graynk · 2018-12-11T17:46:03Z

Thanks for this. How is backwards compatibility achieved? Does this force ORMLite to Java 8?

Java 8 is required on compile, but it can still be compiled to source and target of 1.6. New methods import java.time, but they don't get called (since java.time isn't on the classpath and there are no classes to persist with those methods). The issues (still solvable) arise in the ormlite-core package, I wrote more there:
j256/ormlite-core#159 (comment)

There are also backports of java.time like ThreeTen and ThreeTenABP, for which users in theory would be able to write custom persisters based on new DataTypes and SqlTypes without needing to add anything to ormlite-jdbc.

Also in this PR I forgot to commit 2 new overriden methods, getTime and getDate, sorry, I'll add them a bit later.

j256 · 2018-12-11T18:19:02Z

Thanks. Good stuff.

If you don't mind, please minimize incidental line changes. I see changes to imports to use .* but my eclipse settings will expand that again so please fix.

Unfolded imports

graynk · 2018-12-12T09:22:11Z

If you don't mind, please minimize incidental line changes. I see changes to imports to use .* but my eclipse settings will expand that again so please fix.

Sure, not a problem. Unfolded imports back, made changes to new classes to avoid calling non-existent classes due to backwards compatibility, verified that compiled artifacts work on Java 1.6

Bo98 · 2018-12-12T15:19:54Z

src/main/java/com/j256/ormlite/db/PostgresDatabaseType.java

+
+	@Override
+	public FieldConverter getFieldConverter(DataPersister dataPersister, FieldType fieldType) {
+		// H2 doesn't support TIME WITH TIME ZONE


Minor: this should refer to Postgres - not H2.

Fixed, thanks

Bo98 · 2018-12-14T13:17:19Z

By the way which databases did you end up being able to test? I can try out a few later today if needed.

I set up a few yesterday for testing. Struggled to get the Db2 JDBC driver to connect, Netezza seems unobtainable now (support ends formally in a few months so perhaps support for it on ORMLite's side should be deprecated), and ODBC bridge is dead (#38) but I've got everything else up to Postgres so far. Will do SQLite and SQL Server today.

graynk · 2018-12-14T14:18:52Z

By the way which databases did you end up being able to test? I can try out a few later today if needed.

Only H2, sorry, I would be very grateful if you checked what you can, I just don't have the time. I used only documentation for each DB to understand what it does or does not support.

Bo98 · 2018-12-14T14:56:20Z

Ok, will do. There's already a few issues with different DBs right now anyway without these changes which I'll be looking into too.

Oracle's JDBC driver is a disaster. Yes, the organisation behind Java made a bad JDBC driver. Existing Date/Time classes types don't even work 100% right now. Character.class is unsupported too (#2).

Bo98 · 2018-12-17T20:05:10Z

src/main/java/com/j256/ormlite/db/H2DatabaseType.java

+	public FieldConverter getFieldConverter(DataPersister dataPersister, FieldType fieldType) {
+		// H2 doesn't support TIME WITH TIME ZONE
+		if (dataPersister.getSqlType() == SqlType.OFFSET_TIME)
+			return DataType.OFFSET_TIME_SQL.getDataPersister();


Following the change in -core, this should now be OFFSET_TIME_COMPAT. Similarly in Postgres too.

Bo98 · 2018-12-17T21:31:18Z

Basic tests for these should be added to JdbcBaseDaoImplTest.

Bo98

I've started testing things but a couple things popped up in the process so I haven't gotten too far yet.

For the H2 update, all I had to do was change this to use getAbsolutePath(), merge #39, and remove testQueryKeyHolderNoKeys, testIdColumnInteger and testIdColumnChangedFromStringToNumber (and I renamed testIdColumnInvalid to testGeneratedIdColumn).

These tests were testing unspecified JDBC behaviour (or at least not clearly specified enough for JDBC drivers to agree on it). PreparedStatement.getGeneratedKeys() is only guaranteed to work when you don't explicitly use your own value for the generated ID column. It is only guaranteed to return data that was actually generated. Hence these tests, which broke after the update (and was broke already in some other drivers), were not correct to begin with. ormlite-core uses the correct behaviour and only passes GeneratedKeyHolder when a generated ID isn't set in the insert operation, so these tests were not testing normal functionality of ORMLite.

I can share a commit of the H2 changes tomorrow if desired. I do have some tests in JdbcBaseDaoImplTest now too in order to help me test the different drivers but they're not final yet.

Bo98 · 2018-12-19T02:21:12Z

src/main/java/com/j256/ormlite/db/Db2DatabaseType.java

+				return DataType.LOCAL_DATE_TIME_SQL.getDataPersister();
+			case OFFSET_TIME: // db2 doesn't seem to support TIME/STAMP WITH TIME ZONE
+			case OFFSET_DATE_TIME:
+				return null;


This pattern is going to cause problems. Returning null leads to hard-to-debug NullPointerExceptions within ORMLite.

We have two options here I can think of:

Throw a friendly exception. It'll be a bit messy to take this into account in the tests though.

Make a best effort at converting. We should be able to easily emulate the behaviour of PostgreSQL and H2. They, at database level, just convert the timestamp to UTC and store it as such. On converting back it uses the system timezone. The timezone is never actually stored, so the WITH TIMEZONE didn't actually mean much in the first place. We could do the same here using a FieldConverter to LocalTime/LocalDateTime. We want to take Instant into account too (which doesn't have its own SQL type because it rightly doesn't need one) so perhaps a FieldConverter wrapper around the data persister would work better than just creating a new DataPersister. It gives more flexibility for custom persisters too (say if you wanted to create persisters for ThreeTen for example). See Fixed exceptions in some JDBC drivers when working with chars #40 for an approach I took in another situation. I think this approach will work best. We won't be doing any worse than PostgreSQL and H2's efforts.

Sorry, haven't had a lot of free time on my hands. I'll be taking a vacation somewhere in the end of January/beginning of February and will do my best to catch up on both PRs then.

Sure, no problem. I’ll test everything again when you manage to take a look.

graynk added 2 commits December 7, 2018 12:42

changes to support java.time and and override to account for H2 not s…

678b0c1

…upporting time with timezone

Added forced conversions to sql types for DBs that are non-compliant …

559a93e

…to JDBC 4.2 Not sure about Db2 and Netezza.

graynk added 4 commits December 12, 2018 14:11

Override of new missing methods

e86d9c8

Unfolded imports

Delete JdbcDatabaseConnection.java

ed1adcb

Whoops.

3eb089b

whoops

11d1d9f

Bo98 reviewed Dec 12, 2018

View reviewed changes

Typo

5569c60

Bo98 reviewed Dec 17, 2018

View reviewed changes

Enum change

2f47ff5

Bo98 reviewed Dec 19, 2018

View reviewed changes

graynk and others added 2 commits February 9, 2019 19:37

Merge branch 'master' into jsr310-persister-pr

4805f2e

Draft for FieldConverter wrapper for Offset types

ab77ff7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for java.time #37

Support for java.time #37

graynk commented Dec 11, 2018

noordawod commented Dec 11, 2018

j256 commented Dec 11, 2018

graynk commented Dec 11, 2018 •

edited

Loading

j256 commented Dec 11, 2018

graynk commented Dec 12, 2018

Bo98 Dec 12, 2018

graynk Dec 13, 2018

Bo98 commented Dec 14, 2018

graynk commented Dec 14, 2018

Bo98 commented Dec 14, 2018

Bo98 Dec 17, 2018 •

edited

Loading

graynk Dec 18, 2018

Bo98 commented Dec 17, 2018

Bo98 left a comment •

edited

Loading

Bo98 Dec 19, 2018 •

edited

Loading

graynk Jan 16, 2019

Bo98 Jan 16, 2019

Support for java.time #37

Are you sure you want to change the base?

Support for java.time #37

Conversation

graynk commented Dec 11, 2018

noordawod commented Dec 11, 2018

j256 commented Dec 11, 2018

graynk commented Dec 11, 2018 • edited Loading

j256 commented Dec 11, 2018

graynk commented Dec 12, 2018

Bo98 Dec 12, 2018

Choose a reason for hiding this comment

graynk Dec 13, 2018

Choose a reason for hiding this comment

Bo98 commented Dec 14, 2018

graynk commented Dec 14, 2018

Bo98 commented Dec 14, 2018

Bo98 Dec 17, 2018 • edited Loading

Choose a reason for hiding this comment

graynk Dec 18, 2018

Choose a reason for hiding this comment

Bo98 commented Dec 17, 2018

Bo98 left a comment • edited Loading

Choose a reason for hiding this comment

Bo98 Dec 19, 2018 • edited Loading

Choose a reason for hiding this comment

graynk Jan 16, 2019

Choose a reason for hiding this comment

Bo98 Jan 16, 2019

Choose a reason for hiding this comment

graynk commented Dec 11, 2018 •

edited

Loading

Bo98 Dec 17, 2018 •

edited

Loading

Bo98 left a comment •

edited

Loading

Bo98 Dec 19, 2018 •

edited

Loading