Add cop for has_many :through without disabled autoloadingfix-n-plus-one-when-getting-notification-settings-for-recipients

Goldiloader is great, but has several issues with has_many :through relations:

* https://github.com/salsify/goldiloader/issues/12
* https://github.com/salsify/goldiloader/issues/14
* https://github.com/salsify/goldiloader/issues/18

Rather than try to figure out which applies in each case, we should just do the
drudge work of manually disabling autoloading for all relations of this type. We
can always use regular preloading for specific cases, but this way we avoid
generating invalid queries through Goldiloader's magic.

12 files changed, 149 insertions, 29 deletions

diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index 5a4c56ec0dc..ee0d8df8eb7 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -13,7 +13,7 @@ module Ci
 
     has_many :builds
     has_many :runner_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
-    has_many :projects, through: :runner_projects
+    has_many :projects, -> { auto_include(false) }, through: :runner_projects
 
     has_one :last_build, ->() { order('id DESC') }, class_name: 'Ci::Build'
 
diff --git a/app/models/deploy_key.rb b/app/models/deploy_key.rb
index 89a74b7dcb1..858b7ef533e 100644
--- a/app/models/deploy_key.rb
+++ b/app/models/deploy_key.rb
@@ -2,7 +2,7 @@ class DeployKey < Key
   include IgnorableColumn
 
   has_many :deploy_keys_projects, inverse_of: :deploy_key, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
-  has_many :projects, through: :deploy_keys_projects
+  has_many :projects, -> { auto_include(false) }, through: :deploy_keys_projects
 
   scope :in_projects, ->(projects) { joins(:deploy_keys_projects).where('deploy_keys_projects.project_id in (?)', projects) }
   scope :are_public,  -> { where(public: true) }
diff --git a/app/models/deploy_token.rb b/app/models/deploy_token.rb
index fe726b156d4..b47b2ff4c3f 100644
--- a/app/models/deploy_token.rb
+++ b/app/models/deploy_token.rb
@@ -8,7 +8,7 @@ class DeployToken < ActiveRecord::Base
   default_value_for(:expires_at) { Forever.date }
 
   has_many :project_deploy_tokens, inverse_of: :deploy_token
-  has_many :projects, through: :project_deploy_tokens
+  has_many :projects, -> { auto_include(false) }, through: :project_deploy_tokens
 
   validate :ensure_at_least_one_scope
   before_save :ensure_token
diff --git a/app/models/fork_network.rb b/app/models/fork_network.rb
index 7f1728e8c77..aad3509b895 100644
--- a/app/models/fork_network.rb
+++ b/app/models/fork_network.rb
@@ -1,7 +1,7 @@
 class ForkNetwork < ActiveRecord::Base
   belongs_to :root_project, class_name: 'Project'
   has_many :fork_network_members
-  has_many :projects, through: :fork_network_members
+  has_many :projects, -> { auto_include(false) }, through: :fork_network_members
 
   after_create :add_root_as_member, if: :root_project
 
diff --git a/app/models/group.rb b/app/models/group.rb
index 8ff781059cc..202988d743d 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -12,9 +12,9 @@ class Group < Namespace
 
   has_many :group_members, -> { where(requested_at: nil) }, dependent: :destroy, as: :source # rubocop:disable Cop/ActiveRecordDependent
   alias_method :members, :group_members
-  has_many :users, through: :group_members
+  has_many :users, -> { auto_include(false) }, through: :group_members
   has_many :owners,
-    -> { where(members: { access_level: Gitlab::Access::OWNER }) },
+    -> { where(members: { access_level: Gitlab::Access::OWNER }).auto_include(false) },
     through: :group_members,
     source: :user
 
@@ -23,7 +23,7 @@ class Group < Namespace
 
   has_many :milestones
   has_many :project_group_links, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
-  has_many :shared_projects, through: :project_group_links, source: :project
+  has_many :shared_projects, -> { auto_include(false) }, through: :project_group_links, source: :project
   has_many :notification_settings, dependent: :destroy, as: :source # rubocop:disable Cop/ActiveRecordDependent
   has_many :labels, class_name: 'GroupLabel'
   has_many :variables, class_name: 'Ci::GroupVariable'
