diff --git a/.rubocop_cc.yml b/.rubocop_cc.yml index bc55373b36f..5e68caa7463 100644 --- a/.rubocop_cc.yml +++ b/.rubocop_cc.yml @@ -11,6 +11,7 @@ plugins: require: - ./spec/linters/migration/add_constraint_name.rb - ./spec/linters/migration/include_string_size.rb + - ./spec/linters/migration/no_model_in_specs.rb - ./spec/linters/migration/require_primary_key.rb - ./spec/linters/migration/too_many_migration_runs.rb - ./spec/linters/match_requires_with_includes.rb @@ -67,6 +68,12 @@ Migration/IncludeStringSize: # Exclude for old Migrations Exclude: - !ruby/regexp /db/migrations/201([0-9]).+\.rb$/ - !ruby/regexp /db/migrations/202([0-2]).+\.rb$/ +Migration/NoModelInSpecs: + Include: + - spec/migrations/**/* + Exclude: + # Uses custom tmp_migrations_dir so schema is never rolled back + - spec/migrations/20190712210940_backfill_status_for_deployments_spec.rb Migration/RequirePrimaryKey: # Exclude for old Migrations Include: - db/migrations/**/* diff --git a/spec/linters/migration/no_model_in_specs.rb b/spec/linters/migration/no_model_in_specs.rb new file mode 100644 index 00000000000..4b8fb31f182 --- /dev/null +++ b/spec/linters/migration/no_model_in_specs.rb @@ -0,0 +1,25 @@ +module RuboCop + module Cop + module Migration + class NoModelInSpecs < RuboCop::Cop::Base + MSG = 'Do not use model classes in migration specs. ' \ + 'Use raw Sequel operations (e.g. db[:table].insert) instead. ' \ + 'See spec/migrations/Readme.md for details.'.freeze + + def on_send(node) + add_offense(node) if model_receiver?(node.receiver) + end + + private + + def model_receiver?(receiver) + return false unless receiver + return false unless receiver.const_type? + + name = receiver.const_name.to_s + name.end_with?('Model') && name != 'Sequel::Model' + end + end + end + end +end diff --git a/spec/linters/migration/no_model_in_specs_spec.rb b/spec/linters/migration/no_model_in_specs_spec.rb new file mode 100644 index 00000000000..d77c864e674 --- /dev/null +++ b/spec/linters/migration/no_model_in_specs_spec.rb @@ -0,0 +1,74 @@ +require 'spec_helper' +require 'rubocop' +require 'rubocop/rspec/cop_helper' +require 'rubocop/config' +require 'linters/migration/no_model_in_specs' + +RSpec.describe RuboCop::Cop::Migration::NoModelInSpecs do + include CopHelper + + subject(:cop) { described_class.new(RuboCop::Config.new({})) } + + let(:message) do + 'Do not use model classes in migration specs. ' \ + 'Use raw Sequel operations (e.g. db[:table].insert) instead. ' \ + 'See spec/migrations/Readme.md for details.' + end + + it 'registers an offense for Model.make' do + result = inspect_source(<<~RUBY) + VCAP::CloudController::AppModel.make + RUBY + + expect(result.size).to eq(1) + expect(result.map(&:message)).to eq([message]) + end + + it 'registers an offense for Model.make!' do + result = inspect_source(<<~RUBY) + VCAP::CloudController::AppModel.make! + RUBY + + expect(result.size).to eq(1) + end + + it 'registers an offense for Model.create' do + result = inspect_source(<<~RUBY) + VCAP::CloudController::DeploymentModel.create(guid: 'test') + RUBY + + expect(result.size).to eq(1) + end + + it 'registers an offense for Model.where' do + result = inspect_source(<<~RUBY) + VCAP::CloudController::AppModel.where(guid: 'test').first + RUBY + + expect(result.size).to eq(1) + end + + it 'does not register an offense for raw Sequel inserts' do + result = inspect_source(<<~RUBY) + db[:apps].insert(guid: 'test', name: 'test-app') + RUBY + + expect(result.size).to eq(0) + end + + it 'does not register an offense for Sequel::Model' do + result = inspect_source(<<~RUBY) + Sequel::Model.db + RUBY + + expect(result.size).to eq(0) + end + + it 'does not register an offense for non-Model constants' do + result = inspect_source(<<~RUBY) + SecureRandom.uuid + RUBY + + expect(result.size).to eq(0) + end +end diff --git a/spec/migrations/20251030100000_add_unique_constraint_to_sidecar_process_types_spec.rb b/spec/migrations/20251030100000_add_unique_constraint_to_sidecar_process_types_spec.rb index 51f9ea639dc..1186fe84586 100644 --- a/spec/migrations/20251030100000_add_unique_constraint_to_sidecar_process_types_spec.rb +++ b/spec/migrations/20251030100000_add_unique_constraint_to_sidecar_process_types_spec.rb @@ -6,22 +6,27 @@ let(:migration_filename) { '20251030100000_add_unique_constraint_to_sidecar_process_types.rb' } end - let!(:app) { VCAP::CloudController::AppModel.make } - let!(:sidecar) { VCAP::CloudController::SidecarModel.make(app:) } - let!(:revision) { VCAP::CloudController::RevisionModel.make(app:) } - let!(:revision_sidecar) { VCAP::CloudController::RevisionSidecarModel.make(revision:) } + let(:app_guid) { SecureRandom.uuid } + let(:sidecar_guid) { SecureRandom.uuid } + let(:revision_guid) { SecureRandom.uuid } + let(:revision_sidecar_guid) { SecureRandom.uuid } it 'removes duplicates, adds unique constraints, and is reversible' do + db[:apps].insert(guid: app_guid, name: 'test-app') + db[:sidecars].insert(guid: sidecar_guid, name: 'test-sidecar', command: 'command', app_guid: app_guid) + db[:revisions].insert(guid: revision_guid, app_guid: app_guid, description: 'some description') + db[:revision_sidecars].insert(guid: revision_sidecar_guid, name: 'test-sidecar', command: 'command', revision_guid: revision_guid) + # ========================================================================================= # SETUP: Create duplicate entries for both tables to test the de-duplication logic. # ========================================================================================= - db[:sidecar_process_types].insert(sidecar_guid: sidecar.guid, type: 'web', app_guid: app.guid, guid: SecureRandom.uuid) - db[:sidecar_process_types].insert(sidecar_guid: sidecar.guid, type: 'web', app_guid: app.guid, guid: SecureRandom.uuid) - expect(db[:sidecar_process_types].where(sidecar_guid: sidecar.guid, type: 'web').count).to eq(2) + db[:sidecar_process_types].insert(sidecar_guid: sidecar_guid, type: 'web', app_guid: app_guid, guid: SecureRandom.uuid) + db[:sidecar_process_types].insert(sidecar_guid: sidecar_guid, type: 'web', app_guid: app_guid, guid: SecureRandom.uuid) + expect(db[:sidecar_process_types].where(sidecar_guid: sidecar_guid, type: 'web').count).to eq(2) - db[:revision_sidecar_process_types].insert(revision_sidecar_guid: revision_sidecar.guid, type: 'worker', guid: SecureRandom.uuid) - db[:revision_sidecar_process_types].insert(revision_sidecar_guid: revision_sidecar.guid, type: 'worker', guid: SecureRandom.uuid) - expect(db[:revision_sidecar_process_types].where(revision_sidecar_guid: revision_sidecar.guid, type: 'worker').count).to eq(2) + db[:revision_sidecar_process_types].insert(revision_sidecar_guid: revision_sidecar_guid, type: 'worker', guid: SecureRandom.uuid) + db[:revision_sidecar_process_types].insert(revision_sidecar_guid: revision_sidecar_guid, type: 'worker', guid: SecureRandom.uuid) + expect(db[:revision_sidecar_process_types].where(revision_sidecar_guid: revision_sidecar_guid, type: 'worker').count).to eq(2) # ========================================================================================= # UP MIGRATION: Run the migration to apply the unique constraints. @@ -31,13 +36,13 @@ # ========================================================================================= # ASSERT UP MIGRATION: Verify that duplicates are removed and constraints are enforced. # ========================================================================================= - expect(db[:sidecar_process_types].where(sidecar_guid: sidecar.guid, type: 'web').count).to eq(1) + expect(db[:sidecar_process_types].where(sidecar_guid: sidecar_guid, type: 'web').count).to eq(1) expect(db.indexes(:sidecar_process_types)).to include(:sidecar_process_types_sidecar_guid_type_index) - expect { db[:sidecar_process_types].insert(sidecar_guid: sidecar.guid, type: 'web', app_guid: app.guid, guid: SecureRandom.uuid) }.to raise_error(Sequel::UniqueConstraintViolation) + expect { db[:sidecar_process_types].insert(sidecar_guid: sidecar_guid, type: 'web', app_guid: app_guid, guid: SecureRandom.uuid) }.to raise_error(Sequel::UniqueConstraintViolation) - expect(db[:revision_sidecar_process_types].where(revision_sidecar_guid: revision_sidecar.guid, type: 'worker').count).to eq(1) + expect(db[:revision_sidecar_process_types].where(revision_sidecar_guid: revision_sidecar_guid, type: 'worker').count).to eq(1) expect(db.indexes(:revision_sidecar_process_types)).to include(:revision_sidecar_process_types_revision_sidecar_guid_type_index) - expect { db[:revision_sidecar_process_types].insert(revision_sidecar_guid: revision_sidecar.guid, type: 'worker', guid: SecureRandom.uuid) }.to raise_error(Sequel::UniqueConstraintViolation) + expect { db[:revision_sidecar_process_types].insert(revision_sidecar_guid: revision_sidecar_guid, type: 'worker', guid: SecureRandom.uuid) }.to raise_error(Sequel::UniqueConstraintViolation) # ========================================================================================= # TEST IDEMPOTENCY: Running the migration again should not cause any errors. @@ -53,9 +58,9 @@ # ASSERT DOWN MIGRATION: Verify that constraints are removed and duplicates can be re-inserted. # ========================================================================================= expect(db.indexes(:sidecar_process_types)).not_to include(:sidecar_process_types_sidecar_guid_type_index) - expect { db[:sidecar_process_types].insert(sidecar_guid: sidecar.guid, type: 'web', app_guid: app.guid, guid: SecureRandom.uuid) }.not_to raise_error + expect { db[:sidecar_process_types].insert(sidecar_guid: sidecar_guid, type: 'web', app_guid: app_guid, guid: SecureRandom.uuid) }.not_to raise_error expect(db.indexes(:revision_sidecar_process_types)).not_to include(:revision_sidecar_process_types_revision_sidecar_guid_type_index) - expect { db[:revision_sidecar_process_types].insert(revision_sidecar_guid: revision_sidecar.guid, type: 'worker', guid: SecureRandom.uuid) }.not_to raise_error + expect { db[:revision_sidecar_process_types].insert(revision_sidecar_guid: revision_sidecar_guid, type: 'worker', guid: SecureRandom.uuid) }.not_to raise_error end end diff --git a/spec/migrations/20260320141005_add_unique_constraint_to_revision_sidecars_spec.rb b/spec/migrations/20260320141005_add_unique_constraint_to_revision_sidecars_spec.rb index 6dd325426d9..cf427949f09 100644 --- a/spec/migrations/20260320141005_add_unique_constraint_to_revision_sidecars_spec.rb +++ b/spec/migrations/20260320141005_add_unique_constraint_to_revision_sidecars_spec.rb @@ -5,22 +5,25 @@ let(:migration_filename) { '20260320141005_add_unique_constraint_to_revision_sidecars.rb' } end - let!(:app) { VCAP::CloudController::AppModel.make } - let!(:revision) { VCAP::CloudController::RevisionModel.make(:app) } + let(:app_guid) { SecureRandom.uuid } + let(:revision_guid) { SecureRandom.uuid } it 'remove duplicates, add constraint and revert migration' do + db[:apps].insert(guid: app_guid, name: 'test-app') + db[:revisions].insert(guid: revision_guid, app_guid: app_guid, description: 'some description') + # create duplicate entries - db[:revision_sidecars].insert(guid: SecureRandom.uuid, name: 'app', command: 'command', revision_guid: revision.guid) - db[:revision_sidecars].insert(guid: SecureRandom.uuid, name: 'app', command: 'command', revision_guid: revision.guid) - expect(db[:revision_sidecars].where(name: 'app', revision_guid: revision.guid).count).to eq(2) + db[:revision_sidecars].insert(guid: SecureRandom.uuid, name: 'app', command: 'command', revision_guid: revision_guid) + db[:revision_sidecars].insert(guid: SecureRandom.uuid, name: 'app', command: 'command', revision_guid: revision_guid) + expect(db[:revision_sidecars].where(name: 'app', revision_guid: revision_guid).count).to eq(2) # run the migration Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) # verify duplicates are removed and constraint is enforced - expect(db[:revision_sidecars].where(name: 'app', revision_guid: revision.guid).count).to eq(1) + expect(db[:revision_sidecars].where(name: 'app', revision_guid: revision_guid).count).to eq(1) expect(db.indexes(:revision_sidecars)).to include(:revision_sidecars_revision_guid_name_index) - expect { db[:revision_sidecars].insert(guid: SecureRandom.uuid, name: 'app', command: 'command', revision_guid: revision.guid) }.to raise_error(Sequel::UniqueConstraintViolation) + expect { db[:revision_sidecars].insert(guid: SecureRandom.uuid, name: 'app', command: 'command', revision_guid: revision_guid) }.to raise_error(Sequel::UniqueConstraintViolation) # running the migration again should not cause any errors expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error @@ -30,6 +33,6 @@ # verify constraint is removed and duplicates can be re-inserted expect(db.indexes(:revision_sidecars)).not_to include(:revision_sidecars_revision_guid_name_index) - expect { db[:revision_sidecars].insert(guid: SecureRandom.uuid, name: 'app', command: 'command', revision_guid: revision.guid) }.not_to raise_error + expect { db[:revision_sidecars].insert(guid: SecureRandom.uuid, name: 'app', command: 'command', revision_guid: revision_guid) }.not_to raise_error end end diff --git a/spec/migrations/20260323092954_add_unique_constraint_to_sidecars_spec.rb b/spec/migrations/20260323092954_add_unique_constraint_to_sidecars_spec.rb index ca979bc041f..cbc84da70db 100644 --- a/spec/migrations/20260323092954_add_unique_constraint_to_sidecars_spec.rb +++ b/spec/migrations/20260323092954_add_unique_constraint_to_sidecars_spec.rb @@ -5,21 +5,23 @@ let(:migration_filename) { '20260323092954_add_unique_constraint_to_sidecars.rb' } end - let!(:app) { VCAP::CloudController::AppModel.make } + let(:app_guid) { SecureRandom.uuid } it 'remove duplicates, add constraint and revert migration' do + db[:apps].insert(guid: app_guid, name: 'test-app') + # create duplicate entries - db[:sidecars].insert(guid: SecureRandom.uuid, name: 'app', command: 'command', app_guid: app.guid) - db[:sidecars].insert(guid: SecureRandom.uuid, name: 'app', command: 'command', app_guid: app.guid) - expect(db[:sidecars].where(name: 'app', app_guid: app.guid).count).to eq(2) + db[:sidecars].insert(guid: SecureRandom.uuid, name: 'app', command: 'command', app_guid: app_guid) + db[:sidecars].insert(guid: SecureRandom.uuid, name: 'app', command: 'command', app_guid: app_guid) + expect(db[:sidecars].where(name: 'app', app_guid: app_guid).count).to eq(2) # run the migration Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) # verify duplicates are removed and constraint is enforced - expect(db[:sidecars].where(name: 'app', app_guid: app.guid).count).to eq(1) + expect(db[:sidecars].where(name: 'app', app_guid: app_guid).count).to eq(1) expect(db.indexes(:sidecars)).to include(:sidecars_app_guid_name_index) - expect { db[:sidecars].insert(guid: SecureRandom.uuid, name: 'app', command: 'command', app_guid: app.guid) }.to raise_error(Sequel::UniqueConstraintViolation) + expect { db[:sidecars].insert(guid: SecureRandom.uuid, name: 'app', command: 'command', app_guid: app_guid) }.to raise_error(Sequel::UniqueConstraintViolation) # running the migration again should not cause any errors expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error @@ -29,6 +31,6 @@ # verify constraint is removed and duplicates can be re-inserted expect(db.indexes(:sidecars)).not_to include(:sidecars_app_guid_name_index) - expect { db[:sidecars].insert(guid: SecureRandom.uuid, name: 'app', command: 'command', app_guid: app.guid) }.not_to raise_error + expect { db[:sidecars].insert(guid: SecureRandom.uuid, name: 'app', command: 'command', app_guid: app_guid) }.not_to raise_error end end diff --git a/spec/migrations/20260323144429_add_unique_constraint_to_revision_process_commands_spec.rb b/spec/migrations/20260323144429_add_unique_constraint_to_revision_process_commands_spec.rb index f4a59565e90..ae49122ee66 100644 --- a/spec/migrations/20260323144429_add_unique_constraint_to_revision_process_commands_spec.rb +++ b/spec/migrations/20260323144429_add_unique_constraint_to_revision_process_commands_spec.rb @@ -6,22 +6,26 @@ let(:migration_filename) { '20260323144429_add_unique_constraint_to_revision_process_commands.rb' } end - let!(:revision) { VCAP::CloudController::RevisionModel.make } + let(:app_guid) { SecureRandom.uuid } + let(:revision_guid) { SecureRandom.uuid } it 'removes duplicates, adds constraint and reverts migration' do + db[:apps].insert(guid: app_guid, name: 'test-app') + db[:revisions].insert(guid: revision_guid, app_guid: app_guid, description: 'some description') + # create duplicate entries - db[:revision_process_commands].insert(guid: SecureRandom.uuid, revision_guid: revision.guid, process_type: 'worker') - db[:revision_process_commands].insert(guid: SecureRandom.uuid, revision_guid: revision.guid, process_type: 'worker') - expect(db[:revision_process_commands].where(revision_guid: revision.guid, process_type: 'worker').count).to eq(2) + db[:revision_process_commands].insert(guid: SecureRandom.uuid, revision_guid: revision_guid, process_type: 'worker') + db[:revision_process_commands].insert(guid: SecureRandom.uuid, revision_guid: revision_guid, process_type: 'worker') + expect(db[:revision_process_commands].where(revision_guid: revision_guid, process_type: 'worker').count).to eq(2) # run the migration Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) # verify duplicates are removed and constraint is enforced - expect(db[:revision_process_commands].where(revision_guid: revision.guid, process_type: 'worker').count).to eq(1) + expect(db[:revision_process_commands].where(revision_guid: revision_guid, process_type: 'worker').count).to eq(1) expect(db.indexes(:revision_process_commands)).to include(:revision_process_commands_revision_guid_process_type_index) expect do - db[:revision_process_commands].insert(guid: SecureRandom.uuid, revision_guid: revision.guid, process_type: 'worker') + db[:revision_process_commands].insert(guid: SecureRandom.uuid, revision_guid: revision_guid, process_type: 'worker') end.to raise_error(Sequel::UniqueConstraintViolation) # running the migration again should not cause any errors @@ -33,7 +37,7 @@ # verify constraint is removed and duplicates can be re-inserted expect(db.indexes(:revision_process_commands)).not_to include(:revision_process_commands_revision_guid_process_type_index) expect do - db[:revision_process_commands].insert(guid: SecureRandom.uuid, revision_guid: revision.guid, process_type: 'worker') + db[:revision_process_commands].insert(guid: SecureRandom.uuid, revision_guid: revision_guid, process_type: 'worker') end.not_to raise_error end end