From b5326a431f8b5c1c0fde524a50d75a4e19833e68 Mon Sep 17 00:00:00 2001 From: Chl <42654312+xlii-chl@users.noreply.github.com> Date: Tue, 25 Jun 2024 19:09:13 +0200 Subject: [PATCH] Optimization of labels handling in issue_search (#26460) This PR enhances the labels handling in issue_search by optimizing the SQL query and de-duplicate the IDs when generating the query string. --------- Co-authored-by: techknowlogick --- models/issues/issue_search.go | 26 ++++++++++++++++++++++---- models/issues/label.go | 21 ++++++++++++++++----- models/issues/label_test.go | 21 +++++++++++++++++++++ 3 files changed, 59 insertions(+), 9 deletions(-) diff --git a/models/issues/issue_search.go b/models/issues/issue_search.go index c1d7d921a..b238f831a 100644 --- a/models/issues/issue_search.go +++ b/models/issues/issue_search.go @@ -6,6 +6,7 @@ package issues import ( "context" "fmt" + "strconv" "strings" "code.gitea.io/gitea/models/db" @@ -13,6 +14,7 @@ import ( 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" @@ -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}) + } } } diff --git a/models/issues/label.go b/models/issues/label.go index 2397a29e3..2d7acb7f0 100644 --- a/models/issues/label.go +++ b/models/issues/label.go @@ -7,6 +7,7 @@ package issues import ( "context" "fmt" + "slices" "strconv" "strings" @@ -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 { @@ -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) + // 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 diff --git a/models/issues/label_test.go b/models/issues/label_test.go index 517a3cf1a..396de809e 100644 --- a/models/issues/label_test.go +++ b/models/issues/label_test.go @@ -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})