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
base: master
from

Conversation

Projects
None yet
@gaelmuller

gaelmuller commented Jan 23, 2013

ActiveRecord::Relation#or 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, contraty to the default behaviour that uses AND.

ActiveRecord::Relation#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 ActiveRecord::Relation#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

ActiveRecord::Relation#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'))

This is my first contribution to rails and I tried to keep things coherent with the current code, but I am obviously open for feedback.

@mdespuits

View changes

activerecord/lib/active_record/relation/query_methods.rb Outdated
@@ -452,6 +468,60 @@ 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, contraty to the default behaviour that uses AND.

This comment has been minimized.

@mdespuits

mdespuits Jan 24, 2013

Contributor

Typo contrary

@mdespuits

This comment has been minimized.

Contributor

mdespuits commented Jan 24, 2013

👍 Would love to see this one added. Simple idea that has been missing for a long time just like not.

@mdespuits

View changes

activerecord/lib/active_record/relation/query_methods.rb Outdated
def method_missing(method, *args, &block)
right_relation = @scope.klass.unscoped {
@scope.klass.send(method, *args, &block)
}

This comment has been minimized.

@mdespuits

mdespuits Jan 24, 2013

Contributor

Generally multi-line blocks use the do..end style.

@gaelmuller

This comment has been minimized.

gaelmuller commented Jan 24, 2013

Thank you @mattdbridges for your feedback. I updated the commit with corrections for the typo and the multiline block.

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Jan 24, 2013

Sorry but some time ago we reached the conclusion to not add this method.

Some reason:

  1. It opens the door for wanting to expand this line of DSL further (including undesireable ventures into greater_or_equal_than).
  2. It's being a fairly uncommon query not in need of optimization.
  3. OR is usually a query smell

Thank you so much.

@gaelmuller

This comment has been minimized.

gaelmuller commented Mar 13, 2013

