fix cache cleanup deleting more statuses than it should (#2348)

* fix cache cleanup deleting more statuses than it should * reset LOAD_AT_ONCE * improve tests * move cache clean code back to ViewModel
2022-02-21 19:33:10 +01:00 · 2022-02-21 19:33:10 +01:00 · 69bcc92c46
commit 69bcc92c46
parent a4c2ca6cfc
5 changed files with 118 additions and 42 deletions
--- a/app/src/main/java/com/keylesspalace/tusky/appstore/CacheUpdater.kt
+++ b/app/src/main/java/com/keylesspalace/tusky/appstore/CacheUpdater.kt
@ -6,12 +6,11 @@ import com.keylesspalace.tusky.db.AppDatabase
 import io.reactivex.rxjava3.core.Single
 import io.reactivex.rxjava3.disposables.Disposable
 import io.reactivex.rxjava3.schedulers.Schedulers
-import java.util.concurrent.TimeUnit
 import javax.inject.Inject

 class CacheUpdater @Inject constructor(
    eventHub: EventHub,
-    accountManager: AccountManager,
+    private val accountManager: AccountManager,
    private val appDatabase: AppDatabase,
    gson: Gson
 ) {
@ -21,11 +20,6 @@ class CacheUpdater @Inject constructor(
    init {
        val timelineDao = appDatabase.timelineDao()

-        Schedulers.io().scheduleDirect {
-            val olderThan = System.currentTimeMillis() - CLEANUP_INTERVAL
-            appDatabase.timelineDao().cleanup(olderThan)
-        }
-
        disposable = eventHub.events.subscribe { event ->
            val accountId = accountManager.activeAccount?.id ?: return@subscribe
            when (event) {
@ -61,8 +55,4 @@ class CacheUpdater @Inject constructor(
            .subscribeOn(Schedulers.io())
            .subscribe()
    }
-
-    companion object {
-        val CLEANUP_INTERVAL = TimeUnit.DAYS.toMillis(14)
-    }
 }
--- a/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/CachedTimelineViewModel.kt
+++ b/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/CachedTimelineViewModel.kt
@ -43,11 +43,14 @@ import com.keylesspalace.tusky.network.TimelineCases
 import com.keylesspalace.tusky.util.dec
 import com.keylesspalace.tusky.util.inc
 import com.keylesspalace.tusky.viewdata.StatusViewData
+import kotlinx.coroutines.delay
 import kotlinx.coroutines.flow.map
 import kotlinx.coroutines.launch
 import kotlinx.coroutines.rx3.await
 import retrofit2.HttpException
 import javax.inject.Inject
+import kotlin.time.DurationUnit
+import kotlin.time.toDuration

 /**
 * TimelineViewModel that caches all statuses in a local database
@ -81,6 +84,16 @@ class CachedTimelineViewModel @Inject constructor(
        }
        .cachedIn(viewModelScope)

+    init {
+        viewModelScope.launch {
+            delay(5.toDuration(DurationUnit.SECONDS)) // delay so the db is not locked during initial ui refresh
+            accountManager.activeAccount?.id?.let { accountId ->
+                db.timelineDao().cleanup(accountId, MAX_STATUSES_IN_CACHE)
+                db.timelineDao().cleanupAccounts(accountId)
+            }
+        }
+    }
+
    override fun updatePoll(newPoll: Poll, status: StatusViewData.Concrete) {
        // handled by CacheUpdater
    }
@ -207,4 +220,8 @@ class CachedTimelineViewModel @Inject constructor(
            }
        }
    }
+
+    companion object {
+        private const val MAX_STATUSES_IN_CACHE = 1000
+    }
 }
--- a/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/TimelineViewModel.kt
+++ b/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/TimelineViewModel.kt
@ -43,7 +43,6 @@ import com.keylesspalace.tusky.settings.PrefKeys
 import com.keylesspalace.tusky.viewdata.StatusViewData
 import kotlinx.coroutines.Job
 import kotlinx.coroutines.flow.Flow
-import kotlinx.coroutines.flow.collect
 import kotlinx.coroutines.launch
 import kotlinx.coroutines.rx3.asFlow
 import kotlinx.coroutines.rx3.await
--- a/app/src/main/java/com/keylesspalace/tusky/db/TimelineDao.kt
+++ b/app/src/main/java/com/keylesspalace/tusky/db/TimelineDao.kt
@ -98,8 +98,29 @@ AND serverId = :statusId"""
    )
    abstract fun delete(accountId: Long, statusId: String)

-    @Query("""DELETE FROM TimelineStatusEntity WHERE createdAt < :olderThan""")
-    abstract fun cleanup(olderThan: Long)
+    /**
+     * Cleans the TimelineStatusEntity table from old status entries.
+     * @param accountId id of the account for which to clean statuses
+     * @param limit how many statuses to keep
+     */
+    @Query(
+        """DELETE FROM TimelineStatusEntity WHERE timelineUserId = :accountId AND serverId NOT IN
+        (SELECT serverId FROM TimelineStatusEntity WHERE timelineUserId = :accountId ORDER BY LENGTH(serverId) DESC, serverId DESC LIMIT :limit)
+    """
+    )
+    abstract suspend fun cleanup(accountId: Long, limit: Int)
+
+    /**
+     * Cleans the TimelineAccountEntity table from accounts that are no longer referenced in the TimelineStatusEntity table
+     * @param accountId id of the user account for which to clean timeline accounts
+     */
+    @Query(
+        """DELETE FROM TimelineAccountEntity WHERE timelineUserId = :accountId AND serverId NOT IN
+        (SELECT authorServerId FROM TimelineStatusEntity WHERE timelineUserId = :accountId) 
+        AND serverId NOT IN 
+        (SELECT reblogAccountId FROM TimelineStatusEntity WHERE timelineUserId = :accountId AND reblogAccountId IS NOT NULL)"""
+    )
+    abstract suspend fun cleanupAccounts(accountId: Long)

    @Query(
        """UPDATE TimelineStatusEntity SET poll = :poll
--- a/app/src/test/java/com/keylesspalace/tusky/db/TimelineDaoTest.kt
+++ b/app/src/test/java/com/keylesspalace/tusky/db/TimelineDaoTest.kt
@ -5,7 +5,6 @@ import androidx.room.Room
 import androidx.test.ext.junit.runners.AndroidJUnit4
 import androidx.test.platform.app.InstrumentationRegistry
 import com.google.gson.Gson
-import com.keylesspalace.tusky.appstore.CacheUpdater
 import com.keylesspalace.tusky.components.timeline.Placeholder
 import com.keylesspalace.tusky.components.timeline.toEntity
 import com.keylesspalace.tusky.entity.Status
@ -66,28 +65,25 @@ class TimelineDaoTest {

    @Test
    fun cleanup() = runBlocking {
-        val now = System.currentTimeMillis()
-        val oldDate = now - CacheUpdater.CLEANUP_INTERVAL - 20_000
-        val oldThisAccount = makeStatus(
-            statusId = 5,
-            createdAt = oldDate
-        )
-        val oldAnotherAccount = makeStatus(
-            statusId = 10,
-            createdAt = oldDate,
-            accountId = 2
-        )
-        val recentThisAccount = makeStatus(
-            statusId = 30,
-            createdAt = System.currentTimeMillis()
-        )
-        val recentAnotherAccount = makeStatus(
-            statusId = 60,
-            createdAt = System.currentTimeMillis(),
-            accountId = 2
+
+        val statusesBeforeCleanup = listOf(
+            makeStatus(statusId = 100),
+            makeStatus(statusId = 10, authorServerId = "3"),
+            makeStatus(statusId = 8, reblog = true, authorServerId = "10"),
+            makeStatus(statusId = 5),
+            makeStatus(statusId = 3, authorServerId = "4"),
+            makeStatus(statusId = 2, accountId = 2, authorServerId = "5"),
+            makeStatus(statusId = 1, authorServerId = "5")
        )

-        for ((status, author, reblogAuthor) in listOf(oldThisAccount, oldAnotherAccount, recentThisAccount, recentAnotherAccount)) {
+        val statusesAfterCleanup = listOf(
+            makeStatus(statusId = 100),
+            makeStatus(statusId = 10, authorServerId = "3"),
+            makeStatus(statusId = 8, reblog = true, authorServerId = "10"),
+            makeStatus(statusId = 2, accountId = 2, authorServerId = "5"),
+        )
+
+        for ((status, author, reblogAuthor) in statusesBeforeCleanup) {
            timelineDao.insertAccount(author)
            reblogAuthor?.let {
                timelineDao.insertAccount(it)
@ -95,15 +91,34 @@ class TimelineDaoTest {
            timelineDao.insertStatus(status)
        }

-        timelineDao.cleanup(now - CacheUpdater.CLEANUP_INTERVAL)
+        timelineDao.cleanup(accountId = 1, limit = 3)
+        timelineDao.cleanupAccounts(accountId = 1)

        val loadParams: PagingSource.LoadParams<Int> = PagingSource.LoadParams.Refresh(null, 100, false)

-        val loadedStatusAccount1 = (timelineDao.getStatusesForAccount(1).load(loadParams) as PagingSource.LoadResult.Page).data
-        val loadedStatusAccount2 = (timelineDao.getStatusesForAccount(2).load(loadParams) as PagingSource.LoadResult.Page).data
+        val loadedStatuses = (timelineDao.getStatusesForAccount(1).load(loadParams) as PagingSource.LoadResult.Page).data

-        assertStatuses(listOf(recentThisAccount), loadedStatusAccount1)
-        assertStatuses(listOf(recentAnotherAccount), loadedStatusAccount2)
+        assertStatuses(statusesAfterCleanup, loadedStatuses)
+
+        val loadedAccounts: MutableList<Pair<Long, String>> = mutableListOf()
+        val accountCursor = db.query("SELECT timelineUserId, serverId FROM TimelineAccountEntity", null)
+        accountCursor.moveToFirst()
+        while (!accountCursor.isAfterLast) {
+            val accountId: Long = accountCursor.getLong(accountCursor.getColumnIndex("timelineUserId"))
+            val serverId: String = accountCursor.getString(accountCursor.getColumnIndex("serverId"))
+            loadedAccounts.add(accountId to serverId)
+            accountCursor.moveToNext()
+        }
+
+        val expectedAccounts = listOf(
+            1L to "3",
+            1L to "10",
+            1L to "R10",
+            1L to "20",
+            2L to "5"
+        )
+
+        assertEquals(expectedAccounts, loadedAccounts)
    }

    @Test
@ -115,8 +130,6 @@ class TimelineDaoTest {
            makeStatus(statusId = 1)
        )

-        timelineDao.deleteRange(1, oldStatuses.last().first.serverId, oldStatuses.first().first.serverId)
-
        for ((status, author, reblogAuthor) in oldStatuses) {
            timelineDao.insertAccount(author)
            reblogAuthor?.let {
@ -152,6 +165,42 @@ class TimelineDaoTest {
        assertStatuses(newStatuses, loadedStatuses)
    }

+    @Test
+    fun deleteRange() = runBlocking {
+        val statuses = listOf(
+            makeStatus(statusId = 100),
+            makeStatus(statusId = 15),
+            makeStatus(statusId = 14),
+            makeStatus(statusId = 13),
+            makeStatus(statusId = 12),
+            makeStatus(statusId = 11),
+            makeStatus(statusId = 9)
+        )
+
+        for ((status, author, reblogAuthor) in statuses) {
+            timelineDao.insertAccount(author)
+            reblogAuthor?.let {
+                timelineDao.insertAccount(it)
+            }
+            timelineDao.insertStatus(status)
+        }
+
+        timelineDao.deleteRange(1, "12", "14")
+
+        val pagingSource = timelineDao.getStatusesForAccount(1)
+        val loadResult = pagingSource.load(PagingSource.LoadParams.Refresh(null, 100, false))
+        val loadedStatuses = (loadResult as PagingSource.LoadResult.Page).data
+
+        val remainingStatuses = listOf(
+            makeStatus(statusId = 100),
+            makeStatus(statusId = 15),
+            makeStatus(statusId = 11),
+            makeStatus(statusId = 9)
+        )
+
+        assertStatuses(remainingStatuses, loadedStatuses)
+    }
+
    @Test
    fun deleteAllForInstance() = runBlocking {