From b8f029eea63c755ec67e67dbcf0b5bbe2f7dcf8f Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Fri, 23 Oct 2020 18:54:58 +1100 Subject: [PATCH] feat: use a sequence for the verification number on postgres --- ...023_create_verification_number_sequence.rb | 20 ++++++ lib/db.rb | 4 ++ lib/pact_broker/verifications/sequence.rb | 38 +++++----- .../verifications/sequence_spec.rb | 70 +++++++++++++------ tasks/database.rb | 7 ++ 5 files changed, 101 insertions(+), 38 deletions(-) create mode 100644 db/migrations/20201023_create_verification_number_sequence.rb diff --git a/db/migrations/20201023_create_verification_number_sequence.rb b/db/migrations/20201023_create_verification_number_sequence.rb new file mode 100644 index 000000000..299b4a36c --- /dev/null +++ b/db/migrations/20201023_create_verification_number_sequence.rb @@ -0,0 +1,20 @@ +require_relative 'migration_helper' + +Sequel.migration do + up do + if PactBroker::MigrationHelper.postgres? + row = from(:verification_sequence_number).select(:value).limit(1).first + start = row ? row[:value] + 1 : 1 + run("CREATE SEQUENCE verification_number_sequence START WITH #{start}") + end + end + + down do + if PactBroker::MigrationHelper.postgres? + nextval = execute("SELECT nextval('verification_number_sequence') as val") { |v| v.first["val"].to_i } + # Add a safety margin! + from(:verification_sequence_number).update(value: nextval + 100) + run("DROP SEQUENCE verification_number_sequence") + end + end +end diff --git a/lib/db.rb b/lib/db.rb index 6020f425b..ef1c1676b 100644 --- a/lib/db.rb +++ b/lib/db.rb @@ -60,6 +60,10 @@ def self.mysql? !!(PACT_BROKER_DB.adapter_scheme.to_s =~ /mysql/) end + def self.postgres? + !!(PACT_BROKER_DB.adapter_scheme.to_s == "postgres") + end + PACT_BROKER_DB ||= connection_for_env ENV.fetch('RACK_ENV') def self.health_check diff --git a/lib/pact_broker/verifications/sequence.rb b/lib/pact_broker/verifications/sequence.rb index 99e3992f6..02da1c8ab 100644 --- a/lib/pact_broker/verifications/sequence.rb +++ b/lib/pact_broker/verifications/sequence.rb @@ -1,4 +1,6 @@ require 'sequel' +require 'pact_broker/repositories/helpers' + module PactBroker module Verifications @@ -7,22 +9,26 @@ class Sequence < Sequel::Model(:verification_sequence_number) # 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. - # 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_verification_number = PactBroker::Domain::Verification.max(:number) - value = max_verification_number ? max_verification_number + 100 : 1 - insert(value: value) - value + if PactBroker::Repositories::Helpers.postgres? + db.execute("SELECT nextval('verification_number_sequence') as val") { |v| v.first["val"].to_i } + else + 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. + # 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_verification_number = PactBroker::Domain::Verification.max(:number) + value = max_verification_number ? max_verification_number + 100 : 1 + insert(value: value) + value + end end end end diff --git a/spec/lib/pact_broker/verifications/sequence_spec.rb b/spec/lib/pact_broker/verifications/sequence_spec.rb index 5a51ea69c..acbaa5941 100644 --- a/spec/lib/pact_broker/verifications/sequence_spec.rb +++ b/spec/lib/pact_broker/verifications/sequence_spec.rb @@ -4,41 +4,67 @@ module PactBroker module Verifications describe Sequence do describe "#next_val", migration: true do + context "for proper databases with proper sequences", skip: !::DB.postgres? do + it "increments the value each time" do + PactBroker::Database.migrate + expect(Sequence.next_val).to eq 101 + expect(Sequence.next_val).to eq 102 + end - before do - PactBroker::Database.migrate - end - - context "when there is a row in the verification_sequence_number table" do - before do - Sequence.select_all.delete - Sequence.insert(value: 1) + it "can rollback without duplicating a sequence number" do + PactBroker::Database.migrate + row = database.from(:verification_sequence_number).select(:value).limit(1).first + expect(row[:value]).to eq 100 + Sequence.next_val + PactBroker::Database.migrate(20201006) + row = database.from(:verification_sequence_number).select(:value).limit(1).first + expect(row[:value]).to eq 202 end - it "increments the value and returns it" do - expect(Sequence.next_val).to eq 2 + it "can deal with there not being an existing value in the verification_sequence_number table" do + PactBroker::Database.migrate(20201006) + database.from(:verification_sequence_number).delete + PactBroker::Database.migrate + expect(Sequence.next_val).to eq 1 end end - context "when there is no row in the verification_sequence_number table and no existing verifications" do + context "for databases without sequences", skip: ::DB.postgres? do before do - Sequence.select_all.delete + PactBroker::Database.migrate end - it "inserts and returns the value 1" do - expect(Sequence.next_val).to eq 1 + context "when there is a row in the verification_sequence_number table" do + before do + Sequence.select_all.delete + Sequence.insert(value: 1) + end + + it "increments the value and returns it" do + expect(Sequence.next_val).to eq 2 + end end - end - context "when there is no row in the verification_sequence_number table and there are existing verifications" do - before do - Sequence.select_all.delete - TestDataBuilder.new.create_pact_with_hierarchy("A", "1", "B") - .create_verification(provider_version: "2") + context "when there is no row in the verification_sequence_number table and no existing verifications" do + before do + Sequence.select_all.delete + end + + it "inserts and returns the value 1" do + expect(Sequence.next_val).to eq 1 + end end - it "inserts a number that is guaranteed to be higher than any of the existing verification numbers from when we tried to do this without a sequence" do - expect(Sequence.next_val).to eq 101 + context "when there is no row in the verification_sequence_number table and there are existing verifications" do + before do + Sequence.select_all.delete + TestDataBuilder.new.create_pact_with_hierarchy("A", "1", "B") + .create_verification(provider_version: "2") + end + + it "inserts a number that is guaranteed to be higher than any of the existing verification numbers from when we tried to do this without a sequence" do + expect(Sequence.next_val).to eq 101 + end end end end diff --git a/tasks/database.rb b/tasks/database.rb index a5edcb758..f714bfdad 100644 --- a/tasks/database.rb +++ b/tasks/database.rb @@ -35,6 +35,7 @@ def ensure_database_dir_exists def drop_objects drop_views drop_tables + drop_sequences end def drop_tables @@ -56,6 +57,12 @@ def drop_views end end + def drop_sequences + if psql? + database.run('DROP SEQUENCE verification_number_sequence') + end + end + def create if psql? system('psql postgres -c "create database pact_broker"')