Merge branch '43080-speed-up-deploy-keys' into 'master'

Improve the performance of viewing deploy keys

Closes #43080

See merge request gitlab-org/gitlab-ce!31384

app/models/deploy_key.rb

@@ -2,12 +2,14 @@
 
 class DeployKey < Key
   include IgnorableColumn
+  include FromUnion
 
   has_many :deploy_keys_projects, inverse_of: :deploy_key, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
   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) }
+  scope :with_projects, -> { includes(deploy_keys_projects: { project: [:route, :namespace] }) }
 
   ignore_column :can_push
 
@@ -22,7 +24,7 @@ class DeployKey < Key
   end
 
   def almost_orphaned?
-    self.deploy_keys_projects.length == 1
+    self.deploy_keys_projects.count == 1
   end
 
   def destroyed_when_orphaned?
@@ -46,6 +48,6 @@ class DeployKey < Key
   end
 
   def projects_with_write_access
-    Project.preload(:route).where(id: deploy_keys_projects.with_write_access.select(:project_id))
+    Project.with_route.where(id: deploy_keys_projects.with_write_access.select(:project_id))
   end
 end

app/models/deploy_keys_project.rb

 # frozen_string_literal: true
 
 class DeployKeysProject < ApplicationRecord
-  belongs_to :project
+  belongs_to :project, inverse_of: :deploy_keys_projects
   belongs_to :deploy_key, inverse_of: :deploy_keys_projects
-
   scope :without_project_deleted,  -> { joins(:project).where(projects: { pending_delete: false }) }
   scope :in_project, ->(project) { where(project: project) }
   scope :with_write_access, -> { where(can_push: true) }

app/models/project.rb

@@ -214,7 +214,7 @@ class Project < ApplicationRecord
     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_projects, inverse_of: :project
   has_many :deploy_keys, through: :deploy_keys_projects
   has_many :users_star_projects
   has_many :starrers, through: :users_star_projects, source: :user

app/models/user.rb

@@ -933,7 +933,7 @@ class User < ApplicationRecord
   end
 
   def project_deploy_keys
-    DeployKey.unscoped.in_projects(authorized_projects.pluck(:id)).distinct(:id)
+    DeployKey.in_projects(authorized_projects.select(:id)).distinct(:id)
   end
 
   def highest_role
@@ -941,11 +941,10 @@ class User < ApplicationRecord
   end
 
   def accessible_deploy_keys
-    @accessible_deploy_keys ||= begin
-      key_ids = project_deploy_keys.pluck(:id)
-      key_ids.push(*DeployKey.are_public.pluck(:id))
-      DeployKey.where(id: key_ids)
-    end
+    DeployKey.from_union([
+      DeployKey.where(id: project_deploy_keys.select(:deploy_key_id)),
+      DeployKey.are_public
+    ])
   end
 
   def created_by

app/presenters/projects/settings/deploy_keys_presenter.rb

@@ -12,48 +12,38 @@ module Projects
         @key ||= DeployKey.new.tap { |dk| dk.deploy_keys_projects.build }
       end
 
-      # rubocop: disable CodeReuse/ActiveRecord
       def enabled_keys
-        @enabled_keys ||= project.deploy_keys.includes(:projects)
-      end
-      # rubocop: enable CodeReuse/ActiveRecord
-
-      def any_keys_enabled?
-        enabled_keys.any?
+        project.deploy_keys
       end
 
       def available_keys
-        @available_keys ||= current_user.accessible_deploy_keys - enabled_keys
+        current_user
+          .accessible_deploy_keys
+          .id_not_in(enabled_keys.select(:id))
+          .with_projects
       end
 
-      # rubocop: disable CodeReuse/ActiveRecord
       def available_project_keys
-        @available_project_keys ||= current_user.project_deploy_keys.includes(:projects) - enabled_keys
-      end
-      # rubocop: enable CodeReuse/ActiveRecord
-
-      def key_available?(deploy_key)
-        available_keys.include?(deploy_key)
+        current_user
+          .project_deploy_keys
+          .id_not_in(enabled_keys.select(:id))
+          .with_projects
       end
 
-      # rubocop: disable CodeReuse/ActiveRecord
       def available_public_keys
