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

Incorrect values resulting from to_date on a Time object #5279

Closed
WToa opened this issue Aug 9, 2018 · 8 comments
Closed

Incorrect values resulting from to_date on a Time object #5279

WToa opened this issue Aug 9, 2018 · 8 comments
Milestone

Comments

@WToa
Copy link

@WToa WToa commented Aug 9, 2018

We were upgrading our environments to match JRuby 9.2.0.0 and noticed that calling to_date on Time objects lead to different results compared to previous iterations of JRuby and MRI.

Environment

JRuby 9.2.0.0

Expected Behavior

JRuby 9.1.17.0

jruby-9.1.17.0 :006 > Date.today.eql?(Time.now.to_date)
 => true 
jruby-9.1.17.0 :007 > Time.now.to_date.inspect
 => "#<Date: 2018-08-09 ((2458340j,0s,0n),+0s,2299161j)>" 
jruby-9.1.17.0 :008 > Date.today.inspect
 => "#<Date: 2018-08-09 ((2458340j,0s,0n),+0s,2299161j)>" 

Ruby 2.3.3

2.3.3 :005 > Date.today.eql?(Time.now.to_date)
 => true 
2.3.3 :006 > Time.now.to_date.inspect
 => "#<Date: 2018-08-09 ((2458340j,0s,0n),+0s,2299161j)>" 
2.3.3 :007 > Date.today.inspect
 => "#<Date: 2018-08-09 ((2458340j,0s,0n),+0s,2299161j)>" 

Ruby 2.5.1

2.5.1 :004 > Date.today.eql?(Time.now.to_date)
 => true 
2.5.1 :005 > Time.now.to_date.inspect
 => "#<Date: 2018-08-09 ((2458340j,0s,0n),+0s,2299161j)>" 
2.5.1 :006 > Date.today.inspect
 => "#<Date: 2018-08-09 ((2458340j,0s,0n),+0s,2299161j)>" 

Actual Behavior

jruby-9.2.0.0 :002 > Date.today.eql?(Time.now.to_date)
 => false 
jruby-9.2.0.0 :003 > Time.now.to_date.inspect
 => "#<Date: 2018-08-09 ((2458340j,0s,0n),+0s,0j)>" 
jruby-9.2.0.0 :004 > Date.today.inspect
 => "#<Date: 2018-08-09 ((2458340j,0s,0n),+0s,2299161j)>" 
@WToa
Copy link
Author

@WToa WToa commented Aug 9, 2018

Not sure why this is, we looked around in the commits and found:

f30fdfe#diff-c549944ab214525639bedaeffbaac520R61

It seems that the constant ITALY is being replaced by 0 in this.

@dr-itz
Copy link
Contributor

@dr-itz dr-itz commented Aug 10, 2018

I run into date related problems on 9.2.0 recently. They were already fixed on master in my case and it looks like this one is ok too:

jruby-head :001 > require 'date'
 => true 
jruby-head :002 > Date.today.eql?(Time.now.to_date)
 => true 
jruby-head :003 > Time.now.to_date.inspect
 => "#<Date: 2018-08-10 ((2458341j,0s,0n),+0s,2299161j)>" 
jruby-head :004 > Date.today.inspect
 => "#<Date: 2018-08-10 ((2458341j,0s,0n),+0s,2299161j)>" 

for reference, it's not exactly the latest, but close enough:

> jruby -v
jruby 9.2.1.0-SNAPSHOT (2.5.0) 2018-07-30 13b2df5 Java HotSpot(TM) 64-Bit Server VM 25.162-b12 on 1.8.0_162-b12 +jit [darwin-x86_64]

So the upcoming 9.2.1 should be ok..

@headius
Copy link
Member

@headius headius commented Aug 16, 2018

@dr-itz Thanks for confirming it's fixed on master! We did do some rework of Date in 9.2. and some subsequent fixes in 9.2.1.

@nightsurge
Copy link

@nightsurge nightsurge commented Jan 22, 2021

I am getting this same exact issue on 9.2.14.0 (among other newer versions I also tried)

Time.parse('0001-01-01').to_date Is returning Jan 3, 0001, instead of Jan 1, 0001.
In our app there are RPG/DB2 legacy tables that store 0001-01-01 as a default date. We are trying to check for that default value and it is now failing because of this bug. It appears when this string is parsed as a Date when creating the Ruby/ActiveRecord object, it must be using Time#to_date at some point, yes?

Thoughts? @dr-itz @headius @kares

@ahorek
Copy link
Contributor

@ahorek ahorek commented Jan 22, 2021

@nightsurge this is expected. If you call Time.parse('0001-01-01') Ruby doesn't know if you mean Gregorian or Julian calendar. There's a leap year's difference. It may be surprising, but this conversion is actually unsafe for years < 1583. I don't think it'll ever be fixed for compatibility reasons, but JRuby behaves the same way as CRuby.

you may consider using DateTime instead

Time.parse("0001-01-01").to_datetime
=> #<DateTime: 0001-01-01T00:00:00+01:00 ((1721423j,82800s,0n),+3600s,2299161j)>
Time.parse("0001-01-01").to_datetime.julian?
=> true

take a look at https://stevemorse.org/jcal/julian.html

@nightsurge
Copy link

@nightsurge nightsurge commented Jan 22, 2021

@ahorek when did this behavior change? We recently updated from Rails 4.1 to Rails 5.2, JRuby 9.1.15 to 9.2.14, and activerecord-jdbc-adapter to the 52-stable version.

Prior to these updates, our database models would correctly translate the '0001-01-01' database value to the correct model attribute of a Date with Jan 1, 0001.

I'm saying that when we do an ActiveRecord lookup policy = Policy.find(1234) and look at policy.some_date we used to get the Date object of Jan 01, 0001, but now doing that same process it equates to Jan 03, 0001. I get that it is related to Gregorian calendars, but I am trying to figure out where this behavior stems from so I can determine if we need to monkey-patch something, or if we have to just update our Rails code to look for Jan 3, 0001 instead of Jan 1, 0001.

I'm not even sure if this is a JRuby issue or something in ActiveModel/ActiveRecord, or JDBC Adapter...

@ahorek
Copy link
Contributor

@ahorek ahorek commented Jan 22, 2021

could you open a new issue? it looks more like a bug somewhere in activerecord-jdbc-adapter.

it would be great if you could track where's the conversion being made, make a test case, or compare it with a different db adapter (on CRuby)

@nightsurge
Copy link

@nightsurge nightsurge commented Jan 26, 2021

I was able to track the change to the adapter when it moved to 50-stable and newer! That's when the gregorian calendar was put in place. I didn't "fix" it by checking for dates and handling pre-post gregorian, but I did make the fix for my scenario seen here:

jruby/activerecord-jdbc-adapter#1084

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

Successfully merging a pull request may close this issue.

None yet
5 participants