spring-projects / spring-data-relational

Spring Data Relational. Home of Spring Data JDBC and Spring Data R2DBC.
https://spring.io/projects/spring-data-jdbc
Apache License 2.0
736 stars 339 forks source link

Timezone handling #986

Open lpandzic opened 3 years ago

lpandzic commented 3 years ago

For an Instant defined as:

LocalDate.of(2021, 1, 1).atStartOfDay(ZoneOffset.UTC).toInstant()

when using Spring Data JDBC repository save method I'd expect the end result in MSSQL database column of type datetime2 to be of value: 2021-01-01 00:00:00.0000000 but it contains value 2021-01-01 01:00:00.0000000 suggesting that Spring Data JDBC uses my machine timezone when converting Instant.

Is this behavior documented anywhere and can it be configured somehow? I'd like my application to always use UTC unless a type with explicit timezone and/or offset is used.

schauder commented 3 years ago

Is it possible, that you get mislead by the toString of the timestamp?

If I save an entity with the instant as described, and load the timestamp using a JdbcTemplate and ResultSet.getTimestamp and print the result with the following code

                WithInstant entity = new WithInstant();
        entity.testTime = LocalDate.of(2021, 1, 1).atStartOfDay(ZoneOffset.UTC).toInstant();
        entity.id = 23L;
        template.insert(entity);

        WithInstant loaded = template.findById(23L, WithInstant.class);

        assertThat(loaded.testTime).isEqualTo(entity.testTime);

        Timestamp timestamp = jdbcTemplate.queryForObject("select test_time from with_instant where id = 23", emptyMap(), (rs, i) -> rs.getTimestamp(1));
        System.out.println(timestamp);
        System.out.println(timestamp.getTime());
        System.out.println(entity.testTime);
        System.out.println(entity.testTime.getEpochSecond());

I get the following output:

2021-01-01 01:00:00.0
1609459200000
2021-01-01T00:00:00Z
1609459200

I'm in CET, which is one hour ahead of UTC right now.

lpandzic commented 3 years ago

I've tested against 2.2.1 and 2.3.0-SNAPSHOT.

I'm looking in database viewer (IDEA): Screenshot 2021-06-14 at 13 27 49.

I'm in CEST (+1).

In your case - what's the value stored in database?

lpandzic commented 3 years ago

I've tested against JPA and it seems it also results in spring data jdbc result - 2021-01-01 01:00:00.0000000. Can you please confirm and provide your results?

lpandzic commented 3 years ago

Is it possible, that you get mislead by the toString of the timestamp?

The original source of my issue is located here in this test - https://github.com/infobip/infobip-spring-data-querydsl/blob/issue-31-fix/infobip-spring-data-jdbc-querydsl/src/test/java/com/infobip/spring/data/jdbc/QuerydslJdbcRepositoryTest.java#L198-L212 Both Querydsl and Spring Data say that they store the correct value in database and that the other one is wrong.

When I query the database with following tsql:

SELECT DATEDIFF(SECOND, '19700101', CreatedAt) FROM Person

I get 1609462800 as result for Spring Data which https://www.epochconverter.com/ tells me is GMT: Friday, January 1, 2021 1:00:00 or Your time zone: Friday, January 1, 2021 2:00:00 which doesn't seem right.

schauder commented 3 years ago

I think we got bitten by the JDBC API which seems to have a rather creative interpretation of dates.

This Stackoverflow answer tipped me of. It talks about variants of setTimestamp. We don't use that but setObject but I guess the JDBC driver just delegates to setTimestamp.

So here is what seems to happen.

I assume setObject(int, Object) delegates to setTimestamp(int, Timestamp)

void setTimestamp(int parameterIndex, Timestamp x) throws SQLException Sets the designated parameter to the given java.sql.Timestamp value. The driver converts this to an SQL TIMESTAMP value when it sends it to the database.

This intern probably delegates to

