Commit d6dfbf02 authored by Sean McGivern's avatar Sean McGivern

Merge branch 'rs-add_column_with_default_cops' into 'master'

Add cop to blacklist specific tables for `add_column_with_default`

Closes #31293

See merge request !10892
parents e4160fab 30cc6b20
# rubocop:disable Migration/AddColumnWithDefaultToLargeTable
class AddOnlyAllowMergeIfBuildSucceedsToProjects < ActiveRecord::Migration class AddOnlyAllowMergeIfBuildSucceedsToProjects < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers include Gitlab::Database::MigrationHelpers
disable_ddl_transaction! disable_ddl_transaction!
......
# rubocop:disable Migration/AddColumnWithDefaultToLargeTable
class AddRepositoryStorageToProjects < ActiveRecord::Migration class AddRepositoryStorageToProjects < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers include Gitlab::Database::MigrationHelpers
disable_ddl_transaction! disable_ddl_transaction!
......
# rubocop:disable Migration/AddColumnWithDefaultToLargeTable
class AddRequestAccessEnabledToProjects < ActiveRecord::Migration class AddRequestAccessEnabledToProjects < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers include Gitlab::Database::MigrationHelpers
disable_ddl_transaction! disable_ddl_transaction!
......
# rubocop:disable Migration/AddColumnWithDefaultToLargeTable
class AddRequestAccessEnabledToGroups < ActiveRecord::Migration class AddRequestAccessEnabledToGroups < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers include Gitlab::Database::MigrationHelpers
disable_ddl_transaction! disable_ddl_transaction!
......
# See http://doc.gitlab.com/ce/development/migration_style_guide.html # See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab. # for more information on how to write migrations for GitLab.
# rubocop:disable Migration/AddColumnWithDefaultToLargeTable
class RemoveFeaturesEnabledFromProjects < ActiveRecord::Migration class RemoveFeaturesEnabledFromProjects < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers include Gitlab::Database::MigrationHelpers
disable_ddl_transaction! disable_ddl_transaction!
......
# See http://doc.gitlab.com/ce/development/migration_style_guide.html # See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab. # for more information on how to write migrations for GitLab.
# rubocop:disable Migration/AddColumnWithDefaultToLargeTable
class RemoveProjectsPushesSinceGc < ActiveRecord::Migration class RemoveProjectsPushesSinceGc < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers include Gitlab::Database::MigrationHelpers
......
# rubocop:disable Migration/AddColumnWithDefaultToLargeTable
class AddTwoFactorColumnsToNamespaces < ActiveRecord::Migration class AddTwoFactorColumnsToNamespaces < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers include Gitlab::Database::MigrationHelpers
......
# rubocop:disable Migration/AddColumnWithDefaultToLargeTable
class AddTwoFactorColumnsToUsers < ActiveRecord::Migration class AddTwoFactorColumnsToUsers < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers include Gitlab::Database::MigrationHelpers
......
# See http://doc.gitlab.com/ce/development/migration_style_guide.html # See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab. # for more information on how to write migrations for GitLab.
# rubocop:disable Migration/AddColumnWithDefaultToLargeTable
class AddPrintingMergeRequestLinkEnabledToProject < ActiveRecord::Migration class AddPrintingMergeRequestLinkEnabledToProject < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers include Gitlab::Database::MigrationHelpers
disable_ddl_transaction! disable_ddl_transaction!
......
# rubocop:disable Migration/AddColumnWithDefaultToLargeTable
class AddAutoCancelPendingPipelinesToProject < ActiveRecord::Migration class AddAutoCancelPendingPipelinesToProject < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers include Gitlab::Database::MigrationHelpers
......
# rubocop:disable Migration/AddColumnWithDefaultToLargeTable
class RevertAddNotifiedOfOwnActivityToUsers < ActiveRecord::Migration class RevertAddNotifiedOfOwnActivityToUsers < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers include Gitlab::Database::MigrationHelpers
disable_ddl_transaction! disable_ddl_transaction!
......
require_relative '../../migration_helpers'
module RuboCop
module Cop
module Migration
# This cop checks for `add_column_with_default` on a table that's been
# explicitly blacklisted because of its size.
#
# Even though this helper performs the update in batches to avoid
# downtime, using it with tables with millions of rows still causes a
# significant delay in the deploy process and is best avoided.
#
# See https://gitlab.com/gitlab-com/infrastructure/issues/1602 for more
# information.
class AddColumnWithDefaultToLargeTable < RuboCop::Cop::Cop
include MigrationHelpers
MSG = 'Using `add_column_with_default` on the `%s` table will take a ' \
'long time to complete, and should be avoided unless absolutely ' \
'necessary'.freeze
LARGE_TABLES = %i[
events
issues
merge_requests
namespaces
notes
projects
routes
users
].freeze
def_node_matcher :add_column_with_default?, <<~PATTERN
(send nil :add_column_with_default $(sym ...) ...)
PATTERN
def on_send(node)
return unless in_migration?(node)
matched = add_column_with_default?(node)
return unless matched
table = matched.to_a.first
return unless LARGE_TABLES.include?(table)
add_offense(node, :expression, format(MSG, table))
end
end
end
end
end
...@@ -5,29 +5,30 @@ module RuboCop ...@@ -5,29 +5,30 @@ module RuboCop
module Migration module Migration
# Cop that checks if `add_column_with_default` is used with `up`/`down` methods # Cop that checks if `add_column_with_default` is used with `up`/`down` methods
# and not `change`. # and not `change`.
class AddColumnWithDefault < RuboCop::Cop::Cop class ReversibleAddColumnWithDefault < RuboCop::Cop::Cop
include MigrationHelpers include MigrationHelpers
def_node_matcher :add_column_with_default?, <<~PATTERN
(send nil :add_column_with_default $...)
PATTERN
def_node_matcher :defines_change?, <<~PATTERN
(def :change ...)
PATTERN
MSG = '`add_column_with_default` is not reversible so you must manually define ' \ 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`'.freeze 'the `up` and `down` methods in your migration class, using `remove_column` in `down`'.freeze
def on_send(node) def on_send(node)
return unless in_migration?(node) return unless in_migration?(node)
return unless add_column_with_default?(node)
name = node.children[1]
return unless name == :add_column_with_default
node.each_ancestor(:def) do |def_node| node.each_ancestor(:def) do |def_node|
next unless method_name(def_node) == :change next unless defines_change?(def_node)
add_offense(def_node, :name) add_offense(def_node, :name)
end end
end end
def method_name(node)
node.children.first
end
end end
end end
end end
......
require_relative 'cop/custom_error_class' require_relative 'cop/custom_error_class'
require_relative 'cop/gem_fetcher' require_relative 'cop/gem_fetcher'
require_relative 'cop/migration/add_column' require_relative 'cop/migration/add_column'
require_relative 'cop/migration/add_column_with_default' require_relative 'cop/migration/add_column_with_default_to_large_table'
require_relative 'cop/migration/add_concurrent_foreign_key' require_relative 'cop/migration/add_concurrent_foreign_key'
require_relative 'cop/migration/add_concurrent_index' require_relative 'cop/migration/add_concurrent_index'
require_relative 'cop/migration/add_index' require_relative 'cop/migration/add_index'
require_relative 'cop/migration/remove_concurrent_index' require_relative 'cop/migration/remove_concurrent_index'
require_relative 'cop/migration/remove_index' require_relative 'cop/migration/remove_index'
require_relative 'cop/migration/reversible_add_column_with_default'
require 'spec_helper'
require 'rubocop'
require 'rubocop/rspec/support'
require_relative '../../../../rubocop/cop/migration/add_column_with_default_to_large_table'
describe RuboCop::Cop::Migration::AddColumnWithDefaultToLargeTable do
include CopHelper
subject(:cop) { described_class.new }
context 'in migration' do
before do
allow(cop).to receive(:in_migration?).and_return(true)
end
described_class::LARGE_TABLES.each do |table|
it "registers an offense for the #{table} table" do
inspect_source(cop, "add_column_with_default :#{table}, :column, default: true")
aggregate_failures do
expect(cop.offenses.size).to eq(1)
expect(cop.offenses.map(&:line)).to eq([1])
end
end
end
it 'registers no offense for non-blacklisted tables' do
inspect_source(cop, "add_column_with_default :table, :column, default: true")
expect(cop.offenses).to be_empty
end
end
context 'outside of migration' do
it 'registers no offense' do
table = described_class::LARGE_TABLES.sample
inspect_source(cop, "add_column_with_default :#{table}, :column, default: true")
expect(cop.offenses).to be_empty
end
end
end
...@@ -3,9 +3,9 @@ require 'spec_helper' ...@@ -3,9 +3,9 @@ require 'spec_helper'
require 'rubocop' require 'rubocop'
require 'rubocop/rspec/support' require 'rubocop/rspec/support'
require_relative '../../../../rubocop/cop/migration/add_column_with_default' require_relative '../../../../rubocop/cop/migration/reversible_add_column_with_default'
describe RuboCop::Cop::Migration::AddColumnWithDefault do describe RuboCop::Cop::Migration::ReversibleAddColumnWithDefault do
include CopHelper include CopHelper
subject(:cop) { described_class.new } subject(:cop) { described_class.new }
......
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