Add latest changes from gitlab-org/gitlab@14-3-stable-eev14.3.0-rc42

8 files changed, 444 insertions, 21 deletions

diff --git a/spec/rubocop/cop/gitlab/mark_used_feature_flags_spec.rb b/spec/rubocop/cop/gitlab/mark_used_feature_flags_spec.rb
index 35b21477d80..6b5b07fb357 100644
--- a/spec/rubocop/cop/gitlab/mark_used_feature_flags_spec.rb
+++ b/spec/rubocop/cop/gitlab/mark_used_feature_flags_spec.rb
@@ -16,6 +16,7 @@ RSpec.describe RuboCop::Cop::Gitlab::MarkUsedFeatureFlags do
     stub_const("#{described_class}::DYNAMIC_FEATURE_FLAGS", [])
     allow(cop).to receive(:defined_feature_flags).and_return(defined_feature_flags)
     allow(cop).to receive(:usage_data_counters_known_event_feature_flags).and_return([])
+    described_class.feature_flags_already_tracked = false
   end
 
   def feature_flag_path(feature_flag_name)
diff --git a/spec/rubocop/cop/migration/add_limit_to_text_columns_spec.rb b/spec/rubocop/cop/migration/add_limit_to_text_columns_spec.rb
index 899872859a9..f6bed0d74fb 100644
--- a/spec/rubocop/cop/migration/add_limit_to_text_columns_spec.rb
+++ b/spec/rubocop/cop/migration/add_limit_to_text_columns_spec.rb
@@ -11,6 +11,7 @@ RSpec.describe RuboCop::Cop::Migration::AddLimitToTextColumns do
 
     before do
       allow(cop).to receive(:in_migration?).and_return(true)
+      allow(cop).to receive(:version).and_return(described_class::TEXT_LIMIT_ATTRIBUTE_ALLOWED_SINCE + 5)
     end
 
     context 'when text columns are defined without a limit' do
@@ -26,7 +27,7 @@ RSpec.describe RuboCop::Cop::Migration::AddLimitToTextColumns do
                   ^^^^ #{msg}
               end
 
-              create_table_with_constraints :test_text_limits_create do |t|
+              create_table :test_text_limits_create do |t|
                 t.integer :test_id, null: false
                 t.text :title
                 t.text :description
@@ -61,13 +62,10 @@ RSpec.describe RuboCop::Cop::Migration::AddLimitToTextColumns do
                 t.text :name
               end
 
-              create_table_with_constraints :test_text_limits_create do |t|
+              create_table :test_text_limits_create do |t|
                 t.integer :test_id, null: false
-                t.text :title
-                t.text :description
-
-                t.text_limit :title, 100
-                t.text_limit :description, 255
+                t.text :title, limit: 100
+                t.text :description, limit: 255
               end
 
               add_column :test_text_limits, :email, :text
@@ -82,6 +80,30 @@ RSpec.describe RuboCop::Cop::Migration::AddLimitToTextColumns do
           end
         RUBY
       end
+
+      context 'for migrations before 2021_09_10_00_00_00' do
+        it 'when limit: attribute is used (which is not supported yet for this version): registers an offense' do
+          allow(cop).to receive(:version).and_return(described_class::TEXT_LIMIT_ATTRIBUTE_ALLOWED_SINCE - 5)
+
+          expect_offense(<<~RUBY)
+            class TestTextLimits < ActiveRecord::Migration[6.0]
+              def up
+                create_table :test_text_limit_attribute do |t|
+                  t.integer :test_id, null: false
+                  t.text :name, limit: 100
+                    ^^^^ Text columns should always have a limit set (255 is suggested). Using limit: is not supported in this version. You can add a limit to a `text` column by using `add_text_limit` or `.text_limit` inside `create_table`
+                end
+
+                create_table_with_constraints :test_text_limit_attribute do |t|
+                  t.integer :test_id, null: false
+                  t.text :name, limit: 100
+                    ^^^^ Text columns should always have a limit set (255 is suggested). Using limit: is not supported in this version. You can add a limit to a `text` column by using `add_text_limit` or `.text_limit` inside `create_table`
+                end
+              end
+            end
+          RUBY
+        end
+      end
     end
 
     context 'when text array columns are defined without a limit' do
