diff options
| -rw-r--r-- | activerecord/CHANGELOG.md | 26 | ||||
| -rw-r--r-- | activerecord/lib/active_record/associations/association_scope.rb | 12 | ||||
| -rw-r--r-- | activerecord/lib/active_record/relation/merger.rb | 36 | ||||
| -rw-r--r-- | activerecord/test/cases/associations/has_many_through_associations_test.rb | 7 | ||||
| -rw-r--r-- | activerecord/test/cases/relations_test.rb | 13 | ||||
| -rw-r--r-- | activerecord/test/fixtures/readers.yml | 4 | ||||
| -rw-r--r-- | activerecord/test/models/person.rb | 1 | ||||
| -rw-r--r-- | activerecord/test/models/reader.rb | 1 | ||||
| -rw-r--r-- | activerecord/test/schema/schema.rb | 1 |
9 files changed, 79 insertions, 22 deletions
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 9e22be1b94..c6a6e44724 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 300f67959d..c5fb1fe2c7 100644 --- a/activerecord/lib/active_record/associations/association_scope.rb +++ b/activerecord/lib/active_record/associations/association_scope.rb @@ -15,7 +15,6 @@ module ActiveRecord 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 @@ module ActiveRecord 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 @@ module ActiveRecord # 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 59226d316e..eb23e92fb8 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 @@ module ActiveRecord 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 af91fb2920..67d18f313a 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 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase 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 64f1c9f6cc..379c0c0758 100644 --- a/activerecord/test/cases/relations_test.rb +++ b/activerecord/test/cases/relations_test.rb @@ -1488,4 +1488,17 @@ class RelationTest < ActiveRecord::TestCase 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 8a6076655b..14b883f041 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 c602ca5eac..fa717ef8d6 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 f8fb9c573e..3a6b7fad34 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 46219c53db..cd9835259a 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -567,6 +567,7 @@ ActiveRecord::Schema.define do 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| |