Open graynk opened 6 years ago
@graynk sounds like a good idea!
@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.
That would require the approval of the maintainer, @j256.
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
.
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.
ORMLite at the moment only has
SqlType.DATE
which maps toTIMESTAMP
(yeah, not theDATE
SQL type), but really we should have ones that maps toDATE
andTIME
.
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
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)
.
Something also to consider is whether we also need SqlType
s for TIME WITH TIME ZONE
and TIMESTAMP WITH TIME ZONE
. Mostly for OffsetDateTime
.
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.
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 LocalDate
s 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
SqlType
s forTIME WITH TIME ZONE
andTIMESTAMP WITH TIME ZONE
. Mostly forOffsetDateTime
.
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 tojava.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, returningnull
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.
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.
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.
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, noOffsetTime
, 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, thetime
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.
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 asLocalTime
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 _DatabaseType
s 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).
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.
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 ByteArray
s, BigInteger
s 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.
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
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?