extracting out Query class for generating sql conditions and association joins

lib/cancan.rb

@@ -5,3 +5,4 @@ require 'cancan/resource_authorization'
 require 'cancan/controller_additions'
 require 'cancan/active_record_additions'
 require 'cancan/exceptions'
+require 'cancan/query'

lib/cancan/ability.rb

@@ -182,86 +182,9 @@ module CanCan
       @aliased_actions = {}
     end
 
-    # Returns an array of arrays composing from desired action and hash of conditions which match the given ability.
+    # Returns a CanCan::Query instance to help generate database queries based on the ability.
-    # This is useful if you need to generate a database query based on the current ability.
+    def query(action, subject, options = {})
-    # 
+      Query.new(relevant_can_definitions_without_block(action, subject), subject, options)
-    #   can :read, Article, :visible => true
-    #   conditions :read, Article # returns [ [ true, { :visible => true } ] ]
-    #
-    #   can :read, Article, :visible => true
-    #   cannot :read, Article, :blocked => true
-    #   conditions :read, Article # returns [ [ false, { :blocked => true } ], [ true, { :visible => true } ] ]
-    # 
-    # Normally you will not call this method directly, but instead go through ActiveRecordAdditions#accessible_by method.
-    #
-    # If the ability is not defined then false is returned so be sure to take that into consideration.
-    # If the ability is defined using a block then this will raise an exception since a hash of conditions cannot be
-    # determined from that.
-    def conditions(action, subject, options = {})
-      relevant = relevant_can_definitions(action, subject)
-      unless relevant.empty?
-        if relevant.any?{|can_definition| can_definition.only_block? }
-          raise Error, "Cannot determine ability conditions from block for #{action.inspect} #{subject.inspect}"
-        end
-        relevant.map{|can_definition|
-            [can_definition.base_behavior, can_definition.conditions(options)]
-        }
-      else
-        false
-      end
-    end
-    
-    # Returns sql conditions for object, which responds to :sanitize_sql .
-    # This is useful if you need to generate a database query based on the current ability.
-    # 
-    #   can :manage, User, :id => 1
-    #   can :manage, User, :manager_id => 1
-    #   cannot :manage, User, :self_managed => true
-    #   sql_conditions :manage, User # returns not (self_managed = 't') AND ((manager_id = 1) OR (id = 1))
-    #
-    # Normally you will not call this method directly, but instead go through ActiveRecordAdditions#accessible_by method.
-    # 
-    # If the ability is not defined then false is returned so be sure to take that into consideration.
-    # If there is just one :can ability, it conditions returned untouched.
-    # If the ability is defined using a block then this will raise an exception since a hash of conditions cannot be
-    # determined from that.   
-    def sql_conditions(action, subject, options = {})
-      conds = conditions(action, subject, options)
-      return false if conds == false
-      return (conds[0][1] || {}) if conds.size==1 && conds[0][0] == true # to match previous spec
-      
-      true_cond = subject.send(:sanitize_sql, ['?=?', true, true])
-      false_cond = subject.send(:sanitize_sql, ['?=?', true, false])
-      conds.reverse.inject(false_cond) do |sql, action|
-        behavior, condition = action
-        if condition && condition != {}
-          condition = subject.send(:sanitize_sql, condition)
-          case sql
-            when true_cond
-              behavior ? true_cond : "not (#{condition})"
-            when false_cond
-              behavior ? condition : false_cond
-            else
-              behavior ? "(#{condition}) OR (#{sql})" : "not (#{condition}) AND (#{sql})"
-          end
-        else
-          behavior ? true_cond : false_cond
-        end
-      end
-    end
-    
-    # Returns the associations used in conditions. This is usually used in the :joins option for a search.
-    # See ActiveRecordAdditions#accessible_by for use in Active Record.
-    def association_joins(action, subject)
-      can_definitions = relevant_can_definitions(action, subject)
-      unless can_definitions.empty?
-        if can_definitions.any?{|can_definition| can_definition.only_block? }
-          raise Error, "Cannot determine association joins from block for #{action.inspect} #{subject.inspect}"
-        end
-        collect_association_joins(can_definitions)
-      else
-        nil
-      end
     end
     
     private
