From 20fdbbe86a6cffbf467f08d50a0d8ef0f5c87f50 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Thu, 5 Apr 2018 13:19:24 +0200 Subject: Use Goldiloader for handling N+1 queries Goldiloader (https://github.com/salsify/goldiloader) can eager load associations automatically. This removes the need for adding "includes" calls in a variety of different places. This also comes with the added benefit of not having to eager load data if it's not used. --- Gemfile | 2 ++ Gemfile.lock | 4 ++++ app/models/clusters/cluster.rb | 2 +- app/models/concerns/issuable.rb | 2 +- app/models/concerns/resolvable_discussion.rb | 2 +- app/models/issue.rb | 2 +- app/models/label.rb | 4 ++-- app/models/project.rb | 6 +++--- app/models/todo.rb | 2 +- app/models/user.rb | 6 +++--- ...etting-notification-settings-for-recipients.yml | 5 +++++ spec/requests/api/pipeline_schedules_spec.rb | 23 ++++++++++++++-------- 12 files changed, 39 insertions(+), 21 deletions(-) create mode 100644 changelogs/unreleased/fix-n-plus-one-when-getting-notification-settings-for-recipients.yml diff --git a/Gemfile b/Gemfile index 7f5b1f1d3ef..647138cd90f 100644 --- a/Gemfile +++ b/Gemfile @@ -441,3 +441,5 @@ gem 'grape_logging', '~> 1.7' # Asset synchronization gem 'asset_sync', '~> 2.2.0' + +gem 'goldiloader', '~> 2.0' diff --git a/Gemfile.lock b/Gemfile.lock index a1150dfccdd..76e1a17155f 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -320,6 +320,9 @@ 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) gollum-lib (4.2.7) @@ -1067,6 +1070,7 @@ DEPENDENCIES gitlab-markup (~> 1.6.2) gitlab-styles (~> 2.3) gitlab_omniauth-ldap (~> 2.0.4) + goldiloader (~> 2.0) gollum-lib (~> 4.2) gollum-rugged_adapter (~> 0.4.4) gon (~> 6.1.0) diff --git a/app/models/clusters/cluster.rb b/app/models/clusters/cluster.rb index 77947d515c1..e4a06f3f976 100644 --- a/app/models/clusters/cluster.rb +++ b/app/models/clusters/cluster.rb @@ -15,7 +15,7 @@ module Clusters belongs_to :user has_many :cluster_projects, class_name: 'Clusters::Project' - has_many :projects, through: :cluster_projects, class_name: '::Project' + has_many :projects, -> { auto_include(false) }, 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 diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index b45395343cc..d9416352f9c 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -48,7 +48,7 @@ module Issuable end has_many :label_links, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent - has_many :labels, through: :label_links + has_many :labels, -> { auto_include(false) }, through: :label_links has_many :todos, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_one :metrics diff --git a/app/models/concerns/resolvable_discussion.rb b/app/models/concerns/resolvable_discussion.rb index 7c236369793..399abb67c9d 100644 --- a/app/models/concerns/resolvable_discussion.rb +++ b/app/models/concerns/resolvable_discussion.rb @@ -102,7 +102,7 @@ module ResolvableDiscussion yield(notes_relation) # Set the notes array to the updated notes - @notes = notes_relation.fresh.to_a # rubocop:disable Gitlab/ModuleWithInstanceVariables + @notes = notes_relation.fresh.auto_include(false).to_a # rubocop:disable Gitlab/ModuleWithInstanceVariables self.class.memoized_values.each do |name| clear_memoization(name) diff --git a/app/models/issue.rb b/app/models/issue.rb index 13d9e42bcc8..c1ffe6512ea 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -34,7 +34,7 @@ class Issue < ActiveRecord::Base dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent has_many :issue_assignees - has_many :assignees, class_name: "User", through: :issue_assignees + has_many :assignees, -> { auto_include(false) }, class_name: "User", through: :issue_assignees validates :project, presence: true diff --git a/app/models/label.rb b/app/models/label.rb index de7f1d56c64..f3496884cff 100644 --- a/app/models/label.rb +++ b/app/models/label.rb @@ -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, through: :label_links, source: :target, source_type: 'Issue' - has_many :merge_requests, through: :label_links, source: :target, source_type: 'MergeRequest' + 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' before_validation :strip_whitespace_from_title_and_color diff --git a/app/models/project.rb b/app/models/project.rb index 3f805dd1fc9..3f94b89c36b 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -179,7 +179,7 @@ class Project < ActiveRecord::Base has_many :members_and_requesters, as: :source, class_name: 'ProjectMember' has_many :deploy_keys_projects - has_many :deploy_keys, through: :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 :releases @@ -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, through: :cluster_project, class_name: 'Clusters::Cluster' + has_many :clusters, -> { auto_include(false) }, 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 @@ -225,7 +225,7 @@ class Project < ActiveRecord::Base has_many :project_deploy_tokens has_many :deploy_tokens, through: :project_deploy_tokens - has_many :active_runners, -> { active }, through: :runner_projects, source: :runner, class_name: 'Ci::Runner' + has_many :active_runners, -> { active.auto_include(false) }, through: :runner_projects, source: :runner, class_name: 'Ci::Runner' has_one :auto_devops, class_name: 'ProjectAutoDevops' has_many :custom_attributes, class_name: 'ProjectCustomAttribute' diff --git a/app/models/todo.rb b/app/models/todo.rb index a2ab405fdbe..aad2c1dac4e 100644 --- a/app/models/todo.rb +++ b/app/models/todo.rb @@ -22,7 +22,7 @@ class Todo < ActiveRecord::Base belongs_to :author, class_name: "User" belongs_to :note belongs_to :project - belongs_to :target, polymorphic: true, touch: true # rubocop:disable Cop/PolymorphicAssociations + belongs_to :target, -> { auto_include(false) }, polymorphic: true, touch: true # rubocop:disable Cop/PolymorphicAssociations belongs_to :user delegate :name, :email, to: :author, prefix: true, allow_nil: true diff --git a/app/models/user.rb b/app/models/user.rb index 2b95be3f888..c7e1dfaf595 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -96,9 +96,9 @@ class User < ActiveRecord::Base # Groups has_many :members has_many :group_members, -> { where(requested_at: nil) }, source: 'GroupMember' - 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 + 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 # Projects has_many :groups_projects, through: :groups, source: :projects diff --git a/changelogs/unreleased/fix-n-plus-one-when-getting-notification-settings-for-recipients.yml b/changelogs/unreleased/fix-n-plus-one-when-getting-notification-settings-for-recipients.yml new file mode 100644 index 00000000000..837bfed19b7 --- /dev/null +++ b/changelogs/unreleased/fix-n-plus-one-when-getting-notification-settings-for-recipients.yml @@ -0,0 +1,5 @@ +--- +title: Add Goldiloader to fix N+1 issues when calculating email recipients +merge_request: +author: +type: performance diff --git a/spec/requests/api/pipeline_schedules_spec.rb b/spec/requests/api/pipeline_schedules_spec.rb index 7ea25059756..91d4d5d3de9 100644 --- a/spec/requests/api/pipeline_schedules_spec.rb +++ b/spec/requests/api/pipeline_schedules_spec.rb @@ -17,6 +17,17 @@ describe API::PipelineSchedules do pipeline_schedule.pipelines << build(:ci_pipeline, project: project) end + def create_pipeline_schedules(count) + create_list(:ci_pipeline_schedule, count, project: project) + .each do |pipeline_schedule| + create(:user).tap do |user| + project.add_developer(user) + pipeline_schedule.update_attributes(owner: user) + end + pipeline_schedule.pipelines << build(:ci_pipeline, project: project) + end + end + it 'returns list of pipeline_schedules' do get api("/projects/#{project.id}/pipeline_schedules", developer) @@ -26,18 +37,14 @@ describe API::PipelineSchedules do end it 'avoids N + 1 queries' do + # We need at least two users to trigger a preload for that relation. + create_pipeline_schedules(1) + control_count = ActiveRecord::QueryRecorder.new do get api("/projects/#{project.id}/pipeline_schedules", developer) end.count - create_list(:ci_pipeline_schedule, 10, project: project) - .each do |pipeline_schedule| - create(:user).tap do |user| - project.add_developer(user) - pipeline_schedule.update_attributes(owner: user) - end - pipeline_schedule.pipelines << build(:ci_pipeline, project: project) - end + create_pipeline_schedules(10) expect do get api("/projects/#{project.id}/pipeline_schedules", developer) -- cgit v1.2.1