Do not create unnecessary joins when performing algebraic simplificat…

…ion.

This ensures that joins are not unnecessarily placed in the query to improve SQL execution speed.

lib/cancancan/squeel/expression_builder.rb

@@ -12,6 +12,9 @@ module CanCanCan::Squeel::ExpressionBuilder
   # @param [Class] model_class The model class which the conditions reference.
   # @param [Symbol] comparator The comparator to use when generating the comparison.
   # @param [Hash] conditions The values to compare the given node's attributes against.
+  # @return [Array<(Squeel::Nodes::Node, Array<Array<Symbol>>)>] A tuple containing the Squeel
+  #   expression representing the rule's conditions, as well as an array of joins which the Squeel
+  #   expression must be joined to.
   def build(squeel, model_class, comparator, conditions)
     build_expression_node(squeel, model_class, comparator, conditions, true)
   end
@@ -24,14 +27,17 @@ def build(squeel, model_class, comparator, conditions)
   # @param [Hash] conditions The values to compare the given node's attributes against.
   # @param [Boolean] root True if the node being built is from the root. The root node is special
   #   because it does not mutate itself; all other nodes do.
+  # @return [Array<(Squeel::Nodes::Node, Array<Array<Symbol>>)>] A tuple containing the Squeel
+  #   expression representing the rule's conditions, as well as an array of joins which the Squeel
+  #   expression must be joined to.
   def build_expression_node(node, model_class, comparator, conditions, root = false)
-    conditions.reduce(nil) do |left_expression, (key, value)|
-      comparison_node = build_comparison_node(root ? node : node.dup, model_class, key,
-                                              comparator, value)
+    conditions.reduce([nil, []]) do |(left_expression, joins), (key, value)|
+      comparison_node, node_joins = build_comparison_node(root ? node : node.dup, model_class,
+                                                          key, comparator, value)
       if left_expression
-        left_expression & comparison_node
+        [left_expression & comparison_node, joins.concat(node_joins)]
       else
-        comparison_node
+        [comparison_node, node_joins]
       end
     end
   end
