From ebb284c0dc59e2fd5bc76921609b60034f000c53 Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Tue, 26 Feb 2019 11:30:43 -0800
Subject: [PATCH] Remove N+1 query for tags in /admin/runners page

As discussed in https://github.com/mbleigh/acts-as-taggable-on/issues/91,
we can avoid N+1 queries if we use `tags` instead of `tag_list`.

Seen while reviewing
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/19740.
---
 app/finders/admin/runners_finder.rb           |  2 +-
 app/models/ci/runner.rb                       |  1 +
 app/views/admin/runners/_runner.html.haml     |  2 +-
 .../sh-remove-nplusone-admin-runners-tags.yml |  5 +++++
 .../admin/runners_controller_spec.rb          | 19 ++++++++++++++++++-
 5 files changed, 26 insertions(+), 3 deletions(-)
 create mode 100644 changelogs/unreleased/sh-remove-nplusone-admin-runners-tags.yml

diff --git a/app/finders/admin/runners_finder.rb b/app/finders/admin/runners_finder.rb
index fbb1cfc5c66..8d936b8121c 100644
--- a/app/finders/admin/runners_finder.rb
+++ b/app/finders/admin/runners_finder.rb
@@ -14,7 +14,7 @@ class Admin::RunnersFinder < UnionFinder
     sort!
     paginate!
 
-    @runners
+    @runners.with_tags
   end
 
   def sort_key
diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index 5aae31de6e2..d82e11bbb89 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -97,6 +97,7 @@ module Ci
 
     scope :order_contacted_at_asc, -> { order(contacted_at: :asc) }
     scope :order_created_at_desc, -> { order(created_at: :desc) }
+    scope :with_tags, -> { preload(:tags) }
 
     validate :tag_constraints
     validates :access_level, presence: true
diff --git a/app/views/admin/runners/_runner.html.haml b/app/views/admin/runners/_runner.html.haml
index 4641986cb56..ecf2b1d60ba 100644
--- a/app/views/admin/runners/_runner.html.haml
+++ b/app/views/admin/runners/_runner.html.haml
@@ -49,7 +49,7 @@
   .table-section.section-10.section-wrap
     .table-mobile-header{ role: 'rowheader' }= _('Tags')
     .table-mobile-content
-      - runner.tag_list.sort.each do |tag|
+      - runner.tags.map(&:name).sort.each do |tag|
         %span.badge.badge-primary
           = tag
 
diff --git a/changelogs/unreleased/sh-remove-nplusone-admin-runners-tags.yml b/changelogs/unreleased/sh-remove-nplusone-admin-runners-tags.yml
new file mode 100644
index 00000000000..f8ac345bc95
--- /dev/null
+++ b/changelogs/unreleased/sh-remove-nplusone-admin-runners-tags.yml
@@ -0,0 +1,5 @@
+---
+title: Remove N+1 query for tags in /admin/runners page
+merge_request: 25572
+author:
+type: performance
diff --git a/spec/controllers/admin/runners_controller_spec.rb b/spec/controllers/admin/runners_controller_spec.rb
index 4cf14030ca1..82e24213408 100644
--- a/spec/controllers/admin/runners_controller_spec.rb
+++ b/spec/controllers/admin/runners_controller_spec.rb
@@ -1,18 +1,35 @@
 require 'spec_helper'
 
 describe Admin::RunnersController do
-  let(:runner) { create(:ci_runner) }
+  let!(:runner) { create(:ci_runner) }
 
   before do
     sign_in(create(:admin))
   end
 
   describe '#index' do
+    render_views
+
     it 'lists all runners' do
       get :index
 
       expect(response).to have_gitlab_http_status(200)
     end
+
+    it 'avoids N+1 queries', :request_store do
+      get :index
+
+      control_count = ActiveRecord::QueryRecorder.new { get :index }.count
+
+      create(:ci_runner, :tagged_only)
+
+      # There is still an N+1 query for `runner.builds.count`
+      expect { get :index }.not_to exceed_query_limit(control_count + 1)
+
+      expect(response).to have_gitlab_http_status(200)
+      expect(response.body).to have_content('tag1')
+      expect(response.body).to have_content('tag2')
+    end
   end
 
   describe '#show' do