Merge branch 'migrate-case-order-value-to-new-keyset-pagination' into 'master'

Update report sorting to generic keyset pagination

See merge request gitlab-org/gitlab!83402

ee/app/models/ee/vulnerability.rb

@@ -113,14 +113,20 @@ module EE
       scope :order_title_desc, -> { reorder(title: :desc, id: :desc) }
       scope :order_created_at_asc, -> { reorder(created_at: :asc, id: :desc) }
       scope :order_created_at_desc, -> { reorder(created_at: :desc, id: :desc) }
-      scope :order_report_type_asc, -> { select(*arel.projections, report_type_order.as('case_order_value')).reorder(report_type_order.asc, id: :desc) }
-      scope :order_report_type_desc, -> { select(*arel.projections, report_type_order.as('case_order_value')).reorder(report_type_order.desc, id: :desc) }
+      scope :order_report_type_asc, -> do
+        order = with_keyset_order(report_type_order, 'case_order_value', :asc, :desc)
+        order.apply_cursor_conditions(self).reorder(order)
+      end
+      scope :order_report_type_desc, -> do
+        order = with_keyset_order(report_type_order, 'case_order_value', :desc, :desc)
+        order.apply_cursor_conditions(self).reorder(order)
+      end
       scope :order_state_asc, -> do
-        order = with_state_order(:asc)
+        order = with_keyset_order(state_order, 'state_order', :asc)
         order.apply_cursor_conditions(self).reorder(order)
       end
       scope :order_state_desc, -> do
-        order = with_state_order(:desc)
+        order = with_keyset_order(state_order, 'state_order', :desc)
         order.apply_cursor_conditions(self).reorder(order)
       end
       scope :order_id_desc, -> { reorder(id: :desc) }
@@ -285,13 +291,14 @@ module EE
         end
       end
 
-      def with_state_order(direction)
+      def with_keyset_order(arel_function, name, direction, tie_breaker_direction = nil)
         raise "unknown sort direction given: #{direction}" unless %i[asc desc].include?(direction)
