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

add support to save/fetch java.time data types #756

Merged
merged 19 commits into from
Mar 8, 2017

Conversation

darkv
Copy link
Member

@darkv darkv commented Jun 7, 2016

This patch aims to add support of the new Java 8 Date & Time API and is a work-in-progress to be discussed. The current Joda data types are kept for backward compatibility.

  • EOF support
  • component support
  • ERRest support

@darkv
Copy link
Member Author

darkv commented Jun 7, 2016

Looking at different databases and the current Joda prototypes I see that timezones are unfortunately sometimes a problem. The current JDBC specification states that java.time.OffsetDateTime should be mapped to TIMESTAMP WITH TIMEZONE but alas there are quite many database vendors out there who only support that data type partially or not at all.

Currently there would/should be the following data types:

  • local time
  • local date
  • local date time
  • date time with specific timezone

The last one is currently supported in:

  • PostgreSQL
  • Oracle
  • FrontBase
  • H2
  • MS SQL Server
  • DB2

Timezones are currently not supported as data type in:

  • Derby
  • MySQL
  • OpenBase (unclear)
  • MS Access
  • dBase (unclear)
  • Firebird
  • Interbase
  • Neo4J (unclear)

So there are a couple of ideas:

  1. drop that prototype altogether
  2. use it with timezone for databases that support it and drop it for all others
  3. convert everything to UTC before saving so no timezone at all on database side but then loosing information about source timezone

I would tend to 1. as to prevent confusion. If you really need timestamps with timezones you can either create your own prototype if your database supports it or otherwise use two attributes and put some logic into your EO to save a UTC timestamp and the timezone ID or offset separately so you can reconstruct the correct value after fetch.

Please post your thoughts :)

@paulhoadley
Copy link
Contributor

Firstly, thanks for doing this work.

Please post your thoughts

Here's a thought: times, dates, timestamps and timezones are horrible.

drop that prototype altogether

Could you be more specific about exactly which prototype you're referring to? You're not talking about dropping any legacy prototypes, right? You mean a prototype whose Java-side representation is a Java 8 "date time with specific timezone"?

@darkv
Copy link
Member Author

darkv commented Jun 7, 2016

Could you be more specific about exactly which prototype you're referring to? You're not talking about dropping any legacy prototypes, right? You mean a prototype whose Java-side representation is a Java 8 "date time with specific timezone"?

No dropping of legacy prototypes, just not create an equivalent to the current jodaDateTime.

@paulhoadley
Copy link
Contributor

No dropping of legacy prototypes, just not create an equivalent to the current jodaDateTime.

Great, thanks.

