Merge branch 'add-rubocop-for-drop-tables' into 'master'

Add Migration/DropTable rubocop cop

Closes #25528

See merge request gitlab-org/gitlab!34473

db/migrate/20190315191339_create_merge_request_assignees_table.rb

@@ -17,6 +17,8 @@ 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/20190402150158_backport_enterprise_schema.rb

@@ -193,7 +193,9 @@ class BackportEnterpriseSchema < ActiveRecord::Migration[5.0]
   end
 
   def drop_table_if_exists(table)
+    # rubocop:disable Migration/DropTable
     drop_table(table) if table_exists?(table)
+    # rubocop:enable Migration/DropTable
   end
 
   def add_column_with_default_if_not_exists(table, name, *args)

db/migrate/20190722144316_create_milestone_releases_table.rb

@@ -15,6 +15,8 @@ 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,6 +24,8 @@ 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,6 +23,8 @@ 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,6 +23,8 @@ 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/20200123091622_drop_analytics_repository_files_table.rb

@@ -7,7 +7,9 @@ class DropAnalyticsRepositoryFilesTable < ActiveRecord::Migration[5.2]
 
   def up
     # Requires ExclusiveLock on the table. Not in use, no records, no FKs.
+    # rubocop:disable Migration/DropTable
     drop_table :analytics_repository_files
+    # rubocop:enable Migration/DropTable
   end
 
   def down

db/migrate/20200123091734_drop_analytics_repository_file_commits_table.rb

@@ -7,7 +7,9 @@ class DropAnalyticsRepositoryFileCommitsTable < ActiveRecord::Migration[5.2]
 
   def up
     # Requires ExclusiveLock on the table. Not in use, no records, no FKs.
+    # rubocop:disable Migration/DropTable
     drop_table :analytics_repository_file_commits
+    # rubocop:enable Migration/DropTable
   end
 
   def down

db/migrate/20200123091854_drop_analytics_repository_file_edits_table.rb

@@ -7,7 +7,9 @@ class DropAnalyticsRepositoryFileEditsTable < ActiveRecord::Migration[5.2]
 
   def up
     # Requires ExclusiveLock on the table. Not in use, no records, no FKs.
+    # rubocop:disable Migration/DropTable
     drop_table :analytics_repository_file_edits if table_exists?(:analytics_repository_file_edits) # this table might be already dropped on development environment
+    # rubocop:enable Migration/DropTable
   end
 
   def down

db/migrate/20200214025454_add_canonical_emails.rb

@@ -20,7 +20,9 @@ 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/20200215225103_drop_forked_project_links_table.rb

@@ -8,6 +8,7 @@ class DropForkedProjectLinksTable < ActiveRecord::Migration[6.0]
   DOWNTIME = false
 
   def change
+    # rubocop:disable Migration/DropTable
     drop_table "forked_project_links", id: :serial do |t|
       t.integer "forked_to_project_id", null: false
       t.integer "forked_from_project_id", null: false
@@ -15,5 +16,6 @@ class DropForkedProjectLinksTable < ActiveRecord::Migration[6.0]
       t.datetime "updated_at"
       t.index ["forked_to_project_id"], name: "index_forked_project_links_on_forked_to_project_id", unique: true
     end
+    # rubocop:enable Migration/DropTable
   end
 end

db/migrate/20200227165129_create_user_details.rb

@@ -20,7 +20,9 @@ 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,7 +19,9 @@ 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,6 +30,8 @@ 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

@@ -9,7 +9,9 @@ class RecreateCiRef < ActiveRecord::Migration[6.0]
 
   def up
     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: :bigint
@@ -23,7 +25,9 @@ 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,7 +16,9 @@ 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,6 +26,8 @@ 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,6 +26,8 @@ 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,6 +25,8 @@ 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,6 +15,8 @@ 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,6 +39,8 @@ 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,6 +20,8 @@ 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,6 +20,8 @@ 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,6 +26,8 @@ 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,6 +21,8 @@ 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,6 +29,8 @@ 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,6 +31,8 @@ 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,6 +17,8 @@ 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,7 +21,9 @@ 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,6 +15,8 @@ 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,7 +52,10 @@ 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()')
   end

rubocop/cop/migration/drop_table.rb

+# frozen_string_literal: true
+
+require_relative '../../migration_helpers'
+
+module RuboCop
+  module Cop
+    module Migration
+      # Cop that checks if `drop_table` is called in deployment migrations.
+      # Calling it in deployment migrations can cause downtimes as there still may be code using the target tables.
+      class DropTable < RuboCop::Cop::Cop
+        include MigrationHelpers
+
+        MSG = <<-MESSAGE.delete("\n").squeeze
+          `drop_table` in deployment migrations requires downtime.
+          Drop tables in post-deployment migrations instead.
+        MESSAGE
+
+        def on_def(node)
+          return unless in_deployment_migration?(node)
+
+          node.each_descendant(:send) do |send_node|
+            next unless offensible?(send_node)
+
+            add_offense(send_node, location: :selector)
+          end
+        end
+
+        private
+
+        def offensible?(node)
+          drop_table?(node) || drop_table_in_execute?(node)
+        end
+
+        def drop_table?(node)
+          node.children[1] == :drop_table
+        end
+
+        def drop_table_in_execute?(node)
+          execute?(node) && drop_table_in_execute_sql?(node)
+        end
+
+        def execute?(node)
+          node.children[1] == :execute
+        end
+
+        def drop_table_in_execute_sql?(node)
+          node.children[2].to_s.match?(/drop\s+table/i)
+        end
+      end
+    end
+  end
+end

rubocop/migration_helpers.rb

@@ -23,7 +23,11 @@ module RuboCop
 
     # Returns true if the given node originated from the db/migrate directory.
     def in_migration?(node)
-      dirname(node).end_with?('db/migrate', 'db/geo/migrate') || in_post_deployment_migration?(node)
+      in_deployment_migration?(node) || in_post_deployment_migration?(node)
+    end
+
+    def in_deployment_migration?(node)
+      dirname(node).end_with?('db/migrate', 'db/geo/migrate')
     end
 
     def in_post_deployment_migration?(node)

spec/rubocop/cop/migration/drop_table_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+require 'rubocop'
+require 'rubocop/rspec/support'
+
+require_relative '../../../../rubocop/cop/migration/drop_table'
+
+describe RuboCop::Cop::Migration::DropTable do
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+
+  context 'when in deployment migration' do
+    before 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)
+
+          execute "DROP TABLE table"
+          ^^^^^^^ #{described_class::MSG}
+
+          execute "CREATE UNIQUE INDEX email_index ON users (email);"
+        end
+      PATTERN
+    end
+  end
+
+  context 'when in post-deployment migration' do
+    before do
+      allow(cop).to receive(:in_post_deployment_migration?).and_return(true)
+    end
+
+    it 'registers no offense' do
+      expect_no_offenses(<<~PATTERN)
+        def change
+          drop_table :table
+          execute "DROP TABLE table"
+        end
+      PATTERN
+    end
+  end
+
+  context 'when outside of migration' do
+    it 'registers no offense' do
+      expect_no_offenses(<<~PATTERN)
+        def change
+          drop_table :table
+          execute "DROP TABLE table"
+        end
+      PATTERN
+    end
+  end
+end