diff options
| -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 |