From f5fd936cbb1b5560c0235dd5f193c4c0aec1821a Mon Sep 17 00:00:00 2001 From: Giteabot Date: Mon, 12 Oct 2026 17:53:02 -0700 Subject: [PATCH] fix: correct label deletion and release attachment load (#39743) (#39752) Backport #39743 by wxiaoguang Co-authored-by: wxiaoguang --- models/issues/label.go | 32 ++++++++++++--------------- models/issues/label_test.go | 22 ++++++++++++++++--- models/repo/release.go | 37 +++++++++++++++++++------------- routers/api/v1/org/label.go | 2 +- routers/api/v1/repo/label.go | 2 +- routers/web/org/org_labels.go | 2 +- routers/web/repo/issue_label.go | 2 +- services/release/release_test.go | 6 +++++- 8 files changed, 64 insertions(+), 41 deletions(-) diff --git a/models/issues/label.go b/models/issues/label.go index 24ca9d8c1cb..25fbacf16b7 100644 --- a/models/issues/label.go +++ b/models/issues/label.go @@ -269,27 +269,23 @@ func UpdateLabel(ctx context.Context, l *Label) error { return updateLabelCols(ctx, l, "name", "description", "color", "exclusive", "exclusive_order", "archived_unix") } -// DeleteLabel delete a label -func DeleteLabel(ctx context.Context, id, labelID int64) error { - l, err := GetLabelByID(ctx, labelID) - if err != nil { - if IsErrLabelNotExist(err) { - return nil - } - return err - } +// DeleteRepoLabel deletes a repository label. +func DeleteRepoLabel(ctx context.Context, repoID, labelID int64) error { + return deleteLabel(ctx, labelID, builder.Eq{"repo_id": repoID, "org_id": 0}) +} +// DeleteOrgLabel deletes an organization label. +func DeleteOrgLabel(ctx context.Context, orgID, labelID int64) error { + return deleteLabel(ctx, labelID, builder.Eq{"org_id": orgID, "repo_id": 0}) +} + +func deleteLabel(ctx context.Context, labelID int64, ownerCond builder.Cond) error { return db.WithTx(ctx, func(ctx context.Context) error { - if l.BelongsToOrg() && l.OrgID != id { - return nil - } - if l.BelongsToRepo() && l.RepoID != id { - return nil - } - - if _, err = db.DeleteByID[Label](ctx, labelID); err != nil { + deleted, err := db.GetEngine(ctx).ID(labelID).Where(ownerCond).Delete(new(Label)) + if err != nil || deleted == 0 { return err - } else if _, err = db.GetEngine(ctx). + } + if _, err = db.GetEngine(ctx). Where("label_id = ?", labelID). Delete(new(IssueLabel)); err != nil { return err diff --git a/models/issues/label_test.go b/models/issues/label_test.go index 94958e414fc..08113b2ad7f 100644 --- a/models/issues/label_test.go +++ b/models/issues/label_test.go @@ -320,13 +320,29 @@ func TestUpdateLabel(t *testing.T) { func TestDeleteLabel(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) label := unittest.AssertExistsAndLoadBean(t, &issues_model.Label{ID: 1}) - assert.NoError(t, issues_model.DeleteLabel(t.Context(), label.RepoID, label.ID)) + orgLabel := unittest.AssertExistsAndLoadBean(t, &issues_model.Label{ID: 3}) + repoLabel := unittest.AssertExistsAndLoadBean(t, &issues_model.Label{ID: 10, RepoID: orgLabel.OrgID}) + assert.NoError(t, issues_model.DeleteOrgLabel(t.Context(), repoLabel.RepoID, repoLabel.ID)) + unittest.AssertExistsAndLoadBean(t, &issues_model.Label{ID: repoLabel.ID}) + + assert.NoError(t, issues_model.DeleteRepoLabel(t.Context(), orgLabel.OrgID, orgLabel.ID)) + unittest.AssertExistsAndLoadBean(t, &issues_model.Label{ID: orgLabel.ID}) + + assert.NoError(t, issues_model.DeleteOrgLabel(t.Context(), unittest.NonexistentID, orgLabel.ID)) + unittest.AssertExistsAndLoadBean(t, &issues_model.Label{ID: orgLabel.ID}) + assert.NoError(t, issues_model.DeleteOrgLabel(t.Context(), orgLabel.OrgID, orgLabel.ID)) + unittest.AssertNotExistsBean(t, &issues_model.Label{ID: orgLabel.ID}) + + assert.NoError(t, issues_model.DeleteRepoLabel(t.Context(), unittest.NonexistentID, label.ID)) + unittest.AssertExistsAndLoadBean(t, &issues_model.Label{ID: label.ID}) + assert.NoError(t, issues_model.DeleteRepoLabel(t.Context(), label.RepoID, label.ID)) unittest.AssertNotExistsBean(t, &issues_model.Label{ID: label.ID, RepoID: label.RepoID}) - assert.NoError(t, issues_model.DeleteLabel(t.Context(), label.RepoID, label.ID)) + assert.NoError(t, issues_model.DeleteRepoLabel(t.Context(), label.RepoID, label.ID)) unittest.AssertNotExistsBean(t, &issues_model.Label{ID: label.ID}) - assert.NoError(t, issues_model.DeleteLabel(t.Context(), unittest.NonexistentID, unittest.NonexistentID)) + assert.NoError(t, issues_model.DeleteRepoLabel(t.Context(), unittest.NonexistentID, unittest.NonexistentID)) + assert.NoError(t, issues_model.DeleteOrgLabel(t.Context(), unittest.NonexistentID, unittest.NonexistentID)) unittest.CheckConsistencyFor(t, &issues_model.Label{}, &repo_model.Repository{}) } diff --git a/models/repo/release.go b/models/repo/release.go index 87b59f8f202..f97d51aac23 100644 --- a/models/repo/release.go +++ b/models/repo/release.go @@ -357,21 +357,21 @@ func GetLatestReleaseByRepoID(ctx context.Context, repoID int64) (*Release, erro } type releaseMetaSearch struct { - ID []int64 - Rel []*Release + IDs []int64 + Releases []*Release } func (s releaseMetaSearch) Len() int { - return len(s.ID) + return len(s.IDs) } func (s releaseMetaSearch) Swap(i, j int) { - s.ID[i], s.ID[j] = s.ID[j], s.ID[i] - s.Rel[i], s.Rel[j] = s.Rel[j], s.Rel[i] + s.IDs[i], s.IDs[j] = s.IDs[j], s.IDs[i] + s.Releases[i], s.Releases[j] = s.Releases[j], s.Releases[i] } func (s releaseMetaSearch) Less(i, j int) bool { - return s.ID[i] < s.ID[j] + return s.IDs[i] < s.IDs[j] } func hasDuplicateName(attaches []*Attachment) bool { @@ -396,19 +396,26 @@ func GetReleaseAttachments(ctx context.Context, rels ...*Release) (err error) { // then merge join them // Sort - sortedRels := releaseMetaSearch{ID: make([]int64, len(rels)), Rel: make([]*Release, len(rels))} + sortedRels := releaseMetaSearch{IDs: make([]int64, 0, len(rels)), Releases: make([]*Release, 0, len(rels))} var attachments []*Attachment - for index, element := range rels { - element.Attachments = []*Attachment{} - sortedRels.ID[index] = element.ID - sortedRels.Rel[index] = element + for _, rel := range rels { + // if a provided "Release" has no ID, don't query it, otherwise, the "IN" query will return all other attachments for the repo + if rel.ID == 0 { + continue + } + rel.Attachments = []*Attachment{} + sortedRels.IDs = append(sortedRels.IDs, rel.ID) + sortedRels.Releases = append(sortedRels.Releases, rel) + } + if len(sortedRels.IDs) == 0 { + return nil } sort.Sort(sortedRels) // Select attachments err = db.GetEngine(ctx). Asc("release_id", "name"). - In("release_id", sortedRels.ID). + In("release_id", sortedRels.IDs). Find(&attachments) if err != nil { return err @@ -417,14 +424,14 @@ func GetReleaseAttachments(ctx context.Context, rels ...*Release) (err error) { // merge join currentIndex := 0 for _, attachment := range attachments { - for sortedRels.ID[currentIndex] < attachment.ReleaseID { + for sortedRels.IDs[currentIndex] < attachment.ReleaseID { currentIndex++ } - sortedRels.Rel[currentIndex].Attachments = append(sortedRels.Rel[currentIndex].Attachments, attachment) + sortedRels.Releases[currentIndex].Attachments = append(sortedRels.Releases[currentIndex].Attachments, attachment) } // Makes URL's predictable - for _, release := range rels { + for _, release := range sortedRels.Releases { // If we have no Repo, we don't need to execute this loop if release.Repo == nil { continue diff --git a/routers/api/v1/org/label.go b/routers/api/v1/org/label.go index f3a19b29136..0c5c8386996 100644 --- a/routers/api/v1/org/label.go +++ b/routers/api/v1/org/label.go @@ -249,7 +249,7 @@ func DeleteLabel(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" - if err := issues_model.DeleteLabel(ctx, ctx.Org.Organization.ID, ctx.PathParamInt64("id")); err != nil { + if err := issues_model.DeleteOrgLabel(ctx, ctx.Org.Organization.ID, ctx.PathParamInt64("id")); err != nil { ctx.APIErrorInternal(err) return } diff --git a/routers/api/v1/repo/label.go b/routers/api/v1/repo/label.go index 9ce52de6dd8..755951debb0 100644 --- a/routers/api/v1/repo/label.go +++ b/routers/api/v1/repo/label.go @@ -268,7 +268,7 @@ func DeleteLabel(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" - if err := issues_model.DeleteLabel(ctx, ctx.Repo.Repository.ID, ctx.PathParamInt64("id")); err != nil { + if err := issues_model.DeleteRepoLabel(ctx, ctx.Repo.Repository.ID, ctx.PathParamInt64("id")); err != nil { ctx.APIErrorInternal(err) return } diff --git a/routers/web/org/org_labels.go b/routers/web/org/org_labels.go index 3d76c25844f..2e39b2ea10e 100644 --- a/routers/web/org/org_labels.go +++ b/routers/web/org/org_labels.go @@ -85,7 +85,7 @@ func UpdateLabel(ctx *context.Context) { // DeleteLabel delete a label func DeleteLabel(ctx *context.Context) { - if err := issues_model.DeleteLabel(ctx, ctx.Org.Organization.ID, ctx.FormInt64("id")); err != nil { + if err := issues_model.DeleteOrgLabel(ctx, ctx.Org.Organization.ID, ctx.FormInt64("id")); err != nil { ctx.Flash.Error("DeleteLabel: " + err.Error()) } else { ctx.Flash.Success(ctx.Tr("repo.issues.label_deletion_success")) diff --git a/routers/web/repo/issue_label.go b/routers/web/repo/issue_label.go index 5004b9d9783..310556940a2 100644 --- a/routers/web/repo/issue_label.go +++ b/routers/web/repo/issue_label.go @@ -152,7 +152,7 @@ func UpdateLabel(ctx *context.Context) { // DeleteLabel delete a label func DeleteLabel(ctx *context.Context) { - if err := issues_model.DeleteLabel(ctx, ctx.Repo.Repository.ID, ctx.FormInt64("id")); err != nil { + if err := issues_model.DeleteRepoLabel(ctx, ctx.Repo.Repository.ID, ctx.FormInt64("id")); err != nil { ctx.Flash.Error("DeleteLabel: " + err.Error()) } else { ctx.Flash.Success(ctx.Tr("repo.issues.label_deletion_success")) diff --git a/services/release/release_test.go b/services/release/release_test.go index f45ab2030e5..35a14665552 100644 --- a/services/release/release_test.go +++ b/services/release/release_test.go @@ -252,6 +252,7 @@ func TestRelease_Update(t *testing.T) { } assert.NoError(t, CreateRelease(t.Context(), gitRepo, release, nil, "")) assert.Positive(t, release.ID) + assert.Empty(t, release.Attachments) release.IsDraft = false tagName := release.TagName @@ -271,7 +272,10 @@ func TestRelease_Update(t *testing.T) { assert.NoError(t, err) assert.NoError(t, UpdateRelease(t.Context(), user, gitRepo, release, []string{attach.UUID}, nil, nil)) - assert.NoError(t, repo_model.GetReleaseAttachments(t.Context(), release)) + unsavedRelease := &repo_model.Release{} + release.Attachments = nil + assert.NoError(t, repo_model.GetReleaseAttachments(t.Context(), unsavedRelease, release)) + assert.Empty(t, unsavedRelease.Attachments) assert.Len(t, release.Attachments, 1) assert.Equal(t, attach.UUID, release.Attachments[0].UUID) assert.Equal(t, release.ID, release.Attachments[0].ReleaseID)