(Although in the past I've been a lot more blasé about backwards incompatibility and the need to move things forward where we can, dates and times is an area that makes me a little uncomfortable. I know from bitter past experience how hard it can be to get things right, and I also know what we're doing here right now Just Works because of that pain, so I'd be keen to keep the legacy prototypes around.)

@spelletier
Copy link
Member

I think the proposed prototypes are OK, they match the JodaDate. I'm not sure about the code for the localdatetime and dateTime. The easiest way is to insert some values in a time zone and restart the app in another timezone to check how they react.

Users of database without timezone support are either not using them or already save the information in another attribute.

@darkv
Copy link
Member Author

darkv commented Jun 10, 2016

I did some testing with different settings and timezones. I would tend for this behavior:

  • LocalDateTime is saved to database as is into a TIMESTAMP [WITHOUT TIME ZONE] field. When fetched from database you get the exact same timestamp regardless what time zone the fetching app is in, i.e. if you had an app in time zone America/Panama with a LocalDateTime 2016-03-10T12:13:45 you will get the exact same value in an app in the time zone Europe/Berlin.
  • OffsetDateTime is converted to UTC first and then saved to database into a TIMESTAMP [WITHOUT TIME ZONE] field. Saving the value 2016-03-10T06:13:45-05:00 in an app in the time zone America/Panama will put 2016-03-10T11:13:45 into the database. Fetching from an app in the time zone Europe/Berlin will get that UTC value and convert it into your local time zone resulting in 2016-03-10T12:13:45+01:00. So this will denote an explicit instant in time though the originating time zone will be lost. If you need that piece of information too then you would need to separately save the zone ID yourself.

Using that you don't need time zone support on the database side and thus can use any vendor.

@lbane
Copy link
Contributor

lbane commented Jun 10, 2016

Can you name this prototype "javaDateTimeToUTC" to make it clear, with what we are dealing here?

How does this approach work with existing tables in databases with full timestamp support, when the new date time classes get used in the EOModel?

@spelletier
Copy link
Member

spelletier commented Jun 10, 2016

To comple te the tests, the server need to be started in a different time zone than the client. I spend a lot of time trying to figure it out before fixing the FrontBase prototypes. For based on the SQL standard, you should uses Timestamp with Time zone if the server and client lib are TZ aware. At least in FB some case does not work as expected with regular timestamp because the client lib adjust the result with the offset difference between the server and client timezone. The WITH TIME ZONE indicate the client software understand timezone and will handle them.

PostgreSQL changed the default timestamp behaviour in the past, from their doc:

The SQL standard requires that writing just timestamp be equivalent to timestamp without time zone, and PostgreSQL honors that behavior. (Releases prior to 7.3 treated it as timestamp with time zone.)

They are many possible TZ conversions in the process that may depend from the database server, client and frameworks used.

@darkv
Copy link
Member Author

darkv commented Jun 10, 2016

To comple te the tests, the server need to be started in a different time zone than the client.

I suppose with "server" you mean the db server? I don't think that that time zone matters as we are sending UTC timestamps to it and save in a data type that is time zone agnostic. All the time zone mungling happens at the client. I think that is the very same behavior as it is for the JodaTime prototypes?

I spend a lot of time trying to figure it out before fixing the FrontBase prototypes.

I will try to test with FrontBase in the days coming.

For based on the SQL standard, you should uses Timestamp with Time zone if the server and client lib are TZ aware.

To support all databases we cannot assume that the db server is TZ aware. That would be the fifth prototype where we would preserve the original time zone. But this must be left as an exercise to the programmer if his/her db is supporting that data type ;)

@darkv
Copy link
Member Author

darkv commented Jun 10, 2016

Can you name this prototype "javaDateTimeToUTC" to make it clear, with what we are dealing here?

I am not sure if that would be a good idea because UTC is only an implementation detail of the database. Within your WO-App you get a timestamp in your local time zone not UTC.
I think the best would be to add some documentation about all prototypes with some details and suggestions.

How does this approach work with existing tables in databases with full timestamp support, when the new date time classes get used in the EOModel?

You mean if you currently have the JodaTime-prototypes and want to switch? That would need some testing but as long as the external data types are the same it probably should be no problem at all to switch to the new ones. In worst case you would throw in an SQL-update to adapt the values if you really need to change to java.time.

darkv added 12 commits July 14, 2016 17:00
add optional pattern variable for consumers that need the pattern of the formatter which you cannot reconstruct from a DateTimeFormatter unfortunately
remove unnecessary suppress warnings, use diamond operator, add missing override annotations
many tests still need fixing as EO conversion seems to include primary key which did not when the tests were originally written
@darkv
Copy link
Member Author

darkv commented Aug 10, 2016

The test project ERRestTest is now runnable as JUnit target. In Eclipse you can right-click on the class file ERRestTest and choose Debug As > JUnit Test. Currently there are 11 of 40 test methods that are failing. Most of them due to ERRest automatically adding the primary key for EOs which apparently was not the case back when the test were written and by this not really critical. Though it would be good to fix those tests. If someone feels elected… ;-)

@paulhoadley paulhoadley self-requested a review March 8, 2017 01:04
Copy link
Contributor

@paulhoadley paulhoadley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested these new prototypes using PostgreSQL and they look good to me. Unless anyone else wants to test using their favourite database, I suggest this gets merged.

@darkv darkv merged commit e82c8e8 into wocommunity:master Mar 8, 2017
@darkv darkv deleted the datetime_patch branch March 8, 2017 08:23
@paulhoadley
Copy link
Contributor

Thanks for your work here Johann. Really nice feature.

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

Successfully merging this pull request may close these issues.

4 participants