Pull request #806 breaks ability condition building

[koukou73gr]

I just pulled master to get #806 which hasn't made it into any relase yet, however it appears that CanCan now fails even when doing something like this:

can  :read, Comment, :article => { :condition => true , :condition2 => true }

ie, specifiing 2 conditions, even for a single join. The above works correctly for the released 1.6.9 version. If I remove the condition2, it works. Same goes for a 2-level join, like the one in the original issue description:

can  :read, Comment, :article => { :category => { :condition => true, :condition2 => true } }

Or is this just me? Rails 3.2.12 + Ruby 1.9.3p327

[jaredbeck]

When you say "cancan fails", do you mean that a test is failing? Are you saying that the issue described in #806 still exists? If the tests are all passing, can you describe the error, or better yet write a failing test? Thanks!

[koukou73gr]

Thanks for looking into this.

I mean it errors inside CanCan.

With a schema like:

User has_many Alerts, Alert belongs_to InstallationSite, InstallationSite belongs_to CityArea

and an ability definition such as:

    can :read, Alert , :installation_site => {
      :city_area => {
        :country => target.country, :region_name => target.region
      }
    }

I get:

1.9.3p327 :001 > a= User.find(2)
  User Load (0.4ms)  SELECT `users`.* FROM `users` WHERE `users`.`id` = 2 LIMIT 1
 => #<User id: 2, name: "Kosta .... <output snipped>
1.9.3p327 :002 > b= Ability.new(a)
  ApplianceType Load (0.4ms)  SELECT `appliance_types`.* FROM `appliance_types` ORDER BY display_order
  ApplianceType Load (0.3ms)  SELECT `appliance_types`.* FROM `appliance_types` ORDER BY display_order
  Permission Load (0.2ms)  SELECT `permissions`.* FROM `permissions` WHERE `permissions`.`user_id` = 2
  Role Load (0.2ms)  SELECT `roles`.* FROM `roles` WHERE `roles`.`id` = 3 LIMIT 1
 => #<Ability:0x00000003d4f348 @rules=[#<CanCan::Rule:0x00000004abaff0   ... <output snipped>