diff --git a/spec/rubocop/cop/migration/prevent_index_creation_spec.rb b/spec/rubocop/cop/migration/prevent_index_creation_spec.rb
index a3965f54bbd..ed7c8974d8d 100644
--- a/spec/rubocop/cop/migration/prevent_index_creation_spec.rb
+++ b/spec/rubocop/cop/migration/prevent_index_creation_spec.rb
@@ -6,28 +6,76 @@ require_relative '../../../../rubocop/cop/migration/prevent_index_creation'
 RSpec.describe RuboCop::Cop::Migration::PreventIndexCreation do
   subject(:cop) { described_class.new }
 
+  let(:forbidden_tables) { %w(ci_builds) }
+  let(:forbidden_tables_list) { forbidden_tables.join(', ') }
+
   context 'when in migration' do
     before do
       allow(cop).to receive(:in_migration?).and_return(true)
     end
 
     context 'when adding an index to a forbidden table' do
-      it 'registers an offense when add_index is used' do
-        expect_offense(<<~RUBY)
-          def change
-            add_index :ci_builds, :protected
-            ^^^^^^^^^ Adding new index to ci_builds is forbidden, see https://gitlab.com/gitlab-org/gitlab/-/issues/332886
+      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
-        RUBY
+        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' do
-        expect_offense(<<~RUBY)
-          def change
-            add_concurrent_index :ci_builds, :protected
-            ^^^^^^^^^^^^^^^^^^^^ Adding new index to ci_builds is forbidden, see https://gitlab.com/gitlab-org/gitlab/-/issues/332886
+      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
-        RUBY
+        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_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
 
@@ -39,6 +87,20 @@ RSpec.describe RuboCop::Cop::Migration::PreventIndexCreation do
           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
 
