Commit 661afad0 authored by Adam Hegyi's avatar Adam Hegyi

Merge branch 'prevent-adding-columns-to-wide-tables' into 'master'

Prevent adding columns to wide tables

See merge request gitlab-org/gitlab!28161
parents 258ea127 44dc7d49
...@@ -4,6 +4,6 @@ class AddPagesHttpsOnlyToProjects < ActiveRecord::Migration[4.2] ...@@ -4,6 +4,6 @@ class AddPagesHttpsOnlyToProjects < ActiveRecord::Migration[4.2]
DOWNTIME = false DOWNTIME = false
def change def change
add_column :projects, :pages_https_only, :boolean add_column :projects, :pages_https_only, :boolean # rubocop:disable Migration/AddColumnsToWideTables
end end
end end
...@@ -2,6 +2,6 @@ class AddIncludePrivateContributionsToUsers < ActiveRecord::Migration[4.2] ...@@ -2,6 +2,6 @@ class AddIncludePrivateContributionsToUsers < ActiveRecord::Migration[4.2]
DOWNTIME = false DOWNTIME = false
def change def change
add_column :users, :include_private_contributions, :boolean add_column :users, :include_private_contributions, :boolean # rubocop:disable Migration/AddColumnsToWideTables
end end
end end
...@@ -6,7 +6,7 @@ class AddRemoteMirrorAvailableOverriddenToProjects < ActiveRecord::Migration[4.2 ...@@ -6,7 +6,7 @@ class AddRemoteMirrorAvailableOverriddenToProjects < ActiveRecord::Migration[4.2
disable_ddl_transaction! disable_ddl_transaction!
def up def up
add_column(:projects, :remote_mirror_available_overridden, :boolean) unless column_exists?(:projects, :remote_mirror_available_overridden) add_column(:projects, :remote_mirror_available_overridden, :boolean) unless column_exists?(:projects, :remote_mirror_available_overridden) # rubocop:disable Migration/AddColumnsToWideTables
end end
def down def down
......
...@@ -5,6 +5,6 @@ class AddPrivateProfileToUsers < ActiveRecord::Migration[4.2] ...@@ -5,6 +5,6 @@ class AddPrivateProfileToUsers < ActiveRecord::Migration[4.2]
DOWNTIME = false DOWNTIME = false
def change def change
add_column :users, :private_profile, :boolean add_column :users, :private_profile, :boolean # rubocop:disable Migration/AddColumnsToWideTables
end end
end end
...@@ -27,7 +27,11 @@ class AddCommitEmailToUsers < ActiveRecord::Migration[4.2] ...@@ -27,7 +27,11 @@ class AddCommitEmailToUsers < ActiveRecord::Migration[4.2]
# comments: # comments:
# disable_ddl_transaction! # disable_ddl_transaction!
# rubocop:disable Migration/AddLimitToStringColumns
# rubocop:disable Migration/AddColumnsToWideTables
def change def change
add_column :users, :commit_email, :string # rubocop:disable Migration/AddLimitToStringColumns add_column :users, :commit_email, :string
end end
# rubocop:enable Migration/AddLimitToStringColumns
# rubocop:enable Migration/AddColumnsToWideTables
end end
...@@ -4,6 +4,6 @@ class AddScheduledAtToCiBuilds < ActiveRecord::Migration[4.2] ...@@ -4,6 +4,6 @@ class AddScheduledAtToCiBuilds < ActiveRecord::Migration[4.2]
DOWNTIME = false DOWNTIME = false
def change def change
add_column :ci_builds, :scheduled_at, :datetime_with_timezone add_column :ci_builds, :scheduled_at, :datetime_with_timezone # rubocop:disable Migration/AddColumnsToWideTables
end end
end end
...@@ -9,7 +9,7 @@ class AddRepositoriesTable < ActiveRecord::Migration[4.2] ...@@ -9,7 +9,7 @@ class AddRepositoriesTable < ActiveRecord::Migration[4.2]
t.string :disk_path, null: false, index: { unique: true } # rubocop:disable Migration/AddLimitToStringColumns t.string :disk_path, null: false, index: { unique: true } # rubocop:disable Migration/AddLimitToStringColumns
end end
add_column :projects, :pool_repository_id, :bigint add_column :projects, :pool_repository_id, :bigint # rubocop:disable Migration/AddColumnsToWideTables
add_index :projects, :pool_repository_id, where: 'pool_repository_id IS NOT NULL' add_index :projects, :pool_repository_id, where: 'pool_repository_id IS NOT NULL'
end end
end end
...@@ -5,7 +5,11 @@ class AddEncryptedRunnersTokenToProjects < ActiveRecord::Migration[4.2] ...@@ -5,7 +5,11 @@ class AddEncryptedRunnersTokenToProjects < ActiveRecord::Migration[4.2]
DOWNTIME = false DOWNTIME = false
# rubocop:disable Migration/AddColumnsToWideTables
# rubocop:disable Migration/AddLimitToStringColumns
def change def change
add_column :projects, :runners_token_encrypted, :string # rubocop:disable Migration/AddLimitToStringColumns add_column :projects, :runners_token_encrypted, :string
end end
# rubocop:enable Migration/AddColumnsToWideTables
# rubocop:enable Migration/AddLimitToStringColumns
end end
...@@ -5,7 +5,11 @@ class AddTokenEncryptedToCiBuilds < ActiveRecord::Migration[5.0] ...@@ -5,7 +5,11 @@ class AddTokenEncryptedToCiBuilds < ActiveRecord::Migration[5.0]
DOWNTIME = false DOWNTIME = false
# rubocop:disable Migration/AddColumnsToWideTables
# rubocop:disable Migration/AddLimitToStringColumns
def change def change
add_column :ci_builds, :token_encrypted, :string # rubocop:disable Migration/AddLimitToStringColumns add_column :ci_builds, :token_encrypted, :string
end end
# rubocop:enable Migration/AddColumnsToWideTables
# rubocop:enable Migration/AddLimitToStringColumns
end end
...@@ -3,7 +3,11 @@ ...@@ -3,7 +3,11 @@
class AddProjectBfgObjectMapColumn < ActiveRecord::Migration[5.0] class AddProjectBfgObjectMapColumn < ActiveRecord::Migration[5.0]
DOWNTIME = false DOWNTIME = false
# rubocop:disable Migration/AddColumnsToWideTables
# rubocop:disable Migration/AddLimitToStringColumns
def change def change
add_column :projects, :bfg_object_map, :string # rubocop:disable Migration/AddLimitToStringColumns add_column :projects, :bfg_object_map, :string
end end
# rubocop:enable Migration/AddColumnsToWideTables
# rubocop:enable Migration/AddLimitToStringColumns
end end
...@@ -7,6 +7,6 @@ class AddDetectedRepositoryLanguagesToProjects < ActiveRecord::Migration[5.0] ...@@ -7,6 +7,6 @@ class AddDetectedRepositoryLanguagesToProjects < ActiveRecord::Migration[5.0]
DOWNTIME = false DOWNTIME = false
def change def change
add_column :projects, :detected_repository_languages, :boolean add_column :projects, :detected_repository_languages, :boolean # rubocop:disable Migration/AddColumnsToWideTables
end end
end end
...@@ -10,6 +10,6 @@ class AddBridgedPipelineIdToBridges < ActiveRecord::Migration[5.0] ...@@ -10,6 +10,6 @@ class AddBridgedPipelineIdToBridges < ActiveRecord::Migration[5.0]
DOWNTIME = false DOWNTIME = false
def change def change
add_column :ci_builds, :upstream_pipeline_id, :integer add_column :ci_builds, :upstream_pipeline_id, :integer # rubocop:disable Migration/AddColumnsToWideTables
end end
end end
...@@ -4,6 +4,6 @@ class AddProjectEmailsDisabled < ActiveRecord::Migration[5.2] ...@@ -4,6 +4,6 @@ class AddProjectEmailsDisabled < ActiveRecord::Migration[5.2]
DOWNTIME = false DOWNTIME = false
def change def change
add_column :projects, :emails_disabled, :boolean add_column :projects, :emails_disabled, :boolean # rubocop:disable Migration/AddColumnsToWideTables
end end
end end
...@@ -9,7 +9,7 @@ class AddStaticObjectTokenToUsers < ActiveRecord::Migration[5.2] ...@@ -9,7 +9,7 @@ class AddStaticObjectTokenToUsers < ActiveRecord::Migration[5.2]
disable_ddl_transaction! disable_ddl_transaction!
def up def up
add_column :users, :static_object_token, :string, limit: 255 add_column :users, :static_object_token, :string, limit: 255 # rubocop:disable Migration/AddColumnsToWideTables
end end
def down def down
......
...@@ -7,8 +7,10 @@ class AddFirstLastNameToUser < ActiveRecord::Migration[5.2] ...@@ -7,8 +7,10 @@ class AddFirstLastNameToUser < ActiveRecord::Migration[5.2]
# Set this constant to true if this migration requires downtime. # Set this constant to true if this migration requires downtime.
DOWNTIME = false DOWNTIME = false
# rubocop:disable Migration/AddColumnsToWideTables
def change def change
add_column(:users, :first_name, :string, null: true, limit: 255) add_column(:users, :first_name, :string, null: true, limit: 255)
add_column(:users, :last_name, :string, null: true, limit: 255) add_column(:users, :last_name, :string, null: true, limit: 255)
end end
# rubocop:enable Migration/AddColumnsToWideTables
end end
...@@ -4,6 +4,6 @@ class AddProjectsMaxPagesSize < ActiveRecord::Migration[5.2] ...@@ -4,6 +4,6 @@ class AddProjectsMaxPagesSize < ActiveRecord::Migration[5.2]
DOWNTIME = false DOWNTIME = false
def change def change
add_column :projects, :max_pages_size, :integer add_column :projects, :max_pages_size, :integer # rubocop:disable Migration/AddColumnsToWideTables
end end
end end
...@@ -4,6 +4,6 @@ class AddProjectsMaxArtifactsSize < ActiveRecord::Migration[5.2] ...@@ -4,6 +4,6 @@ class AddProjectsMaxArtifactsSize < ActiveRecord::Migration[5.2]
DOWNTIME = false DOWNTIME = false
def change def change
add_column :projects, :max_artifacts_size, :integer add_column :projects, :max_artifacts_size, :integer # rubocop:disable Migration/AddColumnsToWideTables
end end
end end
...@@ -7,6 +7,6 @@ class AddRoleToUsers < ActiveRecord::Migration[5.2] ...@@ -7,6 +7,6 @@ class AddRoleToUsers < ActiveRecord::Migration[5.2]
DOWNTIME = false DOWNTIME = false
def change def change
add_column :users, :role, :smallint add_column :users, :role, :smallint # rubocop:disable Migration/AddColumnsToWideTables
end end
end end
...@@ -4,6 +4,6 @@ class AddPullMirrorBranchPrefixToProjects < ActiveRecord::Migration[5.2] ...@@ -4,6 +4,6 @@ class AddPullMirrorBranchPrefixToProjects < ActiveRecord::Migration[5.2]
DOWNTIME = false DOWNTIME = false
def change def change
add_column :projects, :pull_mirror_branch_prefix, :string, limit: 50 add_column :projects, :pull_mirror_branch_prefix, :string, limit: 50 # rubocop:disable Migration/AddColumnsToWideTables
end end
end end
...@@ -4,8 +4,10 @@ class AddMarkForDeletionToProjects < ActiveRecord::Migration[5.2] ...@@ -4,8 +4,10 @@ class AddMarkForDeletionToProjects < ActiveRecord::Migration[5.2]
# Set this constant to true if this migration requires downtime. # Set this constant to true if this migration requires downtime.
DOWNTIME = false DOWNTIME = false
# rubocop:disable Migration/AddColumnsToWideTables
def change def change
add_column :projects, :marked_for_deletion_at, :date add_column :projects, :marked_for_deletion_at, :date
add_column :projects, :marked_for_deletion_by_user_id, :integer add_column :projects, :marked_for_deletion_by_user_id, :integer
end end
# rubocop:enable Migration/AddColumnsToWideTables
end end
...@@ -8,7 +8,7 @@ class AddRemoveSourceBranchAfterMergeToProjects < ActiveRecord::Migration[5.1] ...@@ -8,7 +8,7 @@ class AddRemoveSourceBranchAfterMergeToProjects < ActiveRecord::Migration[5.1]
DOWNTIME = false DOWNTIME = false
def up def up
add_column :projects, :remove_source_branch_after_merge, :boolean add_column :projects, :remove_source_branch_after_merge, :boolean # rubocop:disable Migration/AddColumnsToWideTables
end end
def down def down
......
...@@ -4,6 +4,6 @@ class AddProcessedToCiBuilds < ActiveRecord::Migration[5.2] ...@@ -4,6 +4,6 @@ class AddProcessedToCiBuilds < ActiveRecord::Migration[5.2]
DOWNTIME = false DOWNTIME = false
def change def change
add_column :ci_builds, :processed, :boolean add_column :ci_builds, :processed, :boolean # rubocop:disable Migration/AddColumnsToWideTables
end end
end end
...@@ -3,6 +3,7 @@ ...@@ -3,6 +3,7 @@
class AddResourceGroupIdToCiBuilds < ActiveRecord::Migration[5.2] class AddResourceGroupIdToCiBuilds < ActiveRecord::Migration[5.2]
DOWNTIME = false DOWNTIME = false
# rubocop:disable Migration/AddColumnsToWideTables
def up def up
unless column_exists?(:ci_builds, :resource_group_id) unless column_exists?(:ci_builds, :resource_group_id)
add_column :ci_builds, :resource_group_id, :bigint add_column :ci_builds, :resource_group_id, :bigint
...@@ -12,6 +13,7 @@ class AddResourceGroupIdToCiBuilds < ActiveRecord::Migration[5.2] ...@@ -12,6 +13,7 @@ class AddResourceGroupIdToCiBuilds < ActiveRecord::Migration[5.2]
add_column :ci_builds, :waiting_for_resource_at, :datetime_with_timezone add_column :ci_builds, :waiting_for_resource_at, :datetime_with_timezone
end end
end end
# rubocop:enable Migration/AddColumnsToWideTables
def down def down
if column_exists?(:ci_builds, :resource_group_id) if column_exists?(:ci_builds, :resource_group_id)
......
...@@ -4,6 +4,6 @@ class AddSuggestionCommitMessageToProjects < ActiveRecord::Migration[5.2] ...@@ -4,6 +4,6 @@ class AddSuggestionCommitMessageToProjects < ActiveRecord::Migration[5.2]
DOWNTIME = false DOWNTIME = false
def change def change
add_column :projects, :suggestion_commit_message, :string, limit: 255 add_column :projects, :suggestion_commit_message, :string, limit: 255 # rubocop:disable Migration/AddColumnsToWideTables
end end
end end
...@@ -6,6 +6,6 @@ class AddSchedulingTypeToCiBuilds < ActiveRecord::Migration[5.2] ...@@ -6,6 +6,6 @@ class AddSchedulingTypeToCiBuilds < ActiveRecord::Migration[5.2]
DOWNTIME = false DOWNTIME = false
def change def change
add_column :ci_builds, :scheduling_type, :integer, limit: 2 add_column :ci_builds, :scheduling_type, :integer, limit: 2 # rubocop:disable Migration/AddColumnsToWideTables
end end
end end
...@@ -4,6 +4,6 @@ class AddAutocloseReferencedIssuesToProjects < ActiveRecord::Migration[5.2] ...@@ -4,6 +4,6 @@ class AddAutocloseReferencedIssuesToProjects < ActiveRecord::Migration[5.2]
DOWNTIME = false DOWNTIME = false
def change def change
add_column :projects, :autoclose_referenced_issues, :boolean add_column :projects, :autoclose_referenced_issues, :boolean # rubocop:disable Migration/AddColumnsToWideTables
end end
end end
...@@ -7,7 +7,7 @@ class AddUserType < ActiveRecord::Migration[6.0] ...@@ -7,7 +7,7 @@ class AddUserType < ActiveRecord::Migration[6.0]
def up def up
with_lock_retries do with_lock_retries do
add_column :users, :user_type, :integer, limit: 2 add_column :users, :user_type, :integer, limit: 2 # rubocop:disable Migration/AddColumnsToWideTables
end end
end end
......
...@@ -23,7 +23,7 @@ class DropProjectsCiId < ActiveRecord::Migration[5.1] ...@@ -23,7 +23,7 @@ class DropProjectsCiId < ActiveRecord::Migration[5.1]
def down def down
unless column_exists?(:projects, :ci_id) unless column_exists?(:projects, :ci_id)
add_column :projects, :ci_id, :integer add_column :projects, :ci_id, :integer # rubocop:disable Migration/AddColumnsToWideTables
end end
unless index_exists?(:projects, :ci_id) unless index_exists?(:projects, :ci_id)
......
...@@ -17,7 +17,7 @@ class RemoveUsersSupportType < ActiveRecord::Migration[5.1] ...@@ -17,7 +17,7 @@ class RemoveUsersSupportType < ActiveRecord::Migration[5.1]
end end
def down def down
add_column :users, :support_bot, :boolean add_column :users, :support_bot, :boolean # rubocop:disable Migration/AddColumnsToWideTables
add_concurrent_index :users, :support_bot add_concurrent_index :users, :support_bot
add_concurrent_index :users, :state, add_concurrent_index :users, :state,
......
...@@ -15,7 +15,7 @@ class DropMergeRequestsRequireCodeOwnerApprovalFromProjects < ActiveRecord::Migr ...@@ -15,7 +15,7 @@ class DropMergeRequestsRequireCodeOwnerApprovalFromProjects < ActiveRecord::Migr
end end
def down def down
add_column :projects, :merge_requests_require_code_owner_approval, :boolean add_column :projects, :merge_requests_require_code_owner_approval, :boolean # rubocop:disable Migration/AddColumnsToWideTables
add_concurrent_index( add_concurrent_index(
:projects, :projects,
......
# frozen_string_literal: true
require_relative '../../migration_helpers'
module RuboCop
module Cop
module Migration
# Cop that prevents adding columns to wide tables.
class AddColumnsToWideTables < RuboCop::Cop::Cop
include MigrationHelpers
MSG = '`%s` is a wide table with several columns, addig more should be avoided unless absolutely necessary.' \
' Consider storing the column in a different table or creating a new one.'.freeze
BLACKLISTED_METHODS = %i[
add_column
add_column_with_default
add_reference
add_timestamps_with_timezone
].freeze
def on_send(node)
return unless in_migration?(node)
method_name = node.children[1]
table_name = node.children[2]
return unless offense?(method_name, table_name)
add_offense(node, location: :selector, message: format(MSG, table_name.value))
end
private
def offense?(method_name, table_name)
wide_table?(table_name) &&
BLACKLISTED_METHODS.include?(method_name)
end
def wide_table?(table_name)
table_name && table_name.type == :sym &&
WIDE_TABLES.include?(table_name.value)
end
end
end
end
end
...@@ -6,7 +6,7 @@ module RuboCop ...@@ -6,7 +6,7 @@ module RuboCop
plan_limits plan_limits
].freeze ].freeze
# Blacklisted table due to: # Blacklisted tables due to:
# - size in GB (>= 10 GB on GitLab.com as of 02/2020) # - size in GB (>= 10 GB on GitLab.com as of 02/2020)
# - number of records # - number of records
BLACKLISTED_TABLES = %i[ BLACKLISTED_TABLES = %i[
...@@ -44,6 +44,15 @@ module RuboCop ...@@ -44,6 +44,15 @@ module RuboCop
web_hook_logs web_hook_logs
].freeze ].freeze
# Blacklisted tables due to:
# - number of columns (> 50 on GitLab.com as of 03/2020)
# - number of records
WIDE_TABLES = %i[
users
projects
ci_builds
].freeze
# Returns true if the given node originated from the db/migrate directory. # Returns true if the given node originated from the db/migrate directory.
def in_migration?(node) def in_migration?(node)
dirname(node).end_with?('db/migrate', 'db/geo/migrate') || in_post_deployment_migration?(node) dirname(node).end_with?('db/migrate', 'db/geo/migrate') || in_post_deployment_migration?(node)
......
# frozen_string_literal: true
require 'spec_helper'
require 'rubocop'
require_relative '../../../../rubocop/cop/migration/add_columns_to_wide_tables'
describe RuboCop::Cop::Migration::AddColumnsToWideTables do
include CopHelper
let(:cop) { described_class.new }
context 'outside of a migration' do
it 'does not register any offenses' do
expect_no_offenses(<<~RUBY)
def up
add_column(:users, :another_column, :string)
end
RUBY
end
end
context 'in a migration' do
before do
allow(cop).to receive(:in_migration?).and_return(true)
end
context 'with wide tables' do
it 'registers an offense when adding a column to a wide table' do
offense = '`projects` is a wide table with several columns, addig more should be avoided unless absolutely necessary. Consider storing the column in a different table or creating a new one.'
expect_offense(<<~RUBY)
def up
add_column(:projects, :another_column, :integer)
^^^^^^^^^^ #{offense}
end
RUBY
end
it 'registers an offense when adding a column with default to a wide table' do
offense = '`users` is a wide table with several columns, addig more should be avoided unless absolutely necessary. Consider storing the column in a different table or creating a new one.'
expect_offense(<<~RUBY)
def up
add_column_with_default(:users, :another_column, :boolean, default: false)
^^^^^^^^^^^^^^^^^^^^^^^ #{offense}
end
RUBY
end
it 'registers an offense when adding a reference' do
offense = '`ci_builds` is a wide table with several columns, addig more should be avoided unless absolutely necessary. Consider storing the column in a different table or creating a new one.'
expect_offense(<<~RUBY)
def up
add_reference(:ci_builds, :issue, :boolean, index: true)
^^^^^^^^^^^^^ #{offense}
end
RUBY
end
it 'registers an offense when adding timestamps' do
offense = '`projects` is a wide table with several columns, addig more should be avoided unless absolutely necessary. Consider storing the column in a different table or creating a new one.'
expect_offense(<<~RUBY)
def up
add_timestamps_with_timezone(:projects, null: false)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{offense}
end
RUBY
end
it 'register no offense when using other method' do
expect_no_offenses(<<~RUBY)
def up
add_concurrent_index(:projects, :new_index)
end
RUBY
end
end
context 'with a regular table' do
it 'registers no offense for notes' do
expect_no_offenses(<<~RUBY)
def up
add_column(:notes, :another_column, :boolean)
end
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