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

Any plans to support Java 8 Time API? #154

Open
graynk opened this issue Dec 4, 2018 · 16 comments · May be fixed by #159
Open

Any plans to support Java 8 Time API? #154

graynk opened this issue Dec 4, 2018 · 16 comments · May be fixed by #159

Comments

@graynk
Copy link
Contributor

graynk commented Dec 4, 2018

Java 8 has come and already almost gone, java.time (JSR-310) is now part of JDK and the recommended datetime API that is meant to replace old and clunky java.util.time in new projects (since they can't really deprecate it even if they'd love to). Are the any plans to support it? I saw a few StackOverflow questions about it with the answers that recommended to write custom persisters, but haven't found any in the wild. I'll start writing my own and will make a PR if I feel that my implementation is good enough to be used by general public, but aren't there any implementations that have already been tested?

@noordawod
Copy link
Collaborator

@graynk sounds like a good idea!

@graynk
Copy link
Contributor Author

graynk commented Dec 4, 2018

@graynk sounds like a good idea!

However, doing it the usual way would mean upping the target for the project from 1.6 to 1.8, which I'm not sure is acceptable, since it's being used for Android development. And I'm not that familiar with reflection, nor do I see the reason to make custom persisters based on reflection for my use cases.

@noordawod
Copy link
Collaborator

noordawod commented Dec 4, 2018

That would require the approval of the maintainer, @j256.

@Bo98
Copy link
Collaborator

Bo98 commented Dec 4, 2018

I already had a basic implementation but it all maps to TIMESTAMP which kinda sucks. ORMLite at the moment only has SqlType.DATE which maps to TIMESTAMP (yeah, not the DATE SQL type), but really we should have ones that maps to DATE and TIME.

@Bo98
Copy link
Collaborator

Bo98 commented Dec 4, 2018

As for the Android situation, you'll probably be able to avoid reflection for the most part (JDK 8 itself is fine if set to compile to 1.7 bytecode) but you will still need some sort of runtime check. Perhaps something in getSingleton() of the datatype definitions, returning null otherwise.

I think javac doesn't throw compile errors if you use newer API than -source (as long as the JDK itself is updated) and only errors on newer language features. Someone should double-check that though.

@graynk
Copy link
Contributor Author

graynk commented Dec 5, 2018

ORMLite at the moment only has SqlType.DATE which maps to TIMESTAMP (yeah, not the DATE SQL type), but really we should have ones that maps to DATE and TIME.