diff --git a/spec/rubocop/cop/migration/versioned_migration_class_spec.rb b/spec/rubocop/cop/migration/versioned_migration_class_spec.rb
new file mode 100644
index 00000000000..d9b0cd4546c
--- /dev/null
+++ b/spec/rubocop/cop/migration/versioned_migration_class_spec.rb
@@ -0,0 +1,81 @@
+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require_relative '../../../../rubocop/cop/migration/versioned_migration_class'
+
+RSpec.describe RuboCop::Cop::Migration::VersionedMigrationClass do
+  subject(:cop) { described_class.new }
+
+  let(:migration) do
+    <<~SOURCE
+      class TestMigration < Gitlab::Database::Migration[1.0]
+        def up
+          execute 'select 1'
+        end
+
+        def down
+          execute 'select 1'
+        end
+      end
+    SOURCE
+  end
+
+  shared_examples 'a disabled cop' do
+    it 'does not register any offenses' do
+      expect_no_offenses(migration)
+    end
+  end
+
+  context 'outside of a migration' do
+    it_behaves_like 'a disabled cop'
+  end
+
+  context 'in migration' do
+    before do
+      allow(cop).to receive(:in_migration?).and_return(true)
+    end
+
+    context 'in an old migration' do
+      before do
+        allow(cop).to receive(:version).and_return(described_class::ENFORCED_SINCE - 5)
+      end
+
+      it_behaves_like 'a disabled cop'
+    end
+
+    context 'that is recent' do
+      before do
+        allow(cop).to receive(:version).and_return(described_class::ENFORCED_SINCE + 5)
+      end
+
+      it 'adds an offence if inheriting from ActiveRecord::Migration' do
+        expect_offense(<<~RUBY)
+          class MyMigration < ActiveRecord::Migration[6.1]
+          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Don't inherit from ActiveRecord::Migration but use Gitlab::Database::Migration[1.0] instead. See https://docs.gitlab.com/ee/development/migration_style_guide.html#migration-helpers-and-versioning.
+          end
+        RUBY
+      end
+
+      it 'adds an offence if including Gitlab::Database::MigrationHelpers directly' do
+        expect_offense(<<~RUBY)
+          class MyMigration < Gitlab::Database::Migration[1.0]
+            include Gitlab::Database::MigrationHelpers
+            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Don't include migration helper modules directly. Inherit from Gitlab::Database::Migration[1.0] instead. See https://docs.gitlab.com/ee/development/migration_style_guide.html#migration-helpers-and-versioning.
+          end
+        RUBY
+      end
+
+      it 'excludes ActiveRecord classes defined inside the migration' do
+        expect_no_offenses(<<~RUBY)
+          class TestMigration < Gitlab::Database::Migration[1.0]
+            class TestModel < ApplicationRecord
+            end
+
+            class AnotherTestModel < ActiveRecord::Base
+            end
+          end
+        RUBY
+      end
+    end
+  end
+end
diff --git a/spec/rubocop/cop/performance/active_record_subtransaction_methods_spec.rb b/spec/rubocop/cop/performance/active_record_subtransaction_methods_spec.rb
new file mode 100644
index 00000000000..df18121e2df
--- /dev/null
+++ b/spec/rubocop/cop/performance/active_record_subtransaction_methods_spec.rb
@@ -0,0 +1,29 @@
+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require 'rspec-parameterized'
+
+require_relative '../../../../rubocop/cop/performance/active_record_subtransaction_methods'
+
+RSpec.describe RuboCop::Cop::Performance::ActiveRecordSubtransactionMethods do
+  subject(:cop) { described_class.new }
+
+  let(:message) { described_class::MSG }
+
+  shared_examples 'a method that uses a subtransaction' do |method_name|
+    it 'registers an offense' do
+      expect_offense(<<~RUBY, method_name: method_name, message: message)
+        Project.%{method_name}
+                ^{method_name} %{message}
+      RUBY
+    end
+  end
+
+  context 'when the method uses a subtransaction' do
+    where(:method) { described_class::DISALLOWED_METHODS.to_a }
+
+    with_them do
+      include_examples 'a method that uses a subtransaction', params[:method]
+    end
+  end
+end
diff --git a/spec/rubocop/cop/performance/active_record_subtransactions_spec.rb b/spec/rubocop/cop/performance/active_record_subtransactions_spec.rb
new file mode 100644
index 00000000000..0da2e30062a
--- /dev/null
+++ b/spec/rubocop/cop/performance/active_record_subtransactions_spec.rb
@@ -0,0 +1,62 @@
+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require_relative '../../../../rubocop/cop/performance/active_record_subtransactions'
+
+RSpec.describe RuboCop::Cop::Performance::ActiveRecordSubtransactions do
+  subject(:cop) { described_class.new }
+
+  let(:message) { described_class::MSG }
+
+  context 'when calling #transaction with only requires_new: true' do
+    it 'registers an offense' do
+      expect_offense(<<~RUBY)
+        ApplicationRecord.transaction(requires_new: true) do
+                                      ^^^^^^^^^^^^^^^^^^ #{message}
+          Project.create!(name: 'MyProject')
+        end
+      RUBY
+    end
+  end
+
+  context 'when passing multiple arguments to #transaction, including requires_new: true' do
+    it 'registers an offense' do
+      expect_offense(<<~RUBY)
+        ApplicationRecord.transaction(isolation: :read_committed, requires_new: true) do
+                                                                  ^^^^^^^^^^^^^^^^^^ #{message}
+          Project.create!(name: 'MyProject')
+        end
+      RUBY
+    end
+  end
+
+  context 'when calling #transaction with requires_new: false' do
+    it 'does not register an offense' do
+      expect_no_offenses(<<~RUBY)
+        ApplicationRecord.transaction(requires_new: false) do
+          Project.create!(name: 'MyProject')
+        end
+      RUBY
+    end
+  end
+
+  context 'when calling #transaction with other options' do
+    it 'does not register an offense' do
+      expect_no_offenses(<<~RUBY)
+        ApplicationRecord.transaction(isolation: :read_committed) do
+          Project.create!(name: 'MyProject')
+        end
+      RUBY
+    end
+  end
+
+  context 'when calling #transaction with no arguments' do
+    it 'does not register an offense' do
+      expect_no_offenses(<<~RUBY)
+        ApplicationRecord.transaction do
+          Project.create!(name: 'MyProject')
+        end
+      RUBY
+    end
+  end
+end
diff --git a/spec/rubocop/cop/worker_data_consistency_spec.rb b/spec/rubocop/cop/sidekiq_load_balancing/worker_data_consistency_spec.rb
index 5fa42bf2b87..cf8d0d1b66f 100644
--- a/spec/rubocop/cop/worker_data_consistency_spec.rb
+++ b/spec/rubocop/cop/sidekiq_load_balancing/worker_data_consistency_spec.rb
@@ -1,9 +1,9 @@
 # frozen_string_literal: true
 
 require 'fast_spec_helper'
-require_relative '../../../rubocop/cop/worker_data_consistency'
+require_relative '../../../../rubocop/cop/sidekiq_load_balancing/worker_data_consistency'
 
-RSpec.describe RuboCop::Cop::WorkerDataConsistency do
+RSpec.describe RuboCop::Cop::SidekiqLoadBalancing::WorkerDataConsistency do
   subject(:cop) { described_class.new }
 
   before do
