From 39f2c3b3ea6fac371e79c284494e3d4cfdc1e929 Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Mon, 26 Jan 2015 14:44:05 -0700 Subject: Change `having_values` to use the `WhereClause` class This fixed an issue where `having` can only be called after the last call to `where`, because it messes with the same `bind_values` array. With this change, the two can be called as many times as needed, in any order, and the final query will be correct. However, once something assigns `bind_values`, that stops. This is because we have to move all of the bind values from the having clause over to the where clause since we can't differentiate the two, and assignment was likely in the form of: `relation.bind_values += other.bind_values` This will go away once we remove all places that are assigning `bind_values`, which is next on the list. While this fixes a bug that was present in at least 4.2 (more likely present going back as far as 3.0, becoming more likely in 4.1 and later as we switched to prepared statements in more cases), I don't think this can be easily backported. The internal changes to `Relation` are non-trivial, anything that involves modifying the `bind_values` array would need to change, and I'm not confident that we have sufficient test coverage of all of those locations (when `having` was called with a hash that could generate bind values). [Sean Griffin & anthonynavarre] --- activerecord/lib/active_record/relation.rb | 8 +++++--- .../lib/active_record/relation/calculations.rb | 2 +- .../lib/active_record/relation/query_methods.rb | 9 ++++++--- .../lib/active_record/relation/where_clause.rb | 2 +- activerecord/test/cases/relations_test.rb | 20 ++++++++++++++++++-- 5 files changed, 31 insertions(+), 10 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index 5aee74ae61..8789bdc9ca 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -5,12 +5,12 @@ module ActiveRecord # = Active Record Relation class Relation MULTI_VALUE_METHODS = [:includes, :eager_load, :preload, :select, :group, - :order, :joins, :having, :references, + :order, :joins, :references, :extending, :unscope] SINGLE_VALUE_METHODS = [:limit, :offset, :lock, :readonly, :from, :reordering, :reverse_order, :distinct, :create_with, :uniq] - CLAUSE_METHODS = [:where] + CLAUSE_METHODS = [:where, :having] INVALID_METHODS_FOR_DELETE_ALL = [:limit, :distinct, :offset, :group, :having] VALUE_METHODS = MULTI_VALUE_METHODS + SINGLE_VALUE_METHODS + CLAUSE_METHODS @@ -467,8 +467,10 @@ module ActiveRecord invalid_methods = INVALID_METHODS_FOR_DELETE_ALL.select { |method| if MULTI_VALUE_METHODS.include?(method) send("#{method}_values").any? - else + elsif SINGLE_VALUE_METHODS.include?(method) send("#{method}_value") + elsif CLAUSE_METHODS.include?(method) + send("#{method}_clause").any? end } if invalid_methods.any? diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb index 1d4cb1a83b..67ac350b76 100644 --- a/activerecord/lib/active_record/relation/calculations.rb +++ b/activerecord/lib/active_record/relation/calculations.rb @@ -290,7 +290,7 @@ module ActiveRecord operation, distinct).as(aggregate_alias) ] - select_values += select_values unless having_values.empty? + select_values += select_values unless having_clause.empty? select_values.concat group_fields.zip(group_aliases).map { |field,aliaz| if field.respond_to?(:as) diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb index 31533b0939..9e72dd319e 100644 --- a/activerecord/lib/active_record/relation/query_methods.rb +++ b/activerecord/lib/active_record/relation/query_methods.rb @@ -93,10 +93,11 @@ module ActiveRecord end def bind_values - where_clause.binds + 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 || []) end @@ -605,7 +606,7 @@ module ActiveRecord def having!(opts, *rest) # :nodoc: references!(PredicateBuilder.references(opts)) if Hash === opts - self.having_values += build_where(opts, rest) + self.having_clause += having_clause_factory.build(opts, rest) self end @@ -869,7 +870,7 @@ module ActiveRecord collapse_wheres(arel, (where_clause.predicates - [''])) #TODO: Add uniq with real value comparison / ignore uniqs that have binds - arel.having(*having_values.uniq.reject(&:blank?)) unless having_values.empty? + arel.having(*having_clause.predicates.uniq.reject(&:blank?)) if having_clause.any? arel.take(connection.sanitize_limit(limit_value)) if limit_value arel.skip(offset_value.to_i) if offset_value @@ -1108,9 +1109,11 @@ module ActiveRecord def new_where_clause Relation::WhereClause.empty end + alias new_having_clause new_where_clause def where_clause_factory @where_clause_factory ||= Relation::WhereClauseFactory.new(klass, predicate_builder) end + alias having_clause_factory where_clause_factory end end diff --git a/activerecord/lib/active_record/relation/where_clause.rb b/activerecord/lib/active_record/relation/where_clause.rb index 90fb85cbf1..8b9ba3e633 100644 --- a/activerecord/lib/active_record/relation/where_clause.rb +++ b/activerecord/lib/active_record/relation/where_clause.rb @@ -3,7 +3,7 @@ module ActiveRecord class WhereClause # :nodoc: attr_reader :predicates, :binds - delegate :empty?, to: :predicates + delegate :any?, :empty?, to: :predicates def initialize(predicates, binds) @predicates = predicates diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb index d272b9b929..e0e04a0891 100644 --- a/activerecord/test/cases/relations_test.rb +++ b/activerecord/test/cases/relations_test.rb @@ -1468,10 +1468,26 @@ class RelationTest < ActiveRecord::TestCase def test_doesnt_add_having_values_if_options_are_blank scope = Post.having('') - assert_equal [], scope.having_values + assert scope.having_clause.empty? scope = Post.having([]) - assert_equal [], scope.having_values + assert scope.having_clause.empty? + end + + def test_having_with_binds_for_both_where_and_having + post = Post.first + having_then_where = Post.having(id: post.id).where(title: post.title).group(:title) + where_then_having = Post.where(title: post.title).having(id: post.id).group(:title) + + assert_equal [post], having_then_where + assert_equal [post], where_then_having + end + + def test_multiple_where_and_having_clauses + post = Post.first + having_then_where = Post.having(id: post.id).where(title: post.title).having(id: post.id).where(title: post.title).group(:title) + + assert_equal [post], having_then_where end def test_references_triggers_eager_loading -- cgit v1.2.3