Add cop for use of remove_column

remove_column should only be used in the up (or change) step of a migration if
it's a post-deployment migration. Otherwise there will be downtime due to the
ActiveRecord column cache, which we can avoid by using the IgnorableColumn
concern in combination with a post-deployment migration.

db/migrate/20160610194713_remove_deprecated_issues_tracker_columns_from_projects.rb

+# rubocop:disable Migration/RemoveColumn
 class RemoveDeprecatedIssuesTrackerColumnsFromProjects < ActiveRecord::Migration
   def change
     remove_column :projects, :issues_tracker, :string, default: 'gitlab', null: false

db/migrate/20160610301627_remove_notification_level_from_users.rb

+# rubocop:disable Migration/RemoveColumn
 class RemoveNotificationLevelFromUsers < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20160705055809_remove_developers_can_push_from_protected_branches.rb

+# rubocop:disable Migration/RemoveColumn
 # See http://doc.gitlab.com/ce/development/migration_style_guide.html
 # for more information on how to write migrations for GitLab.
 

db/migrate/20160705055813_remove_developers_can_merge_from_protected_branches.rb

+# rubocop:disable Migration/RemoveColumn
 # See http://doc.gitlab.com/ce/development/migration_style_guide.html
 # for more information on how to write migrations for GitLab.
 

db/migrate/20160729173930_remove_project_id_from_spam_logs.rb

+# rubocop:disable Migration/RemoveColumn
 # See http://doc.gitlab.com/ce/development/migration_style_guide.html
 # for more information on how to write migrations for GitLab.
 

db/migrate/20160831223750_remove_features_enabled_from_projects.rb

+# rubocop:disable Migration/RemoveColumn
 # See http://doc.gitlab.com/ce/development/migration_style_guide.html
 # for more information on how to write migrations for GitLab.
 

db/migrate/20160913162434_remove_projects_pushes_since_gc.rb

+# rubocop:disable Migration/RemoveColumn
 # See http://doc.gitlab.com/ce/development/migration_style_guide.html
 # for more information on how to write migrations for GitLab.
 

db/migrate/20161018024550_remove_priority_from_labels.rb

+# rubocop:disable Migration/RemoveColumn
 class RemovePriorityFromLabels < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20161201160452_migrate_project_statistics.rb

+# rubocop:disable Migration/RemoveColumn
 class MigrateProjectStatistics < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 

db/migrate/20170222143500_remove_old_project_id_columns.rb

+# rubocop:disable Migration/RemoveColumn
 # rubocop:disable RemoveIndex
 class RemoveOldProjectIdColumns < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers

db/migrate/20170301205639_remove_unused_ci_tables_and_columns.rb

+# rubocop:disable Migration/RemoveColumn
 # rubocop:disable Migration/Datetime
 class RemoveUnusedCiTablesAndColumns < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers

db/migrate/20170315174634_revert_add_notified_of_own_activity_to_users.rb

+# rubocop:disable Migration/RemoveColumn
 # rubocop:disable Migration/UpdateLargeTable
 class RevertAddNotifiedOfOwnActivityToUsers < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers

db/migrate/20171106154015_remove_issues_branch_name.rb

+# rubocop:disable Migration/RemoveColumn
 # See http://doc.gitlab.com/ce/development/migration_style_guide.html
 # for more information on how to write migrations for GitLab.
 

rubocop/cop/migration/remove_column.rb

+require_relative '../../migration_helpers'
+
+module RuboCop
+  module Cop
+    module Migration
+      # Cop that checks if remove_column is used in a regular (not
+      # post-deployment) migration.
+      class RemoveColumn < RuboCop::Cop::Cop
+        include MigrationHelpers
+
+        MSG = '`remove_column` must only be used in post-deployment migrations'.freeze
+
+        def on_def(node)
+          def_method = node.children[0]
+
+          return unless in_migration?(node) && !in_post_deployment_migration?(node)
+          return unless def_method == :change || def_method == :up
+
+          node.each_descendant(:send) do |send_node|
+            send_method = send_node.children[1]
+
+            if send_method == :remove_column
+              add_offense(send_node, :selector)
+            end
+          end
+        end
+      end
+    end
+  end
+end

rubocop/migration_helpers.rb

@@ -7,5 +7,11 @@ module RuboCop
 
       dirname.end_with?('db/migrate', 'db/post_migrate')
     end
+
+    def in_post_deployment_migration?(node)
+      dirname = File.dirname(node.location.expression.source_buffer.name)
+
+      dirname.end_with?('db/post_migrate')
+    end
   end
 end

rubocop/rubocop.rb

@@ -14,6 +14,7 @@ require_relative 'cop/migration/add_index'
 require_relative 'cop/migration/add_timestamps'
 require_relative 'cop/migration/datetime'
 require_relative 'cop/migration/hash_index'
+require_relative 'cop/migration/remove_column'
 require_relative 'cop/migration/remove_concurrent_index'
 require_relative 'cop/migration/remove_index'
 require_relative 'cop/migration/reversible_add_column_with_default'

spec/rubocop/cop/migration/remove_column_spec.rb

+require 'spec_helper'
+
+require 'rubocop'
+require 'rubocop/rspec/support'
+
+require_relative '../../../../rubocop/cop/migration/remove_column'
+
+describe RuboCop::Cop::Migration::RemoveColumn do
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+
+  def source(meth = 'change')
+    "def #{meth}; remove_column :table, :column; end"
+  end
+
+  context 'in a regular migration' do
+    before do
+      allow(cop).to receive(:in_migration?).and_return(true)
+      allow(cop).to receive(:in_post_deployment_migration?).and_return(false)
+    end
+
+    it 'registers an offense when remove_column is used in the change method' do
+      inspect_source(cop, source('change'))
+
+      aggregate_failures do
+        expect(cop.offenses.size).to eq(1)
+        expect(cop.offenses.map(&:line)).to eq([1])
+      end
+    end
+
+    it 'registers an offense when remove_column is used in the up method' do
+      inspect_source(cop, source('up'))
+
+      aggregate_failures do
+        expect(cop.offenses.size).to eq(1)
+        expect(cop.offenses.map(&:line)).to eq([1])
+      end
+    end
+
+    it 'registers no offense when remove_column is used in the down method' do
+      inspect_source(cop, source('down'))
+
+      expect(cop.offenses.size).to eq(0)
+    end
+  end
+
+  context 'in a post-deployment migration' do
+    before do
+      allow(cop).to receive(:in_migration?).and_return(true)
+      allow(cop).to receive(:in_post_deployment_migration?).and_return(true)
+    end
+
+    it 'registers no offense' do
+      inspect_source(cop, source)
+
+      expect(cop.offenses.size).to eq(0)
+    end
+  end
+
+  context 'outside of a migration' do
+    it 'registers no offense' do
+      inspect_source(cop, source)
+
+      expect(cop.offenses.size).to eq(0)
+    end
+  end
+end