consider specificity when finding relevant rules so generic rules will not override specific ones - closes #321

lib/cancan/ability.rb

@@ -299,10 +299,14 @@ module CanCan
     # Returns an array of Rule instances which match the action and subject
     # This does not take into consideration any hash conditions or block statements
     def relevant_rules(action, subject, attribute = nil)
-      rules.reverse.select do |rule|
+      specificity = 0
+      rules.reverse.each_with_object([]) do |rule, relevant_rules|
         rule.expanded_actions = expand_aliases(:actions, rule.actions)
         rule.expanded_subjects = expand_aliases(:subjects, rule.subjects)
-        rule.relevant? action, subject, attribute
+        if rule.relevant?(action, subject, attribute) && rule.specificity >= specificity
+          specificity = rule.specificity if rule.base_behavior
+          relevant_rules << rule
+        end
       end
     end
 

lib/cancan/rule.rb

@@ -44,23 +44,23 @@ module CanCan
     end
 
     def only_block?
-      conditions_empty? && !@block.nil?
+      !conditions? && !@block.nil?
     end
 
     def only_raw_sql?
-      @block.nil? && !conditions_empty? && !@conditions.kind_of?(Hash)
+      @block.nil? && conditions? && !@conditions.kind_of?(Hash)
     end
 
     def attributes?
       @attributes.present?
     end
 
-    def instance_conditions?
+    def conditions?
-      @block || !conditions_empty?
+      @conditions.present?
     end
 
-    def conditions_empty?
+    def instance_conditions?
-      @conditions == {} || @conditions.nil?
+      @block || conditions?
     end
 
     def associations_hash(conditions = @conditions)
@@ -79,6 +79,13 @@ module CanCan
       attributes
     end
 
+    def specificity
+      specificity = 1
+      specificity += 1 if attributes? || conditions?
+      specificity += 2 unless base_behavior
+      specificity
+    end
+
     private
 
     def subject_object?(subject)

spec/cancan/ability_spec.rb

@@ -138,6 +138,13 @@ describe CanCan::Ability do
     @ability.can?(:read, 4..6).should be_false
   end
 
+  it "takes presedence over rule defined without a condition" do
+    @ability.can :read, :ranges
+    @ability.can :read, :ranges, :begin => 1
+    @ability.can?(:read, 1..5).should be_true
+    @ability.can?(:read, 4..6).should be_false
+  end
+
 
   # Block Conditions
 

spec/cancan/model_adapters/active_record_adapter_spec.rb

@@ -176,7 +176,7 @@ if ENV["MODEL_ADAPTER"].nil? || ENV["MODEL_ADAPTER"] == "active_record"
     it "should return true condition for single `can` definition in front of default `can` condition" do
       @ability.can :read, :articles
       @ability.can :read, :articles, :published => false, :secret => true
-      @ability.model_adapter(Article, :read).conditions.should  == "'t'='t'"
+      @ability.model_adapter(Article, :read).conditions.should eq(:secret => true, :published => false)
     end
 
     it "should return `false condition` for single `cannot` definition in front of default `cannot` condition" do
@@ -198,7 +198,7 @@ if ENV["MODEL_ADAPTER"].nil? || ENV["MODEL_ADAPTER"] == "active_record"
       @ability.cannot :update, :articles, :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, :access).conditions.should == {:id => 1}
-      @ability.model_adapter(Article, :read).conditions.should == "'t'='t'"
+      @ability.model_adapter(Article, :read).conditions.should == {:id => 1} # used to be "t=t" but changed with new specificity rule (issue #321)
     end
 
     it "should not forget conditions when calling with SQL string" do

spec/cancan/rule_spec.rb

@@ -36,4 +36,13 @@ describe CanCan::Rule do
     rule = CanCan::Rule.new(true, :read, :integers)
     rule.associations_hash.should == {}
   end
+
+  it "should have higher specificity for attributes/conditions" do
+    CanCan::Rule.new(true, :read, :integers).specificity.should eq(1)
+    CanCan::Rule.new(true, :read, :integers, :foo => :bar).specificity.should eq(2)
+    CanCan::Rule.new(true, :read, :integers, :foo).specificity.should eq(2)
+    CanCan::Rule.new(false, :read, :integers).specificity.should eq(3)
+    CanCan::Rule.new(false, :read, :integers, :foo => :bar).specificity.should eq(4)
+    CanCan::Rule.new(false, :read, :integers, :foo).specificity.should eq(4)
+  end
 end