From bdc2599ca38e3cc847fc67bc94da53732cbf708f Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Tue, 17 May 2022 17:15:39 +1000 Subject: [PATCH] feat: remove inefficient skynet query for tags --- lib/pact_broker/domain/tag.rb | 40 +++++-------------------- lib/pact_broker/repositories/helpers.rb | 13 ++++++++ spec/lib/pact_broker/domain/tag_spec.rb | 22 ++++++++++++++ 3 files changed, 43 insertions(+), 32 deletions(-) diff --git a/lib/pact_broker/domain/tag.rb b/lib/pact_broker/domain/tag.rb index 28879e6be..1e6025366 100644 --- a/lib/pact_broker/domain/tag.rb +++ b/lib/pact_broker/domain/tag.rb @@ -39,53 +39,28 @@ def for(pacticipant_name, version_number, tag_name) end def latest_tags - self_join = { - Sequel[:tags][:pacticipant_id] => Sequel[:tags_2][:pacticipant_id], - Sequel[:tags][:name] => Sequel[:tags_2][:name] - } - - PactBroker::Domain::Tag + Tag .select_all_qualified - .left_join(:tags, self_join, { table_alias: :tags_2 }) do - Sequel[:tags_2][:version_order] > Sequel[:tags][:version_order] - end - .where(Sequel[:tags_2][:name] => nil) + .max_group_by(:version_order, [:pacticipant_id, :name]) end # Does NOT care about whether or not there is a pact publication # for the version def latest_tags_for_pacticipant_ids(pacticipant_ids) - self_join = { - Sequel[:tags][:pacticipant_id] => Sequel[:tags_2][:pacticipant_id], - Sequel[:tags][:name] => Sequel[:tags_2][:name], - Sequel[:tags_2][:pacticipant_id] => pacticipant_ids, - } - Tag .select_all_qualified - .left_join(:tags, self_join, { table_alias: :tags_2 }) do - Sequel[:tags_2][:version_order] > Sequel[:tags][:version_order] + .max_group_by(:version_order, [:pacticipant_id, :name]) do | ds | + ds.where(Sequel[:tags][:pacticipant_id] => pacticipant_ids) end - .where(Sequel[:tags_2][:name] => nil) - .where(Sequel[:tags][:pacticipant_id] => pacticipant_ids) end def latest_tags_for_pacticipant_ids_and_tag_names(pacticipant_ids, tag_names) - self_join = { - Sequel[:tags][:pacticipant_id] => Sequel[:tags_2][:pacticipant_id], - Sequel[:tags][:name] => Sequel[:tags_2][:name], - Sequel[:tags_2][:pacticipant_id] => pacticipant_ids, - Sequel[:tags_2][:name] => tag_names - } - Tag .select_all_qualified - .left_join(:tags, self_join, { table_alias: :tags_2 }) do - Sequel[:tags_2][:version_order] > Sequel[:tags][:version_order] + .max_group_by(:version_order, [:pacticipant_id, :name]) do | ds | + ds.where(Sequel[:tags][:name] => tag_names) + .where(Sequel[:tags][:pacticipant_id] => pacticipant_ids) end - .where(Sequel[:tags_2][:name] => nil) - .where(Sequel[:tags][:pacticipant_id] => pacticipant_ids) - .where(Sequel[:tags][:name] => tag_names) end # ignores tags that don't have a pact publication @@ -110,6 +85,7 @@ def head_tags_for_consumer_id(consumer_id) .join(lp, versions_pact_publications_join) end + # TODO convert this to use max and join def head_tags_for_pact_publication(pact_publication) Tag.where(version_id: pact_publication.consumer_version_id).all.select do | tag | tag_pp_join = { diff --git a/lib/pact_broker/repositories/helpers.rb b/lib/pact_broker/repositories/helpers.rb index de5b0d710..be2c5d89e 100644 --- a/lib/pact_broker/repositories/helpers.rb +++ b/lib/pact_broker/repositories/helpers.rb @@ -55,6 +55,19 @@ def select_append_all_qualified select_append(Sequel[model.table_name].*) end + # @param [Symbol] max_column the name of the column of which to calculate the maxiumum + # @param [Array] group_by_columns the names of the columns by which to group + def max_group_by(max_column, group_by_columns, &extra_criteria_block) + maximums_base_query = extra_criteria_block ? extra_criteria_block.call(self) : self + maximums = maximums_base_query.select_group(*group_by_columns).select_append(Sequel.function(:max, max_column).as(:max_value)) + + max_join = group_by_columns.each_with_object({ Sequel[:maximums][:max_value] => max_column }) do | column_name, joins | + joins[Sequel[:maximums][column_name]] = column_name + end + + join(maximums, max_join, table_alias: :maximums) + end + def select_for_subquery column if mysql? #stoopid mysql doesn't allow you to modify datasets with subqueries column_name = column.respond_to?(:alias) ? column.alias : column diff --git a/spec/lib/pact_broker/domain/tag_spec.rb b/spec/lib/pact_broker/domain/tag_spec.rb index 776e8a5fc..3b30d8312 100644 --- a/spec/lib/pact_broker/domain/tag_spec.rb +++ b/spec/lib/pact_broker/domain/tag_spec.rb @@ -3,6 +3,28 @@ module PactBroker module Domain describe Tag do + describe "#latest_tags_for_pacticipant_ids_and_tag_names" do + before do + td.create_consumer("bar") + .create_consumer_version("1", tag_names: ["dev", "prod"]) + .create_consumer("foo") + .create_consumer_version("1", tag_names: ["dev", "prod"]) + .create_consumer_version("2", tag_names: ["dev"]) + .create_consumer_version("3") + end + + let(:foo) { td.find_pacticipant("foo") } + let(:foo_version_1) { td.find_version("foo", "1") } + let(:foo_version_2) { td.find_version("foo", "2") } + + subject { Tag.latest_tags_for_pacticipant_ids_and_tag_names([foo.id], ["dev", "prod"]).order(:name).all } + + it "returns the latest tag grouped by pacticipant id and tag name" do + expect(subject.size).to eq 2 + expect(subject.first).to have_attributes(version_id: foo_version_2.id, pacticipant_id: foo.id) + expect(subject.last).to have_attributes(version_id: foo_version_1.id, pacticipant_id: foo.id) + end + end describe "#latest_tags_for_pacticipant_ids" do before do