summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorSean McGivern <sean@gitlab.com>2018-04-02 16:14:20 +0100
committerSean McGivern <sean@gitlab.com>2018-04-05 13:59:05 +0100
commite7b1d201dd56611eff7e796e9a390a7b21df51d1 (patch)
tree77918cabd27f31980c5f2d49d4fcd562682688b3
parent8dca091ff7f04bb92a7835ebeff783b7f0ef76cd (diff)
downloadgitlab-ce-e7b1d201dd56611eff7e796e9a390a7b21df51d1.tar.gz
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.
-rw-r--r--app/policies/issuable_policy.rb14
-rw-r--r--app/policies/issue_policy.rb2
-rw-r--r--app/policies/merge_request_policy.rb1
-rw-r--r--app/policies/project_policy.rb35
-rw-r--r--changelogs/unreleased/43098-controller-projects-issuescontroller-show-executes-more-than-100-sql-queries.yml5
-rw-r--r--spec/lib/banzai/reference_parser/issue_parser_spec.rb23
-rw-r--r--spec/lib/banzai/reference_parser/merge_request_parser_spec.rb30
-rw-r--r--spec/policies/project_policy_spec.rb12
-rw-r--r--spec/support/reference_parser_helpers.rb30
9 files changed, 124 insertions, 28 deletions
diff --git a/app/policies/issuable_policy.rb b/app/policies/issuable_policy.rb
index 3f6d7d04667..e86d1c8f98e 100644
--- a/app/policies/issuable_policy.rb
+++ b/app/policies/issuable_policy.rb
@@ -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"
diff --git a/app/policies/issue_policy.rb b/app/policies/issue_policy.rb
index ed499511999..263c6e3039c 100644
--- a/app/policies/issue_policy.rb
+++ b/app/policies/issue_policy.rb
@@ -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
diff --git a/app/policies/merge_request_policy.rb b/app/policies/merge_request_policy.rb
index e003376d219..c3fe857f8a2 100644
--- a/app/policies/merge_request_policy.rb
+++ b/app/policies/merge_request_policy.rb
@@ -1,3 +1,2 @@
class MergeRequestPolicy < IssuablePolicy
- rule { can?(:read_merge_request) | visible_to_user }.enable :read_merge_request_iid
end
diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb
index 57ab0c23dcd..b1ed034cd00 100644
--- a/app/policies/project_policy.rb
+++ b/app/policies/project_policy.rb
@@ -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?
diff --git a/changelogs/unreleased/43098-controller-projects-issuescontroller-show-executes-more-than-100-sql-queries.yml b/changelogs/unreleased/43098-controller-projects-issuescontroller-show-executes-more-than-100-sql-queries.yml
new file mode 100644
index 00000000000..686258460e0
--- /dev/null
+++ b/changelogs/unreleased/43098-controller-projects-issuescontroller-show-executes-more-than-100-sql-queries.yml
@@ -0,0 +1,5 @@
+---
+title: Improve performance of loading issues with lots of references to merge requests
+merge_request: 17986
+author:
+type: performance
diff --git a/spec/lib/banzai/reference_parser/issue_parser_spec.rb b/spec/lib/banzai/reference_parser/issue_parser_spec.rb
index 0a63567ee40..cb7f8b20dda 100644
--- a/spec/lib/banzai/reference_parser/issue_parser_spec.rb
+++ b/spec/lib/banzai/reference_parser/issue_parser_spec.rb
@@ -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
diff --git a/spec/lib/banzai/reference_parser/merge_request_parser_spec.rb b/spec/lib/banzai/reference_parser/merge_request_parser_spec.rb
index 775749ae3a7..14542342cf6 100644
--- a/spec/lib/banzai/reference_parser/merge_request_parser_spec.rb
+++ b/spec/lib/banzai/reference_parser/merge_request_parser_spec.rb
@@ -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
diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb
index ea76e604153..905d82b3bb1 100644
--- a/spec/policies/project_policy_spec.rb
+++ b/spec/policies/project_policy_spec.rb
@@ -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
diff --git a/spec/support/reference_parser_helpers.rb b/spec/support/reference_parser_helpers.rb
index 01689194eac..5d5e80851e6 100644
--- a/spec/support/reference_parser_helpers.rb
+++ b/spec/support/reference_parser_helpers.rb
@@ -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