diff --git a/db/migrate/20160610194713_remove_deprecated_issues_tracker_columns_from_projects.rb b/db/migrate/20160610194713_remove_deprecated_issues_tracker_columns_from_projects.rb index 477b2106dead5e5d23b06f0c5bafb93d26476733..21b367711c3ab0b90169483893c8243976dcf4b3 100644 --- a/db/migrate/20160610194713_remove_deprecated_issues_tracker_columns_from_projects.rb +++ b/db/migrate/20160610194713_remove_deprecated_issues_tracker_columns_from_projects.rb @@ -1,3 +1,4 @@ +# rubocop:disable Migration/RemoveColumn class RemoveDeprecatedIssuesTrackerColumnsFromProjects < ActiveRecord::Migration def change remove_column :projects, :issues_tracker, :string, default: 'gitlab', null: false diff --git a/db/migrate/20160610301627_remove_notification_level_from_users.rb b/db/migrate/20160610301627_remove_notification_level_from_users.rb index 8afb14df2cf6b88232296d9ddcb23698f3d5b45a..356e53b4b23bdf9c99832de40f116d93cad708c3 100644 --- a/db/migrate/20160610301627_remove_notification_level_from_users.rb +++ b/db/migrate/20160610301627_remove_notification_level_from_users.rb @@ -1,3 +1,4 @@ +# rubocop:disable Migration/RemoveColumn class RemoveNotificationLevelFromUsers < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers diff --git a/db/migrate/20160705055809_remove_developers_can_push_from_protected_branches.rb b/db/migrate/20160705055809_remove_developers_can_push_from_protected_branches.rb index 52a9819c6287b97dbb8b933faf6e11df5d622012..058bd539e65c8d9e580d5763939a81dddf5bffec 100644 --- a/db/migrate/20160705055809_remove_developers_can_push_from_protected_branches.rb +++ b/db/migrate/20160705055809_remove_developers_can_push_from_protected_branches.rb @@ -1,3 +1,4 @@ +# 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. diff --git a/db/migrate/20160705055813_remove_developers_can_merge_from_protected_branches.rb b/db/migrate/20160705055813_remove_developers_can_merge_from_protected_branches.rb index 4a7bde7f9f33774f14e95bac9647bcd0b68b6629..d0e5da4d28b77e3f934020a8036c5cdbfc90ad23 100644 --- a/db/migrate/20160705055813_remove_developers_can_merge_from_protected_branches.rb +++ b/db/migrate/20160705055813_remove_developers_can_merge_from_protected_branches.rb @@ -1,3 +1,4 @@ +# 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. diff --git a/db/migrate/20160729173930_remove_project_id_from_spam_logs.rb b/db/migrate/20160729173930_remove_project_id_from_spam_logs.rb index e28ab31d629d049a458d00c24559dd7210511235..baf254c3bcc4a4ff09781b135004f2cad3dc7c11 100644 --- a/db/migrate/20160729173930_remove_project_id_from_spam_logs.rb +++ b/db/migrate/20160729173930_remove_project_id_from_spam_logs.rb @@ -1,3 +1,4 @@ +# 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. diff --git a/db/migrate/20160831223750_remove_features_enabled_from_projects.rb b/db/migrate/20160831223750_remove_features_enabled_from_projects.rb index aec709aaf59f203d1518f1a32e9972becfda6eb2..9eafd8b947766418f95394e6c29dbe38c5406e78 100644 --- a/db/migrate/20160831223750_remove_features_enabled_from_projects.rb +++ b/db/migrate/20160831223750_remove_features_enabled_from_projects.rb @@ -1,3 +1,4 @@ +# 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. diff --git a/db/migrate/20160913162434_remove_projects_pushes_since_gc.rb b/db/migrate/20160913162434_remove_projects_pushes_since_gc.rb index df7d922b8167657cc0de6f58ca348f2670d44f64..f32167037e0863a3c8c8f84e5f5ecc0b26e38f80 100644 --- a/db/migrate/20160913162434_remove_projects_pushes_since_gc.rb +++ b/db/migrate/20160913162434_remove_projects_pushes_since_gc.rb @@ -1,3 +1,4 @@ +# 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. diff --git a/db/migrate/20161018024550_remove_priority_from_labels.rb b/db/migrate/20161018024550_remove_priority_from_labels.rb index b7416cca6646432207bca38927097412b5136ce5..bc25a43526cc267cdef169678f3ddea7a8779add 100644 --- a/db/migrate/20161018024550_remove_priority_from_labels.rb +++ b/db/migrate/20161018024550_remove_priority_from_labels.rb @@ -1,3 +1,4 @@ +# rubocop:disable Migration/RemoveColumn class RemovePriorityFromLabels < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers diff --git a/db/migrate/20161201160452_migrate_project_statistics.rb b/db/migrate/20161201160452_migrate_project_statistics.rb index 82fbdf02444d04fab44dca8e1df33bf13c3ea80d..a547409aaa5418ed1fa41783e5258db28b5820e0 100644 --- a/db/migrate/20161201160452_migrate_project_statistics.rb +++ b/db/migrate/20161201160452_migrate_project_statistics.rb @@ -1,3 +1,4 @@ +# rubocop:disable Migration/RemoveColumn class MigrateProjectStatistics < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers diff --git a/db/migrate/20170222143500_remove_old_project_id_columns.rb b/db/migrate/20170222143500_remove_old_project_id_columns.rb index 268144a25529a1b9f0ddc62b307939ee13c56a26..9bed38a34443f67ebf6285e9cf3fd533fe075494 100644 --- a/db/migrate/20170222143500_remove_old_project_id_columns.rb +++ b/db/migrate/20170222143500_remove_old_project_id_columns.rb @@ -1,3 +1,4 @@ +# rubocop:disable Migration/RemoveColumn # rubocop:disable RemoveIndex class RemoveOldProjectIdColumns < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers diff --git a/db/migrate/20170301205639_remove_unused_ci_tables_and_columns.rb b/db/migrate/20170301205639_remove_unused_ci_tables_and_columns.rb index 1a77d5934a3eca0a8ce7c836308ef79a485ec48e..0535c2ddaf261fbb45d60fd2d579774c42fd9106 100644 --- a/db/migrate/20170301205639_remove_unused_ci_tables_and_columns.rb +++ b/db/migrate/20170301205639_remove_unused_ci_tables_and_columns.rb @@ -1,3 +1,4 @@ +# rubocop:disable Migration/RemoveColumn # rubocop:disable Migration/Datetime class RemoveUnusedCiTablesAndColumns < 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 807dfcb385df3bec6bcad10b0fd10ca39ac2d94c..9b9098d115df33c493e092c8ec1bb60521cdd258 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/RemoveColumn # rubocop:disable Migration/UpdateLargeTable class RevertAddNotifiedOfOwnActivityToUsers < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers diff --git a/db/migrate/20171106154015_remove_issues_branch_name.rb b/db/migrate/20171106154015_remove_issues_branch_name.rb index 3d08225c96de9f25124e30ef7cc29225b3db3e7d..162b6bafab43e6f6958553d2ae3e5ebcf9443dcc 100644 --- a/db/migrate/20171106154015_remove_issues_branch_name.rb +++ b/db/migrate/20171106154015_remove_issues_branch_name.rb @@ -1,3 +1,4 @@ +# 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. diff --git a/rubocop/cop/migration/remove_column.rb b/rubocop/cop/migration/remove_column.rb new file mode 100644 index 0000000000000000000000000000000000000000..e53eb2e07b2439c1ac7cc5b1adda3fca11c05ec9 --- /dev/null +++ b/rubocop/cop/migration/remove_column.rb @@ -0,0 +1,30 @@ +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 diff --git a/rubocop/migration_helpers.rb b/rubocop/migration_helpers.rb index c3473771178ca465594993594933c931bde1fca9..c066d424437257e0b540163f69432c00253a4d0b 100644 --- a/rubocop/migration_helpers.rb +++ b/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 diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb index 7621ea50da9380bdc690e2936d1daf65c054a1cc..eb52be3d731da92ac296abe20ba32bb9c4020507 100644 --- a/rubocop/rubocop.rb +++ b/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' diff --git a/spec/rubocop/cop/migration/remove_column_spec.rb b/spec/rubocop/cop/migration/remove_column_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..89112f017238506fea10b4c0ff90be7c1294ab25 --- /dev/null +++ b/spec/rubocop/cop/migration/remove_column_spec.rb @@ -0,0 +1,68 @@ +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