Use Goldiloader for handling N+1 queries

Goldiloader (https://github.com/salsify/goldiloader) can eager load
associations automatically. This removes the need for adding "includes"
calls in a variety of different places. This also comes with the added
benefit of not having to eager load data if it's not used.

12 files changed, 39 insertions, 21 deletions

diff --git a/Gemfile b/Gemfile
index 7f5b1f1d3ef..647138cd90f 100644
--- a/Gemfile
+++ b/Gemfile
@@ -441,3 +441,5 @@ gem 'grape_logging', '~> 1.7'
 
 # Asset synchronization
 gem 'asset_sync', '~> 2.2.0'
+
+gem 'goldiloader', '~> 2.0'
diff --git a/Gemfile.lock b/Gemfile.lock
index a1150dfccdd..76e1a17155f 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -320,6 +320,9 @@ GEM
       rubyntlm (~> 0.5)
     globalid (0.4.1)
       activesupport (>= 4.2.0)
+    goldiloader (2.0.1)
+      activerecord (>= 4.2, < 5.2)
+      activesupport (>= 4.2, < 5.2)
     gollum-grit_adapter (1.0.1)
       gitlab-grit (~> 2.7, >= 2.7.1)
     gollum-lib (4.2.7)
@@ -1067,6 +1070,7 @@ DEPENDENCIES
   gitlab-markup (~> 1.6.2)
   gitlab-styles (~> 2.3)
   gitlab_omniauth-ldap (~> 2.0.4)
+  goldiloader (~> 2.0)
   gollum-lib (~> 4.2)
   gollum-rugged_adapter (~> 0.4.4)
   gon (~> 6.1.0)
diff --git a/app/models/clusters/cluster.rb b/app/models/clusters/cluster.rb
index 77947d515c1..e4a06f3f976 100644
--- a/app/models/clusters/cluster.rb
+++ b/app/models/clusters/cluster.rb
@@ -15,7 +15,7 @@ module Clusters
     belongs_to :user
 
     has_many :cluster_projects, class_name: 'Clusters::Project'
-    has_many :projects, through: :cluster_projects, class_name: '::Project'
+    has_many :projects, -> { auto_include(false) }, through: :cluster_projects, class_name: '::Project'
 
     # we force autosave to happen when we save `Cluster` model
     has_one :provider_gcp, class_name: 'Clusters::Providers::Gcp', autosave: true
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index b45395343cc..d9416352f9c 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -48,7 +48,7 @@ module Issuable
     end
 
     has_many :label_links, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
-    has_many :labels, through: :label_links
+    has_many :labels, -> { auto_include(false) }, through: :label_links
     has_many :todos, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
 
     has_one :metrics
diff --git a/app/models/concerns/resolvable_discussion.rb b/app/models/concerns/resolvable_discussion.rb
index 7c236369793..399abb67c9d 100644
--- a/app/models/concerns/resolvable_discussion.rb
+++ b/app/models/concerns/resolvable_discussion.rb
@@ -102,7 +102,7 @@ module ResolvableDiscussion
     yield(notes_relation)
 
     # Set the notes array to the updated notes
-    @notes = notes_relation.fresh.to_a # rubocop:disable Gitlab/ModuleWithInstanceVariables
+    @notes = notes_relation.fresh.auto_include(false).to_a # rubocop:disable Gitlab/ModuleWithInstanceVariables
 
     self.class.memoized_values.each do |name|
       clear_memoization(name)
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 13d9e42bcc8..c1ffe6512ea 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -34,7 +34,7 @@ class Issue < ActiveRecord::Base
     dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
 
   has_many :issue_assignees
-  has_many :assignees, class_name: "User", through: :issue_assignees
+  has_many :assignees, -> { auto_include(false) }, class_name: "User", through: :issue_assignees
 
   validates :project, presence: true
 
diff --git a/app/models/label.rb b/app/models/label.rb
index de7f1d56c64..f3496884cff 100644
--- a/app/models/label.rb
+++ b/app/models/label.rb
@@ -18,8 +18,8 @@ class Label < ActiveRecord::Base
   has_many :lists, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
   has_many :priorities, class_name: 'LabelPriority'
   has_many :label_links, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
-  has_many :issues, through: :label_links, source: :target, source_type: 'Issue'
-  has_many :merge_requests, through: :label_links, source: :target, source_type: 'MergeRequest'
+  has_many :issues, -> { auto_include(false) }, through: :label_links, source: :target, source_type: 'Issue'
+  has_many :merge_requests, -> { auto_include(false) }, through: :label_links, source: :target, source_type: 'MergeRequest'
 
   before_validation :strip_whitespace_from_title_and_color
 
diff --git a/app/models/project.rb b/app/models/project.rb
index 3f805dd1fc9..3f94b89c36b 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -179,7 +179,7 @@ class Project < ActiveRecord::Base
   has_many :members_and_requesters, as: :source, class_name: 'ProjectMember'
 
   has_many :deploy_keys_projects
-  has_many :deploy_keys, through: :deploy_keys_projects
+  has_many :deploy_keys, -> { auto_include(false) }, through: :deploy_keys_projects
   has_many :users_star_projects
   has_many :starrers, through: :users_star_projects, source: :user
   has_many :releases
@@ -199,7 +199,7 @@ class Project < ActiveRecord::Base
   has_one :statistics, class_name: 'ProjectStatistics'
 
   has_one :cluster_project, class_name: 'Clusters::Project'
-  has_many :clusters, through: :cluster_project, class_name: 'Clusters::Cluster'
+  has_many :clusters, -> { auto_include(false) }, through: :cluster_project, class_name: 'Clusters::Cluster'
 
   # Container repositories need to remove data from the container registry,
   # which is not managed by the DB. Hence we're still using dependent: :destroy
@@ -225,7 +225,7 @@ class Project < ActiveRecord::Base
   has_many :project_deploy_tokens
   has_many :deploy_tokens, through: :project_deploy_tokens
 
-  has_many :active_runners, -> { active }, through: :runner_projects, source: :runner, class_name: 'Ci::Runner'
+  has_many :active_runners, -> { active.auto_include(false) }, through: :runner_projects, source: :runner, class_name: 'Ci::Runner'
 
   has_one :auto_devops, class_name: 'ProjectAutoDevops'
   has_many :custom_attributes, class_name: 'ProjectCustomAttribute'
diff --git a/app/models/todo.rb b/app/models/todo.rb
index a2ab405fdbe..aad2c1dac4e 100644
--- a/app/models/todo.rb
+++ b/app/models/todo.rb
@@ -22,7 +22,7 @@ class Todo < ActiveRecord::Base
   belongs_to :author, class_name: "User"
   belongs_to :note
   belongs_to :project
-  belongs_to :target, polymorphic: true, touch: true # rubocop:disable Cop/PolymorphicAssociations
+  belongs_to :target, -> { auto_include(false) }, polymorphic: true, touch: true # rubocop:disable Cop/PolymorphicAssociations
   belongs_to :user
 
   delegate :name, :email, to: :author, prefix: true, allow_nil: true
diff --git a/app/models/user.rb b/app/models/user.rb
index 2b95be3f888..c7e1dfaf595 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -96,9 +96,9 @@ class User < ActiveRecord::Base
   # Groups
   has_many :members
   has_many :group_members, -> { where(requested_at: nil) }, source: 'GroupMember'
-  has_many :groups, through: :group_members
-  has_many :owned_groups, -> { where members: { access_level: Gitlab::Access::OWNER } }, through: :group_members, source: :group
-  has_many :masters_groups, -> { where members: { access_level: Gitlab::Access::MASTER } }, through: :group_members, source: :group
+  has_many :groups, -> { auto_include(false) }, through: :group_members
+  has_many :owned_groups, -> { where(members: { access_level: Gitlab::Access::OWNER }).auto_include(false) }, through: :group_members, source: :group
+  has_many :masters_groups, -> { where(members: { access_level: Gitlab::Access::MASTER }).auto_include(false) }, through: :group_members, source: :group
 
   # Projects
   has_many :groups_projects,          through: :groups, source: :projects
diff --git a/changelogs/unreleased/fix-n-plus-one-when-getting-notification-settings-for-recipients.yml b/changelogs/unreleased/fix-n-plus-one-when-getting-notification-settings-for-recipients.yml
new file mode 100644
index 00000000000..837bfed19b7
--- /dev/null
+++ b/changelogs/unreleased/fix-n-plus-one-when-getting-notification-settings-for-recipients.yml
@@ -0,0 +1,5 @@
+---
+title: Add Goldiloader to fix N+1 issues when calculating email recipients
+merge_request:
+author:
+type: performance
diff --git a/spec/requests/api/pipeline_schedules_spec.rb b/spec/requests/api/pipeline_schedules_spec.rb
index 7ea25059756..91d4d5d3de9 100644
--- a/spec/requests/api/pipeline_schedules_spec.rb
+++ b/spec/requests/api/pipeline_schedules_spec.rb
@@ -17,6 +17,17 @@ describe API::PipelineSchedules do
         pipeline_schedule.pipelines << build(:ci_pipeline, project: project)
       end
 
+      def create_pipeline_schedules(count)
+        create_list(:ci_pipeline_schedule, count, project: project)
+          .each do |pipeline_schedule|
+          create(:user).tap do |user|
+            project.add_developer(user)
+            pipeline_schedule.update_attributes(owner: user)
+          end
+          pipeline_schedule.pipelines << build(:ci_pipeline, project: project)
+        end
+      end
+
       it 'returns list of pipeline_schedules' do
         get api("/projects/#{project.id}/pipeline_schedules", developer)
 
@@ -26,18 +37,14 @@ describe API::PipelineSchedules do
       end
 
       it 'avoids N + 1 queries' do
+        # We need at least two users to trigger a preload for that relation.
+        create_pipeline_schedules(1)
+
         control_count = ActiveRecord::QueryRecorder.new do
           get api("/projects/#{project.id}/pipeline_schedules", developer)
         end.count
 
-        create_list(:ci_pipeline_schedule, 10, project: project)
-          .each do |pipeline_schedule|
-          create(:user).tap do |user|
-            project.add_developer(user)
-            pipeline_schedule.update_attributes(owner: user)
-          end
-          pipeline_schedule.pipelines << build(:ci_pipeline, project: project)
-        end
+        create_pipeline_schedules(10)
 
         expect do
           get api("/projects/#{project.id}/pipeline_schedules", developer)