fix: ensure publishing a verification does not cause a unique constraint violation

Author: bethesque

db/migrations/20180315_create_verification_sequence.rb

@@ -0,0 +1,14 @@
+Sequel.migration do
+  up do
+    create_table(:verification_sequence_number) do
+      Integer :value, null: false
+    end
+
+    start = (from(:verifications).max(:number) || 0) + 100
+    from(:verification_sequence_number).insert(value: start)
+  end
+
+  down do
+    drop_table(:verification_sequence_number)
+  end
+end

lib/pact_broker/api/resources/verifications.rb

@@ -55,7 +55,7 @@ def pact
         end
 
         def next_verification_number
-          @next_verification_number ||= verification_service.next_number_for(pact)
+          @next_verification_number ||= verification_service.next_number
         end
 
         def decorator_for model

lib/pact_broker/verifications/repository.rb

@@ -2,6 +2,7 @@
 require 'pact_broker/domain/verification'
 require 'pact_broker/verifications/latest_verifications_by_consumer_version'
 require 'pact_broker/verifications/all_verifications'
+require 'pact_broker/verifications/sequence'
 
 module PactBroker
   module Verifications
@@ -10,6 +11,14 @@ class Repository
       include PactBroker::Repositories::Helpers
       include PactBroker::Repositories
 
+      # Ideally this would just be a sequence, but Sqlite and MySQL don't support sequences
+      # in the way we need to use them ie. determining what the next number will be before we
+      # create the record, because Webmachine wants to set the URL of the resource that is about
+      # to be created *before* we actually create it.
+      def next_number
+        Sequence.next_val
+      end
+
       def create verification, provider_version_number, pact
         provider = pacticipant_repository.find_by_name(pact.provider_name)
         version = version_repository.find_by_pacticipant_id_and_number_or_create(provider.id, provider_version_number)
@@ -18,10 +27,6 @@ def create verification, provider_version_number, pact
         verification.save
       end
 
-      def verification_count_for_pact pact
-        PactBroker::Domain::Verification.where(pact_version_id: pact_version_id_for(pact)).count
-      end
-
       def find consumer_name, provider_name, pact_version_sha, verification_number
         PactBroker::Domain::Verification
           .select_all_qualified

lib/pact_broker/verifications/sequence.rb

@@ -0,0 +1,34 @@
+
+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.
+        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
+            end
+          end
+        end
+      end
+    end
+  end
+end

lib/pact_broker/verifications/service.rb

@@ -12,8 +12,8 @@ module Service
       extend PactBroker::Repositories
       extend PactBroker::Services
 
-      def next_number_for pact
-        verification_repository.verification_count_for_pact(pact) + 1
+      def next_number
+        verification_repository.next_number
       end
 
       def create next_verification_number, params, pact

spec/lib/pact_broker/api/resources/verifications_spec.rb

@@ -46,7 +46,7 @@ module Resources
 
             before do
               allow(Pacts::Service).to receive(:find_pact).and_return(pact)
-              allow(PactBroker::Verifications::Service).to receive(:next_number_for).and_return(next_verification_number)
+              allow(PactBroker::Verifications::Service).to receive(:next_number).and_return(next_verification_number)
               allow(PactBroker::Api::Decorators::VerificationDecorator).to receive(:new).and_return(decorator)
             end
 

spec/lib/pact_broker/verifications/repository_spec.rb

@@ -3,35 +3,6 @@
 module PactBroker
   module Verifications
     describe Repository do
-
-      describe "#verification_count_for_pact" do
-        let!(:pact_1) do
-          TestDataBuilder.new
-            .create_consumer("Consumer")
-            .create_provider("Provider")
-            .create_consumer_version("1.2.3")
-            .create_pact
-            .create_verification(number: 1)
-            .create_verification(number: 2)
-            .and_return(:pact)
-        end
-        let!(:pact_2) do
-          TestDataBuilder.new
-            .create_consumer("Foo")
-            .create_provider("Bar")
-            .create_consumer_version("4.5.6")
-            .create_pact
-            .create_verification(number: 1)
-            .and_return(:pact)
-        end
-
-        subject { Repository.new.verification_count_for_pact(pact_1) }
-
-        it "returns the number of verifications for the given pact" do
-          expect(subject).to eq 2
-        end
-      end
-
       describe "#find" do
         let!(:pact) do
           builder = TestDataBuilder.new

spec/lib/pact_broker/verifications/sequence_spec.rb

@@ -0,0 +1,47 @@
+require 'pact_broker/verifications/sequence'
+
+module PactBroker
+  module Verifications
+    describe Sequence do
+      describe "#next_val", migration: true do
+
+        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)
+          end
+
+          it "increments the value and returns it" do
+            expect(Sequence.next_val).to eq 2
+          end
+        end
+
+        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
+
+        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
+  end
+end

spec/lib/pact_broker/verifications/service_spec.rb

@@ -8,19 +8,6 @@ module Verifications
 
       subject { PactBroker::Verifications::Service }
 
-      describe "#next_number_for" do
-
-        let(:pact) { double(:pact) }
-
-        before do
-          allow_any_instance_of(PactBroker::Verifications::Repository).to receive(:verification_count_for_pact).and_return(2)
-        end
-
-        it "returns the number for the next build to be recorded for a pact" do
-          expect(subject.next_number_for(pact)).to eq 3
-        end
-      end
-
       describe "#create" do
         let(:params) { {'success' => true, 'providerApplicationVersion' => '4.5.6'} }
         let(:pact) { TestDataBuilder.new.create_pact_with_hierarchy.and_return(:pact) }