diff --git a/app/models/lfs_object.rb b/app/models/lfs_object.rb
index b7de46fa202..ed95613ee59 100644
--- a/app/models/lfs_object.rb
+++ b/app/models/lfs_object.rb
@@ -3,7 +3,7 @@ class LfsObject < ActiveRecord::Base
   include ObjectStorage::BackgroundMove
 
   has_many :lfs_objects_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
-  has_many :projects, through: :lfs_objects_projects
+  has_many :projects, -> { auto_include(false) }, through: :lfs_objects_projects
 
   scope :with_files_stored_locally, -> { where(file_store: [nil, LfsObjectUploader::Store::LOCAL]) }
 
diff --git a/app/models/milestone.rb b/app/models/milestone.rb
index a66a0015827..8e33bab81c1 100644
--- a/app/models/milestone.rb
+++ b/app/models/milestone.rb
@@ -22,7 +22,7 @@ class Milestone < ActiveRecord::Base
   belongs_to :group
 
   has_many :issues
-  has_many :labels, -> { distinct.reorder('labels.title') },  through: :issues
+  has_many :labels, -> { distinct.reorder('labels.title').auto_include(false) }, through: :issues
   has_many :merge_requests
   has_many :events, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
 
diff --git a/app/models/project.rb b/app/models/project.rb
index 3f94b89c36b..ffd78d3ab70 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -138,11 +138,11 @@ class Project < ActiveRecord::Base
   has_one :packagist_service
 
   # TODO: replace these relations with the fork network versions
-  has_one  :forked_project_link,  foreign_key: "forked_to_project_id"
-  has_one  :forked_from_project,  through:   :forked_project_link
+  has_one  :forked_project_link, foreign_key: "forked_to_project_id"
+  has_one  :forked_from_project, -> { auto_include(false) }, through:   :forked_project_link
 
   has_many :forked_project_links, foreign_key: "forked_from_project_id"
-  has_many :forks,                through:     :forked_project_links, source: :forked_to_project
+  has_many :forks, -> { auto_include(false) }, through: :forked_project_links, source: :forked_to_project
   # TODO: replace these relations with the fork network versions
 
   has_one :root_of_fork_network,
@@ -150,7 +150,7 @@ class Project < ActiveRecord::Base
           inverse_of: :root_project,
           class_name: 'ForkNetwork'
   has_one :fork_network_member
-  has_one :fork_network, through: :fork_network_member
+  has_one :fork_network, -> { auto_include(false) }, through: :fork_network_member
 
   # Merge Requests for target project should be removed with it
   has_many :merge_requests, foreign_key: 'target_project_id'
@@ -167,12 +167,12 @@ class Project < ActiveRecord::Base
   has_many :protected_tags
 
   has_many :project_authorizations
-  has_many :authorized_users, through: :project_authorizations, source: :user, class_name: 'User'
+  has_many :authorized_users, -> { auto_include(false) }, through: :project_authorizations, source: :user, class_name: 'User'
   has_many :project_members, -> { where(requested_at: nil) },
     as: :source, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
 
   alias_method :members, :project_members
-  has_many :users, through: :project_members
+  has_many :users, -> { auto_include(false) }, through: :project_members
 
   has_many :requesters, -> { where.not(requested_at: nil) },
     as: :source, class_name: 'ProjectMember', dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
@@ -181,13 +181,13 @@ class Project < ActiveRecord::Base
   has_many :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 :starrers, -> { auto_include(false) }, through: :users_star_projects, source: :user
   has_many :releases
   has_many :lfs_objects_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
-  has_many :lfs_objects, through: :lfs_objects_projects
+  has_many :lfs_objects, -> { auto_include(false) }, through: :lfs_objects_projects
   has_many :lfs_file_locks
   has_many :project_group_links
-  has_many :invited_groups, through: :project_group_links, source: :group
+  has_many :invited_groups, -> { auto_include(false) }, through: :project_group_links, source: :group
   has_many :pages_domains
   has_many :todos
   has_many :notification_settings, as: :source, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
