diff --git a/spec/migrations/20190712210940_backfill_status_for_deployments_spec.rb b/spec/migrations/20190712210940_backfill_status_for_deployments_spec.rb index 39101ca3703..99224af9022 100644 --- a/spec/migrations/20190712210940_backfill_status_for_deployments_spec.rb +++ b/spec/migrations/20190712210940_backfill_status_for_deployments_spec.rb @@ -12,169 +12,117 @@ let(:app) { VCAP::CloudController::AppModel.make } - context 'when a deployment has state DEPLOYED' do - let!(:deployment_with_state_deployed) do - VCAP::CloudController::DeploymentModel.create( - guid: 'with-state-deployed', - state: VCAP::CloudController::DeploymentModel::DEPLOYED_STATE, - app: app, - original_web_process_instance_count: 1 - ) - end - - it 'sets status_value to FINALIZED without changing the state' do - Sequel::Migrator.run(VCAP::CloudController::DeploymentModel.db, tmp_migrations_dir, table: :my_fake_table) - deployment = VCAP::CloudController::DeploymentModel.where(guid: deployment_with_state_deployed.guid).first - - expect(deployment.state).to eq(VCAP::CloudController::DeploymentModel::DEPLOYED_STATE) - expect(deployment.status_value).to eq(VCAP::CloudController::DeploymentModel::FINALIZED_STATUS_VALUE) - expect(deployment.status_reason).to be_nil - end - end + it 'backfills status_value based on deployment state' do + # Create all deployment variations + deployment_deployed = VCAP::CloudController::DeploymentModel.create( + guid: 'with-state-deployed', + state: VCAP::CloudController::DeploymentModel::DEPLOYED_STATE, + app: app, + original_web_process_instance_count: 1 + ) - context 'when a deployment has state CANCELED' do - let!(:deployment_with_state_canceled) do - VCAP::CloudController::DeploymentModel.create( - guid: 'with-state-canceled', - state: VCAP::CloudController::DeploymentModel::CANCELED_STATE, - app: app, - original_web_process_instance_count: 1 - ) - end - - it 'sets status_value to FINALIZED without changing the state' do - Sequel::Migrator.run(VCAP::CloudController::DeploymentModel.db, tmp_migrations_dir, table: :my_fake_table) - deployment = VCAP::CloudController::DeploymentModel.where(guid: deployment_with_state_canceled.guid).first - - expect(deployment.state).to eq(VCAP::CloudController::DeploymentModel::CANCELED_STATE) - expect(deployment.status_value).to eq(VCAP::CloudController::DeploymentModel::FINALIZED_STATUS_VALUE) - expect(deployment.status_reason).to be_nil - end - end + deployment_canceled = VCAP::CloudController::DeploymentModel.create( + guid: 'with-state-canceled', + state: VCAP::CloudController::DeploymentModel::CANCELED_STATE, + app: app, + original_web_process_instance_count: 1 + ) - context 'when a deployment has state FAILED' do - let!(:deployment_with_state_failed) do - VCAP::CloudController::DeploymentModel.create( - guid: 'with-state-failed', - state: 'FAILED', - app: app, - original_web_process_instance_count: 1 - ) - end - - it 'sets status_value to FINALIZED without changing the state' do - Sequel::Migrator.run(VCAP::CloudController::DeploymentModel.db, tmp_migrations_dir, table: :my_fake_table) - deployment = VCAP::CloudController::DeploymentModel.where(guid: deployment_with_state_failed.guid).first - - expect(deployment.state).to eq(VCAP::CloudController::DeploymentModel::DEPLOYED_STATE) - expect(deployment.status_value).to eq(VCAP::CloudController::DeploymentModel::FINALIZED_STATUS_VALUE) - expect(deployment.status_reason).to be_nil - end - end + deployment_failed = VCAP::CloudController::DeploymentModel.create( + guid: 'with-state-failed', + state: 'FAILED', + app: app, + original_web_process_instance_count: 1 + ) - context 'when a deployment has state DEPLOYING' do - let!(:deployment_with_state_deploying) do - VCAP::CloudController::DeploymentModel.create( - guid: 'with-state-deploying', - state: VCAP::CloudController::DeploymentModel::DEPLOYING_STATE, - app: app, - original_web_process_instance_count: 1 - ) - end - - it 'sets status_value to DEPLOYING without changing the state' do - Sequel::Migrator.run(VCAP::CloudController::DeploymentModel.db, tmp_migrations_dir, table: :my_fake_table) - deployment = VCAP::CloudController::DeploymentModel.where(guid: deployment_with_state_deploying.guid).first - - expect(deployment.state).to eq(VCAP::CloudController::DeploymentModel::DEPLOYING_STATE) - expect(deployment.status_value).to eq('DEPLOYING') - expect(deployment.status_reason).to be_nil - end - end + deployment_deploying = VCAP::CloudController::DeploymentModel.create( + guid: 'with-state-deploying', + state: VCAP::CloudController::DeploymentModel::DEPLOYING_STATE, + app: app, + original_web_process_instance_count: 1 + ) - context 'when a deployment has state CANCELING' do - let!(:deployment_with_state_canceling) do - VCAP::CloudController::DeploymentModel.create( - guid: 'with-state-canceling', - state: VCAP::CloudController::DeploymentModel::CANCELING_STATE, - app: app, - original_web_process_instance_count: 1 - ) - end - - it 'sets status_value to DEPLOYING without changing the state' do - Sequel::Migrator.run(VCAP::CloudController::DeploymentModel.db, tmp_migrations_dir, table: :my_fake_table) - deployment = VCAP::CloudController::DeploymentModel.where(guid: deployment_with_state_canceling.guid).first - - expect(deployment.state).to eq(VCAP::CloudController::DeploymentModel::CANCELING_STATE) - expect(deployment.status_value).to eq('DEPLOYING') - expect(deployment.status_reason).to be_nil - end - end + deployment_canceling = VCAP::CloudController::DeploymentModel.create( + guid: 'with-state-canceling', + state: VCAP::CloudController::DeploymentModel::CANCELING_STATE, + app: app, + original_web_process_instance_count: 1 + ) - context 'when a deployment has state FAILING' do - let!(:deployment_with_state_failing) do - VCAP::CloudController::DeploymentModel.create( - guid: 'with-state-failing', - state: 'FAILING', - app: app, - original_web_process_instance_count: 1 - ) - end - - it 'sets status_value to DEPLOYING without changing the state' do - Sequel::Migrator.run(VCAP::CloudController::DeploymentModel.db, tmp_migrations_dir, table: :my_fake_table) - deployment = VCAP::CloudController::DeploymentModel.where(guid: deployment_with_state_failing.guid).first - - expect(deployment.state).to eq(VCAP::CloudController::DeploymentModel::DEPLOYING_STATE) - expect(deployment.status_value).to eq('DEPLOYING') - expect(deployment.status_reason).to be_nil - end - end + deployment_failing = VCAP::CloudController::DeploymentModel.create( + guid: 'with-state-failing', + state: 'FAILING', + app: app, + original_web_process_instance_count: 1 + ) + + deployment_with_existing_status = VCAP::CloudController::DeploymentModel.create( + guid: 'with-existing-status', + state: VCAP::CloudController::DeploymentModel::DEPLOYED_STATE, + status_value: 'foo', + status_reason: 'bar', + app: app, + original_web_process_instance_count: 1 + ) + + deployment_failing_with_reason = VCAP::CloudController::DeploymentModel.create( + guid: 'failing-with-reason', + state: 'FAILING', + status_value: 'foo', + status_reason: 'bar', + app: app, + original_web_process_instance_count: 1 + ) - context 'when the deployment already has a status' do - context 'when the deployment has state DEPLOYED' do - let!(:deployment_with_state_deployed) do - VCAP::CloudController::DeploymentModel.create( - guid: 'with-state-deployed', - state: VCAP::CloudController::DeploymentModel::DEPLOYED_STATE, - status_value: 'foo', - status_reason: 'bar', - app: app, - original_web_process_instance_count: 1 - ) - end - - it 'does not reset the status value' do - Sequel::Migrator.run(VCAP::CloudController::DeploymentModel.db, tmp_migrations_dir, table: :my_fake_table) - deployment = VCAP::CloudController::DeploymentModel.where(guid: deployment_with_state_deployed.guid).first - - expect(deployment.state).to eq(VCAP::CloudController::DeploymentModel::DEPLOYED_STATE) - expect(deployment.status_value).to eq('foo') - expect(deployment.status_reason).to eq('bar') - end - end - - context 'when the deployment has state FAILING' do - let!(:deployment_with_state_failing) do - VCAP::CloudController::DeploymentModel.create( - guid: 'with-state-deployed', - state: 'FAILING', - status_value: 'foo', - status_reason: 'bar', - app: app, - original_web_process_instance_count: 1 - ) - end - - it 'does not reset the status reason' do - Sequel::Migrator.run(VCAP::CloudController::DeploymentModel.db, tmp_migrations_dir, table: :my_fake_table) - deployment = VCAP::CloudController::DeploymentModel.where(guid: deployment_with_state_failing.guid).first - - expect(deployment.state).to eq(VCAP::CloudController::DeploymentModel::DEPLOYING_STATE) - expect(deployment.status_value).to eq('DEPLOYING') - expect(deployment.status_reason).to eq('bar') - end - end + # Run migration once + Sequel::Migrator.run(VCAP::CloudController::DeploymentModel.db, tmp_migrations_dir, table: :my_fake_table) + + # Test: DEPLOYED state -> FINALIZED status + deployment = VCAP::CloudController::DeploymentModel.where(guid: deployment_deployed.guid).first + expect(deployment.state).to eq(VCAP::CloudController::DeploymentModel::DEPLOYED_STATE) + expect(deployment.status_value).to eq(VCAP::CloudController::DeploymentModel::FINALIZED_STATUS_VALUE) + expect(deployment.status_reason).to be_nil + + # Test: CANCELED state -> FINALIZED status + deployment = VCAP::CloudController::DeploymentModel.where(guid: deployment_canceled.guid).first + expect(deployment.state).to eq(VCAP::CloudController::DeploymentModel::CANCELED_STATE) + expect(deployment.status_value).to eq(VCAP::CloudController::DeploymentModel::FINALIZED_STATUS_VALUE) + expect(deployment.status_reason).to be_nil + + # Test: FAILED state -> DEPLOYED state + FINALIZED status + deployment = VCAP::CloudController::DeploymentModel.where(guid: deployment_failed.guid).first + expect(deployment.state).to eq(VCAP::CloudController::DeploymentModel::DEPLOYED_STATE) + expect(deployment.status_value).to eq(VCAP::CloudController::DeploymentModel::FINALIZED_STATUS_VALUE) + expect(deployment.status_reason).to be_nil + + # Test: DEPLOYING state -> DEPLOYING status + deployment = VCAP::CloudController::DeploymentModel.where(guid: deployment_deploying.guid).first + expect(deployment.state).to eq(VCAP::CloudController::DeploymentModel::DEPLOYING_STATE) + expect(deployment.status_value).to eq('DEPLOYING') + expect(deployment.status_reason).to be_nil + + # Test: CANCELING state -> DEPLOYING status + deployment = VCAP::CloudController::DeploymentModel.where(guid: deployment_canceling.guid).first + expect(deployment.state).to eq(VCAP::CloudController::DeploymentModel::CANCELING_STATE) + expect(deployment.status_value).to eq('DEPLOYING') + expect(deployment.status_reason).to be_nil + + # Test: FAILING state -> DEPLOYING state + DEPLOYING status + deployment = VCAP::CloudController::DeploymentModel.where(guid: deployment_failing.guid).first + expect(deployment.state).to eq(VCAP::CloudController::DeploymentModel::DEPLOYING_STATE) + expect(deployment.status_value).to eq('DEPLOYING') + expect(deployment.status_reason).to be_nil + + # Test: existing status_value is not reset + deployment = VCAP::CloudController::DeploymentModel.where(guid: deployment_with_existing_status.guid).first + expect(deployment.state).to eq(VCAP::CloudController::DeploymentModel::DEPLOYED_STATE) + expect(deployment.status_value).to eq('foo') + expect(deployment.status_reason).to eq('bar') + + # Test: existing status_reason is preserved + deployment = VCAP::CloudController::DeploymentModel.where(guid: deployment_failing_with_reason.guid).first + expect(deployment.state).to eq(VCAP::CloudController::DeploymentModel::DEPLOYING_STATE) + expect(deployment.status_value).to eq('DEPLOYING') + expect(deployment.status_reason).to eq('bar') end end diff --git a/spec/migrations/20230822153000_streamline_annotation_key_prefix_spec.rb b/spec/migrations/20230822153000_streamline_annotation_key_prefix_spec.rb index b3e70aaeec6..8732775be59 100644 --- a/spec/migrations/20230822153000_streamline_annotation_key_prefix_spec.rb +++ b/spec/migrations/20230822153000_streamline_annotation_key_prefix_spec.rb @@ -7,40 +7,35 @@ end describe 'annotation tables' do - it 'converts all legacy key_prefixes to annotations with prefixes in the key_prefix column' do - db[:isolation_segments].insert(name: 'bommel', guid: '123') - db[:isolation_segment_annotations].insert( - guid: 'bommel', - created_at: Time.now - 60, - updated_at: Time.now - 60, - resource_guid: '123', - key: 'mylegacyprefix/mykey', - value: 'some_value' - ) - a1 = db[:isolation_segment_annotations].first(resource_guid: '123') - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - b1 = db[:isolation_segment_annotations].first(resource_guid: '123') - expect(b1[:guid]).to eq a1[:guid] - expect(b1[:created_at]).to eq a1[:created_at] - expect(b1[:updated_at]).not_to eq a1[:updated_at] - expect(b1[:resource_guid]).to eq a1[:resource_guid] - expect(b1[:key_prefix]).not_to eq a1[:key_prefix] - expect(b1[:key]).not_to eq a1[:key] - expect(b1[:key_prefix]).to eq 'mylegacyprefix' - expect(b1[:key]).to eq 'mykey' - end + it 'converts legacy key_prefixes to prefixes in key_prefix column and leaves non-legacy values unchanged' do + resource_guid = 'iso-seg-guid' + db[:isolation_segments].insert(name: 'iso_seg', guid: resource_guid) + db[:isolation_segment_annotations].insert(guid: 'anno-1-guid', resource_guid: resource_guid, key: 'mylegacyprefix/mykey', value: 'some_value') + db[:isolation_segment_annotations].insert(guid: 'anno-2-guid', resource_guid: resource_guid, key_prefix: 'myprefix', key: 'mykey', value: 'some_value') + db[:isolation_segment_annotations].insert(guid: 'anno-3-guid', resource_guid: resource_guid, key: 'yourkey', value: 'some_other_value') + + anno1 = db[:isolation_segment_annotations].first(guid: 'anno-1-guid') + anno2 = db[:isolation_segment_annotations].first(key: 'mykey') + anno3 = db[:isolation_segment_annotations].first(key: 'yourkey') - it 'doesnt touch any values that have no legacy key_prefix in its key field' do - db[:isolation_segments].insert(name: 'bommel', guid: '123') - db[:isolation_segment_annotations].insert(guid: 'bommel', resource_guid: '123', key_prefix: 'myprefix', key: 'mykey', value: 'some_value') - db[:isolation_segment_annotations].insert(guid: 'bommel2', resource_guid: '123', key: 'mykey2', value: 'some_value2') - b1 = db[:isolation_segment_annotations].first(key: 'mykey') - b2 = db[:isolation_segment_annotations].first(key: 'mykey2') expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - c1 = db[:isolation_segment_annotations].first(key: 'mykey') - c2 = db[:isolation_segment_annotations].first(key: 'mykey2') - expect(b1.values).to eq(c1.values) - expect(b2.values).to eq(c2.values) + + # Check legacy prefix was converted + anno1_after_mig = db[:isolation_segment_annotations].first(guid: 'anno-1-guid') + expect(anno1_after_mig[:guid]).to eq anno1[:guid] + expect(anno1_after_mig[:created_at]).to eq anno1[:created_at] + expect(anno1_after_mig[:updated_at]).not_to eq anno1[:updated_at] + expect(anno1_after_mig[:resource_guid]).to eq anno1[:resource_guid] + expect(anno1_after_mig[:key_prefix]).not_to eq anno1[:key_prefix] + expect(anno1_after_mig[:key]).not_to eq anno1[:key] + expect(anno1_after_mig[:key_prefix]).to eq 'mylegacyprefix' + expect(anno1_after_mig[:key]).to eq 'mykey' + + # Check non-legacy values unchanged + anno2_after_mig = db[:isolation_segment_annotations].first(guid: 'anno-2-guid') + anno3_after_mig = db[:isolation_segment_annotations].first(guid: 'anno-3-guid') + expect(anno2.values).to eq(anno2_after_mig.values) + expect(anno3.values).to eq(anno3_after_mig.values) end end end diff --git a/spec/migrations/20240102150000_add_annotation_label_uniqueness_spec.rb b/spec/migrations/20240102150000_add_annotation_label_uniqueness_spec.rb index fea59a4f8a9..6a9d27aad46 100644 --- a/spec/migrations/20240102150000_add_annotation_label_uniqueness_spec.rb +++ b/spec/migrations/20240102150000_add_annotation_label_uniqueness_spec.rb @@ -1,237 +1,195 @@ require 'spec_helper' require 'migrations/helpers/migration_shared_context' - RSpec.describe 'migration to add unique constraint to annotation and labels', isolation: :truncation, type: :migration do include_context 'migration' do let(:migration_filename) { '20240102150000_add_annotation_label_uniqueness.rb' } end - let(:isolation_segment) { VCAP::CloudController::IsolationSegmentModel } let(:annotation) { VCAP::CloudController::IsolationSegmentAnnotationModel } let(:label) { VCAP::CloudController::IsolationSegmentLabelModel } describe 'annotation tables' do - it 'truncates keys to 63 characters' do + it 'handles key truncation, duplicate removal, and uniqueness constraints' do + # Setup data for truncation test i1 = isolation_segment.create(name: 'bommel') - key_name = 'a' * 64 + key_name_long = 'a' * 64 truncated_key_name = 'a' * 63 + key_name_short = 'b' * 63 + trunc_a1 = annotation.create(resource_guid: i1.guid, key_name: key_name_long, value: 'some_value') + trunc_a2 = annotation.create(resource_guid: i1.guid, key_name: key_name_short, value: 'some_value2') - a1 = annotation.create(resource_guid: i1.guid, key_name: key_name, value: 'some_value') - - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - expect(a1.reload.key_name).to eq(truncated_key_name) - end - - it 'leaves keys that are shorter than 64 characters unchanged' do - i1 = isolation_segment.create(name: 'bommel') - key_name = 'a' * 63 - - a1 = annotation.create(resource_guid: i1.guid, key_name: key_name, value: 'some_value') - - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - expect(a1.reload.key_name).to eq(key_name) - end - - it 'removes duplicate annotations but keeps one with smallest id' do - i1 = isolation_segment.create(name: 'bommel') - key_name = 'a' * 63 + # Setup data for duplicate removal test + i2 = isolation_segment.create(name: 'duplicate_test') + key_c = 'c' * 63 # In case key_prefix is not set - a1 = annotation.create(resource_guid: i1.guid, key_name: key_name, value: 'v1') - a2 = annotation.create(resource_guid: i1.guid, key_name: key_name, value: 'v2') - a3 = annotation.create(resource_guid: i1.guid, key_name: key_name, value: 'v3') - # In case key_prefix is set - b1 = annotation.create(resource_guid: i1.guid, key_prefix: 'bommel', key_name: key_name, value: 'v1') - b2 = annotation.create(resource_guid: i1.guid, key_prefix: 'bommel', key_name: key_name, value: 'v2') - b3 = annotation.create(resource_guid: i1.guid, key_prefix: 'bommel', key_name: key_name, value: 'v3') - - expect(a1.id).to be < a2.id - expect(a1.id).to be < a3.id - expect(b1.id).to be < b2.id - expect(b1.id).to be < b3.id - - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - expect(annotation.where(key_name:).count).to eq(2) - expect(a1.reload).to be_a(annotation) - expect { a2.reload }.to raise_error(Sequel::NoExistingObject) - expect { a3.reload }.to raise_error(Sequel::NoExistingObject) - expect(b1.reload).to be_a(annotation) - expect { b2.reload }.to raise_error(Sequel::NoExistingObject) - expect { b3.reload }.to raise_error(Sequel::NoExistingObject) - end - - it 'does not remove records if any column of key, key_prefix or resource_guid is different' do - i1 = isolation_segment.create(name: 'bommel') - i2 = isolation_segment.create(name: 'sword') - key_a = 'a' * 63 - key_b = 'b' * 63 + dup_a1 = annotation.create(resource_guid: i2.guid, key_name: key_c, value: 'v1') + dup_a2 = annotation.create(resource_guid: i2.guid, key_name: key_c, value: 'v2') + dup_a3 = annotation.create(resource_guid: i2.guid, key_name: key_c, value: 'v3') - # In case key_prefix is not set - a1 = annotation.create(resource_guid: i1.guid, key_name: key_a, value: 'v1') - a2 = annotation.create(resource_guid: i2.guid, key_name: key_a, value: 'v2') - a3 = annotation.create(resource_guid: i1.guid, key_name: key_b, value: 'v3') # In case key_prefix is set - b1 = annotation.create(resource_guid: i1.guid, key_prefix: 'bommel', key_name: key_a, value: 'v1') - b2 = annotation.create(resource_guid: i2.guid, key_prefix: 'bommel', key_name: key_a, value: 'v2') - b3 = annotation.create(resource_guid: i1.guid, key_prefix: 'bommel', key_name: key_b, value: 'v3') - b4 = annotation.create(resource_guid: i1.guid, key_prefix: 'sword', key_name: key_a, value: 'v4') - - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + dup_b1 = annotation.create(resource_guid: i2.guid, key_prefix: 'bommel', key_name: key_c, value: 'v1') + dup_b2 = annotation.create(resource_guid: i2.guid, key_prefix: 'bommel', key_name: key_c, value: 'v2') + dup_b3 = annotation.create(resource_guid: i2.guid, key_prefix: 'bommel', key_name: key_c, value: 'v3') - expect(annotation.all.count).to eq(7) - expect(a1.reload).to be_a(annotation) - expect(a2.reload).to be_a(annotation) - expect(a3.reload).to be_a(annotation) - expect(b1.reload).to be_a(annotation) - expect(b2.reload).to be_a(annotation) - expect(b3.reload).to be_a(annotation) - expect(b4.reload).to be_a(annotation) - end + expect(dup_a1.id).to be < dup_a2.id + expect(dup_a1.id).to be < dup_a3.id + expect(dup_b1.id).to be < dup_b2.id + expect(dup_b1.id).to be < dup_b3.id - it 'does not allow adding a duplicate' do - i1 = isolation_segment.create(name: 'bommel') - i2 = isolation_segment.create(name: 'sword') - key = 'a' * 63 + # Setup data for preservation test (different columns) + i3 = isolation_segment.create(name: 'sword') + key_d = 'd' * 63 + key_e = 'e' * 63 # In case key_prefix is not set - annotation.create(resource_guid: i1.guid, key_name: key, value: 'v1') + pres_a1 = annotation.create(resource_guid: i1.guid, key_name: key_d, value: 'v1') + pres_a2 = annotation.create(resource_guid: i3.guid, key_name: key_d, value: 'v2') + pres_a3 = annotation.create(resource_guid: i1.guid, key_name: key_e, value: 'v3') + # In case key_prefix is set - annotation.create(resource_guid: i2.guid, key_prefix: 'bommel', key_name: key, value: 'v1') + pres_b1 = annotation.create(resource_guid: i1.guid, key_prefix: 'prefix1', key_name: key_d, value: 'v1') + pres_b2 = annotation.create(resource_guid: i3.guid, key_prefix: 'prefix1', key_name: key_d, value: 'v2') + pres_b3 = annotation.create(resource_guid: i1.guid, key_prefix: 'prefix1', key_name: key_e, value: 'v3') + pres_b4 = annotation.create(resource_guid: i1.guid, key_prefix: 'prefix2', key_name: key_d, value: 'v4') - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + # Setup data for uniqueness constraint test + i4 = isolation_segment.create(name: 'unique_test') + i5 = isolation_segment.create(name: 'unique_test2') + key_f = 'f' * 63 + key_g = 'g' * 63 - expect { annotation.create(resource_guid: i1.guid, key_name: key, value: 'v2') }.to raise_error(Sequel::UniqueConstraintViolation) - expect { annotation.create(resource_guid: i2.guid, key_prefix: 'bommel', key_name: key, value: 'v2') }.to raise_error(Sequel::UniqueConstraintViolation) - end + # In case key_prefix is not set + annotation.create(resource_guid: i4.guid, key_name: key_f, value: 'v1') - it 'does allow adding a different annotation' do - i1 = isolation_segment.create(name: 'bommel') - i2 = isolation_segment.create(name: 'sword') - key_a = 'a' * 63 - key_b = 'b' * 63 + # In case key_prefix is set + annotation.create(resource_guid: i5.guid, key_prefix: 'unique_prefix', key_name: key_f, value: 'v1') + # Run migration once expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - # In case key_prefix is not set - a1 = annotation.create(resource_guid: i1.guid, key_name: key_a, value: 'v1') - a2 = annotation.create(resource_guid: i2.guid, key_name: key_a, value: 'v2') - a3 = annotation.create(resource_guid: i1.guid, key_name: key_b, value: 'v3') - # In case key_prefix is set - b1 = annotation.create(resource_guid: i1.guid, key_prefix: 'bommel', key_name: key_a, value: 'v1') - b2 = annotation.create(resource_guid: i2.guid, key_prefix: 'bommel', key_name: key_a, value: 'v2') - b3 = annotation.create(resource_guid: i1.guid, key_prefix: 'bommel', key_name: key_b, value: 'v3') - b4 = annotation.create(resource_guid: i1.guid, key_prefix: 'sword', key_name: key_a, value: 'v4') - - expect(annotation.all.count).to eq(7) - expect(a1.reload).to be_a(annotation) - expect(a2.reload).to be_a(annotation) - expect(a3.reload).to be_a(annotation) - expect(b1.reload).to be_a(annotation) - expect(b2.reload).to be_a(annotation) - expect(b3.reload).to be_a(annotation) - expect(b4.reload).to be_a(annotation) + # Verify truncation behavior + expect(trunc_a1.reload.key_name).to eq(truncated_key_name) + expect(trunc_a2.reload.key_name).to eq(key_name_short) + + # Verify duplicate removal (keeps smallest id) + expect(annotation.where(resource_guid: i2.guid, key_name: key_c).count).to eq(2) + expect(dup_a1.reload).to be_a(annotation) + expect { dup_a2.reload }.to raise_error(Sequel::NoExistingObject) + expect { dup_a3.reload }.to raise_error(Sequel::NoExistingObject) + expect(dup_b1.reload).to be_a(annotation) + expect { dup_b2.reload }.to raise_error(Sequel::NoExistingObject) + expect { dup_b3.reload }.to raise_error(Sequel::NoExistingObject) + + # Verify preservation of records with different columns + expect(pres_a1.reload).to be_a(annotation) + expect(pres_a2.reload).to be_a(annotation) + expect(pres_a3.reload).to be_a(annotation) + expect(pres_b1.reload).to be_a(annotation) + expect(pres_b2.reload).to be_a(annotation) + expect(pres_b3.reload).to be_a(annotation) + expect(pres_b4.reload).to be_a(annotation) + + # Verify uniqueness constraints: does not allow adding a duplicate + expect { annotation.create(resource_guid: i4.guid, key_name: key_f, value: 'v2') }.to raise_error(Sequel::UniqueConstraintViolation) + expect { annotation.create(resource_guid: i5.guid, key_prefix: 'unique_prefix', key_name: key_f, value: 'v2') }.to raise_error(Sequel::UniqueConstraintViolation) + + # Verify uniqueness constraints: does allow adding different annotations + uniq_a1 = annotation.create(resource_guid: i4.guid, key_name: key_g, value: 'v3') + uniq_a2 = annotation.create(resource_guid: i5.guid, key_name: key_g, value: 'v2') + uniq_b1 = annotation.create(resource_guid: i4.guid, key_prefix: 'other_prefix', key_name: key_f, value: 'v4') + uniq_b2 = annotation.create(resource_guid: i5.guid, key_prefix: 'other_prefix', key_name: key_f, value: 'v5') + expect(annotation.where(key_name: key_g).count).to eq(2) + expect(uniq_a1.reload).to be_a(annotation) + expect(uniq_a2.reload).to be_a(annotation) + expect(uniq_b1.reload).to be_a(annotation) + expect(uniq_b2.reload).to be_a(annotation) end end describe 'labels tables' do - it 'removes duplicate annotations but keeps one with smallest id' do - i1 = isolation_segment.create(name: 'bommel') - key = 'a' * 63 + it 'handles duplicate removal and uniqueness constraints' do + # Setup data for duplicate removal test + i1 = isolation_segment.create(name: 'label_dup_test') + key_a = 'a' * 63 # In case key_prefix is not set - a1 = label.create(resource_guid: i1.guid, key_name: key, value: 'v1') - a2 = label.create(resource_guid: i1.guid, key_name: key, value: 'v2') - a3 = label.create(resource_guid: i1.guid, key_name: key, value: 'v3') - # In case key_prefix is set - b1 = label.create(resource_guid: i1.guid, key_prefix: 'bommel', key_name: key, value: 'v1') - b2 = label.create(resource_guid: i1.guid, key_prefix: 'bommel', key_name: key, value: 'v2') - b3 = label.create(resource_guid: i1.guid, key_prefix: 'bommel', key_name: key, value: 'v3') + dup_a1 = label.create(resource_guid: i1.guid, key_name: key_a, value: 'v1') + dup_a2 = label.create(resource_guid: i1.guid, key_name: key_a, value: 'v2') + dup_a3 = label.create(resource_guid: i1.guid, key_name: key_a, value: 'v3') - expect(a1.id).to be < a2.id - expect(a1.id).to be < a3.id - expect(b1.id).to be < b2.id - expect(b1.id).to be < b3.id - - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - expect(label.where(key_name: key).count).to eq(2) - expect(a1.reload).to be_a(label) - expect { a2.reload }.to raise_error(Sequel::NoExistingObject) - expect { a3.reload }.to raise_error(Sequel::NoExistingObject) - expect(b1.reload).to be_a(label) - expect { b2.reload }.to raise_error(Sequel::NoExistingObject) - expect { b3.reload }.to raise_error(Sequel::NoExistingObject) - end - - it 'does not remove records if any column of key_name, key_prefix or resource_guid is different' do - i1 = isolation_segment.create(name: 'bommel') - i2 = isolation_segment.create(name: 'sword') - key_a = 'a' * 63 + # In case key_prefix is set + dup_b1 = label.create(resource_guid: i1.guid, key_prefix: 'bommel', key_name: key_a, value: 'v1') + dup_b2 = label.create(resource_guid: i1.guid, key_prefix: 'bommel', key_name: key_a, value: 'v2') + dup_b3 = label.create(resource_guid: i1.guid, key_prefix: 'bommel', key_name: key_a, value: 'v3') + expect(dup_a1.id).to be < dup_a2.id + expect(dup_a1.id).to be < dup_a3.id + expect(dup_b1.id).to be < dup_b2.id + expect(dup_b1.id).to be < dup_b3.id + + # Setup data for preservation test (different columns) + i2 = isolation_segment.create(name: 'label_preserve_test') key_b = 'b' * 63 + key_c = 'c' * 63 # In case key_prefix is not set - a1 = label.create(resource_guid: i1.guid, key_name: key_a, value: 'v1') - a2 = label.create(resource_guid: i2.guid, key_name: key_a, value: 'v2') - a3 = label.create(resource_guid: i1.guid, key_name: key_b, value: 'v3') - # In case key_prefix is set - b1 = label.create(resource_guid: i1.guid, key_prefix: 'bommel', key_name: key_a, value: 'v1') - b2 = label.create(resource_guid: i2.guid, key_prefix: 'bommel', key_name: key_a, value: 'v2') - b3 = label.create(resource_guid: i1.guid, key_prefix: 'bommel', key_name: key_b, value: 'v3') - b4 = label.create(resource_guid: i1.guid, key_prefix: 'sword', key_name: key_a, value: 'v4') + pres_a1 = label.create(resource_guid: i1.guid, key_name: key_b, value: 'v1') + pres_a2 = label.create(resource_guid: i2.guid, key_name: key_b, value: 'v2') + pres_a3 = label.create(resource_guid: i1.guid, key_name: key_c, value: 'v3') - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - - expect(label.all.count).to eq(7) - expect(a1.reload).to be_a(label) - expect(a2.reload).to be_a(label) - expect(a3.reload).to be_a(label) - expect(b1.reload).to be_a(label) - expect(b2.reload).to be_a(label) - expect(b3.reload).to be_a(label) - expect(b4.reload).to be_a(label) - end - - it 'does not allow adding a duplicate' do - i1 = isolation_segment.create(name: 'bommel') - i2 = isolation_segment.create(name: 'sword') - key = 'a' * 63 - - # In case key_prefix is not set - label.create(resource_guid: i1.guid, key_name: key, value: 'v1') # In case key_prefix is set - label.create(resource_guid: i2.guid, key_prefix: 'bommel', key_name: key, value: 'v1') + pres_b1 = label.create(resource_guid: i1.guid, key_prefix: 'prefix1', key_name: key_b, value: 'v1') + pres_b2 = label.create(resource_guid: i2.guid, key_prefix: 'prefix1', key_name: key_b, value: 'v2') + pres_b3 = label.create(resource_guid: i1.guid, key_prefix: 'prefix1', key_name: key_c, value: 'v3') + pres_b4 = label.create(resource_guid: i1.guid, key_prefix: 'prefix2', key_name: key_b, value: 'v4') - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + # Setup data for uniqueness constraint test + i3 = isolation_segment.create(name: 'label_unique_test') + i4 = isolation_segment.create(name: 'label_unique_test2') + key_d = 'd' * 63 + key_e = 'e' * 63 - expect { label.create(resource_guid: i1.guid, key_name: key, value: 'v2') }.to raise_error(Sequel::UniqueConstraintViolation) - expect { label.create(resource_guid: i2.guid, key_prefix: 'bommel', key_name: key, value: 'v2') }.to raise_error(Sequel::UniqueConstraintViolation) - end + # In case key_prefix is not set + label.create(resource_guid: i3.guid, key_name: key_d, value: 'v1') - it 'does allow adding a different label' do - i1 = isolation_segment.create(name: 'bommel') - i2 = isolation_segment.create(name: 'sword') - key_a = 'a' * 63 - key_b = 'b' * 63 + # In case key_prefix is set + label.create(resource_guid: i4.guid, key_prefix: 'unique_prefix', key_name: key_d, value: 'v1') + # Run migration once expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - # In case key_prefix is not set - a1 = label.create(resource_guid: i1.guid, key_name: key_a, value: 'v1') - a2 = label.create(resource_guid: i2.guid, key_name: key_a, value: 'v2') - a3 = label.create(resource_guid: i1.guid, key_name: key_b, value: 'v3') - # In case key_prefix is set - b1 = label.create(resource_guid: i1.guid, key_prefix: 'bommel', key_name: key_a, value: 'v1') - b2 = label.create(resource_guid: i2.guid, key_prefix: 'bommel', key_name: key_a, value: 'v2') - b3 = label.create(resource_guid: i1.guid, key_prefix: 'bommel', key_name: key_b, value: 'v3') - b4 = label.create(resource_guid: i1.guid, key_prefix: 'sword', key_name: key_a, value: 'v4') - - expect(label.all.count).to eq(7) - expect(a1.reload).to be_a(label) - expect(a2.reload).to be_a(label) - expect(a3.reload).to be_a(label) - expect(b1.reload).to be_a(label) - expect(b2.reload).to be_a(label) - expect(b3.reload).to be_a(label) - expect(b4.reload).to be_a(label) + # Verify duplicate removal (keeps smallest id) + expect(label.where(resource_guid: i1.guid, key_name: key_a).count).to eq(2) + expect(dup_a1.reload).to be_a(label) + expect { dup_a2.reload }.to raise_error(Sequel::NoExistingObject) + expect { dup_a3.reload }.to raise_error(Sequel::NoExistingObject) + expect(dup_b1.reload).to be_a(label) + expect { dup_b2.reload }.to raise_error(Sequel::NoExistingObject) + expect { dup_b3.reload }.to raise_error(Sequel::NoExistingObject) + + # Verify preservation of records with different columns + expect(pres_a1.reload).to be_a(label) + expect(pres_a2.reload).to be_a(label) + expect(pres_a3.reload).to be_a(label) + expect(pres_b1.reload).to be_a(label) + expect(pres_b2.reload).to be_a(label) + expect(pres_b3.reload).to be_a(label) + expect(pres_b4.reload).to be_a(label) + + # Verify uniqueness constraints: does not allow adding a duplicate + expect { label.create(resource_guid: i3.guid, key_name: key_d, value: 'v2') }.to raise_error(Sequel::UniqueConstraintViolation) + expect { label.create(resource_guid: i4.guid, key_prefix: 'unique_prefix', key_name: key_d, value: 'v2') }.to raise_error(Sequel::UniqueConstraintViolation) + + # Verify uniqueness constraints: does allow adding different labels + uniq_a1 = label.create(resource_guid: i3.guid, key_name: key_e, value: 'v3') + uniq_a2 = label.create(resource_guid: i4.guid, key_name: key_e, value: 'v2') + uniq_b1 = label.create(resource_guid: i3.guid, key_prefix: 'other_prefix', key_name: key_d, value: 'v4') + uniq_b2 = label.create(resource_guid: i4.guid, key_prefix: 'other_prefix', key_name: key_d, value: 'v5') + expect(label.where(key_name: key_e).count).to eq(2) + expect(uniq_a1.reload).to be_a(label) + expect(uniq_a2.reload).to be_a(label) + expect(uniq_b1.reload).to be_a(label) + expect(uniq_b2.reload).to be_a(label) end end end diff --git a/spec/migrations/20240115163000_add_delete_cascade_to_foreign_keys_spec.rb b/spec/migrations/20240115163000_add_delete_cascade_to_foreign_keys_spec.rb index 0a0ac990d42..9e0d290847a 100644 --- a/spec/migrations/20240115163000_add_delete_cascade_to_foreign_keys_spec.rb +++ b/spec/migrations/20240115163000_add_delete_cascade_to_foreign_keys_spec.rb @@ -12,33 +12,28 @@ db[:builds].delete end - context 'before adding the foreign key' do - it 'allows inserts with a build_guid that does not exist' do - expect { db[:buildpack_lifecycle_data].insert(guid: 'bld_guid', build_guid: 'not_exists') }.not_to raise_error - end - end - - context 'after adding the foreign key' do - it 'prevents inserts with a build_guid that does not exist' do - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) + it 'adds foreign key constraint and deletes orphaned entries' do + # Before migration: allows inserts with non-existent build_guid + expect { db[:buildpack_lifecycle_data].insert(guid: 'bld_guid_test', build_guid: 'not_exists') }.not_to raise_error + db[:buildpack_lifecycle_data].delete - expect { db[:buildpack_lifecycle_data].insert(guid: 'bld_guid', build_guid: 'not_exists') }.to raise_error(Sequel::ForeignKeyConstraintViolation) - end + # Setup test data + db[:builds].insert(guid: 'build_guid') + db[:buildpack_lifecycle_data].insert(guid: 'bld_guid', build_guid: 'build_guid') + db[:buildpack_lifecycle_buildpacks].insert(guid: 'blb_guid', buildpack_lifecycle_data_guid: 'bld_guid') + db[:buildpack_lifecycle_data].insert(guid: 'another_bld_guid', build_guid: 'not_exists') + db[:buildpack_lifecycle_buildpacks].insert(guid: 'another_blb_guid', buildpack_lifecycle_data_guid: 'another_bld_guid') - it 'deleted orphaned buildpack_lifecycle_data entries but kept valid ones' do - db[:builds].insert(guid: 'build_guid') - db[:buildpack_lifecycle_data].insert(guid: 'bld_guid', build_guid: 'build_guid') - db[:buildpack_lifecycle_buildpacks].insert(guid: 'blb_guid', buildpack_lifecycle_data_guid: 'bld_guid') - db[:buildpack_lifecycle_data].insert(guid: 'another_bld_guid', build_guid: 'not_exists') - db[:buildpack_lifecycle_buildpacks].insert(guid: 'another_blb_guid', buildpack_lifecycle_data_guid: 'another_bld_guid') + Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) + # After migration: prevents inserts with non-existent build_guid + expect { db[:buildpack_lifecycle_data].insert(guid: 'bld_guid_new', build_guid: 'not_exists') }.to raise_error(Sequel::ForeignKeyConstraintViolation) - expect(db[:buildpack_lifecycle_data].where(guid: 'bld_guid').count).to eq(1) - expect(db[:buildpack_lifecycle_buildpacks].where(guid: 'blb_guid').count).to eq(1) - expect(db[:buildpack_lifecycle_data].where(guid: 'another_bld_guid').count).to eq(0) - expect(db[:buildpack_lifecycle_buildpacks].where(guid: 'another_blb_guid').count).to eq(0) - end + # After migration: orphaned entries deleted, valid ones kept + expect(db[:buildpack_lifecycle_data].where(guid: 'bld_guid').count).to eq(1) + expect(db[:buildpack_lifecycle_buildpacks].where(guid: 'blb_guid').count).to eq(1) + expect(db[:buildpack_lifecycle_data].where(guid: 'another_bld_guid').count).to eq(0) + expect(db[:buildpack_lifecycle_buildpacks].where(guid: 'another_blb_guid').count).to eq(0) end end end diff --git a/spec/migrations/20240314131908_add_user_guid_to_jobs_table_spec.rb b/spec/migrations/20240314131908_add_user_guid_to_jobs_table_spec.rb index 52800f06c55..7fc29acbacd 100644 --- a/spec/migrations/20240314131908_add_user_guid_to_jobs_table_spec.rb +++ b/spec/migrations/20240314131908_add_user_guid_to_jobs_table_spec.rb @@ -7,84 +7,30 @@ end describe 'jobs table' do - it 'adds a column `user_guid`' do + it 'adds column and index, and handles idempotency gracefully' do + # Test basic up migration expect(db[:jobs].columns).not_to include(:user_guid) + expect(db.indexes(:jobs)).not_to include(:jobs_user_guid_index) + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error expect(db[:jobs].columns).to include(:user_guid) - end + expect(db.indexes(:jobs)).to include(:jobs_user_guid_index) - it 'adds an index on the user_guid column' do - expect(db.indexes(:jobs)).not_to include(:jobs_user_guid_index) + # Test up migration again (both column and index already exist - idempotency) expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + expect(db[:jobs].columns).to include(:user_guid) expect(db.indexes(:jobs)).to include(:jobs_user_guid_index) - end - - describe 'idempotency of up' do - context '`user_guid` column already exists' do - before do - db.add_column :jobs, :user_guid, String, size: 255 - end - - it 'does not fail' do - expect(db[:jobs].columns).to include(:user_guid) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - end - - it 'continues to create the index' do - expect(db[:jobs].columns).to include(:user_guid) - expect(db.indexes(:jobs)).not_to include(:jobs_user_guid_index) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(:jobs)).to include(:jobs_user_guid_index) - end - end - context 'index already exists' do - before do - db.add_column :jobs, :user_guid, String, size: 255 - db.add_index :jobs, :user_guid, name: :jobs_user_guid_index - end - - it 'does not fail' do - expect(db.indexes(:jobs)).to include(:jobs_user_guid_index) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - end - end - end - - describe 'idempotency of down' do - context 'index does not exist' do - before do - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - db.drop_index :jobs, :user_guid, name: :jobs_user_guid_index - end - - it 'does not fail' do - expect(db[:jobs].columns).to include(:user_guid) - expect(db.indexes(:jobs)).not_to include(:jobs_user_guid_index) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - end - - it 'continues to remove the `user_guid` column' do - expect(db[:jobs].columns).to include(:user_guid) - expect(db.indexes(:jobs)).not_to include(:jobs_user_guid_index) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - expect(db[:jobs].columns).not_to include(:user_guid) - end - end - - context 'index and column do not exist' do - before do - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - db.drop_index :jobs, :user_guid, name: :jobs_user_guid_index - db.drop_column :jobs, :user_guid - end + # Test down migration with pre-dropped index + db.drop_index :jobs, :user_guid, name: :jobs_user_guid_index + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db[:jobs].columns).not_to include(:user_guid) + expect(db.indexes(:jobs)).not_to include(:jobs_user_guid_index) - it 'does not fail' do - expect(db[:jobs].columns).not_to include(:user_guid) - expect(db.indexes(:jobs)).not_to include(:jobs_user_guid_index) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - end - end + # Test down migration when both already removed (idempotency) + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db[:jobs].columns).not_to include(:user_guid) + expect(db.indexes(:jobs)).not_to include(:jobs_user_guid_index) end end end diff --git a/spec/migrations/20240529195136_add_task_guid_index_to_app_usage_events_spec.rb b/spec/migrations/20240529195136_add_task_guid_index_to_app_usage_events_spec.rb index 29f484a1532..48b1513cd1f 100644 --- a/spec/migrations/20240529195136_add_task_guid_index_to_app_usage_events_spec.rb +++ b/spec/migrations/20240529195136_add_task_guid_index_to_app_usage_events_spec.rb @@ -7,53 +7,23 @@ end describe 'app_usage_events table' do - describe 'up migration' do - context 'index does not exist' do - it 'adds an index on the task_guid column' do - expect(db.indexes(:app_usage_events)).not_to include(:app_usage_events_task_guid_index) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(:app_usage_events)).to include(:app_usage_events_task_guid_index) - end - end - - context 'index already exists' do - before do - db.add_index :app_usage_events, :task_guid, name: :app_usage_events_task_guid_index - end - - it 'does not fail' do - expect(db.indexes(:app_usage_events)).to include(:app_usage_events_task_guid_index) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(:app_usage_events)).to include(:app_usage_events_task_guid_index) - end - end - end - - describe 'down migration' do - context 'index does not exist' do - before do - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - db.drop_index :app_usage_events, :task_guid, name: :app_usage_events_task_guid_index - end - - it 'does not fail' do - expect(db.indexes(:app_usage_events)).not_to include(:app_usage_events_task_guid_index) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(:app_usage_events)).not_to include(:app_usage_events_task_guid_index) - end - end - - context 'index does exist' do - before do - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - end - - it 'removes the index' do - expect(db.indexes(:app_usage_events)).to include(:app_usage_events_task_guid_index) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(:app_usage_events)).not_to include(:app_usage_events_task_guid_index) - end - end + it 'adds and removes index with idempotency' do + # Test up migration + expect(db.indexes(:app_usage_events)).not_to include(:app_usage_events_task_guid_index) + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:app_usage_events)).to include(:app_usage_events_task_guid_index) + + # Test up migration idempotency: running again when index exists should not fail + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:app_usage_events)).to include(:app_usage_events_task_guid_index) + + # Test down migration + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:app_usage_events)).not_to include(:app_usage_events_task_guid_index) + + # Test down migration idempotency: running rollback again when index doesn't exist should not fail + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:app_usage_events)).not_to include(:app_usage_events_task_guid_index) end end end diff --git a/spec/migrations/20240820070742_add_jobs_user_guid_state_index_spec.rb b/spec/migrations/20240820070742_add_jobs_user_guid_state_index_spec.rb index 13b4a929d2a..f8601a9ef64 100644 --- a/spec/migrations/20240820070742_add_jobs_user_guid_state_index_spec.rb +++ b/spec/migrations/20240820070742_add_jobs_user_guid_state_index_spec.rb @@ -19,81 +19,32 @@ def partial_index_present end describe 'jobs table' do - it 'removes index `jobs_user_guid_index`' do + it 'replaces indexes and handles idempotency gracefully' do skip if db.database_type != :postgres + + # Test basic up migration expect(db.indexes(:jobs)).to include(:jobs_user_guid_index) + expect(partial_index_present).to be_falsey + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:jobs)).not_to include(:jobs_user_guid_index) - end + expect(partial_index_present).to be_truthy - it 'adds an index `jobs_user_guid_state_index`' do - skip if db.database_type != :postgres - expect(partial_index_present).to be_falsey + # Test up migration idempotency: running again when new index exists should not fail expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:jobs)).not_to include(:jobs_user_guid_index) expect(partial_index_present).to be_truthy - end - - describe 'idempotency of up' do - context '`jobs_user_guid_index` does not exist' do - before do - skip if db.database_type != :postgres - db.drop_index :jobs, :user_guid, name: :jobs_user_guid_index - end - it 'continues to create the index' do - expect(db.indexes(:jobs)).not_to include(:jobs_user_guid_index) - expect(partial_index_present).to be_falsey - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - expect(partial_index_present).to be_truthy - expect(db.indexes(:jobs)).not_to include(:jobs_user_guid_index) - end - end - - context '`jobs_user_guid_state_index` already exists' do - before do - skip if db.database_type != :postgres - db.add_index :jobs, %i[user_guid state], name: :jobs_user_guid_state_index, where: "state IN ('PROCESSING', 'POLLING')" - end - - it 'does not fail' do - expect(partial_index_present).to be_truthy - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - end - end - end - - describe 'idempotency of down' do - context '`jobs_user_guid_state_index` does not exist' do - before do - skip if db.database_type != :postgres - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - db.drop_index :jobs, %i[user_guid state], name: :jobs_user_guid_state_index - end - - it 'restores `jobs_user_guid_index`' do - expect(partial_index_present).to be_falsey - expect(db.indexes(:jobs)).not_to include(:jobs_user_guid_index) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - expect(partial_index_present).to be_falsey - expect(db.indexes(:jobs)).to include(:jobs_user_guid_index) - end - end - - context '`jobs_user_guid_index` already exists' do - before do - skip if db.database_type != :postgres - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - db.add_index :jobs, :user_guid, name: :jobs_user_guid_index - end + # Test down migration + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(partial_index_present).to be_falsey + expect(db.indexes(:jobs)).to include(:jobs_user_guid_index) - it 'does not fail' do - expect(db.indexes(:jobs)).to include(:jobs_user_guid_index) - expect(partial_index_present).to be_truthy - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - expect(partial_index_present).to be_falsey - expect(db.indexes(:jobs)).to include(:jobs_user_guid_index) - end - end + # Test down migration idempotency: running again when old index exists should not fail + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(partial_index_present).to be_falsey + expect(db.indexes(:jobs)).to include(:jobs_user_guid_index) end end end diff --git a/spec/migrations/20240904132000_add_foreign_key_apps_droplet_guid_spec.rb b/spec/migrations/20240904132000_add_foreign_key_apps_droplet_guid_spec.rb index dcaa4483b22..1d2940b8673 100644 --- a/spec/migrations/20240904132000_add_foreign_key_apps_droplet_guid_spec.rb +++ b/spec/migrations/20240904132000_add_foreign_key_apps_droplet_guid_spec.rb @@ -11,29 +11,24 @@ db[:apps].delete end - context 'before adding the foreign key' do - it 'allows inserts with a droplet_guid that does not exist' do - expect { db[:apps].insert(guid: 'app_guid', droplet_guid: 'not_exists') }.not_to raise_error - end - end - - context 'after adding the foreign key' do - it 'prevents inserts with a droplet_guid that does not exist' do - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) + it 'adds foreign key constraint and removes invalid references' do + # Before migration: allows inserts with non-existent droplet_guid + expect { db[:apps].insert(guid: 'app_guid_test', droplet_guid: 'not_exists') }.not_to raise_error + db[:apps].delete - expect { db[:apps].insert(guid: 'app_guid', droplet_guid: 'not_exists') }.to raise_error(Sequel::ForeignKeyConstraintViolation) - end + # Setup test data + db[:droplets].insert(guid: 'droplet_guid', state: 'some_state') + db[:apps].insert(guid: 'app_guid', droplet_guid: 'droplet_guid') + db[:apps].insert(guid: 'another_app_guid', droplet_guid: 'not_exists') - it 'removed references to not existing droplets' do - db[:droplets].insert(guid: 'droplet_guid', state: 'some_state') - db[:apps].insert(guid: 'app_guid', droplet_guid: 'droplet_guid') - db[:apps].insert(guid: 'another_app_guid', droplet_guid: 'not_exists') + Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) + # After migration: prevents inserts with non-existent droplet_guid + expect { db[:apps].insert(guid: 'app_guid_new', droplet_guid: 'not_exists') }.to raise_error(Sequel::ForeignKeyConstraintViolation) - expect(db[:apps].where(guid: 'app_guid').get(:droplet_guid)).to eq('droplet_guid') - expect(db[:apps].where(guid: 'another_app_guid').get(:droplet_guid)).to be_nil - end + # After migration: invalid references removed, valid ones kept + expect(db[:apps].where(guid: 'app_guid').get(:droplet_guid)).to eq('droplet_guid') + expect(db[:apps].where(guid: 'another_app_guid').get(:droplet_guid)).to be_nil end end end diff --git a/spec/migrations/20241016118000_drop_unique_constraint_quota_definitions_name_key_spec.rb b/spec/migrations/20241016118000_drop_unique_constraint_quota_definitions_name_key_spec.rb index be08fa3a352..62bb7ff1d02 100644 --- a/spec/migrations/20241016118000_drop_unique_constraint_quota_definitions_name_key_spec.rb +++ b/spec/migrations/20241016118000_drop_unique_constraint_quota_definitions_name_key_spec.rb @@ -3,119 +3,53 @@ RSpec.describe 'migration to add or remove unique constraint on name column in quota_definitions table', isolation: :truncation, type: :migration do include_context 'migration' do - let(:migration_filename) { '20241016118000_drop_unique_constraint_quota_definitions_name_key_spec.rb' } + let(:migration_filename) { '20241016118000_drop_unique_constraint_quota_definitions_name_key.rb' } end - describe 'up migration' do + describe 'quota_definitions table' do context 'mysql' do - before do + it 'removes and restores unique constraint with idempotency' do skip if db.database_type != :mysql - end - it 'removes the unique constraint' do + # Test up migration - removes unique constraint expect(db.indexes(:quota_definitions)).to include(:name) expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error expect(db.indexes(:quota_definitions)).not_to include(:name) - end - context 'unique constraint on name column does not exist' do - before do - db.drop_index :quota_definitions, :name, name: :name - end + # Test up migration idempotency - constraint already removed + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:quota_definitions)).not_to include(:name) - it 'does not throw an error' do - expect(db.indexes(:quota_definitions)).not_to include(:name) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(:quota_definitions)).not_to include(:name) - end + # Test down migration - restores unique constraint + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:quota_definitions)).to include(:name) + + # Test down migration idempotency - constraint already exists + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:quota_definitions)).to include(:name) end end context 'postgres' do - before do + it 'removes and restores unique constraint with idempotency' do skip if db.database_type != :postgres - end - it 'removes the unique constraint' do + # Test up migration - removes unique constraint expect(db.indexes(:quota_definitions)).to include(:quota_definitions_name_key) expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error expect(db.indexes(:quota_definitions)).not_to include(:quota_definitions_name_key) - end - context 'unique constraint on name column does not exist' do - before do - db.alter_table :quota_definitions do - drop_constraint :quota_definitions_name_key - end - end - - it 'does not throw an error' do - expect(db.indexes(:quota_definitions)).not_to include(:quota_definitions_name_key) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(:quota_definitions)).not_to include(:quota_definitions_name_key) - end - end - end - end - - describe 'down migration' do - before do - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - end - - context 'mysql' do - before do - skip if db.database_type != :mysql - end + # Test up migration idempotency - constraint already removed + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:quota_definitions)).not_to include(:quota_definitions_name_key) - it 'adds the unique constraint' do - expect(db.indexes(:quota_definitions)).not_to include(:name) + # Test down migration - restores unique constraint expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(:quota_definitions)).to include(:name) - end - - context 'unique constraint on name column already exists' do - before do - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - - db.alter_table :quota_definitions do - add_index :name, name: :name - end - end - - it 'does not fail' do - expect(db.indexes(:quota_definitions)).to include(:name) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(:quota_definitions)).to include(:name) - end - end - end - - context 'postgres' do - before do - skip if db.database_type != :postgres - end + expect(db.indexes(:quota_definitions)).to include(:quota_definitions_name_key) - it 'adds the unique constraint' do - expect(db.indexes(:quota_definitions)).not_to include(:quota_definitions_name_key) + # Test down migration idempotency - constraint already exists expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error expect(db.indexes(:quota_definitions)).to include(:quota_definitions_name_key) end - - context 'unique constraint on name column already exists' do - before do - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - - db.alter_table :quota_definitions do - add_unique_constraint :name, name: :quota_definitions_name_key - end - end - - it 'does not fail' do - expect(db.indexes(:quota_definitions)).to include(:quota_definitions_name_key) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(:quota_definitions)).to include(:quota_definitions_name_key) - end - end end end end diff --git a/spec/migrations/20241203085500_add_apps_file_based_service_bindings_enabled_column_spec.rb b/spec/migrations/20241203085500_add_apps_file_based_service_bindings_enabled_column_spec.rb index 184bb16bb90..8d48828fe0f 100644 --- a/spec/migrations/20241203085500_add_apps_file_based_service_bindings_enabled_column_spec.rb +++ b/spec/migrations/20241203085500_add_apps_file_based_service_bindings_enabled_column_spec.rb @@ -7,28 +7,27 @@ end describe 'apps table' do - subject(:run_migration) { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) } + it 'adds column with correct properties and default values' do + # Insert an app before migration to test default value on existing entries + db[:apps].insert(guid: 'existing_app_guid') - it 'adds a column `file_based_service_bindings_enabled`' do + # Verify column doesn't exist yet expect(db[:apps].columns).not_to include(:file_based_service_bindings_enabled) - run_migration + + # Run migration + Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) + + # Verify column was added expect(db[:apps].columns).to include(:file_based_service_bindings_enabled) - end - it 'sets the default value of existing entries to false' do - db[:apps].insert(guid: 'existing_app_guid') - run_migration + # Verify default value on existing entries is false expect(db[:apps].first(guid: 'existing_app_guid')[:file_based_service_bindings_enabled]).to be(false) - end - it 'sets the default value of new entries to false' do - run_migration + # Verify default value on new entries is false db[:apps].insert(guid: 'new_app_guid') expect(db[:apps].first(guid: 'new_app_guid')[:file_based_service_bindings_enabled]).to be(false) - end - it 'forbids null values' do - run_migration + # Verify null values are forbidden expect { db[:apps].insert(guid: 'app_guid__nil', file_based_service_bindings_enabled: nil) }.to raise_error(Sequel::NotNullConstraintViolation) end end diff --git a/spec/migrations/20250116144231_remove_unnecessary_fk_in_job_warnings_spec.rb b/spec/migrations/20250116144231_remove_unnecessary_fk_in_job_warnings_spec.rb index d3dd97ef9d6..c954f0cb703 100644 --- a/spec/migrations/20250116144231_remove_unnecessary_fk_in_job_warnings_spec.rb +++ b/spec/migrations/20250116144231_remove_unnecessary_fk_in_job_warnings_spec.rb @@ -7,25 +7,15 @@ end describe 'job_warnings table' do - it 'removes the fk constraint as well as the column' do + it 'removes the fk constraint and column, handles idempotency' do + # Run migration Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) expect(db.foreign_key_list(:job_warnings)).to be_empty expect(db[:job_warnings].columns).not_to include(:fk_jobs_id) - end - - context 'foreign key constraint does not exist' do - before do - unless db.foreign_key_list(:job_warnings).empty? - db.alter_table(:job_warnings) do - drop_foreign_key :fk_jobs_id - end - end - end - it 'does not fail' do - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - end + # Test idempotency: running again when constraint doesn't exist should not fail + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error end end end diff --git a/spec/migrations/20250225132929_add_apps_file_based_service_binding_feature_columns_spec.rb b/spec/migrations/20250225132929_add_apps_file_based_service_binding_feature_columns_spec.rb index 3a897e77723..436a459aa72 100644 --- a/spec/migrations/20250225132929_add_apps_file_based_service_binding_feature_columns_spec.rb +++ b/spec/migrations/20250225132929_add_apps_file_based_service_binding_feature_columns_spec.rb @@ -7,235 +7,62 @@ end describe 'apps table' do - subject(:run_migration) { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) } - - describe 'up' do - describe 'column service_binding_k8s_enabled' do - it 'adds a column `service_binding_k8s_enabled`' do - expect(db[:apps].columns).not_to include(:service_binding_k8s_enabled) - run_migration - expect(db[:apps].columns).to include(:service_binding_k8s_enabled) - end - - it 'sets the default value of existing entries to false' do - db[:apps].insert(guid: 'existing_app_guid') - run_migration - expect(db[:apps].first(guid: 'existing_app_guid')[:service_binding_k8s_enabled]).to be(false) - end - - it 'sets the default value of new entries to false' do - run_migration - db[:apps].insert(guid: 'new_app_guid') - expect(db[:apps].first(guid: 'new_app_guid')[:service_binding_k8s_enabled]).to be(false) - end - - it 'forbids null values' do - run_migration - expect { db[:apps].insert(guid: 'app_guid__nil', service_binding_k8s_enabled: nil) }.to raise_error(Sequel::NotNullConstraintViolation) - end - - context 'when it already exists' do - before do - db.add_column :apps, :service_binding_k8s_enabled, :boolean, default: false, null: false, if_not_exists: true - end - - it 'does not fail' do - expect(db[:apps].columns).to include(:service_binding_k8s_enabled) - expect { run_migration }.not_to raise_error - expect(db[:apps].columns).to include(:service_binding_k8s_enabled) - expect(db[:apps].columns).to include(:file_based_vcap_services_enabled) - expect(check_constraint_exists?(db)).to be(true) if check_constraint_supported?(db) - end - end - end - - describe 'column file_based_vcap_services_enabled' do - it 'adds a column `file_based_vcap_services_enabled`' do - expect(db[:apps].columns).not_to include(:file_based_vcap_services_enabled) - run_migration - expect(db[:apps].columns).to include(:file_based_vcap_services_enabled) - end - - it 'sets the default value of existing entries to false' do - db[:apps].insert(guid: 'existing_app_guid') - run_migration - expect(db[:apps].first(guid: 'existing_app_guid')[:file_based_vcap_services_enabled]).to be(false) - end - - it 'sets the default value of new entries to false' do - run_migration - db[:apps].insert(guid: 'new_app_guid') - expect(db[:apps].first(guid: 'new_app_guid')[:file_based_vcap_services_enabled]).to be(false) - end - - it 'forbids null values' do - run_migration - expect { db[:apps].insert(guid: 'app_guid__nil', file_based_vcap_services_enabled: nil) }.to raise_error(Sequel::NotNullConstraintViolation) - end - - context 'when it already exists' do - before do - db.add_column :apps, :file_based_vcap_services_enabled, :boolean, default: false, null: false, if_not_exists: true - end - - it 'does not fail' do - expect(db[:apps].columns).to include(:file_based_vcap_services_enabled) - expect { run_migration }.not_to raise_error - expect(db[:apps].columns).to include(:service_binding_k8s_enabled) - expect(db[:apps].columns).to include(:file_based_vcap_services_enabled) - expect(check_constraint_exists?(db)).to be(true) if check_constraint_supported?(db) - end - end + it 'adds and removes columns with defaults, constraints, and handles idempotency' do + # Setup: Insert existing app to test default values + db[:apps].insert(guid: 'existing_app_guid') + + # Verify initial state + expect(db[:apps].columns).not_to include(:service_binding_k8s_enabled) + expect(db[:apps].columns).not_to include(:file_based_vcap_services_enabled) + expect(check_constraint_exists?(db)).to be(false) if check_constraint_supported?(db) + + # === UP MIGRATION === + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + + # Verify both columns were added + expect(db[:apps].columns).to include(:service_binding_k8s_enabled) + expect(db[:apps].columns).to include(:file_based_vcap_services_enabled) + + # Verify default values for existing entries + expect(db[:apps].first(guid: 'existing_app_guid')[:service_binding_k8s_enabled]).to be(false) + expect(db[:apps].first(guid: 'existing_app_guid')[:file_based_vcap_services_enabled]).to be(false) + + # Verify default values for new entries + db[:apps].insert(guid: 'new_app_guid') + expect(db[:apps].first(guid: 'new_app_guid')[:service_binding_k8s_enabled]).to be(false) + expect(db[:apps].first(guid: 'new_app_guid')[:file_based_vcap_services_enabled]).to be(false) + + # Verify null constraints + expect { db[:apps].insert(guid: 'app_guid_nil_k8s', service_binding_k8s_enabled: nil) }.to raise_error(Sequel::NotNullConstraintViolation) + expect { db[:apps].insert(guid: 'app_guid_nil_vcap', file_based_vcap_services_enabled: nil) }.to raise_error(Sequel::NotNullConstraintViolation) + + # Verify check constraint (if supported) + if check_constraint_supported?(db) + expect(check_constraint_exists?(db)).to be(true) + expect { db[:apps].insert(guid: 'app_both_true', file_based_vcap_services_enabled: true, service_binding_k8s_enabled: true) }.to(raise_error do |error| + expect(error.inspect).to include('only_one_sb_feature_enabled') + end) end - describe 'check constraint' do - context 'when supported' do - before do - skip 'check constraint not supported by db' unless check_constraint_supported?(db) - end - - it 'adds the check constraint' do - expect(check_constraint_exists?(db)).to be(false) - run_migration - expect(check_constraint_exists?(db)).to be(true) - end - - it 'forbids setting both features to true' do - run_migration - expect { db[:apps].insert(guid: 'some_app', file_based_vcap_services_enabled: true, service_binding_k8s_enabled: true) }.to(raise_error do |error| - expect(error.inspect).to include('only_one_sb_feature_enabled') - end) - end - - context 'when it already exists' do - before do - db.add_column :apps, :service_binding_k8s_enabled, :boolean, default: false, null: false, if_not_exists: true - db.add_column :apps, :file_based_vcap_services_enabled, :boolean, default: false, null: false, if_not_exists: true - db.alter_table :apps do - add_constraint(name: :only_one_sb_feature_enabled) do - Sequel.lit('NOT (service_binding_k8s_enabled AND file_based_vcap_services_enabled)') - end - end - end - - it 'does not fail' do - expect { run_migration }.not_to raise_error - end - end - end - - context 'when not supported' do - before do - skip 'check constraint supported by db' if check_constraint_supported?(db) - end - - it 'does not fail' do - expect { run_migration }.not_to raise_error - expect(db[:apps].columns).to include(:service_binding_k8s_enabled) - expect(db[:apps].columns).to include(:file_based_vcap_services_enabled) - end - end - end - end - - describe 'down' do - subject(:run_rollback) { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) } - - before do - run_migration - end - - describe 'column service_binding_k8s_enabled' do - it 'removes column `service_binding_k8s_enabled`' do - expect(db[:apps].columns).to include(:service_binding_k8s_enabled) - run_rollback - expect(db[:apps].columns).not_to include(:service_binding_k8s_enabled) - end - - context 'when it does not exist' do - before do - db.alter_table :apps do - drop_constraint :only_one_sb_feature_enabled - drop_column :service_binding_k8s_enabled - end - end - - it 'does not fail' do - expect(db[:apps].columns).not_to include(:service_binding_k8s_enabled) - expect { run_rollback }.not_to raise_error - expect(db[:apps].columns).not_to include(:service_binding_k8s_enabled) - expect(db[:apps].columns).not_to include(:file_based_vcap_services_enabled) - expect(check_constraint_exists?(db)).to be(false) - end - end - end - - describe 'column file_based_vcap_services_enabled' do - it 'removes column `file_based_vcap_services_enabled`' do - expect(db[:apps].columns).to include(:file_based_vcap_services_enabled) - run_rollback - expect(db[:apps].columns).not_to include(:file_based_vcap_services_enabled) - end - - context 'when it does not exist' do - before do - db.alter_table :apps do - drop_constraint :only_one_sb_feature_enabled - drop_column :file_based_vcap_services_enabled - end - end - - it 'does not fail' do - expect(db[:apps].columns).not_to include(:file_based_vcap_services_enabled) - expect { run_rollback }.not_to raise_error - expect(db[:apps].columns).not_to include(:service_binding_k8s_enabled) - expect(db[:apps].columns).not_to include(:file_based_vcap_services_enabled) - expect(check_constraint_exists?(db)).to be(false) - end - end - end - - describe 'check constraint' do - context 'when supported' do - before do - skip 'check constraint not supported by db' unless check_constraint_supported?(db) - end - - it 'removes the check constraint' do - expect(check_constraint_exists?(db)).to be(true) - run_rollback - expect(check_constraint_exists?(db)).to be(false) - end - - context 'when it does not exist' do - before do - db.alter_table :apps do - drop_constraint :only_one_sb_feature_enabled - end - end - - it 'does not fail' do - expect(check_constraint_exists?(db)).to be(false) - expect { run_rollback }.not_to raise_error - expect(db[:apps].columns).not_to include(:service_binding_k8s_enabled) - expect(db[:apps].columns).not_to include(:file_based_vcap_services_enabled) - expect(check_constraint_exists?(db)).to be(false) - end - end - end - - context 'when not supported' do - before do - skip 'check constraint supported by db' if check_constraint_supported?(db) - end - - it 'does not fail' do - expect { run_rollback }.not_to raise_error - expect(db[:apps].columns).not_to include(:service_binding_k8s_enabled) - expect(db[:apps].columns).not_to include(:file_based_vcap_services_enabled) - end - end - end + # Test up migration idempotency: running migration again should not fail + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + expect(db[:apps].columns).to include(:service_binding_k8s_enabled) + expect(db[:apps].columns).to include(:file_based_vcap_services_enabled) + expect(check_constraint_exists?(db)).to be(true) if check_constraint_supported?(db) + + # === DOWN MIGRATION === + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + + # Verify columns were removed + expect(db[:apps].columns).not_to include(:service_binding_k8s_enabled) + expect(db[:apps].columns).not_to include(:file_based_vcap_services_enabled) + expect(check_constraint_exists?(db)).to be(false) + + # Test down migration idempotency: running rollback again should not fail + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db[:apps].columns).not_to include(:service_binding_k8s_enabled) + expect(db[:apps].columns).not_to include(:file_based_vcap_services_enabled) + expect(check_constraint_exists?(db)).to be(false) end end end diff --git a/spec/migrations/20250318112800_add_user_id_index_to_roles_tables_spec.rb b/spec/migrations/20250318112800_add_user_id_index_to_roles_tables_spec.rb index 5e66ac7a0aa..87e7c9c1da5 100644 --- a/spec/migrations/20250318112800_add_user_id_index_to_roles_tables_spec.rb +++ b/spec/migrations/20250318112800_add_user_id_index_to_roles_tables_spec.rb @@ -1,76 +1,72 @@ require 'spec_helper' require 'migrations/helpers/migration_shared_context' -RSpec.shared_examples 'adding an index for table' do |table| - describe "#{table} table" do - let(:table_sym) { table.to_sym } - let(:index_sym) { :"#{table}_user_id_index" } +RSpec.describe 'migration to add an index for user_id on all roles tables', isolation: :truncation, type: :migration do + include_context 'migration' do + let(:migration_filename) { '20250318112800_add_user_id_index_to_roles_tables.rb' } + end - before do - skip unless db.database_type == :postgres - end + let(:tables) do + %w[ + organizations_auditors + organizations_billing_managers + organizations_managers + organizations_users + spaces_auditors + spaces_developers + spaces_managers + spaces_supporters + ] + end + + before do + skip unless db.database_type == :postgres + end - describe 'up migration' do - context 'index does not exist' do - it 'adds the index' do - expect(db.indexes(table_sym)).not_to include(index_sym) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(table_sym)).to include(index_sym) - end + describe 'roles tables' do + it 'adds and removes user_id indexes for all tables with idempotency' do + # Verify initial state: no indexes exist + tables.each do |table| + table_sym = table.to_sym + index_sym = :"#{table}_user_id_index" + expect(db.indexes(table_sym)).not_to include(index_sym) end - context 'index already exists' do - before do - db.add_index table_sym, :user_id, name: index_sym - end + # === UP MIGRATION === + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - it 'does not fail' do - expect(db.indexes(table_sym)).to include(index_sym) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(table_sym)).to include(index_sym) - end + # Verify all indexes were created + tables.each do |table| + table_sym = table.to_sym + index_sym = :"#{table}_user_id_index" + expect(db.indexes(table_sym)).to include(index_sym) end - end - describe 'down migration' do - before do - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) + # Test up migration idempotency + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + tables.each do |table| + table_sym = table.to_sym + index_sym = :"#{table}_user_id_index" + expect(db.indexes(table_sym)).to include(index_sym) end - context 'index exists' do - it 'removes the index' do - expect(db.indexes(table_sym)).to include(index_sym) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(table_sym)).not_to include(index_sym) - end - end + # === DOWN MIGRATION === + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - context 'index does not exist' do - before do - db.drop_index table_sym, :user_id, name: index_sym - end + # Verify all indexes were removed + tables.each do |table| + table_sym = table.to_sym + index_sym = :"#{table}_user_id_index" + expect(db.indexes(table_sym)).not_to include(index_sym) + end - it 'does not fail' do - expect(db.indexes(table_sym)).not_to include(index_sym) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(table_sym)).not_to include(index_sym) - end + # Test down migration idempotency + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + tables.each do |table| + table_sym = table.to_sym + index_sym = :"#{table}_user_id_index" + expect(db.indexes(table_sym)).not_to include(index_sym) end end end end - -RSpec.describe 'migration to add an index for user_id on all roles tables', isolation: :truncation, type: :migration do - include_context 'migration' do - let(:migration_filename) { '20250318112800_add_user_id_index_to_roles_tables.rb' } - end - - include_examples 'adding an index for table', 'organizations_auditors' - include_examples 'adding an index for table', 'organizations_billing_managers' - include_examples 'adding an index for table', 'organizations_managers' - include_examples 'adding an index for table', 'organizations_users' - include_examples 'adding an index for table', 'spaces_auditors' - include_examples 'adding an index for table', 'spaces_developers' - include_examples 'adding an index for table', 'spaces_managers' - include_examples 'adding an index for table', 'spaces_supporters' -end diff --git a/spec/migrations/20250610212414_add_user_to_processes_spec.rb b/spec/migrations/20250610212414_add_user_to_processes_spec.rb index 37deae8fcc6..6b859a26139 100644 --- a/spec/migrations/20250610212414_add_user_to_processes_spec.rb +++ b/spec/migrations/20250610212414_add_user_to_processes_spec.rb @@ -7,49 +7,25 @@ end describe 'processes table' do - it 'adds a column `user`' do + it 'adds and removes user column with idempotency' do + # Verify initial state expect(db[:processes].columns).not_to include(:user) + + # === UP MIGRATION === expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error expect(db[:processes].columns).to include(:user) - end - - describe 'idempotency of up' do - context '`user` column already exists' do - before do - db.add_column :processes, :user, String, size: 255, if_not_exists: true - end - - it 'does not fail' do - expect(db[:processes].columns).to include(:user) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - end - end - end - describe 'idempotency of down' do - context 'user column exists' do - before do - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - end - - it 'continues to remove the `user_guid` column' do - expect(db[:processes].columns).to include(:user) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - expect(db[:processes].columns).not_to include(:user) - end - end + # Test up migration idempotency + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + expect(db[:processes].columns).to include(:user) - context 'column does not exist' do - before do - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - db.drop_column :processes, :user - end + # === DOWN MIGRATION === + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db[:processes].columns).not_to include(:user) - it 'does not fail' do - expect(db[:processes].columns).not_to include(:user) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - end - end + # Test down migration idempotency + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db[:processes].columns).not_to include(:user) end end end diff --git a/spec/migrations/20250630170610_add_user_to_tasks_spec.rb b/spec/migrations/20250630170610_add_user_to_tasks_spec.rb index 6d52d8bfca2..015487f6a7b 100644 --- a/spec/migrations/20250630170610_add_user_to_tasks_spec.rb +++ b/spec/migrations/20250630170610_add_user_to_tasks_spec.rb @@ -7,49 +7,25 @@ end describe 'tasks table' do - it 'adds a column `user`' do + it 'adds and removes user column with idempotency' do + # Verify initial state expect(db[:tasks].columns).not_to include(:user) + + # === UP MIGRATION === expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error expect(db[:tasks].columns).to include(:user) - end - - describe 'idempotency of up' do - context '`user` column already exists' do - before do - db.add_column :tasks, :user, String, size: 255, if_not_exists: true - end - - it 'does not fail' do - expect(db[:tasks].columns).to include(:user) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - end - end - end - describe 'idempotency of down' do - context 'user column exists' do - before do - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - end - - it 'continues to remove the `user_guid` column' do - expect(db[:tasks].columns).to include(:user) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - expect(db[:tasks].columns).not_to include(:user) - end - end + # Test up migration idempotency + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + expect(db[:tasks].columns).to include(:user) - context 'column does not exist' do - before do - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - db.drop_column :tasks, :user - end + # === DOWN MIGRATION === + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db[:tasks].columns).not_to include(:user) - it 'does not fail' do - expect(db[:tasks].columns).not_to include(:user) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - end - end + # Test down migration idempotency + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db[:tasks].columns).not_to include(:user) end end end diff --git a/spec/migrations/20251015071027_allow_multiple_service_bindings_spec.rb b/spec/migrations/20251015071027_allow_multiple_service_bindings_spec.rb index 707a727609a..d1a75a4db87 100644 --- a/spec/migrations/20251015071027_allow_multiple_service_bindings_spec.rb +++ b/spec/migrations/20251015071027_allow_multiple_service_bindings_spec.rb @@ -7,63 +7,36 @@ end describe 'service_bindings table' do - context 'up migration' do - it 'is in the correct state before migration' do - expect(db.indexes(:service_bindings)).to include(:unique_service_binding_service_instance_guid_app_guid) - expect(db.indexes(:service_bindings)).to include(:unique_service_binding_app_guid_name) - expect(db.indexes(:service_bindings)).not_to include(:service_bindings_app_guid_service_instance_guid_index) - expect(db.indexes(:service_bindings)).not_to include(:service_bindings_app_guid_name_index) - end + it 'replaces unique constraints with indexes and handles idempotency' do + # Verify initial state + expect(db.indexes(:service_bindings)).to include(:unique_service_binding_service_instance_guid_app_guid) + expect(db.indexes(:service_bindings)).to include(:unique_service_binding_app_guid_name) + expect(db.indexes(:service_bindings)).not_to include(:service_bindings_app_guid_service_instance_guid_index) + expect(db.indexes(:service_bindings)).not_to include(:service_bindings_app_guid_name_index) - it 'migrates successfully' do - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(:service_bindings)).not_to include(:unique_service_binding_app_guid_name) - expect(db.indexes(:service_bindings)).not_to include(:unique_service_binding_service_instance_guid_app_guid) - expect(db.indexes(:service_bindings)).to include(:service_bindings_app_guid_service_instance_guid_index) - expect(db.indexes(:service_bindings)).to include(:service_bindings_app_guid_name_index) - end + # === UP MIGRATION === + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:service_bindings)).not_to include(:unique_service_binding_app_guid_name) + expect(db.indexes(:service_bindings)).not_to include(:unique_service_binding_service_instance_guid_app_guid) + expect(db.indexes(:service_bindings)).to include(:service_bindings_app_guid_service_instance_guid_index) + expect(db.indexes(:service_bindings)).to include(:service_bindings_app_guid_name_index) - it 'does not fail if indexes/constraints are already in desired state' do - db.alter_table :service_bindings do - drop_constraint :unique_service_binding_service_instance_guid_app_guid - drop_constraint :unique_service_binding_app_guid_name - end - if db.database_type == :postgres - db.add_index :service_bindings, %i[app_guid service_instance_guid], name: :service_bindings_app_guid_service_instance_guid_index, if_not_exists: true, concurrently: true - db.add_index :service_bindings, %i[app_guid name], name: :service_bindings_app_guid_name_index, if_not_exists: true, concurrently: true - else - db.add_index :service_bindings, %i[app_guid service_instance_guid], name: :service_bindings_app_guid_service_instance_guid_index, if_not_exists: true - db.add_index :service_bindings, %i[app_guid name], name: :service_bindings_app_guid_name_index, if_not_exists: true - end - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - end - end - - context 'down migration' do - it 'rolls back successfully' do - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(:service_bindings)).to include(:unique_service_binding_service_instance_guid_app_guid) - expect(db.indexes(:service_bindings)).to include(:unique_service_binding_app_guid_name) - expect(db.indexes(:service_bindings)).not_to include(:service_bindings_app_guid_service_instance_guid_index) - expect(db.indexes(:service_bindings)).not_to include(:service_bindings_app_guid_name_index) - end + # Test up migration idempotency + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:service_bindings)).to include(:service_bindings_app_guid_service_instance_guid_index) + expect(db.indexes(:service_bindings)).to include(:service_bindings_app_guid_name_index) - it 'does not fail if indexes/constraints are already in desired state' do - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - db.alter_table :service_bindings do - add_unique_constraint %i[service_instance_guid app_guid], name: :unique_service_binding_service_instance_guid_app_guid - add_unique_constraint %i[app_guid name], name: :unique_service_binding_app_guid_name - end - if db.database_type == :postgres - db.drop_index :service_bindings, %i[app_guid service_instance_guid], name: :service_bindings_app_guid_service_instance_guid_index, if_exists: true, concurrently: true - db.drop_index :service_bindings, %i[app_guid name], name: :service_bindings_app_guid_name_index, if_exists: true, concurrently: true - else - db.drop_index :service_bindings, %i[app_guid service_instance_guid], name: :service_bindings_app_guid_service_instance_guid_index, if_exists: true - db.drop_index :service_bindings, %i[app_guid name], name: :service_bindings_app_guid_name_index, if_exists: true - end + # === DOWN MIGRATION === + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:service_bindings)).to include(:unique_service_binding_service_instance_guid_app_guid) + expect(db.indexes(:service_bindings)).to include(:unique_service_binding_app_guid_name) + expect(db.indexes(:service_bindings)).not_to include(:service_bindings_app_guid_service_instance_guid_index) + expect(db.indexes(:service_bindings)).not_to include(:service_bindings_app_guid_name_index) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - end + # Test down migration idempotency + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:service_bindings)).to include(:unique_service_binding_service_instance_guid_app_guid) + expect(db.indexes(:service_bindings)).to include(:unique_service_binding_app_guid_name) end end end diff --git a/spec/migrations/20251016120006_security_groups_spaces_unique_index_spec.rb b/spec/migrations/20251016120006_security_groups_spaces_unique_index_spec.rb index 94affbcf364..2a0d4a378f4 100644 --- a/spec/migrations/20251016120006_security_groups_spaces_unique_index_spec.rb +++ b/spec/migrations/20251016120006_security_groups_spaces_unique_index_spec.rb @@ -12,65 +12,52 @@ let(:sec_group_2) { VCAP::CloudController::SecurityGroup.make } describe 'security_groups_spaces table' do - context 'up migration' do - it 'is in the correct state before migration' do - expect(db.indexes(:security_groups_spaces)).to include(:sgs_spaces_ids) - expect(db.indexes(:security_groups_spaces)).not_to include(:security_groups_spaces_ids) - end - - it 'removes duplicates and migrates successfully by adding unique index' do - db[:security_groups_spaces].insert(security_group_id: sec_group_1.id, space_id: space_1.id) - db[:security_groups_spaces].insert(security_group_id: sec_group_1.id, space_id: space_1.id) - db[:security_groups_spaces].insert(security_group_id: sec_group_1.id, space_id: space_2.id) - db[:security_groups_spaces].insert(security_group_id: sec_group_2.id, space_id: space_1.id) - db[:security_groups_spaces].insert(security_group_id: sec_group_2.id, space_id: space_2.id) - db[:security_groups_spaces].insert(security_group_id: sec_group_2.id, space_id: space_2.id) - db[:security_groups_spaces].insert(security_group_id: sec_group_2.id, space_id: space_2.id) - - # Count duplicates before migration - expect(db[:security_groups_spaces].where(security_group_id: sec_group_1.id, space_id: space_1.id).count).to eq(2) - expect(db[:security_groups_spaces].where(security_group_id: sec_group_1.id, space_id: space_2.id).count).to eq(1) - expect(db[:security_groups_spaces].where(security_group_id: sec_group_2.id, space_id: space_1.id).count).to eq(1) - expect(db[:security_groups_spaces].where(security_group_id: sec_group_2.id, space_id: space_2.id).count).to eq(3) - - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - - # Verify duplicates are removed after migration - expect(db[:security_groups_spaces].where(security_group_id: sec_group_1.id, space_id: space_1.id).count).to eq(1) - expect(db[:security_groups_spaces].where(security_group_id: sec_group_1.id, space_id: space_2.id).count).to eq(1) - expect(db[:security_groups_spaces].where(security_group_id: sec_group_2.id, space_id: space_1.id).count).to eq(1) - expect(db[:security_groups_spaces].where(security_group_id: sec_group_2.id, space_id: space_2.id).count).to eq(1) - - # Verify indexes are updated - expect(db.indexes(:security_groups_spaces)).not_to include(:sgs_spaces_ids) - expect(db.indexes(:security_groups_spaces)).to include(:security_groups_spaces_ids) - end - - it 'does not fail if indexes/constraints are already in desired state' do - db.alter_table :security_groups_spaces do - add_index %i[security_group_id space_id], unique: true, name: :security_groups_spaces_ids - drop_index %i[security_group_id space_id], name: :sgs_spaces_ids - end - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - end - end - - context 'down migration' do - it 'rolls back successfully' do - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(:security_groups_spaces)).to include(:sgs_spaces_ids) - expect(db.indexes(:security_groups_spaces)).not_to include(:security_groups_spaces_ids) - end - - it 'does not fail if indexes/constraints are already in desired state' do - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - db.alter_table :security_groups_spaces do - add_index %i[security_group_id space_id], name: :sgs_spaces_ids - drop_index %i[security_group_id space_id], unique: true, name: :security_groups_spaces_ids - end - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - end + it 'removes duplicates, updates indexes, and handles idempotency' do + # Verify initial state + expect(db.indexes(:security_groups_spaces)).to include(:sgs_spaces_ids) + expect(db.indexes(:security_groups_spaces)).not_to include(:security_groups_spaces_ids) + + # Insert test data with duplicates + db[:security_groups_spaces].insert(security_group_id: sec_group_1.id, space_id: space_1.id) + db[:security_groups_spaces].insert(security_group_id: sec_group_1.id, space_id: space_1.id) + db[:security_groups_spaces].insert(security_group_id: sec_group_1.id, space_id: space_2.id) + db[:security_groups_spaces].insert(security_group_id: sec_group_2.id, space_id: space_1.id) + db[:security_groups_spaces].insert(security_group_id: sec_group_2.id, space_id: space_2.id) + db[:security_groups_spaces].insert(security_group_id: sec_group_2.id, space_id: space_2.id) + db[:security_groups_spaces].insert(security_group_id: sec_group_2.id, space_id: space_2.id) + + # Count duplicates before migration + expect(db[:security_groups_spaces].where(security_group_id: sec_group_1.id, space_id: space_1.id).count).to eq(2) + expect(db[:security_groups_spaces].where(security_group_id: sec_group_1.id, space_id: space_2.id).count).to eq(1) + expect(db[:security_groups_spaces].where(security_group_id: sec_group_2.id, space_id: space_1.id).count).to eq(1) + expect(db[:security_groups_spaces].where(security_group_id: sec_group_2.id, space_id: space_2.id).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 + + # Verify duplicates are removed after migration + expect(db[:security_groups_spaces].where(security_group_id: sec_group_1.id, space_id: space_1.id).count).to eq(1) + expect(db[:security_groups_spaces].where(security_group_id: sec_group_1.id, space_id: space_2.id).count).to eq(1) + expect(db[:security_groups_spaces].where(security_group_id: sec_group_2.id, space_id: space_1.id).count).to eq(1) + expect(db[:security_groups_spaces].where(security_group_id: sec_group_2.id, space_id: space_2.id).count).to eq(1) + + # Verify indexes are updated + expect(db.indexes(:security_groups_spaces)).not_to include(:sgs_spaces_ids) + expect(db.indexes(:security_groups_spaces)).to include(:security_groups_spaces_ids) + + # Test up migration idempotency + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:security_groups_spaces)).to include(:security_groups_spaces_ids) + + # === DOWN MIGRATION === + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:security_groups_spaces)).to include(:sgs_spaces_ids) + expect(db.indexes(:security_groups_spaces)).not_to include(:security_groups_spaces_ids) + + # Test down migration idempotency + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:security_groups_spaces)).to include(:sgs_spaces_ids) + expect(db.indexes(:security_groups_spaces)).not_to include(:security_groups_spaces_ids) end end end diff --git a/spec/migrations/20251028135214_route_bindings_unique_index_spec.rb b/spec/migrations/20251028135214_route_bindings_unique_index_spec.rb index 6b385e908e2..9cbc119a0f0 100644 --- a/spec/migrations/20251028135214_route_bindings_unique_index_spec.rb +++ b/spec/migrations/20251028135214_route_bindings_unique_index_spec.rb @@ -13,57 +13,49 @@ let(:route_2) { VCAP::CloudController::Route.make(space:) } describe 'route_bindings table' do - context 'up migration' do - it 'is in the correct state before migration' do - expect(db.indexes(:route_bindings)).not_to include(:route_bindings_route_id_service_instance_id_index) - end - - it 'removes duplicates and migrates successfully by adding unique index' do - db[:route_bindings].insert(route_id: route_1.id, service_instance_id: service_instance_1.id, guid: SecureRandom.uuid) - db[:route_bindings].insert(route_id: route_1.id, service_instance_id: service_instance_1.id, guid: SecureRandom.uuid) - db[:route_bindings].insert(route_id: route_2.id, service_instance_id: service_instance_1.id, guid: SecureRandom.uuid) - db[:route_bindings].insert(route_id: route_1.id, service_instance_id: service_instance_2.id, guid: SecureRandom.uuid) - db[:route_bindings].insert(route_id: route_2.id, service_instance_id: service_instance_2.id, guid: SecureRandom.uuid) - db[:route_bindings].insert(route_id: route_2.id, service_instance_id: service_instance_2.id, guid: SecureRandom.uuid) - db[:route_bindings].insert(route_id: route_2.id, service_instance_id: service_instance_2.id, guid: SecureRandom.uuid) - - # Count duplicates before migration - expect(db[:route_bindings].where(service_instance_id: service_instance_1.id, route_id: route_1.id).count).to eq(2) - expect(db[:route_bindings].where(service_instance_id: service_instance_1.id, route_id: route_2.id).count).to eq(1) - expect(db[:route_bindings].where(service_instance_id: service_instance_2.id, route_id: route_1.id).count).to eq(1) - expect(db[:route_bindings].where(service_instance_id: service_instance_2.id, route_id: route_2.id).count).to eq(3) - - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - - # Verify duplicates are removed after migration - expect(db[:route_bindings].where(service_instance_id: service_instance_1.id, route_id: route_1.id).count).to eq(1) - expect(db[:route_bindings].where(service_instance_id: service_instance_1.id, route_id: route_2.id).count).to eq(1) - expect(db[:route_bindings].where(service_instance_id: service_instance_2.id, route_id: route_1.id).count).to eq(1) - expect(db[:route_bindings].where(service_instance_id: service_instance_2.id, route_id: route_2.id).count).to eq(1) - - # Verify index is added - expect(db.indexes(:route_bindings)).to include(:route_bindings_route_id_service_instance_id_index) - end - - it 'does not fail if indexes/constraints are already in desired state' do - db.alter_table(:route_bindings) { add_index %i[route_id service_instance_id], unique: true, name: :route_bindings_route_id_service_instance_id_index } - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - end - end - - context 'down migration' do - it 'rolls back successfully' do - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(:route_bindings)).not_to include(:route_bindings_route_id_service_instance_id_index) - expect(db.indexes(:route_bindings)).to include(:route_id) if db.database_type == :mysql - end - - it 'does not fail if indexes/constraints are already in desired state' do - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - db.alter_table(:route_bindings) { drop_index %i[route_id service_instance_id], unique: true, name: :route_bindings_route_id_service_instance_id_index } - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - end + it 'removes duplicates, manages unique index, and handles idempotency' do + # Verify initial state + expect(db.indexes(:route_bindings)).not_to include(:route_bindings_route_id_service_instance_id_index) + + # Insert test data with duplicates + db[:route_bindings].insert(route_id: route_1.id, service_instance_id: service_instance_1.id, guid: SecureRandom.uuid) + db[:route_bindings].insert(route_id: route_1.id, service_instance_id: service_instance_1.id, guid: SecureRandom.uuid) + db[:route_bindings].insert(route_id: route_2.id, service_instance_id: service_instance_1.id, guid: SecureRandom.uuid) + db[:route_bindings].insert(route_id: route_1.id, service_instance_id: service_instance_2.id, guid: SecureRandom.uuid) + db[:route_bindings].insert(route_id: route_2.id, service_instance_id: service_instance_2.id, guid: SecureRandom.uuid) + db[:route_bindings].insert(route_id: route_2.id, service_instance_id: service_instance_2.id, guid: SecureRandom.uuid) + db[:route_bindings].insert(route_id: route_2.id, service_instance_id: service_instance_2.id, guid: SecureRandom.uuid) + + # Count duplicates before migration + expect(db[:route_bindings].where(service_instance_id: service_instance_1.id, route_id: route_1.id).count).to eq(2) + expect(db[:route_bindings].where(service_instance_id: service_instance_1.id, route_id: route_2.id).count).to eq(1) + expect(db[:route_bindings].where(service_instance_id: service_instance_2.id, route_id: route_1.id).count).to eq(1) + expect(db[:route_bindings].where(service_instance_id: service_instance_2.id, route_id: route_2.id).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 + + # Verify duplicates are removed after migration + expect(db[:route_bindings].where(service_instance_id: service_instance_1.id, route_id: route_1.id).count).to eq(1) + expect(db[:route_bindings].where(service_instance_id: service_instance_1.id, route_id: route_2.id).count).to eq(1) + expect(db[:route_bindings].where(service_instance_id: service_instance_2.id, route_id: route_1.id).count).to eq(1) + expect(db[:route_bindings].where(service_instance_id: service_instance_2.id, route_id: route_2.id).count).to eq(1) + + # Verify index is added + expect(db.indexes(:route_bindings)).to include(:route_bindings_route_id_service_instance_id_index) + + # Test up migration idempotency + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:route_bindings)).to include(:route_bindings_route_id_service_instance_id_index) + + # === DOWN MIGRATION === + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:route_bindings)).not_to include(:route_bindings_route_id_service_instance_id_index) + expect(db.indexes(:route_bindings)).to include(:route_id) if db.database_type == :mysql + + # Test down migration idempotency + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:route_bindings)).not_to include(:route_bindings_route_id_service_instance_id_index) end end end diff --git a/spec/migrations/20251121174647_add_broker_provided_metadata_to_service_instances_spec.rb b/spec/migrations/20251121174647_add_broker_provided_metadata_to_service_instances_spec.rb index eb0797a4559..0be03ed1fbd 100644 --- a/spec/migrations/20251121174647_add_broker_provided_metadata_to_service_instances_spec.rb +++ b/spec/migrations/20251121174647_add_broker_provided_metadata_to_service_instances_spec.rb @@ -12,33 +12,41 @@ end describe 'service_instances table' do - subject(:run_migration) { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) } - let(:space) { VCAP::CloudController::Space.make } - it 'adds a column `broker_provided_metadata`' do + it 'adds broker_provided_metadata column with correct properties' do + # Insert a service instance before migration to test preservation + db[:service_instances].insert( + guid: 'existing-service-instance-guid', + name: 'existing-instance', + space_id: space.id + ) + + # Verify column doesn't exist yet expect(db[:service_instances].columns).not_to include(:broker_provided_metadata) - run_migration + + # Run migration + Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) + + # Verify column was added expect(db[:service_instances].columns).to include(:broker_provided_metadata) - end - it 'allows null values for broker_provided_metadata' do - run_migration - # Insert a service instance without broker_provided_metadata + # Verify existing instance was preserved with null metadata + existing_instance = db[:service_instances].first(guid: 'existing-service-instance-guid') + expect(existing_instance).not_to be_nil + expect(existing_instance[:broker_provided_metadata]).to be_nil + + # Verify null values are allowed db[:service_instances].insert( guid: 'test-service-instance-guid', name: 'test-instance', space_id: space.id, broker_provided_metadata: nil ) - # Verify the insert succeeded and the column is null - instance = db[:service_instances].first(guid: 'test-service-instance-guid') - expect(instance[:broker_provided_metadata]).to be_nil - end + instance_with_null = db[:service_instances].first(guid: 'test-service-instance-guid') + expect(instance_with_null[:broker_provided_metadata]).to be_nil - it 'accepts text values for broker_provided_metadata' do - run_migration - # Insert a service instance with broker_provided_metadata + # Verify text values are accepted metadata_json = '{"labels": {"version": "1.0"}, "attributes": {"engine": "postgresql"}}' db[:service_instances].insert( guid: 'test-service-instance-with-metadata', @@ -46,23 +54,8 @@ space_id: space.id, broker_provided_metadata: metadata_json ) - # Verify the metadata was stored correctly - instance = db[:service_instances].first(guid: 'test-service-instance-with-metadata') - expect(instance[:broker_provided_metadata]).to eq(metadata_json) - end - - it 'preserves existing service instances after migration' do - # Insert a service instance before migration - db[:service_instances].insert( - guid: 'existing-service-instance-guid', - name: 'existing-instance', - space_id: space.id - ) - run_migration - # Verify the existing instance still exists and has null metadata - instance = db[:service_instances].first(guid: 'existing-service-instance-guid') - expect(instance).not_to be_nil - expect(instance[:broker_provided_metadata]).to be_nil + instance_with_metadata = db[:service_instances].first(guid: 'test-service-instance-with-metadata') + expect(instance_with_metadata[:broker_provided_metadata]).to eq(metadata_json) end end end diff --git a/spec/migrations/Readme.md b/spec/migrations/Readme.md index eba2e86e178..22693090e8a 100644 --- a/spec/migrations/Readme.md +++ b/spec/migrations/Readme.md @@ -77,8 +77,10 @@ To create resilient and reliable migrations, follow these guidelines: Sequel migration tests have a distinct operation compared to conventional RSpec tests. Primarily, they execute the Down migration and restore the database state to its previous form before the test-specific migration was carried out. The process includes running a test, creating assets, executing a specific migration file for testing, and asserting certain behaviors. However, be aware that Sequel migration tests impose specific limitations and requirements on test writing. -1. The migration spec should not be influenced by any Cloud Controller code. This requirement is the same as for the migration itself. Any model changes can modify the old migrations and alter the behaviors and results of the tests. Therefore, don't select data via the CC models, instead make the selects in raw Sequel and assert the behavior you'd like to test. -1. It's recommended to use the `migration` shared context, as it ensures that the database schema first reverts to the version before the migration you aim to test. This shared context also provides a directory containing a single migration for running a particular migration within a test. When the test is done, this shared context makes sure to restore the correct schema by running the migrations that post-date the one being tested. Thus, avoiding cases of a half-migrated database that could result in random test failures. It also makes sure to test not every migration that comes after the test migration, but just a single migration is executed and then the expected behavior is evaluated. +### Limitations and Requirements +1. **Independence**: The migration spec should not be influenced by any Cloud Controller code. This requirement is the same as for the migration itself. Any model changes can modify the old migrations and alter the behaviors and results of the tests. Therefore, don't select data via the CC models, instead make the selects in raw Sequel and assert the behavior you'd like to test. +1. **Shared Context**: It's recommended to use the `migration` shared context, as it ensures that the database schema first reverts to the version before the migration you aim to test. This shared context also provides a directory containing a single migration for running a particular migration within a test. When the test is done, this shared context makes sure to restore the correct schema by running the migrations that post-date the one being tested. Thus, avoiding cases of a half-migrated database that could result in random test failures. It also makes sure to test not every migration that comes after the test migration, but just a single migration is executed and then the expected behavior is evaluated. +1. **Performance**: Running migrations (`Sequel::Migrator.run(...)`) can be time-consuming and take serval seconds with each call. Therefore, it's advisable to limit the number of migration calls within a single spec file as much as possible. Group related tests into a single it block even if it does not follow the usual RSpec best practices. This approach minimizes the number of migration calls and enhances test performance. ### Usage @@ -95,12 +97,15 @@ RSpec.describe 'migration to modify isolation_segments', isolation: :truncation describe 'isolation_segments table' do it 'retains the initial name and guid' do - db[:isolation_segments].insert(name: 'bommel', guid: '123') + db[:isolation_segments].insert(name: 'name', guid: '123') a1 = db[:isolation_segment_annotations].first(resource_guid: '123') expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error b1 = db[:isolation_segment_annotations].first(resource_guid: '123') expect(b1[:guid]).to eq a1[:guid] expect(b1[:name]).to eq a1[:name] + + # Test idempotency of the migration + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error end end end diff --git a/spec/migrations/helpers/bigint_migration_step1_shared_context.rb b/spec/migrations/helpers/bigint_migration_step1_shared_context.rb index 6bae6daf9ed..e288c34f12d 100644 --- a/spec/migrations/helpers/bigint_migration_step1_shared_context.rb +++ b/spec/migrations/helpers/bigint_migration_step1_shared_context.rb @@ -2,8 +2,6 @@ require 'database/bigint_migration' RSpec.shared_context 'bigint migration step1' do - subject(:run_migration) { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) } - include_context 'migration' let(:skip_bigint_id_migration) { nil } @@ -29,25 +27,19 @@ db[table].delete end - it "changes the id column's type to bigint" do + it "changes the id column's type to bigint and does not add id_bigint column" do expect(db).to have_table_with_column_and_type(table, :id, 'integer') - - run_migration - - expect(db).to have_table_with_column_and_type(table, :id, 'bigint') - end - - it 'does not add the id_bigint column' do expect(db).not_to have_table_with_column(table, :id_bigint) - run_migration + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + expect(db).to have_table_with_column_and_type(table, :id, 'bigint') expect(db).not_to have_table_with_column(table, :id_bigint) end describe 'backfill' do before do - run_migration + Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) end it 'fails with a proper error message' do @@ -65,39 +57,26 @@ db[table].delete # Necessary to successfully run subsequent migrations in the after block of the migration shared context... end - it "does not change the id column's type" do + it 'keeps id as integer, adds id_bigint column and creates trigger function' do expect(db).to have_table_with_column_and_type(table, :id, 'integer') - - run_migration - - expect(db).to have_table_with_column_and_type(table, :id, 'integer') - end - - it 'adds the id_bigint column' do expect(db).not_to have_table_with_column(table, :id_bigint) + expect(db).not_to have_trigger_function_for_table(table) - run_migration + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + expect(db).to have_table_with_column_and_type(table, :id, 'integer') expect(db).to have_table_with_column_and_type(table, :id_bigint, 'bigint') - end - - it 'creates the trigger function' do - expect(db).not_to have_trigger_function_for_table(table) - - run_migration expect(db).to have_trigger_function_for_table(table) end - it 'does not populate the id_bigint column for an existing entry' do - run_migration + it 'does not populate id_bigint for existing entries but does for new entries' do + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + # Existing entry should not have id_bigint populated expect(db[table].where(id: old_id).get(:id_bigint)).to be_nil - end - - it 'automatically populates the id_bigint column for a new entry' do - run_migration + # New entry should have id_bigint automatically populated new_id = insert.call(db) expect(db[table].where(id: new_id).get(:id_bigint)).to eq(new_id) end @@ -105,8 +84,7 @@ describe 'backfill' do before do 100.times { insert.call(db) } - - run_migration + Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) end context 'default batch size' do @@ -159,7 +137,7 @@ expect(db).to have_table_with_column_and_type(table, :id, 'integer') expect(db).not_to have_table_with_column(table, :id_bigint) - run_migration + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error expect(db).to have_table_with_column_and_type(table, :id, 'integer') expect(db).not_to have_table_with_column(table, :id_bigint) @@ -168,18 +146,16 @@ end describe 'down' do - subject(:run_rollback) { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) } - context 'when the table is empty' do before do db[table].delete - run_migration + Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) end it "reverts the id column's type to integer" do expect(db).to have_table_with_column_and_type(table, :id, 'bigint') - run_rollback + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error expect(db).to have_table_with_column_and_type(table, :id, 'integer') end @@ -188,26 +164,20 @@ context 'when the table is not empty' do before do insert.call(db) - run_migration + Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) end after do db[table].delete # Necessary to successfully run subsequent migrations in the after block of the migration shared context... end - it 'drops the id_bigint column' do + it 'drops the id_bigint column and trigger function' do expect(db).to have_table_with_column(table, :id_bigint) - - run_rollback - - expect(db).not_to have_table_with_column(table, :id_bigint) - end - - it 'drops the trigger function' do expect(db).to have_trigger_function_for_table(table) - run_rollback + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db).not_to have_table_with_column(table, :id_bigint) expect(db).not_to have_trigger_function_for_table(table) end end diff --git a/spec/migrations/helpers/bigint_migration_step3_shared_context.rb b/spec/migrations/helpers/bigint_migration_step3_shared_context.rb index 064e3a5292a..f17dd1d0860 100644 --- a/spec/migrations/helpers/bigint_migration_step3_shared_context.rb +++ b/spec/migrations/helpers/bigint_migration_step3_shared_context.rb @@ -2,10 +2,6 @@ require 'database/bigint_migration' RSpec.shared_context 'bigint migration step3a' do - subject(:run_migration_step1) { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) } - - subject(:run_migration_step3a) { Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3a, allow_missing_migration_files: true) } - let(:migration_filename) { migration_filename_step1 } let(:current_migration_index_step3a) { migration_filename_step3a.match(/\A\d+/)[0].to_i } @@ -26,7 +22,7 @@ context 'when the id_bigint column was added' do before do insert.call(db) - run_migration_step1 + Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) end context 'when backfilling was completed' do @@ -37,7 +33,7 @@ it 'adds a check constraint' do expect(db).not_to have_table_with_check_constraint(table) - run_migration_step3a + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3a, allow_missing_migration_files: true) }.not_to raise_error expect(db).to have_table_with_check_constraint(table) end @@ -50,7 +46,7 @@ it 'fails ...' do expect do - run_migration_step3a + Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3a, allow_missing_migration_files: true) end.to raise_error(/Failed to add check constraint on '#{table}' table!/) end end @@ -59,13 +55,13 @@ context "when the migration was concluded (id column's type switched)" do before do db[table].delete - run_migration_step1 + Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) end it 'does not add a check constraint' do expect(db).not_to have_table_with_check_constraint(table) - run_migration_step3a + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3a, allow_missing_migration_files: true) }.not_to raise_error expect(db).not_to have_table_with_check_constraint(table) end @@ -76,13 +72,13 @@ let(:skip_bigint_id_migration) { true } before do - run_migration_step1 + Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) end it 'does not add a check constraint' do expect(db).not_to have_table_with_check_constraint(table) - run_migration_step3a + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3a, allow_missing_migration_files: true) }.not_to raise_error expect(db).not_to have_table_with_check_constraint(table) end @@ -90,20 +86,18 @@ end describe 'down' do - subject(:run_rollback_step3a) { Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3a - 1, allow_missing_migration_files: true) } - context 'when migration step 3a was executed' do before do insert.call(db) - run_migration_step1 + Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) VCAP::BigintMigration.backfill(logger, db, table) - run_migration_step3a + Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3a, allow_missing_migration_files: true) end it 'drops the check constraint' do expect(db).to have_table_with_check_constraint(table) - run_rollback_step3a + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3a - 1, allow_missing_migration_files: true) }.not_to raise_error expect(db).not_to have_table_with_check_constraint(table) end @@ -112,12 +106,6 @@ end RSpec.shared_context 'bigint migration step3b' do - subject(:run_migration_step1) { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) } - - subject(:run_migration_step3a) { Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3a, allow_missing_migration_files: true) } - - subject(:run_migration_step3b) { Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3b, allow_missing_migration_files: true) } - let(:migration_filename) { migration_filename_step1 } let(:current_migration_index_step3a) { migration_filename_step3a.match(/\A\d+/)[0].to_i } let(:current_migration_index_step3b) { migration_filename_step3b.match(/\A\d+/)[0].to_i } @@ -138,41 +126,26 @@ context 'when migration step 3a was executed' do before do insert.call(db) - run_migration_step1 + Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) VCAP::BigintMigration.backfill(logger, db, table) - run_migration_step3a + Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3a, allow_missing_migration_files: true) end - it 'drops the check constraint' do + it 'completes the bigint migration: drops constraints, renames columns, and maintains primary key' do + # Verify pre-migration state expect(db).to have_table_with_check_constraint(table) - - run_migration_step3b - - expect(db).not_to have_table_with_check_constraint(table) - end - - it 'drops the trigger function' do expect(db).to have_trigger_function_for_table(table) - - run_migration_step3b - - expect(db).not_to have_trigger_function_for_table(table) - end - - it 'drops the id column and renames the id_bigint column to id' do expect(db).to have_table_with_column_and_type(table, :id, 'integer') expect(db).to have_table_with_column_and_type(table, :id_bigint, 'bigint') + expect(db).to have_table_with_primary_key(table, :id) - run_migration_step3b + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3b, allow_missing_migration_files: true) }.not_to raise_error + # Verify post-migration state + expect(db).not_to have_table_with_check_constraint(table) + expect(db).not_to have_trigger_function_for_table(table) expect(db).to have_table_with_column_and_type(table, :id, 'bigint') expect(db).not_to have_table_with_column(table, :id_bigint) - end - - it 'uses the (bigint) id column as primary key' do - expect(db).to have_table_with_primary_key(table, :id) - - run_migration_step3b expect(db).to have_table_with_primary_key(table, :id) end @@ -182,7 +155,7 @@ expect(db).to have_table_with_index_on_columns(table, %i[timestamp id]) - run_migration_step3b + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3b, allow_missing_migration_files: true) }.not_to raise_error expect(db).to have_table_with_index_on_columns(table, %i[timestamp id]) end @@ -191,7 +164,7 @@ it 'uses an identity with correct start value for the (bigint) id column' do last_id_before_migration = insert.call(db) - run_migration_step3b + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3b, allow_missing_migration_files: true) }.not_to raise_error first_id_after_migration = insert.call(db) expect(first_id_after_migration).to eq(last_id_before_migration + 1) @@ -200,71 +173,47 @@ end describe 'down' do - subject(:run_rollback_step3b) { Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3b - 1, allow_missing_migration_files: true) } - context 'when migration step 3b was executed' do before do insert.call(db) - run_migration_step1 + Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) VCAP::BigintMigration.backfill(logger, db, table) - run_migration_step3a - run_migration_step3b + Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3a, allow_missing_migration_files: true) + Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3b, allow_missing_migration_files: true) end it 'uses an identity with correct start value for the (integer) id column' do last_id_before_migration = insert.call(db) - run_rollback_step3b + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3b - 1, allow_missing_migration_files: true) }.not_to raise_error first_id_after_migration = insert.call(db) expect(first_id_after_migration).to eq(last_id_before_migration + 1) end - it 'has an index on timestamp + (integer) id column' do - if db.schema(table).any? { |col| col[0] == :timestamp } - - expect(db).to have_table_with_index_on_columns(table, %i[timestamp id]) - - run_rollback_step3b - - expect(db).to have_table_with_index_on_columns(table, %i[timestamp id]) - expect(db).not_to have_table_with_index_on_columns(table, %i[timestamp id_bigint]) - end - end - - it 'uses the (integer) id column as primary key' do - expect(db).to have_table_with_primary_key(table, :id) - - run_rollback_step3b - - expect(db).to have_table_with_primary_key(table, :id) - end - - it 'renames the id column to id_bigint and re-adds the (integer) id column' do + it 'reverts the bigint migration: restores columns, constraints, and indexes' do + # Verify pre-rollback state expect(db).to have_table_with_column_and_type(table, :id, 'bigint') expect(db).not_to have_table_with_column(table, :id_bigint) + expect(db).to have_table_with_primary_key(table, :id) + expect(db).not_to have_trigger_function_for_table(table) + expect(db).not_to have_table_with_check_constraint(table) - run_rollback_step3b + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3b - 1, allow_missing_migration_files: true) }.not_to raise_error + # Verify post-rollback state expect(db).to have_table_with_column_and_type(table, :id, 'integer') expect(db).to have_table_with_column_and_type(table, :id_bigint, 'bigint') expect(db).to have_table_with_column_and_attribute(table, :id_bigint, :allow_null, true) - end - - it 're-creates the trigger function' do - expect(db).not_to have_trigger_function_for_table(table) - - run_rollback_step3b - + expect(db).to have_table_with_primary_key(table, :id) expect(db).to have_trigger_function_for_table(table) - end - - it 're-adds the check constraint (this also ensures that id was correctly backfilled)' do - expect(db).not_to have_table_with_check_constraint(table) - - run_rollback_step3b - expect(db).to have_table_with_check_constraint(table) + + # Verify timestamp index if applicable + if db.schema(table).any? { |col| col[0] == :timestamp } + expect(db).to have_table_with_index_on_columns(table, %i[timestamp id]) + expect(db).not_to have_table_with_index_on_columns(table, %i[timestamp id_bigint]) + end end end end