refactor: migrate :core:database to Room Kotlin Multiplatform#4702
refactor: migrate :core:database to Room Kotlin Multiplatform#4702jamesarich wants to merge 2 commits intomainfrom
Conversation
- Move Room entities, DAOs, converters, and the `MeshtasticDatabase` definition from `androidMain` to `commonMain`. - Implement `MeshtasticDatabaseConstructor` using the Room KMP pattern. - Update `DatabaseManager` to use `BundledSQLiteDriver` and shared database configuration. - Relocate `DatabaseConstants` and address normalization helpers to `commonMain`, utilizing `okio` for cross-platform SHA-1 hashing. - Move `DatabaseModule` to `:core:data` and refactor `LocalDataSource` classes to access DAOs via `DatabaseManager` instead of direct injection. - Update build configuration to use the KMP library plugin and add required Room/SQLite KMP dependencies. - Update database tests to utilize the new constructor-based initialization. Signed-off-by: James Rich <2199651+jamesarich@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Migrates :core:database to Room Kotlin Multiplatform so the Room schema/DAOs/entities can live in commonMain and the database can be constructed via the KMP @ConstructedBy pattern, with Android using the bundled SQLite driver.
Changes:
- Moved Room entities/DAOs/converters and the
MeshtasticDatabasedefinition tocommonMain, adding the KMP constructor pattern. - Updated Android
DatabaseManagerand DI wiring so callers access DAOs viaDatabaseManagerand use shared DB builder configuration (BundledSQLiteDriver, IO query context). - Updated build setup and tests to use the constructor-based Room builders.
Reviewed changes
Copilot reviewed 19 out of 32 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| gradle/libs.versions.toml | Adds androidx.sqlite:sqlite-bundled version catalog entry for Room KMP driver usage. |
| core/database/src/test/kotlin/org/meshtastic/core/database/dao/MigrationTest.kt | Updates DB initialization to use the KMP constructor-based in-memory builder. |
| core/database/src/main/kotlin/org/meshtastic/core/database/di/DatabaseModule.kt | Removes legacy Hilt module that provided a singleton DB + injected DAOs directly. |
| core/database/src/commonMain/kotlin/org/meshtastic/core/database/entity/TracerouteNodePositionEntity.kt | Adds traceroute node position entity in commonMain. |
| core/database/src/commonMain/kotlin/org/meshtastic/core/database/entity/QuickChatAction.kt | Updates header/formatting as part of commonMain move. |
| core/database/src/commonMain/kotlin/org/meshtastic/core/database/entity/Packet.kt | Adds packet/contact/reaction entities and mapping helpers in commonMain. |
| core/database/src/commonMain/kotlin/org/meshtastic/core/database/entity/NodeEntity.kt | Adds node + metadata entities and relation mapping in commonMain. |
| core/database/src/commonMain/kotlin/org/meshtastic/core/database/entity/MyNodeEntity.kt | Adds “my node” entity in commonMain. |
| core/database/src/commonMain/kotlin/org/meshtastic/core/database/entity/MeshLog.kt | Adds mesh log entity and helpers in commonMain. |
| core/database/src/commonMain/kotlin/org/meshtastic/core/database/entity/FirmwareReleaseEntity.kt | Adds firmware release entity + mapping helpers in commonMain. |
| core/database/src/commonMain/kotlin/org/meshtastic/core/database/entity/DeviceHardwareEntity.kt | Adds device hardware entity + mapping helpers in commonMain. |
| core/database/src/commonMain/kotlin/org/meshtastic/core/database/dao/TracerouteNodePositionDao.kt | Updates header/formatting as part of commonMain move. |
| core/database/src/commonMain/kotlin/org/meshtastic/core/database/dao/QuickChatActionDao.kt | Adds Quick Chat DAO in commonMain. |
| core/database/src/commonMain/kotlin/org/meshtastic/core/database/dao/PacketDao.kt | Adds Packet DAO in commonMain (paging, queries, migrations helpers). |
| core/database/src/commonMain/kotlin/org/meshtastic/core/database/dao/NodeInfoDao.kt | Adds NodeInfo DAO and PK/public-key resolution logic in commonMain. |
| core/database/src/commonMain/kotlin/org/meshtastic/core/database/dao/MeshLogDao.kt | Adds MeshLog DAO in commonMain. |
| core/database/src/commonMain/kotlin/org/meshtastic/core/database/dao/FirmwareReleaseDao.kt | Updates header/formatting as part of commonMain move. |
| core/database/src/commonMain/kotlin/org/meshtastic/core/database/dao/DeviceHardwareDao.kt | Adds DeviceHardware DAO in commonMain. |
| core/database/src/commonMain/kotlin/org/meshtastic/core/database/MeshtasticDatabaseConstructor.kt | Introduces Room KMP expect constructor entrypoint. |
| core/database/src/commonMain/kotlin/org/meshtastic/core/database/MeshtasticDatabase.kt | Adds @ConstructedBy and a shared configureCommon() builder configuration using bundled SQLite. |
| core/database/src/commonMain/kotlin/org/meshtastic/core/database/DatabaseConstants.kt | Moves DB naming/normalization/anonymization helpers to commonMain and uses okio hashing. |
| core/database/src/commonMain/kotlin/org/meshtastic/core/database/Converters.kt | Adds KMP-safe Room type converters in commonMain. |
| core/database/src/androidTest/kotlin/org/meshtastic/core/database/dao/PacketDaoTest.kt | Updates in-memory DB creation to use KMP constructor-based builder. |
| core/database/src/androidTest/kotlin/org/meshtastic/core/database/dao/NodeInfoDaoTest.kt | Updates in-memory DB creation to use KMP constructor-based builder. |
| core/database/src/androidTest/kotlin/org/meshtastic/core/database/MeshtasticDatabaseTest.kt | Updates migration test builder to use KMP constructor + shared configuration. |
| core/database/src/androidMain/kotlin/org/meshtastic/core/database/DatabaseManager.kt | Updates Android DB creation to use constructor-based builder + common configuration; refactors DB name tracking. |
| core/database/build.gradle.kts | Switches module to KMP library plugin and rewires dependencies/KSP for Room KMP. |
| core/database/README.md | Updates module documentation to reflect Room KMP and new component layout. |
| core/data/src/main/kotlin/org/meshtastic/core/data/di/DatabaseModule.kt | Moves Hilt binding of DatabaseManager into :core:data. |
| core/data/src/main/kotlin/org/meshtastic/core/data/datasource/FirmwareReleaseLocalDataSource.kt | Refactors to obtain DAO via DatabaseManager rather than direct injection. |
| core/data/src/main/kotlin/org/meshtastic/core/data/datasource/DeviceHardwareLocalDataSource.kt | Refactors to obtain DAO via DatabaseManager rather than direct injection. |
| core/data/build.gradle.kts | Adds sqlite-bundled dependency alongside Room deps. |
Comments suppressed due to low confidence (2)
core/database/src/androidMain/kotlin/org/meshtastic/core/database/DatabaseManager.kt:132
withDb()derivesactivefrom_currentAddress.valuewhile using the DB instance from_currentDb.value. BecauseswitchActiveDatabase()updates_currentDband_currentAddressseparately (andwithDb()does not take the mutex), there is a race wherewithDb()can observe a new DB with the previous address (or vice versa) and recordlastUsedagainst the wrong DB name. Consider tracking the activedbNamein the same atomic state as the DB (e.g., a singleStateFlow/data class holding both, or an additional_currentDbNameupdated under the same mutex) and use that formarkLastUsed.
core/database/src/androidMain/kotlin/org/meshtastic/core/database/DatabaseManager.kt:203setCacheLimit()computesactiveusing_currentAddress.valuewhen_currentDb.valueis non-null. Since_currentDband_currentAddressare updated independently and this method doesn’t take the mutex, it can record/enforce eviction against the wrong active DB name during a concurrentswitchActiveDatabase(). Using a single source of truth for the activedbName(updated atomically under the mutex) would avoid this inconsistency.
You can also share your feedback on Copilot code review. Take the survey.
| implementation(libs.kotlinx.coroutines.test) | ||
| implementation(libs.androidx.room.testing) | ||
| } | ||
| androidMain.dependencies { implementation(libs.javax.inject) } |
There was a problem hiding this comment.
This module now applies the Android KMP library plugin, but the Gradle source set config doesn’t set up / declare dependencies for the KMP test source sets (androidHostTest / androidDeviceTest) that the repo uses elsewhere (e.g. core/model). As a result, the existing tests under src/test and src/androidTest may no longer be compiled/run, and migration tests may also miss the exported Room schemas as test assets. Consider migrating tests to src/androidHostTest and src/androidDeviceTest, adding the appropriate test dependencies there (JUnit/Robolectric vs AndroidX runner), and wiring $projectDir/schemas into the device-test assets so MigrationTestHelper can load schemas.
| androidMain.dependencies { implementation(libs.javax.inject) } | |
| androidMain.dependencies { implementation(libs.javax.inject) } | |
| val androidHostTest by getting { | |
| dependencies { | |
| implementation(libs.androidx.room.testing) | |
| } | |
| } | |
| val androidDeviceTest by getting { | |
| dependencies { | |
| implementation(libs.androidx.room.testing) | |
| } | |
| resources.srcDir("$projectDir/schemas") | |
| } |
| dependencies { | ||
| implementation(projects.core.repository) | ||
| implementation(projects.core.common) | ||
| implementation(projects.core.di) | ||
| implementation(projects.core.model) | ||
| implementation(projects.core.proto) | ||
| implementation(projects.core.resources) | ||
|
|
||
| implementation(libs.androidx.room.paging) | ||
| implementation(libs.kotlinx.serialization.json) | ||
| implementation(libs.kermit) | ||
|
|
||
| ksp(libs.androidx.room.compiler) | ||
|
|
||
| testImplementation(libs.junit) | ||
| testImplementation(libs.kotlinx.coroutines.test) | ||
| testImplementation(libs.robolectric) | ||
| testImplementation(libs.androidx.test.core) | ||
| testImplementation(libs.androidx.test.ext.junit) | ||
| testImplementation(libs.androidx.room.testing) | ||
|
|
||
| androidTestImplementation(libs.androidx.test.runner) | ||
| androidTestImplementation(libs.androidx.test.ext.junit) | ||
| androidTestImplementation(libs.androidx.room.testing) | ||
| "kspCommonMainMetadata"(libs.androidx.room.compiler) | ||
| "kspAndroid"(libs.androidx.room.compiler) | ||
| } |
There was a problem hiding this comment.
The Room convention plugin (meshtastic.android.room) already adds kspCommonMainMetadata and kspAndroid dependencies for room-compiler when org.jetbrains.kotlin.multiplatform is applied. Duplicating them here is redundant and can make it harder to reason about where KSP wiring comes from; consider removing this block and relying on the convention plugin for KSP configuration.
| Room.databaseBuilder<MeshtasticDatabase>( | ||
| context = context, | ||
| name = context.getDatabasePath(TEST_DB).absolutePath, | ||
| factory = { MeshtasticDatabaseConstructor.initialize() }, | ||
| ) | ||
| .configureCommon() | ||
| .build() | ||
| .apply { openHelper.writableDatabase.close() } | ||
| .apply { close() } |
There was a problem hiding this comment.
In migrateAll(), the DB is now only build()-ed and immediately close()-d. With Room, build() can construct the RoomDatabase without necessarily opening the underlying SQLite connection, which can prevent migrations/schema validation from actually running (the previous openHelper.writableDatabase.close() forced the open). To keep this test meaningful, force-open the DB after build() (e.g., touch openHelper.writableDatabase or run a simple query) before closing it.
|
- Move existing instrumented tests from `androidTest` to `androidDeviceTest`. - Move existing unit tests from `test` to `androidHostTest`. - Update `build.gradle.kts` to configure the new test source sets and associated dependencies. - Ensure the database is explicitly opened in `MeshtasticDatabaseTest` to trigger migrations. Signed-off-by: James Rich <2199651+jamesarich@users.noreply.github.com>
follows https://developer.android.com/kotlin/multiplatform/room to update Room to be able to be used Multiplatform
MeshtasticDatabasedefinition fromandroidMaintocommonMain.MeshtasticDatabaseConstructorusing the Room KMP pattern.DatabaseManagerto useBundledSQLiteDriverand shared database configuration.DatabaseConstantsand address normalization helpers tocommonMain, utilizingokiofor cross-platform SHA-1 hashing.DatabaseModuleto:core:dataand refactorLocalDataSourceclasses to access DAOs viaDatabaseManagerinstead of direct injection.