From abc9548f8afae6d666942f37715d2f7f8258d63b Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 8 Feb 2017 16:17:15 -0600 Subject: Add cop that checks if add_column_with_default is used with up/down methods --- rubocop/cop/migration/add_column.rb | 50 ++++++++++++++++++++++ rubocop/cop/migration/add_column_with_default.rb | 34 +++++++++++++++ rubocop/cop/migration/add_index.rb | 2 +- rubocop/cop/migration/column_with_default.rb | 50 ---------------------- rubocop/rubocop.rb | 3 +- .../cop/migration/add_column_with_default_spec.rb | 41 ++++++++++++++++++ 6 files changed, 128 insertions(+), 52 deletions(-) create mode 100644 rubocop/cop/migration/add_column.rb create mode 100644 rubocop/cop/migration/add_column_with_default.rb delete mode 100644 rubocop/cop/migration/column_with_default.rb create mode 100644 spec/rubocop/cop/migration/add_column_with_default_spec.rb diff --git a/rubocop/cop/migration/add_column.rb b/rubocop/cop/migration/add_column.rb new file mode 100644 index 00000000000..d5c8eeeea1f --- /dev/null +++ b/rubocop/cop/migration/add_column.rb @@ -0,0 +1,50 @@ +module RuboCop + module Cop + module Migration + # Cop that checks if columns are added in a way that doesn't require + # downtime. + 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' + + def on_send(node) + return unless in_migration?(node) + + name = node.children[1] + + return unless name == :add_column + + # Ignore whitelisted tables. + return if table_whitelisted?(node.children[2]) + + opts = node.children.last + + return unless opts && opts.type == :hash + + opts.each_node(:pair) do |pair| + if hash_key_type(pair) == :sym && hash_key_name(pair) == :default + add_offense(node, :selector) + end + end + end + + def table_whitelisted?(symbol) + symbol && symbol.type == :sym && + WHITELISTED_TABLES.include?(symbol.children[0]) + end + + def hash_key_type(pair) + pair.children[0].type + end + + def hash_key_name(pair) + pair.children[0].children[0] + end + end + end + end +end 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..506af97866f 100644 --- a/rubocop/cop/migration/add_index.rb +++ b/rubocop/cop/migration/add_index.rb @@ -5,7 +5,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/cop/migration/column_with_default.rb b/rubocop/cop/migration/column_with_default.rb deleted file mode 100644 index 97ee8b11044..00000000000 --- a/rubocop/cop/migration/column_with_default.rb +++ /dev/null @@ -1,50 +0,0 @@ -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 - include MigrationHelpers - - WHITELISTED_TABLES = [:application_settings] - - MSG = 'add_column with a default value requires downtime, ' \ - 'use add_column_with_default instead' - - def on_send(node) - return unless in_migration?(node) - - name = node.children[1] - - return unless name == :add_column - - # Ignore whitelisted tables. - return if table_whitelisted?(node.children[2]) - - opts = node.children.last - - return unless opts && opts.type == :hash - - opts.each_node(:pair) do |pair| - if hash_key_type(pair) == :sym && hash_key_name(pair) == :default - add_offense(node, :selector) - end - end - end - - def table_whitelisted?(symbol) - symbol && symbol.type == :sym && - WHITELISTED_TABLES.include?(symbol.children[0]) - end - - def hash_key_type(pair) - pair.children[0].type - end - - def hash_key_name(pair) - pair.children[0].children[0] - end - end - end - end -end diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb index 7f20754ee51..3c596bff7ea 100644 --- a/rubocop/rubocop.rb +++ b/rubocop/rubocop.rb @@ -1,4 +1,5 @@ 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 -- cgit v1.2.1 From b0afed1aac83996747820728ee2a5d618f5caf3c Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 8 Feb 2017 16:31:15 -0600 Subject: Explicitly require rubocop migration_helpers --- rubocop/cop/migration/add_column.rb | 2 ++ rubocop/cop/migration/add_index.rb | 2 ++ rubocop/rubocop.rb | 1 - 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/rubocop/cop/migration/add_column.rb b/rubocop/cop/migration/add_column.rb index d5c8eeeea1f..1490fcdd814 100644 --- a/rubocop/cop/migration/add_column.rb +++ b/rubocop/cop/migration/add_column.rb @@ -1,3 +1,5 @@ +require_relative '../../migration_helpers' + module RuboCop module Cop module Migration diff --git a/rubocop/cop/migration/add_index.rb b/rubocop/cop/migration/add_index.rb index 506af97866f..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 diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb index 3c596bff7ea..3e292a4527c 100644 --- a/rubocop/rubocop.rb +++ b/rubocop/rubocop.rb @@ -1,4 +1,3 @@ -require_relative 'migration_helpers' require_relative 'cop/migration/add_index' require_relative 'cop/migration/add_column' require_relative 'cop/migration/add_column_with_default' -- cgit v1.2.1 From 63c5b541a3be014341b3b239cb795479026e9a99 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 8 Feb 2017 18:12:16 -0600 Subject: Satisfy the new rubocop :) --- ...20160519203051_add_developers_can_merge_to_protected_branches.rb | 6 +++++- db/migrate/20160801163709_add_submitted_as_ham_to_spam_logs.rb | 6 +++++- 2 files changed, 10 insertions(+), 2 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..6ca97e39a3d 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 @@ -3,7 +3,11 @@ class AddDevelopersCanMergeToProtectedBranches < ActiveRecord::Migration 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 -- cgit v1.2.1 From 29a32cfc229798f8214b57dd03d74b3072544c8b Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 8 Feb 2017 19:20:27 -0600 Subject: Migration does not require downtime --- .../20160519203051_add_developers_can_merge_to_protected_branches.rb | 2 ++ 1 file changed, 2 insertions(+) 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 6ca97e39a3d..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,6 +1,8 @@ class AddDevelopersCanMergeToProtectedBranches < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers + DOWNTIME = false + disable_ddl_transaction! def up -- cgit v1.2.1