Refactor build_arel: move joins out and simplify havings. [#4860 state:resolved]

Signed-off-by: José Valim <jose.valim@gmail.com>

1 files changed, 46 insertions, 46 deletions

diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb
index 7a48a6596a..50e94134f5 100644
--- a/activerecord/lib/active_record/relation/query_methods.rb
+++ b/activerecord/lib/active_record/relation/query_methods.rb
@@ -116,45 +116,7 @@ module ActiveRecord
     def build_arel
       arel = table
 
-      joined_associations = []
-      association_joins = []
-
-      joins = @joins_values.map {|j| j.respond_to?(:strip) ? j.strip : j}.uniq
-
-      joins.each do |join|
-        association_joins << join if [Hash, Array, Symbol].include?(join.class) && !array_of_strings?(join)
-      end
-
-      stashed_association_joins = joins.select {|j| j.is_a?(ActiveRecord::Associations::ClassMethods::JoinDependency::JoinAssociation)}
-
-      non_association_joins = (joins - association_joins - stashed_association_joins).reject {|j| j.blank?}
-      custom_joins = custom_join_sql(*non_association_joins)
-
-      join_dependency = ActiveRecord::Associations::ClassMethods::JoinDependency.new(@klass, association_joins, custom_joins)
-
-      join_dependency.graft(*stashed_association_joins)
-
-      @implicit_readonly = true unless association_joins.empty? && stashed_association_joins.empty?
-
-      to_join = []
-
-      join_dependency.join_associations.each do |association|
-        if (association_relation = association.relation).is_a?(Array)
-          to_join << [association_relation.first, association.join_class, association.association_join.first]
-          to_join << [association_relation.last, association.join_class, association.association_join.last]
-        else
-          to_join << [association_relation, association.join_class, association.association_join]
-        end
-      end
-
-      to_join.each do |tj|
-        unless joined_associations.detect {|ja| ja[0] == tj[0] && ja[1] == tj[1] && ja[2] == tj[2] }
-          joined_associations << tj
-          arel = arel.join(tj[0], tj[1]).on(*tj[2])
-        end
-      end
-
-      arel = arel.join(custom_joins)
+      arel = build_joins(arel, @joins_values) if @joins_values.present?
 
       @where_values.uniq.each do |where|
         next if where.blank?
@@ -168,9 +130,7 @@ module ActiveRecord
         end
       end
 
-      @having_values.uniq.each do |h|
-        arel = h.is_a?(String) ? arel.having(h) : arel.having(*h)
-      end
+      arel = arel.having(*@having_values.uniq.select{|h| h.present?})
 
       arel = arel.take(@limit_value) if @limit_value.present?
       arel = arel.skip(@offset_value) if @offset_value.present?
@@ -181,18 +141,16 @@ module ActiveRecord
 
       selects = @select_values.uniq
 
-      quoted_table_name = @klass.quoted_table_name
-
       if selects.present?
         selects.each do |s|
           @implicit_readonly = false
           arel = arel.project(s) if s.present?
         end
       else
-        arel = arel.project(quoted_table_name + '.*')
+        arel = arel.project(@klass.quoted_table_name + '.*')
       end
 
-      arel = @from_value.present? ? arel.from(@from_value) : arel.from(quoted_table_name)
+      arel = @from_value.present? ? arel.from(@from_value) : arel.from(@klass.quoted_table_name)
 
       case @lock_value
       when TrueClass
@@ -221,6 +179,48 @@ module ActiveRecord
 
     private
 
+    def build_joins(relation, joins)
+      joined_associations = []
+      association_joins = []
+
+      joins = @joins_values.map {|j| j.respond_to?(:strip) ? j.strip : j}.uniq
+
+      joins.each do |join|
+        association_joins << join if [Hash, Array, Symbol].include?(join.class) && !array_of_strings?(join)
+      end
+
+      stashed_association_joins = joins.select {|j| j.is_a?(ActiveRecord::Associations::ClassMethods::JoinDependency::JoinAssociation)}
+
+      non_association_joins = (joins - association_joins - stashed_association_joins).reject {|j| j.blank?}
+      custom_joins = custom_join_sql(*non_association_joins)
+
+      join_dependency = ActiveRecord::Associations::ClassMethods::JoinDependency.new(@klass, association_joins, custom_joins)
+
+      join_dependency.graft(*stashed_association_joins)
+
+      @implicit_readonly = true unless association_joins.empty? && stashed_association_joins.empty?
+
+      to_join = []
+
+      join_dependency.join_associations.each do |association|
+        if (association_relation = association.relation).is_a?(Array)
+          to_join << [association_relation.first, association.join_class, association.association_join.first]
+          to_join << [association_relation.last, association.join_class, association.association_join.last]
+        else
+          to_join << [association_relation, association.join_class, association.association_join]
+        end
+      end
+
+      to_join.each do |tj|
+        unless joined_associations.detect {|ja| ja[0] == tj[0] && ja[1] == tj[1] && ja[2] == tj[2] }
+          joined_associations << tj
+          relation = relation.join(tj[0], tj[1]).on(*tj[2])
+        end
+      end
+
+      relation.join(custom_joins)
+    end
+
     def apply_modules(modules)
       values = Array.wrap(modules)
       @extensions += values if values.present?