fixing active record adapter behavior and improving specs for it

Gemfile

@@ -2,7 +2,9 @@ source "http://rubygems.org"
 
 case ENV["MODEL_ADAPTER"]
 when nil, "active_record"
+  gem "sqlite3-ruby", :require => "sqlite3"
   gem "activerecord", :require => "active_record"
+  gem "with_model"
 when "data_mapper"
   gem "dm-core", "~> 1.0.2"
 when "mongoid"

lib/cancan/model_adapters/active_record_adapter.rb

@@ -19,14 +19,26 @@ module CanCan
       def conditions
         if @rules.size == 1 && @rules.first.base_behavior
           # Return the conditions directly if there's just one definition
-          @rules.first.tableized_conditions.dup
+          tableized_conditions(@rules.first.conditions).dup
         else
           @rules.reverse.inject(false_sql) do |sql, rule|
-            merge_conditions(sql, rule.tableized_conditions.dup, rule.base_behavior)
+            merge_conditions(sql, tableized_conditions(rule.conditions).dup, rule.base_behavior)
           end
         end
       end
 
+      def tableized_conditions(conditions)
+        return conditions unless conditions.kind_of? Hash
+        conditions.inject({}) do |result_hash, (name, value)|
+          if value.kind_of? Hash
+            name = @model_class.reflect_on_association(name).table_name
+            value = tableized_conditions(value)
+          end
+          result_hash[name] = value
+          result_hash
+        end
+      end
+
       # Returns the associations used in conditions for the :joins option of a search.
       # See ActiveRecordAdditions#accessible_by for use in Active Record.
       def joins

lib/cancan/rule.rb

@@ -3,7 +3,7 @@ module CanCan
   # it holds the information about a "can" call made on Ability and provides
   # helpful methods to determine permission checking and conditions hash generation.
   class Rule # :nodoc:
-    attr_reader :base_behavior, :actions
+    attr_reader :base_behavior, :actions, :conditions
     attr_writer :expanded_actions
 
     # The first argument when initializing is the base_behavior which is a true/false
@@ -41,18 +41,6 @@ module CanCan
       end
     end
 
-    def tableized_conditions(conditions = @conditions)
-      return conditions unless conditions.kind_of? Hash
-      conditions.inject({}) do |result_hash, (name, value)|
-        if value.kind_of? Hash
-          name = name.to_s.tableize.to_sym
-          value = tableized_conditions(value)
-        end
-        result_hash[name] = value
-        result_hash
-      end
-    end
-
     def only_block?
       conditions_empty? && !@block.nil?
     end

spec/cancan/model_adapters/active_record_adapter_spec.rb

@@ -1,77 +1,175 @@
 if ENV["MODEL_ADAPTER"].nil? || ENV["MODEL_ADAPTER"] == "active_record"
   require "spec_helper"
 
+  ActiveRecord::Base.establish_connection(:adapter => "sqlite3", :database => ":memory:")
+
   describe CanCan::ModelAdapters::ActiveRecordAdapter do
+    with_model :article do
+      table do |t|
+        t.boolean "published"
+        t.boolean "secret"
+      end
+      model do
+        has_many :comments
+      end
+    end
+
+    with_model :comment do
+      table do |t|
+        t.boolean "spam"
+        t.integer "article_id"
+      end
+      model do
+        belongs_to :article
+      end
+    end
+
     before(:each) do
-      @model_class = Class.new(Project)
+      Article.delete_all
-      stub(@model_class).scoped { :scoped_stub }
+      Comment.delete_all
-      @model_class.send(:include, CanCan::ActiveRecordAdditions)
       @ability = Object.new
       @ability.extend(CanCan::Ability)
+      @article_table = Article.table_name
+      @comment_table = Comment.table_name
     end
 
