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

Part 2: Tests for other date time types #2709

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

nPraml
Copy link
Contributor

@nPraml nPraml commented May 31, 2022

Hello @rbygrave ,

in this second PR I created some tests with other date/time/datetime classes.
I can also refactor the tests in one model class and in one test class if you want. That's why I made this 2nd PR from the 1st.

Tests with LocalDateTime and Timestamp are failing. In this area I have a need for discussion.
Our assumptions: LocalDateTime is never converted (it is local) but Instant and Timestamp should be converted from the database timezone to the application/java time zone. Do you agree or do you have other aspects as well?

The fix would be from the old PR in RsetDataReader.getTimestamp(boolean isLocal) and the modification of the ScalarTypeXYZ constructors.

Kind regards
Noemi

@nPraml
Copy link
Contributor Author

nPraml commented May 31, 2022

Note: on Github the tests with LocalDateTime and Timestamp are not failing, but on my local machine. I think it is the case because I have local TimeZone "Europe/Berlin" and github some other one.

@nPraml
Copy link
Contributor Author

nPraml commented Jun 21, 2022

Hello @rbygrave,

can you please give a statement whether I should implement the approach in the old PR with the ´isLocal´?

Cheers
Noemi

@rbygrave
Copy link
Member

I should implement the approach in the old PR with the ´isLocal´?

That PR was not reviewed in that it was too big and not sufficiently focused. We need to start by first looking at the failing test(s) and understanding them and understanding their impact noting that these can be JDBC driver specific. In this sense it's better to start with 1 failing test (like timestamp), forget everything else. Reproduce with H2, run with various other databases and jdbc drivers. Assess the state and impact.

@rPraml rPraml force-pushed the tests-for-other-date-types branch 2 times, most recently from 52ca271 to 2876639 Compare January 23, 2023 16:29
@rPraml
Copy link
Contributor

rPraml commented Aug 10, 2023

Statusupdate: This PR is quite old, and I want to summarize what is open:

If you store an Instant, LocalDate, or LocalTime or Timestampin the database, and read the result on a different machine, you will get the SAME value, if the TZ of the ebean-server matches, but the TZ of the JVM has changed.

This behaviour is not true for LocalDateTime - LocalDateTimes may change their value, if the JVM-TZ is changed¹

¹) A TZ change of the JVM might occur, if you fire up one docker in USA and one in Europe.

Expected

    dbGmt.sqlUpdate("insert into mtime_test (id, local_date_time) values (1, '2021-11-21 05:15:15')").execute();
    LocalDateTime ldt = LocalDateTime.parse("2021-11-21T05:15:15");
    assertThat(dbEurope.find(MTimeTest.class).findOne().getLocalDateTime()).isEqualTo(ldt);
    assertThat(dbPST.find(MTimeTest.class).findOne().getLocalDateTime()).isEqualTo(ldt);

A LocalDatetTime should have always the same value, not depending on the TZ-setting of the ebean server.

Actual (if machine runs in Europe/Berlin)

    assertThat(dbGmt.find(MTimeTest.class).findOne().getLocalDateTime()).isEqualTo(ldt.plusSeconds(3600));
    assertThat(dbEurope.find(MTimeTest.class).findOne().getLocalDateTime()).isEqualTo(ldt);
    assertThat(dbPST.find(MTimeTest.class).findOne().getLocalDateTime()).isEqualTo(ldt.plusSeconds(9 * 3600));

So it depends on the TZ-setting of the ebean server, as it will convert the LDT-value

I know, there may be different point of views, but I think, this behaviour might be incorrect or at least inconsistent, because

  • A LocalDate or LocalTime column is not affected by timeshift. A LocalDateTime is just a combination of LocalDate + LocalTime, but is affected.
  • if you serialize a LocalDateTime on one machine + deserialize it on an other machine, running on a different timezone (either by writeExternal/readExternal or by toString()/parse), you will get a LDT with same value, while "serializing/deserializing" it in the database will change the object's value, if TZ of machine differs

@rbygrave We always use Instants instead of LocalDateTimes in our data model, so we currently do not have a problem with that issue. It would be OK, if this PR is closed and the behaviour is documented.

@rbygrave
Copy link
Member

If you think there is a problem with LocalDateTime then I'd like to see a PR that is focused on showing that problem.

Noting that the issue you describe with LocalDateTime could be dependent on the DB and Driver so might need to be very specific to demonstrate the issue.

@rPraml
Copy link
Contributor

rPraml commented Aug 10, 2023

I know that sometimes it is incredibly difficult for me to explain a problem to someone else and I would have hoped that I could convey the problem better with the help of the test class.

So I'll try it by questioning the expectations:

Imagine, you have a DB hosted at AWS and one server in New York and an other in Berlin. (Total 3 servers, 1 DB, 2 app servers running ebean)
Both app server run on different time-zone (=OS default) - the TZ of the DB should not matter here
To get not crazy with time shifts at all, we store time values as GMT in the DB and configure ebean to use "GMT" as "dataTimeZone"
Till here, we have a standard use case, right?

