From df0acfa371c3fa9d4242ef4e39f8acc7826bacef Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Mon, 22 Nov 2021 16:41:45 +1100 Subject: [PATCH] feat: use a separate table to track the successful verifications of pact versions for each provider version tag (feature toggled with "new_wip_calculation") This is due to performance issues with calculating it using a live query on the verifications table. --- ...n_provider_tag_successful_verifications.rb | 23 +++++++++ ...vider_tag_successful_verifications_data.rb | 11 ++++ ...n_provider_tag_successful_verifications.rb | 38 ++++++++++++++ lib/pact_broker/db/migrate_data.rb | 1 + .../pact_publication_wip_dataset_module.rb | 44 ++++++++++++++++ lib/pact_broker/test/test_data_builder.rb | 45 ++++++++++++---- ...on_provider_tag_successful_verification.rb | 11 ++++ lib/pact_broker/verifications/repository.rb | 16 ++++++ ...vider_tag_successful_verifications_spec.rb | 51 +++++++++++++++++++ .../verifications/repository_spec.rb | 11 +++- 10 files changed, 241 insertions(+), 10 deletions(-) create mode 100644 db/migrations/20211120_create_pact_version_provider_tag_successful_verifications.rb create mode 100644 db/migrations/20211121_migrate_pact_version_provider_tag_successful_verifications_data.rb create mode 100644 lib/pact_broker/db/data_migrations/migrate_pact_version_provider_tag_successful_verifications.rb create mode 100644 lib/pact_broker/verifications/pact_version_provider_tag_successful_verification.rb create mode 100644 spec/lib/pact_broker/db/data_migrations/migrate_pact_version_provider_tag_successful_verifications_spec.rb diff --git a/db/migrations/20211120_create_pact_version_provider_tag_successful_verifications.rb b/db/migrations/20211120_create_pact_version_provider_tag_successful_verifications.rb new file mode 100644 index 000000000..4364cd2af --- /dev/null +++ b/db/migrations/20211120_create_pact_version_provider_tag_successful_verifications.rb @@ -0,0 +1,23 @@ +Sequel.migration do + up do + create_table(:pact_version_provider_tag_successful_verifications, charset: "utf8") do + primary_key :id + foreign_key :pact_version_id, :pact_versions, null: false, on_delete: :cascade, foreign_key_constraint_name: "pact_version_provider_tag_successful_verifications_pact_version_id_fk" + String :provider_version_tag_name, null: false + Boolean :wip, null: false + Integer :verification_id + DateTime :execution_date, null: false + index([:pact_version_id, :provider_version_tag_name, :wip], unique: true, name: "pact_version_provider_tag_verifications_pv_pvtn_wip_unique") + # The implication of the on_delete: :set_null for verification_id is + # that even if the verification result is deleted from the broker, + # the wip/pending status stays the same. + # We may or may not want this. Will have to wait and see. + # Have made the foreign key a separate declaration so it can more easily be remade. + foreign_key([:verification_id], :verifications, on_delete: :set_null, name: "pact_version_provider_tag_successful_verifications_verification_id_fk") + end + end + + down do + drop_table(:pact_version_provider_tag_successful_verifications) + end +end diff --git a/db/migrations/20211121_migrate_pact_version_provider_tag_successful_verifications_data.rb b/db/migrations/20211121_migrate_pact_version_provider_tag_successful_verifications_data.rb new file mode 100644 index 000000000..4020234d0 --- /dev/null +++ b/db/migrations/20211121_migrate_pact_version_provider_tag_successful_verifications_data.rb @@ -0,0 +1,11 @@ +require "pact_broker/db/data_migrations/migrate_pact_version_provider_tag_successful_verifications" + +Sequel.migration do + up do + PactBroker::DB::DataMigrations::MigratePactVersionProviderTagSuccessfulVerifications.call(self) + end + + down do + + end +end diff --git a/lib/pact_broker/db/data_migrations/migrate_pact_version_provider_tag_successful_verifications.rb b/lib/pact_broker/db/data_migrations/migrate_pact_version_provider_tag_successful_verifications.rb new file mode 100644 index 000000000..728577f0d --- /dev/null +++ b/lib/pact_broker/db/data_migrations/migrate_pact_version_provider_tag_successful_verifications.rb @@ -0,0 +1,38 @@ +require "pact_broker/db/data_migrations/helpers" + +module PactBroker + module DB + module DataMigrations + class MigratePactVersionProviderTagSuccessfulVerifications + extend Helpers + + def self.call(connection) + successful_verifications_join = { + Sequel[:sv][:pact_version_id] => Sequel[:verifications][:pact_version_id], + Sequel[:sv][:provider_version_tag_name] => Sequel[:tags][:name], + Sequel[:sv][:wip] => Sequel[:verifications][:wip] + } + + missing_verifications = connection + .select( + Sequel[:verifications][:pact_version_id], + Sequel[:tags][:name], + Sequel[:verifications][:wip], + Sequel[:verifications][:id], + Sequel[:verifications][:execution_date] + ) + .order(Sequel[:verifications][:execution_date], Sequel[:verifications][:id]) + .from(:verifications) + .join(:tags, { Sequel[:verifications][:provider_version_id] => Sequel[:tags][:version_id] }) + .left_outer_join(:pact_version_provider_tag_successful_verifications, successful_verifications_join, { table_alias: :sv }) + .where(Sequel[:sv][:pact_version_id] => nil) + .where(Sequel[:verifications][:success] => true) + + connection[:pact_version_provider_tag_successful_verifications] + .insert_ignore + .insert([:pact_version_id, :provider_version_tag_name, :wip, :verification_id, :execution_date], missing_verifications) + end + end + end + end +end diff --git a/lib/pact_broker/db/migrate_data.rb b/lib/pact_broker/db/migrate_data.rb index e9647583a..42bfba3d6 100644 --- a/lib/pact_broker/db/migrate_data.rb +++ b/lib/pact_broker/db/migrate_data.rb @@ -28,6 +28,7 @@ def self.call database_connection, _options = {} DataMigrations::SetExtraColumnsForTags.call(database_connection) DataMigrations::CreateBranches.call(database_connection) DataMigrations::MigrateIntegrations.call(database_connection) + DataMigrations::MigratePactVersionProviderTagSuccessfulVerifications.call(database_connection) end end end diff --git a/lib/pact_broker/pacts/pact_publication_wip_dataset_module.rb b/lib/pact_broker/pacts/pact_publication_wip_dataset_module.rb index 5605a691c..a8364c6ed 100644 --- a/lib/pact_broker/pacts/pact_publication_wip_dataset_module.rb +++ b/lib/pact_broker/pacts/pact_publication_wip_dataset_module.rb @@ -33,6 +33,8 @@ def join_branch_versions_excluding_branch(branch_name) end def successfully_verified_by_provider_tag_when_not_wip(provider_id, provider_tag) + return new_successfully_verified_by_provider_tag_when_not_wip(provider_id, provider_tag) if PactBroker.feature_enabled?(:new_wip_calculation) + from_self(alias: :pp) .select(Sequel[:pp].*) .where(Sequel[:pp][:provider_id] => provider_id) @@ -41,7 +43,23 @@ def successfully_verified_by_provider_tag_when_not_wip(provider_id, provider_tag .distinct end + def new_successfully_verified_by_provider_tag_when_not_wip(provider_id, provider_tag) + pact_version_provider_tag_verifications_join = { + Sequel[:sv][:pact_version_id] => Sequel[:pp][:pact_version_id], + Sequel[:sv][:provider_version_tag_name] => provider_tag, + Sequel[:sv][:wip] => false + } + + from_self(alias: :pp) + .select(Sequel[:pp].*) + .join(:pact_version_provider_tag_successful_verifications, pact_version_provider_tag_verifications_join, { table_alias: :sv }) + .distinct + + end + def successfully_verified_by_provider_another_tag_before_this_tag_first_created(provider_id, provider_tag) + return new_successfully_verified_by_provider_another_tag_before_this_tag_first_created(provider_id, provider_tag) if PactBroker.feature_enabled?(:new_wip_calculation) + first_tag_with_name = PactBroker::Domain::Tag.where(pacticipant_id: provider_id, name: provider_tag).order(:created_at).first from_self(alias: :pp) .select(Sequel[:pp].*) @@ -54,6 +72,32 @@ def successfully_verified_by_provider_another_tag_before_this_tag_first_created( .distinct end + def new_successfully_verified_by_provider_another_tag_before_this_tag_first_created(provider_id, provider_tag) + first_tag_with_name = PactBroker::Domain::Tag.where(pacticipant_id: provider_id, name: provider_tag).order(:created_at).first + + pact_version_provider_tag_verifications_join = { + Sequel[:sv][:pact_version_id] => Sequel[:pp][:pact_version_id], + Sequel[:sv][:wip] => false + } + + created_at_criteria = if first_tag_with_name + Sequel.lit("sv.execution_date < ?", first_tag_with_name.created_at) + else + Sequel.lit("1 = 1") + end + + from_self(alias: :pp) + .select(Sequel[:pp].*) + .where(Sequel[:pp][:provider_id] => provider_id) + .join(:pact_version_provider_tag_successful_verifications, pact_version_provider_tag_verifications_join, { table_alias: :sv }) do + Sequel.&( + Sequel.lit("sv.provider_version_tag_name NOT IN (?)", provider_tag), + created_at_criteria + ) + end + .distinct + end + protected def verified_before_date(date) diff --git a/lib/pact_broker/test/test_data_builder.rb b/lib/pact_broker/test/test_data_builder.rb index 6c7a4f472..e9f59324e 100644 --- a/lib/pact_broker/test/test_data_builder.rb +++ b/lib/pact_broker/test/test_data_builder.rb @@ -31,6 +31,8 @@ require "pact_broker/deployments/released_version_service" require "pact_broker/versions/branch_version_repository" require "pact_broker/integrations/repository" +require "pact_broker/contracts/service" + require "ostruct" module PactBroker @@ -219,6 +221,27 @@ def create_label label_name self end + def publish_pact(consumer_name:, provider_name:, consumer_version_number: , tags: nil, branch: nil, build_url: nil, json_content: nil) + json_content = json_content || random_json_content(consumer_name, provider_name) + contracts = [ + PactBroker::Contracts::ContractToPublish.from_hash(consumer_name: consumer_name, provider_name: provider_name, decoded_content: json_content, content_type: "application/json", specification: "pact") + ] + contracts_to_publish = PactBroker::Contracts::ContractsToPublish.from_hash( + pacticipant_name: consumer_name, + pacticipant_version_number: consumer_version_number, + tags: tags, + branch: branch, + build_url: build_url, + contracts: contracts + ) + PactBroker::Contracts::Service.publish(contracts_to_publish, base_url: "http://example.org") + @consumer = find_pacticipant(consumer_name) + @consumer_version = find_version(consumer_name, consumer_version_number) + @provider = find_pacticipant(provider_name) + @pact = PactBroker::Pacts::PactPublication.last.to_domain + self + end + def create_pact params = {} params.delete(:comment) json_content = params[:json_content] || default_json_content @@ -363,6 +386,7 @@ def create_webhook_execution params = {} end def create_verification parameters = {} + # This should use the verification service. what a mess parameters.delete(:comment) branch = parameters.delete(:branch) tag_names = [parameters.delete(:tag_names), parameters.delete(:tag_name)].flatten.compact @@ -373,20 +397,23 @@ def create_verification parameters = {} parameters.delete(:provider_version) verification = PactBroker::Domain::Verification.new(parameters) pact_version = PactBroker::Pacts::Repository.new.find_pact_version(@consumer, @provider, pact.pact_version_sha) - @verification = PactBroker::Verifications::Repository.new.create(verification, provider_version_number, pact_version) - @provider_version = PactBroker::Domain::Version.where(pacticipant_id: @provider.id, number: provider_version_number).single_record + @provider_version = version_repository.find_by_pacticipant_id_and_number_or_create(provider.id, provider_version_number) PactBroker::Versions::BranchVersionRepository.new.add_branch(@provider_version, branch) if branch - set_created_at_if_set(parameters[:created_at], :verifications, id: @verification.id) - set_created_at_if_set(parameters[:created_at], :versions, id: @provider_version.id) - set_created_at_if_set(parameters[:created_at], :latest_verification_id_for_pact_version_and_provider_version, pact_version_id: pact_version_id, provider_version_id: @provider_version.id) - if tag_names.any? tag_names.each do | tag_name | - PactBroker::Domain::Tag.new(name: tag_name, version: @provider_version).insert_ignore + PactBroker::Domain::Tag.new(name: tag_name, version: @provider_version, version_order: @provider_version.order).insert_ignore set_created_at_if_set(parameters[:created_at], :tags, version_id: @provider_version.id, name: tag_name) end end + + @verification = PactBroker::Verifications::Repository.new.create(verification, provider_version_number, pact_version) + + set_created_at_if_set(parameters[:created_at], :verifications, id: @verification.id) + set_created_at_if_set(parameters[:created_at], :versions, id: @provider_version.id) + set_created_at_if_set(parameters[:created_at], :latest_verification_id_for_pact_version_and_provider_version, pact_version_id: pact_version_id, provider_version_id: @provider_version.id) + set_created_at_if_set(parameters[:created_at], :pact_version_provider_tag_successful_verifications, { verification_id: @verification.id }, :execution_date) + self end @@ -594,10 +621,10 @@ def prepare_json_content(json_content) PactBroker::Pacts::Content.from_json(json_content).with_ids(false).to_json end - def set_created_at_if_set created_at, table_name, selector + def set_created_at_if_set created_at, table_name, selector, date_column_name = :created_at date_to_set = created_at || @now if date_to_set - Sequel::Model.db[table_name].where(selector).update(created_at: date_to_set) + Sequel::Model.db[table_name].where(selector).update(date_column_name => date_to_set) if Sequel::Model.db.schema(table_name).any?{ |col| col.first == :updated_at } Sequel::Model.db[table_name].where(selector.keys.first => selector.values.first).update(updated_at: date_to_set) end diff --git a/lib/pact_broker/verifications/pact_version_provider_tag_successful_verification.rb b/lib/pact_broker/verifications/pact_version_provider_tag_successful_verification.rb new file mode 100644 index 000000000..93a7d4d98 --- /dev/null +++ b/lib/pact_broker/verifications/pact_version_provider_tag_successful_verification.rb @@ -0,0 +1,11 @@ +require "pact_broker/domain/verification" + +# Represents a non WIP, successful verification for a provider version with a tag. + +module PactBroker + module Verifications + class PactVersionProviderTagSuccessfulVerification < Sequel::Model + plugin :insert_ignore, identifying_columns: [:pact_version_id, :provider_version_tag_name, :wip] + end + end +end diff --git a/lib/pact_broker/verifications/repository.rb b/lib/pact_broker/verifications/repository.rb index c72ecdb98..1accdaa87 100644 --- a/lib/pact_broker/verifications/repository.rb +++ b/lib/pact_broker/verifications/repository.rb @@ -2,6 +2,7 @@ require "pact_broker/domain/verification" require "pact_broker/verifications/sequence" require "pact_broker/verifications/latest_verification_id_for_pact_version_and_provider_version" +require "pact_broker/verifications/pact_version_provider_tag_successful_verification" module PactBroker module Verifications @@ -27,6 +28,7 @@ def create verification, provider_version_number, pact_version verification.tag_names = version.tag_names # TODO pass this in from contracts service verification.save update_latest_verification_id(verification) + update_pact_version_provider_tag_verifications(verification, version.tag_names) verification end @@ -46,6 +48,20 @@ def update_latest_verification_id verification LatestVerificationIdForPactVersionAndProviderVersion.new(params).upsert end + def update_pact_version_provider_tag_verifications(verification, tag_names) + if verification.success + tag_names&.each do | tag_name | + PactVersionProviderTagSuccessfulVerification.new( + pact_version_id: verification.pact_version_id, + provider_version_tag_name: tag_name, + wip: verification.wip, + verification_id: verification.id, + execution_date: verification.execution_date + ).insert_ignore + end + end + end + def find consumer_name, provider_name, pact_version_sha, verification_number PactBroker::Domain::Verification .select_all_qualified diff --git a/spec/lib/pact_broker/db/data_migrations/migrate_pact_version_provider_tag_successful_verifications_spec.rb b/spec/lib/pact_broker/db/data_migrations/migrate_pact_version_provider_tag_successful_verifications_spec.rb new file mode 100644 index 000000000..1ef61a74c --- /dev/null +++ b/spec/lib/pact_broker/db/data_migrations/migrate_pact_version_provider_tag_successful_verifications_spec.rb @@ -0,0 +1,51 @@ +require 'pact_broker/db/data_migrations/migrate_pact_version_provider_tag_successful_verifications' + +module PactBroker + module DB + module DataMigrations + describe MigratePactVersionProviderTagSuccessfulVerifications do + describe ".call" do + before do + td.set_now(Date.new(2020, 2, 1)) + .publish_pact(consumer_name: "Foo", provider_name: "Bar", consumer_version_number: "1") + .set_now(Date.new(2021, 2, 1)) + .create_verification(provider_version: "2", tag_names: ["feat/x"], wip: true, success: false) + .publish_pact(consumer_name: "Foo", provider_name: "Bar", consumer_version_number: "2") + .set_now(Date.new(2022, 2, 1)) + .create_verification(provider_version: "3", tag_names: ["feat/x"], wip: true, success: true) + .set_now(Date.new(2023, 2, 1)) + .create_verification(provider_version: "4", tag_names: ["feat/x"], wip: true, success: true, number: 2) + .set_now(Date.new(2024, 2, 1)) + .create_verification(provider_version: "5", tag_names: ["feat/x"], wip: false, success: true, number: 3) + .set_now(Date.new(2025, 2, 1)) + .create_verification(provider_version: "6", tag_names: ["feat/x"], wip: false, success: true, number: 4) + + Sequel::Model.db[:pact_version_provider_tag_successful_verifications].delete + end + + subject do + MigratePactVersionProviderTagSuccessfulVerifications.call(Sequel::Model.db) + MigratePactVersionProviderTagSuccessfulVerifications.call(Sequel::Model.db) + end + + let(:first_verification) { Sequel::Model.db[:verifications].where(provider_version_id: Sequel::Model.db[:versions].select(:id).where(number: "3")).first } + let(:last_verification) { Sequel::Model.db[:verifications].where(provider_version_id: Sequel::Model.db[:versions].select(:id).where(number: "5")).first } + + it "uses the date of the first verification for each wip status (true/false) for the row" do + expect { subject }.to change { Sequel::Model.db[:pact_version_provider_tag_successful_verifications].count }.by(2) + + first_row = Sequel::Model.db[:pact_version_provider_tag_successful_verifications].order(:id).first + + expect(first_row[:execution_date].to_s).to include "2022" + expect(first_row[:verification_id]).to eq first_verification[:id] + + last_row = Sequel::Model.db[:pact_version_provider_tag_successful_verifications].order(:id).last + + expect(last_row[:execution_date].to_s).to include "2024" + expect(last_row[:verification_id]).to eq last_verification[:id] + end + end + end + end + end +end diff --git a/spec/lib/pact_broker/verifications/repository_spec.rb b/spec/lib/pact_broker/verifications/repository_spec.rb index e6c99752f..ccc74f257 100644 --- a/spec/lib/pact_broker/verifications/repository_spec.rb +++ b/spec/lib/pact_broker/verifications/repository_spec.rb @@ -11,7 +11,8 @@ module Verifications let(:verification) do PactBroker::Domain::Verification.new( success: true, - consumer_version_selector_hashes: [{ foo: "bar" }] + consumer_version_selector_hashes: [{ foo: "bar" }], + wip: false ) end @@ -42,6 +43,14 @@ module Verifications it "saves the consumer_version_selector_hashes" do expect(subject.reload.consumer_version_selector_hashes).to eq [{ foo: "bar" }] end + + 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" + ) + end end end