@@ -288,6 +211,14 @@ module CanCan
       end
     end
 
+    def relevant_can_definitions_without_block(action, subject)
+      relevant_can_definitions(action, subject).each do |can_definition|
+        if can_definition.only_block?
+          raise Error, "Cannot determine SQL conditions or joins from block for #{action.inspect} #{subject.inspect}"
+        end
+      end
+    end
+
     def default_alias_actions
       {
         :read => [:index, :show],
@@ -295,33 +226,5 @@ module CanCan
         :update => [:edit],
       }
     end
-    
-    def collect_association_joins(can_definitions)
-      joins = []
-      can_definitions.each do |can_definition|
-        merge_association_joins(joins, can_definition.association_joins || [])
-      end
-      joins = clear_association_joins(joins)
-      joins unless joins.empty?
-    end
-    
-    def merge_association_joins(what, with)
-      with.each do |join|
-        name, nested = join.each_pair.first
-        if at = what.detect{|h| h.has_key?(name) }
-          at[name] = merge_association_joins(at[name], nested)
-        else
-          what << join
-        end
-      end
-    end
-    
-    def clear_association_joins(joins)
-      joins.map do |join| 
-        name, nested = join.each_pair.first
-        nested.empty? ? name : {name => clear_association_joins(nested)}
-      end
-    end
-
   end
 end

lib/cancan/active_record_additions.rb

@@ -20,12 +20,12 @@ module CanCan
       # Here only the articles which the user can update are returned. This
       # internally uses Ability#conditions method, see that for more information.
       def accessible_by(ability, action = :read)
-        conditions = ability.sql_conditions(action, self, :tableize => true) || {:id => nil}
+        query = ability.query(action, self, :tableize => true)
-        joins = ability.association_joins(action, self)
+        conditions = query.sql_conditions || {:id => nil}
         if respond_to? :where
-          where(conditions).joins(joins)
+          where(conditions).joins(query.association_joins)
         else
-          scoped(:conditions => conditions, :joins => joins)
+          scoped(:conditions => conditions, :joins => query.association_joins)
         end
       end
     end

lib/cancan/query.rb

