diff --git a/docs/content/doc/usage/errors.md b/docs/content/doc/usage/errors.md index 6c31ced38..f3a6c8e4f 100644 --- a/docs/content/doc/usage/errors.md +++ b/docs/content/doc/usage/errors.md @@ -70,30 +70,31 @@ This document describes the different errors Vikunja can return. ## Task -| ErrorCode | HTTP Status Code | Description | -|-----------|------------------|-------------| -| 4001 | 400 | The project task text cannot be empty. | -| 4002 | 404 | The project task does not exist. | -| 4003 | 403 | All bulk editing tasks must belong to the same project. | -| 4004 | 403 | Need at least one task when bulk editing tasks. | -| 4005 | 403 | The user does not have the right to see the task. | -| 4006 | 403 | The user tried to set a parent task as the task itself. | +| ErrorCode | HTTP Status Code | Description | +|-----------|------------------|----------------------------------------------------------------------------| +| 4001 | 400 | The project task text cannot be empty. | +| 4002 | 404 | The project task does not exist. | +| 4003 | 403 | All bulk editing tasks must belong to the same project. | +| 4004 | 403 | Need at least one task when bulk editing tasks. | +| 4005 | 403 | The user does not have the right to see the task. | +| 4006 | 403 | The user tried to set a parent task as the task itself. | | 4007 | 400 | The user tried to create a task relation with an invalid kind of relation. | -| 4008 | 409 | The user tried to create a task relation which already exists. | -| 4009 | 404 | The task relation does not exist. | -| 4010 | 400 | Cannot relate a task with itself. | -| 4011 | 404 | The task attachment does not exist. | -| 4012 | 400 | The task attachment is too large. | -| 4013 | 400 | The task sort param is invalid. | -| 4014 | 400 | The task sort order is invalid. | -| 4015 | 404 | The task comment does not exist. | -| 4016 | 400 | Invalid task field. | -| 4017 | 400 | Invalid task filter comparator. | -| 4018 | 400 | Invalid task filter concatinator. | -| 4019 | 400 | Invalid task filter value. | -| 4020 | 400 | The provided attachment does not belong to that task. | -| 4021 | 400 | This user is already assigned to that task. | -| 4022 | 400 | The task has a relative reminder which does not specify relative to what. | +| 4008 | 409 | The user tried to create a task relation which already exists. | +| 4009 | 404 | The task relation does not exist. | +| 4010 | 400 | Cannot relate a task with itself. | +| 4011 | 404 | The task attachment does not exist. | +| 4012 | 400 | The task attachment is too large. | +| 4013 | 400 | The task sort param is invalid. | +| 4014 | 400 | The task sort order is invalid. | +| 4015 | 404 | The task comment does not exist. | +| 4016 | 400 | Invalid task field. | +| 4017 | 400 | Invalid task filter comparator. | +| 4018 | 400 | Invalid task filter concatinator. | +| 4019 | 400 | Invalid task filter value. | +| 4020 | 400 | The provided attachment does not belong to that task. | +| 4021 | 400 | This user is already assigned to that task. | +| 4022 | 400 | The task has a relative reminder which does not specify relative to what. | +| 4023 | 409 | Tried to create a task relation which would create a cycle. | ## Team diff --git a/pkg/models/error.go b/pkg/models/error.go index 195bf402d..b20267b55 100644 --- a/pkg/models/error.go +++ b/pkg/models/error.go @@ -1003,6 +1003,35 @@ func (err ErrReminderRelativeToMissing) HTTPError() web.HTTPError { } } +// ErrTaskRelationCycle represents an error where the user tries to create an already existing relation +type ErrTaskRelationCycle struct { + Kind RelationKind + TaskID int64 + OtherTaskID int64 +} + +// IsErrTaskRelationCycle checks if an error is ErrTaskRelationCycle. +func IsErrTaskRelationCycle(err error) bool { + _, ok := err.(ErrTaskRelationCycle) + return ok +} + +func (err ErrTaskRelationCycle) Error() string { + return fmt.Sprintf("Task relation cycle detectetd [TaskID: %v, OtherTaskID: %v, Kind: %v]", err.TaskID, err.OtherTaskID, err.Kind) +} + +// ErrCodeTaskRelationCycle holds the unique world-error code of this error +const ErrCodeTaskRelationCycle = 4022 + +// HTTPError holds the http error description +func (err ErrTaskRelationCycle) HTTPError() web.HTTPError { + return web.HTTPError{ + HTTPCode: http.StatusConflict, + Code: ErrCodeTaskRelationCycle, + Message: "This task relation would create a cycle.", + } +} + // ============ // Team errors // ============ diff --git a/pkg/models/task_relation.go b/pkg/models/task_relation.go index ff8973509..23f0e578d 100644 --- a/pkg/models/task_relation.go +++ b/pkg/models/task_relation.go @@ -138,6 +138,54 @@ func getInverseRelation(kind RelationKind) RelationKind { return RelationKindUnknown } +func checkTaskRelationCycle(s *xorm.Session, relation *TaskRelation, otherTaskIDToCheck int64, visited map[int64]bool, currentPath map[int64]bool) (err error) { + if visited == nil { + visited = make(map[int64]bool) + } + + if currentPath == nil { + currentPath = make(map[int64]bool) + } + + if visited[relation.TaskID] { + return nil // Node already visited, no cycle detected + } + + if relation.TaskID == otherTaskIDToCheck || // This checks for cycles between leaf nodes + currentPath[relation.TaskID] || + currentPath[otherTaskIDToCheck] { + // Cycle detected + return ErrTaskRelationCycle{ + TaskID: relation.TaskID, + OtherTaskID: relation.OtherTaskID, + Kind: relation.RelationKind, + } + } + + visited[relation.TaskID] = true + currentPath[relation.TaskID] = true + + parenttasks := []*TaskRelation{} + // where child = relation.id + err = s.Where("other_task_id = ? AND relation_kind = ?", relation.TaskID, relation.RelationKind). + Find(&parenttasks) + if err != nil { + return + } + + for _, parent := range parenttasks { + err = checkTaskRelationCycle(s, parent, otherTaskIDToCheck, visited, currentPath) + if err != nil { + return err + } + } + + // Remove the current node from the currentPath to avoid false positives + delete(currentPath, relation.TaskID) + + return nil +} + // Create creates a new task relation // @Summary Create a new relation between two tasks // @Description Creates a new relation between two tasks. The user needs to have update rights on the base task and at least read rights on the other task. Both tasks do not need to be on the same project. Take a look at the docs for available task relation kinds. @@ -191,6 +239,14 @@ func (rel *TaskRelation) Create(s *xorm.Session, a web.Auth) error { RelationKind: getInverseRelation(rel.RelationKind), } + // If we're creating a subtask relation, check if we're about to create a cycle + if rel.RelationKind == RelationKindSubtask || rel.RelationKind == RelationKindParenttask { + err = checkTaskRelationCycle(s, rel, rel.OtherTaskID, nil, nil) + if err != nil { + return err + } + } + // Finally insert everything _, err = s.Insert(&[]*TaskRelation{ rel, diff --git a/pkg/models/task_relation_test.go b/pkg/models/task_relation_test.go index fe02ce60a..03765a9a9 100644 --- a/pkg/models/task_relation_test.go +++ b/pkg/models/task_relation_test.go @@ -96,6 +96,182 @@ func TestTaskRelation_Create(t *testing.T) { require.Error(t, err) assert.True(t, IsErrRelationTasksCannotBeTheSame(err)) }) + t.Run("cycle with one subtask", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + rel := TaskRelation{ + TaskID: 29, + OtherTaskID: 1, + RelationKind: RelationKindSubtask, + } + err := rel.Create(s, &user.User{ID: 1}) + require.Error(t, err) + assert.True(t, IsErrTaskRelationCycle(err)) + }) + t.Run("cycle with multiple subtasks", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + rel1 := TaskRelation{ + TaskID: 1, + OtherTaskID: 2, + RelationKind: RelationKindSubtask, + } + err := rel1.Create(s, &user.User{ID: 1}) + require.NoError(t, err) + rel2 := TaskRelation{ + TaskID: 2, + OtherTaskID: 3, + RelationKind: RelationKindSubtask, + } + err = rel2.Create(s, &user.User{ID: 1}) + require.NoError(t, err) + rel3 := TaskRelation{ + TaskID: 3, + OtherTaskID: 4, + RelationKind: RelationKindSubtask, + } + err = rel3.Create(s, &user.User{ID: 1}) + require.NoError(t, err) + + // Cycle happens here + rel4 := TaskRelation{ + TaskID: 4, + OtherTaskID: 2, + RelationKind: RelationKindSubtask, + } + err = rel4.Create(s, &user.User{ID: 1}) + require.Error(t, err) + assert.True(t, IsErrTaskRelationCycle(err)) + }) + t.Run("cycle with multiple subtasks tasks and relation back to parent", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + rel1 := TaskRelation{ + TaskID: 1, + OtherTaskID: 2, + RelationKind: RelationKindSubtask, + } + err := rel1.Create(s, &user.User{ID: 1}) + require.NoError(t, err) + rel2 := TaskRelation{ + TaskID: 2, + OtherTaskID: 3, + RelationKind: RelationKindSubtask, + } + err = rel2.Create(s, &user.User{ID: 1}) + require.NoError(t, err) + rel3 := TaskRelation{ + TaskID: 3, + OtherTaskID: 4, + RelationKind: RelationKindSubtask, + } + err = rel3.Create(s, &user.User{ID: 1}) + require.NoError(t, err) + + // Cycle happens here + rel4 := TaskRelation{ + TaskID: 4, + OtherTaskID: 1, + RelationKind: RelationKindSubtask, + } + err = rel4.Create(s, &user.User{ID: 1}) + require.Error(t, err) + assert.True(t, IsErrTaskRelationCycle(err)) + }) + t.Run("cycle with one parenttask", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + rel := TaskRelation{ + TaskID: 1, + OtherTaskID: 29, + RelationKind: RelationKindParenttask, + } + err := rel.Create(s, &user.User{ID: 1}) + require.Error(t, err) + assert.True(t, IsErrTaskRelationCycle(err)) + }) + t.Run("cycle with multiple parenttasks", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + rel1 := TaskRelation{ + TaskID: 1, + OtherTaskID: 2, + RelationKind: RelationKindParenttask, + } + err := rel1.Create(s, &user.User{ID: 1}) + require.NoError(t, err) + rel2 := TaskRelation{ + TaskID: 2, + OtherTaskID: 3, + RelationKind: RelationKindParenttask, + } + err = rel2.Create(s, &user.User{ID: 1}) + require.NoError(t, err) + rel3 := TaskRelation{ + TaskID: 3, + OtherTaskID: 4, + RelationKind: RelationKindParenttask, + } + err = rel3.Create(s, &user.User{ID: 1}) + require.NoError(t, err) + + // Cycle happens here + rel4 := TaskRelation{ + TaskID: 4, + OtherTaskID: 2, + RelationKind: RelationKindParenttask, + } + err = rel4.Create(s, &user.User{ID: 1}) + require.Error(t, err) + assert.True(t, IsErrTaskRelationCycle(err)) + }) + t.Run("cycle with multiple parenttasks and relation back to parent", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + rel1 := TaskRelation{ + TaskID: 1, + OtherTaskID: 2, + RelationKind: RelationKindParenttask, + } + err := rel1.Create(s, &user.User{ID: 1}) + require.NoError(t, err) + rel2 := TaskRelation{ + TaskID: 2, + OtherTaskID: 3, + RelationKind: RelationKindParenttask, + } + err = rel2.Create(s, &user.User{ID: 1}) + require.NoError(t, err) + rel3 := TaskRelation{ + TaskID: 3, + OtherTaskID: 4, + RelationKind: RelationKindParenttask, + } + err = rel3.Create(s, &user.User{ID: 1}) + require.NoError(t, err) + + // Cycle happens here + rel4 := TaskRelation{ + TaskID: 4, + OtherTaskID: 1, + RelationKind: RelationKindParenttask, + } + err = rel4.Create(s, &user.User{ID: 1}) + require.Error(t, err) + assert.True(t, IsErrTaskRelationCycle(err)) + }) } func TestTaskRelation_Delete(t *testing.T) {