Merge branch...

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

Fix N+1 in MergeRequestParser

See merge request gitlab-org/gitlab-ee!5206

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"

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

app/policies/merge_request_policy.rb

 class MergeRequestPolicy < IssuablePolicy
   prepend EE::MergeRequestPolicy
-
-  rule { can?(:read_merge_request) | visible_to_user }.enable :read_merge_request_iid
 end

app/policies/project_policy.rb

@@ -68,6 +68,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
@@ -83,6 +99,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
@@ -152,6 +172,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
@@ -257,7 +278,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
@@ -307,6 +332,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?

changelogs/unreleased/43098-controller-projects-issuescontroller-show-executes-more-than-100-sql-queries.yml

+---
+title: Improve performance of loading issues with lots of references to merge requests
+merge_request: 17986
+author:
+type: performance

ee/app/policies/ee/project_policy.rb

@@ -114,6 +114,9 @@ module EE
         #
         # All other actions should explicitly check read project, which would
         # trigger the `classification_label_authorized` condition.
+        #
+        # `:read_project_for_iids` is not prevented by this condition, as it is
+        # used for cross-project reference checks.
         prevent :guest_access
         prevent :public_access
         prevent :public_user_access

ee/spec/features/projects/issues/viewing_issues_with_external_authorization_enabled_spec.rb

@@ -19,10 +19,16 @@ describe 'viewing an issue with cross project references' do
            title: 'I am in another project and confidential',
            project: other_project)
   end
+  let(:other_merge_request) do
+    create(:merge_request, :closed,
+           title: 'I am a merge request in another project',
+           source_project: other_project)
+  end
   let(:description_referencing_other_issue) do
-    "Referencing: #{other_issue.to_reference(project)}, and "\
-    "a confidential issue #{confidential_issue.to_reference}"\
-    "a cross project confidential issue #{other_confidential_issue.to_reference(project)}"
+    "Referencing: #{other_issue.to_reference(project)}, "\
+    "a confidential issue #{confidential_issue.to_reference}, "\
+    "a cross project confidential issue #{other_confidential_issue.to_reference(project)}, and "\
+    "a cross project merge request #{other_merge_request.to_reference(project)}"
   end
   let(:project) { create(:project) }
   let(:issue) do
@@ -85,12 +91,15 @@ describe 'viewing an issue with cross project references' do
       visit project_issue_path(project, issue)
     end
 
-    it 'shows only the link to the cross project reference' do
+    it 'shows only the link to the cross project references' do
       visit project_issue_path(project, issue)
 
       expect(page).to have_link("#{other_issue.to_reference(project)}")
+      expect(page).to have_link("#{other_merge_request.to_reference(project)}")
       expect(page).not_to have_content("#{other_issue.to_reference(project)} (#{other_issue.state})")
       expect(page).not_to have_xpath("//a[@title='#{other_issue.title}']")
+      expect(page).not_to have_content("#{other_merge_request.to_reference(project)} (#{other_merge_request.state})")
+      expect(page).not_to have_xpath("//a[@title='#{other_merge_request.title}']")
     end
 
     it 'does not link a cross project confidential issue if the user does not have access' do

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

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

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
@@ -315,10 +315,11 @@ describe ProjectPolicy do
     let(:auditor_permissions) do
       %i[
         download_code download_wiki_code read_project read_board read_list
-        read_wiki read_issue read_label read_issue_link read_milestone read_project_snippet
-        read_project_member read_note read_cycle_analytics read_pipeline
-        read_build read_commit_status read_container_image read_environment
-        read_deployment read_merge_request read_pages
+        read_project_for_iids read_issue_iid read_merge_request_iid read_wiki
+        read_issue read_label read_issue_link read_milestone
+        read_project_snippet read_project_member read_note read_cycle_analytics
+        read_pipeline read_build read_commit_status read_container_image
+        read_environment read_deployment read_merge_request read_pages
       ]
     end
 

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