void setTimestamp(int parameterIndex, Timestamp x, Calendar cal) throws SQLException Sets the designated parameter to the given java.sql.Timestamp value, using the given Calendar object. The driver uses the Calendar object to construct an SQL TIMESTAMP value, which the driver then sends to the database. With a Calendar object, the driver can calculate the timestamp taking into account a custom timezone. If no Calendar object is specified, the driver uses the default timezone, which is that of the virtual machine running the application.

Now the interesting part is of course the statement:

The driver uses the Calendar object to construct an SQL TIMESTAMP value

java.sql.Timestamp are java.util.Date instances and as such are supposed to be in UT = GMT ~= UTC and this is the way Spring Data uses them.

But JDBC (or at least the driver of MS SQL Server) seems to do the following:

And it odes the reverse when loading the timestamp from the database again.

At least that is my current interpretation of what I'm seeing and reading. I'm not asking if this makes sens

lpandzic commented 3 years ago

I've debugged the test and Spring Data JDBC invokes setTimestamp method on PreparedStatement with java.sql.Timestamp and my understanding is that it's game over by that point. Querydsl SQL module on the other hand doesn't seem to be using PreparedStatement but generates the Insert Statement with com.querydsl.sql.SQLSerializer. Not sure I agree with either approach but I can understand/appreciate how it got there.

schauder commented 3 years ago

While I think the JDBC API or SQL Servers driver implementation is using Timestamp wrong there isn't much of a point in bitching about this.

But we can't really switch to an approach where we provide a Calendar or change the way we convert to Timestamp without breaking peoples existing data. So we'll probably introduce some kind of configuration property that allows to specify a timezone to be used with arguments that are passed as Timestamp to statements.

lpandzic commented 3 years ago

I agree and to clarify - I wanted to identify root of the issue before proceeding.

I'm trying to solve this issue (https://github.com/infobip/infobip-spring-data-querydsl/issues/31) because I've run out of easy options to prevent users from stumbling on this issue - that is time handling difference between Spring Data JDBC and Querydsl. Previously I've opted in to Spring Data JDBC of doing things but current infrastructure is entity focused and is not, at least on API surface, friendly to projections. I'm aware of https://github.com/spring-projects/spring-data-jdbc/pull/980 and wanted to ask what's the recommended approach on how to bridge this problem. Critical code path is located here - https://github.com/infobip/infobip-spring-data-querydsl/blob/master/infobip-spring-data-jdbc-querydsl/src/main/java/com/infobip/spring/data/jdbc/QuerydslJdbcPredicateExecutor.java#L160-L170.

I believe the following "hack" would work but I have yet to test it:

<O> List<O> queryMany(SQLQuery<O> query) {
    @SuppressWarnings("unchecked")
    RowMapper<O> rowMapper = (RowMapper<O>) new EntityRowMapper<>(entity, converter);
    RowMapperResultSetExtractor<O> rowMapperResultSetExtractor = new RowMapperResultSetExtractor<>(rowMapper);
    List<O> result = query(query, rowMapperResultSetExtractor);

    if (Objects.isNull(result)) {
        return Collections.emptyList();
    }

    return result;
}
lpandzic commented 3 years ago

After further investigation the "hack" I mentioned above doesn't work and mapping SQLQuery projection expression to a custom row mapper or something similar doesn't seem like something that would scale or make sense really. The only workaround I could find is to set JVM level timezone TimeZone.setDefault(TimeZone.getTimeZone(ZoneOffset.UTC))which is bad, really bad.

It'd be great if Spring Data JDBC provided some sort of override mechanism.

schauder commented 3 years ago

We'll probably implement some way to configure this. In the meantime you should be able to register custom converters for the conversion from and to Timestamp.

schauder commented 2 years ago

@Demo-80 please do not hijack only vaguely related tickets. Also, your question seems to be a usage question. Please ask those over on Stackoverflow.

If you think you found a bug or want to share an idea for a feature or improvement feel free to create a ticket.

Demo-80 commented 2 years ago

@Demo-80请不要只劫持相关的票证。另外,您的问题似乎是一个用法问题。请在 Stackoverflow 上询问那些人。

如果您认为自己发现了错误或想要分享功能或改进的想法,请随时创建票证。 I'm sorry