Commit 886360d9 authored by Adam Hegyi's avatar Adam Hegyi

Eliminate Profiles::NotificationsController N+1

parent b57d8696
......@@ -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
......@@ -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)
......
......@@ -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
......
......@@ -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
......
---
title: Eliminage N+1 database queries on the user notifications page
merge_request: 58397
author:
type: performance
......@@ -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 }
......
......@@ -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
......@@ -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
......@@ -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
......@@ -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
......
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