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

Add `#any_of` query method to active_record #10891

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@oelmekki

oelmekki commented Jun 8, 2013

Note : This PR has been extracted as a gem, usable in both
rails-3 and rails-4.

This method is inspired by any_of from mongoid.

It is hooked on WhereChain, just like #not, and allows to compute
an OR like query that leverages AR's #where syntax:

manual_removal = User.where(id: params[:users][:destroy_ids])
users = User.where.any_of(manual_removal, "email like '%@example.com'", {banned: true}).destroy_all

It can be used anywhere #where is expected to work :

manual_removal = User.where(id: params[:users][:destroy_ids])
User.where.any_of(manual_removal, "email like '%@example.com'", {banned: true})
@company.users.where.any_of(manual_removal, "email like '%@example.com'", {banned: true})
User.where(offline: false).where.any_of( manual_removal, "email like '%@example.com'", {banned: true})

Its main purpose is to both :

  • remove the need to write a sql string when we want an OR
  • allow to write dynamic OR queries, which would be a pain with a string

There's also a negative version, #none_of. This will return all active
users :

banned_users = User.where(banned: true)
unconfirmed_users = User.where("confirmed_at IS NULL")
active_users = User.where.none_of(banned_users, unconfirmed_users)
@robin850

This comment has been minimized.

Member

robin850 commented Jun 9, 2013

I like the idea but few months later, I've seen a pull request with an idea of implementing a or method in ActiveRecord. Is this got merged? If it isn't the case, what was the reason?

If this get merged, we should also update the "Active Record Query Interface" guide I think.

@diatmpravin

This comment has been minimized.

Contributor

diatmpravin commented Jun 9, 2013

It's really nice feature, As i used a lot in Mongoid. It's working fine https://gist.github.com/diatmpravin/5744138 , here is the demo app https://github.com/diatmpravin/my-test-app

@bogdan

This comment has been minimized.

Contributor

bogdan commented Jun 10, 2013

+1 for this.
AR should support OR operation in some way.

@christophemaximin

This comment has been minimized.

Contributor

christophemaximin commented Jun 12, 2013

+1 for this also!

@oelmekki

This comment has been minimized.

oelmekki commented Jun 15, 2013

Rebased against current master, fixed and squashed.

@oelmekki

This comment has been minimized.

oelmekki commented Jun 15, 2013

