From 06738b8aa4e994c14d78d7ae807adb32aac3394f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= <sybren@blender.org>
Date: Mon, 10 Jul 2023 14:06:21 +0200
Subject: [PATCH] Manager: disable SQLite foreign key constraints when
 migrating the database

There is an issue with the GORM auto-migration, in that it doesn't
always disable foreign key constraints when it should. Due to
limitations of SQLite, not all 'alter table' commands you'd want to use
are available. As a workaround, these steps are performed:

1. create a new table with the desired schema,
2. copy the data over,
3. drop the old table,
4. rename the new table to the old name.

Step #3 will wreak havoc with the database when foreign key constraint
checks are active, so no we temporarily deactivate them while performing
database migration.
---
 internal/manager/persistence/db.go           | 47 +++++++++++++++-----
 internal/manager/persistence/db_migration.go | 28 ++++++++++++
 2 files changed, 64 insertions(+), 11 deletions(-)

diff --git a/internal/manager/persistence/db.go b/internal/manager/persistence/db.go
index 08e60779..1701c8fc 100644
--- a/internal/manager/persistence/db.go
+++ b/internal/manager/persistence/db.go
@@ -117,17 +117,9 @@ func openDBWithConfig(dsn string, config *gorm.Config) (*DB, error) {
 	sqlDB.SetMaxIdleConns(1) // Max num of connections in the idle connection pool.
 	sqlDB.SetMaxOpenConns(1) // Max num of open connections to the database.
 
-	// Enable foreign key checks.
-	log.Trace().Msg("enabling SQLite foreign key checks")
-	if tx := gormDB.Exec("PRAGMA foreign_keys = 1"); tx.Error != nil {
-		return nil, fmt.Errorf("enabling foreign keys: %w", tx.Error)
-	}
-	var fkEnabled int
-	if tx := gormDB.Raw("PRAGMA foreign_keys").Scan(&fkEnabled); tx.Error != nil {
-		return nil, fmt.Errorf("checking whether the database has foreign key checks enabled: %w", tx.Error)
-	}
-	if fkEnabled == 0 {
-		log.Error().Msg("SQLite database does not want to enable foreign keys, this may cause data loss")
+	// Always enable foreign key checks, to make SQLite behave like a real database.
+	if err := db.pragmaForeignKeys(true); err != nil {
+		return nil, err
 	}
 
 	// Write-ahead-log journal may improve writing speed.
@@ -167,3 +159,36 @@ func (db *DB) Close() error {
 	}
 	return sqldb.Close()
 }
+
+func (db *DB) pragmaForeignKeys(enabled bool) error {
+	var (
+		value int
+		noun  string
+	)
+	switch enabled {
+	case false:
+		value = 0
+		noun = "disabl"
+	case true:
+		value = 1
+		noun = "enabl"
+	}
+
+	log.Trace().Msgf("%sing SQLite foreign key checks", noun)
+
+	// SQLite doesn't seem to like SQL parameters for `PRAGMA`, so `PRAGMA foreign_keys = ?` doesn't work.
+	sql := fmt.Sprintf("PRAGMA foreign_keys = %d", value)
+
+	if tx := db.gormDB.Exec(sql); tx.Error != nil {
+		return fmt.Errorf("%sing foreign keys: %w", noun, tx.Error)
+	}
+	var fkEnabled int
+	if tx := db.gormDB.Raw("PRAGMA foreign_keys").Scan(&fkEnabled); tx.Error != nil {
+		return fmt.Errorf("checking whether the database has foreign key checks %sed: %w", noun, tx.Error)
+	}
+	if fkEnabled != value {
+		return fmt.Errorf("SQLite database does not want to %se foreign keys, this may cause data loss", noun)
+	}
+
+	return nil
+}
diff --git a/internal/manager/persistence/db_migration.go b/internal/manager/persistence/db_migration.go
index 6c8bc570..2c400ca5 100644
--- a/internal/manager/persistence/db_migration.go
+++ b/internal/manager/persistence/db_migration.go
@@ -4,9 +4,37 @@ package persistence
 
 import (
 	"fmt"
+
+	"github.com/rs/zerolog/log"
 )
 
 func (db *DB) migrate() error {
+	log.Debug().Msg("auto-migrating database")
+
+	// There is an issue with the GORM auto-migration, in that it doesn't always
+	// disable foreign key constraints when it should. Due to limitations of
+	// SQLite, not all 'alter table' commands you'd want to use are available. As
+	// a workaround, these steps are performed:
+	//
+	// 1. create a new table with the desired schema,
+	// 2. copy the data over,
+	// 3. drop the old table,
+	// 4. rename the new table to the old name.
+	//
+	// Step #3 will wreak havoc with the database when foreign key constraint
+	// checks are active.
+
+	if err := db.pragmaForeignKeys(false); err != nil {
+		return fmt.Errorf("disabling foreign key checks before auto-migration: %w", err)
+	}
+	defer func() {
+		err := db.pragmaForeignKeys(true)
+		if err != nil {
+			// There is no way that Flamenco Manager should be runnign with foreign key checks disabled.
+			log.Fatal().Err(err).Msg("re-enabling foreign key checks after auto-migration failed")
+		}
+	}()
+
 	err := db.gormDB.AutoMigrate(
 		&Job{},
 		&JobBlock{},