@@ -45,12 +51,14 @@ def build_expression_node(node, model_class, comparator, conditions, root = fals
   # @param value The value to compare the column against.
   def build_comparison_node(node, model_class, key, comparator, value)
     if value.is_a?(Hash)
-      reflection = model_class.reflect_on_association(key)
-      build_expression_node(node.__send__(key), reflection.klass, comparator, value)
+      reflection_class = model_class.reflect_on_association(key).klass
+      expression, joins = build_expression_node(node.__send__(key), reflection_class, comparator,
+                                                value)
+      [expression, joins.map { |join| join.unshift(key) }.unshift([key])]
     else
       key, comparator, value = CanCanCan::Squeel::AttributeMapper.
                                squeel_comparison_for(model_class, key, comparator, value)
-      node.__send__(key).public_send(comparator, value)
+      [node.__send__(key).public_send(comparator, value), []]
     end
   end
 end

lib/cancancan/squeel/expression_combinator.rb

@@ -10,13 +10,21 @@ module CanCanCan::Squeel::ExpressionCombinator
   # constants and performs simplification.
   #
   # @param [Squeel::Nodes::Node] left_expression The left expression.
+  # @param [Array] left_expression_joins An array of joins which the Squeel expression must be
+  #   joined to.
   # @param [Symbol] operator The operator to combine with. This must be either +:&+ or +:|+.
   # @param [Squeel::Nodes::Node] right_expression The right expression.
-  # @return [Squeel::Nodes::Node] The combination of the given expressions.
-  def combine_squeel_expressions(left_expression, operator, right_expression)
+  # @param [Array] right_expression_joins An array of joins which the Squeel expression must be
+  #   joined to.
+  # @return [Array<(Squeel::Nodes::Node, Array)>] A tuple containing the combination of the given
+  #   expressions, as well as an array of joins which the Squeel expression must be joined to.
+  def combine_squeel_expressions(left_expression, left_expression_joins, operator,
+                                 right_expression, right_expression_joins)
     case operator
-    when :& then conjunction_expressions(left_expression, right_expression)
-    when :| then disjunction_expressions(left_expression, right_expression)
+    when :& then conjunction_expressions(left_expression, left_expression_joins,
+                                         right_expression, right_expression_joins)
+    when :| then disjunction_expressions(left_expression, left_expression_joins,
+                                         right_expression, right_expression_joins)
     else
       raise ArgumentError, "#{operator} must either be :& or :|"
     end
@@ -26,35 +34,47 @@ def combine_squeel_expressions(left_expression, operator, right_expression)
   #
   # Boolean simplification is done for the +ALWAYS_TRUE+ and +ALWAYS_FALSE+ values.
   # @param [Squeel::Nodes::Node] left_expression The left expression.
+  # @param [Array] left_expression_joins An array of joins which the Squeel expression must be
+  #   joined to.
   # @param [Squeel::Nodes::Node] right_expression The right expression.
-  # @return [Squeel::Nodes::Node] The conjunction of the left and right expression.
-  def conjunction_expressions(left_expression, right_expression)
+  # @param [Array] right_expression_joins An array of joins which the Squeel expression must be
+  #   joined to.
+  # @return [Array<(Squeel::Nodes::Node, Array)>] A tuple containing the conjunction of the left and
+  #   right expressions, as well as an array of joins which the Squeel expression must be joined to.
+  def conjunction_expressions(left_expression, left_expression_joins, right_expression,
+                              right_expression_joins)
     if left_expression == ALWAYS_FALSE || right_expression == ALWAYS_FALSE
-      ALWAYS_FALSE
+      [ALWAYS_FALSE, []]
     elsif left_expression == ALWAYS_TRUE
-      right_expression
+      [right_expression, right_expression_joins]
     elsif right_expression == ALWAYS_TRUE
-      left_expression
+      [left_expression, left_expression_joins]
     else
-      left_expression & right_expression
+      [left_expression & right_expression, left_expression_joins + right_expression_joins]
     end
   end
 
   # Computes the disjunction of the two Squeel expressions.
   #
   # Boolean simplification is done for the +ALWAYS_TRUE+ and +ALWAYS_FALSE+ values.
   # @param [Squeel::Nodes::Node] left_expression The left expression.
+  # @param [Array] left_expression_joins An array of joins which the Squeel expression must be
+  #   joined to.
   # @param [Squeel::Nodes::Node] right_expression The right expression.
-  # @return [Squeel::Nodes::Node] The disjunction of the left and right expression.
-  def disjunction_expressions(left_expression, right_expression)
+  # @param [Array] right_expression_joins An array of joins which the Squeel expression must be
+  #   joined to.
+  # @return [Array<(Squeel::Nodes::Node, Array)>] A tuple containing the disjunction of the left and
+  #   right expressions, as well as an array of joins which the Squeel expression must be joined to.
+  def disjunction_expressions(left_expression, left_expression_joins, right_expression,
+                              right_expression_joins)
     if left_expression == ALWAYS_TRUE || right_expression == ALWAYS_TRUE
-      ALWAYS_TRUE
+      [ALWAYS_TRUE, []]
     elsif left_expression == ALWAYS_FALSE
-      right_expression
+      [right_expression, right_expression_joins]
     elsif right_expression == ALWAYS_FALSE
-      left_expression
+      [left_expression, left_expression_joins]
     else
-      left_expression | right_expression
+      [left_expression | right_expression, left_expression_joins + right_expression_joins]
     end
   end
 end

lib/cancancan/squeel/squeel_adapter.rb

@@ -53,7 +53,10 @@ def self.matches_relation?(subject, name, value)
     relation = subject.public_send(name)
     klass = subject.class.reflect_on_association(name).klass
 
-    relation.where { CanCanCan::Squeel::ExpressionBuilder.build(self, klass, :==, value) }.any?
+    relation.where do
+      expression, = CanCanCan::Squeel::ExpressionBuilder.build(self, klass, :==, value)
+      expression
+    end.any?
   end
   private_class_method :matches_relation?
 
@@ -66,32 +69,17 @@ def database_records
 
   # Builds a relation that expresses the set of provided rules.
   #
-  # This first joins all the tables specified in the rules, then builds the corresponding Squeel
-  # expression for the conditions.
+  # The required Squeel expression is built, then the joins which are necessary to satisfy the
+  # expressions are added to the query scope.
   def relation
-    join_scope = @rules.reduce(@model_class.where(nil)) do |scope, rule|
-      add_joins_to_scope(scope, build_join_list(rule.conditions))
+    adapter = self
+    join_list = nil
+    scope = @model_class.where(nil).where do
+      expression, join_list = adapter.send(:build_accessible_by_expression, self)
+      expression
     end
 
-    add_conditions_to_scope(join_scope)
-  end
-
-  # Builds an array of joins for the given conditions hash.
-  #
-  # For example:
-  #
-  # a: { b: { c: 3 }, d: { e: 4 }} => [[:a, :b], [:a, :d]]
-  #
-  # @param [Hash] conditions The conditions to build the joins.
-  # @return [Array<Array<Symbol>>] The joins needed to satisfy the given conditions
-  def build_join_list(conditions)
-    conditions.flat_map do |key, value|
-      if value.is_a?(Hash)
-        [[key]].concat(build_join_list(value).map { |join| Array(join).unshift(key) })
-      else
-        []
-      end
-    end
+    add_joins_to_scope(scope, join_list)
   end
 
   # Builds a relation, outer joined on the provided associations.
@@ -109,46 +97,47 @@ def add_joins_to_scope(scope, joins)
     end
   end
 
-  # Adds the rule conditions to the scope.
-  #
   # This builds Squeel expression for each rule, and combines the expression with those to the left
   # using a fold-left.
   #
   # The rules provided by Cancancan are in reverse order, i.e. the lowest priority rule is first.
   #
-  # @param [ActiveRecord::Relation] scope The scope to add the rule conditions to.
-  def add_conditions_to_scope(scope)
-    adapter = self
-    rules = @rules
-
-    scope.where do
-      rules.reverse.reduce(ALWAYS_FALSE) do |left_expression, rule|
-        adapter.send(:combine_expression_with_rule, self, left_expression, rule)
-      end
+  # @param squeel The Squeel scope.
+  # @return [Array<(Squeel::Nodes::Node, Array<Array<Symbol>>)>] A tuple containing the Squeel
+  #   expression, as well as an array of joins which the Squeel expression must be joined to.
+  def build_accessible_by_expression(squeel)
+    @rules.reverse.reduce([ALWAYS_FALSE, []]) do |(left_expression, joins), rule|
+      combine_expression_with_rule(squeel, left_expression, joins, rule)
     end
   end
 
   # Combines the given expression with the new rule.
   #
   # @param squeel The Squeel scope.
   # @param left_expression The Squeel expression for all preceding rules.
+  # @param [Array<Array<Symbol>>] joins An array of joins which the Squeel expression must be
+  #   joined to.
   # @param [CanCan::Rule] rule The rule being added.
-  # @return [Squeel::Nodes::Node] If the rule has an expression.
-  def combine_expression_with_rule(squeel, left_expression, rule)
-    right_expression = build_expression_from_rule(squeel, rule)
+  # @return [Array<(Squeel::Nodes::Node, Array<Array<Symbol>>)>] A tuple containing the Squeel
+  #   expression, as well as an array of joins which the Squeel expression must be joined to.
+  def combine_expression_with_rule(squeel, left_expression, joins, rule)
+    right_expression, right_expression_joins = build_expression_from_rule(squeel, rule)
 
     operator = rule.base_behavior ? :| : :&
-    combine_squeel_expressions(left_expression, operator, right_expression)
+    combine_squeel_expressions(left_expression, joins, operator, right_expression,
+                               right_expression_joins)
   end
 
   # Builds a Squeel expression representing the rule's conditions.
   #
   # @param squeel The Squeel scope.
   # @param [CanCan::Rule] rule The rule being built.
-  # @return [Squeel::Nodes::Node] The expression presenting the rule's conditions.
+  # @return [Array<(Squeel::Nodes::Node, Array<Array<Symbol>>)>] A tuple containing the Squeel
+  #   expression representing the rule's conditions, as well as an array of joins which the Squeel
+  #   expression must be joined to.
   def build_expression_from_rule(squeel, rule)
     if rule.conditions.empty?
-      rule.base_behavior ? ALWAYS_TRUE : ALWAYS_FALSE
+      [rule.base_behavior ? ALWAYS_TRUE : ALWAYS_FALSE, []]
     else
       comparator = rule.base_behavior ? :== : :!=
       CanCanCan::Squeel::ExpressionBuilder.build(squeel, @model_class, comparator, rule.conditions)

spec/cancancan/squeel_spec.rb

@@ -63,6 +63,14 @@
       expect(Child.accessible_by(ability)).to contain_exactly(child1, child2)
     end
 
+    it 'does not create unnecessary joins' do
+      ability.can(:read, Parent, children: { id: 1 })
+      ability.can(:read, Parent)
+
+      accessible = Parent.accessible_by(ability)
+      expect(accessible.joins_values).to be_empty
+    end
+
     it 'allows alternative values for the same attribute' do
       red = Shape.create!(color: :red)
       _green = Shape.create!(color: :green)