+        raise "unknown tie breaker sort direction given: #{tie_breaker_direction}" if tie_breaker_direction.present? && !%i[asc desc].include?(tie_breaker_direction)
 
         ::Gitlab::Pagination::Keyset::Order.build([
           ::Gitlab::Pagination::Keyset::ColumnOrderDefinition.new(
-            attribute_name: 'array_position',
-            order_expression: state_order.public_send(direction), # rubocop: disable GitlabSecurity/PublicSend
+            attribute_name: name,
+            order_expression: arel_function.public_send(direction), # rubocop: disable GitlabSecurity/PublicSend
             nullable: :not_nullable,
             order_direction: direction,
             distinct: false,
@@ -299,7 +306,7 @@ module EE
           ),
           ::Gitlab::Pagination::Keyset::ColumnOrderDefinition.new(
             attribute_name: 'id',
-            order_expression: arel_table[:id].public_send(direction), # rubocop: disable GitlabSecurity/PublicSend
+            order_expression: arel_table[:id].public_send(tie_breaker_direction || direction), # rubocop: disable GitlabSecurity/PublicSend
             nullable: :not_nullable,
             distinct: true
           )

ee/spec/lib/gitlab/pagination/keyset/simple_order_builder_spec.rb

@@ -4,7 +4,7 @@ require 'spec_helper'
 
 RSpec.describe Gitlab::Pagination::Keyset::SimpleOrderBuilder do
   context 'when ordering by a CASE expression and id' do
-    let(:scope) { Vulnerability.order_report_type_asc }
+    let(:scope) { Vulnerability.order(Vulnerability.report_type_order.asc) }
 
     subject(:result) { described_class.build(scope) }
 

ee/spec/models/ee/vulnerability_spec.rb

@@ -894,4 +894,20 @@ RSpec.describe Vulnerability do
       it { is_expected.to include(vulnerability) }
     end
   end
+
+  describe '.with_keyset_order' do
+    let(:unknown_sort_direction) { :abc }
+
+    it 'raises an error when an unknown sort direction given' do
+      expect do
+        described_class.with_keyset_order(Vulnerability.state_order, 'state_order', unknown_sort_direction)
+      end.to raise_error("unknown sort direction given: abc")
+    end
+
+    it 'raises an error when an unknown sort direction given for tie breaking column' do
+      expect do
+        described_class.with_keyset_order(Vulnerability.state_order, 'state_order', :asc, unknown_sort_direction)
+      end.to raise_error("unknown tie breaker sort direction given: abc")
+    end
+  end
 end

ee/spec/requests/api/graphql/vulnerabilities/sort_spec.rb

@@ -9,13 +9,6 @@ RSpec.describe 'Query.vulnerabilities.sort' do
   let_it_be(:current_user) { create(:user, security_dashboard_projects: [project]) }
   let_it_be(:data_path) { %w[vulnerabilities] }
 
-  let_it_be(:vulnerability_confirmed1) { create(:vulnerability, :confirmed, project: project) }
-  let_it_be(:vulnerability_confirmed2) { create(:vulnerability, :confirmed, project: project) }
-  let_it_be(:vulnerability_detected1) { create(:vulnerability, :detected, project: project) }
-  let_it_be(:vulnerability_dismissed1) { create(:vulnerability, :dismissed, project: project) }
-  let_it_be(:vulnerability_resolved1) { create(:vulnerability, :resolved, project: project) }
-  let_it_be(:vulnerability_detected2) { create(:vulnerability, :detected, project: project) }
-
   before do
     project.add_developer(current_user)
     stub_licensed_features(security_dashboard: true)
@@ -30,41 +23,93 @@ RSpec.describe 'Query.vulnerabilities.sort' do
   end
 
   [true, false].each do |flag_enabled|
-    context "when the new_graphql_keyset_pagination FF state #{flag_enabled}" do
-      before do
-        stub_feature_flags(new_graphql_keyset_pagination: flag_enabled)
-      end
+    context 'sort by state' do
+      let_it_be(:vulnerability_confirmed1) { create(:vulnerability, :confirmed, project: project) }
+      let_it_be(:vulnerability_confirmed2) { create(:vulnerability, :confirmed, project: project) }
+      let_it_be(:vulnerability_detected1) { create(:vulnerability, :detected, project: project) }
+      let_it_be(:vulnerability_dismissed1) { create(:vulnerability, :dismissed, project: project) }
+      let_it_be(:vulnerability_resolved1) { create(:vulnerability, :resolved, project: project) }
+      let_it_be(:vulnerability_detected2) { create(:vulnerability, :detected, project: project) }
+
+      context "when the new_graphql_keyset_pagination FF state #{flag_enabled}" do
+        before do
+          stub_feature_flags(new_graphql_keyset_pagination: flag_enabled)
+        end
+
+        context 'sort by STATE_ASC' do
+          it_behaves_like 'sorted paginated query' do
+            let(:sort_param) { :state_asc }
+            let(:first_param) { 2 }
+            let(:all_records) do
+              [
+                vulnerability_detected1,
+                vulnerability_detected2,
+                vulnerability_confirmed1,
+                vulnerability_confirmed2,
+                vulnerability_resolved1,
+                vulnerability_dismissed1
+              ].map(&:to_gid).map(&:to_s)
+            end
+          end
+        end
 
-      context 'sort by STATE_ASC' do
-        it_behaves_like 'sorted paginated query' do
-          let(:sort_param) { :state_asc }
-          let(:first_param) { 2 }
-          let(:all_records) do
-            [
-              vulnerability_detected1,
-              vulnerability_detected2,
-              vulnerability_confirmed1,
-              vulnerability_confirmed2,
-              vulnerability_resolved1,
-              vulnerability_dismissed1
-            ].map(&:to_gid).map(&:to_s)
+        context 'sort by STATE_ASC' do
+          it_behaves_like 'sorted paginated query' do
+            let(:sort_param) { :state_desc }
+            let(:first_param) { 2 }
+            let(:all_records) do
+              [
+                vulnerability_dismissed1,
+                vulnerability_resolved1,
+                vulnerability_confirmed2,
+                vulnerability_confirmed1,
+                vulnerability_detected2,
+                vulnerability_detected1
+              ].map(&:to_gid).map(&:to_s)
+            end
           end
         end
       end
+    end
+
+    context 'sort by report type' do
+      let_it_be(:vulnerability1) { create(:vulnerability, :secret_detection, project: project) }
+      let_it_be(:vulnerability2) { create(:vulnerability, :sast, project: project) }
+      let_it_be(:vulnerability3) { create(:vulnerability, :secret_detection, project: project) }
+      let_it_be(:vulnerability4) { create(:vulnerability, :sast, project: project) }
+
+      context "when the new_graphql_keyset_pagination FF state #{flag_enabled}" do
+        before do
+          stub_feature_flags(new_graphql_keyset_pagination: flag_enabled)
+        end
+
+        context 'sort by REPORT_TYPE_ASC' do
+          it_behaves_like 'sorted paginated query' do
+            let(:sort_param) { :report_type_asc }
+            let(:first_param) { 2 }
+            let(:all_records) do
+              [
+                vulnerability4,
+                vulnerability2,
+                vulnerability3,
+                vulnerability1
+              ].map(&:to_gid).map(&:to_s)
+            end
+          end
+        end
 
-      context 'sort by STATE_ASC' do
-        it_behaves_like 'sorted paginated query' do
-          let(:sort_param) { :state_desc }
-          let(:first_param) { 2 }
-          let(:all_records) do
-            [
-              vulnerability_dismissed1,
-              vulnerability_resolved1,
-              vulnerability_confirmed2,
-              vulnerability_confirmed1,
-              vulnerability_detected2,
-              vulnerability_detected1
-            ].map(&:to_gid).map(&:to_s)
+        context 'sort by STATE_ASC' do
+          it_behaves_like 'sorted paginated query' do
+            let(:sort_param) { :report_type_desc }
+            let(:first_param) { 2 }
+            let(:all_records) do
+              [
+                vulnerability3,
+                vulnerability1,
+                vulnerability4,
+                vulnerability2
+              ].map(&:to_gid).map(&:to_s)
+            end
           end
         end
       end