jruby / activerecord-jdbc-adapter

JRuby's ActiveRecord adapter using JDBC.
BSD 2-Clause "Simplified" License
461 stars 385 forks source link

Java::OrgJodaTime::IllegalInstantException on date columns #1065

Closed felipefava closed 3 years ago

felipefava commented 4 years ago

I recently updated activerecord 4 to activerecord 5 in a non Rails app with jruby-9.2.7.0. Now we have this version of the gem:

activerecord-jdbcmysql-adapter (52.4-java)
activerecord-jdbc-adapter (= 52.4)
jdbc-mysql (~> 5.1.36, < 9)

I tried also upgrading the version to 52.6 but we still have the issue.

The config ActiveRecord::Base.default_timezone value is :utc.


The first problem we faced, was that datetimes were cast wrongly, but we fixed it with this https://github.com/jruby/activerecord-jdbc-adapter#mysql-specific-notes. More details of that issue here.

However, that fix brings us the following issue.

If we have a record with a Date column where the value of the date is the day where the Daylight Saving Time was made, we have this exception:

Java::OrgJodaTime::IllegalInstantException (Illegal instant due to time zone offset transition (daylight savings time 'gap'): 2019-10-06T00:00:00.000 (America/Asuncion))

The important part of the above exception is this 2019-10-06T00:00:00.000 (America/Asuncion)). To understand better the problem, in the timezone America/Asuncion, on the 2019-10-06 begun the Daylight Saving Time. And if I try to find a record with ActiveRecord that has a column date with the value 2019-10-06 it raises the mentioned exception.

In the code of this gem, I saw that for dates columns initialize a dateTime at the beginning fo the day:

# RubyJbdcConnection.java -> method dateToRuby

return DateTimeUtils.newDateAsTime(context, value, null).callMethod(context, "to_date");

# DateTimeUtils.java -> method newDateAsTime

DateTime dateTime = new DateTime(year, month, day, 0, 0, 0, 0, zone);
return RubyTime.newTime(context.runtime, dateTime, 0);

I don't have experience with java and I don't know if it is necessary, but why it creates a datetime and not a date?

I think it shouldn't fail in this case when what I want to instantiate is a date column and not a datetime.

FYI: with activerecord 4 and activerecord-jdbc-adapter (= 1.3.25) this wasn't an issue

Please let me know if I'm missing something, and thanks for the help.


More details of my problem:

jruby-9.2.7.0 :001 > Bill.find(48129508)
Traceback (most recent call last):
       16: from arjdbc.jdbc.RubyJdbcConnection.withConnection(RubyJdbcConnection.java:3514)
       15: from arjdbc.jdbc.RubyJdbcConnection$15.call(RubyJdbcConnection.java:1148)
       14: from arjdbc.jdbc.RubyJdbcConnection$15.call(RubyJdbcConnection.java:1157)
       13: from arjdbc.jdbc.RubyJdbcConnection.mapQueryResult(RubyJdbcConnection.java:1251)
       12: from arjdbc.jdbc.RubyJdbcConnection.mapToResult(RubyJdbcConnection.java:2151)
       11: from arjdbc.jdbc.RubyJdbcConnection.mapRow(RubyJdbcConnection.java:3718)
       10: from arjdbc.mysql.MySQLRubyJdbcConnection.jdbcToRuby(MySQLRubyJdbcConnection.java:184)
        9: from arjdbc.jdbc.RubyJdbcConnection.jdbcToRuby(RubyJdbcConnection.java:2189)
        8: from arjdbc.jdbc.RubyJdbcConnection.dateToRuby(RubyJdbcConnection.java:2351)
        7: from arjdbc.util.DateTimeUtils.newDateAsTime(DateTimeUtils.java:256)
        6: from org.joda.time.DateTime.<init>(DateTime.java:503)
        5: from org.joda.time.base.BaseDateTime.<init>(BaseDateTime.java:226)
        4: from org.joda.time.base.BaseDateTime.<init>(BaseDateTime.java:257)
        3: from org.joda.time.chrono.AssembledChronology.getDateTimeMillis(AssembledChronology.java:133)
        2: from org.joda.time.chrono.ZonedChronology.getDateTimeMillis(ZonedChronology.java:122)
        1: from org.joda.time.chrono.ZonedChronology.localToUTC(ZonedChronology.java:157)
