Merge branch 'bwill/add-sorting-by-severity' into 'master'

Add sorting by `highest_severity` to dependency list aggregations

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/154054



Merged-by: Dzmitry Meshcharakou <12459192-dmeshcharakou@users.noreply.gitlab.com>
Approved-by: Ian Baum <ibaum@gitlab.com>
Approved-by: Dylan Griffith <dyl.griffith@gmail.com>
Approved-by: Dzmitry Meshcharakou <12459192-dmeshcharakou@users.noreply.gitlab.com>
Reviewed-by: Mehmet Emin INAC <minac@gitlab.com>
Reviewed-by: Brian Williams <bwilliams@gitlab.com>
Reviewed-by: Dzmitry Meshcharakou <12459192-dmeshcharakou@users.noreply.gitlab.com>
Co-authored-by: Brian Williams <bwilliams@gitlab.com>

db/post_migrate/20240528184057_index_unarchived_sbom_occurrences_for_aggregations_severity.rb

+# frozen_string_literal: true
+
+class IndexUnarchivedSbomOccurrencesForAggregationsSeverity < Gitlab::Database::Migration[2.2]
+  INDEX_NAME = 'index_unarchived_occurrences_for_aggregations_severity'
+
+  milestone '17.1'
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :sbom_occurrences, [:traversal_ids, :highest_severity, :component_id, :component_version_id],
+      where: 'archived = false',
+      name: INDEX_NAME
+  end
+
+  def down
+    remove_concurrent_index_by_name :sbom_occurrences, INDEX_NAME
+  end
+end

db/schema_migrations/20240528184057

+6dd7b4fdb49b3d4f0466eb467dfac20eceffeac0599dd5118fb54384ab992e95
\ No newline at end of file

db/structure.sql

