Merge branch...

Merge branch '356408-bug-compliance-violations-pagination-doesn-t-show-expected-results' into 'master'

Fix compliance violations pagination so it paginates all sort options properly

See merge request gitlab-org/gitlab!83303

ee/app/graphql/resolvers/compliance_management/merge_requests/compliance_violation_resolver.rb

@@ -24,8 +24,24 @@ module Resolvers
                  default_value: 'SEVERITY_LEVEL_DESC',
                  description: 'List compliance violations by sort order.'
 
+        NON_STABLE_CURSOR_SORTS = %w[MERGE_REQUEST_TITLE_ASC MERGE_REQUEST_TITLE_DESC MERGED_AT_ASC MERGED_AT_DESC].freeze
+
         def resolve(filters: {}, sort: 'SEVERITY_LEVEL_DESC')
-          ::ComplianceManagement::MergeRequests::ComplianceViolationsFinder.new(current_user: current_user, group: group, params: filters.to_h.merge(sort: sort)).execute
+          violations = ::ComplianceManagement::MergeRequests::ComplianceViolationsFinder.new(current_user: current_user, group: group, params: filters.to_h.merge(sort: sort)).execute
+
+          if non_stable_cursor_sort?(sort)
+            # Certain complex sorts are not supported by the stable cursor pagination yet.
+            # In these cases, we use offset pagination, so we return the correct connection.
+            offset_pagination(violations)
+          else
+            violations
+          end
+        end
+
+        private
+
+        def non_stable_cursor_sort?(sort)
+          NON_STABLE_CURSOR_SORTS.include?(sort)
         end
       end
     end

ee/spec/graphql/resolvers/compliance_management/merge_requests/compliance_violation_resolver_spec.rb

@@ -73,16 +73,16 @@ RSpec.describe Resolvers::ComplianceManagement::MergeRequests::ComplianceViolati
       end
 
       context 'sorting the results' do
-        where(:direction, :result) do
-          'SEVERITY_LEVEL_ASC' | lazy { [compliance_violation, compliance_violation2] }
-          'SEVERITY_LEVEL_DESC' | lazy { [compliance_violation2, compliance_violation] }
-          'VIOLATION_REASON_ASC' | lazy { [compliance_violation, compliance_violation2] }
-          'VIOLATION_REASON_DESC' | lazy { [compliance_violation2, compliance_violation] }
-          'MERGE_REQUEST_TITLE_ASC' | lazy { [compliance_violation, compliance_violation2] }
-          'MERGE_REQUEST_TITLE_DESC' | lazy { [compliance_violation2, compliance_violation] }
-          'MERGED_AT_ASC' | lazy { [compliance_violation, compliance_violation2] }
-          'MERGED_AT_DESC' | lazy { [compliance_violation2, compliance_violation] }
-          'UNKNOWN_SORT' | lazy { [compliance_violation, compliance_violation2] }
+        where(:direction, :pagination_type, :result) do
+          'SEVERITY_LEVEL_ASC' | :keyset | lazy { [compliance_violation, compliance_violation2] }
+          'SEVERITY_LEVEL_DESC' | :keyset | lazy { [compliance_violation2, compliance_violation] }
+          'VIOLATION_REASON_ASC' | :keyset | lazy { [compliance_violation, compliance_violation2] }
+          'VIOLATION_REASON_DESC' | :keyset | lazy { [compliance_violation2, compliance_violation] }
+          'MERGE_REQUEST_TITLE_ASC' | :offset | lazy { [compliance_violation, compliance_violation2] }
+          'MERGE_REQUEST_TITLE_DESC' | :offset | lazy { [compliance_violation2, compliance_violation] }
+          'MERGED_AT_ASC' | :offset |  lazy { [compliance_violation, compliance_violation2] }
+          'MERGED_AT_DESC' | :offset | lazy { [compliance_violation2, compliance_violation] }
+          'UNKNOWN_SORT' | :keyset | lazy { [compliance_violation, compliance_violation2] }
         end
 
         with_them do
