Merge branch 'dm-add-column-with-default-cop' into 'master'

Add cop that checks if add_column_with_default is used with up/down methods

See merge request !9077

7 files changed, 97 insertions, 8 deletions

diff --git a/db/migrate/20160519203051_add_developers_can_merge_to_protected_branches.rb b/db/migrate/20160519203051_add_developers_can_merge_to_protected_branches.rb
index 15ad8e8bcbb..ac50035eba4 100644
--- a/db/migrate/20160519203051_add_developers_can_merge_to_protected_branches.rb
+++ b/db/migrate/20160519203051_add_developers_can_merge_to_protected_branches.rb
@@ -1,9 +1,15 @@
 class AddDevelopersCanMergeToProtectedBranches < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 
+  DOWNTIME = false
+
   disable_ddl_transaction!
 
-  def change
+  def up
     add_column_with_default :protected_branches, :developers_can_merge, :boolean, default: false, allow_null: false
   end
+
+  def down
+    remove_column :protected_branches, :developers_can_merge
+  end
 end
diff --git a/db/migrate/20160801163709_add_submitted_as_ham_to_spam_logs.rb b/db/migrate/20160801163709_add_submitted_as_ham_to_spam_logs.rb
index 296f1dfac7b..20a77000ba8 100644
--- a/db/migrate/20160801163709_add_submitted_as_ham_to_spam_logs.rb
+++ b/db/migrate/20160801163709_add_submitted_as_ham_to_spam_logs.rb
@@ -14,7 +14,11 @@ class AddSubmittedAsHamToSpamLogs < ActiveRecord::Migration
 
   disable_ddl_transaction!
 
-  def change
+  def up
     add_column_with_default :spam_logs, :submitted_as_ham, :boolean, default: false
   end
+
+  def down
+    remove_column :spam_logs, :submitted_as_ham
+  end
 end
diff --git a/rubocop/cop/migration/column_with_default.rb b/rubocop/cop/migration/add_column.rb
index 97ee8b11044..1490fcdd814 100644
--- a/rubocop/cop/migration/column_with_default.rb
+++ b/rubocop/cop/migration/add_column.rb
@@ -1,15 +1,17 @@
+require_relative '../../migration_helpers'
+
 module RuboCop
   module Cop
     module Migration
       # Cop that checks if columns are added in a way that doesn't require
       # downtime.
-      class ColumnWithDefault < RuboCop::Cop::Cop
+      class AddColumn < RuboCop::Cop::Cop
         include MigrationHelpers
 
         WHITELISTED_TABLES = [:application_settings]
 
-        MSG = 'add_column with a default value requires downtime, ' \
-          'use add_column_with_default instead'
+        MSG = '`add_column` with a default value requires downtime, ' \
+          'use `add_column_with_default` instead'
 
         def on_send(node)
           return unless in_migration?(node)
diff --git a/rubocop/cop/migration/add_column_with_default.rb b/rubocop/cop/migration/add_column_with_default.rb
new file mode 100644
index 00000000000..747d7caf1ef
--- /dev/null
+++ b/rubocop/cop/migration/add_column_with_default.rb
@@ -0,0 +1,34 @@
+require_relative '../../migration_helpers'
+
+module RuboCop
+  module Cop
+    module Migration
+      # Cop that checks if `add_column_with_default` is used with `up`/`down` methods
+      # and not `change`.
+      class AddColumnWithDefault < RuboCop::Cop::Cop
+        include MigrationHelpers
+
+        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`'
+
+        def on_send(node)
+          return unless in_migration?(node)
+
+          name = node.children[1]
+
+          return unless name == :add_column_with_default
+
+          node.each_ancestor(:def) do |def_node|
+            next unless method_name(def_node) == :change
+
+            add_offense(def_node, :name)
+          end
+        end
+
+        def method_name(node)
+          node.children.first
+        end
+      end
+    end
+  end
+end
diff --git a/rubocop/cop/migration/add_index.rb b/rubocop/cop/migration/add_index.rb
index d9247a1f7ea..5e6766f6994 100644
--- a/rubocop/cop/migration/add_index.rb
+++ b/rubocop/cop/migration/add_index.rb
@@ -1,3 +1,5 @@
+require_relative '../../migration_helpers'
+
 module RuboCop
   module Cop
     module Migration
@@ -5,7 +7,7 @@ module RuboCop
       class AddIndex < RuboCop::Cop::Cop
         include MigrationHelpers
 
-        MSG = 'add_index requires downtime, use add_concurrent_index instead'
+        MSG = '`add_index` requires downtime, use `add_concurrent_index` instead'
 
         def on_def(node)
           return unless in_migration?(node)
diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb
index 7f20754ee51..3e292a4527c 100644
--- a/rubocop/rubocop.rb
+++ b/rubocop/rubocop.rb
@@ -1,4 +1,4 @@
-require_relative 'migration_helpers'
 require_relative 'cop/migration/add_index'
-require_relative 'cop/migration/column_with_default'
+require_relative 'cop/migration/add_column'
+require_relative 'cop/migration/add_column_with_default'
 require_relative 'cop/gem_fetcher'
diff --git a/spec/rubocop/cop/migration/add_column_with_default_spec.rb b/spec/rubocop/cop/migration/add_column_with_default_spec.rb
new file mode 100644
index 00000000000..6b9b6b19650
--- /dev/null
+++ b/spec/rubocop/cop/migration/add_column_with_default_spec.rb
@@ -0,0 +1,41 @@
+require 'spec_helper'
+
+require 'rubocop'
+require 'rubocop/rspec/support'
+
+require_relative '../../../../rubocop/cop/migration/add_column_with_default'
+
+describe RuboCop::Cop::Migration::AddColumnWithDefault do
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+
+  context 'in migration' do
+    before do
+      allow(cop).to receive(:in_migration?).and_return(true)
+    end
+
+    it 'registers an offense when add_column_with_default is used inside a change method' do
+      inspect_source(cop, 'def change; add_column_with_default :table, :column, default: false; end')
+
+      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 add_column_with_default is used inside an up method' do
+      inspect_source(cop, 'def up; add_column_with_default :table, :column, default: false; end')
+
+      expect(cop.offenses.size).to eq(0)
+    end
+  end
+
+  context 'outside of migration' do
+    it 'registers no offense' do
+      inspect_source(cop, 'def change; add_column_with_default :table, :column, default: false; end')
+
+      expect(cop.offenses.size).to eq(0)
+    end
+  end
+end