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

timezones are not handled in DefaultBinding #5504

Open
BrentDouglas opened this issue Aug 19, 2016 · 10 comments
Open

timezones are not handled in DefaultBinding #5504

BrentDouglas opened this issue Aug 19, 2016 · 10 comments

Comments

@BrentDouglas
Copy link
Contributor

BrentDouglas commented Aug 19, 2016

When setting and getting timestamps, times and dates from the result set DefaultBinding calls:

java.sql.Timestamp getTimestamp(int columnIndex);
java.sql.Time getTime(int columnIndex);
java.sql.Date getDate(int columnIndex);
void setTimestamp(int parameterIndex, java.sql.Timestamp x) throws SQLException;
void setTime(int parameterIndex, java.sql.Time x) throws SQLException;
void setDate(int parameterIndex, java.sql.Date x) throws SQLException;

rather than

java.sql.Timestamp getTimestamp(int columnIndex, Calendar cal);
java.sql.Time getTime(int columnIndex, Calendar cal);
java.sql.Date getDate(int columnIndex, Calendar cal);
void setTimestamp(int parameterIndex, java.sql.Timestamp x, Calendar cal) throws SQLException;
void setTime(int parameterIndex, java.sql.Time x, Calendar cal) throws SQLException;
void setDate(int parameterIndex, java.sql.Date x, Calendar cal) throws SQLException;

As a result the timezone offset is not accounted for and the retrieved value is incorrect.
This might be a PG only issue.

jooq pro 3.8.3
postgresql 9.5.5
oracle java 1.8.0_92
OSX

@BrentDouglas
Copy link
Contributor Author

Nope, never mind, there was some wrapper code in our project I had missed that was causing the issue.

@BrentDouglas
Copy link
Contributor Author

BrentDouglas commented Aug 20, 2016

I'm going to reopen this, after a lot of being confused this was actually the cause of my issue. Implementing a binding that provides a correct calendar results in the correct values. I think that it is probably specific to PG.

To clarify what is causing the issue, PG's timezone is set to Australia/Victoria, my laptop's TZ is set to America/Toronto. Before running our testsuite I'm setting the joda, jdk default timezones and the PG session timezone to Europe/Berlin:

    public static final DateTimeZone CEST = DateTimeZone.forOffsetHours(2);

    @Before
    public void before() {
        final TimeZone zone = CEST.toTimeZone();
        TimeZone.setDefault(zone);
        DateTimeZone.setDefault(CEST);
        db.execute("set timezone to 'Europe/Berlin'");
    }

