Fix migration v292 (#30153)

Fix https://github.com/go-gitea/gitea/pull/29874#discussion_r1542227686

- The migration of v292 will miss many projects. These projects will
have no default board. This PR introduced a new migration number and
removed v292 migration.

- This PR also added the missed transactions on project-related
operations.

- Only `SetDefaultBoard` will remove duplicated defaults but not in
`GetDefaultBoard`

models/migrations/migrations.go

@@ -569,6 +569,8 @@ var migrations = []Migration{
 	// v291 -> v292
 	NewMigration("Add Index to attachment.comment_id", v1_22.AddCommentIDIndexofAttachment),
 	// v292 -> v293
+	NewMigration("Ensure every project has exactly one default column - No Op", noopMigration),
+	// v293 -> v294
 	NewMigration("Ensure every project has exactly one default column", v1_22.CheckProjectColumnsConsistency),
 }
 

models/migrations/v1_22/v292.go

@@ -3,83 +3,7 @@
 
 package v1_22 //nolint
 
-import (
+// NOTE: noop the original migration has bug which some projects will be skip, so
-	"code.gitea.io/gitea/models/project"
+// these projects will have no default board.
-	"code.gitea.io/gitea/modules/setting"
+// So that this migration will be skipped and go to v293.go
-
+// This file is a placeholder so that readers can know what happened
-	"xorm.io/builder"
-	"xorm.io/xorm"
-)
-
-// CheckProjectColumnsConsistency ensures there is exactly one default board per project present
-func CheckProjectColumnsConsistency(x *xorm.Engine) error {
-	sess := x.NewSession()
-	defer sess.Close()
-
-	if err := sess.Begin(); err != nil {
-		return err
-	}
-
-	limit := setting.Database.IterateBufferSize
-	if limit <= 0 {
-		limit = 50
-	}
-
-	start := 0
-
-	for {
-		var projects []project.Project
-		if err := sess.SQL("SELECT DISTINCT `p`.`id`, `p`.`creator_id` FROM `project` `p` WHERE (SELECT COUNT(*) FROM `project_board` `pb` WHERE `pb`.`project_id` = `p`.`id` AND `pb`.`default` = ?) != 1", true).
-			Limit(limit, start).
-			Find(&projects); err != nil {
-			return err
-		}
-
-		if len(projects) == 0 {
-			break
-		}
-		start += len(projects)
-
-		for _, p := range projects {
-			var boards []project.Board
-			if err := sess.Where("project_id=? AND `default` = ?", p.ID, true).OrderBy("sorting").Find(&boards); err != nil {
-				return err
-			}
-
-			if len(boards) == 0 {
-				if _, err := sess.Insert(project.Board{
-					ProjectID: p.ID,
-					Default:   true,
-					Title:     "Uncategorized",
-					CreatorID: p.CreatorID,
-				}); err != nil {
-					return err
-				}
-				continue
-			}
-
-			var boardsToUpdate []int64
-			for id, b := range boards {
-				if id > 0 {
-					boardsToUpdate = append(boardsToUpdate, b.ID)
-				}
-			}
-
-			if _, err := sess.Where(builder.Eq{"project_id": p.ID}.And(builder.In("id", boardsToUpdate))).
-				Cols("`default`").Update(&project.Board{Default: false}); err != nil {
-				return err
-			}
-		}
-
-		if start%1000 == 0 {
-			if err := sess.Commit(); err != nil {
-				return err
-			}
-			if err := sess.Begin(); err != nil {
-				return err
-			}
-		}
-	}
-
-	return sess.Commit()
-}

models/migrations/v1_22/v293.go

@@ -0,0 +1,108 @@
+// Copyright 2024 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package v1_22 //nolint
+
+import (
+	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/timeutil"
+
+	"xorm.io/xorm"
+)
+
+// CheckProjectColumnsConsistency ensures there is exactly one default board per project present
+func CheckProjectColumnsConsistency(x *xorm.Engine) error {
+	sess := x.NewSession()
+	defer sess.Close()
+
+	limit := setting.Database.IterateBufferSize
+	if limit <= 0 {
+		limit = 50
+	}
+
+	type Project struct {
+		ID        int64
+		CreatorID int64
+		BoardID   int64
+	}
+
+	type ProjectBoard struct {
+		ID      int64 `xorm:"pk autoincr"`
+		Title   string
+		Default bool   `xorm:"NOT NULL DEFAULT false"` // issues not assigned to a specific board will be assigned to this board
+		Sorting int8   `xorm:"NOT NULL DEFAULT 0"`
+		Color   string `xorm:"VARCHAR(7)"`
+
+		ProjectID int64 `xorm:"INDEX NOT NULL"`
+		CreatorID int64 `xorm:"NOT NULL"`
+
+		CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"`
+		UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"`
+	}
+
+	for {
+		if err := sess.Begin(); err != nil {
+			return err
+		}
+
+		// all these projects without defaults will be fixed in the same loop, so
+		// we just need to always get projects without defaults until no such project
+		var projects []*Project
+		if err := sess.Select("project.id as id, project.creator_id, project_board.id as board_id").
+			Join("LEFT", "project_board", "project_board.project_id = project.id AND project_board.`default`=?", true).
+			Where("project_board.id is NULL OR project_board.id = 0").
+			Limit(limit).
+			Find(&projects); err != nil {
+			return err
+		}
+
+		for _, p := range projects {
+			if _, err := sess.Insert(ProjectBoard{
+				ProjectID: p.ID,
+				Default:   true,
+				Title:     "Uncategorized",
+				CreatorID: p.CreatorID,
+			}); err != nil {
+				return err
+			}
+		}
+		if err := sess.Commit(); err != nil {
+			return err
+		}
+
+		if len(projects) == 0 {
+			break
+		}
+	}
+	sess.Close()
+
+	return removeDuplicatedBoardDefault(x)
+}
+
+func removeDuplicatedBoardDefault(x *xorm.Engine) error {
+	type ProjectInfo struct {
+		ProjectID  int64
+		DefaultNum int
+	}
+	var projects []ProjectInfo
+	if err := x.Select("project_id, count(*) AS default_num").
+		Table("project_board").
+		Where("`default` = ?", true).
+		GroupBy("project_id").
+		Having("count(*) > 1").
+		Find(&projects); err != nil {
+		return err
+	}
+
+	for _, project := range projects {
+		if _, err := x.Where("project_id=?", project.ProjectID).
+			Table("project_board").
+			Limit(project.DefaultNum - 1).
+			Update(map[string]bool{
+				"`default`": false,
+			}); err != nil {
+			return err
+		}
+	}
+	return nil
+}

models/migrations/v1_22/v293_test.go

@@ -31,14 +31,14 @@ func Test_CheckProjectColumnsConsistency(t *testing.T) {
 	assert.Equal(t, int64(1), defaultBoard.ProjectID)
 	assert.True(t, defaultBoard.Default)
 
-	// check if multiple defaults were removed
+	// check if multiple defaults, previous were removed and last will be kept
 	expectDefaultBoard, err := project.GetBoard(db.DefaultContext, 2)
 	assert.NoError(t, err)
 	assert.Equal(t, int64(2), expectDefaultBoard.ProjectID)
-	assert.True(t, expectDefaultBoard.Default)
+	assert.False(t, expectDefaultBoard.Default)
 
 	expectNonDefaultBoard, err := project.GetBoard(db.DefaultContext, 3)
 	assert.NoError(t, err)
 	assert.Equal(t, int64(2), expectNonDefaultBoard.ProjectID)
-	assert.False(t, expectNonDefaultBoard.Default)
+	assert.True(t, expectNonDefaultBoard.Default)
 }

models/project/board.go

@@ -209,7 +209,6 @@ func deleteBoardByProjectID(ctx context.Context, projectID int64) error {
 // GetBoard fetches the current board of a project
 func GetBoard(ctx context.Context, boardID int64) (*Board, error) {
 	board := new(Board)
-
 	has, err := db.GetEngine(ctx).ID(boardID).Get(board)
 	if err != nil {
 		return nil, err
@@ -260,45 +259,34 @@ func (p *Project) GetBoards(ctx context.Context) (BoardList, error) {
 
 // getDefaultBoard return default board and ensure only one exists
 func (p *Project) getDefaultBoard(ctx context.Context) (*Board, error) {
-	var boards []Board
+	var board Board
-	if err := db.GetEngine(ctx).Where("project_id=? AND `default` = ?", p.ID, true).OrderBy("sorting").Find(&boards); err != nil {
+	has, err := db.GetEngine(ctx).
+		Where("project_id=? AND `default` = ?", p.ID, true).
+		Desc("id").Get(&board)
+	if err != nil {
 		return nil, err
 	}
 
+	if has {
+		return &board, nil
+	}
+
 	// create a default board if none is found
-	if len(boards) == 0 {
+	board = Board{
-		board := Board{
 		ProjectID: p.ID,
 		Default:   true,
 		Title:     "Uncategorized",
 		CreatorID: p.CreatorID,
 	}
-		if _, err := db.GetEngine(ctx).Insert(); err != nil {
+	if _, err := db.GetEngine(ctx).Insert(&board); err != nil {
 		return nil, err
 	}
 	return &board, nil
-	}
-
-	// unset default boards where too many default boards exist
-	if len(boards) > 1 {
-		var boardsToUpdate []int64
-		for id, b := range boards {
-			if id > 0 {
-				boardsToUpdate = append(boardsToUpdate, b.ID)
-			}
-		}
-
-		if _, err := db.GetEngine(ctx).Where(builder.Eq{"project_id": p.ID}.And(builder.In("id", boardsToUpdate))).
-			Cols("`default`").Update(&Board{Default: false}); err != nil {
-			return nil, err
-		}
-	}
-
-	return &boards[0], nil
 }
 
 // SetDefaultBoard represents a board for issues not assigned to one
 func SetDefaultBoard(ctx context.Context, projectID, boardID int64) error {
+	return db.WithTx(ctx, func(ctx context.Context) error {
 		if _, err := GetBoard(ctx, boardID); err != nil {
 			return err
 		}
@@ -310,21 +298,23 @@ func SetDefaultBoard(ctx context.Context, projectID, boardID int64) error {
 			return err
 		}
 
-	_, err := db.GetEngine(ctx).ID(boardID).Where(builder.Eq{"project_id": projectID}).
+		_, err := db.GetEngine(ctx).ID(boardID).
+			Where(builder.Eq{"project_id": projectID}).
 			Cols("`default`").Update(&Board{Default: true})
-
 		return err
+	})
 }
 
 // UpdateBoardSorting update project board sorting
 func UpdateBoardSorting(ctx context.Context, bs BoardList) error {
+	return db.WithTx(ctx, func(ctx context.Context) error {
 		for i := range bs {
-		_, err := db.GetEngine(ctx).ID(bs[i].ID).Cols(
+			if _, err := db.GetEngine(ctx).ID(bs[i].ID).Cols(
 				"sorting",
-		).Update(bs[i])
+			).Update(bs[i]); err != nil {
-		if err != nil {
 				return err
 			}
 		}
 		return nil
+	})
 }

models/project/board_test.go

@@ -31,8 +31,12 @@ func TestGetDefaultBoard(t *testing.T) {
 	board, err = projectWithMultipleDefaults.getDefaultBoard(db.DefaultContext)
 	assert.NoError(t, err)
 	assert.Equal(t, int64(6), board.ProjectID)
-	assert.Equal(t, int64(8), board.ID)
+	assert.Equal(t, int64(9), board.ID)
 
+	// set 8 as default board
+	assert.NoError(t, SetDefaultBoard(db.DefaultContext, board.ProjectID, 8))
+
+	// then 9 will become a non-default board
 	board, err = GetBoard(db.DefaultContext, 9)
 	assert.NoError(t, err)
 	assert.Equal(t, int64(6), board.ProjectID)