-
Notifications
You must be signed in to change notification settings - Fork 3
fix(image): sort images by vulnerability count by default #1156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -57,6 +57,47 @@ var _ = Describe("Getting Images via API", Label("e2e", "Images"), func() { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Expect(err).ToNot(HaveOccurred()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Expect(*respData.Images.Counts).To(Equal(imgTest.counts)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| It("returns images sorted by vulnerability severity counts then by repository name", func() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| respData, err := e2e_common.ExecuteGqlQueryFromFile[struct { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Images model.ImageConnection `json:"Images"` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }]( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| imgTest.port, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "../api/graphql/graph/queryCollection/image/query.graphql", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| map[string]interface{}{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "filter": map[string]any{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "service": lo.Map(imgTest.services, func(item mariadb.BaseServiceRow, index int) string { return item.CCRN.String }), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "first": 10, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "after": "", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Expect(err).ToNot(HaveOccurred()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Expect(respData.Images.Edges).To(HaveLen(5), "Should return all 5 images") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Verify images are sorted by vulnerability counts in descending order | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // 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") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+78
to
+99
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // 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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.