Bring the implementation of Relation#or up to speed

5 files changed, 48 insertions, 44 deletions

diff --git a/activerecord/lib/active_record/null_relation.rb b/activerecord/lib/active_record/null_relation.rb
index 802adca908..63ca29305d 100644
--- a/activerecord/lib/active_record/null_relation.rb
+++ b/activerecord/lib/active_record/null_relation.rb
@@ -77,11 +77,7 @@ module ActiveRecord
     end
 
     def or(other)
-      if other.is_a?(NullRelation)
-        super
-      else
-        other.or(self)
-      end
+      other.spawn
     end
   end
 end
diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb
index 78ee8b4580..b0edb3b1f2 100644
--- a/activerecord/lib/active_record/relation/query_methods.rb
+++ b/activerecord/lib/active_record/relation/query_methods.rb
@@ -596,50 +596,22 @@ module ActiveRecord
       spawn.or!(other)
     end
 
-    def or!(other)
-      combining = group_values.any? ? :having : :where
-
-      unless structurally_compatible?(other, combining)
+    def or!(other) # :nodoc:
+      unless structurally_compatible_for_or?(other)
         raise ArgumentError, 'Relation passed to #or must be structurally compatible'
       end
 
-      unless other.is_a?(NullRelation)
-        left_values = send("#{combining}_values")
-        right_values = other.send("#{combining}_values")
-
-        common = left_values & right_values
-        mine = left_values - common
-        theirs = right_values - common
-
-        if mine.any? && theirs.any?
-          mine = mine.map { |x| String === x ? Arel.sql(x) : x }
-          theirs = theirs.map { |x| String === x ? Arel.sql(x) : x }
-
-          mine = [Arel::Nodes::And.new(mine)] if mine.size > 1
-          theirs = [Arel::Nodes::And.new(theirs)] if theirs.size > 1
-
-          common << Arel::Nodes::Or.new(mine.first, theirs.first)
-        end
-
-        send("#{combining}_values=", common)
-      end
+      self.where_clause = self.where_clause.or(other.where_clause)
+      self.having_clause = self.having_clause.or(other.having_clause)
 
       self
     end
 
-    def structurally_compatible?(other, allowed_to_vary)
-      Relation::SINGLE_VALUE_METHODS.all? do |name|
-        send("#{name}_value") == other.send("#{name}_value")
-      end &&
-        (Relation::MULTI_VALUE_METHODS - [allowed_to_vary, :extending]).all? do |name|
-          send("#{name}_values") == other.send("#{name}_values")
-        end &&
-        (extending_values - [NullRelation]) == (other.extending_values - [NullRelation]) &&
-        !limit_value &&
-        !offset_value &&
-        !uniq_value
+    private def structurally_compatible_for_or?(other) # :nodoc:
+      Relation::SINGLE_VALUE_METHODS.all? { |m| send("#{m}_value") == other.send("#{m}_value") } &&
+        (Relation::MULTI_VALUE_METHODS - [:extending]).all? { |m| send("#{m}_values") == other.send("#{m}_values") } &&
+        (Relation::CLAUSE_METHODS - [:having, :where]).all? { |m| send("#{m}_clause") != other.send("#{m}_clause") }
     end
-    private :structurally_compatible?
 
     # Allows to specify a HAVING clause. Note that you can't use HAVING
     # without also specifying a GROUP clause.
diff --git a/activerecord/lib/active_record/relation/where_clause.rb b/activerecord/lib/active_record/relation/where_clause.rb
index ae5667dfd6..ce307e4f0c 100644
--- a/activerecord/lib/active_record/relation/where_clause.rb
+++ b/activerecord/lib/active_record/relation/where_clause.rb
@@ -31,6 +31,19 @@ module ActiveRecord
         )
       end
 
+      def or(other)
+        if empty?
+          other
+        elsif other.empty?
+          self
+        else
+          WhereClause.new(
+            [ast.or(other.ast)],
+            binds + other.binds
+          )
+        end
+      end
+
       def to_h(table_name = nil)
         equalities = predicates.grep(Arel::Nodes::Equality)
         if table_name
diff --git a/activerecord/test/cases/relation/or_test.rb b/activerecord/test/cases/relation/or_test.rb
index f2115d8aa6..1515b9c454 100644
--- a/activerecord/test/cases/relation/or_test.rb
+++ b/activerecord/test/cases/relation/or_test.rb
@@ -25,18 +25,22 @@ module ActiveRecord
       assert_equal expected, Post.where('id = 1').or(Post.none).to_a
     end
 
+    def test_or_with_bind_params
+      assert_equal Post.find([1, 2]), Post.where(id: 1).or(Post.where(id: 2)).to_a
+    end
+
     def test_or_with_null_both
       expected = Post.none.to_a
       assert_equal expected, Post.none.or(Post.none).to_a
     end
 
     def test_or_without_left_where
-      expected = Post.all.to_a
+      expected = Post.where('id = 1')
       assert_equal expected, Post.or(Post.where('id = 1')).to_a
     end
 
     def test_or_without_right_where
-      expected = Post.all.to_a
+      expected = Post.where('id = 1')
       assert_equal expected, Post.where('id = 1').or(Post.all).to_a
     end
 
@@ -76,6 +80,5 @@ module ActiveRecord
       assert_equal p.loaded?, true
       assert_equal expected, p.or(Post.where('id = 2')).to_a
     end
-
   end
 end
diff --git a/activerecord/test/cases/relation/where_clause_test.rb b/activerecord/test/cases/relation/where_clause_test.rb
index db18980e0b..7325aec0a9 100644
--- a/activerecord/test/cases/relation/where_clause_test.rb
+++ b/activerecord/test/cases/relation/where_clause_test.rb
@@ -145,6 +145,26 @@ class ActiveRecord::Relation
       assert_equal where_clause.ast, where_clause_with_empty.ast
     end
 
+    test "or joins the two clauses using OR" do
+      where_clause = WhereClause.new([table["id"].eq(bind_param)], [attribute("id", 1)])
+      other_clause = WhereClause.new([table["name"].eq(bind_param)], [attribute("name", "Sean")])
+      expected_ast =
+        Arel::Nodes::Grouping.new(
+          Arel::Nodes::Or.new(table["id"].eq(bind_param), table["name"].eq(bind_param))
+        )
+      expected_binds = where_clause.binds + other_clause.binds
+
+      assert_equal expected_ast.to_sql, where_clause.or(other_clause).ast.to_sql
+      assert_equal expected_binds, where_clause.or(other_clause).binds
+    end
+
+    test "or does nothing with an empty where clause" do
+      where_clause = WhereClause.new([table["id"].eq(bind_param)], [attribute("id", 1)])
+
+      assert_equal where_clause, where_clause.or(WhereClause.empty)
+      assert_equal where_clause, WhereClause.empty.or(where_clause)
+    end
+
     private
 
     def table