feat(vulnerability): Added target remediation date to default ordering#1228
Open
tsim-sap wants to merge 3 commits into
Open
feat(vulnerability): Added target remediation date to default ordering#1228tsim-sap wants to merge 3 commits into
tsim-sap wants to merge 3 commits into
Conversation
b00ed66 to
697f8fa
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the vulnerability list’s default ordering to include a remediation-date tie-breaker (to address issue #1124) and adds an e2e regression test to validate the new ordering behavior.
Changes:
- Introduces a new
OrderByField(IssueEarliestTargetRemediationDate) and maps it to a DB column name for ordering. - Updates issue query/cursor plumbing to carry an “earliest target remediation date” ordering component (MV-backed path).
- Adds an e2e regression test that seeds deterministic vulnerabilities and asserts ordering by severity desc → earliest remediation date asc → name asc.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/entity/order.go | Adds a new order-by field enum for earliest remediation date (note: impacts cursor stability if inserted mid-iota). |
| internal/e2e/vulnerability_query_test.go | Adds regression test for ordering within equal severities using earliest remediation date and name tie-break. |
| internal/database/mariadb/order.go | Maps the new order-by field to its SQL column/alias name. |
| internal/database/mariadb/issue.go | Selects MV earliest remediation date as an extra column for ordering/cursor; plumbs into cursor creation. |
| internal/database/mariadb/issue_test.go | Updates pagination helper cursor construction to match new WithIssue signature. |
| internal/database/mariadb/entity.go | Adds scan target for issue_earliest_target_remediation_date into IssueVariantRow. |
| internal/database/mariadb/cursor.go | Extends WithIssue signature and adds cursor field support for earliest remediation date (currently only when non-NULL). |
| internal/app/issue/issue_handler_test.go | Updates cursor creation calls to match new WithIssue signature. |
| internal/api/graphql/graph/baseResolver/vulnerability.go | Changes default ordering to include remediation date (MV vs IssueMatch path). |
Comments suppressed due to low confidence (2)
internal/entity/order.go:44
- Adding a new
OrderByFieldin the middle of theiotablock shifts the numeric values of all subsequent constants. SinceOrderByFieldis JSON-encoded into pagination cursors (Field.Nameininternal/database/mariadb/cursor.go), this can break cursor decoding across deployments (old cursors will map to the wrong fields). To keep existing values stable, append new fields at the end (or switch to explicit values).
IssueEarliestTargetRemediationDate
CriticalCount
HighCount
MediumCount
internal/database/mariadb/cursor.go:354
WithIssuedoes not add a cursor field forentity.IssueMatchTargetRemediationDate, even though the vulnerability resolver now uses that field in the ORDER BY for non-MV (parent-scoped) queries. Cursor-based pagination will therefore ignore that ordering component and can return duplicates/skip rows.
Either extend cursor generation to include the remediation-date field when it’s part of the order sequence, or switch the resolver to an ordering field that is already supported and selected deterministically (e.g., an aggregated earliest remediation date).
func WithIssue(order []entity.Order, issue entity.Issue, ivRating int64, earliestTargetRemediation sql.NullTime) NewCursor {
return func(cursors *cursors) error {
order = GetDefaultOrder(order, entity.IssueId, entity.OrderDirectionAsc)
for _, o := range order {
switch o.By {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Valiantsin Tsimoshyk <v.tsimoshyk@sap.com>
Signed-off-by: Valiantsin Tsimoshyk <v.tsimoshyk@sap.com>
Signed-off-by: Valiantsin Tsimoshyk <v.tsimoshyk@sap.com>
d5b3624 to
04fd1ec
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.
What type of PR is this? (check all applicable)
Related Tickets & Documents
Added tests?
Checklist