If you read/write an Instant from/to the DB. You expect that you get the same Instant (=same value) in NY and Berlin, right?
(This is, what I'll try to prove in testInstant#61)
If I have different dataTimeZone settings, the Instant gets converted accordingly, shown by the following lines. Works as expected, right?

If you read/write a LocalDate or LocalTime - these values should never change, neither depending on the JVM TZ or the dataTimeZone. I think it is clear, that christmas is as 2024-12-24 everywhere and work hours are from 08:00-17:00 (it depends from country to country, but nor really on the TZ)
There WAS a bug, that has been fixed with #2708
The tests testLocalDate and testLocalTime verify that there occurs no time shift during JVM TZ (my machine runs in Europe/Berlin) and not during dataTimeZone.

But now comes the LocalDateTime - while LocalDate and LocalTime are not affected by any timezone, LocalDateTime is.
And here I ask: Why?
I would expect, that a LocalDateTime behaves the same as LocalDate or LocalTime - so no time conversion should occur regardless which time zones are set.

Or if I take this very simple example test code

    LocalDateTime ldt = LocalDateTime.of(2023, 3, 26, 2, 0, 0); // DST jump in Europe/Berlin
    MTimeTest model = new MTimeTest();
    model.setLocalDateTime(ldt);
    server().save(model);
    MTimeTest model2 = server().find(MTimeTest.class, model.getId());
    assertThat(model2.getLocalDateTime()).isEqualTo(model.getLocalDateTime());

This fails, if JVM TZ = Europe/Berlin - ebean cannot store LocalDateTimes that do not exist due DST jump

To get back to the point, in my opinion there is an unnecessary/incorrect conversion being performed on LocalDateTime.

Of course, I also understand the argument that LocalDateTime is always converted to the current JVM time zone
(which is what LocalDateTime.ofInstant(instant, ZoneOffset.systemDefault()) would return) and as I said, I don't have any trouble with that at the moment either, because I always use Instant and the conversion to the user time zone is done by the application.

@rbygrave I hope I was able to explain the problem better now. We would just have to determine now whether you think it's a bug or works as designed and then adjust the documentation if necessary

@rob-bygrave
Copy link
Contributor

Yeah cool. Thanks for you patience - I think I've got a handle on the issue now, thanks !!!

Both app server run on different time-zone

Yup. I got that.

Caveat: Just to say, my personal view/recommendation is that "All our servers run in UTC". For anyone reading this I want to be clear that servers in different timezones is something I'd never do/suggest/recommend and maybe that is a NZ or cloud centric perspective (we never expect or rely on our servers running in local time).

... so that said, lets keep going with this with "Both app server run on different time-zone" scenario.

Background JDBC, Calendar time-zone and Ebean:

JDBC PreparedStatement can take a Calendar that can explicitly specify the timezone to be used when binding and reading the resultSet.

The tests here are doing this and in this way simulating / getting the same result as different app servers running in different time zones.

Ebean will use the Calendar for Timestamp(y) types Instant, OffsetDateTime, ZonedDateTime, java.sql.Timestamp AND LocalDateTime [TLDR - this is ultimately the issue/question here I believe].

Ebean will NOT use the Calendar for Date(y) & Time(y) types - LocalDate, LocalTime, Date, Time [except for MySql which works differently from all the others].

LocalDateTime (gets the timezone treatment)

When we create ebean Database with explicit time-zone, then we have the Calendar and that IS used when binding and reading LocalDateTime.

Note that this is unlike LocalDate, LocalTime, Date, Time which all operate without using the Calendar (which makes sense).


Q: With LocalDateTime, should ebean use the Calendar when binding preparedStatement and reading resultSet?

Is this the question we are asking here?
Is this exactly the same as running the JVM on different timezones?

@rPraml Did I understand correctly?

@rPraml
Copy link
Contributor

rPraml commented Aug 11, 2023

Q: With LocalDateTime, should ebean use the Calendar when binding preparedStatement and reading resultSet?

Q2: Are there other options in the JDBC API (and if so, are they supported by all drivers) to read/write LocalDateTimes, so that they get NO timezone treatment?

maybe rs.getObject / stmt.setObject, but AFAIR, the DB2 driver does not support this.

Is this the question we are asking here?

I'm unsure. I would ask: Why does a LocalDateTime gets a timezone treatment at all (= the values will change, if the JVM TZ changes) and why does the value change, if the JVM TZ changes.

Facts:

  • If I serialize a LDT and deserialize it on an other machine with different TZ, I get the same value
  • the same, if I write a LDT to JSON and read the JSON on an other machine (if ISO8601 is used)
  • the same would I expect from ebean. Launching the app on a server with a different TZ should not mix up the LDTs.

BTW: Hibernate seems to have/had similar problems with LocalDateTimes

Is this exactly the same as running the JVM on different timezones?

I'm unsure here, what happens, if you do not pass a calendar. There are so many tz-conversions in the JDBC API when saving timestamps and not all drivers behave the same (see Mysql for example) - maybe it requires some platform specific workarounds.

@rbygrave
Copy link
Member

why does the value change, if the JVM TZ changes

Well, LocalDateTime is challenging to the point that there isn't an official implementation / pure JDBC use case to compare to as I understand it.

As I see it there really isn't a "perfect solution" that works for LocalDateTime use with multiple JVMs in different timezones. So imo I don't think we will find a "solution" here unless it is mapped to some database type that most databases do not have or alternatively mapped to a varchar (which people could do themselves if they wanted to).

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 this pull request may close these issues.

None yet

4 participants