feat: refactor file storage to fix thread safety issues

amplitude · Aug 23, 2023 · 095cfdb · 095cfdb
1 parent 67af8e3
commit 095cfdb
Show file tree

Hide file tree

Showing 10 changed files with 618 additions and 206 deletions.
diff --git a/android/src/main/java/com/amplitude/android/migration/StorageKeyMigration.kt b/android/src/main/java/com/amplitude/android/migration/StorageKeyMigration.kt
@@ -4,7 +4,6 @@ import com.amplitude.android.utilities.AndroidStorage
 import com.amplitude.common.Logger
 import com.amplitude.core.Storage
 import java.io.File
-import java.util.UUID
 
 class StorageKeyMigration(
     private val source: AndroidStorage,
@@ -29,22 +28,22 @@ class StorageKeyMigration(
 
             for (sourceEventFilePath in sourceEventFiles) {
                 val sourceEventFile = File(sourceEventFilePath)
-                var destinationEventFile =
-                    File(sourceEventFilePath.replace(source.storageKey, destination.storageKey))
+                val destinationFilePath = sourceEventFilePath.replace(
+                    "/${source.storageKey}/",
+                    "/${destination.storageKey}/"
+                ).replace(
+                    source.eventsFile.id,
+                    destination.eventsFile.id,
+                )
+                val destinationEventFile = File(destinationFilePath)
                 if (destinationEventFile.exists()) {
-                    var fileExtension = destinationEventFile.extension
-                    if (fileExtension != "") {
-                        fileExtension = ".$fileExtension"
+                    logger.error("destination ${destinationEventFile.absoluteFile} already exists")
+                } else {
+                    try {
+                        sourceEventFile.renameTo(destinationEventFile)
+                    } catch (e: Exception) {
+                        logger.error("can't rename $sourceEventFile to $destinationEventFile: ${e.message}")
                     }
-                    destinationEventFile = File(
-                        destinationEventFile.parent,
-                        "${destinationEventFile.nameWithoutExtension}-${UUID.randomUUID()}$fileExtension"
-                    )
-                }
-                try {
-                    sourceEventFile.renameTo(destinationEventFile)
-                } catch (e: Exception) {
-                    logger.error("can't rename $sourceEventFile to $destinationEventFile: ${e.message}")
                 }
             }
         } catch (e: Exception) {

diff --git a/android/src/main/java/com/amplitude/android/utilities/AndroidStorage.kt b/android/src/main/java/com/amplitude/android/utilities/AndroidStorage.kt
@@ -13,7 +13,6 @@ import com.amplitude.core.platform.EventPipeline
 import com.amplitude.core.utilities.EventsFileManager
 import com.amplitude.core.utilities.EventsFileStorage
 import com.amplitude.core.utilities.FileResponseHandler
-import com.amplitude.core.utilities.JSONUtil
 import com.amplitude.core.utilities.ResponseHandler
 import kotlinx.coroutines.CoroutineDispatcher
 import kotlinx.coroutines.CoroutineScope
@@ -34,12 +33,12 @@ class AndroidStorage(
     internal val sharedPreferences: SharedPreferences =
         context.getSharedPreferences("${getPrefix()}-$storageKey", Context.MODE_PRIVATE)
     private val storageDirectory: File = context.getDir(getDir(), Context.MODE_PRIVATE)
-    private val eventsFile =
-        EventsFileManager(storageDirectory, storageKey, AndroidKVS(sharedPreferences))
+    internal val eventsFile =
+        EventsFileManager(storageDirectory, storageKey, logger)
     private val eventCallbacksMap = mutableMapOf<String, EventCallBack>()
 
     override suspend fun writeEvent(event: BaseEvent) {
-        eventsFile.storeEvent(JSONUtil.eventToString(event))
+        eventsFile.storeEvent(event)
         event.callback?.let { callback ->
             event.insertId?.let {
                 eventCallbacksMap.put(it, callback)
@@ -67,11 +66,7 @@ class AndroidStorage(
         return eventsFile.read()
     }
 
-    override fun releaseFile(filePath: String) {
-        eventsFile.release(filePath)
-    }
-
-    override suspend fun getEventsString(content: Any): String {
+    override fun getEventsString(content: Any): String {
         return eventsFile.getEventString(content as String)
     }
 

diff --git a/android/src/test/java/com/amplitude/android/migration/StorageKeyMigrationTest.kt b/android/src/test/java/com/amplitude/android/migration/StorageKeyMigrationTest.kt
@@ -145,50 +145,6 @@ class StorageKeyMigrationTest {
         Assertions.assertEquals(0, destinationEventFiles.size)
     }
 
-    @Test
-    fun `event files with duplicated names should be migrated`() {
-        val context = ApplicationProvider.getApplicationContext<Context>()
-        val logger = ConsoleLogger()
-
-        val source = AndroidStorage(context, UUID.randomUUID().toString(), logger, null)
-        val destination = AndroidStorage(context, UUID.randomUUID().toString(), logger, null)
-
-        runBlocking {
-            source.writeEvent(createEvent(1))
-            source.rollover()
-            source.writeEvent(createEvent(22))
-            source.rollover()
-        }
-
-        val sourceEventFiles = source.readEventsContent() as List<String>
-        Assertions.assertEquals(2, sourceEventFiles.size)
-
-        val sourceFileSizes = sourceEventFiles.map { File(it).length() }
-
-        runBlocking {
-            destination.writeEvent(createEvent(333))
-            destination.rollover()
-        }
-
-        var destinationEventFiles = destination.readEventsContent() as List<String>
-        Assertions.assertEquals(1, destinationEventFiles.size)
-
-        val destinationFileSizes = destinationEventFiles.map { File(it).length() }
-
-        val migration = StorageKeyMigration(source, destination, logger)
-        runBlocking {
-            migration.execute()
-        }
-
-        destinationEventFiles = destination.readEventsContent() as List<String>
-        Assertions.assertEquals("-0", destinationEventFiles[0].substring(destinationEventFiles[0].length - 2))
-        Assertions.assertTrue(destinationEventFiles[1].contains("-0-"))
-        Assertions.assertEquals("-1", destinationEventFiles[2].substring(destinationEventFiles[0].length - 2))
-        Assertions.assertEquals(destinationFileSizes[0], File(destinationEventFiles[0]).length())
-        Assertions.assertEquals(sourceFileSizes[0], File(destinationEventFiles[1]).length())
-        Assertions.assertEquals(sourceFileSizes[1], File(destinationEventFiles[2]).length())
-    }
-
     private fun createEvent(eventIndex: Int): BaseEvent {
         val event = BaseEvent()
         event.eventType = "event-$eventIndex"

diff --git a/core/src/main/java/com/amplitude/core/Storage.kt b/core/src/main/java/com/amplitude/core/Storage.kt
@@ -30,7 +30,7 @@ interface Storage {
 
     fun readEventsContent(): List<Any>
 
-    suspend fun getEventsString(content: Any): String
+    fun getEventsString(content: Any): String
 
     fun getResponseHandler(eventPipeline: EventPipeline, configuration: Configuration, scope: CoroutineScope, dispatcher: CoroutineDispatcher): ResponseHandler
 }