From 2d4a3e56580d2d939f1eee7e5047b82a9959d42e Mon Sep 17 00:00:00 2001 From: Myers Carpenter Date: Fri, 27 Feb 2026 16:25:20 +0100 Subject: [PATCH] fix(models): deduplicate project sorting values and add unique constraints (#11334) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Project columns and cards use a sorting field for ordering, but nothing prevents duplicate values from being inserted. This causes unpredictable ordering and makes swap-based reordering unsafe. Additionally, the same issue can be added to a project multiple times, which shouldn't be possible. This PR adds a migration to clean up existing data and enforce three unique constraints: - (project_id, sorting) on project_board — one sorting value per column per project - (project_id, issue_id) on project_issue — one card per issue per project - (project_board_id, sorting) on project_issue — one sorting value per card per column The migration deduplicates existing rows and reassigns sequential sorting values before adding the constraints. Changes - Migration: fix duplicate sorting values in project_board and project_issue, remove duplicate (project_id, issue_id) rows, add three unique constraints - MoveColumnsOnProject: two-phase swap (negate then set) to avoid constraint collisions - MoveIssuesOnProjectColumn: three-phase approach with duplicate validation and sorted lock ordering - UpdateColumn: always persist sorting field (allows setting to 0) - GetDefaultColumn: query max sorting before auto-creating - createDefaultColumnsForProject: explicit sequential sorting - changeProjectStatus: only set ClosedDateUnix when closing ## Checklist The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org). ### Tests - I added test coverage for Go changes... - [x] in their respective `*_test.go` for unit tests. - [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I added test coverage for JavaScript changes... - [ ] in `web_src/js/*.test.js` if it can be unit tested. - [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)). ### Documentation - [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [x] I did not document these changes and I do not expect someone else to do it. ### Release notes - [ ] This change will be noticed by a Forgejo user or admin (feature, bug fix, performance, etc.). I suggest to include a release note for this change. - [x] This change is not visible to a Forgejo user or admin (refactor, dependency upgrade, etc.). I think there is no need to add a release note for this change. *The decision if the pull request will be shown in the release notes is up to the mergers / release team.* The content of the `release-notes/.md` file will serve as the basis for the release notes. If the file does not exist, the title of the pull request will be used instead. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11334 Reviewed-by: Mathieu Fenniak Co-authored-by: Myers Carpenter Co-committed-by: Myers Carpenter --- .../PrivateIssueProjects/project_board.yml | 2 + .../PrivateIssueProjects/project_issue.yml | 4 + .../TestPrivateRepoProjects/project_board.yml | 1 + .../TestPrivateRepoProjects/project_issue.yml | 2 + models/fixtures/project_board.yml | 9 + models/fixtures/project_issue.yml | 4 + ..._fix-project-sorting-unique-constraints.go | 218 ++++++++++++++++++ .../project_board.yml | 3 + models/project/column.go | 69 ++++-- models/project/column_test.go | 59 ++++- models/project/issue.go | 79 ++++++- models/project/issue_test.go | 149 ++++++++++++ 12 files changed, 574 insertions(+), 25 deletions(-) create mode 100644 models/forgejo_migrations/v15c_fix-project-sorting-unique-constraints.go create mode 100644 models/project/issue_test.go diff --git a/models/fixtures/PrivateIssueProjects/project_board.yml b/models/fixtures/PrivateIssueProjects/project_board.yml index 3f1fe1e705..a1c257dacc 100644 --- a/models/fixtures/PrivateIssueProjects/project_board.yml +++ b/models/fixtures/PrivateIssueProjects/project_board.yml @@ -4,6 +4,7 @@ title: Triage creator_id: 2 default: true + sorting: 0 created_unix: 1738000000 updated_unix: 1738000000 @@ -13,5 +14,6 @@ title: Triage creator_id: 2 default: true + sorting: 0 created_unix: 1738000000 updated_unix: 1738000000 diff --git a/models/fixtures/PrivateIssueProjects/project_issue.yml b/models/fixtures/PrivateIssueProjects/project_issue.yml index 0245fb47f3..edbaa9640b 100644 --- a/models/fixtures/PrivateIssueProjects/project_issue.yml +++ b/models/fixtures/PrivateIssueProjects/project_issue.yml @@ -3,21 +3,25 @@ issue_id: 6 project_id: 1001 project_board_id: 1001 + sorting: 1 - id: 1002 issue_id: 7 project_id: 1002 project_board_id: 1002 + sorting: 0 - id: 1003 issue_id: 16 project_id: 1001 project_board_id: 1001 + sorting: 0 - id: 1004 issue_id: 1 project_id: 1002 project_board_id: 1002 + sorting: 1 diff --git a/models/fixtures/TestPrivateRepoProjects/project_board.yml b/models/fixtures/TestPrivateRepoProjects/project_board.yml index 9829cf7e27..68121387b9 100644 --- a/models/fixtures/TestPrivateRepoProjects/project_board.yml +++ b/models/fixtures/TestPrivateRepoProjects/project_board.yml @@ -4,5 +4,6 @@ title: Triage creator_id: 2 default: true + sorting: 0 created_unix: 1738000000 updated_unix: 1738000000 diff --git a/models/fixtures/TestPrivateRepoProjects/project_issue.yml b/models/fixtures/TestPrivateRepoProjects/project_issue.yml index 3e8c1dca9e..aa3082af27 100644 --- a/models/fixtures/TestPrivateRepoProjects/project_issue.yml +++ b/models/fixtures/TestPrivateRepoProjects/project_issue.yml @@ -3,9 +3,11 @@ issue_id: 6 project_id: 1001 project_board_id: 1001 + sorting: 1 - id: 1002 issue_id: 15 project_id: 1001 project_board_id: 1001 + sorting: 0 diff --git a/models/fixtures/project_board.yml b/models/fixtures/project_board.yml index 3293dea6ed..e205c01a21 100644 --- a/models/fixtures/project_board.yml +++ b/models/fixtures/project_board.yml @@ -4,6 +4,7 @@ title: To Do creator_id: 2 default: true + sorting: 0 created_unix: 1588117528 updated_unix: 1588117528 @@ -12,6 +13,7 @@ project_id: 1 title: In Progress creator_id: 2 + sorting: 1 created_unix: 1588117528 updated_unix: 1588117528 @@ -20,6 +22,7 @@ project_id: 1 title: Done creator_id: 2 + sorting: 2 created_unix: 1588117528 updated_unix: 1588117528 @@ -28,6 +31,7 @@ project_id: 4 title: Done creator_id: 2 + sorting: 0 created_unix: 1588117528 updated_unix: 1588117528 @@ -37,6 +41,7 @@ title: Backlog creator_id: 2 default: true + sorting: 0 created_unix: 1588117528 updated_unix: 1588117528 @@ -46,6 +51,7 @@ title: Backlog creator_id: 2 default: true + sorting: 1 created_unix: 1588117528 updated_unix: 1588117528 @@ -55,6 +61,7 @@ title: Done creator_id: 2 default: false + sorting: 0 created_unix: 1588117528 updated_unix: 1588117528 @@ -64,6 +71,7 @@ title: Backlog creator_id: 2 default: true + sorting: 0 created_unix: 1588117528 updated_unix: 1588117528 @@ -73,5 +81,6 @@ title: Uncategorized creator_id: 2 default: true + sorting: 1 created_unix: 1588117528 updated_unix: 1588117528 diff --git a/models/fixtures/project_issue.yml b/models/fixtures/project_issue.yml index b1af05908a..61ad79cd3e 100644 --- a/models/fixtures/project_issue.yml +++ b/models/fixtures/project_issue.yml @@ -3,21 +3,25 @@ issue_id: 1 project_id: 1 project_board_id: 1 + sorting: 0 - id: 2 issue_id: 2 project_id: 1 project_board_id: 0 # no board assigned + sorting: 0 - id: 3 issue_id: 3 project_id: 1 project_board_id: 2 + sorting: 0 - id: 4 issue_id: 5 project_id: 1 project_board_id: 3 + sorting: 0 diff --git a/models/forgejo_migrations/v15c_fix-project-sorting-unique-constraints.go b/models/forgejo_migrations/v15c_fix-project-sorting-unique-constraints.go new file mode 100644 index 0000000000..9fc28d523c --- /dev/null +++ b/models/forgejo_migrations/v15c_fix-project-sorting-unique-constraints.go @@ -0,0 +1,218 @@ +// Copyright 2025 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package forgejo_migrations + +import ( + "fmt" + + "forgejo.org/models/gitea_migrations/base" + "forgejo.org/modules/setting" + + "xorm.io/xorm" +) + +func init() { + registerMigration(&Migration{ + Description: "Fix duplicate project sorting values and add unique constraints", + Upgrade: fixProjectSortingUniqueConstraints, + }) +} + +func fixProjectSortingUniqueConstraints(x *xorm.Engine) error { + // Step 1: Fix existing duplicates in project_issue (cards) + // Reassign sequential sorting values within each column + if err := fixProjectIssueDuplicates(x); err != nil { + return err + } + + // Step 2: Fix existing duplicates in project_board (columns) + // Reassign sequential sorting values within each project + if err := fixProjectBoardDuplicates(x); err != nil { + return err + } + + // Step 3: Remove duplicate (project_id, issue_id) rows keeping the lowest id + if err := fixProjectIssueDuplicateAssignments(x); err != nil { + return err + } + + // Step 4: Add unique constraints (idempotent — skip if already exists) + if err := createUniqueIndexIfNotExists(x, "project_issue", "UQE_project_issue_column_sorting", "project_board_id, sorting"); err != nil { + return err + } + if err := createUniqueIndexIfNotExists(x, "project_issue", "UQE_project_issue_project_issue", "project_id, issue_id"); err != nil { + return err + } + if err := createUniqueIndexIfNotExists(x, "project_board", "UQE_project_board_project_sorting", "project_id, sorting"); err != nil { + return err + } + + // Step 5: Enforce NOT NULL on project_issue columns. + // The struct tags declare NOT NULL but the DB may not enforce it. + // On SQLite, RecreateTables rebuilds the table with NOT NULL and unique constraints. + return enforceProjectIssueNotNull(x) +} + +func createUniqueIndexIfNotExists(x *xorm.Engine, tableName, indexName, columns string) error { + exists, err := indexExists(x, tableName, indexName) + if err != nil { + return err + } + if exists { + return nil + } + _, err = x.Exec(fmt.Sprintf("CREATE UNIQUE INDEX %s ON %s (%s)", indexName, tableName, columns)) + return err +} + +func fixProjectIssueDuplicates(x *xorm.Engine) error { + switch { + case setting.Database.Type.IsSQLite3(): + // SQLite: Use UPDATE with subquery + _, err := x.Exec(` + UPDATE project_issue SET sorting = ( + SELECT new_sort FROM ( + SELECT id, ROW_NUMBER() OVER (PARTITION BY project_board_id ORDER BY sorting, id) - 1 as new_sort + FROM project_issue + ) ranked WHERE ranked.id = project_issue.id + ) + `) + return err + + case setting.Database.Type.IsPostgreSQL(): + // PostgreSQL: Use UPDATE FROM with subquery + _, err := x.Exec(` + UPDATE project_issue pi SET sorting = ranked.new_sort + FROM ( + SELECT id, ROW_NUMBER() OVER (PARTITION BY project_board_id ORDER BY sorting, id) - 1 as new_sort + FROM project_issue + ) ranked + WHERE pi.id = ranked.id + `) + return err + + case setting.Database.Type.IsMySQL(): + // MySQL: Use UPDATE with JOIN + _, err := x.Exec(` + UPDATE project_issue pi + INNER JOIN ( + SELECT id, ROW_NUMBER() OVER (PARTITION BY project_board_id ORDER BY sorting, id) - 1 as new_sort + FROM project_issue + ) ranked ON pi.id = ranked.id + SET pi.sorting = ranked.new_sort + `) + return err + + default: + return fmt.Errorf("unsupported db dialect type %v", x.Dialect().URI().DBType) + } +} + +func fixProjectBoardDuplicates(x *xorm.Engine) error { + switch { + case setting.Database.Type.IsSQLite3(): + // SQLite: Use UPDATE with subquery + _, err := x.Exec(` + UPDATE project_board SET sorting = ( + SELECT new_sort FROM ( + SELECT id, ROW_NUMBER() OVER (PARTITION BY project_id ORDER BY sorting, id) - 1 as new_sort + FROM project_board + ) ranked WHERE ranked.id = project_board.id + ) + `) + return err + + case setting.Database.Type.IsPostgreSQL(): + // PostgreSQL: Use UPDATE FROM with subquery + _, err := x.Exec(` + UPDATE project_board pb SET sorting = ranked.new_sort + FROM ( + SELECT id, ROW_NUMBER() OVER (PARTITION BY project_id ORDER BY sorting, id) - 1 as new_sort + FROM project_board + ) ranked + WHERE pb.id = ranked.id + `) + return err + + case setting.Database.Type.IsMySQL(): + // MySQL: Use UPDATE with JOIN + _, err := x.Exec(` + UPDATE project_board pb + INNER JOIN ( + SELECT id, ROW_NUMBER() OVER (PARTITION BY project_id ORDER BY sorting, id) - 1 as new_sort + FROM project_board + ) ranked ON pb.id = ranked.id + SET pb.sorting = ranked.new_sort + `) + return err + + default: + return fmt.Errorf("unsupported db dialect type %v", x.Dialect().URI().DBType) + } +} + +func enforceProjectIssueNotNull(x *xorm.Engine) error { + switch { + case setting.Database.Type.IsSQLite3(): + type ProjectIssue struct { + ID int64 `xorm:"pk autoincr"` + IssueID int64 `xorm:"INDEX NOT NULL unique(project_issue)"` + ProjectID int64 `xorm:"INDEX NOT NULL unique(project_issue)"` + ProjectColumnID int64 `xorm:"'project_board_id' INDEX NOT NULL unique(column_sorting)"` + Sorting int64 `xorm:"NOT NULL DEFAULT 0 unique(column_sorting)"` + } + return base.RecreateTables(new(ProjectIssue))(x) + + case setting.Database.Type.IsPostgreSQL(): + for _, col := range []string{"issue_id", "project_id", "project_board_id"} { + if _, err := x.Exec(fmt.Sprintf("ALTER TABLE project_issue ALTER COLUMN %s SET NOT NULL", col)); err != nil { + return err + } + } + return nil + + case setting.Database.Type.IsMySQL(): + for _, col := range []string{"issue_id", "project_id", "project_board_id"} { + if _, err := x.Exec(fmt.Sprintf("ALTER TABLE project_issue MODIFY COLUMN %s BIGINT NOT NULL", col)); err != nil { + return err + } + } + return nil + + default: + return fmt.Errorf("unsupported db dialect type %v", x.Dialect().URI().DBType) + } +} + +// fixProjectIssueDuplicateAssignments removes duplicate (project_id, issue_id) rows, +// keeping only the row with the lowest id for each pair. +func fixProjectIssueDuplicateAssignments(x *xorm.Engine) error { + switch { + case setting.Database.Type.IsSQLite3(): + _, err := x.Exec(` + DELETE FROM project_issue WHERE id NOT IN ( + SELECT MIN(id) FROM project_issue GROUP BY project_id, issue_id + ) + `) + return err + + case setting.Database.Type.IsPostgreSQL(): + _, err := x.Exec(` + DELETE FROM project_issue pi USING project_issue pi2 + WHERE pi.project_id = pi2.project_id AND pi.issue_id = pi2.issue_id AND pi.id > pi2.id + `) + return err + + case setting.Database.Type.IsMySQL(): + _, err := x.Exec(` + DELETE pi FROM project_issue pi + INNER JOIN project_issue pi2 + ON pi.project_id = pi2.project_id AND pi.issue_id = pi2.issue_id AND pi.id > pi2.id + `) + return err + + default: + return fmt.Errorf("unsupported db dialect type %v", x.Dialect().URI().DBType) + } +} diff --git a/models/gitea_migrations/fixtures/Test_CheckProjectColumnsConsistency/project_board.yml b/models/gitea_migrations/fixtures/Test_CheckProjectColumnsConsistency/project_board.yml index 2e1b1c7eee..d7eab38185 100644 --- a/models/gitea_migrations/fixtures/Test_CheckProjectColumnsConsistency/project_board.yml +++ b/models/gitea_migrations/fixtures/Test_CheckProjectColumnsConsistency/project_board.yml @@ -4,6 +4,7 @@ title: Done creator_id: 2 default: false + sorting: 1 created_unix: 1588117528 updated_unix: 1588117528 @@ -13,6 +14,7 @@ title: Backlog creator_id: 2 default: true + sorting: 0 created_unix: 1588117528 updated_unix: 1588117528 @@ -22,5 +24,6 @@ title: Uncategorized creator_id: 2 default: true + sorting: 1 created_unix: 1588117528 updated_unix: 1588117528 diff --git a/models/project/column.go b/models/project/column.go index 20de39357b..858a531bfc 100644 --- a/models/project/column.go +++ b/models/project/column.go @@ -10,6 +10,7 @@ import ( "regexp" "forgejo.org/models/db" + "forgejo.org/modules/container" "forgejo.org/modules/setting" "forgejo.org/modules/timeutil" "forgejo.org/modules/util" @@ -42,10 +43,10 @@ type Column struct { ID int64 `xorm:"pk autoincr"` Title string Default bool `xorm:"NOT NULL DEFAULT false"` // issues not assigned to a specific column will be assigned to this column - Sorting int8 `xorm:"NOT NULL DEFAULT 0"` + Sorting int8 `xorm:"NOT NULL DEFAULT 0 unique(project_sorting)"` Color string `xorm:"VARCHAR(7)"` - ProjectID int64 `xorm:"INDEX NOT NULL"` + ProjectID int64 `xorm:"INDEX NOT NULL unique(project_sorting)"` CreatorID int64 `xorm:"NOT NULL"` CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"` @@ -103,8 +104,9 @@ func createDefaultColumnsForProject(ctx context.Context, project *Project) error Title: "Backlog", ProjectID: project.ID, Default: true, + Sorting: 0, } - if err := db.Insert(ctx, column); err != nil { + if err := db.Insert(ctx, &column); err != nil { return err } @@ -113,12 +115,13 @@ func createDefaultColumnsForProject(ctx context.Context, project *Project) error } columns := make([]Column, 0, len(items)) - for _, v := range items { + for i, v := range items { columns = append(columns, Column{ CreatedUnix: timeutil.TimeStampNow(), CreatorID: project.CreatorID, Title: v, ProjectID: project.ID, + Sorting: int8(i + 1), }) } @@ -215,9 +218,7 @@ func GetColumn(ctx context.Context, columnID int64) (*Column, error) { func UpdateColumn(ctx context.Context, column *Column) error { var fieldToUpdate []string - if column.Sorting != 0 { - fieldToUpdate = append(fieldToUpdate, "sorting") - } + fieldToUpdate = append(fieldToUpdate, "sorting") if column.Title != "" { fieldToUpdate = append(fieldToUpdate, "title") @@ -257,12 +258,22 @@ func (p *Project) GetDefaultColumn(ctx context.Context) (*Column, error) { return &column, nil } - // create a default column if none is found + // create a default column if none is found, using the next available sorting value + res := struct { + MaxSorting int64 + ColumnCount int64 + }{} + if _, err := db.GetEngine(ctx).Select("max(sorting) as max_sorting, count(*) as column_count"). + Table("project_board").Where("project_id=?", p.ID).Get(&res); err != nil { + return nil, err + } + column = Column{ ProjectID: p.ID, Default: true, Title: "Uncategorized", CreatorID: p.CreatorID, + Sorting: int8(util.Iif(res.ColumnCount > 0, res.MaxSorting+1, 0)), } if _, err := db.GetEngine(ctx).Insert(&column); err != nil { return nil, err @@ -305,27 +316,59 @@ func GetColumnsByIDs(ctx context.Context, projectID int64, columnsIDs []int64) ( return columns, nil } -// MoveColumnsOnProject sorts columns in a project +// MoveColumnsOnProject sorts columns in a project using a two-phase approach +// to avoid unique constraint collisions during swap operations. +// All columns in the project must be included in the sortedColumnIDs map. func MoveColumnsOnProject(ctx context.Context, project *Project, sortedColumnIDs map[int64]int64) error { return db.WithTx(ctx, func(ctx context.Context) error { sess := db.GetEngine(ctx) + + // Validate no duplicate column IDs in map values + columnIDSet := make(container.Set[int64], len(sortedColumnIDs)) + for _, columnID := range sortedColumnIDs { + if !columnIDSet.Add(columnID) { + return errors.New("duplicate column ID in reorder request") + } + } + + // Validate all columns exist and belong to this project + allColumns, err := project.GetColumns(ctx) + if err != nil { + return err + } + if len(allColumns) != len(sortedColumnIDs) { + return errors.New("all columns in the project must be included in the reorder request") + } + columnIDs := util.ValuesOfMap(sortedColumnIDs) movedColumns, err := GetColumnsByIDs(ctx, project.ID, columnIDs) if err != nil { return err } if len(movedColumns) != len(sortedColumnIDs) { - return errors.New("some columns do not exist") + return errors.New("some columns do not exist in this project") } + // Build reverse map: columnID → target sorting + targetSortingByColumn := make(map[int64]int64, len(sortedColumnIDs)) + for sorting, columnID := range sortedColumnIDs { + targetSortingByColumn[columnID] = sorting + } + + // Phase 1: negate using target sorting values (guaranteed unique since + // they are map keys) to avoid unique constraint collisions during swap for _, column := range movedColumns { - if column.ProjectID != project.ID { - return fmt.Errorf("column[%d]'s projectID is not equal to project's ID [%d]", column.ProjectID, project.ID) + targetSorting := targetSortingByColumn[column.ID] + if _, err := sess.Exec("UPDATE `project_board` SET sorting=? WHERE id=?", + -(targetSorting + 1), column.ID); err != nil { + return err } } + // Phase 2: set final values for sorting, columnID := range sortedColumnIDs { - if _, err := sess.Exec("UPDATE `project_board` SET sorting=? WHERE id=?", sorting, columnID); err != nil { + if _, err := sess.Exec("UPDATE `project_board` SET sorting=? WHERE id=?", + sorting, columnID); err != nil { return err } } diff --git a/models/project/column_test.go b/models/project/column_test.go index 4fc452df33..aef7a6f9d4 100644 --- a/models/project/column_test.go +++ b/models/project/column_test.go @@ -84,9 +84,9 @@ func Test_MoveColumnsOnProject(t *testing.T) { columns, err := project1.GetColumns(db.DefaultContext) require.NoError(t, err) assert.Len(t, columns, 3) - assert.EqualValues(t, 0, columns[0].Sorting) // even if there is no default sorting, the code should also work - assert.EqualValues(t, 0, columns[1].Sorting) - assert.EqualValues(t, 0, columns[2].Sorting) + assert.EqualValues(t, 0, columns[0].Sorting) + assert.EqualValues(t, 1, columns[1].Sorting) + assert.EqualValues(t, 2, columns[2].Sorting) err = MoveColumnsOnProject(db.DefaultContext, project1, map[int64]int64{ 0: columns[1].ID, @@ -103,6 +103,59 @@ func Test_MoveColumnsOnProject(t *testing.T) { assert.Equal(t, columns[0].ID, columnsAfter[2].ID) } +func TestMoveColumnsOnProjectSwap(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + project1 := unittest.AssertExistsAndLoadBean(t, &Project{ID: 1}) + columns, err := project1.GetColumns(db.DefaultContext) + require.NoError(t, err) + require.Len(t, columns, 3) + + // First give them distinct positions + err = MoveColumnsOnProject(db.DefaultContext, project1, map[int64]int64{ + 0: columns[0].ID, + 1: columns[1].ID, + 2: columns[2].ID, + }) + require.NoError(t, err) + + // Now swap columns 0 and 1 (would collide under single-phase update) + err = MoveColumnsOnProject(db.DefaultContext, project1, map[int64]int64{ + 0: columns[1].ID, + 1: columns[0].ID, + 2: columns[2].ID, + }) + require.NoError(t, err) + + columnsAfter, err := project1.GetColumns(db.DefaultContext) + require.NoError(t, err) + assert.Len(t, columnsAfter, 3) + assert.Equal(t, columns[1].ID, columnsAfter[0].ID) + assert.Equal(t, columns[0].ID, columnsAfter[1].ID) + assert.Equal(t, columns[2].ID, columnsAfter[2].ID) +} + +func TestUpdateColumnSortingZero(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + column := unittest.AssertExistsAndLoadBean(t, &Column{ID: 1}) + column.Sorting = 5 + require.NoError(t, UpdateColumn(db.DefaultContext, column)) + + // Verify it was set to 5 + updated, err := GetColumn(db.DefaultContext, column.ID) + require.NoError(t, err) + assert.Equal(t, int8(5), updated.Sorting) + + // Now set it back to 0 + column.Sorting = 0 + require.NoError(t, UpdateColumn(db.DefaultContext, column)) + + updated, err = GetColumn(db.DefaultContext, column.ID) + require.NoError(t, err) + assert.Equal(t, int8(0), updated.Sorting) +} + func Test_NewColumn(t *testing.T) { require.NoError(t, unittest.PrepareTestDatabase()) diff --git a/models/project/issue.go b/models/project/issue.go index d404033446..41caac991a 100644 --- a/models/project/issue.go +++ b/models/project/issue.go @@ -6,6 +6,7 @@ package project import ( "context" "errors" + "slices" "forgejo.org/models/db" "forgejo.org/modules/log" @@ -15,14 +16,14 @@ import ( // ProjectIssue saves relation from issue to a project type ProjectIssue struct { //revive:disable-line:exported ID int64 `xorm:"pk autoincr"` - IssueID int64 `xorm:"INDEX"` - ProjectID int64 `xorm:"INDEX"` + IssueID int64 `xorm:"INDEX NOT NULL unique(project_issue)"` + ProjectID int64 `xorm:"INDEX NOT NULL unique(project_issue)"` // ProjectColumnID should not be zero since 1.22. If it's zero, the issue will not be displayed on UI and it might result in errors. - ProjectColumnID int64 `xorm:"'project_board_id' INDEX"` + ProjectColumnID int64 `xorm:"'project_board_id' INDEX NOT NULL unique(column_sorting)"` // the sorting order on the column - Sorting int64 `xorm:"NOT NULL DEFAULT 0"` + Sorting int64 `xorm:"NOT NULL DEFAULT 0 unique(column_sorting)"` } func init() { @@ -62,26 +63,86 @@ func (p *Project) NumOpenIssues(ctx context.Context) int { return int(c) } -// MoveIssuesOnProjectColumn moves or keeps issues in a column and sorts them inside that column +// MoveIssuesOnProjectColumn moves or keeps issues in a column and sorts them inside that column. +// The sortedIssueIDs map keys are sorting positions and values are issue IDs. +// Cards not in the map that already exist in the target column are shifted to +// positions after the highest requested sorting value. func MoveIssuesOnProjectColumn(ctx context.Context, column *Column, sortedIssueIDs map[int64]int64) error { + if len(sortedIssueIDs) == 0 { + return nil + } return db.WithTx(ctx, func(ctx context.Context) error { sess := db.GetEngine(ctx) issueIDs := util.ValuesOfMap(sortedIssueIDs) - count, err := sess.Table(new(ProjectIssue)).Where("project_id=?", column.ProjectID).In("issue_id", issueIDs).Count() + // Build reverse map: issueID → sorting and validate no duplicate issue IDs + sortingByIssue := make(map[int64]int64, len(sortedIssueIDs)) + for sorting, issueID := range sortedIssueIDs { + sortingByIssue[issueID] = sorting + } + if len(sortingByIssue) != len(sortedIssueIDs) { + return errors.New("duplicate issue IDs in reorder request") + } + + // Validate all issues exist and belong to this project + count, err := sess.Table(new(ProjectIssue)). + Where("project_id=?", column.ProjectID). + In("issue_id", issueIDs).Count() if err != nil { return err } if int(count) != len(sortedIssueIDs) { - return errors.New("all issues have to be added to a project first") + return errors.New("all issues must belong to the specified project") } - for sorting, issueID := range sortedIssueIDs { - _, err = sess.Exec("UPDATE `project_issue` SET project_board_id=?, sorting=? WHERE issue_id=?", column.ID, sorting, issueID) + // Sort issue IDs to ensure consistent lock ordering across concurrent transactions. + // This prevents deadlocks when multiple transactions update overlapping rows. + slices.Sort(issueIDs) + + // Phase 1: Negate sorting for ALL cards currently in the target column + // to free up all positive sorting positions. This prevents collisions + // when moved cards are assigned their final positions. + if _, err := sess.Exec("UPDATE `project_issue` SET sorting = -(sorting + 1) WHERE project_board_id=? AND sorting >= 0", + column.ID); err != nil { + return err + } + + // Phase 2: Move the specified cards to the target column with their + // final sorting values. Since all existing cards in the column now have + // negative sorting, there are no collisions. + for _, issueID := range issueIDs { + _, err := sess.Exec("UPDATE `project_issue` SET project_board_id=?, sorting=? WHERE project_id=? AND issue_id=?", + column.ID, sortingByIssue[issueID], column.ProjectID, issueID) if err != nil { return err } } + + // Phase 3: Re-pack any remaining cards in the column that still have + // negative sorting (these are pre-existing cards NOT in the move set). + // Assign them positions after the highest requested sorting value. + var maxSorting int64 + for sorting := range sortedIssueIDs { + if sorting > maxSorting { + maxSorting = sorting + } + } + + var remainingCards []ProjectIssue + if err := sess.Where("project_board_id=? AND sorting < 0", column.ID). + OrderBy("sorting DESC"). // original order was -(original+1), so DESC gives ascending original order + Find(&remainingCards); err != nil { + return err + } + nextSorting := maxSorting + 1 + for _, card := range remainingCards { + if _, err := sess.Exec("UPDATE `project_issue` SET sorting=? WHERE id=?", + nextSorting, card.ID); err != nil { + return err + } + nextSorting++ + } + return nil }) } diff --git a/models/project/issue_test.go b/models/project/issue_test.go new file mode 100644 index 0000000000..95a10182d3 --- /dev/null +++ b/models/project/issue_test.go @@ -0,0 +1,149 @@ +// Copyright 2020 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package project + +import ( + "testing" + + "forgejo.org/models/db" + "forgejo.org/models/unittest" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestMoveIssuesOnProjectColumn(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + // Get column 1 which belongs to project 1 and has issue 1 + column := unittest.AssertExistsAndLoadBean(t, &Column{ID: 1}) + require.Equal(t, int64(1), column.ProjectID) + + t.Run("Success", func(t *testing.T) { + // Issue 1 is in column 1 (from fixtures) + sortedIssueIDs := map[int64]int64{ + 0: 1, // sorting position 0 -> issue_id 1 + } + err := MoveIssuesOnProjectColumn(db.DefaultContext, column, sortedIssueIDs) + require.NoError(t, err) + + // Verify the sorting was updated using direct DB query + var card ProjectIssue + has, err := db.GetEngine(db.DefaultContext).Where("project_id=? AND issue_id=?", column.ProjectID, 1).Get(&card) + require.NoError(t, err) + require.True(t, has) + assert.Equal(t, int64(0), card.Sorting) + }) + + t.Run("MoveIssueFromDifferentColumn", func(t *testing.T) { + // Issue 3 is in column 2, not column 1 — but same project, so cross-column move should succeed + sortedIssueIDs := map[int64]int64{ + 0: 3, + } + err := MoveIssuesOnProjectColumn(db.DefaultContext, column, sortedIssueIDs) + require.NoError(t, err) + + // Verify the card was moved to column 1 and sorting updated + var card ProjectIssue + has, err := db.GetEngine(db.DefaultContext).Where("project_id=? AND issue_id=?", column.ProjectID, 3).Get(&card) + require.NoError(t, err) + require.True(t, has) + assert.Equal(t, column.ID, card.ProjectColumnID) + assert.Equal(t, int64(0), card.Sorting) + }) + + t.Run("ErrorIssueNotInProject", func(t *testing.T) { + // Issue 999 doesn't exist + sortedIssueIDs := map[int64]int64{ + 0: 999, + } + err := MoveIssuesOnProjectColumn(db.DefaultContext, column, sortedIssueIDs) + require.Error(t, err) + assert.Contains(t, err.Error(), "all issues must belong to the specified project") + }) +} + +func TestMoveIssuesOnProjectColumnSwap(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + column := unittest.AssertExistsAndLoadBean(t, &Column{ID: 1}) + + // Setup: insert two cards at distinct positions using direct DB inserts + card1 := &ProjectIssue{ + IssueID: 14, + ProjectID: 1, + ProjectColumnID: column.ID, + Sorting: 10, + } + card2 := &ProjectIssue{ + IssueID: 15, + ProjectID: 1, + ProjectColumnID: column.ID, + Sorting: 11, + } + _, err := db.GetEngine(db.DefaultContext).Insert(card1) + require.NoError(t, err) + _, err = db.GetEngine(db.DefaultContext).Insert(card2) + require.NoError(t, err) + + // Swap them: card at 10→11, card at 11→10 + sortedIssueIDs := map[int64]int64{ + 11: 14, // issue 14 goes to position 11 + 10: 15, // issue 15 goes to position 10 + } + err = MoveIssuesOnProjectColumn(db.DefaultContext, column, sortedIssueIDs) + require.NoError(t, err) + + var resultCard14 ProjectIssue + has, err := db.GetEngine(db.DefaultContext).Where("project_id=? AND issue_id=?", 1, 14).Get(&resultCard14) + require.NoError(t, err) + require.True(t, has) + assert.Equal(t, int64(11), resultCard14.Sorting) + + var resultCard15 ProjectIssue + has, err = db.GetEngine(db.DefaultContext).Where("project_id=? AND issue_id=?", 1, 15).Get(&resultCard15) + require.NoError(t, err) + require.True(t, has) + assert.Equal(t, int64(10), resultCard15.Sorting) +} + +func TestMoveIssuesOnProjectColumnEmptyMap(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + column := unittest.AssertExistsAndLoadBean(t, &Column{ID: 1}) + err := MoveIssuesOnProjectColumn(db.DefaultContext, column, map[int64]int64{}) + require.NoError(t, err) // empty map should be a no-op +} + +func TestMoveIssuesOnProjectColumnDuplicateIssueIDs(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + column := unittest.AssertExistsAndLoadBean(t, &Column{ID: 1}) + err := MoveIssuesOnProjectColumn(db.DefaultContext, column, map[int64]int64{ + 0: 1, + 1: 1, // duplicate issue ID + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "duplicate issue IDs") +} + +func TestMoveIssuesToAnotherColumnErrorPaths(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + t.Run("DifferentProject", func(t *testing.T) { + col1 := unittest.AssertExistsAndLoadBean(t, &Column{ID: 1, ProjectID: 1}) + col5 := unittest.AssertExistsAndLoadBean(t, &Column{ID: 5, ProjectID: 2}) + + err := col1.moveIssuesToAnotherColumn(db.DefaultContext, col5) + require.Error(t, err) + assert.Contains(t, err.Error(), "columns have to be in the same project") + }) + + t.Run("SameColumnIsNoOp", func(t *testing.T) { + col1 := unittest.AssertExistsAndLoadBean(t, &Column{ID: 1, ProjectID: 1}) + + err := col1.moveIssuesToAnotherColumn(db.DefaultContext, col1) + require.NoError(t, err) + }) +}