Merge branch 'rs-add_column_with_default_cops' into 'master'

Add cop to blacklist specific tables for `add_column_with_default`

Closes #31293

See merge request !10892

16 files changed, 121 insertions, 13 deletions

diff --git a/db/migrate/20160419122101_add_only_allow_merge_if_build_succeeds_to_projects.rb b/db/migrate/20160419122101_add_only_allow_merge_if_build_succeeds_to_projects.rb
index 69d64ccd006..22bac46e25c 100644
--- a/db/migrate/20160419122101_add_only_allow_merge_if_build_succeeds_to_projects.rb
+++ b/db/migrate/20160419122101_add_only_allow_merge_if_build_succeeds_to_projects.rb
@@ -1,3 +1,4 @@
+# rubocop:disable Migration/AddColumnWithDefaultToLargeTable
 class AddOnlyAllowMergeIfBuildSucceedsToProjects < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
   disable_ddl_transaction!
diff --git a/db/migrate/20160608195742_add_repository_storage_to_projects.rb b/db/migrate/20160608195742_add_repository_storage_to_projects.rb
index c700d2b569d..0f3664c13ef 100644
--- a/db/migrate/20160608195742_add_repository_storage_to_projects.rb
+++ b/db/migrate/20160608195742_add_repository_storage_to_projects.rb
@@ -1,3 +1,4 @@
+# rubocop:disable Migration/AddColumnWithDefaultToLargeTable
 class AddRepositoryStorageToProjects < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
   disable_ddl_transaction!
diff --git a/db/migrate/20160715154212_add_request_access_enabled_to_projects.rb b/db/migrate/20160715154212_add_request_access_enabled_to_projects.rb
index bf0131c6d76..5dc26f8982a 100644
--- a/db/migrate/20160715154212_add_request_access_enabled_to_projects.rb
+++ b/db/migrate/20160715154212_add_request_access_enabled_to_projects.rb
@@ -1,3 +1,4 @@
+# rubocop:disable Migration/AddColumnWithDefaultToLargeTable
 class AddRequestAccessEnabledToProjects < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
   disable_ddl_transaction!
diff --git a/db/migrate/20160715204316_add_request_access_enabled_to_groups.rb b/db/migrate/20160715204316_add_request_access_enabled_to_groups.rb
index e7b14cd3ee2..4a317646788 100644
--- a/db/migrate/20160715204316_add_request_access_enabled_to_groups.rb
+++ b/db/migrate/20160715204316_add_request_access_enabled_to_groups.rb
@@ -1,3 +1,4 @@
+# rubocop:disable Migration/AddColumnWithDefaultToLargeTable
 class AddRequestAccessEnabledToGroups < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
   disable_ddl_transaction!
diff --git a/db/migrate/20160831223750_remove_features_enabled_from_projects.rb b/db/migrate/20160831223750_remove_features_enabled_from_projects.rb
index a2c207b49ea..7414a28ac97 100644
--- a/db/migrate/20160831223750_remove_features_enabled_from_projects.rb
+++ b/db/migrate/20160831223750_remove_features_enabled_from_projects.rb
@@ -1,6 +1,7 @@
 # See http://doc.gitlab.com/ce/development/migration_style_guide.html
 # for more information on how to write migrations for GitLab.
 
+# rubocop:disable Migration/AddColumnWithDefaultToLargeTable
 class RemoveFeaturesEnabledFromProjects < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
   disable_ddl_transaction!
diff --git a/db/migrate/20160913162434_remove_projects_pushes_since_gc.rb b/db/migrate/20160913162434_remove_projects_pushes_since_gc.rb
index 18ea9d43a43..0100e30a733 100644
--- a/db/migrate/20160913162434_remove_projects_pushes_since_gc.rb
+++ b/db/migrate/20160913162434_remove_projects_pushes_since_gc.rb
@@ -1,6 +1,7 @@
 # See http://doc.gitlab.com/ce/development/migration_style_guide.html
 # for more information on how to write migrations for GitLab.
 
+# rubocop:disable Migration/AddColumnWithDefaultToLargeTable
 class RemoveProjectsPushesSinceGc < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 
diff --git a/db/migrate/20170124193147_add_two_factor_columns_to_namespaces.rb b/db/migrate/20170124193147_add_two_factor_columns_to_namespaces.rb
index df5cddeb205..ae37da275fd 100644
--- a/db/migrate/20170124193147_add_two_factor_columns_to_namespaces.rb
+++ b/db/migrate/20170124193147_add_two_factor_columns_to_namespaces.rb
@@ -1,3 +1,4 @@
+# rubocop:disable Migration/AddColumnWithDefaultToLargeTable
 class AddTwoFactorColumnsToNamespaces < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 
