From 412c4289dcc839162cb78bbb2057ab497006c76d Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Mon, 1 May 2023 11:31:04 +1000 Subject: [PATCH] fix(pacts for verification): do not allow empty string for provider version branch when it is required for calculating WIP/pending pacts --- ...acts_for_verification_json_query_schema.rb | 7 +- lib/pact_broker/locale/en.yml | 2 +- ...for_verification_json_query_schema_spec.rb | 130 ++++++++++++++++++ 3 files changed, 133 insertions(+), 6 deletions(-) diff --git a/lib/pact_broker/api/contracts/pacts_for_verification_json_query_schema.rb b/lib/pact_broker/api/contracts/pacts_for_verification_json_query_schema.rb index 563436324..25f2661d2 100644 --- a/lib/pact_broker/api/contracts/pacts_for_verification_json_query_schema.rb +++ b/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 diff --git a/lib/pact_broker/locale/en.yml b/lib/pact_broker/locale/en.yml index 8855e2350..5c4b4c324 100644 --- a/lib/pact_broker/locale/en.yml +++ b/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}'" diff --git a/spec/lib/pact_broker/api/contracts/pacts_for_verification_json_query_schema_spec.rb b/spec/lib/pact_broker/api/contracts/pacts_for_verification_json_query_schema_spec.rb index 80b36d572..79aa74022 100644 --- a/spec/lib/pact_broker/api/contracts/pacts_for_verification_json_query_schema_spec.rb +++ b/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