From 39505452deda442d0b59d8a4224d0132e77995f1 Mon Sep 17 00:00:00 2001 From: olly Date: Tue, 29 Jan 2019 11:01:26 +0000 Subject: [PATCH] Make VersionTable static The way it is currently, it's very unclear that an operation on the version table will correctly belong to a transaction in code such as this, taken from DefaultDownloadIndex: writableDatabase.beginTransaction(); try { writableDatabase.execSQL(...); versionTable.setVersion(...); writableDatabase.setTransactionSuccessful(); } finally { writableDatabase.endTransaction(); } This change explicitly passes the database, to make it obvious that the operation will really go into the same transaction: writableDatabase.beginTransaction(); try { writableDatabase.execSQL(....); VersionTable.setVersion(writableDatabase, ...); writableDatabase.setTransactionSuccessful(); } finally { writableDatabase.endTransaction(); } PiperOrigin-RevId: 231374933 --- .../exoplayer2/database/VersionTable.java | 53 +++++++++---------- .../offline/DefaultDownloadIndex.java | 7 +-- .../exoplayer2/database/VersionTableTest.java | 28 +++++----- .../offline/DefaultDownloadIndexTest.java | 13 ++--- 4 files changed, 52 insertions(+), 49 deletions(-) diff --git a/library/core/src/main/java/com/google/android/exoplayer2/database/VersionTable.java b/library/core/src/main/java/com/google/android/exoplayer2/database/VersionTable.java index 0b6ef3d816..5193d1836f 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/database/VersionTable.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/database/VersionTable.java @@ -20,17 +20,18 @@ import android.database.Cursor; import android.database.DatabaseUtils; import android.database.sqlite.SQLiteDatabase; import android.support.annotation.IntDef; +import android.support.annotation.VisibleForTesting; import java.lang.annotation.Documented; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; /** - * A table that holds version information about other ExoPlayer tables. This allows ExoPlayer tables - * to be versioned independently to the version of the containing database. + * Utility methods for accessing versions of ExoPlayer database components. This allows them to be + * versioned independently to the version of the containing database. */ public final class VersionTable { - /** Returned by {@link #getVersion(int)} if the version is unset. */ + /** Returned by {@link #getVersion(SQLiteDatabase, int)} if the version is unset. */ public static final int VERSION_UNSET = -1; /** Version of tables used for offline functionality. */ public static final int FEATURE_OFFLINE = 0; @@ -56,48 +57,46 @@ public final class VersionTable { @IntDef({FEATURE_OFFLINE, FEATURE_CACHE}) private @interface Feature {} - private final DatabaseProvider databaseProvider; - - public VersionTable(DatabaseProvider databaseProvider) { - this.databaseProvider = databaseProvider; - // Check whether the table exists to avoid getting a writable database if we don't need one. - if (!doesTableExist(databaseProvider, TABLE_NAME)) { - databaseProvider.getWritableDatabase().execSQL(SQL_CREATE_TABLE_IF_NOT_EXISTS); - } - } + private VersionTable() {} /** * Sets the version of tables belonging to the specified feature. * + * @param writableDatabase The database to update. * @param feature The feature. * @param version The version. */ - public void setVersion(@Feature int feature, int version) { + public static void setVersion( + SQLiteDatabase writableDatabase, @Feature int feature, int version) { + writableDatabase.execSQL(SQL_CREATE_TABLE_IF_NOT_EXISTS); ContentValues values = new ContentValues(); values.put(COLUMN_FEATURE, feature); values.put(COLUMN_VERSION, version); - SQLiteDatabase writableDatabase = databaseProvider.getWritableDatabase(); writableDatabase.replace(TABLE_NAME, /* nullColumnHack= */ null, values); } /** * Returns the version of tables belonging to the specified feature, or {@link #VERSION_UNSET} if * no version information is available. + * + * @param database The database to query. + * @param feature The feature. */ - public int getVersion(@Feature int feature) { + public static int getVersion(SQLiteDatabase database, @Feature int feature) { + if (!tableExists(database, TABLE_NAME)) { + return VERSION_UNSET; + } String selection = COLUMN_FEATURE + " = ?"; String[] selectionArgs = {Integer.toString(feature)}; try (Cursor cursor = - databaseProvider - .getReadableDatabase() - .query( - TABLE_NAME, - new String[] {COLUMN_VERSION}, - selection, - selectionArgs, - /* groupBy= */ null, - /* having= */ null, - /* orderBy= */ null)) { + database.query( + TABLE_NAME, + new String[] {COLUMN_VERSION}, + selection, + selectionArgs, + /* groupBy= */ null, + /* having= */ null, + /* orderBy= */ null)) { if (cursor.getCount() == 0) { return VERSION_UNSET; } @@ -106,8 +105,8 @@ public final class VersionTable { } } - /* package */ static boolean doesTableExist(DatabaseProvider databaseProvider, String tableName) { - SQLiteDatabase readableDatabase = databaseProvider.getReadableDatabase(); + @VisibleForTesting + /* package */ static boolean tableExists(SQLiteDatabase readableDatabase, String tableName) { long count = DatabaseUtils.queryNumEntries( readableDatabase, "sqlite_master", "tbl_name = ?", new String[] {tableName}); diff --git a/library/core/src/main/java/com/google/android/exoplayer2/offline/DefaultDownloadIndex.java b/library/core/src/main/java/com/google/android/exoplayer2/offline/DefaultDownloadIndex.java index 71ee76d0f7..b9fae6e2db 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/offline/DefaultDownloadIndex.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/offline/DefaultDownloadIndex.java @@ -215,15 +215,16 @@ public final class DefaultDownloadIndex implements DownloadIndex { public DownloadsTable(DatabaseProvider databaseProvider) { this.databaseProvider = databaseProvider; - VersionTable versionTable = new VersionTable(databaseProvider); - int version = versionTable.getVersion(VersionTable.FEATURE_OFFLINE); + int version = + VersionTable.getVersion( + databaseProvider.getReadableDatabase(), VersionTable.FEATURE_OFFLINE); if (version == VersionTable.VERSION_UNSET || version > TABLE_VERSION) { SQLiteDatabase writableDatabase = databaseProvider.getWritableDatabase(); writableDatabase.beginTransaction(); try { + VersionTable.setVersion(writableDatabase, VersionTable.FEATURE_OFFLINE, TABLE_VERSION); writableDatabase.execSQL(SQL_DROP_TABLE_IF_EXISTS); writableDatabase.execSQL(SQL_CREATE_TABLE); - versionTable.setVersion(VersionTable.FEATURE_OFFLINE, TABLE_VERSION); writableDatabase.setTransactionSuccessful(); } finally { writableDatabase.endTransaction(); diff --git a/library/core/src/test/java/com/google/android/exoplayer2/database/VersionTableTest.java b/library/core/src/test/java/com/google/android/exoplayer2/database/VersionTableTest.java index dd9184ccdd..a607cc01db 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/database/VersionTableTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/database/VersionTableTest.java @@ -19,6 +19,7 @@ import static com.google.android.exoplayer2.database.VersionTable.FEATURE_CACHE; import static com.google.android.exoplayer2.database.VersionTable.FEATURE_OFFLINE; import static com.google.common.truth.Truth.assertThat; +import android.database.sqlite.SQLiteDatabase; import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -31,10 +32,14 @@ import org.robolectric.RuntimeEnvironment; public class VersionTableTest { private ExoDatabaseProvider databaseProvider; + private SQLiteDatabase readableDatabase; + private SQLiteDatabase writableDatabase; @Before public void setUp() { databaseProvider = new ExoDatabaseProvider(RuntimeEnvironment.application); + readableDatabase = databaseProvider.getReadableDatabase(); + writableDatabase = databaseProvider.getWritableDatabase(); } @After @@ -44,35 +49,32 @@ public class VersionTableTest { @Test public void getVersion_nonExistingTable_returnsVersionUnset() { - VersionTable versionTable = new VersionTable(databaseProvider); - int version = versionTable.getVersion(FEATURE_OFFLINE); + int version = VersionTable.getVersion(readableDatabase, FEATURE_OFFLINE); assertThat(version).isEqualTo(VersionTable.VERSION_UNSET); } @Test public void getVersion_returnsSetVersion() { - VersionTable versionTable = new VersionTable(databaseProvider); + VersionTable.setVersion(writableDatabase, FEATURE_OFFLINE, 1); + assertThat(VersionTable.getVersion(readableDatabase, FEATURE_OFFLINE)).isEqualTo(1); - versionTable.setVersion(FEATURE_OFFLINE, 1); - assertThat(versionTable.getVersion(FEATURE_OFFLINE)).isEqualTo(1); + VersionTable.setVersion(writableDatabase, FEATURE_OFFLINE, 10); + assertThat(VersionTable.getVersion(readableDatabase, FEATURE_OFFLINE)).isEqualTo(10); - versionTable.setVersion(FEATURE_OFFLINE, 10); - assertThat(versionTable.getVersion(FEATURE_OFFLINE)).isEqualTo(10); - - versionTable.setVersion(FEATURE_CACHE, 5); - assertThat(versionTable.getVersion(FEATURE_CACHE)).isEqualTo(5); - assertThat(versionTable.getVersion(FEATURE_OFFLINE)).isEqualTo(10); + VersionTable.setVersion(writableDatabase, FEATURE_CACHE, 5); + assertThat(VersionTable.getVersion(readableDatabase, FEATURE_CACHE)).isEqualTo(5); + assertThat(VersionTable.getVersion(readableDatabase, FEATURE_OFFLINE)).isEqualTo(10); } @Test public void doesTableExist_nonExistingTable_returnsFalse() { - assertThat(VersionTable.doesTableExist(databaseProvider, "NonExistingTable")).isFalse(); + assertThat(VersionTable.tableExists(readableDatabase, "NonExistingTable")).isFalse(); } @Test public void doesTableExist_existingTable_returnsTrue() { String table = "TestTable"; databaseProvider.getWritableDatabase().execSQL("CREATE TABLE " + table + " (dummy INTEGER)"); - assertThat(VersionTable.doesTableExist(databaseProvider, table)).isTrue(); + assertThat(VersionTable.tableExists(readableDatabase, table)).isTrue(); } } diff --git a/library/core/src/test/java/com/google/android/exoplayer2/offline/DefaultDownloadIndexTest.java b/library/core/src/test/java/com/google/android/exoplayer2/offline/DefaultDownloadIndexTest.java index df68f7ad34..badbb58eff 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/offline/DefaultDownloadIndexTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/offline/DefaultDownloadIndexTest.java @@ -17,6 +17,7 @@ package com.google.android.exoplayer2.offline; import static com.google.common.truth.Truth.assertThat; +import android.database.sqlite.SQLiteDatabase; import android.net.Uri; import android.support.annotation.Nullable; import com.google.android.exoplayer2.C; @@ -180,13 +181,13 @@ public class DefaultDownloadIndexTest { @Test public void putDownloadState_setsVersion() { - VersionTable versionTable = new VersionTable(databaseProvider); - assertThat(versionTable.getVersion(VersionTable.FEATURE_OFFLINE)) + SQLiteDatabase readableDatabase = databaseProvider.getReadableDatabase(); + assertThat(VersionTable.getVersion(readableDatabase, VersionTable.FEATURE_OFFLINE)) .isEqualTo(VersionTable.VERSION_UNSET); downloadIndex.putDownloadState(new DownloadStateBuilder("id1").build()); - assertThat(versionTable.getVersion(VersionTable.FEATURE_OFFLINE)) + assertThat(VersionTable.getVersion(readableDatabase, VersionTable.FEATURE_OFFLINE)) .isEqualTo(DefaultDownloadIndex.TABLE_VERSION); } @@ -198,15 +199,15 @@ public class DefaultDownloadIndexTest { assertThat(cursor.getCount()).isEqualTo(1); cursor.close(); - VersionTable versionTable = new VersionTable(databaseProvider); - versionTable.setVersion(VersionTable.FEATURE_OFFLINE, Integer.MAX_VALUE); + SQLiteDatabase writableDatabase = databaseProvider.getWritableDatabase(); + VersionTable.setVersion(writableDatabase, VersionTable.FEATURE_OFFLINE, Integer.MAX_VALUE); downloadIndex = new DefaultDownloadIndex(databaseProvider); cursor = downloadIndex.getDownloadStates(); assertThat(cursor.getCount()).isEqualTo(0); cursor.close(); - assertThat(versionTable.getVersion(VersionTable.FEATURE_OFFLINE)) + assertThat(VersionTable.getVersion(writableDatabase, VersionTable.FEATURE_OFFLINE)) .isEqualTo(DefaultDownloadIndex.TABLE_VERSION); }