@@ -28073,6 +28073,8 @@ CREATE INDEX index_topics_total_projects_count ON topics USING btree (total_proj
 CREATE UNIQUE INDEX index_trending_projects_on_project_id ON trending_projects USING btree (project_id);
+CREATE INDEX index_unarchived_occurrences_for_aggregations_severity ON sbom_occurrences USING btree (traversal_ids, highest_severity, component_id, component_version_id) WHERE (archived = false);
+
 CREATE INDEX index_unarchived_occurrences_on_version_id_and_traversal_ids ON sbom_occurrences USING btree (component_version_id, traversal_ids) WHERE (archived = false);
 CREATE INDEX index_unarchived_sbom_occurrences_for_aggregations ON sbom_occurrences USING btree (traversal_ids, component_id, component_version_id) WHERE (archived = false);

ee/app/controllers/groups/dependencies_controller.rb

@@ -87,20 +87,24 @@ def dependencies
     end
 
     def dependencies_finder_params
-      if below_group_limit?
-        params.permit(
-          :page,
-          :per_page,
-          :sort,
-          :sort_by,
-          component_names: [],
-          licenses: [],
-          package_managers: [],
-          project_ids: []
-        )
-      else
-        params.permit(:page, :per_page, :sort, :sort_by)
-      end
+      finder_params = if below_group_limit?
+                        params.permit(
+                          :page,
+                          :per_page,
+                          :sort,
+                          :sort_by,
+                          component_names: [],
+                          licenses: [],
+                          package_managers: [],
+                          project_ids: []
+                        )
+                      else
+                        params.permit(:page, :per_page, :sort, :sort_by)
+                      end
+
+      finder_params[:sort_by] = map_sort_by(finder_params[:sort_by]) if using_new_query?
+
+      finder_params
     end
 
     def dependencies_serializer
@@ -128,6 +132,15 @@ def render_error(status, message)
       end
     end
 
+    def map_sort_by(sort_by)
+      case sort_by
+      when 'severity'
+        :highest_severity
+      else
+        sort_by&.to_sym
+      end
+    end
+
     def set_below_group_limit
       @below_group_limit = below_group_limit?
     end

ee/app/finders/dependency_management/aggregations_finder.rb

@@ -5,7 +5,8 @@ module DependencyManagement
   class AggregationsFinder
     DEFAULT_PAGE_SIZE = 20
     MAX_PAGE_SIZE = 20
-    DISTINCT_BY = %i[component_id component_version_id].freeze
+    DEFAULT_SORT_COLUMNS = %i[component_id component_version_id].freeze
+    SUPPORTED_SORT_COLUMNS = %i[highest_severity].freeze
 
     def initialize(namespace, params: {})
       @namespace = namespace
@@ -13,6 +14,12 @@ def initialize(namespace, params: {})
     end
 
     def execute
+      group = distinct_columns.map { |column| "outer_occurrences.#{column}" }
+
+      order = orderings.map do |column, direction|
+        "MIN(outer_occurrences.#{column}) #{direction.to_s.upcase}"
+      end
+
       Sbom::Occurrence.with(namespaces_cte.to_arel)
         .select(
           'MIN(outer_occurrences.id)::bigint AS id',
@@ -26,14 +33,18 @@ def execute
           'SUM(counts.project_count)::integer AS project_count'
         )
         .from("(#{outer_occurrences.to_sql}) outer_occurrences, LATERAL (#{counts.to_sql}) counts")
-        .group('outer_occurrences.component_id', 'outer_occurrences.component_version_id')
-        .order('MIN(outer_occurrences.component_id) ASC', 'MIN(outer_occurrences.component_version_id) ASC')
+        .group(*group)
+        .order(*order)
     end
 
     private
 
     attr_reader :namespace, :params
 
+    def distinct_columns
+      orderings.keys
+    end
+
     def namespaces_cte
       ::Gitlab::SQL::CTE.new(:namespaces, namespace.self_and_descendants.select(:traversal_ids))
     end
@@ -41,15 +52,19 @@ def namespaces_cte
     def inner_occurrences
       Sbom::Occurrence.where('sbom_occurrences.traversal_ids = namespaces.traversal_ids::bigint[]')
         .unarchived
-        .order(*DISTINCT_BY)
+        .order(**orderings)
         .limit(page_size)
-        .select_distinct(on: DISTINCT_BY)
+        .select_distinct(on: distinct_columns)
     end
 
     def outer_occurrences
-      Sbom::Occurrence.select_distinct(on: DISTINCT_BY, table_name: '"inner_occurrences"')
+      order = orderings.map do |column, direction|
+        "inner_occurrences.#{column} #{direction.to_s.upcase}"
+      end
+
+      Sbom::Occurrence.select_distinct(on: distinct_columns, table_name: '"inner_occurrences"')
       .from("namespaces, LATERAL (#{inner_occurrences.to_sql}) inner_occurrences")
-      .order('inner_occurrences.component_id ASC', 'inner_occurrences.component_version_id ASC')
+      .order(*order)
       .limit(page_size)
     end
 
@@ -65,6 +80,28 @@ def counts
     def page_size
       [params.fetch(:per_page, DEFAULT_PAGE_SIZE), MAX_PAGE_SIZE].min
     end
+
+    def orderings
+      default_orderings = DEFAULT_SORT_COLUMNS.index_with { sort_direction }
+
+      return default_orderings unless sort_by.present?
+
+      # The `sort_by` column must come first in the `ORDER BY` statement.
+      # Create a new hash to ensure that it is in the front when enumerating.
+      Hash[sort_by => sort_direction, **default_orderings]
+    end
+
+    def sort_by
+      sort_by = params[:sort_by]&.to_sym
+
+      return unless sort_by && SUPPORTED_SORT_COLUMNS.include?(sort_by)
+
+      sort_by
+    end
+
+    def sort_direction
+      params[:sort]&.downcase&.to_sym == :desc ? :desc : :asc
+    end
   end
   # rubocop:enable CodeReuse/ActiveRecord
 end

ee/spec/finders/dependency_management/aggregations_finder_spec.rb

@@ -66,5 +66,35 @@
         end
       end
     end
+
+    context 'when sorting by highest_severity' do
+      let_it_be(:project) { target_projects.first }
+      let_it_be(:low) { create(:sbom_occurrence, highest_severity: 'low', project: project) }
+      let_it_be(:medium) { create(:sbom_occurrence, highest_severity: 'medium', project: project) }
+      let_it_be(:high) { create(:sbom_occurrence, highest_severity: 'high', project: project) }
+      let_it_be(:critical) { create(:sbom_occurrence, highest_severity: 'critical', project: project) }
+
+      let(:params) { { sort_by: 'highest_severity', sort: direction } }
+
+      before_all do
+        Sbom::Occurrence.id_in(target_occurrences.map(&:id)).delete_all
+      end
+
+      context 'in ascending order' do
+        let(:direction) { :asc }
+
+        it 'returns occurrences in ascending order of severity' do
+          expect(execute.to_a).to eq([low, medium, high, critical])
+        end
+      end
+
+      context 'in descending order' do
+        let(:direction) { :desc }
+
+        it 'returns occurrences in descending order of severity' do
+          expect(execute.to_a).to eq([critical, high, medium, low])
+        end
+      end
+    end
   end
 end

ee/spec/requests/groups/dependencies_controller_spec.rb

@@ -128,8 +128,17 @@
 
           context 'with existing dependencies' do
             let_it_be(:project) { create(:project, group: group) }
-            let_it_be(:sbom_occurrence_npm) { create(:sbom_occurrence, :mit, :npm, project: project) }
-            let_it_be(:sbom_occurrence_bundler) { create(:sbom_occurrence, :apache_2, :bundler, project: project) }
+            let_it_be(:sbom_occurrence_npm) do
+              component = create(:sbom_component, name: 'a')
+              create(:sbom_occurrence, :mit, :npm, highest_severity: 'low', component: component, project: project)
+            end
+
+            let_it_be(:sbom_occurrence_bundler) do
+              component = create(:sbom_component, name: 'b')
+              create(:sbom_occurrence, :apache_2, :bundler, highest_severity: 'high', component: component,
+                project: project)
+            end
+
             let_it_be(:archived_occurrence) do
               create(:sbom_occurrence, project: create(:project, :archived, group: group))
             end
@@ -200,6 +209,28 @@
               expect(recording).not_to exceed_all_query_limit(1).for_model(::Sbom::Source)
             end
 
+            context 'when sorted by severity in ascending order' do
+              let(:params) { { group_id: group.to_param, sort_by: 'severity', sort: 'asc' } }
+
+              it 'returns sorted list' do
+                subject
+
+                expect(json_response['dependencies'].first['name']).to eq(sbom_occurrence_npm.name)
+                expect(json_response['dependencies'].last['name']).to eq(sbom_occurrence_bundler.name)
+              end
+            end
+
+            context 'when sorted by severity in descending order' do
+              let(:params) { { group_id: group.to_param, sort_by: 'severity', sort: 'desc' } }
+
+              it 'returns sorted list' do
+                subject
+
+                expect(json_response['dependencies'].first['name']).to eq(sbom_occurrence_bundler.name)
+                expect(json_response['dependencies'].last['name']).to eq(sbom_occurrence_npm.name)
+              end
+            end
+
             context 'when using old query' do
               let(:using_new_query) { false }