From 170c3af03a0868516310ab8cdcf822247a6e1c01 Mon Sep 17 00:00:00 2001
From: Adam Hegyi <ahegyi@gitlab.com>
Date: Fri, 29 Nov 2019 14:35:05 +0100
Subject: [PATCH] Reduce N+1 queries on deploy keys index page
- Add test to prevent further N+1 queries
---
app/models/deploy_key.rb | 10 ++-
app/models/user.rb | 2 +-
app/policies/deploy_key_policy.rb | 5 +-
.../settings/deploy_keys_presenter.rb | 62 ++++++++++++++-----
app/serializers/deploy_key_entity.rb | 11 +++-
.../21059-optimize-deploy-keys-index-page.yml | 5 ++
...ndex_to_projects_deploy_keys_deploy_key.rb | 17 +++++
db/schema.rb | 1 +
.../projects/deploy_keys_controller_spec.rb | 26 +++++---
.../settings/deploy_keys_presenter_spec.rb | 61 ++++++++++++++++--
10 files changed, 161 insertions(+), 39 deletions(-)
create mode 100644 changelogs/unreleased/21059-optimize-deploy-keys-index-page.yml
create mode 100644 db/migrate/20191205145647_add_index_to_projects_deploy_keys_deploy_key.rb
diff --git a/app/models/deploy_key.rb b/app/models/deploy_key.rb
index 19216281e48de..793ea3c29c3a1 100644
--- a/app/models/deploy_key.rb
+++ b/app/models/deploy_key.rb
@@ -9,7 +9,7 @@ class DeployKey < Key
scope :in_projects, ->(projects) { joins(:deploy_keys_projects).where('deploy_keys_projects.project_id in (?)', projects) }
scope :are_public, -> { where(public: true) }
- scope :with_projects, -> { includes(deploy_keys_projects: { project: [:route, :namespace] }) }
+ scope :with_projects, -> { includes(deploy_keys_projects: { project: [:route, namespace: :route] }) }
ignore_column :can_push, remove_after: '2019-12-15', remove_with: '12.6'
@@ -24,7 +24,7 @@ def orphaned?
end
def almost_orphaned?
- self.deploy_keys_projects.count == 1
+ self.deploy_keys_projects.size == 1
end
def destroyed_when_orphaned?
@@ -44,7 +44,11 @@ def can_push_to?(project)
end
def deploy_keys_project_for(project)
- deploy_keys_projects.find_by(project: project)
+ if association(:deploy_keys_projects).loaded?
+ deploy_keys_projects.find { |dkp| dkp.project_id.eql?(project&.id) }
+ else
+ deploy_keys_projects.find_by(project: project)
+ end
end
def projects_with_write_access
diff --git a/app/models/user.rb b/app/models/user.rb
index fd02db865828b..7b6caa9070e9f 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -997,7 +997,7 @@ def ldap_identity
end
def project_deploy_keys
- DeployKey.in_projects(authorized_projects.select(:id)).distinct(:id)
+ @project_deploy_keys ||= DeployKey.in_projects(authorized_projects.select(:id)).distinct(:id)
end
def highest_role
diff --git a/app/policies/deploy_key_policy.rb b/app/policies/deploy_key_policy.rb
index 7f0ec011e792f..b117bb5792198 100644
--- a/app/policies/deploy_key_policy.rb
+++ b/app/policies/deploy_key_policy.rb
@@ -3,10 +3,7 @@
class DeployKeyPolicy < BasePolicy
with_options scope: :subject, score: 0
condition(:private_deploy_key) { @subject.private? }
-
- # rubocop: disable CodeReuse/ActiveRecord
- condition(:has_deploy_key) { @user.project_deploy_keys.exists?(id: @subject.id) }
- # rubocop: enable CodeReuse/ActiveRecord
+ condition(:has_deploy_key) { @user.project_deploy_keys.any? { |pdk| pdk.id.eql?(@subject.id) } }
rule { anonymous }.prevent_all
diff --git a/app/presenters/projects/settings/deploy_keys_presenter.rb b/app/presenters/projects/settings/deploy_keys_presenter.rb
index 9bb7fe13593b7..66211d0269675 100644
--- a/app/presenters/projects/settings/deploy_keys_presenter.rb
+++ b/app/presenters/projects/settings/deploy_keys_presenter.rb
@@ -3,6 +3,8 @@
module Projects
module Settings
class DeployKeysPresenter < Gitlab::View::Presenter::Simple
+ include Gitlab::Utils::StrongMemoize
+
presents :project
delegate :size, to: :enabled_keys, prefix: true
delegate :size, to: :available_project_keys, prefix: true
@@ -13,37 +15,45 @@ def new_key
end
def enabled_keys
- project.deploy_keys
+ strong_memoize(:enabled_keys) do
+ project.deploy_keys.with_projects
+ end
end
def available_keys
- current_user
- .accessible_deploy_keys
- .id_not_in(enabled_keys.select(:id))
- .with_projects
+ strong_memoize(:available_keys) do
+ current_user
+ .accessible_deploy_keys
+ .id_not_in(enabled_keys.select(:id))
+ .with_projects
+ end
end
def available_project_keys
- current_user
- .project_deploy_keys
- .id_not_in(enabled_keys.select(:id))
- .with_projects
+ strong_memoize(:available_project_keys) do
+ current_user
+ .project_deploy_keys
+ .id_not_in(enabled_keys.select(:id))
+ .with_projects
+ end
end
def available_public_keys
- DeployKey
- .are_public
- .id_not_in(enabled_keys.select(:id))
- .id_not_in(available_project_keys.select(:id))
- .with_projects
+ strong_memoize(:available_public_keys) do
+ DeployKey
+ .are_public
+ .id_not_in(enabled_keys.select(:id))
+ .id_not_in(available_project_keys.select(:id))
+ .with_projects
+ end
end
def as_json
serializer = DeployKeySerializer.new # rubocop: disable CodeReuse/Serializer
- opts = { user: current_user, project: project }
+ opts = { user: current_user, project: project, readable_project_ids: readable_project_ids }
{
- enabled_keys: serializer.represent(enabled_keys.with_projects, opts),
+ enabled_keys: serializer.represent(enabled_keys, opts),
available_project_keys: serializer.represent(available_project_keys, opts),
public_keys: serializer.represent(available_public_keys, opts)
}
@@ -56,6 +66,26 @@ def to_partial_path
def form_partial_path
'projects/deploy_keys/form'
end
+
+ private
+
+ # Caching all readable project ids for the user that are associated with the queried deploy keys
+ def readable_project_ids
+ strong_memoize(:readable_projects_by_id) do
+ Set.new(user_readable_project_ids)
+ end
+ end
+
+ # rubocop: disable CodeReuse/ActiveRecord
+ def user_readable_project_ids
+ project_ids = (available_keys + available_project_keys + available_public_keys)
+ .flat_map { |deploy_key| deploy_key.deploy_keys_projects.map(&:project_id) }
+ .compact
+ .uniq
+
+ current_user.authorized_projects(Gitlab::Access::GUEST).id_in(project_ids).pluck(:id)
+ end
+ # rubocop: enable CodeReuse/ActiveRecord
end
end
end
diff --git a/app/serializers/deploy_key_entity.rb b/app/serializers/deploy_key_entity.rb
index 9a558d12bec41..2682a47fbaaee 100644
--- a/app/serializers/deploy_key_entity.rb
+++ b/app/serializers/deploy_key_entity.rb
@@ -11,8 +11,7 @@ class DeployKeyEntity < Grape::Entity
expose :updated_at
expose :deploy_keys_projects, using: DeployKeysProjectEntity do |deploy_key|
deploy_key.deploy_keys_projects.select do |deploy_key_project|
- !deploy_key_project.project&.pending_delete? &&
- Ability.allowed?(options[:user], :read_project, deploy_key_project.project)
+ !deploy_key_project.project&.pending_delete? && (allowed_to_read_project?(deploy_key_project.project) || options[:user].admin?)
end
end
expose :can_edit
@@ -23,4 +22,12 @@ def can_edit
Ability.allowed?(options[:user], :update_deploy_key, object) ||
Ability.allowed?(options[:user], :update_deploy_keys_project, object.deploy_keys_project_for(options[:project]))
end
+
+ def allowed_to_read_project?(project)
+ if options[:readable_project_ids]
+ options[:readable_project_ids].include?(project.id)
+ else
+ Ability.allowed?(options[:user], :read_project, project)
+ end
+ end
end
diff --git a/changelogs/unreleased/21059-optimize-deploy-keys-index-page.yml b/changelogs/unreleased/21059-optimize-deploy-keys-index-page.yml
new file mode 100644
index 0000000000000..18a3b275641f1
--- /dev/null
+++ b/changelogs/unreleased/21059-optimize-deploy-keys-index-page.yml
@@ -0,0 +1,5 @@
+---
+title: Optimize loading the repository deploy keys page
+merge_request: 20970
+author:
+type: performance
diff --git a/db/migrate/20191205145647_add_index_to_projects_deploy_keys_deploy_key.rb b/db/migrate/20191205145647_add_index_to_projects_deploy_keys_deploy_key.rb
new file mode 100644
index 0000000000000..f9cdc226e4ded
--- /dev/null
+++ b/db/migrate/20191205145647_add_index_to_projects_deploy_keys_deploy_key.rb
@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+class AddIndexToProjectsDeployKeysDeployKey < ActiveRecord::Migration[5.2]
+ include Gitlab::Database::MigrationHelpers
+
+ disable_ddl_transaction!
+
+ DOWNTIME = false
+
+ def up
+ add_concurrent_index :deploy_keys_projects, :deploy_key_id
+ end
+
+ def down
+ remove_concurrent_index :deploy_keys_projects, :deploy_key_id
+ end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 42420e653aa90..84b9cce3c98d0 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -1293,6 +1293,7 @@
t.datetime "created_at"
t.datetime "updated_at"
t.boolean "can_push", default: false, null: false
+ t.index ["deploy_key_id"], name: "index_deploy_keys_projects_on_deploy_key_id"
t.index ["project_id"], name: "index_deploy_keys_projects_on_project_id"
end
diff --git a/spec/controllers/projects/deploy_keys_controller_spec.rb b/spec/controllers/projects/deploy_keys_controller_spec.rb
index 8b1ca2efab2ae..2c7c99eabf67b 100644
--- a/spec/controllers/projects/deploy_keys_controller_spec.rb
+++ b/spec/controllers/projects/deploy_keys_controller_spec.rb
@@ -46,17 +46,27 @@
create(:deploy_keys_project, project: project_private, deploy_key: create(:another_deploy_key))
end
- before do
- project2.add_developer(user)
+ context 'when user has access to all projects where deploy keys are used' do
+ before do
+ project2.add_developer(user)
+ end
+
+ it 'returns json in a correct format' do
+ get :index, params: params.merge(format: :json)
+
+ expect(json_response.keys).to match_array(%w(enabled_keys available_project_keys public_keys))
+ expect(json_response['enabled_keys'].count).to eq(1)
+ expect(json_response['available_project_keys'].count).to eq(1)
+ expect(json_response['public_keys'].count).to eq(1)
+ end
end
- it 'returns json in a correct format' do
- get :index, params: params.merge(format: :json)
+ context 'when user has no access to all projects where deploy keys are used' do
+ it 'returns json in a correct format' do
+ get :index, params: params.merge(format: :json)
- expect(json_response.keys).to match_array(%w(enabled_keys available_project_keys public_keys))
- expect(json_response['enabled_keys'].count).to eq(1)
- expect(json_response['available_project_keys'].count).to eq(1)
- expect(json_response['public_keys'].count).to eq(1)
+ expect(json_response['available_project_keys'].count).to eq(0)
+ end
end
end
end
diff --git a/spec/presenters/projects/settings/deploy_keys_presenter_spec.rb b/spec/presenters/projects/settings/deploy_keys_presenter_spec.rb
index de58733c8ea15..b9cb60e414f45 100644
--- a/spec/presenters/projects/settings/deploy_keys_presenter_spec.rb
+++ b/spec/presenters/projects/settings/deploy_keys_presenter_spec.rb
@@ -5,11 +5,6 @@
describe Projects::Settings::DeployKeysPresenter do
let(:project) { create(:project) }
let(:user) { create(:user) }
- let(:deploy_key) { create(:deploy_key, public: true) }
-
- let!(:deploy_keys_project) do
- create(:deploy_keys_project, project: project, deploy_key: deploy_key)
- end
subject(:presenter) do
described_class.new(project, current_user: user)
@@ -20,6 +15,12 @@
end
describe '#enabled_keys' do
+ let!(:deploy_key) { create(:deploy_key, public: true) }
+
+ let!(:deploy_keys_project) do
+ create(:deploy_keys_project, project: project, deploy_key: deploy_key)
+ end
+
it 'returns currently enabled keys' do
expect(presenter.enabled_keys).to eq [deploy_keys_project.deploy_key]
end
@@ -53,4 +54,54 @@
expect(presenter.available_project_keys_size).to eq(1)
end
end
+
+ context 'prevent N + 1 queries' do
+ before do
+ create_records
+
+ project.add_maintainer(user)
+ end
+
+ def create_records
+ other_project = create(:project)
+ other_project.add_maintainer(user)
+
+ create(:deploy_keys_project, project: project, deploy_key: create(:deploy_key))
+ create(:deploy_keys_project, project: other_project, deploy_key: create(:deploy_key))
+ create(:deploy_key, public: true)
+ end
+
+ def execute_with_query_count
+ ActiveRecord::QueryRecorder.new { execute_presenter }.count
+ end
+
+ def execute_presenter
+ described_class.new(project, current_user: user).as_json
+ end
+
+ it 'returns correct counts' do
+ result = execute_presenter
+
+ expect(result[:enabled_keys].size).to eq(1)
+ expect(result[:available_project_keys].size).to eq(1)
+ expect(result[:public_keys].size).to eq(1)
+ end
+
+ it 'does not increase the query count' do
+ execute_presenter # make sure everything is cached
+
+ count_before = execute_with_query_count
+
+ 3.times { create_records }
+
+ count_after = execute_with_query_count
+
+ expect(count_after).to eq(count_before)
+
+ result = execute_presenter
+ expect(result[:enabled_keys].size).to eq(4)
+ expect(result[:available_project_keys].size).to eq(4)
+ expect(result[:public_keys].size).to eq(4)
+ end
+ end
end
--
GitLab