From 249ddd0c39e6f24145ae1150d4c8eec9f11219b1 Mon Sep 17 00:00:00 2001
From: Maxime Lapointe <hunter_spawn@hotmail.com>
Date: Wed, 17 Aug 2016 11:16:36 -0400
Subject: Keep INNER JOIN when merging relations Doing
 `Author.joins(:posts).merge(Post.joins(:comments))` does this `SELECT ...
 INNER JOIN posts ON... LEFT OUTER JOIN comments ON...` instead of doing
 `SELECT ... INNER JOIN posts ON... INNER JOIN comments ON...`.

This behavior is unexpected and makes little sense as, basically, doing
`Post.joins(:comments)` means I want posts that have comments. Turning
it to a LEFT JOIN means I want posts and join the comments data, if
any.

We can see this problem directly in the existing tests.
The test_relation_merging_with_merged_joins_as_symbols only does joins
from posts to comments to ratings while the ratings fixture isn't
loaded, but the count is non-zero.
---
 activerecord/CHANGELOG.md                          | 16 +++++++++++++++
 .../active_record/associations/join_dependency.rb  |  6 +++---
 activerecord/test/cases/relation_test.rb           | 23 ++++++++++++++++++++--
 3 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index c4ee75c9a2..d4f4041910 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,19 @@
+*   Merging two relations representing nested joins no longer transforms the joins of
+    the merged relation into LEFT OUTER JOIN. Example to clarify:
+
+    ```
+    Author.joins(:posts).merge(Post.joins(:comments))
+    # Before the change:
+    #=> SELECT ... FROM authors INNER JOIN posts ON ... LEFT OUTER JOIN comments ON...
+
+    # After the change:
+    #=> SELECT ... FROM authors INNER JOIN posts ON ... INNER JOIN comments ON...
+    ```
+
+    TODO: Add to the Rails 5.2 upgrade guide
+
+    *Maxime Handfield Lapointe*
+
 *   `ActiveRecord::Persistence#touch` does not work well when optimistic locking enabled and
     `locking_column`, without default value, is null in the database.
 
diff --git a/activerecord/lib/active_record/associations/join_dependency.rb b/activerecord/lib/active_record/associations/join_dependency.rb
index 643226267c..83e5c23cc4 100644
--- a/activerecord/lib/active_record/associations/join_dependency.rb
+++ b/activerecord/lib/active_record/associations/join_dependency.rb
@@ -104,17 +104,17 @@ module ActiveRecord
         join_root.drop(1).map!(&:reflection)
       end
 
-      def join_constraints(outer_joins, join_type)
+      def join_constraints(joins_to_add, join_type)
         joins = join_root.children.flat_map { |child|
           make_join_constraints(join_root, child, join_type)
         }
 
-        joins.concat outer_joins.flat_map { |oj|
+        joins.concat joins_to_add.flat_map { |oj|
           if join_root.match? oj.join_root
             walk join_root, oj.join_root
           else
             oj.join_root.children.flat_map { |child|
-              make_outer_joins oj.join_root, child
+              make_join_constraints(oj.join_root, child, join_type)
             }
           end
         }
diff --git a/activerecord/test/cases/relation_test.rb b/activerecord/test/cases/relation_test.rb
index 5fb32270b7..73ce1cda27 100644
--- a/activerecord/test/cases/relation_test.rb
+++ b/activerecord/test/cases/relation_test.rb
@@ -6,7 +6,7 @@ require "models/rating"
 
 module ActiveRecord
   class RelationTest < ActiveRecord::TestCase
-    fixtures :posts, :comments, :authors, :author_addresses
+    fixtures :posts, :comments, :authors, :author_addresses, :ratings
 
     FakeKlass = Struct.new(:table_name, :name) do
       extend ActiveRecord::Delegation::DelegateCache
@@ -224,7 +224,26 @@ module ActiveRecord
     def test_relation_merging_with_merged_joins_as_symbols
       special_comments_with_ratings = SpecialComment.joins(:ratings)
       posts_with_special_comments_with_ratings = Post.group("posts.id").joins(:special_comments).merge(special_comments_with_ratings)
-      assert_equal({ 2 => 1, 4 => 3, 5 => 1 }, authors(:david).posts.merge(posts_with_special_comments_with_ratings).count)
+      assert_equal({ 4 => 2 }, authors(:david).posts.merge(posts_with_special_comments_with_ratings).count)
+    end
+
+    def test_relation_merging_with_merged_symbol_joins_keeps_inner_joins
+      queries = capture_sql { authors_with_commented_posts = Author.joins(:posts).merge(Post.joins(:comments)).to_a }
+
+      nb_inner_join = queries.sum { |sql| sql.scan(/INNER\s+JOIN/i).size }
+      assert_equal 2, nb_inner_join, "Wrong amount of INNER JOIN in query"
+      assert queries.none? { |sql| /LEFT\s+(OUTER)?\s+JOIN/i.match?(sql) }, "Shouldn't have any LEFT JOIN in query"
+    end
+
+    def test_relation_merging_with_merged_symbol_joins_has_correct_size_and_count
+      # Has one entry per comment
+      merged_authors_with_commented_posts_relation = Author.joins(:posts).merge(Post.joins(:comments))
+
+      post_ids_with_author = Post.joins(:author).pluck(:id)
+      manual_comments_on_post_that_have_author = Comment.where(post_id: post_ids_with_author).pluck(:id)
+
+      assert_equal manual_comments_on_post_that_have_author.size, merged_authors_with_commented_posts_relation.count
+      assert_equal manual_comments_on_post_that_have_author.size, merged_authors_with_commented_posts_relation.to_a.size
     end
 
     def test_relation_merging_with_joins_as_join_dependency_pick_proper_parent
-- 
cgit v1.2.3