@rafaelfranca, I feel like the arguments to not add this method are those used for the like method that was rollbacked, but I think it doesn't apply here.

  1. This seems like a common need. You only have to look at this issue (#5545), or the several forums/stack overflow posts to realize that.
  2. This actually is not just an optimization, but a missing feature. With like or greater_or_equal_than, you can always easily write a where clause that would do the exact same thing. I don't see any way to join two scopes with a "OR" without rewriting the whole query in sql (which defeats the purpose of ActiveRecord).

If you still disagree, I will try to extract it as a plugin.

@gaelmuller

This comment has been minimized.

gaelmuller commented Mar 15, 2013

@tenderlove, @steveklabnik, you commented on a similar pull request (#6817). What is your opinion about this ?

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Mar 15, 2013

I don't have strong opinions about this one, so I think we can reopen to discuss.

@jeremy thoughts?

@rafaelfranca rafaelfranca reopened this Mar 15, 2013

@housekeeper

This comment has been minimized.

housekeeper commented Mar 27, 2013

I would +1 this too but for a true scoping issue...

Developing a re-occurring calendaring gem and it would be awesome to chain scopes in the following ways

Calendar::Event.for_current_user.today
Calendar::Event.for_current_user.this_week
Calendar::Event.for_current_user.next_week

there are 3 scoped chains for 3 tables... an events table based on whether the user created the event or whether the user has been assigned/notified to/of the event in an event_users table. This is handled with no problem, my issue is when there is a re-occurring event which falls between a reoccurring start and end as well as an event start and end of which I need to OR - this is perfectly valid reason for this request and I believe a missing feature especially when wanting to create complex but efficient queries

FYI
An event only has one entry in the table
if this event re-occurs, its reoccurrence type is stored and the start/stop datetimes NOT each reoccurrence as an event.

@gregmolnar

This comment has been minimized.

Member

gregmolnar commented Mar 27, 2013

👍 for this feature

@midu

This comment has been minimized.

midu commented Mar 27, 2013

👍 ❤️

@crmaxx

This comment has been minimized.

crmaxx commented Apr 9, 2013

👍 for this feature

@ilyakatz

This comment has been minimized.

Contributor

ilyakatz commented Apr 9, 2013

+1

1 similar comment
@KevinBongart

This comment has been minimized.

KevinBongart commented Apr 9, 2013

👍

@niuage

This comment has been minimized.

niuage commented Apr 9, 2013

👍 💛

Added #or to ActiveRecord::Relation
ActiveRecord::Relation#or 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, contraty to the default behaviour that uses AND.

ActiveRecord::Relation#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 ActiveRecord::Relation#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

ActiveRecord::Relation#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'))
@ghalley

This comment has been minimized.

ghalley commented Apr 25, 2013

+1

@descentintomael

This comment has been minimized.

descentintomael commented Apr 25, 2013

+1 for this feature. I need some way to merge scopes together without having to do it manually column-by-column in squeel.

@atestu

This comment has been minimized.

atestu commented Apr 25, 2013

👍

1 similar comment
@jcapron

This comment has been minimized.

jcapron commented Apr 25, 2013

👍

@christhekeele

This comment has been minimized.

christhekeele commented Apr 25, 2013

+1
This would clean up those corners of my code where I feel like I'm not using an ORM anymore.

@williscool

This comment has been minimized.

williscool commented Apr 26, 2013

In my humble opinion, leaving this to arel is a much more elegant

ie

user_arel_table = User.arel_table
User.where(user_arel_table[:name].eq('bob').or(user_arel_table[:age].lt(25))).to_sql

translates to

SELECT "users".* FROM "users"  WHERE (("users"."name" = 'bob' OR "users"."age" < 25))

or to convert one of your examples from the pull request

Post.where("id = 1").or(Post.where("id = 2"))

posts = Post.arel_table
Post.where(posts[:id].eq(1).or(posts[:id].eq(2))).to_sql
SELECT "posts".* FROM "posts"  WHERE (("posts"."id" = 1 OR "posts"."id" = 2))

you can also check out a couple other good examples here (only the upvoted ones of course)

http://stackoverflow.com/questions/7976358/activerecord-arel-or-condition

you can also join in other model/tables and then use their arel tables also. I can add some examples like that if anyone wants to see.

@laskaridis

This comment has been minimized.

laskaridis commented Apr 26, 2013

It's embarrassing enough that this feature is NOT in ActiveRecord yet. +1

@descentintomael

This comment has been minimized.

descentintomael commented Apr 26, 2013

@williscool That example works for small queries, but what if I have a large number of columns to search across? When I try the inject methods in the SO example I end up with Hash objects instead of ARel objects which don't work. I would prefer to have a native solution to combining scopes with OR. Rails is really good about having elegant solutions to many things, but this doesn't seem to be one of them.

@williscool

This comment has been minimized.

williscool commented Apr 26, 2013

@descentintomael can you give an example with a large number of columns where the arel version is cumbersome?

And you would do your method chaining in between the where functions on the ActiveRecord::Relation objects. Not in the middle of them on the arel ones.

Man the naming of those things is quite confusing I wish they had completely different names.

What I mean is

User.containing_the_letter_a.where(admins.or(authors))

not

User.where(admins.or(authors).containing_the_letter_a)
@descentintomael

This comment has been minimized.

descentintomael commented Apr 26, 2013

Right now I'm trying to search through my Order model which has quite a few columns. For instance, it has an amount and a currency column, those two need to be joined with 'AND' but then there are other columns like customer ID and PO number that come into play and need to be combined with 'OR'.

I'm not really sure yet how the syntax would go (without using Squeel), but I do like the direction taken in the first example:

Post.where("id = 1").or(Post.where("id = 2"))
right = (ActiveRecord::Relation === opts) ? opts : klass.unscoped.where(opts, rest)
unless left.where_values.empty? || right.where_values.empty?
left.where_values = [left.where_ast.or(right.where_ast)]

This comment has been minimized.

@schuetzm

schuetzm Apr 27, 2013

Contributor

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
@gaelmuller

This comment has been minimized.

gaelmuller commented Apr 30, 2013

@schuetzm, thank you for your feedback. I updated the commit to avoid ImmutableRelation exceptions.

@gaelmuller

This comment has been minimized.

gaelmuller commented Jan 16, 2014

You have to think about how chaining works to be able to understand what behavior or will have. Keep in mind that:

relation.where(id: 1).or(id: 2).where(id: 3)

is equivalent to:

relation = relation.where(id: 1)
relation = relation.or(id: 2)
relation = relation.where(id: 3)

When trying to understand what this will do, you should always use the second block to decompose the chaining. In this case, it is pretty obvious that or applies to everything on the left, and only one element on the right (we have no way of knowing if there will be more than one).

A few examples taken from this thread:

>> relation = Post.where(id: 1).or.where(id: 2)
>> relation.where(name: "foo").to_sql
=> "SELECT \"posts\".* FROM \"posts\"  WHERE \"posts\".\"name\" = 'foo' AND 
((\"posts\".\"id\" = 1 OR \"posts\".\"id\" = 2))"

As noted by @al2o3cr we cannot produce (id = 1) OR (id = 2 AND name = 'foo') using .where(id: 1).or.where(id: 2).where(name: 'foo'). This is why the or method is able to take a Relation as an argument:

>> Post.where(id: 1).or(Post.where(id: 2).where(name: "foo")).to_sql
=> "SELECT \"posts\".* FROM \"posts\"  WHERE ((\"posts\".\"id\" = 1 OR \"posts\".\"id\" = 2 
AND \"posts\".\"name\" = 'foo'))"

Please not that in SQL, AND takes precedence over OR.

There is another case that can be not as elegant as we could hope: when you want to apply or only to one element on the left. Let's say you want (id = 1) AND ((id = 2) OR (title = "foo")). The naive chaining will not work:

>> relation = Post.where(id: 1)
>> relation.where(id: 2).or(title: "foo").to_sql
=> "SELECT \"posts\".* FROM \"posts\"  WHERE ((\"posts\".\"id\" = 1 AND \"posts\".\"id\" = 2 
OR \"posts\".\"title\" = 'foo'))"

You have two possibilities. The first one is to change the order, to first call the or:

>> relation = Post.where(id: 2).or(title: "foo")
>> relation.where(id: 1).to_sql
=> "SELECT \"posts\".* FROM \"posts\"  WHERE \"posts\".\"id\" = 1 AND ((\"posts\".\"id\" = 2 
OR \"posts\".\"title\" = 'foo'))"

Or, if you don't really get to manage what comes first:

>> relation = Post.where(id: 1)
>> relation.merge(Post.where(id: 2).or(title: "foo")).to_sql
=> "SELECT \"posts\".* FROM \"posts\"  WHERE \"posts\".\"id\" = 1 AND ((\"posts\".\"id\" = 2 
OR \"posts\".\"title\" = 'foo'))"

This is where we could probably use something like and that would allow use to write .where(id: 1).and(Post.where(id: 2).or(title: "foo"))

@ghost

This comment has been minimized.

ghost commented Jan 17, 2014

@al2o3cr test cases would be great! it also solves a problem that is quite complex to solve with just AREL or plain old SQL(SQL variant being simple).

for example, imagine:

class Car < ActiveRecord::LOL
  has_many :registrations
  def self.registered_on(start, finish)
    joins(:registrations).where(registrations: {value: start .. finish})
  end
end
Car.registered_on(1.days.ago, 10.days.ago).or.registered_on(65.days.ago, 68.days.ago)

unless i'm missing something, AR doesn't make that very easy right now unless you fallback to AREL - which is overly complicated to solve this problem and leaves ActiveRecord altogether. other option being a really long SQL string.

@ghost

This comment has been minimized.

ghost commented Jan 17, 2014

@gaelmuller is the above query possible with your pull request? do I need to do anything different?

@gaelmuller

This comment has been minimized.

gaelmuller commented Jan 17, 2014

Yes, @robgleeson it should work.

@ghost

This comment has been minimized.

ghost commented Jan 17, 2014

awesome, thank you.

@br3nt

This comment has been minimized.

br3nt commented Apr 29, 2014

Here's another example that needs or: Gist: Advanced Search utilising scopes (problem with match on any).

The PatientSearch#results method should be able to select between matching any of the criteria (results.or(query)), or all of the criteria (results.where(query)).

To make this work in rails currently, all query methods in PatientSearch would need to return arel instead of a relation so that they can be ORed together. This has a flow on effect such that any scopes used would also need to be rewritten as methods that return arel. Basically, throw the ActiveRecord query dsl and all its conveniences out the window.

This wouldn't be an issue if or existed.

  queries.each do |query|
    next if !query # skip if no query
    if match_criteria == 'all'
      results = results.where(query)  # each search gets ANDed
    elsif match_criteria == 'any'
      results = results.or(query)  # each search gets ORed
    end
  end
@midu

This comment has been minimized.

midu commented Apr 29, 2014

The problem is more that all these chainable methods are an incitation to violating the law of demeter, which we all know is wrong.

@vlad-shatskyi

This comment has been minimized.

Contributor

vlad-shatskyi commented Apr 29, 2014

@midu I would say that the law of demeter isn't an issue here. You can think of it as the builder pattern: we don't rely on object's internals, or dependencies of dependency, if you wish; instead we add pieces to one object. The returning object is always the same. In alternative syntax it could look like

query = Query.new
query.add_where_clause!(...)
query.add_where_clause!(...)
@br3nt

This comment has been minimized.

br3nt commented Apr 30, 2014

@midu, the example I provided is not an 'incitation to violating the law of demeter'. In fact it should quite cleanly build a query if there was an or operator.

The example I provided is a very legitimate example of where or requires support from ActiveRecord.

Besides, so long as its implementation is consistent with the rest of the dsl it should be the programmers prerogative to decide how they build queries.

@br3nt

This comment has been minimized.

br3nt commented Apr 30, 2014

@midu, the law of demeter (and all other concerns) is secondary to a much larger issue.

Here is why an 'or` operator should be supported by the ActiveRecord query dsl:

  • and and or are the core building blocks of mathematical logic and set theory
  • The SQL grammar is based on set theory and mathematical logic theory
  • SQL builds up a logic statement that defines the set to be returned
  • Excluding the 'or' operator from ActiveRecord's query dsl is denying the use of a foundational logic operator

This fact should be considered above all else as it is at the heart of SQL and all the mathematical theory that it is built around.

(on a side note, imagine if ruby didn't have an || operator)

@tarmotalu

This comment has been minimized.

tarmotalu commented May 5, 2014

+1

1 similar comment
@paxer

This comment has been minimized.

paxer commented May 9, 2014

+1

@ghost

This comment has been minimized.

ghost commented May 9, 2014

sounds good to me, but i have no interest in rails/AR.

@ghost

This comment has been minimized.

ghost commented May 9, 2014

all the +1's are also strange. you would swear someone was using a bot-net to sway public opinion.

@ghost

This comment has been minimized.

ghost commented May 9, 2014

"law of demeter", oh get on that, +1 +1 +1 +1.

@matthewd matthewd self-assigned this May 9, 2014

@dmgarland

This comment has been minimized.

dmgarland commented May 13, 2014

+1

1 similar comment
@zavan

This comment has been minimized.

Contributor

zavan commented May 29, 2014

+1

@fuyi

This comment has been minimized.

fuyi commented May 30, 2014

would like to see this feature be added

@Sporky023

This comment has been minimized.

Sporky023 commented Jun 12, 2014

@br3nt said it right: OR is a fundamental mathematical operation which deserves support. Without using OR, our actions are limited.

@rails rails locked and limited conversation to collaborators Jun 13, 2014

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Jun 13, 2014

I locked the conversation. We already have enough reasons to thinks about it so we don't need more support comments like "+1" or "I agree with this". We are going to review and if we decide to merge we will reopen the discussion.

@rafaelfranca rafaelfranca modified the milestones: 4.2.0, 5.0.0 Aug 18, 2014

@sgrif

This comment has been minimized.

Member

sgrif commented Jan 28, 2015

Closing in favor of #18706 or #16052

@sgrif sgrif closed this Jan 28, 2015

@rafaelfranca rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.