From 38d56c53aa4a08b100725e82abb08d3e0b364de6 Mon Sep 17 00:00:00 2001 From: Andrey Sokolov Date: Wed, 6 Sep 2023 21:41:47 +0400 Subject: [PATCH 1/2] fix: do not throw NotImplemented error for existing "legacy database" file if the file is not a sqlite file --- .../android/migration/DatabaseStorage.kt | 12 +++++++++++- .../android/migration/DatabaseStorageTest.kt | 7 ------- .../migration/RemnantDataMigrationTest.kt | 16 +++++++++++----- android/src/test/resources/not_db_file | 1 + 4 files changed, 23 insertions(+), 13 deletions(-) create mode 100644 android/src/test/resources/not_db_file diff --git a/android/src/main/java/com/amplitude/android/migration/DatabaseStorage.kt b/android/src/main/java/com/amplitude/android/migration/DatabaseStorage.kt index f3fb917a..4e575db2 100644 --- a/android/src/main/java/com/amplitude/android/migration/DatabaseStorage.kt +++ b/android/src/main/java/com/amplitude/android/migration/DatabaseStorage.kt @@ -46,11 +46,13 @@ class DatabaseStorage(context: Context, databaseName: String) : SQLiteOpenHelper 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 } override fun onUpgrade(db: SQLiteDatabase?, oldVersion: Int, newVersion: Int) { @@ -121,6 +123,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 +226,10 @@ class DatabaseStorage(context: Context, databaseName: String) : SQLiteOpenHelper var cursor: Cursor? = null try { val db = readableDatabase + if (!isValidDatabaseFile) { + return null + } + cursor = queryDb( db, table, diff --git a/android/src/test/java/com/amplitude/android/migration/DatabaseStorageTest.kt b/android/src/test/java/com/amplitude/android/migration/DatabaseStorageTest.kt index eff584ce..3638f482 100644 --- a/android/src/test/java/com/amplitude/android/migration/DatabaseStorageTest.kt +++ b/android/src/test/java/com/amplitude/android/migration/DatabaseStorageTest.kt @@ -33,13 +33,6 @@ class DatabaseStorageTest { 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) diff --git a/android/src/test/java/com/amplitude/android/migration/RemnantDataMigrationTest.kt b/android/src/test/java/com/amplitude/android/migration/RemnantDataMigrationTest.kt index ef7ca6b0..9bfaf201 100644 --- a/android/src/test/java/com/amplitude/android/migration/RemnantDataMigrationTest.kt +++ b/android/src/test/java/com/amplitude/android/migration/RemnantDataMigrationTest.kt @@ -37,6 +37,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() @@ -46,6 +51,7 @@ class RemnantDataMigrationTest { if (inputStream != null) { copyStream(inputStream, dbPath) } + val isValidDbFile = inputStream != null && legacyDbName != "not_db_file" val amplitude = Amplitude( Configuration( @@ -63,7 +69,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 +80,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 +91,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 +134,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 +176,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 new file mode 100644 index 00000000..286b9c9f --- /dev/null +++ b/android/src/test/resources/not_db_file @@ -0,0 +1 @@ +It is not a sqlite database. \ No newline at end of file From f9df70c17a96110e6895e70786faa86e6e2c4f14 Mon Sep 17 00:00:00 2001 From: Andrey Sokolov Date: Wed, 13 Sep 2023 19:07:17 +0400 Subject: [PATCH 2/2] chore: add logging if a legacy database file exists but it is not a valid sqlite file for some reason. --- .../com/amplitude/android/migration/DatabaseStorage.kt | 10 ++++++++-- .../amplitude/android/migration/DatabaseStorageTest.kt | 7 ++++--- .../android/migration/RemnantDataMigrationTest.kt | 2 ++ 3 files changed, 14 insertions(+), 5 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 index 4e575db2..a0dd66b8 100644 --- 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,7 +40,11 @@ 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, @@ -53,6 +58,7 @@ class DatabaseStorage(context: Context, databaseName: String) : SQLiteOpenHelper override fun onCreate(db: SQLiteDatabase) { // 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) { @@ -314,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 index 3638f482..5067eeed 100644 --- 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,7 +30,7 @@ class DatabaseStorageTest { @BeforeEach fun setUp() { context = mockk(relaxed = true) - databaseStorage = DatabaseStorage(context!!, "") + databaseStorage = DatabaseStorage(context!!, "", ConsoleLogger()) db = mockk() } @@ -57,7 +58,7 @@ class DatabaseStorageTest { every { anyConstructed().put(any(), any()) } returns JSONObject() every { anyConstructed().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(), @@ -79,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 index 9bfaf201..4d2b7d77 100644 --- 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 @@ -59,6 +60,7 @@ class RemnantDataMigrationTest { context, instanceName = instanceName, migrateLegacyData = migrateLegacyData, + loggerProvider = ConsoleLoggerProvider() ) )