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

Already on GitHub? Sign in to your account

Relation.where with no args can be chained with not, like, and not_like #8332

Merged
merged 1 commit into from Dec 7, 2012

Conversation

Projects
None yet
Member

amatsuda commented Nov 27, 2012

Finally implemented @jeremy's suggestion here #5950 (comment)

This patch adds a chainable mode to AR::Relation for new AR not, like, and not_like query methods.

examples:

Model.where.not field: nil
#=> "SELECT * FROM models WHERE field IS NOT NULL

Model.where.like name: 'Jeremy%'
#=> "SELECT * FROM models WHERE name LIKE 'Jeremy%'

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Nov 27, 2012

activerecord/lib/active_record/relation/query_methods.rb
+ end
+ end
+
+ # Returns a new relation expressing WHERE + NOT condition
+ # according to the conditions in the arguments.
+ #
+ # User.where.not(name: "Jon")
+ # # SELECT * FROM users WHERE name <> 'Jon'
+ #
+ # User.where.not(name: nil)
+ # # SELECT * FROM users WHERE name IS NOT NULL
+ #
+ # User.where.not(name: %(Ko1 Nobu))
+ # # SELECT * FROM users WHERE name NOT IN ('Ko1', 'Nobu')
+ def not(opts, *rest)
+ extend(NotBuilder).where(opts, *rest).dup
@carlosantoniodasilva

carlosantoniodasilva Nov 27, 2012

Owner

Is it necessary to dup, since where should already be a cloned relation?

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Nov 27, 2012

activerecord/lib/active_record/relation/query_methods.rb
@@ -379,20 +446,36 @@ def bind!(value)
# User.joins(:posts).where({ "posts.published" => true })
# User.joins(:posts).where({ posts: { published: true } })
#
+ # === no argument or nil
+ #
+ # If <tt>where</tt> was called with no argument, it returns a special relation that can be
+ # chained with <tt>not</tt>, <tt>like</tt>, and <tt>not_like</tt> query methods.
@carlosantoniodasilva

carlosantoniodasilva Nov 27, 2012

Owner

It may be useful to copy over one or two examples here as well, wdyt?

Looks awesome 👍❤️, thanks @amatsuda.

We'll probably need a changelog entry and some guides updates for that, let me know if you need help with anything.

Contributor

josevalim commented Nov 27, 2012

Oh, this feature is amazing. Thanks!

One question, couldn't we implement it not using mixins? Something like:

class WhereChain
  def initialize(scope)
    @scope = scope
  end

  def not_like(conditions)
    @scope.where(handle(conditions))
  end
end

def where(opts)
  if opts.nil?
    WhereChain.new(spawn)
  elsif opts.blank?
    self
  else
    spawn.where!(opts, *rest)
  end
end

The downside of using mixins is that not will not be available in the relation until the end of times. So one can effectively do: where.not().order.not(). Using a class clears this up, but requires using where before each not and so forth.

Member

amatsuda commented Nov 27, 2012

@carlosantoniodasilva Alright. I will work on more documentations :)

@josevalim I just wanted to always keep the returned Relation as an absolute Relation, for example responds to all and to_sql and everything, because I thought that's the beauty of ActiveRecord::Relation.
But it's OK to change the behaviour if you core team guys prefer simpler implementation.

Contributor

josevalim commented Nov 27, 2012

@amatsuda ah, I see what you mean. Yes, you are correct, my proposal is not going to work as expected. :shipit:

Contributor

josevalim commented Nov 27, 2012

Wait, actually, my proposal is still going to return a relation, except when you call .where without any argument and do not follow it by predicate like .not. Which seems acceptable in my opinion. :)

In this case, I would indeed prefer to go with the simpler implementation since it also won't permanently pollute the relation.

/cc @jonleighton @jeremy what do you think?

Owner

jeremy commented Nov 27, 2012

Lovely ❤️ Agree with treating WhereChain as a builder object. Its responsibility is to construct a where clause and return a new relation, not to act as a relation itself. Limiting its API to not, like, etc is a positive thing.

Member

jonleighton commented Nov 27, 2012

This looks good to me. Agree about using a builder object.

Member

claudiob commented Nov 29, 2012

@amatsuda I have edited your code following the suggestion by @josevalim so WhereChain is now a class with a builder, rather than a mixin. I have also added documentation and edited the CHANGELOG. It's all in this new pull request: #8365

@josevalim I think that, after my commit, it becomes clearer how to chain those three new methods. If you agree, I would like to make it even clearer by explicitly raising an ActiveRecord::StatementInvalid if one of those methods is used without being preceded by where, e.g.:

Post.not(name: 'Jose')

Right now, the previous line would just raise a generic error saying that the method not is not defined on Post. The same for Post.like(conditions) and Post.not_like(conditions).

