Merge pull request #36120 from kamipo/should_maintain_join_type

Fix merging left_joins to maintain its own `join_type` context

activerecord/CHANGELOG.md

+*   Fix merging left_joins to maintain its own `join_type` context.
+
+    Fixes #36103.
+
+    *Ryuta Kamizono*
+
+
 ## Rails 6.0.0.rc1 (April 24, 2019) ##
 
 *   Add `touch` option to `has_one` association.

activerecord/lib/active_record/associations/join_dependency.rb

@@ -64,16 +64,17 @@ def self.walk_tree(associations, hash)
         end
       end
 
-      def initialize(base, table, associations)
+      def initialize(base, table, associations, join_type)
         tree = self.class.make_tree associations
         @join_root = JoinBase.new(base, table, build(tree, base))
+        @join_type = join_type
       end
 
       def reflections
         join_root.drop(1).map!(&:reflection)
       end
 
-      def join_constraints(joins_to_add, join_type, alias_tracker)
+      def join_constraints(joins_to_add, alias_tracker)
         @alias_tracker = alias_tracker
 
         construct_tables!(join_root)
@@ -82,9 +83,9 @@ def join_constraints(joins_to_add, join_type, alias_tracker)
         joins.concat joins_to_add.flat_map { |oj|
           construct_tables!(oj.join_root)
           if join_root.match? oj.join_root
-            walk join_root, oj.join_root
+            walk(join_root, oj.join_root, oj.join_type)
           else
-            make_join_constraints(oj.join_root, join_type)
+            make_join_constraints(oj.join_root, oj.join_type)
           end
         }
       end
@@ -125,7 +126,7 @@ def apply_column_aliases(relation)
       end
 
       protected
-        attr_reader :join_root
+        attr_reader :join_root, :join_type
 
       private
         attr_reader :alias_tracker
@@ -151,7 +152,7 @@ def make_join_constraints(join_root, join_type)
           end
         end
 
-        def make_constraints(parent, child, join_type = Arel::Nodes::OuterJoin)
+        def make_constraints(parent, child, join_type)
           foreign_table = parent.table
           foreign_klass = parent.base_klass
           joins = child.join_constraints(foreign_table, foreign_klass, join_type, alias_tracker)
@@ -173,13 +174,13 @@ def table_alias_for(reflection, parent, join)
           join ? "#{name}_join" : name
         end
 
-        def walk(left, right)
+        def walk(left, right, join_type)
           intersection, missing = right.children.map { |node1|
             [left.children.find { |node2| node1.match? node2 }, node1]
           }.partition(&:first)
 
-          joins = intersection.flat_map { |l, r| r.table = l.table; walk(l, r) }
-          joins.concat missing.flat_map { |_, n| make_constraints(left, n) }
+          joins = intersection.flat_map { |l, r| r.table = l.table; walk(l, r, join_type) }
+          joins.concat missing.flat_map { |_, n| make_constraints(left, n, join_type) }
         end
 
         def find_reflection(klass, name)

activerecord/lib/active_record/relation/finder_methods.rb

@@ -371,7 +371,9 @@ def construct_relation_for_exists(conditions)
       end
 
       def apply_join_dependency(eager_loading: group_values.empty?)
-        join_dependency = construct_join_dependency(eager_load_values + includes_values)
+        join_dependency = construct_join_dependency(
+          eager_load_values + includes_values, Arel::Nodes::OuterJoin
+        )
         relation = except(:includes, :eager_load, :preload).joins!(join_dependency)
 
         if eager_loading && !using_limitable_reflections?(join_dependency.reflections)

activerecord/lib/active_record/relation/merger.rb

@@ -123,7 +123,9 @@ def merge_joins
               end
             end
 
-            join_dependency = other.construct_join_dependency(associations)
+            join_dependency = other.construct_join_dependency(
+              associations, Arel::Nodes::InnerJoin
+            )
             relation.joins!(join_dependency, *others)
           end
         end
@@ -135,7 +137,9 @@ def merge_outer_joins
             relation.left_outer_joins!(*other.left_outer_joins_values)
           else
             associations = other.left_outer_joins_values
-            join_dependency = other.construct_join_dependency(associations)
+            join_dependency = other.construct_join_dependency(
+              associations, Arel::Nodes::OuterJoin
+            )
             relation.joins!(join_dependency)
           end
         end

activerecord/lib/active_record/relation/query_methods.rb

@@ -1005,9 +1005,9 @@ def arel(aliases = nil) # :nodoc:
       @arel ||= build_arel(aliases)
     end
 
-    def construct_join_dependency(associations) # :nodoc:
+    def construct_join_dependency(associations, join_type) # :nodoc:
       ActiveRecord::Associations::JoinDependency.new(
-        klass, table, associations
+        klass, table, associations, join_type
       )
     end
 
@@ -1102,7 +1102,7 @@ def build_left_outer_joins(manager, outer_joins, aliases)
       def build_joins(manager, joins, aliases)
         unless left_outer_joins_values.empty?
           left_joins = valid_association_list(left_outer_joins_values.flatten)
-          joins << construct_join_dependency(left_joins)
+          joins.unshift construct_join_dependency(left_joins, Arel::Nodes::OuterJoin)
         end
 
         buckets = joins.group_by do |join|
@@ -1134,9 +1134,9 @@ def build_join_query(manager, buckets, join_type, aliases)
         join_list = join_nodes + convert_join_strings_to_ast(string_joins)
         alias_tracker = alias_tracker(join_list, aliases)
 
-        join_dependency = construct_join_dependency(association_joins)
+        join_dependency = construct_join_dependency(association_joins, join_type)
 
-        joins = join_dependency.join_constraints(stashed_joins, join_type, alias_tracker)
+        joins = join_dependency.join_constraints(stashed_joins, alias_tracker)
         joins.each { |join| manager.from(join) }
 
         manager.join_sources.concat(join_list)

activerecord/test/cases/associations/left_outer_join_association_test.rb

@@ -32,6 +32,10 @@ def test_left_outer_joins_count_is_same_as_size_of_loaded_results
     assert_equal 17, Post.left_outer_joins(:comments).count
   end
 
+  def test_merging_left_joins_should_be_left_joins
+    assert_equal 5, Author.left_joins(:posts).merge(Post.no_comments).count
+  end
+
   def test_left_joins_aliases_left_outer_joins
     assert_equal Post.left_outer_joins(:comments).to_sql, Post.left_joins(:comments).to_sql
   end

activerecord/test/models/post.rb

@@ -43,6 +43,7 @@ def first_comment
   has_one :first_comment, -> { order("id ASC") }, class_name: "Comment"
   has_one :last_comment, -> { order("id desc") }, class_name: "Comment"
 
+  scope :no_comments, -> { left_joins(:comments).where(comments: { id: nil }) }
   scope :with_special_comments, -> { joins(:comments).where(comments: { type: "SpecialComment" }) }
   scope :with_very_special_comments, -> { joins(:comments).where(comments: { type: "VerySpecialComment" }) }
   scope :with_post, ->(post_id) { joins(:comments).where(comments: { post_id: post_id }) }