diff --git a/spec/rubocop/cop/sidekiq_load_balancing/worker_data_consistency_with_deduplication_spec.rb b/spec/rubocop/cop/sidekiq_load_balancing/worker_data_consistency_with_deduplication_spec.rb
new file mode 100644
index 00000000000..6e7212b1002
--- /dev/null
+++ b/spec/rubocop/cop/sidekiq_load_balancing/worker_data_consistency_with_deduplication_spec.rb
@@ -0,0 +1,166 @@
+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require 'rspec-parameterized'
+require_relative '../../../../rubocop/cop/sidekiq_load_balancing/worker_data_consistency_with_deduplication'
+
+RSpec.describe RuboCop::Cop::SidekiqLoadBalancing::WorkerDataConsistencyWithDeduplication do
+  using RSpec::Parameterized::TableSyntax
+
+  subject(:cop) { described_class.new }
+
+  before do
+    allow(cop)
+      .to receive(:in_worker?)
+      .and_return(true)
+  end
+
+  where(:data_consistency) { %i[delayed sticky] }
+
+  with_them do
+    let(:strategy) { described_class::DEFAULT_STRATEGY }
+    let(:corrected) do
+      <<~CORRECTED
+      class SomeWorker
+        include ApplicationWorker
+
+        data_consistency :#{data_consistency}
+
+        deduplicate #{strategy}, including_scheduled: true
+        idempotent!
+      end
+      CORRECTED
+    end
+
+    context 'when deduplication strategy is not explicitly set' do
+      it 'registers an offense and corrects using default strategy' do
+        expect_offense(<<~CODE)
+          class SomeWorker
+            include ApplicationWorker
+
+            data_consistency :#{data_consistency}
+
+            idempotent!
+            ^^^^^^^^^^^ Workers that declare either `:sticky` or `:delayed` data consistency [...]
+          end
+        CODE
+
+        expect_correction(corrected)
+      end
+
+      context 'when identation is different' do
+        let(:corrected) do
+          <<~CORRECTED
+            class SomeWorker
+                include ApplicationWorker
+
+                data_consistency :#{data_consistency}
+
+                deduplicate #{strategy}, including_scheduled: true
+                idempotent!
+            end
+          CORRECTED
+        end
+
+        it 'registers an offense and corrects with correct identation' do
+          expect_offense(<<~CODE)
+            class SomeWorker
+                include ApplicationWorker
+
+                data_consistency :#{data_consistency}
+
+                idempotent!
+                ^^^^^^^^^^^ Workers that declare either `:sticky` or `:delayed` data consistency [...]
+            end
+          CODE
+
+          expect_correction(corrected)
+        end
+      end
+    end
+
+    context 'when deduplication strategy does not include including_scheduling option' do
+      let(:strategy) { ':until_executed' }
+
+      it 'registers an offense and corrects' do
+        expect_offense(<<~CODE)
+          class SomeWorker
+            include ApplicationWorker
+
+            data_consistency :#{data_consistency}
+
+            deduplicate :until_executed
+            ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Workers that declare either `:sticky` or `:delayed` data consistency [...]
+            idempotent!
+          end
+        CODE
+
+        expect_correction(corrected)
+      end
+    end
+
+    context 'when deduplication strategy has including_scheduling option disabled' do
+      let(:strategy) { ':until_executed' }
+
+      it 'registers an offense and corrects' do
+        expect_offense(<<~CODE)
+          class SomeWorker
+            include ApplicationWorker
+
+            data_consistency :#{data_consistency}
+
+            deduplicate :until_executed, including_scheduled: false
+            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Workers that declare either `:sticky` or `:delayed` data consistency [...]
+            idempotent!
+          end
+        CODE
+
+        expect_correction(corrected)
+      end
+    end
+
+    context "when deduplication strategy is :none" do
+      it 'does not register an offense' do
+        expect_no_offenses(<<~CODE)
+          class SomeWorker
+            include ApplicationWorker
+
+            data_consistency :always
+
+            deduplicate :none
+            idempotent!
+          end
+        CODE
+      end
+    end
+
+    context "when deduplication strategy has including_scheduling option enabled" do
+      it 'does not register an offense' do
+        expect_no_offenses(<<~CODE)
+          class SomeWorker
+            include ApplicationWorker
+
+            data_consistency :always
+
+            deduplicate :until_executing, including_scheduled: true
+            idempotent!
+          end
+        CODE
+      end
+    end
+  end
+
+  context "data_consistency: :always" do
+    it 'does not register an offense' do
+      expect_no_offenses(<<~CODE)
+        class SomeWorker
+          include ApplicationWorker
+
+          data_consistency :always
+
+          idempotent!
+        end
+      CODE
+    end
+  end
+end