PM-15177: Improve destructive fallback logic (#4373)

2024-11-23 09:56:11 +03:00 · 2024-11-22 16:21:15 -06:00 · 2024-11-22 16:21:15 -06:00 · 8e7ec7af4c
commit 8e7ec7af4c
parent 2e1845887c
11 changed files with 104 additions and 242 deletions
--- a/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/disk/di/PlatformDiskModule.kt
+++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/disk/di/PlatformDiskModule.kt
@ -37,7 +37,6 @@ import dagger.hilt.InstallIn
 import dagger.hilt.android.qualifiers.ApplicationContext
 import dagger.hilt.components.SingletonComponent
 import kotlinx.serialization.json.Json
-import java.time.Clock
 import javax.inject.Singleton

 /**
@ -74,7 +73,6 @@ object PlatformDiskModule {
    fun provideEventDatabase(
        app: Application,
        databaseSchemeManager: DatabaseSchemeManager,
-        clock: Clock,
    ): PlatformDatabase =
        Room
            .databaseBuilder(
@ -84,12 +82,7 @@ object PlatformDiskModule {
            )
            .fallbackToDestructiveMigration()
            .addTypeConverter(ZonedDateTimeTypeConverter())
-            .addCallback(
-                DatabaseSchemeCallback(
-                    databaseSchemeManager = databaseSchemeManager,
-                    clock = clock,
-                ),
-            )
+            .addCallback(DatabaseSchemeCallback(databaseSchemeManager = databaseSchemeManager))
            .build()

    @Provides
--- a/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/DatabaseSchemeManager.kt
+++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/DatabaseSchemeManager.kt
@ -1,23 +1,18 @@
 package com.x8bit.bitwarden.data.platform.manager

 import kotlinx.coroutines.flow.Flow
-import java.time.Instant

 /**
 * Manager for tracking changes to database scheme(s).
 */
 interface DatabaseSchemeManager {
+    /**
+     * Clears the sync state for all users and emits on the [databaseSchemeChangeFlow].
+     */
+    fun clearSyncState()

    /**
-     * The instant of the last database schema change performed on the database, if any.
-     *
-     * There is only a single scheme change instant tracked for all database schemes. It is expected
-     * that a scheme change to any database will update this value and trigger a sync.
+     * Emits whenever the sync state hs been cleared.
     */
-    var lastDatabaseSchemeChangeInstant: Instant?
-
-    /**
-     * A flow of the last database schema change instant.
-     */
-    val lastDatabaseSchemeChangeInstantFlow: Flow<Instant?>
+    val databaseSchemeChangeFlow: Flow<Unit>
 }
--- a/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/DatabaseSchemeManagerImpl.kt
+++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/DatabaseSchemeManagerImpl.kt
@ -1,34 +1,27 @@
 package com.x8bit.bitwarden.data.platform.manager

+import com.x8bit.bitwarden.data.auth.datasource.disk.AuthDiskSource
 import com.x8bit.bitwarden.data.platform.datasource.disk.SettingsDiskSource
-import com.x8bit.bitwarden.data.platform.manager.dispatcher.DispatcherManager
-import kotlinx.coroutines.CoroutineScope
-import kotlinx.coroutines.flow.SharingStarted
-import kotlinx.coroutines.flow.stateIn
-import java.time.Instant
+import com.x8bit.bitwarden.data.platform.repository.util.bufferedMutableSharedFlow
+import kotlinx.coroutines.flow.Flow
+import kotlinx.coroutines.flow.MutableSharedFlow
+import kotlinx.coroutines.flow.asSharedFlow

 /**
 * Primary implementation of [DatabaseSchemeManager].
 */
 class DatabaseSchemeManagerImpl(
+    val authDiskSource: AuthDiskSource,
    val settingsDiskSource: SettingsDiskSource,
-    val dispatcherManager: DispatcherManager,
 ) : DatabaseSchemeManager {
+    private val mutableSharedFlow: MutableSharedFlow<Unit> = bufferedMutableSharedFlow()

-    private val unconfinedScope = CoroutineScope(dispatcherManager.unconfined)
-
-    override var lastDatabaseSchemeChangeInstant: Instant?
-        get() = settingsDiskSource.lastDatabaseSchemeChangeInstant
-        set(value) {
-            settingsDiskSource.lastDatabaseSchemeChangeInstant = value
+    override fun clearSyncState() {
+        authDiskSource.userState?.accounts?.forEach { (userId, _) ->
+            settingsDiskSource.storeLastSyncTime(userId = userId, lastSyncTime = null)
+        }
+        mutableSharedFlow.tryEmit(Unit)
    }

-    override val lastDatabaseSchemeChangeInstantFlow =
-        settingsDiskSource
-            .lastDatabaseSchemeChangeInstantFlow
-            .stateIn(
-                scope = unconfinedScope,
-                started = SharingStarted.Eagerly,
-                initialValue = settingsDiskSource.lastDatabaseSchemeChangeInstant,
-            )
+    override val databaseSchemeChangeFlow: Flow<Unit> = mutableSharedFlow.asSharedFlow()
 }
--- a/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/di/PlatformManagerModule.kt
+++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/di/PlatformManagerModule.kt
@ -304,10 +304,10 @@ object PlatformManagerModule {
    @Provides
    @Singleton
    fun provideDatabaseSchemeManager(
+        authDiskSource: AuthDiskSource,
        settingsDiskSource: SettingsDiskSource,
-        dispatcherManager: DispatcherManager,
    ): DatabaseSchemeManager = DatabaseSchemeManagerImpl(
+        authDiskSource = authDiskSource,
        settingsDiskSource = settingsDiskSource,
-        dispatcherManager = dispatcherManager,
    )
 }
--- a/app/src/main/java/com/x8bit/bitwarden/data/tools/generator/datasource/disk/di/GeneratorDiskModule.kt
+++ b/app/src/main/java/com/x8bit/bitwarden/data/tools/generator/datasource/disk/di/GeneratorDiskModule.kt
@ -17,7 +17,6 @@ import dagger.Provides
 import dagger.hilt.InstallIn
 import dagger.hilt.components.SingletonComponent
 import kotlinx.serialization.json.Json
-import java.time.Clock
 import javax.inject.Singleton

 /**
@ -51,7 +50,6 @@ object GeneratorDiskModule {
    fun providePasswordHistoryDatabase(
        app: Application,
        databaseSchemeManager: DatabaseSchemeManager,
-        clock: Clock,
    ): PasswordHistoryDatabase {
        return Room
            .databaseBuilder(
@ -59,12 +57,7 @@ object GeneratorDiskModule {
                klass = PasswordHistoryDatabase::class.java,
                name = "passcode_history_database",
            )
-            .addCallback(
-                DatabaseSchemeCallback(
-                    databaseSchemeManager = databaseSchemeManager,
-                    clock = clock,
-                ),
-            )
+            .addCallback(DatabaseSchemeCallback(databaseSchemeManager = databaseSchemeManager))
            .build()
    }

--- a/app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/disk/callback/DatabaseSchemeCallback.kt
+++ b/app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/disk/callback/DatabaseSchemeCallback.kt
@ -3,16 +3,14 @@ package com.x8bit.bitwarden.data.vault.datasource.disk.callback
 import androidx.room.RoomDatabase
 import androidx.sqlite.db.SupportSQLiteDatabase
 import com.x8bit.bitwarden.data.platform.manager.DatabaseSchemeManager
-import java.time.Clock

 /**
 * A [RoomDatabase.Callback] for tracking database scheme changes.
 */
 class DatabaseSchemeCallback(
    private val databaseSchemeManager: DatabaseSchemeManager,
-    private val clock: Clock,
 ) : RoomDatabase.Callback() {
    override fun onDestructiveMigration(db: SupportSQLiteDatabase) {
-        databaseSchemeManager.lastDatabaseSchemeChangeInstant = clock.instant()
+        databaseSchemeManager.clearSyncState()
    }
 }
--- a/app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/disk/di/VaultDiskModule.kt
+++ b/app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/disk/di/VaultDiskModule.kt
@ -19,7 +19,6 @@ import dagger.Provides
 import dagger.hilt.InstallIn
 import dagger.hilt.components.SingletonComponent
 import kotlinx.serialization.json.Json
-import java.time.Clock
 import javax.inject.Singleton

 /**
@ -34,7 +33,6 @@ class VaultDiskModule {
    fun provideVaultDatabase(
        app: Application,
        databaseSchemeManager: DatabaseSchemeManager,
-        clock: Clock,
    ): VaultDatabase =
        Room
            .databaseBuilder(
@ -43,7 +41,7 @@ class VaultDiskModule {
                name = "vault_database",
            )
            .fallbackToDestructiveMigration()
-            .addCallback(DatabaseSchemeCallback(databaseSchemeManager, clock))
+            .addCallback(DatabaseSchemeCallback(databaseSchemeManager = databaseSchemeManager))
            .addTypeConverter(ZonedDateTimeTypeConverter())
            .build()

--- a/app/src/main/java/com/x8bit/bitwarden/data/vault/repository/VaultRepositoryImpl.kt
+++ b/app/src/main/java/com/x8bit/bitwarden/data/vault/repository/VaultRepositoryImpl.kt
@ -99,7 +99,6 @@ import kotlinx.coroutines.flow.asSharedFlow
 import kotlinx.coroutines.flow.asStateFlow
 import kotlinx.coroutines.flow.combine
 import kotlinx.coroutines.flow.filter
-import kotlinx.coroutines.flow.filterNotNull
 import kotlinx.coroutines.flow.first
 import kotlinx.coroutines.flow.firstOrNull
 import kotlinx.coroutines.flow.flatMapLatest
@ -326,9 +325,8 @@ class VaultRepositoryImpl(
            .launchIn(ioScope)

        databaseSchemeManager
-            .lastDatabaseSchemeChangeInstantFlow
-            .filterNotNull()
-            .onEach { sync() }
+            .databaseSchemeChangeFlow
+            .onEach { sync(forced = true) }
            .launchIn(ioScope)
    }

@ -361,13 +359,11 @@ class VaultRepositoryImpl(
        val userId = activeUserId ?: return
        val currentInstant = clock.instant()
        val lastSyncInstant = settingsDiskSource.getLastSyncTime(userId = userId)
-        val lastDatabaseSchemeChangeInstant = databaseSchemeManager.lastDatabaseSchemeChangeInstant

        // Sync if we have never done so, the last time was at last 30 minutes ago, or the database
        // scheme changed since the last sync.
        if (lastSyncInstant == null ||
-            currentInstant.isAfter(lastSyncInstant.plus(30, ChronoUnit.MINUTES)) ||
-            lastDatabaseSchemeChangeInstant?.isAfter(lastSyncInstant) == true
+            currentInstant.isAfter(lastSyncInstant.plus(30, ChronoUnit.MINUTES))
        ) {
            sync()
        }
@ -1347,20 +1343,15 @@ class VaultRepositoryImpl(
            val lastSyncInstant = settingsDiskSource
                .getLastSyncTime(userId = userId)
                ?.toEpochMilli()
-                ?: 0
-            val lastDatabaseSchemeChangeInstant = databaseSchemeManager
-                .lastDatabaseSchemeChangeInstant
-                ?.toEpochMilli()
-                ?: 0
+            lastSyncInstant?.let { lastSyncTimeMs ->
+                // If the lasSyncState is null we just sync, no checks required.
                syncService
                    .getAccountRevisionDateMillis()
                    .fold(
                        onSuccess = { serverRevisionDate ->
-                        if (serverRevisionDate < lastSyncInstant &&
-                            lastDatabaseSchemeChangeInstant < lastSyncInstant
-                        ) {
-                            // We can skip the actual sync call if there is no new data or database
-                            // scheme changes since the last sync.
+                            if (serverRevisionDate < lastSyncTimeMs) {
+                                // We can skip the actual sync call if there is no new data or
+                                // database scheme changes since the last sync.
                                vaultDiskSource.resyncVaultData(userId = userId)
                                settingsDiskSource.storeLastSyncTime(
                                    userId = userId,
@ -1379,6 +1370,7 @@ class VaultRepositoryImpl(
                        },
                    )
            }
+        }

        return syncService
            .sync()
--- a/app/src/test/java/com/x8bit/bitwarden/data/platform/manager/DatabaseSchemeManagerTest.kt
+++ b/app/src/test/java/com/x8bit/bitwarden/data/platform/manager/DatabaseSchemeManagerTest.kt
@ -1,73 +1,61 @@
 package com.x8bit.bitwarden.data.platform.manager

 import app.cash.turbine.test
-import com.x8bit.bitwarden.data.platform.base.FakeDispatcherManager
-import com.x8bit.bitwarden.data.platform.datasource.disk.SettingsDiskSource
-import io.mockk.every
+import com.x8bit.bitwarden.data.auth.datasource.disk.model.UserStateJson
+import com.x8bit.bitwarden.data.auth.datasource.disk.util.FakeAuthDiskSource
+import com.x8bit.bitwarden.data.platform.datasource.disk.util.FakeSettingsDiskSource
 import io.mockk.mockk
-import io.mockk.verify
-import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.test.runTest
-import org.junit.Test
-import org.junit.jupiter.api.Assertions.assertEquals
+import org.junit.jupiter.api.Assertions.assertNotNull
+import org.junit.jupiter.api.Assertions.assertNull
+import org.junit.jupiter.api.BeforeEach
+import org.junit.jupiter.api.Test
 import java.time.Clock
 import java.time.Instant
 import java.time.ZoneOffset

 class DatabaseSchemeManagerTest {

-    private val mutableLastDatabaseSchemeChangeInstantFlow = MutableStateFlow<Instant?>(null)
-    private val mockSettingsDiskSource: SettingsDiskSource = mockk {
-        every {
-            lastDatabaseSchemeChangeInstant
-        } returns mutableLastDatabaseSchemeChangeInstantFlow.value
-        every { lastDatabaseSchemeChangeInstant = any() } answers {
-            mutableLastDatabaseSchemeChangeInstantFlow.value = firstArg()
-        }
-        every {
-            lastDatabaseSchemeChangeInstantFlow
-        } returns mutableLastDatabaseSchemeChangeInstantFlow
-    }
-    private val dispatcherManager = FakeDispatcherManager()
+    private val fakeAuthDiskSource = FakeAuthDiskSource()
+    private val fakeSettingsDiskSource = FakeSettingsDiskSource()
    private val databaseSchemeManager = DatabaseSchemeManagerImpl(
-        settingsDiskSource = mockSettingsDiskSource,
-        dispatcherManager = dispatcherManager,
+        authDiskSource = fakeAuthDiskSource,
+        settingsDiskSource = fakeSettingsDiskSource,
    )

-    @Suppress("MaxLineLength")
-    @Test
-    fun `setLastDatabaseSchemeChangeInstant persists value in settingsDiskSource`() {
-        databaseSchemeManager.lastDatabaseSchemeChangeInstant = FIXED_CLOCK.instant()
-        verify {
-            mockSettingsDiskSource.lastDatabaseSchemeChangeInstant = FIXED_CLOCK.instant()
-        }
+    @BeforeEach
+    fun setup() {
+        fakeAuthDiskSource.userState = USER_STATE
+        fakeSettingsDiskSource.storeLastSyncTime(USER_ID_1, FIXED_CLOCK.instant())
+        fakeSettingsDiskSource.storeLastSyncTime(USER_ID_2, FIXED_CLOCK.instant())
    }

    @Test
-    fun `setLastDatabaseSchemeChangeInstant does emit value`() = runTest {
-        databaseSchemeManager.lastDatabaseSchemeChangeInstantFlow.test {
-            // Assert the value is initialized to null
-            assertEquals(
-                null,
-                awaitItem(),
-            )
-            // Assert the new value is emitted
-            databaseSchemeManager.lastDatabaseSchemeChangeInstant = FIXED_CLOCK.instant()
-            assertEquals(
-                FIXED_CLOCK.instant(),
-                awaitItem(),
-            )
+    fun `clearSyncState clears lastSyncTimes and emit`() = runTest {
+        assertNotNull(fakeSettingsDiskSource.getLastSyncTime(USER_ID_1))
+        assertNotNull(fakeSettingsDiskSource.getLastSyncTime(USER_ID_2))
+
+        databaseSchemeManager.databaseSchemeChangeFlow.test {
+            databaseSchemeManager.clearSyncState()
+            awaitItem()
+            expectNoEvents()
+        }
+
+        assertNull(fakeSettingsDiskSource.getLastSyncTime(USER_ID_1))
+        assertNull(fakeSettingsDiskSource.getLastSyncTime(USER_ID_2))
    }
 }

-    @Test
-    fun `getLastDatabaseSchemeChangeInstant retrieves stored value from settingsDiskSource`() {
-        databaseSchemeManager.lastDatabaseSchemeChangeInstant
-        verify {
-            mockSettingsDiskSource.lastDatabaseSchemeChangeInstant
-        }
-    }
-}
+private const val USER_ID_1: String = "USER_ID_1"
+private const val USER_ID_2: String = "USER_ID_2"
+
+private val USER_STATE: UserStateJson = UserStateJson(
+    activeUserId = USER_ID_1,
+    accounts = mapOf(
+        USER_ID_1 to mockk(),
+        USER_ID_2 to mockk(),
+    ),
+)

 private val FIXED_CLOCK: Clock = Clock.fixed(
    Instant.parse("2023-10-27T12:00:00Z"),
--- a/app/src/test/java/com/x8bit/bitwarden/data/vault/datasource/disk/callback/DatabaseSchemeCallbackTest.kt
+++ b/app/src/test/java/com/x8bit/bitwarden/data/vault/datasource/disk/callback/DatabaseSchemeCallbackTest.kt
@ -7,26 +7,17 @@ import io.mockk.mockk
 import io.mockk.runs
 import io.mockk.verify
 import org.junit.Test
-import java.time.Clock
-import java.time.Instant
-import java.time.ZoneOffset

 class DatabaseSchemeCallbackTest {

    private val databaseSchemeManager: DatabaseSchemeManager = mockk {
-        every { lastDatabaseSchemeChangeInstant = any() } just runs
+        every { clearSyncState() } just runs
    }
-    private val callback = DatabaseSchemeCallback(databaseSchemeManager, FIXED_CLOCK)
+    private val callback = DatabaseSchemeCallback(databaseSchemeManager)

    @Test
-    fun `onDestructiveMigration updates lastDatabaseSchemeChangeInstant`() {
+    fun `onDestructiveMigration calls clearSyncState`() {
        callback.onDestructiveMigration(mockk())
-
-        verify { databaseSchemeManager.lastDatabaseSchemeChangeInstant = FIXED_CLOCK.instant() }
+        verify(exactly = 1) { databaseSchemeManager.clearSyncState() }
    }
 }
-
-private val FIXED_CLOCK: Clock = Clock.fixed(
-    Instant.parse("2023-10-27T12:00:00Z"),
-    ZoneOffset.UTC,
-)
--- a/app/src/test/java/com/x8bit/bitwarden/data/vault/repository/VaultRepositoryTest.kt
+++ b/app/src/test/java/com/x8bit/bitwarden/data/vault/repository/VaultRepositoryTest.kt
@ -193,14 +193,9 @@ class VaultRepositoryTest {
            mutableUnlockedUserIdsStateFlow.first { userId in it }
        }
    }
-    private val mutableLastDatabaseSchemeChangeInstantFlow = MutableStateFlow<Instant?>(null)
+    private val mutableDatabaseSchemeChangeFlow = bufferedMutableSharedFlow<Unit>()
    private val databaseSchemeManager: DatabaseSchemeManager = mockk {
-        every {
-            lastDatabaseSchemeChangeInstant
-        } returns mutableLastDatabaseSchemeChangeInstantFlow.value
-        every {
-            lastDatabaseSchemeChangeInstantFlow
-        } returns mutableLastDatabaseSchemeChangeInstantFlow
+        every { databaseSchemeChangeFlow } returns mutableDatabaseSchemeChangeFlow
    }

    private val mutableFullSyncFlow = bufferedMutableSharedFlow<Unit>()
@ -787,35 +782,15 @@ class VaultRepositoryTest {
    }

    @Test
-    fun `lastDatabaseSchemeChangeInstantFlow should trigger sync when new value is not null`() =
-        runTest {
+    fun `databaseSchemeChangeFlow should trigger sync on emission`() = runTest {
        fakeAuthDiskSource.userState = MOCK_USER_STATE
-            every {
-                databaseSchemeManager.lastDatabaseSchemeChangeInstant
-            } returns mutableLastDatabaseSchemeChangeInstantFlow.value
        coEvery { syncService.sync() } just awaits

-            mutableLastDatabaseSchemeChangeInstantFlow.value = clock.instant()
+        mutableDatabaseSchemeChangeFlow.tryEmit(Unit)

        coVerify(exactly = 1) { syncService.sync() }
    }

-    @Test
-    fun `lastDatabaseSchemeChangeInstantFlow should not sync when new value is null`() =
-        runTest {
-            fakeAuthDiskSource.userState = MOCK_USER_STATE
-
-            every {
-                databaseSchemeManager.lastDatabaseSchemeChangeInstant
-            } returns mutableLastDatabaseSchemeChangeInstantFlow.value
-
-            coEvery { syncService.sync() } just awaits
-
-            mutableLastDatabaseSchemeChangeInstantFlow.value = null
-
-            coVerify(exactly = 0) { syncService.sync() }
-        }
-
    @Test
    fun `sync with forced should skip checks and call the syncService sync`() {
        fakeAuthDiskSource.userState = MOCK_USER_STATE
@ -1123,60 +1098,6 @@ class VaultRepositoryTest {
        coVerify(exactly = 0) { syncService.sync() }
    }

-    @Test
-    fun `syncIfNecessary when there is no last scheme change should not sync the vault`() {
-        val userId = "mockId-1"
-        fakeAuthDiskSource.userState = MOCK_USER_STATE
-        every {
-            settingsDiskSource.getLastSyncTime(userId)
-        } returns clock.instant().minus(1, ChronoUnit.MINUTES)
-        every {
-            databaseSchemeManager.lastDatabaseSchemeChangeInstant
-        } returns null
-        coEvery { syncService.sync() } just awaits
-
-        vaultRepository.syncIfNecessary()
-
-        coVerify(exactly = 0) { syncService.sync() }
-    }
-
-    @Suppress("MaxLineLength")
-    @Test
-    fun `syncIfNecessary when the last scheme change is before the last sync time should not sync the vault`() {
-        val userId = "mockId-1"
-        fakeAuthDiskSource.userState = MOCK_USER_STATE
-        every {
-            settingsDiskSource.getLastSyncTime(userId)
-        } returns clock.instant().plus(1, ChronoUnit.MINUTES)
-        every {
-            databaseSchemeManager.lastDatabaseSchemeChangeInstant
-        } returns clock.instant().minus(1, ChronoUnit.MINUTES)
-
-        coEvery { syncService.sync() } just awaits
-
-        vaultRepository.syncIfNecessary()
-
-        coVerify(exactly = 0) { syncService.sync() }
-    }
-
-    @Suppress("MaxLineLength")
-    @Test
-    fun `syncIfNecessary when the last scheme change is after the last sync time should sync the vault`() {
-        val userId = "mockId-1"
-        fakeAuthDiskSource.userState = MOCK_USER_STATE
-        every {
-            settingsDiskSource.getLastSyncTime(userId)
-        } returns clock.instant().minus(1, ChronoUnit.MINUTES)
-        every {
-            databaseSchemeManager.lastDatabaseSchemeChangeInstant
-        } returns clock.instant().plus(1, ChronoUnit.MINUTES)
-        coEvery { syncService.sync() } just awaits
-
-        vaultRepository.syncIfNecessary()
-
-        coVerify { syncService.sync() }
-    }
-
    @Test
    fun `sync when the last sync time is older than the revision date should sync the vault`() {
        val userId = "mockId-1"