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

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