Finally, I'd like to draw your attention to this required change in the test: claudiob/rails@be71e1a#L2L302.
In other words, calling Post.where(nil) now is equivalent to Post.where, which is not a no-op anymore, but instead creates an instance of WhereChain to be chained with not, like, or not_like.
Is this the behavior you were looking for? Or would you rather still have Post.where(nil) equivalent to Post.where("")?

Owner

rafaelfranca commented Nov 29, 2012

Seems good to me.

@amatsuda we will need changelog entry and updates on the guides.

@josevalim @jonleighton anything else?

@carlosantoniodasilva carlosantoniodasilva commented on the diff Nov 29, 2012

...test/cases/associations/has_many_associations_test.rb
@@ -271,7 +271,7 @@ def test_finding_array_compatibility
end
def test_find_with_blank_conditions
- [[], {}, nil, ""].each do |blank|
+ [[], {}, ""].each do |blank|
@carlosantoniodasilva

carlosantoniodasilva Nov 29, 2012

Owner

If you want to prevent that to happen - changing the meaning of where(nil) - you can add a special name to the opts argument, lets say :chain, or even use false as a check instead of nil. This was done previously with exists? 359592b. Just an idea :)

Relation.where with no args can be chained with not, like, and not_like
examples:

  Model.where.not field: nil
  #=> "SELECT * FROM models WHERE field IS NOT NULL

  Model.where.like name: 'Jeremy%'
  #=> "SELECT * FROM models WHERE name LIKE 'Jeremy%'

this feature was originally suggested by Jeremy Kemper #5950 (comment)

Closes #5950
Member

amatsuda commented Nov 29, 2012

Just push -fed some fixes suggested by @josevalim and @carlosantoniodasilva, and a bit of documentations.

@claudiob Your version would not work with varargs, e.g.:
User.where.not('name = ?', 'Tenderlove')
But thank you for your help anyway :) I actually took your test fix.

Thanks @amatsuda 👍

Member

claudiob commented Nov 29, 2012

@amatsuda I thought about that, but then again, why would not support varargs, and like, not_like not?
If you can write:

User.where.not('name = ?', 'Tenderlove')  # instead of User.where.not(name: 'tenderlove')

then shouldn't you also be able to write the following?

User.where.like('name = ?', 'tender%')  # instead of User.where.like(name: 'tender%')

In other words, should not, like, and not_like parse the arguments in the same way as where does or not?
If this is the case, I suggest we add more tests to indicate all these variations.

I also think it wouldn't hurt to restore this test: claudiob/rails@be71e1a#L4R89 or at least something that makes clear what happens if you call where with blank conditions.

@carlosantoniodasilva I understand your suggestion, but before delving into coding that I would like to know what is the most desired behavior. What makes most sense to you? That where(nil) should be treated as where or as where("")? Personally, I vote for the latter, and force coders not to pass any argument at all if they want to chain where with not, like, or not_like. I don't see the case where someone would need to write:

User.where(nil).not(name: 'John')

so personally I would keep the old behavior in this case (having User.where(nil) == User.all) and only create a WhereChain if where is called with no arguments at all. What's the opinion of Rails core members?

Member

claudiob commented Dec 3, 2012

@josevalim @rafaelfranca any opinion on the comment above?

@claudiob answering your question, to me, where(nil) shouldn't change its behavior with the introduction of this feature.

About raising another/specific error, I think a NoMethodError should be clear enough, since the docs should explain that those methods should be only available chaining where with no args.

carlosantoniodasilva added a commit to carlosantoniodasilva/rails that referenced this pull request Dec 7, 2012

Merge pull request #8332 from amatsuda/ar_where_chain
Relation.where with no args can be chained with not, like, and not_like

Conflicts:
	activerecord/CHANGELOG.md
	activerecord/lib/active_record/relation/query_methods.rb

carlosantoniodasilva added a commit to carlosantoniodasilva/rails that referenced this pull request Dec 7, 2012

Ensure there won't be any regression with where(nil) calls
Consider this scenario:

    if params[:foo]
      conditions = { foo: true }
    end

    foos = Foo.where(conditions).order(:id)

When params[:foo] is nil, this would call:

    foos = Foo.where(nil).order(:id)

In this scenario, we want Foo.where(conditions) to be the same as calling
Foo.all, otherwise we'd get a "NoMethodError order for WhereChain".

Related to #8332.

@carlosantoniodasilva carlosantoniodasilva merged commit de75af7 into rails:master Dec 7, 2012

@amatsuda thank you 💚💛❤️💜💙

@claudiob please feel free to improve the test coverage with other scenarios as you suggest, would be more than welcome.

I've reverted back the where(nil) change in 6ba0f97 to avoid regressions. Let me know about any feedback, thanks!

(apparently github decided to point the linked commits only to my fork since I pushed there first, dunno why ;)

Member

claudiob commented Dec 7, 2012

@carlosantoniodasilva Using the :chain symbol as a default looks great! I just added a new pull request to have the RDOC match your change: #8445

Member

claudiob commented Dec 7, 2012