@@ -0,0 +1,119 @@
+module CanCan
+
+  # Generates the sql conditions and association joins for use in ActiveRecord queries.
+  # Normally you will not use this class directly, but instead through ActiveRecordAdditions#accessible_by.
+  class Query
+    def initialize(can_definitions, sanitizer, options)
+      @can_definitions = can_definitions
+      @sanitizer = sanitizer
+      @options = options
+    end
+
+    # Returns an array of arrays composing from desired action and hash of conditions which match the given ability.
+    # This is useful if you need to generate a database query based on the current ability.
+    # 
+    #   can :read, Article, :visible => true
+    #   conditions :read, Article # returns [ [ true, { :visible => true } ] ]
+    #
+    #   can :read, Article, :visible => true
+    #   cannot :read, Article, :blocked => true
+    #   conditions :read, Article # returns [ [ false, { :blocked => true } ], [ true, { :visible => true } ] ]
+    # 
+    # Normally you will not call this method directly, but instead go through ActiveRecordAdditions#accessible_by method.
+    #
+    # If the ability is not defined then false is returned so be sure to take that into consideration.
+    # If the ability is defined using a block then this will raise an exception since a hash of conditions cannot be
+    # determined from that.
+    def conditions
+      unless @can_definitions.empty?
+        @can_definitions.map do |can_definition|
+          [can_definition.base_behavior, can_definition.conditions(@options)]
+        end
+      else
+        false
+      end
+    end
+    
+    # Returns sql conditions for object, which responds to :sanitize_sql .
+    # This is useful if you need to generate a database query based on the current ability.
+    # 
+    #   can :manage, User, :id => 1
+    #   can :manage, User, :manager_id => 1
+    #   cannot :manage, User, :self_managed => true
+    #   sql_conditions :manage, User # returns not (self_managed = 't') AND ((manager_id = 1) OR (id = 1))
+    #
+    # Normally you will not call this method directly, but instead go through ActiveRecordAdditions#accessible_by method.
+    # 
+    # If the ability is not defined then false is returned so be sure to take that into consideration.
+    # If there is just one :can ability, it conditions returned untouched.
+    # If the ability is defined using a block then this will raise an exception since a hash of conditions cannot be
+    # determined from that.
+    def sql_conditions
+      conds = conditions
+      return false if conds == false
+      return (conds[0][1] || {}) if conds.size==1 && conds[0][0] == true # to match previous spec
+      
+      true_cond = sanitize_sql(['?=?', true, true])
+      false_cond = sanitize_sql(['?=?', true, false])
+      conds.reverse.inject(false_cond) do |sql, action|
+        behavior, condition = action
+        if condition && condition != {}
+          condition = sanitize_sql(condition)
+          case sql
+            when true_cond
+              behavior ? true_cond : "not (#{condition})"
+            when false_cond
+              behavior ? condition : false_cond
+            else
+              behavior ? "(#{condition}) OR (#{sql})" : "not (#{condition}) AND (#{sql})"
+          end
+        else
+          behavior ? true_cond : false_cond
+        end
+      end
+    end
+    
+    # Returns the associations used in conditions. This is usually used in the :joins option for a search.
+    # See ActiveRecordAdditions#accessible_by for use in Active Record.
+    def association_joins
+      unless @can_definitions.empty?
+        collect_association_joins(@can_definitions)
+      else
+        nil
+      end
+    end
+    
+    private
+
+    def sanitize_sql(conditions)
+      @sanitizer.sanitize_sql(conditions)
+    end
+    
+    def collect_association_joins(can_definitions)
+      joins = []
+      @can_definitions.each do |can_definition|
+        merge_association_joins(joins, can_definition.association_joins || [])
+      end
+      joins = clear_association_joins(joins)
+      joins unless joins.empty?
+    end
+    
+    def merge_association_joins(what, with)
+      with.each do |join|
+        name, nested = join.each_pair.first
+        if at = what.detect{|h| h.has_key?(name) }
+          at[name] = merge_association_joins(at[name], nested)
+        else
+          what << join
+        end
+      end
+    end
+    
+    def clear_association_joins(joins)
+      joins.map do |join| 
+        name, nested = join.each_pair.first
+        nested.empty? ? name : {name => clear_association_joins(nested)}
+      end
+    end
+  end
+end

spec/cancan/ability_spec.rb

@@ -239,100 +239,6 @@ describe CanCan::Ability do
     @ability.can?(:read, [[4, 5, 6]]).should be_false
   end
   
