Commit e714d569 authored by Yorick Peterse's avatar Yorick Peterse

Merge branch 'dm-add-concurrent-index-cop' into 'master'

Add cop to ensure reversibility of add_concurrent_index

See merge request !9771
parents 51cddc96 e3ddd871
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
......@@ -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
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
......@@ -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
......@@ -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
......@@ -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
......@@ -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
......@@ -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
......@@ -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
......@@ -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
......@@ -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
......@@ -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
......@@ -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
......@@ -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
......@@ -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
......@@ -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
......@@ -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
......@@ -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
......@@ -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
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
......@@ -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'
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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment