From 8436e2c2bd91c1a57fb1273218a5428cc2c6b45a Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Mon, 26 Jan 2015 16:20:58 -0700 Subject: [PATCH] Remove `Relation#bind_values=` The last place that was assigning it was when `from` is called with a relation to use as a subquery. The implementation was actually completely broken, and would break if you called `from` more than once, or if you called it on a relation, which also had its own join clause, as the bind values would get completely scrambled. The simplest solution was to just move it into its own array, since creating a `FromClause` class for this would be overkill. --- activerecord/lib/active_record/relation.rb | 2 +- activerecord/lib/active_record/relation/merger.rb | 7 ++++--- .../lib/active_record/relation/query_methods.rb | 11 ++++------- activerecord/test/cases/relation/mutation_test.rb | 2 +- 4 files changed, 10 insertions(+), 12 deletions(-) diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index 8789bdc9ca..d4cd63155c 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -5,7 +5,7 @@ module ActiveRecord # = Active Record Relation class Relation MULTI_VALUE_METHODS = [:includes, :eager_load, :preload, :select, :group, - :order, :joins, :references, + :order, :joins, :references, :from_bind, :extending, :unscope] SINGLE_VALUE_METHODS = [:limit, :offset, :lock, :readonly, :from, :reordering, diff --git a/activerecord/lib/active_record/relation/merger.rb b/activerecord/lib/active_record/relation/merger.rb index 82b38b6ec6..1a7b180e77 100644 --- a/activerecord/lib/active_record/relation/merger.rb +++ b/activerecord/lib/active_record/relation/merger.rb @@ -51,7 +51,7 @@ module ActiveRecord NORMAL_VALUES = Relation::VALUE_METHODS - Relation::CLAUSE_METHODS - - [:joins, :order, :reverse_order, :lock, :create_with, :reordering, :from] # :nodoc: + [:joins, :order, :reverse_order, :lock, :create_with, :reordering, :from, :from_bind] # :nodoc: def normal_values NORMAL_VALUES @@ -120,8 +120,9 @@ module ActiveRecord end def merge_single_values - relation.from_value = other.from_value unless relation.from_value - relation.lock_value = other.lock_value unless relation.lock_value + relation.from_value ||= other.from_value + relation.from_bind_values = other.from_bind_values unless relation.from_value + relation.lock_value ||= other.lock_value unless other.create_with_value.blank? relation.create_with_value = (relation.create_with_value || {}).merge(other.create_with_value) diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb index c42882a947..3e33eb8b06 100644 --- a/activerecord/lib/active_record/relation/query_methods.rb +++ b/activerecord/lib/active_record/relation/query_methods.rb @@ -93,12 +93,7 @@ module ActiveRecord end def bind_values - where_clause.binds + having_clause.binds - end - - def bind_values=(values) - self.having_clause = Relation::WhereClause.new(having_clause.predicates, []) - self.where_clause = Relation::WhereClause.new(where_clause.predicates, values || []) + from_bind_values + where_clause.binds + having_clause.binds end def create_with_value # :nodoc: @@ -747,7 +742,9 @@ module ActiveRecord def from!(value, subquery_name = nil) # :nodoc: self.from_value = [value, subquery_name] if value.is_a? Relation - self.bind_values = value.arel.bind_values + value.bind_values + bind_values + self.from_bind_values = value.arel.bind_values + value.bind_values + else + self.from_bind_values = [] end self end diff --git a/activerecord/test/cases/relation/mutation_test.rb b/activerecord/test/cases/relation/mutation_test.rb index 003d870045..d25d8ac06e 100644 --- a/activerecord/test/cases/relation/mutation_test.rb +++ b/activerecord/test/cases/relation/mutation_test.rb @@ -28,7 +28,7 @@ module ActiveRecord @relation ||= Relation.new FakeKlass.new('posts'), Post.arel_table, Post.predicate_builder end - (Relation::MULTI_VALUE_METHODS - [:references, :extending, :order, :unscope, :select]).each do |method| + (Relation::MULTI_VALUE_METHODS - [:references, :extending, :order, :unscope, :select, :from_bind]).each do |method| test "##{method}!" do assert relation.public_send("#{method}!", :foo).equal?(relation) assert_equal [:foo], relation.public_send("#{method}_values")