-    it "should call where('true=false') when no ability is defined so no records are found" do
+    it "should not fetch any records when no abilities are defined" do
-      stub(@model_class).joins { true } # just so it responds to .joins as well
+      Article.create!
-      stub(@model_class).where('true=false').stub!.joins(nil) { :no_match }
+      Article.accessible_by(@ability).should be_empty
-      @model_class.accessible_by(@ability, :read).should == :no_match
     end
 
-    it "should call where with matching ability conditions" do
+    it "should fetch all articles when one can read all" do
-      @ability.can :read, @model_class, :foo => {:bar => 1}
+      @ability.can :read, Article
-      stub(@model_class).joins { true } # just so it responds to .joins as well
+      article = Article.create!
-      stub(@model_class).where(:foos => { :bar => 1 }).stub!.joins([:foo]) { :found_records }
+      Article.accessible_by(@ability).should == [article]
-      @model_class.accessible_by(@ability, :read).should == :found_records
     end
 
-    it "should default to :read ability and use scoped when where isn't available" do
+    it "should fetch only the articles that are published" do
-      @ability.can :read, @model_class, :foo => {:bar => 1}
+      @ability.can :read, Article, :published => true
-      stub(@model_class).scoped(:conditions => {:foos => {:bar => 1}}, :joins => [:foo]) { :found_records }
+      article1 = Article.create!(:published => true)
-      @model_class.accessible_by(@ability).should == :found_records
+      article2 = Article.create!(:published => false)
+      Article.accessible_by(@ability).should == [article1]
     end
 
-    it "should merge association joins and sanitize conditions" do
+    it "should fetch any articles which are published or secret" do
-      @ability.can :read, @model_class, :foo => {:bar => 1}
+      @ability.can :read, Article, :published => true
-      @ability.can :read, @model_class, :too => {:car => 1, :far => {:bar => 1}}
+      @ability.can :read, Article, :secret => true
-
+      article1 = Article.create!(:published => true, :secret => false)
-      condition_variants = [
+      article2 = Article.create!(:published => true, :secret => true)
-          '(toos.fars.bar=1 AND toos.car=1) OR (foos.bar=1)', # faked sql sanitizer is stupid ;-)
+      article3 = Article.create!(:published => false, :secret => true)
-          '(toos.car=1 AND toos.fars.bar=1) OR (foos.bar=1)'
+      article4 = Article.create!(:published => false, :secret => false)
-      ]
+      Article.accessible_by(@ability).should == [article1, article2, article3]
-      joins_variants = [
-          [:foo, {:too => [:far]}],
-          [{:too => [:far]}, :foo]
-      ]
-
-      condition_variants.each do |condition|
-        joins_variants.each do |joins|
-          stub(@model_class).scoped( :conditions => condition, :joins => joins ) { :found_records }
-        end
-      end
-      # @ability.conditions(:read, @model_class).should == '(too.car=1 AND too.far.bar=1) OR (foo.bar=1)'
-      # @ability.associations_hash(:read, @model_class).should == [{:too => [:far]}, :foo]
-      @model_class.accessible_by(@ability).should == :found_records
     end
 
-    it "should allow to define sql conditions by not hash" do
+    it "should fetch only the articles that are published and not secret" do
-      @ability.can :read, @model_class, :foo => 1
+      @ability.can :read, Article, :published => true
-      @ability.can :read, @model_class, ['bar = ?', 1]
+      @ability.cannot :read, Article, :secret => true
-      stub(@model_class).scoped( :conditions => '(bar = 1) OR (foo=1)', :joins => nil ) { :found_records }
+      article1 = Article.create!(:published => true, :secret => false)
-      stub(@model_class).scoped{|*args| args.inspect}
+      article2 = Article.create!(:published => true, :secret => true)
-      @model_class.accessible_by(@ability).should == :found_records
+      article3 = Article.create!(:published => false, :secret => true)
+      article4 = Article.create!(:published => false, :secret => false)
+      Article.accessible_by(@ability).should == [article1]
+    end
+
+    it "should only read comments for articles which are published" do
+      @ability.can :read, Comment, :article => { :published => true }
+      comment1 = Comment.create!(:article => Article.create!(:published => true))
+      comment2 = Comment.create!(:article => Article.create!(:published => false))
+      Comment.accessible_by(@ability).should == [comment1]
+    end
+
+    it "should allow conditions in SQL and merge with hash conditions" do
+      @ability.can :read, Article, :published => true
+      @ability.can :read, Article, ["secret=?", true]
+      article1 = Article.create!(:published => true, :secret => false)
+      article4 = Article.create!(:published => false, :secret => false)
+      Article.accessible_by(@ability).should == [article1]
     end
 
     it "should not allow to fetch records when ability with just block present" do
