fix: enable foreign key check for sqlite (#863)

Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>

backend/internal/bootstrap/db_bootstrap.go

@@ -86,9 +86,6 @@ func connectDatabase() (db *gorm.DB, err error) {
 		if common.EnvConfig.DbConnectionString == "" {
 			return nil, errors.New("missing required env var 'DB_CONNECTION_STRING' for SQLite database")
 		}
-		if !strings.HasPrefix(common.EnvConfig.DbConnectionString, "file:") {
-			return nil, errors.New("invalid value for env var 'DB_CONNECTION_STRING': does not begin with 'file:'")
-		}
 		sqliteutil.RegisterSqliteFunctions()
 		connString, err := parseSqliteConnectionString(common.EnvConfig.DbConnectionString)
 		if err != nil {
@@ -123,25 +120,43 @@ func connectDatabase() (db *gorm.DB, err error) {
 	return nil, err
 }
 
-// The official C implementation of SQLite allows some additional properties in the connection string
-// that are not supported in the in the modernc.org/sqlite driver, and which must be passed as PRAGMA args instead.
-// To ensure that people can use similar args as in the C driver, which was also used by Pocket ID
-// previously (via github.com/mattn/go-sqlite3), we are converting some options.
 func parseSqliteConnectionString(connString string) (string, error) {
 	if !strings.HasPrefix(connString, "file:") {
 		connString = "file:" + connString
 	}
 
+	// Check if we're using an in-memory database
+	isMemoryDB := isSqliteInMemory(connString)
+
+	// Parse the connection string
 	connStringUrl, err := url.Parse(connString)
 	if err != nil {
 		return "", fmt.Errorf("failed to parse SQLite connection string: %w", err)
 	}
 
+	// Convert options for the old SQLite driver to the new one
+	convertSqlitePragmaArgs(connStringUrl)
+
+	// Add the default and required params
+	err = addSqliteDefaultParameters(connStringUrl, isMemoryDB)
+	if err != nil {
+		return "", fmt.Errorf("invalid SQLite connection string: %w", err)
+	}
+
+	return connStringUrl.String(), nil
+}
+
+// The official C implementation of SQLite allows some additional properties in the connection string
+// that are not supported in the in the modernc.org/sqlite driver, and which must be passed as PRAGMA args instead.
+// To ensure that people can use similar args as in the C driver, which was also used by Pocket ID
+// previously (via github.com/mattn/go-sqlite3), we are converting some options.
+// Note this function updates connStringUrl.
+func convertSqlitePragmaArgs(connStringUrl *url.URL) {
 	// Reference: https://github.com/mattn/go-sqlite3?tab=readme-ov-file#connection-string
 	// This only includes a subset of options, excluding those that are not relevant to us
 	qs := make(url.Values, len(connStringUrl.Query()))
 	for k, v := range connStringUrl.Query() {
-		switch k {
+		switch strings.ToLower(k) {
 		case "_auto_vacuum", "_vacuum":
 			qs.Add("_pragma", "auto_vacuum("+v[0]+")")
 		case "_busy_timeout", "_timeout":
@@ -162,9 +177,123 @@ func parseSqliteConnectionString(connString string) (string, error) {
 		}
 	}
 
+	// Update the connStringUrl object
+	connStringUrl.RawQuery = qs.Encode()
+}
+
+// Adds the default (and some required) parameters to the SQLite connection string.
+// Note this function updates connStringUrl.
+func addSqliteDefaultParameters(connStringUrl *url.URL, isMemoryDB bool) error {
+	// This function include code adapted from https://github.com/dapr/components-contrib/blob/v1.14.6/
+	// Copyright (C) 2023 The Dapr Authors
+	// License: Apache2
+	const defaultBusyTimeout = 2500 * time.Millisecond
+
+	// Get the "query string" from the connection string if present
+	qs := connStringUrl.Query()
+	if len(qs) == 0 {
+		qs = make(url.Values, 2)
+	}
+
+	// If the database is in-memory, we must ensure that cache=shared is set
+	if isMemoryDB {
+		qs["cache"] = []string{"shared"}
+	}
+
+	// Check if the database is read-only or immutable
+	isReadOnly := false
+	if len(qs["mode"]) > 0 {
+		// Keep the first value only
+		qs["mode"] = []string{
+			strings.ToLower(qs["mode"][0]),
+		}
+		if qs["mode"][0] == "ro" {
+			isReadOnly = true
+		}
+	}
+	if len(qs["immutable"]) > 0 {
+		// Keep the first value only
+		qs["immutable"] = []string{
+			strings.ToLower(qs["immutable"][0]),
+		}
+		if qs["immutable"][0] == "1" {
+			isReadOnly = true
+		}
+	}
+
+	// We do not want to override a _txlock if set, but we'll show a warning if it's not "immediate"
+	if len(qs["_txlock"]) > 0 {
+		// Keep the first value only
+		qs["_txlock"] = []string{
+			strings.ToLower(qs["_txlock"][0]),
+		}
+		if qs["_txlock"][0] != "immediate" {
+			slog.Warn("SQLite connection is being created with a _txlock different from the recommended value 'immediate'")
+		}
+	} else {
+		qs["_txlock"] = []string{"immediate"}
+	}
+
+	// Add pragma values
+	var hasBusyTimeout, hasJournalMode bool
+	if len(qs["_pragma"]) == 0 {
+		qs["_pragma"] = make([]string, 0, 3)
+	} else {
+		for _, p := range qs["_pragma"] {
+			p = strings.ToLower(p)
+			switch {
+			case strings.HasPrefix(p, "busy_timeout"):
+				hasBusyTimeout = true
+			case strings.HasPrefix(p, "journal_mode"):
+				hasJournalMode = true
+			case strings.HasPrefix(p, "foreign_keys"):
+				return errors.New("found forbidden option '_pragma=foreign_keys' in the connection string")
+			}
+		}
+	}
+	if !hasBusyTimeout {
+		qs["_pragma"] = append(qs["_pragma"], fmt.Sprintf("busy_timeout(%d)", defaultBusyTimeout.Milliseconds()))
+	}
+	if !hasJournalMode {
+		switch {
+		case isMemoryDB:
+			// For in-memory databases, set the journal to MEMORY, the only allowed option besides OFF (which would make transactions ineffective)
+			qs["_pragma"] = append(qs["_pragma"], "journal_mode(MEMORY)")
+		case isReadOnly:
+			// Set the journaling mode to "DELETE" (the default) if the database is read-only
+			qs["_pragma"] = append(qs["_pragma"], "journal_mode(DELETE)")
+		default:
+			// Enable WAL
+			qs["_pragma"] = append(qs["_pragma"], "journal_mode(WAL)")
+		}
+	}
+
+	// Forcefully enable foreign keys
+	qs["_pragma"] = append(qs["_pragma"], "foreign_keys(1)")
+
+	// Update the connStringUrl object
 	connStringUrl.RawQuery = qs.Encode()
 
-	return connStringUrl.String(), nil
+	return nil
+}
+
+// isSqliteInMemory returns true if the connection string is for an in-memory database.
+func isSqliteInMemory(connString string) bool {
+	lc := strings.ToLower(connString)
+
+	// First way to define an in-memory database is to use ":memory:" or "file::memory:" as connection string
+	if strings.HasPrefix(lc, ":memory:") || strings.HasPrefix(lc, "file::memory:") {
+		return true
+	}
+
+	// Another way is to pass "mode=memory" in the "query string"
+	idx := strings.IndexRune(lc, '?')
+	if idx < 0 {
+		return false
+	}
+	qs, _ := url.ParseQuery(lc[(idx + 1):])
+
+	return len(qs["mode"]) > 0 && qs["mode"][0] == "memory"
 }
 
 func getGormLogger() gormLogger.Interface {

backend/internal/bootstrap/db_bootstrap_test.go

@@ -8,23 +8,93 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestParseSqliteConnectionString(t *testing.T) {
+func TestIsSqliteInMemory(t *testing.T) {
 	tests := []struct {
-		name          string
-		input         string
-		expected      string
-		expectedError bool
+		name     string
+		connStr  string
+		expected bool
+	}{
+		{
+			name:     "memory database with :memory:",
+			connStr:  ":memory:",
+			expected: true,
+		},
+		{
+			name:     "memory database with file::memory:",
+			connStr:  "file::memory:",
+			expected: true,
+		},
+		{
+			name:     "memory database with :MEMORY: (uppercase)",
+			connStr:  ":MEMORY:",
+			expected: true,
+		},
+		{
+			name:     "memory database with FILE::MEMORY: (uppercase)",
+			connStr:  "FILE::MEMORY:",
+			expected: true,
+		},
+		{
+			name:     "memory database with mixed case",
+			connStr:  ":Memory:",
+			expected: true,
+		},
+		{
+			name:     "has mode=memory",
+			connStr:  "file:data?mode=memory",
+			expected: true,
+		},
+		{
+			name:     "file database",
+			connStr:  "data.db",
+			expected: false,
+		},
+		{
+			name:     "file database with path",
+			connStr:  "/path/to/data.db",
+			expected: false,
+		},
+		{
+			name:     "file database with file: prefix",
+			connStr:  "file:data.db",
+			expected: false,
+		},
+		{
+			name:     "empty string",
+			connStr:  "",
+			expected: false,
+		},
+		{
+			name:     "string containing memory but not at start",
+			connStr:  "data:memory:.db",
+			expected: false,
+		},
+		{
+			name:     "has mode=ro",
+			connStr:  "file:data?mode=ro",
+			expected: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := isSqliteInMemory(tt.connStr)
+			assert.Equal(t, tt.expected, result)
+		})
+	}
+}
+
+func TestConvertSqlitePragmaArgs(t *testing.T) {
+	tests := []struct {
+		name     string
+		input    string
+		expected string
 	}{
 		{
 			name:     "basic file path",
 			input:    "file:test.db",
 			expected: "file:test.db",
 		},
-		{
-			name:     "adds file: prefix if missing",
-			input:    "test.db",
-			expected: "file:test.db",
-		},
 		{
 			name:     "converts _busy_timeout to pragma",
 			input:    "file:test.db?_busy_timeout=5000",
@@ -100,46 +170,161 @@ func TestParseSqliteConnectionString(t *testing.T) {
 			input:    "file:test.db?_fk=1&mode=rw&_timeout=5000",
 			expected: "file:test.db?_pragma=foreign_keys%281%29&_pragma=busy_timeout%285000%29&mode=rw",
 		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			resultURL, _ := url.Parse(tt.input)
+			convertSqlitePragmaArgs(resultURL)
+
+			// Parse both URLs to compare components independently
+			expectedURL, err := url.Parse(tt.expected)
+			require.NoError(t, err)
+
+			// Compare scheme and path components
+			compareQueryStrings(t, expectedURL, resultURL)
+		})
+	}
+}
+
+func TestAddSqliteDefaultParameters(t *testing.T) {
+	tests := []struct {
+		name        string
+		input       string
+		isMemoryDB  bool
+		expected    string
+		expectError bool
+	}{
 		{
-			name:          "invalid URL format",
-			input:         "file:invalid#$%^&*@test.db",
-			expectedError: true,
+			name:       "basic file database",
+			input:      "file:test.db",
+			isMemoryDB: false,
+			expected:   "file:test.db?_pragma=busy_timeout%282500%29&_pragma=foreign_keys%281%29&_pragma=journal_mode%28WAL%29&_txlock=immediate",
+		},
+		{
+			name:       "in-memory database",
+			input:      "file::memory:",
+			isMemoryDB: true,
+			expected:   "file::memory:?_pragma=busy_timeout%282500%29&_pragma=foreign_keys%281%29&_pragma=journal_mode%28MEMORY%29&_txlock=immediate&cache=shared",
+		},
+		{
+			name:       "read-only database with mode=ro",
+			input:      "file:test.db?mode=ro",
+			isMemoryDB: false,
+			expected:   "file:test.db?_pragma=busy_timeout%282500%29&_pragma=foreign_keys%281%29&_pragma=journal_mode%28DELETE%29&_txlock=immediate&mode=ro",
+		},
+		{
+			name:       "immutable database",
+			input:      "file:test.db?immutable=1",
+			isMemoryDB: false,
+			expected:   "file:test.db?_pragma=busy_timeout%282500%29&_pragma=foreign_keys%281%29&_pragma=journal_mode%28DELETE%29&_txlock=immediate&immutable=1",
+		},
+		{
+			name:       "database with existing _txlock",
+			input:      "file:test.db?_txlock=deferred",
+			isMemoryDB: false,
+			expected:   "file:test.db?_pragma=busy_timeout%282500%29&_pragma=foreign_keys%281%29&_pragma=journal_mode%28WAL%29&_txlock=deferred",
+		},
+		{
+			name:       "database with existing busy_timeout pragma",
+			input:      "file:test.db?_pragma=busy_timeout%285000%29",
+			isMemoryDB: false,
+			expected:   "file:test.db?_pragma=busy_timeout%285000%29&_pragma=foreign_keys%281%29&_pragma=journal_mode%28WAL%29&_txlock=immediate",
+		},
+		{
+			name:       "database with existing journal_mode pragma",
+			input:      "file:test.db?_pragma=journal_mode%28DELETE%29",
+			isMemoryDB: false,
+			expected:   "file:test.db?_pragma=busy_timeout%282500%29&_pragma=foreign_keys%281%29&_pragma=journal_mode%28DELETE%29&_txlock=immediate",
+		},
+		{
+			name:        "database with forbidden foreign_keys pragma",
+			input:       "file:test.db?_pragma=foreign_keys%280%29",
+			isMemoryDB:  false,
+			expectError: true,
+		},
+		{
+			name:       "database with multiple existing pragmas",
+			input:      "file:test.db?_pragma=busy_timeout%283000%29&_pragma=journal_mode%28TRUNCATE%29&_pragma=synchronous%28NORMAL%29",
+			isMemoryDB: false,
+			expected:   "file:test.db?_pragma=busy_timeout%283000%29&_pragma=foreign_keys%281%29&_pragma=journal_mode%28TRUNCATE%29&_pragma=synchronous%28NORMAL%29&_txlock=immediate",
+		},
+		{
+			name:       "in-memory database with cache already set",
+			input:      "file::memory:?cache=private",
+			isMemoryDB: true,
+			expected:   "file::memory:?_pragma=busy_timeout%282500%29&_pragma=foreign_keys%281%29&_pragma=journal_mode%28MEMORY%29&_txlock=immediate&cache=shared",
+		},
+		{
+			name:       "database with mode=rw (not read-only)",
+			input:      "file:test.db?mode=rw",
+			isMemoryDB: false,
+			expected:   "file:test.db?_pragma=busy_timeout%282500%29&_pragma=foreign_keys%281%29&_pragma=journal_mode%28WAL%29&_txlock=immediate&mode=rw",
+		},
+		{
+			name:       "database with immutable=0 (not immutable)",
+			input:      "file:test.db?immutable=0",
+			isMemoryDB: false,
+			expected:   "file:test.db?_pragma=busy_timeout%282500%29&_pragma=foreign_keys%281%29&_pragma=journal_mode%28WAL%29&_txlock=immediate&immutable=0",
+		},
+		{
+			name:       "database with mixed case mode=RO",
+			input:      "file:test.db?mode=RO",
+			isMemoryDB: false,
+			expected:   "file:test.db?_pragma=busy_timeout%282500%29&_pragma=foreign_keys%281%29&_pragma=journal_mode%28DELETE%29&_txlock=immediate&mode=ro",
+		},
+		{
+			name:       "database with mixed case immutable=1",
+			input:      "file:test.db?immutable=1",
+			isMemoryDB: false,
+			expected:   "file:test.db?_pragma=busy_timeout%282500%29&_pragma=foreign_keys%281%29&_pragma=journal_mode%28DELETE%29&_txlock=immediate&immutable=1",
+		},
+		{
+			name:       "complex database configuration",
+			input:      "file:test.db?cache=shared&mode=rwc&_txlock=immediate&_pragma=synchronous%28FULL%29",
+			isMemoryDB: false,
+			expected:   "file:test.db?_pragma=busy_timeout%282500%29&_pragma=foreign_keys%281%29&_pragma=journal_mode%28WAL%29&_pragma=synchronous%28FULL%29&_txlock=immediate&cache=shared&mode=rwc",
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			result, err := parseSqliteConnectionString(tt.input)
+			resultURL, err := url.Parse(tt.input)
+			require.NoError(t, err)
 
-			if tt.expectedError {
+			err = addSqliteDefaultParameters(resultURL, tt.isMemoryDB)
+
+			if tt.expectError {
 				require.Error(t, err)
 				return
 			}
 
 			require.NoError(t, err)
 
-			// Parse both URLs to compare components independently
 			expectedURL, err := url.Parse(tt.expected)
 			require.NoError(t, err)
 
-			resultURL, err := url.Parse(result)
-			require.NoError(t, err)
-
-			// Compare scheme and path components
-			assert.Equal(t, expectedURL.Scheme, resultURL.Scheme)
-			assert.Equal(t, expectedURL.Path, resultURL.Path)
-
-			// Compare query parameters regardless of order
-			expectedQuery := expectedURL.Query()
-			resultQuery := resultURL.Query()
-
-			assert.Len(t, expectedQuery, len(resultQuery))
-
-			for key, expectedValues := range expectedQuery {
-				resultValues, ok := resultQuery[key]
-				_ = assert.True(t, ok) &&
-					assert.ElementsMatch(t, expectedValues, resultValues)
-			}
+			compareQueryStrings(t, expectedURL, resultURL)
 		})
 	}
 }
+
+func compareQueryStrings(t *testing.T, expectedURL *url.URL, resultURL *url.URL) {
+	t.Helper()
+
+	// Compare scheme and path components
+	assert.Equal(t, expectedURL.Scheme, resultURL.Scheme)
+	assert.Equal(t, expectedURL.Path, resultURL.Path)
+
+	// Compare query parameters regardless of order
+	expectedQuery := expectedURL.Query()
+	resultQuery := resultURL.Query()
+
+	assert.Len(t, expectedQuery, len(resultQuery))
+
+	for key, expectedValues := range expectedQuery {
+		resultValues, ok := resultQuery[key]
+		_ = assert.True(t, ok) &&
+			assert.ElementsMatch(t, expectedValues, resultValues)
+	}
+}