@@ -91,6 +91,16 @@ RSpec.describe Resolvers::ComplianceManagement::MergeRequests::ComplianceViolati
           it 'finds the filtered compliance violations' do
             expect(subject).to match_array(result)
           end
+
+          if params[:pagination_type] == :keyset
+            it "uses keyset pagination" do
+              expect(subject).to be_a(::Gitlab::Graphql::Pagination::Keyset::Connection)
+            end
+          else
+            it "uses offset pagination" do
+              expect(subject).to be_a(::Gitlab::Graphql::Pagination::OffsetActiveRecordRelationConnection)
+            end
+          end
         end
       end
     end

ee/spec/requests/api/graphql/compliance_management/merge_requests/compliance_violations_spec.rb

@@ -21,17 +21,17 @@ RSpec.describe 'getting the compliance violations for a group' do
 
   let(:fields) do
     <<~GRAPHQL
-        nodes {
+      nodes {
+        id
+        severityLevel
+        reason
+        violatingUser {
           id
-          severityLevel
-          reason
-          violatingUser {
-            id
-          }
-          mergeRequest {
-            id
-          }
         }
+        mergeRequest {
+          id
+        }
+      }
     GRAPHQL
   end
 
@@ -117,23 +117,36 @@ RSpec.describe 'getting the compliance violations for a group' do
       end
     end
 
-    context 'sorting the results' do
-      where(:direction, :result) do
-        :SEVERITY_LEVEL_ASC | lazy { [violation_output, violation2_output] }
-        :SEVERITY_LEVEL_DESC | lazy { [violation2_output, violation_output] }
-        :VIOLATION_REASON_ASC | lazy { [violation_output, violation2_output] }
-        :VIOLATION_REASON_DESC | lazy { [violation2_output, violation_output] }
-        :MERGE_REQUEST_TITLE_ASC | lazy { [violation_output, violation2_output] }
-        :MERGE_REQUEST_TITLE_DESC | lazy { [violation2_output, violation_output] }
-        :MERGED_AT_ASC | lazy { [violation_output, violation2_output] }
-        :MERGED_AT_DESC | lazy { [violation2_output, violation_output] }
+    describe 'sorting and pagination' do
+      let_it_be(:data_path) { [:group, :merge_request_violations] }
+
+      let(:violation_ids_asc) { [violation_output['id'].to_i, violation2_output['id'].to_i] }
+      let(:violation_ids_desc) { [violation2_output['id'].to_i, violation_output['id'].to_i] }
+
+      def pagination_query(params)
+        graphql_query_for(
+          :group, { full_path: group.full_path }, query_nodes(:merge_request_violations, :id, include_pagination_info: true, args: params)
+        )
       end
 
-      with_them do
-        it 'finds all the compliance violations' do
-          post_graphql(query({ sort: direction }), current_user: current_user)
+      def pagination_results_data(data)
+        data.map { |merge_request_violations| merge_request_violations['id'].to_i }
+      end
 
-          expect(compliance_violations).to match_array(result)
+      where(:sort_param, :all_records) do
+        :SEVERITY_LEVEL_ASC       | ref(:violation_ids_asc)
+        :SEVERITY_LEVEL_DESC      | ref(:violation_ids_desc)
+        :VIOLATION_REASON_ASC     | ref(:violation_ids_asc)
+        :VIOLATION_REASON_DESC    | ref(:violation_ids_desc)
+        :MERGE_REQUEST_TITLE_ASC  | ref(:violation_ids_asc)
+        :MERGE_REQUEST_TITLE_DESC | ref(:violation_ids_desc)
+        :MERGED_AT_ASC            | ref(:violation_ids_asc)
+        :MERGED_AT_DESC           | ref(:violation_ids_desc)
+      end
+
+      with_them do
+        it_behaves_like 'sorted paginated query' do
+          let(:first_param) { 2 }
         end
       end
     end