Skip to content

Refactor issue label selection #31497

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 2 commits into from
Jun 26, 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: 13 additions & 13 deletions models/issues/issue_search.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,29 +118,29 @@ 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 {
// We sort and deduplicate the labels' ids
IncludedLabelIDs := make(container.Set[int64])
ExcludedLabelIDs := make(container.Set[int64])
// deduplicate the label IDs for inclusion and exclusion
includedLabelIDs := make(container.Set[int64])
excludedLabelIDs := make(container.Set[int64])
for _, labelID := range opts.LabelIDs {
if labelID > 0 {
IncludedLabelIDs.Add(labelID)
includedLabelIDs.Add(labelID)
} else if labelID < 0 { // 0 is not supported here, so just ignore it
ExcludedLabelIDs.Add(-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})
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})
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
30 changes: 13 additions & 17 deletions models/issues/label.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"strings"

"code.gitea.io/gitea/models/db"
"code.gitea.io/gitea/modules/container"
"code.gitea.io/gitea/modules/label"
"code.gitea.io/gitea/modules/optional"
"code.gitea.io/gitea/modules/timeutil"
Expand Down Expand Up @@ -143,37 +144,32 @@ 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) {
labelQuerySlice := []int64{}
labelQueryParams := container.Set[string]{}
labelSelected := false
labelScope := l.ExclusiveScope()
for i, s := range currentSelectedLabels {
if s == l.ID {
exclusiveScope := l.ExclusiveScope()
for i, curSel := range currentSelectedLabels {
if curSel == l.ID {
labelSelected = true
} else if -s == l.ID {
} else if -curSel == l.ID {
labelSelected = true
l.IsExcluded = true
} else if s != 0 {
} else if curSel != 0 {
// Exclude other labels in the same scope from selection
if s < 0 || labelScope == "" || labelScope != currentSelectedExclusiveScopes[i] {
labelQuerySlice = append(labelQuerySlice, s)
if curSel < 0 || exclusiveScope == "" || exclusiveScope != currentSelectedExclusiveScopes[i] {
labelQueryParams.Add(strconv.FormatInt(curSel, 10))
}
}
}

if !labelSelected {
labelQuerySlice = append(labelQuerySlice, l.ID)
labelQueryParams.Add(strconv.FormatInt(l.ID, 10))
}
l.IsSelected = labelSelected

// 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)
}
labelQuerySliceStrings := labelQueryParams.Values()
slices.Sort(labelQuerySliceStrings) // the sort is still needed because the underlying map of Set doesn't guarantee order
Copy link
Contributor

Choose a reason for hiding this comment

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

Little warning: probably acceptable in this case but you're sorting strings now so it will output something like 1,10,2 instead of 1,2,10.

Copy link
Contributor Author

@wxiaoguang wxiaoguang Jun 26, 2024

Choose a reason for hiding this comment

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

Don't really see the necessity.

They are IDs and are totally transparent to other modules, the only thing we need to do is to make them ordered, so string comparing or number comparing doesn't matter.

Think about another case, if we have a lot of UUIDs as IDs, we could also just sort them, and we do not really care about why A<B or A>B

l.QueryString = strings.Join(labelQuerySliceStrings, ",")
}

Expand All @@ -187,7 +183,7 @@ func (l *Label) BelongsToRepo() bool {
return l.RepoID > 0
}

// Return scope substring of label name, or empty string if none exists
// ExclusiveScope returns scope substring of label name, or empty string if none exists
func (l *Label) ExclusiveScope() string {
if !l.Exclusive {
return ""
Expand Down