fix: do not throw NotImplemented error for existing "legacy database"…

…file if the file is not a sqlite file (#147) * fix: do not throw NotImplemented error for existing "legacy database" file if the file is not a sqlite file * chore: add logging if a legacy database file exists but it is not a valid sqlite file for some reason.
amplitude · Sep 13, 2023 · 3e0b851 · 3e0b851
1 parent 4651c31
commit 3e0b851
Show file tree

Hide file tree

Showing 4 changed files with 37 additions and 18 deletions.
diff --git a/android/src/main/java/com/amplitude/android/migration/DatabaseStorage.kt b/android/src/main/java/com/amplitude/android/migration/DatabaseStorage.kt
@@ -5,6 +5,7 @@ import android.database.Cursor
 import android.database.sqlite.SQLiteDatabase
 import android.database.sqlite.SQLiteException
 import android.database.sqlite.SQLiteOpenHelper
+import com.amplitude.common.Logger
 import com.amplitude.common.android.LogcatLogger
 import com.amplitude.core.Amplitude
 import com.amplitude.core.Configuration
@@ -39,18 +40,25 @@ object DatabaseConstants {
  * The SDK doesn't need to write/read from local sqlite database.
  * This storage class is used for migrating events only.
  */
-class DatabaseStorage(context: Context, databaseName: String) : SQLiteOpenHelper(
+class DatabaseStorage(
+    context: Context,
+    databaseName: String,
+    private val logger: Logger
+) : SQLiteOpenHelper(
     context,
     databaseName,
     null,
     DatabaseConstants.DATABASE_VERSION
 ) {
     private var file: File = context.getDatabasePath(databaseName)
+    private var isValidDatabaseFile = true
     var currentDbVersion: Int = DatabaseConstants.DATABASE_VERSION
         private set
 
     override fun onCreate(db: SQLiteDatabase) {
-        throw NotImplementedError()
+        // File exists but it is not a legacy database for some reason.
+        this.isValidDatabaseFile = false
+        logger.error("Attempt to re-create existing legacy database file ${file.absolutePath}")
     }
 
     override fun onUpgrade(db: SQLiteDatabase?, oldVersion: Int, newVersion: Int) {
@@ -121,6 +129,10 @@ class DatabaseStorage(context: Context, databaseName: String) : SQLiteOpenHelper
         var cursor: Cursor? = null
         try {
             val db = readableDatabase
+            if (!isValidDatabaseFile) {
+                return arrayListOf()
+            }
+
             cursor = queryDb(
                 db,
                 table,
@@ -220,6 +232,10 @@ class DatabaseStorage(context: Context, databaseName: String) : SQLiteOpenHelper
         var cursor: Cursor? = null
         try {
             val db = readableDatabase
+            if (!isValidDatabaseFile) {
+                return null
+            }
+
             cursor = queryDb(
                 db,
                 table,
@@ -304,7 +320,7 @@ object DatabaseStorageProvider {
         val databaseName = getDatabaseName(configuration.instanceName)
         var storage = instances[databaseName]
         if (storage == null) {
-            storage = DatabaseStorage(configuration.context, databaseName)
+            storage = DatabaseStorage(configuration.context, databaseName, configuration.loggerProvider.getLogger(amplitude))
             instances[databaseName] = storage
         }
 

diff --git a/android/src/test/java/com/amplitude/android/migration/DatabaseStorageTest.kt b/android/src/test/java/com/amplitude/android/migration/DatabaseStorageTest.kt
@@ -3,6 +3,7 @@ package com.amplitude.android.migration
 import android.content.Context
 import android.database.Cursor
 import android.database.sqlite.SQLiteDatabase
+import com.amplitude.common.jvm.ConsoleLogger
 import io.mockk.every
 import io.mockk.mockk
 import io.mockk.mockkConstructor
@@ -29,17 +30,10 @@ class DatabaseStorageTest {
     @BeforeEach
     fun setUp() {
         context = mockk(relaxed = true)
-        databaseStorage = DatabaseStorage(context!!, "")
+        databaseStorage = DatabaseStorage(context!!, "", ConsoleLogger())
         db = mockk()
     }
 
-    @Test
-    fun databaseStorage_onCreate_throwsNotImplementedError() {
-        Assertions.assertThrows(NotImplementedError::class.java) {
-            databaseStorage?.onCreate(db!!)
-        }
-    }
-
     @Test
     fun databaseStorage_onUpgrade_shouldSaveCurrentDbVersion() {
         Assertions.assertEquals(DatabaseConstants.DATABASE_VERSION, databaseStorage?.currentDbVersion)
@@ -64,7 +58,7 @@ class DatabaseStorageTest {
         every { anyConstructed<JSONObject>().put(any(), any<Long>()) } returns JSONObject()
         every { anyConstructed<JSONObject>().get("event_id") } returnsMany listOf(1, 2)
 
-        val mockedDatabaseStorage = spyk(DatabaseStorage(context!!, ""), recordPrivateCalls = true)
+        val mockedDatabaseStorage = spyk(DatabaseStorage(context!!, "", ConsoleLogger()), recordPrivateCalls = true)
         every {
             mockedDatabaseStorage["queryDb"](
                 any<SQLiteDatabase>(),
@@ -86,7 +80,7 @@ class DatabaseStorageTest {
 
     @Test
     fun databaseStorage_removeEvent_returnsEventsContent() {
-        val mockedDatabaseStorage = spyk(DatabaseStorage(context!!, ""), recordPrivateCalls = true)
+        val mockedDatabaseStorage = spyk(DatabaseStorage(context!!, "", ConsoleLogger()), recordPrivateCalls = true)
         every { mockedDatabaseStorage.close() } answers { nothing }
         every { mockedDatabaseStorage.writableDatabase } returns db
         every { db!!.delete(any(), any(), arrayOf("2")) } returns 2

diff --git a/android/src/test/java/com/amplitude/android/migration/RemnantDataMigrationTest.kt b/android/src/test/java/com/amplitude/android/migration/RemnantDataMigrationTest.kt
@@ -5,6 +5,7 @@ import androidx.test.core.app.ApplicationProvider
 import com.amplitude.android.Amplitude
 import com.amplitude.android.Configuration
 import com.amplitude.core.Storage
+import com.amplitude.core.utilities.ConsoleLoggerProvider
 import kotlinx.coroutines.runBlocking
 import org.json.JSONArray
 import org.junit.Test
@@ -37,6 +38,11 @@ class RemnantDataMigrationTest {
         checkLegacyDataMigration("legacy_v4.sqlite", 4, false)
     }
 
+    @Test
+    fun `not database file should not fail`() {
+        checkLegacyDataMigration("not_db_file", 0)
+    }
+
     private fun checkLegacyDataMigration(legacyDbName: String, dbVersion: Int, migrateLegacyData: Boolean = true) {
         val context = ApplicationProvider.getApplicationContext<Context>()
 
@@ -46,13 +52,15 @@ class RemnantDataMigrationTest {
         if (inputStream != null) {
             copyStream(inputStream, dbPath)
         }
+        val isValidDbFile = inputStream != null && legacyDbName != "not_db_file"
 
         val amplitude = Amplitude(
             Configuration(
                 "test-api-key",
                 context,
                 instanceName = instanceName,
                 migrateLegacyData = migrateLegacyData,
+                loggerProvider = ConsoleLoggerProvider()
             )
         )
 
@@ -63,7 +71,7 @@ class RemnantDataMigrationTest {
             amplitude.isBuilt.await()
 
             val identity = amplitude.idContainer.identityManager.getIdentity()
-            if (inputStream != null && migrateLegacyData) {
+            if (isValidDbFile && migrateLegacyData) {
                 Assertions.assertEquals(deviceId, identity.deviceId)
                 Assertions.assertEquals(userId, identity.userId)
             } else {
@@ -74,7 +82,7 @@ class RemnantDataMigrationTest {
             amplitude.storage.rollover()
             amplitude.identifyInterceptStorage.rollover()
 
-            if (inputStream != null && migrateLegacyData) {
+            if (isValidDbFile && migrateLegacyData) {
                 Assertions.assertEquals(1684219150343, amplitude.storage.read(Storage.Constants.PREVIOUS_SESSION_ID)?.toLongOrNull())
                 Assertions.assertEquals(1684219150344, amplitude.storage.read(Storage.Constants.LAST_EVENT_TIME)?.toLongOrNull())
                 Assertions.assertEquals(2, amplitude.storage.read(Storage.Constants.LAST_EVENT_ID)?.toLongOrNull())
@@ -85,7 +93,7 @@ class RemnantDataMigrationTest {
             }
 
             val eventsData = amplitude.storage.readEventsContent()
-            if (inputStream != null && migrateLegacyData) {
+            if (isValidDbFile && migrateLegacyData) {
                 val jsonEvents = JSONArray()
                 for (eventsPath in eventsData) {
                     val eventsString = amplitude.storage.getEventsString(eventsPath)
@@ -128,7 +136,7 @@ class RemnantDataMigrationTest {
             }
 
             val interceptedIdentifiesData = amplitude.identifyInterceptStorage.readEventsContent()
-            if (inputStream != null && dbVersion >= 4 && migrateLegacyData) {
+            if (isValidDbFile && dbVersion >= 4 && migrateLegacyData) {
                 val jsonInterceptedIdentifies = JSONArray()
                 for (eventsPath in interceptedIdentifiesData) {
                     val eventsString = amplitude.storage.getEventsString(eventsPath)
@@ -170,7 +178,7 @@ class RemnantDataMigrationTest {
         }
 
         // User/device id should not be cleaned.
-        if (inputStream != null) {
+        if (isValidDbFile) {
             Assertions.assertEquals(deviceId, databaseStorage.getValue(RemnantDataMigration.DEVICE_ID_KEY))
             Assertions.assertEquals(userId, databaseStorage.getValue(RemnantDataMigration.USER_ID_KEY))
         } else {

diff --git a/android/src/test/resources/not_db_file b/android/src/test/resources/not_db_file
@@ -0,0 +1 @@
+It is not a sqlite database.