refactoring query.joins

lib/cancan/can_definition.rb

@@ -3,9 +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 CanDefinition # :nodoc:
-    attr_reader :conditions, :block, :base_behavior
+    attr_reader :base_behavior, :actions
-    attr_reader :block
-    attr_reader :actions
     attr_writer :expanded_actions
 
     # The first argument when initializing is the base_behavior which is a true/false
@@ -55,20 +53,12 @@ module CanCan
       @conditions == {} || @conditions.nil?
     end
 
-    def association_joins(conditions = @conditions)
+    def associations_hash(conditions = @conditions)
-      return nil unless conditions.kind_of?(Hash)
+      hash = {}
-      joins = []
+      conditions.map do |name, value|
-      conditions.each do |name, value|
+        hash[name] = associations_hash(value) if value.kind_of? Hash
-        if value.kind_of? Hash
-          nested = association_joins(value)
-          if nested
-            joins << {name => nested}
-          else
-            joins << {name => []}
       end
-        end
+      hash
-      end
-      joins unless joins.empty?
     end
 
     private

lib/cancan/query.rb

@@ -32,9 +32,11 @@ module CanCan
     # Returns the associations used in conditions for the :joins option of a search
     # See ActiveRecordAdditions#accessible_by for use in Active Record.
     def joins
-      unless @can_definitions.empty?
+      joins_hash = {}
-        collect_association_joins(@can_definitions)
+      @can_definitions.each do |can_definition|
+        merge_joins(joins_hash, can_definition.associations_hash)
       end
+      clean_joins(joins_hash) unless joins_hash.empty?
     end
     
     private
@@ -67,31 +69,22 @@ module CanCan
       @sanitizer.sanitize_sql(conditions)
     end
     
-    def collect_association_joins(can_definitions)
+    def merge_joins(base, add)
-      joins = []
+      add.each do |name, nested|
-      @can_definitions.each do |can_definition|
+        if base[name].is_a?(Hash) && !nested.empty?
-        merge_association_joins(joins, can_definition.association_joins || [])
+          merge_joins(base[name], nested)
-      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
+          base[name] = nested
         end
       end
     end
     
-    def clear_association_joins(joins)
+    def clean_joins(joins_hash)
-      joins.map do |join| 
+      joins = []
-        name, nested = join.each_pair.first
+      joins_hash.each do |name, nested|
-        nested.empty? ? name : {name => clear_association_joins(nested)}
+        joins << (nested.empty? ? name : {name => clean_joins(nested)})
-      end
+      end
+      joins
     end
   end
 end

spec/cancan/active_record_additions_spec.rb

@@ -45,7 +45,7 @@ describe CanCan::ActiveRecordAdditions do
       end
     end
     # @ability.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.associations_hash(:read, @model_class).should == [{:too => [:far]}, :foo]
     @model_class.accessible_by(@ability).should == :found_records
   end
 end

spec/cancan/can_definition_spec.rb

@@ -7,28 +7,28 @@ describe CanCan::CanDefinition do
   end
 
   it "should return no association joins if none exist" do
-    @can.association_joins.should be_nil
+    @can.associations_hash.should == {}
   end
 
   it "should return no association for joins if just attributes" do
     @conditions[:foo] = :bar
-    @can.association_joins.should be_nil
+    @can.associations_hash.should == {}
   end
 
   it "should return single association for joins" do
     @conditions[:foo] = {:bar => 1}
-    @can.association_joins.should == [{:foo=>[]}]
+    @can.associations_hash.should == {:foo => {}}
   end
 
   it "should return multiple associations for joins" do
     @conditions[:foo] = {:bar => 1}
     @conditions[:test] = {1 => 2}
-    @can.association_joins.map(&:to_s).sort.should == [:foo, :test].map(&:to_s).sort
+    @can.associations_hash.should == {:foo => {}, :test => {}}
   end
 
   it "should return nested associations for joins" do
     @conditions[:foo] = {:bar => {1 => 2}}
-    @can.association_joins.should == [{:foo => [{:bar=>[]}]}]
+    @can.associations_hash.should == {:foo => {:bar => {}}}
   end
 
   it "should return table names in conditions for association joins" do
@@ -39,6 +39,6 @@ describe CanCan::CanDefinition do
 
   it "should return no association joins if conditions is nil" do
     can = CanCan::CanDefinition.new(true, :read, Integer, nil, nil)
-    can.association_joins.should be_nil
+    can.associations_hash.should == {}
   end
 end

spec/cancan/query_spec.rb

@@ -41,35 +41,30 @@ describe CanCan::Query do
   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
-    
+    @ability.query(:read, Person).conditions.should orderlessly_match("blocked=false AND user_id=1")
-    result = @ability.query(:read, Person).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, Person
     @ability.can :read, Person, :blocked => false, :user_id => 1
-    
     @ability.query(:read, Person).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).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).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
-    
+    @ability.query(:read, Person).conditions.should orderlessly_match("not (blocked=true AND user_id=1)")
-    result = @ability.query(:read, Person).conditions.should orderlessly_match("not (blocked=true AND user_id=1)")
   end
   
   it "should return appropriate sql conditions in complex case" do
@@ -77,9 +72,36 @@ describe CanCan::Query do
     @ability.can :manage, Person, :id => 1
     @ability.can :update, Person, :manager_id => 1
     @ability.cannot :update, Person, :self_managed => true
-    
     @ability.query(:update, Person).conditions.should == 'not (self_managed=true) AND ((manager_id=1) OR (id=1))'
     @ability.query(:manage, Person).conditions.should == {:id=>1}
     @ability.query(:read, Person).conditions.should == 'true=true'
   end
+  
+  it "should have nil joins if no can definitions" do
+    @ability.query(:read, Person).joins.should be_nil
+  end
+  
+  it "should have nil joins if no nested hashes specified in conditions" do
+    @ability.can :read, Person, :blocked => false
+    @ability.can :read, Person, :admin => true
+    @ability.query(:read, Person).joins.should be_nil
+  end
+  
+  it "should merge separate joins into a single array" do
+    @ability.can :read, Person, :project => { :blocked => false }
+    @ability.can :read, Person, :company => { :admin => true }
+    @ability.query(:read, Person).joins.inspect.should orderlessly_match([:company, :project].inspect)
+  end
+  
+  it "should merge same joins into a single array" do
+    @ability.can :read, Person, :project => { :blocked => false }
+    @ability.can :read, Person, :project => { :admin => true }
+    @ability.query(:read, Person).joins.should == [:project]
+  end
+  
+  it "should merge complex, nested joins" do
+    @ability.can :read, Person, :project => { :bar => {:test => true} }, :company => { :bar => {:test => true} }
+    @ability.can :read, Person, :project => { :foo => {:bar => true}, :bar => {:zip => :zap} }
+    @ability.query(:read, Person).joins.inspect.should orderlessly_match([{:project => [:bar, :foo]}, {:company => [:bar]}].inspect)
+  end
 end