feat: simplify database migration system for local-first application #274
Conversation
- Remove unnecessary features (checksums, reversible migrations) - Rename methods for clarity (up→apply, upSql→sql) - Fix schema mismatches in V1 migration - Implement MigrationAwareDatabaseProvider pattern - Add transaction support for atomic migrations - Update native image configuration - Remove DatabaseMigrationBootstrapper - All tests passing (32 tests)
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a platform-wide migration subsystem (discovery, execution, tracking) and integrates migration-aware SqlDelight providers across Scope Management, Event Store, and Device Sync; adds initial SQL migrations, DI wiring, native resource patterns, and tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant DI as Koin Module
participant MADP as MigrationAwareDatabaseProvider
participant Driver as ManagedSqlDriver
participant MM as DefaultMigrationManager
participant Repo as SchemaVersionStore
participant DB as SQLDelight Database
App->>DI: request ScopeManagementDatabase
DI->>MADP: createDatabase(path)
MADP->>Driver: init(file/in-memory)
MADP->>MM: create with Executor + Repo + Migrations
MM->>Repo: ensure schema_versions
MM->>MM: getStatus()
alt pending migrations
MM->>MM: migrateUp()
MM->>Repo: save AppliedMigration(s)
else up-to-date
MM-->>MADP: status unchanged
end
MADP->>DB: construct via databaseFactory(driver)
MADP-->>DI: Either.Right(DB)
DI-->>App: database instance
sequenceDiagram
autonumber
actor Operator
participant Manager as MigrationManager
participant Exec as MigrationExecutor
participant Repo as SchemaVersionStore
participant Clock as Clock
Operator->>Manager: migrateUp()
Manager->>Repo: getCurrentVersion()
Manager->>Manager: determine pending migrations
loop each pending migration (ascending)
Manager->>Exec: executeSql(statements)
Manager->>Clock: now()
Manager->>Repo: saveAppliedMigration(AppliedMigration)
end
Manager-->>Operator: MigrationSummary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
Summary of ChangesHello @kamiazya, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors and simplifies the database migration system for the local-first application. The primary goal is to enhance reliability, safety, and maintainability by streamlining the migration process, ensuring automatic execution on startup, and guaranteeing database consistency through atomic transactions. The changes introduce a robust provider pattern for database initialization, centralizing migration logic and improving overall code clarity and performance. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable simplification of the database migration system, moving towards a more robust, provider-based pattern. The changes are well-structured, introducing a clear, reusable migration framework in the platform/infrastructure module and applying it across different contexts. The adoption of atomic transactions for migrations and the fail-fast approach to database initialization are excellent improvements for reliability.
My review focuses on several areas for further refinement. The most critical point is the use of runBlocking within Koin dependency injection modules, which contradicts the PR's goal of removing it and can lead to performance issues. Additionally, there are multiple instances of code duplication across the new database providers and integration helpers, which could be refactored for better maintainability. I've also identified some potentially unused code, a misleading method implementation, and a minor potential bug in SQL parsing. Addressing these points will help solidify this new migration architecture.
| kotlinx.coroutines.runBlocking { | ||
| provider.createDatabase(dbPath).fold( | ||
| ifLeft = { err -> | ||
| // Fail-fast using Kotlin's error() function | ||
| error("Failed to create database: ${err.message}") | ||
| }, | ||
| ifRight = { database -> | ||
| database | ||
| }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
The use of runBlocking here to initialize the database within a Koin module is concerning. The pull request description mentions, "Removed runBlocking from 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 a Deferred<Database> or by making the creation suspendable and handling it at the application's entry point within a coroutine scope.
| 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) | ||
| } | ||
|
|
||
| /** | ||
| * 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) | ||
| } | ||
| } |
There was a problem hiding this comment.
There is significant code duplication between createDatabase and createInMemoryDatabase. The logic for creating the driver, getting migrations, and applying them via runBlocking is nearly identical. This could be extracted into a private helper function to improve maintainability and reduce redundancy. The helper could take the database path or driver as a parameter.
| 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) | ||
| } | ||
|
|
||
| /** | ||
| * 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) | ||
| } | ||
| } |
There was a problem hiding this comment.
Similar to other database providers in this PR, there's a lot of duplicated code between the createDatabase and createInMemoryDatabase functions. The process of creating a driver, fetching migrations, and applying them is repeated. To improve maintainability, this common logic should be extracted into a private function.
| private fun provider(loggerName: String = "ScopeManagementDB"): MigrationAwareDatabaseProvider<ScopeManagementDatabase> { | ||
| val logger = ConsoleLogger(loggerName) | ||
| val migrations = { ScopeManagementMigrationProvider(logger = logger).getMigrations() } | ||
| return MigrationAwareDatabaseProvider( | ||
| migrationProvider = migrations, | ||
| config = MigrationConfig(maxRetries = 3), | ||
| logger = logger, | ||
| databaseFactory = { driver -> ScopeManagementDatabase(driver) }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
This private function provider appears to be unused within the SqlDelightDatabaseProvider object. It should be removed to avoid dead code.
| fun createDatabase(databasePath: String): ScopeManagementDatabase { | ||
| val managedDriver = ManagedSqlDriver.createWithDefaults(databasePath) | ||
| val driver = managedDriver.driver | ||
|
|
||
| // Create the database schema | ||
| ScopeManagementDatabase.Schema.create(driver) | ||
| val logger = ConsoleLogger("ScopeManagementDB") | ||
| val migrations = ScopeManagementMigrationProvider(logger = logger).getMigrations() | ||
| runBlocking { | ||
| driver.applyMigrations(migrations, logger).fold( | ||
| ifLeft = { err -> error("Migration failed: ${err.message}") }, | ||
| ifRight = { }, | ||
| ) | ||
| } | ||
|
|
||
| return ManagedDatabase(ScopeManagementDatabase(driver), managedDriver) | ||
| val db = ScopeManagementDatabase(driver) | ||
| return ManagedDatabase(db, managedDriver) | ||
| } | ||
|
|
||
| /** | ||
| * Creates an in-memory database for testing. | ||
| * Creates an in-memory database for testing with migrations applied. | ||
| */ | ||
| fun createInMemoryDatabase(): ScopeManagementDatabase { | ||
| val managedDriver = ManagedSqlDriver(":memory:") | ||
| val driver = managedDriver.driver | ||
|
|
||
| ScopeManagementDatabase.Schema.create(driver) | ||
| return ManagedDatabase(ScopeManagementDatabase(driver), managedDriver) | ||
| val logger = ConsoleLogger("ScopeManagementDB-InMemory") | ||
| val migrations = ScopeManagementMigrationProvider(logger = logger).getMigrations() | ||
| runBlocking { | ||
| driver.applyMigrations(migrations, logger).fold( | ||
| ifLeft = { err -> error("Migration failed: ${err.message}") }, | ||
| ifRight = { }, | ||
| ) | ||
| } | ||
|
|
||
| val db = ScopeManagementDatabase(driver) | ||
| return ManagedDatabase(db, managedDriver) | ||
| } |
There was a problem hiding this comment.
The createDatabase and createInMemoryDatabase functions contain a lot of duplicated logic. The steps for creating a driver, getting migrations, and applying them are repeated. This common functionality can be refactored into a shared private function to make the code more concise and easier to maintain.
| suspend fun createDatabase(databasePath: String): Either<MigrationError, T> = either { | ||
| logger.info("Initializing migration-aware database at: $databasePath") | ||
|
|
||
| // Create the managed driver with default SQLite settings | ||
| val managedDriver = ManagedSqlDriver.createWithDefaults(databasePath) | ||
| val driver = managedDriver.driver | ||
|
|
||
| // Explicitly apply PRAGMA settings to ensure they are set | ||
| // This is redundant with connection properties but ensures settings are applied | ||
| driver.execute(null, "PRAGMA foreign_keys = ON", 0) | ||
| driver.execute(null, "PRAGMA journal_mode = WAL", 0) | ||
| driver.execute(null, "PRAGMA synchronous = NORMAL", 0) | ||
| driver.execute(null, "PRAGMA cache_size = -64000", 0) | ||
| driver.execute(null, "PRAGMA temp_store = MEMORY", 0) | ||
|
|
||
| // Initialize migration components | ||
| val platformDatabase = io.github.kamiazya.scopes.platform.db.PlatformDatabase(driver) | ||
| val executor = SqlDelightMigrationExecutor(driver) | ||
| val repository = SqlDelightSchemaVersionStore(platformDatabase) | ||
| val migrationManager = DefaultMigrationManager( | ||
| executor = executor, | ||
| repository = repository, | ||
| migrationProvider = migrationProvider, | ||
| clock = clock, | ||
| ) | ||
|
|
||
| // Ensure schema_versions table exists | ||
| executor.ensureSchemaVersionsTable().bind() | ||
|
|
||
| // Always apply pending migrations | ||
| logger.debug("Checking for pending migrations") | ||
| val status = migrationManager.getStatus().bind() | ||
|
|
||
| if (!status.isUpToDate) { | ||
| logger.info("Applying ${status.pendingMigrations.size} pending migrations") | ||
|
|
||
| val migrationResult = migrationManager.migrateUp().bind() | ||
|
|
||
| logger.info( | ||
| "Applied ${migrationResult.executedMigrations.size} migrations " + | ||
| "in ${migrationResult.totalExecutionTime.inWholeMilliseconds}ms " + | ||
| "(${migrationResult.fromVersion} -> ${migrationResult.toVersion})", | ||
| ) | ||
| } else { | ||
| logger.debug("Database is up to date (version ${status.currentVersion})") | ||
| } | ||
|
|
||
| // Create the specific database instance | ||
| val database = databaseFactory(driver) | ||
| logger.info("Database initialized successfully") | ||
|
|
||
| database | ||
| } | ||
|
|
||
| /** | ||
| * Creates an in-memory database for testing with migration support. | ||
| * | ||
| * @return Either an error or the database instance | ||
| */ | ||
| suspend fun createInMemoryDatabase(): Either<MigrationError, T> = either { | ||
| logger.debug("Creating in-memory migration-aware database") | ||
|
|
||
| val managedDriver = ManagedSqlDriver(":memory:") | ||
| val driver = managedDriver.driver | ||
|
|
||
| // Apply PRAGMA settings for in-memory database | ||
| driver.execute(null, "PRAGMA foreign_keys = ON", 0) | ||
| driver.execute(null, "PRAGMA synchronous = OFF", 0) // OFF is safe for in-memory | ||
| driver.execute(null, "PRAGMA temp_store = MEMORY", 0) | ||
|
|
||
| // Initialize migration components | ||
| val platformDatabase = io.github.kamiazya.scopes.platform.db.PlatformDatabase(driver) | ||
| val executor = SqlDelightMigrationExecutor(driver) | ||
| val repository = SqlDelightSchemaVersionStore(platformDatabase) | ||
| val migrationManager = DefaultMigrationManager( | ||
| executor = executor, | ||
| repository = repository, | ||
| migrationProvider = migrationProvider, | ||
| clock = clock, | ||
| ) | ||
|
|
||
| // Ensure schema_versions table exists | ||
| executor.ensureSchemaVersionsTable().bind() | ||
|
|
||
| // Always migrate in-memory databases to latest | ||
| logger.debug("Applying all migrations to in-memory database") | ||
| val migrationResult = migrationManager.migrateUp().bind() | ||
|
|
||
| logger.info( | ||
| "Applied ${migrationResult.executedMigrations.size} migrations to in-memory database", | ||
| ) | ||
|
|
||
| // Create the specific database instance | ||
| val database = databaseFactory(driver) | ||
| logger.debug("In-memory database initialized successfully") | ||
|
|
||
| database | ||
| } |
There was a problem hiding this comment.
The createDatabase and createInMemoryDatabase methods share a large amount of code for initializing the migration components and running the migrations. This logic should be extracted into a private function to avoid duplication. This function could take the ManagedSqlDriver as a parameter and handle the common setup and migration execution steps.
| * | ||
| * @property maxRetries Maximum number of retry attempts for failed migrations | ||
| */ | ||
| data class MigrationConfig(val maxRetries: Int = 3) |
There was a problem hiding this comment.
The maxRetries property is defined in MigrationConfig but it doesn't appear to be used anywhere in the migration execution logic. If retry logic is planned for the future, it should be implemented. Otherwise, this property should be removed to avoid confusion and dead code.
| override suspend fun validate(repair: Boolean): Either<MigrationError, SequenceValidationReport> = withContext(Dispatchers.IO) { | ||
| either { | ||
| // First perform basic validation | ||
| val validationResult = repository.validateMigrationSequence().bind() | ||
|
|
||
| if (validationResult.isValid || !repair) { | ||
| validationResult | ||
| } else { | ||
| // If repair is requested and there are issues, attempt to fix them | ||
| val availableMigrations = migrationProvider().sortedBy { it.version } | ||
| val appliedMigrations = repository.getAllAppliedMigrations().bind() | ||
|
|
||
| val inconsistencies = mutableListOf<String>() | ||
|
|
||
| // Check for migrations that have been applied but are no longer available | ||
| for (appliedMigration in appliedMigrations) { | ||
| val availableMigration = availableMigrations.find { it.version == appliedMigration.version } | ||
| if (availableMigration == null) { | ||
| inconsistencies.add("Applied migration ${appliedMigration.version} is no longer available") | ||
| continue | ||
| } | ||
| } | ||
|
|
||
| SequenceValidationReport( | ||
| isValid = inconsistencies.isEmpty(), | ||
| gaps = validationResult.gaps, | ||
| inconsistencies = inconsistencies, | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The validate method's repair parameter is misleading. When repair is true, the method performs additional checks and reports more inconsistencies, but it doesn't actually perform any repair actions. The method name and parameter suggest it would attempt to fix issues like gaps. The implementation should either be updated to perform repairs or the method/parameter should be renamed to more accurately reflect its behavior (e.g., validate(deepCheck: Boolean)).
| version = version, | ||
| description = description, | ||
| ) { | ||
| override val sql: List<String> = sql.split(";").filter { it.isNotBlank() }.map { it.trim() } |
There was a problem hiding this comment.
Using sql.split(";") to separate SQL statements is a naive approach that can lead to errors. It will fail if a semicolon appears within a string literal, a comment, or a trigger definition. While it may work for simple DDL, it's fragile. Consider using a more robust SQL parsing library or at least adding a warning in the documentation about this limitation.
| getAllAppliedPaged: | ||
| SELECT * | ||
| FROM schema_versions | ||
| ORDER BY version ASC | ||
| LIMIT ? OFFSET :value_; |
There was a problem hiding this comment.
The getAllAppliedPaged query uses a mix of positional (?) and named (:value_) placeholders. This is confusing and inconsistent. It's better to use positional placeholders for both LIMIT and OFFSET for consistency with other queries.
LIMIT ? OFFSET ?;
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
| -- Scope aliases table | ||
| CREATE TABLE IF NOT EXISTS scope_aliases ( | ||
| id TEXT PRIMARY KEY NOT NULL, | ||
| scope_id TEXT NOT NULL, | ||
| alias_name TEXT NOT NULL UNIQUE, | ||
| alias_type TEXT NOT NULL, | ||
| created_at INTEGER NOT NULL, | ||
| updated_at INTEGER NOT NULL, | ||
| FOREIGN KEY (scope_id) REFERENCES scopes(id) ON DELETE CASCADE | ||
| ); |
There was a problem hiding this comment.
[P1] Provide upgrade path for existing scope_aliases table
The new scope-management migration only runs CREATE TABLE IF NOT EXISTS scope_aliases … without altering the table when it already exists. Installations created before this commit have a scope_aliases table without the new id, alias_type, and updated_at columns. Because the migration is a no-op in that case, the database stays on the old schema while the Kotlin code and .sq queries now read and write those columns. Upgrading users will therefore hit runtime errors like no such column: alias_type the first time the repository executes one of the new queries. The migration needs to perform ALTER TABLE (or a data copy) to bring existing databases into the new schema instead of relying solely on CREATE TABLE IF NOT EXISTS.
Useful? React with 👍 / 👎.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| !entry.isDirectory | ||
| } | ||
| .forEach { entry -> | ||
| classLoader.getResource(entry.name)?.let { resources.add(it) } |
Check failure
Code scanning / CodeQL
Arbitrary file access during archive extraction ("Zip Slip") High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
To fix this vulnerability, we must ensure each archive entry's path is sanitized so that it cannot reference files outside of the expected migration resource directory. Specifically, we should normalize the entry path and verify that the resulting path starts with the expected resource path prefix.
For JAR entries, this is done by:
- Normalizing the
entry.nameto remove any traversals like../or redundant slashes. - Explicitly checking that the normalized entry path starts with the resource directory (prefix) determined earlier in the code.
- Skipping any entry that fails this check.
To enact this:
- Edit the code in the
findResources()function, specifically within the"jar"protocol handling (lines 110–112). - Before calling
classLoader.getResource(entry.name), normalizeentry.nameas a path and check that it starts with the intended resource directory prefix (as a path). - Only add resources to the list if the path validation succeeds.
- No additional imports are necessary, since the code is already using
java.nio.file.Path-related functionality throughtoPath()already; however, if extra methods are required, one can usejava.nio.file.Pathsand.normalize()from the standard library.
| @@ -108,7 +108,13 @@ | ||
| !entry.isDirectory | ||
| } | ||
| .forEach { entry -> | ||
| classLoader.getResource(entry.name)?.let { resources.add(it) } | ||
| val entryPath = java.nio.file.Paths.get(entry.name).normalize() | ||
| val prefixPath = java.nio.file.Paths.get(prefix).normalize() | ||
| if (entryPath.startsWith(prefixPath) && !entryPath.toString().contains("..")) { | ||
| classLoader.getResource(entry.name)?.let { resources.add(it) } | ||
| } else { | ||
| logger.warn("Skipping potentially unsafe migration entry from jar: ${entry.name}") | ||
| } | ||
| } | ||
| } | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (42)
platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationConfig.kt (2)
8-8: Validate non-negative retriesGuard against invalid config values to fail fast.
Apply this diff:
-data class MigrationConfig(val maxRetries: Int = 3) +data class MigrationConfig(val maxRetries: Int = 3) { + init { + require(maxRetries >= 0) { "maxRetries must be >= 0" } + } +}
3-7: Clarify retry semantics in KDocSpecify whether maxRetries excludes the initial attempt to avoid off‑by‑one confusion.
contexts/event-store/infrastructure/src/main/resources/migrations/event-store/V1__Initial_event_store_schema.sql (1)
4-4: Avoid AUTOINCREMENT unless requiredSQLite AUTOINCREMENT has downsides (contention, no reuse). Regular INTEGER PRIMARY KEY already auto-increments safely.
Apply this diff:
- sequence_number INTEGER PRIMARY KEY AUTOINCREMENT, + sequence_number INTEGER PRIMARY KEY,contexts/device-synchronization/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/devicesync/infrastructure/migration/DeviceSyncMigrationProvider.kt (1)
10-18: Unify providers behind an interfaceConsider introducing a MigrationProvider interface to standardize getMigrations() across contexts.
contexts/event-store/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/eventstore/infrastructure/migration/EventStoreMigrationProvider.kt (1)
14-16: Consider context class loader fallbackOptionally prefer Thread.currentThread().contextClassLoader ?: this::class.java.classLoader for environments that rely on TCCL.
Apply this diff:
- classLoader = this::class.java.classLoader, + classLoader = Thread.currentThread().contextClassLoader ?: this::class.java.classLoader,contexts/device-synchronization/infrastructure/src/main/resources/migrations/device-sync/V1__Initial_device_sync_schema.sql (3)
3-12: Constrain counters to valid rangesEnsure pending_changes is non-negative.
Apply this diff:
pending_changes INTEGER NOT NULL DEFAULT 0, + CHECK (pending_changes >= 0),
3-12: Optionally constrain sync_status valuesIf statuses are enumerated, add a CHECK to prevent invalid states.
Example:
CHECK (sync_status IN ('NEVER_SYNCED','IDLE','SYNCING','ERROR'))
17-22: Composite PK table optimizationConsider WITHOUT ROWID for composite PK tables.
Example:
CREATE TABLE ... WITHOUT ROWID;contexts/scope-management/infrastructure/src/main/resources/migrations/scope-management/V1__Initial_scope_management_schema.sql (3)
35-37: Drop redundant index on UNIQUE columnscope_aliases.alias_name is UNIQUE; an extra index duplicates work.
Apply this diff:
-CREATE INDEX IF NOT EXISTS idx_scope_aliases_alias_name ON scope_aliases(alias_name);
66-71: Remove redundant/ineffective indexes on context_views
- key and name are UNIQUE; extra indexes duplicate the implicit unique indexes.
- key, filter partial index uses WHERE filter IS NOT NULL but filter is NOT NULL; it’s equivalent to a full index and unnecessary.
Apply this diff:
-CREATE INDEX IF NOT EXISTS idx_context_views_key ON context_views(key); -CREATE INDEX IF NOT EXISTS idx_context_views_name ON context_views(name); -CREATE INDEX IF NOT EXISTS idx_context_views_key_filter ON context_views(key, filter) WHERE filter IS NOT NULL;
91-92: Remove redundant UNIQUE index on PKactive_context.id is the PRIMARY KEY; the extra UNIQUE index is redundant.
Apply this diff:
-CREATE UNIQUE INDEX IF NOT EXISTS idx_active_context_single ON active_context(id);contexts/scope-management/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/scopemanagement/infrastructure/migration/ScopeManagementMigrationProvider.kt (1)
15-17: Prefer TCCL fallback for class loadingSee similar note in EventStore provider.
- classLoader = this::class.java.classLoader, + classLoader = Thread.currentThread().contextClassLoader ?: this::class.java.classLoader,platform/infrastructure/src/test/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationExecutorTest.kt (1)
104-119: Add idempotency check for ensureSchemaVersionsTableCall ensureSchemaVersionsTable twice and assert success both times to guard against accidental DDL re-execution.
Apply this diff:
it("should create schema_versions table") { runTest { val (driver, executor) = createTestExecutor() try { val result = executor.ensureSchemaVersionsTable() result.shouldBeRight() + // Call again to verify idempotency + executor.ensureSchemaVersionsTable().shouldBeRight() // Verify table exists val tableExists = executor.tableExists("schema_versions").shouldBeRight() tableExists shouldBe true } finally { driver.close() } } }platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/discovery/ResourceMigrationDiscovery.kt (6)
96-101: Robust JAR path handlingParsing url.path and removePrefix("file:") can break on Windows and URL‑encoded paths. Prefer JarURLConnection consistently and, if falling back, use URLDecoder + URI to resolve the file path safely.
Apply this diff for safer fallback:
- val jarPath = url.path.substringBefore("!") - jarFile = JarFile(jarPath.removePrefix("file:")) + val raw = url.path.substringBefore("!") + val cleaned = java.net.URL(raw).toURI() + jarFile = JarFile(java.nio.file.Paths.get(cleaned).toFile())
148-149: Naive SQL split by ';' can break on strings/triggersSplitting on ';' will misparse statements containing semicolons in string literals or complex DDL. If feasible, store one statement per file or implement a minimal parser that ignores semicolons inside quotes.
I can provide a lightweight splitter that respects single/double quotes if you want to keep multi‑statement files.
55-57: Detect duplicate migration versionsBefore sorting, detect duplicates (same version) and fail with a clear error to prevent undefined execution order.
I can add a check that groups by version and returns a Left with the offending filenames. Based on learnings
40-44: Jar URL filename extractionUsing substringAfterLast('/') on a jar: URL is fine, but for consistency and fewer surprises consider using Paths.get(URI).fileName when protocol is file, and entry.name when protocol is jar, instead of relying on URL string formatting.
33-54: Surface discovery summary at info level, detailed at debugLogging is good. Consider logging the list of discovered versions/descriptions at debug to aid troubleshooting classpath issues.
127-136: Prefer check()/require() for arguments but not for I/O statesrequire() on filename format is fine. For I/O failures, prefer Either as noted above so callers can decide policy instead of throwing.
As per coding guidelines
contexts/device-synchronization/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/devicesync/infrastructure/sqldelight/SqlDelightDatabaseProvider.kt (3)
25-33: Prefer shared MigrationAwareDatabaseProvider to avoid duplicationThis logic duplicates the migration flow introduced in the platform provider. Consider delegating to MigrationAwareDatabaseProvider for consistency and less boilerplate.
Based on PR objectives
27-32: Avoid runBlocking on creation path if possibleIf applyMigrations is suspend, prefer exposing a suspend factory or moving blocking to a higher level to avoid blocking threads on DB init.
As per coding guidelines
37-49: Use consistent SQLite defaults for in-memory driver
Replace the bare constructor withManagedSqlDriver.createWithDefaults(":memory:")to apply the same default pragmas.- val managedDriver = ManagedSqlDriver(":memory:") + val managedDriver = ManagedSqlDriver.createWithDefaults(":memory:")contexts/event-store/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/eventstore/infrastructure/sqldelight/SqlDelightDatabaseProvider.kt (3)
25-33: Deduplicate via MigrationAwareDatabaseProviderRecommend delegating migration init to the shared provider for consistency.
Based on PR objectives
27-32: Avoid runBlocking on DB init pathIf feasible, expose a suspend factory or let the higher layer handle blocking.
As per coding guidelines
37-49: Use SQLite defaults in in-memory driverMirror production defaults by using the factory:
- val managedDriver = ManagedSqlDriver(":memory:") + val managedDriver = ManagedSqlDriver.createWithDefaults(":memory:")platform/infrastructure/src/test/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationManagerIntegrationTest.kt (1)
66-86: Minor: consider using use {} for driverYou already close in finally; using driver.use { … } would be slightly cleaner.
apps/scopes/src/main/kotlin/io/github/kamiazya/scopes/apps/cli/di/scopemanagement/ScopeManagementInfrastructureModule.kt (1)
84-95: Avoid runBlocking in DI module; move blocking call behind provider or bootstrap.Blocking the DI graph init can stall startup. Prefer a provider-level blocking wrapper (e.g., createDatabaseBlocking) or initialize in an ApplicationBootstrapper and inject the ready DB. Keep fail‑fast with error().
If you add a blocking wrapper to MigrationAwareDatabaseProvider:
- kotlinx.coroutines.runBlocking { - provider.createDatabase(dbPath).fold( + provider.createDatabaseBlocking(dbPath).fold( ifLeft = { err -> // Fail-fast using Kotlin's error() function error("Failed to create database: ${err.message}") }, ifRight = { database -> database }, ) - }And in MigrationAwareDatabaseProvider:
fun <T> MigrationAwareDatabaseProvider<T>.createDatabaseBlocking(path: String): Either<MigrationError, T> = kotlinx.coroutines.runBlocking { createDatabase(path) }As per coding guidelines
contexts/scope-management/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/scopemanagement/infrastructure/sqldelight/SqlDelightDatabaseProvider.kt (3)
31-40: Remove unused private provider() to avoid drift.The helper isn’t used; it duplicates configuration already done below. Drop it or refactor callers to use it consistently.
Apply this diff to remove it:
- private fun provider(loggerName: String = "ScopeManagementDB"): MigrationAwareDatabaseProvider<ScopeManagementDatabase> { - val logger = ConsoleLogger(loggerName) - val migrations = { ScopeManagementMigrationProvider(logger = logger).getMigrations() } - return MigrationAwareDatabaseProvider( - migrationProvider = migrations, - config = MigrationConfig(maxRetries = 3), - logger = logger, - databaseFactory = { driver -> ScopeManagementDatabase(driver) }, - ) - }
23-29: Expose closable handle to prevent driver leaks in tests/tools.ManagedDatabase implements AutoCloseable but the factory returns ScopeManagementDatabase, hiding close(). Consider returning ManagedDatabase (or an interface that includes close()) or add an overload returning the wrapper.
Example overload:
fun createManagedDatabase(databasePath: String): ManagedDatabase { ... } fun createManagedInMemoryDatabase(): ManagedDatabase { ... }
50-57: DRY: factor duplicated migration-apply block.Both functions build logger, fetch migrations, and apply them. Extract a small private applyMigrationsOrThrow(driver, loggerName) to reduce duplication.
+ private fun applyMigrationsOrThrow(driver: app.cash.sqldelight.db.SqlDriver, loggerName: String) { + val logger = ConsoleLogger(loggerName) + val migrations = ScopeManagementMigrationProvider(logger = logger).getMigrations() + runBlocking { + driver.applyMigrations(migrations, logger).fold( + ifLeft = { err -> error("Migration failed: ${err.message}") }, + ifRight = { }, + ) + } + } @@ - val logger = ConsoleLogger("ScopeManagementDB") - val migrations = ScopeManagementMigrationProvider(logger = logger).getMigrations() - runBlocking { - driver.applyMigrations(migrations, logger).fold( - ifLeft = { err -> error("Migration failed: ${err.message}") }, - ifRight = { }, - ) - } + applyMigrationsOrThrow(driver, "ScopeManagementDB") @@ - val logger = ConsoleLogger("ScopeManagementDB-InMemory") - val migrations = ScopeManagementMigrationProvider(logger = logger).getMigrations() - runBlocking { - driver.applyMigrations(migrations, logger).fold( - ifLeft = { err -> error("Migration failed: ${err.message}") }, - ifRight = { }, - ) - } + applyMigrationsOrThrow(driver, "ScopeManagementDB-InMemory")Also applies to: 70-77
platform/infrastructure/src/main/sqldelight/io/github/kamiazya/scopes/platform/db/SchemaVersion.sq (1)
43-47: Prefer EXISTS for existence checks.COUNT(*) > 0 can be less efficient and may produce less clear types in codegen. Use EXISTS subquery.
-existsByVersion: -SELECT COUNT(*) > 0 AS result -FROM schema_versions -WHERE version = ?; +existsByVersion: +SELECT EXISTS(SELECT 1 FROM schema_versions WHERE version = ?) AS result;platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationExecutor.kt (3)
117-135: Minor: simplify QueryResult mapping.Use QueryResult.Unit for clarity and slightly less allocation.
- driver.executeQuery(null, query, { cursor -> - if (cursor.next().value) { - exists = cursor.getLong(0)?.let { it > 0 } ?: false - } - QueryResult.Value(Unit) - }, 1) { + driver.executeQuery(null, query, { cursor -> + if (cursor.next().value) { + exists = cursor.getLong(0)?.let { it > 0 } ?: false + } + QueryResult.Unit + }, 1) { bindString(0, tableName) }
170-191: Avoid duplicating schema DDL across layers.ensureSchemaVersionsTable duplicates SchemaVersion.sq. If possible, centralize DDL to a single source (e.g., SQL migration V0) to reduce drift. Keep as-is if convenience outweighs risk.
151-159: Minor: use QueryResult.Unit for consistency.Same micro‑nit as above.
- driver.executeQuery(null, query, { cursor -> + driver.executeQuery(null, query, { cursor -> if (cursor.next().value) { version = cursor.getLong(0) ?: 0L } - QueryResult.Value(Unit) + QueryResult.Unit }, 0) @@ - driver.executeQuery(null, "SELECT 1", { QueryResult.Value(Unit) }, 0) + driver.executeQuery(null, "SELECT 1", { QueryResult.Unit }, 0)Also applies to: 193-196
platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationDiscovery.kt (1)
9-14: Avoid name collision with MigrationError.ValidationErrorThis file defines a top-level ValidationError in the same package where MigrationError.ValidationError also exists. This increases ambiguity and hurts readability. Prefer a distinct name for discovery-time issues.
Apply this diff to rename the discovery error type:
data class DiscoveryReport( val migrations: List<Migration>, val discoveredFiles: Int, val validMigrations: Int, - val invalidMigrations: List<ValidationError>, + val invalidMigrations: List<MigrationFileValidationError>, val duplicateVersions: List<Long>, ) - data class ValidationError(val file: String, val version: Long?, val reason: String, val cause: Throwable? = null) + data class MigrationFileValidationError(val file: String, val version: Long?, val reason: String, val cause: Throwable? = null) - suspend fun validateMigrations(migrations: List<Migration>): Either<MigrationError, List<ValidationError>> + suspend fun validateMigrations(migrations: List<Migration>): Either<MigrationError, List<MigrationFileValidationError>>As per coding guidelines
Also applies to: 19-19, 63-63
platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationAwareDatabaseProvider.kt (1)
24-29: Unused config parameterconfig is stored but never used. Detekt will likely flag unusedPrivateMember. Either use it (e.g., to drive PRAGMA/validation options) or remove it.
As per coding guidelines
platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationError.kt (1)
6-6: Consider passing message/cause to Exception baseCurrent subclasses override message/cause properties without calling Exception(message, cause). Prefer modeling MigrationError(message, cause) : Exception(message, cause) and delegate from subclasses. This improves stack traces and Java interop.
As per coding guidelines
platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/DatabaseIntegration.kt (1)
25-53: Optional: run integration steps on Dispatchers.IOThese operations interact with the driver. Consider withContext(Dispatchers.IO) to avoid blocking default dispatcher.
As per coding guidelines
platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/SchemaVersionRepository.kt (4)
135-148: Validate limit/offset to fail fast and avoid invalid SQLAdd require(...) checks to prevent negative limit/offset and align with “fail fast” guideline.
As per coding guidelines
- override suspend fun getAppliedMigrations(limit: Int, offset: Int): Either<MigrationError, List<AppliedMigration>> = withContext(Dispatchers.IO) { + override suspend fun getAppliedMigrations(limit: Int, offset: Int): Either<MigrationError, List<AppliedMigration>> = withContext(Dispatchers.IO) { + require(limit >= 0) { "limit must be >= 0" } + require(offset >= 0) { "offset must be >= 0" } try { val migrations = database.schemaVersionQueries.getAllAppliedPaged( limit.toLong(), offset.toLong(), ).executeAsList().map { it.toAppliedMigration() } migrations.right() } catch (e: Exception) {
104-119: Centralize IO + Either error handling to reduce boilerplateEach method repeats withContext + try/catch + Either mapping. Extract a small helper to keep call sites lean and consistent.
Apply to this method (and similarly to others):
- override suspend fun saveAppliedMigration(migration: AppliedMigration): Either<MigrationError, Unit> = withContext(Dispatchers.IO) { - try { - database.schemaVersionQueries.insertMigration( - version = migration.version, - description = migration.description, - applied_at = migration.appliedAt.toEpochMilliseconds(), - execution_time_ms = migration.executionTime.inWholeMilliseconds, - ) - Unit.right() - } catch (e: Exception) { - MigrationError.DatabaseError( - operation = "save applied migration", - cause = e, - ).left() - } - } + override suspend fun saveAppliedMigration(migration: AppliedMigration): Either<MigrationError, Unit> = + ioEither("save applied migration") { + database.schemaVersionQueries.insertMigration( + version = migration.version, + description = migration.description, + applied_at = migration.appliedAt.toEpochMilliseconds(), + execution_time_ms = migration.executionTime.inWholeMilliseconds, + ) + }Add this helper (outside the selected range):
private suspend fun <T> ioEither( operation: String, block: suspend () -> T, ): Either<MigrationError, T> = withContext(Dispatchers.IO) { runCatching { block() } .fold({ it.right() }, { MigrationError.DatabaseError(operation = operation, cause = it).left() }) }
1-2: Optional: Rename file to match the interfaceConsider renaming the file to SchemaVersionStore.kt for consistency with the primary public type.
190-201: Coalesce aggregates in SQL
In platform/infrastructure/src/main/sqldelight/io/github/kamiazya/scopes/platform/db/SchemaVersion.sq’s getMigrationStats query, wrap SUM(execution_time) AS total_execution_time (and optionally COUNT(*) AS count) in COALESCE(..., 0) so they never return null. You can then remove the Elvis defaults for totalExecutionTime (and totalMigrations) in SchemaVersionRepository.getMigrationStatistics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
apps/scopes/src/main/kotlin/io/github/kamiazya/scopes/apps/cli/di/scopemanagement/ScopeManagementInfrastructureModule.kt(4 hunks)apps/scopes/src/main/resources/META-INF/native-image/resource-config.json(1 hunks)apps/scopes/src/main/resources/META-INF/services/com.github.ajalt.mordant.terminal.TerminalInterfaceProvider(0 hunks)contexts/device-synchronization/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/devicesync/infrastructure/migration/DeviceSyncMigrationProvider.kt(1 hunks)contexts/device-synchronization/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/devicesync/infrastructure/sqldelight/SqlDelightDatabaseProvider.kt(1 hunks)contexts/device-synchronization/infrastructure/src/main/resources/migrations/device-sync/V1__Initial_device_sync_schema.sql(1 hunks)contexts/event-store/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/eventstore/infrastructure/migration/EventStoreMigrationProvider.kt(1 hunks)contexts/event-store/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/eventstore/infrastructure/sqldelight/SqlDelightDatabaseProvider.kt(1 hunks)contexts/event-store/infrastructure/src/main/resources/migrations/event-store/V1__Initial_event_store_schema.sql(1 hunks)contexts/scope-management/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/scopemanagement/infrastructure/migration/ScopeManagementMigrationProvider.kt(1 hunks)contexts/scope-management/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/scopemanagement/infrastructure/sqldelight/SqlDelightDatabaseProvider.kt(2 hunks)contexts/scope-management/infrastructure/src/main/resources/migrations/scope-management/V1__Initial_scope_management_schema.sql(1 hunks)platform/infrastructure/build.gradle.kts(2 hunks)platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/DatabaseIntegration.kt(1 hunks)platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/Migration.kt(1 hunks)platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationAwareDatabaseProvider.kt(1 hunks)platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationConfig.kt(1 hunks)platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationDiscovery.kt(1 hunks)platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationError.kt(1 hunks)platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationExecutor.kt(1 hunks)platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationManager.kt(1 hunks)platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/SchemaVersionRepository.kt(1 hunks)platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/discovery/ResourceMigrationDiscovery.kt(1 hunks)platform/infrastructure/src/main/sqldelight/io/github/kamiazya/scopes/platform/db/SchemaVersion.sq(1 hunks)platform/infrastructure/src/test/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationExecutorTest.kt(1 hunks)platform/infrastructure/src/test/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationManagerIntegrationTest.kt(1 hunks)
💤 Files with no reviewable changes (1)
- apps/scopes/src/main/resources/META-INF/services/com.github.ajalt.mordant.terminal.TerminalInterfaceProvider
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{kt,kts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{kt,kts}: Adhere to ktlint formatting rules for all Kotlin source and script files; run ktlintFormat before committing
Code must pass Detekt static analysis; avoid suppressing rules without clear justification
Files:
platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationConfig.ktplatform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationAwareDatabaseProvider.ktcontexts/scope-management/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/scopemanagement/infrastructure/migration/ScopeManagementMigrationProvider.ktplatform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/Migration.ktplatform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationError.ktplatform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/SchemaVersionRepository.ktplatform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationExecutor.ktplatform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/DatabaseIntegration.ktplatform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/discovery/ResourceMigrationDiscovery.ktplatform/infrastructure/src/test/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationExecutorTest.ktplatform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationDiscovery.ktcontexts/scope-management/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/scopemanagement/infrastructure/sqldelight/SqlDelightDatabaseProvider.ktplatform/infrastructure/src/test/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationManagerIntegrationTest.ktcontexts/device-synchronization/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/devicesync/infrastructure/migration/DeviceSyncMigrationProvider.ktcontexts/device-synchronization/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/devicesync/infrastructure/sqldelight/SqlDelightDatabaseProvider.ktplatform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationManager.ktapps/scopes/src/main/kotlin/io/github/kamiazya/scopes/apps/cli/di/scopemanagement/ScopeManagementInfrastructureModule.ktplatform/infrastructure/build.gradle.ktscontexts/event-store/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/eventstore/infrastructure/migration/EventStoreMigrationProvider.ktcontexts/event-store/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/eventstore/infrastructure/sqldelight/SqlDelightDatabaseProvider.kt
**/src/**/kotlin/**/*.kt
📄 CodeRabbit inference engine (CLAUDE.md)
**/src/**/kotlin/**/*.kt: Use Kotlin’s error(), check(), and require() instead of throwing exceptions directly
Never use "unknown" or default fallbacks that could mask data corruption
Use Arrow’s Either for functional error handling
Fail fast for critical configuration issues (e.g., missing text model)
Files:
platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationConfig.ktplatform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationAwareDatabaseProvider.ktcontexts/scope-management/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/scopemanagement/infrastructure/migration/ScopeManagementMigrationProvider.ktplatform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/Migration.ktplatform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationError.ktplatform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/SchemaVersionRepository.ktplatform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationExecutor.ktplatform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/DatabaseIntegration.ktplatform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/discovery/ResourceMigrationDiscovery.ktplatform/infrastructure/src/test/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationExecutorTest.ktplatform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationDiscovery.ktcontexts/scope-management/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/scopemanagement/infrastructure/sqldelight/SqlDelightDatabaseProvider.ktplatform/infrastructure/src/test/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationManagerIntegrationTest.ktcontexts/device-synchronization/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/devicesync/infrastructure/migration/DeviceSyncMigrationProvider.ktcontexts/device-synchronization/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/devicesync/infrastructure/sqldelight/SqlDelightDatabaseProvider.ktplatform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationManager.ktapps/scopes/src/main/kotlin/io/github/kamiazya/scopes/apps/cli/di/scopemanagement/ScopeManagementInfrastructureModule.ktcontexts/event-store/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/eventstore/infrastructure/migration/EventStoreMigrationProvider.ktcontexts/event-store/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/eventstore/infrastructure/sqldelight/SqlDelightDatabaseProvider.kt
**/src/test/kotlin/**/*.kt
📄 CodeRabbit inference engine (CLAUDE.md)
**/src/test/kotlin/**/*.kt: Do not use mock services in tests
Make tests verbose to aid debugging
Use Kotest for testing (project-standard test framework)
Files:
platform/infrastructure/src/test/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationExecutorTest.ktplatform/infrastructure/src/test/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationManagerIntegrationTest.kt
{gradle/libs.versions.toml,settings.gradle.kts,**/build.gradle.kts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not modify dependencies to work around the JitPack/kulid resolution issue; keep declared dependencies unchanged until connectivity is restored
Files:
platform/infrastructure/build.gradle.kts
{**/build.gradle.kts,gradle.properties}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep the project targeting Java 21 in Gradle configuration; do not downgrade the toolchain
Files:
platform/infrastructure/build.gradle.kts
🧠 Learnings (1)
📚 Learning: 2025-09-23T16:30:41.977Z
Learnt from: CR
PR: kamiazya/scopes#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-23T16:30:41.977Z
Learning: Applies to **/src/test/kotlin/**/*.kt : Use Kotest for testing (project-standard test framework)
Applied to files:
platform/infrastructure/build.gradle.kts
🧬 Code graph analysis (4)
platform/infrastructure/src/test/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationExecutorTest.kt (1)
platform/infrastructure/src/test/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationManagerIntegrationTest.kt (1)
describe(13-159)
platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationDiscovery.kt (1)
platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/discovery/ResourceMigrationDiscovery.kt (1)
discoverMigrations(30-64)
platform/infrastructure/src/test/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationManagerIntegrationTest.kt (1)
platform/infrastructure/src/test/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationExecutorTest.kt (1)
describe(12-134)
platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationManager.kt (1)
platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationAwareDatabaseProvider.kt (1)
migrationProvider(23-154)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Cross-Platform Native Build (windows-latest, win32, arm64, true)
- GitHub Check: Cross-Platform Native Build (macos-13, darwin, x64)
- GitHub Check: Cross-Platform Native Build (macos-14, darwin, arm64)
- GitHub Check: Cross-Platform Native Build (ubuntu-latest, linux, arm64)
- GitHub Check: Cross-Platform Native Build (windows-latest, win32, x64, true)
- GitHub Check: Cross-Platform Native Build (ubuntu-latest, linux, x64)
- GitHub Check: Unit Tests
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (23)
apps/scopes/src/main/resources/META-INF/native-image/resource-config.json (1)
23-26: Resource patterns correctly include migrationsThe added patterns cover both root-level and nested migration directories, ensuring the native image bundles all SQL migration resources as intended. Looks good.
contexts/device-synchronization/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/devicesync/infrastructure/migration/DeviceSyncMigrationProvider.kt (2)
10-18: LGTMProvider wiring and discovery path look good.
11-15: Resource availability confirmed Verified thatcontexts/device-synchronization/infrastructure/src/main/resources/migrations/device-sync/V1__Initial_device_sync_schema.sqlexists undermigrations/device-syncand will be included on the classpath by default.platform/infrastructure/build.gradle.kts (3)
35-42: LGTM: SQLDelight configDatabase name, package, and dialect configuration look correct.
26-29: Kotest test setup aligns with project preferenceKotest dependencies plus JUnit Platform are consistent.
Based on learnings
3-3: No changes required: sqldelight alias definitions present
The sqldelight plugin alias and its sqlite driver and dialect library aliases are defined in gradle/libs.versions.toml.contexts/event-store/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/eventstore/infrastructure/migration/EventStoreMigrationProvider.kt (1)
12-20: LGTMDiscovery path and API mirror other providers.
contexts/scope-management/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/scopemanagement/infrastructure/migration/ScopeManagementMigrationProvider.kt (1)
13-24: LGTMMatches the provider pattern used elsewhere.
contexts/scope-management/infrastructure/src/main/resources/migrations/scope-management/V1__Initial_scope_management_schema.sql (1)
84-89: Foreign key enforcement enabled
PRAGMA foreign_keys=ON is already executed in MigrationAwareDatabaseProvider for both persistent and in-memory databases; no action required.platform/infrastructure/src/test/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationExecutorTest.kt (2)
31-53: Solid executor happy‑path coverageSingle statement execution + existence check looks correct and idiomatic with Kotest + Arrow matchers.
81-102: Great atomicity assertionThe failure case correctly asserts rollback semantics and captures the offending SQL in SqlExecutionError.
contexts/device-synchronization/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/devicesync/infrastructure/sqldelight/SqlDelightDatabaseProvider.kt (1)
15-19: Lifecycle wrapper looks goodManagedDatabase delegates to DeviceSyncDatabase and exposes close() for the driver lifecycle.
contexts/event-store/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/eventstore/infrastructure/sqldelight/SqlDelightDatabaseProvider.kt (1)
15-19: Lifecycle wrapper looks goodManagedDatabase delegation and close() are correct.
platform/infrastructure/src/test/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationManagerIntegrationTest.kt (2)
88-111: End‑to‑end coverage looks correctHappy‑path migration and status assertions are comprehensive and use Arrow/Kotest idioms well.
113-135: Good targeted migrateTo() testValidates partial application and pending count correctly.
platform/infrastructure/src/main/sqldelight/io/github/kamiazya/scopes/platform/db/SchemaVersion.sq (1)
66-71: Verify LAG() window function support
Window functions require SQLite 3.25+. Confirm all supported runtimes (Android, JVM/embedded, iOS, etc.) ship with ≥3.25 or add a fallback (join-based gap check or version-gated diagnostic).platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/Migration.kt (1)
4-7: No action needed for Instant compatibility. TheInstantinio.github.kamiazya.scopes.platform.commons.timeis atypealiasforkotlinx.datetime.Instant, soClock.System.now()is directly compatible.apps/scopes/src/main/kotlin/io/github/kamiazya/scopes/apps/cli/di/scopemanagement/ScopeManagementInfrastructureModule.kt (1)
78-83: Create parent directory before initializing the database. Add code to ensure the file-backeddatabasePath’s parent directory exists (or confirm thatManagedSqlDriverwill create it) before passingdbPath.platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationAwareDatabaseProvider.kt (1)
37-89: Verify driver/resource lifecycle on migration failuresIf any bind() returns Left, the either block short-circuits. Confirm ManagedSqlDriver is not leaked (e.g., file handles locked) when initialization fails. If needed, wrap acquisition/usage in a safe bracket and close on error.
Would you like me to propose a safe acquisition/cleanup pattern using Arrow’s resource/bracket or a try/finally that preserves the Left while closing the driver? As per coding guidelines
platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationManager.kt (1)
121-136: LGTM: migrateUp happy-path and no-op semanticsmigrateUp correctly returns a no-op summary when up-to-date and delegates execution otherwise.
platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/SchemaVersionRepository.kt (3)
104-119: [new_run_scripts]#!/bin/bash # 1) Locate MigrationExecutor definition rg -nP 'class\s+MigrationExecutor' --type kotlin rg -nP 'object\s+MigrationExecutor' --type kotlin rg -nP 'interface\s+MigrationExecutor' --type kotlin # 2) Locate transaction scopes rg -nP '\.transaction\s*\{' --type kotlin # 3) Locate saveAppliedMigration invocations rg -n "saveAppliedMigration" -t kotlin
164-175: getCurrentVersion correctly defaults to 0 when no rows exist
SQL usesSELECT COALESCE(MAX(version), 0) …to satisfy the KDoc promise.
35-42: Stable ordering for pagination already present:getAllAppliedPagedinSchemaVersion.sqincludesORDER BY version ASC, ensuring deterministic pages.
| 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) | ||
| } |
There was a problem hiding this comment.
Leaked lifecycle: returning DeviceSyncDatabase hides close()
createDatabase returns DeviceSyncDatabase, so callers can’t invoke close() on the ManagedDatabase wrapper, risking driver leaks.
Two options:
- Preferred: change return type to ManagedDatabase.
- fun createDatabase(databasePath: String): DeviceSyncDatabase {
+ fun createDatabase(databasePath: String): ManagedDatabase {
...
- return ManagedDatabase(DeviceSyncDatabase(driver), managedDriver)
+ return ManagedDatabase(DeviceSyncDatabase(driver), managedDriver)
}- Backward‑compatible: add a new createManagedDatabase(databasePath) returning ManagedDatabase and keep the old method (deprecated) delegating to it.
I can provide the overload if you want to preserve the existing API for now.
🤖 Prompt for AI Agents
In
contexts/device-synchronization/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/devicesync/infrastructure/sqldelight/SqlDelightDatabaseProvider.kt
around lines 21–34, the method returns DeviceSyncDatabase which hides the
ManagedDatabase.close() lifecycle and risks leaking the driver; change the API
to return ManagedDatabase instead (preferred) by updating the function signature
to return ManagedDatabase and return the existing
ManagedDatabase(DeviceSyncDatabase(driver), managedDriver) so callers can call
close(), or if you must preserve backward compatibility add a new
createManagedDatabase(databasePath: String): ManagedDatabase implementing the
same body and make the old createDatabase(databasePath: String):
DeviceSyncDatabase deprecated and delegate to createManagedDatabase(). Ensure
callers are updated to use the new managed return type (or the deprecated
wrapper) and keep the migration/runBlocking behavior unchanged.
| 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) | ||
| ); |
There was a problem hiding this comment.
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
In
contexts/device-synchronization/infrastructure/src/main/resources/migrations/device-sync/V1__Initial_device_sync_schema.sql
around lines 17 to 22, the vector_clocks table lacks a foreign key to devices;
modify the CREATE TABLE to add a foreign key constraint on device_id referencing
devices(device_id) (e.g. add ", FOREIGN KEY (device_id) REFERENCES
devices(device_id)" inside the table definition) so that device_id enforces
referential integrity; ensure syntax is valid for the target DB and that the
devices table exists before this migration.
| 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) | ||
| } |
There was a problem hiding this comment.
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
In
contexts/event-store/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/eventstore/infrastructure/sqldelight/SqlDelightDatabaseProvider.kt
around lines 21 to 34, the current createDatabase(databasePath: String):
EventStoreDatabase leaks the driver's lifecycle because callers cannot close the
underlying driver; change the API to return the managed wrapper (e.g.,
ManagedDatabase or a new ManagedEventStoreDatabase) so ownership/close is
explicit and update all call sites to use the managed type, or alternatively add
a new createManagedDatabase(databasePath: String): ManagedDatabase (or similar)
that returns the wrapper and mark the old createDatabase as deprecated
forwarding to the new method to preserve compatibility; ensure migrations and
returned object are the managed instance and update imports/tests accordingly.
| 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 | ||
| ); |
There was a problem hiding this comment.
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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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 | |
| ); | |
| 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, | |
| UNIQUE (aggregate_id, aggregate_version) | |
| ); |
🤖 Prompt for AI Agents
In
contexts/event-store/infrastructure/src/main/resources/migrations/event-store/V1__Initial_event_store_schema.sql
around lines 3 to 12, the events table allows duplicate (aggregate_id,
aggregate_version) pairs; add a table-level UNIQUE constraint on (aggregate_id,
aggregate_version) to prevent duplicate versions per aggregate, and then either
remove the separate index that duplicates this uniqueness or update that index
to be unique as well so you don't have redundant non-unique indexes.
| stored_at INTEGER NOT NULL | ||
| ); | ||
|
|
||
| CREATE INDEX IF NOT EXISTS idx_events_aggregate_id_version ON events(aggregate_id, aggregate_version); |
There was a problem hiding this comment.
🛠️ 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:
- Make it unique:
-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);- Or drop this index if you keep the table-level UNIQUE; SQLite will auto-create a unique index for the constraint.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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); |
🤖 Prompt for AI Agents
In
contexts/event-store/infrastructure/src/main/resources/migrations/event-store/V1__Initial_event_store_schema.sql
around line 14, the non-unique index on (aggregate_id, aggregate_version)
conflicts with the table-level UNIQUE constraint; either convert the index to a
UNIQUE INDEX (CREATE UNIQUE INDEX IF NOT EXISTS idx_events_aggregate_id_version
ON events(aggregate_id, aggregate_version)) or remove the explicit index
entirely if the table already declares a UNIQUE constraint (SQLite will
auto-create the unique index for that constraint). Ensure the migration reflects
one of these two options consistently so you don’t have a duplicate/non-unique
index conflicting with the integrity constraint.
| fun createMigrationManager(driver: SqlDriver): Either<MigrationError, DefaultMigrationManager> = either { | ||
| val platformDatabase = io.github.kamiazya.scopes.platform.db.PlatformDatabase(driver) | ||
| val executor = SqlDelightMigrationExecutor(driver) | ||
| val repository = SqlDelightSchemaVersionStore(platformDatabase) | ||
|
|
||
| DefaultMigrationManager( | ||
| executor = executor, | ||
| repository = repository, | ||
| migrationProvider = migrationProvider, | ||
| clock = clock, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Ensure schema_versions exists when creating a manager
createMigrationManager returns a manager without ensuring the tracking table exists, unlike DatabaseIntegration.createMigrationManager which does. Align behavior to avoid first-use failures.
Apply this diff:
fun createMigrationManager(driver: SqlDriver): Either<MigrationError, DefaultMigrationManager> = either {
val platformDatabase = io.github.kamiazya.scopes.platform.db.PlatformDatabase(driver)
val executor = SqlDelightMigrationExecutor(driver)
val repository = SqlDelightSchemaVersionStore(platformDatabase)
+ // Ensure schema_versions table exists before returning a manager
+ executor.ensureSchemaVersionsTable().bind()
+
DefaultMigrationManager(
executor = executor,
repository = repository,
migrationProvider = migrationProvider,
clock = clock,
)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fun createMigrationManager(driver: SqlDriver): Either<MigrationError, DefaultMigrationManager> = either { | |
| val platformDatabase = io.github.kamiazya.scopes.platform.db.PlatformDatabase(driver) | |
| val executor = SqlDelightMigrationExecutor(driver) | |
| val repository = SqlDelightSchemaVersionStore(platformDatabase) | |
| DefaultMigrationManager( | |
| executor = executor, | |
| repository = repository, | |
| migrationProvider = migrationProvider, | |
| clock = clock, | |
| ) | |
| } | |
| fun createMigrationManager(driver: SqlDriver): Either<MigrationError, DefaultMigrationManager> = either { | |
| val platformDatabase = io.github.kamiazya.scopes.platform.db.PlatformDatabase(driver) | |
| val executor = SqlDelightMigrationExecutor(driver) | |
| val repository = SqlDelightSchemaVersionStore(platformDatabase) | |
| // Ensure schema_versions table exists before returning a manager | |
| executor.ensureSchemaVersionsTable().bind() | |
| DefaultMigrationManager( | |
| executor = executor, | |
| repository = repository, | |
| migrationProvider = migrationProvider, | |
| clock = clock, | |
| ) | |
| } |
🤖 Prompt for AI Agents
In
platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationAwareDatabaseProvider.kt
around lines 142-153, the method creates a DefaultMigrationManager but does not
ensure the schema_versions tracking table exists; update the function to
create/ensure the tracking table after constructing the
SqlDelightSchemaVersionStore (repository) and before returning the manager. Call
the repository method that creates or ensures the schema_versions table (or
invoke the appropriate PlatformDatabase routine) and propagate any error so the
Either short-circuits on failure, then return the DefaultMigrationManager only
after the table-existence action succeeds.
| raise( | ||
| MigrationError.SqlExecutionError( | ||
| migration.version, | ||
| "Migration execution failed", | ||
| e, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Use MigrationFailed for unknown failures instead of SqlExecutionError
SqlExecutionError expects the failing SQL, but you’re passing a generic string. Avoid placeholders that mask details; use MigrationFailed to keep semantics clear.
Apply this diff:
- raise(
- MigrationError.SqlExecutionError(
- migration.version,
- "Migration execution failed",
- e,
- ),
- )
+ raise(
+ MigrationError.MigrationFailed(
+ version = migration.version,
+ message = "Migration execution failed",
+ cause = e,
+ ),
+ )As per coding guidelines
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| raise( | |
| MigrationError.SqlExecutionError( | |
| migration.version, | |
| "Migration execution failed", | |
| e, | |
| ), | |
| ) | |
| raise( | |
| MigrationError.MigrationFailed( | |
| version = migration.version, | |
| message = "Migration execution failed", | |
| cause = e, | |
| ), | |
| ) |
🤖 Prompt for AI Agents
In
platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationManager.kt
around lines 271 to 277, replace the raise of MigrationError.SqlExecutionError
(which expects the failing SQL and currently receives a generic string) with
MigrationError.MigrationFailed so the error type reflects an unknown failure;
pass the migration.version and the caught exception (e) to MigrationFailed and
remove the placeholder message to preserve correct semantics.
| * SQLDelight implementation of SchemaVersionRepository. | ||
| */ | ||
| class SqlDelightSchemaVersionStore(private val database: PlatformDatabase) : SchemaVersionStore { |
There was a problem hiding this comment.
Doc mismatch: “SchemaVersionRepository” vs “SchemaVersionStore”
Update KDoc to match the interface/class naming.
Apply this diff:
- * SQLDelight implementation of SchemaVersionRepository.
+ * SQLDelight implementation of SchemaVersionStore.🤖 Prompt for AI Agents
In
platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/SchemaVersionRepository.kt
around lines 100 to 102, the KDoc header mentions "SchemaVersionRepository" but
the class implements/uses "SchemaVersionStore"; change the KDoc to reference
"SchemaVersionStore" (and adjust any surrounding KDoc text to use the correct
name), ensuring the doc matches the actual class/interface name and is
consistent throughout the file.
| override suspend fun validateMigrationSequence(): Either<MigrationError, SequenceValidationReport> = withContext(Dispatchers.IO) { | ||
| try { | ||
| val sequences = database.schemaVersionQueries.validateMigrationSequence() | ||
| .executeAsList() | ||
|
|
||
| val gaps = mutableListOf<Long>() | ||
| val inconsistencies = mutableListOf<String>() | ||
|
|
||
| for (sequence in sequences) { | ||
| val version = sequence.version | ||
| val gap = sequence.gap | ||
| if ((gap ?: 0L) > 1) { | ||
| gaps.add(version ?: 0L) | ||
| inconsistencies.add( | ||
| "Gap detected: missing versions between ${(version ?: 0L) - (gap ?: 0L)} and ${version ?: 0L}", | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| SequenceValidationReport( | ||
| isValid = gaps.isEmpty(), | ||
| gaps = gaps, | ||
| inconsistencies = inconsistencies, | ||
| ).right() | ||
| } catch (e: Exception) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid masking data issues: don’t default nulls to 0 in sequence validation
Defaulting version/gap to 0 can hide corruption. Treat nulls as inconsistencies and only compute gaps when both fields are present.
As per coding guidelines
- for (sequence in sequences) {
- val version = sequence.version
- val gap = sequence.gap
- if ((gap ?: 0L) > 1) {
- gaps.add(version ?: 0L)
- inconsistencies.add(
- "Gap detected: missing versions between ${(version ?: 0L) - (gap ?: 0L)} and ${version ?: 0L}",
- )
- }
- }
+ for (sequence in sequences) {
+ val version = sequence.version
+ val gap = sequence.gap
+ if (version == null || gap == null) {
+ inconsistencies.add("validateMigrationSequence returned nulls: version=$version, gap=$gap")
+ continue
+ }
+ if (gap > 1) {
+ gaps.add(version)
+ inconsistencies.add("Gap detected: missing versions between ${version - gap} and $version")
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| override suspend fun validateMigrationSequence(): Either<MigrationError, SequenceValidationReport> = withContext(Dispatchers.IO) { | |
| try { | |
| val sequences = database.schemaVersionQueries.validateMigrationSequence() | |
| .executeAsList() | |
| val gaps = mutableListOf<Long>() | |
| val inconsistencies = mutableListOf<String>() | |
| for (sequence in sequences) { | |
| val version = sequence.version | |
| val gap = sequence.gap | |
| if ((gap ?: 0L) > 1) { | |
| gaps.add(version ?: 0L) | |
| inconsistencies.add( | |
| "Gap detected: missing versions between ${(version ?: 0L) - (gap ?: 0L)} and ${version ?: 0L}", | |
| ) | |
| } | |
| } | |
| SequenceValidationReport( | |
| isValid = gaps.isEmpty(), | |
| gaps = gaps, | |
| inconsistencies = inconsistencies, | |
| ).right() | |
| } catch (e: Exception) { | |
| override suspend fun validateMigrationSequence(): Either<MigrationError, SequenceValidationReport> = withContext(Dispatchers.IO) { | |
| try { | |
| val sequences = database.schemaVersionQueries.validateMigrationSequence() | |
| .executeAsList() | |
| val gaps = mutableListOf<Long>() | |
| val inconsistencies = mutableListOf<String>() | |
| for (sequence in sequences) { | |
| val version = sequence.version | |
| val gap = sequence.gap | |
| if (version == null || gap == null) { | |
| inconsistencies.add("validateMigrationSequence returned nulls: version=$version, gap=$gap") | |
| continue | |
| } | |
| if (gap > 1) { | |
| gaps.add(version) | |
| inconsistencies.add("Gap detected: missing versions between ${version - gap} and $version") | |
| } | |
| } | |
| SequenceValidationReport( | |
| isValid = gaps.isEmpty(), | |
| gaps = gaps, | |
| inconsistencies = inconsistencies, | |
| ).right() | |
| } catch (e: Exception) { | |
| // ... | |
| } | |
| } |
🤖 Prompt for AI Agents
In
platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/SchemaVersionRepository.kt
around lines 210 to 234, the current validation logic masks null version/gap
values by defaulting them to 0 which can hide data corruption; update the loop
to treat nulls as inconsistencies (add a clear inconsistency message when either
version or gap is null) and only compute and report a gap when both version and
gap are non-null and gap > 1; preserve existing gaps/inconsistencies collections
and return the SequenceValidationReport as before.
| getAllAppliedPaged: | ||
| SELECT * | ||
| FROM schema_versions | ||
| ORDER BY version ASC | ||
| LIMIT ? OFFSET :value_; | ||
|
|
There was a problem hiding this comment.
Fix SQLDelight parameters in getAllAppliedPaged.
:value_ is invalid. Use named params matching the signature or positional placeholders.
Apply this diff:
-getAllAppliedPaged:
-SELECT *
-FROM schema_versions
-ORDER BY version ASC
-LIMIT ? OFFSET :value_;
+getAllAppliedPaged:
+SELECT *
+FROM schema_versions
+ORDER BY version ASC
+LIMIT :limit OFFSET :offset;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| getAllAppliedPaged: | |
| SELECT * | |
| FROM schema_versions | |
| ORDER BY version ASC | |
| LIMIT ? OFFSET :value_; | |
| getAllAppliedPaged: | |
| SELECT * | |
| FROM schema_versions | |
| ORDER BY version ASC | |
| LIMIT :limit OFFSET :offset; |
🤖 Prompt for AI Agents
In
platform/infrastructure/src/main/sqldelight/io/github/kamiazya/scopes/platform/db/SchemaVersion.sq
around lines 30-35, the SQL uses an invalid parameter name ":value_" in the
getAllAppliedPaged query; change the parameters to match SQLDelight expectations
by using either positional placeholders (LIMIT ? OFFSET ?) or a valid named
parameter that matches the function signature (e.g., LIMIT :limit OFFSET
:offset), and update the query signature or call sites accordingly so parameter
names/types align.
Based on CodeRabbit and Sonar analysis: 1. Fixed unused import by using kotlinx.datetime.Instant 2. Optimized SQL indexes - removed redundant indexes on columns with UNIQUE constraints 3. Removed AUTOINCREMENT from scope_aspects table (SQLite best practice) 4. Removed redundant PRAGMA settings already set in ManagedSqlDriver These changes improve performance by reducing unnecessary database overhead while maintaining all functionality. All tests pass (32/32 in platform-infrastructure).
Code Quality Improvements AppliedBased on the comprehensive AI review from CodeRabbit and Sonar analysis, I've implemented the following critical improvements: ✅ Completed Improvements
📊 Quality Status
🎯 ImpactThese changes improve performance by:
📝 Notes on Other Suggestions
The migration system is now cleaner, more performant, and follows best practices while maintaining full functionality. |
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/Migration.kt (1)
55-60: UseDurationin the factory to prevent unit driftKeeping the factory parameter as a raw millisecond
Longinvites accidental unit mismatches at the call sites. Accepting aDurationdirectly eliminates that risk and keeps the conversion concerns at the measurement point.- fun from(migration: Migration, appliedAt: Instant = Clock.System.now(), executionTimeMs: Long): AppliedMigration = AppliedMigration( + fun from(migration: Migration, appliedAt: Instant = Clock.System.now(), executionTime: Duration): AppliedMigration = AppliedMigration( version = migration.version, description = migration.description, appliedAt = appliedAt, - executionTime = executionTimeMs.milliseconds, + executionTime = executionTime, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
contexts/scope-management/infrastructure/src/main/resources/migrations/scope-management/V1__Initial_scope_management_schema.sql(1 hunks)package.json(1 hunks)platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/Migration.kt(1 hunks)platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationAwareDatabaseProvider.kt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{kt,kts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{kt,kts}: Adhere to ktlint formatting rules for all Kotlin source and script files; run ktlintFormat before committing
Code must pass Detekt static analysis; avoid suppressing rules without clear justification
Files:
platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationAwareDatabaseProvider.ktplatform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/Migration.kt
**/src/**/kotlin/**/*.kt
📄 CodeRabbit inference engine (CLAUDE.md)
**/src/**/kotlin/**/*.kt: Use Kotlin’s error(), check(), and require() instead of throwing exceptions directly
Never use "unknown" or default fallbacks that could mask data corruption
Use Arrow’s Either for functional error handling
Fail fast for critical configuration issues (e.g., missing text model)
Files:
platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationAwareDatabaseProvider.ktplatform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/Migration.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Cross-Platform Native Build (windows-latest, win32, x64, true)
- GitHub Check: Cross-Platform Native Build (macos-13, darwin, x64)
- GitHub Check: Cross-Platform Native Build (windows-latest, win32, arm64, true)
- GitHub Check: Cross-Platform Native Build (macos-14, darwin, arm64)
- GitHub Check: Cross-Platform Native Build (ubuntu-latest, linux, x64)
- GitHub Check: Unit Tests
- GitHub Check: Cross-Platform Native Build (ubuntu-latest, linux, arm64)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (2)
contexts/scope-management/infrastructure/src/main/resources/migrations/scope-management/V1__Initial_scope_management_schema.sql (1)
23-32: Blocking: existing databases never receive the newscope_aliasescolumns
CREATE TABLE IF NOT EXISTS scope_aliases …is still a no-op for installations that already have the legacy table (withoutid,alias_type,updated_at, etc.). After this migration runs, those deployments keep the old layout while the Kotlin code and SQLDelight queries now reference the new columns, so the very first read/write hitsno such column: alias_type. Please add ALTER/Table-copy steps (or an equivalent data migration) so existing databases are upgraded in place instead of silently skipping the schema change.platform/infrastructure/src/main/kotlin/io/github/kamiazya/scopes/platform/infrastructure/database/migration/MigrationAwareDatabaseProvider.kt (1)
129-140: Ensure schema_versions table before returning manager
createMigrationManagerstill returns a manager without first guaranteeing the tracking table exists. If the caller uses a fresh driver and immediately asks for status/migrateUp, the first query will crash becauseschema_versionsis missing. Please mirror the initialization you already do increateDatabase/createInMemoryDatabaseby ensuring the table before constructing the manager.val repository = SqlDelightSchemaVersionStore(platformDatabase) + executor.ensureSchemaVersionsTable().bind() + DefaultMigrationManager( executor = executor, repository = repository, migrationProvider = migrationProvider,
Overview
This PR simplifies the database migration system for our local-first application by removing unnecessary features and fixing critical architecture issues using the Provider pattern approach.
Changes
1. Core Migration System Simplification
Removed unnecessary features:
down()method andisReversibleproperty)autoMigrateconfiguration option (migrations always run automatically)validateChecksumsconfiguration optionMethod renaming for clarity:
up()→apply()upSql→sql2. Fixed Critical Architecture Issues
Schema Mismatch Fixes (V1 Migration)
idas primary key, madekeyuniqueidas primary key, addedalias_typeandupdated_atcolumnsactive_contexts, restructured with proper constraintsProvider Pattern Implementation
MigrationAwareDatabaseProviderthat encapsulates:3. Dependency Injection Refactoring
runBlockingfrom DI modules in favor of lazy initializationDatabaseMigrationBootstrapperfrom ApplicationBootstrapper list4. Transaction Support
5. Native Image Configuration
6. Test Suite Updates
MigrationExecutorTest: Verifies atomic transaction behaviorMigrationManagerIntegrationTest: End-to-end migration flow testingBenefits
Testing
Migration Guide
No breaking changes for end users. The migration system will automatically run on application startup.
Summary by CodeRabbit