-
-
Notifications
You must be signed in to change notification settings - Fork 100
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 #37
Open
graynk
wants to merge
10
commits into
j256:main
Choose a base branch
from
graynk:jsr310-persister-pr
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
678b0c1
changes to support java.time and and override to account for H2 not s…
graynk 559a93e
Added forced conversions to sql types for DBs that are non-compliant …
graynk e86d9c8
Override of new missing methods
graynk ed1adcb
Delete JdbcDatabaseConnection.java
graynk 3eb089b
Whoops.
graynk 11d1d9f
whoops
graynk 5569c60
Typo
graynk 2f47ff5
Enum change
graynk 4805f2e
Merge branch 'master' into jsr310-persister-pr
graynk ab77ff7
Draft for FieldConverter wrapper for Offset types
graynk File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,10 @@ | |
import java.util.List; | ||
|
||
import com.j256.ormlite.field.FieldType; | ||
import com.j256.ormlite.field.SqlType; | ||
import com.j256.ormlite.field.DataType; | ||
import com.j256.ormlite.field.DataPersister; | ||
import com.j256.ormlite.field.FieldConverter; | ||
|
||
/** | ||
* H2 database type information used to create the tables, etc.. | ||
|
@@ -52,6 +56,20 @@ public void appendLimitValue(StringBuilder sb, long limit, Long offset) { | |
sb.append(limit).append(' '); | ||
} | ||
|
||
@Override | ||
public void appendOffsetTimeType(StringBuilder sb, FieldType fieldType, int fieldWidth) { | ||
sb.append("TIMESTAMP WITH TIME ZONE"); | ||
} | ||
|
||
@Override | ||
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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Following the change in -core, this should now be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
// default is to use the dataPersister itself | ||
return dataPersister; | ||
} | ||
|
||
@Override | ||
public boolean isOffsetLimitArgument() { | ||
return true; | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 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 aFieldConverter
toLocalTime
/LocalDateTime
. We want to takeInstant
into account too (which doesn't have its own SQL type because it rightly doesn't need one) so perhaps aFieldConverter
wrapper around the data persister would work better than just creating a newDataPersister
. 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.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.
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.
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.
Sure, no problem. I’ll test everything again when you manage to take a look.