Fix eager loading polymorphic association with mixed table conditions

This fixes a bug that the `foreign_key` and the `foreign_type` are
separated as different table conditions if a polymorphic association has
a scope that joins another tables.

6 files changed, 43 insertions, 26 deletions

diff --git a/activerecord/lib/active_record/associations/join_dependency/join_association.rb b/activerecord/lib/active_record/associations/join_dependency/join_association.rb
index 4583d89cba..ca0305abbb 100644
--- a/activerecord/lib/active_record/associations/join_dependency/join_association.rb
+++ b/activerecord/lib/active_record/associations/join_dependency/join_association.rb
@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 require "active_record/associations/join_dependency/join_part"
+require "active_support/core_ext/array/extract"
 
 module ActiveRecord
   module Associations
@@ -30,17 +31,21 @@ module ActiveRecord
             table = tables[-i]
             klass = reflection.klass
 
-            constraint = reflection.build_join_constraint(table, foreign_table)
+            join_scope = reflection.join_scope(table, foreign_table, foreign_klass)
 
-            joins << table.create_join(table, table.create_on(constraint), join_type)
-
-            join_scope = reflection.join_scope(table, foreign_klass)
             arel = join_scope.arel(alias_tracker.aliases)
+            nodes = arel.constraints.first
+
+            others = nodes.children.extract! do |node|
+              Arel.fetch_attribute(node) { |attr| attr.relation.name != table.name }
+            end
+
+            joins << table.create_join(table, table.create_on(nodes), join_type)
 
-            if arel.constraints.any?
+            unless others.empty?
               joins.concat arel.join_sources
               right = joins.last.right
-              right.expr = right.expr.and(arel.constraints)
+              right.expr.children.concat(others)
             end
 
             # The current table in this iteration becomes the foreign table in the next
diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb
index 6d2f75a3ae..3452cf971b 100644
--- a/activerecord/lib/active_record/reflection.rb
+++ b/activerecord/lib/active_record/reflection.rb
@@ -178,28 +178,24 @@ module ActiveRecord
         scope ? [scope] : []
       end
 
-      def build_join_constraint(table, foreign_table)
-        key         = join_keys.key
-        foreign_key = join_keys.foreign_key
-
-        constraint = table[key].eq(foreign_table[foreign_key])
-
-        if klass.finder_needs_type_condition?
-          table.create_and([constraint, klass.send(:type_condition, table)])
-        else
-          constraint
-        end
-      end
-
-      def join_scope(table, foreign_klass)
+      def join_scope(table, foreign_table, foreign_klass)
         predicate_builder = predicate_builder(table)
         scope_chain_items = join_scopes(table, predicate_builder)
         klass_scope       = klass_join_scope(table, predicate_builder)
 
+        key         = join_keys.key
+        foreign_key = join_keys.foreign_key
+
+        klass_scope.where!(table[key].eq(foreign_table[foreign_key]))
+
         if type
           klass_scope.where!(type => foreign_klass.polymorphic_name)
         end
 
+        if klass.finder_needs_type_condition?
+          klass_scope.where!(klass.send(:type_condition, table))
+        end
+
         scope_chain_items.inject(klass_scope, &:merge!)
       end
 
diff --git a/activerecord/lib/active_record/relation/where_clause.rb b/activerecord/lib/active_record/relation/where_clause.rb
index e225628bae..47728aac30 100644
--- a/activerecord/lib/active_record/relation/where_clause.rb
+++ b/activerecord/lib/active_record/relation/where_clause.rb
@@ -140,11 +140,7 @@ module ActiveRecord
 
         def except_predicates(columns)
           predicates.reject do |node|
