Commit 8ee6ad54 authored by Andreas Brandl's avatar Andreas Brandl Committed by Mayra Cabrera

Only allow add_reference for newly created tables

Relates to https://gitlab.com/gitlab-org/gitlab-ce/issues/67210
parent 24368908
......@@ -6,9 +6,9 @@ class AddTargetProjectIdToMergeTrains < ActiveRecord::Migration[5.1]
DOWNTIME = false
def change
# rubocop: disable Rails/NotNullColumn
# rubocop:disable Rails/NotNullColumn, Migration/AddReference
add_reference :merge_trains, :target_project, null: false, index: true, foreign_key: { on_delete: :cascade, to_table: :projects }, type: :integer
add_column :merge_trains, :target_branch, :text, null: false
# rubocop: enable Rails/NotNullColumn
# rubocop:enable Rails/NotNullColumn, Migration/AddReference
end
end
......@@ -4,7 +4,9 @@ class AddEnvironmentIdToClustersKubernetesNamespaces < ActiveRecord::Migration[5
DOWNTIME = false
def change
# rubocop:disable Migration/AddReference
add_reference :clusters_kubernetes_namespaces, :environment,
index: true, type: :bigint, foreign_key: { on_delete: :nullify }
# rubocop:enable Migration/AddReference
end
end
......@@ -4,7 +4,9 @@ class AddIssueIdToVersions < ActiveRecord::Migration[5.2]
DOWNTIME = false
def up
# rubocop:disable Migration/AddReference
add_reference :design_management_versions, :issue, index: true, foreign_key: { on_delete: :cascade }
# rubocop:enable Migration/AddReference
end
def down
......
......@@ -4,11 +4,13 @@ class AddColumnForSelfMonitoringProjectId < ActiveRecord::Migration[5.2]
DOWNTIME = false
def change
# rubocop:disable Migration/AddReference
add_reference(
:application_settings,
:instance_administration_project,
index: { name: 'index_applicationsettings_on_instance_administration_project_id' },
foreign_key: { to_table: :projects, on_delete: :nullify }
)
# rubocop:enable Migration/AddReference
end
end
......@@ -4,22 +4,62 @@ require_relative '../../migration_helpers'
module RuboCop
module Cop
module Migration
# Cop that checks if a foreign key constraint is added and require a index for it
# add_reference can only be used with newly created tables.
# Additionally, the cop here checks that we create an index for the foreign key, too.
class AddReference < RuboCop::Cop::Cop
include MigrationHelpers
MSG = '`add_reference` requires `index: true` or `index: { options... }`'
MSG = '`add_reference` requires downtime for existing tables, use `add_concurrent_foreign_key` instead. When used for new tables, `index: true` or `index: { options... } is required.`'
def on_send(node)
def on_def(node)
return unless in_migration?(node)
name = node.children[1]
new_tables = []
return unless name == :add_reference
node.each_descendant(:send) do |send_node|
first_arg = first_argument(send_node)
# The first argument of "create_table" / "add_reference" is the table
# name.
new_tables << first_arg if create_table?(send_node)
next if method_name(send_node) != :add_reference
# Using "add_reference" is fine for newly created tables as there's no
# data in these tables yet.
if existing_table?(new_tables, first_arg)
add_offense(send_node, location: :selector)
end
# We require an index on the foreign key column.
if index_missing?(node)
add_offense(send_node, location: :selector)
end
end
end
private
def existing_table?(new_tables, table)
!new_tables.include?(table)
end
def create_table?(node)
method_name(node) == :create_table
end
def method_name(node)
node.children[1]
end
def first_argument(node)
node.children[2]
end
def index_missing?(node)
opts = node.children.last
add_offense(node, location: :selector) unless opts && opts.type == :hash
return true if opts && opts.type == :hash
index_present = false
......@@ -27,11 +67,9 @@ module RuboCop
index_present ||= index_enabled?(pair)
end
add_offense(node, location: :selector) unless index_present
!index_present
end
private
def index_enabled?(pair)
return unless hash_key_type(pair) == :sym
return unless hash_key_name(pair) == :index
......
......@@ -25,38 +25,86 @@ describe RuboCop::Cop::Migration::AddReference do
allow(cop).to receive(:in_migration?).and_return(true)
end
it 'registers an offense when using add_reference without index' do
expect_offense(<<~RUBY)
call do
add_reference(:projects, :users)
^^^^^^^^^^^^^ `add_reference` requires `index: true` or `index: { options... }`
let(:offense) { '`add_reference` requires downtime for existing tables, use `add_concurrent_foreign_key` instead. When used for new tables, `index: true` or `index: { options... } is required.`' }
context 'when the table existed before' do
it 'registers an offense when using add_reference' do
expect_offense(<<~RUBY)
def up
add_reference(:projects, :users)
^^^^^^^^^^^^^ #{offense}
end
RUBY
end
it 'registers an offense when using add_reference with index enabled' do
expect_offense(<<~RUBY)
def up
add_reference(:projects, :users, index: true)
^^^^^^^^^^^^^ #{offense}
end
RUBY
RUBY
end
it 'registers an offense if only a different table was created' do
expect_offense(<<~RUBY)
def up
create_table(:foo) do |t|
t.string :name
end
add_reference(:projects, :users, index: true)
^^^^^^^^^^^^^ #{offense}
end
RUBY
end
end
it 'registers an offense when using add_reference index disabled' do
expect_offense(<<~RUBY)
context 'when creating the table at the same time' do
let(:create_table_statement) do
<<~RUBY
create_table(:projects) do |t|
t.string :name
end
RUBY
end
it 'registers an offense when using add_reference without index' do
expect_offense(<<~RUBY)
def up
#{create_table_statement}
add_reference(:projects, :users)
^^^^^^^^^^^^^ #{offense}
end
RUBY
end
it 'registers an offense when using add_reference index disabled' do
expect_offense(<<~RUBY)
def up
#{create_table_statement}
add_reference(:projects, :users, index: false)
^^^^^^^^^^^^^ `add_reference` requires `index: true` or `index: { options... }`
^^^^^^^^^^^^^ #{offense}
end
RUBY
end
RUBY
end
it 'does not register an offense when using add_reference with index enabled' do
expect_no_offenses(<<~RUBY)
it 'does not register an offense when using add_reference with index enabled' do
expect_no_offenses(<<~RUBY)
def up
#{create_table_statement}
add_reference(:projects, :users, index: true)
end
RUBY
end
RUBY
end
it 'does not register an offense when the index is unique' do
expect_no_offenses(<<~RUBY)
it 'does not register an offense when the index is unique' do
expect_no_offenses(<<~RUBY)
def up
#{create_table_statement}
add_reference(:projects, :users, index: { unique: true } )
end
RUBY
RUBY
end
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