search

dkubb / axiom

Simplifies querying of structured data using relational algebra
https://github.com/dkubb/axiom
MIT License
459 stars 22 forks source link

Breaks with activesupport 3.2.8 #15

Closed ghost closed 11 years ago

ghost commented 11 years ago

veritas master and activesupport 3.2.8

require "active_support/core_ext/date_time/conversions"
require "veritas"
/home/lars/.rvm/gems/ruby-1.9.3-p327@datamapper/bundler/gems/veritas-7e587d210e36/lib/veritas/attribute/date_time.rb:10:in `<class:DateTime>': bad value for range (ArgumentError)
    from /home/lars/.rvm/gems/ruby-1.9.3-p327@datamapper/bundler/gems/veritas-7e587d210e36/lib/veritas/attribute/date_time.rb:7:in `<class:Attribute>'
    from /home/lars/.rvm/gems/ruby-1.9.3-p327@datamapper/bundler/gems/veritas-7e587d210e36/lib/veritas/attribute/date_time.rb:4:in `<module:Veritas>'
    from /home/lars/.rvm/gems/ruby-1.9.3-p327@datamapper/bundler/gems/veritas-7e587d210e36/lib/veritas/attribute/date_time.rb:3:in `<top (required)>'
    from /home/lars/.rvm/gems/ruby-1.9.3-p327@datamapper/gems/backports-2.6.5/lib/backports/tools.rb:314:in `require'
    from /home/lars/.rvm/gems/ruby-1.9.3-p327@datamapper/gems/backports-2.6.5/lib/backports/tools.rb:314:in `require_with_backports'
    from /home/lars/.rvm/gems/ruby-1.9.3-p327@datamapper/bundler/gems/veritas-7e587d210e36/lib/veritas.rb:108:in `<top (required)>'
    from test.rb:4:in `require'
dkubb commented 11 years ago

@lgierth here's my first attempt to reproduce the problem:

04:09:19 veritas (master) $ git rev-parse HEAD
7e587d210e3600179b874742ee98d760af44a000
04:09:21 veritas (master) $ irb -Ilib -rrubygems
1.8.7 :001 > require 'active_support/version'
 => true 
1.8.7 :002 > ActiveSupport::VERSION::STRING
 => "3.2.8" 
1.8.7 :003 > require 'active_support/core_ext/date_time/conversions'
 => true 
1.8.7 :004 > require 'veritas'
 => true

I'll try again with 1.9.3, maybe it's something specific to that version.

dkubb commented 11 years ago

@lgierth ok, it does appear to be something 1.9.3 specific:

04:11:49 veritas (master) $ git rev-parse HEAD
7e587d210e3600179b874742ee98d760af44a000
04:11:53 veritas (master) $ irb -Ilib -rrubygems
1.9.3p286 :001 > require 'active_support/version'
 => true 
1.9.3p286 :002 > ActiveSupport::VERSION::STRING
 => "3.2.8" 
1.9.3p286 :003 > require 'active_support/core_ext/date_time/conversions'
 => true 
1.9.3p286 :004 > require 'veritas'
ArgumentError: bad value for range
    from /Users/dkubb/Dropbox/Home/Programming/open-source/veritas/lib/veritas/attribute/date_time.rb:10:in `<class:DateTime>'
    from /Users/dkubb/Dropbox/Home/Programming/open-source/veritas/lib/veritas/attribute/date_time.rb:7:in `<class:Attribute>'
    from /Users/dkubb/Dropbox/Home/Programming/open-source/veritas/lib/veritas/attribute/date_time.rb:4:in `<module:Veritas>'
    from /Users/dkubb/Dropbox/Home/Programming/open-source/veritas/lib/veritas/attribute/date_time.rb:3:in `<top (required)>'
    from /Users/dkubb/.rvm/rubies/ruby-1.9.3-p286/lib/ruby/site_ruby/1.9.1/rubygems/custom_require.rb:36:in `require'
    from /Users/dkubb/.rvm/rubies/ruby-1.9.3-p286/lib/ruby/site_ruby/1.9.1/rubygems/custom_require.rb:36:in `require'
    from /Users/dkubb/.rvm/gems/ruby-1.9.3-p286/gems/backports-2.6.4/lib/backports/tools.rb:314:in `require_with_backports'
    from /Users/dkubb/Dropbox/Home/Programming/open-source/veritas/lib/veritas.rb:108:in `<top (required)>'
    from /Users/dkubb/.rvm/rubies/ruby-1.9.3-p286/lib/ruby/site_ruby/1.9.1/rubygems/custom_require.rb:36:in `require'
    from /Users/dkubb/.rvm/rubies/ruby-1.9.3-p286/lib/ruby/site_ruby/1.9.1/rubygems/custom_require.rb:36:in `require'
    from (irb):4
    from /Users/dkubb/.rvm/rubies/ruby-1.9.3-p286/bin/irb:16:in `<main>'

I'll look into this more and see why there's a behavioral difference between both versions. My gut feeling is that it's a bug in AS with how it handles DateTime::Infinite instances, and veritas is just triggering the bug, but I'll need to isolate it a bit better before knowing for sure.

dkubb commented 11 years ago

@lgierth check this out:

04:16:15 veritas (master) $ irb -Ilib -rrubygems -rdate
1.9.3p286 :001 > ::DateTime.new..::DateTime::Infinity.new
 => #<DateTime: -4712-01-01T00:00:00+00:00 ((0j,0s,0n),+0s,2299161j)>..#<Date::Infinity:0x007f9e12988700 @d=1> 
1.9.3p286 :002 > require 'active_support/core_ext/date_time/conversions'
 => true 
1.9.3p286 :003 > ::DateTime.new..::DateTime::Infinity.new
ArgumentError: bad value for range
    from (irb):3
    from /Users/dkubb/.rvm/rubies/ruby-1.9.3-p286/bin/irb:16:in `<main>'

