diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 9e22be1b947920181696b87c5bbd11d7e42f3c2f..c6a6e44724d9eba627643bd75fce6b0d00d19c1b 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,5 +1,31 @@ ## 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 diff --git a/activerecord/lib/active_record/associations/association_scope.rb b/activerecord/lib/active_record/associations/association_scope.rb index 300f67959d646e4396d4167656831b7dfa0954d3..c5fb1fe2c7330ae4c5d61be94dddfb2d8c6bface 100644 --- a/activerecord/lib/active_record/associations/association_scope.rb +++ b/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 diff --git a/activerecord/lib/active_record/relation/merger.rb b/activerecord/lib/active_record/relation/merger.rb index 59226d316ebc2708d38415652b155afbccf604b5..eb23e92fb8b2caf0421a31fe70cb052d3d1735f1 100644 --- a/activerecord/lib/active_record/relation/merger.rb +++ b/activerecord/lib/active_record/relation/merger.rb @@ -1,4 +1,5 @@ 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 diff --git a/activerecord/test/cases/associations/has_many_through_associations_test.rb b/activerecord/test/cases/associations/has_many_through_associations_test.rb index af91fb29208f7dded5b0e03dc72a0d8c9a7713bf..67d18f313a0ce1204a2f68b81c485f57887ed992 100644 --- a/activerecord/test/cases/associations/has_many_through_associations_test.rb +++ b/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 diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb index 64f1c9f6ccf8832433721da3e5024ac842e4d374..379c0c0758cf42b06d463f5bd0122436e1ba85de 100644 --- a/activerecord/test/cases/relations_test.rb +++ b/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 diff --git a/activerecord/test/fixtures/readers.yml b/activerecord/test/fixtures/readers.yml index 8a6076655b7b23a789ae8b2c698b6d56eb3c9db2..14b883f04104649cf4462467ee34a340d82f0a11 100644 --- a/activerecord/test/fixtures/readers.yml +++ b/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 diff --git a/activerecord/test/models/person.rb b/activerecord/test/models/person.rb index c602ca5eac9662ae4c1e66d7d204177518cc20bc..fa717ef8d6944bc39da437909e6e6a1acf3c1429 100644 --- a/activerecord/test/models/person.rb +++ b/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 diff --git a/activerecord/test/models/reader.rb b/activerecord/test/models/reader.rb index f8fb9c573e0d2eae9a542493a5d33ce55f06b9b7..3a6b7fad343802ae51beda1f163e36c5685c5ef2 100644 --- a/activerecord/test/models/reader.rb +++ b/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 diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb index 46219c53dbd27a8f6e1767b4e6fe5ae97cbb9f09..cd9835259a08592177de8f534bd7a6afc18884d2 100644 --- a/activerecord/test/schema/schema.rb +++ b/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|