diff --git a/db/migrations/20260318083940_add_unique_constraint_to_buildpacks.rb b/db/migrations/20260318083940_add_unique_constraint_to_buildpacks.rb index 10e28885ee0..f63fa4e9617 100644 --- a/db/migrations/20260318083940_add_unique_constraint_to_buildpacks.rb +++ b/db/migrations/20260318083940_add_unique_constraint_to_buildpacks.rb @@ -6,13 +6,14 @@ duplicates = self[:buildpacks].select(:name, :stack, :lifecycle).group(:name, :stack, :lifecycle).having { count(1) > 1 } duplicates.each do |dup| - ids_to_remove = self[:buildpacks]. - where(name: dup[:name], stack: dup[:stack], lifecycle: dup[:lifecycle]). - select(:id). - order(:id). - offset(1). - map(:id) + rows_to_remove = self[:buildpacks].where(name: dup[:name], stack: dup[:stack], lifecycle: dup[:lifecycle]). + select(:id, :guid).order(:id).offset(1).all + guids_to_remove = rows_to_remove.map { |r| r[:guid] } + ids_to_remove = rows_to_remove.map { |r| r[:id] } + + self[:buildpack_annotations].where(resource_guid: guids_to_remove).delete + self[:buildpack_labels].where(resource_guid: guids_to_remove).delete self[:buildpacks].where(id: ids_to_remove).delete end end diff --git a/db/migrations/20260323130619_add_unique_constraint_to_security_groups.rb b/db/migrations/20260323130619_add_unique_constraint_to_security_groups.rb index cef9f459f88..052a10738da 100644 --- a/db/migrations/20260323130619_add_unique_constraint_to_security_groups.rb +++ b/db/migrations/20260323130619_add_unique_constraint_to_security_groups.rb @@ -14,6 +14,8 @@ order(:id). offset(1). map(:id) + self[:security_groups_spaces].where(security_group_id: ids_to_remove).delete + self[:staging_security_groups_spaces].where(staging_security_group_id: ids_to_remove).delete self[:security_groups].where(id: ids_to_remove).delete end end diff --git a/spec/migrations/20260318083940_add_unique_constraint_to_buildpacks_spec.rb b/spec/migrations/20260318083940_add_unique_constraint_to_buildpacks_spec.rb index a424ba278b5..2347f8b1b59 100644 --- a/spec/migrations/20260318083940_add_unique_constraint_to_buildpacks_spec.rb +++ b/spec/migrations/20260318083940_add_unique_constraint_to_buildpacks_spec.rb @@ -11,8 +11,11 @@ # Drop old unique index so we can insert duplicates db.alter_table(:buildpacks) { drop_index %i[name stack], name: :unique_name_and_stack } - db[:buildpacks].insert(guid: SecureRandom.uuid, name: 'ruby', stack: 'cflinuxfs3', lifecycle: 'buildpack', position: 1) - db[:buildpacks].insert(guid: SecureRandom.uuid, name: 'ruby', stack: 'cflinuxfs3', lifecycle: 'buildpack', position: 2) + surviving_guid = SecureRandom.uuid + duplicate_guid = SecureRandom.uuid + + db[:buildpacks].insert(guid: surviving_guid, name: 'ruby', stack: 'cflinuxfs3', lifecycle: 'buildpack', position: 1) + db[:buildpacks].insert(guid: duplicate_guid, name: 'ruby', stack: 'cflinuxfs3', lifecycle: 'buildpack', position: 2) db[:buildpacks].insert(guid: SecureRandom.uuid, name: 'ruby', stack: 'cflinuxfs3', lifecycle: 'cnb', position: 3) db[:buildpacks].insert(guid: SecureRandom.uuid, name: 'ruby', stack: 'cflinuxfs4', lifecycle: 'buildpack', position: 4) db[:buildpacks].insert(guid: SecureRandom.uuid, name: 'go', stack: 'cflinuxfs3', lifecycle: 'buildpack', position: 5) @@ -22,8 +25,14 @@ expect(db[:buildpacks].where(name: 'ruby', stack: 'cflinuxfs3', lifecycle: 'buildpack').count).to eq(2) expect(db[:buildpacks].where(name: 'go', stack: 'cflinuxfs3', lifecycle: 'buildpack').count).to eq(3) - # === UP MIGRATION === - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + # add annotations and labels referencing the duplicate buildpack + db[:buildpack_annotations].insert(guid: SecureRandom.uuid, resource_guid: surviving_guid, key_name: 'env', value: 'prod') + db[:buildpack_annotations].insert(guid: SecureRandom.uuid, resource_guid: duplicate_guid, key_name: 'env', value: 'staging') + db[:buildpack_labels].insert(guid: SecureRandom.uuid, resource_guid: surviving_guid, key_name: 'team', value: 'a') + db[:buildpack_labels].insert(guid: SecureRandom.uuid, resource_guid: duplicate_guid, key_name: 'team', value: 'b') + + # run the migration + Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) # Verify duplicates are removed, keeping one per (name, stack, lifecycle) expect(db[:buildpacks].where(name: 'ruby', stack: 'cflinuxfs3', lifecycle: 'buildpack').count).to eq(1) @@ -31,6 +40,12 @@ expect(db[:buildpacks].where(name: 'ruby', stack: 'cflinuxfs4', lifecycle: 'buildpack').count).to eq(1) expect(db[:buildpacks].where(name: 'go', stack: 'cflinuxfs3', lifecycle: 'buildpack').count).to eq(1) + # verify annotations and labels for the duplicate are removed, surviving buildpack intact + expect(db[:buildpack_annotations].where(resource_guid: duplicate_guid).count).to eq(0) + expect(db[:buildpack_labels].where(resource_guid: duplicate_guid).count).to eq(0) + expect(db[:buildpack_annotations].where(resource_guid: surviving_guid).count).to eq(1) + expect(db[:buildpack_labels].where(resource_guid: surviving_guid).count).to eq(1) + # Verify old index is dropped and new index is added expect(db.indexes(:buildpacks)).not_to include(:unique_name_and_stack) expect(db.indexes(:buildpacks)).to include(:buildpacks_name_stack_lifecycle_index) @@ -38,8 +53,9 @@ # Test up migration idempotency expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - # === DOWN MIGRATION === # First remove test data that would conflict with the old (name, stack) unique index + db[:buildpack_annotations].delete + db[:buildpack_labels].delete db[:buildpacks].delete expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error diff --git a/spec/migrations/20260323130619_add_unique_constraint_to_security_groups_spec.rb b/spec/migrations/20260323130619_add_unique_constraint_to_security_groups_spec.rb index e91848b9298..0f75d2e1366 100644 --- a/spec/migrations/20260323130619_add_unique_constraint_to_security_groups_spec.rb +++ b/spec/migrations/20260323130619_add_unique_constraint_to_security_groups_spec.rb @@ -5,17 +5,32 @@ let(:migration_filename) { '20260323130619_add_unique_constraint_to_security_groups.rb' } end - it 'remove duplicates, add constraint and revert migration' do - # create duplicate entries - db[:security_groups].insert(guid: SecureRandom.uuid, name: 'sec1') - db[:security_groups].insert(guid: SecureRandom.uuid, name: 'sec1') + it 'removes duplicates, adds constraint and reverts migration' do + space = VCAP::CloudController::Space.make + + # create duplicate entries with join table references + surviving_id = db[:security_groups].insert(guid: SecureRandom.uuid, name: 'sec1') + duplicate_id = db[:security_groups].insert(guid: SecureRandom.uuid, name: 'sec1') expect(db[:security_groups].where(name: 'sec1').count).to eq(2) + # add security_groups_spaces and staging_security_groups_spaces referencing the duplicate security_groups + db[:security_groups_spaces].insert(security_group_id: surviving_id, space_id: space.id) + db[:security_groups_spaces].insert(security_group_id: duplicate_id, space_id: space.id) + db[:staging_security_groups_spaces].insert(staging_security_group_id: surviving_id, staging_space_id: space.id) + db[:staging_security_groups_spaces].insert(staging_security_group_id: duplicate_id, staging_space_id: space.id) + # 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 + # verify duplicates and their join table references are removed, surviving group intact expect(db[:security_groups].where(name: 'sec1').count).to eq(1) + expect(db[:security_groups].where(name: 'sec1').first[:id]).to eq(surviving_id) + expect(db[:security_groups_spaces].where(security_group_id: duplicate_id).count).to eq(0) + expect(db[:staging_security_groups_spaces].where(staging_security_group_id: duplicate_id).count).to eq(0) + expect(db[:security_groups_spaces].where(security_group_id: surviving_id, space_id: space.id).count).to eq(1) + expect(db[:staging_security_groups_spaces].where(staging_security_group_id: surviving_id, staging_space_id: space.id).count).to eq(1) + + # verify constraint is enforced expect(db.indexes(:security_groups)).to include(:security_groups_name_index) expect { db[:security_groups].insert(guid: SecureRandom.uuid, name: 'sec1') }.to raise_error(Sequel::UniqueConstraintViolation)