Merge pull request #3698 from CruGlobal/preventAttachmentsDaoCrash

GT-2427 Prevent storing Attachments for Tools that don't exist yet

library/db/src/main/kotlin/org/cru/godtools/db/room/repository/AttachmentsRoomRepository.kt

@@ -26,8 +26,11 @@ internal abstract class AttachmentsRoomRepository(private val db: GodToolsRoomDa
     override suspend fun updateAttachmentDownloaded(id: Long, isDownloaded: Boolean) =
         dao.updateAttachmentDownloaded(id, isDownloaded)
 
-    override suspend fun storeInitialAttachments(attachments: Collection<Attachment>) =
-        dao.insertOrIgnoreAttachments(attachments.map { AttachmentEntity(it) })
+    @Transaction
+    override suspend fun storeInitialAttachments(attachments: Collection<Attachment>) {
+        val tools = db.toolsDao.getTools().mapTo(mutableSetOf()) { it.code }
+        dao.insertOrIgnoreAttachments(attachments.filter { it.toolCode in tools }.map { AttachmentEntity(it) })
+    }
 
     // region Sync Methods
     @Transaction

library/db/src/test/kotlin/org/cru/godtools/db/repository/AttachmentsRepositoryIT.kt

@@ -1,13 +1,15 @@
 package org.cru.godtools.db.repository
 
 import app.cash.turbine.test
+import kotlin.test.BeforeTest
 import kotlin.test.Test
 import kotlin.test.assertEquals
 import kotlin.test.assertFalse
 import kotlin.test.assertNotNull
 import kotlin.test.assertNull
 import kotlin.test.assertTrue
 import kotlinx.coroutines.ExperimentalCoroutinesApi
+import kotlinx.coroutines.runBlocking
 import kotlinx.coroutines.test.TestScope
 import kotlinx.coroutines.test.runCurrent
 import kotlinx.coroutines.test.runTest
@@ -20,11 +22,17 @@ abstract class AttachmentsRepositoryIT {
     abstract val repository: AttachmentsRepository
     abstract val toolsRepository: ToolsRepository
 
+    private val tool = randomTool("tool")
+    private val tool2 = randomTool("tool2")
+
+    @BeforeTest
+    fun createTools() = runBlocking { toolsRepository.storeToolsFromSync(setOf(tool, tool2)) }
+
     @Test
     fun `findAttachment()`() = testScope.runTest {
         assertNull(repository.findAttachment(1))
 
-        val attachment = Attachment().apply {
+        val attachment = Attachment(tool = tool).apply {
             id = 1
             filename = "test.ext"
         }
@@ -34,7 +42,7 @@ abstract class AttachmentsRepositoryIT {
 
     @Test
     fun `findAttachmentFlow()`() = testScope.runTest {
-        val attachment = Attachment().apply {
+        val attachment = Attachment(tool = tool).apply {
             id = 1
             filename = "test.ext"
         }
@@ -51,12 +59,12 @@ abstract class AttachmentsRepositoryIT {
     fun `getAttachments()`() = testScope.runTest {
         repository.storeInitialAttachments(
             listOf(
-                Attachment().apply {
+                Attachment(tool = tool).apply {
                     id = 1
                     filename = "name1.bin"
                     isDownloaded = true
                 },
-                Attachment().apply {
+                Attachment(tool = tool).apply {
                     id = 2
                     filename = "name2.bin"
                 }
@@ -80,7 +88,7 @@ abstract class AttachmentsRepositoryIT {
             runCurrent()
             expectMostRecentItem()
 
-            val attachment = Attachment().apply {
+            val attachment = Attachment(tool = tool).apply {
                 id = 1
                 filename = "name1.bin"
                 isDownloaded = false
@@ -100,7 +108,7 @@ abstract class AttachmentsRepositoryIT {
     fun `updateAttachmentDownloaded()`() = testScope.runTest {
         repository.storeInitialAttachments(
             listOf(
-                Attachment().apply {
+                Attachment(tool = tool).apply {
                     id = 1
                     isDownloaded = false
                 }
@@ -121,8 +129,6 @@ abstract class AttachmentsRepositoryIT {
     // region storeInitialAttachments()
     @Test
     fun `storeInitialAttachments() - Don't replace already existing attachments`() = testScope.runTest {
-        val tool = randomTool("tool")
-        toolsRepository.storeToolsFromSync(setOf(tool))
         val attachment = Attachment(tool = tool) { filename = "sync.bin" }
         repository.storeAttachmentsFromSync(tool, listOf(attachment))
 
@@ -132,13 +138,20 @@ abstract class AttachmentsRepositoryIT {
             assertEquals("sync.bin", it.filename)
         }
     }
+
+    @Test
+    fun `storeInitialAttachments() - Ignore attachments for tools that don't exist`() = testScope.runTest {
+        val missingTool = randomTool("missing")
+        val attachment = Attachment(tool = missingTool)
+
+        repository.storeInitialAttachments(listOf(attachment))
+        assertNull(repository.findAttachment(attachment.id))
+    }
     // endregion storeInitialAttachments()
 
     // region storeAttachmentsFromSync()
     @Test
     fun `storeAttachmentsFromSync() - Update existing attachment`() = testScope.runTest {
-        val tool = randomTool("tool")
-        toolsRepository.storeToolsFromSync(setOf(tool))
         val attachment = Attachment(tool = tool) {
             filename = "initial.ext"
             sha256 = "initial"
@@ -161,8 +174,6 @@ abstract class AttachmentsRepositoryIT {
 
     @Test
     fun `storeAttachmentsFromSync() - Don't overwrite downloaded flag`() = testScope.runTest {
-        val tool = randomTool("tool")
-        toolsRepository.storeToolsFromSync(setOf(tool))
         val attachment = Attachment(tool = tool) {
             filename = "file.ext"
             sha256 = "file"
@@ -178,16 +189,14 @@ abstract class AttachmentsRepositoryIT {
 
     @Test
     fun `storeAttachmentsFromSync() - remove stale attachments`() = testScope.runTest {
-        val tool1 = randomTool("tool1")
-        val tool2 = randomTool("tool2")
-        val attachment = Attachment(tool = tool1)
-        val attachmentStale = Attachment(tool = tool1)
-        val attachmentNew = Attachment(tool = tool1)
+        val attachment = Attachment(tool = tool)
+        val attachmentStale = Attachment(tool = tool)
+        val attachmentNew = Attachment(tool = tool)
         val attachmentOtherTool = Attachment(tool = tool2)
-        toolsRepository.storeToolsFromSync(listOf(tool1, tool2))
+        toolsRepository.storeToolsFromSync(listOf(tool, tool2))
         repository.storeInitialAttachments(listOf(attachment, attachmentStale, attachmentOtherTool))
 
-        repository.storeAttachmentsFromSync(tool1, listOf(attachment, attachmentNew))
+        repository.storeAttachmentsFromSync(tool, listOf(attachment, attachmentNew))
         assertNotNull(repository.getAttachments()) {
             assertEquals(3, it.size)
             assertEquals(setOf(attachment.id, attachmentNew.id, attachmentOtherTool.id), it.map { it.id }.toSet())
@@ -198,16 +207,13 @@ abstract class AttachmentsRepositoryIT {
     // region deleteAttachmentsFor()
     @Test
     fun `deleteAttachmentsFor()`() = testScope.runTest {
-        val tool1 = randomTool("tool1")
-        val tool2 = randomTool("tool2")
-        toolsRepository.storeToolsFromSync(setOf(tool1, tool2))
         val attachments = listOf(
-            Attachment(tool = tool1),
+            Attachment(tool = tool),
             Attachment(tool = tool2)
         )
         repository.storeInitialAttachments(attachments)
 
-        repository.deleteAttachmentsFor(tool1)
+        repository.deleteAttachmentsFor(tool)
         assertNotNull(repository.getAttachments()) {
             assertEquals(1, it.size)
             assertEquals(attachments[1].id, it[0].id)

library/initial-content/src/main/kotlin/org/cru/godtools/init/content/task/Tasks.kt

@@ -89,7 +89,10 @@ internal class Tasks @Inject constructor(
     }
 
     suspend fun loadBundledAttachments(data: BundledData) {
-        attachmentsRepository.storeInitialAttachments(data.getTools().flatMap { it.attachments.orEmpty() })
+        // short-circuit if we already have any attachments loaded
+        if (attachmentsRepository.getAttachments().isNotEmpty()) return
+
+        attachmentsRepository.storeInitialAttachments(data.getTools().flatMap { it.apiAttachments.orEmpty() })
     }
 
     suspend fun loadBundledTranslations(data: BundledData) {

library/initial-content/src/test/kotlin/org/cru/godtools/init/content/task/TasksTest.kt

@@ -10,17 +10,21 @@ import io.mockk.confirmVerified
 import io.mockk.every
 import io.mockk.just
 import io.mockk.mockk
+import io.mockk.slot
 import java.io.ByteArrayInputStream
 import java.util.Locale
 import kotlin.test.Test
+import kotlin.test.assertEquals
 import kotlinx.coroutines.test.runTest
 import org.ccci.gto.android.common.jsonapi.JsonApiConverter
 import org.ccci.gto.android.common.jsonapi.model.JsonApiObject
 import org.cru.godtools.base.Settings
+import org.cru.godtools.db.repository.AttachmentsRepository
 import org.cru.godtools.db.repository.LastSyncTimeRepository
 import org.cru.godtools.db.repository.ToolsRepository
 import org.cru.godtools.db.repository.TranslationsRepository
 import org.cru.godtools.downloadmanager.GodToolsDownloadManager
+import org.cru.godtools.model.Attachment
 import org.cru.godtools.model.Tool
 import org.cru.godtools.model.randomTool
 import org.cru.godtools.model.randomTranslation
@@ -40,14 +44,17 @@ class TasksTest {
     private val settings: Settings = mockk {
         every { appLanguage } returns Locale("x")
     }
+    private val attachmentsRepository: AttachmentsRepository = mockk {
+        coEvery { getAttachments() } returns emptyList()
+    }
     private val toolsRepository: ToolsRepository = mockk(relaxUnitFun = true) {
         coEvery { getNormalTools() } returns emptyList()
     }
     private val translationsRepository: TranslationsRepository = mockk()
 
     private val tasks = Tasks(
         context,
-        mockk(),
+        attachmentsRepository = attachmentsRepository,
         downloadManager,
         jsonApiConverter,
         languagesRepository = mockk(),
@@ -101,4 +108,33 @@ class TasksTest {
         confirmVerified(toolsRepository)
     }
     // endregion initFavoriteTools()
+
+    // region loadBundledAttachments()
+    private val storedAttachments = slot<Collection<Attachment>>()
+
+    @Test
+    fun `loadBundledAttachments()`() = runTest {
+        val tools = Array(5) {
+            randomTool(type = Tool.Type.TRACT, apiId = it.toLong(), apiAttachments = List(5) { Attachment() })
+        }
+        every { jsonApiConverter.fromJson(any(), Tool::class.java) } returns JsonApiObject.of(*tools)
+        coEvery { attachmentsRepository.storeInitialAttachments(capture(storedAttachments)) } just Runs
+
+        tasks.loadBundledAttachments(tasks.bundledData())
+        coVerifyAll {
+            attachmentsRepository.getAttachments()
+            attachmentsRepository.storeInitialAttachments(any())
+        }
+        assertEquals(25, storedAttachments.captured.size)
+    }
+
+    @Test
+    fun `loadBundledAttachments() - Already Ran - Has attachments`() = runTest {
+        val attachments = List(5) { Attachment() }
+        coEvery { attachmentsRepository.getAttachments() } returns attachments
+
+        tasks.loadBundledAttachments(tasks.bundledData())
+        coVerifyAll { attachmentsRepository.getAttachments() }
+    }
+    // endregion loadBundledAttachments()
 }

library/model/src/main/kotlin/org/cru/godtools/model/Tool.kt

@@ -79,6 +79,8 @@ class Tool(
     defaultVariantCode: String? = null,
     @JsonApiId
     val apiId: Long? = null,
+    @JsonApiAttribute(JSON_ATTACHMENTS)
+    val apiAttachments: List<Attachment>? = null,
     @JsonApiAttribute(JSON_LATEST_TRANSLATIONS)
     val translations: List<Translation>? = null,
     @JsonApiIgnore
@@ -171,8 +173,6 @@ class Tool(
 
     @JsonApiAttribute(JSON_INITIAL_FAVORITES_PRIORITY)
     val initialFavoritesPriority: Int? = null
-    @JsonApiAttribute(JSON_ATTACHMENTS)
-    val attachments: List<Attachment>? = null
 
     @Suppress("SENSELESS_COMPARISON")
     val isValid
@@ -266,6 +266,7 @@ fun randomTool(
     metatoolCode: String? = UUID.randomUUID().toString().takeIf { Random.nextBoolean() },
     defaultVariantCode: String? = UUID.randomUUID().toString().takeIf { Random.nextBoolean() },
     apiId: Long? = Random.nextLong().takeIf { Random.nextBoolean() },
+    apiAttachments: List<Attachment>? = null,
     changedFieldsStr: String = setOf(ATTR_IS_FAVORITE).filter { Random.nextBoolean() }.joinToString(","),
 ) = Tool(
     code = code,
@@ -289,5 +290,6 @@ fun randomTool(
     metatoolCode = metatoolCode,
     defaultVariantCode = defaultVariantCode,
     apiId = apiId,
+    apiAttachments = apiAttachments,
     changedFieldsStr = changedFieldsStr,
 )

library/model/src/test/kotlin/org/cru/godtools/model/ToolTest.kt

@@ -39,7 +39,7 @@ class ToolTest {
         assertEquals(Tool.DEFAULT_DEFAULT_LOCALE, tool.defaultLocale)
         assertEquals(10, tool.defaultOrder)
         assertEquals(1L, tool.apiId)
-        assertThat(tool.attachments, hasSize(3))
+        assertThat(tool.apiAttachments, hasSize(3))
         assertThat(tool.translations, hasSize(2))
     }
 

library/sync/src/main/kotlin/org/cru/godtools/sync/repository/SyncRepository.kt

@@ -64,7 +64,7 @@ internal class SyncRepository @Inject constructor(
             }
         }
         if (includes.include(Tool.JSON_ATTACHMENTS)) {
-            tool.attachments?.let { launch { attachmentsRepository.storeAttachmentsFromSync(tool, it) } }
+            tool.apiAttachments?.let { launch { attachmentsRepository.storeAttachmentsFromSync(tool, it) } }
         }
 
         val metatool = tool.metatool