Commit 2693edc4 authored by Douwe Maan's avatar Douwe Maan

Merge branch...

Merge branch '43098-controller-projects-issuescontroller-show-executes-more-than-100-sql-queries' into 'master'

Resolve "Controller Projects::IssuesController#show executes more than 100 SQL queries"

Closes #43098

See merge request gitlab-org/gitlab-ce!17986
parents 082bf1c6 e7b1d201
......@@ -2,20 +2,6 @@ class IssuablePolicy < BasePolicy
delegate { @subject.project }
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) }
desc "User is the assignee or author"
......
......@@ -17,6 +17,4 @@ class IssuePolicy < IssuablePolicy
prevent :update_issue
prevent :admin_issue
end
rule { can?(:read_issue) | visible_to_user }.enable :read_issue_iid
end
class MergeRequestPolicy < IssuablePolicy
rule { can?(:read_merge_request) | visible_to_user }.enable :read_merge_request_iid
end
......@@ -66,6 +66,22 @@ class ProjectPolicy < BasePolicy
project.merge_requests_allowing_push_to_user(user).any?
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[
merge_requests
issues
......@@ -81,6 +97,10 @@ class ProjectPolicy < BasePolicy
condition(:"#{f}_disabled", score: 32) { !feature_available?(f.to_sym) }
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 { reporter }.enable :reporter_access
rule { developer }.enable :developer_access
......@@ -150,6 +170,7 @@ class ProjectPolicy < BasePolicy
# where we enable or prevent it based on other coditions.
rule { (~anonymous & public_project) | internal_access }.policy do
enable :public_user_access
enable :read_project_for_iids
end
rule { can?(:public_user_access) }.policy do
......@@ -255,7 +276,11 @@ class ProjectPolicy < BasePolicy
end
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
enable :read_project
......@@ -305,6 +330,14 @@ class ProjectPolicy < BasePolicy
enable :update_pipeline
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
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
expect(subject.records_for_nodes(nodes)).to eq({ link => issue })
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
......@@ -4,14 +4,13 @@ describe Banzai::ReferenceParser::MergeRequestParser do
include ReferenceParserHelpers
let(:user) { create(:user) }
let(:merge_request) { create(:merge_request) }
subject { described_class.new(merge_request.target_project, user) }
let(:project) { create(:project, :public) }
let(:merge_request) { create(:merge_request, source_project: project) }
subject { described_class.new(project, user) }
let(:link) { empty_html_link }
describe '#nodes_visible_to_user' do
context 'when the link has a data-issue attribute' do
let(:project) { merge_request.target_project }
before do
project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC)
link['data-merge-request'] = merge_request.id.to_s
......@@ -40,4 +39,27 @@ describe Banzai::ReferenceParser::MergeRequestParser do
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
......@@ -11,10 +11,10 @@ describe ProjectPolicy do
let(:base_guest_permissions) do
%i[
read_project read_board read_list read_wiki read_issue read_label
read_milestone read_project_snippet read_project_member
read_note create_project create_issue create_note
upload_file
read_project read_board read_list read_wiki read_issue
read_project_for_iids read_issue_iid read_merge_request_iid read_label
read_milestone read_project_snippet read_project_member read_note
create_project create_issue create_note upload_file
]
end
......@@ -120,7 +120,7 @@ describe ProjectPolicy do
project.issues_enabled = false
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
......@@ -131,7 +131,7 @@ describe ProjectPolicy do
project.issues_enabled = false
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
......
......@@ -2,4 +2,34 @@ module ReferenceParserHelpers
def empty_html_link
Nokogiri::HTML.fragment('<a></a>').children[0]
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
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