Add latest changes from gitlab-org/gitlab@master

app/models/packages/package_file.rb

@@ -32,6 +32,7 @@ class Packages::PackageFile < ApplicationRecord
   scope :with_file_name, ->(file_name) { where(file_name: file_name) }
   scope :with_file_name_like, ->(file_name) { where(arel_table[:file_name].matches(file_name)) }
   scope :with_files_stored_locally, -> { where(file_store: ::Packages::PackageFileUploader::Store::LOCAL) }
+  scope :with_format, ->(format) { where(::Packages::PackageFile.arel_table[:file_name].matches("%.#{format}")) }
   scope :preload_conan_file_metadata, -> { preload(:conan_file_metadatum) }
   scope :preload_debian_file_metadata, -> { preload(:debian_file_metadatum) }
   scope :preload_helm_file_metadata, -> { preload(:helm_file_metadatum) }

app/presenters/packages/nuget/presenter_helpers.rb

@@ -8,6 +8,7 @@ module Packages
       BLANK_STRING = ''
       PACKAGE_DEPENDENCY_GROUP = 'PackageDependencyGroup'
       PACKAGE_DEPENDENCY = 'PackageDependency'
+      NUGET_PACKAGE_FORMAT = 'nupkg'
 
       private
 
@@ -31,7 +32,7 @@ module Packages
             id: package.project_id,
             package_name: package.name,
             package_version: package.version,
-            package_filename: package.package_files.last&.file_name
+            package_filename: package.package_files.with_format(NUGET_PACKAGE_FORMAT).last&.file_name
           },
           true
         )

lib/banzai/filter/references/label_reference_filter.rb

@@ -11,13 +11,21 @@ module Banzai
         def parent_records(parent, ids)
           return Label.none unless parent.is_a?(Project) || parent.is_a?(Group)
 
-          labels         = find_labels(parent)
-          label_ids      = ids.map {|y| y[:label_id]}.compact
-          label_names    = ids.map {|y| y[:label_name]}.compact
-          id_relation    = labels.where(id: label_ids)
-          label_relation = labels.where(title: label_names)
+          labels = find_labels(parent)
+          label_ids = ids.map {|y| y[:label_id]}.compact
 
-          Label.from_union([id_relation, label_relation])
+          unless label_ids.empty?
+            id_relation = labels.where(id: label_ids)
+          end
+
+          label_names = ids.map {|y| y[:label_name]}.compact
+          unless label_names.empty?
+            label_relation = labels.where(title: label_names)
+          end
+
+          return Label.none if (relation = [id_relation, label_relation].compact).empty?
+
+          Label.from_union(relation)
         end
 
         def find_object(parent_object, id)

lib/banzai/filter/references/milestone_reference_filter.rb

@@ -10,19 +10,55 @@ module Banzai
         self.reference_type = :milestone
         self.object_class   = Milestone
 
-        # Links to project milestones contain the IID, but when we're handling
-        # 'regular' references, we need to use the global ID to disambiguate
-        # between group and project milestones.
-        def find_object(parent, id)
-          return unless valid_context?(parent)
+        def parent_records(parent, ids)
+          return Milestone.none unless valid_context?(parent)
 
-          find_milestone_with_finder(parent, id: id)
+          milestone_iids = ids.map {|y| y[:milestone_iid]}.compact
+          unless milestone_iids.empty?
+            iid_relation = find_milestones(parent, true).where(iid: milestone_iids)
+          end
+
+          milestone_names = ids.map {|y| y[:milestone_name]}.compact
+          unless milestone_names.empty?
+            milestone_relation = find_milestones(parent, false).where(name: milestone_names)
+          end
+
+          return Milestone.none if (relation = [iid_relation, milestone_relation].compact).empty?
+
+          Milestone.from_union(relation).includes(:project, :group)
         end
 