@robin850 : I did a search before beginning to implement it, but could not find anything (I suspect github search does not accept two characters search word, because there's simply no result on "or"). Do you have any other keyword in mind that would help to retrieve this issue ?

If it is something like implementing an #or method, it would be a problem, though :

User.where( "email LIKE '%@example.com" ).where( active: true ).or( offline: true )

What does this query ? where (email LIKE '%@example.com' AND active = '1' ) OR offline = '1' ? Or where email LIKE '%@example.com' AND ( active = '1' OR offline = '1' ) ? This can quickly get messy and counter intuitive.

The MongoId solution is quite elegant, IMO. Using #any_of, it is made clear which conditions are grouped through OR and which are grouped through AND :

  • User.where( "email LIKE '%@example.com" ).any_of({ active: true }, { offline: true })
  • fakes = User.where( "email LIKE '%@example.com'" ).where( active: true ); User.any_of( fakes, { offline: true })
@schuetzm

This comment has been minimized.

Contributor

schuetzm commented Jun 16, 2013

The PR that robin850 mentioned is probably #9052.

@robin850

This comment has been minimized.

Member

robin850 commented Jun 16, 2013

@schuetzm : Yup, nice!

@oelmekki

This comment has been minimized.

oelmekki commented Jun 16, 2013

Thanks @schuetzm .

Ok, so it seems there are some philosophical resistances against implementing OR. Not sure if it will be the same with #any_of (which has been demonstrated to work quite well for mongoid), but I suppose it may takes ages to be merged, if possible at all.

I think I'll just make a gem out of this, so we can use it immediately.

@schuetzm

This comment has been minimized.

Contributor

schuetzm commented Jun 17, 2013

@oelmekki: I still hope this will get merged eventually. I like any_of, because it's not as ambiguous as just using or, where it's sometimes not obvious how the parts of the query will be grouped with brackets in the resulting SQL. This one is actually something like "... and any of the following:", which is easier to understand.

@apneadiving

This comment has been minimized.

apneadiving commented Jun 17, 2013

Already monkey patched activerecord to include this, really helpful

@oelmekki

This comment has been minimized.

oelmekki commented Jun 17, 2013

@schuetzm Sure, I will continue to rebase it against master anyway, I can't hope to have a more complete and db engine aware test suite than one coming with activerecord :)

@jiripospisil

This comment has been minimized.

Contributor

jiripospisil commented Jun 17, 2013

+1; the best take on the OR operation so far.

@MaximeD

This comment has been minimized.

MaximeD commented Jun 17, 2013

+1; definitly a feature non mongoid users were missing!

@oelmekki

This comment has been minimized.

oelmekki commented Jun 18, 2013

Extracted as a gem. It supports both rails-3.2.13 and rails-4.

@schuetzm

This comment has been minimized.

Contributor

schuetzm commented Jun 18, 2013

There's one corner case: what will (should) happen if you pass it no arguments? Should it return an empty relation (using the new none), or the original relation? IMO, an empty relation would be more consistent, as with any additional argument the result is broadened, so it makes sense to start with an empty one.

@oelmekki

This comment has been minimized.

oelmekki commented Jun 18, 2013

I did not thought of that. Currently, due to the implementation more than by conscious choice, it applies a noop #where when passed no argument, so it returns the original relation.

I wonder if it wouldn't be better to throw an exception, though, because :

  • it's consistent with what #where does
  • the term "any_of" makes it clear ("of") we expect something as parameter and wouldn't have much sense without any
@schuetzm

This comment has been minimized.

Contributor

schuetzm commented Jun 18, 2013

Right, but even if you decide to return the relation unchanged, it should probably be duped, so that subsequent in-place modifications (with the ! methods) won't affect the original relation.

@oelmekki

This comment has been minimized.

oelmekki commented Jun 18, 2013

You're right, and :

it's consistent with what #where does

Apparently not in rails-4 (which returns original relation).

We could also consider #any_of without any argument means "any of nothing", for which it would be correct to return #none.

I have no strong opinion on that, let's see what people think before implementing it.

@leemour

This comment has been minimized.

leemour commented Jun 18, 2013

+1, it looks elegant enough for me.

@christhekeele

This comment has been minimized.

christhekeele commented Jun 18, 2013

+1 for consistant interface (over .or).

@rShetty

This comment has been minimized.

Contributor

rShetty commented Jun 18, 2013

👍

1 similar comment
@artemeff

This comment has been minimized.

Contributor

artemeff commented Jun 18, 2013

👍

@oelmekki

This comment has been minimized.

oelmekki commented Jun 19, 2013

@schuetzm I got a few more thoughts on no parameter problem. I think the best would be to stick to the nearest as possible of #where implementation, as I see #any_of as a close parent to #where (just like AND and OR are close parents).

The more predictable implementation would probably be this one :

  • on rails-3, #any_of with no parameters throws an exception (like #where)
  • on rails-4, #any_of with no parameters returns an AnyOfChain, just like #where returns a WhereChain, so that it's chainable with #not.

This of course means we have to alter the #not method. I'll check this week-end if it's not too tricky.

@NARKOZ

This comment has been minimized.

Contributor

NARKOZ commented Jun 19, 2013

I want this in active_record

@schuetzm

This comment has been minimized.

Contributor

schuetzm commented Jun 19, 2013

@oelmekki I agree this is a good solution, but the downside is that Animal.any_of.not(...) looks strange, where none_of would better describe what it does.

@oelmekki

This comment has been minimized.

oelmekki commented Jun 19, 2013

Indeed. I'm not an english native speaker, so I was unsure about the grammatical validity of "any of not".

My first thought was "well, we can implement AnyOfChain then add a syntactic sugar through a #none_of method". But now, what about passing no arguments to #none_of ? :)

I think we'll keep that simple : #any_of raises an exception when passed no arguments, and a #none_of method reflects the same kind of difference than between #where and #where.not (and #none_of raises an exception too if no arguments).

EDIT : (because I suppose not raising exceptions anymore in #where without arguments fullfill the sole purpose of being chainable to #not).

@mauricioszabo

This comment has been minimized.

mauricioszabo commented Jun 19, 2013

I like the idea, but what happens if something like this occurs:

on_br = User.where('addesses.country = ?', "BR").joins(:addresses)
on_us = User.where('addresses.country = ?', 'US').joins(:addresses)
User.any_of(on_us, login: "Admin") #this should do the "join" above
User.any_of(on_us, on_br) #this should not duplicate the "join"

I don't think that "merging" ActiveRecord::Relations is a quite simple thing to do.

@oelmekki

This comment has been minimized.

oelmekki commented Jun 19, 2013

@mauricioszabo This is currently a problem, thanks for pointing it. A workaround is to use #joins on final query :

on_br = User.where('addesses.country = ?', "BR")
on_us = User.where('addresses.country = ?', 'US')
User.any_of(on_us, login: "Admin").joins(:addresses)
User.any_of(on_us, on_br).joins(:addresses)

But we should be able to use #joins seamlessly on any query. I create an issue on the gem repos to investigate this.

@fjsj

This comment has been minimized.

fjsj commented Jun 20, 2013

+1

@stiff

This comment has been minimized.

Contributor

stiff commented Aug 4, 2013

+1 for this feature and naming where_any and where_none, seems good to me.

@oelmekki

This comment has been minimized.

oelmekki commented Aug 31, 2013

Actually, I think we could take advantage of new #where proxy, here,
just like it's used for #where.not. We thus could have :

inactive_users = User.where.any_of( { banned: true }, "confirmed_at = NULL" )
newsletter_users = User.where.none_of( inactive_users, { subscribed: false } ) 

Which should make it clear we only handle conditions.

@oelmekki

This comment has been minimized.

oelmekki commented Aug 31, 2013

Ok, PR has been updated.

So, as for now, #any_of and #none_of are behind WhereChain, just
like #not.

It means that the correct syntax is like this :

inactive_users = User.where.any_of( { banned: true }, "confirmed_at = NULL" )
newsletter_users = User.where.none_of( inactive_users, { subscribed: false } ) 
@robin850

This comment has been minimized.

Member

robin850 commented Aug 31, 2013

@oelmekki : The Travis build is failing pretty hard. Could you have a look please ? Don't hesitate to ping me if you need to restart a specific job.

@oelmekki

This comment has been minimized.

oelmekki commented Aug 31, 2013

@robin850 Yep, I saw that while running locally. I finaly realized it
had nothing to do with my changes : it fails in master as well.

Builds from other branches suffer the same regression, with the same
errors :

As I rebase this branch almost every day, I expect fails to disappear
once it's fixed in master.

@oelmekki

This comment has been minimized.

oelmekki commented Sep 2, 2013

Ok, master has been fixed. Rebased, all green.

@oelmekki

This comment has been minimized.

oelmekki commented Sep 3, 2013

Actually, I realize tests (both for master and this branch) only pass
when run locally, not on travis.

@tenderlove Do you have any input on why tests are failing on travis in
master branch, or if anyone is handling it ?

@oelmekki

This comment has been minimized.

oelmekki commented Sep 3, 2013

(nevermind, I realize what is still failing are rubinus and jruby
builds, which are considered ok to fail).

@garysweaver

This comment has been minimized.

Contributor

garysweaver commented Oct 1, 2013

On first try with activerecord_any_of v1.1, we did an AND by accident with activerecord_any_of, e.g.

This is an AND:

q.any_of(ds: {foo: :bar}, ds_cs: {foo: :bar})

This is an OR:

q.any_of({ds: {foo: :bar}}, {ds_cs: {foo: :bar}})

I understand that Mongoid users are already familiar with this eccentricity since the usage comes from that, but having not been introduced to it until now and it being misunderstood at first pass, I just wonder if "any_of" is clear enough method name for something that allows a single hash arg to indicate an AND of those key/value pairs as conditions.

Also:

  • If chaining and you specify the where with a condition and chain off of that, e.g. where(some_condition).any_of(...), the some_condition gets AND'd within each OR, which isn't totally intuitive or expected.
  • OR's, etc. can also be done with ARel, but I understand the point is not that it can't be done with SQL fragments in strings or in ARel, but that it can help make queries DRYer, and it does.

Anyway, this is a really nice patch. +1 because we're using the gem, and it would be nice to have something to do ORs in AR that was as clear and brief as this without having to use SQL fragment strings or ARel. Thanks for your work!

@oelmekki

This comment has been minimized.

oelmekki commented Oct 2, 2013

@garysweaver Thanks for feedback, I didn't realize this may be an issue.

The problem here is that #any_of expects an expended array of hashes
(via *args parameter), and this form would only pass one hash :

q.any_of(ds: {foo: :bar}, ds_cs: {foo: :bar})

It's the same as :

q.any_of({ds: {foo: :bar}, ds_cs: {foo: :bar}})

But this may indeed be confusing when you don't have the implementation
in mind.

Since there's no point in saying "any of a single condition" (#any_of( cond )),
I think we could add a friendly warning, here :

q.any_of(ds: {foo: :bar}, ds_cs: {foo: :bar})
# Warning : #any_of called with a single condition. Maybe you forgot to write explicit hashes : #any_of({foo: 'bar'}, {foo: 'baz'}) ?
@timothyrobb

This comment has been minimized.

timothyrobb commented Nov 18, 2013

👍
This would be incredibly useful. Currently we have hacks in projects to do this ourselves.

Add `#any_of` query method to active_record
This method is inspired by [any_of from
mongoid](http://two.mongoid.org/docs/querying/criteria.html#any_of).

It is hooked on `WhereChain`, just like `#not`, and allows to compute an
`OR` like query that leverages AR's `#where` syntax:

    manual_removal = User.where(id: params[:users][:destroy_ids])
    users = User.where.any_of(manual_removal, "email like '%@example.com'", {banned: true}).destroy_all

It can be used anywhere `#where` is expected to work :

    manual_removal = User.where(id: params[:users][:destroy_ids])
    User.where.any_of(manual_removal, "email like '%@example.com'", {banned: true})
    @company.users.where.any_of(manual_removal, "email like '%@example.com'", {banned: true})
    User.where(offline: false).where.any_of( manual_removal, "email like '%@example.com'", {banned: true})

Its main purpose is to both :

* remove the need to write a sql string when we want an `OR`
* allow to write dynamic `OR` queries, which would be a pain with a string

There's also a negative version, `#none_of`. This will return all active
users :

    banned_users = User.where(banned: true)
    unconfirmed_users = User.where("confirmed_at IS NULL")
    active_users = User.where.none_of(banned_users, unconfirmed_users)
@kriansa

This comment has been minimized.

kriansa commented May 14, 2014

Any updates on this?

@robin850 robin850 added this to the 4.2.0 milestone May 14, 2014

@oelmekki

This comment has been minimized.

oelmekki commented May 15, 2014

@kriansa still haven't heard much of rails team about it. Meanwhile,
you can use the extracted gem to have the feature.

@thedarkone

This comment has been minimized.

Contributor

thedarkone commented May 16, 2014

@oelmekki your old changelog entry uses an old any_of syntax (instead of the new where.any_of).

I think you should turn AlternativeBuilder into plain module and QueryMethods#any_of|none_of method should just refer to the appropriate builder classes:

def any_of(*queries)
  raise ArgumentError, 'Called any_of() with no arguments.' if queries.none?
  AlternativeBuilder::PositiveBuilder.new(@scope, queries).build
end

also since relation construction is a semi-hot code path, you should avoid splatting and then de-splatting queries array and change the Builder#initialize into accepting the raw source_queries array:

def initialize(context, source_queries) # notice the missing start -> *source_queries
  @context, @source_queries = context, source_queries
  @queries_bind_values, @queries_joins_values = [], { includes: [],  joins: [], references: [] }
end

While at it, I think it might be worthwhile to DRY-up the PositiveBuilder and NegativeBuilder by moving both with_statement_cache and without_statement_cache into Builder and just adding a dispatch_where method:

class PositiveBuilder < Builder
  private
    def dispatch_where(args)
      where(args)
    end
end

class NegativeBuilder < Builder
  private
    def dispatch_where(args)
      where.not(args)
    end
end

or (here I'm thinking out loud) what would happen if Builder#queries used dispatch_where while iterating @source_queries? Or does this have to happen later in with_statement_cache and without_statement_cache?

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

@pywebdesign

This comment has been minimized.

pywebdesign commented Oct 2, 2014

+1

1 similar comment
@katranci

This comment has been minimized.

katranci commented Oct 7, 2014

+1

@rails rails locked and limited conversation to collaborators Oct 7, 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

CarsomyrJ referenced this pull request in fools-gold/swindler Jun 11, 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.