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

@@ -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?

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)

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

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

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