Merge branch '354016-resolve-growing-query-with-more-invites' into 'master'

Look for existing users by email and cache in member add process

See merge request gitlab-org/gitlab!82196

app/models/concerns/bulk_users_by_email_load.rb

+# 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

app/models/group.rb

@@ -17,6 +17,7 @@ class Group < Namespace
   include GroupAPICompatibility
   include EachBatch
   include BulkMemberAccessLoad
+  include BulkUsersByEmailLoad
   include ChronicDurationAttribute
   include RunnerTokenExpirationInterval
 

app/models/project.rb

@@ -37,6 +37,7 @@ class Project < ApplicationRecord
   include EachBatch
   include GitlabRoutingHelper
   include BulkMemberAccessLoad
+  include BulkUsersByEmailLoad
   include RunnerTokenExpirationInterval
   include BlocksUnsafeSerialization
 

app/services/concerns/members/bulk_create_users.rb

@@ -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.with_user(users).index_by(&:user_id)
           end
 
-          [emails, users, existing_members]
+          [parsed_emails, users, existing_members]
         end
       end
     end

app/services/members/creator_service.rb

@@ -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
 

ee/spec/finders/epics_finder_spec.rb

@@ -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

ee/spec/requests/api/status_checks_spec.rb

@@ -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

ee/spec/services/deployments/approval_service_spec.rb

@@ -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:|

lib/api/invitations.rb

@@ -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])

spec/models/concerns/approvable_base_spec.rb

@@ -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

spec/models/group_spec.rb

@@ -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) }

spec/models/project_spec.rb

@@ -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) }
 

spec/requests/api/invitations_spec.rb

@@ -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)
 

spec/requests/api/project_import_spec.rb

@@ -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

spec/requests/api/projects_spec.rb

@@ -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

spec/requests/api/repositories_spec.rb

@@ -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')

spec/requests/api/v3/github_spec.rb

@@ -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

spec/services/ci/retry_build_service_spec.rb

@@ -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' }
 

spec/services/metrics/dashboard/custom_dashboard_service_spec.rb

@@ -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

spec/services/metrics/dashboard/custom_metric_embed_service_spec.rb

@@ -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 }

spec/services/metrics/dashboard/default_embed_service_spec.rb

@@ -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

spec/services/metrics/dashboard/dynamic_embed_service_spec.rb

@@ -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' }

spec/services/metrics/dashboard/self_monitoring_dashboard_service_spec.rb

@@ -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
 

spec/services/metrics/dashboard/system_dashboard_service_spec.rb

@@ -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

spec/services/metrics/dashboard/transient_embed_service_spec.rb

@@ -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

spec/services/notification_recipients/builder/default_spec.rb

@@ -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) }

spec/services/projects/container_repository/cleanup_tags_service_spec.rb

@@ -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)
 

spec/support/shared_examples/models/concerns/bulk_users_by_email_load_shared_examples.rb

+# 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

spec/support/shared_examples/models/member_shared_examples.rb

@@ -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)