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

[amatsuda]

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]

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

[carlosantoniodasilva]

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.

[josevalim]

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.

[amatsuda]

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

[josevalim]

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

[josevalim]

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?

[jeremy]

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.

[jonleighton]

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

[claudiob]

@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@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("")?

[rafaelfranca]

Seems good to me.

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

@josevalim @jonleighton anything else?

[carlosantoniodasilva]

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

[amatsuda]

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.

[carlosantoniodasilva]

Thanks @amatsuda 👍

[claudiob]

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

[claudiob]

@josevalim @rafaelfranca any opinion on the comment above?

[carlosantoniodasilva]

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

@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 ;)

[claudiob]

@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

[claudiob]

@carlosantoniodasilva I have one more doubt on this commit. Because of the code at carlosantoniodasilva@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.

[carlosantoniodasilva]

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

[rafaelfranca]

I think it is fine as it is.

[jweslley]

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.

[claudiob]

@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

[amatsuda]

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

[claudiob]

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?

[carlosantoniodasilva]

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

[claudiob]

@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')

[claudiob]

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

[rafbm]

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.