feat(pacticipant versions): do not allow branch to be modified once set

lib/pact_broker/api/resources/version.rb

@@ -11,23 +11,46 @@ def content_types_provided
         end
 
         def content_types_accepted
-          [["application/json", :from_json]]
+          [
+            ["application/json", :from_json],
+            ["application/merge-patch+json", :from_merge_patch_json]
+          ]
         end
 
         def allowed_methods
-          ["GET", "PUT", "DELETE", "OPTIONS"]
+          ["GET", "PUT", "PATCH", "DELETE", "OPTIONS"]
+        end
+
+        def is_conflict?
+          if (errors = version_service.conflict_errors(version, parsed_version, resource_url)).any?
+            set_json_validation_error_messages(errors)
+          else
+            false
+          end
         end
 
         def resource_exists?
           !!version
         end
 
         def from_json
-          response_code = version ? 200 : 201
-          parsed_version = Decorators::VersionDecorator.new(OpenStruct.new).from_json(request_body)
-          @version = version_service.create_or_overwrite(pacticipant_name, pacticipant_version_number, parsed_version)
-          response.body = to_json
-          response_code
+          if request.really_put?
+            handle_request do
+              version_service.create_or_overwrite(pacticipant_name, pacticipant_version_number, parsed_version)
+            end
+          else
+            415
+          end
+        end
+
+        def from_merge_patch_json
+          if request.patch?
+            handle_request do
+              version_service.create_or_update(pacticipant_name, pacticipant_version_number, parsed_version)
+            end
+          else
+            415
+          end
         end
 
         def to_json
@@ -45,6 +68,17 @@ def policy_name
 
         private
 
+        def handle_request
+          response_code = version ? 200 : 201
+          @version = yield
+          response.body = to_json
+          response_code
+        end
+
+        def parsed_version
+          @parsed_version ||= Decorators::VersionDecorator.new(OpenStruct.new).from_json(request_body)
+        end
+
         def environments
           @environments ||= environment_service.find_for_pacticipant(version.pacticipant)
         end

lib/pact_broker/locale/en.yml

@@ -52,6 +52,7 @@ en:
         cannot_specify_tag_and_environment: Cannot specify both a 'to' tag and an environment.
         cannot_specify_latest_and_environment: Cannot specify both latest=true and an environment.
         environment_with_name_not_found: "Environment with name '%{name}' does not exist"
+        cannot_modify_version_branch: "The branch for a pacticipant version cannot be changed once set (currently '%{old_branch}', proposed value '%{new_branch}'). Your pact publication/verification publication configurations may be in conflict with each other if you are seeing this error. If the current branch value is incorrect, you must delete the pacticipant version resource at %{version_url} and publish the pacts/verification results again with a consistent branch name."
       duplicate_pacticipant: |
         This is the first time a pact has been published for "%{new_name}".
         The name "%{new_name}" is very similar to the following existing consumers/providers:

lib/pact_broker/versions/repository.rb

@@ -61,6 +61,27 @@ def create args
         PactBroker::Domain::Version.new(version_params).upsert
       end
 
+      def create_or_update(pacticipant, version_number, open_struct_version)
+        saved_version = PactBroker::Domain::Version.where(pacticipant_id: pacticipant.id, number: version_number).single_record
+        params = open_struct_version.to_h
+        tags = params.delete(:tags)
+        if saved_version
+          saved_version.update(params)
+        else
+          # Upsert is only for race conditions
+          # Upsert blanks out any fields that are not provided
+          saved_version = PactBroker::Domain::Version.new(
+            open_struct_version.to_h.merge(
+              pacticipant_id: pacticipant,
+              number: version_number
+            )
+          ).upsert
+        end
+
+        replace_tags(saved_version, tags) if tags
+        saved_version
+      end
+
       def create_or_overwrite(pacticipant, version_number, open_struct_version)
         saved_version = PactBroker::Domain::Version.new(
           number: version_number,
@@ -70,16 +91,20 @@ def create_or_overwrite(pacticipant, version_number, open_struct_version)
         ).upsert
 
         if open_struct_version.tags
