-
-
Notifications
You must be signed in to change notification settings - Fork 3
feat: simplify database migration system for local-first application #274
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| package io.github.kamiazya.scopes.devicesync.infrastructure.migration | ||
|
|
||
| import io.github.kamiazya.scopes.platform.infrastructure.database.migration.Migration | ||
| import io.github.kamiazya.scopes.platform.infrastructure.database.migration.scanner.ResourceMigrationDiscovery | ||
| import io.github.kamiazya.scopes.platform.observability.logging.Logger | ||
|
|
||
| /** | ||
| * Provides migrations for the Device Synchronization bounded context. | ||
| */ | ||
| class DeviceSyncMigrationProvider(private val logger: Logger) { | ||
| private val discovery = ResourceMigrationDiscovery( | ||
| resourcePath = "migrations/device-sync", | ||
| classLoader = this::class.java.classLoader, | ||
| logger = logger, | ||
| ) | ||
|
|
||
| fun getMigrations(): List<Migration> = discovery.discoverMigrations() | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,35 +1,50 @@ | ||
| package io.github.kamiazya.scopes.devicesync.infrastructure.sqldelight | ||
|
|
||
| import app.cash.sqldelight.db.SqlDriver | ||
| import app.cash.sqldelight.driver.jdbc.sqlite.JdbcSqliteDriver | ||
| import io.github.kamiazya.scopes.devicesync.db.DeviceSyncDatabase | ||
| import io.github.kamiazya.scopes.devicesync.infrastructure.migration.DeviceSyncMigrationProvider | ||
| import io.github.kamiazya.scopes.platform.infrastructure.database.ManagedSqlDriver | ||
| import io.github.kamiazya.scopes.platform.infrastructure.database.migration.applyMigrations | ||
| import io.github.kamiazya.scopes.platform.observability.logging.ConsoleLogger | ||
| import kotlinx.coroutines.runBlocking | ||
|
|
||
| /** | ||
| * Provides SQLDelight database instances for Device Synchronization. | ||
| * Provides SQLDelight database instances for Device Synchronization with automatic migrations. | ||
| */ | ||
| object SqlDelightDatabaseProvider { | ||
|
|
||
| /** | ||
| * Creates a new DeviceSyncDatabase instance. | ||
| */ | ||
| fun createDatabase(databasePath: String): DeviceSyncDatabase { | ||
| val driver: SqlDriver = JdbcSqliteDriver("jdbc:sqlite:$databasePath") | ||
|
|
||
| // Create the database schema | ||
| DeviceSyncDatabase.Schema.create(driver) | ||
| class ManagedDatabase(private val database: DeviceSyncDatabase, private val managedDriver: AutoCloseable) : | ||
| DeviceSyncDatabase by database, | ||
| AutoCloseable { | ||
| override fun close() = managedDriver.close() | ||
| } | ||
|
|
||
| // Enable foreign keys | ||
| driver.execute(null, "PRAGMA foreign_keys=ON", 0) | ||
| fun createDatabase(databasePath: String): DeviceSyncDatabase { | ||
| val managedDriver = ManagedSqlDriver.createWithDefaults(databasePath) | ||
| val driver = managedDriver.driver | ||
|
|
||
| return DeviceSyncDatabase(driver) | ||
| val logger = ConsoleLogger("DeviceSyncDB") | ||
| val migrations = DeviceSyncMigrationProvider(logger).getMigrations() | ||
| runBlocking { | ||
| driver.applyMigrations(migrations, logger).fold( | ||
| ifLeft = { err -> error("Migration failed: ${err.message}") }, | ||
| ifRight = { }, | ||
| ) | ||
| } | ||
| return ManagedDatabase(DeviceSyncDatabase(driver), managedDriver) | ||
| } | ||
|
Comment on lines
+21
to
34
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Leaked lifecycle: returning DeviceSyncDatabase hides close() createDatabase returns DeviceSyncDatabase, so callers can’t invoke close() on the ManagedDatabase wrapper, risking driver leaks. Two options:
- fun createDatabase(databasePath: String): DeviceSyncDatabase {
+ fun createDatabase(databasePath: String): ManagedDatabase {
...
- return ManagedDatabase(DeviceSyncDatabase(driver), managedDriver)
+ return ManagedDatabase(DeviceSyncDatabase(driver), managedDriver)
}
🤖 Prompt for AI Agents |
||
|
|
||
| /** | ||
| * Creates an in-memory database for testing. | ||
| */ | ||
| fun createInMemoryDatabase(): DeviceSyncDatabase { | ||
| val driver: SqlDriver = JdbcSqliteDriver(JdbcSqliteDriver.IN_MEMORY) | ||
| DeviceSyncDatabase.Schema.create(driver) | ||
| return DeviceSyncDatabase(driver) | ||
| val managedDriver = ManagedSqlDriver(":memory:") | ||
| val driver = managedDriver.driver | ||
|
|
||
| val logger = ConsoleLogger("DeviceSyncDB-InMemory") | ||
| val migrations = DeviceSyncMigrationProvider(logger).getMigrations() | ||
| runBlocking { | ||
| driver.applyMigrations(migrations, logger).fold( | ||
| ifLeft = { err -> error("Migration failed: ${err.message}") }, | ||
| ifRight = { }, | ||
| ) | ||
| } | ||
| return ManagedDatabase(DeviceSyncDatabase(driver), managedDriver) | ||
| } | ||
| } | ||
|
Comment on lines
+21
to
50
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is significant code duplication between |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| -- Initial schema for Device Synchronization | ||
|
|
||
| CREATE TABLE IF NOT EXISTS devices ( | ||
| device_id TEXT PRIMARY KEY NOT NULL, | ||
| last_sync_at INTEGER, | ||
| last_successful_push INTEGER, | ||
| last_successful_pull INTEGER, | ||
| sync_status TEXT NOT NULL DEFAULT 'NEVER_SYNCED', | ||
| pending_changes INTEGER NOT NULL DEFAULT 0, | ||
| created_at INTEGER NOT NULL, | ||
| updated_at INTEGER NOT NULL | ||
| ); | ||
|
|
||
| CREATE INDEX IF NOT EXISTS idx_devices_sync_status ON devices(sync_status); | ||
| CREATE INDEX IF NOT EXISTS idx_devices_updated_at ON devices(updated_at); | ||
|
|
||
| CREATE TABLE IF NOT EXISTS vector_clocks ( | ||
| device_id TEXT NOT NULL, | ||
| component_device TEXT NOT NULL, | ||
| timestamp INTEGER NOT NULL, | ||
| PRIMARY KEY (device_id, component_device) | ||
| ); | ||
|
Comment on lines
+17
to
+22
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add FK to devices for referential integrity vector_clocks.device_id should reference devices(device_id). Apply this diff: CREATE TABLE IF NOT EXISTS vector_clocks (
device_id TEXT NOT NULL,
component_device TEXT NOT NULL,
timestamp INTEGER NOT NULL,
- PRIMARY KEY (device_id, component_device)
+ PRIMARY KEY (device_id, component_device),
+ FOREIGN KEY (device_id) REFERENCES devices(device_id) ON DELETE CASCADE
);🤖 Prompt for AI Agents |
||
|
|
||
| CREATE INDEX IF NOT EXISTS idx_vector_clocks_device_id ON vector_clocks(device_id); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| package io.github.kamiazya.scopes.eventstore.infrastructure.migration | ||
|
|
||
| import io.github.kamiazya.scopes.platform.infrastructure.database.migration.Migration | ||
| import io.github.kamiazya.scopes.platform.infrastructure.database.migration.scanner.ResourceMigrationDiscovery | ||
| import io.github.kamiazya.scopes.platform.observability.logging.Logger | ||
|
|
||
| /** | ||
| * Provides migrations for the Event Store bounded context. | ||
| * | ||
| * Migrations are discovered from classpath resources under migrations/event-store. | ||
| */ | ||
| class EventStoreMigrationProvider(private val logger: Logger) { | ||
| private val discovery = ResourceMigrationDiscovery( | ||
| resourcePath = "migrations/event-store", | ||
| classLoader = this::class.java.classLoader, | ||
| logger = logger, | ||
| ) | ||
|
|
||
| fun getMigrations(): List<Migration> = discovery.discoverMigrations() | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,35 +1,50 @@ | ||
| package io.github.kamiazya.scopes.eventstore.infrastructure.sqldelight | ||
|
|
||
| import app.cash.sqldelight.db.SqlDriver | ||
| import app.cash.sqldelight.driver.jdbc.sqlite.JdbcSqliteDriver | ||
| import io.github.kamiazya.scopes.eventstore.db.EventStoreDatabase | ||
| import io.github.kamiazya.scopes.eventstore.infrastructure.migration.EventStoreMigrationProvider | ||
| import io.github.kamiazya.scopes.platform.infrastructure.database.ManagedSqlDriver | ||
| import io.github.kamiazya.scopes.platform.infrastructure.database.migration.applyMigrations | ||
| import io.github.kamiazya.scopes.platform.observability.logging.ConsoleLogger | ||
| import kotlinx.coroutines.runBlocking | ||
|
|
||
| /** | ||
| * Provides SQLDelight database instances for Event Store. | ||
| * Provides SQLDelight database instances for Event Store with automatic migrations. | ||
| */ | ||
| object SqlDelightDatabaseProvider { | ||
|
|
||
| /** | ||
| * Creates a new EventStoreDatabase instance. | ||
| */ | ||
| fun createDatabase(databasePath: String): EventStoreDatabase { | ||
| val driver: SqlDriver = JdbcSqliteDriver("jdbc:sqlite:$databasePath") | ||
|
|
||
| // Create the database schema | ||
| EventStoreDatabase.Schema.create(driver) | ||
| class ManagedDatabase(private val database: EventStoreDatabase, private val managedDriver: AutoCloseable) : | ||
| EventStoreDatabase by database, | ||
| AutoCloseable { | ||
| override fun close() = managedDriver.close() | ||
| } | ||
|
|
||
| // Enable foreign keys | ||
| driver.execute(null, "PRAGMA foreign_keys=ON", 0) | ||
| fun createDatabase(databasePath: String): EventStoreDatabase { | ||
| val managedDriver = ManagedSqlDriver.createWithDefaults(databasePath) | ||
| val driver = managedDriver.driver | ||
|
|
||
| return EventStoreDatabase(driver) | ||
| val logger = ConsoleLogger("EventStoreDB") | ||
| val migrations = EventStoreMigrationProvider(logger).getMigrations() | ||
| runBlocking { | ||
| driver.applyMigrations(migrations, logger).fold( | ||
| ifLeft = { err -> error("Migration failed: ${err.message}") }, | ||
| ifRight = { }, | ||
| ) | ||
| } | ||
| return ManagedDatabase(EventStoreDatabase(driver), managedDriver) | ||
| } | ||
|
Comment on lines
+21
to
34
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Leaked lifecycle: returning EventStoreDatabase hides close() Same as device‑sync: callers can’t close the driver if return type is EventStoreDatabase. Preferred change: - fun createDatabase(databasePath: String): EventStoreDatabase {
+ fun createDatabase(databasePath: String): ManagedDatabase {
...
- return ManagedDatabase(EventStoreDatabase(driver), managedDriver)
+ return ManagedDatabase(EventStoreDatabase(driver), managedDriver)
}Alternatively, add createManagedDatabase(...) and deprecate the old method to maintain API compatibility. 🤖 Prompt for AI Agents |
||
|
|
||
| /** | ||
| * Creates an in-memory database for testing. | ||
| */ | ||
| fun createInMemoryDatabase(): EventStoreDatabase { | ||
| val driver: SqlDriver = JdbcSqliteDriver(JdbcSqliteDriver.IN_MEMORY) | ||
| EventStoreDatabase.Schema.create(driver) | ||
| return EventStoreDatabase(driver) | ||
| val managedDriver = ManagedSqlDriver(":memory:") | ||
| val driver = managedDriver.driver | ||
|
|
||
| val logger = ConsoleLogger("EventStoreDB-InMemory") | ||
| val migrations = EventStoreMigrationProvider(logger).getMigrations() | ||
| runBlocking { | ||
| driver.applyMigrations(migrations, logger).fold( | ||
| ifLeft = { err -> error("Migration failed: ${err.message}") }, | ||
| ifRight = { }, | ||
| ) | ||
| } | ||
| return ManagedDatabase(EventStoreDatabase(driver), managedDriver) | ||
| } | ||
| } | ||
|
Comment on lines
+21
to
50
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to other database providers in this PR, there's a lot of duplicated code between the |
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,19 @@ | ||||||||||||||||||||||||||||||||||||||||||||
| -- Initial schema for Event Store | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| CREATE TABLE IF NOT EXISTS events ( | ||||||||||||||||||||||||||||||||||||||||||||
| sequence_number INTEGER PRIMARY KEY AUTOINCREMENT, | ||||||||||||||||||||||||||||||||||||||||||||
| event_id TEXT NOT NULL UNIQUE, | ||||||||||||||||||||||||||||||||||||||||||||
| aggregate_id TEXT NOT NULL, | ||||||||||||||||||||||||||||||||||||||||||||
| aggregate_version INTEGER NOT NULL, | ||||||||||||||||||||||||||||||||||||||||||||
| event_type TEXT NOT NULL, | ||||||||||||||||||||||||||||||||||||||||||||
| event_data TEXT NOT NULL, | ||||||||||||||||||||||||||||||||||||||||||||
| occurred_at INTEGER NOT NULL, | ||||||||||||||||||||||||||||||||||||||||||||
| stored_at INTEGER NOT NULL | ||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+3
to
+12
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Enforce uniqueness of (aggregate_id, aggregate_version) Event stores must prevent duplicate versions per aggregate. Apply this diff to add a table-level constraint: CREATE TABLE IF NOT EXISTS events (
sequence_number INTEGER PRIMARY KEY AUTOINCREMENT,
event_id TEXT NOT NULL UNIQUE,
aggregate_id TEXT NOT NULL,
aggregate_version INTEGER NOT NULL,
event_type TEXT NOT NULL,
event_data TEXT NOT NULL,
occurred_at INTEGER NOT NULL,
- stored_at INTEGER NOT NULL
+ stored_at INTEGER NOT NULL,
+ UNIQUE (aggregate_id, aggregate_version)
);And update the index below (or remove it to avoid duplication). 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| CREATE INDEX IF NOT EXISTS idx_events_aggregate_id_version ON events(aggregate_id, aggregate_version); | ||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major Make the aggregate/version index UNIQUE or drop it The non-unique index conflicts with the integrity requirement above. Apply one of:
-CREATE INDEX IF NOT EXISTS idx_events_aggregate_id_version ON events(aggregate_id, aggregate_version);
+CREATE UNIQUE INDEX IF NOT EXISTS idx_events_aggregate_id_version ON events(aggregate_id, aggregate_version);
📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||
| CREATE INDEX IF NOT EXISTS idx_events_stored_at ON events(stored_at); | ||||||||||||||||||||||||||||||||||||||||||||
| CREATE INDEX IF NOT EXISTS idx_events_occurred_at ON events(occurred_at); | ||||||||||||||||||||||||||||||||||||||||||||
| CREATE INDEX IF NOT EXISTS idx_events_event_type_sequence_number ON events(event_type, sequence_number); | ||||||||||||||||||||||||||||||||||||||||||||
| CREATE INDEX IF NOT EXISTS idx_events_event_type_occurred_at ON events(event_type, occurred_at); | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| package io.github.kamiazya.scopes.scopemanagement.infrastructure.migration | ||
|
|
||
| import io.github.kamiazya.scopes.platform.infrastructure.database.migration.Migration | ||
| import io.github.kamiazya.scopes.platform.infrastructure.database.migration.scanner.ResourceMigrationDiscovery | ||
| import io.github.kamiazya.scopes.platform.observability.logging.Logger | ||
|
|
||
| /** | ||
| * Provides migrations for the Scope Management bounded context. | ||
| * | ||
| * Migrations are discovered from the classpath under the migrations directory. | ||
| * They follow the naming convention: V{version}__{description}.sql | ||
| */ | ||
| class ScopeManagementMigrationProvider(private val logger: Logger) { | ||
| private val discovery = ResourceMigrationDiscovery( | ||
| resourcePath = "migrations/scope-management", | ||
| classLoader = this::class.java.classLoader, | ||
| logger = logger, | ||
| ) | ||
|
|
||
| /** | ||
| * Gets all migrations for the scope management context. | ||
| */ | ||
| fun getMigrations(): List<Migration> = discovery.discoverMigrations() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of
runBlockinghere to initialize the database within a Koin module is concerning. The pull request description mentions, "RemovedrunBlockingfrom DI modules in favor of lazy initialization," but this change seems to re-introduce it. While this is for a CLI application where blocking the main thread during startup might be less critical than in a GUI application, it's still an anti-pattern that can lead to performance problems and make the startup process rigid. It would be better to stick to asynchronous initialization, perhaps by having the provider return aDeferred<Database>or by making the creation suspendable and handling it at the application's entry point within a coroutine scope.