Prevent Relation#merge from collapsing wheres on the RHS

This caused a bug with the new associations implementation, because now
association conditions are represented as Arel nodes internally right up
to when the whole thing gets turned to SQL.

In Rails 3.2, association conditions get turned to raw SQL early on,
which prevents Relation#merge from interfering.

The current implementation was buggy when a default_scope existed on the
target model, since we would basically end up doing:

  default_scope.merge(association_scope)

If default_scope contained a where(foo: 'a') and association_scope
contained a where(foo: 'b').where(foo: 'c') then the merger would see
that the same column is representated on both sides of the merge and
collapse the wheres to all but the last: where(foo: 'c')

Now, the RHS of the merge is left alone.

Fixes #8990

activerecord/CHANGELOG.md

 ## Rails 4.0.0 (unreleased) ##
 
+*   Relation#merge now only overwrites where values on the LHS of the
+    merge. Consider:
+
+        left  = Person.where(age: [13, 14, 15])
+        right = Person.where(age: [13, 14]).where(age: [14, 15])
+
+    `left` results in the following SQL:
+
+        WHERE age IN (13, 14, 15)
+
+    `right` results in the following SQL:
+
+        WHERE age IN (13, 14) AND age IN (14, 15)
+
+    Previously, `left.merge(right)` would result in all but the last
+    condition being removed:
+
+        WHERE age IN (14, 15)
+
+    Now it results in the LHS condition(s) for `age` being removed, but
+    the RHS remains as it is:
+
+        WHERE age IN (13, 14) AND age IN (14, 15)
+
+    *Jon Leighton*
+
 *   Fix handling of dirty time zone aware attributes
 
     Previously, when `time_zone_aware_attributes` were enabled, after

activerecord/lib/active_record/associations/association_scope.rb

@@ -15,7 +15,6 @@ def initialize(association)
 
       def scope
         scope = klass.unscoped
-        scope.merge! eval_scope(klass, reflection.scope) if reflection.scope
         scope.extending! Array(options[:extend])
         add_constraints(scope)
       end
@@ -59,7 +58,7 @@ def add_constraints(scope)
 
           if reflection.source_macro == :belongs_to
             if reflection.options[:polymorphic]
-              key = reflection.association_primary_key(klass)
+              key = reflection.association_primary_key(self.klass)
             else
               key = reflection.association_primary_key
             end
@@ -92,8 +91,13 @@ def add_constraints(scope)
 
           # Exclude the scope of the association itself, because that
           # was already merged in the #scope method.
-          (scope_chain[i] - [self.reflection.scope]).each do |scope_chain_item|
-            item = eval_scope(reflection.klass, scope_chain_item)
+          scope_chain[i].each do |scope_chain_item|
+            klass = i == 0 ? self.klass : reflection.klass
+            item  = eval_scope(klass, scope_chain_item)
+
+            if scope_chain_item == self.reflection.scope
+              scope.merge! item.except(:where, :includes)
+            end
 
             scope.includes! item.includes_values
             scope.where_values += item.where_values

activerecord/lib/active_record/relation/merger.rb

 require 'active_support/core_ext/hash/keys'
+require "set"
 
 module ActiveRecord
   class Relation
@@ -105,25 +106,26 @@ def merged_binds
       end
 
       def merged_wheres
-        if values[:where]
-          merged_wheres = relation.where_values + values[:where]
-
-          unless relation.where_values.empty?
-            # Remove equalities with duplicated left-hand. Last one wins.
-            seen = {}
-            merged_wheres = merged_wheres.reverse.reject { |w|
-              nuke = false
-              if w.respond_to?(:operator) && w.operator == :==
-                nuke         = seen[w.left]
-                seen[w.left] = true
-              end
-              nuke
-            }.reverse
-          end
+        values[:where] ||= []
 
-          merged_wheres
+        if values[:where].empty? || relation.where_values.empty?
+          relation.where_values + values[:where]
         else
-          relation.where_values
+          # Remove equalities from the existing relation with a LHS which is
+          # present in the relation being merged in.
+
+          seen = Set.new
+          values[:where].each { |w|
+            if w.respond_to?(:operator) && w.operator == :==
+              seen << w.left
+            end
+          }
+
+          relation.where_values.reject { |w|
+            w.respond_to?(:operator) &&
+              w.operator == :== &&
+              seen.include?(w.left)
+          } + values[:where]
         end
       end
     end

activerecord/test/cases/associations/has_many_through_associations_test.rb

@@ -894,4 +894,11 @@ def test_has_many_through_with_polymorphic_source
     end
   end
 
+  test "has many through with default scope on the target" do
+    person = people(:michael)
+    assert_equal [posts(:thinking)], person.first_posts
+
+    readers(:michael_authorless).update(first_post_id: 1)
+    assert_equal [posts(:thinking)], person.reload.first_posts
+  end
 end

activerecord/test/cases/relations_test.rb

@@ -1488,4 +1488,17 @@ def __omg__
       Array.send(:remove_method, :__omg__)
     end
   end
+
+  test "merge collapses wheres from the LHS only" do
+    left  = Post.where(title: "omg").where(comments_count: 1)
+    right = Post.where(title: "wtf").where(title: "bbq")
+
+    expected = [left.where_values[1]] + right.where_values
+    merged   = left.merge(right)
+
+    assert_equal expected, merged.where_values
+    assert !merged.to_sql.include?("omg")
+    assert merged.to_sql.include?("wtf")
+    assert merged.to_sql.include?("bbq")
+  end
 end

activerecord/test/fixtures/readers.yml

@@ -2,8 +2,10 @@ michael_welcome:
   id: 1
   post_id: 1
   person_id: 1
+  first_post_id: 2
 
 michael_authorless:
   id: 2
   post_id: 3
-  person_id: 1
\ No newline at end of file
+  person_id: 1
+  first_post_id: 3

activerecord/test/models/person.rb

@@ -15,6 +15,7 @@ class Person < ActiveRecord::Base
   has_many :fixed_bad_references, -> { where :favourite => true }, :class_name => 'BadReference'
   has_one  :favourite_reference, -> { where 'favourite=?', true }, :class_name => 'Reference'
   has_many :posts_with_comments_sorted_by_comment_id, -> { includes(:comments).order('comments.id') }, :through => :readers, :source => :post
+  has_many :first_posts, -> { where(id: [1, 2]) }, through: :readers
 
   has_many :jobs, :through => :references
   has_many :jobs_with_dependent_destroy,    :source => :job, :through => :references, :dependent => :destroy

activerecord/test/models/reader.rb

@@ -2,6 +2,7 @@ class Reader < ActiveRecord::Base
   belongs_to :post
   belongs_to :person, :inverse_of => :readers
   belongs_to :single_person, :class_name => 'Person', :foreign_key => :person_id, :inverse_of => :reader
+  belongs_to :first_post, -> { where(id: [2, 3]) }
 end
 
 class SecureReader < ActiveRecord::Base

activerecord/test/schema/schema.rb

@@ -567,6 +567,7 @@ def create_table(*args, &block)
     t.integer :post_id, :null => false
     t.integer :person_id, :null => false
     t.boolean :skimmer, :default => false
+    t.integer :first_post_id
   end
 
   create_table :references, :force => true do |t|