Introduce `Relation::WhereClause`

The way that bind values are currently stored on Relation is a mess.
They can come from `having`, `where`, or `join`. I'm almost certain that
`having` is actually broken, and calling `where` followed by `having`
followed by `where` will completely scramble the binds.

Joins don't actually add the bind parameters to the relation itself, but
instead add it onto an accessor on the arel AST which is undocumented,
and unused in Arel itself. This means that the bind values must always
be accessed as `relation.arel.bind_values + relation.bind_values`.
Anything that doesn't is likely broken (and tons of bugs have come up
for exactly that reason)

The result is that everything dealing with `Relation` instances has to
know far too much about the internals. The binds are split, combined,
and re-stored in non-obvious ways that makes it difficult to change
anything about the internal representation of `bind_values`, and is
extremely prone to bugs.

So the goal is to move a lot of logic off of `Relation`, and into
separate objects. This is not the same as what is currently done with
`JoinDependency`, as `Relation` knows far too much about its internals,
and vice versa. Instead these objects need to be black boxes that can
have their implementations swapped easily.

The end result will be two classes, `WhereClause` and `JoinClause`
(`having` will just re-use `WhereClause`), and there will be a single
method to access the bind values of a `Relation` which will be
implemented as

```
join_clause.binds + where_clause.binds + having_clause.binds
```

This is the first step towards that refactoring, with the internal
representation of where changed, and an intermediate representation of
`where_values` and `bind_values` to let the refactoring take small
steps. These will be removed shortly.

5 files changed, 110 insertions, 11 deletions

diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb
index 9c4db8a05e..54cb6861b8 100644
--- a/activerecord/lib/active_record/relation.rb
+++ b/activerecord/lib/active_record/relation.rb
@@ -1,18 +1,19 @@
 # -*- coding: utf-8 -*-
-require 'arel/collectors/bind'
+require "arel/collectors/bind"
 
 module ActiveRecord
   # = Active Record Relation
   class Relation
     MULTI_VALUE_METHODS  = [:includes, :eager_load, :preload, :select, :group,
-                            :order, :joins, :where, :having, :bind, :references,
+                            :order, :joins, :having, :references,
                             :extending, :unscope]
 
     SINGLE_VALUE_METHODS = [:limit, :offset, :lock, :readonly, :from, :reordering,
                             :reverse_order, :distinct, :create_with, :uniq]
+    CLAUSE_METHODS = [:where]
     INVALID_METHODS_FOR_DELETE_ALL = [:limit, :distinct, :offset, :group, :having]
 
-    VALUE_METHODS = MULTI_VALUE_METHODS + SINGLE_VALUE_METHODS
+    VALUE_METHODS = MULTI_VALUE_METHODS + SINGLE_VALUE_METHODS + CLAUSE_METHODS
 
     include FinderMethods, Calculations, SpawnMethods, QueryMethods, Batches, Explain, Delegation
 
diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb
index e1c74ce90a..9a636bc527 100644
--- a/activerecord/lib/active_record/relation/query_methods.rb
+++ b/activerecord/lib/active_record/relation/query_methods.rb
@@ -1,6 +1,7 @@
 require 'active_support/core_ext/array/wrap'
 require 'active_support/core_ext/string/filters'
 require 'active_model/forbidden_attributes_protection'
+require "active_record/relation/where_clause"
 
 module ActiveRecord
   module QueryMethods
@@ -90,6 +91,35 @@ module ActiveRecord
       CODE
     end
 
