From 7df09e31fa2700454beecbaf3c0721e13d6086f4 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 17 Feb 2025 11:28:37 -0800 Subject: [PATCH] Move issue pin to an standalone table for querying performance (#33452) Noticed a SQL in gitea.com has a bigger load. It seems both `is_pull` and `pin_order` are not indexed columns in the database. ```SQL SELECT `id`, `repo_id`, `index`, `poster_id`, `original_author`, `original_author_id`, `name`, `content`, `content_version`, `milestone_id`, `priority`, `is_closed`, `is_pull`, `num_comments`, `ref`, `pin_order`, `deadline_unix`, `created_unix`, `updated_unix`, `closed_unix`, `is_locked`, `time_estimate` FROM `issue` WHERE (repo_id =?) AND (is_pull = 0) AND (pin_order > 0) ORDER BY pin_order ``` I came across a comment https://github.com/go-gitea/gitea/pull/24406#issuecomment-1527747296 from @delvh , which presents a more reasonable approach. Based on this, this PR will migrate all issue and pull request pin data from the `issue` table to the `issue_pin` table. This change benefits larger Gitea instances by improving scalability and performance. --------- Co-authored-by: wxiaoguang --- models/fixtures/issue_pin.yml | 6 + models/issues/issue.go | 215 ++++----------------------- models/issues/issue_list.go | 33 +++++ models/issues/issue_pin.go | 246 +++++++++++++++++++++++++++++++ models/migrations/migrations.go | 1 + models/migrations/v1_24/v313.go | 31 ++++ routers/api/v1/repo/issue_pin.go | 6 +- routers/web/repo/issue_pin.go | 47 +++--- routers/web/repo/issue_view.go | 6 +- services/convert/issue.go | 7 +- services/convert/pull.go | 7 +- services/issue/issue.go | 8 +- services/repository/delete.go | 1 + 13 files changed, 396 insertions(+), 218 deletions(-) create mode 100644 models/fixtures/issue_pin.yml create mode 100644 models/issues/issue_pin.go create mode 100644 models/migrations/v1_24/v313.go diff --git a/models/fixtures/issue_pin.yml b/models/fixtures/issue_pin.yml new file mode 100644 index 0000000000..14b7a72d84 --- /dev/null +++ b/models/fixtures/issue_pin.yml @@ -0,0 +1,6 @@ +- + id: 1 + repo_id: 2 + issue_id: 4 + is_pull: false + pin_order: 1 diff --git a/models/issues/issue.go b/models/issues/issue.go index 5d52f0dd5d..7e72bb776c 100644 --- a/models/issues/issue.go +++ b/models/issues/issue.go @@ -97,7 +97,7 @@ type Issue struct { // TODO: RemoveIssueRef: see "repo/issue/branch_selector_field.tmpl" Ref string - PinOrder int `xorm:"DEFAULT 0"` + PinOrder int `xorm:"-"` // 0 means not loaded, -1 means loaded but not pinned DeadlineUnix timeutil.TimeStamp `xorm:"INDEX"` @@ -291,6 +291,23 @@ func (issue *Issue) LoadMilestone(ctx context.Context) (err error) { return nil } +func (issue *Issue) LoadPinOrder(ctx context.Context) error { + if issue.PinOrder != 0 { + return nil + } + issuePin, err := GetIssuePin(ctx, issue) + if err != nil && !db.IsErrNotExist(err) { + return err + } + + if issuePin != nil { + issue.PinOrder = issuePin.PinOrder + } else { + issue.PinOrder = -1 + } + return nil +} + // LoadAttributes loads the attribute of this issue. func (issue *Issue) LoadAttributes(ctx context.Context) (err error) { if err = issue.LoadRepo(ctx); err != nil { @@ -330,6 +347,10 @@ func (issue *Issue) LoadAttributes(ctx context.Context) (err error) { return err } + if err = issue.LoadPinOrder(ctx); err != nil { + return err + } + if err = issue.Comments.LoadAttributes(ctx); err != nil { return err } @@ -342,6 +363,14 @@ func (issue *Issue) LoadAttributes(ctx context.Context) (err error) { return issue.loadReactions(ctx) } +// IsPinned returns if a Issue is pinned +func (issue *Issue) IsPinned() bool { + if issue.PinOrder == 0 { + setting.PanicInDevOrTesting("issue's pinorder has not been loaded") + } + return issue.PinOrder > 0 +} + func (issue *Issue) ResetAttributesLoaded() { issue.isLabelsLoaded = false issue.isMilestoneLoaded = false @@ -720,190 +749,6 @@ func (issue *Issue) HasOriginalAuthor() bool { return issue.OriginalAuthor != "" && issue.OriginalAuthorID != 0 } -var ErrIssueMaxPinReached = util.NewInvalidArgumentErrorf("the max number of pinned issues has been readched") - -// IsPinned returns if a Issue is pinned -func (issue *Issue) IsPinned() bool { - return issue.PinOrder != 0 -} - -// Pin pins a Issue -func (issue *Issue) Pin(ctx context.Context, user *user_model.User) error { - // If the Issue is already pinned, we don't need to pin it twice - if issue.IsPinned() { - return nil - } - - var maxPin int - _, err := db.GetEngine(ctx).SQL("SELECT MAX(pin_order) FROM issue WHERE repo_id = ? AND is_pull = ?", issue.RepoID, issue.IsPull).Get(&maxPin) - if err != nil { - return err - } - - // Check if the maximum allowed Pins reached - if maxPin >= setting.Repository.Issue.MaxPinned { - return ErrIssueMaxPinReached - } - - _, err = db.GetEngine(ctx).Table("issue"). - Where("id = ?", issue.ID). - Update(map[string]any{ - "pin_order": maxPin + 1, - }) - if err != nil { - return err - } - - // Add the pin event to the history - opts := &CreateCommentOptions{ - Type: CommentTypePin, - Doer: user, - Repo: issue.Repo, - Issue: issue, - } - if _, err = CreateComment(ctx, opts); err != nil { - return err - } - - return nil -} - -// UnpinIssue unpins a Issue -func (issue *Issue) Unpin(ctx context.Context, user *user_model.User) error { - // If the Issue is not pinned, we don't need to unpin it - if !issue.IsPinned() { - return nil - } - - // This sets the Pin for all Issues that come after the unpined Issue to the correct value - _, err := db.GetEngine(ctx).Exec("UPDATE issue SET pin_order = pin_order - 1 WHERE repo_id = ? AND is_pull = ? AND pin_order > ?", issue.RepoID, issue.IsPull, issue.PinOrder) - if err != nil { - return err - } - - _, err = db.GetEngine(ctx).Table("issue"). - Where("id = ?", issue.ID). - Update(map[string]any{ - "pin_order": 0, - }) - if err != nil { - return err - } - - // Add the unpin event to the history - opts := &CreateCommentOptions{ - Type: CommentTypeUnpin, - Doer: user, - Repo: issue.Repo, - Issue: issue, - } - if _, err = CreateComment(ctx, opts); err != nil { - return err - } - - return nil -} - -// PinOrUnpin pins or unpins a Issue -func (issue *Issue) PinOrUnpin(ctx context.Context, user *user_model.User) error { - if !issue.IsPinned() { - return issue.Pin(ctx, user) - } - - return issue.Unpin(ctx, user) -} - -// MovePin moves a Pinned Issue to a new Position -func (issue *Issue) MovePin(ctx context.Context, newPosition int) error { - // If the Issue is not pinned, we can't move them - if !issue.IsPinned() { - return nil - } - - if newPosition < 1 { - return fmt.Errorf("The Position can't be lower than 1") - } - - dbctx, committer, err := db.TxContext(ctx) - if err != nil { - return err - } - defer committer.Close() - - var maxPin int - _, err = db.GetEngine(dbctx).SQL("SELECT MAX(pin_order) FROM issue WHERE repo_id = ? AND is_pull = ?", issue.RepoID, issue.IsPull).Get(&maxPin) - if err != nil { - return err - } - - // If the new Position bigger than the current Maximum, set it to the Maximum - if newPosition > maxPin+1 { - newPosition = maxPin + 1 - } - - // Lower the Position of all Pinned Issue that came after the current Position - _, err = db.GetEngine(dbctx).Exec("UPDATE issue SET pin_order = pin_order - 1 WHERE repo_id = ? AND is_pull = ? AND pin_order > ?", issue.RepoID, issue.IsPull, issue.PinOrder) - if err != nil { - return err - } - - // Higher the Position of all Pinned Issues that comes after the new Position - _, err = db.GetEngine(dbctx).Exec("UPDATE issue SET pin_order = pin_order + 1 WHERE repo_id = ? AND is_pull = ? AND pin_order >= ?", issue.RepoID, issue.IsPull, newPosition) - if err != nil { - return err - } - - _, err = db.GetEngine(dbctx).Table("issue"). - Where("id = ?", issue.ID). - Update(map[string]any{ - "pin_order": newPosition, - }) - if err != nil { - return err - } - - return committer.Commit() -} - -// GetPinnedIssues returns the pinned Issues for the given Repo and type -func GetPinnedIssues(ctx context.Context, repoID int64, isPull bool) (IssueList, error) { - issues := make(IssueList, 0) - - err := db.GetEngine(ctx). - Table("issue"). - Where("repo_id = ?", repoID). - And("is_pull = ?", isPull). - And("pin_order > 0"). - OrderBy("pin_order"). - Find(&issues) - if err != nil { - return nil, err - } - - err = issues.LoadAttributes(ctx) - if err != nil { - return nil, err - } - - return issues, nil -} - -// IsNewPinAllowed returns if a new Issue or Pull request can be pinned -func IsNewPinAllowed(ctx context.Context, repoID int64, isPull bool) (bool, error) { - var maxPin int - _, err := db.GetEngine(ctx).SQL("SELECT COUNT(pin_order) FROM issue WHERE repo_id = ? AND is_pull = ? AND pin_order > 0", repoID, isPull).Get(&maxPin) - if err != nil { - return false, err - } - - return maxPin < setting.Repository.Issue.MaxPinned, nil -} - -// IsErrIssueMaxPinReached returns if the error is, that the User can't pin more Issues -func IsErrIssueMaxPinReached(err error) bool { - return err == ErrIssueMaxPinReached -} - // InsertIssues insert issues to database func InsertIssues(ctx context.Context, issues ...*Issue) error { ctx, committer, err := db.TxContext(ctx) diff --git a/models/issues/issue_list.go b/models/issues/issue_list.go index 02fd330f0a..6c74b533b3 100644 --- a/models/issues/issue_list.go +++ b/models/issues/issue_list.go @@ -506,6 +506,39 @@ func (issues IssueList) loadTotalTrackedTimes(ctx context.Context) (err error) { return nil } +func (issues IssueList) LoadPinOrder(ctx context.Context) error { + if len(issues) == 0 { + return nil + } + + issueIDs := container.FilterSlice(issues, func(issue *Issue) (int64, bool) { + return issue.ID, issue.PinOrder == 0 + }) + if len(issueIDs) == 0 { + return nil + } + issuePins, err := GetIssuePinsByIssueIDs(ctx, issueIDs) + if err != nil { + return err + } + + for _, issue := range issues { + if issue.PinOrder != 0 { + continue + } + for _, pin := range issuePins { + if pin.IssueID == issue.ID { + issue.PinOrder = pin.PinOrder + break + } + } + if issue.PinOrder == 0 { + issue.PinOrder = -1 + } + } + return nil +} + // loadAttributes loads all attributes, expect for attachments and comments func (issues IssueList) LoadAttributes(ctx context.Context) error { if _, err := issues.LoadRepositories(ctx); err != nil { diff --git a/models/issues/issue_pin.go b/models/issues/issue_pin.go new file mode 100644 index 0000000000..ae6195b05d --- /dev/null +++ b/models/issues/issue_pin.go @@ -0,0 +1,246 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package issues + +import ( + "context" + "errors" + "sort" + + "code.gitea.io/gitea/models/db" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/util" +) + +type IssuePin struct { + ID int64 `xorm:"pk autoincr"` + RepoID int64 `xorm:"UNIQUE(s) NOT NULL"` + IssueID int64 `xorm:"UNIQUE(s) NOT NULL"` + IsPull bool `xorm:"NOT NULL"` + PinOrder int `xorm:"DEFAULT 0"` +} + +var ErrIssueMaxPinReached = util.NewInvalidArgumentErrorf("the max number of pinned issues has been readched") + +// IsErrIssueMaxPinReached returns if the error is, that the User can't pin more Issues +func IsErrIssueMaxPinReached(err error) bool { + return err == ErrIssueMaxPinReached +} + +func init() { + db.RegisterModel(new(IssuePin)) +} + +func GetIssuePin(ctx context.Context, issue *Issue) (*IssuePin, error) { + pin := new(IssuePin) + has, err := db.GetEngine(ctx). + Where("repo_id = ?", issue.RepoID). + And("issue_id = ?", issue.ID).Get(pin) + if err != nil { + return nil, err + } else if !has { + return nil, db.ErrNotExist{ + Resource: "IssuePin", + ID: issue.ID, + } + } + return pin, nil +} + +func GetIssuePinsByIssueIDs(ctx context.Context, issueIDs []int64) ([]IssuePin, error) { + var pins []IssuePin + if err := db.GetEngine(ctx).In("issue_id", issueIDs).Find(&pins); err != nil { + return nil, err + } + return pins, nil +} + +// Pin pins a Issue +func PinIssue(ctx context.Context, issue *Issue, user *user_model.User) error { + return db.WithTx(ctx, func(ctx context.Context) error { + pinnedIssuesNum, err := getPinnedIssuesNum(ctx, issue.RepoID, issue.IsPull) + if err != nil { + return err + } + + // Check if the maximum allowed Pins reached + if pinnedIssuesNum >= setting.Repository.Issue.MaxPinned { + return ErrIssueMaxPinReached + } + + pinnedIssuesMaxPinOrder, err := getPinnedIssuesMaxPinOrder(ctx, issue.RepoID, issue.IsPull) + if err != nil { + return err + } + + if _, err = db.GetEngine(ctx).Insert(&IssuePin{ + RepoID: issue.RepoID, + IssueID: issue.ID, + IsPull: issue.IsPull, + PinOrder: pinnedIssuesMaxPinOrder + 1, + }); err != nil { + return err + } + + // Add the pin event to the history + _, err = CreateComment(ctx, &CreateCommentOptions{ + Type: CommentTypePin, + Doer: user, + Repo: issue.Repo, + Issue: issue, + }) + return err + }) +} + +// UnpinIssue unpins a Issue +func UnpinIssue(ctx context.Context, issue *Issue, user *user_model.User) error { + return db.WithTx(ctx, func(ctx context.Context) error { + // This sets the Pin for all Issues that come after the unpined Issue to the correct value + cnt, err := db.GetEngine(ctx).Where("issue_id=?", issue.ID).Delete(new(IssuePin)) + if err != nil { + return err + } + if cnt == 0 { + return nil + } + + // Add the unpin event to the history + _, err = CreateComment(ctx, &CreateCommentOptions{ + Type: CommentTypeUnpin, + Doer: user, + Repo: issue.Repo, + Issue: issue, + }) + return err + }) +} + +func getPinnedIssuesNum(ctx context.Context, repoID int64, isPull bool) (int, error) { + var pinnedIssuesNum int + _, err := db.GetEngine(ctx).SQL("SELECT count(pin_order) FROM issue_pin WHERE repo_id = ? AND is_pull = ?", repoID, isPull).Get(&pinnedIssuesNum) + return pinnedIssuesNum, err +} + +func getPinnedIssuesMaxPinOrder(ctx context.Context, repoID int64, isPull bool) (int, error) { + var maxPinnedIssuesMaxPinOrder int + _, err := db.GetEngine(ctx).SQL("SELECT max(pin_order) FROM issue_pin WHERE repo_id = ? AND is_pull = ?", repoID, isPull).Get(&maxPinnedIssuesMaxPinOrder) + return maxPinnedIssuesMaxPinOrder, err +} + +// MovePin moves a Pinned Issue to a new Position +func MovePin(ctx context.Context, issue *Issue, newPosition int) error { + if newPosition < 1 { + return errors.New("The Position can't be lower than 1") + } + + issuePin, err := GetIssuePin(ctx, issue) + if err != nil { + return err + } + if issuePin.PinOrder == newPosition { + return nil + } + + return db.WithTx(ctx, func(ctx context.Context) error { + if issuePin.PinOrder > newPosition { // move the issue to a lower position + _, err = db.GetEngine(ctx).Exec("UPDATE issue_pin SET pin_order = pin_order + 1 WHERE repo_id = ? AND is_pull = ? AND pin_order >= ? AND pin_order < ?", issue.RepoID, issue.IsPull, newPosition, issuePin.PinOrder) + } else { // move the issue to a higher position + // Lower the Position of all Pinned Issue that came after the current Position + _, err = db.GetEngine(ctx).Exec("UPDATE issue_pin SET pin_order = pin_order - 1 WHERE repo_id = ? AND is_pull = ? AND pin_order > ? AND pin_order <= ?", issue.RepoID, issue.IsPull, issuePin.PinOrder, newPosition) + } + if err != nil { + return err + } + + _, err = db.GetEngine(ctx). + Table("issue_pin"). + Where("id = ?", issuePin.ID). + Update(map[string]any{ + "pin_order": newPosition, + }) + return err + }) +} + +func GetPinnedIssueIDs(ctx context.Context, repoID int64, isPull bool) ([]int64, error) { + var issuePins []IssuePin + if err := db.GetEngine(ctx). + Table("issue_pin"). + Where("repo_id = ?", repoID). + And("is_pull = ?", isPull). + Find(&issuePins); err != nil { + return nil, err + } + + sort.Slice(issuePins, func(i, j int) bool { + return issuePins[i].PinOrder < issuePins[j].PinOrder + }) + + var ids []int64 + for _, pin := range issuePins { + ids = append(ids, pin.IssueID) + } + return ids, nil +} + +func GetIssuePinsByRepoID(ctx context.Context, repoID int64, isPull bool) ([]*IssuePin, error) { + var pins []*IssuePin + if err := db.GetEngine(ctx).Where("repo_id = ? AND is_pull = ?", repoID, isPull).Find(&pins); err != nil { + return nil, err + } + return pins, nil +} + +// GetPinnedIssues returns the pinned Issues for the given Repo and type +func GetPinnedIssues(ctx context.Context, repoID int64, isPull bool) (IssueList, error) { + issuePins, err := GetIssuePinsByRepoID(ctx, repoID, isPull) + if err != nil { + return nil, err + } + if len(issuePins) == 0 { + return IssueList{}, nil + } + ids := make([]int64, 0, len(issuePins)) + for _, pin := range issuePins { + ids = append(ids, pin.IssueID) + } + + issues := make(IssueList, 0, len(ids)) + if err := db.GetEngine(ctx).In("id", ids).Find(&issues); err != nil { + return nil, err + } + for _, issue := range issues { + for _, pin := range issuePins { + if pin.IssueID == issue.ID { + issue.PinOrder = pin.PinOrder + break + } + } + if (!setting.IsProd || setting.IsInTesting) && issue.PinOrder == 0 { + panic("It should not happen that a pinned Issue has no PinOrder") + } + } + sort.Slice(issues, func(i, j int) bool { + return issues[i].PinOrder < issues[j].PinOrder + }) + + if err = issues.LoadAttributes(ctx); err != nil { + return nil, err + } + + return issues, nil +} + +// IsNewPinAllowed returns if a new Issue or Pull request can be pinned +func IsNewPinAllowed(ctx context.Context, repoID int64, isPull bool) (bool, error) { + var maxPin int + _, err := db.GetEngine(ctx).SQL("SELECT COUNT(pin_order) FROM issue_pin WHERE repo_id = ? AND is_pull = ?", repoID, isPull).Get(&maxPin) + if err != nil { + return false, err + } + + return maxPin < setting.Repository.Issue.MaxPinned, nil +} diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 95364ab705..87d674a440 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -373,6 +373,7 @@ func prepareMigrationTasks() []*migration { // Gitea 1.23.0-rc0 ends at migration ID number 311 (database version 312) newMigration(312, "Add DeleteBranchAfterMerge to AutoMerge", v1_24.AddDeleteBranchAfterMergeForAutoMerge), + newMigration(313, "Move PinOrder from issue table to a new table issue_pin", v1_24.MovePinOrderToTableIssuePin), } return preparedMigrations } diff --git a/models/migrations/v1_24/v313.go b/models/migrations/v1_24/v313.go new file mode 100644 index 0000000000..ee9d479340 --- /dev/null +++ b/models/migrations/v1_24/v313.go @@ -0,0 +1,31 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package v1_24 //nolint + +import ( + "code.gitea.io/gitea/models/migrations/base" + + "xorm.io/xorm" +) + +func MovePinOrderToTableIssuePin(x *xorm.Engine) error { + type IssuePin struct { + ID int64 `xorm:"pk autoincr"` + RepoID int64 `xorm:"UNIQUE(s) NOT NULL"` + IssueID int64 `xorm:"UNIQUE(s) NOT NULL"` + IsPull bool `xorm:"NOT NULL"` + PinOrder int `xorm:"DEFAULT 0"` + } + + if err := x.Sync(new(IssuePin)); err != nil { + return err + } + + if _, err := x.Exec("INSERT INTO issue_pin (repo_id, issue_id, is_pull, pin_order) SELECT repo_id, id, is_pull, pin_order FROM issue WHERE pin_order > 0"); err != nil { + return err + } + sess := x.NewSession() + defer sess.Close() + return base.DropTableColumns(sess, "issue", "pin_order") +} diff --git a/routers/api/v1/repo/issue_pin.go b/routers/api/v1/repo/issue_pin.go index f4cbc8e762..5e55b7c2d6 100644 --- a/routers/api/v1/repo/issue_pin.go +++ b/routers/api/v1/repo/issue_pin.go @@ -60,7 +60,7 @@ func PinIssue(ctx *context.APIContext) { return } - err = issue.Pin(ctx, ctx.Doer) + err = issues_model.PinIssue(ctx, issue, ctx.Doer) if err != nil { ctx.APIError(http.StatusInternalServerError, err) return @@ -115,7 +115,7 @@ func UnpinIssue(ctx *context.APIContext) { return } - err = issue.Unpin(ctx, ctx.Doer) + err = issues_model.UnpinIssue(ctx, issue, ctx.Doer) if err != nil { ctx.APIError(http.StatusInternalServerError, err) return @@ -169,7 +169,7 @@ func MoveIssuePin(ctx *context.APIContext) { return } - err = issue.MovePin(ctx, int(ctx.PathParamInt64("position"))) + err = issues_model.MovePin(ctx, issue, int(ctx.PathParamInt64("position"))) if err != nil { ctx.APIError(http.StatusInternalServerError, err) return diff --git a/routers/web/repo/issue_pin.go b/routers/web/repo/issue_pin.go index d7d3205c37..8d3de90d25 100644 --- a/routers/web/repo/issue_pin.go +++ b/routers/web/repo/issue_pin.go @@ -6,6 +6,7 @@ package repo import ( "net/http" + "code.gitea.io/gitea/models/db" issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/modules/json" "code.gitea.io/gitea/modules/log" @@ -22,15 +23,29 @@ func IssuePinOrUnpin(ctx *context.Context) { // If we don't do this, it will crash when trying to add the pin event to the comment history err := issue.LoadRepo(ctx) if err != nil { - ctx.Status(http.StatusInternalServerError) - log.Error(err.Error()) + ctx.ServerError("LoadRepo", err) return } - err = issue.PinOrUnpin(ctx, ctx.Doer) + // PinOrUnpin pins or unpins a Issue + _, err = issues_model.GetIssuePin(ctx, issue) + if err != nil && !db.IsErrNotExist(err) { + ctx.ServerError("GetIssuePin", err) + return + } + + if db.IsErrNotExist(err) { + err = issues_model.PinIssue(ctx, issue, ctx.Doer) + } else { + err = issues_model.UnpinIssue(ctx, issue, ctx.Doer) + } + if err != nil { - ctx.Status(http.StatusInternalServerError) - log.Error(err.Error()) + if issues_model.IsErrIssueMaxPinReached(err) { + ctx.JSONError(ctx.Tr("repo.issues.max_pinned")) + } else { + ctx.ServerError("Pin/Unpin failed", err) + } return } @@ -41,23 +56,20 @@ func IssuePinOrUnpin(ctx *context.Context) { func IssueUnpin(ctx *context.Context) { issue, err := issues_model.GetIssueByIndex(ctx, ctx.Repo.Repository.ID, ctx.PathParamInt64("index")) if err != nil { - ctx.Status(http.StatusInternalServerError) - log.Error(err.Error()) + ctx.ServerError("GetIssueByIndex", err) return } // If we don't do this, it will crash when trying to add the pin event to the comment history err = issue.LoadRepo(ctx) if err != nil { - ctx.Status(http.StatusInternalServerError) - log.Error(err.Error()) + ctx.ServerError("LoadRepo", err) return } - err = issue.Unpin(ctx, ctx.Doer) + err = issues_model.UnpinIssue(ctx, issue, ctx.Doer) if err != nil { - ctx.Status(http.StatusInternalServerError) - log.Error(err.Error()) + ctx.ServerError("UnpinIssue", err) return } @@ -78,15 +90,13 @@ func IssuePinMove(ctx *context.Context) { form := &movePinIssueForm{} if err := json.NewDecoder(ctx.Req.Body).Decode(&form); err != nil { - ctx.Status(http.StatusInternalServerError) - log.Error(err.Error()) + ctx.ServerError("Decode", err) return } issue, err := issues_model.GetIssueByID(ctx, form.ID) if err != nil { - ctx.Status(http.StatusInternalServerError) - log.Error(err.Error()) + ctx.ServerError("GetIssueByID", err) return } @@ -96,10 +106,9 @@ func IssuePinMove(ctx *context.Context) { return } - err = issue.MovePin(ctx, form.Position) + err = issues_model.MovePin(ctx, issue, form.Position) if err != nil { - ctx.Status(http.StatusInternalServerError) - log.Error(err.Error()) + ctx.ServerError("MovePin", err) return } diff --git a/routers/web/repo/issue_view.go b/routers/web/repo/issue_view.go index 06a906b494..37e1b27931 100644 --- a/routers/web/repo/issue_view.go +++ b/routers/web/repo/issue_view.go @@ -543,7 +543,11 @@ func preparePullViewDeleteBranch(ctx *context.Context, issue *issues_model.Issue func prepareIssueViewSidebarPin(ctx *context.Context, issue *issues_model.Issue) { var pinAllowed bool - if !issue.IsPinned() { + if err := issue.LoadPinOrder(ctx); err != nil { + ctx.ServerError("LoadPinOrder", err) + return + } + if issue.PinOrder == 0 { var err error pinAllowed, err = issues_model.IsNewPinAllowed(ctx, issue.RepoID, issue.IsPull) if err != nil { diff --git a/services/convert/issue.go b/services/convert/issue.go index 62d0a3b3e6..7f386e6293 100644 --- a/services/convert/issue.go +++ b/services/convert/issue.go @@ -41,6 +41,9 @@ func toIssue(ctx context.Context, doer *user_model.User, issue *issues_model.Iss if err := issue.LoadAttachments(ctx); err != nil { return &api.Issue{} } + if err := issue.LoadPinOrder(ctx); err != nil { + return &api.Issue{} + } apiIssue := &api.Issue{ ID: issue.ID, @@ -55,7 +58,7 @@ func toIssue(ctx context.Context, doer *user_model.User, issue *issues_model.Iss Comments: issue.NumComments, Created: issue.CreatedUnix.AsTime(), Updated: issue.UpdatedUnix.AsTime(), - PinOrder: issue.PinOrder, + PinOrder: util.Iif(issue.PinOrder == -1, 0, issue.PinOrder), // -1 means loaded with no pin order } if issue.Repo != nil { @@ -122,6 +125,7 @@ func toIssue(ctx context.Context, doer *user_model.User, issue *issues_model.Iss // ToIssueList converts an IssueList to API format func ToIssueList(ctx context.Context, doer *user_model.User, il issues_model.IssueList) []*api.Issue { result := make([]*api.Issue, len(il)) + _ = il.LoadPinOrder(ctx) for i := range il { result[i] = ToIssue(ctx, doer, il[i]) } @@ -131,6 +135,7 @@ func ToIssueList(ctx context.Context, doer *user_model.User, il issues_model.Iss // ToAPIIssueList converts an IssueList to API format func ToAPIIssueList(ctx context.Context, doer *user_model.User, il issues_model.IssueList) []*api.Issue { result := make([]*api.Issue, len(il)) + _ = il.LoadPinOrder(ctx) for i := range il { result[i] = ToAPIIssue(ctx, doer, il[i]) } diff --git a/services/convert/pull.go b/services/convert/pull.go index 209d2bd79d..ad4f08fa91 100644 --- a/services/convert/pull.go +++ b/services/convert/pull.go @@ -93,7 +93,7 @@ func ToAPIPullRequest(ctx context.Context, pr *issues_model.PullRequest, doer *u Deadline: apiIssue.Deadline, Created: pr.Issue.CreatedUnix.AsTimePtr(), Updated: pr.Issue.UpdatedUnix.AsTimePtr(), - PinOrder: apiIssue.PinOrder, + PinOrder: util.Iif(apiIssue.PinOrder == -1, 0, apiIssue.PinOrder), // output "[]" rather than null to align to github outputs RequestedReviewers: []*api.User{}, @@ -304,6 +304,9 @@ func ToAPIPullRequests(ctx context.Context, baseRepo *repo_model.Repository, prs if err := issueList.LoadAssignees(ctx); err != nil { return nil, err } + if err = issueList.LoadPinOrder(ctx); err != nil { + return nil, err + } reviews, err := prs.LoadReviews(ctx) if err != nil { @@ -368,7 +371,7 @@ func ToAPIPullRequests(ctx context.Context, baseRepo *repo_model.Repository, prs Deadline: apiIssue.Deadline, Created: pr.Issue.CreatedUnix.AsTimePtr(), Updated: pr.Issue.UpdatedUnix.AsTimePtr(), - PinOrder: apiIssue.PinOrder, + PinOrder: util.Iif(apiIssue.PinOrder == -1, 0, apiIssue.PinOrder), AllowMaintainerEdit: pr.AllowMaintainerEdit, diff --git a/services/issue/issue.go b/services/issue/issue.go index 091b7c02d7..586b6031c8 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -197,13 +197,6 @@ func DeleteIssue(ctx context.Context, doer *user_model.User, gitRepo *git.Reposi } } - // If the Issue is pinned, we should unpin it before deletion to avoid problems with other pinned Issues - if issue.IsPinned() { - if err := issue.Unpin(ctx, doer); err != nil { - return err - } - } - notify_service.DeleteIssue(ctx, doer, issue) return nil @@ -319,6 +312,7 @@ func deleteIssue(ctx context.Context, issue *issues_model.Issue) error { &issues_model.Comment{RefIssueID: issue.ID}, &issues_model.IssueDependency{DependencyID: issue.ID}, &issues_model.Comment{DependentIssueID: issue.ID}, + &issues_model.IssuePin{IssueID: issue.ID}, ); err != nil { return err } diff --git a/services/repository/delete.go b/services/repository/delete.go index 2166b4dd5c..fb3fffdca7 100644 --- a/services/repository/delete.go +++ b/services/repository/delete.go @@ -158,6 +158,7 @@ func DeleteRepositoryDirectly(ctx context.Context, doer *user_model.User, repoID &actions_model.ActionSchedule{RepoID: repoID}, &actions_model.ActionArtifact{RepoID: repoID}, &actions_model.ActionRunnerToken{RepoID: repoID}, + &issues_model.IssuePin{RepoID: repoID}, ); err != nil { return fmt.Errorf("deleteBeans: %w", err) }