diff options
author | Sean McGivern <sean@gitlab.com> | 2018-04-09 14:19:18 +0300 |
---|---|---|
committer | Sean McGivern <sean@gitlab.com> | 2018-04-09 14:47:04 +0300 |
commit | 4ef3e3491e2ecc34e7f4de1221d5ad7b8b4a1e24 (patch) | |
tree | 6c151944cf2791fe4526633603f3e888268ae431 | |
parent | 20fdbbe86a6cffbf467f08d50a0d8ef0f5c87f50 (diff) |
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.
-rw-r--r-- | app/models/ci/runner.rb | 2 | ||||
-rw-r--r-- | app/models/deploy_key.rb | 2 | ||||
-rw-r--r-- | app/models/deploy_token.rb | 2 | ||||
-rw-r--r-- | app/models/fork_network.rb | 2 | ||||
-rw-r--r-- | app/models/group.rb | 6 | ||||
-rw-r--r-- | app/models/lfs_object.rb | 2 | ||||
-rw-r--r-- | app/models/milestone.rb | 2 | ||||
-rw-r--r-- | app/models/project.rb | 22 | ||||
-rw-r--r-- | app/models/user.rb | 16 | ||||
-rw-r--r-- | rubocop/cop/gitlab/has_many_through_scope.rb | 45 | ||||
-rw-r--r-- | rubocop/rubocop.rb | 3 | ||||
-rw-r--r-- | spec/rubocop/cop/gitlab/has_many_through_scope_spec.rb | 74 |
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 |