Skip to content

Optimization of labels handling in issue_search #26460

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

Merged
merged 6 commits into from
Jun 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 22 additions & 4 deletions models/issues/issue_search.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,15 @@ package issues
import (
"context"
"fmt"
"strconv"
"strings"

"code.gitea.io/gitea/models/db"
"code.gitea.io/gitea/models/organization"
repo_model "code.gitea.io/gitea/models/repo"
"code.gitea.io/gitea/models/unit"
user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/container"
"code.gitea.io/gitea/modules/optional"

"xorm.io/builder"
Expand Down Expand Up @@ -116,14 +118,30 @@ func applyLabelsCondition(sess *xorm.Session, opts *IssuesOptions) {
if opts.LabelIDs[0] == 0 {
sess.Where("issue.id NOT IN (SELECT issue_id FROM issue_label)")
} else {
for i, labelID := range opts.LabelIDs {
// We sort and deduplicate the labels' ids
IncludedLabelIDs := make(container.Set[int64])
ExcludedLabelIDs := make(container.Set[int64])
for _, labelID := range opts.LabelIDs {
if labelID > 0 {
sess.Join("INNER", fmt.Sprintf("issue_label il%d", i),
fmt.Sprintf("issue.id = il%[1]d.issue_id AND il%[1]d.label_id = %[2]d", i, labelID))
IncludedLabelIDs.Add(labelID)
} else if labelID < 0 { // 0 is not supported here, so just ignore it
sess.Where("issue.id not in (select issue_id from issue_label where label_id = ?)", -labelID)
ExcludedLabelIDs.Add(-labelID)
}
}
// ... and use them in a subquery of the form :
// where (select count(*) from issue_label where issue_id=issue.id and label_id in (2, 4, 6)) = 3
// This equality is guaranteed thanks to unique index (issue_id,label_id) on table issue_label.
if len(IncludedLabelIDs) > 0 {
subquery := builder.Select("count(*)").From("issue_label").Where(builder.Expr("issue_id = issue.id")).
And(builder.In("label_id", IncludedLabelIDs.Values()))
sess.Where(builder.Eq{strconv.Itoa(len(IncludedLabelIDs)): subquery})
}
// or (select count(*)...) = 0 for excluded labels
if len(ExcludedLabelIDs) > 0 {
subquery := builder.Select("count(*)").From("issue_label").Where(builder.Expr("issue_id = issue.id")).
And(builder.In("label_id", ExcludedLabelIDs.Values()))
sess.Where(builder.Eq{"0": subquery})
}
}
}

Expand Down
21 changes: 16 additions & 5 deletions models/issues/label.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ package issues
import (
"context"
"fmt"
"slices"
"strconv"
"strings"

Expand Down Expand Up @@ -142,9 +143,8 @@ func (l *Label) CalOpenOrgIssues(ctx context.Context, repoID, labelID int64) {

// LoadSelectedLabelsAfterClick calculates the set of selected labels when a label is clicked
func (l *Label) LoadSelectedLabelsAfterClick(currentSelectedLabels []int64, currentSelectedExclusiveScopes []string) {
var labelQuerySlice []string
labelQuerySlice := []int64{}
labelSelected := false
labelID := strconv.FormatInt(l.ID, 10)
labelScope := l.ExclusiveScope()
for i, s := range currentSelectedLabels {
if s == l.ID {
Expand All @@ -155,15 +155,26 @@ func (l *Label) LoadSelectedLabelsAfterClick(currentSelectedLabels []int64, curr
} else if s != 0 {
// Exclude other labels in the same scope from selection
if s < 0 || labelScope == "" || labelScope != currentSelectedExclusiveScopes[i] {
labelQuerySlice = append(labelQuerySlice, strconv.FormatInt(s, 10))
labelQuerySlice = append(labelQuerySlice, s)
}
}
}

if !labelSelected {
labelQuerySlice = append(labelQuerySlice, labelID)
labelQuerySlice = append(labelQuerySlice, l.ID)
}
l.IsSelected = labelSelected
l.QueryString = strings.Join(labelQuerySlice, ",")

// Sort and deduplicate the ids to avoid the crawlers asking for the
// same thing with simply a different order of parameters
slices.Sort(labelQuerySlice)
labelQuerySlice = slices.Compact(labelQuerySlice)
Comment on lines +170 to +171
Copy link
Member

Choose a reason for hiding this comment

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

Just use code.gitea.io/gitea/modules/container.Set. Then you don't need to sort and compact.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review ! However, I used container.Set in my previous iteration and the code was less readable and the output's ordering was random (container.Set uses map).
I still use it in models/issues/issue_search.go where its Add() method and the on-the-fly de-duplication are quite handy.

Copy link
Member

Choose a reason for hiding this comment

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

Didn't see that the output is not just used in the backend. Then you could just use the set with slices.Sort at the end but that is only better if labelQuerySlice is very large.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think KN4CK3R 's suggestion is right. Managed to do some rewriting: #31497

// Quick conversion (strings.Join() doesn't accept slices of Int64)
labelQuerySliceStrings := make([]string, len(labelQuerySlice))
for i, x := range labelQuerySlice {
labelQuerySliceStrings[i] = strconv.FormatInt(x, 10)
}
l.QueryString = strings.Join(labelQuerySliceStrings, ",")
}

// BelongsToOrg returns true if label is an organization label
Expand Down
21 changes: 21 additions & 0 deletions models/issues/label_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,27 @@ func TestLabel_CalOpenIssues(t *testing.T) {
assert.EqualValues(t, 2, label.NumOpenIssues)
}

func TestLabel_LoadSelectedLabelsAfterClick(t *testing.T) {
assert.NoError(t, unittest.PrepareTestDatabase())
// Loading the label id:8 which have a scope and an exclusivity
label := unittest.AssertExistsAndLoadBean(t, &issues_model.Label{ID: 8})

// First test : with negative and scope
label.LoadSelectedLabelsAfterClick([]int64{1, -8}, []string{"", "scope"})
assert.Equal(t, "1", label.QueryString)
assert.Equal(t, true, label.IsSelected)

// Second test : with duplicates
label.LoadSelectedLabelsAfterClick([]int64{1, 7, 1, 7, 7}, []string{"", "scope", "", "scope", "scope"})
assert.Equal(t, "1,8", label.QueryString)
assert.Equal(t, false, label.IsSelected)

// Third test : empty set
label.LoadSelectedLabelsAfterClick([]int64{}, []string{})
assert.False(t, label.IsSelected)
assert.Equal(t, "8", label.QueryString)
}

func TestLabel_ExclusiveScope(t *testing.T) {
assert.NoError(t, unittest.PrepareTestDatabase())
label := unittest.AssertExistsAndLoadBean(t, &issues_model.Label{ID: 7})
Expand Down