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 #9052

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions activerecord/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,18 @@

*Alexander Grebennik*

* Added the `#or` method on ActiveRecord::Relation, in order to use the OR
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move changelog entry to the first line

operand when joining WHERE clauses.

Examples:

Post.where('id = 1').or.where('id = 2')
Post.where('id = 1').or.containing_the_letter_a
Post.where('id = 1').or(Post.where('id = 2')
Post.where('id = 1').or('id = 2')

*Gael Muller*

* Added a state instance variable to each transaction. Will allow other objects
to know whether a transaction has been committed or rolled back.

Expand Down
2 changes: 1 addition & 1 deletion activerecord/lib/active_record/querying.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ module Querying
delegate :find_each, :find_in_batches, :to => :all
delegate :select, :group, :order, :except, :reorder, :limit, :offset, :joins,
:where, :preload, :eager_load, :includes, :from, :lock, :readonly,
:having, :create_with, :uniq, :references, :none, :to => :all
:having, :create_with, :uniq, :references, :none, :or, :to => :all
delegate :count, :average, :minimum, :maximum, :sum, :calculate, :pluck, :ids, :to => :all

# Executes a custom SQL query against your database and returns all the results. The results will
Expand Down
88 changes: 88 additions & 0 deletions activerecord/lib/active_record/relation/query_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,22 @@ def not(opts, *rest)
end
end

# OrChain objects act as placeholder for queries in which #or does not have any parameter.
# In this case, #or must be chained with any other relation method to return a new relation.
# It is intended to allow .or.where() and .or.named_scope.
class OrChain
def initialize(scope)
@scope = scope
end

def method_missing(method, *args, &block)
right_relation = @scope.klass.unscoped do
@scope.klass.send(method, *args, &block)
end
@scope.or(right_relation)
end
end

Relation::MULTI_VALUE_METHODS.each do |name|
class_eval <<-CODE, __FILE__, __LINE__ + 1
def #{name}_values # def select_values
Expand Down Expand Up @@ -452,6 +468,61 @@ def where!(opts = :chain, *rest) # :nodoc:
end
end

# Returns a new relation, which is the result of filtering the current relation
# according to the conditions in the arguments, joining WHERE clauses with OR
# operand, contrary to the default behaviour that uses AND.
#
# #or accepts conditions in one of several formats. In the examples below, the resulting
# SQL is given as an illustration; the actual query generated may be different depending
# on the database adapter.
#
# === without arguments
#
# If #or is used without arguments, it returns an ActiveRecord::OrChain object that can
# be used to chain queries with any other relation method, like where:
#
# Post.where("id = 1").or.where("id = 2")
# # SELECT `posts`.* FROM `posts` WHERE (('id = 1' OR 'id = 2'))
#
# It can also be chained with a named scope:
#
# Post.where("id = 1").or.containing_the_letter_a
# # SELECT `posts`.* FROM `posts` WHERE (('id = 1' OR 'body LIKE \\'%a%\\''))
#
# === ActiveRecord::Relation
#
# When #or is used with an ActiveRecord::Relation as an argument, it merges the two
# relations, with the exception of the WHERE clauses, that are joined using the OR
# operand.
#
# Post.where("id = 1").or(Post.where("id = 2"))
# # SELECT `posts`.* FROM `posts` WHERE (('id = 1' OR 'id = 2'))
#
# === anything you would pass to #where
#
# #or also accepts anything that could be passed to the #where method, as
# a shortcut:
#
# Post.where("id = 1").or("id = ?", 2)
# # SELECT `posts`.* FROM `posts` WHERE (('id = 1' OR 'id = 2'))
#
def or(opts = :chain, *rest)
if opts == :chain
OrChain.new(self)
else
left = with_default_scope
right = (ActiveRecord::Relation === opts) ? opts : klass.unscoped.where(opts, rest)

unless left.where_values.empty? || right.where_values.empty?
left, right = left.spawn, right.spawn
left.where_values = [left.where_ast.or(right.where_ast)]
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a problem here: you are modifying the original relations. If you're lucky, this will fail with ActiveRecord::ImmutableRelation, but this only happens if the relations have already been loaded.

The fix is simple:

diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb
index 1e291e6..ced097c 100644
--- a/activerecord/lib/active_record/relation/query_methods.rb
+++ b/activerecord/lib/active_record/relation/query_methods.rb
@@ -607,6 +607,7 @@ module ActiveRecord
         right = (ActiveRecord::Relation === opts) ? opts : klass.unscoped.where(opts, rest)

         unless left.where_values.empty? || right.where_values.empty?
+          left, right = left.dup, right.dup
           left.where_values = [left.where_ast.or(right.where_ast)]
           right.where_values = []
         end

right.where_values = []
end

left.merge!(right)
end
end

# Allows to specify a HAVING clause. Note that you can't use HAVING
# without also specifying a GROUP clause.
#
Expand Down Expand Up @@ -727,6 +798,23 @@ def build_arel
arel
end

# Returns an Arel AST containing only where_values
def where_ast
arel_wheres = []

where_values.each do |where|
arel_wheres << (String === where ? Arel.sql(where) : where)
end

return Arel::Nodes::And.new(arel_wheres) if arel_wheres.length >= 2

if Arel::Nodes::SqlLiteral === arel_wheres.first
Arel::Nodes::Grouping.new(arel_wheres.first)
else
arel_wheres.first
end
end

private

def custom_join_ast(table, joins)
Expand Down
52 changes: 52 additions & 0 deletions activerecord/test/cases/relation/or_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
require "cases/helper"
Copy link
Contributor

Choose a reason for hiding this comment

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

to be consistent with other code I think better to use: require 'cases/helper'

Copy link

Choose a reason for hiding this comment

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

i love your attention to detail but that's extreme :)

Copy link
Member

Choose a reason for hiding this comment

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

I don't see as extreme. I'm not saying that you need to change but this make totally sense to me.

Copy link

Choose a reason for hiding this comment

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

i only see it as extreme because it doesn't really have any practical implications.
if i had to apply logic to this I think double quotes because I can easily use string interpolation there too but it seems like a small thing to worry about. i dont know how precise rails contributions need to be.

Copy link
Member

Choose a reason for hiding this comment

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

Single or double quotes are fine, but like I said don't need to change.

I Don't know how precise rails contributions need to be.

Copy link

Choose a reason for hiding this comment

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

ty

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

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

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

def test_or_without_right_where
expected = Post.where('id = 1').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
assert_equal expected, Post.where('id = 1').order('body asc').or(:id => [2, 3]).to_a
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.containing_the_letter_a
end

def test_or_on_loaded_relation
expected = Post.where('id = 1 or id = 2').to_a
p = Post.where('id = 1')
Copy link
Contributor

Choose a reason for hiding this comment

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

p = Post.where('id = 1').load will use less lines

p.load
assert_equal p.loaded?, true
assert_equal expected, p.or('id = 2').to_a
end

end
end