From 45955aed0085c453917aac2e2e5fbe5f3e73f705 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Mon, 24 Jul 2017 13:28:38 +0900 Subject: [PATCH 1/2] `Relation::Merger` should not fill `values` with empty values Currently `Relation#merge` will almost fill `values` with empty values (e.g. `other.order_values` is always true, it should be `other.order_values.any?`). This means that `Relation#merge` always changes `values` even if actually `values` is nothing changed. This behavior will makes `Relation#empty_scope?` fragile. So `Relation#merge` should avoid unnecessary changes. --- .../lib/active_record/relation/merger.rb | 22 ++++++++++--------- activerecord/test/cases/relation_test.rb | 8 +++++++ 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/activerecord/lib/active_record/relation/merger.rb b/activerecord/lib/active_record/relation/merger.rb index 182f6548973be..03824ffff960b 100644 --- a/activerecord/lib/active_record/relation/merger.rb +++ b/activerecord/lib/active_record/relation/merger.rb @@ -135,19 +135,17 @@ def merge_multi_values if other.reordering_value # override any order specified in the original relation relation.reorder! other.order_values - elsif other.order_values + elsif other.order_values.any? # merge in order_values from relation relation.order! other.order_values end - relation.extend(*other.extending_values) unless other.extending_values.blank? + extensions = other.extensions - relation.extensions + relation.extending!(*extensions) if extensions.any? end def merge_single_values - if relation.from_clause.empty? - relation.from_clause = other.from_clause - end - relation.lock_value ||= other.lock_value + relation.lock_value ||= other.lock_value if other.lock_value unless other.create_with_value.blank? relation.create_with_value = (relation.create_with_value || {}).merge(other.create_with_value) @@ -155,11 +153,15 @@ def merge_single_values end def merge_clauses - CLAUSE_METHODS.each do |method| - clause = relation.get_value(method) - other_clause = other.get_value(method) - relation.set_value(method, clause.merge(other_clause)) + if relation.from_clause.empty? && !other.from_clause.empty? + relation.from_clause = other.from_clause end + + where_clause = relation.where_clause.merge(other.where_clause) + relation.where_clause = where_clause unless where_clause.empty? + + having_clause = relation.having_clause.merge(other.having_clause) + relation.having_clause = having_clause unless having_clause.empty? end end end diff --git a/activerecord/test/cases/relation_test.rb b/activerecord/test/cases/relation_test.rb index f22fcd7b5aa86..c1805aa5926c3 100644 --- a/activerecord/test/cases/relation_test.rb +++ b/activerecord/test/cases/relation_test.rb @@ -100,6 +100,14 @@ def test_create_with_value_with_wheres assert_equal({ "hello" => "world", "id" => 10 }, relation.scope_for_create) end + def test_empty_scope + relation = Relation.new(Post, Post.arel_table, Post.predicate_builder) + assert relation.empty_scope? + + relation.merge!(relation) + assert relation.empty_scope? + end + def test_bad_constants_raise_errors assert_raises(NameError) do ActiveRecord::Relation::HelloWorld From f0e6ecc9966f5a569313658bedff54bde8517e65 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Mon, 24 Jul 2017 15:38:40 +0900 Subject: [PATCH 2/2] `get_value` and `set_value` in `Relation` are no longer used externally --- .../active_record/relation/query_methods.rb | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb index 812c8e7e3f5be..ee49930c90da8 100644 --- a/activerecord/lib/active_record/relation/query_methods.rb +++ b/activerecord/lib/active_record/relation/query_methods.rb @@ -901,16 +901,17 @@ def arel # :nodoc: @arel ||= build_arel end - # Returns a relation value with a given name - def get_value(name) # :nodoc: - @values[name] || default_value_for(name) - end + protected + # Returns a relation value with a given name + def get_value(name) # :nodoc: + @values[name] || default_value_for(name) + end - # Sets the relation value with the given name - def set_value(name, value) # :nodoc: - assert_mutability! - @values[name] = value - end + # Sets the relation value with the given name + def set_value(name, value) # :nodoc: + assert_mutability! + @values[name] = value + end private