Skip to content

G
gitlab-ce
Commit e330a77c authored by Sean McGivern's avatar Sean McGivern
Browse files
Options

Speed up checking for approvers remaining 

1. Use the ProjectAuthorization model to find users with developer access to
   this project. This is dramatically faster than the old version, with the
   complicated ORs, on staging: ~10ms vs ~800ms.
2. Use `to_a` explicitly when checking `any?` on a relation. Refactoring out the
   `any?` checks would be painful, but as we often use the results anyway,
   `to_a` lets us save a query.

Adding this up gets us the below (for a MR on the gitlab-ce project in staging).

Before:

    Project Load (2.3ms) SELECT "projects".* FROM "projects" WHERE
    "projects"."pending_delete" = 'f' AND "projects"."id" = 13083 ORDER BY
    "projects"."id" DESC LIMIT 1 [["pending_delete", false], ["id", 13083]]

    Approver Exists (1.2ms) SELECT 1 AS one FROM "approvers" WHERE
    "approvers"."target_id" = 2294769 AND "approvers"."target_type" =
    'MergeRequest' LIMIT 1 [["target_id", 2294769], ["target_type",
    "MergeRequest"]]

    ApproverGroup Exists (0.8ms) SELECT 1 AS one FROM "approver_groups" WHERE
    "approver_groups"."target_id" = 2294769 AND "approver_groups"."target_type"
    = 'MergeRequest' LIMIT 1 [["target_id", 2294769], ["target_type",
    "MergeRequest"]]

    User Load (2.7ms) SELECT "users".* FROM "users" WHERE "users"."id" = 840794
    ORDER BY "users"."id" DESC LIMIT 1 [["id", 840794]]

    Approver Load (0.9ms) SELECT "approvers".* FROM "approvers" WHERE
    "approvers"."target_id" = 13083 AND "approvers"."target_type" = 'Project'
    AND ("approvers"."user_id" != 840794) [["target_id", 13083], ["target_type",
    "Project"], ["user_id", 840794]]

    Approver Exists (0.6ms) SELECT 1 AS one FROM "approvers" WHERE
    "approvers"."target_id" = 2294769 AND "approvers"."target_type" =
    'MergeRequest' LIMIT 1 [["target_id", 2294769], ["target_type",
    "MergeRequest"]]

    ApproverGroup Exists (0.6ms) SELECT 1 AS one FROM "approver_groups" WHERE
    "approver_groups"."target_id" = 2294769 AND "approver_groups"."target_type"
    = 'MergeRequest' LIMIT 1 [["target_id", 2294769], ["target_type",
    "MergeRequest"]]

    ApproverGroup Load (0.7ms) SELECT "approver_groups".* FROM "approver_groups"
    WHERE "approver_groups"."target_id" = 13083 AND
    "approver_groups"."target_type" = 'Project' [["target_id", 13083],
    ["target_type", "Project"]]

    Group Load (1.3ms) SELECT "namespaces".* FROM "namespaces" WHERE
    "namespaces"."deleted_at" IS NULL AND "namespaces"."type" IN ('Group') AND
    "namespaces"."id" = 9970 AND "namespaces"."type" IN ('Group') AND
    "namespaces"."type" = 'Group' ORDER BY "namespaces"."id" DESC LIMIT 1
    [["id", 9970], ["type", "Group"]]

    (1222.4ms) SELECT COUNT(*) FROM "users" WHERE ("users"."state"
    IN ('active')) AND ("users"."ghost" = 'f' OR "users"."ghost" IS NULL)
    AND ("users"."support_bot" = 'f' OR "users"."support_bot" IS NULL) AND ((id
    IN (SELECT "members"."user_id" FROM "members" WHERE "members"."source_type"
    = 'Project' AND "members"."type" IN ('ProjectMember') AND
    "members"."source_id" = 13083 AND "members"."source_type" = 'Project' AND
    "members"."type" IN ('ProjectMember') AND "members"."requested_at" IS NULL
    AND (access_level > 20) ORDER BY "members"."id" DESC) OR id IN (SELECT
    "members"."user_id" FROM "members" WHERE "members"."source_type" =
    'Namespace' AND "members"."type" IN ('GroupMember') AND
    "members"."source_id" = 9970 AND "members"."source_type" = 'Namespace' AND
    "members"."type" IN ('GroupMember') AND "members"."requested_at" IS NULL
    AND (access_level > 20) ORDER BY "members"."id" DESC)) AND id NOT IN (SELECT
    "approvals"."user_id" FROM "approvals" WHERE "approvals"."merge_request_id"
    = 2294769)) AND ("users"."id" != 840794) [["id", 840794]]

