From e0acf56f1e27cbf737e3f68947a56492f08c82ce Mon Sep 17 00:00:00 2001 From: Philipp Thun Date: Tue, 28 Apr 2026 12:03:31 +0200 Subject: [PATCH] Add rubocop cop to prevent model usage in migration specs Migration specs roll back the schema before running, so model classes may reference columns that don't exist yet. Using raw Sequel operations ensures specs are independent of the current model code, as documented in spec/migrations/Readme.md. - Add Migration/NoModelInSpecs cop that flags any method call on a *Model constant receiver in spec/migrations/ - Replace model .make calls with raw db[:table].insert in four migration specs: sidecars, revision_sidecars, sidecar_process_types, and revision_process_commands --- .rubocop_cc.yml | 7 ++ spec/linters/migration/no_model_in_specs.rb | 25 +++++++ .../migration/no_model_in_specs_spec.rb | 74 +++++++++++++++++++ ...onstraint_to_sidecar_process_types_spec.rb | 37 ++++++---- ...ue_constraint_to_revision_sidecars_spec.rb | 19 +++-- ..._add_unique_constraint_to_sidecars_spec.rb | 16 ++-- ...raint_to_revision_process_commands_spec.rb | 18 +++-- 7 files changed, 158 insertions(+), 38 deletions(-) create mode 100644 spec/linters/migration/no_model_in_specs.rb create mode 100644 spec/linters/migration/no_model_in_specs_spec.rb 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