-            case node
-            when Arel::Nodes::Between, Arel::Nodes::In, Arel::Nodes::NotIn, Arel::Nodes::Equality, Arel::Nodes::NotEqual, Arel::Nodes::LessThan, Arel::Nodes::LessThanOrEqual, Arel::Nodes::GreaterThan, Arel::Nodes::GreaterThanOrEqual
-              subrelation = (node.left.kind_of?(Arel::Attributes::Attribute) ? node.left : node.right)
-              columns.include?(subrelation.name.to_s)
-            end
+            Arel.fetch_attribute(node) { |attr| columns.include?(attr.name.to_s) }
           end
         end
 
diff --git a/activerecord/lib/arel.rb b/activerecord/lib/arel.rb
index 7411b5c41b..361cd915cc 100644
--- a/activerecord/lib/arel.rb
+++ b/activerecord/lib/arel.rb
@@ -39,6 +39,13 @@ module Arel # :nodoc: all
     value.is_a?(Arel::Node) || value.is_a?(Arel::Attribute) || value.is_a?(Arel::Nodes::SqlLiteral)
   end
 
+  def self.fetch_attribute(value)
+    case value
+    when Arel::Nodes::Between, Arel::Nodes::In, Arel::Nodes::NotIn, Arel::Nodes::Equality, Arel::Nodes::NotEqual, Arel::Nodes::LessThan, Arel::Nodes::LessThanOrEqual, Arel::Nodes::GreaterThan, Arel::Nodes::GreaterThanOrEqual
+      yield value.left.is_a?(Arel::Attributes::Attribute) ? value.left : value.right
+    end
+  end
+
   ## Convenience Alias
   Node = Arel::Nodes::Node
 end
diff --git a/activerecord/test/cases/associations/eager_test.rb b/activerecord/test/cases/associations/eager_test.rb
index b37e59038e..126d512068 100644
--- a/activerecord/test/cases/associations/eager_test.rb
+++ b/activerecord/test/cases/associations/eager_test.rb
@@ -4,6 +4,7 @@ require "cases/helper"
 require "models/post"
 require "models/tagging"
 require "models/tag"
+require "models/rating"
 require "models/comment"
 require "models/author"
 require "models/essay"
@@ -44,7 +45,7 @@ end
 
 class EagerAssociationTest < ActiveRecord::TestCase
   fixtures :posts, :comments, :authors, :essays, :author_addresses, :categories, :categories_posts,
-            :companies, :accounts, :tags, :taggings, :people, :readers, :categorizations,
+            :companies, :accounts, :tags, :taggings, :ratings, :people, :readers, :categorizations,
             :owners, :pets, :author_favorites, :jobs, :references, :subscribers, :subscriptions, :books,
             :developers, :projects, :developers_projects, :members, :memberships, :clubs, :sponsors
 
@@ -89,6 +90,17 @@ class EagerAssociationTest < ActiveRecord::TestCase
       "expected to find only david's posts"
   end
 
+  def test_loading_polymorphic_association_with_mixed_table_conditions
+    rating = Rating.first
+    assert_equal [taggings(:normal_comment_rating)], rating.taggings_without_tag
+
+    rating = Rating.preload(:taggings_without_tag).first
+    assert_equal [taggings(:normal_comment_rating)], rating.taggings_without_tag
+
+    rating = Rating.eager_load(:taggings_without_tag).first
+    assert_equal [taggings(:normal_comment_rating)], rating.taggings_without_tag
+  end
+
   def test_loading_with_scope_including_joins
     member = Member.first
     assert_equal members(:groucho), member
diff --git a/activerecord/test/models/rating.rb b/activerecord/test/models/rating.rb
index cf06bc6931..49aa38285f 100644
--- a/activerecord/test/models/rating.rb
+++ b/activerecord/test/models/rating.rb
@@ -3,4 +3,5 @@
 class Rating < ActiveRecord::Base
   belongs_to :comment
   has_many :taggings, as: :taggable
+  has_many :taggings_without_tag, -> { left_joins(:tag).where("tags.id": nil) }, as: :taggable, class_name: "Tagging"
 end