From 91c3e1496f4d3e29ca84ac6ed7c5e2eabc5bf630 Mon Sep 17 00:00:00 2001 From: Andras Banki-Horvath Date: Mon, 25 Nov 2024 20:29:08 +0100 Subject: [PATCH] sqldb: separate migration execution from construction This commit separates the execution of SQL and in-code migrations from their construction. This change is necessary because, currently, the SQL schema is migrated during the construction phase in the lncfg package. However, migrations are typically executed when individual stores are constructed within the configuration builder. --- config_builder.go | 26 +++++++++++++++++++------ lncfg/db.go | 19 +++++++++---------- sqldb/interfaces.go | 12 ++++++++++++ sqldb/migrations_test.go | 31 ++++++++++++++++++------------ sqldb/no_sqlite.go | 24 ++++++++++++++++++++++- sqldb/postgres.go | 34 ++++++++++++++++++++------------- sqldb/postgres_fixture.go | 8 ++++++-- sqldb/sqlite.go | 40 ++++++++++++++++++++++++++------------- 8 files changed, 137 insertions(+), 57 deletions(-) diff --git a/config_builder.go b/config_builder.go index afafbed75..b5e19e5dc 100644 --- a/config_builder.go +++ b/config_builder.go @@ -932,10 +932,10 @@ type DatabaseInstances struct { // the btcwallet's loader. WalletDB btcwallet.LoaderOption - // NativeSQLStore is a pointer to a native SQL store that can be used - // for native SQL queries for tables that already support it. This may - // be nil if the use-native-sql flag was not set. - NativeSQLStore *sqldb.BaseDB + // NativeSQLStore holds a reference to the native SQL store that can + // be used for native SQL queries for tables that already support it. + // This may be nil if the use-native-sql flag was not set. + NativeSQLStore sqldb.DB } // DefaultDatabaseBuilder is a type that builds the default database backends @@ -1079,6 +1079,19 @@ func (d *DefaultDatabaseBuilder) BuildDatabase( // Instantiate a native SQL invoice store if the flag is set. if d.cfg.DB.UseNativeSQL { + // We need to apply all migrations to the native SQL store + // before we can use it. + err := dbs.NativeSQLStore.ApplyAllMigrations( + ctx, sqldb.GetMigrations(), + ) + if err != nil { + cleanUp() + err := fmt.Errorf("unable to apply migrations: %w", err) + d.logger.Error(err) + + return nil, nil, err + } + // KV invoice db resides in the same database as the channel // state DB. Let's query the database to see if we have any // invoices there. If we do, we won't allow the user to start @@ -1107,10 +1120,11 @@ func (d *DefaultDatabaseBuilder) BuildDatabase( return nil, nil, err } + baseDB := dbs.NativeSQLStore.GetBaseDB() executor := sqldb.NewTransactionExecutor( - dbs.NativeSQLStore, + baseDB, func(tx *sql.Tx) invoices.SQLInvoiceQueries { - return dbs.NativeSQLStore.WithTx(tx) + return baseDB.WithTx(tx) }, ) diff --git a/lncfg/db.go b/lncfg/db.go index a6598e66d..040b3e8d1 100644 --- a/lncfg/db.go +++ b/lncfg/db.go @@ -231,10 +231,10 @@ type DatabaseBackends struct { // the underlying wallet database from. WalletDB btcwallet.LoaderOption - // NativeSQLStore is a pointer to a native SQL store that can be used - // for native SQL queries for tables that already support it. This may - // be nil if the use-native-sql flag was not set. - NativeSQLStore *sqldb.BaseDB + // NativeSQLStore holds a reference to the native SQL store that can + // be used for native SQL queries for tables that already support it. + // This may be nil if the use-native-sql flag was not set. + NativeSQLStore sqldb.DB // Remote indicates whether the database backends are remote, possibly // replicated instances or local bbolt or sqlite backed databases. @@ -449,17 +449,17 @@ func (db *DB) GetBackends(ctx context.Context, chanDBPath, } closeFuncs[NSWalletDB] = postgresWalletBackend.Close - var nativeSQLStore *sqldb.BaseDB + var nativeSQLStore sqldb.DB if db.UseNativeSQL { nativePostgresStore, err := sqldb.NewPostgresStore( - db.Postgres, sqldb.GetMigrations(), + db.Postgres, ) if err != nil { return nil, fmt.Errorf("error opening "+ "native postgres store: %v", err) } - nativeSQLStore = nativePostgresStore.BaseDB + nativeSQLStore = nativePostgresStore closeFuncs[PostgresBackend] = nativePostgresStore.Close } @@ -571,19 +571,18 @@ func (db *DB) GetBackends(ctx context.Context, chanDBPath, } closeFuncs[NSWalletDB] = sqliteWalletBackend.Close - var nativeSQLStore *sqldb.BaseDB + var nativeSQLStore sqldb.DB if db.UseNativeSQL { nativeSQLiteStore, err := sqldb.NewSqliteStore( db.Sqlite, path.Join(chanDBPath, SqliteNativeDBName), - sqldb.GetMigrations(), ) if err != nil { return nil, fmt.Errorf("error opening "+ "native SQLite store: %v", err) } - nativeSQLStore = nativeSQLiteStore.BaseDB + nativeSQLStore = nativeSQLiteStore closeFuncs[SqliteBackend] = nativeSQLiteStore.Close } diff --git a/sqldb/interfaces.go b/sqldb/interfaces.go index 3c042aa5a..1c5b4878f 100644 --- a/sqldb/interfaces.go +++ b/sqldb/interfaces.go @@ -355,6 +355,18 @@ func (t *TransactionExecutor[Q]) ExecTx(ctx context.Context, ) } +// DB is an interface that represents a generic SQL database. It provides +// methods to apply migrations and access the underlying database connection. +type DB interface { + // GetBaseDB returns the underlying BaseDB instance. + GetBaseDB() *BaseDB + + // ApplyAllMigrations applies all migrations to the database including + // both sqlc and custom in-code migrations. + ApplyAllMigrations(ctx context.Context, + customMigrations []MigrationConfig) error +} + // BaseDB is the base database struct that each implementation can embed to // gain some common functionality. type BaseDB struct { diff --git a/sqldb/migrations_test.go b/sqldb/migrations_test.go index 284ba8e99..385840364 100644 --- a/sqldb/migrations_test.go +++ b/sqldb/migrations_test.go @@ -314,16 +314,19 @@ func TestCustomMigration(t *testing.T) { for i := 0; i < 3; i++ { db, err = NewSqliteStore(&SqliteConfig{ SkipMigrations: false, - }, dbFileName, test.migrations) - if db != nil { - dbToCleanup := db.DB - t.Cleanup(func() { - require.NoError( - t, dbToCleanup.Close(), - ) - }) - } + }, dbFileName) + require.NoError(t, err) + dbToCleanup := db.DB + t.Cleanup(func() { + require.NoError( + t, dbToCleanup.Close(), + ) + }) + + err = db.ApplyAllMigrations( + ctxb, test.migrations, + ) if test.expectedSuccess { require.NoError(t, err) } else { @@ -333,7 +336,7 @@ func TestCustomMigration(t *testing.T) { // so we can read versions. db, err = NewSqliteStore(&SqliteConfig{ SkipMigrations: true, - }, dbFileName, nil) + }, dbFileName) require.NoError(t, err) } @@ -399,8 +402,12 @@ func TestCustomMigration(t *testing.T) { // are idempotent. for i := 0; i < 3; i++ { cfg.SkipMigrations = false - db, err = NewPostgresStore(cfg, test.migrations) + db, err = NewPostgresStore(cfg) + require.NoError(t, err) + err = db.ApplyAllMigrations( + ctxb, test.migrations, + ) if test.expectedSuccess { require.NoError(t, err) } else { @@ -409,7 +416,7 @@ func TestCustomMigration(t *testing.T) { // Also repoen the DB without migrations // so we can read versions. cfg.SkipMigrations = true - db, err = NewPostgresStore(cfg, nil) + db, err = NewPostgresStore(cfg) require.NoError(t, err) } diff --git a/sqldb/no_sqlite.go b/sqldb/no_sqlite.go index 9ea35c43c..ad0cae6e4 100644 --- a/sqldb/no_sqlite.go +++ b/sqldb/no_sqlite.go @@ -2,7 +2,15 @@ package sqldb -import "fmt" +import ( + "context" + "fmt" +) + +var ( + // Make sure SqliteStore implements the DB interface. + _ DB = (*SqliteStore)(nil) +) // SqliteStore is a database store implementation that uses a sqlite backend. type SqliteStore struct { @@ -16,3 +24,17 @@ type SqliteStore struct { func NewSqliteStore(cfg *SqliteConfig, dbPath string) (*SqliteStore, error) { return nil, fmt.Errorf("SQLite backend not supported in WebAssembly") } + +// GetBaseDB returns the underlying BaseDB instance for the SQLite store. +// It is a trivial helper method to comply with the sqldb.DB interface. +func (s *SqliteStore) GetBaseDB() *BaseDB { + return s.BaseDB +} + +// ApplyAllMigrations applies both the SQLC and custom in-code migrations to +// the SQLite database. +func (s *SqliteStore) ApplyAllMigrations(context.Context, + []MigrationConfig) error { + + return fmt.Errorf("SQLite backend not supported in WebAssembly") +} diff --git a/sqldb/postgres.go b/sqldb/postgres.go index 4884943f0..455ecb405 100644 --- a/sqldb/postgres.go +++ b/sqldb/postgres.go @@ -36,6 +36,9 @@ var ( // Make sure PostgresStore implements the MigrationExecutor interface. _ MigrationExecutor = (*PostgresStore)(nil) + + // Make sure PostgresStore implements the DB interface. + _ DB = (*PostgresStore)(nil) ) // replacePasswordInDSN takes a DSN string and returns it with the password @@ -89,9 +92,7 @@ type PostgresStore struct { // NewPostgresStore creates a new store that is backed by a Postgres database // backend. -func NewPostgresStore(cfg *PostgresConfig, migrations []MigrationConfig) ( - *PostgresStore, error) { - +func NewPostgresStore(cfg *PostgresConfig) (*PostgresStore, error) { sanitizedDSN, err := replacePasswordInDSN(cfg.Dsn) if err != nil { return nil, err @@ -130,25 +131,32 @@ func NewPostgresStore(cfg *PostgresConfig, migrations []MigrationConfig) ( queries := sqlc.New(db) - s := &PostgresStore{ + return &PostgresStore{ cfg: cfg, BaseDB: &BaseDB{ DB: db, Queries: queries, }, - } + }, nil +} + +// GetBaseDB returns the underlying BaseDB instance for the Postgres store. +// It is a trivial helper method to comply with the sqldb.DB interface. +func (s *PostgresStore) GetBaseDB() *BaseDB { + return s.BaseDB +} + +// ApplyAllMigrations applies both the SQLC and custom in-code migrations to the +// Postgres database. +func (s *PostgresStore) ApplyAllMigrations(ctx context.Context, + migrations []MigrationConfig) error { // Execute migrations unless configured to skip them. - if !cfg.SkipMigrations { - err := ApplyMigrations( - context.Background(), s.BaseDB, s, migrations, - ) - if err != nil { - return nil, err - } + if s.cfg.SkipMigrations { + return nil } - return s, nil + return ApplyMigrations(ctx, s.BaseDB, s, migrations) } // ExecuteMigrations runs migrations for the Postgres database, depending on the diff --git a/sqldb/postgres_fixture.go b/sqldb/postgres_fixture.go index 284cd0c8c..ce21aab7d 100644 --- a/sqldb/postgres_fixture.go +++ b/sqldb/postgres_fixture.go @@ -148,9 +148,13 @@ func NewTestPostgresDB(t *testing.T, fixture *TestPgFixture) *PostgresStore { require.NoError(t, err) cfg := fixture.GetConfig(dbName) - store, err := NewPostgresStore(cfg, GetMigrations()) + store, err := NewPostgresStore(cfg) require.NoError(t, err) + require.NoError(t, store.ApplyAllMigrations( + context.Background(), GetMigrations()), + ) + return store } @@ -172,7 +176,7 @@ func NewTestPostgresDBWithVersion(t *testing.T, fixture *TestPgFixture, storeCfg := fixture.GetConfig(dbName) storeCfg.SkipMigrations = true - store, err := NewPostgresStore(storeCfg, GetMigrations()) + store, err := NewPostgresStore(storeCfg) require.NoError(t, err) err = store.ExecuteMigrations(TargetVersion(version)) diff --git a/sqldb/sqlite.go b/sqldb/sqlite.go index bf192eb0f..59cb03569 100644 --- a/sqldb/sqlite.go +++ b/sqldb/sqlite.go @@ -38,6 +38,9 @@ var ( // Make sure SqliteStore implements the MigrationExecutor interface. _ MigrationExecutor = (*SqliteStore)(nil) + + // Make sure SqliteStore implements the DB interface. + _ DB = (*SqliteStore)(nil) ) // SqliteStore is a database store implementation that uses a sqlite backend. @@ -49,9 +52,7 @@ type SqliteStore struct { // NewSqliteStore attempts to open a new sqlite database based on the passed // config. -func NewSqliteStore(cfg *SqliteConfig, dbPath string, - migrations []MigrationConfig) (*SqliteStore, error) { - +func NewSqliteStore(cfg *SqliteConfig, dbPath string) (*SqliteStore, error) { // The set of pragma options are accepted using query options. For now // we only want to ensure that foreign key constraints are properly // enforced. @@ -138,17 +139,26 @@ func NewSqliteStore(cfg *SqliteConfig, dbPath string, }, } + return s, nil +} + +// GetBaseDB returns the underlying BaseDB instance for the SQLite store. +// It is a trivial helper method to comply with the sqldb.DB interface. +func (s *SqliteStore) GetBaseDB() *BaseDB { + return s.BaseDB +} + +// ApplyAllMigrations applies both the SQLC and custom in-code migrations to the +// SQLite database. +func (s *SqliteStore) ApplyAllMigrations(ctx context.Context, + migrations []MigrationConfig) error { + // Execute migrations unless configured to skip them. - if !cfg.SkipMigrations { - err := ApplyMigrations( - context.Background(), s.BaseDB, s, migrations, - ) - if err != nil { - return nil, err - } + if s.cfg.SkipMigrations { + return nil } - return s, nil + return ApplyMigrations(ctx, s.BaseDB, s, migrations) } // ExecuteMigrations runs migrations for the sqlite database, depending on the @@ -181,9 +191,13 @@ func NewTestSqliteDB(t *testing.T) *SqliteStore { dbFileName := filepath.Join(t.TempDir(), "tmp.db") sqlDB, err := NewSqliteStore(&SqliteConfig{ SkipMigrations: false, - }, dbFileName, GetMigrations()) + }, dbFileName) require.NoError(t, err) + require.NoError(t, sqlDB.ApplyAllMigrations( + context.Background(), GetMigrations()), + ) + t.Cleanup(func() { require.NoError(t, sqlDB.DB.Close()) }) @@ -204,7 +218,7 @@ func NewTestSqliteDBWithVersion(t *testing.T, version uint) *SqliteStore { dbFileName := filepath.Join(t.TempDir(), "tmp.db") sqlDB, err := NewSqliteStore(&SqliteConfig{ SkipMigrations: true, - }, dbFileName, nil) + }, dbFileName) require.NoError(t, err) err = sqlDB.ExecuteMigrations(TargetVersion(version))