Merge branch 'fix_drop_table_cop' into 'master'

Do not register offense for drop table in down method

See merge request gitlab-org/gitlab!34739

db/migrate/20190315191339_create_merge_request_assignees_table.rb

@@ -17,8 +17,6 @@ class CreateMergeRequestAssigneesTable < ActiveRecord::Migration[5.0]
   end
 
   def down
-    # rubocop:disable Migration/DropTable
     drop_table :merge_request_assignees
-    # rubocop:enable Migration/DropTable
   end
 end

db/migrate/20190722144316_create_milestone_releases_table.rb

@@ -15,8 +15,6 @@ class CreateMilestoneReleasesTable < ActiveRecord::Migration[5.2]
   end
 
   def down
-    # rubocop:disable Migration/DropTable
     drop_table :milestone_releases
-    # rubocop:enable Migration/DropTable
   end
 end

db/migrate/20190927055500_create_description_versions.rb

@@ -24,8 +24,6 @@ class CreateDescriptionVersions < ActiveRecord::Migration[5.2]
   def down
     remove_column :system_note_metadata, :description_version_id
 
-    # rubocop:disable Migration/DropTable
     drop_table :description_versions
-    # rubocop:enable Migration/DropTable
   end
 end

db/migrate/20191118053631_add_group_deletion_schedules.rb

@@ -23,8 +23,6 @@ class AddGroupDeletionSchedules < ActiveRecord::Migration[5.2]
   end
 
   def down
-    # rubocop:disable Migration/DropTable
     drop_table :group_deletion_schedules
-    # rubocop:enable Migration/DropTable
   end
 end

db/migrate/20191127151619_create_gitlab_subscription_histories.rb

@@ -23,8 +23,6 @@ class CreateGitlabSubscriptionHistories < ActiveRecord::Migration[5.2]
   end
 
   def down
-    # rubocop:disable Migration/DropTable
     drop_table :gitlab_subscription_histories
-    # rubocop:enable Migration/DropTable
   end
 end

db/migrate/20200214025454_add_canonical_emails.rb

@@ -20,9 +20,7 @@ class AddCanonicalEmails < ActiveRecord::Migration[6.0]
 
   def down
     with_lock_retries do
-      # rubocop:disable Migration/DropTable
       drop_table(:user_canonical_emails)
-      # rubocop:enable Migration/DropTable
     end
   end
 end

db/migrate/20200227165129_create_user_details.rb

@@ -20,9 +20,7 @@ class CreateUserDetails < ActiveRecord::Migration[6.0]
 
   def down
     with_lock_retries do
-      # rubocop:disable Migration/DropTable
       drop_table :user_details
-      # rubocop:enable Migration/DropTable
     end
   end
 end

db/migrate/20200311093210_create_user_highest_roles.rb

@@ -19,9 +19,7 @@ class CreateUserHighestRoles < ActiveRecord::Migration[6.0]
 
   def down
     with_lock_retries do
-      # rubocop:disable Migration/DropTable
       drop_table :user_highest_roles
-      # rubocop:enable Migration/DropTable
     end
   end
 end

db/migrate/20200326122700_create_diff_note_positions.rb

@@ -30,8 +30,6 @@ class CreateDiffNotePositions < ActiveRecord::Migration[6.0]
   # rubocop:enable Migration/AddLimitToTextColumns
 
   def down
-    # rubocop:disable Migration/DropTable
     drop_table :diff_note_positions
-    # rubocop:enable Migration/DropTable
   end
 end

db/migrate/20200330203837_recreate_ci_ref.rb

@@ -25,9 +25,7 @@ class RecreateCiRef < ActiveRecord::Migration[6.0]
 
   def down
     with_lock_retries do
-      # rubocop:disable Migration/DropTable
       drop_table :ci_refs
-      # rubocop:enable Migration/DropTable
 
       create_table :ci_refs do |t|
         t.references :project, null: false, index: false, foreign_key: { on_delete: :cascade }, type: :integer

db/migrate/20200331132103_add_project_compliance_framework_settings_table.rb

@@ -16,9 +16,7 @@ class AddProjectComplianceFrameworkSettingsTable < ActiveRecord::Migration[6.0]
 
   def down
     with_lock_retries do
-      # rubocop:disable Migration/DropTable
       drop_table :project_compliance_framework_settings
-      # rubocop:enable Migration/DropTable
     end
   end
 end