-      @ability.can :read, @model_class do false end
+      @ability.can :read, Article do
-      lambda {
+        false
-        @model_class.accessible_by(@ability)
+      end
-      }.should raise_error(CanCan::Error)
+      lambda { Article.accessible_by(@ability) }.should raise_error(CanCan::Error)
     end
 
-    it "should not allow to check ability on object when nonhash sql ability definition without block present" do
+    it "should not allow to check ability on object against SQL conditions without block" do
-      @ability.can :read, @model_class, ['bar = ?', 1]
+      @ability.can :read, Article, ["secret=?", true]
-      lambda {
+      lambda { @ability.can? :read, Article.new }.should raise_error(CanCan::Error)
-        @ability.can? :read, @model_class.new
+    end
-      }.should raise_error(CanCan::Error)
+
+    it "should have false conditions if no abilities match" do
+      @ability.model_adapter(Article, :read).conditions.should == "'t'='f'"
+    end
+
+    it "should return false conditions for cannot clause" do
+      @ability.cannot :read, Article
+      @ability.model_adapter(Article, :read).conditions.should == "'t'='f'"
+    end
+
+    it "should return SQL for single `can` definition in front of default `cannot` condition" do
+      @ability.cannot :read, Article
+      @ability.can :read, Article, :published => false, :secret => true
+      @ability.model_adapter(Article, :read).conditions.should orderlessly_match(%Q["#{@article_table}"."published" = 'f' AND "#{@article_table}"."secret" = 't'])
+    end
+
+    it "should return true condition for single `can` definition in front of default `can` condition" do
+      @ability.can :read, Article
+      @ability.can :read, Article, :published => false, :secret => true
+      @ability.model_adapter(Article, :read).conditions.should  == "'t'='t'"
+    end
+
+    it "should return `false condition` for single `cannot` definition in front of default `cannot` condition" do
+      @ability.cannot :read, Article
+      @ability.cannot :read, Article, :published => false, :secret => true
+      @ability.model_adapter(Article, :read).conditions.should  == "'t'='f'"
+    end
+
+    it "should return `not (sql)` for single `cannot` definition in front of default `can` condition" do
+      @ability.can :read, Article
+      @ability.cannot :read, Article, :published => false, :secret => true
+      @ability.model_adapter(Article, :read).conditions.should orderlessly_match(%Q["not (#{@article_table}"."published" = 'f' AND "#{@article_table}"."secret" = 't')])
+    end
+
+    it "should return appropriate sql conditions in complex case" do
+      @ability.can :read, Article
+      @ability.can :manage, Article, :id => 1
+      @ability.can :update, Article, :published => true
+      @ability.cannot :update, Article, :secret => true
+      @ability.model_adapter(Article, :update).conditions.should == %Q[not ("#{@article_table}"."secret" = 't') AND (("#{@article_table}"."published" = 't') OR ("#{@article_table}"."id" = 1))]
+      @ability.model_adapter(Article, :manage).conditions.should == {:id => 1}
+      @ability.model_adapter(Article, :read).conditions.should == "'t'='t'"
+    end
+
+    it "should not forget conditions when calling with SQL string" do
+      @ability.can :read, Article, :published => true
+      @ability.can :read, Article, ['secret=?', false]
+      adapter = @ability.model_adapter(Article, :read)
+      2.times do
+        adapter.conditions.should == %Q[(secret='f') OR ("#{@article_table}"."published" = 't')]
+      end
+    end
+
+    it "should have nil joins if no rules" do
+      @ability.model_adapter(Article, :read).joins.should be_nil
+    end
+
+    it "should have nil joins if no nested hashes specified in conditions" do
+      @ability.can :read, Article, :published => false
+      @ability.can :read, Article, :secret => true
+      @ability.model_adapter(Article, :read).joins.should be_nil
+    end
+
+    it "should merge separate joins into a single array" do
+      @ability.can :read, Article, :project => { :blocked => false }
+      @ability.can :read, Article, :company => { :admin => true }
+      @ability.model_adapter(Article, :read).joins.inspect.should orderlessly_match([:company, :project].inspect)
+    end
+
+    it "should merge same joins into a single array" do
+      @ability.can :read, Article, :project => { :blocked => false }
+      @ability.can :read, Article, :project => { :admin => true }
+      @ability.model_adapter(Article, :read).joins.should == [:project]
     end
   end
 end

