From 943e420e5a8c521c999366de4ff2eb2111658d6c Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Mon, 9 Apr 2018 12:19:18 +0100
Subject: [PATCH] Add cop for has_many :through without disabled autoloading
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.
---
app/models/ci/pipeline.rb | 2 +-
app/models/ci/runner.rb | 2 +-
app/models/deploy_key.rb | 2 +-
app/models/deploy_token.rb | 2 +-
app/models/fork_network.rb | 2 +-
app/models/group.rb | 6 +-
app/models/lfs_object.rb | 2 +-
app/models/milestone.rb | 2 +-
app/models/project.rb | 22 +++---
app/models/user.rb | 16 ++--
rubocop/cop/gitlab/has_many_through_scope.rb | 45 +++++++++++
rubocop/rubocop.rb | 3 +-
.../cop/gitlab/has_many_through_scope_spec.rb | 74 +++++++++++++++++++
13 files changed, 150 insertions(+), 30 deletions(-)
create mode 100644 rubocop/cop/gitlab/has_many_through_scope.rb
create mode 100644 spec/rubocop/cop/gitlab/has_many_through_scope_spec.rb
diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index 6fc018bb04c56..2a7c3fb58fb90 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -20,7 +20,7 @@ class Pipeline < ActiveRecord::Base
has_many :sourced_pipelines, class_name: Ci::Sources::Pipeline, foreign_key: :source_pipeline_id
has_one :triggered_by_pipeline, through: :source_pipeline, source: :source_pipeline
- has_many :triggered_pipelines, through: :sourced_pipelines, source: :pipeline
+ has_many :triggered_pipelines, -> { auto_include(false) }, through: :sourced_pipelines, source: :pipeline
has_many :auto_canceled_pipelines, class_name: 'Ci::Pipeline', foreign_key: 'auto_canceled_by_id'
has_many :auto_canceled_jobs, class_name: 'CommitStatus', foreign_key: 'auto_canceled_by_id'
diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index 05aeb6b20e910..a1c04d7bdf86b 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -14,7 +14,7 @@ class Runner < ActiveRecord::Base
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 89a74b7dcb1d9..858b7ef533e7a 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 fe726b156d43a..b47b2ff4c3fb1 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 7f1728e8c772b..aad3509b89562 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 90d28b17dd547..b50a72c874551 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -15,9 +15,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
@@ -26,7 +26,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 6e7e200f0fc1a..7ab2be36b9ad9 100644
--- a/app/models/lfs_object.rb
+++ b/app/models/lfs_object.rb
@@ -4,7 +4,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]) }
scope :with_files_stored_remotely, -> { where(file_store: LfsObjectUploader::Store::REMOTE) }
diff --git a/app/models/milestone.rb b/app/models/milestone.rb
index 194e01815f573..b8544f06c5df2 100644
--- a/app/models/milestone.rb
+++ b/app/models/milestone.rb
@@ -26,7 +26,7 @@ class Milestone < ActiveRecord::Base
has_many :boards
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 9e369d94047b0..16bc8dc14bca5 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -143,11 +143,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,
@@ -155,7 +155,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'
@@ -172,12 +172,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
@@ -186,13 +186,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
@@ -223,14 +223,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 cde119c4feeb0..4241b0ed36bdf 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -103,18 +103,18 @@ def update_tracked_fields!(request)
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
@@ -134,7 +134,7 @@ def update_tracked_fields!(request)
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 0000000000000..770a2a0529f28
--- /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 406ec95ffc93f..c2254332e7dd5 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 0000000000000..6d769c8e6fd00
--- /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
--
GitLab