Skip to content
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

draft(issue_match): initial ordering proposal based on entity tags #356

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

drochow
Copy link
Collaborator

@drochow drochow commented Nov 6, 2024

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)

  • 🍕 Feature
  • 🐛 Bug Fix
  • 📝 Documentation Update
  • 🎨 Style
  • 🧑‍💻 Code Refactor
  • 🔥 Performance Improvements
  • ✅ Test
  • 🤖 Build
  • 🔁 CI
  • 📦 Chore (Release)
  • ⏩ Revert

Related Tickets & Documents

#310

Added tests?

  • 👍 yes
  • 🙅 no, because they aren't needed
  • 🙋 no, because I need help
  • Separate ticket for tests # (issue/pr)

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 functionality tests
  • Added fuzz testing for Reflection based logic

Added to documentation?

  • 📜 README.md
  • 🤝 Documentation pages updated
  • 🙅 no documentation needed
  • (if applicable) generated OpenAPI docs for CRD changes

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

filterStr := s.getIssueMatchFilterString(filter)
joins := s.getIssueMatchJoins(filter)
cursor := getCursor(filter.Paginated, filterStr, "IM.issuematch_id > ?")
if withOrder {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we just check len(order) > 0 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True

@@ -120,6 +120,8 @@ func IssueMatchBaseResolver(app app.Heureka, ctx context.Context, filter *model.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe ParseCursor should be renamed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True

@drochow
Copy link
Collaborator Author

drochow commented Nov 14, 2024

After internal discussions, we want to rather use a Builder Like pattern

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) {
Copy link
Collaborator

@dorneanu dorneanu Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants