Merge branch '21043-fix-more-n-plus-one-queries' into 'master'

Fix more N+1 on notification settings page

See merge request gitlab-org/gitlab!59029

app/controllers/profiles/notifications_controller.rb

@@ -3,18 +3,13 @@
 class Profiles::NotificationsController < Profiles::ApplicationController
   feature_category :users
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def show
     @user = current_user
     @user_groups = user_groups
     @group_notifications = UserGroupNotificationSettingsFinder.new(current_user, user_groups).execute
-
-    @project_notifications = current_user.notification_settings.for_projects.order(:id)
-                             .preload_source_route
-                             .select { |notification| current_user.can?(:read_project, notification.source) }
+    @project_notifications = project_notifications_with_preloaded_associations
     @global_notification_setting = current_user.global_notification_setting
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
   def update
     result = Users::UpdateService.new(current_user, user_params.merge(user: current_user)).execute
@@ -37,4 +32,20 @@ class Profiles::NotificationsController < Profiles::ApplicationController
   def user_groups
     GroupsFinder.new(current_user, all_available: false).execute.order_name_asc.page(params[:page])
   end
+
+  # rubocop: disable CodeReuse/ActiveRecord
+  def project_notifications_with_preloaded_associations
+    project_notifications = current_user
+      .notification_settings
+      .for_projects
+      .order_by_id_asc
+      .preload_source_route
+
+    projects = project_notifications.map(&:source)
+    ActiveRecord::Associations::Preloader.new.preload(projects, { namespace: [:route, :owner], group: [] })
+    Preloaders::UserMaxAccessLevelInProjectsPreloader.new(projects, current_user).execute
+
+    project_notifications.select { |notification| current_user.can?(:read_project, notification.source) }
+  end
+  # rubocop: enable CodeReuse/ActiveRecord
 end

app/models/concerns/bulk_member_access_load.rb

@@ -13,13 +13,7 @@ module BulkMemberAccessLoad
       raise 'Block is mandatory' unless block_given?
 
       resource_ids = resource_ids.uniq
-      key = max_member_access_for_resource_key(resource_klass, memoization_index)
-      access = {}
-
-      if Gitlab::SafeRequestStore.active?
-        Gitlab::SafeRequestStore[key] ||= {}
-        access = Gitlab::SafeRequestStore[key]
-      end
+      access = load_access_hash(resource_klass, memoization_index)
 
       # Look up only the IDs we need
       resource_ids -= access.keys
@@ -39,10 +33,28 @@ module BulkMemberAccessLoad
       access
     end
 
+    def merge_value_to_request_store(resource_klass, resource_id, memoization_index, value)
+      max_member_access_for_resource_ids(resource_klass, [resource_id], memoization_index) do
+        { resource_id => value }
+      end
+    end
+
     private
 
     def max_member_access_for_resource_key(klass, memoization_index)
       "max_member_access_for_#{klass.name.underscore.pluralize}:#{memoization_index}"
     end
+
+    def load_access_hash(resource_klass, memoization_index)
+      key = max_member_access_for_resource_key(resource_klass, memoization_index)
+
+      access = {}
+      if Gitlab::SafeRequestStore.active?
+        Gitlab::SafeRequestStore[key] ||= {}
+        access = Gitlab::SafeRequestStore[key]
+      end
+
+      access
+    end
   end
 end

app/models/notification_setting.rb

@@ -30,6 +30,8 @@ class NotificationSetting < ApplicationRecord
 
   scope :preload_source_route, -> { preload(source: [:route]) }
 
