Merge branch '230383-add-sort-to-graphql-vulnerabilities' into 'master'

Add sorting to GraphQL Vulnerabilities API

See merge request gitlab-org/gitlab!40856

doc/api/graphql/reference/gitlab_schema.graphql

@@ -6927,6 +6927,11 @@ type Group {
     """
     severity: [VulnerabilitySeverity!]
 
+    """
+    List vulnerabilities by sort order
+    """
+    sort: VulnerabilitySort = severity_desc
+
     """
     Filter vulnerabilities by state
     """
@@ -12689,6 +12694,11 @@ type Project {
     """
     severity: [VulnerabilitySeverity!]
 
+    """
+    List vulnerabilities by sort order
+    """
+    sort: VulnerabilitySort = severity_desc
+
     """
     Filter vulnerabilities by state
     """
@@ -13451,6 +13461,11 @@ type Query {
     """
     severity: [VulnerabilitySeverity!]
 
+    """
+    List vulnerabilities by sort order
+    """
+    sort: VulnerabilitySort = severity_desc
+
     """
     Filter vulnerabilities by state
     """
@@ -18389,6 +18404,21 @@ enum VulnerabilitySeverity {
   UNKNOWN
 }
 
+"""
+Vulnerability sort values
+"""
+enum VulnerabilitySort {
+  """
+  Severity in ascending order
+  """
+  severity_asc
+
+  """
+  Severity in descending order
+  """
+  severity_desc
+}
+
 """
 The state of the vulnerability.
 """

doc/api/graphql/reference/gitlab_schema.json

@@ -19047,6 +19047,16 @@
                   },
                   "defaultValue": null
                 },
+                {
+                  "name": "sort",
+                  "description": "List vulnerabilities by sort order",
+                  "type": {
+                    "kind": "ENUM",
+                    "name": "VulnerabilitySort",
+                    "ofType": null
+                  },
+                  "defaultValue": "severity_desc"
+                },
                 {
                   "name": "after",
                   "description": "Returns the elements in the list that come after the specified cursor.",
@@ -37132,6 +37142,16 @@
                   },
                   "defaultValue": null
                 },
+                {
+                  "name": "sort",
+                  "description": "List vulnerabilities by sort order",
+                  "type": {
+                    "kind": "ENUM",
+                    "name": "VulnerabilitySort",
+                    "ofType": null
+                  },
+                  "defaultValue": "severity_desc"
+                },
                 {
                   "name": "after",
                   "description": "Returns the elements in the list that come after the specified cursor.",
@@ -39461,6 +39481,16 @@
                   },
                   "defaultValue": null
                 },
+                {
+                  "name": "sort",
+                  "description": "List vulnerabilities by sort order",
+                  "type": {
+                    "kind": "ENUM",
+                    "name": "VulnerabilitySort",
+                    "ofType": null
+                  },
+                  "defaultValue": "severity_desc"
+                },
                 {
                   "name": "after",
                   "description": "Returns the elements in the list that come after the specified cursor.",
@@ -54034,6 +54064,29 @@
           ],
           "possibleTypes": null
         },
+        {
+          "kind": "ENUM",
+          "name": "VulnerabilitySort",
+          "description": "Vulnerability sort values",
+          "fields": null,
+          "inputFields": null,
+          "interfaces": null,
+          "enumValues": [
+            {
+              "name": "severity_desc",
+              "description": "Severity in descending order",
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
+            {
+              "name": "severity_asc",
+              "description": "Severity in ascending order",
+              "isDeprecated": false,
+              "deprecationReason": null
+            }
+          ],
+          "possibleTypes": null
+        },
         {
           "kind": "ENUM",
           "name": "VulnerabilityState",

ee/app/finders/security/vulnerabilities_finder.rb

@@ -6,19 +6,20 @@
 #
 # Arguments:
 #   vulnerable: any object that has a #vulnerabilities method that returns a collection of `Vulnerability`s
-#   filters: optional! a hash with one or more of the following:
+#   params: optional! a hash with one or more of the following:
 #     project_ids: if `vulnerable` includes multiple projects (like a Group), this filter will restrict
 #                   the vulnerabilities returned to those in the group's projects that also match these IDs
 #     report_types: only return vulnerabilities from these report types
 #     severities: only return vulnerabilities with these severities
 #     states: only return vulnerabilities in these states
+#     sort: return vulnerabilities ordered by severity_asc or severity_desc
 
 module Security
   class VulnerabilitiesFinder
     include FinderMethods
 
-    def initialize(vulnerable, filters = {})
-      @filters = filters
+    def initialize(vulnerable, params = {})
+      @params = params
       @vulnerabilities = vulnerable.vulnerabilities
     end
 
@@ -29,41 +30,45 @@ module Security
       filter_by_states
       filter_by_scanners
 
-      vulnerabilities
+      sort(vulnerabilities)
     end
 
     private
 
-    attr_reader :filters, :vulnerabilities
+    attr_reader :params, :vulnerabilities
 
     def filter_by_projects
-      if filters[:project_id].present?
-        @vulnerabilities = vulnerabilities.for_projects(filters[:project_id])
+      if params[:project_id].present?
+        @vulnerabilities = vulnerabilities.for_projects(params[:project_id])
       end
     end
 
     def filter_by_report_types
-      if filters[:report_type].present?
-        @vulnerabilities = vulnerabilities.with_report_types(filters[:report_type])
+      if params[:report_type].present?
+        @vulnerabilities = vulnerabilities.with_report_types(params[:report_type])
       end
     end
 
     def filter_by_severities
-      if filters[:severity].present?
-        @vulnerabilities = vulnerabilities.with_severities(filters[:severity])
+      if params[:severity].present?
+        @vulnerabilities = vulnerabilities.with_severities(params[:severity])
       end
     end
 
     def filter_by_states
-      if filters[:state].present?
-        @vulnerabilities = vulnerabilities.with_states(filters[:state])
+      if params[:state].present?
+        @vulnerabilities = vulnerabilities.with_states(params[:state])
       end
     end
 
     def filter_by_scanners
-      if filters[:scanner].present?
-        @vulnerabilities = vulnerabilities.with_scanners(filters[:scanner])
+      if params[:scanner].present?
+        @vulnerabilities = vulnerabilities.with_scanners(params[:scanner])
       end
     end
+
+    def sort(items)
+      items.order_by(params[:sort])
+    end
   end
 end

ee/app/graphql/resolvers/vulnerabilities_resolver.rb

@@ -26,19 +26,23 @@ module Resolvers
              required: false,
              description: 'Filter vulnerabilities by scanner'
 
+    argument :sort, Types::VulnerabilitySortEnum,
+             required: false,
+             default_value: 'severity_desc',
+             description: 'List vulnerabilities by sort order'
+
     def resolve(**args)
       return Vulnerability.none unless vulnerable
 
       vulnerabilities(args)
         .with_findings_scanner_and_identifiers
         .with_created_issue_links_and_issues
-        .ordered
     end
 
     private
 
-    def vulnerabilities(filters)
-      Security::VulnerabilitiesFinder.new(vulnerable, filters).execute
+    def vulnerabilities(params)
+      Security::VulnerabilitiesFinder.new(vulnerable, params).execute
     end
   end
 end

ee/app/graphql/types/vulnerability_sort_enum.rb

+# frozen_string_literal: true
+
+module Types
+  class VulnerabilitySortEnum < BaseEnum
+    graphql_name 'VulnerabilitySort'
+    description 'Vulnerability sort values'
+
+    value 'severity_desc', 'Severity in descending order'
+    value 'severity_asc', 'Severity in ascending order'
+  end
+end

ee/app/models/vulnerability.rb

@@ -59,8 +59,6 @@ class Vulnerability < ApplicationRecord
   validates :description, length: { maximum: Issuable::DESCRIPTION_LENGTH_MAX }, allow_blank: true
   validates :description_html, length: { maximum: Issuable::DESCRIPTION_HTML_LENGTH_MAX }, allow_blank: true
 
-  scope :ordered, -> { order(severity: :desc) }
-
   scope :with_findings, -> { includes(:findings) }
   scope :with_findings_and_scanner, -> { includes(findings: :scanner) }
   scope :with_findings_scanner_and_identifiers, -> { includes(findings: [:scanner, :identifiers, finding_identifiers: :identifier]) }
@@ -71,7 +69,10 @@ class Vulnerability < ApplicationRecord
   scope :with_severities, -> (severities) { where(severity: severities) }
   scope :with_states, -> (states) { where(state: states) }
   scope :with_scanners, -> (scanners) { joins(findings: :scanner).merge(Vulnerabilities::Scanner.with_external_id(scanners)) }
-  scope :grouped_by_severity, -> { group(:severity) }
+  scope :grouped_by_severity, -> { reorder(severity: :desc).group(:severity) }
+
+  scope :order_severity_asc, -> { reorder(severity: :asc, id: :desc) }
+  scope :order_severity_desc, -> { reorder(severity: :desc, id: :desc) }
 
   class << self
     def parent_class
@@ -117,6 +118,15 @@ class Vulnerability < ApplicationRecord
     def active_state_values
       states.values_at(*active_states)
     end
+
+    def order_by(method)
+      case method.to_s
+      when 'severity_desc' then order_severity_desc
+      when 'severity_asc' then order_severity_asc
+      else
+        order_severity_desc
+      end
+    end
   end
 
   # There will only be one finding associated with a vulnerability for the foreseeable future

ee/changelogs/unreleased/230383-add-sort-to-graphql-vulnerabilities.yml

+---
+title: Add ability to sort vulnerabilities by severity in GraphQL API
+merge_request: 40856
+author:
+type: added

ee/spec/finders/security/vulnerabilities_finder_spec.rb

@@ -10,11 +10,11 @@ RSpec.describe Security::VulnerabilitiesFinder do
   end
 
   let_it_be(:vulnerability2) do
-    create(:vulnerability, :with_findings, severity: :medium, report_type: :dast, state: :dismissed, project: project)
+    create(:vulnerability, :with_findings, severity: :high, report_type: :dependency_scanning, state: :confirmed, project: project)
   end
 
   let_it_be(:vulnerability3) do
-    create(:vulnerability, :with_findings, severity: :high, report_type: :dependency_scanning, state: :confirmed, project: project)
+    create(:vulnerability, :with_findings, severity: :medium, report_type: :dast, state: :dismissed, project: project)
   end
 
   let(:filters) { {} }
@@ -38,7 +38,7 @@ RSpec.describe Security::VulnerabilitiesFinder do
     let(:filters) { { report_type: %w[sast dast] } }
 
     it 'only returns vulnerabilities matching the given report types' do
-      is_expected.to contain_exactly(vulnerability1, vulnerability2)
+      is_expected.to contain_exactly(vulnerability1, vulnerability3)
     end
   end
 
@@ -46,7 +46,7 @@ RSpec.describe Security::VulnerabilitiesFinder do
     let(:filters) { { severity: %w[medium high] } }
 
     it 'only returns vulnerabilities matching the given severities' do
-      is_expected.to contain_exactly(vulnerability2, vulnerability3)
+      is_expected.to contain_exactly(vulnerability3, vulnerability2)
     end
   end
 
@@ -54,15 +54,15 @@ RSpec.describe Security::VulnerabilitiesFinder do
     let(:filters) { { state: %w[detected confirmed] } }
 
     it 'only returns vulnerabilities matching the given states' do
-      is_expected.to contain_exactly(vulnerability1, vulnerability3)
+      is_expected.to contain_exactly(vulnerability1, vulnerability2)
     end
   end
 
   context 'when filtered by scanner' do
-    let(:filters) { { scanner: [vulnerability1.finding_scanner_external_id, vulnerability3.finding_scanner_external_id] } }
+    let(:filters) { { scanner: [vulnerability1.finding_scanner_external_id, vulnerability2.finding_scanner_external_id] } }
 
     it 'only returns vulnerabilities matching the given scanners' do
-      is_expected.to contain_exactly(vulnerability1, vulnerability3)
+      is_expected.to contain_exactly(vulnerability1, vulnerability2)
     end
   end
 
@@ -82,6 +82,22 @@ RSpec.describe Security::VulnerabilitiesFinder do
     end
   end
 
+  context 'when sorted' do
+    let(:filters) { { sort: method } }
+
+    context 'ascending by severity' do
+      let(:method) { :severity_asc }
+
+      it { is_expected.to eq([vulnerability1, vulnerability3, vulnerability2]) }
+    end
+
+    context 'descending by severity' do
+      let(:method) { :severity_desc }
+
+      it { is_expected.to eq([vulnerability2, vulnerability3, vulnerability1]) }
+    end
+  end
+
   context 'when filtered by more than one property' do
     let_it_be(:vulnerability4) do
       create(:vulnerability, severity: :medium, report_type: :sast, state: :detected, project: project)

ee/spec/graphql/resolvers/vulnerabilities_resolver_spec.rb

@@ -6,7 +6,7 @@ RSpec.describe Resolvers::VulnerabilitiesResolver do
   include GraphqlHelpers
 
   describe '#resolve' do
-    subject { resolve(described_class, obj: vulnerable, args: filters, ctx: { current_user: current_user }) }
+    subject { resolve(described_class, obj: vulnerable, args: params, ctx: { current_user: current_user }) }
 
     let_it_be(:project) { create(:project) }
     let_it_be(:user) { create(:user, security_dashboard_projects: [project]) }
@@ -24,17 +24,37 @@ RSpec.describe Resolvers::VulnerabilitiesResolver do
     end
 
     let(:current_user) { user }
-    let(:filters) { {} }
+    let(:params) { {} }
     let(:vulnerable) { project }
 
-    it 'orders results by severity' do
-      expect(subject.first).to eq(critical_vulnerability)
-      expect(subject.second).to eq(high_vulnerability)
-      expect(subject.third).to eq(low_vulnerability)
+    context 'when given sort' do
+      context 'when sorting descending by severity' do
+        let(:params) { { sort: :severity_desc } }
+
+        it { is_expected.to eq([critical_vulnerability, high_vulnerability, low_vulnerability]) }
+      end
+
+      context 'when sorting ascending by severity' do
+        let(:params) { { sort: :severity_asc } }
+
+        it { is_expected.to eq([low_vulnerability, high_vulnerability, critical_vulnerability]) }
+      end
+
+      context 'when sorting param is not provided' do
+        let(:params) { {} }
+
+        it { is_expected.to eq([critical_vulnerability, high_vulnerability, low_vulnerability]) }
+      end
+
+      context 'when sorting by invalid param' do
+        let(:params) { { sort: :invalid } }
+
+        it { is_expected.to eq([critical_vulnerability, high_vulnerability, low_vulnerability]) }
+      end
     end
 
     context 'when given severities' do
-      let(:filters) { { severity: ['low'] } }
+      let(:params) { { severity: ['low'] } }
 
       it 'only returns vulnerabilities of the given severities' do
         is_expected.to contain_exactly(low_vulnerability)
@@ -42,7 +62,7 @@ RSpec.describe Resolvers::VulnerabilitiesResolver do
     end
 
     context 'when given states' do
-      let(:filters) { { state: ['dismissed'] } }
+      let(:params) { { state: ['dismissed'] } }
 
       it 'only returns vulnerabilities of the given states' do
         is_expected.to contain_exactly(high_vulnerability)
@@ -50,7 +70,7 @@ RSpec.describe Resolvers::VulnerabilitiesResolver do
     end
 
     context 'when given scanner' do
-      let(:filters) { { scanner: [high_vulnerability.finding_scanner_external_id] } }
+      let(:params) { { scanner: [high_vulnerability.finding_scanner_external_id] } }
 
       it 'only returns vulnerabilities of the given scanner' do
         is_expected.to contain_exactly(high_vulnerability)
@@ -58,7 +78,7 @@ RSpec.describe Resolvers::VulnerabilitiesResolver do
     end
 
     context 'when given report types' do
-      let(:filters) { { report_type: %i[dast sast] } }
+      let(:params) { { report_type: %i[dast sast] } }
 
       it 'only returns vulnerabilities of the given report types' do
         is_expected.to contain_exactly(critical_vulnerability, low_vulnerability)
@@ -70,7 +90,7 @@ RSpec.describe Resolvers::VulnerabilitiesResolver do
       let_it_be(:project2) { create(:project, namespace: group) }
       let_it_be(:project2_vulnerability) { create(:vulnerability, project: project2) }
 
-      let(:filters) { { project_id: [project2.id] } }
+      let(:params) { { project_id: [project2.id] } }
       let(:vulnerable) { group }
 
       before do
@@ -82,7 +102,7 @@ RSpec.describe Resolvers::VulnerabilitiesResolver do
       end
 
       context 'with multiple project IDs' do
-        let(:filters) { { project_id: [project.id, project2.id] } }
+        let(:params) { { project_id: [project.id, project2.id] } }
 
         it 'avoids N+1 queries' do
           control_count = ActiveRecord::QueryRecorder.new do

ee/spec/graphql/types/vulnerability_sort_enum_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe GitlabSchema.types['VulnerabilitySort'] do
+  it { expect(described_class.graphql_name).to eq('VulnerabilitySort') }
+
+  it 'exposes all the existing Vulnerability sort orders' do
+    expect(described_class.values.keys).to include(*%w[severity_desc severity_asc])
+  end
+end

ee/spec/models/vulnerability_spec.rb

@@ -156,8 +156,20 @@ RSpec.describe Vulnerability do
     end
   end
 
-  describe '.ordered' do
-    subject { described_class.ordered }
+  describe '.order_severity_asc' do
+    subject { described_class.order_severity_asc }
+
+    it 'returns vulnerabilities ordered by severity' do
+      low_vulnerability = create(:vulnerability, :low)
+      critical_vulnerability = create(:vulnerability, :critical)
+      medium_vulnerability = create(:vulnerability, :medium)
+
+      is_expected.to eq([low_vulnerability, medium_vulnerability, critical_vulnerability])
+    end
+  end
+
+  describe '.order_severity_desc' do
+    subject { described_class.order_severity_desc }
 
     it 'returns vulnerabilities ordered by severity' do
       low_vulnerability = create(:vulnerability, :low)
@@ -168,6 +180,26 @@ RSpec.describe Vulnerability do
     end
   end
 
+  describe '.order_by' do
+    subject { described_class.order_by(method) }
+
+    let!(:low_vulnerability) { create(:vulnerability, :low) }
+    let!(:critical_vulnerability) { create(:vulnerability, :critical) }
+    let!(:medium_vulnerability) { create(:vulnerability, :medium) }
+
+    context 'when ordered by severity_desc' do
+      let(:method) { :severity_desc }
+
+      it { is_expected.to eq([critical_vulnerability, medium_vulnerability, low_vulnerability]) }
+    end
+
+    context 'when ordered by severity_asc' do
+      let(:method) { :severity_asc }
+
+      it { is_expected.to eq([low_vulnerability, medium_vulnerability, critical_vulnerability]) }
+    end
+  end
+
   describe '.counts_by_day_and_severity' do
     context 'when the vulnerability_history feature flag is disabled' do
       before do

ee/spec/requests/api/vulnerabilities_spec.rb

@@ -38,7 +38,7 @@ RSpec.describe API::Vulnerabilities do
           get_vulnerabilities
 
           expect(response).to have_gitlab_http_status(:ok)
-          expect(json_response.map { |v| v['id'] }).to contain_exactly(project.vulnerabilities.second.id)
+          expect(json_response.map { |v| v['id'] }).to contain_exactly(project.vulnerabilities.order_severity_desc.second.id)
         end
       end