1.9.3p327 :003 > Alert.accessible_by(b)
NoMethodError: undefined method `table_name' for nil:NilClass
    from /home/kostas/.rvm/gems/ruby-1.9.3-p327@energy_analytics/bundler/gems/cancan-ff2b632f1524/lib/cancan/model_adapters/active_record_adapter.rb:76:in `block (2 levels) in tableized_conditions'
    from /home/kostas/.rvm/gems/ruby-1.9.3-p327@energy_analytics/bundler/gems/cancan-ff2b632f1524/lib/cancan/model_adapters/active_record_adapter.rb:71:in `each'
    from /home/kostas/.rvm/gems/ruby-1.9.3-p327@energy_analytics/bundler/gems/cancan-ff2b632f1524/lib/cancan/model_adapters/active_record_adapter.rb:71:in `inject'
    from /home/kostas/.rvm/gems/ruby-1.9.3-p327@energy_analytics/bundler/gems/cancan-ff2b632f1524/lib/cancan/model_adapters/active_record_adapter.rb:71:in `block in tableized_conditions'
    from /home/kostas/.rvm/gems/ruby-1.9.3-p327@energy_analytics/bundler/gems/cancan-ff2b632f1524/lib/cancan/model_adapters/active_record_adapter.rb:67:in `each'
    from /home/kostas/.rvm/gems/ruby-1.9.3-p327@energy_analytics/bundler/gems/cancan-ff2b632f1524/lib/cancan/model_adapters/active_record_adapter.rb:67:in `inject'
    from /home/kostas/.rvm/gems/ruby-1.9.3-p327@energy_analytics/bundler/gems/cancan-ff2b632f1524/lib/cancan/model_adapters/active_record_adapter.rb:67:in `tableized_conditions'
    from /home/kostas/.rvm/gems/ruby-1.9.3-p327@energy_analytics/bundler/gems/cancan-ff2b632f1524/lib/cancan/model_adapters/active_record_adapter.rb:81:in `block in tableized_conditions'
    from /home/kostas/.rvm/gems/ruby-1.9.3-p327@energy_analytics/bundler/gems/cancan-ff2b632f1524/lib/cancan/model_adapters/active_record_adapter.rb:67:in `each'
    from /home/kostas/.rvm/gems/ruby-1.9.3-p327@energy_analytics/bundler/gems/cancan-ff2b632f1524/lib/cancan/model_adapters/active_record_adapter.rb:67:in `inject'
    from /home/kostas/.rvm/gems/ruby-1.9.3-p327@energy_analytics/bundler/gems/cancan-ff2b632f1524/lib/cancan/model_adapters/active_record_adapter.rb:67:in `tableized_conditions'
    from /home/kostas/.rvm/gems/ruby-1.9.3-p327@energy_analytics/bundler/gems/cancan-ff2b632f1524/lib/cancan/model_adapters/active_record_adapter.rb:60:in `block in conditions'
    from /home/kostas/.rvm/gems/ruby-1.9.3-p327@energy_analytics/bundler/gems/cancan-ff2b632f1524/lib/cancan/model_adapters/active_record_adapter.rb:59:in `each'
    from /home/kostas/.rvm/gems/ruby-1.9.3-p327@energy_analytics/bundler/gems/cancan-ff2b632f1524/lib/cancan/model_adapters/active_record_adapter.rb:59:in `inject'
    from /home/kostas/.rvm/gems/ruby-1.9.3-p327@energy_analytics/bundler/gems/cancan-ff2b632f1524/lib/cancan/model_adapters/active_record_adapter.rb:59:in `conditions'
    from /home/kostas/.rvm/gems/ruby-1.9.3-p327@energy_analytics/bundler/gems/cancan-ff2b632f1524/lib/cancan/model_adapters/active_record_adapter.rb:105:in `database_records'
    from /home/kostas/.rvm/gems/ruby-1.9.3-p327@energy_analytics/bundler/gems/cancan-ff2b632f1524/lib/cancan/model_additions.rb:23:in `accessible_by'
    from (irb):3
    from /home/kostas/.rvm/gems/ruby-1.9.3-p327@energy_analytics/gems/railties-3.2.12/lib/rails/commands/console.rb:47:in `start'
    from /home/kostas/.rvm/gems/ruby-1.9.3-p327@energy_analytics/gems/railties-3.2.12/lib/rails/commands/console.rb:8:in `start'
    from /home/kostas/.rvm/gems/ruby-1.9.3-p327@energy_analytics/gems/railties-3.2.12/lib/rails/commands.rb:41:in `<top (required)>'
    from script/rails:6:in `require'
    from script/rails:6:in `<main>'

If only one condition is specified, say:

    can :read, Alert , :installation_site => {
      :city_area => {
        :country => target.country
      }
    }

It works as expected.
It errors out the same way even if there is only a simple join to process, ie if the ability could be expressed like this:

    can :read, InstallationSite,
      :city_area => {
        :country => target.country, :region => target.region
      }

it stills errors at the same point.

Master was at @ff2b632f1524 when I submited this. I also tried ther recent, 1.6.10 release, the result is the same. With the 1.6.9 release there is no error generated, just the messed SQL string when doing two joins (first case above) that #806 was trying to solve.

I am arguing that #806 is to blame because it was the only change since 1.6.9 release that touched active_record_adapter.rb (although I could be wrong, my GitHub skills are not that great :( )

[jaredbeck]

I've written a failing test which produces the NoMethodError you describe.

jaredbeck@45bc237

It's just a starting point though. Can you help me refine the test so that it asserts the correct behavior? Thanks.

[koukou73gr]

Your test looks right to me. To play safe, I'd also add one for the single join as well. Something like

it "should support more than one nested conditions" do 
   @ability.can :read, Article, :category => { 
     :name => 'foo', :visible => true 
   } 
   expect { Article.accessible_by(@ability) }.to_not raise_error 
end

[koukou73gr]

As for asserting the correct behaviour, I'd just copy-paste the existing deeply-nested test from @yuszuv :
(warning, completely untested ...)

it "should return appropriate sql conditions in complex case with nested joins and more than one nested conditions" do 
  @ability.can :read, Comment, :article => { :category => { :visible => true , :name => 'foo'} } 
  @ability.model_adapter(Comment, :read).conditions.should == { Category.table_name.to_sym => { :visible => true , :name => 'foo' } } 
end 

it "should return appropriate sql conditions in complex case with nested joins of different depth  and more than one nested conditions" do 
  @ability.can :read, Comment, :article => { :published => true, :category => { :visible => true, :name => 'foo' } } 
  @ability.model_adapter(Comment, :read).conditions.should == { Article.table_name.to_sym => { :published => true }, Category.table_name.to_sym => { :visible => true, :name => 'foo' } } 
end 

[ricec]

I added pull request #871 that fixes the issue and incorporates @jaredbeck's test condition from 45bc237

[koukou73gr]

Hey, that was a good catch !!! :)

I confirm that #871 fixes my problem as reported. Please consider applying.

[brendon]

Just came across this issue too. Would it be possible to have this merged and a new gem version released?

[ZachBeta]

Also ran into this. Pretty painful.