This results in Timestamps and Dates being fetched incorrectly. Adding a specific binding for dates and timestamps with a calendar (that has it's timezone updated in the above method) passed into those methods fixes the issue.

    public static final DateTimeZone CEST = DateTimeZone.forOffsetHours(2);
    public static final Calendar CALENDAR = Calendar.getInstance();

    @Before
    public void before() {
        final TimeZone zone = CEST.toTimeZone();
        TimeZone.setDefault(zone);
        CALENDAR.setTimeZone(zone);
        DateTimeZone.setDefault(CEST);
        db.execute("set timezone to 'Europe/Berlin'");
    }

The bindings that work:

public class DateTimeBinding extends DefaultBinding<Timestamp, DateTime> {

    private static final long serialVersionUID = 1L;

    public DateTimeBinding() {
        super(new DateTimeConverter());
    }

    @Override
    public void sql(final BindingSQLContext<DateTime> ctx) {
        final RenderContext render = ctx.render();
        if (render.paramType() == INLINED) {
            final DateTime val = ctx.value();
            if (val == null) {
                render.sql("null");
            } else {
                render.keyword("timestamp '").sql(val.toString()).sql('\'');
            }
        } else {
            super.sql(ctx);
        }
    }

    @Override
    public void register(final BindingRegisterContext<DateTime> ctx) throws SQLException {
        ctx.statement().registerOutParameter(ctx.index(), Types.TIMESTAMP_WITH_TIMEZONE);
    }

    @Override
    public void get(final BindingGetResultSetContext<DateTime> ctx) throws SQLException {
        ctx.value(_convert(ctx.resultSet().getTimestamp(ctx.index(), CALENDAR)));
    }

    @Override
    public void get(final BindingGetStatementContext<DateTime> ctx) throws SQLException {
        ctx.value(_convert(ctx.statement().getTimestamp(ctx.index(), CALENDAR)));
    }

    @Override
    public void get(final BindingGetSQLInputContext<DateTime> ctx) throws SQLException {
        ctx.value(_convert(ctx.input().readTimestamp()));
    }

    @Override
    public void set(final BindingSetStatementContext<DateTime> ctx) throws SQLException {
        ctx.statement().setTimestamp(ctx.index(), _convert(ctx.value()), CALENDAR);
    }

    @Override
    public void set(final BindingSetSQLOutputContext<DateTime> ctx) throws SQLException {
        ctx.output().writeTimestamp(_convert(ctx.value()));
    }

    private DateTime _convert(final Timestamp in) throws SQLException {
        return in == null ? null : converter().from(in);
    }

    private Timestamp _convert(final DateTime in) throws SQLException {
        return in == null ? null : converter().to(in);
    }
}
public class LocalDateBinding extends DefaultBinding<Date, LocalDate> {

    private static final long serialVersionUID = 1L;

    public LocalDateBinding() {
        super(new LocalDateConverter());
    }

    @Override
    public void sql(final BindingSQLContext<LocalDate> ctx) {
        final RenderContext render = ctx.render();
        if (render.paramType() == INLINED) {
            final LocalDate val = ctx.value();
            if (val == null) {
                render.sql("null");
            } else {
                render.keyword("date '").sql(val.toString()).sql('\'');
            }
        } else {
            super.sql(ctx);
        }
    }

    @Override
    public void register(final BindingRegisterContext<LocalDate> ctx) throws SQLException {
        ctx.statement().registerOutParameter(ctx.index(), Types.DATE);
    }

    @Override
    public void get(final BindingGetResultSetContext<LocalDate> ctx) throws SQLException {
        ctx.value(_convert(ctx.resultSet().getDate(ctx.index(), CALENDAR)));
    }

    @Override
    public void get(final BindingGetStatementContext<LocalDate> ctx) throws SQLException {
        ctx.value(_convert(ctx.statement().getDate(ctx.index(), CALENDAR)));
    }

    @Override
    public void get(final BindingGetSQLInputContext<LocalDate> ctx) throws SQLException {
        ctx.value(_convert(ctx.input().readDate()));
    }

    @Override
    public void set(final BindingSetStatementContext<LocalDate> ctx) throws SQLException {
        ctx.statement().setDate(ctx.index(), _convert(ctx.value()), CALENDAR);
    }

    @Override
    public void set(final BindingSetSQLOutputContext<LocalDate> ctx) throws SQLException {
        ctx.output().writeDate(_convert(ctx.value()));
    }

    private LocalDate _convert(final Date in) throws SQLException {
        return in == null ? null : converter().from(in);
    }

    private Date _convert(final LocalDate in) throws SQLException {
        return in == null ? null : converter().to(in);
    }
}

@BrentDouglas BrentDouglas reopened this Aug 20, 2016
@BrentDouglas
Copy link
Contributor Author

I'll also note that in the bindings above the DateTime one works here as it writes the joda DateTime to the DB rather than the jdk Timestamp. The DefaultBinding would need to manually append the timezone info from the Calendar to the Timestamp when writing to a timestamptz column as it does not contain the timezone. This is a slightly different issue that only affects timestamptz columns and results in not having the timezone set, while the calendar part above affects both DateTime and LocalDates and gives them the wrong offset even when the timezone is set.

@chwassme
Copy link

chwassme commented Sep 8, 2016

It seems that we encountered the same problem in an Oracle DB.
jooq pro 3.8.1
oracle db 12c
oracle java 1.8.0_60
OSX

@chwassme
Copy link

chwassme commented Sep 9, 2016

I must admit that we have already a custom DateTimeBinding for "TIMESTAMP WITH TIMEZONE" in place, but it's not working correctly...

@lukaseder
Copy link
Member

Indeed, the TIMESTAMP WITH TIMEZONE data type support is not yet completely implemented in jOOQ. Experimental java.time support has been added to jOOQ 3.7 (#4338), but we're not there yet to say we officially support it in all databases. In particular, the semantics of this data type may differ between databases, e.g. in PostgreSQL, it has java.time.Instant semantics, whereas in Oracle, it's a mixture between java.time.OffsetDateTime and java.time.ZonedDateTime.

I'm hoping this will be part of jOOQ 3.9

@BrentDouglas
Copy link
Contributor Author

@chwassme i thought the oracle driver uses the stream api which doesn't accept a tz. I think you would have to manually fix the offset in the binding.

@BrentDouglas
Copy link
Contributor Author

The more I think about this, I think that jooqs timestamp with timezone datatype should target either String, OffsetDateTime or some jooq specific type wrapping both the Timestamp and Timezone. The way it is at the moment there's no way to recover the lost information. You can still get the correct time by setting the session tz (in pg anyway) or converting to utc or something in the db before sending it but imo that's not a great solution. It would be better if it 'just worked' out of the box

@eatgrass
Copy link

now I am using 3.11.11, code generated with javaTimeTypes=true got a LocalDateTime, but the timestamp we access from database is Instant I think.

@lukaseder
Copy link
Member

@eatgrass now I am using 3.11.11, code generated with javaTimeTypes=true got a LocalDateTime, but the timestamp we access from database is Instant I think.

Thank you for your comment. I'm not sure this is related. This issue here is not about code generation. Would you mind creating a new issue with some more details (including the DDL of the table, the full code generation configuration)?

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

No branches or pull requests

4 participants