@carlosantoniodasilva I have one more doubt on this commit. Because of the code at carlosantoniodasilva/rails@23b9cc8#L1R32 , not accepts conditions in three formats: Hash, String and Array, so these lines are equivalent:

Post.where.not(title: 'hello')
Post.where.not("title = 'hello'")
Post.where.not(['title = ?', 'hello'])

My doubt (also for @amatsuda) is the following: why do we allow not to accept conditions in three formats, whereas we only allow the Hash format for like and not_like?

I think that we should keep coherence among the WhereChain methods: they should all accept the same types of parameters. Therefore, I see two options:

  1. removing support for String and Array conditions from not

  2. adding support for String and Array conditions to like and not_like.

@carlosantoniodasilva and @amatsuda what is your opinion on this?


Personally, I'd vote for option 1) since I don't see a big advantage in writing code like:

Post.where.like('name' = 'jon%')

or:

Post.where.like(['name = ?', 'jon%'])

rather than:

Post.where('name like "jon%"')

Implementing this support would also be cumbersome, since we would have to transform the equal sign in 'name' = 'jon%' into a LIKE SQL statement.

I'm not sure about removing the other types from where.not, since they're already supported without any more work, whereas adding similar support to the other methods could be harder, even though I see your point about making the methods coherent. I don't have a strong opinion on this, I think I'd rather leave them as is, but lets get more feedback on your questions.

/cc @amatsuda @rafaelfranca @josevalim @jeremy @jonleighton

Owner

rafaelfranca commented Dec 7, 2012

I think it is fine as it is.

jweslley commented Dec 7, 2012

guys, taking the opportunity, i guess it's time to add support to Arel::Nodes::GreaterThan, Arel::Nodes::GreaterThanOrEqual, Arel::Nodes::LessThan and Arel::Nodes::LessThanOrEqual using the amazing WhereChain class.

👍! Now if we can also get greater than/less than, I'll be all set.

This looks great! :)

Wondering what if combining, like
User.where("age > 20").where.not(gender: "bisexual").where.not_like(name: "Mike%")

Does this work?

Member

claudiob commented Dec 7, 2012

@rafaelfranca and @carlosantoniodasilva If you think that it's fine leaving things as they are, I suggest we document this discrepancy in the formats of conditions accepted by not, like and not_like.

I have created a pull request with this goal: #8452

Member

amatsuda commented Dec 7, 2012

@claudiob

Post.where.like('title = ?', 'hello%')

doesn't really make sense.
LIKE and = would never go together in one SQL query.

OTOH NOT + = is possible to be combined, and together it should be meaning something different from <>.
I mean,

SELECT * FROM posts WHERE title <> 'hello'

and

SELECT * FROM posts WHERE NOT (title = 'hello')

are not the same.

For more example, think about the difference between these two queries:

Post.where.not(title: 'hello', name: 'jon')
Post.where.not("title = 'hello' and name = 'jon'")

So,

these lines are equivalent

this is false. These are different, and we need both.

Member

claudiob commented Dec 7, 2012

Following comments by @jweslley and @pwightman I have added inequalities to WhereChain in this pull request: #8453

Regarding this commit, I have a couple of questions for @rafaelfranca and @carlosantoniodasilva :

  • are you okay with aliasing each method to exist both in the short and long form, e.g.: greater_than and gt? I feel that users will expect both formats
  • most of the methods inside WhereChain look similar: do you like them as they are (separate method that repeat some code), or would your rather have them refactored with some metaprogramming?

@claudiob I don't know yet if it's going to be accepted, but if it is, I think it's fine for the methods to be aliased. I also don't think there's need for refactoring through metaprogramming yet, even though there seems to be a bit of repetition, it's very clear what each piece of code does, they're very small, and we can add as much docs as we want this way :). Thanks!

Member

claudiob commented Dec 7, 2012

@amatsuda Oh, you are right! I didn't see that, because all the examples that pass conditions as a hash only have one key/value pair! Maybe we can add some documentation or test where a WhereChain methods is called with more than one key/value pair, like in your example?

Post.where.not(title: 'hello', name: 'jon')
Member

claudiob commented Dec 12, 2012

Just for reference in this pull request, support for .like and .not_like was later removed in 8d02afe

Contributor

rafbm commented Jun 2, 2013

Wouldn’t it be cleaner API-wise to just have:

Post.where(title: 'Foo').first
Post.not(title: 'Foo').first

Since like and not_like have been discarded, I don’t see the value in having where with no argument returning a WhereChain.

sgerrand pushed a commit to sgerrand/rails that referenced this pull request Nov 2, 2013

Document the types of arguments accepted by AR#not
This commit stems from rails#8332 (comment)

Since the formats in which conditions can be passed to `not` differ
from the formats in which conditions can be passed to `like` and `not_like`,
then I think it's worth adding rdoc and tests to show this behavior

@amatsuda amatsuda deleted the amatsuda:ar_where_chain branch Apr 8, 2016

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