Handling multiple open events for the same db
Handling multiple open events for the same database.
Deduplication logic:
- For file based databases verifying path
- For in-memory databases treating each new instance as a different db.
Keeping databases open indefinitely until we start tracking.
database-closed-events.
Bug: 143216096
Bug: 153695085
Test: ./gradlew :sqlite:sqlite-inspection:cC
Change-Id: Ie00bf98a5c12170f1d782d7915ad305ebe917108
diff --git a/sqlite/sqlite-inspection/src/androidTest/java/androidx/sqlite/inspection/test/DatabaseExtensions.kt b/sqlite/sqlite-inspection/src/androidTest/java/androidx/sqlite/inspection/test/DatabaseExtensions.kt
index 18bb086..3b3d06b 100644
--- a/sqlite/sqlite-inspection/src/androidTest/java/androidx/sqlite/inspection/test/DatabaseExtensions.kt
+++ b/sqlite/sqlite-inspection/src/androidTest/java/androidx/sqlite/inspection/test/DatabaseExtensions.kt
@@ -21,6 +21,7 @@
import androidx.test.core.app.ApplicationProvider
import com.google.common.truth.Truth.assertThat
import org.junit.rules.TemporaryFolder
+import java.io.File
fun SQLiteDatabase.addTable(table: Table) = execSQL(table.toCreateString())
@@ -34,7 +35,11 @@
}
fun Database.createInstance(temporaryFolder: TemporaryFolder): SQLiteDatabase {
- val path = temporaryFolder.newFile(this.name).absolutePath
+ val path = if (name == null) null else
+ File(temporaryFolder.root, name)
+ .also { it.createNewFile() } // can handle an existing file
+ .absolutePath
+
val context = ApplicationProvider.getApplicationContext() as android.content.Context
val openHelper = object : SQLiteOpenHelper(context, path, null, 1) {
override fun onCreate(db: SQLiteDatabase?) = Unit
diff --git a/sqlite/sqlite-inspection/src/androidTest/java/androidx/sqlite/inspection/test/Model.kt b/sqlite/sqlite-inspection/src/androidTest/java/androidx/sqlite/inspection/test/Model.kt
index b1b324a..fb46d79 100644
--- a/sqlite/sqlite-inspection/src/androidTest/java/androidx/sqlite/inspection/test/Model.kt
+++ b/sqlite/sqlite-inspection/src/androidTest/java/androidx/sqlite/inspection/test/Model.kt
@@ -16,8 +16,8 @@
package androidx.sqlite.inspection.test
-data class Database(val name: String, val tables: List<Table>) {
- constructor(name: String, vararg tables: Table) : this(name, tables.toList())
+data class Database(val name: String?, val tables: List<Table>) {
+ constructor(name: String?, vararg tables: Table) : this(name, tables.toList())
}
data class Table(val name: String, val columns: List<Column>) {
diff --git a/sqlite/sqlite-inspection/src/androidTest/java/androidx/sqlite/inspection/test/TrackDatabasesTest.kt b/sqlite/sqlite-inspection/src/androidTest/java/androidx/sqlite/inspection/test/TrackDatabasesTest.kt
index c3f783e..47da14a 100644
--- a/sqlite/sqlite-inspection/src/androidTest/java/androidx/sqlite/inspection/test/TrackDatabasesTest.kt
+++ b/sqlite/sqlite-inspection/src/androidTest/java/androidx/sqlite/inspection/test/TrackDatabasesTest.kt
@@ -18,6 +18,7 @@
import android.database.sqlite.SQLiteDatabase
import androidx.inspection.InspectorEnvironment.ExitHook
+import androidx.sqlite.inspection.SqliteInspectorProtocol.Event
import androidx.sqlite.inspection.test.MessageFactory.createTrackDatabasesCommand
import androidx.sqlite.inspection.test.MessageFactory.createTrackDatabasesResponse
import androidx.test.ext.junit.runners.AndroidJUnit4
@@ -71,7 +72,8 @@
// evaluate registered hooks
val hookEntries = testEnvironment.consumeRegisteredHooks()
- assertThat(hookEntries).hasSize(10)
+ .filter { it.originMethod == OPEN_DATABASE_COMMAND_SIGNATURE }
+ assertThat(hookEntries).hasSize(1)
hookEntries.first().let { entry ->
// expect one exit hook tracking database open events
assertThat(entry).isInstanceOf(Hook.ExitHook::class.java)
@@ -94,31 +96,96 @@
}
@Test
- fun test_track_databases_the_same_database_opened_twice() = runBlocking {
+ fun test_track_databases_the_same_database_opened_multiple_times() = runBlocking {
+ // given
testEnvironment.sendCommand(createTrackDatabasesCommand())
- val hooks = testEnvironment.consumeRegisteredHooks()
- assertThat(hooks).hasSize(10)
-
- val onOpenHook = hooks.first()
- assertThat(onOpenHook.originMethod).isEqualTo(OPEN_DATABASE_COMMAND_SIGNATURE)
- val database = Database("db").createInstance(temporaryFolder)
+ val onOpenHook = testEnvironment.consumeRegisteredHooks()
+ .first { it.originMethod == OPEN_DATABASE_COMMAND_SIGNATURE }
@Suppress("UNCHECKED_CAST")
- val onExit = (onOpenHook.asExitHook as ExitHook<SQLiteDatabase>)::onExit
+ val onOpen = (onOpenHook.asExitHook as ExitHook<SQLiteDatabase>)::onExit
- // open event on a database first time
- onExit(database)
- testEnvironment.receiveEvent()
- .let { event -> assertThat(event.hasDatabaseOpened()).isEqualTo(true) }
+ val seenDbIds = mutableSetOf<Int>()
- // open event on the same database for the second time
- // TODO: track close database events or handle the below gracefully
- onExit(database)
- testEnvironment.receiveEvent().let { event ->
- assertThat(event.hasErrorOccurred()).isEqualTo(true)
- val error = event.errorOccurred.content
- assertThat(error.message).contains("Database is already tracked")
- assertThat(error.message).contains(database.path)
- assertThat(error.recoverability.isRecoverable).isEqualTo(false)
+ fun checkDbOpenedEvent(event: Event, database: SQLiteDatabase) {
+ assertThat(event.hasDatabaseOpened()).isEqualTo(true)
+ val isNewId = seenDbIds.add(event.databaseOpened.databaseId)
+ assertThat(isNewId).isEqualTo(true)
+ assertThat(event.databaseOpened.name).isEqualTo(database.path)
}
+
+ // file based db: first open
+ val fileDbPath = "db1"
+ val fileDb = Database(fileDbPath).createInstance(temporaryFolder)
+ onOpen(fileDb)
+ checkDbOpenedEvent(testEnvironment.receiveEvent(), fileDb)
+
+ // file based db: same instance
+ onOpen(fileDb)
+ testEnvironment.assertNoQueuedEvents()
+
+ // file based db: same path
+ onOpen(Database(fileDbPath).createInstance(temporaryFolder))
+ testEnvironment.assertNoQueuedEvents()
+
+ // in-memory database: first open
+ val inMemDb = Database(null).createInstance(temporaryFolder)
+ onOpen(inMemDb)
+ checkDbOpenedEvent(testEnvironment.receiveEvent(), inMemDb)
+
+ // in-memory database: same instance
+ onOpen(inMemDb)
+ testEnvironment.assertNoQueuedEvents()
+
+ // in-memory database: new instances (same path = :memory:)
+ repeat(3) {
+ val db = Database(null).createInstance(temporaryFolder)
+ assertThat(db.path).isEqualTo(":memory:")
+ onOpen(db)
+ checkDbOpenedEvent(testEnvironment.receiveEvent(), db)
+ }
+ }
+
+ @Test
+ fun test_track_databases_keeps_db_open() = runBlocking {
+ // given
+ testEnvironment.sendCommand(createTrackDatabasesCommand())
+ val onOpenHook = testEnvironment.consumeRegisteredHooks()
+ .first { it.originMethod == OPEN_DATABASE_COMMAND_SIGNATURE }
+
+ @Suppress("UNCHECKED_CAST")
+ val onOpen = (onOpenHook.asExitHook as ExitHook<SQLiteDatabase>)::onExit
+
+ // without inspecting
+ val dbNoInspecting = Database("dbNoInspecting").createInstance(temporaryFolder)
+ dbNoInspecting.close()
+ assertThat(dbNoInspecting.isOpen).isFalse()
+
+ // with inspecting
+ listOf("db2", null).forEach { name ->
+ val dbWithInspecting = Database(name).createInstance(temporaryFolder)
+ onOpen(dbWithInspecting) // start inspecting
+ dbWithInspecting.close()
+ assertThat(dbWithInspecting.isOpen).isTrue()
+ }
+ }
+
+ @Test
+ fun test_temporary_databases_same_path_different_database() {
+ // given
+ val db1 = Database(null).createInstance(temporaryFolder)
+ val db2 = Database(null).createInstance(temporaryFolder)
+ fun queryTableCount(db: SQLiteDatabase): Long =
+ db.compileStatement("select count(*) from sqlite_master").simpleQueryForLong()
+ assertThat(queryTableCount(db1)).isEqualTo(1) // android_metadata sole table
+ assertThat(queryTableCount(db2)).isEqualTo(1) // android_metadata sole table
+ assertThat(db1.path).isEqualTo(db2.path)
+ assertThat(db1.path).isEqualTo(":memory:")
+
+ // when
+ db1.execSQL("create table t1 (c1 int)")
+
+ // then
+ assertThat(queryTableCount(db1)).isEqualTo(2)
+ assertThat(queryTableCount(db2)).isEqualTo(1)
}
}
diff --git a/sqlite/sqlite-inspection/src/main/java/androidx/sqlite/inspection/SqliteInspector.java b/sqlite/sqlite-inspection/src/main/java/androidx/sqlite/inspection/SqliteInspector.java
index 0034550..abf1e1b 100644
--- a/sqlite/sqlite-inspection/src/main/java/androidx/sqlite/inspection/SqliteInspector.java
+++ b/sqlite/sqlite-inspection/src/main/java/androidx/sqlite/inspection/SqliteInspector.java
@@ -66,6 +66,7 @@
import java.util.HashSet;
import java.util.List;
import java.util.Map;
+import java.util.Objects;
import java.util.Set;
import java.util.WeakHashMap;
import java.util.concurrent.Executor;
@@ -515,21 +516,15 @@
// avoiding a synthetic accessor
void onDatabaseAdded(SQLiteDatabase database) {
Event response;
- try {
- int id = mDatabaseRegistry.addDatabase(database);
- String name = database.getPath();
- response = createDatabaseOpenedEvent(id, name);
- mRoomInvalidationRegistry.invalidateCache();
- } catch (IllegalArgumentException exception) {
- String message = exception.getMessage();
- // TODO: clean up, e.g. replace Exception message check with a custom Exception class
- if (message != null && message.contains("Database is already tracked")) {
- response = createErrorOccurredEvent(exception, null);
- } else {
- throw exception;
- }
- }
+ int id = mDatabaseRegistry.addDatabase(database);
+ if (id == DatabaseRegistry.ALREADY_TRACKED) return; // Nothing to do
+ // TODO: replace with db open/closed tracking as this will keep the database open
+ database.acquireReference();
+
+ String name = database.getPath();
+ response = createDatabaseOpenedEvent(id, name);
+ mRoomInvalidationRegistry.invalidateCache();
getConnection().sendEvent(response.toByteArray());
}
@@ -551,11 +546,6 @@
.build();
}
- private Event createErrorOccurredEvent(@NonNull Exception exception, Boolean isRecoverable) {
- return createErrorOccurredEvent(exception.getMessage(), stackTraceFromException(exception),
- isRecoverable);
- }
-
private static ErrorContent createErrorContentMessage(@Nullable String message,
@Nullable String stackTrace, Boolean isRecoverable) {
ErrorContent.Builder builder = ErrorContent.newBuilder();
@@ -597,6 +587,9 @@
}
static class DatabaseRegistry {
+ private static final String IN_MEMORY_DB_PATH = ":memory:";
+ static final int ALREADY_TRACKED = -1;
+
private final Object mLock = new Object();
// starting from '1' to distinguish from '0' which could stand for an unset parameter
@@ -608,21 +601,24 @@
* Thread safe
*
* @return id used to track the database
- * @throws IllegalArgumentException if database is already in the registry
*/
int addDatabase(@NonNull SQLiteDatabase database) {
synchronized (mLock) {
- // TODO: decide if compare by path or object-reference; for now using reference
- // TODO: decide if the same database object here twice an Exception
// TODO: decide if to track database close events and update here
// TODO: decide if use weak-references to database objects
// TODO: consider database.acquireReference() approach
// check if already tracked
for (Map.Entry<Integer, SQLiteDatabase> entry : mDatabases.entrySet()) {
+ // Instance already tracked
if (entry.getValue() == database) {
- throw new IllegalArgumentException(
- "Database is already tracked: " + database.getPath());
+ return ALREADY_TRACKED;
+ }
+ // Path already tracked (and not an in-memory database)
+ final String path = database.getPath();
+ if (!Objects.equals(IN_MEMORY_DB_PATH, path)
+ && Objects.equals(path, entry.getValue().getPath())) {
+ return ALREADY_TRACKED;
}
}
