From 3eed8ffc449bc9044d986bf69af05777dd2e242a Mon Sep 17 00:00:00 2001
From: nmilojevic1 <nmilojevic@gitlab.com>
Date: Wed, 23 Mar 2022 18:38:49 +0100
Subject: [PATCH] Fix discussions N+1 queries
- Add specs for html and text node
- Fix N plus one queries for new review email
Changelog: performance
---
app/mailers/emails/reviews.rb | 8 ++++++
app/models/note.rb | 1 +
app/views/notify/_note_email.html.haml | 4 +--
app/views/notify/_note_email.text.erb | 4 +--
app/views/notify/new_review_email.html.haml | 8 +++++-
app/views/notify/new_review_email.text.erb | 8 +++++-
spec/mailers/notify_spec.rb | 31 ++++++++++++++++++++-
7 files changed, 57 insertions(+), 7 deletions(-)
diff --git a/app/mailers/emails/reviews.rb b/app/mailers/emails/reviews.rb
index ddb9e161a80b8..5bbea1a5fe0c1 100644
--- a/app/mailers/emails/reviews.rb
+++ b/app/mailers/emails/reviews.rb
@@ -22,6 +22,8 @@ def setup_review_email(review_id, recipient_id)
review = Review.find_by_id(review_id)
@notes = review.notes
+ discussion_ids = @notes.pluck(:discussion_id)
+ @discussions = discussions(discussion_ids)
@author = review.author
@author_name = review.author_name
@project = review.project
@@ -29,5 +31,11 @@ def setup_review_email(review_id, recipient_id)
@target_url = project_merge_request_url(@project, @merge_request)
@sent_notification = SentNotification.record(@merge_request, recipient_id, reply_key)
end
+
+ def discussions(discussion_ids)
+ notes = Note.where(discussion_id: discussion_ids).inc_note_diff_file.fresh
+ grouped_notes = notes.group_by { |n| n.discussion_id }
+ grouped_notes.transform_values { |notes| Discussion.build(notes) }
+ end
end
end
diff --git a/app/models/note.rb b/app/models/note.rb
index 4f2e7ebe2c52a..8f51873544266 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -121,6 +121,7 @@ class Note < ApplicationRecord
scope :with_discussion_ids, ->(discussion_ids) { where(discussion_id: discussion_ids) }
scope :with_suggestions, -> { joins(:suggestions) }
scope :inc_author, -> { includes(:author) }
+ scope :inc_note_diff_file, -> { includes(:note_diff_file) }
scope :with_api_entity_associations, -> { preload(:note_diff_file, :author) }
scope :inc_relations_for_view, -> do
includes({ project: :group }, { author: :status }, :updated_by, :resolved_by, :award_emoji,
diff --git a/app/views/notify/_note_email.html.haml b/app/views/notify/_note_email.html.haml
index d5398337320c9..7b7a7d5b995cc 100644
--- a/app/views/notify/_note_email.html.haml
+++ b/app/views/notify/_note_email.html.haml
@@ -4,7 +4,7 @@
- note_style = local_assigns.fetch(:note_style, "")
- skip_stylesheet_link = local_assigns.fetch(:skip_stylesheet_link, false)
-- discussion = note.discussion if note.part_of_discussion?
+- discussion = local_assigns.fetch(:discussion){ note.discussion } if note.part_of_discussion?
%p{ style: "color: #777777;" }
= succeed ':' do
@@ -12,7 +12,7 @@
- if discussion.nil?
= link_to 'commented', target_url
- else
- - if note.start_of_discussion?
+ - if discussion.first_note == note
started a new
- else
commented on a
diff --git a/app/views/notify/_note_email.text.erb b/app/views/notify/_note_email.text.erb
index 8e2f7e6f76ef4..8b8b75cb40dca 100644
--- a/app/views/notify/_note_email.text.erb
+++ b/app/views/notify/_note_email.text.erb
@@ -1,13 +1,13 @@
<% note = local_assigns.fetch(:note, @note) -%>
<% diff_limit = local_assigns.fetch(:diff_limit, nil) -%>
<% target_url = local_assigns.fetch(:target_url, @target_url) -%>
-<% discussion = note.discussion if note.part_of_discussion? -%>
+<% discussion = local_assigns.fetch(:discussion){ note.discussion } if note.part_of_discussion? -%>
<%= sanitize_name(note.author_name) -%>
<% if discussion.nil? -%>
<%= 'commented' -%>:
<% else -%>
-<% if note.start_of_discussion? -%>
+<% if discussion.first_note == note -%>
<%= 'started a new discussion' -%>
<% else -%>
<%= 'commented on a discussion' -%>
diff --git a/app/views/notify/new_review_email.html.haml b/app/views/notify/new_review_email.html.haml
index d5399c872d5cb..c0ad773caafd3 100644
--- a/app/views/notify/new_review_email.html.haml
+++ b/app/views/notify/new_review_email.html.haml
@@ -15,5 +15,11 @@
%tr
%td{ style: "overflow:hidden;font-size:14px;line-height:1.4;display:grid;" }
- @notes.each do |note|
+ -# Preload author, to prevent N+1 queries
+ - note.author = @author
+ -# Get preloaded note discussion
+ - discussion = @discussions.try(:[], note.discussion_id) if note.part_of_discussion?
+ -# Preload project for discussions first note
+ - discussion.first_note.project = @project if discussion&.first_note
- target_url = project_merge_request_url(@project, @merge_request, anchor: "note_#{note.id}")
- = render 'note_email', note: note, diff_limit: 3, target_url: target_url, note_style: "border-bottom:1px solid #ededed;", skip_stylesheet_link: true
+ = render 'note_email', note: note, diff_limit: 3, target_url: target_url, note_style: "border-bottom:1px solid #ededed;", skip_stylesheet_link: true, discussion: discussion
diff --git a/app/views/notify/new_review_email.text.erb b/app/views/notify/new_review_email.text.erb
index 164735abad0f8..a6899a65f2bbd 100644
--- a/app/views/notify/new_review_email.text.erb
+++ b/app/views/notify/new_review_email.text.erb
@@ -4,8 +4,14 @@
--
<% @notes.each_with_index do |note, index| %>
+ <!-- Preload author, to prevent N+1 queries-->
+ <% note.author = @author %>
+ <!-- Get preloaded note discussion-->
+ <% discussion = @discussions.try(:[], note.discussion_id) if note.part_of_discussion?%>
+ <!-- Preload project for discussions first note-->
+ <% discussion.first_note.project = @project if discussion&.first_note %>
<% target_url = project_merge_request_url(@project, @merge_request, anchor: "note_#{note.id}") %>
- <%= render 'note_email', note: note, diff_limit: 3, target_url: target_url %>
+ <%= render 'note_email', note: note, diff_limit: 3, target_url: target_url, discussion: discussion %>
<% if index != @notes.length-1 %>
--
diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb
index e2ee63078bbc0..8aefaadae2e78 100644
--- a/spec/mailers/notify_spec.rb
+++ b/spec/mailers/notify_spec.rb
@@ -2181,18 +2181,47 @@ def invite_to_group(group, inviter:, user: nil, tasks_to_be_done: [])
context 'when diff note' do
let!(:notes) { create_list(:diff_note_on_merge_request, 3, review: review, project: project, author: review.author, noteable: merge_request) }
- it 'links to notes' do
+ it 'links to notes and discussions', :aggregate_failures do
+
+ reply_note = create(:diff_note_on_merge_request, review: review, project: project, author: review.author, noteable: merge_request, in_reply_to: notes.first)
+
review.notes.each do |note|
# Text part
expect(subject.text_part.body.raw_source).to include(
project_merge_request_url(project, merge_request, anchor: "note_#{note.id}")
)
+
+ if note == reply_note
+ expect(subject.text_part.body.raw_source).to include("commented on a discussion on #{note.discussion.file_path}")
+ else
+ expect(subject.text_part.body.raw_source).to include("started a new discussion on #{note.discussion.file_path}")
+ end
end
end
it 'includes only one link to the highlighted_diff_email' do
expect(subject.html_part.body.raw_source).to include('assets/mailers/highlighted_diff_email').once
end
+
+ it 'avoids N+1 cached queries when rendering html', :use_sql_query_cache, :request_store do
+ control_count = ActiveRecord::QueryRecorder.new(query_recorder_debug: true, skip_cached: false) do
+ subject.html_part
+ end
+
+ create_list(:diff_note_on_merge_request, 3, review: review, project: project, author: review.author, noteable: merge_request)
+
+ expect { described_class.new_review_email(recipient.id, review.id).html_part }.not_to exceed_all_query_limit(control_count)
+ end
+
+ it 'avoids N+1 cached queries when rendering text', :use_sql_query_cache, :request_store do
+ control_count = ActiveRecord::QueryRecorder.new(query_recorder_debug: true, skip_cached: false) do
+ subject.text_part
+ end
+
+ create_list(:diff_note_on_merge_request, 3, review: review, project: project, author: review.author, noteable: merge_request)
+
+ expect { described_class.new_review_email(recipient.id, review.id).text_part }.not_to exceed_all_query_limit(control_count)
+ end
end
it 'contains review author name' do
--
GitLab