+  scope :order_by_id_asc, -> { order(id: :asc) }
+
   # NOTE: Applicable unfound_translations.rb also needs to be updated when below events are changed.
   EMAIL_EVENTS = [
     :new_release,

app/models/preloaders/user_max_access_level_in_projects_preloader.rb

+# frozen_string_literal: true
+
+module Preloaders
+  # This class preloads the max access level for the user within the given projects and
+  # stores the values in requests store via the ProjectTeam class.
+  class UserMaxAccessLevelInProjectsPreloader
+    def initialize(projects, user)
+      @projects = projects
+      @user = user
+    end
+
+    def execute
+      access_levels = @user
+        .project_authorizations
+        .where(project_id: @projects)
+        .group(:project_id)
+        .maximum(:access_level)
+
+      @projects.each do |project|
+        access_level = access_levels[project.id] || Gitlab::Access::NO_ACCESS
+        ProjectTeam.new(project).write_member_access_for_user_id(@user.id, access_level)
+      end
+    end
+  end
+end

app/models/project_team.rb

@@ -174,6 +174,10 @@ class ProjectTeam
     end
   end
 
+  def write_member_access_for_user_id(user_id, project_access_level)
+    merge_value_to_request_store(User, user_id, project.id, project_access_level)
+  end
+
   def max_member_access(user_id)
     max_member_access_for_user_ids([user_id])[user_id]
   end

changelogs/unreleased/21043-fix-more-n-plus-one-queries.yml

+---
+title: Eliminate N+1 database queries on the user notifications page within the project
+  notifications section
+merge_request: 59029
+author:
+type: performance

spec/controllers/profiles/notifications_controller_spec.rb

@@ -21,6 +21,30 @@ RSpec.describe Profiles::NotificationsController do
       expect(response).to render_template :show
     end
 
+    context 'when personal projects are present', :request_store do
+      let!(:personal_project_1) { create(:project, namespace: user.namespace) }
+
+      context 'N+1 query check' do
+        render_views
+
+        it 'does not have an N+1' do
+          sign_in(user)
+
+          get :show
+
+          control = ActiveRecord::QueryRecorder.new do
+            get :show
+          end
+
+          create_list(:project, 2, namespace: user.namespace)
+
+          expect do
+            get :show
+          end.not_to exceed_query_limit(control)
+        end
+      end
+    end
+
     context 'with groups that do not have notification preferences' do
       let_it_be(:group) { create(:group) }
       let_it_be(:subgroup) { create(:group, parent: group) }

spec/models/notification_setting_spec.rb

@@ -200,4 +200,18 @@ RSpec.describe NotificationSetting do
       subject.email_events
     end
   end
+
+  describe '#order_by_id_asc' do
+    let_it_be(:project) { create(:project) }
+    let_it_be(:other_project) { create(:project) }
+    let_it_be(:notification_setting_1) { create(:notification_setting, project: project) }
+    let_it_be(:notification_setting_2) { create(:notification_setting, project: other_project) }
+    let_it_be(:notification_setting_3) { create(:notification_setting, project: project) }
+
+    let(:ids) { [notification_setting_1, notification_setting_2, notification_setting_3].map(&:id) }
+
+    subject(:ordered_records) { described_class.where(id: ids, source: project).order_by_id_asc }
+
+    it { is_expected.to eq([notification_setting_1, notification_setting_3]) }
+  end
 end

spec/models/preloaders/user_max_access_level_in_projects_preloader_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Preloaders::UserMaxAccessLevelInProjectsPreloader do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:project_1) { create(:project) }
+  let_it_be(:project_2) { create(:project) }
+  let_it_be(:project_3) { create(:project) }
+
+  let(:projects) { [project_1, project_2, project_3] }
+
+  before do
+    project_1.add_developer(user)
+    project_2.add_developer(user)
+  end
+
+  context 'preload maximum access level to avoid querying project_authorizations', :request_store do
+    it 'avoids N+1 queries', :request_store do
+      Preloaders::UserMaxAccessLevelInProjectsPreloader.new(projects, user).execute
+
+      query_count = ActiveRecord::QueryRecorder.new do
+        projects.each { |project| user.can?(:read_project, project) }
+      end.count
+
+      expect(query_count).to eq(0)
+    end
+
+    it 'runs N queries without preloading' do
+      query_count = ActiveRecord::QueryRecorder.new do
+        projects.each { |project| user.can?(:read_project, project) }
+      end.count
+
+      expect(query_count).to eq(projects.size)
+    end
+  end
+end