diff options
author | Yorick Peterse <yorickpeterse@gmail.com> | 2018-08-16 14:28:25 +0200 |
---|---|---|
committer | Yorick Peterse <yorickpeterse@gmail.com> | 2018-08-16 17:29:33 +0200 |
commit | 96ce2da74ee36e88f20cbd7ceaff2ab49f0bb223 (patch) | |
tree | 484f41cb9b2013fde409715bad4d00a602e92fb2 | |
parent | 353360ef824e10cb0197733e7b104a6fb53cf9af (diff) | |
download | gitlab-ce-96ce2da74ee36e88f20cbd7ceaff2ab49f0bb223.tar.gz |
Blacklist the use of "destroy_all"
This method usually has really bad performance implications, as it loads
rows into memory and deletes them one by one.
-rw-r--r-- | rubocop/cop/destroy_all.rb | 22 | ||||
-rw-r--r-- | rubocop/rubocop.rb | 1 | ||||
-rw-r--r-- | spec/rubocop/cop/destroy_all_spec.rb | 43 |
3 files changed, 66 insertions, 0 deletions
diff --git a/rubocop/cop/destroy_all.rb b/rubocop/cop/destroy_all.rb new file mode 100644 index 00000000000..38b6cb40f91 --- /dev/null +++ b/rubocop/cop/destroy_all.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + # Cop that blacklists the use of `destroy_all`. + class DestroyAll < RuboCop::Cop::Cop + MSG = 'Use `delete_all` instead of `destroy_all`. ' \ + '`destroy_all` will load the rows into memory, then execute a ' \ + '`DELETE` for every individual row.' + + def_node_matcher :destroy_all?, <<~PATTERN + (send {send ivar lvar const} :destroy_all ...) + PATTERN + + def on_send(node) + return unless destroy_all?(node) + + add_offense(node, location: :expression) + end + end + end +end diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb index a427208cdab..88c9bbf24f4 100644 --- a/rubocop/rubocop.rb +++ b/rubocop/rubocop.rb @@ -27,3 +27,4 @@ require_relative 'cop/project_path_helper' require_relative 'cop/rspec/env_assignment' require_relative 'cop/rspec/factories_in_migration_specs' require_relative 'cop/sidekiq_options_queue' +require_relative 'cop/destroy_all' diff --git a/spec/rubocop/cop/destroy_all_spec.rb b/spec/rubocop/cop/destroy_all_spec.rb new file mode 100644 index 00000000000..153a5a863cf --- /dev/null +++ b/spec/rubocop/cop/destroy_all_spec.rb @@ -0,0 +1,43 @@ +require 'spec_helper' +require 'rubocop' +require 'rubocop/rspec/support' +require_relative '../../../rubocop/cop/destroy_all' + +describe RuboCop::Cop::DestroyAll do + include CopHelper + + subject(:cop) { described_class.new } + + it 'flags the use of destroy_all with a send receiver' do + inspect_source('foo.destroy_all') + + expect(cop.offenses.size).to eq(1) + end + + it 'flags the use of destroy_all with a constant receiver' do + inspect_source('User.destroy_all') + + expect(cop.offenses.size).to eq(1) + end + + it 'flags the use of destroy_all when passing arguments' do + inspect_source('User.destroy_all([])') + + expect(cop.offenses.size).to eq(1) + end + + it 'flags the use of destroy_all with a local variable receiver' do + inspect_source(<<~RUBY) + users = User.all + users.destroy_all + RUBY + + expect(cop.offenses.size).to eq(1) + end + + it 'does not flag the use of delete_all' do + inspect_source('foo.delete_all') + + expect(cop.offenses).to be_empty + end +end |