Skip to content
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

Add new Rails/ActiveRecordCalculation cop #1371

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
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/new_active_record_calculation_cop.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#1205](https://github.com/rubocop/rubocop-rails/issues/1205): Add new `Rails/ActiveRecordCalculation` cop. ([@fatkodima][])
5 changes: 5 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,11 @@ Rails/ActiveRecordAliases:
VersionAdded: '0.53'
SafeAutoCorrect: false

Rails/ActiveRecordCalculation:
Description: 'Use ActiveRecord calculation methods instead of `pluck` followed by Enumerable methods.'
Enabled: pending
VersionAdded: '<<next>>'
Copy link
Contributor

Choose a reason for hiding this comment

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

This would need to be marked unsafe because Ruby's sum/max aren't always compatible with the DB's:

User.pluck(:name).sum # "AliceBobCathy"
User.sum(:name) # PG::UndefinedFunction: ERROR:  function sum(character varying) does not exist


Rails/ActiveRecordCallbacksOrder:
Description: 'Order callback declarations in the order in which they will be executed.'
StyleGuide: 'https://rails.rubystyle.guide/#callbacks-order'
Expand Down
67 changes: 67 additions & 0 deletions lib/rubocop/cop/rails/active_record_calculation.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
# frozen_string_literal: true

module RuboCop
module Cop
module Rails
# Enforces the use of ActiveRecord calculation methods instead of `pluck`
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add the rationale here: it avoids loading potentially many values into memory by doing the calculations through the equivalent database function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea 👍

# followed by Enumerable methods.
#
# It avoids loading potentially many values into memory by doing the
# calculations inside the database.
#
# @example
# # bad
# User.pluck(:id).max
# User.pluck(:id).min
# User.pluck(:age).sum
#
# # good
# User.maximum(:id)
# User.minimum(:id)
# User.sum(:age)
#
# # good
# User.pluck(:email).max { |email| email.length }
# User.pluck(:email).max(2)
# User.pluck(:id, :company_id).max
# User.pluck(:age).count
Copy link
Contributor

Choose a reason for hiding this comment

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

This is kinda a weird counterexample, because User.count(:age) would actually be more efficient and falls in line with this cop's premise. 🤔

#
class ActiveRecordCalculation < Base
include RangeHelp
extend AutoCorrector

MSG = 'Use `%<good_method>s` instead of `pluck.%<bad_method>s`.'

RESTRICT_ON_SEND = %i[pluck].freeze
OPERATIONS_MAP = { min: :minimum, max: :maximum, sum: :sum }.freeze

def_node_matcher :pluck_calculation?, <<~PATTERN
(send
(send _ :pluck $_)
${:#{OPERATIONS_MAP.keys.join(' :')}})
PATTERN

def on_send(node)
return unless (parent = node.parent)
return if send_with_block?(parent)

pluck_calculation?(parent) do |arg_node, calculation|
good_method = OPERATIONS_MAP.fetch(calculation)
message = format(MSG, good_method: good_method, bad_method: calculation)
offense_range = range_between(node.loc.selector.begin_pos, parent.source_range.end_pos)

add_offense(offense_range, message: message) do |corrector|
corrector.replace(offense_range, "#{good_method}(#{arg_node.source})")
end
end
end

private

def send_with_block?(node)
node.parent&.block_type? && node.parent.send_node == node
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 @@ -14,6 +14,7 @@
require_relative 'rails/action_filter'
require_relative 'rails/action_order'
require_relative 'rails/active_record_aliases'
require_relative 'rails/active_record_calculation'
require_relative 'rails/active_record_callbacks_order'
require_relative 'rails/active_record_override'
require_relative 'rails/active_support_aliases'
Expand Down
82 changes: 82 additions & 0 deletions spec/rubocop/cop/rails/active_record_calculation_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Rails::ActiveRecordCalculation, :config do
it 'registers an offense and corrects when using `pluck.max`' do
expect_offense(<<~RUBY)
Model.pluck(:column).max
^^^^^^^^^^^^^^^^^^ Use `maximum` instead of `pluck.max`.
RUBY

expect_correction(<<~RUBY)
Model.maximum(:column)
RUBY
end

it 'registers an offense and corrects when using `pluck.min`' do
expect_offense(<<~RUBY)
Model.pluck(:column).min
^^^^^^^^^^^^^^^^^^ Use `minimum` instead of `pluck.min`.
RUBY

expect_correction(<<~RUBY)
Model.minimum(:column)
RUBY
end

it 'registers an offense and corrects when using `pluck.sum`' do
expect_offense(<<~RUBY)
Model.pluck(:column).sum
^^^^^^^^^^^^^^^^^^ Use `sum` instead of `pluck.sum`.
RUBY

expect_correction(<<~RUBY)
Model.sum(:column)
RUBY
end

it 'registers an offense and corrects when using `pluck.max` without receiver' do
expect_offense(<<~RUBY)
pluck(:column).max
^^^^^^^^^^^^^^^^^^ Use `maximum` instead of `pluck.max`.
RUBY

expect_correction(<<~RUBY)
maximum(:column)
RUBY
end

it 'registers an offense and corrects when using `pluck.max` with non-literal column' do
expect_offense(<<~RUBY)
Model.pluck(column).max
^^^^^^^^^^^^^^^^^ Use `maximum` instead of `pluck.max`.
RUBY

expect_correction(<<~RUBY)
Model.maximum(column)
RUBY
end

it 'does not register an offense when using `pluck.max` with multiple arguments' do
expect_no_offenses(<<~RUBY)
Model.pluck(:column1, :column2).max
RUBY
end

it 'does not register an offense when using `pluck.max` with block' do
expect_no_offenses(<<~RUBY)
Model.pluck(:column).max { |e| e }
RUBY
end

it 'does not register an offense when using `pluck.max` and `max` has argument' do
expect_no_offenses(<<~RUBY)
Model.pluck(:column).max(1)
RUBY
end

it 'does not register an offense when using `maximum`' do
expect_no_offenses(<<~RUBY)
Model.maximum(:column)
RUBY
end
end