Skip to content
代码片段 群组 项目
提交 dff22bf8 编辑于 作者: Alper Akgun's avatar Alper Akgun
浏览文件

Merge branch 'rolling-back-solving-cross-joins-for-group-users-autocomplete' into 'master' 

Revert "Resolving cross joins on GroupUsersFinder"

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/138534



Merged-by: default avatarAlper Akgun <aakgun@gitlab.com>
Approved-by: default avatarAlper Akgun <aakgun@gitlab.com>
Approved-by: default avatarHeinrich Lee Yu <heinrich@gitlab.com>
Co-authored-by: default avatarOmar Qunsul <oqunsul@gitlab.com>
上级 0d1eb71f 6f104b16
No related branches found
No related tags found
无相关合并请求
隐藏空白变更内容
行内 左右并排
app/finders/autocomplete/group_users_finder.rb
+ 6
− 9
浏览文件 @ dff22bf8
......@@ -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
......
app/finders/autocomplete/users_finder.rb
+ 10
− 35
浏览文件 @ dff22bf8
......@@ -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
......
config/feature_flags/development/group_users_autocomplete_using_batch_reduction.yml 已删除 100644 → 0
+ 0
− 8
浏览文件 @ 0d1eb71f
---
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
spec/finders/autocomplete/group_users_finder_spec.rb
+ 1
− 16
浏览文件 @ dff22bf8
......@@ -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
......
spec/finders/autocomplete/users_finder_spec.rb
+ 1
− 112
浏览文件 @ dff22bf8
......@@ -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]) }
......
0% 或 .
You are about to add 0 people to the discussion. Proceed with caution.
先完成此消息的编辑！
想要评论请 注册 或