Commit 81adce38 authored by Ash McKenzie's avatar Ash McKenzie

Merge branch 'vzagorodny-stabilize-security-pipeline-vulns-finder-sort' into 'master'

Make PipelineVulnerabilitiesFinder sort stable

See merge request gitlab-org/gitlab!19153
parents cca66afa acb56a7f
...@@ -41,11 +41,27 @@ module Security ...@@ -41,11 +41,27 @@ module Security
occurrences.concat(filtered_occurrences) occurrences.concat(filtered_occurrences)
end end
occurrences.sort_by { |x| [-x.severity_value, -x.confidence_value] } sort_occurrences(occurrences)
end end
private private
def sort_occurrences(occurrences)
# This sort is stable (see https://en.wikipedia.org/wiki/Sorting_algorithm#Stability) contrary to the bare
# Ruby sort_by method. Using just sort_by leads to instability across different platforms (e.g., x86_64-linux and
# x86_64-darwin18) which in turn leads to different sorting results for the equal elements across these platforms.
# This is important because it breaks test suite results consistency between local and CI
# environment.
# This is easier to address from within the class rather than from tests because this leads to bad class design
# and exposing too much of its implementation details to the test suite.
# See also https://stackoverflow.com/questions/15442298/is-sort-in-ruby-stable.
stable_sort_by(occurrences) { |x| [-x.severity_value, -x.confidence_value] }
end
def stable_sort_by(occurrences)
occurrences.sort_by.with_index { |x, idx| [yield(x), idx] }
end
def pipeline_reports def pipeline_reports
pipeline&.security_reports&.reports pipeline&.security_reports&.reports
end end
......
...@@ -275,6 +275,21 @@ describe Security::PipelineVulnerabilitiesFinder do ...@@ -275,6 +275,21 @@ describe Security::PipelineVulnerabilitiesFinder do
end end
end end
context 'when being tested for sort stability' do
let(:params) { { report_type: %w[sast] } }
it 'maintains the order of the occurrences having the same severity and confidence' do
select_proc = proc { |o| o.severity == 'medium' && o.confidence == 'high' }
report_occurrences = pipeline.security_reports.reports['sast'].occurrences.select(&select_proc)
found_occurrences = subject.select(&select_proc)
found_occurrences.each_with_index do |found, i|
expect(found.metadata['cve']).to eq(report_occurrences[i].compare_key)
end
end
end
def read_fixture(fixture) def read_fixture(fixture)
JSON.parse(File.read(fixture.file.path)) JSON.parse(File.read(fixture.file.path))
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