-          tag_repository.delete_by_version_id(saved_version.id)
-          open_struct_version.tags.collect do | open_struct_tag |
-            tag_repository.create(version: saved_version, name: open_struct_tag.name)
-          end
-          saved_version.refresh
+          replace_tags(saved_version, open_struct_version.tags)
         end
 
         saved_version
       end
 
+      def replace_tags(saved_version, open_struct_tags)
+        tag_repository.delete_by_version_id(saved_version.id)
+        open_struct_tags.collect do | open_struct_tag |
+          tag_repository.create(version: saved_version, name: open_struct_tag.name)
+        end
+        saved_version.refresh
+      end
+
       def find_by_pacticipant_id_and_number_or_create pacticipant_id, number
         if version = find_by_pacticipant_id_and_number(pacticipant_id, number)
           version

lib/pact_broker/versions/service.rb

@@ -1,11 +1,26 @@
 require 'pact_broker/repositories'
+require 'pact_broker/messages'
 
 module PactBroker
   module Versions
     class Service
-
+      extend PactBroker::Messages
       extend PactBroker::Repositories
 
+      def self.conflict_errors(existing_version, open_struct_version, version_url)
+        if existing_version&.branch && open_struct_version.to_h.key?(:branch) && existing_version.branch != open_struct_version.branch
+          message_params = {
+            old_branch: existing_version&.branch,
+            new_branch: open_struct_version.branch,
+            version_url: version_url
+          }
+          error_message = message("errors.validation.cannot_modify_version_branch", message_params).tap { |it| puts it }
+          { branch: [error_message] }
+        else
+          {}
+        end
+      end
+
       def self.find_latest_by_pacticpant_name params
         version_repository.find_latest_by_pacticpant_name params.fetch(:pacticipant_name)
       end
@@ -23,6 +38,11 @@ def self.create_or_overwrite(pacticipant_name, version_number, version)
         version_repository.create_or_overwrite(pacticipant, version_number, version)
       end
 
+      def self.create_or_update(pacticipant_name, version_number, version)
+        pacticipant = pacticipant_repository.find_by_name_or_create(pacticipant_name)
+        version_repository.create_or_update(pacticipant, version_number, version)
+      end
+
       def self.delete version
         tag_repository.delete_by_version_id version.id
         webhook_repository.delete_triggered_webhooks_by_version_id version.id

spec/features/create_version_spec.rb

@@ -37,25 +37,29 @@
         .create_consumer_version_tag("dev")
     end
 
-    let(:version_hash) { { branch: "new-branch" } }
+    context "when the branch is attempted to be changed" do
+      let(:version_hash) { { branch: "new-branch" } }
 
-    it "returns a 200" do
-      expect(subject.status).to be 200
+      its(:status) { is_expected.to eq 409 }
     end
 
-    it "overwrites the direct properties" do
-      expect(response_body[:branch]).to eq "new-branch"
-      expect(response_body).to_not have_key(:buildUrl)
+    context "when the branch is not attempted to be changed" do
+      let(:version_hash) { { branch: "original-branch" } }
+
+      it "overwrites the direct properties and blanks out any unprovided ones" do
+        expect(response_body[:branch]).to eq "original-branch"
+        expect(response_body).to_not have_key(:buildUrl)
+      end
     end
 
-    context "when not tags are specified" do
+    context "when no tags are specified" do
       it "does not change the tags" do
         expect { subject }.to_not change { PactBroker::Domain::Version.for("Foo", "1234").tags }
       end
     end
 
     context "when tags are specified" do
-      let(:version_hash) { { branch: "new-branch", tags: [ { name: "main" }] } }
+      let(:version_hash) { { branch: "original-branch", tags: [ { name: "main" }] } }
 
       it "overwrites the tags" do
         expect(response_body[:_embedded][:tags].size).to eq 1