Commit a1e8d2bf authored by Doug Stull's avatar Doug Stull Committed by Stan Hu

Caches the by email lookup logic for user in member creation

- reduce queries
- add query counter

Changelog: performance
parent 881dadc0
# frozen_string_literal: true
module BulkUsersByEmailLoad
extend ActiveSupport::Concern
included do
def users_by_emails(emails)
Gitlab::SafeRequestLoader.execute(resource_key: user_by_email_resource_key, resource_ids: emails) do |emails|
# have to consider all emails - even secondary, so use all_emails here
grouped_users_by_email = User.by_any_email(emails).preload(:emails).group_by(&:all_emails)
grouped_users_by_email.each_with_object({}) do |(found_emails, users), h|
found_emails.each { |e| h[e] = users.first if emails.include?(e) } # don't include all emails for an account, only the ones we want
end
end
end
private
def user_by_email_resource_key
"user_by_email_for_#{User.name.underscore.pluralize}:#{self.class}:#{self.id}"
end
end
end
......@@ -17,6 +17,7 @@ class Group < Namespace
include GroupAPICompatibility
include EachBatch
include BulkMemberAccessLoad
include BulkUsersByEmailLoad
include ChronicDurationAttribute
include RunnerTokenExpirationInterval
......
......@@ -37,6 +37,7 @@ class Project < ApplicationRecord
include EachBatch
include GitlabRoutingHelper
include BulkMemberAccessLoad
include BulkUsersByEmailLoad
include RunnerTokenExpirationInterval
include BlocksUnsafeSerialization
......
......@@ -51,12 +51,20 @@ module Members
users.concat(User.id_in(user_ids)) if user_ids.present?
users.uniq! # de-duplicate just in case as there is no controlling if user records and ids are sent multiple times
users_by_emails = source.users_by_emails(emails) # preloads our request store for all emails
# in case emails belong to a user that is being invited by user or user_id, remove them from
# emails and let users/user_ids handle it.
parsed_emails = emails.select do |email|
user = users_by_emails[email]
!user || (users.exclude?(user) && user_ids.exclude?(user.id))
end
if users.present?
# helps not have to perform another query per user id to see if the member exists later on when fetching
existing_members = source.members_and_requesters.where(user_id: users).index_by(&:user_id) # rubocop:disable CodeReuse/ActiveRecord
end
[emails, users, existing_members]
[parsed_emails, users, existing_members]
end
end
end
......
......@@ -114,7 +114,7 @@ module Members
User.find_by(id: user) # rubocop:todo CodeReuse/ActiveRecord
else
# must be an email or at least we'll consider it one
User.find_by_any_email(user) || user
source.users_by_emails([user])[user] || user
end
end
......
......@@ -52,7 +52,7 @@ RSpec.describe EpicsFinder do
context 'with correct params' do
before do
group.add_developer(search_user)
group.add_developer(search_user) if search_user
end
it 'returns all epics that belong to the given group' do
......
......@@ -108,7 +108,7 @@ RSpec.describe API::StatusChecks do
context 'when user has access' do
before do
stub_licensed_features(external_status_checks: true)
project.add_user(user, :maintainer)
project.add_user(user, :maintainer) if user
end
context 'when status_checks_add_status_field flag is disabled' do
......
......@@ -18,7 +18,7 @@ RSpec.describe Deployments::ApprovalService do
before do
stub_licensed_features(protected_environments: true)
create(:protected_environment, :maintainers_can_deploy, name: environment.name, project: project, required_approval_count: required_approval_count)
project.add_maintainer(user)
project.add_maintainer(user) if user
end
shared_examples_for 'error' do |message:|
......
......@@ -28,6 +28,8 @@ module API
optional :tasks_project_id, type: Integer, desc: 'The project ID in which to create the task issues'
end
post ":id/invitations" do
::Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab/-/issues/354016')
params[:source] = find_source(source_type, params[:id])
authorize_admin_source!(source_type, params[:source])
......
......@@ -36,7 +36,7 @@ RSpec.describe ApprovableBase do
subject { merge_request.can_be_approved_by?(user) }
before do
merge_request.project.add_developer(user)
merge_request.project.add_developer(user) if user
end
it 'returns true' do
......@@ -64,7 +64,7 @@ RSpec.describe ApprovableBase do
subject { merge_request.can_be_unapproved_by?(user) }
before do
merge_request.project.add_developer(user)
merge_request.project.add_developer(user) if user
end
it 'returns false' do
......
......@@ -293,6 +293,8 @@ RSpec.describe Group do
end
end
it_behaves_like 'a BulkUsersByEmailLoad model'
context 'traversal_ids on create' do
context 'default traversal_ids' do
let(:group) { build(:group) }
......
......@@ -635,6 +635,8 @@ RSpec.describe Project, factory_default: :keep do
end
end
it_behaves_like 'a BulkUsersByEmailLoad model'
describe '#all_pipelines' do
let_it_be(:project) { create(:project) }
......
......@@ -278,12 +278,90 @@ RSpec.describe API::Invitations do
it_behaves_like 'POST /:source_type/:id/invitations', 'project' do
let(:source) { project }
end
it 'records queries', :request_store, :use_sql_query_cache do
post invitations_url(project, maintainer), params: { email: email, access_level: Member::DEVELOPER }
control = ActiveRecord::QueryRecorder.new(skip_cached: false) do
post invitations_url(project, maintainer), params: { email: email2, access_level: Member::DEVELOPER }
end
emails = 'email3@example.com,email4@example.com,email5@example.com,email6@example.com,email7@example.com'
unresolved_n_plus_ones = 44 # old 48 with 12 per new email, currently there are 11 queries added per email
expect do
post invitations_url(project, maintainer), params: { email: emails, access_level: Member::DEVELOPER }
end.not_to exceed_all_query_limit(control.count).with_threshold(unresolved_n_plus_ones)
end
it 'records queries with secondary emails', :request_store, :use_sql_query_cache do
create(:email, email: email, user: create(:user))
post invitations_url(project, maintainer), params: { email: email, access_level: Member::DEVELOPER }
create(:email, email: email2, user: create(:user))
control = ActiveRecord::QueryRecorder.new(skip_cached: false) do
post invitations_url(project, maintainer), params: { email: email2, access_level: Member::DEVELOPER }
end
create(:email, email: 'email4@example.com', user: create(:user))
create(:email, email: 'email6@example.com', user: create(:user))
emails = 'email3@example.com,email4@example.com,email5@example.com,email6@example.com,email7@example.com'
unresolved_n_plus_ones = 67 # currently there are 11 queries added per email
expect do
post invitations_url(project, maintainer), params: { email: emails, access_level: Member::DEVELOPER }
end.not_to exceed_all_query_limit(control.count).with_threshold(unresolved_n_plus_ones)
end
end
describe 'POST /groups/:id/invitations' do
it_behaves_like 'POST /:source_type/:id/invitations', 'group' do
let(:source) { group }
end
it 'records queries', :request_store, :use_sql_query_cache do
post invitations_url(group, maintainer), params: { email: email, access_level: Member::DEVELOPER }
control = ActiveRecord::QueryRecorder.new(skip_cached: false) do
post invitations_url(group, maintainer), params: { email: email2, access_level: Member::DEVELOPER }
end
emails = 'email3@example.com,email4@example.com,email5@example.com,email6@example.com,email7@example.com'
unresolved_n_plus_ones = 36 # old 40 with 10 per new email, currently there are 9 queries added per email
expect do
post invitations_url(group, maintainer), params: { email: emails, access_level: Member::DEVELOPER }
end.not_to exceed_all_query_limit(control.count).with_threshold(unresolved_n_plus_ones)
end
it 'records queries with secondary emails', :request_store, :use_sql_query_cache do
create(:email, email: email, user: create(:user))
post invitations_url(group, maintainer), params: { email: email, access_level: Member::DEVELOPER }
create(:email, email: email2, user: create(:user))
control = ActiveRecord::QueryRecorder.new(skip_cached: false) do
post invitations_url(group, maintainer), params: { email: email2, access_level: Member::DEVELOPER }
end
create(:email, email: 'email4@example.com', user: create(:user))
create(:email, email: 'email6@example.com', user: create(:user))
emails = 'email3@example.com,email4@example.com,email5@example.com,email6@example.com,email7@example.com'
unresolved_n_plus_ones = 62 # currently there are 9 queries added per email
expect do
post invitations_url(group, maintainer), params: { email: emails, access_level: Member::DEVELOPER }
end.not_to exceed_all_query_limit(control.count).with_threshold(unresolved_n_plus_ones)
end
end
shared_examples 'GET /:source_type/:id/invitations' do |source_type|
......@@ -315,23 +393,6 @@ RSpec.describe API::Invitations do
end
end
it 'avoids N+1 queries' do
invite_member_by_email(source, source_type, email, maintainer)
# Establish baseline
get invitations_url(source, maintainer)
control = ActiveRecord::QueryRecorder.new do
get invitations_url(source, maintainer)
end
invite_member_by_email(source, source_type, email2, maintainer)
expect do
get invitations_url(source, maintainer)
end.not_to exceed_query_limit(control)
end
it 'does not find confirmed members' do
get invitations_url(source, maintainer)
......
......@@ -13,7 +13,7 @@ RSpec.describe API::ProjectImport, :aggregate_failures do
let(:namespace) { create(:group) }
before do
namespace.add_owner(user)
namespace.add_owner(user) if user
end
shared_examples 'requires authentication' do
......
......@@ -777,7 +777,7 @@ RSpec.describe API::Projects do
subject { get api('/projects', current_user), params: params }
before do
group_with_projects.add_owner(current_user)
group_with_projects.add_owner(current_user) if current_user
end
it 'returns non-public items based ordered by similarity' do
......
......@@ -452,7 +452,7 @@ RSpec.describe API::Repositories do
it "compare commits between different projects" do
group = create(:group)
group.add_owner(current_user)
group.add_owner(current_user) if current_user
forked_project = fork_project(project, current_user, repository: true, namespace: group)
forked_project.repository.create_ref('refs/heads/improve/awesome', 'refs/heads/improve/more-awesome')
......
......@@ -9,7 +9,7 @@ RSpec.describe API::V3::Github do
let_it_be_with_reload(:project) { create(:project, :repository, creator: user) }
before do
project.add_maintainer(user)
project.add_maintainer(user) if user
end
describe 'GET /orgs/:namespace/repos' do
......
......@@ -342,7 +342,7 @@ RSpec.describe Ci::RetryBuildService do
end
shared_examples_for 'when build with dynamic environment is retried' do
let_it_be(:other_developer) { create(:user).tap { |u| project.add_developer(other_developer) } }
let_it_be(:other_developer) { create(:user).tap { |u| project.add_developer(u) } }
let(:environment_name) { 'review/$CI_COMMIT_REF_SLUG-$GITLAB_USER_ID' }
......
......@@ -15,7 +15,7 @@ RSpec.describe Metrics::Dashboard::CustomDashboardService, :use_clean_rails_memo
subject { described_class.new(*service_params) }
before do
project.add_maintainer(user)
project.add_maintainer(user) if user
end
describe '#raw_dashboard' do
......
......@@ -10,7 +10,7 @@ RSpec.describe Metrics::Dashboard::CustomMetricEmbedService do
let_it_be(:environment) { create(:environment, project: project) }
before do
project.add_maintainer(user)
project.add_maintainer(user) if user
end
let(:dashboard_path) { system_dashboard_path }
......
......@@ -10,7 +10,7 @@ RSpec.describe Metrics::Dashboard::DefaultEmbedService, :use_clean_rails_memory_
let_it_be(:environment) { create(:environment, project: project) }
before do
project.add_maintainer(user)
project.add_maintainer(user) if user
end
describe '.valid_params?' do
......
......@@ -10,7 +10,7 @@ RSpec.describe Metrics::Dashboard::DynamicEmbedService, :use_clean_rails_memory_
let_it_be(:environment) { create(:environment, project: project) }
before do
project.add_maintainer(user)
project.add_maintainer(user) if user
end
let(:dashboard_path) { '.gitlab/dashboards/test.yml' }
......
......@@ -12,7 +12,7 @@ RSpec.describe Metrics::Dashboard::SelfMonitoringDashboardService, :use_clean_ra
let(:service_params) { [project, user, { environment: environment }] }
before do
project.add_maintainer(user)
project.add_maintainer(user) if user
stub_application_setting(self_monitoring_project_id: project.id)
end
......
......@@ -15,7 +15,7 @@ RSpec.describe Metrics::Dashboard::SystemDashboardService, :use_clean_rails_memo
subject { described_class.new(*service_params) }
before do
project.add_maintainer(user)
project.add_maintainer(user) if user
end
describe '#raw_dashboard' do
......
......@@ -8,7 +8,7 @@ RSpec.describe Metrics::Dashboard::TransientEmbedService, :use_clean_rails_memor
let_it_be(:environment) { create(:environment, project: project) }
before do
project.add_maintainer(user)
project.add_maintainer(user) if user
end
describe '.valid_params?' do
......
......@@ -5,7 +5,7 @@ require 'spec_helper'
RSpec.describe NotificationRecipients::Builder::Default do
describe '#build!' do
let_it_be(:group) { create(:group, :public) }
let_it_be(:project) { create(:project, :public, group: group).tap { |p| p.add_developer(project_watcher) } }
let_it_be(:project) { create(:project, :public, group: group).tap { |p| p.add_developer(project_watcher) if project_watcher } }
let_it_be(:target) { create(:issue, project: project) }
let_it_be(:current_user) { create(:user) }
......
......@@ -13,7 +13,7 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService, :clean_gitlab_
let(:tags) { %w[latest A Ba Bb C D E] }
before do
project.add_maintainer(user)
project.add_maintainer(user) if user
stub_container_registry_config(enabled: true)
......
# frozen_string_literal: true
RSpec.shared_examples 'a BulkUsersByEmailLoad model' do
describe '#users_by_emails' do
let_it_be(:user1) { create(:user, emails: [create(:email, email: 'user1@example.com')]) }
let_it_be(:user2) { create(:user, emails: [create(:email, email: 'user2@example.com')]) }
subject(:model) { described_class.new(id: non_existing_record_id) }
context 'when nothing is loaded' do
let(:passed_emails) { [user1.emails.first.email, user2.email] }
it 'preforms the yielded query and supplies the data with only emails desired' do
expect(model.users_by_emails(passed_emails).keys).to contain_exactly(*passed_emails)
end
end
context 'when store is preloaded', :request_store do
let(:passed_emails) { [user1.emails.first.email, user2.email, user1.email] }
let(:resource_data) do
{
user1.emails.first.email => instance_double('User'),
user2.email => instance_double('User')
}
end
before do
Gitlab::SafeRequestStore["user_by_email_for_users:#{model.class.name}:#{model.id}"] = resource_data
end
it 'passes back loaded data and does not update the items that already exist' do
users_by_emails = model.users_by_emails(passed_emails)
expect(users_by_emails.keys).to contain_exactly(*passed_emails)
expect(users_by_emails).to include(resource_data.merge(user1.email => user1))
end
end
end
end
......@@ -410,6 +410,22 @@ RSpec.shared_examples_for "bulk member creation" do
end
end
it 'with the same user sent more than once by user and by email' do
members = described_class.add_users(source, [user1, user1.email], :maintainer)
expect(members.map(&:user)).to contain_exactly(user1)
expect(members).to all(be_a(member_type))
expect(members).to all(be_persisted)
end
it 'with the same user sent more than once by user id and by email' do
members = described_class.add_users(source, [user1.id, user1.email], :maintainer)
expect(members.map(&:user)).to contain_exactly(user1)
expect(members).to all(be_a(member_type))
expect(members).to all(be_persisted)
end
context 'when a member already exists' do
before do
source.add_user(user1, :developer)
......
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