Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds Room database support for API v2 to handle authenticated user transfers, while maintaining the existing Realm database for guest users. The implementation introduces new database entities, DAOs, platform-specific configurations, and comprehensive test coverage.
Changes:
- Added Room database dependencies (room 2.8.4, ksp plugin, sqlite-bundled, robolectric for testing)
- Created Room entities (TransferDB, FileDB) and DAOs (TransferDao, UploadDao) for v2 transfers
- Implemented platform-specific DatabaseProvider and DatabaseConfig for Android and Apple platforms
- Updated AccountManager to use Room for AuthUser and Realm for GuestUser
- Added PENDING_UPLOAD status to TransferStatus enum and changed Transfer interface defaults
Reviewed changes
Copilot reviewed 29 out of 30 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| gradle/libs.versions.toml | Added Room, KSP, SQLite, Robolectric dependencies |
| build.gradle.kts | Applied KSP and Room plugins at project level |
| STDatabase/build.gradle.kts | Configured Room schema directory and KSP processors |
| DatabaseProvider.kt | Core Room database builder with platform abstractions |
| DatabaseConfig.kt | Platform-specific database configuration class |
| Converters.kt | Type converters for Set serialization |
| TransferDB.kt | Room entity for transfers with TransferWithFiles relation |
| FileDB.kt | Room entity for files with foreign key to TransferDB |
| TransferDao.kt | DAO with queries for transfer CRUD operations |
| UploadDao.kt | DAO with queries specific to upload transfers |
| AccountManager.kt | Updated to handle both Room (AuthUser) and Realm (GuestUser) |
| SwissTransferInjection.kt | Added DatabaseConfig parameter and appDatabase initialization |
| Transfer.kt (v2) | Changed transferDirection/transferStatus from nullable to non-nullable |
| TransferStatus.kt | Added PENDING_UPLOAD status |
| RobolectricTestsBase.kt | Platform-specific test base class for Android/Robolectric |
| DatabaseConfigFactory.kt | Platform-specific test database configuration factory |
| TransfersTest.kt | Comprehensive tests for transfer operations |
| UploadTest.kt | Tests for upload-specific operations |
| DummyTransferForV2.kt | Test data for v2 transfers |
| Database schemas | Generated Room schema JSON files for version 1 |
Comments suppressed due to low confidence (1)
STCore/src/commonMain/kotlin/com/infomaniak/multiplatform_swisstransfer/managers/AccountManager.kt:65
- Inconsistent behavior in loadUser for AuthUser. When a GuestUser is loaded, the database is initialized (lines 61-62), but when an AuthUser is loaded with a different id, no database initialization occurs. This asymmetry could lead to issues where AuthUser data isn't properly initialized. Consider adding appropriate database initialization for AuthUser as well.
mutex.withLock {
if (currentUser?.id != user.id && user is STUser.GuestUser) {
appSettingsController.initAppSettings(emailLanguageUtils.getEmailLanguageFromLocal())
realmProvider.openTransfersDb(user.id)
}
currentUser = user
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...kotlin/com/infomaniak/multiplatform_swisstransfer/common/interfaces/transfers/v2/Transfer.kt
Show resolved
Hide resolved
...base/src/commonMain/kotlin/com/infomaniak/multiplatform_swisstransfer/database/Converters.kt
Outdated
Show resolved
Hide resolved
...e/src/commonMain/kotlin/com/infomaniak/multiplatform_swisstransfer/database/dao/UploadDao.kt
Outdated
Show resolved
Hide resolved
...src/commonMain/kotlin/com/infomaniak/multiplatform_swisstransfer/database/dao/TransferDao.kt
Outdated
Show resolved
Hide resolved
...emas/com.infomaniak.multiplatform_swisstransfer.database.DatabaseProvider.AppDatabase/1.json
Outdated
Show resolved
Hide resolved
...ain/kotlin/com/infomaniak/multiplatform_swisstransfer/database/models/transfers/v2/FileDB.kt
Show resolved
Hide resolved
...base/src/commonMain/kotlin/com/infomaniak/multiplatform_swisstransfer/database/Converters.kt
Show resolved
Hide resolved
...e/src/commonMain/kotlin/com/infomaniak/multiplatform_swisstransfer/database/dao/UploadDao.kt
Outdated
Show resolved
Hide resolved
...e/src/commonMain/kotlin/com/infomaniak/multiplatform_swisstransfer/database/dao/UploadDao.kt
Outdated
Show resolved
Hide resolved
| if (user is STUser.AuthUser) { | ||
| appDatabase.getTransferDao().deleteTransfers(user.id) | ||
| } else { | ||
| uploadController.removeData() | ||
| transferController.removeData() | ||
| realmProvider.closeAllDatabases() | ||
| } |
There was a problem hiding this comment.
Missing appSettingsController.removeData() call for AuthUser logout. In the GuestUser logout path (line 77), appSettingsController.removeData() is called, but it's not called for AuthUser. This inconsistency could leave settings data behind when an authenticated user logs out.
| val transferDirection: TransferDirection get() = TransferDirection.RECEIVED | ||
| val transferStatus: TransferStatus get() = TransferStatus.READY |
There was a problem hiding this comment.
Why do we need default values? Can they not be abstract?
There was a problem hiding this comment.
Probably the best solution we have to avoid polluting the TransferApi model with properties that don't exist on the backend (@Ignore wouldn't give us this advantage).
BTW, code using that doesn't exist yet (apart from tests).
There was a problem hiding this comment.
We might remove this common interface, to just have 2 separate models connected only by toXxx conversion extension functions.
If/when we do this, it will remove this kind of confusion.
| } else { | ||
| uploadController.removeData() | ||
| transferController.removeData() | ||
| realmProvider.closeAllDatabases() |
There was a problem hiding this comment.
Will the appSettingsDb be migrated to Room at some point?
There was a problem hiding this comment.
Yes, it's coming in a subsequent PR (not opened yet)
| import org.robolectric.annotation.Config | ||
|
|
||
| @RunWith(RobolectricTestRunner::class) | ||
| @Config(sdk = [35]) |
There was a problem hiding this comment.
Should we not bump this to 36 for KMP?
There was a problem hiding this comment.
Let's make a PR to update this, and the project's compileSdk too.
Feel free to do it if you have time before I do.
.../kotlin/com/infomaniak/multiplatform_swisstransfer/database/v2/dataset/DummyTransferForV2.kt
Outdated
Show resolved
Hide resolved
| object DummyTransferForV2 { | ||
| val expired: Transfer | ||
| val notExpired: Transfer | ||
|
|
||
| val transfer1 = object : Transfer { | ||
| override val id: String = "transferLinkUUID1" | ||
| override val senderEmail: String = "" | ||
| override val title: String = "" | ||
| override val message: String = "" | ||
| override val createdAt: Long = 0 | ||
| override val expiresAt: Long = 1_730_458_842L // 01/11/2024 | ||
| override val totalSize: Long = 0 | ||
| } | ||
|
|
||
| val transfer2 = object : Transfer by transfer1 { | ||
| override val id: String = "transfer2" | ||
| override var expiresAt: Long = 4_102_441_200L // 01/01/2100 | ||
| override val transferStatus: TransferStatus = TransferStatus.WAIT_VIRUS_CHECK | ||
| } | ||
|
|
||
| // Transfer with downloadCounterCredit greater than 0. | ||
| val transfer3 = object : Transfer by transfer1 { | ||
| override val id: String = "transfer3" | ||
| override var expiresAt: Long = 4_102_441_200L // 01/01/2100 | ||
| override val transferStatus: TransferStatus = TransferStatus.WAIT_VIRUS_CHECK | ||
| } | ||
|
|
||
| // Transfer with downloadCounterCredit equal to 0. | ||
| val transfer4 = object : Transfer by transfer1 { | ||
| override val id: String = "transfer4" | ||
| override var expiresAt: Long = 4_102_441_200L // 01/01/2100 | ||
| override val transferStatus: TransferStatus = TransferStatus.WAIT_VIRUS_CHECK | ||
| } | ||
|
|
||
| init { | ||
| expired = transfer1 | ||
| notExpired = transfer2 | ||
| } |
There was a problem hiding this comment.
Have you considered simplifying it something like this?
object DummyTransferForV2 {
private fun transferOf(
id: String,
expiresAt: Long,
transferStatus: TransferStatus = TransferStatus.READY,
): Transfer = object : Transfer {
override val id: String = id
override val senderEmail: String = ""
override val title: String? = null
override val message: String? = null
override val createdAt: Long = 0
override val expiresAt: Long = expiresAt
override val totalSize: Long = 0
override val transferStatus: TransferStatus = transferStatus
}
val transfer1 = transferOf("transferLinkUUID1", 1_730_458_842L) // 01/11/2024
val transfer2 = transferOf("transfer2", 4_102_441_200L, TransferStatus.WAIT_VIRUS_CHECK) // 01/01/2100
val transfer3 = transferOf("transfer3", 4_102_441_200L, TransferStatus.WAIT_VIRUS_CHECK)
val transfer4 = transferOf("transfer4", 4_102_441_200L, TransferStatus.WAIT_VIRUS_CHECK)
val expired: Transfer = transfer1
val notExpired: Transfer = transfer2There was a problem hiding this comment.
Agreed. Doing it, with some slight improvements.
| val updatedTransfer = TransferDB( | ||
| id = transfer.id, | ||
| senderEmail = transfer.senderEmail, | ||
| title = "Updated Title", | ||
| message = transfer.message, | ||
| createdAt = transfer.createdAt, | ||
| expiresAt = transfer.expiresAt, | ||
| totalSize = transfer.totalSize, | ||
| userOwnerId = userId, | ||
| transferDirection = TransferDirection.SENT, | ||
| transferStatus = TransferStatus.READY, | ||
| ) |
There was a problem hiding this comment.
Can't we copy the old instance and only modify the title?
3bc0bf4 to
6ffe0e1
Compare
|
This PR/issue depends on:
|
1 similar comment
|
This PR/issue depends on:
|
|



Depends on #230