spec/cancan/query_spec_pending.rb

@@ -1,116 +0,0 @@
-require "spec_helper"
-
-describe CanCan::Query do
-  before(:each) do
-    @ability = Object.new
-    @ability.extend(CanCan::Ability)
-  end
-
-  it "should have false conditions if no abilities match" do
-    @ability.query(:destroy, Project).conditions.should == "true=false"
-  end
-
-  it "should return hash for single `can` definition" do
-    @ability.can :read, Project, :blocked => false, :user_id => 1
-    @ability.query(:read, Project).conditions.should == { :blocked => false, :user_id => 1 }
-  end
-
-  it "should merge multiple rules into single SQL string joining with OR" do
-    @ability.can :read, Project, :blocked => false
-    @ability.can :read, Project, :admin => true
-    @ability.query(:read, Project).conditions.should == "(admin=true) OR (blocked=false)"
-  end
-
-  it "should merge multiple rules into single SQL string joining with OR and AND" do
-    @ability.can :read, Project, :blocked => false, :active => true
-    @ability.can :read, Project, :admin => true
-    @ability.query(:read, Project).conditions.should orderlessly_match("(blocked=false AND active=true) OR (admin=true)")
-  end
-
-  it "should merge multiple rules into single SQL string joining with OR and AND" do
-    @ability.can :read, Project, :blocked => false, :active => true
-    @ability.can :read, Project, :admin => true
-    @ability.query(:read, Project).conditions.should orderlessly_match("(blocked=false AND active=true) OR (admin=true)")
-  end
-
-  it "should return false conditions for cannot clause" do
-    @ability.cannot :read, Project
-    @ability.query(:read, Project).conditions.should == "true=false"
-  end
-
-  it "should return SQL for single `can` definition in front of default `cannot` condition" do
-    @ability.cannot :read, Project
-    @ability.can :read, Project, :blocked => false, :user_id => 1
-    @ability.query(:read, Project).conditions.should orderlessly_match("blocked=false AND user_id=1")
-  end
-
-  it "should return true condition for single `can` definition in front of default `can` condition" do
-    @ability.can :read, Project
-    @ability.can :read, Project, :blocked => false, :user_id => 1
-    @ability.query(:read, Project).conditions.should == 'true=true'
-  end
-
-  it "should return false condition for single `cannot` definition" do
-    @ability.cannot :read, Project, :blocked => true, :user_id => 1
-    @ability.query(:read, Project).conditions.should == 'true=false'
-  end
-
-  it "should return `false condition` for single `cannot` definition in front of default `cannot` condition" do
-    @ability.cannot :read, Project
-    @ability.cannot :read, Project, :blocked => true, :user_id => 1
-    @ability.query(:read, Project).conditions.should == 'true=false'
-  end
-
-  it "should return `not (sql)` for single `cannot` definition in front of default `can` condition" do
-    @ability.can :read, Project
-    @ability.cannot :read, Project, :blocked => true, :user_id => 1
-    @ability.query(:read, Project).conditions.should orderlessly_match("not (blocked=true AND user_id=1)")
-  end
-
-  it "should return appropriate sql conditions in complex case" do
-    @ability.can :read, Project
-    @ability.can :manage, Project, :id => 1
-    @ability.can :update, Project, :manager_id => 1
-    @ability.cannot :update, Project, :self_managed => true
-    @ability.query(:update, Project).conditions.should == 'not (self_managed=true) AND ((manager_id=1) OR (id=1))'
-    @ability.query(:manage, Project).conditions.should == {:id=>1}
-    @ability.query(:read, Project).conditions.should == 'true=true'
-  end
-
-  it "should have nil joins if no rules" do
-    @ability.query(:read, Project).joins.should be_nil
-  end
-
-  it "should have nil joins if no nested hashes specified in conditions" do
-    @ability.can :read, Project, :blocked => false
-    @ability.can :read, Project, :admin => true
-    @ability.query(:read, Project).joins.should be_nil
-  end
-
-  it "should merge separate joins into a single array" do
-    @ability.can :read, Project, :project => { :blocked => false }
-    @ability.can :read, Project, :company => { :admin => true }
-    @ability.query(:read, Project).joins.inspect.should orderlessly_match([:company, :project].inspect)
-  end
-
-  it "should merge same joins into a single array" do
-    @ability.can :read, Project, :project => { :blocked => false }
-    @ability.can :read, Project, :project => { :admin => true }
-    @ability.query(:read, Project).joins.should == [:project]
-  end
-
-  it "should merge complex, nested joins" do
-    @ability.can :read, Project, :project => { :bar => {:test => true} }, :company => { :bar => {:test => true} }
-    @ability.can :read, Project, :project => { :foo => {:bar => true}, :bar => {:zip => :zap} }
-    @ability.query(:read, Project).joins.inspect.should orderlessly_match([{:project => [:bar, :foo]}, {:company => [:bar]}].inspect)
-  end
-
-  it "should not forget conditions when calling with SQL string" do
-    @ability.can :read, Project, :foo => 1
-    @ability.can :read, Project, ['bar = ?', 1]
-    query = @ability.query(:read, Project)
-    2.times do
-      query.conditions.should == "(bar = 1) OR (foo=1)"
-    end
-  end
-end

