diff options
author | Thong Kuah <tkuah@gitlab.com> | 2019-08-06 13:39:35 +0000 |
---|---|---|
committer | Thong Kuah <tkuah@gitlab.com> | 2019-08-06 13:39:35 +0000 |
commit | 4c1a5ba822109582d85a19ac4ce7e99e5cfa9641 (patch) | |
tree | db3cb210ca38109fe84411bccc9ff29211e58610 | |
parent | 26087322713e2949f2bf207798512374757a484c (diff) | |
parent | 743497aa0502781072b84eb51c2663180813b5c6 (diff) | |
download | gitlab-ce-4c1a5ba822109582d85a19ac4ce7e99e5cfa9641.tar.gz |
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
-rw-r--r-- | app/models/deploy_key.rb | 6 | ||||
-rw-r--r-- | app/models/deploy_keys_project.rb | 3 | ||||
-rw-r--r-- | app/models/project.rb | 2 | ||||
-rw-r--r-- | app/models/user.rb | 11 | ||||
-rw-r--r-- | app/presenters/projects/settings/deploy_keys_presenter.rb | 40 | ||||
-rw-r--r-- | app/serializers/deploy_key_entity.rb | 7 | ||||
-rw-r--r-- | changelogs/unreleased/43080-speed-up-deploy-keys.yml | 5 | ||||
-rw-r--r-- | db/post_migrate/20190802235445_add_index_on_id_and_type_and_public_to_keys.rb | 23 | ||||
-rw-r--r-- | db/schema.rb | 3 | ||||
-rw-r--r-- | spec/models/user_spec.rb | 18 | ||||
-rw-r--r-- | spec/presenters/projects/settings/deploy_keys_presenter_spec.rb | 8 |
11 files changed, 78 insertions, 48 deletions
diff --git a/app/models/deploy_key.rb b/app/models/deploy_key.rb index db501b4b506..0bd90bd28e3 100644 --- a/app/models/deploy_key.rb +++ b/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 diff --git a/app/models/deploy_keys_project.rb b/app/models/deploy_keys_project.rb index 15906ed8e06..40c66d5bc4c 100644 --- a/app/models/deploy_keys_project.rb +++ b/app/models/deploy_keys_project.rb @@ -1,9 +1,8 @@ # 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) } diff --git a/app/models/project.rb b/app/models/project.rb index 8f234fba04f..33f1077e982 100644 --- a/app/models/project.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index b439d1c0c16..4630552e02e 100644 --- a/app/models/user.rb +++ b/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 diff --git a/app/presenters/projects/settings/deploy_keys_presenter.rb b/app/presenters/projects/settings/deploy_keys_presenter.rb index 85518c9a3a4..6f8c4e1f902 100644 --- a/app/presenters/projects/settings/deploy_keys_presenter.rb +++ b/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) } diff --git a/app/serializers/deploy_key_entity.rb b/app/serializers/deploy_key_entity.rb index 54bf030aba1..e47d6454780 100644 --- a/app/serializers/deploy_key_entity.rb +++ b/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 diff --git a/changelogs/unreleased/43080-speed-up-deploy-keys.yml b/changelogs/unreleased/43080-speed-up-deploy-keys.yml new file mode 100644 index 00000000000..73c9a9e5f82 --- /dev/null +++ b/changelogs/unreleased/43080-speed-up-deploy-keys.yml @@ -0,0 +1,5 @@ +--- +title: Speed up loading and filtering deploy keys and their projects +merge_request: 31384 +author: +type: performance diff --git a/db/post_migrate/20190802235445_add_index_on_id_and_type_and_public_to_keys.rb b/db/post_migrate/20190802235445_add_index_on_id_and_type_and_public_to_keys.rb new file mode 100644 index 00000000000..9b4d74b4bea --- /dev/null +++ b/db/post_migrate/20190802235445_add_index_on_id_and_type_and_public_to_keys.rb @@ -0,0 +1,23 @@ +# 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 diff --git a/db/schema.rb b/db/schema.rb index 90d93ddc736..8d5da4a07dd 100644 --- a/db/schema.rb +++ b/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 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 35c335c5b5c..46b86e8393d 100644 --- a/spec/models/user_spec.rb +++ b/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' diff --git a/spec/presenters/projects/settings/deploy_keys_presenter_spec.rb b/spec/presenters/projects/settings/deploy_keys_presenter_spec.rb index 001545bb5df..b4bf39f3cdb 100644 --- a/spec/presenters/projects/settings/deploy_keys_presenter_spec.rb +++ b/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 |