From 6c71e57b1586f30b9dbce4965323f933690636cc Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Thu, 2 Dec 2021 13:04:46 +1100 Subject: [PATCH] fix: fix clean performance fix (#530) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(cleanup): Improve delete orphans SQL query (#527) * fix(cleanup): Improve delete orphans SQL query Original query to delete orphans was terribly slow and was unable to finish within default 15s timeout. Instead of excluding all ids from union, we left join all 3 tables and select only rows that have no joined values in pact_publications or verifications. Additionaly, the query was fixed for mysql. Whenever the Mysql error occures instead of embedded query, ids are fetched and then directly used in delete query. * fix: Add sql_enable_caller_logging documentation * fix: Fix unreliable spec * fix: fully qualify pact_version table joins in database clean Co-authored-by: Bartek Bułat --- lib/pact_broker/db/clean_incremental.rb | 19 ++++++++++++++++-- .../pact_broker/db/clean_incremental_spec.rb | 20 ++++++++++++------- .../verifications/repository_spec.rb | 6 +++--- 3 files changed, 33 insertions(+), 12 deletions(-) diff --git a/lib/pact_broker/db/clean_incremental.rb b/lib/pact_broker/db/clean_incremental.rb index 81dba6e1f..bb42ef570 100644 --- a/lib/pact_broker/db/clean_incremental.rb +++ b/lib/pact_broker/db/clean_incremental.rb @@ -94,8 +94,23 @@ def dry_run? end def delete_orphan_pact_versions - referenced_pact_version_ids = db[:pact_publications].select(:pact_version_id).union(db[:verifications].select(:pact_version_id)) - db[:pact_versions].where(id: referenced_pact_version_ids).invert.delete + db[:pact_versions].where(id: orphan_pact_versions).delete + rescue Sequel::DatabaseError => e + raise unless e.cause.class.name == "Mysql2::Error" + + ids = orphan_pact_versions.map { |row| row[:id] } + db[:pact_versions].where(id: ids).delete + end + + def orphan_pact_versions + db[:pact_versions] + .left_join(:pact_publications, Sequel[:pact_publications][:pact_version_id]=> Sequel[:pact_versions][:id]) + .left_join(:verifications, Sequel[:verifications][:pact_version_id]=> Sequel[:pact_versions][:id]) + .select(Sequel[:pact_versions][:id]) + .where( + Sequel[:pact_publications][:id] => nil, + Sequel[:verifications][:id] => nil + ) end def version_info(version) diff --git a/spec/lib/pact_broker/db/clean_incremental_spec.rb b/spec/lib/pact_broker/db/clean_incremental_spec.rb index ee43421ad..088e6bd3d 100644 --- a/spec/lib/pact_broker/db/clean_incremental_spec.rb +++ b/spec/lib/pact_broker/db/clean_incremental_spec.rb @@ -3,8 +3,7 @@ module PactBroker module DB - # Inner queries don't work on MySQL. Seriously, MySQL??? - xdescribe CleanIncremental do + describe CleanIncremental do def pact_publication_count_for(consumer_name, version_number) PactBroker::Pacts::PactPublication.where(consumer_version: PactBroker::Domain::Version.where_pacticipant_name(consumer_name).where(number: version_number)).count end @@ -85,19 +84,24 @@ def pact_publication_count_for(consumer_name, version_number) expect { subject }.to_not change { PactBroker::Domain::Version.count } end - # Always fails on github actions, never locally :shrug: - it "returns info on what will be deleted", pending: ENV["CI"] == "true" do + # Randomly fails on github actions, never locally :shrug: + it "returns info on what will be deleted", skip: ENV["CI"] == "true" do Approvals.verify(subject, :name => "clean_incremental_dry_run", format: :json) end end end - context "with orphan pact versions" do before do - # Create a pact that will not be deleted - td.create_pact_with_hierarchy("Foo", "0", "Bar", json_content_1) + # json_content_3 referenced by pact_publication for Foo v1 + td.create_pact_with_hierarchy("Foo", "1", "Bar", json_content_3).comment("Foo v1 kept because latest dev") .create_consumer_version_tag("dev") + + # json_content_4 referenced by a verification (but not a pact_publication) + td.create_pact_with_hierarchy("Waffle", "0", "Meep", json_content_4) + .create_verification(provider_version: "5", tag_names: ["dev"], comment: "Meep v5 kept because latest dev") + PactBroker::Pacts::PactPublication.order(:id).last.delete + # Create an orphan pact version pact_version_params = PactBroker::Pacts::PactVersion.first.to_hash pact_version_params.delete(:id) @@ -107,6 +111,8 @@ def pact_publication_count_for(consumer_name, version_number) let(:json_content_1) { { interactions: ["a", "b"]}.to_json } let(:json_content_2) { { interactions: ["a", "c"]}.to_json } + let(:json_content_3) { { interactions: ["a", "f"]}.to_json } + let(:json_content_4) { { interactions: ["a", "h"]}.to_json } let(:options) { { keep: [latest_dev_selector] } } diff --git a/spec/lib/pact_broker/verifications/repository_spec.rb b/spec/lib/pact_broker/verifications/repository_spec.rb index ccc74f257..e714ec6ab 100644 --- a/spec/lib/pact_broker/verifications/repository_spec.rb +++ b/spec/lib/pact_broker/verifications/repository_spec.rb @@ -46,9 +46,9 @@ module Verifications it "creates a PactVersionProviderTagSuccessfulVerification for each tag" do expect { subject }.to change { PactVersionProviderTagSuccessfulVerification.count }.by(2) - expect(PactVersionProviderTagSuccessfulVerification.first).to have_attributes( - wip: false, - provider_version_tag_name: "foo" + expect(PactVersionProviderTagSuccessfulVerification.all).to contain_exactly( + have_attributes(wip: false, provider_version_tag_name: "foo"), + have_attributes(wip: false, provider_version_tag_name: "bar"), ) end end