rubocop / rails-style-guide

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

Add section for `where.not` with multiple attributes #296

Closed tejasbubane closed 1 year ago

tejasbubane commented 2 years ago

Issue:https://github.com/rubocop/rubocop-rails/issues/565

pirj commented 2 years ago

I find the original reasoning confusing:

Expected behavior NOT(A AND B) = NOT(A) OR NOT(B)

To me, both where.not(a: 1, b: 2) and where.not(a: 1).where.not(b: 2) should be NOT(A) AND NOT(B).

I agree with the bad example:

# bad
User.where.not(status: "active", plan: "basic")

But I don't agree with the message involving boolean algebra reasoning. I'd say:

Avoid passing multiple attributes to a where.not, as Rails logic in this case has changed in Rails 6.1 and will now yield results matching either of those conditions, e.g. where.not(status: 'active', plan: 'basic') would return records with active status when the plan is business.

@andyw8 WDYT?

andyw8 commented 2 years ago

Personally I would avoid not except for trivial cases with a single attribute, but I also wouldn't use a chained not as it can still be highly confusing. Instead I would write it as SQL to be explicit about the intent.

bbatsov commented 2 years ago

So, what are we doing about this? I, myself, am ambivalent on this topic.

pirj commented 2 years ago

I side with @andyw8 and would recommend not using where.not with multiple attributes or chained.

The few real-world usages of where.not with multiple attributes I could find in real-world-rspec repos:

open-source-billing/app/helpers/application_helper.rb
415:    PublicActivity::Activity.where.not(owner_id: current_user.id, key: 'client.update').where(is_read: false).count
423:    PublicActivity::Activity.where.not(owner_id: current_user.id, key: 'client.update').order('created_at desc').page(1).per(10) if current_user.present?

open-source-billing/app/controllers/notifications_controller.rb
3:    @activities = PublicActivity::Activity.where.not(owner_id: current_user.id, key: 'client.update').order('created_at desc').page(params[:page].to_i + 1).per(10) if current_use
r.present?

gitlabhq/app/models/concerns/milestoneable.rb
22:    scope :without_particular_release, -> (tag, project_id) { joins_milestone_releases.where.not(milestones: { releases: { tag: tag, project_id: project_id } }) }

I have no confidence regarding chained where.not(...).where.not(...) calls, but I imagine there should be even less of them in practice.

WDYT of discouraging using it? @tejasbubane Would you adjust the PR?

pirj commented 2 years ago

Ping @tejasbubane

pirj commented 2 years ago

Ping

pirj commented 2 years ago

Ping @tejasbubane

tejasbubane commented 1 year ago

@pirj Sorry for the long delay here, I've updated as per suggestions.

koic commented 1 year ago

Thanks!