diff --git a/db/migrate/20170124193205_add_two_factor_columns_to_users.rb b/db/migrate/20170124193205_add_two_factor_columns_to_users.rb
index 1d1021fcbb3..8d4aefa4365 100644
--- a/db/migrate/20170124193205_add_two_factor_columns_to_users.rb
+++ b/db/migrate/20170124193205_add_two_factor_columns_to_users.rb
@@ -1,3 +1,4 @@
+# rubocop:disable Migration/AddColumnWithDefaultToLargeTable
 class AddTwoFactorColumnsToUsers < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 
diff --git a/db/migrate/20170301125302_add_printing_merge_request_link_enabled_to_project.rb b/db/migrate/20170301125302_add_printing_merge_request_link_enabled_to_project.rb
index f54608ecceb..7ad01a04815 100644
--- a/db/migrate/20170301125302_add_printing_merge_request_link_enabled_to_project.rb
+++ b/db/migrate/20170301125302_add_printing_merge_request_link_enabled_to_project.rb
@@ -1,6 +1,7 @@
 # See http://doc.gitlab.com/ce/development/migration_style_guide.html
 # for more information on how to write migrations for GitLab.
 
+# rubocop:disable Migration/AddColumnWithDefaultToLargeTable
 class AddPrintingMergeRequestLinkEnabledToProject < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
   disable_ddl_transaction!
diff --git a/db/migrate/20170305180853_add_auto_cancel_pending_pipelines_to_project.rb b/db/migrate/20170305180853_add_auto_cancel_pending_pipelines_to_project.rb
index aa64f2dddca..f335e77fb5e 100644
--- a/db/migrate/20170305180853_add_auto_cancel_pending_pipelines_to_project.rb
+++ b/db/migrate/20170305180853_add_auto_cancel_pending_pipelines_to_project.rb
@@ -1,3 +1,4 @@
+# rubocop:disable Migration/AddColumnWithDefaultToLargeTable
 class AddAutoCancelPendingPipelinesToProject < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 
diff --git a/db/migrate/20170315174634_revert_add_notified_of_own_activity_to_users.rb b/db/migrate/20170315174634_revert_add_notified_of_own_activity_to_users.rb
index b39c0a3be0f..6c9fe19ca34 100644
--- a/db/migrate/20170315174634_revert_add_notified_of_own_activity_to_users.rb
+++ b/db/migrate/20170315174634_revert_add_notified_of_own_activity_to_users.rb
@@ -1,3 +1,4 @@
+# rubocop:disable Migration/AddColumnWithDefaultToLargeTable
 class RevertAddNotifiedOfOwnActivityToUsers < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
   disable_ddl_transaction!
diff --git a/rubocop/cop/migration/add_column_with_default_to_large_table.rb b/rubocop/cop/migration/add_column_with_default_to_large_table.rb
new file mode 100644
index 00000000000..2372e6b60ea
--- /dev/null
+++ b/rubocop/cop/migration/add_column_with_default_to_large_table.rb
@@ -0,0 +1,51 @@
+require_relative '../../migration_helpers'
+
+module RuboCop
+  module Cop
+    module Migration
+      # This cop checks for `add_column_with_default` on a table that's been
+      # explicitly blacklisted because of its size.
+      #
+      # Even though this helper performs the update in batches to avoid
+      # downtime, using it with tables with millions of rows still causes a
+      # significant delay in the deploy process and is best avoided.
+      #
+      # See https://gitlab.com/gitlab-com/infrastructure/issues/1602 for more
+      # information.
+      class AddColumnWithDefaultToLargeTable < RuboCop::Cop::Cop
+        include MigrationHelpers
+
+        MSG = 'Using `add_column_with_default` on the `%s` table will take a ' \
+          'long time to complete, and should be avoided unless absolutely ' \
+          'necessary'.freeze
+
+        LARGE_TABLES = %i[
+          events
+          issues
+          merge_requests
+          namespaces
+          notes
+          projects
+          routes
+          users
+        ].freeze
+
+        def_node_matcher :add_column_with_default?, <<~PATTERN
+          (send nil :add_column_with_default $(sym ...) ...)
+        PATTERN
+
+        def on_send(node)
+          return unless in_migration?(node)
+
+          matched = add_column_with_default?(node)
+          return unless matched
+
+          table = matched.to_a.first
+          return unless LARGE_TABLES.include?(table)
+
+          add_offense(node, :expression, format(MSG, table))
+        end
+      end
+    end
+  end
+end
diff --git a/rubocop/cop/migration/add_column_with_default.rb b/rubocop/cop/migration/reversible_add_column_with_default.rb
index 54a920d4b49..f413f06f39b 100644
--- a/rubocop/cop/migration/add_column_with_default.rb
+++ b/rubocop/cop/migration/reversible_add_column_with_default.rb
@@ -5,29 +5,30 @@ module RuboCop
     module Migration
       # Cop that checks if `add_column_with_default` is used with `up`/`down` methods
       # and not `change`.
