From 4bb269eee0839a56f1b2e84b2802c74fa5a79bcd Mon Sep 17 00:00:00 2001
From: Maxime Orefice <morefice@gitlab.com>
Date: Fri, 3 Sep 2021 14:27:28 +0000
Subject: [PATCH] Extend PreventIndexCreation cop
This commit extends our existing cop to catch false positives
when the table_name is a string or a constant.
---
.../cop/migration/prevent_index_creation.rb | 33 ++++++--
.../migration/prevent_index_creation_spec.rb | 77 ++++++++++++++++---
2 files changed, 94 insertions(+), 16 deletions(-)
diff --git a/rubocop/cop/migration/prevent_index_creation.rb b/rubocop/cop/migration/prevent_index_creation.rb
index 1486607acbe8c..aeeec36ecf0fc 100644
--- a/rubocop/cop/migration/prevent_index_creation.rb
+++ b/rubocop/cop/migration/prevent_index_creation.rb
@@ -12,16 +12,29 @@ class PreventIndexCreation < RuboCop::Cop::Cop
MSG = "Adding new index to #{FORBIDDEN_TABLES.join(", ")} is forbidden, see https://gitlab.com/gitlab-org/gitlab/-/issues/332886"
+ def on_new_investigation
+ super
+ @forbidden_tables_used = false
+ end
+
def_node_matcher :add_index?, <<~PATTERN
- (send nil? :add_index (sym #forbidden_tables?) ...)
+ (send nil? :add_index ({sym|str} #forbidden_tables?) ...)
PATTERN
def_node_matcher :add_concurrent_index?, <<~PATTERN
- (send nil? :add_concurrent_index (sym #forbidden_tables?) ...)
+ (send nil? :add_concurrent_index ({sym|str} #forbidden_tables?) ...)
PATTERN
- def forbidden_tables?(node)
- FORBIDDEN_TABLES.include?(node)
+ def_node_matcher :forbidden_constant_defined?, <<~PATTERN
+ (casgn nil? _ ({sym|str} #forbidden_tables?))
+ PATTERN
+
+ def_node_matcher :add_concurrent_index_with_constant?, <<~PATTERN
+ (send nil? :add_concurrent_index (const nil? _) ...)
+ PATTERN
+
+ def on_casgn(node)
+ @forbidden_tables_used = !!forbidden_constant_defined?(node)
end
def on_def(node)
@@ -32,8 +45,18 @@ def on_def(node)
end
end
+ private
+
+ def forbidden_tables?(node)
+ FORBIDDEN_TABLES.include?(node.to_sym)
+ end
+
def offense?(node)
- add_index?(node) || add_concurrent_index?(node)
+ add_index?(node) || add_concurrent_index?(node) || any_constant_used_with_forbidden_tables?(node)
+ end
+
+ def any_constant_used_with_forbidden_tables?(node)
+ add_concurrent_index_with_constant?(node) && @forbidden_tables_used
end
end
end
diff --git a/spec/rubocop/cop/migration/prevent_index_creation_spec.rb b/spec/rubocop/cop/migration/prevent_index_creation_spec.rb
index aa1779a8cd563..b5bb770553a31 100644
--- a/spec/rubocop/cop/migration/prevent_index_creation_spec.rb
+++ b/spec/rubocop/cop/migration/prevent_index_creation_spec.rb
@@ -15,22 +15,63 @@
end
context 'when adding an index to a forbidden table' do
- it "registers an offense when add_index is used", :aggregate_failures do
- forbidden_tables.each do |table|
- expect_offense(<<~RUBY)
- def change
- add_index :#{table}, :protected
- ^^^^^^^^^ Adding new index to #{forbidden_tables_list} is forbidden, see https://gitlab.com/gitlab-org/gitlab/-/issues/332886
- end
- RUBY
+ context 'when table_name is a symbol' do
+ it "registers an offense when add_index is used", :aggregate_failures do
+ forbidden_tables.each do |table_name|
+ expect_offense(<<~RUBY)
+ def change
+ add_index :#{table_name}, :protected
+ ^^^^^^^^^ Adding new index to #{forbidden_tables_list} is forbidden, see https://gitlab.com/gitlab-org/gitlab/-/issues/332886
+ end
+ RUBY
+ end
+ end
+
+ it "registers an offense when add_concurrent_index is used", :aggregate_failures do
+ forbidden_tables.each do |table_name|
+ expect_offense(<<~RUBY)
+ def change
+ add_concurrent_index :#{table_name}, :protected
+ ^^^^^^^^^^^^^^^^^^^^ Adding new index to #{forbidden_tables_list} is forbidden, see https://gitlab.com/gitlab-org/gitlab/-/issues/332886
+ end
+ RUBY
+ end
end
end
- it "registers an offense when add_concurrent_index is used", :aggregate_failures do
- forbidden_tables.each do |table|
+ context 'when table_name is a string' do
+ it "registers an offense when add_index is used", :aggregate_failures do
+ forbidden_tables.each do |table_name|
+ expect_offense(<<~RUBY)
+ def change
+ add_index "#{table_name}", :protected
+ ^^^^^^^^^ Adding new index to #{forbidden_tables_list} is forbidden, see https://gitlab.com/gitlab-org/gitlab/-/issues/332886
+ end
+ RUBY
+ end
+ end
+
+ it "registers an offense when add_concurrent_index is used", :aggregate_failures do
+ forbidden_tables.each do |table_name|
+ expect_offense(<<~RUBY)
+ def change
+ add_concurrent_index "#{table_name}", :protected
+ ^^^^^^^^^^^^^^^^^^^^ Adding new index to #{forbidden_tables_list} is forbidden, see https://gitlab.com/gitlab-org/gitlab/-/issues/332886
+ end
+ RUBY
+ end
+ end
+ end
+
+ context 'when table_name is a constant' do
+ it "registers an offense when add_concurrent_index is used", :aggregate_failures do
expect_offense(<<~RUBY)
+ INDEX_NAME = "index_name"
+ TABLE_NAME = :ci_builds
+ disable_ddl_transaction!
+
def change
- add_concurrent_index :#{table}, :protected
+ add_concurrent_index TABLE_NAME, :protected
^^^^^^^^^^^^^^^^^^^^ Adding new index to #{forbidden_tables_list} is forbidden, see https://gitlab.com/gitlab-org/gitlab/-/issues/332886
end
RUBY
@@ -46,6 +87,20 @@ def change
end
RUBY
end
+
+ context 'when using a constant' do
+ it 'does not register an offense' do
+ expect_no_offenses(<<~RUBY)
+ disable_ddl_transaction!
+
+ TABLE_NAME = "not_forbidden"
+
+ def up
+ add_concurrent_index TABLE_NAME, :protected
+ end
+ RUBY
+ end
+ end
end
end
--
GitLab