fix(pacts for verification): do not allow empty string for provider v…

…ersion branch when it is required for calculating WIP/pending pacts

lib/pact_broker/api/contracts/pacts_for_verification_json_query_schema.rb

@@ -6,8 +6,6 @@ module PactBroker
   module Api
     module Contracts
       class PactsForVerificationJSONQuerySchema < BaseContract
-        include PactBroker::Logging
-
         json do
           optional(:providerVersionBranch).maybe(:string)
           optional(:providerVersionTags).maybe(:array?)
@@ -20,7 +18,7 @@ class PactsForVerificationJSONQuerySchema < BaseContract
         # providerVersionBranch at all (most likely unintentionally.)
         # When we added
         #   optional(:providerVersionBranch).filled(:string)
-        # during the dry-validation upgrade, we discovered that many users/pact clients were requesting pacts for verification
+        # during the dry-validation upgrade, we discovered that some users/pact clients were requesting pacts for verification
         # with an empty string in the providerVersionBranch
         # This complicated logic tries to not break those customers as much as possible, while still raising an error
         # when the blank string is most likely a configuration error
@@ -43,8 +41,7 @@ class PactsForVerificationJSONQuerySchema < BaseContract
             # There are no tags, the user specified wip or pending, and they set a branch, but the branch is an empty or blank string...
             if !tags&.any? && (wip || include_pending) && branch && ValidationHelpers.blank?(branch)
               # most likely a user error - return a message
-              #key.failure(validation_message("pacts_for_verification_selector_provider_version_branch_empty"))
-              logger.info("pacts_for_verification_empty_provider_version", values.data)
+              key.failure(validation_message("pacts_for_verification_selector_provider_version_branch_empty"))
             end
           end
 

lib/pact_broker/locale/en.yml

@@ -106,7 +106,7 @@ en:
         pacts_for_verification_selector_deployed_and_released_disallowed: cannot specify both deployed=true and released=true
         pacts_for_verification_selector_deployed_and_deployed_or_released_disallowed: cannot specify both deployed=true and deployedOrReleased=true
         pacts_for_verification_selector_released_and_deployed_or_released_disallowed: cannot specify both released=true and deployedOrReleased=true
-        pacts_for_verification_selector_provider_version_branch_empty: when pending or WIP pacts are enabled and there are no tags provided, the provider version branch must not be an empty string (as this suggests it is a user error in configuration) - a value must be provided (recommended), or it must not be set at all
+        pacts_for_verification_selector_provider_version_branch_empty: when pending or WIP pacts are enabled and there are no tags provided, the provider version branch must not be an empty string, as it is used in the calculations for WIP/pending. A value must be provided (recommended), or it must not be set at all.
         pact_name_in_path_mismatch_name_in_pact: "name in pact '%{name_in_pact}' does not match name in URL path '%{name_in_path}'."
         base64: "is not valid Base64"
         non_utf_8_char_in_contract: "contract content has a non UTF-8 character at char %{char_number} and cannot be parsed as JSON. Fragment preceding invalid character is: '%{fragment}'"

spec/lib/pact_broker/api/contracts/pacts_for_verification_json_query_schema_spec.rb

@@ -22,6 +22,12 @@ module Contracts
 
         subject { PactsForVerificationJSONQuerySchema.(params) }
 
+        context "when nothing is specified" do
+          let(:params) { {} }
+
+          it { is_expected.to be_empty }
+        end
+
         context "when the params are valid" do
           it "has no errors" do
             expect(subject).to eq({})
@@ -391,6 +397,130 @@ module Contracts
 
           its([:providerVersionBranch, 0]) { is_expected.to include "cannot be blank"}
         end
+
+        context "when the providerVersionBranch is an empty string and there are tags provided" do
+          let(:params) do
+            {
+              providerVersionBranch: provider_version_branch,
+              consumerVersionSelectors: consumer_version_selectors,
+              providerVersionTags: provider_version_tags
+            }
+          end
+
+          let(:provider_version_branch) { "" }
+
+          it "does not have any errors because the original schema (before dry-validation upgrade) didn't have any validation for the providerVersionBranch and if we introduce it now, we'll break everyone's builds (we know because we did it :grimace:)" do
+            expect(subject).to be_empty
+          end
+        end
+
+        context "when the providerVersionBranch is an empty string" do
+          let(:params) do
+            {
+              providerVersionBranch: provider_version_branch,
+              consumerVersionSelectors: consumer_version_selectors,
+              includePendingStatus: include_pending_status,
+              includeWipPactsSince: include_wip_pacts_since,
+              providerVersionTags: provider_version_tags
+            }.compact
+          end
+
+          let(:include_pending_status) { false }
+          let(:include_wip_pacts_since) { nil }
+          let(:provider_version_tags) { nil }
+          let(:provider_version_branch) { "" }
+
+          context "when includePendingStatus is true" do
+            let(:include_pending_status) { true }
+
+            context "there are no tags provided" do
+              it "return an error because we need the branch or tags to properly calculate the pending status, and a blank string is most likely user error" do
+                expect(subject[:providerVersionBranch]).to_not be_empty
+              end
+            end
+
+            context "when the tags is an empty array" do
+              let(:provider_version_tags) { [] }
+
+              its([:providerVersionBranch]) { is_expected.to_not be_empty }
+            end
+
+            context "when the tags are provided" do
+              let(:provider_version_tags) { ["foo"] }
+
+              it { is_expected.to_not have_key(:providerVersionBranch) }
+            end
+          end
+
+          context "when includePendingStatus is false" do
+            it "does not return an error because we're not using the branch for anything anyway" do
+              expect(subject).to be_empty
+            end
+          end
+
+          context "when includeWipPactsSince is set" do
+            let(:include_wip_pacts_since) { "2020-01-01" }
+
+            context "there are no tags provided" do
+              it "return an error because we need the branch or tags to properly calculate the pending status, and a blank string is most likely user error" do
+                expect(subject[:providerVersionBranch]).to_not be_empty
+              end
+            end
+
+            context "when the tags is an empty array" do
+              let(:provider_version_tags) { [] }
+
+              its([:providerVersionBranch]) { is_expected.to_not be_empty }
+            end
+
+            context "when the tags are provided" do
+              let(:provider_version_tags) { ["foo"] }
+
+              it { is_expected.to_not have_key(:providerVersionBranch) }
+            end
+          end
+
+          context "when includeWipPactsSince is not set" do
+            it "does not return an error because we're not using the branch for anything anyway" do
+              expect(subject).to be_empty
+            end
+          end
+        end
+
+        context "when the providerVersionBranch is a space" do
+          let(:params) do
+            {
+              providerVersionBranch: provider_version_branch,
+              consumerVersionSelectors: consumer_version_selectors
+            }
+          end
+
+          let(:provider_version_branch) { " " }
+
+          it "does not allow it" do
+            expect(subject[:providerVersionBranch]).to_not be_empty
+          end
+        end
+
+        context "when mainBranch is true, and latest is false" do
+          let(:params) do
+            {
+              consumerVersionSelectors: [ { mainBranch: true, latest: false }]
+            }
+          end
+
+          its([:consumerVersionSelectors, 0]) { is_expected.to match("cannot specify") }
+        end
+
+        context "when mainBranch is true, and latest is true" do
+          let(:params) do
+            {
+              consumerVersionSelectors: [ { mainBranch: true, latest: true }]
+            }
+          end
+
+          it { is_expected.to_not have_key(:consumerVersionSelectors) }
+        end
       end
     end
   end