fix(tasks): check for cycles during creation of task relations and prevent them

2024-02-10 13:30:41 +01:00 · 2024-02-10 13:30:41 +01:00 · 5ab9fb89bb
commit 5ab9fb89bb
parent 55e271b329
4 changed files with 285 additions and 23 deletions
--- a/docs/content/doc/usage/errors.md
+++ b/docs/content/doc/usage/errors.md
@ -71,7 +71,7 @@ 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.                    |
@ -94,6 +94,7 @@ This document describes the different errors Vikunja can return.
 | 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

--- 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
 // ============
--- 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,
--- 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) {