fix(image): sort images by vulnerability count by default#1156
fix(image): sort images by vulnerability count by default#1156
Conversation
There was a problem hiding this comment.
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return o.By == entity.CriticalCount || o.By == entity.HighCount || o.By == entity.MediumCount || o.By == entity.LowCount || o.By == entity.NoneCount | ||
| }) | ||
| return orderByCount && (len(filter.Id) == 0 && (len(filter.ServiceCCRN) > 0)) | ||
| return !orderByCount && (len(filter.Id) == 0 && (len(filter.ServiceCCRN) > 0)) |
There was a problem hiding this comment.
Logic error in needAllComponentByServiceVulnerabilityCounts: the condition was changed from orderByCount && to !orderByCount &&, which inverts the logic. This means the materialized view will now be joined when NOT ordering by vulnerability counts, which is the opposite of the intended behavior. When vulnerability count ordering is applied globally in ImageBaseResolver, this join will never be used, causing potentially incorrect results.
| return !orderByCount && (len(filter.Id) == 0 && (len(filter.ServiceCCRN) > 0)) | |
| return orderByCount && (len(filter.Id) == 0 && (len(filter.ServiceCCRN) > 0)) |
| // The test data setup creates images with different vulnerability counts | ||
| // We expect them to be ordered by: critical, high, medium, low, none counts (descending) | ||
| // then by repository name (ascending) as tiebreaker | ||
| // TODO: make sure there is a case with same vulnerability counts but different repository names to verify the secondary ordering as well | ||
|
|
||
| // Extract the vulnerability counts for comparison | ||
| var previousCounts model.SeverityCounts | ||
| for i, edge := range respData.Images.Edges { | ||
| counts := *edge.Node.VulnerabilityCounts | ||
|
|
||
| if i > 0 { | ||
| comparison := e2e_common.CompareSeverityCounts(counts, previousCounts) | ||
| Expect(comparison).To(BeNumerically("<=", 0), | ||
| fmt.Sprintf("Image %d (%s) should have equal or lower severity than image %d (%s). Counts: %v vs %v", | ||
| i, *edge.Node.Repository, i-1, *respData.Images.Edges[i-1].Node.Repository, counts, previousCounts)) | ||
| } | ||
|
|
||
| previousCounts = counts | ||
|
|
||
| Expect(edge.Node.Repository).ToNot(BeNil(), "Image should have repository") | ||
| Expect(edge.Node.VulnerabilityCounts).ToNot(BeNil(), "Image should have vulnerability counts") | ||
| } |
There was a problem hiding this comment.
The test has a TODO comment acknowledging that it doesn't verify secondary ordering (by repository name) when vulnerability counts are equal. Consider adding test data with images having the same vulnerability counts but different repository names to fully validate the secondary ordering behavior.
| // The test data setup creates images with different vulnerability counts | |
| // We expect them to be ordered by: critical, high, medium, low, none counts (descending) | |
| // then by repository name (ascending) as tiebreaker | |
| // TODO: make sure there is a case with same vulnerability counts but different repository names to verify the secondary ordering as well | |
| // Extract the vulnerability counts for comparison | |
| var previousCounts model.SeverityCounts | |
| for i, edge := range respData.Images.Edges { | |
| counts := *edge.Node.VulnerabilityCounts | |
| if i > 0 { | |
| comparison := e2e_common.CompareSeverityCounts(counts, previousCounts) | |
| Expect(comparison).To(BeNumerically("<=", 0), | |
| fmt.Sprintf("Image %d (%s) should have equal or lower severity than image %d (%s). Counts: %v vs %v", | |
| i, *edge.Node.Repository, i-1, *respData.Images.Edges[i-1].Node.Repository, counts, previousCounts)) | |
| } | |
| previousCounts = counts | |
| Expect(edge.Node.Repository).ToNot(BeNil(), "Image should have repository") | |
| Expect(edge.Node.VulnerabilityCounts).ToNot(BeNil(), "Image should have vulnerability counts") | |
| } | |
| // and by repository name in ascending order as a tiebreaker. | |
| var previousCounts model.SeverityCounts | |
| var previousRepository string | |
| hadEqualCountsPair := false | |
| for i, edge := range respData.Images.Edges { | |
| Expect(edge.Node.Repository).ToNot(BeNil(), "Image should have repository") | |
| Expect(edge.Node.VulnerabilityCounts).ToNot(BeNil(), "Image should have vulnerability counts") | |
| counts := *edge.Node.VulnerabilityCounts | |
| repository := *edge.Node.Repository | |
| if i > 0 { | |
| comparison := e2e_common.CompareSeverityCounts(counts, previousCounts) | |
| Expect(comparison).To(BeNumerically("<=", 0), | |
| fmt.Sprintf("Image %d (%s) should have equal or lower severity than image %d (%s). Counts: %v vs %v", | |
| i, repository, i-1, previousRepository, counts, previousCounts)) | |
| if comparison == 0 { | |
| hadEqualCountsPair = true | |
| Expect(previousRepository <= repository).To(BeTrue(), | |
| fmt.Sprintf("Image %d (%s) should sort after image %d (%s) when severity counts are equal. Counts: %v", | |
| i, repository, i-1, previousRepository, counts)) | |
| } | |
| } | |
| previousCounts = counts | |
| previousRepository = repository | |
| } | |
| Expect(hadEqualCountsPair).To(BeTrue(), | |
| "Test setup should include at least one pair of images with equal vulnerability counts but different repository names to verify secondary ordering") |
Description
This PR forces the ordering to prioritize sorting by vulnerability count and then use alphabetical ordering as a fallback.
Related Tickets & Documents
Fixes #1125
Added tests?
Added to documentation?
Checklist