From 41bbeb1d87d5948ef0fae82084e90c4acfd6b477 Mon Sep 17 00:00:00 2001
From: Peter Leitzen <pleitzen@gitlab.com>
Date: Tue, 30 May 2023 18:14:50 +0000
Subject: [PATCH] Cop/IgnoredColumns: Flag use of `self.ignored_columns =
[...]`
Previously, this cop only flagged `self.ignored_columns += [...]`.
Because of this change the cop rule is put back in "grace period" to
maintain `master` stability.
---
.rubocop_todo/cop/ignored_columns.yml | 8 +++
...e_end_string_concatenation_indentation.yml | 1 -
.rubocop_todo/naming/inclusive_language.yml | 1 -
.../rspec/missing_feature_category.yml | 1 -
rubocop/cop/ignored_columns.rb | 52 +++++++++++++------
spec/rubocop/cop/ignored_columns_spec.rb | 27 ++++++----
6 files changed, 62 insertions(+), 28 deletions(-)
create mode 100644 .rubocop_todo/cop/ignored_columns.yml
diff --git a/.rubocop_todo/cop/ignored_columns.yml b/.rubocop_todo/cop/ignored_columns.yml
new file mode 100644
index 0000000000000..eaba218d38578
--- /dev/null
+++ b/.rubocop_todo/cop/ignored_columns.yml
@@ -0,0 +1,8 @@
+---
+Cop/IgnoredColumns:
+ Details: grace period
+ Exclude:
+ - 'app/models/loose_foreign_keys/deleted_record.rb'
+ - 'ee/lib/ee/gitlab/background_migration/create_vulnerability_links.rb'
+ - 'ee/lib/ee/gitlab/background_migration/migrate_vulnerabilities_feedback_to_vulnerabilities_state_transition.rb'
+ - 'spec/lib/gitlab/database/partitioning/sliding_list_strategy_spec.rb'
diff --git a/.rubocop_todo/layout/line_end_string_concatenation_indentation.yml b/.rubocop_todo/layout/line_end_string_concatenation_indentation.yml
index d1b6eddb465d6..b7fdd3979bccb 100644
--- a/.rubocop_todo/layout/line_end_string_concatenation_indentation.yml
+++ b/.rubocop_todo/layout/line_end_string_concatenation_indentation.yml
@@ -215,7 +215,6 @@ Layout/LineEndStringConcatenationIndentation:
- 'rubocop/cop/graphql/descriptions.rb'
- 'rubocop/cop/graphql/resolver_type.rb'
- 'rubocop/cop/group_public_or_visible_to_user.rb'
- - 'rubocop/cop/ignored_columns.rb'
- 'rubocop/cop/inject_enterprise_edition_module.rb'
- 'rubocop/cop/migration/add_concurrent_index.rb'
- 'rubocop/cop/migration/add_limit_to_text_columns.rb'
diff --git a/.rubocop_todo/naming/inclusive_language.yml b/.rubocop_todo/naming/inclusive_language.yml
index 465e338bbe73d..8950e2e77f4ff 100644
--- a/.rubocop_todo/naming/inclusive_language.yml
+++ b/.rubocop_todo/naming/inclusive_language.yml
@@ -53,7 +53,6 @@ Naming/InclusiveLanguage:
- 'rubocop/cop/destroy_all.rb'
- 'rubocop/cop/graphql/id_type.rb'
- 'rubocop/cop/group_public_or_visible_to_user.rb'
- - 'rubocop/cop/ignored_columns.rb'
- 'rubocop/cop/inject_enterprise_edition_module.rb'
- 'rubocop/cop/migration/add_columns_to_wide_tables.rb'
- 'spec/controllers/concerns/issuable_collections_spec.rb'
diff --git a/.rubocop_todo/rspec/missing_feature_category.yml b/.rubocop_todo/rspec/missing_feature_category.yml
index 992295c1bcb69..f73bafcaf3c8e 100644
--- a/.rubocop_todo/rspec/missing_feature_category.yml
+++ b/.rubocop_todo/rspec/missing_feature_category.yml
@@ -5312,7 +5312,6 @@ RSpec/MissingFeatureCategory:
- 'spec/rubocop/cop/graphql/old_types_spec.rb'
- 'spec/rubocop/cop/graphql/resolver_type_spec.rb'
- 'spec/rubocop/cop/group_public_or_visible_to_user_spec.rb'
- - 'spec/rubocop/cop/ignored_columns_spec.rb'
- 'spec/rubocop/cop/include_sidekiq_worker_spec.rb'
- 'spec/rubocop/cop/inject_enterprise_edition_module_spec.rb'
- 'spec/rubocop/cop/migration/add_columns_to_wide_tables_spec.rb'
diff --git a/rubocop/cop/ignored_columns.rb b/rubocop/cop/ignored_columns.rb
index 542d78c59e959..8b17447c46b7a 100644
--- a/rubocop/cop/ignored_columns.rb
+++ b/rubocop/cop/ignored_columns.rb
@@ -2,40 +2,60 @@
module RuboCop
module Cop
- # Cop that blacklists the usage of `ActiveRecord::Base.ignored_columns=` directly
+ # Cop that flags the usage of `ActiveRecord::Base.ignored_columns=` directly
+ #
+ # @example
+ # # bad
+ # class User < ApplicationRecord
+ # self.ignored_columns = [:name]
+ # self.ignored_columns += [:full_name]
+ # end
+ #
+ # # good
+ # class User < ApplicationRecord
+ # include IgnorableColumns
+ #
+ # ignore_column :name, remove_after: '2023-05-22', remove_with: '16.0'
+ # ignore_column :full_name, remove_after: '2023-05-22', remove_with: '16.0'
+ # end
class IgnoredColumns < RuboCop::Cop::Base
- USE_CONCERN_MSG = 'Use `IgnoredColumns` concern instead of adding to `self.ignored_columns`.'
- WRONG_MODEL_MSG = 'If the model exists in CE and EE, the column has to be ignored ' \
- 'in the CE model. If the model only exists in EE, then it has to be added there.'
+ USE_CONCERN_ADD_MSG = 'Use `IgnoredColumns` concern instead of adding to `self.ignored_columns`.'
+ USE_CONCERN_SET_MSG = 'Use `IgnoredColumns` concern instead of setting `self.ignored_columns`.'
+ WRONG_MODEL_MSG = <<~MSG
+ If the model exists in CE and EE, the column has to be ignored
+ in the CE model. If the model only exists in EE, then it has to be added there.
+ MSG
- def_node_matcher :ignored_columns?, <<~PATTERN
+ RESTRICT_ON_SEND = %i[ignored_columns ignored_columns= ignore_column ignore_columns].freeze
+
+ def_node_matcher :ignored_columns_add?, <<~PATTERN
(send (self) :ignored_columns)
PATTERN
- def_node_matcher :ignore_columns?, <<~PATTERN
- (send nil? :ignore_columns ...)
+ def_node_matcher :ignored_columns_set?, <<~PATTERN
+ (send (self) :ignored_columns= ...)
PATTERN
- def_node_matcher :ignore_column?, <<~PATTERN
- (send nil? :ignore_column ...)
+ def_node_matcher :using_ignore_columns?, <<~PATTERN
+ (send nil? {:ignore_columns :ignore_column}...)
PATTERN
def on_send(node)
- if ignored_columns?(node)
- add_offense(node, message: USE_CONCERN_MSG)
+ if ignored_columns_add?(node)
+ add_offense(node.loc.selector, message: USE_CONCERN_ADD_MSG)
+ end
+
+ if ignored_columns_set?(node)
+ add_offense(node.loc.selector, message: USE_CONCERN_SET_MSG)
end
- if using_ignore?(node) && used_in_wrong_model?
+ if using_ignore_columns?(node) && used_in_wrong_model?
add_offense(node, message: WRONG_MODEL_MSG)
end
end
private
- def using_ignore?(node)
- ignore_columns?(node) || ignore_column?(node)
- end
-
def used_in_wrong_model?
file_path = processed_source.file_path
diff --git a/spec/rubocop/cop/ignored_columns_spec.rb b/spec/rubocop/cop/ignored_columns_spec.rb
index c6c44399624b8..8d2c6b92c7098 100644
--- a/spec/rubocop/cop/ignored_columns_spec.rb
+++ b/spec/rubocop/cop/ignored_columns_spec.rb
@@ -3,12 +3,21 @@
require 'rubocop_spec_helper'
require_relative '../../../rubocop/cop/ignored_columns'
-RSpec.describe RuboCop::Cop::IgnoredColumns do
- it 'flags direct use of ignored_columns instead of the IgnoredColumns concern' do
+RSpec.describe RuboCop::Cop::IgnoredColumns, feature_category: :database do
+ it 'flags use of `self.ignored_columns +=` instead of the IgnoredColumns concern' do
expect_offense(<<~RUBY)
class Foo < ApplicationRecord
self.ignored_columns += %i[id]
- ^^^^^^^^^^^^^^^^^^^^ Use `IgnoredColumns` concern instead of adding to `self.ignored_columns`.
+ ^^^^^^^^^^^^^^^ Use `IgnoredColumns` concern instead of adding to `self.ignored_columns`.
+ end
+ RUBY
+ end
+
+ it 'flags use of `self.ignored_columns =` instead of the IgnoredColumns concern' do
+ expect_offense(<<~RUBY)
+ class Foo < ApplicationRecord
+ self.ignored_columns = %i[id]
+ ^^^^^^^^^^^^^^^ Use `IgnoredColumns` concern instead of setting `self.ignored_columns`.
end
RUBY
end
@@ -16,7 +25,7 @@ class Foo < ApplicationRecord
context 'when only CE model exist' do
let(:file_path) { full_path('app/models/bar.rb') }
- it 'does not flag ignore_columns usage in CE model' do
+ it 'does not flag `ignore_columns` usage in CE model' do
expect_no_offenses(<<~RUBY, file_path)
class Bar < ApplicationRecord
ignore_columns :foo, remove_with: '14.3', remove_after: '2021-09-22'
@@ -24,7 +33,7 @@ class Bar < ApplicationRecord
RUBY
end
- it 'flags ignore_column usage in EE model' do
+ it 'does not flag `ignore_column` usage in CE model' do
expect_no_offenses(<<~RUBY, file_path)
class Baz < ApplicationRecord
ignore_column :bar, remove_with: '14.3', remove_after: '2021-09-22'
@@ -40,7 +49,7 @@ class Baz < ApplicationRecord
allow(File).to receive(:exist?).with(full_path('app/models/bar.rb')).and_return(false)
end
- it 'flags ignore_columns usage in EE model' do
+ it 'does not flag `ignore_columns` usage in EE model' do
expect_no_offenses(<<~RUBY, file_path)
class Bar < ApplicationRecord
ignore_columns :foo, remove_with: '14.3', remove_after: '2021-09-22'
@@ -48,7 +57,7 @@ class Bar < ApplicationRecord
RUBY
end
- it 'flags ignore_column usage in EE model' do
+ it 'does not flag `ignore_column` usage in EE model' do
expect_no_offenses(<<~RUBY, file_path)
class Bar < ApplicationRecord
ignore_column :foo, remove_with: '14.3', remove_after: '2021-09-22'
@@ -64,7 +73,7 @@ class Bar < ApplicationRecord
allow(File).to receive(:exist?).with(full_path('app/models/bar.rb')).and_return(true)
end
- it 'flags ignore_columns usage in EE model' do
+ it 'flags `ignore_columns` usage in EE model' do
expect_offense(<<~RUBY, file_path)
class Bar < ApplicationRecord
ignore_columns :foo, remove_with: '14.3', remove_after: '2021-09-22'
@@ -73,7 +82,7 @@ class Bar < ApplicationRecord
RUBY
end
- it 'flags ignore_column usage in EE model' do
+ it 'flags `ignore_column` usage in EE model' do
expect_offense(<<~RUBY, file_path)
class Bar < ApplicationRecord
ignore_column :foo, remove_with: '14.3', remove_after: '2021-09-22'
--
GitLab