Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added #or to ActiveRecord::Relation #16052

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions activerecord/CHANGELOG.md
@@ -1,3 +1,13 @@
* Added the `#or` method on ActiveRecord::Relation, allowing use of the OR
operator to combine WHERE or HAVING clauses.

Example:

Post.where('id = 1').or(Post.where('id = 2'))
# => SELECT * FROM posts WHERE (id = 1) OR (id = 2)

*Matthew Draper*, *Gael Muller*, *Olivier El Mekki*

* `has_many :through` associations will no longer save the through record
twice when added in an `after_create` callback defined before the
associations.
Expand Down
8 changes: 8 additions & 0 deletions activerecord/lib/active_record/null_relation.rb
Expand Up @@ -77,5 +77,13 @@ def calculate(operation, _column_name, _options = {})
def exists?(_id = false)
false
end

def or(other)
if other.is_a?(NullRelation)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this just be

def or(other)
  other
end

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My two considerations here were that we should probably return a new relation (even though mutating a relation is not supported, people do it, so I'm hesitant to introduce the first code that actually breaks), and that we should probably still do the compatibility check, lest we mislead someone who's experimenting.

super
else
other.or(self)
end
end
end
end
2 changes: 1 addition & 1 deletion activerecord/lib/active_record/querying.rb
Expand Up @@ -7,7 +7,7 @@ module Querying
delegate :find_by, :find_by!, to: :all
delegate :destroy, :destroy_all, :delete, :delete_all, :update, :update_all, to: :all
delegate :find_each, :find_in_batches, to: :all
delegate :select, :group, :order, :except, :reorder, :limit, :offset, :joins,
delegate :select, :group, :order, :except, :reorder, :limit, :offset, :joins, :or,
:where, :rewhere, :preload, :eager_load, :includes, :from, :lock, :readonly,
:having, :create_with, :uniq, :distinct, :references, :none, :unscope, to: :all
delegate :count, :average, :minimum, :maximum, :sum, :calculate, to: :all
Expand Down
59 changes: 59 additions & 0 deletions activerecord/lib/active_record/relation/query_methods.rb
Expand Up @@ -592,6 +592,65 @@ def rewhere(conditions)
unscope(where: conditions.keys).where(conditions)
end

# Returns a new relation, which is the logical union of this relation and the one passed as an
# argument.
#
# The two relations must be structurally compatible: they must be scoping the same model, and
# they must differ only by +where+ (if no +group+ has been defined) or +having+ (if a +group+ is
# present). Neither relation may have a +limit+, +offset+, or +uniq+ set.
#
# Post.where("id = 1").or(Post.where("id = 2"))
# # SELECT `posts`.* FROM `posts` WHERE (('id = 1' OR 'id = 2'))
#
def or(other)
spawn.or!(other)
end

def or!(other)
combining = group_values.any? ? :having : :where

unless structurally_compatible?(other, combining)
raise ArgumentError, 'Relation passed to #or must be structurally compatible'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we provide more guidance as to why they were structurally incompatible? Perhaps structurally_compatible? could become a structural_incompatibilities method, which returns an array of all of the ways in which they are incompatible, the condition could change to structural_incompatibilities.any?, and we could join the result for the error message.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 -- I'd pondered exactly that, so if we've both thought of it, it must be a good idea :)

end

unless other.is_a?(NullRelation)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we be able to avoid this is_a? check by defining having_values and where_values on NullRelation?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

left_values = send("#{combining}_values")
right_values = other.send("#{combining}_values")

common = left_values & right_values
mine = left_values - common
theirs = right_values - common
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a particular reason for this? Shouldn't

WHERE a = 1 AND (b = 2 OR c = 3)

be equivalent to

WHERE ((a = 1 AND b = 2) OR (a = 1 AND c = 3))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. legibility / length of the generated query
  2. keeps the query the same "shape" as the set of relation calls that created it

So, yes.. but it seemed like a nice thing to do, and not unreasonably expensive.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in your example @sgrif if a != 1 wouldn't the first fail after one check whereas the second would still make two checks?


if mine.any? && theirs.any?
mine = mine.map { |x| String === x ? Arel.sql(x) : x }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be pulled into a private method?

theirs = theirs.map { |x| String === x ? Arel.sql(x) : x }

mine = [Arel::Nodes::And.new(mine)] if mine.size > 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does an And node work with a single child? It feels like it should, the rest of this method could get much simpler if it did.

theirs = [Arel::Nodes::And.new(theirs)] if theirs.size > 1

common << Arel::Nodes::Or.new(mine.first, theirs.first)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reads really weird to me, but I don't have a concrete suggestion to improve it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just renaming common to combined or something. This also could become much simpler if an Or node could work when one of its children were empty, or a specialized node for that case.

end

send("#{combining}_values=", common)
end

self
end

def structurally_compatible?(other, allowed_to_vary)
Relation::SINGLE_VALUE_METHODS.all? do |name|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd extract these conditionals to smaller private methods. I know what they are doing but it would be better if they were easily to nay contributor understand what they are doing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this ended up a bit uglier than I'd originally anticipated, and I didn't go back and clean it up. Will do.

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")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you create a method like this?

def validate_strutucture_for(attr)
  lambda { |name| send("#{name}_#{attr}") == other.send("#{name}_#{attr}") }
end

And with that method, you can write

