Commit 0a9a194b authored by Tetiana Chupryna's avatar Tetiana Chupryna Committed by Bob Van Landuyt

Optimize AvailableLabelsService for multiple labels

parent ab28e0be
......@@ -14,15 +14,16 @@ module Labels
return [] unless labels
labels = labels.split(',') if labels.is_a?(String)
labels = labels.split(',').map(&:strip) if labels.is_a?(String)
existing_labels = LabelsFinder.new(current_user, finder_params(labels)).execute.index_by(&:title)
labels.map do |label_name|
label = Labels::FindOrCreateService.new(
current_user,
parent,
include_ancestor_groups: true,
title: label_name.strip,
available_labels: available_labels
title: label_name,
existing_labels_by_title: existing_labels
).execute(find_only: find_only)
label
......@@ -45,18 +46,19 @@ module Labels
private
def finder_params
params = { include_ancestor_groups: true }
def finder_params(titles = nil)
finder_params = { include_ancestor_groups: true }
finder_params[:title] = titles if titles
case parent
when Group
params[:group_id] = parent.id
params[:only_group_labels] = true
finder_params[:group_id] = parent.id
finder_params[:only_group_labels] = true
when Project
params[:project_id] = parent.id
finder_params[:project_id] = parent.id
end
params
finder_params
end
end
end
......@@ -6,6 +6,7 @@ module Labels
@current_user = current_user
@parent = parent
@available_labels = params.delete(:available_labels)
@existing_labels_by_title = params.delete(:existing_labels_by_title) || {}
@params = params.dup.with_indifferent_access
end
......@@ -16,7 +17,7 @@ module Labels
private
attr_reader :current_user, :parent, :params, :skip_authorization
attr_reader :current_user, :parent, :params, :skip_authorization, :existing_labels_by_title
def available_labels
@available_labels ||= LabelsFinder.new(
......@@ -29,9 +30,8 @@ module Labels
# Only creates the label if current_user can do so, if the label does not exist
# and the user can not create the label, nil is returned
# rubocop: disable CodeReuse/ActiveRecord
def find_or_create_label(find_only: false)
new_label = available_labels.find_by(title: title)
new_label = find_existing_label(title)
return new_label if find_only
......@@ -42,6 +42,11 @@ module Labels
new_label
end
# rubocop: disable CodeReuse/ActiveRecord
def find_existing_label(title)
existing_labels_by_title[title] || available_labels.find_by(title: title)
end
# rubocop: enable CodeReuse/ActiveRecord
def title
......
---
title: Optimize AvailableLabelsService for multiple labels search
merge_request: 59032
author:
type: performance
......@@ -36,6 +36,15 @@ RSpec.describe Labels::AvailableLabelsService do
expect(result).to include(project_label, group_label)
expect(result).not_to include(other_project_label, other_group_label)
end
it 'do not cause additional query for finding labels' do
label_titles = [project_label.title]
control_count = ActiveRecord::QueryRecorder.new { described_class.new(user, project, labels: label_titles).find_or_create_by_titles }
new_label = create(:label, project: project)
label_titles = [project_label.title, new_label.title]
expect { described_class.new(user, project, labels: label_titles).find_or_create_by_titles }.not_to exceed_query_limit(control_count)
end
end
end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment