Commit 6d745e13 authored by Robert Hunt's avatar Robert Hunt

Fix ComplianceViolationResolver pagination with sorting

Some of the sorting options are complex in nature. For these, the
resolver now uses offset pagination rather than keyset.

Changelog: fixed
EE: true
MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/83303
parent 7680b4b9
...@@ -24,8 +24,24 @@ module Resolvers ...@@ -24,8 +24,24 @@ module Resolvers
default_value: 'SEVERITY_LEVEL_DESC', default_value: 'SEVERITY_LEVEL_DESC',
description: 'List compliance violations by sort order.' 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') 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 end
end end
......
...@@ -73,16 +73,16 @@ RSpec.describe Resolvers::ComplianceManagement::MergeRequests::ComplianceViolati ...@@ -73,16 +73,16 @@ RSpec.describe Resolvers::ComplianceManagement::MergeRequests::ComplianceViolati
end end
context 'sorting the results' do context 'sorting the results' do
where(:direction, :result) do where(:direction, :pagination_type, :result) do
'SEVERITY_LEVEL_ASC' | lazy { [compliance_violation, compliance_violation2] } 'SEVERITY_LEVEL_ASC' | :keyset | lazy { [compliance_violation, compliance_violation2] }
'SEVERITY_LEVEL_DESC' | lazy { [compliance_violation2, compliance_violation] } 'SEVERITY_LEVEL_DESC' | :keyset | lazy { [compliance_violation2, compliance_violation] }
'VIOLATION_REASON_ASC' | lazy { [compliance_violation, compliance_violation2] } 'VIOLATION_REASON_ASC' | :keyset | lazy { [compliance_violation, compliance_violation2] }
'VIOLATION_REASON_DESC' | lazy { [compliance_violation2, compliance_violation] } 'VIOLATION_REASON_DESC' | :keyset | lazy { [compliance_violation2, compliance_violation] }
'MERGE_REQUEST_TITLE_ASC' | lazy { [compliance_violation, compliance_violation2] } 'MERGE_REQUEST_TITLE_ASC' | :offset | lazy { [compliance_violation, compliance_violation2] }
'MERGE_REQUEST_TITLE_DESC' | lazy { [compliance_violation2, compliance_violation] } 'MERGE_REQUEST_TITLE_DESC' | :offset | lazy { [compliance_violation2, compliance_violation] }
'MERGED_AT_ASC' | lazy { [compliance_violation, compliance_violation2] } 'MERGED_AT_ASC' | :offset | lazy { [compliance_violation, compliance_violation2] }
'MERGED_AT_DESC' | lazy { [compliance_violation2, compliance_violation] } 'MERGED_AT_DESC' | :offset | lazy { [compliance_violation2, compliance_violation] }
'UNKNOWN_SORT' | lazy { [compliance_violation, compliance_violation2] } 'UNKNOWN_SORT' | :keyset | lazy { [compliance_violation, compliance_violation2] }
end end
with_them do with_them do
...@@ -91,6 +91,16 @@ RSpec.describe Resolvers::ComplianceManagement::MergeRequests::ComplianceViolati ...@@ -91,6 +91,16 @@ RSpec.describe Resolvers::ComplianceManagement::MergeRequests::ComplianceViolati
it 'finds the filtered compliance violations' do it 'finds the filtered compliance violations' do
expect(subject).to match_array(result) expect(subject).to match_array(result)
end 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 end
end end
......
...@@ -21,17 +21,17 @@ RSpec.describe 'getting the compliance violations for a group' do ...@@ -21,17 +21,17 @@ RSpec.describe 'getting the compliance violations for a group' do
let(:fields) do let(:fields) do
<<~GRAPHQL <<~GRAPHQL
nodes { nodes {
id
severityLevel
reason
violatingUser {
id id
severityLevel
reason
violatingUser {
id
}
mergeRequest {
id
}
} }
mergeRequest {
id
}
}
GRAPHQL GRAPHQL
end end
...@@ -117,23 +117,36 @@ RSpec.describe 'getting the compliance violations for a group' do ...@@ -117,23 +117,36 @@ RSpec.describe 'getting the compliance violations for a group' do
end end
end end
context 'sorting the results' do describe 'sorting and pagination' do
where(:direction, :result) do let_it_be(:data_path) { [:group, :merge_request_violations] }
:SEVERITY_LEVEL_ASC | lazy { [violation_output, violation2_output] }
:SEVERITY_LEVEL_DESC | lazy { [violation2_output, violation_output] } let(:violation_ids_asc) { [violation_output['id'].to_i, violation2_output['id'].to_i] }
:VIOLATION_REASON_ASC | lazy { [violation_output, violation2_output] } let(:violation_ids_desc) { [violation2_output['id'].to_i, violation_output['id'].to_i] }
:VIOLATION_REASON_DESC | lazy { [violation2_output, violation_output] }
:MERGE_REQUEST_TITLE_ASC | lazy { [violation_output, violation2_output] } def pagination_query(params)
:MERGE_REQUEST_TITLE_DESC | lazy { [violation2_output, violation_output] } graphql_query_for(
:MERGED_AT_ASC | lazy { [violation_output, violation2_output] } :group, { full_path: group.full_path }, query_nodes(:merge_request_violations, :id, include_pagination_info: true, args: params)
:MERGED_AT_DESC | lazy { [violation2_output, violation_output] } )
end end
with_them do def pagination_results_data(data)
it 'finds all the compliance violations' do data.map { |merge_request_violations| merge_request_violations['id'].to_i }
post_graphql(query({ sort: direction }), current_user: current_user) 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 end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment