From 853354ea84ecd1afc251fa27300687cbdd03b0ae Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bartek=20Bu=C5=82at?= <barthez@users.noreply.github.com>
Date: Sat, 27 Nov 2021 07:28:42 +0100
Subject: [PATCH] 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
---
 docs/configuration.yml                        |  9 +++++++++
 lib/pact_broker/db/clean_incremental.rb       | 19 +++++++++++++++++--
 .../pact_broker/db/clean_incremental_spec.rb  |  8 +++-----
 .../verifications/repository_spec.rb          |  6 +++---
 4 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/docs/configuration.yml b/docs/configuration.yml
index af312ff0d..d94fcf611 100644
--- a/docs/configuration.yml
+++ b/docs/configuration.yml
@@ -100,6 +100,15 @@ groups:
         default_value: 5
         allowed_values_description: A positive integer or float, as a string.
         more_info: https://sequel.jeremyevans.net/rdoc/files/doc/opening_databases_rdoc.html#label-General+connection+options
+      sql_enable_caller_logging:
+        description: |-
+          Whether or not to enable caller_logging extension for database connection.
+          When enabled it logs source path that caused SQL query.
+        default_value: false
+        allowed_values:
+          - true
+          - false
+        more_info: https://sequel.jeremyevans.net/rdoc-plugins/files/lib/sequel/extensions/caller_logging_rb.html
       database_max_connections:
         description: "The maximum size of the connection pool (4 connections by default on most databases)"
         default_value: 4
diff --git a/lib/pact_broker/db/clean_incremental.rb b/lib/pact_broker/db/clean_incremental.rb
index ead40d106..88b5712d5 100644
--- a/lib/pact_broker/db/clean_incremental.rb
+++ b/lib/pact_broker/db/clean_incremental.rb
@@ -92,8 +92,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, pact_version_id: :id)
+          .left_join(:verifications, pact_version_id: :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..bb42e450e 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,14 +84,13 @@ 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
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