Commit 4f6d2f6e authored by Adam Hegyi's avatar Adam Hegyi Committed by Sean McGivern

Optimized label search on group and project level

Optimized Issuable label search on group and project level.
This change introduces a new way of querying issuable records by label.
parent 146c43c9
......@@ -37,6 +37,7 @@ class IssuableFinder
include FinderMethods
include CreatedAtFilter
include Gitlab::Utils::StrongMemoize
prepend OptimizedIssuableLabelFilter
requires_cross_project_access unless: -> { params.project? }
......
......@@ -172,7 +172,14 @@ class LabelsFinder < UnionFinder
ProjectsFinder.new(params: { non_archived: true }, current_user: current_user).execute # rubocop: disable CodeReuse/Finder
end
@projects = @projects.in_namespace(group.id) if group?
if group?
@projects = if params[:include_subgroups]
@projects.in_namespace(group.self_and_descendants.select(:id))
else
@projects.in_namespace(group.id)
end
end
@projects = @projects.where(id: params[:project_ids]) if projects?
@projects = @projects.reorder(nil)
......
# frozen_string_literal: true
module OptimizedIssuableLabelFilter
def by_label(items)
return items unless params.labels?
return super if Feature.disabled?(:optimized_issuable_label_filter)
target_model = items.model
if params.filter_by_no_label?
items.where('NOT EXISTS (?)', optimized_any_label_query(target_model))
elsif params.filter_by_any_label?
items.where('EXISTS (?)', optimized_any_label_query(target_model))
else
issuables_with_selected_labels(items, target_model)
end
end
# Taken from IssuableFinder
def count_by_state
return super if root_namespace.nil?
return super if Feature.disabled?(:optimized_issuable_label_filter)
count_params = params.merge(state: nil, sort: nil, force_cte: true)
finder = self.class.new(current_user, count_params)
state_counts = finder
.execute
.reorder(nil)
.group(:state_id)
.count
counts = state_counts.transform_keys { |key| count_key(key) }
counts[:all] = counts.values.sum
counts.with_indifferent_access
end
private
def issuables_with_selected_labels(items, target_model)
if root_namespace
all_label_ids = find_label_ids(root_namespace)
# Found less labels in the DB than we were searching for. Return nothing.
return items.none if all_label_ids.size != params.label_names.size
all_label_ids.each do |label_ids|
items = items.where('EXISTS (?)', optimized_label_query_by_label_ids(target_model, label_ids))
end
else
params.label_names.each do |label_name|
items = items.where('EXISTS (?)', optimized_label_query_by_label_name(target_model, label_name))
end
end
items
end
def find_label_ids(root_namespace)
finder_params = {
include_subgroups: true,
include_ancestor_groups: true,
include_descendant_groups: true,
group: root_namespace,
title: params.label_names
}
LabelsFinder
.new(nil, finder_params)
.execute(skip_authorization: true)
.pluck(:title, :id)
.group_by(&:first)
.values
.map { |labels| labels.map(&:last) }
end
def root_namespace
strong_memoize(:root_namespace) do
(params.project || params.group)&.root_ancestor
end
end
def optimized_any_label_query(target_model)
LabelLink
.where(target_type: target_model.name)
.where(LabelLink.arel_table['target_id'].eq(target_model.arel_table['id']))
.limit(1)
end
def optimized_label_query_by_label_ids(target_model, label_ids)
LabelLink
.where(target_type: target_model.name)
.where(LabelLink.arel_table['target_id'].eq(target_model.arel_table['id']))
.where(label_id: label_ids)
.limit(1)
end
def optimized_label_query_by_label_name(target_model, label_name)
LabelLink
.joins(:label)
.where(target_type: target_model.name)
.where(LabelLink.arel_table['target_id'].eq(target_model.arel_table['id']))
.where(labels: { name: label_name })
.limit(1)
end
end
---
title: Add indexes to `label_links` database table
merge_request: 34503
author:
type: other
---
name: optimized_issuable_label_filter
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/34503
rollout_issue_url:
group: group::analytics
type: development
default_enabled: false
# frozen_string_literal: true
class AddExtraIndexToLabelLinks < ActiveRecord::Migration[6.0]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
INDEX_COVERING_ALL_COLUMNS = 'index_on_label_links_all_columns'
INDEX_TO_REPLACE = 'index_label_links_on_label_id'
NEW_INDEX = 'index_label_links_on_label_id_and_target_type'
disable_ddl_transaction!
def up
add_concurrent_index :label_links, [:target_id, :label_id, :target_type], name: INDEX_COVERING_ALL_COLUMNS
add_concurrent_index :label_links, [:label_id, :target_type], name: NEW_INDEX
remove_concurrent_index_by_name(:label_links, INDEX_TO_REPLACE)
end
def down
remove_concurrent_index_by_name(:label_links, INDEX_COVERING_ALL_COLUMNS)
add_concurrent_index(:label_links, :label_id, name: INDEX_TO_REPLACE)
remove_concurrent_index_by_name(:label_links, NEW_INDEX)
end
end
9cd0e15dd2c5e70e53fc154a47a76ec066c741b5f6d148972b96d23888f0fcd4
\ No newline at end of file
......@@ -20123,7 +20123,7 @@ CREATE INDEX index_keys_on_user_id ON public.keys USING btree (user_id);
CREATE UNIQUE INDEX index_kubernetes_namespaces_on_cluster_project_environment_id ON public.clusters_kubernetes_namespaces USING btree (cluster_id, project_id, environment_id);
CREATE INDEX index_label_links_on_label_id ON public.label_links USING btree (label_id);
CREATE INDEX index_label_links_on_label_id_and_target_type ON public.label_links USING btree (label_id, target_type);
CREATE INDEX index_label_links_on_target_id_and_target_type ON public.label_links USING btree (target_id, target_type);
......@@ -20409,6 +20409,8 @@ CREATE INDEX index_on_identities_lower_extern_uid_and_provider ON public.identit
CREATE UNIQUE INDEX index_on_instance_statistics_recorded_at_and_identifier ON public.analytics_instance_statistics_measurements USING btree (identifier, recorded_at);
CREATE INDEX index_on_label_links_all_columns ON public.label_links USING btree (target_id, label_id, target_type);
CREATE INDEX index_on_users_name_lower ON public.users USING btree (lower((name)::text));
CREATE INDEX index_open_project_tracker_data_on_service_id ON public.open_project_tracker_data USING btree (service_id);
......
......@@ -192,7 +192,7 @@ RSpec.describe Groups::EpicsController do
end
context 'using label_name filter' do
let(:label) { create(:label) }
let(:label) { create(:group_label, group: group) }
let!(:labeled_epic) { create(:labeled_epic, group: group, labels: [label]) }
it 'returns all epics with given label' do
......
......@@ -107,7 +107,7 @@ RSpec.describe EpicsFinder do
end
context 'by label' do
let_it_be(:label) { create(:label) }
let_it_be(:label) { create(:group_label, group: group) }
let_it_be(:labeled_epic) { create(:labeled_epic, group: group, labels: [label]) }
it 'returns all epics with given label' do
......@@ -461,8 +461,8 @@ RSpec.describe EpicsFinder do
context 'when using group cte for search' do
context 'and two labels more search string are present' do
let_it_be(:label1) { create(:label) }
let_it_be(:label2) { create(:label) }
let_it_be(:label1) { create(:group_label, group: group) }
let_it_be(:label2) { create(:group_label, group: group) }
let_it_be(:labeled_epic) { create(:labeled_epic, group: group, title: 'filtered epic', labels: [label1, label2]) }
it 'returns correct epics' do
......@@ -559,8 +559,8 @@ RSpec.describe EpicsFinder do
end
context 'with negated labels' do
let_it_be(:label) { create(:label) }
let_it_be(:label2) { create(:label) }
let_it_be(:label) { create(:group_label, group: group) }
let_it_be(:label2) { create(:group_label, group: group) }
let_it_be(:negated_epic) { create(:labeled_epic, group: group, labels: [label]) }
let_it_be(:negated_epic2) { create(:labeled_epic, group: group, labels: [label2]) }
let_it_be(:params) { { not: { label_name: [label.title, label2.title].join(',') } } }
......@@ -630,8 +630,8 @@ RSpec.describe EpicsFinder do
end
describe '#row_count' do
let_it_be(:label) { create(:label) }
let_it_be(:label2) { create(:label) }
let_it_be(:label) { create(:group_label, group: group) }
let_it_be(:label2) { create(:group_label, group: group) }
let_it_be(:labeled_epic) { create(:labeled_epic, group: group, labels: [label]) }
let_it_be(:labeled_epic2) { create(:labeled_epic, group: group, labels: [label, label2]) }
......
......@@ -6,7 +6,7 @@ RSpec.describe API::Epics do
let_it_be(:user) { create(:user) }
let(:group) { create(:group) }
let(:project) { create(:project, :public, group: group) }
let_it_be(:label) { create(:label) }
let(:label) { create(:group_label, group: group) }
let!(:epic) { create(:labeled_epic, group: group, labels: [label]) }
let(:params) { nil }
......
......@@ -330,100 +330,139 @@ RSpec.describe IssuesFinder do
end
end
context 'filtering by label' do
let(:params) { { label_name: label.title } }
shared_examples ':label_name parameter' do
context 'filtering by label' do
let(:params) { { label_name: label.title } }
it 'returns issues with that label' do
expect(issues).to contain_exactly(issue2)
end
it 'returns issues with that label' do
expect(issues).to contain_exactly(issue2)
end
context 'using NOT' do
let(:params) { { not: { label_name: label.title } } }
context 'using NOT' do
let(:params) { { not: { label_name: label.title } } }
it 'returns issues that do not have that label' do
expect(issues).to contain_exactly(issue1, issue3, issue4)
end
it 'returns issues that do not have that label' do
expect(issues).to contain_exactly(issue1, issue3, issue4)
end
# IssuableFinder first filters using the outer params (the ones not inside the `not` key.)
# Afterwards, it applies the `not` params to that resultset. This means that things inside the `not` param
# do not take precedence over the outer params with the same name.
context 'shadowing the same outside param' do
let(:params) { { label_name: label2.title, not: { label_name: label.title } } }
# IssuableFinder first filters using the outer params (the ones not inside the `not` key.)
# Afterwards, it applies the `not` params to that resultset. This means that things inside the `not` param
# do not take precedence over the outer params with the same name.
context 'shadowing the same outside param' do
let(:params) { { label_name: label2.title, not: { label_name: label.title } } }
it 'does not take precedence over labels outside NOT' do
expect(issues).to contain_exactly(issue3)
it 'does not take precedence over labels outside NOT' do
expect(issues).to contain_exactly(issue3)
end
end
end
context 'further filtering outside params' do
let(:params) { { label_name: label2.title, not: { assignee_username: user2.username } } }
context 'further filtering outside params' do
let(:params) { { label_name: label2.title, not: { assignee_username: user2.username } } }
it 'further filters on the returned resultset' do
expect(issues).to be_empty
it 'further filters on the returned resultset' do
expect(issues).to be_empty
end
end
end
end
end
context 'filtering by multiple labels' do
let(:params) { { label_name: [label.title, label2.title].join(',') } }
let(:label2) { create(:label, project: project2) }
context 'filtering by multiple labels' do
let(:params) { { label_name: [label.title, label2.title].join(',') } }
let(:label2) { create(:label, project: project2) }
before do
create(:label_link, label: label2, target: issue2)
end
before do
create(:label_link, label: label2, target: issue2)
end
it 'returns the unique issues with all those labels' do
expect(issues).to contain_exactly(issue2)
end
it 'returns the unique issues with all those labels' do
expect(issues).to contain_exactly(issue2)
end
context 'using NOT' do
let(:params) { { not: { label_name: [label.title, label2.title].join(',') } } }
context 'using NOT' do
let(:params) { { not: { label_name: [label.title, label2.title].join(',') } } }
it 'returns issues that do not have any of the labels provided' do
expect(issues).to contain_exactly(issue1, issue4)
it 'returns issues that do not have any of the labels provided' do
expect(issues).to contain_exactly(issue1, issue4)
end
end
end
end
context 'filtering by a label that includes any or none in the title' do
let(:params) { { label_name: [label.title, label2.title].join(',') } }
let(:label) { create(:label, title: 'any foo', project: project2) }
let(:label2) { create(:label, title: 'bar none', project: project2) }
context 'filtering by a label that includes any or none in the title' do
let(:params) { { label_name: [label.title, label2.title].join(',') } }
let(:label) { create(:label, title: 'any foo', project: project2) }
let(:label2) { create(:label, title: 'bar none', project: project2) }
before do
create(:label_link, label: label2, target: issue2)
end
before do
create(:label_link, label: label2, target: issue2)
end
it 'returns the unique issues with all those labels' do
expect(issues).to contain_exactly(issue2)
it 'returns the unique issues with all those labels' do
expect(issues).to contain_exactly(issue2)
end
context 'using NOT' do
let(:params) { { not: { label_name: [label.title, label2.title].join(',') } } }
it 'returns issues that do not have ANY ONE of the labels provided' do
expect(issues).to contain_exactly(issue1, issue4)
end
end
end
context 'using NOT' do
let(:params) { { not: { label_name: [label.title, label2.title].join(',') } } }
context 'filtering by no label' do
let(:params) { { label_name: described_class::Params::FILTER_NONE } }
it 'returns issues that do not have ANY ONE of the labels provided' do
it 'returns issues with no labels' do
expect(issues).to contain_exactly(issue1, issue4)
end
end
end
context 'filtering by no label' do
let(:params) { { label_name: described_class::Params::FILTER_NONE } }
context 'filtering by any label' do
let(:params) { { label_name: described_class::Params::FILTER_ANY } }
it 'returns issues with no labels' do
expect(issues).to contain_exactly(issue1, issue4)
it 'returns issues that have one or more label' do
create_list(:label_link, 2, label: create(:label, project: project2), target: issue3)
expect(issues).to contain_exactly(issue2, issue3)
end
end
context 'when the same label exists on project and group levels' do
let(:issue1) { create(:issue, project: project1) }
let(:issue2) { create(:issue, project: project1) }
# Skipping validation to reproduce a "real-word" scenario.
# We still have legacy labels on PRD that have the same title on the group and project levels, example: `bug`
let(:project_label) { build(:label, title: 'somelabel', project: project1).tap { |r| r.save!(validate: false) } }
let(:group_label) { create(:group_label, title: 'somelabel', group: project1.group) }
let(:params) { { label_name: 'somelabel' } }
before do
create(:label_link, label: group_label, target: issue1)
create(:label_link, label: project_label, target: issue2)
end
it 'finds both issue records' do
expect(issues).to contain_exactly(issue1, issue2)
end
end
end
context 'filtering by any label' do
let(:params) { { label_name: described_class::Params::FILTER_ANY } }
context 'when `optimized_issuable_label_filter` feature flag is off' do
before do
stub_feature_flags(optimized_issuable_label_filter: false)
end
it 'returns issues that have one or more label' do
create_list(:label_link, 2, label: create(:label, project: project2), target: issue3)
it_behaves_like ':label_name parameter'
end
expect(issues).to contain_exactly(issue2, issue3)
context 'when `optimized_issuable_label_filter` feature flag is on' do
before do
stub_feature_flags(optimized_issuable_label_filter: true)
end
it_behaves_like ':label_name parameter'
end
context 'filtering by issue term' do
......
......@@ -167,38 +167,56 @@ RSpec.describe MergeRequestsFinder do
end
end
describe ':label_name parameter' do
let(:common_labels) { create_list(:label, 3) }
let(:distinct_labels) { create_list(:label, 3) }
let(:merge_requests) do
common_attrs = {
source_project: project1, target_project: project1, author: user
}
distinct_labels.map do |label|
labels = [label, *common_labels]
create(:labeled_merge_request, :closed, labels: labels, **common_attrs)
shared_examples ':label_name parameter' do
describe ':label_name parameter' do
let(:common_labels) { create_list(:label, 3) }
let(:distinct_labels) { create_list(:label, 3) }
let(:merge_requests) do
common_attrs = {
source_project: project1, target_project: project1, author: user
}
distinct_labels.map do |label|
labels = [label, *common_labels]
create(:labeled_merge_request, :closed, labels: labels, **common_attrs)
end
end
end
def find(label_name)
described_class.new(user, label_name: label_name).execute
end
def find(label_name)
described_class.new(user, label_name: label_name).execute
end
it 'accepts a single label' do
found = find(distinct_labels.first.title)
common = find(common_labels.first.title)
expect(found).to contain_exactly(merge_requests.first)
expect(common).to match_array(merge_requests)
end
it 'accepts a single label' do
found = find(distinct_labels.first.title)
common = find(common_labels.first.title)
it 'accepts an array of labels, all of which must match' do
all_distinct = find(distinct_labels.pluck(:title))
all_common = find(common_labels.pluck(:title))
expect(found).to contain_exactly(merge_requests.first)
expect(common).to match_array(merge_requests)
expect(all_distinct).to be_empty
expect(all_common).to match_array(merge_requests)
end
end
end
context 'when `optimized_issuable_label_filter` feature flag is off' do
before do
stub_feature_flags(optimized_issuable_label_filter: false)
end
it 'accepts an array of labels, all of which must match' do
all_distinct = find(distinct_labels.pluck(:title))
all_common = find(common_labels.pluck(:title))
it_behaves_like ':label_name parameter'
end
expect(all_distinct).to be_empty
expect(all_common).to match_array(merge_requests)
context 'when `optimized_issuable_label_filter` feature flag is on' do
before do
stub_feature_flags(optimized_issuable_label_filter: true)
end
it_behaves_like ':label_name parameter'
end
it 'filters by source project id' do
......
......@@ -15,7 +15,7 @@ RSpec.describe Resolvers::MergeRequestsResolver do
let_it_be(:merge_request_3) { create(:merge_request, :unique_branches, **common_attrs) }
let_it_be(:merge_request_4) { create(:merge_request, :unique_branches, :locked, **common_attrs) }
let_it_be(:merge_request_5) { create(:merge_request, :simple, :locked, **common_attrs) }
let_it_be(:merge_request_6) { create(:labeled_merge_request, :unique_branches, labels: create_list(:label, 2), **common_attrs) }
let_it_be(:merge_request_6) { create(:labeled_merge_request, :unique_branches, labels: create_list(:label, 2, project: project), **common_attrs) }
let_it_be(:merge_request_with_milestone) { create(:merge_request, :unique_branches, **common_attrs, milestone: milestone) }
let_it_be(:other_project) { create(:project, :repository) }
let_it_be(:other_merge_request) { create(:merge_request, source_project: other_project, target_project: other_project) }
......
......@@ -8,7 +8,7 @@ RSpec.describe 'getting merge request listings nested in a project' do
let_it_be(:project) { create(:project, :repository, :public) }
let_it_be(:current_user) { create(:user) }
let_it_be(:label) { create(:label) }
let_it_be(:label) { create(:label, project: project) }
let_it_be(:merge_request_a) { create(:labeled_merge_request, :unique_branches, source_project: project, labels: [label]) }
let_it_be(:merge_request_b) { create(:merge_request, :closed, :unique_branches, source_project: project) }
let_it_be(:merge_request_c) { create(:labeled_merge_request, :closed, :unique_branches, source_project: project, labels: [label]) }
......
......@@ -402,30 +402,76 @@ RSpec.describe API::Issues do
expect_paginated_array_response([group_closed_issue.id, group_issue.id])
end
it 'returns an array of labeled group issues' do
get api(base_url, user), params: { labels: group_label.title }
shared_examples 'labels parameter' do
it 'returns an array of labeled group issues' do
get api(base_url, user), params: { labels: group_label.title }
expect_paginated_array_response(group_issue.id)
expect(json_response.first['labels']).to eq([group_label.title])
end
expect_paginated_array_response(group_issue.id)
expect(json_response.first['labels']).to eq([group_label.title])
end
it 'returns an array of labeled group issues with labels param as array' do
get api(base_url, user), params: { labels: [group_label.title] }
it 'returns an array of labeled group issues' do
get api(base_url, user), params: { labels: group_label.title }
expect_paginated_array_response(group_issue.id)
expect(json_response.first['labels']).to eq([group_label.title])
expect_paginated_array_response(group_issue.id)
expect(json_response.first['labels']).to eq([group_label.title])
end
it 'returns an array of labeled group issues with labels param as array' do
get api(base_url, user), params: { labels: [group_label.title] }
expect_paginated_array_response(group_issue.id)
expect(json_response.first['labels']).to eq([group_label.title])
end
it 'returns an array of labeled group issues where all labels match' do
get api(base_url, user), params: { labels: "#{group_label.title},foo,bar" }
expect_paginated_array_response([])
end
it 'returns an array of labeled group issues where all labels match with labels param as array' do
get api(base_url, user), params: { labels: [group_label.title, 'foo', 'bar'] }
expect_paginated_array_response([])
end
context 'with labeled issues' do
let(:group_issue2) { create :issue, project: group_project }
let(:label_b) { create(:label, title: 'foo', project: group_project) }
let(:label_c) { create(:label, title: 'bar', project: group_project) }
before do
create(:label_link, label: group_label, target: group_issue2)
create(:label_link, label: label_b, target: group_issue)
create(:label_link, label: label_b, target: group_issue2)
create(:label_link, label: label_c, target: group_issue)
get api(base_url, user), params: params
end
let(:issue) { group_issue }
let(:issue2) { group_issue2 }
let(:label) { group_label }
it_behaves_like 'labeled issues with labels and label_name params'
end
end
it 'returns an array of labeled group issues where all labels match' do
get api(base_url, user), params: { labels: "#{group_label.title},foo,bar" }
context 'when `optimized_issuable_label_filter` feature flag is off' do
before do
stub_feature_flags(optimized_issuable_label_filter: false)
end
expect_paginated_array_response([])
it_behaves_like 'labels parameter'
end
it 'returns an array of labeled group issues where all labels match with labels param as array' do
get api(base_url, user), params: { labels: [group_label.title, 'foo', 'bar'] }
context 'when `optimized_issuable_label_filter` feature flag is on' do
before do
stub_feature_flags(optimized_issuable_label_filter: true)
end
expect_paginated_array_response([])
it_behaves_like 'labels parameter'
end
it 'returns issues matching given search string for title' do
......@@ -440,27 +486,6 @@ RSpec.describe API::Issues do
expect_paginated_array_response(group_issue.id)
end
context 'with labeled issues' do
let(:group_issue2) { create :issue, project: group_project }
let(:label_b) { create(:label, title: 'foo', project: group_project) }
let(:label_c) { create(:label, title: 'bar', project: group_project) }
before do
create(:label_link, label: group_label, target: group_issue2)
create(:label_link, label: label_b, target: group_issue)
create(:label_link, label: label_b, target: group_issue2)
create(:label_link, label: label_c, target: group_issue)
get api(base_url, user), params: params
end
let(:issue) { group_issue }
let(:issue2) { group_issue2 }
let(:label) { group_label }
it_behaves_like 'labeled issues with labels and label_name params'
end
context 'with archived projects' do
let_it_be(:archived_issue) do
create(
......
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