Skip to content

G
gitlab-ce
Commit 81109da9 authored by Brett Walker's avatar Brett Walker
Browse files
Options

Use reference cache for iterations 

And remove some unused specs

Changelog: performance
EE: true
parent 3ee03d28
Hide whitespace changes
Inline Side-by-side
Showing with 149 additions and 147 deletions
ee/app/finders/iterations_finder.rb
View file @ 81109da9
......@@ -20,7 +20,9 @@ class IterationsFinder
@current_user = current_user
end
def execute
def execute(skip_authorization: false)
@skip_authorization = skip_authorization
items = Iteration.all
items = by_id(items)
items = by_iid(items)
......@@ -36,8 +38,10 @@ class IterationsFinder
private
attr_reader :skip_authorization
def by_groups(items)
return Iteration.none unless Ability.allowed?(current_user, :read_iteration, params[:parent])
return Iteration.none unless skip_authorization || Ability.allowed?(current_user, :read_iteration, params[:parent])
items.of_groups(groups)
end
......
ee/lib/ee/banzai/filter/references/iteration_reference_filter.rb
View file @ 81109da9
......@@ -8,10 +8,56 @@ module EE
module IterationReferenceFilter
include ::Gitlab::Utils::StrongMemoize
def find_object(parent, id)
return unless valid_context?(parent)
def parent_records(parent, ids)
return Iteration.none unless valid_context?(parent)
find_iteration(parent, id: id)
iteration_ids = ids.map {|y| y[:iteration_id]}.compact
unless iteration_ids.empty?
id_relation = find_iterations(parent, ids: iteration_ids)
end
iteration_names = ids.map {|y| y[:iteration_name]}.compact
unless iteration_names.empty?
iteration_relation = find_iterations(parent, names: iteration_names)
end
relation = [id_relation, iteration_relation].compact
return ::Iteration.none if relation.all?(::Iteration.none)
::Iteration.from_union(relation).includes(:project, :group) # rubocop: disable CodeReuse/ActiveRecord
end
def find_object(parent_object, id)
key = reference_cache.records_per_parent[parent_object].keys.find do |k|
k[:iteration_id] == id[:iteration_id] || k[:iteration_name] == id[:iteration_name]
end
reference_cache.records_per_parent[parent_object][key] if key
end
# Transform a symbol extracted from the text to a meaningful value
#
# This method has the contract that if a string `ref` refers to a
# record `record`, then `parse_symbol(ref) == record_identifier(record)`.
#
# This contract is slightly broken here, as we only have either the iteration_id
# or the iteration_name, but not both. But below, we have both pieces of information.
# It's accounted for in `find_object`
def parse_symbol(symbol, match_data)
if symbol
# when parsing links, there is no `match_data[:iteration_id]`, but `symbol`
# holds the id
{ iteration_id: symbol.to_i, iteration_name: nil }
else
{ iteration_id: match_data[:iteration_id]&.to_i, iteration_name: match_data[:iteration_name]&.tr('"', '') }
end
end
# This method has the contract that if a string `ref` refers to a
# record `record`, then `class.parse_symbol(ref) == record_identifier(record)`.
# See note in `parse_symbol` above
def record_identifier(record)
{ iteration_id: record.id, iteration_name: record.name }
end
def valid_context?(parent)
......@@ -37,12 +83,14 @@ module EE
return super(text, pattern) if pattern != ::Iteration.reference_pattern
iterations = {}
unescaped_html = unescape_html_entities(text).gsub(pattern) do |match|
iteration = parse_and_find_iteration($~[:project], $~[:namespace], $~[:iteration_id], $~[:iteration_name])
if iteration
iterations[iteration.id] = yield match, iteration.id, $~[:project], $~[:namespace], $~
"#{::Banzai::Filter::References::AbstractReferenceFilter::REFERENCE_PLACEHOLDER}#{iteration.id}"
unescaped_html = unescape_html_entities(text).gsub(pattern).with_index do |match, index|
ident = identifier($~)
iteration = yield match, ident, $~[:project], $~[:namespace], $~
if iteration != match
iterations[index] = iteration
"#{::Banzai::Filter::References::AbstractReferenceFilter::REFERENCE_PLACEHOLDER}#{index}"
else
match
end
......@@ -53,35 +101,16 @@ module EE
escape_with_placeholders(unescaped_html, iterations)
end
def parse_and_find_iteration(project_ref, namespace_ref, iteration_id, iteration_name)
project_path = reference_cache.full_project_path(namespace_ref, project_ref)
# Returns group if project is not found by path
parent = parent_from_ref(project_path)
return unless parent
iteration_params = iteration_params(iteration_id, iteration_name)
find_iteration(parent, iteration_params)
end
def find_iterations(parent, ids: nil, names: nil)
finder_params = iteration_finder_params(parent, ids: ids, names: names)
def iteration_params(id, name)
if name
{ name: name.tr('"', '') }
else
{ id: id.to_i }
end
IterationsFinder.new(user, finder_params).execute(skip_authorization: true)
end
# rubocop: disable CodeReuse/ActiveRecord
def find_iteration(parent, params)
::Iteration.for_projects_and_groups(project_ids(parent), group_and_ancestors_ids(parent)).find_by(**params)
end
# rubocop: enable CodeReuse/ActiveRecord
def iteration_finder_params(parent, ids: nil, names: nil)
parms = ids.present? ? { id: ids } : { title: names }
def project_ids(parent)
parent.id if project_context?(parent)
{ parent: parent, include_ancestors: true }.merge(parms)
end
def group_and_ancestors_ids(parent)
......@@ -94,8 +123,8 @@ module EE
def url_for_object(iteration, _parent)
::Gitlab::Routing
.url_helpers
.iteration_url(iteration, only_path: context[:only_path])
.url_helpers
.iteration_url(iteration, only_path: context[:only_path])
end
def object_link_text(object, matches)
......@@ -112,6 +141,14 @@ module EE
def object_link_title(_object, _matches)
'Iteration'
end
def parent
project || group
end
def requires_unescaping?
true
end
end
end
end
......
ee/spec/finders/iterations_finder_spec.rb
View file @ 81109da9
......@@ -37,6 +37,14 @@ RSpec.describe IterationsFinder do
expect(subject).to be_empty
end
end
context 'when skipping authorization' do
let(:params) { { parent: parent } }
it 'returns iterations' do
expect(described_class.new(user, params).execute(skip_authorization: true)).not_to be_empty
end
end
end
context 'with permissions' do
......
ee/spec/lib/banzai/filter/references/iteration_reference_filter_spec.rb
View file @ 81109da9
......@@ -168,111 +168,6 @@ RSpec.describe Banzai::Filter::References::IterationReferenceFilter do
end
end
shared_examples 'linking to a iteration as the entire link' do
let(:unquoted_reference) { "#{Iteration.reference_prefix}#{iteration.name}" }
let(:link) { urls.iteration_url(iteration) }
let(:link_reference) { %Q{<a href="#{link}">#{link}</a>} }
it 'replaces the link text with the iteration reference' do
doc = reference_filter("See #{link}")
expect(doc.css('a').first.text).to eq(unquoted_reference)
end
it 'includes a data-project attribute' do
doc = reference_filter("Iteration #{link_reference}")
link = doc.css('a').first
expect(link).to have_attribute('data-project')
expect(link.attr('data-project')).to eq project.id.to_s
end
it 'includes a data-iteration attribute' do
doc = reference_filter("See #{link_reference}")
link = doc.css('a').first
expect(link).to have_attribute('data-iteration')
expect(link.attr('data-iteration')).to eq iteration.id.to_s
end
end
shared_examples 'cross-project / cross-namespace complete reference' do
let(:namespace) { create(:namespace) }
let(:another_project) { create(:project, :public, namespace: namespace) }
let(:iteration) { create(:iteration, project: another_project) }
let(:reference) { "#{another_project.full_path}*iteration:#{iteration.iid}" }
let!(:result) { reference_filter("See #{reference}") }
it 'points to referenced project iteration page' do
expect(result.css('a').first.attr('href'))
.to eq(urls.project_iteration_url(another_project, iteration))
end
it 'link has valid text' do
doc = reference_filter("See (#{reference}.)")
expect(doc.css('a').first.text)
.to eq("#{iteration.reference_link_text} in #{another_project.full_path}")
end
it 'has valid text' do
doc = reference_filter("See (#{reference}.)")
expect(doc.text)
.to eq("See (#{iteration.reference_link_text} in #{another_project.full_path}.)")
end
it 'escapes the name attribute' do
allow_next_instance_of(Iteration) do |instance|
allow(instance).to receive(:title).and_return(%{"></a>whatever<a title="})
end
doc = reference_filter("See #{reference}")
expect(doc.css('a').first.text)
.to eq "#{iteration.reference_link_text} in #{another_project.full_path}"
end
end
shared_examples 'cross project shorthand reference' do
let(:namespace) { create(:namespace) }
let(:project) { create(:project, :public, namespace: namespace) }
let(:another_project) { create(:project, :public, namespace: namespace) }
let(:iteration) { create(:iteration, project: another_project) }
let(:reference) { "#{another_project.path}*iteration:#{iteration.iid}" }
let!(:result) { reference_filter("See #{reference}") }
it 'points to referenced project iteration page' do
expect(result.css('a').first.attr('href')).to eq urls
.project_iteration_url(another_project, iteration)
end
it 'link has valid text' do
doc = reference_filter("See (#{reference}.)")
expect(doc.css('a').first.text)
.to eq("#{iteration.reference_link_text} in #{another_project.path}")
end
it 'has valid text' do
doc = reference_filter("See (#{reference}.)")
expect(doc.text)
.to eq("See (#{iteration.reference_link_text} in #{another_project.path}.)")
end
it 'escapes the name attribute' do
allow_next_instance_of(Iteration) do |instance|
allow(instance).to receive(:title).and_return(%{"></a>whatever<a title="})
end
doc = reference_filter("See #{reference}")
expect(doc.css('a').first.text)
.to eq "#{iteration.reference_link_text} in #{another_project.path}"
end
end
shared_examples 'references with HTML entities' do
before do
iteration.update!(title: '&lt;html&gt;')
......@@ -301,11 +196,16 @@ RSpec.describe Banzai::Filter::References::IterationReferenceFilter do
it_behaves_like 'String-based multi-word references in quotes'
it_behaves_like 'referencing a iteration in a link href'
it_behaves_like 'references with HTML entities'
it_behaves_like 'HTML text with references' do
let(:resource) { iteration }
let(:resource_text) { resource.title }
end
it_behaves_like 'Integer-based references' do
let(:reference) { iteration.to_reference(format: :id) }
end
it 'does not support references by IID' do
doc = reference_filter("See #{Iteration.reference_prefix}#{iteration.iid}")
......@@ -335,7 +235,7 @@ RSpec.describe Banzai::Filter::References::IterationReferenceFilter do
end
it 'supports parent group references' do
# we have to update iterations_cadence group first in order to avoid invallid record
# we have to update iterations_cadence group first in order to avoid an invalid record
iteration.iterations_cadence.update_column(:group_id, parent_group.id)
iteration.update_column(:group_id, parent_group.id)
......@@ -399,4 +299,55 @@ RSpec.describe Banzai::Filter::References::IterationReferenceFilter do
include_context 'group iterations'
end
end
context 'checking N+1' do
let_it_be(:group) { create(:group) }
let_it_be(:group2) { create(:group, parent: group) }
let_it_be(:iteration) { create(:iteration, group: group) }
let_it_be(:iteration_reference) { iteration.to_reference(format: :name) }
let_it_be(:iteration2) { create(:iteration, group: group) }
let_it_be(:iteration2_reference) { iteration2.to_reference(format: :id) }
let_it_be(:iteration3) { create(:iteration, group: group2) }
let_it_be(:iteration3_reference) { iteration3.to_reference(format: :name) }
it 'does not have N+1 per multiple references per group', :use_sql_query_cache, :aggregate_failures do
max_count = 3
markdown = "#{iteration_reference}"
# warm the cache
reference_filter(markdown)
expect do
reference_filter(markdown)
end.not_to exceed_all_query_limit(max_count)
markdown = "#{iteration_reference} *iteration:\"Not Found\" *iteration:\"Not Found2\" #{iteration2_reference}"
expect do
reference_filter(markdown)
end.not_to exceed_all_query_limit(max_count)
end
it 'has N+1 for multiple unique group references', :use_sql_query_cache do
markdown = "#{iteration_reference}"
max_count = 3
# warm the cache
reference_filter(markdown, { project: nil, group: group2 })
expect do
reference_filter(markdown, { project: nil, group: group2 })
end.not_to exceed_all_query_limit(max_count)
# Since we're not batching iteration queries across groups,
# queries increase when a new group is referenced.
# TODO: https://gitlab.com/gitlab-org/gitlab/-/issues/330359
markdown = "#{iteration_reference} #{iteration2_reference} #{iteration3_reference}"
max_count += 1
expect do
reference_filter(markdown, { project: nil, group: group2 })
end.not_to exceed_all_query_limit(max_count)
end
end
end
lib/banzai/filter/references/label_reference_filter.rb
View file @ 81109da9
......@@ -23,7 +23,8 @@ module Banzai
label_relation = labels.where(title: label_names)
end
return Label.none if (relation = [id_relation, label_relation].compact).empty?
relation = [id_relation, label_relation].compact
return Label.none if relation.all?(Label.none)
Label.from_union(relation)
end
......
lib/banzai/filter/references/milestone_reference_filter.rb
View file @ 81109da9
......@@ -23,7 +23,8 @@ module Banzai
milestone_relation = find_milestones(parent, false).where(name: milestone_names)
end
return Milestone.none if (relation = [iid_relation, milestone_relation].compact).empty?
relation = [iid_relation, milestone_relation].compact
return Milestone.none if relation.all?(Milestone.none)
Milestone.from_union(relation).includes(:project, :group)
end
......@@ -116,11 +117,11 @@ module Banzai
# We don't support IID lookups because IIDs can clash between
# group/project milestones and group/subgroup milestones.
params[:group_ids] = self_and_ancestors_ids(parent) unless find_by_iid
params[:group_ids] = group_and_ancestors_ids(parent) unless find_by_iid
end
end
def self_and_ancestors_ids(parent)
def group_and_ancestors_ids(parent)
if group_context?(parent)
parent.self_and_ancestors.select(:id)
elsif project_context?(parent)
......
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
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7