-
-
Notifications
You must be signed in to change notification settings - Fork 213
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
Support for java.time #159
base: main
Are you sure you want to change the base?
Conversation
Added new base sql types for local_x types
…on latest H2. No need to @OverRide methods in BaseDaoImplTest - compat. for newer H2 versions
The same will be done for ormlite-jdbc
I got an email with this reply, but it no longer seems to be here. I'll reply anyway. The idea for backwards compatibility was returning However, I just got around to actually testing it on Java 1.6 and found out, that I totally botched it, by initializing static singleton at declaration (duh). Lazily initializing it after checking if java.time class exists on classpath seems to work as intended, though that makes the singleton not final, and as such - a rather poor singleton. The constructor is still private and the field is not reassigned anywhere, so I hope it's okay. I will push a commit that fixes this a bit later, sorry. |
Unfolded imports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally seems okay to me.
A few minor points I'll make:
- Seems like some of copy-pasting between the classes could be reduced by using inheritance. Particularly the
java.sql
type versions which could just be wrappers of the superclass. - Is there a reason why
LocalTime
has nanosecond precision inparseDefaultString
but all the other types only have millisecond precision? - A few of the class-level Javadocs refer to the wrong class.
- I have my concerns about whether the JDBC 4.2 version or the
_SQL
type would be the default persister. Seems at first glance to be purely defined by what appears last inDataType.java
. Maybe the_SQL
persisters should omit the default classes list in the constructor, similar to howUUID_NATIVE
behaves? Or at very least leave a comment not to rearrange the order ofDataType
if you prefer to keep it as it currently is. Still seems dangerous if something calls the singleton before that file does so I think modifying the constructor is safer.
return OffsetDateTime.parse(defaultStr, DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss[.SSS]x")); | ||
} catch (NumberFormatException e) { | ||
throw SqlExceptionUtil.create("Problems with field " + fieldType + | ||
" parsing default LocalDateTime value: " + defaultStr, e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should say "Instant".
return LocalTime.parse(defaultStr, DateTimeFormatter.ofPattern("HH:mm:ss[.SSSSSS]")); | ||
} catch (NumberFormatException e) { | ||
throw SqlExceptionUtil.create("Problems with field " + fieldType + | ||
" parsing default LocalDateTime value: " + defaultStr, e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should say "LocalTime".
return OffsetDateTime.parse(defaultStr, DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss[.SSS]x")); | ||
} catch (NumberFormatException e) { | ||
throw SqlExceptionUtil.create("Problems with field " + fieldType + | ||
" parsing default LocalDateTime value: " + defaultStr, e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should say "OffsetDateTime".
return OffsetDateTime.parse(defaultStr, DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss[.SSS]x")); | ||
} catch (NumberFormatException e) { | ||
throw SqlExceptionUtil.create("Problems with field " + fieldType + | ||
" parsing default LocalDateTime value: " + defaultStr, e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should say "OffsetTime".
} | ||
return singleton; | ||
} | ||
private LocalDateTimeSqlType() { super(SqlType.DATE, new Class<?>[] { LocalDateTime.class }); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't matter by default, but maybe this should be LOCAL_DATE_TIME
to be consistent?
* {@link #OFFSET_TIME} so you will need to specify this using {@link DatabaseField#dataType()}. | ||
* | ||
*/ | ||
OFFSET_TIME_SQL(OffsetTimeSqlType.getSingleton()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the name for this is a little confusing since we are not storing this as a java.sql
type.
Also, I think the Javadocs for the *_SQL
types could be expanded to actually describe what's different about them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, this is true for all *_SQL
types, but I don't know how to name them differently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is true for all
*_SQL
types
Well the only one called that right now is SQL_DATE
and for good reason.
Types more like this situation is DATE_LONG
, DATE_INTEGER
, BOOLEAN_INTEGER
, etc. I'm not sure what the best name is. OFFSET_TIME_TIMESTAMP
? Does that sound clumsy?
I'd still modify the Javadoc for these enums to explain their purpose either way.
return OffsetTime.parse(defaultStr, DateTimeFormatter.ofPattern("HH:mm:ss[.SSS]x")); | ||
} catch (NumberFormatException e) { | ||
throw SqlExceptionUtil.create("Problems with field " + fieldType + | ||
" parsing default LocalDateTime value: " + defaultStr, e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should say "OffsetTime".
Fixed typos, added precision to tests
There's another point I'd like to discuss. What are the downsides of dropping
|
Well you could do things like
Interesting. It's documented to have nanosecond precision.
Did I miss these changes? The class Javadoc for InstantType still refers to LocalDate, for example.
Personally, I prefer feature checking over version checking but others may disagree with me on that. To answer your points specifically:
It has, but it didn't break your check.
I think removal is unlikely. It might change, but I think they'd be cautious about it.
Yep. Java feature & version checking in general is a bit ugly. I've also replied to your |
Quoting the docs for
As this answer on SO put it, the classes are capable of carrying the nanoseconds, but not capturing it. This was changed in Java 9 to microseconds (but still not nano though).
Oh, for the love of.. Yeah, I missed it, I changed just the text of the Exceptions, not the javadocs. I'll push it in the next commit, along with clearer explanations for
I understand that, but this is not really an exceptional situation, it's a regular check (one that will always throw an exception for each class on older JDKs at that), so IMO it should be an actual
True. But they could use something like 2018.3.1, for example (or add letters), I don't know.
I mean, |
If you wanted to keep It's just If you think adding |
Fixed javadocs, renamed OffsetTimeSql
In the end I made the call to do a single static check for Java specification version. My reasoning: our target is 1.6 and java.time became available in Java 8. Judging by the fact that java.util.Date is still around, java.time packages won't be deprecated for the foreseeable future. So, we need to check only that specification string is not equal to 1.6 or 1.7, no need to convert version to Double and so possible change in version numbering won't affect this. I think this is a better approach than I've expanded a bit on javadocs for the new classes (and hopefully fixed all the wrong references), and renamed Ideally this should also include new paragraphs to HTML tutorials, but this can be added later in a separate PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving the tests in a broken state by not updating H2 is something that can't be ignored.
I didn't have to adjust too much to get it to almost pass: Bo98@9b93989.
There are a couple of test failures even after the above. However, those are a part of bigger problems around fetching of generated IDs. A problem that is already affecting PostgreSQL, and now seemingly H2. I have a PR ready to fix it and will link it here shortly.
@Override | ||
public Object parseDefaultString(FieldType fieldType, String defaultStr) throws SQLException { | ||
try { | ||
return Timestamp.valueOf(LocalDateTime.parse(defaultStr, DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss[.SSS]"))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This caused problems in the tests for me. A better approach here (and ultimately more user friendly) would be variable length second fractions.
An example of this is here: Bo98@00fed82, along with adjustments to the tests to make sure they are consistently truncated (H2 supports milliseconds but the tests were comparing it to microseconds on my machine).
#165 for the additional H2 fixes. |
@Override | ||
public Object parseDefaultString(FieldType fieldType, String defaultStr) throws SQLException { | ||
try { | ||
return OffsetDateTime.parse(defaultStr, DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss[.SSS]x")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been building tests over on the JDBC side so sorry for not catching everything at once.
This here is going to cause problems as it is incompatible with regular OffsetTimeType
.
If we did @DatabaseField(defaultValue = "01:23:45+0100")
on OffsetTime
, it would work everywhere except Postgres or H2 which uses the compat persister instead.
A better solution here would be to make the date part optional, parse it into an OffsetTime
and then convert to OffsetDateTime
.
Example: Bo98@bd8ee75 (not tested everywhere but seems to work)
Alternatively, the date can be removed entirely from the parser but you'll then need to also implement resultStringToJava
to parse the date part there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe I've integrated all of your fixes now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll have a look come the end of the week when I'm back home.
Are there any news on this? |
Well, I'm using a build with these changes in my project with H2 and it works for me, the tests are passing as well. There were some problematic DBs that don't follow JDBC standard and we didn't seem to really settle on a final approach for cleanly dealing with them, so whether this commit is ready for general use is not for me to decide. |
Any chance of this to be merged? @j256 |
Yeah I need to work on this. Portability is the worry. I'll look at it this week. |
Hey, know this is a bit old now, but any updates on this? |
Right now I've not forced the library to go to Java 8 which is the holdup. Yes I know Java 7 is ancient but I'm always reluctant to upgrade my libraries. |
Well, Java 8 is about to be deprecated so I would say it's a good time to upgrade ;) |
Could we say get this merged into a different branch or something? Say, as a dev build, just so we can use it? Because I kinda really need this. Or you could just do what Exposed did and make this a separate module (e.g. ormlite-java-time) maybe? Java 8 has already reached the end of LTS by the way, so Java 7 is even further out of support range. |
I like the idea of a separate module, what do you think @j256? Possible? |
I'd rather not do a whole module for this although maybe that would be easier than the reflection hack alternatives. |
For the record, while waiting on this PR to get finalized, it's quite possible to add support for e.g. I could extract the code I have to a GitHub gist/public repo in case anyone's interested. Note: it only supports @Override
public String getSqlOtherType() {
return "TIMESTAMP WITHOUT TIME ZONE";
} |
Hopefully closes #154
Adds support for Java 8 Time API (JSR-310), specifically for
LocalDate
,LocalTime
,LocalDateTime
,OffsetTime
,OffsetDateTime
andInstant
.Default persisters use JDBC 4.2 API and pass values without conversion.
For DBs that don't fully support 4.2 (H2 and PostgreSQL don't support
TIME WITH TIME ZONE
, but supportTIMESTAMP WITH TIME ZONE
) there's a secondary persister forOffsetTime
that convertsOffsetTime
toOffsetDateTime
with date part fixed at epoch. There's an overridenFieldConverter
for those DBs in ormlite-jdbc, but these persisters can also be enabled by specifyingDataType
in annotation of the Dao class.Instant
is not a part of 4.2 API, so it converts to/fromOffsetDateTime
.For
LocalDate|Time
classes there are also secondary persisters, that convert java.time values to java.sql values using methods, added in Java 8. These are, again, forced in overriden 'FieldConverter's for those DBs in ormlite-jdbc and can also be enabled by specifyingDataType
. Someone with a better knowledge of DBs than me should take a look at those, since finding anything concrete in the documentation for all those different DBs is a pain (I presume DB2 and Netezza don't support 4.2, but I'm still not sure. Same for Sqlite).For every persister there's a check in
getSingleton()
method that returnsnull
, if there's no corresponding java.time class in classpath to allow compiling for older JDKs. No actual testing was done for that though.I am not entirely clear on purpose of
isArgumentHolderRequired()
andmoveToNextValue()
methods ofBaseData
, so I aped those off ofDateType
(aped the tests as well).New tests will fail for H2 1.2.128, since that version doesn't support 4.2 yet. For the latest H2 (1.4.197) they pass. However, old tests begin to fail, due to some changes in API, though this does not affect this PR, and I will create a separate issue for that.