Skip to content

[Fix #1016] Add Rails/RedundantActiveRecordAllMethod cop #1020

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

Merged
merged 1 commit into from
Aug 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog/fix_add_redundant_active_record_all_method.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#1016](https://github.com/rubocop/rubocop-rails/issues/1016): Add `Rails/RedundantActiveRecordAllMethod` cop. ([@masato-bkn][])
7 changes: 7 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -775,6 +775,13 @@ Rails/ReadWriteAttribute:
Include:
- app/models/**/*.rb

Rails/RedundantActiveRecordAllMethod:
Description: Detect redundant `all` used as a receiver for Active Record query methods.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rubocop/rails-style-guide#340 has been merged. Can you add the Style Guide URL and squash your commits into one?

Suggested change
Description: Detect redundant `all` used as a receiver for Active Record query methods.
Description: Detect redundant `all` used as a receiver for Active Record query methods.
StyleGuide: 'https://rails.rubystyle.guide/#redundant-all'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've addressed it and squashed the commits!

StyleGuide: 'https://rails.rubystyle.guide/#redundant-all'
Enabled: pending
Safe: false
VersionAdded: "<<next>>"

Rails/RedundantAllowNil:
Description: >-
Finds redundant use of `allow_nil` when `allow_blank` is set to
Expand Down
145 changes: 145 additions & 0 deletions lib/rubocop/cop/rails/redundant_active_record_all_method.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
# frozen_string_literal: true

module RuboCop
module Cop
module Rails
# Detect redundant `all` used as a receiver for Active Record query methods.
#
# @safety
# This cop is unsafe for autocorrection if the receiver for `all` is not an Active Record object.
#
# @example
# # bad
# User.all.find(id)
# User.all.order(:created_at)
# users.all.where(id: ids)
# user.articles.all.order(:created_at)
#
# # good
# User.find(id)
# User.order(:created_at)
# users.where(id: ids)
# user.articles.order(:created_at)
class RedundantActiveRecordAllMethod < Base
include ActiveRecordHelper
extend AutoCorrector

MSG = 'Redundant `all` detected.'

RESTRICT_ON_SEND = [:all].freeze

# Defined methods in `ActiveRecord::Querying::QUERYING_METHODS` on activerecord 7.0.5.
QUERYING_METHODS = %i[
and
annotate
any?
average
calculate
count
create_or_find_by
create_or_find_by!
create_with
delete_all
delete_by
destroy_all
destroy_by
distinct
eager_load
except
excluding
exists?
extending
extract_associated
fifth
fifth!
find
find_by
find_by!
find_each
find_in_batches
find_or_create_by
find_or_create_by!
find_or_initialize_by
find_sole_by
first
first!
first_or_create
first_or_create!
first_or_initialize
forty_two
forty_two!
fourth
fourth!
from
group
having
ids
in_batches
in_order_of
includes
invert_where
joins
last
last!
left_joins
left_outer_joins
limit
lock
many?
maximum
merge
minimum
none
none?
offset
one?
only
optimizer_hints
or
order
pick
pluck
preload
readonly
references
reorder
reselect
rewhere
second
second!
second_to_last
second_to_last!
select
sole
strict_loading
sum
take
take!
third
third!
third_to_last
third_to_last!
touch_all
unscope
update_all
where
without
].freeze

def on_send(node)
query_node = node.parent

return unless query_node&.send_type?
return unless QUERYING_METHODS.include?(query_node.method_name)
return if node.receiver.nil? && !inherit_active_record_base?(node)

range_of_all_method = node.loc.selector
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should also be able to handle the following cases:
rubocop/rails-style-guide#340 (comment)

Copy link
Contributor Author

@masato-bkn masato-bkn Aug 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've already addressed cases where an Active Record query method follows all, as shown below:

# bad
current_user.likes.all.order(:created_at)

# good
current_user.likes.order(:created_at)

https://github.com/rubocop/rubocop-rails/pull/1020/files#diff-8d2292cab6c23118ba7721e07148d49a27043e9202c75084bd49b25053a1575eR216

Should I also consider addressing cases where the receiver is a relation and no method follows all? Like this:

# bad
current_user.likes.all

# good
current_user.likes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, this is my mistake. No problem!

add_offense(range_of_all_method) do |collector|
collector.remove(range_of_all_method)
collector.remove(query_node.loc.dot)
end
end
end
end
end
end
1 change: 1 addition & 0 deletions lib/rubocop/cop/rails_cops.rb
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@
require_relative 'rails/present'
require_relative 'rails/rake_environment'
require_relative 'rails/read_write_attribute'
require_relative 'rails/redundant_active_record_all_method'
require_relative 'rails/redundant_allow_nil'
require_relative 'rails/redundant_foreign_key'
require_relative 'rails/redundant_presence_validation_on_belongs_to'
Expand Down
Loading