feat(remediation): default sort with target date#1127
Conversation
tsim-sap
force-pushed
the
tsim-sap/issue-1124/default-sort-with-target-date
branch
from
cf4a2d7 to
07db4b5
Compare
tsim-sap
requested review from
MR2011,
copyonwrite,
dorneanu and
michalkrzyz
as code owners
There was a problem hiding this comment.
Pull request overview
Adds a new default ordering for vulnerabilities that incorporates the (aggregated) earliest target remediation date, and propagates the new ordering field through MariaDB ordering/cursor logic plus tests/docs.
Changes:
- Introduce
IssueEarliestTargetRemediationDateas a newOrderByFieldand add MariaDB column mapping for it. - Extend issue query/cursor generation to support ordering (and cursor encoding) by earliest target remediation date.
- Update/extend tests and documentation to reflect the new default ordering behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/entity/order.go | Adds a new order-by field constant for earliest target remediation date. |
| internal/database/mariadb/order.go | Maps the new order-by field to the DB column alias used for ordering. |
| internal/database/mariadb/issue.go | Adds SQL column projection + cursor wiring for earliest target remediation date. |
| internal/database/mariadb/cursor.go | Extends issue cursor fields to include earliest target remediation date when ordered by it. |
| internal/database/mariadb/issue_test.go | Updates pagination cursor helpers and adds an ordering test for earliest target remediation date. |
| internal/app/issue/issue_handler_test.go | Adjusts mocks/tests to the updated WithIssue cursor signature. |
| internal/api/graphql/graph/baseResolver/vulnerability.go | Updates the default ordering for vulnerabilities to include earliest target remediation date. |
| docs/ordering.md | Attempts to document the new ordering field / DB column alias. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| IssueMatchId | ||
| IssueMatchRating | ||
| IssueMatchTargetRemediationDate | ||
| IssueEarliestTargetRemediationDate | ||
|
|
There was a problem hiding this comment.
OrderByField values are used in pagination cursors (see mariadb/cursor.go where Field.Name is JSON-encoded as an int). Adding IssueEarliestTargetRemediationDate in the middle of an iota block shifts all subsequent numeric values, which will break decoding of existing cursors and can corrupt pagination across deployments. Consider appending the new constant at the end (preserving existing numbers) or switching to explicit/stable numeric assignments (or string-based cursor field names).
| // - Only returns Issues of type Vulnerability | ||
| // - Only returns Issues with at least one IssueMatch with status "new" | ||
| // - Default ordering is by IssueVariantRating (descending) and IssuePrimaryName (ascending) | ||
| // - Default ordering is by IssueVariantRating (descending), TargetRemediationDate (descending) and IssuePrimaryName (ascending) |
There was a problem hiding this comment.
The function comment says the default ordering includes "TargetRemediationDate", but the implementation orders by IssueEarliestTargetRemediationDate (an aggregated MIN across issue matches). To avoid ambiguity for future maintainers, please update the comment to explicitly say "EarliestTargetRemediationDate" (and/or mention it's aggregated) so it matches the actual order field used.
| // - Default ordering is by IssueVariantRating (descending), TargetRemediationDate (descending) and IssuePrimaryName (ascending) | |
| // - Default ordering is by IssueVariantRating (descending), EarliestTargetRemediationDate (aggregated, descending) and IssuePrimaryName (ascending) |
| var DbColumnNameMap = map[DbColumnName]string{ | ||
| ComponentInstanceCcrn: "componentinstance_ccrn", | ||
| IssuePrimaryName: "issue_primary_name", | ||
| IssueMatchId: "issuematch_id", | ||
| IssueMatchRating: "issuematch_rating", | ||
| IssueMatchTargetRemediationDate: "issuematch_target_remediation_date", | ||
| SupportGroupName: "supportgroup_name", | ||
| ComponentInstanceCcrn: "componentinstance_ccrn", | ||
| IssuePrimaryName: "issue_primary_name", | ||
| IssueMatchId: "issuematch_id", | ||
| IssueMatchRating: "issuematch_rating", | ||
| IssueEarliestTargetRemediationDate: "agg_earliest_target_remediation_date", | ||
| IssueMatchTargetRemediationDate: "issuematch_target_remediation_date", | ||
| SupportGroupName: "supportgroup_name", |
There was a problem hiding this comment.
This documentation snippet appears out of sync with the codebase: it shows a DbColumnNameMap keyed by DbColumnName, but ordering in the code uses entity.OrderByField and mariadb.ColumnName() (switch). Also, the newly edited lines introduced inconsistent tab/spacing in the code block which hurts readability. Please update the docs example to match the current implementation/types and keep formatting consistent.
tsim-sap
force-pushed
the
tsim-sap/issue-1124/default-sort-with-target-date
branch
3 times, most recently
from
ebfb91a to
15292d5
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| func getIssueJoins(filter *entity.IssueFilter, order []entity.Order) string { | ||
| joins := "" | ||
| orderByRating := lo.ContainsBy(order, func(o entity.Order) bool { | ||
| return o.By == entity.IssueVariantRating | ||
| }) | ||
| orderByIssueMatch := lo.ContainsBy(order, func(o entity.Order) bool { | ||
| return o.By == entity.IssueEarliestTargetRemediationDate || | ||
| o.By == entity.IssueMatchId || | ||
| o.By == entity.IssueMatchRating || | ||
| o.By == entity.IssueMatchTargetRemediationDate | ||
| }) | ||
|
|
||
| if filter.AllServices || filter.HasIssueMatches { | ||
| joins = fmt.Sprintf("%s\n%s", joins, ` | ||
| RIGHT JOIN IssueMatch IM ON I.issue_id = IM.issuematch_issue_id | ||
| `) | ||
| } else if len(filter.IssueMatchStatus) > 0 || len(filter.ServiceId) > 0 || len(filter.ServiceCCRN) > 0 || | ||
| len(filter.IssueMatchId) > 0 || len(filter.SupportGroupCCRN) > 0 || len(filter.IssueMatchSeverity) > 0 { | ||
| len(filter.IssueMatchId) > 0 || len(filter.SupportGroupCCRN) > 0 || len(filter.IssueMatchSeverity) > 0 || | ||
| orderByIssueMatch { | ||
| joins = fmt.Sprintf("%s\n%s", joins, ` | ||
| LEFT JOIN IssueMatch IM ON I.issue_id = IM.issuematch_issue_id | ||
| `) |
| // - Only returns Issues of type Vulnerability | ||
| // - Only returns Issues with at least one IssueMatch with status "new" | ||
| // - Default ordering is by IssueVariantRating (descending) and IssuePrimaryName (ascending) | ||
| // - Default ordering is by IssueVariantRating (descending), TargetRemediationDate (descending) and IssuePrimaryName (ascending) |
tsim-sap
force-pushed
the
tsim-sap/issue-1124/default-sort-with-target-date
branch
from
15292d5 to
9cfe886
Compare
tsim-sap
force-pushed
the
tsim-sap/issue-1124/default-sort-with-target-date
branch
from
9cfe886 to
e2818f5
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| IssueRepositoryID | ||
|
|
||
| IssueMatchId | ||
| IssueMatchRating | ||
| IssueMatchTargetRemediationDate | ||
| IssueEarliestTargetRemediationDate | ||
|
|
||
| CriticalCount | ||
| HighCount |
| whereClause := getIssueFilterWhereClause(filter) | ||
|
|
||
| cursorQuery := CreateCursorQuery("", cursorFields) | ||
| filterStr := issueObject.GetFilterQuery(filter) | ||
| if filterStr != "" && cursorQuery != "" { | ||
| cursorQuery = fmt.Sprintf(" AND (%s)", cursorQuery) | ||
| } |
| @@ -250,8 +265,6 @@ | |||
|
|
|||
| baseCiQuery := ` | |||
| SELECT I.*, SUM(CI.componentinstance_count) AS agg_affected_component_instances %s FROM Issue I | |||
| // - Only returns Issues of type Vulnerability | ||
| // - Only returns Issues with at least one IssueMatch with status "new" | ||
| // - Default ordering is by IssueVariantRating (descending) and IssuePrimaryName (ascending) | ||
| // - Default ordering is by IssueVariantRating (descending), TargetRemediationDate (descending) and IssuePrimaryName (ascending) |
| ```go | ||
| type Order struct { | ||
| By DbColumnName | ||
| Direction OrderDirection | ||
| } | ||
| ``` | ||
|
|
||
| The `By` field is the database column name and is defined as a constant: | ||
|
|
||
| ```go | ||
| var DbColumnNameMap = map[DbColumnName]string{ | ||
| ComponentInstanceCcrn: "componentinstance_ccrn", | ||
| IssuePrimaryName: "issue_primary_name", | ||
| IssueMatchId: "issuematch_id", | ||
| IssueMatchRating: "issuematch_rating", | ||
| IssueMatchTargetRemediationDate: "issuematch_target_remediation_date", | ||
| SupportGroupName: "supportgroup_name", | ||
| ComponentInstanceCcrn: "componentinstance_ccrn", | ||
| IssuePrimaryName: "issue_primary_name", | ||
| IssueMatchId: "issuematch_id", | ||
| IssueMatchRating: "issuematch_rating", | ||
| IssueEarliestTargetRemediationDate: "agg_earliest_target_remediation_date", | ||
| IssueMatchTargetRemediationDate: "issuematch_target_remediation_date", | ||
| SupportGroupName: "supportgroup_name", | ||
| } |
Description
Added default sort by earliestTargetRemediationDate. Adjusted existing tests, added a test for this feature.
What type of PR is this? (check all applicable)
Related Tickets & Documents
Added tests?
Added to documentation?
Checklist