From 88abb2cfc06c73c9257dbd0aea339734b0d7b0c8 Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Thu, 28 Apr 2022 10:11:09 +1000 Subject: [PATCH] feat: timeout long running pact content diff requests (#555) --- docs/CONFIGURATION.md | 9 +++++ docs/configuration.yml | 5 +++ .../api/resources/pact_content_diff.rb | 9 +++-- .../config/runtime_configuration.rb | 3 +- .../pacts/create_formatted_diff.rb | 1 + spec/features/get_diff_spec.rb | 6 +--- .../api/resources/pact_content_diff_spec.rb | 36 +++++++++++++++++++ 7 files changed, 61 insertions(+), 8 deletions(-) create mode 100644 spec/lib/pact_broker/api/resources/pact_content_diff_spec.rb diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index f0eee7ea3..fff5a9fcc 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -655,6 +655,15 @@ with a unique version number. **Allowed values:** `true`, `false`
**More information:** https://docs.pact.io/versioning
+### pact_content_diff_timeout + +The maximum amount of time in seconds to attempt to generate the diff between two pacts before aborting the request. This is required due to performance issues in the underlying diff generation code. + +**Supported versions:** From 2.99.0
+**Environment variable name:** `PACT_BROKER_PACT_CONTENT_DIFF_TIMEOUT`
+**YAML configuration key name:** `pact_content_diff_timeout`
+**Default:** `15`
+
## Miscellaneous diff --git a/docs/configuration.yml b/docs/configuration.yml index 1824cd925..dbf93d63c 100644 --- a/docs/configuration.yml +++ b/docs/configuration.yml @@ -453,6 +453,11 @@ groups: - true - false more_info: https://docs.pact.io/versioning + pact_content_diff_timeout: + description: |- + The maximum amount of time in seconds to attempt to generate the diff between two pacts before aborting the request. This is required due to performance issues in the underlying diff generation code. + default_value: 15 + supported_versions: From 2.99.0 - title: Miscellaneous vars: features: diff --git a/lib/pact_broker/api/resources/pact_content_diff.rb b/lib/pact_broker/api/resources/pact_content_diff.rb index acba7a548..fc82e55fe 100644 --- a/lib/pact_broker/api/resources/pact_content_diff.rb +++ b/lib/pact_broker/api/resources/pact_content_diff.rb @@ -1,6 +1,7 @@ require "pact_broker/api/resources/base_resource" require "pact_broker/pacts/pact_params" require "pact_broker/pacts/diff" +require "timeout" module PactBroker module Api @@ -19,8 +20,12 @@ def resource_exists? end def to_text - output = PactBroker::Pacts::Diff.new.process pact_params.merge(base_url: base_url), comparison_pact_params, raw: false - response.body = output + Timeout::timeout(PactBroker.configuration.pact_content_diff_timeout) do + output = PactBroker::Pacts::Diff.new.process pact_params.merge(base_url: base_url), comparison_pact_params, raw: false + response.body = output + end + rescue Timeout::Error + 408 end def pact diff --git a/lib/pact_broker/config/runtime_configuration.rb b/lib/pact_broker/config/runtime_configuration.rb index 2e1b03398..45d22ec3d 100644 --- a/lib/pact_broker/config/runtime_configuration.rb +++ b/lib/pact_broker/config/runtime_configuration.rb @@ -71,7 +71,8 @@ class RuntimeConfiguration < Anyway::Config badge_provider_mode: :redirect, enable_public_badge_access: false, shields_io_base_url: "https://img.shields.io", - use_case_sensitive_resource_names: true + use_case_sensitive_resource_names: true, + pact_content_diff_timeout: 15 ) # domain attributes diff --git a/lib/pact_broker/pacts/create_formatted_diff.rb b/lib/pact_broker/pacts/create_formatted_diff.rb index 684ec47b8..fdd029913 100644 --- a/lib/pact_broker/pacts/create_formatted_diff.rb +++ b/lib/pact_broker/pacts/create_formatted_diff.rb @@ -19,6 +19,7 @@ def self.call pact_json_content, previous_pact_json_content, raw: false end difference = diff(previous_pact_hash, pact_hash) + Pact::Matchers::UnixDiffFormatter.call(difference, colour: false, include_explanation: false) end end diff --git a/spec/features/get_diff_spec.rb b/spec/features/get_diff_spec.rb index e43779da8..ddd36a6b7 100644 --- a/spec/features/get_diff_spec.rb +++ b/spec/features/get_diff_spec.rb @@ -1,12 +1,10 @@ require "spec/support/test_data_builder" describe "Get diff between versions" do - let(:path) { "/pacts/provider/Provider/consumer/Consumer/version/3/diff/previous-distinct" } - let(:last_response_body) { subject.body } - subject { get path; last_response } + subject { get(path) } let(:pact_content_version_1) do hash = load_json_fixture("consumer-provider.json") @@ -42,12 +40,10 @@ end context "when either version does not exist" do - let(:path) { "/pacts/provider/Provider/consumer/Consumer/versions/1/diff/previous-distinct" } it "returns a 404 response" do expect(subject).to be_a_404_response end - end end diff --git a/spec/lib/pact_broker/api/resources/pact_content_diff_spec.rb b/spec/lib/pact_broker/api/resources/pact_content_diff_spec.rb new file mode 100644 index 000000000..24d312c62 --- /dev/null +++ b/spec/lib/pact_broker/api/resources/pact_content_diff_spec.rb @@ -0,0 +1,36 @@ +require "pact_broker/api/resources/pact_content_diff" + +module PactBroker + module Api + module Resources + describe PactContentDiff do + include_context "stubbed services" + + before do + allow(pact_service).to receive(:find_pact).and_return(pact) + allow(PactBroker::Pacts::Diff).to receive(:new).and_return(diff) + end + + let(:pact) { double("pact") } + let(:diff) { instance_double("PactBroker::Pacts::Diff", process: diff_content) } + let(:diff_content) { "diff_content" } + let(:path) { "/pacts/provider/Provider/consumer/Consumer/version/3/diff/previous-distinct" } + let(:last_response_body) { subject.body } + + subject { get(path) } + + its(:status) { is_expected.to eq 200 } + its(:body) { is_expected.to eq "diff_content" } + + context "when the diff takes too long to generate" do + before do + allow(diff).to receive(:process).and_raise(Timeout::Error) + end + + its(:status) { is_expected.to eq 408 } + its(:body) { is_expected.to eq "" } + end + end + end + end +end