From 4303e4f795be9d377982cb6b15f1402da99cfff9 Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Fri, 10 May 2019 10:13:05 +1000 Subject: [PATCH] fix: duplicate key value violates unique constraint uq_ver_ppt_ord --- .../20190509_create_version_sequence.rb | 8 +++ .../20190510_set_version_sequence.rb | 9 +++ .../set_latest_version_sequence_value.rb | 29 ++++++++ lib/pact_broker/db/migrate_data.rb | 1 + lib/pact_broker/domain/order_versions.rb | 15 +++- lib/pact_broker/verifications/sequence.rb | 2 - lib/pact_broker/versions/sequence.rb | 38 +++++++++++ .../set_latest_version_sequence_value_spec.rb | 68 +++++++++++++++++++ .../pact_broker/domain/order_versions_spec.rb | 5 +- .../pact_broker/versions/repository_spec.rb | 2 +- spec/support/database_cleaner.rb | 12 +++- 11 files changed, 180 insertions(+), 9 deletions(-) create mode 100644 db/migrations/20190509_create_version_sequence.rb create mode 100644 db/migrations/20190510_set_version_sequence.rb create mode 100644 lib/pact_broker/db/data_migrations/set_latest_version_sequence_value.rb create mode 100644 lib/pact_broker/versions/sequence.rb create mode 100644 spec/lib/pact_broker/db/data_migrations/set_latest_version_sequence_value_spec.rb diff --git a/db/migrations/20190509_create_version_sequence.rb b/db/migrations/20190509_create_version_sequence.rb new file mode 100644 index 000000000..3c5771c32 --- /dev/null +++ b/db/migrations/20190509_create_version_sequence.rb @@ -0,0 +1,8 @@ +require 'pact_broker/db/data_migrations/set_latest_version_sequence_value' +Sequel.migration do + change do + create_table(:version_sequence_number) do + Integer :value, null: false + end + end +end diff --git a/db/migrations/20190510_set_version_sequence.rb b/db/migrations/20190510_set_version_sequence.rb new file mode 100644 index 000000000..26aab63bb --- /dev/null +++ b/db/migrations/20190510_set_version_sequence.rb @@ -0,0 +1,9 @@ +require 'pact_broker/db/data_migrations/set_latest_version_sequence_value' +Sequel.migration do + up do + PactBroker::DB::DataMigrations::SetLatestVersionSequenceValue.call(self) + end + + down do + end +end diff --git a/lib/pact_broker/db/data_migrations/set_latest_version_sequence_value.rb b/lib/pact_broker/db/data_migrations/set_latest_version_sequence_value.rb new file mode 100644 index 000000000..9491fec72 --- /dev/null +++ b/lib/pact_broker/db/data_migrations/set_latest_version_sequence_value.rb @@ -0,0 +1,29 @@ +module PactBroker + module DB + module DataMigrations + class SetLatestVersionSequenceValue + def self.call connection + if columns_exist?(connection) + max_order = connection[:versions].max(:order) || 0 + sequence_row = connection[:version_sequence_number].first + if sequence_row.nil? || sequence_row[:value] <= max_order + new_value = max_order + 100 + connection[:version_sequence_number].insert(value: new_value) + # Make sure there is only ever one row in case there is a race condition + connection[:version_sequence_number].exclude(value: new_value).delete + end + end + end + + def self.columns_exist?(connection) + column_exists?(connection, :versions, :order) && + column_exists?(connection, :version_sequence_number, :value) + end + + def self.column_exists?(connection, table, column) + connection.table_exists?(table) && connection.schema(table).find{|col| col.first == column } + end + end + end + end +end diff --git a/lib/pact_broker/db/migrate_data.rb b/lib/pact_broker/db/migrate_data.rb index 4eaad71d4..7880c08d9 100644 --- a/lib/pact_broker/db/migrate_data.rb +++ b/lib/pact_broker/db/migrate_data.rb @@ -16,6 +16,7 @@ class MigrateData def self.call database_connection, options = {} DataMigrations::SetPacticipantIdsForVerifications.call(database_connection) DataMigrations::SetConsumerIdsForPactPublications.call(database_connection) + DataMigrations::SetLatestVersionSequenceValue.call(database_connection) end end end diff --git a/lib/pact_broker/domain/order_versions.rb b/lib/pact_broker/domain/order_versions.rb index bed5f388c..9edff6056 100644 --- a/lib/pact_broker/domain/order_versions.rb +++ b/lib/pact_broker/domain/order_versions.rb @@ -1,13 +1,26 @@ require 'pact_broker/configuration' +require 'pact_broker/versions/sequence' module PactBroker module Domain class OrderVersions - include PactBroker::Logging def self.call new_version new_version.lock! + + if PactBroker.configuration.order_versions_by_date + set_sequential_order(new_version) + else + set_semantic_order(new_version) + end + end + + def self.set_sequential_order(new_version) + set_order new_version, PactBroker::Versions::Sequence.next_val + end + + def self.set_semantic_order(new_version) order_set = false PactBroker::Domain::Version.for_update.where(pacticipant_id: new_version.pacticipant_id).exclude(order: nil).reverse(:order).each do | existing_version | diff --git a/lib/pact_broker/verifications/sequence.rb b/lib/pact_broker/verifications/sequence.rb index 38996d53f..99e3992f6 100644 --- a/lib/pact_broker/verifications/sequence.rb +++ b/lib/pact_broker/verifications/sequence.rb @@ -1,10 +1,8 @@ - require 'sequel' module PactBroker module Verifications class Sequence < Sequel::Model(:verification_sequence_number) - dataset_module do # The easiest way to implement a cross database compatible sequence. # Sad, I know. diff --git a/lib/pact_broker/versions/sequence.rb b/lib/pact_broker/versions/sequence.rb new file mode 100644 index 000000000..e6a65e816 --- /dev/null +++ b/lib/pact_broker/versions/sequence.rb @@ -0,0 +1,38 @@ +require 'sequel' + +module PactBroker + module Versions + class Sequence < Sequel::Model(:version_sequence_number) + + dataset_module do + # The easiest way to implement a cross database compatible sequence. + # Sad, I know. + def next_val + db.transaction do + for_update.first + select_all.update(value: Sequel[:value]+1) + row = first + if row + row.value + else + # The first row should have been created in the migration, so this code + # should only ever be executed in a test context, after we've truncated all the + # tables after a test. + # There would be a risk of a race condition creating two rows if this + # code executed in prod, as I don't think you can lock an empty table + # to prevent another record being inserted. + max_version_order = PactBroker::Domain::Version.max(:order) + value = max_version_order ? max_version_order + 100 : 1 + insert(value: value) + value + end + end + end + end + end + end +end + +# Table: version_sequence_number +# Columns: +# value | integer | NOT NULL diff --git a/spec/lib/pact_broker/db/data_migrations/set_latest_version_sequence_value_spec.rb b/spec/lib/pact_broker/db/data_migrations/set_latest_version_sequence_value_spec.rb new file mode 100644 index 000000000..0a6e4fb93 --- /dev/null +++ b/spec/lib/pact_broker/db/data_migrations/set_latest_version_sequence_value_spec.rb @@ -0,0 +1,68 @@ +require 'pact_broker/db/data_migrations/set_latest_version_sequence_value' + +module PactBroker + module DB + module DataMigrations + describe SetLatestVersionSequenceValue, data_migration: true do + include MigrationHelpers + + describe ".call" do + before (:all) do + PactBroker::Database.migrate(20190509) + end + + let(:now) { DateTime.new(2018, 2, 2) } + + subject { SetLatestVersionSequenceValue.call(database) } + + context "when there is no sequence value set" do + context "when there are no versions" do + it "initializes the sequence value - this is required at start up each time in case someone has changed the ordering configuration (date vs semantic)" do + subject + expect(database[:version_sequence_number].first[:value]).to eq 100 + end + end + + context "when there are pre-existing versions" do + let!(:consumer) { create(:pacticipants, {name: 'Consumer', created_at: now, updated_at: now}) } + let!(:consumer_version) { create(:versions, {number: '1.2.3', order: 1, pacticipant_id: consumer[:id], created_at: now, updated_at: now}) } + let!(:consumer_version) { create(:versions, {number: '1.2.5', order: 3, pacticipant_id: consumer[:id], created_at: now, updated_at: now}) } + + it "initializes the sequence value to the max version order with a margin of error" do + subject + expect(database[:version_sequence_number].first[:value]).to eq 103 + end + end + end + + context "when a value already exists and it is already higher than the max_order" do + before do + database[:version_sequence_number].insert(value: 5) + end + + it "does not update the value" do + subject + expect(database[:version_sequence_number].first[:value]).to eq 5 + expect(database[:version_sequence_number].count).to eq 1 + end + end + + context "when a value already exists and it not higher than the max_order" do + before do + database[:version_sequence_number].insert(value: 3) + end + + let!(:consumer) { create(:pacticipants, {name: 'Consumer', created_at: now, updated_at: now}) } + let!(:consumer_version) { create(:versions, {number: '1.2.3', order: 1, pacticipant_id: consumer[:id], created_at: now, updated_at: now}) } + let!(:consumer_version) { create(:versions, {number: '1.2.5', order: 3, pacticipant_id: consumer[:id], created_at: now, updated_at: now}) } + + it "updates the value" do + subject + expect(database[:version_sequence_number].first[:value]).to eq 103 + end + end + end + end + end + end +end diff --git a/spec/lib/pact_broker/domain/order_versions_spec.rb b/spec/lib/pact_broker/domain/order_versions_spec.rb index 287c8e870..983fa4c98 100644 --- a/spec/lib/pact_broker/domain/order_versions_spec.rb +++ b/spec/lib/pact_broker/domain/order_versions_spec.rb @@ -24,7 +24,7 @@ end end - context "when order_versions_by_date is true (not recommended)" do + context "when order_versions_by_date is true (recommended)" do before do allow(PactBroker.configuration).to receive(:order_versions_by_date).and_return(true) end @@ -69,7 +69,6 @@ end context "when the new version is considered to be earlier than the previous latest version" do - before do Sequel::Model.db[:versions].where(number: '2').update(number: 'z') Sequel::Model.db[:versions].where(number: '3').update(number: 'a') @@ -80,8 +79,6 @@ PactBroker::Domain::Version.create(number: '2', pacticipant_id: consumer.id) expect(ordered_versions).to eq(['1', 'z', 'a', '2', '4']) end - end end - end diff --git a/spec/lib/pact_broker/versions/repository_spec.rb b/spec/lib/pact_broker/versions/repository_spec.rb index 4fad8eb83..ddcdae004 100644 --- a/spec/lib/pact_broker/versions/repository_spec.rb +++ b/spec/lib/pact_broker/versions/repository_spec.rb @@ -93,7 +93,7 @@ module Versions expect(subject.number).to eq version_number expect(subject.pacticipant.name).to eq pacticipant_name expect(subject.tags.first.name).to eq "prod" - expect(subject.order).to eq 0 + expect(subject.order).to_not be nil end context "when case sensitivity is turned off and names with different cases are used" do diff --git a/spec/support/database_cleaner.rb b/spec/support/database_cleaner.rb index 005f9f022..0198d1cda 100644 --- a/spec/support/database_cleaner.rb +++ b/spec/support/database_cleaner.rb @@ -3,7 +3,7 @@ RSpec.configure do |config| - config.include MigrationHelpers, migration: true + config.include MigrationHelpers, migration: true, data_migration: true config.before(:suite) do if defined?(::DB) @@ -20,11 +20,21 @@ PactBroker::Database.drop_objects end + config.after :each, migration: true do PactBroker::Database.migrate PactBroker::Database.truncate end + config.after :each, data_migration: true do + PactBroker::Database.truncate + end + + config.after :all, data_migration: true do + PactBroker::Database.migrate + PactBroker::Database.truncate + end + config.before(:each) do | example | unless example.metadata[:no_db_clean] || example.metadata[:migration] DatabaseCleaner.start if defined?(::DB)