diff --git a/README.md b/README.md index 7acab63..411b41a 100644 --- a/README.md +++ b/README.md @@ -2611,7 +2611,7 @@ listener. | Variable | Type | Default | Description | | ---------------------- | ------ | -------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | `PORT` | int | `8080` | HTTP listen port | -| `DBURL` | string | `file:///var/lib/neoirc/state.db?_pragma=journal_mode(WAL)&_pragma=busy_timeout(5000)` | SQLite connection string. For file-based: `file:///path/to/db.db?_pragma=journal_mode(WAL)&_pragma=busy_timeout(5000)`. For in-memory (testing): `file::memory:?cache=shared`. See [SQLite Considerations](#sqlite-considerations). | +| `DBURL` | string | `file:///var/lib/neoirc/state.db?_pragma=journal_mode(WAL)&_pragma=busy_timeout(5000)` | SQLite connection string. For file-based: `file:///path/to/db.db?_pragma=journal_mode(WAL)&_pragma=busy_timeout(5000)`. For in-memory (testing): `file::memory:?cache=shared`. It must not set `foreign_keys`. See [SQLite Considerations](#sqlite-considerations). | | `DEBUG` | bool | `false` | Switch logging to debug level | | `MESSAGE_MAX_AGE` | string | `720h` | Maximum age of messages as a Go duration string (e.g. `720h`, `24h`). Messages older than this are pruned. Default is 30 days. | | `SESSION_IDLE_TIMEOUT` | string | `720h` | Idle timeout as a Go duration string (e.g. `720h`, `24h`). Clients with no authenticated HTTP API request or IRC listener command for this long are removed, and a session left with none is expired and its nick released (see [Data Lifecycle](#data-lifecycle)). Default is 30 days. | diff --git a/internal/db/db.go b/internal/db/db.go index e2dce3a..b669061 100644 --- a/internal/db/db.go +++ b/internal/db/db.go @@ -5,9 +5,11 @@ import ( "context" "database/sql" "embed" + "errors" "fmt" "io/fs" "log/slog" + "net/url" "sort" "strconv" "strings" @@ -22,6 +24,10 @@ import ( const minMigrationParts = 2 +var errForeignKeysInDBURL = errors.New( + "DBURL must not set foreign_keys, which the server always turns on", +) + // SchemaFiles contains embedded SQL migration files. // //go:embed schema/*.sql @@ -90,6 +96,33 @@ func (database *Database) connect(ctx context.Context) error { dbURL = config.DefaultDBURL } + // SQLite keeps the foreign keys setting per connection. The driver + // runs each _pragma parameter on every connection it opens. So the + // one added below also covers the connection the pool opens after + // a cancelled query breaks the old one. A foreign_keys pragma in + // DBURL could undo it, because the driver sorts the pragmas. + _, query, hasQuery := strings.Cut(dbURL, "?") + + urlParams, err := url.ParseQuery(query) + if err != nil { + return fmt.Errorf("parse DBURL: %w", err) + } + + for _, pragma := range urlParams["_pragma"] { + if strings.Contains(strings.ToLower(pragma), "foreign_keys") { + return fmt.Errorf( + "%w: _pragma=%s", errForeignKeysInDBURL, pragma, + ) + } + } + + separator := "?" + if hasQuery { + separator = "&" + } + + dbURL += separator + "_pragma=foreign_keys(1)" + database.log.Info( "connecting to database", "url", dbURL, ) @@ -109,13 +142,6 @@ func (database *Database) connect(ctx context.Context) error { database.conn = conn database.log.Info("database connected") - _, err = database.conn.ExecContext( - ctx, "PRAGMA foreign_keys = ON", - ) - if err != nil { - return fmt.Errorf("enable foreign keys: %w", err) - } - return database.runMigrations(ctx) } diff --git a/internal/db/db_test.go b/internal/db/db_test.go index c79fdfc..9b9833f 100644 --- a/internal/db/db_test.go +++ b/internal/db/db_test.go @@ -1,9 +1,13 @@ package db_test import ( + "context" + "errors" + "net/url" "path/filepath" "strings" "testing" + "time" "go.uber.org/fx/fxtest" "sneak.berlin/go/neoirc/internal/config" @@ -14,6 +18,16 @@ import ( const wantBusyTimeoutMS = 5000 +const ( + // endlessQuery counts the rows of a recursive query that never + // stops, so it runs until its context ends. + endlessQuery = `WITH RECURSIVE n(x) AS + (SELECT 1 UNION ALL SELECT x + 1 FROM n) + SELECT COUNT(*) FROM n` + + endlessQueryTimeout = 100 * time.Millisecond +) + // TestDefaultDBURLSetsWALAndBusyTimeout opens a file database // with the default URL's parameters the way the server does, // and checks the journal mode and busy timeout it ends up with. @@ -73,3 +87,145 @@ func TestDefaultDBURLSetsWALAndBusyTimeout(t *testing.T) { ) } } + +// TestForeignKeysOnAfterCancelledQuery cancels a query while it +// runs, which makes the pool close its only connection and open a +// new one. A TEMP table lasts only as long as its connection, so +// its absence shows the connection was replaced. The test then +// checks that the file database keeps its tables and that the new +// connection has foreign keys on. +func TestForeignKeysOnAfterCancelledQuery(t *testing.T) { + t.Parallel() + + _, defaultParams, _ := strings.Cut(config.DefaultDBURL, "?") + + urlParams := map[string]string{ + "default parameters": "?" + defaultParams, + "no parameters": "", + } + + for name, params := range urlParams { + t.Run(name, func(t *testing.T) { + t.Parallel() + + dbURL := "file://" + + filepath.Join(t.TempDir(), "state.db") + params + + database, err := startDatabase(t, dbURL) + if err != nil { + t.Fatal(err) + } + + _, err = database.GetDB().ExecContext( + t.Context(), "CREATE TEMP TABLE first_connection (x)", + ) + if err != nil { + t.Fatal(err) + } + + ctx, cancel := context.WithTimeout( + t.Context(), endlessQueryTimeout, + ) + defer cancel() + + _, err = database.GetDB().ExecContext(ctx, endlessQuery) + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("endless query: err = %v", err) + } + + tempTables := queryInt( + t, database, "SELECT COUNT(*) FROM sqlite_temp_schema", + ) + if tempTables != 0 { + t.Fatal("the pool kept its first connection") + } + + migrations := queryInt( + t, database, "SELECT COUNT(*) FROM schema_migrations", + ) + if migrations == 0 { + t.Error("schema_migrations is empty") + } + + foreignKeys := queryInt(t, database, "PRAGMA foreign_keys") + if foreignKeys != 1 { + t.Errorf("foreign_keys = %d, want 1", foreignKeys) + } + }) + } +} + +// TestDBURLSettingForeignKeysFailsStartup checks that the database +// refuses to start when DBURL sets foreign_keys itself, however it +// is spelled. +func TestDBURLSettingForeignKeysFailsStartup(t *testing.T) { + t.Parallel() + + pragmas := []string{ + "foreign_keys(0)", + "foreign_keys(off)", + "foreign_keys=0", + "FOREIGN_KEYS(FALSE)", + " Foreign_Keys = 1", + } + + for _, pragma := range pragmas { + t.Run(pragma, func(t *testing.T) { + t.Parallel() + + dbURL := "file://" + + filepath.Join(t.TempDir(), "state.db") + + "?_pragma=" + url.QueryEscape(pragma) + + _, err := startDatabase(t, dbURL) + if err == nil { + t.Fatal("the database started") + } + + if !strings.Contains(err.Error(), "DBURL") { + t.Errorf("error %q does not name DBURL", err) + } + }) + } +} + +// startDatabase starts a Database for dbURL the way the server does. +// It returns the error that stopped it from starting, if any. +func startDatabase(t *testing.T, dbURL string) (*db.Database, error) { + t.Helper() + + lifecycle := fxtest.NewLifecycle(t) + + log, err := logger.New(lifecycle, logger.Params{ + Globals: &globals.Globals{}, + }) + if err != nil { + t.Fatal(err) + } + + database, err := db.New(lifecycle, db.Params{ + Logger: log, + Config: &config.Config{DBURL: dbURL}, + }) + if err != nil { + t.Fatal(err) + } + + t.Cleanup(lifecycle.RequireStop) + + return database, lifecycle.Start(t.Context()) +} + +// queryInt runs a query that returns a single number. +func queryInt(t *testing.T, database *db.Database, query string) int { + t.Helper() + + var result int + + err := database.GetDB().QueryRowContext(t.Context(), query).Scan(&result) + if err != nil { + t.Fatal(err) + } + + return result +}