From 5882935cf8844b306034d82f302f58f161906581 Mon Sep 17 00:00:00 2001 From: "Felipe M." Date: Tue, 10 Dec 2024 11:26:13 +0100 Subject: [PATCH 01/19] refactor: Improve SQLite performance with connection pooling and retry logic --- internal/database/sqlite.go | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/internal/database/sqlite.go b/internal/database/sqlite.go index 0675db6aa..1a514ea7a 100644 --- a/internal/database/sqlite.go +++ b/internal/database/sqlite.go @@ -69,6 +69,39 @@ type SQLiteDatabase struct { dbbase } +// InitializeSQLite sets up the SQLite database with optimal settings +func (db *SQLiteDatabase) InitializeSQLite() error { + // Set connection pool settings + db.SetMaxOpenConns(1) // SQLite works best with a single connection + db.SetMaxIdleConns(1) + db.SetConnMaxLifetime(time.Hour) + + // Enable WAL mode for better concurrency + if _, err := db.Exec(`PRAGMA journal_mode=WAL`); err != nil { + return errors.WithStack(err) + } + + // Set busy timeout to avoid "database is locked" errors + if _, err := db.Exec(`PRAGMA busy_timeout=5000`); err != nil { + return errors.WithStack(err) + } + + // Other performance and reliability settings + pragmas := []string{ + `PRAGMA synchronous=NORMAL`, + `PRAGMA cache_size=-2000`, // Use 2MB of memory for cache + `PRAGMA foreign_keys=ON`, + } + + for _, pragma := range pragmas { + if _, err := db.Exec(pragma); err != nil { + return errors.WithStack(err) + } + } + + return nil +} + type bookmarkContent struct { ID int `db:"docid"` Content string `db:"content"` @@ -87,6 +120,10 @@ func (db *SQLiteDatabase) DBx() *sqlx.DB { // Migrate runs migrations for this database engine func (db *SQLiteDatabase) Migrate(ctx context.Context) error { + if err := db.InitializeSQLite(); err != nil { + return errors.WithStack(err) + } + if err := runMigrations(ctx, db, sqliteMigrations); err != nil { return errors.WithStack(err) } From 9a024411cb081ecc3417b097b4c2bcf289e53c08 Mon Sep 17 00:00:00 2001 From: "Felipe M." Date: Tue, 10 Dec 2024 11:26:46 +0100 Subject: [PATCH 02/19] feat: Add withTx and withTxRetry methods to SQLiteDatabase for handling database locks --- internal/database/sqlite.go | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/internal/database/sqlite.go b/internal/database/sqlite.go index 1a514ea7a..593604873 100644 --- a/internal/database/sqlite.go +++ b/internal/database/sqlite.go @@ -69,6 +69,32 @@ type SQLiteDatabase struct { dbbase } +func (db *SQLiteDatabase) withTx(ctx context.Context, fn func(tx *sqlx.Tx) error) error { + return db.withTxRetry(ctx, fn) +} + +func (db *SQLiteDatabase) withTxRetry(ctx context.Context, fn func(tx *sqlx.Tx) error) error { + maxRetries := 3 + var lastErr error + + for i := 0; i < maxRetries; i++ { + err := db.dbbase.withTx(ctx, fn) + if err == nil { + return nil + } + + if strings.Contains(err.Error(), "database is locked") { + lastErr = err + time.Sleep(time.Duration(i+1) * 100 * time.Millisecond) + continue + } + + return errors.WithStack(err) + } + + return errors.WithStack(lastErr) +} + // InitializeSQLite sets up the SQLite database with optimal settings func (db *SQLiteDatabase) InitializeSQLite() error { // Set connection pool settings From b52dd108066e09bc3b9edec47bff29e9aef9bdc5 Mon Sep 17 00:00:00 2001 From: "Felipe M." Date: Tue, 10 Dec 2024 11:30:49 +0100 Subject: [PATCH 03/19] refactor: add Init command to all databases --- internal/database/database.go | 3 +++ internal/database/mysql.go | 5 +++++ internal/database/pg.go | 5 +++++ internal/database/sqlite.go | 14 +++++++------- 4 files changed, 20 insertions(+), 7 deletions(-) diff --git a/internal/database/database.go b/internal/database/database.go index 211a91919..298fb81d2 100644 --- a/internal/database/database.go +++ b/internal/database/database.go @@ -67,6 +67,9 @@ type DB interface { // DBx is the underlying sqlx.DB DBx() *sqlx.DB + // Init initializes the database + Init(ctx context.Context) error + // Migrate runs migrations for this database Migrate(ctx context.Context) error diff --git a/internal/database/mysql.go b/internal/database/mysql.go index 27db774d5..16032d4d2 100644 --- a/internal/database/mysql.go +++ b/internal/database/mysql.go @@ -95,6 +95,11 @@ func (db *MySQLDatabase) DBx() *sqlx.DB { return db.DB } +// Init initializes the database +func (db *MySQLDatabase) Init(ctx context.Context) error { + return nil +} + // Migrate runs migrations for this database engine func (db *MySQLDatabase) Migrate(ctx context.Context) error { if err := runMigrations(ctx, db, mysqlMigrations); err != nil { diff --git a/internal/database/pg.go b/internal/database/pg.go index 66df2dd8d..bc718600a 100644 --- a/internal/database/pg.go +++ b/internal/database/pg.go @@ -86,6 +86,11 @@ func (db *PGDatabase) DBx() *sqlx.DB { return db.DB } +// Init initializes the database +func (db *PGDatabase) Init(ctx context.Context) error { + return nil +} + // Migrate runs migrations for this database engine func (db *PGDatabase) Migrate(ctx context.Context) error { if err := runMigrations(ctx, db, postgresMigrations); err != nil { diff --git a/internal/database/sqlite.go b/internal/database/sqlite.go index 593604873..60c292474 100644 --- a/internal/database/sqlite.go +++ b/internal/database/sqlite.go @@ -95,20 +95,20 @@ func (db *SQLiteDatabase) withTxRetry(ctx context.Context, fn func(tx *sqlx.Tx) return errors.WithStack(lastErr) } -// InitializeSQLite sets up the SQLite database with optimal settings -func (db *SQLiteDatabase) InitializeSQLite() error { +// Init sets up the SQLite database with optimal settings +func (db *SQLiteDatabase) Init(ctx context.Context) error { // Set connection pool settings - db.SetMaxOpenConns(1) // SQLite works best with a single connection + db.SetMaxOpenConns(1) // SQLite works best with a single connection db.SetMaxIdleConns(1) db.SetConnMaxLifetime(time.Hour) // Enable WAL mode for better concurrency - if _, err := db.Exec(`PRAGMA journal_mode=WAL`); err != nil { + if _, err := db.ExecContext(ctx, `PRAGMA journal_mode=WAL`); err != nil { return errors.WithStack(err) } // Set busy timeout to avoid "database is locked" errors - if _, err := db.Exec(`PRAGMA busy_timeout=5000`); err != nil { + if _, err := db.ExecContext(ctx, `PRAGMA busy_timeout=5000`); err != nil { return errors.WithStack(err) } @@ -120,7 +120,7 @@ func (db *SQLiteDatabase) InitializeSQLite() error { } for _, pragma := range pragmas { - if _, err := db.Exec(pragma); err != nil { + if _, err := db.ExecContext(ctx, pragma); err != nil { return errors.WithStack(err) } } @@ -146,7 +146,7 @@ func (db *SQLiteDatabase) DBx() *sqlx.DB { // Migrate runs migrations for this database engine func (db *SQLiteDatabase) Migrate(ctx context.Context) error { - if err := db.InitializeSQLite(); err != nil { + if err := db.Init(ctx); err != nil { return errors.WithStack(err) } From e4e059bd60fc857db211e4b43723455492ef9d11 Mon Sep 17 00:00:00 2001 From: "Felipe M." Date: Tue, 10 Dec 2024 11:35:14 +0100 Subject: [PATCH 04/19] refactor: Improve transaction handling with retry and error management --- internal/database/sqlite.go | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/internal/database/sqlite.go b/internal/database/sqlite.go index 60c292474..3ed2048cc 100644 --- a/internal/database/sqlite.go +++ b/internal/database/sqlite.go @@ -70,7 +70,30 @@ type SQLiteDatabase struct { } func (db *SQLiteDatabase) withTx(ctx context.Context, fn func(tx *sqlx.Tx) error) error { - return db.withTxRetry(ctx, fn) + tx, err := db.BeginTxx(ctx, nil) + if err != nil { + return errors.WithStack(err) + } + + defer func() { + if p := recover(); p != nil { + _ = tx.Rollback() + panic(p) + } + }() + + if err := fn(tx); err != nil { + if rbErr := tx.Rollback(); rbErr != nil { + return errors.WithStack(fmt.Errorf("error rolling back: %v (original error: %w)", rbErr, err)) + } + return errors.WithStack(err) + } + + if err := tx.Commit(); err != nil { + return errors.WithStack(err) + } + + return nil } func (db *SQLiteDatabase) withTxRetry(ctx context.Context, fn func(tx *sqlx.Tx) error) error { @@ -78,7 +101,7 @@ func (db *SQLiteDatabase) withTxRetry(ctx context.Context, fn func(tx *sqlx.Tx) var lastErr error for i := 0; i < maxRetries; i++ { - err := db.dbbase.withTx(ctx, fn) + err := db.withTx(ctx, fn) if err == nil { return nil } From 99ab3d0ed20783499c1b991e1c4f63647b8973d9 Mon Sep 17 00:00:00 2001 From: "Felipe M." Date: Tue, 10 Dec 2024 11:35:57 +0100 Subject: [PATCH 05/19] refactor: Remove panic/recover pattern in transaction handling --- internal/database/sqlite.go | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/internal/database/sqlite.go b/internal/database/sqlite.go index 3ed2048cc..44600bc97 100644 --- a/internal/database/sqlite.go +++ b/internal/database/sqlite.go @@ -75,15 +75,11 @@ func (db *SQLiteDatabase) withTx(ctx context.Context, fn func(tx *sqlx.Tx) error return errors.WithStack(err) } - defer func() { - if p := recover(); p != nil { - _ = tx.Rollback() - panic(p) - } - }() - if err := fn(tx); err != nil { - if rbErr := tx.Rollback(); rbErr != nil { + err = fn(tx) + if err != nil { + rbErr := tx.Rollback() + if rbErr != nil { return errors.WithStack(fmt.Errorf("error rolling back: %v (original error: %w)", rbErr, err)) } return errors.WithStack(err) From 739f860523caf3958e04fd48cfa5018485c2fe99 Mon Sep 17 00:00:00 2001 From: "Felipe M." Date: Tue, 10 Dec 2024 11:37:02 +0100 Subject: [PATCH 06/19] refactor: Replace `errors.WithStack` with `fmt.Errorf` in transaction methods --- internal/database/sqlite.go | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/internal/database/sqlite.go b/internal/database/sqlite.go index 44600bc97..667948c35 100644 --- a/internal/database/sqlite.go +++ b/internal/database/sqlite.go @@ -72,21 +72,20 @@ type SQLiteDatabase struct { func (db *SQLiteDatabase) withTx(ctx context.Context, fn func(tx *sqlx.Tx) error) error { tx, err := db.BeginTxx(ctx, nil) if err != nil { - return errors.WithStack(err) + return fmt.Errorf("failed to begin transaction: %w", err) } - err = fn(tx) if err != nil { rbErr := tx.Rollback() if rbErr != nil { - return errors.WithStack(fmt.Errorf("error rolling back: %v (original error: %w)", rbErr, err)) + return fmt.Errorf("error rolling back: %v (original error: %w)", rbErr, err) } - return errors.WithStack(err) + return fmt.Errorf("transaction failed: %w", err) } if err := tx.Commit(); err != nil { - return errors.WithStack(err) + return fmt.Errorf("failed to commit transaction: %w", err) } return nil @@ -108,10 +107,10 @@ func (db *SQLiteDatabase) withTxRetry(ctx context.Context, fn func(tx *sqlx.Tx) continue } - return errors.WithStack(err) + return fmt.Errorf("transaction failed after retry: %w", err) } - return errors.WithStack(lastErr) + return fmt.Errorf("transaction failed after max retries, last error: %w", lastErr) } // Init sets up the SQLite database with optimal settings From ac4c32935f3136eff19977a1d773b932c1bd3840 Mon Sep 17 00:00:00 2001 From: "Felipe M." Date: Tue, 10 Dec 2024 11:37:20 +0100 Subject: [PATCH 07/19] docs: Add docstrings to `withTx` and `withTxRetry` methods in SQLite database implementation --- internal/database/sqlite.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/internal/database/sqlite.go b/internal/database/sqlite.go index 667948c35..c89dc014f 100644 --- a/internal/database/sqlite.go +++ b/internal/database/sqlite.go @@ -69,6 +69,9 @@ type SQLiteDatabase struct { dbbase } +// withTx executes the given function within a transaction. +// If the function returns an error, the transaction is rolled back. +// Otherwise, the transaction is committed. func (db *SQLiteDatabase) withTx(ctx context.Context, fn func(tx *sqlx.Tx) error) error { tx, err := db.BeginTxx(ctx, nil) if err != nil { @@ -91,6 +94,9 @@ func (db *SQLiteDatabase) withTx(ctx context.Context, fn func(tx *sqlx.Tx) error return nil } +// withTxRetry executes the given function within a transaction with retry logic. +// It will retry up to 3 times if the database is locked, with exponential backoff. +// For other errors, it returns immediately. func (db *SQLiteDatabase) withTxRetry(ctx context.Context, fn func(tx *sqlx.Tx) error) error { maxRetries := 3 var lastErr error From 673778fb129b934b958a9c4f71a1ac0684f5272d Mon Sep 17 00:00:00 2001 From: "Felipe M." Date: Tue, 10 Dec 2024 11:42:26 +0100 Subject: [PATCH 08/19] feat: use new withTxRetry in SaveBookmarks --- internal/database/sqlite.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/database/sqlite.go b/internal/database/sqlite.go index c89dc014f..4cf7c79b8 100644 --- a/internal/database/sqlite.go +++ b/internal/database/sqlite.go @@ -211,7 +211,7 @@ func (db *SQLiteDatabase) SetDatabaseSchemaVersion(ctx context.Context, version func (db *SQLiteDatabase) SaveBookmarks(ctx context.Context, create bool, bookmarks ...model.BookmarkDTO) ([]model.BookmarkDTO, error) { var result []model.BookmarkDTO - if err := db.withTx(ctx, func(tx *sqlx.Tx) error { + if err := db.withTxRetry(ctx, func(tx *sqlx.Tx) error { // Prepare statement stmtInsertBook, err := tx.PreparexContext(ctx, `INSERT INTO bookmark From f89af361d66d2cf0d98df4a8b713de8e40222e4c Mon Sep 17 00:00:00 2001 From: "Felipe M." Date: Tue, 10 Dec 2024 13:15:48 +0100 Subject: [PATCH 09/19] feat: sqlite inmmediate transactions by default --- internal/config/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/config/config.go b/internal/config/config.go index 390b642d6..5c954227b 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -111,7 +111,7 @@ func (c Config) SetDefaults(logger *logrus.Logger, portableMode bool) { // Set default database url if not set if c.Database.DBMS == "" && c.Database.URL == "" { - c.Database.URL = fmt.Sprintf("sqlite:///%s", filepath.Join(c.Storage.DataDir, "shiori.db")) + c.Database.URL = fmt.Sprintf("sqlite:///%s?_txlock=immediate", filepath.Join(c.Storage.DataDir, "shiori.db")) } c.Http.SetDefaults(logger) From bc0eb96a35677426c3bc2fb86ad48b63787a07a9 Mon Sep 17 00:00:00 2001 From: "Felipe M." Date: Tue, 10 Dec 2024 15:56:09 +0100 Subject: [PATCH 10/19] refactor: Split SQLiteDatabase into separate writer and reader dbbase instances --- internal/database/sqlite.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/database/sqlite.go b/internal/database/sqlite.go index 4cf7c79b8..7e8d918d8 100644 --- a/internal/database/sqlite.go +++ b/internal/database/sqlite.go @@ -66,14 +66,15 @@ var sqliteMigrations = []migration{ // SQLiteDatabase is implementation of Database interface // for connecting to SQLite3 database. type SQLiteDatabase struct { - dbbase + writer *dbbase + reader *dbbase } // withTx executes the given function within a transaction. // If the function returns an error, the transaction is rolled back. // Otherwise, the transaction is committed. func (db *SQLiteDatabase) withTx(ctx context.Context, fn func(tx *sqlx.Tx) error) error { - tx, err := db.BeginTxx(ctx, nil) + tx, err := db.writer.BeginTxx(ctx, nil) if err != nil { return fmt.Errorf("failed to begin transaction: %w", err) } From 59822bd008d7c5edae61091ea65c40d667450148 Mon Sep 17 00:00:00 2001 From: "Felipe M." Date: Tue, 10 Dec 2024 15:56:11 +0100 Subject: [PATCH 11/19] refactor: Update Init method to configure both reader and writer database connections --- internal/database/sqlite.go | 49 ++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 23 deletions(-) diff --git a/internal/database/sqlite.go b/internal/database/sqlite.go index 7e8d918d8..58499329e 100644 --- a/internal/database/sqlite.go +++ b/internal/database/sqlite.go @@ -120,33 +120,36 @@ func (db *SQLiteDatabase) withTxRetry(ctx context.Context, fn func(tx *sqlx.Tx) return fmt.Errorf("transaction failed after max retries, last error: %w", lastErr) } -// Init sets up the SQLite database with optimal settings +// Init sets up the SQLite database with optimal settings for both reader and writer connections func (db *SQLiteDatabase) Init(ctx context.Context) error { - // Set connection pool settings - db.SetMaxOpenConns(1) // SQLite works best with a single connection - db.SetMaxIdleConns(1) - db.SetConnMaxLifetime(time.Hour) - - // Enable WAL mode for better concurrency - if _, err := db.ExecContext(ctx, `PRAGMA journal_mode=WAL`); err != nil { - return errors.WithStack(err) - } + // Initialize both connections with appropriate settings + for _, conn := range []*dbbase{db.writer, db.reader} { + // Set connection pool settings + conn.SetMaxOpenConns(1) // SQLite works best with a single connection + conn.SetMaxIdleConns(1) + conn.SetConnMaxLifetime(time.Hour) + + // Enable WAL mode for better concurrency + if _, err := conn.ExecContext(ctx, `PRAGMA journal_mode=WAL`); err != nil { + return errors.WithStack(err) + } - // Set busy timeout to avoid "database is locked" errors - if _, err := db.ExecContext(ctx, `PRAGMA busy_timeout=5000`); err != nil { - return errors.WithStack(err) - } + // Set busy timeout to avoid "database is locked" errors + if _, err := conn.ExecContext(ctx, `PRAGMA busy_timeout=5000`); err != nil { + return errors.WithStack(err) + } - // Other performance and reliability settings - pragmas := []string{ - `PRAGMA synchronous=NORMAL`, - `PRAGMA cache_size=-2000`, // Use 2MB of memory for cache - `PRAGMA foreign_keys=ON`, - } + // Other performance and reliability settings + pragmas := []string{ + `PRAGMA synchronous=NORMAL`, + `PRAGMA cache_size=-2000`, // Use 2MB of memory for cache + `PRAGMA foreign_keys=ON`, + } - for _, pragma := range pragmas { - if _, err := db.ExecContext(ctx, pragma); err != nil { - return errors.WithStack(err) + for _, pragma := range pragmas { + if _, err := conn.ExecContext(ctx, pragma); err != nil { + return errors.WithStack(err) + } } } From 51138f71916732361697e46448d279ac82b92571 Mon Sep 17 00:00:00 2001 From: "Felipe M." Date: Tue, 10 Dec 2024 16:02:08 +0100 Subject: [PATCH 12/19] feat: use writer/reader sqlite databases --- internal/database/sqlite.go | 26 ++++++++++++++++---------- internal/database/sqlite_noncgo.go | 21 +++++++++++++++++---- 2 files changed, 33 insertions(+), 14 deletions(-) diff --git a/internal/database/sqlite.go b/internal/database/sqlite.go index 58499329e..10104117b 100644 --- a/internal/database/sqlite.go +++ b/internal/database/sqlite.go @@ -5,6 +5,7 @@ import ( "database/sql" "fmt" "log" + "runtime" "strings" "time" @@ -124,9 +125,7 @@ func (db *SQLiteDatabase) withTxRetry(ctx context.Context, fn func(tx *sqlx.Tx) func (db *SQLiteDatabase) Init(ctx context.Context) error { // Initialize both connections with appropriate settings for _, conn := range []*dbbase{db.writer, db.reader} { - // Set connection pool settings - conn.SetMaxOpenConns(1) // SQLite works best with a single connection - conn.SetMaxIdleConns(1) + // Reuse connections for up to one hour conn.SetConnMaxLifetime(time.Hour) // Enable WAL mode for better concurrency @@ -153,6 +152,12 @@ func (db *SQLiteDatabase) Init(ctx context.Context) error { } } + // Use a single connection on the writer to avoid database is locked errors + db.writer.SetMaxOpenConns(1) + + // Set maximum idle connections for the reader to number of CPUs (maxing at 4) + db.reader.SetMaxIdleConns(max(4, runtime.NumCPU())) + return nil } @@ -167,17 +172,18 @@ type tagContent struct { model.Tag } -// DBX returns the underlying sqlx.DB object +// DBX returns the underlying sqlx.DB object for writes func (db *SQLiteDatabase) DBx() *sqlx.DB { - return db.DB + return db.writer.DB +} + +// ReaderDBx returns the underlying sqlx.DB object for reading +func (db *SQLiteDatabase) ReaderDBx() *sqlx.DB { + return db.reader.DB } // Migrate runs migrations for this database engine func (db *SQLiteDatabase) Migrate(ctx context.Context) error { - if err := db.Init(ctx); err != nil { - return errors.WithStack(err) - } - if err := runMigrations(ctx, db, sqliteMigrations); err != nil { return errors.WithStack(err) } @@ -189,7 +195,7 @@ func (db *SQLiteDatabase) Migrate(ctx context.Context) error { func (db *SQLiteDatabase) GetDatabaseSchemaVersion(ctx context.Context) (string, error) { var version string - err := db.GetContext(ctx, &version, "SELECT database_schema_version FROM shiori_system") + err := db.reader.GetContext(ctx, &version, "SELECT database_schema_version FROM shiori_system") if err != nil { return "", errors.WithStack(err) } diff --git a/internal/database/sqlite_noncgo.go b/internal/database/sqlite_noncgo.go index f2ee6b9c1..d25ec0ad1 100644 --- a/internal/database/sqlite_noncgo.go +++ b/internal/database/sqlite_noncgo.go @@ -5,9 +5,9 @@ package database import ( "context" + "fmt" "github.com/jmoiron/sqlx" - "github.com/pkg/errors" _ "modernc.org/sqlite" ) @@ -15,11 +15,24 @@ import ( // OpenSQLiteDatabase creates and open connection to new SQLite3 database. func OpenSQLiteDatabase(ctx context.Context, databasePath string) (sqliteDB *SQLiteDatabase, err error) { // Open database - db, err := sqlx.ConnectContext(ctx, "sqlite", databasePath) + rwDB, err := sqlx.ConnectContext(ctx, "sqlite", databasePath) if err != nil { - return nil, errors.WithStack(err) + return nil, fmt.Errorf("error opening writer database: %w", err) + } + + rDB, err := sqlx.ConnectContext(ctx, "sqlite", databasePath) + if err != nil { + return nil, fmt.Errorf("error opening reader database: %w", err) + } + + sqliteDB = &SQLiteDatabase{ + writer: &dbbase{rwDB}, + reader: &dbbase{rDB}, + } + + if err := sqliteDB.Init(ctx); err != nil { + return nil, fmt.Errorf("error initializing database: %w", err) } - sqliteDB = &SQLiteDatabase{dbbase: dbbase{db}} return sqliteDB, nil } From 4829ef1209966319e2b2546fd19f1e348097fbbd Mon Sep 17 00:00:00 2001 From: "Felipe M." Date: Tue, 10 Dec 2024 16:03:02 +0100 Subject: [PATCH 13/19] refactor: Replace all read calls to use the `reader` database instance --- internal/database/sqlite.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/internal/database/sqlite.go b/internal/database/sqlite.go index 10104117b..594a18dac 100644 --- a/internal/database/sqlite.go +++ b/internal/database/sqlite.go @@ -507,7 +507,7 @@ func (db *SQLiteDatabase) GetBookmarks(ctx context.Context, opts GetBookmarksOpt // Fetch bookmarks bookmarks := []model.BookmarkDTO{} - err = db.SelectContext(ctx, &bookmarks, query, args...) + err = db.reader.SelectContext(ctx, &bookmarks, query, args...) if err != nil && err != sql.ErrNoRows { return nil, errors.WithStack(err) } @@ -529,12 +529,12 @@ func (db *SQLiteDatabase) GetBookmarks(ctx context.Context, opts GetBookmarksOpt contentMap := make(map[int]bookmarkContent, len(bookmarks)) contentQuery, args, err := sqlx.In(`SELECT docid, content, html FROM bookmark_content WHERE docid IN (?)`, bookmarkIds) - contentQuery = db.Rebind(contentQuery) + contentQuery = db.reader.Rebind(contentQuery) if err != nil { return nil, errors.WithStack(err) } - err = db.Select(&contents, contentQuery, args...) + err = db.reader.Select(&contents, contentQuery, args...) if err != nil && err != sql.ErrNoRows { return nil, errors.WithStack(err) } @@ -562,12 +562,12 @@ func (db *SQLiteDatabase) GetBookmarks(ctx context.Context, opts GetBookmarksOpt LEFT JOIN tag t ON bt.tag_id = t.id WHERE bt.bookmark_id IN (?) ORDER BY t.name`, bookmarkIds) - tagsQuery = db.Rebind(tagsQuery) + tagsQuery = db.reader.Rebind(tagsQuery) if err != nil { return nil, errors.WithStack(err) } - err = db.Select(&tags, tagsQuery, tagArgs...) + err = db.reader.Select(&tags, tagsQuery, tagArgs...) if err != nil && err != sql.ErrNoRows { return nil, errors.WithStack(err) } @@ -686,7 +686,7 @@ func (db *SQLiteDatabase) GetBookmarksCount(ctx context.Context, opts GetBookmar // Fetch count var nBookmarks int - err = db.GetContext(ctx, &nBookmarks, query, args...) + err = db.reader.GetContext(ctx, &nBookmarks, query, args...) if err != nil && err != sql.ErrNoRows { return 0, errors.WithStack(err) } @@ -781,7 +781,7 @@ func (db *SQLiteDatabase) GetBookmark(ctx context.Context, id int, url string) ( } book := model.BookmarkDTO{} - if err := db.GetContext(ctx, &book, query, args...); err != nil && err != sql.ErrNoRows { + if err := db.reader.GetContext(ctx, &book, query, args...); err != nil && err != sql.ErrNoRows { return book, false, errors.WithStack(err) } @@ -847,7 +847,7 @@ func (db *SQLiteDatabase) GetAccounts(ctx context.Context, opts GetAccountsOptio // Fetch list account accounts := []model.Account{} - err := db.SelectContext(ctx, &accounts, query, args...) + err := db.reader.SelectContext(ctx, &accounts, query, args...) if err != nil && err != sql.ErrNoRows { return nil, errors.WithStack(err) } @@ -859,7 +859,7 @@ func (db *SQLiteDatabase) GetAccounts(ctx context.Context, opts GetAccountsOptio // Returns the account and boolean whether it's exist or not. func (db *SQLiteDatabase) GetAccount(ctx context.Context, username string) (model.Account, bool, error) { account := model.Account{} - if err := db.GetContext(ctx, &account, `SELECT + if err := db.reader.GetContext(ctx, &account, `SELECT id, username, password, owner, config FROM account WHERE username = ?`, username, ); err != nil { @@ -931,7 +931,7 @@ func (db *SQLiteDatabase) GetTags(ctx context.Context) ([]model.Tag, error) { LEFT JOIN tag t ON bt.tag_id = t.id GROUP BY bt.tag_id ORDER BY t.name` - err := db.SelectContext(ctx, &tags, query) + err := db.reader.SelectContext(ctx, &tags, query) if err != nil && err != sql.ErrNoRows { return nil, errors.WithStack(err) } From c7f0fb656c3e55b8bdf14f1f1e46fa7778c1a524 Mon Sep 17 00:00:00 2001 From: "Felipe M." Date: Tue, 10 Dec 2024 16:20:05 +0100 Subject: [PATCH 14/19] refactor: Replace errors.WithStack with fmt.Errorf and add nil checks refactor: Replace errors.WithStack with fmt.Errorf and add proper error handling fix: Handle potential database connection errors with improved error wrapping refactor: Replace errors.WithStack with fmt.Errorf and improve error handling refactor: Replace error handling with fmt.Errorf and proper nil checks refactor: Replace errors.WithStack with fmt.Errorf and add nil error checks refactor: Replace errors.WithStack with fmt.Errorf and add nil checks in sqlite.go refactor: Replace errors.WithStack with fmt.Errorf and add nil checks refactor: Replace errors.WithStack with fmt.Errorf and improve error handling refactor: Replace remaining errors.WithStack with fmt.Errorf in sqlite.go --- internal/database/sqlite.go | 276 ++++++++++++++++++++++++++++-------- 1 file changed, 219 insertions(+), 57 deletions(-) diff --git a/internal/database/sqlite.go b/internal/database/sqlite.go index 594a18dac..a856b9bcb 100644 --- a/internal/database/sqlite.go +++ b/internal/database/sqlite.go @@ -130,12 +130,18 @@ func (db *SQLiteDatabase) Init(ctx context.Context) error { // Enable WAL mode for better concurrency if _, err := conn.ExecContext(ctx, `PRAGMA journal_mode=WAL`); err != nil { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to set journal mode: %w", err) + } + return nil } // Set busy timeout to avoid "database is locked" errors if _, err := conn.ExecContext(ctx, `PRAGMA busy_timeout=5000`); err != nil { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to set busy timeout: %w", err) + } + return nil } // Other performance and reliability settings @@ -147,7 +153,10 @@ func (db *SQLiteDatabase) Init(ctx context.Context) error { for _, pragma := range pragmas { if _, err := conn.ExecContext(ctx, pragma); err != nil { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to set pragma %s: %w", pragma, err) + } + return nil } } } @@ -185,7 +194,10 @@ func (db *SQLiteDatabase) ReaderDBx() *sqlx.DB { // Migrate runs migrations for this database engine func (db *SQLiteDatabase) Migrate(ctx context.Context) error { if err := runMigrations(ctx, db, sqliteMigrations); err != nil { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to run migrations: %w", err) + } + return nil } return nil @@ -197,7 +209,7 @@ func (db *SQLiteDatabase) GetDatabaseSchemaVersion(ctx context.Context) (string, err := db.reader.GetContext(ctx, &version, "SELECT database_schema_version FROM shiori_system") if err != nil { - return "", errors.WithStack(err) + return "", fmt.Errorf("failed to get database schema version: %w", err) } return version, nil @@ -210,7 +222,10 @@ func (db *SQLiteDatabase) SetDatabaseSchemaVersion(ctx context.Context, version _, err := tx.Exec("UPDATE shiori_system SET database_schema_version = ?", version) if err != nil { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to get database schema version: %w", err) + } + return nil } return tx.Commit() @@ -228,7 +243,10 @@ func (db *SQLiteDatabase) SaveBookmarks(ctx context.Context, create bool, bookma (url, title, excerpt, author, public, modified_at, has_content, created_at) VALUES(?, ?, ?, ?, ?, ?, ?, ?) RETURNING id`) if err != nil { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to prepare insert book statement: %w", err) + } + return nil } stmtUpdateBook, err := tx.PreparexContext(ctx, `UPDATE bookmark SET @@ -236,43 +254,64 @@ func (db *SQLiteDatabase) SaveBookmarks(ctx context.Context, create bool, bookma public = ?, modified_at = ?, has_content = ? WHERE id = ?`) if err != nil { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to prepare update book statement: %w", err) + } + return nil } stmtInsertBookContent, err := tx.PreparexContext(ctx, `INSERT OR REPLACE INTO bookmark_content (docid, title, content, html) VALUES (?, ?, ?, ?)`) if err != nil { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to prepare insert book content statement: %w", err) + } + return nil } stmtUpdateBookContent, err := tx.PreparexContext(ctx, `UPDATE bookmark_content SET title = ?, content = ?, html = ? WHERE docid = ?`) if err != nil { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to prepare update book content statement: %w", err) + } + return nil } stmtGetTag, err := tx.PreparexContext(ctx, `SELECT id FROM tag WHERE name = ?`) if err != nil { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to prepare get tag statement: %w", err) + } + return nil } stmtInsertTag, err := tx.PreparexContext(ctx, `INSERT INTO tag (name) VALUES (?)`) if err != nil { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to prepare insert tag statement: %w", err) + } + return nil } stmtInsertBookTag, err := tx.PreparexContext(ctx, `INSERT OR IGNORE INTO bookmark_tag (tag_id, bookmark_id) VALUES (?, ?)`) if err != nil { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to prepare insert book tag statement: %w", err) + } + return nil } stmtDeleteBookTag, err := tx.PreparexContext(ctx, `DELETE FROM bookmark_tag WHERE bookmark_id = ? AND tag_id = ?`) if err != nil { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to execute delete statement: %w", err) + } + return nil } // Prepare modified time @@ -308,25 +347,37 @@ func (db *SQLiteDatabase) SaveBookmarks(ctx context.Context, create bool, bookma book.URL, book.Title, book.Excerpt, book.Author, book.Public, book.ModifiedAt, hasContent, book.ID) } if err != nil { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to delete bookmark content: %w", err) + } + return nil } // Try to update it first to check for existence, we can't do an UPSERT here because // bookmant_content is a virtual table res, err := stmtUpdateBookContent.ExecContext(ctx, book.Title, book.Content, book.HTML, book.ID) if err != nil { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to delete bookmark tag: %w", err) + } + return nil } rows, err := res.RowsAffected() if err != nil { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to delete bookmark: %w", err) + } + return nil } if rows == 0 { _, err = stmtInsertBookContent.ExecContext(ctx, book.ID, book.Title, book.Content, book.HTML) if err != nil { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to execute delete bookmark tag statement: %w", err) + } + return nil } } @@ -337,7 +388,10 @@ func (db *SQLiteDatabase) SaveBookmarks(ctx context.Context, create bool, bookma if tag.Deleted { _, err = stmtDeleteBookTag.ExecContext(ctx, book.ID, tag.ID) if err != nil { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to execute delete bookmark statement: %w", err) + } + return nil } continue } @@ -349,26 +403,38 @@ func (db *SQLiteDatabase) SaveBookmarks(ctx context.Context, create bool, bookma // If tag doesn't have any ID, fetch it from database if tag.ID == 0 { if err := stmtGetTag.GetContext(ctx, &tag.ID, tagName); err != nil && err != sql.ErrNoRows { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to get tag ID: %w", err) + } + return nil } // If tag doesn't exist in database, save it if tag.ID == 0 { res, err := stmtInsertTag.ExecContext(ctx, tagName) if err != nil { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to get last insert ID for tag: %w", err) + } + return nil } tagID64, err := res.LastInsertId() if err != nil && err != sql.ErrNoRows { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to insert bookmark tag: %w", err) + } + return nil } tag.ID = int(tagID64) } if _, err := stmtInsertBookTag.ExecContext(ctx, tag.ID, book.ID); err != nil { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to execute bookmark tag statement: %w", err) + } + return nil } } @@ -381,7 +447,10 @@ func (db *SQLiteDatabase) SaveBookmarks(ctx context.Context, create bool, bookma return nil }); err != nil { - return nil, errors.WithStack(err) + if err != nil { + return nil, fmt.Errorf("failed to execute select query for bookmark content: %w", err) + } + return nil, nil } return result, nil @@ -502,14 +571,20 @@ func (db *SQLiteDatabase) GetBookmarks(ctx context.Context, opts GetBookmarksOpt // Expand query, because some of the args might be an array query, args, err := sqlx.In(query, args...) if err != nil { - return nil, errors.WithStack(err) + if err != nil { + return nil, fmt.Errorf("failed to execute select query for tags: %w", err) + } + return nil, nil } // Fetch bookmarks bookmarks := []model.BookmarkDTO{} err = db.reader.SelectContext(ctx, &bookmarks, query, args...) if err != nil && err != sql.ErrNoRows { - return nil, errors.WithStack(err) + if err != nil { + return nil, fmt.Errorf("failed to fetch accounts: %w", err) + } + return nil, nil } // store bookmark IDs for further enrichment @@ -531,12 +606,18 @@ func (db *SQLiteDatabase) GetBookmarks(ctx context.Context, opts GetBookmarksOpt contentQuery, args, err := sqlx.In(`SELECT docid, content, html FROM bookmark_content WHERE docid IN (?)`, bookmarkIds) contentQuery = db.reader.Rebind(contentQuery) if err != nil { - return nil, errors.WithStack(err) + if err != nil { + return nil, fmt.Errorf("failed to expand tags query with IN clause: %w", err) + } + return nil, nil } err = db.reader.Select(&contents, contentQuery, args...) if err != nil && err != sql.ErrNoRows { - return nil, errors.WithStack(err) + if err != nil { + return nil, fmt.Errorf("failed to get tags: %w", err) + } + return nil, nil } for _, content := range contents { contentMap[content.ID] = content @@ -564,12 +645,18 @@ func (db *SQLiteDatabase) GetBookmarks(ctx context.Context, opts GetBookmarksOpt ORDER BY t.name`, bookmarkIds) tagsQuery = db.reader.Rebind(tagsQuery) if err != nil { - return nil, errors.WithStack(err) + if err != nil { + return nil, fmt.Errorf("failed to delete bookmark and related records: %w", err) + } + return nil, nil } err = db.reader.Select(&tags, tagsQuery, tagArgs...) if err != nil && err != sql.ErrNoRows { - return nil, errors.WithStack(err) + if err != nil { + return nil, fmt.Errorf("failed to get tags: %w", err) + } + return nil, nil } for _, fetchedTag := range tags { if tags, found := tagsMap[fetchedTag.ID]; found { @@ -681,14 +768,20 @@ func (db *SQLiteDatabase) GetBookmarksCount(ctx context.Context, opts GetBookmar // Expand query, because some of the args might be an array query, args, err := sqlx.In(query, args...) if err != nil { - return 0, errors.WithStack(err) + if err != nil { + return 0, fmt.Errorf("failed to expand query with IN clause: %w", err) + } + return 0, nil } // Fetch count var nBookmarks int err = db.reader.GetContext(ctx, &nBookmarks, query, args...) if err != nil && err != sql.ErrNoRows { - return 0, errors.WithStack(err) + if err != nil { + return 0, fmt.Errorf("failed to get bookmark count: %w", err) + } + return 0, nil } return nBookmarks, nil @@ -706,17 +799,26 @@ func (db *SQLiteDatabase) DeleteBookmarks(ctx context.Context, ids ...int) error if len(ids) == 0 { _, err := tx.ExecContext(ctx, delBookmarkContent) if err != nil { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to prepare delete statement: %w", err) + } + return nil } _, err = tx.ExecContext(ctx, delBookmarkTag) if err != nil { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to execute delete account statement: %w", err) + } + return nil } _, err = tx.ExecContext(ctx, delBookmark) if err != nil { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to execute delete bookmark statement: %w", err) + } + return nil } } else { delBookmark += ` WHERE id = ?` @@ -725,40 +827,55 @@ func (db *SQLiteDatabase) DeleteBookmarks(ctx context.Context, ids ...int) error stmtDelBookmark, err := tx.Preparex(delBookmark) if err != nil { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to get bookmark: %w", err) + } + return nil } stmtDelBookmarkTag, err := tx.Preparex(delBookmarkTag) if err != nil { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to expand query with IN clause: %w", err) + } + return nil } stmtDelBookmarkContent, err := tx.Preparex(delBookmarkContent) if err != nil { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to delete bookmark content: %w", err) + } + return nil } for _, id := range ids { _, err = stmtDelBookmarkContent.ExecContext(ctx, id) if err != nil { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to delete bookmark: %w", err) + } + return nil } _, err = stmtDelBookmarkTag.ExecContext(ctx, id) if err != nil { - return errors.WithStack(err) + return fmt.Errorf("failed to delete bookmark tag: %w", err) } _, err = stmtDelBookmark.ExecContext(ctx, id) if err != nil { - return errors.WithStack(err) + return fmt.Errorf("failed to delete bookmark: %w", err) } } } return nil }); err != nil { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to update database schema version: %w", err) + } + return nil } return nil @@ -782,7 +899,10 @@ func (db *SQLiteDatabase) GetBookmark(ctx context.Context, id int, url string) ( book := model.BookmarkDTO{} if err := db.reader.GetContext(ctx, &book, query, args...); err != nil && err != sql.ErrNoRows { - return book, false, errors.WithStack(err) + if err != nil { + return book, false, fmt.Errorf("failed to get bookmark: %w", err) + } + return book, false, nil } return book, book.ID != 0, nil @@ -804,9 +924,15 @@ func (db *SQLiteDatabase) SaveAccount(ctx context.Context, account model.Account password = ?, owner = ?`, account.Username, hashedPassword, account.Owner, account.Config, hashedPassword, account.Owner, account.Config) - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to hash password: %w", err) + } + return nil }); err != nil { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to insert/update account: %w", err) + } + return nil } return nil @@ -820,9 +946,15 @@ func (db *SQLiteDatabase) SaveAccountSettings(ctx context.Context, account model SET config = ? WHERE username = ?`, account.Config, account.Username) - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to update account settings: %w", err) + } + return nil }); err != nil { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to prepare delete book tag statement: %w", err) + } + return nil } return nil @@ -849,7 +981,10 @@ func (db *SQLiteDatabase) GetAccounts(ctx context.Context, opts GetAccountsOptio accounts := []model.Account{} err := db.reader.SelectContext(ctx, &accounts, query, args...) if err != nil && err != sql.ErrNoRows { - return nil, errors.WithStack(err) + if err != nil { + return nil, fmt.Errorf("failed to execute select query: %w", err) + } + return nil, nil } return accounts, nil @@ -863,7 +998,10 @@ func (db *SQLiteDatabase) GetAccount(ctx context.Context, username string) (mode id, username, password, owner, config FROM account WHERE username = ?`, username, ); err != nil { - return account, false, errors.WithStack(err) + if err != nil && err != sql.ErrNoRows { + return account, false, fmt.Errorf("failed to get account: %w", err) + } + return account, false, nil } return account, account.ID != 0, nil @@ -875,19 +1013,28 @@ func (db *SQLiteDatabase) DeleteAccounts(ctx context.Context, usernames ...strin // Delete account stmtDelete, err := tx.Preparex(`DELETE FROM account WHERE username = ?`) if err != nil { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to insert tag: %w", err) + } + return nil } for _, username := range usernames { _, err := stmtDelete.ExecContext(ctx, username) if err != nil { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to delete bookmark tag: %w", err) + } + return nil } } return nil }); err != nil { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to prepare statement: %w", err) + } + return nil } return nil @@ -907,17 +1054,26 @@ func (db *SQLiteDatabase) CreateTags(ctx context.Context, tags ...model.Tag) err if err := db.withTx(ctx, func(tx *sqlx.Tx) error { stmt, err := tx.Preparex(query) if err != nil { - return errors.Wrap(errors.WithStack(err), "error preparing query") + if err != nil { + return fmt.Errorf("failed to prepare tag creation query: %w", err) + } + return nil } _, err = stmt.ExecContext(ctx, values...) if err != nil { - return errors.Wrap(errors.WithStack(err), "error executing query") + if err != nil { + return fmt.Errorf("failed to execute tag creation query: %w", err) + } + return nil } return nil }); err != nil { - return errors.Wrap(errors.WithStack(err), "error running transaction") + if err != nil { + return fmt.Errorf("failed to run tag creation transaction: %w", err) + } + return nil } return nil @@ -933,7 +1089,10 @@ func (db *SQLiteDatabase) GetTags(ctx context.Context) ([]model.Tag, error) { err := db.reader.SelectContext(ctx, &tags, query) if err != nil && err != sql.ErrNoRows { - return nil, errors.WithStack(err) + if err != nil { + return nil, fmt.Errorf("failed to prepare delete bookmark content statement: %w", err) + } + return nil, nil } return tags, nil @@ -945,7 +1104,10 @@ func (db *SQLiteDatabase) RenameTag(ctx context.Context, id int, newName string) _, err := tx.ExecContext(ctx, `UPDATE tag SET name = ? WHERE id = ?`, newName, id) return err }); err != nil { - return errors.WithStack(err) + if err != nil { + return fmt.Errorf("failed to rename tag: %w", err) + } + return nil } return nil From e17a93027f21e2331e0fbac4dadd73fb3efdc57e Mon Sep 17 00:00:00 2001 From: "Felipe M." Date: Tue, 10 Dec 2024 16:26:44 +0100 Subject: [PATCH 15/19] refactor: Use withTxRetry for SetDatabaseSchemaVersion method --- internal/database/sqlite.go | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/internal/database/sqlite.go b/internal/database/sqlite.go index a856b9bcb..5f42e5e2c 100644 --- a/internal/database/sqlite.go +++ b/internal/database/sqlite.go @@ -217,18 +217,17 @@ func (db *SQLiteDatabase) GetDatabaseSchemaVersion(ctx context.Context) (string, // SetDatabaseSchemaVersion sets the current migrations version of the database func (db *SQLiteDatabase) SetDatabaseSchemaVersion(ctx context.Context, version string) error { - tx := db.MustBegin() - defer tx.Rollback() - - _, err := tx.Exec("UPDATE shiori_system SET database_schema_version = ?", version) - if err != nil { + if err := db.withTxRetry(ctx, func(tx *sqlx.Tx) error { + _, err := tx.ExecContext(ctx, "UPDATE shiori_system SET database_schema_version = ?", version) if err != nil { - return fmt.Errorf("failed to get database schema version: %w", err) + return err } return nil + }); err != nil { + return fmt.Errorf("failed to set database schema version: %w", err) } - return tx.Commit() + return nil } // SaveBookmarks saves new or updated bookmarks to database. From cac7580a5f68f153adc4f8a69bfd84b882979060 Mon Sep 17 00:00:00 2001 From: "Felipe M." Date: Tue, 10 Dec 2024 16:31:36 +0100 Subject: [PATCH 16/19] fix: Simplify error handling in GetBookmark and GetAccount methods --- internal/database/sqlite.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/internal/database/sqlite.go b/internal/database/sqlite.go index 5f42e5e2c..59fecb331 100644 --- a/internal/database/sqlite.go +++ b/internal/database/sqlite.go @@ -898,10 +898,7 @@ func (db *SQLiteDatabase) GetBookmark(ctx context.Context, id int, url string) ( book := model.BookmarkDTO{} if err := db.reader.GetContext(ctx, &book, query, args...); err != nil && err != sql.ErrNoRows { - if err != nil { - return book, false, fmt.Errorf("failed to get bookmark: %w", err) - } - return book, false, nil + return book, false, fmt.Errorf("failed to get bookmark: %w", err) } return book, book.ID != 0, nil @@ -1000,7 +997,7 @@ func (db *SQLiteDatabase) GetAccount(ctx context.Context, username string) (mode if err != nil && err != sql.ErrNoRows { return account, false, fmt.Errorf("failed to get account: %w", err) } - return account, false, nil + return account, true, nil } return account, account.ID != 0, nil From 3d893060dba60a6a05bf99623ebe08f9d2f225c9 Mon Sep 17 00:00:00 2001 From: "Felipe M." Date: Tue, 10 Dec 2024 16:31:38 +0100 Subject: [PATCH 17/19] refactor: Remove duplicated non-nil error checks in sqlite.go fix: duplicated non-nil checks --- internal/database/sqlite.go | 251 ++++++++---------------------------- 1 file changed, 52 insertions(+), 199 deletions(-) diff --git a/internal/database/sqlite.go b/internal/database/sqlite.go index 59fecb331..c9e663f37 100644 --- a/internal/database/sqlite.go +++ b/internal/database/sqlite.go @@ -130,18 +130,12 @@ func (db *SQLiteDatabase) Init(ctx context.Context) error { // Enable WAL mode for better concurrency if _, err := conn.ExecContext(ctx, `PRAGMA journal_mode=WAL`); err != nil { - if err != nil { - return fmt.Errorf("failed to set journal mode: %w", err) - } - return nil + return fmt.Errorf("failed to set journal mode: %w", err) } // Set busy timeout to avoid "database is locked" errors if _, err := conn.ExecContext(ctx, `PRAGMA busy_timeout=5000`); err != nil { - if err != nil { - return fmt.Errorf("failed to set busy timeout: %w", err) - } - return nil + return fmt.Errorf("failed to set busy timeout: %w", err) } // Other performance and reliability settings @@ -153,10 +147,7 @@ func (db *SQLiteDatabase) Init(ctx context.Context) error { for _, pragma := range pragmas { if _, err := conn.ExecContext(ctx, pragma); err != nil { - if err != nil { - return fmt.Errorf("failed to set pragma %s: %w", pragma, err) - } - return nil + return fmt.Errorf("failed to set pragma %s: %w", pragma, err) } } } @@ -194,10 +185,7 @@ func (db *SQLiteDatabase) ReaderDBx() *sqlx.DB { // Migrate runs migrations for this database engine func (db *SQLiteDatabase) Migrate(ctx context.Context) error { if err := runMigrations(ctx, db, sqliteMigrations); err != nil { - if err != nil { - return fmt.Errorf("failed to run migrations: %w", err) - } - return nil + return fmt.Errorf("failed to run migrations: %w", err) } return nil @@ -242,10 +230,7 @@ func (db *SQLiteDatabase) SaveBookmarks(ctx context.Context, create bool, bookma (url, title, excerpt, author, public, modified_at, has_content, created_at) VALUES(?, ?, ?, ?, ?, ?, ?, ?) RETURNING id`) if err != nil { - if err != nil { - return fmt.Errorf("failed to prepare insert book statement: %w", err) - } - return nil + return fmt.Errorf("failed to prepare insert book statement: %w", err) } stmtUpdateBook, err := tx.PreparexContext(ctx, `UPDATE bookmark SET @@ -253,64 +238,43 @@ func (db *SQLiteDatabase) SaveBookmarks(ctx context.Context, create bool, bookma public = ?, modified_at = ?, has_content = ? WHERE id = ?`) if err != nil { - if err != nil { - return fmt.Errorf("failed to prepare update book statement: %w", err) - } - return nil + return fmt.Errorf("failed to prepare update book statement: %w", err) } stmtInsertBookContent, err := tx.PreparexContext(ctx, `INSERT OR REPLACE INTO bookmark_content (docid, title, content, html) VALUES (?, ?, ?, ?)`) if err != nil { - if err != nil { - return fmt.Errorf("failed to prepare insert book content statement: %w", err) - } - return nil + return fmt.Errorf("failed to prepare insert book content statement: %w", err) } stmtUpdateBookContent, err := tx.PreparexContext(ctx, `UPDATE bookmark_content SET title = ?, content = ?, html = ? WHERE docid = ?`) if err != nil { - if err != nil { - return fmt.Errorf("failed to prepare update book content statement: %w", err) - } - return nil + return fmt.Errorf("failed to prepare update book content statement: %w", err) } stmtGetTag, err := tx.PreparexContext(ctx, `SELECT id FROM tag WHERE name = ?`) if err != nil { - if err != nil { - return fmt.Errorf("failed to prepare get tag statement: %w", err) - } - return nil + return fmt.Errorf("failed to prepare get tag statement: %w", err) } stmtInsertTag, err := tx.PreparexContext(ctx, `INSERT INTO tag (name) VALUES (?)`) if err != nil { - if err != nil { - return fmt.Errorf("failed to prepare insert tag statement: %w", err) - } - return nil + return fmt.Errorf("failed to prepare insert tag statement: %w", err) } stmtInsertBookTag, err := tx.PreparexContext(ctx, `INSERT OR IGNORE INTO bookmark_tag (tag_id, bookmark_id) VALUES (?, ?)`) if err != nil { - if err != nil { - return fmt.Errorf("failed to prepare insert book tag statement: %w", err) - } - return nil + return fmt.Errorf("failed to prepare insert book tag statement: %w", err) } stmtDeleteBookTag, err := tx.PreparexContext(ctx, `DELETE FROM bookmark_tag WHERE bookmark_id = ? AND tag_id = ?`) if err != nil { - if err != nil { - return fmt.Errorf("failed to execute delete statement: %w", err) - } - return nil + return fmt.Errorf("failed to execute delete statement: %w", err) } // Prepare modified time @@ -346,37 +310,25 @@ func (db *SQLiteDatabase) SaveBookmarks(ctx context.Context, create bool, bookma book.URL, book.Title, book.Excerpt, book.Author, book.Public, book.ModifiedAt, hasContent, book.ID) } if err != nil { - if err != nil { - return fmt.Errorf("failed to delete bookmark content: %w", err) - } - return nil + return fmt.Errorf("failed to delete bookmark content: %w", err) } // Try to update it first to check for existence, we can't do an UPSERT here because // bookmant_content is a virtual table res, err := stmtUpdateBookContent.ExecContext(ctx, book.Title, book.Content, book.HTML, book.ID) if err != nil { - if err != nil { - return fmt.Errorf("failed to delete bookmark tag: %w", err) - } - return nil + return fmt.Errorf("failed to delete bookmark tag: %w", err) } rows, err := res.RowsAffected() if err != nil { - if err != nil { - return fmt.Errorf("failed to delete bookmark: %w", err) - } - return nil + return fmt.Errorf("failed to delete bookmark: %w", err) } if rows == 0 { _, err = stmtInsertBookContent.ExecContext(ctx, book.ID, book.Title, book.Content, book.HTML) if err != nil { - if err != nil { - return fmt.Errorf("failed to execute delete bookmark tag statement: %w", err) - } - return nil + return fmt.Errorf("failed to execute delete bookmark tag statement: %w", err) } } @@ -387,10 +339,7 @@ func (db *SQLiteDatabase) SaveBookmarks(ctx context.Context, create bool, bookma if tag.Deleted { _, err = stmtDeleteBookTag.ExecContext(ctx, book.ID, tag.ID) if err != nil { - if err != nil { - return fmt.Errorf("failed to execute delete bookmark statement: %w", err) - } - return nil + return fmt.Errorf("failed to execute delete bookmark statement: %w", err) } continue } @@ -402,38 +351,26 @@ func (db *SQLiteDatabase) SaveBookmarks(ctx context.Context, create bool, bookma // If tag doesn't have any ID, fetch it from database if tag.ID == 0 { if err := stmtGetTag.GetContext(ctx, &tag.ID, tagName); err != nil && err != sql.ErrNoRows { - if err != nil { - return fmt.Errorf("failed to get tag ID: %w", err) - } - return nil + return fmt.Errorf("failed to get tag ID: %w", err) } // If tag doesn't exist in database, save it if tag.ID == 0 { res, err := stmtInsertTag.ExecContext(ctx, tagName) if err != nil { - if err != nil { - return fmt.Errorf("failed to get last insert ID for tag: %w", err) - } - return nil + return fmt.Errorf("failed to get last insert ID for tag: %w", err) } tagID64, err := res.LastInsertId() if err != nil && err != sql.ErrNoRows { - if err != nil { - return fmt.Errorf("failed to insert bookmark tag: %w", err) - } - return nil + return fmt.Errorf("failed to insert bookmark tag: %w", err) } tag.ID = int(tagID64) } if _, err := stmtInsertBookTag.ExecContext(ctx, tag.ID, book.ID); err != nil { - if err != nil { - return fmt.Errorf("failed to execute bookmark tag statement: %w", err) - } - return nil + return fmt.Errorf("failed to execute bookmark tag statement: %w", err) } } @@ -446,10 +383,7 @@ func (db *SQLiteDatabase) SaveBookmarks(ctx context.Context, create bool, bookma return nil }); err != nil { - if err != nil { - return nil, fmt.Errorf("failed to execute select query for bookmark content: %w", err) - } - return nil, nil + return nil, fmt.Errorf("failed to execute select query for bookmark content: %w", err) } return result, nil @@ -570,20 +504,14 @@ func (db *SQLiteDatabase) GetBookmarks(ctx context.Context, opts GetBookmarksOpt // Expand query, because some of the args might be an array query, args, err := sqlx.In(query, args...) if err != nil { - if err != nil { - return nil, fmt.Errorf("failed to execute select query for tags: %w", err) - } - return nil, nil + return nil, fmt.Errorf("failed to execute select query for tags: %w", err) } // Fetch bookmarks bookmarks := []model.BookmarkDTO{} err = db.reader.SelectContext(ctx, &bookmarks, query, args...) if err != nil && err != sql.ErrNoRows { - if err != nil { - return nil, fmt.Errorf("failed to fetch accounts: %w", err) - } - return nil, nil + return nil, fmt.Errorf("failed to fetch accounts: %w", err) } // store bookmark IDs for further enrichment @@ -605,18 +533,12 @@ func (db *SQLiteDatabase) GetBookmarks(ctx context.Context, opts GetBookmarksOpt contentQuery, args, err := sqlx.In(`SELECT docid, content, html FROM bookmark_content WHERE docid IN (?)`, bookmarkIds) contentQuery = db.reader.Rebind(contentQuery) if err != nil { - if err != nil { - return nil, fmt.Errorf("failed to expand tags query with IN clause: %w", err) - } - return nil, nil + return nil, fmt.Errorf("failed to expand tags query with IN clause: %w", err) } err = db.reader.Select(&contents, contentQuery, args...) if err != nil && err != sql.ErrNoRows { - if err != nil { - return nil, fmt.Errorf("failed to get tags: %w", err) - } - return nil, nil + return nil, fmt.Errorf("failed to get tags: %w", err) } for _, content := range contents { contentMap[content.ID] = content @@ -644,18 +566,12 @@ func (db *SQLiteDatabase) GetBookmarks(ctx context.Context, opts GetBookmarksOpt ORDER BY t.name`, bookmarkIds) tagsQuery = db.reader.Rebind(tagsQuery) if err != nil { - if err != nil { - return nil, fmt.Errorf("failed to delete bookmark and related records: %w", err) - } - return nil, nil + return nil, fmt.Errorf("failed to delete bookmark and related records: %w", err) } err = db.reader.Select(&tags, tagsQuery, tagArgs...) if err != nil && err != sql.ErrNoRows { - if err != nil { - return nil, fmt.Errorf("failed to get tags: %w", err) - } - return nil, nil + return nil, fmt.Errorf("failed to get tags: %w", err) } for _, fetchedTag := range tags { if tags, found := tagsMap[fetchedTag.ID]; found { @@ -767,20 +683,14 @@ func (db *SQLiteDatabase) GetBookmarksCount(ctx context.Context, opts GetBookmar // Expand query, because some of the args might be an array query, args, err := sqlx.In(query, args...) if err != nil { - if err != nil { - return 0, fmt.Errorf("failed to expand query with IN clause: %w", err) - } - return 0, nil + return 0, fmt.Errorf("failed to expand query with IN clause: %w", err) } // Fetch count var nBookmarks int err = db.reader.GetContext(ctx, &nBookmarks, query, args...) if err != nil && err != sql.ErrNoRows { - if err != nil { - return 0, fmt.Errorf("failed to get bookmark count: %w", err) - } - return 0, nil + return 0, fmt.Errorf("failed to get bookmark count: %w", err) } return nBookmarks, nil @@ -798,26 +708,17 @@ func (db *SQLiteDatabase) DeleteBookmarks(ctx context.Context, ids ...int) error if len(ids) == 0 { _, err := tx.ExecContext(ctx, delBookmarkContent) if err != nil { - if err != nil { - return fmt.Errorf("failed to prepare delete statement: %w", err) - } - return nil + return fmt.Errorf("failed to prepare delete statement: %w", err) } _, err = tx.ExecContext(ctx, delBookmarkTag) if err != nil { - if err != nil { - return fmt.Errorf("failed to execute delete account statement: %w", err) - } - return nil + return fmt.Errorf("failed to execute delete account statement: %w", err) } _, err = tx.ExecContext(ctx, delBookmark) if err != nil { - if err != nil { - return fmt.Errorf("failed to execute delete bookmark statement: %w", err) - } - return nil + return fmt.Errorf("failed to execute delete bookmark statement: %w", err) } } else { delBookmark += ` WHERE id = ?` @@ -826,35 +727,23 @@ func (db *SQLiteDatabase) DeleteBookmarks(ctx context.Context, ids ...int) error stmtDelBookmark, err := tx.Preparex(delBookmark) if err != nil { - if err != nil { - return fmt.Errorf("failed to get bookmark: %w", err) - } - return nil + return fmt.Errorf("failed to get bookmark: %w", err) } stmtDelBookmarkTag, err := tx.Preparex(delBookmarkTag) if err != nil { - if err != nil { - return fmt.Errorf("failed to expand query with IN clause: %w", err) - } - return nil + return fmt.Errorf("failed to expand query with IN clause: %w", err) } stmtDelBookmarkContent, err := tx.Preparex(delBookmarkContent) if err != nil { - if err != nil { - return fmt.Errorf("failed to delete bookmark content: %w", err) - } - return nil + return fmt.Errorf("failed to delete bookmark content: %w", err) } for _, id := range ids { _, err = stmtDelBookmarkContent.ExecContext(ctx, id) if err != nil { - if err != nil { - return fmt.Errorf("failed to delete bookmark: %w", err) - } - return nil + return fmt.Errorf("failed to delete bookmark: %w", err) } _, err = stmtDelBookmarkTag.ExecContext(ctx, id) @@ -871,10 +760,7 @@ func (db *SQLiteDatabase) DeleteBookmarks(ctx context.Context, ids ...int) error return nil }); err != nil { - if err != nil { - return fmt.Errorf("failed to update database schema version: %w", err) - } - return nil + return fmt.Errorf("failed to update database schema version: %w", err) } return nil @@ -925,10 +811,7 @@ func (db *SQLiteDatabase) SaveAccount(ctx context.Context, account model.Account } return nil }); err != nil { - if err != nil { - return fmt.Errorf("failed to insert/update account: %w", err) - } - return nil + return fmt.Errorf("failed to insert/update account: %w", err) } return nil @@ -947,10 +830,7 @@ func (db *SQLiteDatabase) SaveAccountSettings(ctx context.Context, account model } return nil }); err != nil { - if err != nil { - return fmt.Errorf("failed to prepare delete book tag statement: %w", err) - } - return nil + return fmt.Errorf("failed to prepare delete book tag statement: %w", err) } return nil @@ -977,10 +857,7 @@ func (db *SQLiteDatabase) GetAccounts(ctx context.Context, opts GetAccountsOptio accounts := []model.Account{} err := db.reader.SelectContext(ctx, &accounts, query, args...) if err != nil && err != sql.ErrNoRows { - if err != nil { - return nil, fmt.Errorf("failed to execute select query: %w", err) - } - return nil, nil + return nil, fmt.Errorf("failed to execute select query: %w", err) } return accounts, nil @@ -994,10 +871,10 @@ func (db *SQLiteDatabase) GetAccount(ctx context.Context, username string) (mode id, username, password, owner, config FROM account WHERE username = ?`, username, ); err != nil { - if err != nil && err != sql.ErrNoRows { - return account, false, fmt.Errorf("failed to get account: %w", err) + if err != sql.ErrNoRows { + return account, false, fmt.Errorf("account does not exist %w", err) } - return account, true, nil + return account, false, fmt.Errorf("failed to get account: %w", err) } return account, account.ID != 0, nil @@ -1009,28 +886,19 @@ func (db *SQLiteDatabase) DeleteAccounts(ctx context.Context, usernames ...strin // Delete account stmtDelete, err := tx.Preparex(`DELETE FROM account WHERE username = ?`) if err != nil { - if err != nil { - return fmt.Errorf("failed to insert tag: %w", err) - } - return nil + return fmt.Errorf("failed to insert tag: %w", err) } for _, username := range usernames { _, err := stmtDelete.ExecContext(ctx, username) if err != nil { - if err != nil { - return fmt.Errorf("failed to delete bookmark tag: %w", err) - } - return nil + return fmt.Errorf("failed to delete bookmark tag: %w", err) } } return nil }); err != nil { - if err != nil { - return fmt.Errorf("failed to prepare statement: %w", err) - } - return nil + return fmt.Errorf("failed to prepare statement: %w", err) } return nil @@ -1050,26 +918,17 @@ func (db *SQLiteDatabase) CreateTags(ctx context.Context, tags ...model.Tag) err if err := db.withTx(ctx, func(tx *sqlx.Tx) error { stmt, err := tx.Preparex(query) if err != nil { - if err != nil { - return fmt.Errorf("failed to prepare tag creation query: %w", err) - } - return nil + return fmt.Errorf("failed to prepare tag creation query: %w", err) } _, err = stmt.ExecContext(ctx, values...) if err != nil { - if err != nil { - return fmt.Errorf("failed to execute tag creation query: %w", err) - } - return nil + return fmt.Errorf("failed to execute tag creation query: %w", err) } return nil }); err != nil { - if err != nil { - return fmt.Errorf("failed to run tag creation transaction: %w", err) - } - return nil + return fmt.Errorf("failed to run tag creation transaction: %w", err) } return nil @@ -1085,10 +944,7 @@ func (db *SQLiteDatabase) GetTags(ctx context.Context) ([]model.Tag, error) { err := db.reader.SelectContext(ctx, &tags, query) if err != nil && err != sql.ErrNoRows { - if err != nil { - return nil, fmt.Errorf("failed to prepare delete bookmark content statement: %w", err) - } - return nil, nil + return nil, fmt.Errorf("failed to prepare delete bookmark content statement: %w", err) } return tags, nil @@ -1100,10 +956,7 @@ func (db *SQLiteDatabase) RenameTag(ctx context.Context, id int, newName string) _, err := tx.ExecContext(ctx, `UPDATE tag SET name = ? WHERE id = ?`, newName, id) return err }); err != nil { - if err != nil { - return fmt.Errorf("failed to rename tag: %w", err) - } - return nil + return fmt.Errorf("failed to rename tag: %w", err) } return nil From 555803e710b0b9839ef2babf5248f5205c53bad5 Mon Sep 17 00:00:00 2001 From: "Felipe M." Date: Tue, 10 Dec 2024 16:51:50 +0100 Subject: [PATCH 18/19] tests: use testutil instead of a manual in memory sqlite db --- internal/domains/bookmarks_test.go | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/internal/domains/bookmarks_test.go b/internal/domains/bookmarks_test.go index c02f16960..ba0e9d315 100644 --- a/internal/domains/bookmarks_test.go +++ b/internal/domains/bookmarks_test.go @@ -4,9 +4,6 @@ import ( "context" "testing" - "github.com/go-shiori/shiori/internal/config" - "github.com/go-shiori/shiori/internal/database" - "github.com/go-shiori/shiori/internal/dependencies" "github.com/go-shiori/shiori/internal/domains" "github.com/go-shiori/shiori/internal/model" "github.com/go-shiori/shiori/internal/testutil" @@ -17,17 +14,10 @@ import ( func TestBookmarkDomain(t *testing.T) { fs := afero.NewMemMapFs() + ctx := context.Background() + logger := logrus.New() + _, deps := testutil.GetTestConfigurationAndDependencies(t, ctx, logger) - db, err := database.OpenSQLiteDatabase(context.TODO(), ":memory:") - require.NoError(t, err) - require.NoError(t, db.Migrate(context.TODO())) - - deps := &dependencies.Dependencies{ - Database: db, - Config: config.ParseServerConfiguration(context.TODO(), logrus.New()), - Log: logrus.New(), - Domains: &dependencies.Domains{}, - } deps.Domains.Storage = domains.NewStorageDomain(deps, fs) fs.MkdirAll("thumb", 0755) From 6cdcf3be7042327561005dfbe020c9a42895686f Mon Sep 17 00:00:00 2001 From: "Felipe M." Date: Tue, 10 Dec 2024 17:01:03 +0100 Subject: [PATCH 19/19] fix: openbsd sqlite connection --- internal/database/sqlite_openbsd.go | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/internal/database/sqlite_openbsd.go b/internal/database/sqlite_openbsd.go index 64d9c7d00..ad0919906 100644 --- a/internal/database/sqlite_openbsd.go +++ b/internal/database/sqlite_openbsd.go @@ -5,9 +5,9 @@ package database import ( "context" + "fmt" "github.com/jmoiron/sqlx" - "github.com/pkg/errors" _ "git.sr.ht/~emersion/go-sqlite3-fts5" _ "github.com/mattn/go-sqlite3" @@ -16,11 +16,24 @@ import ( // OpenSQLiteDatabase creates and open connection to new SQLite3 database. func OpenSQLiteDatabase(ctx context.Context, databasePath string) (sqliteDB *SQLiteDatabase, err error) { // Open database - db, err := sqlx.ConnectContext(ctx, "sqlite3", databasePath) + rwDB, err := sqlx.ConnectContext(ctx, "sqlite", databasePath) if err != nil { - return nil, errors.WithStack(err) + return nil, fmt.Errorf("error opening writer database: %w", err) + } + + rDB, err := sqlx.ConnectContext(ctx, "sqlite", databasePath) + if err != nil { + return nil, fmt.Errorf("error opening reader database: %w", err) + } + + sqliteDB = &SQLiteDatabase{ + writer: &dbbase{rwDB}, + reader: &dbbase{rDB}, + } + + if err := sqliteDB.Init(ctx); err != nil { + return nil, fmt.Errorf("error initializing database: %w", err) } - sqliteDB = &SQLiteDatabase{dbbase: dbbase{db}} return sqliteDB, nil }