After:

    Project Load (2.5ms) SELECT "projects".* FROM "projects" WHERE
    "projects"."pending_delete" = 'f' AND "projects"."id" = 13083 ORDER BY
    "projects"."id" DESC LIMIT 1 [["pending_delete", false], ["id", 13083]]

    Approver Load (1.3ms) SELECT "approvers".* FROM "approvers" WHERE
    "approvers"."target_id" = 2294769 AND "approvers"."target_type" =
    'MergeRequest' [["target_id", 2294769], ["target_type", "MergeRequest"]]

    ApproverGroup Load (1.5ms) SELECT "approver_groups".* FROM "approver_groups"
    WHERE "approver_groups"."target_id" = 2294769 AND
    "approver_groups"."target_type" = 'MergeRequest' [["target_id", 2294769],
    ["target_type", "MergeRequest"]]

    User Load (2.9ms) SELECT "users".* FROM "users" WHERE "users"."id" = 840794
    ORDER BY "users"."id" DESC LIMIT 1 [["id", 840794]]

    Approver Load (0.9ms) SELECT "approvers".* FROM "approvers" WHERE
    "approvers"."target_id" = 13083 AND "approvers"."target_type" = 'Project'
    AND ("approvers"."user_id" != 840794) [["target_id", 13083], ["target_type",
    "Project"], ["user_id", 840794]]

    ApproverGroup Load (0.7ms) SELECT "approver_groups".* FROM "approver_groups"
    WHERE "approver_groups"."target_id" = 13083 AND
    "approver_groups"."target_type" = 'Project' [["target_id", 13083],
    ["target_type", "Project"]]

    (6.2ms) SELECT COUNT(*) FROM "users" WHERE ("users"."state" IN ('active'))
    AND ("users"."ghost" = 'f' OR "users"."ghost" IS NULL)
    AND ("users"."support_bot" = 'f' OR "users"."support_bot" IS NULL) AND ((id
    IN (SELECT "project_authorizations"."user_id" FROM "project_authorizations"
    WHERE (project_id = 13083 AND access_level > 20))) AND id NOT IN (SELECT
    "approvals"."user_id" FROM "approvals" WHERE "approvals"."merge_request_id"
    = 2294769)) AND ("users"."id" != 840794) [["id", 840794]]
parent 27a8cc7b
Hide whitespace changes
Inline Side-by-side
Showing with 19 additions and 20 deletions
app/models/concerns/approvable.rb
View file @ e330a77c
......@@ -36,20 +36,16 @@ module Approvable
#
def number_of_potential_approvers
has_access = ['access_level > ?', Member::REPORTER]
all_approvers = all_approvers_including_groups
wheres = [
"id IN (#{project.members.where(has_access).select(:user_id).to_sql})"
"id IN (#{project.project_authorizations.where(has_access).select(:user_id).to_sql})"
]
all_approvers = all_approvers_including_groups
if all_approvers.any?
wheres << "id IN (#{all_approvers.map(&:id).join(', ')})"
end
if project.group
wheres << "id IN (#{project.group.members.where(has_access).select(:user_id).to_sql})"
end
users = User
.active
.where("(#{wheres.join(' OR ')}) AND id NOT IN (#{approvals.select(:user_id).to_sql})")
......@@ -73,7 +69,6 @@ module Approvable
#
def overall_approvers
approvers_relation = approvers_overwritten? ? approvers : target_project.approvers
approvers_relation = approvers_relation.where.not(user_id: author.id) if author
approvers_relation
......@@ -113,7 +108,7 @@ module Approvable
end
def approvers_overwritten?
approvers.any? || approver_groups.any?
approvers.to_a.any? || approver_groups.to_a.any?
end
def can_approve?(user)
......
changelogs/unreleased-ee/improve-approvers-queries.yml 0 → 100644
View file @ e330a77c
---
title: Speed up checking for approvers remaining
merge_request:
author:
spec/models/merge_request_spec.rb
View file @ e330a77c
......@@ -523,7 +523,7 @@ describe MergeRequest, models: true do
it "includes approvers set on the MR" do
expect do
create(:approver, user: create(:user), target: merge_request)
end.to change { merge_request.number_of_potential_approvers }.by(1)
end.to change { merge_request.reload.number_of_potential_approvers }.by(1)
end
it "includes approvers from group" do
......@@ -531,16 +531,16 @@ describe MergeRequest, models: true do
expect do
create(:approver_group, group: group, target: merge_request)
end.to change { merge_request.number_of_potential_approvers }.by(1)
end.to change { merge_request.reload.number_of_potential_approvers }.by(1)
end
it "includes project members with developer access and up" do
expect do
project.team << [create(:user), :guest]
project.team << [create(:user), :reporter]
project.team << [create(:user), :developer]
project.team << [create(:user), :master]
end.to change { merge_request.number_of_potential_approvers }.by(2)
project.add_guest(create(:user))
project.add_reporter(create(:user))
project.add_developer(create(:user))
project.add_master(create(:user))
end.to change { merge_request.reload.number_of_potential_approvers }.by(2)
end
it "excludes users who have already approved the MR" do
......@@ -548,14 +548,14 @@ describe MergeRequest, models: true do
approver = create(:user)
create(:approver, user: approver, target: merge_request)
create(:approval, user: approver, merge_request: merge_request)
end.not_to change { merge_request.number_of_potential_approvers }
end.not_to change { merge_request.reload.number_of_potential_approvers }
end
it "excludes the MR author" do
expect do
create(:approver, user: create(:user), target: merge_request)
create(:approver, user: author, target: merge_request)
end.to change { merge_request.number_of_potential_approvers }.by(1)
end.to change { merge_request.reload.number_of_potential_approvers }.by(1)
end
it "excludes blocked users" do
......@@ -564,7 +564,7 @@ describe MergeRequest, models: true do
project.team << [developer, :developer]
project.team << [blocked_developer, :developer]
expect(merge_request.number_of_potential_approvers).to eq(1)
expect(merge_request.reload.number_of_potential_approvers).to eq(1)
end
context "when the project is part of a group" do
......@@ -579,7 +579,7 @@ describe MergeRequest, models: true do
group.add_master(create(:user))
blocked_developer = create(:user).tap { |u| u.block! }
group.add_developer(blocked_developer)
end.to change { merge_request.number_of_potential_approvers }.by(2)
end.to change { merge_request.reload.number_of_potential_approvers }.by(2)
end
end
end
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7