@@ -216,14 +216,14 @@ class Project < ActiveRecord::Base
   has_many :builds, class_name: 'Ci::Build', inverse_of: :project, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
   has_many :build_trace_section_names, class_name: 'Ci::BuildTraceSectionName'
   has_many :runner_projects, class_name: 'Ci::RunnerProject'
-  has_many :runners, through: :runner_projects, source: :runner, class_name: 'Ci::Runner'
+  has_many :runners, -> { auto_include(false) }, through: :runner_projects, source: :runner, class_name: 'Ci::Runner'
   has_many :variables, class_name: 'Ci::Variable'
   has_many :triggers, class_name: 'Ci::Trigger'
   has_many :environments
   has_many :deployments
   has_many :pipeline_schedules, class_name: 'Ci::PipelineSchedule'
   has_many :project_deploy_tokens
-  has_many :deploy_tokens, through: :project_deploy_tokens
+  has_many :deploy_tokens, -> { auto_include(false) }, through: :project_deploy_tokens
 
   has_many :active_runners, -> { active.auto_include(false) }, through: :runner_projects, source: :runner, class_name: 'Ci::Runner'
 
diff --git a/app/models/user.rb b/app/models/user.rb
index c7e1dfaf595..fc87ada7447 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -101,18 +101,18 @@ class User < ActiveRecord::Base
   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
-  has_many :personal_projects,        through: :namespace, source: :projects
+  has_many :groups_projects, -> { auto_include(false) }, through: :groups, source: :projects
+  has_many :personal_projects, -> { auto_include(false) }, through: :namespace, source: :projects
   has_many :project_members, -> { where(requested_at: nil) }
-  has_many :projects,                 through: :project_members
-  has_many :created_projects,         foreign_key: :creator_id, class_name: 'Project'
+  has_many :projects, -> { auto_include(false) }, through: :project_members
+  has_many :created_projects, foreign_key: :creator_id, class_name: 'Project'
   has_many :users_star_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
-  has_many :starred_projects, through: :users_star_projects, source: :project
+  has_many :starred_projects, -> { auto_include(false) }, through: :users_star_projects, source: :project
   has_many :project_authorizations
-  has_many :authorized_projects, through: :project_authorizations, source: :project
+  has_many :authorized_projects, -> { auto_include(false) }, through: :project_authorizations, source: :project
 
   has_many :user_interacted_projects
-  has_many :project_interactions, through: :user_interacted_projects, source: :project, class_name: 'Project'
+  has_many :project_interactions, -> { auto_include(false) }, through: :user_interacted_projects, source: :project, class_name: 'Project'
 
   has_many :snippets,                 dependent: :destroy, foreign_key: :author_id # rubocop:disable Cop/ActiveRecordDependent
   has_many :notes,                    dependent: :destroy, foreign_key: :author_id # rubocop:disable Cop/ActiveRecordDependent
@@ -132,7 +132,7 @@ class User < ActiveRecord::Base
   has_many :triggers,                 dependent: :destroy, class_name: 'Ci::Trigger', foreign_key: :owner_id # rubocop:disable Cop/ActiveRecordDependent
 
   has_many :issue_assignees
-  has_many :assigned_issues, class_name: "Issue", through: :issue_assignees, source: :issue
+  has_many :assigned_issues, -> { auto_include(false) }, class_name: "Issue", through: :issue_assignees, source: :issue
   has_many :assigned_merge_requests,  dependent: :nullify, foreign_key: :assignee_id, class_name: "MergeRequest" # rubocop:disable Cop/ActiveRecordDependent
 
   has_many :custom_attributes, class_name: 'UserCustomAttribute'
