From dac51ace521d7b2b2a5a5bb19167a8690ead242e Mon Sep 17 00:00:00 2001
From: Yorick Peterse <yorickpeterse@gmail.com>
Date: Wed, 3 Jan 2018 17:44:29 +0100
Subject: [PATCH] Eager load event target authors whenever possible

This ensures that the "author" association of an event's "target"
association is eager loaded whenever the "target" association defines an
"author" association. This in turn solves the N+1 query problem we first
tried to solve in
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/15788 but caused
problems when displaying milestones as those don't define an "author"
association.

The approach in this commit does mean that the authors are _always_
eager loaded since this takes place in the "belongs_to" block. This
however shouldn't pose too much of a problem, and as far as I can tell
there's no real way around this unfortunately.
---
 app/models/event.rb                              | 13 ++++++++++++-
 ...itionally-eager-load-event-target-authors.yml |  5 +++++
 spec/features/dashboard/activity_spec.rb         |  7 +++++++
 spec/models/event_spec.rb                        | 16 ++++++++++++++++
 4 files changed, 40 insertions(+), 1 deletion(-)
 create mode 100644 changelogs/unreleased/conditionally-eager-load-event-target-authors.yml

diff --git a/app/models/event.rb b/app/models/event.rb
index 0997b056c6a..8a79100de5a 100644
--- a/app/models/event.rb
+++ b/app/models/event.rb
@@ -48,7 +48,18 @@ class Event < ActiveRecord::Base
 
   belongs_to :author, class_name: "User"
   belongs_to :project
-  belongs_to :target, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations
+
+  belongs_to :target, -> {
+    # If the association for "target" defines an "author" association we want to
+    # eager-load this so Banzai & friends don't end up performing N+1 queries to
+    # get the authors of notes, issues, etc.
+    if reflections['events'].active_record.reflect_on_association(:author)
+      includes(:author)
+    else
+      self
+    end
+  }, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations
+
   has_one :push_event_payload
 
   # Callbacks
diff --git a/changelogs/unreleased/conditionally-eager-load-event-target-authors.yml b/changelogs/unreleased/conditionally-eager-load-event-target-authors.yml
new file mode 100644
index 00000000000..a5f1a958fa8
--- /dev/null
+++ b/changelogs/unreleased/conditionally-eager-load-event-target-authors.yml
@@ -0,0 +1,5 @@
+---
+title: Eager load event target authors whenever possible
+merge_request:
+author:
+type: performance
diff --git a/spec/features/dashboard/activity_spec.rb b/spec/features/dashboard/activity_spec.rb
index bd115785646..a74a8aac2b2 100644
--- a/spec/features/dashboard/activity_spec.rb
+++ b/spec/features/dashboard/activity_spec.rb
@@ -24,6 +24,7 @@ feature 'Dashboard > Activity' do
     end
 
     let(:note) { create(:note, project: project, noteable: merge_request) }
+    let(:milestone) { create(:milestone, :active, project: project, title: '1.0') }
 
     let!(:push_event) do
       event = create(:push_event, project: project, author: user)
@@ -54,6 +55,10 @@ feature 'Dashboard > Activity' do
       create(:event, :commented, project: project, target: note, author: user)
     end
 
+    let!(:milestone_event) do
+      create(:event, :closed, project: project, target: milestone, author: user)
+    end
+
     before do
       project.add_master(user)
 
@@ -68,6 +73,7 @@ feature 'Dashboard > Activity' do
         expect(page).to have_content('accepted')
         expect(page).to have_content('closed')
         expect(page).to have_content('commented on')
+        expect(page).to have_content('closed milestone')
       end
     end
 
@@ -107,6 +113,7 @@ feature 'Dashboard > Activity' do
         expect(page).not_to have_content('accepted')
         expect(page).to have_content('closed')
         expect(page).not_to have_content('commented on')
+        expect(page).to have_content('closed milestone')
       end
     end
 
diff --git a/spec/models/event_spec.rb b/spec/models/event_spec.rb
index e999192940c..67f49348acb 100644
--- a/spec/models/event_spec.rb
+++ b/spec/models/event_spec.rb
@@ -347,6 +347,22 @@ describe Event do
     end
   end
 
+  describe '#target' do
+    it 'eager loads the author of an event target' do
+      create(:closed_issue_event)
+
+      events = described_class.preload(:target).all.to_a
+      count = ActiveRecord::QueryRecorder
+        .new { events.first.target.author }.count
+
+      # This expectation exists to make sure the test doesn't pass when the
+      # author is for some reason not loaded at all.
+      expect(events.first.target.author).to be_an_instance_of(User)
+
+      expect(count).to be_zero
+    end
+  end
+
   def create_push_event(project, user)
     event = create(:push_event, project: project, author: user)
 
-- 
GitLab