diff options
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 |