diff --git a/rubocop/cop/gitlab/has_many_through_scope.rb b/rubocop/cop/gitlab/has_many_through_scope.rb
new file mode 100644
index 00000000000..770a2a0529f
--- /dev/null
+++ b/rubocop/cop/gitlab/has_many_through_scope.rb
@@ -0,0 +1,45 @@
+require 'gitlab/styles/rubocop/model_helpers'
+
+module RuboCop
+  module Cop
+    module Gitlab
+      class HasManyThroughScope < RuboCop::Cop::Cop
+        include ::Gitlab::Styles::Rubocop::ModelHelpers
+
+        MSG = 'Always provide an explicit scope calling auto_include(false) when using has_many :through'.freeze
+
+        def_node_search :through?, <<~PATTERN
+          (pair (sym :through) _)
+        PATTERN
+
+        def_node_matcher :has_many_through?, <<~PATTERN
+          (send nil? :has_many ... #through?)
+        PATTERN
+
+        def_node_search :disables_auto_include?, <<~PATTERN
+          (send _ :auto_include false)
+        PATTERN
+
+        def_node_matcher :scope_disables_auto_include?, <<~PATTERN
+          (block (send nil? :lambda) _ #disables_auto_include?)
+        PATTERN
+
+        def on_send(node)
+          return unless in_model?(node)
+          return unless has_many_through?(node)
+
+          target = node
+          scope_argument = node.children[3]
+
+          if scope_argument.children[0].children.last == :lambda
+            return if scope_disables_auto_include?(scope_argument)
+
+            target = scope_argument
+          end
+
+          add_offense(target, location: :expression)
+        end
+      end
+    end
+  end
+end
diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb
index 406ec95ffc9..c2254332e7d 100644
--- a/rubocop/rubocop.rb
+++ b/rubocop/rubocop.rb
@@ -1,7 +1,8 @@
 # rubocop:disable Naming/FileName
+require_relative 'cop/gitlab/has_many_through_scope'
+require_relative 'cop/gitlab/httparty'
 require_relative 'cop/gitlab/module_with_instance_variables'
 require_relative 'cop/gitlab/predicate_memoization'
-require_relative 'cop/gitlab/httparty'
 require_relative 'cop/include_sidekiq_worker'
 require_relative 'cop/line_break_around_conditional_block'
 require_relative 'cop/migration/add_column'
diff --git a/spec/rubocop/cop/gitlab/has_many_through_scope_spec.rb b/spec/rubocop/cop/gitlab/has_many_through_scope_spec.rb
new file mode 100644
index 00000000000..6d769c8e6fd
--- /dev/null
+++ b/spec/rubocop/cop/gitlab/has_many_through_scope_spec.rb
@@ -0,0 +1,74 @@
+require 'spec_helper'
+
+require 'rubocop'
+require 'rubocop/rspec/support'
+
+require_relative '../../../../rubocop/cop/gitlab/has_many_through_scope'
+
+describe RuboCop::Cop::Gitlab::HasManyThroughScope do # rubocop:disable RSpec/FilePath
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+
+  context 'in a model file' do
+    before do
+      allow(cop).to receive(:in_model?).and_return(true)
+    end
+
+    context 'when the model does not use has_many :through' do
+      it 'does not register an offense' do
+        expect_no_offenses(<<-RUBY)
+          class User < ActiveRecord::Base
+            has_many :tags, source: 'UserTag'
+          end
+        RUBY
+      end
+    end
+
+    context 'when the model uses has_many :through' do
+      context 'when the association has no scope defined' do
+        it 'registers an offense on the association' do
+          expect_offense(<<-RUBY)
+            class User < ActiveRecord::Base
+              has_many :tags, through: :user_tags
+              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{described_class::MSG}
+            end
+           RUBY
+        end
+      end
+
+      context 'when the association has a scope defined' do
+        context 'when the scope does not disable auto-loading' do
+          it 'registers an offense on the scope' do
+            expect_offense(<<-RUBY)
+              class User < ActiveRecord::Base
+                has_many :tags, -> { where(active: true) }, through: :user_tags
+                                ^^^^^^^^^^^^^^^^^^^^^^^^^^ #{described_class::MSG}
+              end
+             RUBY
+          end
+        end
+
+        context 'when the scope has auto_include(false)' do
+          it 'does not register an offense' do
+            expect_no_offenses(<<-RUBY)
+              class User < ActiveRecord::Base
+                has_many :tags, -> { where(active: true).auto_include(false).reorder(nil) }, through: :user_tags
+              end
+            RUBY
+          end
+        end
+      end
+    end
+  end
+
+  context 'outside of a migration spec file' do
+    it 'does not register an offense' do
+      expect_no_offenses(<<-RUBY)
+        class User < ActiveRecord::Base
+          has_many :tags, through: :user_tags
+        end
+      RUBY
+    end
+  end
+end