From 488d876148d07155b800dc36b1efcfd0694e7d96 Mon Sep 17 00:00:00 2001 From: Florian Braun <5863788+FloThinksPi@users.noreply.github.com> Date: Wed, 3 Jan 2024 14:01:33 +0100 Subject: [PATCH] Ensure uniqueness of labels and annotations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit includes both code change and a database migration, contributing to ensuring labels and annotations' uniqueness. The code update inserts an empty string as prefix into the database when the prefix is empty. It also addresses the race condition issue where two concurrent updates on an object’s labels could result in the label/annotation being created twice in the DB. A uniqueness constraint error is now caught, and the find_or_create operation is retried to handle this issue. The database migration: - Modifies the default value of the prefix column to an empty string. - Converts all NULL values in the table to an empty string. - Restricts the column from accepting NULL values. - Reduces the length of the 'key' column in annotation tables to a maximum of 63 characters, in accordance with the documentation and current API input sanitation practices. - Identifies and removes all duplicates based on identical values of resource_id, key_prefix, and key (annotation tables)/key_name (label tables). - Implements a unique key constraint on resource_id, key_prefix, and key (annotation tables)/key_name (label tables) to prevent new duplicates. Note that this change heavily utilizes table locking and transactions to ensure the change is carried out consistently and with resilience. Also the change in the datatype of the key_prefix column is essential as as for a unique constraint NULL vales are allways different as NULL != NULL in SQL99. A unique constraint containing NULL values would be not of any use since it wont provide proper uniqueness for all columns it contains. Co-authored-by: Katharina Przybill --- .rubocop_cc.yml | 8 +- app/actions/labels_update.rb | 11 +- .../label_selector_query_generator.rb | 6 +- app/models/helpers/metadata_model_mixin.rb | 12 +- ...221123000_rename_annotations_key_column.rb | 56 ++--- ...2150000_add_annotation_label_uniqueness.rb | 126 ++++++++++ ...3000_rename_annotations_key_column_spec.rb | 33 +-- ...00_add_annotation_label_uniqueness_spec.rb | 237 ++++++++++++++++++ 8 files changed, 415 insertions(+), 74 deletions(-) create mode 100644 db/migrations/20240102150000_add_annotation_label_uniqueness.rb create mode 100644 spec/migrations/20240102150000_add_annotation_label_uniqueness_spec.rb diff --git a/.rubocop_cc.yml b/.rubocop_cc.yml index 9a5b91f82ad..e23fc419548 100644 --- a/.rubocop_cc.yml +++ b/.rubocop_cc.yml @@ -126,12 +126,8 @@ Style/ExpandPathArguments: Exclude: - 'db/migrations/20130911111938_encrypt_app_env_json.rb' - 'db/migrations/201805*' -Rails/DangerousColumnNames: # Disabled for old migrations as we cannot change content of those - Enabled: true - Exclude: - - !ruby/regexp /db/migrations/201([0-9]).+\.rb$/ - - !ruby/regexp /db/migrations/202([0-2]).+\.rb$/ - - db/migrations/20221125134500_add_request_count_table.rb +Rails/DangerousColumnNames: # Disabled, in comparison to active_record we need to add the id column manually in sequel + Enabled: false Rails/SkipsModelValidations: # We don`t want any model at all in migrations and migration specs Enabled: true Exclude: diff --git a/app/actions/labels_update.rb b/app/actions/labels_update.rb index 9ff3d71a167..5f7adab786a 100644 --- a/app/actions/labels_update.rb +++ b/app/actions/labels_update.rb @@ -12,16 +12,19 @@ def update(resource, labels, label_klass, destroy_nil: true) prefix, name = VCAP::CloudController::MetadataHelpers.extract_prefix(label_key) if label_value.nil? && destroy_nil # Delete Label - label_klass.where(resource_guid: resource.guid, key_name: name).where(Sequel.or([[:key_prefix, prefix], [:key_prefix, prefix.to_s]])).try(:destroy) + label_klass.find(key_prefix: prefix.to_s, resource_guid: resource.guid, key_name: name)&.destroy next end begin tries ||= 2 label_klass.db.transaction(savepoint: true) do - label = label_klass.where(resource_guid: resource.guid, key_name: name).where(Sequel.or([[:key_prefix, prefix], [:key_prefix, prefix.to_s]])).first - label ||= label_klass.create(resource_guid: resource.guid, key_name: name, key_prefix: prefix) - label.update(value: label_value) + label = label_klass.find(key_prefix: prefix.to_s, resource_guid: resource.guid, key_name: name) + if label.nil? + label_klass.create(resource_guid: resource.guid, key_name: name, key_prefix: prefix, value: label_value) + else + label.update(value: label_value) + end end rescue Sequel::UniqueConstraintViolation => e if (tries -= 1).positive? diff --git a/app/fetchers/label_selector_query_generator.rb b/app/fetchers/label_selector_query_generator.rb index 3f2440280e1..fde07a3e2bb 100644 --- a/app/fetchers/label_selector_query_generator.rb +++ b/app/fetchers/label_selector_query_generator.rb @@ -23,15 +23,13 @@ def add_selector_queries(label_klass:, resource_dataset:, requirements:, resourc def guids_for_set_inclusion(label_klass, requirement) label_klass. select(:resource_guid). - where(key_name: requirement.key_name, value: requirement.values). - where(Sequel.or([[:key_prefix, requirement.key_prefix], [:key_prefix, requirement.key_prefix.to_s]])) + where(key_prefix: requirement.key_prefix.to_s, key_name: requirement.key_name, value: requirement.values) end def guids_for_existence(label_klass, requirement) label_klass. select(:resource_guid). - where(key_name: requirement.key_name). - where(Sequel.or([[:key_prefix, requirement.key_prefix], [:key_prefix, requirement.key_prefix.to_s]])) + where(key_prefix: requirement.key_prefix.to_s, key_name: requirement.key_name) end end end diff --git a/app/models/helpers/metadata_model_mixin.rb b/app/models/helpers/metadata_model_mixin.rb index b906490c11a..f9245972645 100644 --- a/app/models/helpers/metadata_model_mixin.rb +++ b/app/models/helpers/metadata_model_mixin.rb @@ -4,12 +4,12 @@ def self.included(included_class) included_class.class_eval <<-RUBY, __FILE__, __LINE__ + 1 # Transparently convert datatypes of key_prefix so empty strings are persisted in the DB instead of NULL def key_prefix - self[:key_prefix].presence - end - def key_prefix=(value) - self[:key_prefix] = value.nil? ? '' : value - end - def key_name=(val) + self[:key_prefix].presence + end + def key_prefix=(value) + self[:key_prefix] = value.nil? ? '' : value + end + def key_name=(val) self[:key_name] = val.nil? ? '' : val end def key_name diff --git a/db/migrations/20231221123000_rename_annotations_key_column.rb b/db/migrations/20231221123000_rename_annotations_key_column.rb index d45e86d8a0b..3bc743e9c57 100644 --- a/db/migrations/20231221123000_rename_annotations_key_column.rb +++ b/db/migrations/20231221123000_rename_annotations_key_column.rb @@ -1,32 +1,32 @@ -TABLE_BASE_NAMES = %w[ - app - build - buildpack - deployment - domain - droplet - isolation_segment - organization - package - process - revision - route_binding - route - service_binding - service_broker - service_broker_update_request - service_instance - service_key - service_offering - service_plan - space - stack - task - user -].freeze -annotation_tables = TABLE_BASE_NAMES.map { |tbn| "#{tbn}_annotations" }.freeze - Sequel.migration do + table_base_names = %w[ + app + build + buildpack + deployment + domain + droplet + isolation_segment + organization + package + process + revision + route_binding + route + service_binding + service_broker + service_broker_update_request + service_instance + service_key + service_offering + service_plan + space + stack + task + user + ].freeze + annotation_tables = table_base_names.map { |tbn| "#{tbn}_annotations" }.freeze + no_transaction # Disable automatic transactions up do diff --git a/db/migrations/20240102150000_add_annotation_label_uniqueness.rb b/db/migrations/20240102150000_add_annotation_label_uniqueness.rb new file mode 100644 index 00000000000..1dc3fc25194 --- /dev/null +++ b/db/migrations/20240102150000_add_annotation_label_uniqueness.rb @@ -0,0 +1,126 @@ +require 'logger' + +def unique_index_name(table) + :"#{table}_unique" +end + +def with_dropped_view(table) + # Recreate view just for postgres as it cannot alter types while a view is on the table + if database_type == :postgres + drop_view(:"#{table}_migration_view", if_exists: true) + yield if block_given? + create_view(:"#{table}_migration_view", self[table.to_sym].select do + [id, guid, created_at, updated_at, resource_guid, key_prefix, key_name, value] + end) + elsif block_given? + yield + end +end + +Sequel.migration do + table_base_names = %w[ + app + build + buildpack + deployment + domain + droplet + isolation_segment + organization + package + process + revision + route_binding + route + service_binding + service_broker + service_broker_update_request + service_instance + service_key + service_offering + service_plan + space + stack + task + user + ].freeze + annotation_tables = table_base_names.map { |tbn| "#{tbn}_annotations" }.freeze + label_tables = table_base_names.map { |tbn| "#{tbn}_labels" }.freeze + + no_transaction # Disable automatic transactions + + up do + (annotation_tables + label_tables).each do |table| + transaction do + # Create Temporary table for use later on + create_table! :"#{table}_temp", temp: true do + primary_key :id, name: :id + Integer :min_id, null: false + end + + # Just allow selects on this table while the migration runs for full consistency + run "LOCK TABLE #{table}, #{table}_temp IN SHARE MODE;" if database_type == :postgres + run "LOCK TABLES #{table} WRITE, #{table}_temp WRITE;" if database_type == :mysql + + # Updating the temporary column with truncated keys(should never chop of anything since the api just allows 63 chars) + # We run this in the DB as to minimize the time we hold the share mode lock on the table + self[table.to_sym].update(key_name: Sequel::SQL::Function.new(:SUBSTR, :key_name, 1, 63)) + + # Make en empty string the default for key_prefix as null in the unique constraint would not work. + # Null values are not equal to other Null values so a row that has NULL can be a duplicate then. + self[table.to_sym].where(key_prefix: nil).update(key_prefix: '') + self[table.to_sym].where(key_name: nil).update(key_name: '') + + # Recreate view just for postgres as it cannot alter types while a view is on the table + with_dropped_view(table) do + alter_table(table.to_sym) do + set_column_default :key_prefix, '' + set_column_not_null :key_prefix + set_column_not_null :key_name + set_column_type :key_name, String, size: 63 + end + end + + # Delete duplicates (in the DB as doing it in ruby is slow), we need to use a temporary table + # as mysql doesnt allow subselects on the same table it deletes from + min_ids = from(table.to_sym). + select(Sequel.function(:MIN, :id).as(:min_id)). + group_by(:resource_guid, :key_prefix, :key_name) + self[:"#{table}_temp"].import([:min_id], min_ids) + self[table.to_sym].exclude(id: from(:"#{table}_temp").select(:min_id)).delete + + # Add unique constraint if not already present + if indexes(table.to_sym)[unique_index_name(table)].nil? + alter_table(table.to_sym) do + add_unique_constraint %i[resource_guid key_prefix key_name], name: unique_index_name(table) + end + end + ensure + # Be sure to unlock the table on errors as this does not happen automatically by rolling back a transaction mysql + run 'UNLOCK TABLES;' if database_type == :mysql + end + end + end + + down do + (annotation_tables + label_tables).each do |table| + transaction do + # Drop unique constraint + if indexes(table.to_sym)[unique_index_name(table)].present? + alter_table(table.to_sym) do + drop_constraint(unique_index_name(table), type: :unique) + end + end + # Revert default type in key_prefix and null values handling + with_dropped_view(table) do + alter_table(table.to_sym) do + set_column_allow_null :key_prefix + set_column_allow_null :key_name + set_column_default :key_prefix, nil + set_column_type :key_name, String, size: 1000 if table.end_with?('_annotations') + end + end + end + end + end +end diff --git a/spec/migrations/20231221123000_rename_annotations_key_column_spec.rb b/spec/migrations/20231221123000_rename_annotations_key_column_spec.rb index e9b7292a13a..46a7990bc37 100644 --- a/spec/migrations/20231221123000_rename_annotations_key_column_spec.rb +++ b/spec/migrations/20231221123000_rename_annotations_key_column_spec.rb @@ -1,10 +1,11 @@ require 'spec_helper' +require 'migrations/helpers/migration_shared_context' + +RSpec.describe 'migration to streamline changes to annotation_key_prefix', isolation: :truncation, type: :migration do + include_context 'migration' do + let(:migration_filename) { '20231221123000_rename_annotations_key_column.rb' } + end -RSpec.describe 'migration to streamline changes to annotation_key_prefix', isolation: :truncation do - let(:filename) { '20231221123000_rename_annotations_key_column.rb' } - let(:tmp_down_migrations_dir) { Dir.mktmpdir } - let(:tmp_up_migrations_dir) { Dir.mktmpdir } - let(:db) { Sequel::Model.db } let(:tables) do %w[ app @@ -36,33 +37,13 @@ let(:annotation_tables) { tables.map { |tbn| "#{tbn}_annotations" }.freeze } - before do - Sequel.extension :migration - # Find all migrations - migration_files = Dir.glob("#{DBMigrator::SEQUEL_MIGRATIONS}/*.rb") - # Calculate the index of our migration file we`d like to test - migration_index = migration_files.find_index { |file| file.end_with?(filename) } - # Make a file list of the migration file we like to test plus all migrations after the one we want to test - migration_files_after_test = migration_files[migration_index...] - # Copy them to a temp directory - FileUtils.cp(migration_files_after_test, tmp_down_migrations_dir) - FileUtils.cp(File.join(DBMigrator::SEQUEL_MIGRATIONS, filename), tmp_up_migrations_dir) - # Revert the given migration and everything newer so we are at the database version exactly before our migration we want to test. - Sequel::Migrator.run(db, tmp_down_migrations_dir, target: 0, allow_missing_migration_files: true) - end - - after do - FileUtils.rm_rf(tmp_up_migrations_dir) - FileUtils.rm_rf(tmp_down_migrations_dir) - end - describe 'annotation tables' do it 'has renamed the column key to key_name' do annotation_tables.each do |table| expect(db[table.to_sym].columns).to include(:key) expect(db[table.to_sym].columns).not_to include(:key_name) end - expect { Sequel::Migrator.run(db, tmp_up_migrations_dir, allow_missing_migration_files: true) }.not_to raise_error + expect { Sequel::Migrator.run(db, migration_to_test, allow_missing_migration_files: true) }.not_to raise_error annotation_tables.each do |table| expect(db[table.to_sym].columns).not_to include(:key) expect(db[table.to_sym].columns).to include(:key_name) diff --git a/spec/migrations/20240102150000_add_annotation_label_uniqueness_spec.rb b/spec/migrations/20240102150000_add_annotation_label_uniqueness_spec.rb new file mode 100644 index 00000000000..9c38a976dae --- /dev/null +++ b/spec/migrations/20240102150000_add_annotation_label_uniqueness_spec.rb @@ -0,0 +1,237 @@ +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 + i1 = isolation_segment.create(name: 'bommel') + key_name = 'a' * 64 + truncated_key_name = 'a' * 63 + + a1 = annotation.create(resource_guid: i1.guid, key_name: key_name, value: 'some_value') + + expect { Sequel::Migrator.run(db, migration_to_test, 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, migration_to_test, 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 + + # 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, migration_to_test, 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 + + # 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, migration_to_test, allow_missing_migration_files: true) }.not_to raise_error + + 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 + + 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 + annotation.create(resource_guid: i1.guid, key_name: key, value: 'v1') + # In case key_prefix is set + annotation.create(resource_guid: i2.guid, key_prefix: 'bommel', key_name: key, value: 'v1') + + expect { Sequel::Migrator.run(db, migration_to_test, allow_missing_migration_files: true) }.not_to raise_error + + 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 + + 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 + + expect { Sequel::Migrator.run(db, migration_to_test, 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) + 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 + + # 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') + + 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, migration_to_test, 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 + key_b = 'b' * 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') + + expect { Sequel::Migrator.run(db, migration_to_test, 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') + + expect { Sequel::Migrator.run(db, migration_to_test, allow_missing_migration_files: true) }.not_to raise_error + + 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 + + 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 + + expect { Sequel::Migrator.run(db, migration_to_test, 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) + end + end +end