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

Valid DateTime treated as invalid #1065

Closed
nirvdrum opened this Issue Oct 1, 2013 · 10 comments

Comments

Projects
None yet
4 participants
@nirvdrum
Copy link
Contributor

nirvdrum commented Oct 1, 2013

In JRuby 1.7.5 dev, DateTime.new(0) raises an ArgumentError and indicates it is an invalid date. This same invocation works fine in JRuby 1.7.4 and MRI 2.0.0-p247. My guess is the Joda-Time changes that made their way into 1.7.5 recently are the cause.

I can work around this in my app for now, but it seems an exception shouldn't be raised. As for a use case, I use this (clearly old) date is used as a null object for sorting operations.

@BanzaiMan

This comment has been minimized.

Copy link
Member

BanzaiMan commented Oct 1, 2013

irb(main):001:0> RUBY_DESCRIPTION
=> "jruby 1.7.5.dev (1.9.3p392) 2013-10-01 70428b0 on Java HotSpot(TM) 64-Bit Server VM 1.7.0_40-b43 [darwin-x86_64]"
irb(main):002:0> require 'date'
=> true
irb(main):003:0> DateTime.new(0)
ArgumentError: invalid date
        from /Users/asari/Development/src/jruby/lib/ruby/1.9/date.rb:1702:in `civil'
        from (irb):3:in `evaluate'
        from org/jruby/RubyKernel.java:1121:in `eval'
        from org/jruby/RubyKernel.java:1517:in `loop'
        from org/jruby/RubyKernel.java:1282:in `catch'
        from org/jruby/RubyKernel.java:1282:in `catch'
        from /Users/asari/Development/src/jruby/bin/jirb:13:in `(root)'

jruby/lib/ruby/1.9/date.rb

Lines 1699 to 1703 in 8d8f342

begin
dt = JODA::DateTime.new(y, m, d, h, min, s, ms, chronology(sg, of))
rescue JODA::IllegalFieldValueException, Java::JavaLang::IllegalArgumentException
raise ArgumentError, 'invalid date'
end

@BanzaiMan

This comment has been minimized.

Copy link
Member

BanzaiMan commented Oct 1, 2013

By not rescuing the Joda exception, we get a more informative error message:

JulianChronology.java:80:in `adjustYearForSet': org.joda.time.IllegalFieldValueException: Value 0 for year is not supported
        from JulianChronology.java:207:in `getDateMidnightMillis'
@headius

This comment has been minimized.

Copy link
Member

headius commented Oct 1, 2013

Copying @eregon and @enebo. This probably should be fixed before release... high priority.

@headius

This comment has been minimized.

Copy link
Member

headius commented Oct 1, 2013

Oh...and we obviously need a test for this somewhere :-)

@eregon eregon closed this in aa03f16 Oct 1, 2013

@eregon

This comment has been minimized.

Copy link
Member

eregon commented Oct 1, 2013

That off-by-one error ... tests added in test/ for simplicity (a428949).
Sorry about that, it is quite a problem to map joda-time years to Date years during Julian chronology.
Currently, the code assumes the calendar reform is always after year 0, or Date::GREGORIAN = -Infinity.new (anyway, joda-time does not support reform before 0001-01-01 and it makes no sense).

@eregon

This comment has been minimized.

Copy link
Member

eregon commented Oct 1, 2013

@nirvdrum BTW, you might want to use simply DateTime.new (that is -4712-01-01, jd 0) as an older-than-anything date.

@nirvdrum

This comment has been minimized.

Copy link
Contributor Author

nirvdrum commented Oct 1, 2013

Ahh, thanks for the tip.

eregon added a commit that referenced this issue Oct 1, 2013

@BanzaiMan

This comment has been minimized.

Copy link
Member

BanzaiMan commented Oct 1, 2013

I cherry-picked these commits to the 1.7 branch as e3b1080 and 47e7cb5.

@nirvdrum

This comment has been minimized.

Copy link
Contributor Author

nirvdrum commented Oct 2, 2013

Looking good for me now. Thanks for the quick turnaround.

@headius

This comment has been minimized.

Copy link
Member

headius commented Oct 2, 2013

Thanks for fixing, @eregon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.