db/migrate/20200407182205_create_partitioned_foreign_keys.rb

@@ -26,8 +26,6 @@ class CreatePartitionedForeignKeys < ActiveRecord::Migration[6.0]
   end
 
   def down
-    # rubocop:disable Migration/DropTable
     drop_table :partitioned_foreign_keys
-    # rubocop:enable Migration/DropTable
   end
 end

db/migrate/20200407222647_create_project_repository_storage_moves.rb

@@ -26,8 +26,6 @@ class CreateProjectRepositoryStorageMoves < ActiveRecord::Migration[6.0]
     remove_check_constraint(:project_repository_storage_moves, 'project_repository_storage_moves_source_storage_name')
     remove_check_constraint(:project_repository_storage_moves, 'project_repository_storage_moves_destination_storage_name')
 
-    # rubocop:disable Migration/DropTable
     drop_table :project_repository_storage_moves
-    # rubocop:enable Migration/DropTable
   end
 end

db/migrate/20200408125046_create_ci_freeze_periods.rb

@@ -25,8 +25,6 @@ class CreateCiFreezePeriods < ActiveRecord::Migration[6.0]
   end
 
   def down
-    # rubocop:disable Migration/DropTable
     drop_table :ci_freeze_periods
-    # rubocop:enable Migration/DropTable
   end
 end

db/migrate/20200416005331_create_status_page_published_incidents.rb

@@ -15,8 +15,6 @@ class CreateStatusPagePublishedIncidents < ActiveRecord::Migration[6.0]
   end
 
   def down
-    # rubocop:disable Migration/DropTable
     drop_table :status_page_published_incidents
-    # rubocop:enable Migration/DropTable
   end
 end

db/migrate/20200417044453_create_alert_management_alerts.rb

@@ -39,8 +39,6 @@ class CreateAlertManagementAlerts < ActiveRecord::Migration[6.0]
   end
 
   def down
-    # rubocop:disable Migration/DropTable
     drop_table :alert_management_alerts
-    # rubocop:enable Migration/DropTable
   end
 end

db/migrate/20200420104303_add_group_import_states_table.rb

@@ -20,8 +20,6 @@ class AddGroupImportStatesTable < ActiveRecord::Migration[6.0]
   # rubocop:enable Migration/AddLimitToTextColumns
 
   def down
-    # rubocop:disable Migration/DropTable
     drop_table :group_import_states
-    # rubocop:enable Migration/DropTable
   end
 end

db/migrate/20200420115948_create_metrics_users_starred_dashboard.rb

@@ -20,8 +20,6 @@ class CreateMetricsUsersStarredDashboard < ActiveRecord::Migration[6.0]
   # rubocop: enable Migration/AddLimitToTextColumns
 
   def down
-    # rubocop:disable Migration/DropTable
     drop_table :metrics_users_starred_dashboards
-    # rubocop:enable Migration/DropTable
   end
 end

db/migrate/20200422091541_create_ci_instance_variables.rb

@@ -26,8 +26,6 @@ class CreateCiInstanceVariables < ActiveRecord::Migration[6.0]
   end
 
   def down
-    # rubocop:disable Migration/DropTable
     drop_table :ci_instance_variables
-    # rubocop:enable Migration/DropTable
   end
 end

db/migrate/20200424135319_create_nuget_dependency_link_metadata.rb

@@ -21,8 +21,6 @@ class CreateNugetDependencyLinkMetadata < ActiveRecord::Migration[6.0]
   end
 
   def down
-    # rubocop:disable Migration/DropTable
     drop_table :packages_nuget_dependency_link_metadata
-    # rubocop:enable Migration/DropTable
   end
 end

db/migrate/20200430130048_create_packages_nuget_metadata.rb

@@ -29,8 +29,6 @@ class CreatePackagesNugetMetadata < ActiveRecord::Migration[6.0]
   end
 
   def down
-    # rubocop:disable Migration/DropTable
     drop_table :packages_nuget_metadata
-    # rubocop:enable Migration/DropTable
   end
 end

db/migrate/20200430174637_create_group_deploy_keys.rb

@@ -31,8 +31,6 @@ class CreateGroupDeployKeys < ActiveRecord::Migration[6.0]
   end
 
   def down
-    # rubocop:disable Migration/DropTable
     drop_table :group_deploy_keys
-    # rubocop:enable Migration/DropTable
   end
 end

db/migrate/20200521225327_create_alert_management_alert_assignees.rb

