Skip to content

Commit

Permalink
[Fix #1016] Add Rails/RedundantAll cop
Browse files Browse the repository at this point in the history
  • Loading branch information
masato-bkn committed Aug 11, 2023
1 parent fc1be54 commit e6a3ff5
Show file tree
Hide file tree
Showing 5 changed files with 343 additions and 0 deletions.
1 change: 1 addition & 0 deletions changelog/fix_add_rails_redundant_all_cop.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/RedundantAll` cop. ([@masato-bkn][])
6 changes: 6 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -775,6 +775,12 @@ Rails/ReadWriteAttribute:
Include:
- app/models/**/*.rb

Rails/RedundantAll:
Description: Detect redundant `all` used as a receiver for Active Record query methods.
Enabled: true
Safe: false
VersionAdded: "<<next>>"

Rails/RedundantAllowNil:
Description: >-
Finds redundant use of `allow_nil` when `allow_blank` is set to
Expand Down
143 changes: 143 additions & 0 deletions lib/rubocop/cop/rails/redundant_all.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
# 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 if the receiver object is not an Active Record object.
#
# @example
# # bad
# User.all.order(:created_at)
# User.all.find(id)
# User.all.where(id: ids)
#
# # good
# User.order(:created_at)
# User.find(id)
# User.where(id: ids)
class RedundantAll < 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)

all_range = node.loc.selector
add_offense(all_range) do |collector|
collector.remove(all_range)
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_all'
require_relative 'rails/redundant_allow_nil'
require_relative 'rails/redundant_foreign_key'
require_relative 'rails/redundant_presence_validation_on_belongs_to'
Expand Down
192 changes: 192 additions & 0 deletions spec/rubocop/cop/rails/redundant_all_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,192 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Rails::RedundantAll, :config do
sources = [
'and(User.where(age: 30))',
'annotate("selecting id")',
'any?',
'average(:age)',
'calculate(:average, :age)',
'count',
'create_or_find_by(name: name)',
'create_or_find_by!(name: name)',
'create_with(name: name)',
'delete_all',
'delete_by(id: id)',
'destroy_all',
'destroy_by(id: id)',
'distinct',
'eager_load(:posts)',
'except(:order)',
'excluding(user)',
'exists?',
'extending(Pagination)',
'extract_associated(:posts)',
'fifth',
'fifth!',
'find(id)',
'find_by(name: name)',
'find_by!(name: name)',
'find_each(&:do_something)',
'find_in_batches(&:do_something)',
'find_or_create_by(name: name)',
'find_or_create_by!(name: name)',
'find_or_initialize_by(name: name)',
'find_sole_by(name: name)',
'first',
'first!',
'first_or_create(name: name)',
'first_or_create!(name: name)',
'first_or_initialize(name: name)',
'forty_two',
'forty_two!',
'fourth',
'fourth!',
'from("users")',
'group(:age)',
'having("AVG(age) > 30")',
'ids',
'in_batches(&:do_something)',
'in_order_of(:id, ids)',
'includes(:posts)',
'invert_where',
'joins(:posts)',
'last',
'last!',
'left_joins(:posts)',
'left_outer_joins(:posts)',
'limit(n)',
'lock',
'many?',
'maximum(:age)',
'merge(users)',
'minimum(:age)',
'none',
'none?',
'offset(n)',
'one?',
'only(:order)',
'optimizer_hints("SeqScan(users)", "Parallel(users 8)")',
'or(User.where(age: 30))',
'order(:created_at)',
'pick(:id)',
'pluck(:age)',
'preload(:posts)',
'readonly',
'references(:posts)',
'reorder(:created_at)',
'reselect(:age)',
'rewhere(id: ids)',
'second',
'second!',
'second_to_last',
'second_to_last!',
'select(:age)',
'sole',
'strict_loading',
'sum(:age)',
'take(n)',
'take!',
'third',
'third!',
'third_to_last',
'third_to_last!',
'touch_all',
'unscope(:order)',
'update_all(name: name)',
'where(id: ids)',
'without(user)'
]

describe '::QUERYING_METHODS' do
it 'contains all of the querying methods' do
methods = sources.map { |s| s.split('(')[0].to_sym }
expect(described_class::QUERYING_METHODS).to eq(methods)
end
end

context 'with receiver' do
sources.each do |source|
it "registers an offense and corrects when `all.#{source}`" do
expect_offense(<<~RUBY)
User.all.#{source}
^^^ Redundant `all` detected
RUBY

expect_correction(<<~RUBY)
User.#{source}
RUBY
end
end

it 'does not register an offense when not using any method' do
expect_no_offenses(<<~RUBY)
User.all
RUBY
end

it 'does not register an offense when not using defined methods in ::QUERYING_METHODS' do
expect_no_offenses(<<~RUBY)
User.all.map(&:do_something)
RUBY
end
end

context 'with no receiver' do
it 'does not register an offense when not inheriting any class' do
expect_no_offenses(<<~RUBY)
class User
def do_something
all.where(id: ids)
end
end
RUBY
end

it 'does not register an offense when not inheriting `ApplicationRecord`' do
expect_no_offenses(<<~RUBY)
class User < Foo
def do_something
all.where(id: ids)
end
end
RUBY
end

it 'registers an offense when inheriting `ApplicationRecord`' do
expect_offense(<<~RUBY)
class User < ApplicationRecord
scope :admins, -> { all.where(admin: true) }
^^^ Redundant `all` detected
end
RUBY
end

it 'registers an offense when inheriting `::ApplicationRecord`' do
expect_offense(<<~RUBY)
class User < ::ApplicationRecord
scope :admins, -> { all.where(admin: true) }
^^^ Redundant `all` detected
end
RUBY
end

it 'registers an offense when inheriting `ActiveRecord::Base`' do
expect_offense(<<~RUBY)
class User < ActiveRecord::Base
scope :admins, -> { all.where(admin: true) }
^^^ Redundant `all` detected
end
RUBY
end

it 'registers an offense when inheriting `::ActiveRecord::Base`' do
expect_offense(<<~RUBY)
class User < ::ActiveRecord::Base
scope :admins, -> { all.where(admin: true) }
^^^ Redundant `all` detected
end
RUBY
end
end
end

0 comments on commit e6a3ff5

Please sign in to comment.