Relation::SINGLE_VALUE_METHODS.all? &validate_strutucture_for("value")

and

(Relation::MULTI_VALUE_METHODS - [allowed_to_vary, :extending]).all? &validate_strutucture_for("values")

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's a bit harder to read in Ruby than what we have now (though it could be broken up)

end &&
(extending_values - [NullRelation]) == (other.extending_values - [NullRelation]) &&
!limit_value &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could put these simple conditional on the begging of the expression since they are cheaper and Ruby would not evaluate the rest of the expression if any of these are true.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An early exit just means we're heading for an exception, so it probably doesn't gain us much.

!offset_value &&
!uniq_value
end
private :structurally_compatible?

# Allows to specify a HAVING clause. Note that you can't use HAVING
# without also specifying a GROUP clause.
#
Expand Down
81 changes: 81 additions & 0 deletions activerecord/test/cases/relation/or_test.rb
@@ -0,0 +1,81 @@
require "cases/helper"
require 'models/post'

module ActiveRecord
class OrTest < ActiveRecord::TestCase
fixtures :posts

def test_or_with_relation
expected = Post.where('id = 1 or id = 2').to_a
assert_equal expected, Post.where('id = 1').or(Post.where('id = 2')).to_a
end

def test_or_identity
expected = Post.where('id = 1').to_a
assert_equal expected, Post.where('id = 1').or(Post.where('id = 1')).to_a
end

def test_or_with_null_left
expected = Post.where('id = 1').to_a
assert_equal expected, Post.none.or(Post.where('id = 1')).to_a
end

def test_or_with_null_right
expected = Post.where('id = 1').to_a
assert_equal expected, Post.where('id = 1').or(Post.none).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
assert_equal expected, Post.or(Post.where('id = 1')).to_a
end

def test_or_without_right_where
expected = Post.all.to_a
assert_equal expected, Post.where('id = 1').or(Post.all).to_a
end

def test_or_preserves_other_querying_methods
expected = Post.where('id = 1 or id = 2 or id = 3').order('body asc').to_a
partial = Post.order('body asc')
assert_equal expected, partial.where('id = 1').or(partial.where(:id => [2, 3])).to_a
assert_equal expected, Post.order('body asc').where('id = 1').or(Post.order('body asc').where(:id => [2, 3])).to_a
end

def test_or_with_incompatible_relations
assert_raises ArgumentError do
Post.order('body asc').where('id = 1').or(Post.order('id desc').where(:id => [2, 3])).to_a
end
end

def test_or_when_grouping
groups = Post.where('id < 10').group('body').select('body, COUNT(*) AS c')
expected = groups.having("COUNT(*) > 1 OR body like 'Such%'").to_a.map {|o| [o.body, o.c] }
assert_equal expected, groups.having('COUNT(*) > 1').or(groups.having("body like 'Such%'")).to_a.map {|o| [o.body, o.c] }
end

def test_or_with_named_scope
expected = Post.where("id = 1 or body LIKE '\%a\%'").to_a
assert_equal expected, Post.where('id = 1').or(Post.containing_the_letter_a)
end

def test_or_inside_named_scope
expected = Post.where("body LIKE '\%a\%' OR title LIKE ?", "%'%").order('id DESC').to_a
assert_equal expected, Post.order(id: :desc).typographically_interesting
end

def test_or_on_loaded_relation
expected = Post.where('id = 1 or id = 2').to_a
p = Post.where('id = 1')
p.load
assert_equal p.loaded?, true
assert_equal expected, p.or(Post.where('id = 2')).to_a
end

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary line?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code in this PR differs from what was merged. See
9e42cf0

On Fri, Aug 28, 2015, 12:21 PM Alberto notifications@github.com wrote:

In activerecord/test/cases/relation/or_test.rb
#16052 (comment):

  •  assert_equal expected, Post.where('id = 1').or(Post.containing_the_letter_a)
    
  • end
  • def test_or_inside_named_scope
  •  expected = Post.where("body LIKE '\%a\%' OR title LIKE ?", "%'%").order('id DESC').to_a
    
  •  assert_equal expected, Post.order(id: :desc).typographically_interesting
    
  • end
  • def test_or_on_loaded_relation
  •  expected = Post.where('id = 1 or id = 2').to_a
    
  •  p = Post.where('id = 1')
    
  •  p.load
    
  •  assert_equal p.loaded?, true
    
  •  assert_equal expected, p.or(Post.where('id = 2')).to_a
    
  • end

Is this line necessary?


Reply to this email directly or view it on GitHub
https://github.com/rails/rails/pull/16052/files#r38228595.

end
end
3 changes: 3 additions & 0 deletions activerecord/test/models/post.rb
Expand Up @@ -18,6 +18,7 @@ def greeting
end

scope :containing_the_letter_a, -> { where("body LIKE '%a%'") }
scope :titled_with_an_apostrophe, -> { where("title LIKE '%''%'") }
scope :ranked_by_comments, -> { order("comments_count DESC") }

scope :limit_by, lambda {|l| limit(l) }
Expand All @@ -42,6 +43,8 @@ def first_comment

scope :tagged_with, ->(id) { joins(:taggings).where(taggings: { tag_id: id }) }

scope :typographically_interesting, -> { containing_the_letter_a.or(titled_with_an_apostrophe) }

has_many :comments do
def find_most_recent
order("id DESC").first
Expand Down