@@ -17,8 +17,6 @@ class CreateAlertManagementAlertAssignees < ActiveRecord::Migration[6.0]
   end
 
   def down
-    # rubocop:disable Migration/DropTable
     drop_table :alert_management_alert_assignees
-    # rubocop:enable Migration/DropTable
   end
 end

db/migrate/20200604143628_create_project_security_settings.rb

@@ -21,9 +21,7 @@ class CreateProjectSecuritySettings < ActiveRecord::Migration[6.0]
 
   def down
     with_lock_retries do
-      # rubocop:disable Migration/DropTable
       drop_table :project_security_settings
-      # rubocop:enable Migration/DropTable
     end
   end
 end

db/migrate/20200604145731_create_board_user_preferences.rb

@@ -15,8 +15,6 @@ class CreateBoardUserPreferences < ActiveRecord::Migration[6.0]
   end
 
   def down
-    # rubocop:disable Migration/DropTable
     drop_table :board_user_preferences
-    # rubocop:enable Migration/DropTable
   end
 end

ee/db/geo/migrate/20180322062741_migrate_ci_job_artifacts_to_separate_registry.rb

@@ -52,9 +52,7 @@ class MigrateCiJobArtifactsToSeparateRegistry < ActiveRecord::Migration[4.2]
   end
 
   def down
-    # rubocop:disable Migration/DropTable
     tracking_db.drop_table :job_artifact_registry
-    # rubocop:enable Migration/DropTable
 
     execute('DROP TRIGGER IF EXISTS replicate_job_artifact_registry ON file_registry')
     execute('DROP FUNCTION IF EXISTS replicate_job_artifact_registry()')

rubocop/cop/migration/drop_table.rb

@@ -17,6 +17,7 @@ module RuboCop
 
         def on_def(node)
           return unless in_deployment_migration?(node)
+          return if down_method?(node)
 
           node.each_descendant(:send) do |send_node|
             next unless offensible?(send_node)
@@ -27,6 +28,10 @@ module RuboCop
 
         private
 
+        def down_method?(node)
+          node.method?(:down)
+        end
+
         def offensible?(node)
           drop_table?(node) || drop_table_in_execute?(node)
         end

spec/rubocop/cop/migration/drop_table_spec.rb

@@ -17,20 +17,70 @@ describe RuboCop::Cop::Migration::DropTable do
       allow(cop).to receive(:in_deployment_migration?).and_return(true)
     end
 
-    it 'registers an offense' do
-      expect_offense(<<~PATTERN)
-        def change
-          drop_table :table
-          ^^^^^^^^^^ #{described_class::MSG}
-
-          add_column(:users, :username, :text)
+    context 'with drop_table DSL method' do
+      context 'when in down method' do
+        it 'does not register an offense' do
+          expect_no_offenses(<<~PATTERN)
+            def down
+              drop_table :table
+            end
+          PATTERN
+        end
+      end
 
-          execute "DROP TABLE table"
-          ^^^^^^^ #{described_class::MSG}
+      context 'when in up method' do
+        it 'registers an offense' do
+          expect_offense(<<~PATTERN)
+            def up
+              drop_table :table
+              ^^^^^^^^^^ #{described_class::MSG}
+            end
+          PATTERN
+        end
+      end
 
-          execute "CREATE UNIQUE INDEX email_index ON users (email);"
+      context 'when in change method' do
+        it 'registers an offense' do
+          expect_offense(<<~PATTERN)
+            def change
+              drop_table :table
+              ^^^^^^^^^^ #{described_class::MSG}
+            end
+          PATTERN
         end
-      PATTERN
+      end
+    end
+
+    context 'with DROP TABLE SQL literal' do
+      it 'does not register an offense' do
+        expect_no_offenses(<<~PATTERN)
+          def down
+            execute "DROP TABLE table"
+          end
+        PATTERN
+      end
+    end
+
+    context 'when in up method' do
+      it 'registers an offense' do
+        expect_offense(<<~PATTERN)
+          def up
+            execute "DROP TABLE table"
+            ^^^^^^^ #{described_class::MSG}
+          end
+        PATTERN
+      end
+    end
+
+    context 'when in change method' do
+      it 'registers an offense' do
+        expect_offense(<<~PATTERN)
+          def change
+            execute "DROP TABLE table"
+            ^^^^^^^ #{described_class::MSG}
+          end
+        PATTERN
+      end
     end
   end