-        def find_object_from_link(parent, iid)
-          return unless valid_context?(parent)
+        def find_object(parent_object, id)
+          key = reference_cache.records_per_parent[parent_object].keys.find do |k|
+            k[:milestone_iid] == id[:milestone_iid] || k[:milestone_name] == id[:milestone_name]
+          end
 
-          find_milestone_with_finder(parent, iid: iid)
+          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 milestone_iid
+        # or the milestone_name, but not both.  But below, we have both pieces of information.
+        # But it's accounted for in `find_object`
+        def parse_symbol(symbol, match_data)
+          if symbol
+            # when parsing links, there is no `match_data[:milestone_iid]`, but `symbol`
+            # holds the iid
+            { milestone_iid: symbol.to_i, milestone_name: nil }
+          else
+            { milestone_iid: match_data[:milestone_iid]&.to_i, milestone_name: match_data[:milestone_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)
+          { milestone_iid: record.iid, milestone_name: record.name }
         end
 
         def valid_context?(parent)
@@ -50,12 +86,14 @@ module Banzai
           return super(text, pattern) if pattern != Milestone.reference_pattern
 
           milestones = {}
-          unescaped_html = unescape_html_entities(text).gsub(pattern) do |match|
-            milestone = find_milestone($~[:project], $~[:namespace], $~[:milestone_iid], $~[:milestone_name])
 
-            if milestone
-              milestones[milestone.id] = yield match, milestone.id, $~[:project], $~[:namespace], $~
-              "#{REFERENCE_PLACEHOLDER}#{milestone.id}"
+          unescaped_html = unescape_html_entities(text).gsub(pattern).with_index do |match, index|
+            ident = identifier($~)
+            milestone = yield match, ident, $~[:project], $~[:namespace], $~
+
+            if milestone != match
+              milestones[index] = milestone
+              "#{REFERENCE_PLACEHOLDER}#{index}"
             else
               match
             end
@@ -66,31 +104,10 @@ module Banzai
           escape_with_placeholders(unescaped_html, milestones)
         end
 
-        def find_milestone(project_ref, namespace_ref, milestone_id, milestone_name)
-          project_path = reference_cache.full_project_path(namespace_ref, project_ref)
+        def find_milestones(parent, find_by_iid = false)
+          finder_params = milestone_finder_params(parent, find_by_iid)
 
-          # Returns group if project is not found by path
-          parent = parent_from_ref(project_path)
-
-          return unless parent
-
-          milestone_params = milestone_params(milestone_id, milestone_name)
-
-          find_milestone_with_finder(parent, milestone_params)
-        end
-
-        def milestone_params(iid, name)
-          if name
-            { name: name.tr('"', '') }
-          else
-            { iid: iid.to_i }
-          end
-        end
-
-        def find_milestone_with_finder(parent, params)
-          finder_params = milestone_finder_params(parent, params[:iid].present?)
-
-          MilestonesFinder.new(finder_params).find_by(params)
+          MilestonesFinder.new(finder_params).execute
         end
 
         def milestone_finder_params(parent, find_by_iid)
@@ -131,6 +148,14 @@ module Banzai
         def object_link_title(object, matches)
           nil
         end
+
+        def parent
+          project || group
+        end
+
+        def requires_unescaping?
+          true
+        end
       end
     end
   end

lib/gitlab/ci/ansi2json/line.rb

@@ -76,8 +76,14 @@ module Gitlab
           @section_header = true
         end
 
-        def set_section_duration(duration)
-          @section_duration = Time.at(duration.to_i).utc.strftime('%M:%S')
+        def set_section_duration(duration_in_seconds)
+          duration = ActiveSupport::Duration.build(duration_in_seconds.to_i)
+          hours = duration.in_hours.floor
+          hours = hours > 0 ? "%02d" % hours : nil
+          minutes = "%02d" % duration.parts[:minutes].to_i
+          seconds = "%02d" % duration.parts[:seconds].to_i
+
+          @section_duration = [hours, minutes, seconds].compact.join(':')
         end
 
         def flush_current_segment!

spec/lib/banzai/filter/references/milestone_reference_filter_spec.rb

@@ -327,6 +327,7 @@ RSpec.describe Banzai::Filter::References::MilestoneReferenceFilter do
     it_behaves_like 'String-based single-word references'
     it_behaves_like 'String-based multi-word references in quotes'
     it_behaves_like 'referencing a milestone in a link href'
+    it_behaves_like 'linking to a milestone as the entire link'
     it_behaves_like 'cross-project / cross-namespace complete reference'
     it_behaves_like 'cross-project / same-namespace complete reference'
     it_behaves_like 'cross project shorthand reference'
@@ -460,4 +461,76 @@ RSpec.describe Banzai::Filter::References::MilestoneReferenceFilter do
       include_context 'group milestones'
     end
   end
+
+  context 'checking N+1' do
+    let_it_be(:group)              { create(:group) }
+    let_it_be(:group2)             { create(:group) }
+    let_it_be(:project)            { create(:project, :public, namespace: group) }
+    let_it_be(:project2)           { create(:project, :public, namespace: group2) }
+    let_it_be(:project3)           { create(:project, :public) }
+    let_it_be(:project_milestone)  { create(:milestone, project: project) }
+    let_it_be(:project_milestone2) { create(:milestone, project: project) }
+    let_it_be(:project2_milestone) { create(:milestone, project: project2) }
+    let_it_be(:group2_milestone)   { create(:milestone, group: group2) }
+    let_it_be(:project_reference)  { "#{project_milestone.to_reference}" }
+    let_it_be(:project_reference2) { "#{project_milestone2.to_reference}" }
+    let_it_be(:project2_reference) { "#{project2_milestone.to_reference(full: true)}" }
+    let_it_be(:group2_reference)   { "#{project2.full_path}%\"#{group2_milestone.name}\"" }
+
+    it 'does not have N+1 per multiple references per project', :use_sql_query_cache do
+      markdown = "#{project_reference}"
+      control_count = 4
+
+      expect do
+        reference_filter(markdown)
+      end.not_to exceed_all_query_limit(control_count)
+
+      markdown = "#{project_reference} %qwert %werty %ertyu %rtyui #{project_reference2}"
+
+      expect do
+        reference_filter(markdown)
+      end.not_to exceed_all_query_limit(control_count)
+    end
+
+    it 'has N+1 for multiple unique project/group references', :use_sql_query_cache do
+      markdown = "#{project_reference}"
+      control_count = 4
+
+      expect do
+        reference_filter(markdown, project: project)
+      end.not_to exceed_all_query_limit(control_count)
+
+      # Since we're not batching milestone queries across projects/groups,
+      # queries increase when a new project/group is added.
+      # TODO: https://gitlab.com/gitlab-org/gitlab/-/issues/330359
+      markdown = "#{project_reference} #{group2_reference}"
+      control_count += 5
+
+      expect do
+        reference_filter(markdown)
+      end.not_to exceed_all_query_limit(control_count)
+
+      # third reference to already queried project/namespace, nothing extra (no N+1 here)
+      markdown = "#{project_reference} #{group2_reference} #{project_reference2}"
+
+      expect do
+        reference_filter(markdown)
+      end.not_to exceed_all_query_limit(control_count)
+
+      # last reference needs additional queries
+      markdown = "#{project_reference} #{group2_reference} #{project2_reference} #{project3.full_path}%test_milestone"
+      control_count += 6
+
+      expect do
+        reference_filter(markdown)
+      end.not_to exceed_all_query_limit(control_count)
+
+      # Use an iid instead of title reference
+      markdown = "#{project_reference} #{group2_reference} #{project2.full_path}%#{project2_milestone.iid} #{project3.full_path}%test_milestone"
+
+      expect do
+        reference_filter(markdown)
+      end.not_to exceed_all_query_limit(control_count)
+    end
+  end
 end

spec/lib/gitlab/ci/ansi2json/line_spec.rb

@@ -76,30 +76,25 @@ RSpec.describe Gitlab::Ci::Ansi2json::Line do
   end
 
   describe '#set_section_duration' do
-    shared_examples 'set_section_duration' do
-      it 'sets and formats the section_duration' do
-        subject.set_section_duration(75)
+    using RSpec::Parameterized::TableSyntax
 
-        expect(subject.section_duration).to eq('01:15')
-      end
+    where(:duration, :result) do
+      nil                                         | '00:00'
+      'string'                                    | '00:00'
+      0.seconds                                   | '00:00'
+      7.seconds                                   | '00:07'
+      75                                          | '01:15'
+      1.minute + 15.seconds                       | '01:15'
+      13.hours + 14.minutes + 15.seconds          | '13:14:15'
+      1.day + 13.hours + 14.minutes + 15.seconds  | '37:14:15'
     end
 
-    context 'with default timezone' do
-      it_behaves_like 'set_section_duration'
-    end
+    with_them do
+      it do
+        subject.set_section_duration(duration)
 
-    context 'with a timezone carrying minutes offset' do
-      before do
-        # The actual call by does use Time.at(...).utc that the following
-        # rubocop rule (Rails/TimeZone) suggests, but for this specific
-        # test's purposes we needed to mock at the Time.at call point.
-
-        # rubocop:disable Rails/TimeZone
-        allow(Time).to receive(:at).with(75).and_return(Time.at(75, in: '+05:30'))
-        # rubocop:enable Rails/TimeZone
+        expect(subject.section_duration).to eq(result)
       end
-
-      it_behaves_like 'set_section_duration'
     end
   end
 

spec/models/packages/package_file_spec.rb

@@ -5,6 +5,7 @@ RSpec.describe Packages::PackageFile, type: :model do
   let_it_be(:project) { create(:project) }
   let_it_be(:package_file1) { create(:package_file, :xml, file_name: 'FooBar') }
   let_it_be(:package_file2) { create(:package_file, :xml, file_name: 'ThisIsATest') }
+  let_it_be(:package_file3) { create(:package_file, :xml, file_name: 'formatted.zip') }
   let_it_be(:debian_package) { create(:debian_package, project: project) }
 
   describe 'relationships' do
@@ -36,6 +37,12 @@ RSpec.describe Packages::PackageFile, type: :model do
 
       it { is_expected.to match_array([package_file1]) }
     end
+
+    describe '.with_format' do
+      subject { described_class.with_format('zip') }
+
+      it { is_expected.to contain_exactly(package_file3) }
+    end
   end
 
   context 'updating project statistics' do

spec/presenters/packages/nuget/package_metadata_presenter_spec.rb

@@ -5,7 +5,7 @@ require 'spec_helper'
 RSpec.describe Packages::Nuget::PackageMetadataPresenter do
   include_context 'with expected presenters dependency groups'
 
-  let_it_be(:package) { create(:nuget_package, :with_metadatum) }
+  let_it_be(:package) { create(:nuget_package, :with_symbol_package, :with_metadatum) }
   let_it_be(:tag1) { create(:packages_tag, name: 'tag1', package: package) }
   let_it_be(:tag2) { create(:packages_tag, name: 'tag2', package: package) }
   let_it_be(:presenter) { described_class.new(package) }
@@ -19,7 +19,7 @@ RSpec.describe Packages::Nuget::PackageMetadataPresenter do
   end
 
   describe '#archive_url' do
-    let_it_be(:expected_suffix) { "/api/v4/projects/#{package.project_id}/packages/nuget/download/#{package.name}/#{package.version}/#{package.package_files.last.file_name}" }
+    let_it_be(:expected_suffix) { "/api/v4/projects/#{package.project_id}/packages/nuget/download/#{package.name}/#{package.version}/#{package.package_files.with_format('nupkg').last.file_name}" }
 
     subject { presenter.archive_url }