Java::OrgJodaTime::IllegalInstantException (Illegal instant due to time zone offset transition (daylight savings time 'gap'): 2019-10-06T00:00:00.000 (America/Asuncion))

The db record:

mysql> SELECT * FROM bills WHERE id = 48129508 \G;
*************************** 1. row ***************************
                      id: 48129508
                due_date: 2019-10-06
                  amount: 109281
                   ......

The db table:

mysql> desc bills;
+--------------------------+---------------------+------+-----+---------+----------------+
| Field                    | Type                | Null | Key | Default | Extra          |
+--------------------------+---------------------+------+-----+---------+----------------+
| id                       | bigint(20) unsigned | NO   | PRI | NULL    | auto_increment |
| due_date                 | date                | YES  |     | NULL    |                |
...... 
felipefava commented 4 years ago

Could this change be a possible fix? 🤔

I don't know if this could possibly affect other things or create new bugs, but with this change I don't get the Java::OrgJodaTime::IllegalInstantException error anymore.

Original code

    protected IRubyObject dateToRuby(final ThreadContext context,
        final Ruby runtime, final ResultSet resultSet, final int column)
        throws SQLException {

        final Date value = resultSet.getDate(column);
        if ( value == null ) {
            // FIXME: Do we really need this wasNull check here?
            return resultSet.wasNull() ? context.nil : RubyString.newEmptyString(runtime);
        }

        if ( rawDateTime != null && rawDateTime.booleanValue() ) {
            return RubyString.newString(runtime, DateTimeUtils.dateToString(value));
        }

        return DateTimeUtils.newDateAsTime(context, value, null).callMethod(context, "to_date");
    }

New code:

    protected IRubyObject dateToRuby(final ThreadContext context,
        final Ruby runtime, final ResultSet resultSet, final int column)
        throws SQLException {

        final Date value = resultSet.getDate(column);
        if ( value == null ) {
            // FIXME: Do we really need this wasNull check here?
            return resultSet.wasNull() ? context.nil : RubyString.newEmptyString(runtime);
        }

        if ( rawDateTime != null && rawDateTime.booleanValue() ) {
            return RubyString.newString(runtime, DateTimeUtils.dateToString(value));
        }

        // This line is the only change
        return DateTimeUtils.newDate(context, value);
        // Or this also works
        return DateTimeUtils.newDateAsTime(context, value, DateTimeZone.UTC).callMethod(context, "to_date");
    }
headius commented 4 years ago

Sorry for the delay in responding, I'm catching up now.

headius commented 4 years ago

If we have a record with a Date column where the value of the date is the day where the Daylight Saving Time was made, we have this exception

Good find!

I don't have experience with java and I don't know if it is necessary, but why it creates a datetime and not a date?

JRuby's implementation of the Ruby Time object uses Joda Time's DateTime class.

Could this change be a possible fix?

It could be! Do you think you could turn your change into a PR (probably using the non-UTC path) and we'll work on it from there?

headius commented 4 years ago

FWIW I know there's a few methods on DateTime for negotiating these DST boundaries, but I do not currently know the algorithm for properly handling this case.

felipefava commented 4 years ago

Sure, I can create the PR in the afternoon, after work.

I think the more similar approach of the current implementation would be this

return DateTimeUtils.newDateAsTime(context, value, DateTimeZone.UTC).callMethod(context, "to_date");

In fact, I have currently a fork with that same change using it in production, and it is working fine.

What do you think? Should I go with that fix or the one it uses DateTimeUtils.newDate?