Merge branch '21059-optimize-deploy-keys-index-page' into 'master'

Reduce N+1 queries on deploy keys index page

See merge request gitlab-org/gitlab!20970

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 @@ class DeployKey < Key
   end
 
   def almost_orphaned?
-    self.deploy_keys_projects.count == 1
+    self.deploy_keys_projects.size == 1
   end
 
   def destroyed_when_orphaned?
@@ -44,8 +44,12 @@ class DeployKey < Key
   end
 
   def deploy_keys_project_for(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
     Project.with_route.where(id: deploy_keys_projects.with_write_access.select(:project_id))

app/models/user.rb

@@ -1001,7 +1001,7 @@ class User < ApplicationRecord
   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

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
 

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 @@ module Projects
       end
 
       def enabled_keys
-        project.deploy_keys
+        strong_memoize(:enabled_keys) do
+          project.deploy_keys.with_projects
+        end
       end
 
       def available_keys
+        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
+        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
+        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 @@ module Projects
       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

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 @@ class DeployKeyEntity < Grape::Entity
     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

changelogs/unreleased/21059-optimize-deploy-keys-index-page.yml

+---
+title: Optimize loading the repository deploy keys page
+merge_request: 20970
+author:
+type: performance

db/migrate/20191205145647_add_index_to_projects_deploy_keys_deploy_key.rb

+# 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

db/schema.rb

@@ -1305,6 +1305,7 @@ ActiveRecord::Schema.define(version: 2019_12_08_071112) do
     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
 

spec/controllers/projects/deploy_keys_controller_spec.rb

@@ -46,6 +46,7 @@ describe Projects::DeployKeysController do
         create(:deploy_keys_project, project: project_private, deploy_key: create(:another_deploy_key))
       end
 
+      context 'when user has access to all projects where deploy keys are used' do
         before do
           project2.add_developer(user)
         end
@@ -59,6 +60,15 @@ describe Projects::DeployKeysController do
           expect(json_response['public_keys'].count).to eq(1)
         end
       end
+
+      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['available_project_keys'].count).to eq(0)
+        end
+      end
+    end
   end
 
   describe 'POST create' do

spec/presenters/projects/settings/deploy_keys_presenter_spec.rb

@@ -5,11 +5,6 @@ require 'spec_helper'
 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 @@ describe Projects::Settings::DeployKeysPresenter do
   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 @@ describe Projects::Settings::DeployKeysPresenter do
       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