Skip to content

Commit

Permalink
Merge pull request #3394 from cloudfoundry/label_annotation_uniqueness
Browse files Browse the repository at this point in the history
Ensure uniqueness of labels and annotations
  • Loading branch information
FloThinksPi authored Jan 5, 2024
2 parents 9ad0819 + 488d876 commit 56e78e1
Show file tree
Hide file tree
Showing 8 changed files with 415 additions and 74 deletions.
8 changes: 2 additions & 6 deletions .rubocop_cc.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
11 changes: 7 additions & 4 deletions app/actions/labels_update.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Expand Down
6 changes: 2 additions & 4 deletions app/fetchers/label_selector_query_generator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 6 additions & 6 deletions app/models/helpers/metadata_model_mixin.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
56 changes: 28 additions & 28 deletions db/migrations/20231221123000_rename_annotations_key_column.rb
Original file line number Diff line number Diff line change
@@ -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
Expand Down
126 changes: 126 additions & 0 deletions db/migrations/20240102150000_add_annotation_label_uniqueness.rb
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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)
Expand Down
Loading

0 comments on commit 56e78e1

Please sign in to comment.