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
This commit is contained in:
parent
9cad4f388c
commit
a61e594952
|
|
@ -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 <https://www.gnu.org/licenses/>.
|
||||
|
||||
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
|
||||
},
|
||||
})
|
||||
}
|
||||
|
|
@ -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
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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}
|
||||
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)
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
|
|
|
|||
Loading…
Reference in New Issue