rubocop / rails-style-guide

A community-driven Ruby on Rails style guide
http://rails.rubystyle.guide
6.47k stars 1.06k forks source link

Where with Ranges: "good" vs "bad" are not equivalent #321

Closed justindell closed 2 years ago

justindell commented 2 years ago

Using an infinite range actually results in an inclusive query most of the time. Mirroring the documentation (using a "Client" model since my example app doesn't have "User")

Client.where("created_at > ?", 7.days.ago).to_sql
# "SELECT \"clients\".* FROM \"clients\" WHERE (created_at > '2022-06-24 11:00:58.795520')"
Client.where(created_at: 7.days.ago..).to_sql
# "SELECT \"clients\".* FROM \"clients\" WHERE \"clients\".\"created_at\" >= '2022-06-24 11:00:50.103960'"

Notice the second query uses >= instead of >. Curiously, rails doesn't seem to be consistent between .. and ...

Client.where(created_at: 7.days.ago..).to_sql
# "SELECT \"clients\".* FROM \"clients\" WHERE \"clients\".\"created_at\" >= '2022-06-24 11:06:22.127959'"
Client.where(created_at: 7.days.ago...).to_sql
# "SELECT \"clients\".* FROM \"clients\" WHERE \"clients\".\"created_at\" >= '2022-06-24 11:06:25.876704'"
Client.where(created_at: ..7.days.ago).to_sql
# "SELECT \"clients\".* FROM \"clients\" WHERE \"clients\".\"created_at\" <= '2022-06-24 11:06:32.801216'"
Client.where(created_at: ...7.days.ago).to_sql
# "SELECT \"clients\".* FROM \"clients\" WHERE \"clients\".\"created_at\" < '2022-06-24 11:06:35.632238'"

I don't know if this is ActiveRecord's intended behavior, but I avoid using infinite ranges because of this inconsistency.

pirj commented 2 years ago

Thanks for reporting!

(...2).exclude_end? # => true
(..2).exclude_end? # => false
(2...).exclude_end? # => true
(2..).exclude_end? # => false

Rails makes no difference between 7.days.ago.. and 7.days.ago..., and it seems to be a bug. Do you have the same impression?

Since we're recommending this as a replacement in our examples:

# bad
User.where("created_at > ?", 7.days.ago)

# good
User.where(created_at: 7.days.ago..)

and it's actually incorrect, would you like to send a PR with correct examples and maybe more details?

justindell commented 2 years ago

It looks like this is intended behavior, because ranges are inclusive or exclusive based on their ending and beginnings are always inclusive.

There's some discussion here: https://github.com/rails/rails/pull/40628

You can get the intended behavior by using where.not, but I find that to be confusing and counter-intuitive

 Client.where.not(id: ..7).to_sql
# "SELECT \"clients\".* FROM \"clients\" WHERE \"clients\".\"id\" > 7"

I'll work on a PR to make this behavior clearer, but personally I think I'll stick with ranges only with defined start's and end's. With endless ranges, I think the comparative conditions format is more clear and less prone to unexpected bugs.

pirj commented 2 years ago

You're right! I've completely missed that there is no difference between an inclusive and exclusive endless ranges, TIL. Thanks for the interesting reference.