spec/cancan/rule_spec.rb

@@ -32,24 +32,6 @@ describe CanCan::Rule do
     @rule.associations_hash.should == {:foo => {:bar => {}}}
   end
 
-  it "should tableize correctly for absurdly complex permissions" do
-    @conditions[:unit] = {:property=>{:landlord=>{:weasle_id=>560}}}
-    @conditions[:test] = 1
-    @rule.tableized_conditions.should == {:units => {:properties => {:landlords=>{:weasle_id=>560}}}, :test => 1}
-  end
-
-  it "should tableize correctly for complex permissions" do
-    @conditions[:unit] = {:property=>{:landlord_id=>560}}
-    @conditions[:test] = 1
-    @rule.tableized_conditions.should == {:units => {:properties => {:landlord_id=>560}}, :test => 1}
-  end
-
-  it "should return table names in conditions for association joins" do
-    @conditions[:foo] = {:bar => 1}
-    @conditions[:test] = 1
-    @rule.tableized_conditions.should == {:foos => {:bar => 1}, :test => 1}
-  end
-
   it "should return no association joins if conditions is nil" do
     rule = CanCan::Rule.new(true, :read, Integer, nil, nil)
     rule.associations_hash.should == {}

spec/spec_helper.rb

@@ -14,6 +14,7 @@ RSpec.configure do |config|
     Project.delete_all
     Category.delete_all
   end
+  config.extend WithModel if defined? WithModel
 end
 
 class Ability