diff --git a/app/finders/autocomplete/group_users_finder.rb b/app/finders/autocomplete/group_users_finder.rb
index 749d7821f44e67e975fb797b3f3701e9cb283f19..b24f3f7f032a2465db11773ed77199cf162de0bc 100644
--- a/app/finders/autocomplete/group_users_finder.rb
+++ b/app/finders/autocomplete/group_users_finder.rb
@@ -12,9 +12,8 @@ module Autocomplete
class GroupUsersFinder
include Gitlab::Utils::StrongMemoize
- def initialize(group:, members_relation: false)
+ def initialize(group:)
@group = group
- @members_relation = members_relation # If true, it will return Member relation instead
end
def execute
@@ -23,8 +22,6 @@ def execute
.with(descendant_projects_cte.to_arel) # rubocop:disable CodeReuse/ActiveRecord
.from_union(member_relations, remove_duplicates: false)
- return members if @members_relation
-
User
.id_in(members.select(:user_id))
.allow_cross_joins_across_databases(url: "https://gitlab.com/gitlab-org/gitlab/-/issues/420387")
@@ -34,11 +31,11 @@ def execute
def member_relations
[
- members_from_group_hierarchy,
- members_from_hierarchy_group_shares,
- members_from_descendant_projects,
- members_from_descendant_project_shares
- ].map { |relation| relation.select(:id, :user_id) }
+ members_from_group_hierarchy.select(:user_id),
+ members_from_hierarchy_group_shares.select(:user_id),
+ members_from_descendant_projects.select(:user_id),
+ members_from_descendant_project_shares.select(:user_id)
+ ]
end
def members_from_group_hierarchy
diff --git a/app/finders/autocomplete/users_finder.rb b/app/finders/autocomplete/users_finder.rb
index 6f15227efa54fa380a95714c4a581dafb9325191..e7a24cde2bd03c74c35d05b5667956ee1e225018 100644
--- a/app/finders/autocomplete/users_finder.rb
+++ b/app/finders/autocomplete/users_finder.rb
@@ -9,7 +9,6 @@ class UsersFinder
# consistent and removes the need for implementing keyset pagination to
# ensure good performance.
LIMIT = 20
- BATCH_SIZE = 1000
attr_reader :current_user, :project, :group, :search,
:author_id, :todo_filter, :todo_state_filter,
@@ -63,19 +62,7 @@ def limited_users
# When changing the order of these method calls, make sure that
# reorder_by_name() is called _before_ optionally_search(), otherwise
# reorder_by_name will break the ORDER BY applied in optionally_search().
- if project
- project.authorized_users.union_with_user(author_id).merge(users_relation)
- elsif group
- find_group_users(group)
- elsif current_user
- users_relation
- else
- User.none
- end.to_a
- end
-
- def users_relation
- User
+ find_users
.where(state: states)
.non_internal
.reorder_by_name
@@ -86,6 +73,7 @@ def users_relation
todo_state: todo_state_filter
)
.limit(LIMIT)
+ .to_a
end
# rubocop: enable CodeReuse/ActiveRecord
@@ -97,28 +85,15 @@ def prepend_author?
author_id.present? && current_user
end
- def find_group_users(group)
- if Feature.enabled?(:group_users_autocomplete_using_batch_reduction, current_user)
- members_relation = ::Autocomplete::GroupUsersFinder.new(group: group, members_relation: true).execute # rubocop: disable CodeReuse/Finder
-
- user_relations = []
- members_relation.distinct_each_batch(column: :user_id, of: BATCH_SIZE) do |relation|
- user_relations << users_relation.id_in(relation.pluck_user_ids)
- end
-
- # When there is more than 1 batch, we need to apply users_relation again on
- # the top results per-batch so that we return results in the correct order.
- if user_relations.empty?
- User.none
- elsif user_relations.one?
- user_relations.first
- else
- # We pluck the ids per batch so that we don't send an unbounded number of ids in one query
- user_ids = user_relations.flat_map(&:pluck_primary_key)
- users_relation.id_in(user_ids)
- end
+ def find_users
+ if project
+ project.authorized_users.union_with_user(author_id)
+ elsif group
+ ::Autocomplete::GroupUsersFinder.new(group: group).execute # rubocop: disable CodeReuse/Finder
+ elsif current_user
+ User.all
else
- ::Autocomplete::GroupUsersFinder.new(group: group).execute.merge(users_relation) # rubocop: disable CodeReuse/Finder
+ User.none
end
end
diff --git a/config/feature_flags/development/group_users_autocomplete_using_batch_reduction.yml b/config/feature_flags/development/group_users_autocomplete_using_batch_reduction.yml
deleted file mode 100644
index b012afa34cf8d43166b86992253737af10d4d1a0..0000000000000000000000000000000000000000
--- a/config/feature_flags/development/group_users_autocomplete_using_batch_reduction.yml
+++ /dev/null
@@ -1,8 +0,0 @@
----
-name: group_users_autocomplete_using_batch_reduction
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/134915
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/430268
-milestone: '16.7'
-type: development
-group: group::tenant scale
-default_enabled: false
diff --git a/spec/finders/autocomplete/group_users_finder_spec.rb b/spec/finders/autocomplete/group_users_finder_spec.rb
index 9dc1b49a25675eadb8dd9df9888acaf92355c931..78d0663ada6e2837e20045fea4d041ee6909c22b 100644
--- a/spec/finders/autocomplete/group_users_finder_spec.rb
+++ b/spec/finders/autocomplete/group_users_finder_spec.rb
@@ -11,13 +11,9 @@
let_it_be(:group_project) { create(:project, namespace: group) }
let_it_be(:subgroup_project) { create(:project, namespace: subgroup) }
- let(:members_relation) { false }
-
- let(:finder) { described_class.new(group: group, members_relation: members_relation) }
+ let(:finder) { described_class.new(group: group) }
describe '#execute' do
- subject { finder.execute }
-
context 'with group members' do
let_it_be(:parent_group_member) { create(:user).tap { |u| parent_group.add_developer(u) } }
let_it_be(:group_member) { create(:user).tap { |u| group.add_developer(u) } }
@@ -33,17 +29,6 @@
subgroup_member
)
end
-
- context 'when requesting members_relation' do
- let(:members_relation) { true }
- let(:expected_members) do
- [parent_group_member, group_member, subgroup_member].map do |user|
- Member.where(user: user).select(:id, :user_id).first
- end
- end
-
- it { is_expected.to match_array(expected_members) }
- end
end
context 'with project members' do
diff --git a/spec/finders/autocomplete/users_finder_spec.rb b/spec/finders/autocomplete/users_finder_spec.rb
index de6384d2239f5aea78bfb3fcbc1a71d7b77b222d..e4337e52306d4bf5bc3f38330acdeccbfa0cb718 100644
--- a/spec/finders/autocomplete/users_finder_spec.rb
+++ b/spec/finders/autocomplete/users_finder_spec.rb
@@ -2,7 +2,7 @@
require 'spec_helper'
-RSpec.describe Autocomplete::UsersFinder, feature_category: :team_planning do
+RSpec.describe Autocomplete::UsersFinder do
# TODO update when multiple owners are possible in projects
# https://gitlab.com/gitlab-org/gitlab/-/issues/21432
@@ -21,10 +21,6 @@
subject { described_class.new(params: params, current_user: current_user, project: project, group: group).execute.to_a }
- before do
- stub_feature_flags(group_users_autocomplete_using_batch_reduction: false)
- end
-
context 'when current_user not passed or nil' do
let(:current_user) { nil }
@@ -85,86 +81,6 @@
expect(subject).to contain_exactly(user1)
end
end
-
- context 'when querying the users with cross joins disabled' do
- before do
- stub_const("#{described_class.name}::BATCH_SIZE", 5)
- stub_const("#{described_class.name}::LIMIT", 3)
- stub_feature_flags(group_users_autocomplete_using_batch_reduction: true)
- end
-
- # Testing the 0 batches case
- context 'when the group has no matching users' do
- let(:params) { { search: 'non_matching_query' } }
-
- it { is_expected.to eq([]) }
- end
-
- context 'when the group is smaller than the batch size' do
- let(:params) { { search: 'zzz' } }
- let_it_be(:user2) { create(:user, name: 'zzz', username: 'zzz_john_doe') }
-
- before do
- group.add_developer(user2)
- end
-
- specify do
- expect_next_instance_of(described_class) do |instance|
- expect(instance).to receive(:users_relation).once.and_call_original
- end
-
- # order is important. user2 comes first because it matches the username
- is_expected.to eq([user2, user1])
- end
- end
-
- context 'when the group has more users than the batch size' do
- let_it_be(:users_a) do
- build_list(:user, 2) do |record, i|
- record.assign_attributes({ name: "Oliver Doe #{i}", username: "oliver_#{i}" })
- record.save!
- end
- end
-
- let_it_be(:users_b) do
- build_list(:user, 4) do |record, i|
- record.assign_attributes({ name: "Peter Doe #{i}", username: "peter_#{i}" })
- record.save!
- end
- end
-
- let_it_be(:user2) { create(:user, name: 'John Foobar', username: 'john_foobar') }
- let_it_be(:user3) { create(:user, name: 'Johanna Doe', username: 'johanna_doe') }
- let_it_be(:user4) { create(:user, name: 'Oliver Foobar', username: 'oliver_foobar') }
- let_it_be(:non_member) { create(:user, name: 'Johanna Doe', username: 'johanna_doe_2') }
-
- before do
- group.members.delete_all
- group.add_members(users_a, GroupMember::DEVELOPER)
- group.add_members(users_b, GroupMember::DEVELOPER)
- group.add_members([user2, user3, user4], GroupMember::DEVELOPER)
- end
-
- context 'when the query matches several users that span over different batches' do
- let(:params) { { search: 'Oliver' } }
-
- specify do
- # the relation is called once per batch (2 batches), and once at the end to reduce the batches
- expect_next_instance_of(described_class) do |instance|
- expect(instance).to receive(:users_relation).exactly(3).times.and_call_original
- end
-
- is_expected.to eq(users_a + [user4])
- end
- end
-
- context 'when the query matches only one user' do
- let(:params) { { search: 'johanna_doe' } }
-
- it { is_expected.to eq([user3]) }
- end
- end
- end
end
context 'when passed a subgroup' do
@@ -187,33 +103,6 @@
child_project_user
)
end
-
- context 'when querying the users with cross joins disabled' do
- let(:params) { { search: 'zzz' } }
- let_it_be(:user2) { create(:user, name: 'John Doe', username: 'zzz1') }
- let_it_be(:user3) { create(:user, name: 'John Doe', username: 'zzz2') }
- let_it_be(:user4) { create(:user, name: 'John Doe', username: 'zzz3') }
-
- before do
- stub_const("#{described_class.name}::BATCH_SIZE", 3)
- stub_const("#{described_class.name}::LIMIT", 2)
- stub_feature_flags(group_users_autocomplete_using_batch_reduction: true)
- GroupMember.delete_all
- ProjectMember.delete_all
- group.add_members([user2, user3, user4], GroupMember::DEVELOPER)
- parent.add_members([user2, user3, user4], GroupMember::DEVELOPER)
- grandparent.add_members([user2, user3, user4], GroupMember::DEVELOPER)
- end
-
- it 'deduplicates the user_ids in batches to reduce database queries' do
- expect_next_instance_of(described_class) do |instance|
- expect(instance).to receive(:users_relation).once.and_call_original
- end
-
- # order is important. user2 comes first because it matches the username
- is_expected.to eq([user2, user3])
- end
- end
end
it { is_expected.to match_array([user1, external_user, omniauth_user, current_user]) }