diff options
Diffstat (limited to 'spec/rubocop/cop/migration')
5 files changed, 473 insertions, 14 deletions
diff --git a/spec/rubocop/cop/migration/complex_indexes_require_name_spec.rb b/spec/rubocop/cop/migration/complex_indexes_require_name_spec.rb new file mode 100644 index 00000000000..b769109057e --- /dev/null +++ b/spec/rubocop/cop/migration/complex_indexes_require_name_spec.rb @@ -0,0 +1,164 @@ +# frozen_string_literal: true +# +require 'fast_spec_helper' +require 'rubocop' +require_relative '../../../../rubocop/cop/migration/complex_indexes_require_name' + +RSpec.describe RuboCop::Cop::Migration::ComplexIndexesRequireName, type: :rubocop do + include CopHelper + + subject(:cop) { described_class.new } + + context 'in migration' do + before do + allow(cop).to receive(:in_migration?).and_return(true) + end + + context 'when creating complex indexes as part of create_table' do + context 'when indexes are configured with an options hash, but no name' do + it 'registers an offense' do + expect_offense(<<~RUBY) + class TestComplexIndexes < ActiveRecord::Migration[6.0] + DOWNTIME = false + + def up + create_table :test_table do |t| + t.integer :column1, null: false + t.integer :column2, null: false + t.jsonb :column3 + + t.index :column1, unique: true + t.index :column2, where: 'column1 = 0' + ^^^^^ #{described_class::MSG} + t.index :column3, using: :gin + ^^^^^ #{described_class::MSG} + end + end + + def down + drop_table :test_table + end + end + RUBY + + expect(cop.offenses.map(&:cop_name)).to all(eq("Migration/#{described_class.name.demodulize}")) + end + end + + context 'when indexes are configured with an options hash and name' do + it 'registers no offense' do + expect_no_offenses(<<~RUBY) + class TestComplexIndexes < ActiveRecord::Migration[6.0] + DOWNTIME = false + + def up + create_table :test_table do |t| + t.integer :column1, null: false + t.integer :column2, null: false + t.jsonb :column3 + + t.index :column1, unique: true + t.index :column2, where: 'column1 = 0', name: 'my_index_1' + t.index :column3, using: :gin, name: 'my_gin_index' + end + end + + def down + drop_table :test_table + end + end + RUBY + end + end + end + + context 'when indexes are added to an existing table' do + context 'when indexes are configured with an options hash, but no name' do + it 'registers an offense' do + expect_offense(<<~RUBY) + class TestComplexIndexes < ActiveRecord::Migration[6.0] + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_index :test_indexes, :column1 + + add_index :test_indexes, :column2, where: "column2 = 'value'", order: { column4: :desc } + ^^^^^^^^^ #{described_class::MSG} + end + + def down + add_index :test_indexes, :column4, 'unique' => true, where: 'column4 IS NOT NULL' + ^^^^^^^^^ #{described_class::MSG} + + add_concurrent_index :test_indexes, :column6, using: :gin, opclass: :gin_trgm_ops + ^^^^^^^^^^^^^^^^^^^^ #{described_class::MSG} + end + end + RUBY + + expect(cop.offenses.map(&:cop_name)).to all(eq("Migration/#{described_class.name.demodulize}")) + end + end + + context 'when indexes are configured with an options hash and a name' do + it 'registers no offenses' do + expect_no_offenses(<<~RUBY) + class TestComplexIndexes < ActiveRecord::Migration[6.0] + DOWNTIME = false + + INDEX_NAME = 'my_test_name' + + disable_ddl_transaction! + + def up + add_index :test_indexes, :column1 + + add_index :test_indexes, :column2, where: "column2 = 'value'", order: { column4: :desc }, name: 'my_index_1' + + add_concurrent_index :test_indexes, :column3, where: 'column3 = 10', name: 'idx_equal_to_10' + end + + def down + add_index :test_indexes, :column4, 'unique' => true, where: 'column4 IS NOT NULL', name: 'my_index_2' + + add_concurrent_index :test_indexes, :column6, using: :gin, opclass: :gin_trgm_ops, name: INDEX_NAME + end + end + RUBY + end + end + end + end + + context 'outside migration' do + before do + allow(cop).to receive(:in_migration?).and_return(false) + end + + it 'registers no offenses' do + expect_no_offenses(<<~RUBY) + class TestComplexIndexes < ActiveRecord::Migration[6.0] + DOWNTIME = false + + disable_ddl_transaction! + + def up + create_table :test_table do |t| + t.integer :column1 + + t.index :column1, where: 'column2 IS NOT NULL' + end + + add_index :test_indexes, :column1, where: "some_column = 'value'" + end + + def down + add_concurrent_index :test_indexes, :column2, unique: true + end + end + RUBY + end + end +end diff --git a/spec/rubocop/cop/migration/create_table_with_foreign_keys_spec.rb b/spec/rubocop/cop/migration/create_table_with_foreign_keys_spec.rb new file mode 100644 index 00000000000..fa4acc62226 --- /dev/null +++ b/spec/rubocop/cop/migration/create_table_with_foreign_keys_spec.rb @@ -0,0 +1,205 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' +require 'rubocop' +require_relative '../../../../rubocop/cop/migration/create_table_with_foreign_keys' + +RSpec.describe RuboCop::Cop::Migration::CreateTableWithForeignKeys, type: :rubocop 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 + create_table(:foo) do |t| + t.references :bar, foreign_key: { on_delete: 'cascade' } + t.references :zoo, foreign_key: { on_delete: 'cascade' } + end + end + RUBY + end + end + + context 'in a migration' do + before do + allow(cop).to receive(:in_migration?).and_return(true) + end + + context 'without foreign key' do + it 'does not register any offenses' do + expect_no_offenses(<<~RUBY) + def up + create_table(:foo) do |t| + t.references :bar + end + end + RUBY + end + end + + context 'with foreign key' do + context 'with just one foreign key' do + context 'when the foreign_key targets a high traffic table' do + context 'when the foreign_key has to_table option set' do + it 'does not register any offenses' do + expect_no_offenses(<<~RUBY) + def up + create_table(:foo) do |t| + t.references :project, "foreign_key" => { on_delete: 'cascade', to_table: 'projects' } + end + end + RUBY + end + end + + context 'when the foreign_key does not have to_table option set' do + it 'does not register any offenses' do + expect_no_offenses(<<~RUBY) + def up + create_table(:foo) do |t| + t.references :project, foreign_key: { on_delete: 'cascade' } + end + end + RUBY + end + end + end + + context 'when the foreign_key does not target a high traffic table' do + it 'does not register any offenses' do + expect_no_offenses(<<~RUBY) + def up + create_table(:foo) do |t| + t.references :bar, foreign_key: { on_delete: 'cascade' } + end + end + RUBY + end + end + end + + context 'with more than one foreign keys' do + let(:offense) do + 'Creating a table with more than one foreign key at once violates our migration style guide. ' \ + 'For more details check the https://docs.gitlab.com/ce/development/migration_style_guide.html#examples' + end + + shared_examples 'target to high traffic table' do |dsl_method, table_name| + context 'when the target is defined as option' do + it 'registers an offense ' do + expect_offense(<<~RUBY) + def up + create_table(:foo) do |t| + ^^^^^^^^^^^^^^^^^^ #{offense} + t.#{dsl_method} :#{table_name.singularize} #{explicit_target_opts} + t.#{dsl_method} :zoo #{implicit_target_opts} + end + end + RUBY + end + end + + context 'when the target has implicit definition' do + it 'registers an offense ' do + expect_offense(<<~RUBY) + def up + create_table(:foo) do |t| + ^^^^^^^^^^^^^^^^^^ #{offense} + t.#{dsl_method} :#{table_name.singularize} #{implicit_target_opts} + t.#{dsl_method} :zoo #{implicit_target_opts} + end + end + RUBY + end + end + end + + shared_context 'when there is a target to a high traffic table' do |dsl_method| + %w[ + audit_events + ci_build_trace_sections + ci_builds + ci_builds_metadata + ci_job_artifacts + ci_pipeline_variables + ci_pipelines + ci_stages + deployments + events + gitlab_subscriptions + issues + merge_request_diff_commits + merge_request_diff_files + merge_request_diffs + merge_request_metrics + merge_requests + namespaces + note_diff_files + notes + project_authorizations + projects + project_ci_cd_settings + project_features + push_event_payloads + resource_label_events + routes + sent_notifications + system_note_metadata + taggings + todos + users + web_hook_logs + ].each do |table| + let(:table_name) { table } + + it_behaves_like 'target to high traffic table', dsl_method, table + end + end + + context 'when the foreign keys are defined as options' do + context 'when there is no target to a high traffic table' do + it 'does not register any offenses' do + expect_no_offenses(<<~RUBY) + def up + create_table(:foo) do |t| + t.references :bar, foreign_key: { on_delete: 'cascade', to_table: :bars } + t.references :zoo, 'foreign_key' => { on_delete: 'cascade' } + t.references :john, 'foreign_key' => { on_delete: 'cascade' } + t.references :doe, 'foreign_key' => { on_delete: 'cascade', to_table: 'doe' } + end + end + RUBY + end + end + + include_context 'when there is a target to a high traffic table', :references do + let(:explicit_target_opts) { ", foreign_key: { to_table: :#{table_name} }" } + let(:implicit_target_opts) { ", foreign_key: true" } + end + end + + context 'when the foreign keys are defined by standlone migration helper' do + context 'when there is no target to a high traffic table' do + it 'does not register any offenses' do + expect_no_offenses(<<~RUBY) + def up + create_table(:foo) do |t| + t.foreign_key :bar + t.foreign_key :zoo, to_table: 'zoos' + end + end + RUBY + end + end + + include_context 'when there is a target to a high traffic table', :foreign_key do + let(:explicit_target_opts) { ", to_table: :#{table_name}" } + let(:implicit_target_opts) { } + end + end + end + end + end +end diff --git a/spec/rubocop/cop/migration/refer_to_index_by_name_spec.rb b/spec/rubocop/cop/migration/refer_to_index_by_name_spec.rb new file mode 100644 index 00000000000..76554d7446c --- /dev/null +++ b/spec/rubocop/cop/migration/refer_to_index_by_name_spec.rb @@ -0,0 +1,90 @@ +# frozen_string_literal: true +# +require 'fast_spec_helper' +require 'rubocop' +require_relative '../../../../rubocop/cop/migration/refer_to_index_by_name' + +RSpec.describe RuboCop::Cop::Migration::ReferToIndexByName, type: :rubocop do + include CopHelper + + subject(:cop) { described_class.new } + + context 'in migration' do + before do + allow(cop).to receive(:in_migration?).and_return(true) + end + + context 'when existing indexes are referred to without an explicit name' do + it 'registers an offense' do + expect_offense(<<~RUBY) + class TestReferToIndexByName < ActiveRecord::Migration[6.0] + DOWNTIME = false + + INDEX_NAME = 'my_test_name' + + disable_ddl_transaction! + + def up + if index_exists? :test_indexes, :column1, name: 'index_name_1' + remove_index :test_indexes, column: :column1, name: 'index_name_1' + end + + if index_exists? :test_indexes, :column2 + ^^^^^^^^^^^^^ #{described_class::MSG} + remove_index :test_indexes, :column2 + ^^^^^^^^^^^^ #{described_class::MSG} + end + + remove_index :test_indexes, column: column3 + ^^^^^^^^^^^^ #{described_class::MSG} + + remove_index :test_indexes, name: 'index_name_4' + end + + def down + if index_exists? :test_indexes, :column4, using: :gin, opclass: :gin_trgm_ops + ^^^^^^^^^^^^^ #{described_class::MSG} + remove_concurrent_index :test_indexes, :column4, using: :gin, opclass: :gin_trgm_ops + ^^^^^^^^^^^^^^^^^^^^^^^ #{described_class::MSG} + end + + if index_exists? :test_indexes, :column3, unique: true, name: 'index_name_3', where: 'column3 = 10' + remove_concurrent_index :test_indexes, :column3, unique: true, name: 'index_name_3', where: 'column3 = 10' + end + end + end + RUBY + + expect(cop.offenses.map(&:cop_name)).to all(eq("Migration/#{described_class.name.demodulize}")) + end + end + end + + context 'outside migration' do + before do + allow(cop).to receive(:in_migration?).and_return(false) + end + + it 'registers no offenses' do + expect_no_offenses(<<~RUBY) + class TestReferToIndexByName < ActiveRecord::Migration[6.0] + DOWNTIME = false + + disable_ddl_transaction! + + def up + if index_exists? :test_indexes, :column1 + remove_index :test_indexes, :column1 + end + end + + def down + if index_exists? :test_indexes, :column1 + remove_concurrent_index :test_indexes, :column1 + end + end + end + RUBY + end + end +end diff --git a/spec/rubocop/cop/migration/safer_boolean_column_spec.rb b/spec/rubocop/cop/migration/safer_boolean_column_spec.rb index 013f2edc5e9..72b817fde12 100644 --- a/spec/rubocop/cop/migration/safer_boolean_column_spec.rb +++ b/spec/rubocop/cop/migration/safer_boolean_column_spec.rb @@ -14,7 +14,7 @@ RSpec.describe RuboCop::Cop::Migration::SaferBooleanColumn, type: :rubocop do allow(cop).to receive(:in_migration?).and_return(true) end - described_class::WHITELISTED_TABLES.each do |table| + described_class::SMALL_TABLES.each do |table| context "for the #{table} table" do sources_and_offense = [ ["add_column :#{table}, :column, :boolean, default: true", 'should disallow nulls'], @@ -59,14 +59,14 @@ RSpec.describe RuboCop::Cop::Migration::SaferBooleanColumn, type: :rubocop do end end - it 'registers no offense for tables not listed in WHITELISTED_TABLES' do + it 'registers no offense for tables not listed in SMALL_TABLES' do inspect_source("add_column :large_table, :column, :boolean") expect(cop.offenses).to be_empty end it 'registers no offense for non-boolean columns' do - table = described_class::WHITELISTED_TABLES.sample + table = described_class::SMALL_TABLES.sample inspect_source("add_column :#{table}, :column, :string") expect(cop.offenses).to be_empty @@ -75,7 +75,7 @@ RSpec.describe RuboCop::Cop::Migration::SaferBooleanColumn, type: :rubocop do context 'outside of migration' do it 'registers no offense' do - table = described_class::WHITELISTED_TABLES.sample + table = described_class::SMALL_TABLES.sample inspect_source("add_column :#{table}, :column, :boolean") expect(cop.offenses).to be_empty diff --git a/spec/rubocop/cop/migration/update_column_in_batches_spec.rb b/spec/rubocop/cop/migration/update_column_in_batches_spec.rb index 5d96e8048bf..1d50d8c675e 100644 --- a/spec/rubocop/cop/migration/update_column_in_batches_spec.rb +++ b/spec/rubocop/cop/migration/update_column_in_batches_spec.rb @@ -1,15 +1,15 @@ # frozen_string_literal: true -require 'spec_helper' +require 'fast_spec_helper' require 'rubocop' require 'rubocop/rspec/support' require_relative '../../../../rubocop/cop/migration/update_column_in_batches' -RSpec.describe RuboCop::Cop::Migration::UpdateColumnInBatches do +RSpec.describe RuboCop::Cop::Migration::UpdateColumnInBatches, type: :rubocop do let(:cop) { described_class.new } - let(:tmp_rails_root) { Rails.root.join('tmp', 'rails_root') } + let(:tmp_rails_root) { rails_root_join('tmp', 'rails_root') } let(:migration_code) do <<-END def up @@ -27,6 +27,8 @@ RSpec.describe RuboCop::Cop::Migration::UpdateColumnInBatches do FileUtils.rm_rf(tmp_rails_root) end + let(:spec_filepath) { File.join(tmp_rails_root, 'spec', 'migrations', 'my_super_migration_spec.rb') } + context 'outside of a migration' do it 'does not register any offenses' do inspect_source(migration_code) @@ -35,8 +37,6 @@ RSpec.describe RuboCop::Cop::Migration::UpdateColumnInBatches do end end - let(:spec_filepath) { tmp_rails_root.join('spec', 'migrations', 'my_super_migration_spec.rb') } - shared_context 'with a migration file' do before do FileUtils.mkdir_p(File.dirname(migration_filepath)) @@ -83,31 +83,31 @@ RSpec.describe RuboCop::Cop::Migration::UpdateColumnInBatches do end context 'in a migration' do - let(:migration_filepath) { tmp_rails_root.join('db', 'migrate', '20121220064453_my_super_migration.rb') } + let(:migration_filepath) { File.join(tmp_rails_root, 'db', 'migrate', '20121220064453_my_super_migration.rb') } it_behaves_like 'a migration file with no spec file' it_behaves_like 'a migration file with a spec file' end context 'in a post migration' do - let(:migration_filepath) { tmp_rails_root.join('db', 'post_migrate', '20121220064453_my_super_migration.rb') } + let(:migration_filepath) { File.join(tmp_rails_root, 'db', 'post_migrate', '20121220064453_my_super_migration.rb') } it_behaves_like 'a migration file with no spec file' it_behaves_like 'a migration file with a spec file' end context 'EE migrations' do - let(:spec_filepath) { tmp_rails_root.join('ee', 'spec', 'migrations', 'my_super_migration_spec.rb') } + let(:spec_filepath) { File.join(tmp_rails_root, 'ee', 'spec', 'migrations', 'my_super_migration_spec.rb') } context 'in a migration' do - let(:migration_filepath) { tmp_rails_root.join('ee', 'db', 'migrate', '20121220064453_my_super_migration.rb') } + let(:migration_filepath) { File.join(tmp_rails_root, 'ee', 'db', 'migrate', '20121220064453_my_super_migration.rb') } it_behaves_like 'a migration file with no spec file' it_behaves_like 'a migration file with a spec file' end context 'in a post migration' do - let(:migration_filepath) { tmp_rails_root.join('ee', 'db', 'post_migrate', '20121220064453_my_super_migration.rb') } + let(:migration_filepath) { File.join(tmp_rails_root, 'ee', 'db', 'post_migrate', '20121220064453_my_super_migration.rb') } it_behaves_like 'a migration file with no spec file' it_behaves_like 'a migration file with a spec file' |