Merge branch '21043-fix-notifications-controller-n-plus-1-queries' into 'master'

Eliminate Profiles::NotificationsController N+1

See merge request gitlab-org/gitlab!58397

app/finders/user_group_notification_settings_finder.rb

@@ -14,6 +14,8 @@ class UserGroupNotificationSettingsFinder
     @loaded_groups_with_ancestors = groups_with_ancestors.index_by(&:id)
     @loaded_notification_settings = user.notification_settings_for_groups(groups_with_ancestors).preload_source_route.index_by(&:source_id)
 
+    preload_emails_disabled
+
     groups.map do |group|
       find_notification_setting_for(group)
     end
@@ -45,4 +47,19 @@ class UserGroupNotificationSettingsFinder
 
     parent_setting.level != NotificationSetting.levels[:global] || parent_setting.notification_email.present?
   end
+
+  # This method preloads the `emails_disabled` strong memoized method for the given groups.
+  #
+  # For each group, look up the ancestor hierarchy and look for any group where emails_disabled is true.
+  # The lookup is implemented with an EXISTS subquery, so we can look up the ancestor chain for each group individually.
+  # The query will return groups where at least one ancestor has the `emails_disabled` set to true.
+  #
+  # After the query, we set the instance variable.
+  def preload_emails_disabled
+    group_ids_with_disabled_email = Group.ids_with_disabled_email(groups.to_a)
+
+    groups.each do |group|
+      group.emails_disabled_memoized = group_ids_with_disabled_email.include?(group.id) if group.parent_id
+    end
+  end
 end

app/models/group.rb

@@ -180,6 +180,25 @@ class Group < Namespace
       groups.drop(1).each { |group| group.root_ancestor = root }
     end
 
+    # Returns the ids of the passed group models where the `emails_disabled`
+    # column is set to true anywhere in the ancestor hierarchy.
+    def ids_with_disabled_email(groups)
+      innner_query = Gitlab::ObjectHierarchy
+        .new(Group.where('id = namespaces_with_emails_disabled.id'))
+        .base_and_ancestors
+        .where(emails_disabled: true)
+        .select('1')
+        .limit(1)
+
+      group_ids = Namespace
+        .from('(SELECT * FROM namespaces) as namespaces_with_emails_disabled')
+        .where(namespaces_with_emails_disabled: { id: groups })
+        .where('EXISTS (?)', innner_query)
+        .pluck(:id)
+
+      Set.new(group_ids)
+    end
+
     private
 
     def public_to_user_arel(user)

app/models/namespace.rb

@@ -111,7 +111,7 @@ class Namespace < ApplicationRecord
 
   # Make sure that the name is same as strong_memoize name in root_ancestor
   # method
-  attr_writer :root_ancestor
+  attr_writer :root_ancestor, :emails_disabled_memoized
 
   class << self
     def by_path(path)
@@ -239,7 +239,7 @@ class Namespace < ApplicationRecord
 
   # any ancestor can disable emails for all descendants
   def emails_disabled?
-    strong_memoize(:emails_disabled) do
+    strong_memoize(:emails_disabled_memoized) do
       if parent_id
         self_and_ancestors.where(emails_disabled: true).exists?
       else

app/models/user.rb

@@ -1358,10 +1358,12 @@ class User < ApplicationRecord
   end
 
   def public_verified_emails
+    strong_memoize(:public_verified_emails) do
       emails = verified_emails(include_private_email: false)
       emails << email unless temp_oauth_email?
       emails.uniq
     end
+  end
 
   def any_email?(check_email)
     downcased = check_email.downcase

changelogs/unreleased/21043-fix-notifications-controller-n-plus-1-queries.yml

+---
+title: Eliminage N+1 database queries on the user notifications page
+merge_request: 58397
+author:
+type: performance

spec/controllers/profiles/notifications_controller_spec.rb

@@ -37,9 +37,14 @@ RSpec.describe Profiles::NotificationsController do
         expect(assigns(:group_notifications).map(&:source_id)).to include(subgroup.id)
       end
 
+      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
@@ -51,6 +56,7 @@ RSpec.describe Profiles::NotificationsController do
           end.not_to exceed_query_limit(control)
         end
       end
+    end
 
     context 'with group notifications' do
       let(:notifications_per_page) { 5 }

spec/features/groups/group_settings_spec.rb

@@ -175,7 +175,7 @@ RSpec.describe 'Edit group settings' do
   end
 
   def updated_emails_disabled?
-    group.reload.clear_memoization(:emails_disabled)
+    group.reload.clear_memoization(:emails_disabled_memoized)
     group.emails_disabled?
   end
 end

spec/finders/user_group_notification_settings_finder_spec.rb

@@ -129,4 +129,37 @@ RSpec.describe UserGroupNotificationSettingsFinder do
       end
     end
   end
+
+  context 'preloading `emails_disabled`' do
+    let_it_be(:root_group) { create(:group) }
+    let_it_be(:sub_group) { create(:group, parent: root_group) }
+    let_it_be(:sub_sub_group) { create(:group, parent: sub_group) }
+
+    let_it_be(:another_root_group) { create(:group) }
+    let_it_be(:sub_group_with_emails_disabled) { create(:group, emails_disabled: true, parent: another_root_group) }
+    let_it_be(:another_sub_sub_group) { create(:group, parent: sub_group_with_emails_disabled) }
+
+    let_it_be(:root_group_with_emails_disabled) { create(:group, emails_disabled: true) }
+    let_it_be(:group) { create(:group, parent: root_group_with_emails_disabled) }
+
+    let(:groups) { Group.where(id: [sub_sub_group, another_sub_sub_group, group]) }
+
+    before do
+      described_class.new(user, groups).execute
+    end
+
+    it 'preloads the `group.emails_disabled` method' do
+      recorder = ActiveRecord::QueryRecorder.new do
+        groups.each(&:emails_disabled?)
+      end
+
+      expect(recorder.count).to eq(0)
+    end
+
+    it 'preloads the `group.emails_disabled` method correctly' do
+      groups.each do |group|
+        expect(group.emails_disabled?).to eq(Group.find(group.id).emails_disabled?) # compare the memoized and the freshly loaded value
+      end
+    end
+  end
 end

spec/models/group_spec.rb

@@ -2232,4 +2232,18 @@ RSpec.describe Group do
 
     it_behaves_like 'model with Debian distributions'
   end
+
+  describe '.ids_with_disabled_email' do
+    let!(:parent_1) { create(:group, emails_disabled: true) }
+    let!(:child_1) { create(:group, parent: parent_1) }
+
+    let!(:parent_2) { create(:group, emails_disabled: false) }
+    let!(:child_2) { create(:group, parent: parent_2) }
+
+    let!(:other_group) { create(:group, emails_disabled: false) }
+
+    subject(:group_ids_where_email_is_disabled) { described_class.ids_with_disabled_email([child_1, child_2, other_group]) }
+
+    it { is_expected.to eq(Set.new([child_1.id])) }
+  end
 end

spec/support/shared_examples/services/notification_service_shared_examples.rb

@@ -45,7 +45,7 @@ RSpec.shared_examples 'group emails are disabled' do
 
   before do
     reset_delivered_emails!
-    target_group.clear_memoization(:emails_disabled)
+    target_group.clear_memoization(:emails_disabled_memoized)
   end
 
   it 'sends no emails with group emails disabled' do