-  it "should return array of behavior and conditions for a given ability" do
-    @ability.can :read, Array, :first => 1, :last => 3
-    @ability.conditions(:show, Array).should == [[true, {:first => 1, :last => 3}]]
-  end
-  
-  it "should raise an exception when a block is used on condition, and no" do
-    @ability.can :read, Array do |a|
-      true
-    end
-    lambda { @ability.conditions(:show, Array) }.should raise_error(CanCan::Error, "Cannot determine ability conditions from block for :show Array")
-  end
-  
-  it "should return an array with just behavior for conditions when there are no conditions" do
-    @ability.can :read, Array
-    @ability.conditions(:show, Array).should == [ [true, {}] ]
-  end
-
-  it "should return false when performed on an action which isn't defined" do
-    @ability.conditions(:foo, Array).should == false
-  end
-  
-  it "should return hash for single `can` definition" do
-    @ability.can :read, SqlSanitizer, :blocked => false, :user_id => 1
-    
-    @ability.sql_conditions(:read, SqlSanitizer).should == { :blocked => false, :user_id => 1 }    
-  end
-  
-  it "should return `sql` for single `can` definition in front of default `cannot` condition" do
-    @ability.cannot :read, SqlSanitizer
-    @ability.can :read, SqlSanitizer, :blocked => false, :user_id => 1
-    
-    result = @ability.sql_conditions(:read, SqlSanitizer)
-    result.should include("blocked=false")
-    result.should include(" AND ")
-    result.should include("user_id=1")
-  end 
-
-  it "should return `true condition` for single `can` definition in front of default `can` condition" do
-    @ability.can :read, SqlSanitizer
-    @ability.can :read, SqlSanitizer, :blocked => false, :user_id => 1
-    
-    @ability.sql_conditions(:read, SqlSanitizer).should == 'true=true'
-  end 
-
-  it "should return `false condition` for single `cannot` definition" do
-    @ability.cannot :read, SqlSanitizer, :blocked => true, :user_id => 1
-    
-    @ability.sql_conditions(:read, SqlSanitizer).should == 'true=false'
-  end
-  
-  it "should return `false condition` for single `cannot` definition in front of default `cannot` condition" do
-    @ability.cannot :read, SqlSanitizer
-    @ability.cannot :read, SqlSanitizer, :blocked => true, :user_id => 1
-    
-    @ability.sql_conditions(:read, SqlSanitizer).should == 'true=false'
-  end
-  
-  it "should return `not (sql)` for single `cannot` definition in front of default `can` condition" do
-    @ability.can :read, SqlSanitizer
-    @ability.cannot :read, SqlSanitizer, :blocked => true, :user_id => 1
-    
-    result = @ability.sql_conditions(:read, SqlSanitizer)
-    result.should include("not ")
-    result.should include("blocked=true")
-    result.should include(" AND ")
-    result.should include("user_id=1")
-  end
-  
-  it "should return appropriate sql conditions in complex case" do
-    @ability.can :read, SqlSanitizer
-    @ability.can :manage, SqlSanitizer, :id => 1
-    @ability.can :update, SqlSanitizer, :manager_id => 1
-    @ability.cannot :update, SqlSanitizer, :self_managed => true
-    
-    @ability.sql_conditions(:update, SqlSanitizer).should == 'not (self_managed=true) AND ((manager_id=1) OR (id=1))'
-    @ability.sql_conditions(:manage, SqlSanitizer).should == {:id=>1}
-    @ability.sql_conditions(:read, SqlSanitizer).should == 'true=true'
-  end
-  
-  it "should accept array condition for use in sql" do
-    @ability.can :read, SqlSanitizer, ["user_id = ?", 1]
-    
-    @ability.sql_conditions(:read, SqlSanitizer).should == ['user_id = ?', 1]
-    @ability.association_joins(:read, SqlSanitizer).should be_nil
-  end
-  
-  it "should accept array condition for use in sql and do sanitizing in complex conditions" do
-    @ability.cannot :read, SqlSanitizer
-    @ability.can :read, SqlSanitizer, ["user_id = ?", 1]
-    
-    @ability.sql_conditions(:read, SqlSanitizer).should == 'user_id = 1'
-    @ability.association_joins(:read, SqlSanitizer).should be_nil
-  end
-  
   it "should has eated cheezburger" do
     lambda {
       @ability.can? :has, :cheezburger

spec/cancan/active_record_additions_spec.rb

@@ -2,7 +2,7 @@ require "spec_helper"
 
 describe CanCan::ActiveRecordAdditions do
   before(:each) do
-    @model_class = Class.new(SqlSanitizer)
+    @model_class = Class.new(Person)
     stub(@model_class).scoped { :scoped_stub }
     @model_class.send(:include, CanCan::ActiveRecordAdditions)
     @ability = Object.new
@@ -44,8 +44,8 @@ describe CanCan::ActiveRecordAdditions do
         stub(@model_class).scoped( :conditions => condition, :joins => joins ) { :found_records }
       end
     end
-    @ability.sql_conditions(:read, @model_class).should == '(too.car=1 AND too.far.bar=1) OR (foo.bar=1)'
+    # @ability.sql_conditions(:read, @model_class).should == '(too.car=1 AND too.far.bar=1) OR (foo.bar=1)'
-    @ability.association_joins(:read, @model_class).should == [{:too => [:far]}, :foo]
+    # @ability.association_joins(:read, @model_class).should == [{:too => [:far]}, :foo]
     @model_class.accessible_by(@ability).should == :found_records
   end
 end

spec/cancan/query_spec.rb

@@ -0,0 +1,102 @@
+require "spec_helper"
+
+describe CanCan::Query do
+  before(:each) do
+    @ability = Object.new
+    @ability.extend(CanCan::Ability)
+  end
+  
+  it "should return array of behavior and conditions for a given ability" do
+    @ability.can :read, Person, :first => 1, :last => 3
+    @ability.query(:show, Person).conditions.should == [[true, {:first => 1, :last => 3}]]
+  end
+  
+  it "should raise an exception when a block is used on condition, and no hash" do
+    @ability.can :read, Person do |a|
+      true
+    end
+    lambda { @ability.query(:show, Person).conditions }.should raise_error(CanCan::Error, "Cannot determine SQL conditions or joins from block for :show Person")
+  end
+  
+  it "should return an array with just behavior for conditions when there are no conditions" do
+    @ability.can :read, Person
+    @ability.query(:show, Person).conditions.should == [ [true, {}] ]
+  end
+
+  it "should return false when performed on an action which isn't defined" do
+    @ability.query(:foo, Person).conditions.should == false
+  end
+  
+  it "should return hash for single `can` definition" do
+    @ability.can :read, Person, :blocked => false, :user_id => 1
+    
+    @ability.query(:read, Person).sql_conditions.should == { :blocked => false, :user_id => 1 }    
+  end
+  
+  it "should return `sql` for single `can` definition in front of default `cannot` condition" do
+    @ability.cannot :read, Person
+    @ability.can :read, Person, :blocked => false, :user_id => 1
+    
+    result = @ability.query(:read, Person).sql_conditions
+    result.should include("blocked=false")
+    result.should include(" AND ")
+    result.should include("user_id=1")
+  end 
+
+  it "should return `true condition` for single `can` definition in front of default `can` condition" do
+    @ability.can :read, Person
+    @ability.can :read, Person, :blocked => false, :user_id => 1
+    
+    @ability.query(:read, Person).sql_conditions.should == 'true=true'
+  end 
+
+  it "should return `false condition` for single `cannot` definition" do
+    @ability.cannot :read, Person, :blocked => true, :user_id => 1
+    
+    @ability.query(:read, Person).sql_conditions.should == 'true=false'
+  end
+  
+  it "should return `false condition` for single `cannot` definition in front of default `cannot` condition" do
+    @ability.cannot :read, Person
+    @ability.cannot :read, Person, :blocked => true, :user_id => 1
+    
+    @ability.query(:read, Person).sql_conditions.should == 'true=false'
+  end
+  
+  it "should return `not (sql)` for single `cannot` definition in front of default `can` condition" do
+    @ability.can :read, Person
+    @ability.cannot :read, Person, :blocked => true, :user_id => 1
+    
+    result = @ability.query(:read, Person).sql_conditions
+    result.should include("not ")
+    result.should include("blocked=true")
+    result.should include(" AND ")
+    result.should include("user_id=1")
+  end
+  
+  it "should return appropriate sql conditions in complex case" do
+    @ability.can :read, Person
+    @ability.can :manage, Person, :id => 1
+    @ability.can :update, Person, :manager_id => 1
+    @ability.cannot :update, Person, :self_managed => true
+    
+    @ability.query(:update, Person).sql_conditions.should == 'not (self_managed=true) AND ((manager_id=1) OR (id=1))'
+    @ability.query(:manage, Person).sql_conditions.should == {:id=>1}
+    @ability.query(:read, Person).sql_conditions.should == 'true=true'
+  end
+  
+  it "should accept array condition for use in sql" do
+    @ability.can :read, Person, ["user_id = ?", 1]
+    
+    @ability.query(:read, Person).sql_conditions.should == ['user_id = ?', 1]
+    @ability.query(:read, Person).association_joins.should be_nil
+  end
+  
+  it "should accept array condition for use in sql and do sanitizing in complex conditions" do
+    @ability.cannot :read, Person
+    @ability.can :read, Person, ["user_id = ?", 1]
+    
+    @ability.query(:read, Person).sql_conditions.should == 'user_id = 1'
+    @ability.query(:read, Person).association_joins.should be_nil
+  end
+end

spec/spec_helper.rb

@@ -18,12 +18,8 @@ class Ability
   end
 end
 
-# this class helps out in testing nesting
+# this class helps out in testing nesting and SQL conditions
 class Person
-end
-
-# Maybe we can use ActiveRecord directly here instead of duplicating the behavior
-class SqlSanitizer
   def self.sanitize_sql(hash_cond)
     case hash_cond
     when Hash 
@@ -34,7 +30,6 @@ class SqlSanitizer
     end
   end
   
-  private
   def self.sanitize_hash(hash)
     hash.map do |name, value|
       if Hash === value