Commit e7b1d201 authored by Sean McGivern's avatar Sean McGivern

Fix N+1 in MergeRequestParser

read_project can be prevented by a very expensive condition, which we want to
avoid, while still not writing manual SQL queries. read_project_for_iids is used
by read_issue_iid and read_merge_request_iid to satisfy both of those
constraints, and allow the declarative policy runner to use its normal caching
strategy.
parent 8dca091f
...@@ -2,20 +2,6 @@ class IssuablePolicy < BasePolicy ...@@ -2,20 +2,6 @@ class IssuablePolicy < BasePolicy
delegate { @subject.project } delegate { @subject.project }
condition(:locked, scope: :subject, score: 0) { @subject.discussion_locked? } condition(:locked, scope: :subject, score: 0) { @subject.discussion_locked? }
# We aren't checking `:read_issue` or `:read_merge_request` in this case
# because it could be possible for a user to see an issuable-iid
# (`:read_issue_iid` or `:read_merge_request_iid`) but then wouldn't be allowed
# to read the actual issue after a more expensive `:read_issue` check.
#
# `:read_issue` & `:read_issue_iid` could diverge in gitlab-ee.
condition(:visible_to_user, score: 4) do
Project.where(id: @subject.project)
.public_or_visible_to_user(@user)
.with_feature_available_for_user(@subject, @user)
.any?
end
condition(:is_project_member) { @user && @subject.project && @subject.project.team.member?(@user) } condition(:is_project_member) { @user && @subject.project && @subject.project.team.member?(@user) }
desc "User is the assignee or author" desc "User is the assignee or author"
......
...@@ -17,6 +17,4 @@ class IssuePolicy < IssuablePolicy ...@@ -17,6 +17,4 @@ class IssuePolicy < IssuablePolicy
prevent :update_issue prevent :update_issue
prevent :admin_issue prevent :admin_issue
end end
rule { can?(:read_issue) | visible_to_user }.enable :read_issue_iid
end end
class MergeRequestPolicy < IssuablePolicy class MergeRequestPolicy < IssuablePolicy
rule { can?(:read_merge_request) | visible_to_user }.enable :read_merge_request_iid
end end
...@@ -66,6 +66,22 @@ class ProjectPolicy < BasePolicy ...@@ -66,6 +66,22 @@ class ProjectPolicy < BasePolicy
project.merge_requests_allowing_push_to_user(user).any? project.merge_requests_allowing_push_to_user(user).any?
end end
# We aren't checking `:read_issue` or `:read_merge_request` in this case
# because it could be possible for a user to see an issuable-iid
# (`:read_issue_iid` or `:read_merge_request_iid`) but then wouldn't be
# allowed to read the actual issue after a more expensive `:read_issue`
# check. These checks are intended to be used alongside
# `:read_project_for_iids`.
#
# `:read_issue` & `:read_issue_iid` could diverge in gitlab-ee.
condition(:issues_visible_to_user, score: 4) do
@subject.feature_available?(:issues, @user)
end
condition(:merge_requests_visible_to_user, score: 4) do
@subject.feature_available?(:merge_requests, @user)
end
features = %w[ features = %w[
merge_requests merge_requests
issues issues
...@@ -81,6 +97,10 @@ class ProjectPolicy < BasePolicy ...@@ -81,6 +97,10 @@ class ProjectPolicy < BasePolicy
condition(:"#{f}_disabled", score: 32) { !feature_available?(f.to_sym) } condition(:"#{f}_disabled", score: 32) { !feature_available?(f.to_sym) }
end end
# `:read_project` may be prevented in EE, but `:read_project_for_iids` should
# not.
rule { guest | admin }.enable :read_project_for_iids
rule { guest }.enable :guest_access rule { guest }.enable :guest_access
rule { reporter }.enable :reporter_access rule { reporter }.enable :reporter_access
rule { developer }.enable :developer_access rule { developer }.enable :developer_access
...@@ -150,6 +170,7 @@ class ProjectPolicy < BasePolicy ...@@ -150,6 +170,7 @@ class ProjectPolicy < BasePolicy
# where we enable or prevent it based on other coditions. # where we enable or prevent it based on other coditions.
rule { (~anonymous & public_project) | internal_access }.policy do rule { (~anonymous & public_project) | internal_access }.policy do
enable :public_user_access enable :public_user_access
enable :read_project_for_iids
end end
rule { can?(:public_user_access) }.policy do rule { can?(:public_user_access) }.policy do
...@@ -255,7 +276,11 @@ class ProjectPolicy < BasePolicy ...@@ -255,7 +276,11 @@ class ProjectPolicy < BasePolicy
end end
rule { anonymous & ~public_project }.prevent_all rule { anonymous & ~public_project }.prevent_all
rule { public_project }.enable(:public_access)
rule { public_project }.policy do
enable :public_access
enable :read_project_for_iids
end
rule { can?(:public_access) }.policy do rule { can?(:public_access) }.policy do
enable :read_project enable :read_project
...@@ -305,6 +330,14 @@ class ProjectPolicy < BasePolicy ...@@ -305,6 +330,14 @@ class ProjectPolicy < BasePolicy
enable :update_pipeline enable :update_pipeline
end end
rule do
(can?(:read_project_for_iids) & issues_visible_to_user) | can?(:read_issue)
end.enable :read_issue_iid
rule do
(can?(:read_project_for_iids) & merge_requests_visible_to_user) | can?(:read_merge_request)
end.enable :read_merge_request_iid
private private
def team_member? def team_member?
......
---
title: Improve performance of loading issues with lots of references to merge requests
merge_request: 17986
author:
type: performance
...@@ -117,4 +117,27 @@ describe Banzai::ReferenceParser::IssueParser do ...@@ -117,4 +117,27 @@ describe Banzai::ReferenceParser::IssueParser do
expect(subject.records_for_nodes(nodes)).to eq({ link => issue }) expect(subject.records_for_nodes(nodes)).to eq({ link => issue })
end end
end end
context 'when checking multiple merge requests on another project' do
let(:other_project) { create(:project, :public) }
let(:other_issue) { create(:issue, project: other_project) }
let(:control_links) do
[issue_link(other_issue)]
end
let(:actual_links) do
control_links + [issue_link(create(:issue, project: other_project))]
end
def issue_link(issue)
Nokogiri::HTML.fragment(%Q{<a data-issue="#{issue.id}"></a>}).children[0]
end
before do
project.add_developer(user)
end
it_behaves_like 'no N+1 queries'
end
end end
...@@ -4,14 +4,13 @@ describe Banzai::ReferenceParser::MergeRequestParser do ...@@ -4,14 +4,13 @@ describe Banzai::ReferenceParser::MergeRequestParser do
include ReferenceParserHelpers include ReferenceParserHelpers
let(:user) { create(:user) } let(:user) { create(:user) }
let(:merge_request) { create(:merge_request) } let(:project) { create(:project, :public) }
subject { described_class.new(merge_request.target_project, user) } let(:merge_request) { create(:merge_request, source_project: project) }
subject { described_class.new(project, user) }
let(:link) { empty_html_link } let(:link) { empty_html_link }
describe '#nodes_visible_to_user' do describe '#nodes_visible_to_user' do
context 'when the link has a data-issue attribute' do context 'when the link has a data-issue attribute' do
let(:project) { merge_request.target_project }
before do before do
project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC) project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC)
link['data-merge-request'] = merge_request.id.to_s link['data-merge-request'] = merge_request.id.to_s
...@@ -40,4 +39,27 @@ describe Banzai::ReferenceParser::MergeRequestParser do ...@@ -40,4 +39,27 @@ describe Banzai::ReferenceParser::MergeRequestParser do
end end
end end
end end
context 'when checking multiple merge requests on another project' do
let(:other_project) { create(:project, :public) }
let(:other_merge_request) { create(:merge_request, source_project: other_project) }
let(:control_links) do
[merge_request_link(other_merge_request)]
end
let(:actual_links) do
control_links + [merge_request_link(create(:merge_request, :conflict, source_project: other_project))]
end
def merge_request_link(merge_request)
Nokogiri::HTML.fragment(%Q{<a data-merge-request="#{merge_request.id}"></a>}).children[0]
end
before do
project.add_developer(user)
end
it_behaves_like 'no N+1 queries'
end
end end
...@@ -11,10 +11,10 @@ describe ProjectPolicy do ...@@ -11,10 +11,10 @@ describe ProjectPolicy do
let(:base_guest_permissions) do let(:base_guest_permissions) do
%i[ %i[
read_project read_board read_list read_wiki read_issue read_label read_project read_board read_list read_wiki read_issue
read_milestone read_project_snippet read_project_member read_project_for_iids read_issue_iid read_merge_request_iid read_label
read_note create_project create_issue create_note read_milestone read_project_snippet read_project_member read_note
upload_file create_project create_issue create_note upload_file
] ]
end end
...@@ -120,7 +120,7 @@ describe ProjectPolicy do ...@@ -120,7 +120,7 @@ describe ProjectPolicy do
project.issues_enabled = false project.issues_enabled = false
project.save! project.save!
expect_disallowed :read_issue, :create_issue, :update_issue, :admin_issue expect_disallowed :read_issue, :read_issue_iid, :create_issue, :update_issue, :admin_issue
end end
end end
...@@ -131,7 +131,7 @@ describe ProjectPolicy do ...@@ -131,7 +131,7 @@ describe ProjectPolicy do
project.issues_enabled = false project.issues_enabled = false
project.save! project.save!
expect_disallowed :read_issue, :create_issue, :update_issue, :admin_issue expect_disallowed :read_issue, :read_issue_iid, :create_issue, :update_issue, :admin_issue
end end
end end
end end
......
...@@ -2,4 +2,34 @@ module ReferenceParserHelpers ...@@ -2,4 +2,34 @@ module ReferenceParserHelpers
def empty_html_link def empty_html_link
Nokogiri::HTML.fragment('<a></a>').children[0] Nokogiri::HTML.fragment('<a></a>').children[0]
end end
shared_examples 'no N+1 queries' do
it 'avoids N+1 queries in #nodes_visible_to_user', :request_store do
record_queries = lambda do |links|
ActiveRecord::QueryRecorder.new do
described_class.new(project, user).nodes_visible_to_user(user, links)
end
end
control = record_queries.call(control_links)
actual = record_queries.call(actual_links)
expect(actual.count).to be <= control.count
expect(actual.cached_count).to be <= control.cached_count
end
it 'avoids N+1 queries in #records_for_nodes', :request_store do
record_queries = lambda do |links|
ActiveRecord::QueryRecorder.new do
described_class.new(project, user).records_for_nodes(links)
end
end
control = record_queries.call(control_links)
actual = record_queries.call(actual_links)
expect(actual.count).to be <= control.count
expect(actual.cached_count).to be <= control.cached_count
end
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