Eager load event target authors whenever possibleconditionally-eager-load-event-target-authors

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.

4 files changed, 40 insertions, 1 deletions

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)