+    Relation::CLAUSE_METHODS.each do |name|
+      class_eval <<-CODE, __FILE__, __LINE__ + 1
+        def #{name}_clause                           # def where_clause
+          @values[:#{name}] || new_#{name}_clause    #   @values[:where] || new_where_clause
+        end                                          # end
+                                                     #
+        def #{name}_clause=(value)                   # def where_clause=(value)
+          assert_mutability!                         #   assert_mutability!
+          @values[:#{name}] = value                  #   @values[:where] = value
+        end                                          # end
+      CODE
+    end
+
+    def where_values
+      where_clause.parts
+    end
+
+    def where_values=(values)
+      self.where_clause = Relation::WhereClause.new(values || [], where_clause.binds)
+    end
+
+    def bind_values
+      where_clause.binds
+    end
+
+    def bind_values=(values)
+      self.where_clause = Relation::WhereClause.new(where_clause.parts, values || [])
+    end
+
     def create_with_value # :nodoc:
       @values[:create_with] || {}
     end
@@ -1117,5 +1147,9 @@ module ActiveRecord
         raise ArgumentError, "The method .#{method_name}() must contain arguments."
       end
     end
+
+    def new_where_clause
+      Relation::WhereClause.empty
+    end
   end
 end
diff --git a/activerecord/lib/active_record/relation/where_clause.rb b/activerecord/lib/active_record/relation/where_clause.rb
new file mode 100644
index 0000000000..b39fc1fed1
--- /dev/null
+++ b/activerecord/lib/active_record/relation/where_clause.rb
@@ -0,0 +1,29 @@
+module ActiveRecord
+  class Relation
+    class WhereClause
+      attr_reader :parts, :binds
+
+      def initialize(parts, binds)
+        @parts = parts
+        @binds = binds
+      end
+
+      def +(other)
+        WhereClause.new(
+          parts + other.parts,
+          binds + other.binds,
+        )
+      end
+
+      def ==(other)
+        other.is_a?(WhereClause) &&
+          parts == other.parts &&
+          binds == other.binds
+      end
+
+      def self.empty
+        new([], [])
+      end
+    end
+  end
+end
diff --git a/activerecord/test/cases/relation/where_clause_test.rb b/activerecord/test/cases/relation/where_clause_test.rb
new file mode 100644
index 0000000000..66e2f3ab48
--- /dev/null
+++ b/activerecord/test/cases/relation/where_clause_test.rb
@@ -0,0 +1,41 @@
+require "cases/helper"
+
+class ActiveRecord::Relation
+  class WhereClauseTest < ActiveRecord::TestCase
+    test "+ combines two where clauses" do
+      first_clause = WhereClause.new([table["id"].eq(bind_param)], [["id", 1]])
+      second_clause = WhereClause.new([table["name"].eq(bind_param)], [["name", "Sean"]])
+      combined = WhereClause.new(
+        [table["id"].eq(bind_param), table["name"].eq(bind_param)],
+        [["id", 1], ["name", "Sean"]],
+      )
+
+      assert_equal combined, first_clause + second_clause
+    end
+
+    test "+ is associative, but not commutative" do
+      a = WhereClause.new(["a"], ["bind a"])
+      b = WhereClause.new(["b"], ["bind b"])
+      c = WhereClause.new(["c"], ["bind c"])
+
+      assert_equal a + (b + c), (a + b) + c
+      assert_not_equal a + b, b + a
+    end
+
+    test "an empty where clause is the identity value for +" do
+      clause = WhereClause.new([table["id"].eq(bind_param)], [["id", 1]])
+
+      assert_equal clause, clause + WhereClause.empty
+    end
+
+    private
+
+    def table
+      Arel::Table.new("table")
+    end
+
+    def bind_param
+      Arel::Nodes::BindParam.new
+    end
+  end
+end
diff --git a/activerecord/test/cases/relation_test.rb b/activerecord/test/cases/relation_test.rb
index f7cb471984..75d74ddc7b 100644
--- a/activerecord/test/cases/relation_test.rb
+++ b/activerecord/test/cases/relation_test.rb
@@ -177,14 +177,8 @@ module ActiveRecord
     end
 
     test 'relations can be created with a values hash' do
-      relation = Relation.new(FakeKlass, :b, nil, where: [:foo])
-      assert_equal [:foo], relation.where_values
-    end
-
-    test 'merging a single where value' do
-      relation = Relation.new(FakeKlass, :b, nil)
-      relation.merge!(where: :foo)
-      assert_equal [:foo], relation.where_values
+      relation = Relation.new(FakeKlass, :b, nil, select: [:foo])
+      assert_equal [:foo], relation.select_values
     end
 
     test 'merging a hash interpolates conditions' do