-      class AddColumnWithDefault < RuboCop::Cop::Cop
+      class ReversibleAddColumnWithDefault < RuboCop::Cop::Cop
         include MigrationHelpers
 
+        def_node_matcher :add_column_with_default?, <<~PATTERN
+          (send nil :add_column_with_default $...)
+        PATTERN
+
+        def_node_matcher :defines_change?, <<~PATTERN
+          (def :change ...)
+        PATTERN
+
         MSG = '`add_column_with_default` is not reversible so you must manually define ' \
           'the `up` and `down` methods in your migration class, using `remove_column` in `down`'.freeze
 
         def on_send(node)
           return unless in_migration?(node)
-
-          name = node.children[1]
-
-          return unless name == :add_column_with_default
+          return unless add_column_with_default?(node)
 
           node.each_ancestor(:def) do |def_node|
-            next unless method_name(def_node) == :change
+            next unless defines_change?(def_node)
 
             add_offense(def_node, :name)
           end
         end
-
-        def method_name(node)
-          node.children.first
-        end
       end
     end
   end
diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb
index d580aa6857a..4ff204f939e 100644
--- a/rubocop/rubocop.rb
+++ b/rubocop/rubocop.rb
@@ -1,9 +1,10 @@
 require_relative 'cop/custom_error_class'
 require_relative 'cop/gem_fetcher'
 require_relative 'cop/migration/add_column'
-require_relative 'cop/migration/add_column_with_default'
+require_relative 'cop/migration/add_column_with_default_to_large_table'
 require_relative 'cop/migration/add_concurrent_foreign_key'
 require_relative 'cop/migration/add_concurrent_index'
 require_relative 'cop/migration/add_index'
 require_relative 'cop/migration/remove_concurrent_index'
 require_relative 'cop/migration/remove_index'
+require_relative 'cop/migration/reversible_add_column_with_default'
diff --git a/spec/rubocop/cop/migration/add_column_with_default_to_large_table_spec.rb b/spec/rubocop/cop/migration/add_column_with_default_to_large_table_spec.rb
new file mode 100644
index 00000000000..07cb3fc4a2e
--- /dev/null
+++ b/spec/rubocop/cop/migration/add_column_with_default_to_large_table_spec.rb
@@ -0,0 +1,44 @@
+require 'spec_helper'
+
+require 'rubocop'
+require 'rubocop/rspec/support'
+
+require_relative '../../../../rubocop/cop/migration/add_column_with_default_to_large_table'
+
+describe RuboCop::Cop::Migration::AddColumnWithDefaultToLargeTable do
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+
+  context 'in migration' do
+    before do
+      allow(cop).to receive(:in_migration?).and_return(true)
+    end
+
+    described_class::LARGE_TABLES.each do |table|
+      it "registers an offense for the #{table} table" do
+        inspect_source(cop, "add_column_with_default :#{table}, :column, default: true")
+
+        aggregate_failures do
+          expect(cop.offenses.size).to eq(1)
+          expect(cop.offenses.map(&:line)).to eq([1])
+        end
+      end
+    end
+
+    it 'registers no offense for non-blacklisted tables' do
+      inspect_source(cop, "add_column_with_default :table, :column, default: true")
+
+      expect(cop.offenses).to be_empty
+    end
+  end
+
+  context 'outside of migration' do
+    it 'registers no offense' do
+      table = described_class::LARGE_TABLES.sample
+      inspect_source(cop, "add_column_with_default :#{table}, :column, default: true")
+
+      expect(cop.offenses).to be_empty
+    end
+  end
+end
diff --git a/spec/rubocop/cop/migration/add_column_with_default_spec.rb b/spec/rubocop/cop/migration/reversible_add_column_with_default_spec.rb
index 6b9b6b19650..3723d635083 100644
--- a/spec/rubocop/cop/migration/add_column_with_default_spec.rb
+++ b/spec/rubocop/cop/migration/reversible_add_column_with_default_spec.rb
@@ -3,9 +3,9 @@ require 'spec_helper'
 require 'rubocop'
 require 'rubocop/rspec/support'
 
-require_relative '../../../../rubocop/cop/migration/add_column_with_default'
+require_relative '../../../../rubocop/cop/migration/reversible_add_column_with_default'
 
-describe RuboCop::Cop::Migration::AddColumnWithDefault do
+describe RuboCop::Cop::Migration::ReversibleAddColumnWithDefault do
   include CopHelper
 
   subject(:cop) { described_class.new }