Commit 84632f0a authored by Douwe Maan's avatar Douwe Maan

Merge branch 'banzai-issue-filter-queries' into 'master'

Reduce SQL query counts in IssueReferenceFilter

## What does this MR do?

This MR adds a preparation phase for reference filters that allows them to prepare/create data structures used while iterating over HTML nodes. In this particular case the preparation phase is used for issue references to greatly cut down the amount of queries executed to get projects/issues for Markdown references.

## Are there points in the code the reviewer needs to double check?

No.

## Why was this MR needed?

Rendering Markdown containing issue references would run at most two queries for every issue reference: one to get the project and one to get the issue from said project. When rendering Markdown with lots of issue references this would result in _a lot_ of queries being executed.

## What are the relevant issue numbers?

#18042

See merge request !4410
parents 0de44624 19a290e7
...@@ -103,7 +103,7 @@ module Banzai ...@@ -103,7 +103,7 @@ module Banzai
ref_pattern = object_class.reference_pattern ref_pattern = object_class.reference_pattern
link_pattern = object_class.link_reference_pattern link_pattern = object_class.link_reference_pattern
each_node do |node| nodes.each do |node|
if text_node?(node) && ref_pattern if text_node?(node) && ref_pattern
replace_text_when_pattern_matches(node, ref_pattern) do |content| replace_text_when_pattern_matches(node, ref_pattern) do |content|
object_link_filter(content, ref_pattern) object_link_filter(content, ref_pattern)
...@@ -206,6 +206,55 @@ module Banzai ...@@ -206,6 +206,55 @@ module Banzai
text text
end end
# Returns a Hash containing all object references (e.g. issue IDs) per the
# project they belong to.
def references_per_project
@references_per_project ||= begin
refs = Hash.new { |hash, key| hash[key] = Set.new }
regex = Regexp.union(object_class.reference_pattern,
object_class.link_reference_pattern)
nodes.each do |node|
node.to_html.scan(regex) do
project = $~[:project] || current_project_path
refs[project] << $~[object_sym]
end
end
refs
end
end
# Returns a Hash containing referenced projects grouped per their full
# path.
def projects_per_reference
@projects_per_reference ||= begin
hash = {}
refs = Set.new
references_per_project.each do |project_ref, _|
refs << project_ref
end
find_projects_for_paths(refs.to_a).each do |project|
hash[project.path_with_namespace] = project
end
hash
end
end
# Returns the projects for the given paths.
def find_projects_for_paths(paths)
Project.where_paths_in(paths).includes(:namespace)
end
def current_project_path
@current_project_path ||= project.path_with_namespace
end
private private
def project_refs_cache def project_refs_cache
......
...@@ -11,13 +11,40 @@ module Banzai ...@@ -11,13 +11,40 @@ module Banzai
Issue Issue
end end
def find_object(project, id) def find_object(project, iid)
project.get_issue(id) issues_per_project[project][iid]
end end
def url_for_object(issue, project) def url_for_object(issue, project)
IssuesHelper.url_for_issue(issue.iid, project, only_path: context[:only_path]) IssuesHelper.url_for_issue(issue.iid, project, only_path: context[:only_path])
end end
def project_from_ref(ref)
projects_per_reference[ref || current_project_path]
end
# Returns a Hash containing the issues per Project instance.
def issues_per_project
@issues_per_project ||= begin
hash = Hash.new { |h, k| h[k] = {} }
projects_per_reference.each do |path, project|
issue_ids = references_per_project[path]
next unless project.default_issues_tracker?
project.issues.where(iid: issue_ids.to_a).each do |issue|
hash[project][issue.iid] = issue
end
end
hash
end
end
def find_projects_for_paths(paths)
super(paths).includes(:gitlab_issue_tracker_service)
end
end end
end end
end end
require 'spec_helper'
describe Banzai::Filter::AbstractReferenceFilter do
let(:project) { create(:empty_project) }
describe '#references_per_project' do
it 'returns a Hash containing references grouped per project paths' do
doc = Nokogiri::HTML.fragment("#1 #{project.to_reference}#2")
filter = described_class.new(doc, project: project)
expect(filter).to receive(:object_class).twice.and_return(Issue)
expect(filter).to receive(:object_sym).twice.and_return(:issue)
refs = filter.references_per_project
expect(refs).to be_an_instance_of(Hash)
expect(refs[project.to_reference]).to eq(Set.new(%w[1 2]))
end
end
describe '#projects_per_reference' do
it 'returns a Hash containing projects grouped per project paths' do
doc = Nokogiri::HTML.fragment('')
filter = described_class.new(doc, project: project)
expect(filter).to receive(:references_per_project).
and_return({ project.path_with_namespace => Set.new(%w[1]) })
expect(filter.projects_per_reference).
to eq({ project.path_with_namespace => project })
end
end
describe '#find_projects_for_paths' do
it 'returns a list of Projects for a list of paths' do
doc = Nokogiri::HTML.fragment('')
filter = described_class.new(doc, project: project)
expect(filter.find_projects_for_paths([project.path_with_namespace])).
to eq([project])
end
end
describe '#current_project_path' do
it 'returns the path of the current project' do
doc = Nokogiri::HTML.fragment('')
filter = described_class.new(doc, project: project)
expect(filter.current_project_path).to eq(project.path_with_namespace)
end
end
end
...@@ -25,7 +25,9 @@ describe Banzai::Filter::IssueReferenceFilter, lib: true do ...@@ -25,7 +25,9 @@ describe Banzai::Filter::IssueReferenceFilter, lib: true do
let(:reference) { issue.to_reference } let(:reference) { issue.to_reference }
it 'ignores valid references when using non-default tracker' do it 'ignores valid references when using non-default tracker' do
expect(project).to receive(:get_issue).with(issue.iid).and_return(nil) expect_any_instance_of(described_class).to receive(:find_object).
with(project, issue.iid).
and_return(nil)
exp = act = "Issue #{reference}" exp = act = "Issue #{reference}"
expect(reference_filter(act).to_html).to eq exp expect(reference_filter(act).to_html).to eq exp
...@@ -107,8 +109,9 @@ describe Banzai::Filter::IssueReferenceFilter, lib: true do ...@@ -107,8 +109,9 @@ describe Banzai::Filter::IssueReferenceFilter, lib: true do
let(:reference) { issue.to_reference(project) } let(:reference) { issue.to_reference(project) }
it 'ignores valid references when cross-reference project uses external tracker' do it 'ignores valid references when cross-reference project uses external tracker' do
expect_any_instance_of(Project).to receive(:get_issue). expect_any_instance_of(described_class).to receive(:find_object).
with(issue.iid).and_return(nil) with(project2, issue.iid).
and_return(nil)
exp = act = "Issue #{reference}" exp = act = "Issue #{reference}"
expect(reference_filter(act).to_html).to eq exp expect(reference_filter(act).to_html).to eq exp
......
...@@ -312,7 +312,8 @@ describe GitPushService, services: true do ...@@ -312,7 +312,8 @@ describe GitPushService, services: true do
end end
it "doesn't close issues when external issue tracker is in use" do it "doesn't close issues when external issue tracker is in use" do
allow(project).to receive(:default_issues_tracker?).and_return(false) allow_any_instance_of(Project).to receive(:default_issues_tracker?).
and_return(false)
# The push still shouldn't create cross-reference notes. # The push still shouldn't create cross-reference notes.
expect do expect do
......
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