Merge branch 'bw-reference-iteration-caching' into 'master'

Enable iterations to use reference filter cache

See merge request gitlab-org/gitlab!67431

ee/app/finders/iterations_finder.rb

@@ -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

@@ -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

@@ -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

@@ -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

@@ -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

@@ -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)