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

This commit is contained in:
David Perez 2024-11-22 15:59:40 -06:00 committed by GitHub
parent 366c86da41
commit 019bf8d0fa
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
11 changed files with 104 additions and 242 deletions

View file

@ -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

View file

@ -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>
}

View file

@ -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()
}

View file

@ -307,10 +307,10 @@ object PlatformManagerModule {
@Provides
@Singleton
fun provideDatabaseSchemeManager(
authDiskSource: AuthDiskSource,
settingsDiskSource: SettingsDiskSource,
dispatcherManager: DispatcherManager,
): DatabaseSchemeManager = DatabaseSchemeManagerImpl(
authDiskSource = authDiskSource,
settingsDiskSource = settingsDiskSource,
dispatcherManager = dispatcherManager,
)
}

View file

@ -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()
}

View file

@ -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()
}
}

View file

@ -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()

View file

@ -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,37 +1343,33 @@ class VaultRepositoryImpl(
val lastSyncInstant = settingsDiskSource
.getLastSyncTime(userId = userId)
?.toEpochMilli()
?: 0
val lastDatabaseSchemeChangeInstant = databaseSchemeManager
.lastDatabaseSchemeChangeInstant
?.toEpochMilli()
?: 0
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.
vaultDiskSource.resyncVaultData(userId = userId)
settingsDiskSource.storeLastSyncTime(
userId = userId,
lastSyncTime = clock.instant(),
)
val itemsAvailable = vaultDiskSource
.getCiphers(userId)
.firstOrNull()
?.isNotEmpty() == true
return SyncVaultDataResult.Success(itemsAvailable = itemsAvailable)
}
},
onFailure = {
updateVaultStateFlowsToError(throwable = it)
return SyncVaultDataResult.Error(throwable = it)
},
)
lastSyncInstant?.let { lastSyncTimeMs ->
// If the lasSyncState is null we just sync, no checks required.
syncService
.getAccountRevisionDateMillis()
.fold(
onSuccess = { serverRevisionDate ->
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,
lastSyncTime = clock.instant(),
)
val itemsAvailable = vaultDiskSource
.getCiphers(userId)
.firstOrNull()
?.isNotEmpty() == true
return SyncVaultDataResult.Success(itemsAvailable = itemsAvailable)
}
},
onFailure = {
updateVaultStateFlowsToError(throwable = it)
return SyncVaultDataResult.Error(throwable = it)
},
)
}
}
return syncService

View file

@ -1,74 +1,62 @@
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))
@Test
fun `getLastDatabaseSchemeChangeInstant retrieves stored value from settingsDiskSource`() {
databaseSchemeManager.lastDatabaseSchemeChangeInstant
verify {
mockSettingsDiskSource.lastDatabaseSchemeChangeInstant
databaseSchemeManager.databaseSchemeChangeFlow.test {
databaseSchemeManager.clearSyncState()
awaitItem()
expectNoEvents()
}
assertNull(fakeSettingsDiskSource.getLastSyncTime(USER_ID_1))
assertNull(fakeSettingsDiskSource.getLastSyncTime(USER_ID_2))
}
}
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"),
ZoneOffset.UTC,

View file

@ -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,
)

View file

@ -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,34 +782,14 @@ class VaultRepositoryTest {
}
@Test
fun `lastDatabaseSchemeChangeInstantFlow should trigger sync when new value is not null`() =
runTest {
fakeAuthDiskSource.userState = MOCK_USER_STATE
every {
databaseSchemeManager.lastDatabaseSchemeChangeInstant
} returns mutableLastDatabaseSchemeChangeInstantFlow.value
coEvery { syncService.sync() } just awaits
fun `databaseSchemeChangeFlow should trigger sync on emission`() = runTest {
fakeAuthDiskSource.userState = MOCK_USER_STATE
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() }
}
coVerify(exactly = 1) { syncService.sync() }
}
@Test
fun `sync with forced should skip checks and call the syncService sync`() {
@ -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"