-        return @available_public_keys if defined?(@available_public_keys)
-
-        @available_public_keys ||= DeployKey.are_public.includes(:projects) - enabled_keys
-
-        # Public keys that are already used by another accessible project are already
-        # in @available_project_keys.
-        @available_public_keys -= available_project_keys
+        DeployKey
+          .are_public
+          .id_not_in(enabled_keys.select(:id))
+          .id_not_in(available_project_keys.select(:id))
+          .with_projects
       end
-      # rubocop: enable CodeReuse/ActiveRecord
 
       def as_json
         serializer = DeployKeySerializer.new # rubocop: disable CodeReuse/Serializer
         opts = { user: current_user }
 
         {
-          enabled_keys: serializer.represent(enabled_keys, opts),
+          enabled_keys: serializer.represent(enabled_keys.with_projects, opts),
           available_project_keys: serializer.represent(available_project_keys, opts),
           public_keys: serializer.represent(available_public_keys, opts)
         }

app/serializers/deploy_key_entity.rb

@@ -10,9 +10,10 @@ class DeployKeyEntity < Grape::Entity
   expose :created_at
   expose :updated_at
   expose :deploy_keys_projects, using: DeployKeysProjectEntity do |deploy_key|
-    deploy_key.deploy_keys_projects
-              .without_project_deleted
-              .select { |deploy_key_project| Ability.allowed?(options[:user], :read_project, deploy_key_project.project) }
+    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)
+    end
   end
   expose :can_edit
 

changelogs/unreleased/43080-speed-up-deploy-keys.yml

+---
+title: Speed up loading and filtering deploy keys and their projects
+merge_request: 31384
+author:
+type: performance

db/post_migrate/20190802235445_add_index_on_id_and_type_and_public_to_keys.rb

+# frozen_string_literal: true
+
+class AddIndexOnIdAndTypeAndPublicToKeys < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  INDEX_NAME = "index_on_deploy_keys_id_and_type_and_public"
+
+  def up
+    add_concurrent_index(:keys,
+                         [:id, :type],
+                         where: "public = 't'",
+                         unique: true,
+                         name: INDEX_NAME)
+  end
+
+  def down
+    remove_concurrent_index_by_name(:keys, INDEX_NAME)
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2019_08_02_012622) do
+ActiveRecord::Schema.define(version: 2019_08_02_235445) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "pg_trgm"
@@ -1770,6 +1770,7 @@ ActiveRecord::Schema.define(version: 2019_08_02_012622) do
     t.boolean "public", default: false, null: false
     t.datetime "last_used_at"
     t.index ["fingerprint"], name: "index_keys_on_fingerprint", unique: true
+    t.index ["id", "type"], name: "index_on_deploy_keys_id_and_type_and_public", unique: true, where: "(public = true)"
     t.index ["user_id"], name: "index_keys_on_user_id"
   end
 

spec/models/user_spec.rb

@@ -794,6 +794,24 @@ describe User do
     end
   end
 
+  describe '#accessible_deploy_keys' do
+    let(:user) { create(:user) }
+    let(:project) { create(:project) }
+    let!(:private_deploy_keys_project) { create(:deploy_keys_project) }
+    let!(:public_deploy_keys_project) { create(:deploy_keys_project) }
+    let!(:accessible_deploy_keys_project) { create(:deploy_keys_project, project: project) }
+
+    before do
+      public_deploy_keys_project.deploy_key.update(public: true)
+      project.add_developer(user)
+    end
+
+    it 'user can only see deploy keys accessible to right projects' do
+      expect(user.accessible_deploy_keys).to match_array([public_deploy_keys_project.deploy_key,
+                                                          accessible_deploy_keys_project.deploy_key])
+    end
+  end
+
   describe '#deploy_keys' do
     include_context 'user keys'
 

spec/presenters/projects/settings/deploy_keys_presenter_spec.rb

@@ -29,10 +29,6 @@ describe Projects::Settings::DeployKeysPresenter do
     it 'returns the enabled_keys size' do
       expect(presenter.enabled_keys_size).to eq(1)
     end
-
-    it 'returns true if there is any enabled_keys' do
-      expect(presenter.any_keys_enabled?).to eq(true)
-    end
   end
 
   describe '#available_keys/#available_project_keys' do
@@ -54,9 +50,5 @@ describe Projects::Settings::DeployKeysPresenter do
     it 'returns the available_project_keys size' do
       expect(presenter.available_project_keys_size).to eq(1)
     end
-
-    it 'shows if there is an available key' do
-      expect(presenter.key_available?(deploy_key)).to eq(false)
-    end
   end
 end