fix(kanban): upsert race condition in kanban task bucket sync (#2938)

pkg/models/kanban_task_bucket.go

@@ -22,7 +22,9 @@ import (
 	"code.vikunja.io/api/pkg/db"
 	"code.vikunja.io/api/pkg/events"
 	"code.vikunja.io/api/pkg/web"
+
 	"xorm.io/xorm"
+	"xorm.io/xorm/schemas"
 )
 
 // TaskBucket represents the relation between a task and a kanban bucket.
@@ -58,27 +60,19 @@ func (b *TaskBucket) CanUpdate(s *xorm.Session, a web.Auth) (bool, error) {
 }
 
 func (b *TaskBucket) upsert(s *xorm.Session) (err error) {
-	count, err := s.Where("task_id = ? AND project_view_id = ?", b.TaskID, b.ProjectViewID).
-		Cols("bucket_id").
-		Update(b)
-	if err != nil {
-		return
-	}
-
-	if count == 0 {
-		_, err = s.Insert(b)
-		if err != nil {
-			// Check if this is a unique constraint violation for the task_buckets table
-			if db.IsUniqueConstraintError(err, "UQE_task_buckets_task_project_view") {
-				return ErrTaskAlreadyExistsInBucket{
-					TaskID:        b.TaskID,
-					ProjectViewID: b.ProjectViewID,
-				}
-			}
-			return
-		}
+	// A native upsert moves the task in one atomic statement, without
+	// depending on the affected-row count (MySQL/MariaDB report 0 affected
+	// rows for an unchanged value).
+	onConflict := "ON CONFLICT (task_id, project_view_id) DO UPDATE SET bucket_id = excluded.bucket_id"
+	if db.Type() == schemas.MYSQL {
+		onConflict = "ON DUPLICATE KEY UPDATE bucket_id = VALUES(bucket_id)"
 	}
 
+	// Raw SQL bypasses xorm's bean-based table-name handling, so qualify the
+	// table ourselves to honor a configured postgres schema (database.schema).
+	table := s.Engine().TableName(b, true)
+	query := "INSERT INTO " + table + " (task_id, project_view_id, bucket_id) VALUES (?, ?, ?) " + onConflict
+	_, err = s.Exec(query, b.TaskID, b.ProjectViewID, b.BucketID)
 	return
 }
 
@@ -151,10 +145,8 @@ func updateTaskBucket(s *xorm.Session, a web.Auth, b *TaskBucket) (err error) {
 				if err != nil {
 					return err
 				}
-				// If the task is already in the default bucket, skip the
-				// upsert — MySQL's UPDATE returns 0 affected rows when
-				// the value is unchanged, which would make upsert fall
-				// through to INSERT and hit the unique constraint.
+				// The task is already in the default bucket, so there is
+				// nothing to move and no count to bump.
 				if b.BucketID == oldTaskBucket.BucketID {
 					updateBucket = false
 				}

pkg/models/kanban_task_bucket_test.go

@@ -226,6 +226,63 @@ func TestTaskBucket_Update(t *testing.T) {
 		})
 	})
 
+	t.Run("done task already in another view's done bucket", func(t *testing.T) {
+		// Regression test: marking a task done syncs it into the done bucket
+		// of every kanban view in the project. When the task already sits in
+		// such a view's done bucket the sync is a no-op update, but on
+		// MySQL/MariaDB an UPDATE that doesn't change the value reports 0
+		// affected rows. The upsert then mistook that for "row missing" and
+		// inserted, hitting the unique index with ErrTaskAlreadyExistsInBucket.
+		db.LoadAndAssertFixtures(t)
+		s := db.NewSession()
+		defer s.Close()
+
+		// A second manual kanban view on project 1. Creating it auto-generates
+		// the To-Do/Doing/Done buckets and sets its done bucket.
+		secondView := &ProjectView{
+			Title:                   "Second Kanban",
+			ProjectID:               1,
+			ViewKind:                ProjectViewKindKanban,
+			BucketConfigurationMode: BucketConfigurationModeManual,
+		}
+		err := secondView.Create(s, u)
+		require.NoError(t, err)
+		require.NotZero(t, secondView.DoneBucketID)
+
+		// Pre-place task 1 in the second view's done bucket without going
+		// through the done-sync, so the task itself is still open and view 4
+		// still has it in its default bucket.
+		_, err = s.Where("task_id = ? AND project_view_id = ?", 1, secondView.ID).
+			Cols("bucket_id").
+			Update(&TaskBucket{BucketID: secondView.DoneBucketID})
+		require.NoError(t, err)
+
+		// Moving task 1 into view 4's done bucket marks it done and triggers
+		// the cross-view sync into the second view's done bucket, where it
+		// already lives. This must succeed rather than error.
+		tb := &TaskBucket{
+			TaskID:        1,
+			BucketID:      3, // done bucket on view 4
+			ProjectViewID: 4,
+			ProjectID:     1,
+		}
+		err = tb.Update(s, u)
+		require.NoError(t, err)
+		err = s.Commit()
+		require.NoError(t, err)
+		assert.True(t, tb.Task.Done)
+
+		db.AssertExists(t, "task_buckets", map[string]interface{}{
+			"task_id":   1,
+			"bucket_id": 3,
+		}, false)
+		db.AssertExists(t, "task_buckets", map[string]interface{}{
+			"task_id":         1,
+			"project_view_id": secondView.ID,
+			"bucket_id":       secondView.DoneBucketID,
+		}, false)
+	})
+
 	t.Run("saved filter: first task into empty limited bucket is allowed", func(t *testing.T) {
 		// Regression test for #2672: on a saved-filter kanban view the bucket
 		// limit was checked against the total number of tasks matching the