Yeah, I bumped into that already, the naming is a bit misleading. Not even sure how to name this new DATE that maps to DATE as opposed to old DATE that maps to TIMESTAMP (SqlType.ACTUAL_DATE_THIS_TIME is a working title (: ). As I understand it, I can just add needed enumerations to SqlType, then add according cases to appendColumnArg method in BaseDatabaseType and at least H2 will work with that out of the box (I could even return actual LocalDate objects in javaToSqlArg method of my type, though I probably shouldn't, not sure about other DBs would handle it). Then, DatabaseResult needs to have corresponding new methods, and JdbcDatabaseResults in ormlite-jdbc needs to implement them (return resultSet.getDate(columnIndex + 1), basically). Is that correct or is something else needs to be added?

I think javac doesn't throw compile errors if you use newer API than -source (as long as the JDK itself is updated) and only errors on newer language features. Someone should double-check that though.

You're right, it compiled just fine with 1.6 source and target, thanks

@Bo98
Copy link
Collaborator

Bo98 commented Dec 5, 2018

I could even return actual LocalDate objects in javaToSqlArg method of my type, though I probably shouldn't, not sure about other DBs would handle it

Officially, it’s a standard of JDBC 4.2. Although you don’t need to restrict things 4.2, personally I’d argue it’s acceptable as that standard is officially a part of Java SE 8.

return resultSet.getDate(columnIndex + 1), basically

That works, but if you are using JDBC 4.2 the new, more direct API for it is resultSet.getObject(columnIndex + 1, LocalDate.class).

@Bo98
Copy link
Collaborator

Bo98 commented Dec 5, 2018

Something also to consider is whether we also need SqlTypes for TIME WITH TIME ZONE and TIMESTAMP WITH TIME ZONE. Mostly for OffsetDateTime.

screen shot 2018-12-05 at 09 27 40

Note that Instant is not officially referenced in JDBC 4.2. Perhaps that one would need to be converted (luckily there's methods added to java.sql.Timestamp just for this). Maybe worth testing with a few drivers.

@graynk
Copy link
Contributor Author

graynk commented Dec 5, 2018

That works, but if you are using JDBC 4.2 the new, more direct API for it is resultSet.getObject(columnIndex + 1, LocalDate.class).

Okay, thanks for that. At first I made it work via conversions from/to java.sql.Date using default methods, but now re-wrote it to work with LocalDates directly. Well, "made it work" means "I quickly checked that my existing database loads and new objects persist", now I have to actually write some tests.

Something also to consider is whether we also need SqlTypes for TIME WITH TIME ZONE and TIMESTAMP WITH TIME ZONE. Mostly for OffsetDateTime.

Well, it doesn't seem particularly challenging, just more of the same, so I'd say it's worth to support the whole java.time API, not just part of it. I may be wrong though.

Note that Instant is not officially referenced in JDBC 4.2. Perhaps that one would need to be converted (luckily there's methods added to java.sql.Timestamp just for this). Maybe worth testing with a few drivers.

Looking at H2 documentation again, they map both OffsetDateTime and Instant to TIMESTAMP_WITH_TIMEZONE, setting timezone to UTC (not sure why they don't map it to regular TIMESTAMP). If we take the same route, we could use OffsetDateTime.ofInstant(instant, ZoneOffset.UTC) and avoid using java.sql.Timestamp.

As for the Android situation, you'll probably be able to avoid reflection for the most part (JDK 8 itself is fine if set to compile to 1.7 bytecode) but you will still need some sort of runtime check. Perhaps something in getSingleton() of the datatype definitions, returning null otherwise.

Also about that -- considering there're backports like ThreeTen to both Java 6 and Android, it can't be just a simple JRE version check, I'd need to check if there's an appropriate class in classpath. Something like Class.forName("java.time.LocalDate", false, null)? But then again, backports have other package names.

@Bo98
Copy link
Collaborator

Bo98 commented Dec 5, 2018

Looking at H2 documentation again, they map both OffsetDateTime and Instant to TIMESTAMP_WITH_TIMEZONE, setting timezone to UTC (not sure why they don't map it to regular TIMESTAMP).

I fear that there will be differences between implementations on this, some using timezone and some not, particularly without guidance of a standard in JDBC.

Something like Class.forName("java.time.LocalDate", false, null)? But then again, backports have other package names.

Yep, I think that's fine. Other package names will not work out-of-the-box anyway without any check.

@graynk
Copy link
Contributor Author

graynk commented Dec 6, 2018

I fear that there will be differences between implementations on this, some using timezone and some not, particularly without guidance of a standard in JDBC.

Not sure I understand what this means. If JDBC driver complies with 4.2, then it supports mapping OffsetDateTime to TIMESTAMP_WITH_TIMEZONE and LocalDateTime to TIMESTAMP. Our implementation always converts Instant to - and back from - whichever one we choose, either via OffsetDateTime.ofInstant(instant, ZoneOffset.UTC) or via LocalDateTime.ofInstant(instant, ZoneOffset.UTC). We can't, and probably shouldn't guarantee that this is consistent with mappings of other ORMs (Hibernate maps it to TIMESTAMP, for example), this is just our custom persistor, like the existing one for Datetime from Joda-time.

What worries me, is the actual compliance with JDBC 4.2 on the driver side. PostgreSQL is 4.2 "compliant", but then the documentation says:

Note that ZonedDateTime, Instant and OffsetTime / TIME WITH TIME ZONE are not supported. Also note that all OffsetDateTime instances will have be in UTC (have offset 0)

But then there's this driver that follows 4.2 exactly. Derby says that it's 4.2 compliant, but all I could find in their docs point to java.sql types and this 4 year old unresolved JIRA issue. MySQL types "can always be converted" to java.sql types, but it also "follows JDBC specification where appropriate" (what? I'm guessing that's, again, because they don't have TIME/STAMP WITH TIME ZONE, but come on, what is this?). HSQLDB is fine. H2 does not support TIME WITH TIME ZONE (so, no OffsetTime, same as Postgre). Aaaand I couldn't find anything for Sqlite at all, it doesn't even mention 4.2 and there's no getObject(int columnIndex, Class<T> type) in source. I was surprised to find that things are in such a messy state even after all this time.

@Bo98
Copy link
Collaborator

Bo98 commented Dec 6, 2018

Not sure I understand what this means.

Sorry, I worded that extremely poorly and yeah, we can't guarantee much in regards to other JPA ORMs. That was already out of the window anyway with byte array.

PostgreSQL is 4.2 "compliant", but then the documentation says:

PostgreSQL is correct for ZonedTimeZone and Instant but not for OffsetTime. However I'll return to this in a moment.

No idea about the situation of Derby but it just seems no-one there is interested in actually making it 4.2 compliant. I have to say it's hard to follow progress at Apache sometimes. Some libraries they have just seem abandoned before jumping back to life after several years.

MySQL types "can always be converted" to java.sql types, but it also "follows JDBC specification where appropriate" (what? I'm guessing that's, again, because they don't have TIME/STAMP WITH TIME ZONE, but come on, what is this?).

MySQL's documentation is a load of hot trash. But I believe it does support java.time fully. I've not tested it though.

H2 does not support TIME WITH TIME ZONE (so, no OffsetTime, same as Postgre).

This is important to note. If a database does not support TIME WITH TIMEZONE, perhaps we should be throwing an exception. Otherwise we'd just doing the same as LocalTime which may surprise the developer (unless we do some other hacky method).

Any decision above may be relevant to PostgreSQL. On the topic of PostgreSQL, I believe you may find the reason for not supporting OffsetTime here:

The type time with time zone is defined by the SQL standard, but the definition exhibits properties which lead to questionable usefulness.

PostgreSQL endeavors to be compatible with the SQL standard definitions for typical usage. However, the SQL standard has an odd mix of date and time types and capabilities. Two obvious problems are:

  • Although the date type cannot have an associated time zone, the time type can. Time zones in the real world have little meaning unless associated with a date as well as a time, since the offset can vary through the year with daylight-saving time boundaries.
  • The default time zone is specified as a constant numeric offset from UTC. It is therefore impossible to adapt to daylight-saving time when doing date/time arithmetic across DST boundaries.

To address these difficulties, we recommend using date/time types that contain both date and time when using time zones. We do not recommend using the type time with time zone (though it is supported by PostgreSQL for legacy applications and for compliance with the SQL standard).

On the point of OffsetDateTime only supporting UTC in PostgreSQL's JDBC driver, this is a database-level shortcoming. TIMESTAMP WITH TIMEZONE converts the date/time to UTC but does not actually persist the original timezone.

Aaaand I couldn't find anything for Sqlite at all

This could perhaps be because SQLite doesn't actually support date or time types. You just store them as strings, integers or reals.


So in short, it seems to be actual database-level issues apart from Derby which is just pure non-compliance. Perhaps that is one too many though.

@graynk
Copy link
Contributor Author

graynk commented Dec 6, 2018

This is important to note. If a database does not support TIME WITH TIMEZONE, perhaps we should be throwing an exception. Otherwise we'd just doing the same as LocalTime which may surprise the developer (unless we do some other hacky method).

Can't we use TIMESTAMP WITH TIMEZONE for those DBs? Fix the date, set the time and the zone, then throw away the date when converting back. We need to address fully non-compliant DBs anyway (I have an unpleasant idea for that below), this is just one more hack to the bunch.

This could perhaps be because SQLite doesn't actually support date or time types. You just store them as strings, integers or reals.

Well, their current driver is compliant to some older specification of JDBC, it's able to return java.sql.Date and java.sql.Timestamp, so why not java.time? We shouldn't really care what they use internally and how the driver transforms Java objects to it's internals, as long as it exposes an interface, compliant with the specification. In the ideal world, that is. I presume the situation there is the same as with Derby, no one really wants to implement 4.2. So, my hacky suggestion is thus: we make two persistors for the troublesome (Offset/Instant) classes. One is the pure java.time, the other is a converter from java.time to java.sql types. Then we override getFieldConverter() in non-compliant _DatabaseTypes in ormlite-jdbc and redirect them to java.sql converters, like it's done for booleans in OracleDatabaseType. Right now I don't have any other ideas, as we can't check which DB we transform for inside the persistor itself (I think).

@Bo98
Copy link
Collaborator

Bo98 commented Dec 6, 2018

Can't we use TIMESTAMP WITH TIMEZONE for those DBs? Fix the date, set the time and the zone, then throw away the date when converting back.

True, you'll likely need to create a field converter for it.

So, my hacky suggestion is thus: we make two persistors for the troublesome (Offset/Instant) classes. One is the pure java.time, the other is a converter from java.time to java.sql types. Then we override getFieldConverter() in non-compliant _DatabaseTypes in ormlite-jdbc and redirect them to java.sql converters, like it's done for booleans in OracleDatabaseType.

Ultimately that's probably the only solution. You'll probably be unable to save the timezone like PostgreSQL & H2 but it seems like the support for timezones is pretty poor anyway across different DBs.

@graynk
Copy link
Contributor Author

graynk commented Dec 7, 2018

I would also like to understand the reasoning behind parseDefaultString methods in existing DateType and TimeStampType. Right now, the tests fail for me on latest H2 1.4.197 (but pass on 1.2.128. which was used before, I presume).

testDate(com.j256.ormlite.field.types.DateTypeTest): Problems parsing default date string '2018-12-07 09:57:30' using 'yyyy-MM-dd HH:mm:ss.SSSSSS'
testTimeStamp(com.j256.ormlite.field.types.TimeStampTypeTest): Problems parsing default date string '2018-12-07 09:57:30' using 'yyyy-MM-dd HH:mm:ss.SSSSSS'

They all fail on this part, because getString now returns date strings without padded zeroes in the fraction-of-second part:

// test string conversion
String stringVal = results.getString(colNum);
Object convertedJavaVal = fieldType.convertStringToJavaField(stringVal, 0);

What I don't understand is why are we testing dates for this and why parseDefaultString doesn't just return null?

There are a bunch of new failed tests as well (for ByteArrays, BigIntegers and others). There're also changes to org.h2.api.Trigger which cause one test to not compile (adding new no-op methods works, obviously). I don't know if I should create a separate issue for this.

I'd also like to understand what moveToNextValue and isArgumentHolderRequired methods are for. I read the docs and aped the methods and tests for my classes from existing ones, but I'm not entirely clear on their purpose.

@graynk
Copy link
Contributor Author

graynk commented Dec 7, 2018

I pushed base changes to my fork, but I haven't yet fixed issues described above, and haven't even begun adding support to java.sql conversions

https://github.com/graynk/ormlite-core/tree/jsr310-persister-pr
https://github.com/graynk/ormlite-jdbc/tree/jsr310-persister-pr

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

Successfully merging a pull request may close this issue.

3 participants