fix: gracefully handle race conditions when publishing a new revision of a pact

Author: bethesque

lib/pact_broker/pacts/repository.rb

@@ -35,20 +35,30 @@ def update id, params
         existing_model = PactPublication.find(id: id)
         pact_version = find_or_create_pact_version(existing_model.consumer_version.pacticipant_id, existing_model.provider_id, params[:json_content])
         if existing_model.pact_version_id != pact_version.id
-          pact_publication = PactPublication.new(
+          key = {
             consumer_version_id: existing_model.consumer_version_id,
-            consumer_id: existing_model.consumer_id,
             provider_id: existing_model.provider_id,
-            revision_number: (existing_model.revision_number + 1),
-            pact_version: pact_version,
-          ).save
+            revision_number: next_revision_number(existing_model),
+          }
+          new_params = key.merge(
+            consumer_id: existing_model.consumer_id,
+            pact_version_id: pact_version.id,
+            created_at: Sequel.datetime_class.now
+          )
+          PactPublication.upsert(new_params, key.keys)
+          pact_publication = PactPublication.where(key).single_record
           update_latest_pact_publication_ids(pact_publication)
           pact_publication.to_domain
         else
           existing_model.to_domain
         end
       end
 
+      # This logic is a separate method so we can stub it to create a "conflict" scenario
+      def next_revision_number(existing_model)
+        existing_model.revision_number + 1
+      end
+
       def update_latest_pact_publication_ids(pact_publication)
         params = {
           consumer_version_id: pact_publication.consumer_version_id,

lib/pact_broker/repositories/helpers.rb

@@ -36,11 +36,12 @@ def select_for_subquery column
         end
       end
 
-      def upsert row, key_names
+      def upsert row, key_names, columns_to_update = nil
         if postgres?
           insert_conflict(update: row, target: key_names).insert(row)
         elsif mysql?
-          on_duplicate_key_update.insert(row)
+          update_cols = columns_to_update || (row.keys - key_names)
+          on_duplicate_key_update(*update_cols).insert(row)
         else
           # Sqlite
           key = row.reject{ |k, v| !key_names.include?(k) }

spec/lib/pact_broker/pacts/repository_spec.rb

@@ -139,6 +139,7 @@ module Pacts
         let(:existing_pact) do
           TestDataBuilder.new.create_pact_with_hierarchy("A Consumer", "1.2.3", "A Provider", original_json_content).and_return(:pact)
         end
+        let(:repository) { Repository.new }
 
         before do
           ::DB::PACT_BROKER_DB[:pact_publications]
@@ -155,10 +156,8 @@ module Pacts
         let(:original_json_content) { {some: 'json'}.to_json }
         let(:json_content) { {some_other: 'json'}.to_json }
 
-
         context "when the attributes have changed" do
-
-          subject { Repository.new.update existing_pact.id, json_content: json_content }
+          subject { repository.update existing_pact.id, json_content: json_content }
 
           it "creates a new PactVersion" do
             expect { subject }.to change{ PactBroker::Pacts::PactPublication.count }.by(1)
@@ -186,11 +185,27 @@ module Pacts
           it "increments the revision_number by 1" do
             expect(subject.revision_number).to eq 2
           end
+
+          context "when there is a race condition" do
+            before do
+              allow(repository).to receive(:next_revision_number) { | existing_pact | existing_pact.revision_number }
+            end
+
+            it "updates the existing row - yes this is destructive, by MySQL not supporting inner queries stops us doing a SELECT revision_number + 1" do
+              # And if we're conflicting the time between the two publications is so small that nobody
+              # can have depended on the content of the first pact
+              expect { subject }.to_not change{ PactBroker::Pacts::PactPublication.count }
+            end
+
+            it "sets the content to the new content" do
+              expect(subject.json_content).to eq json_content
+            end
+          end
         end
 
         context "when the content has not changed" do
 
-          subject { Repository.new.update existing_pact.id, json_content: original_json_content }
+          subject { repository.update existing_pact.id, json_content: original_json_content }
 
           it "does not create a new PactVersion" do
             expect { subject }.to_not change{ PactBroker::Pacts::PactPublication.count }
@@ -217,6 +232,7 @@ module Pacts
             .create_consumer_version("1.2.3")
             .create_provider(provider_name)
             .create_pact
+            .create_webhook
             .revise_pact
             .create_consumer_version("2.3.4")
             .create_pact