diff --git a/db/migrate/20160615142710_add_index_on_requested_at_to_members.rb b/db/migrate/20160615142710_add_index_on_requested_at_to_members.rb index 63f7392e54fc073445b86680931dc61c21e1fa39..7a8ed99c68f01cd1b0ac34179ad0990d847319cb 100644 --- a/db/migrate/20160615142710_add_index_on_requested_at_to_members.rb +++ b/db/migrate/20160615142710_add_index_on_requested_at_to_members.rb @@ -1,9 +1,15 @@ class AddIndexOnRequestedAtToMembers < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers + DOWNTIME = false + disable_ddl_transaction! - def change + def up add_concurrent_index :members, :requested_at end + + def down + remove_index :members, :requested_at if index_exists? :members, :requested_at + end end diff --git a/db/migrate/20160620115026_add_index_on_runners_locked.rb b/db/migrate/20160620115026_add_index_on_runners_locked.rb index dfa5110dea4b5ce80075c8c59573311a47f07144..6ca486c63d1eac8dea575b17cd3dd2d89b995a0a 100644 --- a/db/migrate/20160620115026_add_index_on_runners_locked.rb +++ b/db/migrate/20160620115026_add_index_on_runners_locked.rb @@ -4,9 +4,15 @@ class AddIndexOnRunnersLocked < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers + DOWNTIME = false + disable_ddl_transaction! - def change + def up add_concurrent_index :ci_runners, :locked end + + def down + remove_index :ci_runners, :locked if index_exists? :ci_runners, :locked + end end diff --git a/db/migrate/20160715134306_add_index_for_pipeline_user_id.rb b/db/migrate/20160715134306_add_index_for_pipeline_user_id.rb index 7c991c6d998d29f5f4124eeda64ac67fb1da9911..a05a4c679e3e97eef2b674bbbc6a8f1d3086a203 100644 --- a/db/migrate/20160715134306_add_index_for_pipeline_user_id.rb +++ b/db/migrate/20160715134306_add_index_for_pipeline_user_id.rb @@ -1,9 +1,15 @@ class AddIndexForPipelineUserId < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers + DOWNTIME = false + disable_ddl_transaction! - def change + def up add_concurrent_index :ci_commits, :user_id end + + def down + remove_index :ci_commits, :user_id if index_exists? :ci_commits, :user_id + end end diff --git a/db/migrate/20160805041956_add_deleted_at_to_namespaces.rb b/db/migrate/20160805041956_add_deleted_at_to_namespaces.rb index a853de3abfbed5842ea423efbe77c077d0163028..3f074723b4ac59ac84f4f1380c1cf5ccc983f606 100644 --- a/db/migrate/20160805041956_add_deleted_at_to_namespaces.rb +++ b/db/migrate/20160805041956_add_deleted_at_to_namespaces.rb @@ -5,8 +5,15 @@ class AddDeletedAtToNamespaces < ActiveRecord::Migration disable_ddl_transaction! - def change + def up add_column :namespaces, :deleted_at, :datetime + add_concurrent_index :namespaces, :deleted_at end + + def down + remove_index :namespaces, :deleted_at if index_exists? :namespaces, :deleted_at + + remove_column :namespaces, :deleted_at + end end diff --git a/db/migrate/20160808085602_add_index_for_build_token.rb b/db/migrate/20160808085602_add_index_for_build_token.rb index 10ef42afce18c316cb54296039c8b792dae65c62..6c5d7268e7201ca9a1b9be90d3224bfb147935c1 100644 --- a/db/migrate/20160808085602_add_index_for_build_token.rb +++ b/db/migrate/20160808085602_add_index_for_build_token.rb @@ -6,7 +6,11 @@ class AddIndexForBuildToken < ActiveRecord::Migration disable_ddl_transaction! - def change + def up add_concurrent_index :ci_builds, :token, unique: true end + + def down + remove_index :ci_builds, :token, unique: true if index_exists? :ci_builds, :token, unique: true + end end diff --git a/db/migrate/20160819221631_add_index_to_note_discussion_id.rb b/db/migrate/20160819221631_add_index_to_note_discussion_id.rb index b6e8bb18e7bf098c17d015323e37e4031457de2b..8f693e97a58959f86db902665c307aea25bce610 100644 --- a/db/migrate/20160819221631_add_index_to_note_discussion_id.rb +++ b/db/migrate/20160819221631_add_index_to_note_discussion_id.rb @@ -8,7 +8,11 @@ class AddIndexToNoteDiscussionId < ActiveRecord::Migration disable_ddl_transaction! - def change + def up add_concurrent_index :notes, :discussion_id end + + def down + remove_index :notes, :discussion_id if index_exists? :notes, :discussion_id + end end diff --git a/db/migrate/20160819232256_add_incoming_email_token_to_users.rb b/db/migrate/20160819232256_add_incoming_email_token_to_users.rb index f2cf956adc96faa27c3e52891c1ac9f7a2d6113a..bcad3416d043a6bd95685d7813c7fa94550d301e 100644 --- a/db/migrate/20160819232256_add_incoming_email_token_to_users.rb +++ b/db/migrate/20160819232256_add_incoming_email_token_to_users.rb @@ -9,8 +9,15 @@ class AddIncomingEmailTokenToUsers < ActiveRecord::Migration disable_ddl_transaction! - def change + def up add_column :users, :incoming_email_token, :string + add_concurrent_index :users, :incoming_email_token end + + def down + remove_index :users, :incoming_email_token if index_exists? :users, :incoming_email_token + + remove_column :users, :incoming_email_token + end end diff --git a/db/migrate/20160919145149_add_group_id_to_labels.rb b/db/migrate/20160919145149_add_group_id_to_labels.rb index d10f3a6d1046f765ca0283b52a7c6360d4b9a83f..828b6afddb1b4b44d167a4ffebf24387e01cf152 100644 --- a/db/migrate/20160919145149_add_group_id_to_labels.rb +++ b/db/migrate/20160919145149_add_group_id_to_labels.rb @@ -5,9 +5,15 @@ class AddGroupIdToLabels < ActiveRecord::Migration disable_ddl_transaction! - def change + def up add_column :labels, :group_id, :integer add_foreign_key :labels, :namespaces, column: :group_id, on_delete: :cascade # rubocop: disable Migration/AddConcurrentForeignKey add_concurrent_index :labels, :group_id end + + def down + remove_index :labels, :group_id if index_exists? :labels, :group_id + remove_foreign_key :labels, :namespaces, column: :group_id + remove_column :labels, :group_id + end end diff --git a/db/migrate/20160920160832_add_index_to_labels_title.rb b/db/migrate/20160920160832_add_index_to_labels_title.rb index b5de552b98cfb3e76e410f6bd089bdd65837fe82..19f7b1076a7bece2aad21f925738ebf310373bbb 100644 --- a/db/migrate/20160920160832_add_index_to_labels_title.rb +++ b/db/migrate/20160920160832_add_index_to_labels_title.rb @@ -5,7 +5,11 @@ class AddIndexToLabelsTitle < ActiveRecord::Migration disable_ddl_transaction! - def change + def up add_concurrent_index :labels, :title end + + def down + remove_index :labels, :title if index_exists? :labels, :title + end end diff --git a/db/migrate/20161020083353_add_pipeline_id_to_merge_request_metrics.rb b/db/migrate/20161020083353_add_pipeline_id_to_merge_request_metrics.rb index 2abfe47b7766a2da5409e544d2baf919680dd886..ad3eb4a26f90424d56ca3dc2c06c78dd858eb0c2 100644 --- a/db/migrate/20161020083353_add_pipeline_id_to_merge_request_metrics.rb +++ b/db/migrate/20161020083353_add_pipeline_id_to_merge_request_metrics.rb @@ -25,9 +25,15 @@ class AddPipelineIdToMergeRequestMetrics < ActiveRecord::Migration # comments: # disable_ddl_transaction! - def change + def up add_column :merge_request_metrics, :pipeline_id, :integer - add_concurrent_index :merge_request_metrics, :pipeline_id add_foreign_key :merge_request_metrics, :ci_commits, column: :pipeline_id, on_delete: :cascade # rubocop: disable Migration/AddConcurrentForeignKey + add_concurrent_index :merge_request_metrics, :pipeline_id + end + + def down + remove_index :merge_request_metrics, :pipeline_id if index_exists? :merge_request_metrics, :pipeline_id + remove_foreign_key :merge_request_metrics, :ci_commits, column: :pipeline_id + remove_column :merge_request_metrics, :pipeline_id end end diff --git a/db/migrate/20161106185620_add_project_import_data_project_index.rb b/db/migrate/20161106185620_add_project_import_data_project_index.rb index 750a6a8c51ebab2aef1adc088a9593e5e43423dd..94b8ddd46f53739d8136a0c63678673d9f298b31 100644 --- a/db/migrate/20161106185620_add_project_import_data_project_index.rb +++ b/db/migrate/20161106185620_add_project_import_data_project_index.rb @@ -6,7 +6,11 @@ class AddProjectImportDataProjectIndex < ActiveRecord::Migration disable_ddl_transaction! - def change + def up add_concurrent_index :project_import_data, :project_id end + + def down + remove_index :project_import_data, :project_id if index_exists? :project_import_data, :project_id + end end diff --git a/db/migrate/20161124111395_add_index_to_parent_id.rb b/db/migrate/20161124111395_add_index_to_parent_id.rb index eab74c01dfd89b5fc400b11bc3d17f44f022077d..73f9d92bb2275e365251f9bb26917e76003c023e 100644 --- a/db/migrate/20161124111395_add_index_to_parent_id.rb +++ b/db/migrate/20161124111395_add_index_to_parent_id.rb @@ -8,7 +8,11 @@ class AddIndexToParentId < ActiveRecord::Migration disable_ddl_transaction! - def change + def up add_concurrent_index(:namespaces, [:parent_id, :id], unique: true) end + + def down + remove_index :namespaces, [:parent_id, :id] if index_exists? :namespaces, [:parent_id, :id] + end end diff --git a/db/migrate/20161202152035_add_index_to_routes.rb b/db/migrate/20161202152035_add_index_to_routes.rb index 4a51337bda638c5e004b813ea90e17fca08911b4..6d6c8906204a24173d439d4a4fadb5e626c3a4cf 100644 --- a/db/migrate/20161202152035_add_index_to_routes.rb +++ b/db/migrate/20161202152035_add_index_to_routes.rb @@ -9,8 +9,13 @@ class AddIndexToRoutes < ActiveRecord::Migration disable_ddl_transaction! - def change + def up add_concurrent_index(:routes, :path, unique: true) add_concurrent_index(:routes, [:source_type, :source_id], unique: true) end + + def down + remove_index(:routes, :path) if index_exists? :routes, :path + remove_index(:routes, [:source_type, :source_id]) if index_exists? :routes, [:source_type, :source_id] + end end diff --git a/db/migrate/20161209153400_add_unique_index_for_environment_slug.rb b/db/migrate/20161209153400_add_unique_index_for_environment_slug.rb index e9fcef1cd45601d53f132d211c7a743a3d2cdbcd..d7ef1aa83d9f211ddd90ffc0efc2719dd13036dc 100644 --- a/db/migrate/20161209153400_add_unique_index_for_environment_slug.rb +++ b/db/migrate/20161209153400_add_unique_index_for_environment_slug.rb @@ -9,7 +9,11 @@ class AddUniqueIndexForEnvironmentSlug < ActiveRecord::Migration disable_ddl_transaction! - def change + def up add_concurrent_index :environments, [:project_id, :slug], unique: true end + + def down + remove_index :environments, [:project_id, :slug], unique: true if index_exists? :environments, [:project_id, :slug] + end end diff --git a/db/migrate/20170204181513_add_index_to_labels_for_type_and_project.rb b/db/migrate/20170204181513_add_index_to_labels_for_type_and_project.rb index 8f944930807afc93fa90acbab92c6cb7ba0f1621..31ef458c44f6ef3bf2ecbf7fd4269ec633b044ed 100644 --- a/db/migrate/20170204181513_add_index_to_labels_for_type_and_project.rb +++ b/db/migrate/20170204181513_add_index_to_labels_for_type_and_project.rb @@ -5,7 +5,11 @@ class AddIndexToLabelsForTypeAndProject < ActiveRecord::Migration disable_ddl_transaction! - def change + def up add_concurrent_index :labels, [:type, :project_id] end + + def down + remove_index :labels, [:type, :project_id] if index_exists? :labels, [:type, :project_id] + end end diff --git a/db/migrate/20170210062829_add_index_to_labels_for_title_and_project.rb b/db/migrate/20170210062829_add_index_to_labels_for_title_and_project.rb index f922ed209aa3f2618f17bf02bcc90650f5d4f847..70fb0ef12f9bf528b97b7e7e0f6d5f432020515b 100644 --- a/db/migrate/20170210062829_add_index_to_labels_for_title_and_project.rb +++ b/db/migrate/20170210062829_add_index_to_labels_for_title_and_project.rb @@ -5,8 +5,13 @@ class AddIndexToLabelsForTitleAndProject < ActiveRecord::Migration disable_ddl_transaction! - def change + def up add_concurrent_index :labels, :title add_concurrent_index :labels, :project_id end + + def down + remove_index :labels, :title if index_exists? :labels, :title + remove_index :labels, :project_id if index_exists? :labels, :project_id + end end diff --git a/db/migrate/20170210075922_add_index_to_ci_trigger_requests_for_commit_id.rb b/db/migrate/20170210075922_add_index_to_ci_trigger_requests_for_commit_id.rb index 61e49c14fc0d2edb9344cf1a34975577e2798661..07d4f8af27fe5fa59c06e04579b5a224af339058 100644 --- a/db/migrate/20170210075922_add_index_to_ci_trigger_requests_for_commit_id.rb +++ b/db/migrate/20170210075922_add_index_to_ci_trigger_requests_for_commit_id.rb @@ -5,7 +5,11 @@ class AddIndexToCiTriggerRequestsForCommitId < ActiveRecord::Migration disable_ddl_transaction! - def change + def up add_concurrent_index :ci_trigger_requests, :commit_id end + + def down + remove_index :ci_trigger_requests, :commit_id if index_exists? :ci_trigger_requests, :commit_id + end end diff --git a/db/migrate/20170210103609_add_index_to_user_agent_detail.rb b/db/migrate/20170210103609_add_index_to_user_agent_detail.rb index c01753cfbd216721eaf7c7a88b4dd3b8e368be7f..2d8329b7862179b2e31883d63a356727776e3b6a 100644 --- a/db/migrate/20170210103609_add_index_to_user_agent_detail.rb +++ b/db/migrate/20170210103609_add_index_to_user_agent_detail.rb @@ -8,7 +8,11 @@ class AddIndexToUserAgentDetail < ActiveRecord::Migration disable_ddl_transaction! - def change - add_concurrent_index(:user_agent_details, [:subject_id, :subject_type]) + def up + add_concurrent_index :user_agent_details, [:subject_id, :subject_type] + end + + def down + remove_index :user_agent_details, [:subject_id, :subject_type] if index_exists? :user_agent_details, [:subject_id, :subject_type] end end diff --git a/db/migrate/20170216135621_add_index_for_latest_successful_pipeline.rb b/db/migrate/20170216135621_add_index_for_latest_successful_pipeline.rb index 7b1e687977b6d6463a1f5366b494fedbe6b451b3..65adc90c2c11a8baaa5dcc1f7adf9dcea89bd8bd 100644 --- a/db/migrate/20170216135621_add_index_for_latest_successful_pipeline.rb +++ b/db/migrate/20170216135621_add_index_for_latest_successful_pipeline.rb @@ -4,7 +4,11 @@ class AddIndexForLatestSuccessfulPipeline < ActiveRecord::Migration disable_ddl_transaction! - def change - add_concurrent_index(:ci_commits, [:gl_project_id, :ref, :status]) + def up + add_concurrent_index :ci_commits, [:gl_project_id, :ref, :status] + end + + def down + remove_index :ci_commits, [:gl_project_id, :ref, :status] if index_exists? :ci_commits, [:gl_project_id, :ref, :status] end end diff --git a/rubocop/cop/migration/add_concurrent_index.rb b/rubocop/cop/migration/add_concurrent_index.rb new file mode 100644 index 0000000000000000000000000000000000000000..332fb7dcbd79689d171f1c569418215dd35bb272 --- /dev/null +++ b/rubocop/cop/migration/add_concurrent_index.rb @@ -0,0 +1,34 @@ +require_relative '../../migration_helpers' + +module RuboCop + module Cop + module Migration + # Cop that checks if `add_concurrent_index` is used with `up`/`down` methods + # and not `change`. + class AddConcurrentIndex < RuboCop::Cop::Cop + include MigrationHelpers + + MSG = '`add_concurrent_index` is not reversible so you must manually define ' \ + 'the `up` and `down` methods in your migration class, using `remove_index` in `down`'.freeze + + def on_send(node) + return unless in_migration?(node) + + name = node.children[1] + + return unless name == :add_concurrent_index + + 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/rubocop.rb b/rubocop/rubocop.rb index ea8e0f64b0d97ad01a1645f8d9a1b4cb0118fcd7..a50a522cf9ded8270543979b180e4512ab43e1c1 100644 --- a/rubocop/rubocop.rb +++ b/rubocop/rubocop.rb @@ -3,4 +3,5 @@ require_relative 'cop/gem_fetcher' require_relative 'cop/migration/add_column' require_relative 'cop/migration/add_column_with_default' require_relative 'cop/migration/add_concurrent_foreign_key' +require_relative 'cop/migration/add_concurrent_index' require_relative 'cop/migration/add_index' diff --git a/spec/rubocop/cop/migration/add_concurrent_index_spec.rb b/spec/rubocop/cop/migration/add_concurrent_index_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..19a5718b0b16ae797f56060c87a5efc3797a1d75 --- /dev/null +++ b/spec/rubocop/cop/migration/add_concurrent_index_spec.rb @@ -0,0 +1,41 @@ +require 'spec_helper' + +require 'rubocop' +require 'rubocop/rspec/support' + +require_relative '../../../../rubocop/cop/migration/add_concurrent_index' + +describe RuboCop::Cop::Migration::AddConcurrentIndex 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_concurrent_index is used inside a change method' do + inspect_source(cop, 'def change; add_concurrent_index :table, :column; 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_concurrent_index is used inside an up method' do + inspect_source(cop, 'def up; add_concurrent_index :table, :column; 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_concurrent_index :table, :column; end') + + expect(cop.offenses.size).to eq(0) + end + end +end