From a61e594952c3afcbf030d9464d5e26304125f8b2 Mon Sep 17 00:00:00 2001 From: kolaente Date: Wed, 17 Jun 2026 15:52:15 +0200 Subject: [PATCH] fix(tasks): prevent duplicate task_positions rows and stale identifiers A task could end up with more than one task_positions row for the same (task_id, project_view_id): rapid/concurrent creation raced the check-then-insert paths, and the create path could insert a position that a triggered RecalculateTaskPositions had already persisted for the new task. The table had no unique constraint, so the duplicates were stored silently (#2844). In the table view this made the LEFT JOIN on task_positions emit the task twice; getTasksForProjects enriched only the map entry, so the duplicate slice row kept an empty identifier and rendered as "#N" instead of "PREFIX-N" (#2725). - Add a unique index on task_positions(task_id, project_view_id) via a dedup migration (mirrors the task_buckets fix in 20250624092830) plus the unique(task_view) struct tag so fresh installs get it too. - Harden the create path: only queue a position insert when one does not already exist for the task+view, and dedupe within the batch. - Dedupe the task slice returned by getTasksForProjects by id, returning the enriched entry, so duplicate position rows can never surface a task twice or with a missing identifier. Fixes #2844 Fixes #2725 --- pkg/migration/20260617153629.go | 120 ++++++++++++++++++++++ pkg/models/kanban.go | 2 +- pkg/models/project.go | 2 +- pkg/models/saved_filter_positions_test.go | 11 +- pkg/models/task_collection_test.go | 2 +- pkg/models/task_position.go | 38 ++++++- pkg/models/task_search_bench_test.go | 2 +- pkg/models/tasks.go | 37 +++++-- 8 files changed, 200 insertions(+), 14 deletions(-) create mode 100644 pkg/migration/20260617153629.go diff --git a/pkg/migration/20260617153629.go b/pkg/migration/20260617153629.go new file mode 100644 index 000000000..f6e0cc9d7 --- /dev/null +++ b/pkg/migration/20260617153629.go @@ -0,0 +1,120 @@ +// Vikunja is a to-do list application to facilitate your life. +// Copyright 2018-present Vikunja and contributors. All rights reserved. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package migration + +import ( + "src.techknowlogick.com/xormigrate" + "xorm.io/xorm" + "xorm.io/xorm/schemas" +) + +type taskPosition20260617153629 struct { + TaskID int64 `xorm:"bigint not null index"` + ProjectViewID int64 `xorm:"bigint not null index"` + Position float64 `xorm:"double not null"` +} + +func (taskPosition20260617153629) TableName() string { + return "task_positions" +} + +func init() { + migrations = append(migrations, &xormigrate.Migration{ + ID: "20260617153629", + Description: "deduplicate task positions and add a unique index on task_id + project_view_id", + Migrate: func(tx *xorm.Engine) error { + + s := tx.NewSession() + defer s.Close() + + err := s.Begin() + if err != nil { + return err + } + + // First remove all duplicate entries. A task may only ever have a + // single position per view; rapid task creation could race and + // insert more than one row before this constraint existed. + duplicates := []taskPosition20260617153629{} + err = s. + Select("task_id, project_view_id"). + GroupBy("task_id, project_view_id"). + Having("count(*) > 1"). + Find(&duplicates) + if err != nil { + _ = s.Rollback() + return err + } + + // Keep the lowest position of each group so the result is + // deterministic across databases. + kept := []taskPosition20260617153629{} + for _, dup := range duplicates { + row := taskPosition20260617153629{} + has, err := s. + Where("task_id = ? AND project_view_id = ?", dup.TaskID, dup.ProjectViewID). + OrderBy("position ASC"). + Get(&row) + if err != nil { + _ = s.Rollback() + return err + } + if !has { + continue + } + kept = append(kept, row) + } + + for _, dup := range duplicates { + _, err = s. + Where("task_id = ? AND project_view_id = ?", dup.TaskID, dup.ProjectViewID). + Delete(&taskPosition20260617153629{}) + if err != nil { + _ = s.Rollback() + return err + } + } + + for _, position := range kept { + _, err = s.Insert(&position) + if err != nil { + _ = s.Rollback() + return err + } + } + + err = s.Commit() + if err != nil { + return err + } + + // Then create the unique index + var query string + switch tx.Dialect().URI().DBType { + case schemas.MYSQL: + query = "CREATE UNIQUE INDEX UQE_task_positions_task_project_view ON task_positions (task_id, project_view_id)" + default: + query = "CREATE UNIQUE INDEX IF NOT EXISTS UQE_task_positions_task_project_view ON task_positions (task_id, project_view_id)" + } + _, err = tx.Exec(query) + return err + }, + Rollback: func(_ *xorm.Engine) error { + return nil + }, + }) +} diff --git a/pkg/models/kanban.go b/pkg/models/kanban.go index 01a6a145f..29b0b5705 100644 --- a/pkg/models/kanban.go +++ b/pkg/models/kanban.go @@ -257,7 +257,7 @@ func GetTasksInBucketsForView(s *xorm.Session, view *ProjectView, projects []*Pr } } - ts, _, total, err := getRawTasksForProjects(s, projects, auth, opts) + ts, total, err := getRawTasksForProjects(s, projects, auth, opts) if err != nil { return nil, err } diff --git a/pkg/models/project.go b/pkg/models/project.go index a799d1815..554f9d920 100644 --- a/pkg/models/project.go +++ b/pkg/models/project.go @@ -1370,7 +1370,7 @@ func (p *Project) Delete(s *xorm.Session, a web.Auth) (err error) { // Delete all tasks on that project // Using the loop to make sure all related entities to all tasks are properly deleted as well. - tasks, _, _, err := getRawTasksForProjects(s, []*Project{p}, a, &taskSearchOptions{}) + tasks, _, err := getRawTasksForProjects(s, []*Project{p}, a, &taskSearchOptions{}) if err != nil { return } diff --git a/pkg/models/saved_filter_positions_test.go b/pkg/models/saved_filter_positions_test.go index d233460d2..91bf09d9c 100644 --- a/pkg/models/saved_filter_positions_test.go +++ b/pkg/models/saved_filter_positions_test.go @@ -79,8 +79,17 @@ func TestCronInsertsNonZeroPosition(t *testing.T) { require.NoError(t, err) require.True(t, exists) + // Force the task to a zero position in this view to simulate the unhealed + // state. A task only ever has one position row per view, so update it if it + // already exists (e.g. created with the filter) instead of inserting a duplicate. tp := &TaskPosition{TaskID: task.ID, ProjectViewID: view.ID, Position: 0} - _, err = s.Insert(tp) + hasPosition, err := s.Where("task_id = ? AND project_view_id = ?", task.ID, view.ID).Exist(&TaskPosition{}) + require.NoError(t, err) + if hasPosition { + _, err = s.Where("task_id = ? AND project_view_id = ?", task.ID, view.ID).Cols("position").Update(tp) + } else { + _, err = s.Insert(tp) + } require.NoError(t, err) _, err = calculateNewPositionForTask(s, u, task, view) diff --git a/pkg/models/task_collection_test.go b/pkg/models/task_collection_test.go index e66d945b2..db918175e 100644 --- a/pkg/models/task_collection_test.go +++ b/pkg/models/task_collection_test.go @@ -1898,7 +1898,7 @@ func TestTaskSearchWithExpandSubtasks(t *testing.T) { expand: []TaskCollectionExpandable{TaskCollectionExpandSubtasks}, } - tasks, _, _, err := getRawTasksForProjects(s, []*Project{project}, &user.User{ID: 15}, opts) + tasks, _, err := getRawTasksForProjects(s, []*Project{project}, &user.User{ID: 15}, opts) require.NoError(t, err) require.NotEmpty(t, tasks) } diff --git a/pkg/models/task_position.go b/pkg/models/task_position.go index 07c2839cc..8ec58a3dd 100644 --- a/pkg/models/task_position.go +++ b/pkg/models/task_position.go @@ -33,9 +33,9 @@ const MinPositionSpacing = 0.01 type TaskPosition struct { // The ID of the task this position is for - TaskID int64 `xorm:"bigint not null index" json:"task_id" param:"task" readOnly:"true" doc:"The numeric id of the task this position belongs to. Taken from the URL; ignored in the request body."` + TaskID int64 `xorm:"bigint not null index unique(task_view)" json:"task_id" param:"task" readOnly:"true" doc:"The numeric id of the task this position belongs to. Taken from the URL; ignored in the request body."` // The project view this task is related to - ProjectViewID int64 `xorm:"bigint not null index" json:"project_view_id" doc:"The id of the project view this position applies to. Positions are stored per view, so the same task has an independent position in each of its project's views."` + ProjectViewID int64 `xorm:"bigint not null index unique(task_view)" json:"project_view_id" doc:"The id of the project view this position applies to. Positions are stored per view, so the same task has an independent position in each of its project's views."` // The position of the task - any task project can be sorted as usual by this parameter. // When accessing tasks via kanban buckets, this is primarily used to sort them based on a range // We're using a float64 here to make it possible to put any task within any two other tasks (by changing the number). @@ -341,6 +341,40 @@ func calculateNewPositionForTask(s *xorm.Session, a web.Auth, t *Task, view *Pro }, nil } +type taskPositionKey struct { + taskID int64 + viewID int64 +} + +// filterNewTaskPositions returns the positions whose (task_id, project_view_id) +// row does not exist yet, also deduplicating within the slice. Position creation +// during task creation can trigger a full recalculation (calculateNewPositionForTask +// or moveTaskToDoneBuckets) that already persists rows for the new task, so inserting +// the queued positions unconditionally would violate the unique index on +// (task_id, project_view_id). +func filterNewTaskPositions(s *xorm.Session, positions []*TaskPosition) ([]*TaskPosition, error) { + filtered := make([]*TaskPosition, 0, len(positions)) + seen := make(map[taskPositionKey]bool, len(positions)) + for _, p := range positions { + key := taskPositionKey{taskID: p.TaskID, viewID: p.ProjectViewID} + if seen[key] { + continue + } + seen[key] = true + + exists, err := s. + Where("task_id = ? AND project_view_id = ?", p.TaskID, p.ProjectViewID). + Exist(&TaskPosition{}) + if err != nil { + return nil, err + } + if !exists { + filtered = append(filtered, p) + } + } + return filtered, nil +} + // DeleteOrphanedTaskPositions removes task position records that reference // tasks or project views that no longer exist. // If dryRun is true, it counts the orphaned records without deleting them. diff --git a/pkg/models/task_search_bench_test.go b/pkg/models/task_search_bench_test.go index 142a58e41..443cf179e 100644 --- a/pkg/models/task_search_bench_test.go +++ b/pkg/models/task_search_bench_test.go @@ -139,7 +139,7 @@ func BenchmarkTaskSearch(b *testing.B) { b.ResetTimer() for i := 0; i < b.N; i++ { s := db.NewSession() - resultSlice, _, _, err := getRawTasksForProjects(s, projects, auth, opts) + resultSlice, _, err := getRawTasksForProjects(s, projects, auth, opts) if len(resultSlice) == 0 { b.Fatalf("no results found for needle %q", needle) } diff --git a/pkg/models/tasks.go b/pkg/models/tasks.go index eb2989694..d57b98b85 100644 --- a/pkg/models/tasks.go +++ b/pkg/models/tasks.go @@ -288,11 +288,11 @@ func getTaskIndexFromSearchString(s string) (index int64) { return } -func getRawTasksForProjects(s *xorm.Session, projects []*Project, a web.Auth, opts *taskSearchOptions) (tasks []*Task, resultCount int, totalItems int64, err error) { +func getRawTasksForProjects(s *xorm.Session, projects []*Project, a web.Auth, opts *taskSearchOptions) (tasks []*Task, totalItems int64, err error) { // If the user does not have any projects, don't try to get any tasks if len(projects) == 0 { - return nil, 0, 0, nil + return nil, 0, nil } // Get all project IDs and get the tasks @@ -324,17 +324,18 @@ func getRawTasksForProjects(s *xorm.Session, projects []*Project, a web.Auth, op } tasks, totalItems, err = dbSearcher.Search(opts) - return tasks, len(tasks), totalItems, err + return tasks, totalItems, err } func getTasksForProjects(s *xorm.Session, projects []*Project, a web.Auth, opts *taskSearchOptions, view *ProjectView) (tasks []*Task, resultCount int, totalItems int64, err error) { - tasks, resultCount, totalItems, err = getRawTasksForProjects(s, projects, a, opts) + tasks, totalItems, err = getRawTasksForProjects(s, projects, a, opts) if err != nil { return nil, 0, 0, err } - taskMap := make(map[int64]*Task, len(tasks)) - for _, t := range tasks { + rawTasks := tasks + taskMap := make(map[int64]*Task, len(rawTasks)) + for _, t := range rawTasks { taskMap[t.ID] = t } @@ -343,7 +344,22 @@ func getTasksForProjects(s *xorm.Session, projects []*Project, a web.Auth, opts return nil, 0, 0, err } - return tasks, resultCount, totalItems, err + // A task can appear more than once in the raw result when it has duplicate + // task_positions rows for the view (the LEFT JOIN multiplies it). Return one + // entry per task, in the original sort order, referencing the enriched map + // value so its identifier and other data are set. totalItems already counts + // distinct tasks, so this also aligns the page size with it. + tasks = make([]*Task, 0, len(taskMap)) + seen := make(map[int64]bool, len(taskMap)) + for _, t := range rawTasks { + if seen[t.ID] { + continue + } + seen[t.ID] = true + tasks = append(tasks, taskMap[t.ID]) + } + + return tasks, len(tasks), totalItems, err } // GetTaskByIDSimple returns a raw task without extra data by the task ID @@ -984,6 +1000,13 @@ func createTask(s *xorm.Session, t *Task, a web.Auth, updateAssignees bool, setB return err } + if len(positions) > 0 { + positions, err = filterNewTaskPositions(s, positions) + if err != nil { + return err + } + } + if len(positions) > 0 { _, err = s.Insert(&positions) if err != nil {