Revert the addition of goldiloader
This reverts the addition of the "goldiloader" Gem and all use of it. While this Gem is very promising it's causing a variety of problems on GitLab.com due to it eager-loading too much data in places where we don't expect/can handle this. At least for the time being this means we have to go back to manually fixing N+1 query problems, but at least those should not cause a negative impact on availability.
This commit is contained in:
parent
019c0d5761
commit
6f292eaa69
21 changed files with 43 additions and 169 deletions
2
Gemfile
2
Gemfile
|
@ -434,5 +434,3 @@ gem 'grape_logging', '~> 1.7'
|
|||
|
||||
# Asset synchronization
|
||||
gem 'asset_sync', '~> 2.2.0'
|
||||
|
||||
gem 'goldiloader', '~> 2.0'
|
||||
|
|
|
@ -331,9 +331,6 @@ 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)
|
||||
gon (6.1.0)
|
||||
|
@ -1072,7 +1069,6 @@ DEPENDENCIES
|
|||
gitlab-markup (~> 1.6.2)
|
||||
gitlab-styles (~> 2.3)
|
||||
gitlab_omniauth-ldap (~> 2.0.4)
|
||||
goldiloader (~> 2.0)
|
||||
gon (~> 6.1.0)
|
||||
google-api-client (~> 0.19.8)
|
||||
google-protobuf (= 3.5.1)
|
||||
|
|
|
@ -13,7 +13,7 @@ module Ci
|
|||
|
||||
has_many :builds
|
||||
has_many :runner_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
|
||||
has_many :projects, -> { auto_include(false) }, through: :runner_projects
|
||||
has_many :projects, through: :runner_projects
|
||||
|
||||
has_one :last_build, ->() { order('id DESC') }, class_name: 'Ci::Build'
|
||||
|
||||
|
|
|
@ -15,7 +15,7 @@ module Clusters
|
|||
belongs_to :user
|
||||
|
||||
has_many :cluster_projects, class_name: 'Clusters::Project'
|
||||
has_many :projects, -> { auto_include(false) }, through: :cluster_projects, class_name: '::Project'
|
||||
has_many :projects, 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
|
||||
|
|
|
@ -48,7 +48,7 @@ module Issuable
|
|||
end
|
||||
|
||||
has_many :label_links, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
|
||||
has_many :labels, -> { auto_include(false) }, through: :label_links
|
||||
has_many :labels, through: :label_links
|
||||
has_many :todos, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
|
||||
|
||||
has_one :metrics
|
||||
|
|
|
@ -102,7 +102,7 @@ module ResolvableDiscussion
|
|||
yield(notes_relation)
|
||||
|
||||
# Set the notes array to the updated notes
|
||||
@notes = notes_relation.fresh.auto_include(false).to_a # rubocop:disable Gitlab/ModuleWithInstanceVariables
|
||||
@notes = notes_relation.fresh.to_a # rubocop:disable Gitlab/ModuleWithInstanceVariables
|
||||
|
||||
self.class.memoized_values.each do |name|
|
||||
clear_memoization(name)
|
||||
|
|
|
@ -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, -> { auto_include(false) }, through: :deploy_keys_projects
|
||||
has_many :projects, 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) }
|
||||
|
|
|
@ -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, -> { auto_include(false) }, through: :project_deploy_tokens
|
||||
has_many :projects, through: :project_deploy_tokens
|
||||
|
||||
validate :ensure_at_least_one_scope
|
||||
before_save :ensure_token
|
||||
|
|
|
@ -1,7 +1,7 @@
|
|||
class ForkNetwork < ActiveRecord::Base
|
||||
belongs_to :root_project, class_name: 'Project'
|
||||
has_many :fork_network_members
|
||||
has_many :projects, -> { auto_include(false) }, through: :fork_network_members
|
||||
has_many :projects, through: :fork_network_members
|
||||
|
||||
after_create :add_root_as_member, if: :root_project
|
||||
|
||||
|
|
|
@ -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, -> { auto_include(false) }, through: :group_members
|
||||
has_many :users, through: :group_members
|
||||
has_many :owners,
|
||||
-> { where(members: { access_level: Gitlab::Access::OWNER }).auto_include(false) },
|
||||
-> { where(members: { access_level: Gitlab::Access::OWNER }) },
|
||||
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, -> { auto_include(false) }, through: :project_group_links, source: :project
|
||||
has_many :shared_projects, 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'
|
||||
|
|
|
@ -34,7 +34,7 @@ class Issue < ActiveRecord::Base
|
|||
dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
|
||||
|
||||
has_many :issue_assignees
|
||||
has_many :assignees, -> { auto_include(false) }, class_name: "User", through: :issue_assignees
|
||||
has_many :assignees, class_name: "User", through: :issue_assignees
|
||||
|
||||
validates :project, presence: true
|
||||
|
||||
|
|
|
@ -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, -> { 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'
|
||||
has_many :issues, through: :label_links, source: :target, source_type: 'Issue'
|
||||
has_many :merge_requests, through: :label_links, source: :target, source_type: 'MergeRequest'
|
||||
|
||||
before_validation :strip_whitespace_from_title_and_color
|
||||
|
||||
|
|
|
@ -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, -> { auto_include(false) }, through: :lfs_objects_projects
|
||||
has_many :projects, through: :lfs_objects_projects
|
||||
|
||||
scope :with_files_stored_locally, -> { where(file_store: [nil, LfsObjectUploader::Store::LOCAL]) }
|
||||
|
||||
|
|
|
@ -22,7 +22,7 @@ class Milestone < ActiveRecord::Base
|
|||
belongs_to :group
|
||||
|
||||
has_many :issues
|
||||
has_many :labels, -> { distinct.reorder('labels.title').auto_include(false) }, through: :issues
|
||||
has_many :labels, -> { distinct.reorder('labels.title') }, through: :issues
|
||||
has_many :merge_requests
|
||||
has_many :events, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
|
||||
|
||||
|
|
|
@ -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, -> { auto_include(false) }, through: :forked_project_link
|
||||
has_one :forked_project_link, foreign_key: "forked_to_project_id"
|
||||
has_one :forked_from_project, through: :forked_project_link
|
||||
|
||||
has_many :forked_project_links, foreign_key: "forked_from_project_id"
|
||||
has_many :forks, -> { auto_include(false) }, through: :forked_project_links, source: :forked_to_project
|
||||
has_many :forks, 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, -> { auto_include(false) }, through: :fork_network_member
|
||||
has_one :fork_network, through: :fork_network_member
|
||||
|
||||
# Merge Requests for target project should be removed with it
|
||||
has_many :merge_requests, foreign_key: 'target_project_id'
|
||||
|
@ -167,27 +167,27 @@ class Project < ActiveRecord::Base
|
|||
has_many :protected_tags
|
||||
|
||||
has_many :project_authorizations
|
||||
has_many :authorized_users, -> { auto_include(false) }, through: :project_authorizations, source: :user, class_name: 'User'
|
||||
has_many :authorized_users, 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, -> { auto_include(false) }, through: :project_members
|
||||
has_many :users, through: :project_members
|
||||
|
||||
has_many :requesters, -> { where.not(requested_at: nil) },
|
||||
as: :source, class_name: 'ProjectMember', dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
|
||||
has_many :members_and_requesters, as: :source, class_name: 'ProjectMember'
|
||||
|
||||
has_many :deploy_keys_projects
|
||||
has_many :deploy_keys, -> { auto_include(false) }, through: :deploy_keys_projects
|
||||
has_many :deploy_keys, through: :deploy_keys_projects
|
||||
has_many :users_star_projects
|
||||
has_many :starrers, -> { auto_include(false) }, through: :users_star_projects, source: :user
|
||||
has_many :starrers, through: :users_star_projects, source: :user
|
||||
has_many :releases
|
||||
has_many :lfs_objects_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
|
||||
has_many :lfs_objects, -> { auto_include(false) }, through: :lfs_objects_projects
|
||||
has_many :lfs_objects, through: :lfs_objects_projects
|
||||
has_many :lfs_file_locks
|
||||
has_many :project_group_links
|
||||
has_many :invited_groups, -> { auto_include(false) }, through: :project_group_links, source: :group
|
||||
has_many :invited_groups, 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
|
||||
|
@ -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, -> { auto_include(false) }, through: :cluster_project, class_name: 'Clusters::Cluster'
|
||||
has_many :clusters, 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
|
||||
|
@ -216,16 +216,16 @@ 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, -> { auto_include(false) }, through: :runner_projects, source: :runner, class_name: 'Ci::Runner'
|
||||
has_many :runners, 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, -> { auto_include(false) }, through: :project_deploy_tokens
|
||||
has_many :deploy_tokens, through: :project_deploy_tokens
|
||||
|
||||
has_many :active_runners, -> { active.auto_include(false) }, through: :runner_projects, source: :runner, class_name: 'Ci::Runner'
|
||||
has_many :active_runners, -> { active }, through: :runner_projects, source: :runner, class_name: 'Ci::Runner'
|
||||
|
||||
has_one :auto_devops, class_name: 'ProjectAutoDevops'
|
||||
has_many :custom_attributes, class_name: 'ProjectCustomAttribute'
|
||||
|
|
|
@ -22,7 +22,7 @@ class Todo < ActiveRecord::Base
|
|||
belongs_to :author, class_name: "User"
|
||||
belongs_to :note
|
||||
belongs_to :project
|
||||
belongs_to :target, -> { auto_include(false) }, polymorphic: true, touch: true # rubocop:disable Cop/PolymorphicAssociations
|
||||
belongs_to :target, polymorphic: true, touch: true # rubocop:disable Cop/PolymorphicAssociations
|
||||
belongs_to :user
|
||||
|
||||
delegate :name, :email, to: :author, prefix: true, allow_nil: true
|
||||
|
|
|
@ -96,23 +96,23 @@ class User < ActiveRecord::Base
|
|||
# Groups
|
||||
has_many :members
|
||||
has_many :group_members, -> { where(requested_at: nil) }, source: 'GroupMember'
|
||||
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
|
||||
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
|
||||
|
||||
# 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 :groups_projects, through: :groups, source: :projects
|
||||
has_many :personal_projects, through: :namespace, source: :projects
|
||||
has_many :project_members, -> { where(requested_at: nil) }
|
||||
has_many :projects, -> { auto_include(false) }, through: :project_members
|
||||
has_many :created_projects, foreign_key: :creator_id, class_name: 'Project'
|
||||
has_many :projects, 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, -> { auto_include(false) }, through: :users_star_projects, source: :project
|
||||
has_many :starred_projects, through: :users_star_projects, source: :project
|
||||
has_many :project_authorizations
|
||||
has_many :authorized_projects, -> { auto_include(false) }, through: :project_authorizations, source: :project
|
||||
has_many :authorized_projects, through: :project_authorizations, source: :project
|
||||
|
||||
has_many :user_interacted_projects
|
||||
has_many :project_interactions, -> { auto_include(false) }, through: :user_interacted_projects, source: :project, class_name: 'Project'
|
||||
has_many :project_interactions, 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, -> { auto_include(false) }, class_name: "Issue", through: :issue_assignees, source: :issue
|
||||
has_many :assigned_issues, 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'
|
||||
|
|
|
@ -33,7 +33,7 @@ module Ci
|
|||
end
|
||||
end
|
||||
|
||||
builds.auto_include(false).find do |build|
|
||||
builds.find do |build|
|
||||
next unless runner.can_pick?(build)
|
||||
|
||||
begin
|
||||
|
|
|
@ -1,45 +0,0 @@
|
|||
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
|
|
@ -1,8 +1,7 @@
|
|||
# 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/avoid_return_from_blocks'
|
||||
require_relative 'cop/avoid_break_from_strong_memoize'
|
||||
|
|
|
@ -1,74 +0,0 @@
|
|||
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
|
Loading…
Reference in a new issue