From 6c28db780fd2c413c4b6260f3204cd256cff4c4b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= <sybren@blender.org>
Date: Fri, 12 Apr 2024 10:46:08 +0200
Subject: [PATCH] Manager: refuse to delete worker tags without foreign key
 constraints

Before deleting a Worker Tag, check that foreign key constraints are
active for the current database connection.

Sometimes GORM decides to create a new database connection by itself,
without telling us, and then foreign key constraints are not active on
it. This commit is a workaround to avoid database corruption.
---
 internal/manager/persistence/worker_tag.go    |  9 +++++++
 .../manager/persistence/worker_tag_test.go    | 25 +++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/internal/manager/persistence/worker_tag.go b/internal/manager/persistence/worker_tag.go
index 6e0c1506..ed318a81 100644
--- a/internal/manager/persistence/worker_tag.go
+++ b/internal/manager/persistence/worker_tag.go
@@ -62,6 +62,15 @@ func (db *DB) SaveWorkerTag(ctx context.Context, tag *WorkerTag) error {
 
 // DeleteWorkerTag deletes the given tag, after unassigning all workers from it.
 func (db *DB) DeleteWorkerTag(ctx context.Context, uuid string) error {
+	// As a safety measure, refuse to delete unless foreign key constraints are active.
+	fkEnabled, err := db.areForeignKeysEnabled()
+	if err != nil {
+		return fmt.Errorf("checking whether foreign keys are enabled: %w", err)
+	}
+	if !fkEnabled {
+		return ErrDeletingWithoutFK
+	}
+
 	tx := db.gormDB.WithContext(ctx).
 		Where("uuid = ?", uuid).
 		Delete(&WorkerTag{})
diff --git a/internal/manager/persistence/worker_tag_test.go b/internal/manager/persistence/worker_tag_test.go
index 609de145..f754cfa6 100644
--- a/internal/manager/persistence/worker_tag_test.go
+++ b/internal/manager/persistence/worker_tag_test.go
@@ -68,6 +68,31 @@ func TestFetchDeleteTags(t *testing.T) {
 	assert.False(t, has, "expecting HasWorkerTags to return false")
 }
 
+func TestDeleteTagsWithoutFK(t *testing.T) {
+	f := workerTestFixtures(t, 1*time.Second)
+	defer f.done()
+
+	// Single tag was created by fixture.
+	has, err := f.db.HasWorkerTags(f.ctx)
+	require.NoError(t, err)
+	assert.True(t, has, "expecting HasWorkerTags to return true")
+
+	secondTag := WorkerTag{
+		UUID:        uuid.New(),
+		Name:        "arbeiderskaartje",
+		Description: "Worker tag in Dutch",
+	}
+	require.NoError(t, f.db.CreateWorkerTag(f.ctx, &secondTag))
+
+	// Try deleting with foreign key constraints disabled.
+	require.NoError(t, f.db.pragmaForeignKeys(false))
+	err = f.db.DeleteWorkerTag(f.ctx, f.tag.UUID)
+	require.ErrorIs(t, err, ErrDeletingWithoutFK)
+
+	// Test the deletion did not happen.
+	assertTagsMatch(t, f, f.tag.UUID, secondTag.UUID)
+}
+
 func TestAssignUnassignWorkerTags(t *testing.T) {
 	f := workerTestFixtures(t, 1*time.Second)
 	defer f.done()