So it seems like the Range is valid only until ActiveSupport DateTime conversions are loaded and it changes the behavior somehow due to a monkey patch.

Do you mind reporting this to the rails bug tracker? It's probably not necessary to mention veritas though, because it's not really anything to do with this lib specifically and I wouldn't want to confuse matters. The core issue is that it's breaking behavior of the stdlib which veritas is relying on.

I'll continue to dig a bit and see if I can identify where the problem is. Maybe there's a fix I can come up with and submit it to the issue once you open it.

ghost commented 11 years ago

Infinity..Infinity works:

1.9.3p327 :014 > ::DateTime::Infinity.new..::DateTime::Infinity.new
 => #<Date::Infinity:0x8adb4e0 @d=1>..#<Date::Infinity:0x8adb4cc @d=1>
ghost commented 11 years ago

Upstream ticket: rails/rails#8178

dkubb commented 11 years ago

@lgierth In lib/active_support/core_ext/date_time/calculations.rb can you try updating the <=> method to look like this:

def <=>(other)
  return -1 if other.kind_of?(Infinity)
  super other.to_datetime
end

This seems to solve the problem for me locally, but I wanted to double check with someone else to make sure.

dkubb commented 11 years ago

Another solution might be something like:

def <=>(other)                                                                 
  super other.kind_of?(Infinity) ? other : other.to_datetime                   
end

Actually, upon reflection I think this is a bit nicer since it delegates more work to the original method.

I suppose a third option would be to define a DateTime::Infinity#to_datetime method that just returns self.

dkubb commented 11 years ago

@lgierth it looks like this ticket hasn't be addressed on the rails side. Are you up for creating a PR that fixes it? I can maybe ask someone on rails-core to look at/merge it sooner rather than later.

dkubb commented 11 years ago

I left a note on the rails issue tracker that I would be willing to submit a patch if I had some direction on which fix they would approve. Hopefully I get a reply. If not I may contact a few people on rails-core directly so I can get the direction I need.

dkubb commented 11 years ago

@lgierth can you try with rails 3.2.12 now and see if this is still an issue? My patch was merged into the 3-2-stable branch.

dkubb commented 11 years ago

Actually, I've just confirmed this is fixed for me, so I'm going to close this for now:

12:03:45 axiom (master) $ irb -Ilib -rrubygems -rdate
2.0.0p0 :001 > ::DateTime.new..::DateTime::Infinity.new
 => #<DateTime: -4712-01-01T00:00:00+00:00 ((0j,0s,0n),+0s,2299161j)>..#<Date::Infinity:0x007ffba30e05a0 @d=1>
2.0.0p0 :002 > require 'active_support/core_ext/date_time/conversions'
 => true
2.0.0p0 :003 > ::DateTime.new..::DateTime::Infinity.new
 => Mon, 01 Jan -4712 00:00:00 +0000..#<Date::Infinity:0x007ffba1863680 @d=1>

@lgierth if you notice any problem, feel free to comment here and I'll reopen the ticket but for now I'm assuming it's fixed all around.