draft(issue_match): initial ordering proposal based on entity tags #356
Conversation
This file contains 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
MR2011
reviewed
Nov 8, 2024
| filterStr := s.getIssueMatchFilterString(filter) | ||
| joins := s.getIssueMatchJoins(filter) | ||
| cursor := getCursor(filter.Paginated, filterStr, "IM.issuematch_id > ?") | ||
| if withOrder { |
There was a problem hiding this comment.
Can't we just check len(order) > 0 ?
MR2011
reviewed
Nov 8, 2024
| @@ -120,6 +120,8 @@ func IssueMatchBaseResolver(app app.Heureka, ctx context.Context, filter *model. | |||
|
|
|||
There was a problem hiding this comment.
I assume line 97 is not required anymore?
issue_match_ids := []*int64{}
for _, issue_match_id := range filter.ID {
filterById, err := ParseCursor(issue_match_id)
There was a problem hiding this comment.
Or maybe ParseCursor should be renamed?
|
After internal discussions, we want to rather use a Builder Like pattern |
dorneanu
reviewed
Nov 18, 2024
| filterParameters = append(filterParameters, cursor.Limit) | ||
| } | ||
|
|
||
| return stmt, filterParameters, nil | ||
| } | ||
|
|
||
| func (s *SqlDatabase) GetAllIssueMatchIds(filter *entity.IssueMatchFilter) ([]int64, error) { | ||
| func (s *SqlDatabase) GetAllIssueMatchCursors(filter *entity.IssueMatchFilter, order []entity.Order) ([]string, error) { |
There was a problem hiding this comment.
Why do we need this method actually? How does the logic differ from ListIssues ?
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
This introduces cursor-based ordering for the IssueMatch functionality in the mariadb package. This is an option for an implementaiton of this.
In general for the ordering based on cursors to work we need a cursor that includes all the fields we can order by, e.g.:
"issuematch_id=1, target_remediation_date=2026-01-01, severity=High"
This is needed as we need to build a where statement based on the ordering values e.g. when we want to order first by sevrity, then by target_remeditation_date and then by some other value and then by id it would look like:
( (severity > :severity) OR (severity = :severity ADN target_remediation_date > :trd) OR (severity = :severity ADN target_remediation_date = :trd AND some_other_value>:sov) OR (severity = :severity ADN target_remediation_date = :trd AND some_other_value=:sov AND issuematch_id>:id) )While the values need to be extracted from the cursor.
This also means we need to be able to generate the cursors accordingly and give cursors for certain list entries back to the caller so that they can use the cursor for pagination. (Getting everything after the cursor)
The here drafted solution does use an approach that marshals and unmarshals cursors into structs based on tags using reflection.
Other potential solutions would be to use a "Builder Pattern" similar to
https://go.dev/play/p/K7aiTQ1zHpW
Another option would be to use JSON as pointers and utilize the already implemented json struct scanning. This would have the disadvantage that it would not support the sql.Null declaration and we could not re-use existing structures.
More details to follow!
What type of PR is this? (check all applicable)
Related Tickets & Documents
#310
Added tests?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Added to documentation?
Checklist