From 1678223cabed4da1dd3f155bc8569d5b2376ffba Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Sat, 24 Sep 2016 08:10:01 +1000 Subject: [PATCH] More robust fix for #763, specifying column names to copy explicitly. This is far less brittle at runtime, but slightly more work at dev time. The following things are undesirable but make it much easier to write: * Use of `CREATE_TABLE_APP.replaceFirst(...)` to create the temp tables. * Having to specify a list fo columns twice in `Schema` (`ALL_COLS` + `COLS`). The `replaceFirst` means we don't need to maintain two separate create table statements. It is a little messy because there is no compile time guarantee that we are creating a valid SQL statement at the end, just our knowledge that a create table statment tends to have the table name first and it probably wont cause problems. The `ALL_COLS` + `COLS` is required so that we don't have to type out a list of fields when copying data in `TempAppProvider`. Otherwise, whenever a new column is added, developers would need to know that it also needs to be added to this third place. Currently it is in the `Schema` and the `CREATE_TABLE_*` statements where one needs to add a new column. These are both intuitive and hopefully easily discoverable. Having to add it to the `TempAppProvider` is less intuitive and likely to result in bugs. --- .../java/org/fdroid/fdroid/data/DBHelper.java | 2 +- .../java/org/fdroid/fdroid/data/Schema.java | 32 +++++++++++++++++++ .../fdroid/fdroid/data/TempApkProvider.java | 3 +- .../fdroid/fdroid/data/TempAppProvider.java | 15 +++++++-- 4 files changed, 47 insertions(+), 5 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/data/DBHelper.java b/app/src/main/java/org/fdroid/fdroid/data/DBHelper.java index f0769f84e..1397c44b4 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/DBHelper.java +++ b/app/src/main/java/org/fdroid/fdroid/data/DBHelper.java @@ -45,7 +45,7 @@ class DBHelper extends SQLiteOpenHelper { + RepoTable.Cols.TIMESTAMP + " integer not null default 0" + ");"; - private static final String CREATE_TABLE_APK = + static final String CREATE_TABLE_APK = "CREATE TABLE " + ApkTable.NAME + " ( " + ApkTable.Cols.APP_ID + " integer not null, " + ApkTable.Cols.VERSION_NAME + " text not null, " diff --git a/app/src/main/java/org/fdroid/fdroid/data/Schema.java b/app/src/main/java/org/fdroid/fdroid/data/Schema.java index 07eb59d30..88987b1de 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/Schema.java +++ b/app/src/main/java/org/fdroid/fdroid/data/Schema.java @@ -76,6 +76,25 @@ public interface Schema { String SIGNATURE = "installedSig"; } + /** + * Each of the physical columns in the sqlite table. Differs from {@link Cols#ALL} in + * that it doesn't include fields which are aliases of other fields (e.g. {@link Cols#_ID} + * or which are from other related tables (e.g. {@link Cols.SuggestedApk#VERSION_NAME}). + */ + String[] ALL_COLS = { + ROW_ID, IS_COMPATIBLE, PACKAGE_NAME, NAME, SUMMARY, ICON, DESCRIPTION, + LICENSE, AUTHOR, EMAIL, WEB_URL, TRACKER_URL, SOURCE_URL, + CHANGELOG_URL, DONATE_URL, BITCOIN_ADDR, LITECOIN_ADDR, FLATTR_ID, + UPSTREAM_VERSION_NAME, UPSTREAM_VERSION_CODE, ADDED, LAST_UPDATED, + CATEGORIES, ANTI_FEATURES, REQUIREMENTS, ICON_URL, ICON_URL_LARGE, + SUGGESTED_VERSION_CODE, + }; + + /** + * Superset of {@link Cols#ALL_COLS} including fields from other tables and also an alias + * to satisfy the Android requirement for an "_ID" field. + * @see Cols#ALL_COLS + */ String[] ALL = { _ID, ROW_ID, IS_COMPATIBLE, PACKAGE_NAME, NAME, SUMMARY, ICON, DESCRIPTION, LICENSE, AUTHOR, EMAIL, WEB_URL, TRACKER_URL, SOURCE_URL, @@ -134,6 +153,19 @@ public interface Schema { String PACKAGE_NAME = "appPackageName"; } + /** + * @see AppMetadataTable.Cols#ALL_COLS + */ + String[] ALL_COLS = { + APP_ID, VERSION_NAME, REPO_ID, HASH, VERSION_CODE, NAME, + SIZE, SIGNATURE, SOURCE_NAME, MIN_SDK_VERSION, TARGET_SDK_VERSION, MAX_SDK_VERSION, + PERMISSIONS, FEATURES, NATIVE_CODE, HASH_TYPE, ADDED_DATE, + IS_COMPATIBLE, INCOMPATIBLE_REASONS, + }; + + /** + * @see AppMetadataTable.Cols#ALL + */ String[] ALL = { _ID, APP_ID, App.PACKAGE_NAME, VERSION_NAME, REPO_ID, HASH, VERSION_CODE, NAME, SIZE, SIGNATURE, SOURCE_NAME, MIN_SDK_VERSION, TARGET_SDK_VERSION, MAX_SDK_VERSION, diff --git a/app/src/main/java/org/fdroid/fdroid/data/TempApkProvider.java b/app/src/main/java/org/fdroid/fdroid/data/TempApkProvider.java index db9d03b65..8cc83f773 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/TempApkProvider.java +++ b/app/src/main/java/org/fdroid/fdroid/data/TempApkProvider.java @@ -125,7 +125,8 @@ public class TempApkProvider extends ApkProvider { private void initTable() { final SQLiteDatabase db = db(); final String memoryDbName = TempAppProvider.DB; - db.execSQL("CREATE TABLE " + memoryDbName + "." + getTableName() + " AS SELECT * FROM main." + ApkTable.NAME); + db.execSQL(DBHelper.CREATE_TABLE_APK.replaceFirst(Schema.ApkTable.NAME, memoryDbName + "." + getTableName())); + db.execSQL(TempAppProvider.copyData(Schema.ApkTable.Cols.ALL_COLS, Schema.ApkTable.NAME, memoryDbName + "." + getTableName())); db.execSQL("CREATE INDEX IF NOT EXISTS " + memoryDbName + ".apk_vercode on " + getTableName() + " (" + ApkTable.Cols.VERSION_CODE + ");"); db.execSQL("CREATE INDEX IF NOT EXISTS " + memoryDbName + ".apk_compatible ON " + getTableName() + " (" + ApkTable.Cols.IS_COMPATIBLE + ");"); } diff --git a/app/src/main/java/org/fdroid/fdroid/data/TempAppProvider.java b/app/src/main/java/org/fdroid/fdroid/data/TempAppProvider.java index af7aa91f1..18520e286 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/TempAppProvider.java +++ b/app/src/main/java/org/fdroid/fdroid/data/TempAppProvider.java @@ -162,12 +162,21 @@ public class TempAppProvider extends AppProvider { ensureTempTableDetached(db); db.execSQL("ATTACH DATABASE ':memory:' AS " + DB); db.execSQL(DBHelper.CREATE_TABLE_APP.replaceFirst(AppMetadataTable.NAME, DB + "." + getTableName())); - db.execSQL("INSERT INTO " + DB + "." + getTableName() + " SELECT * FROM " + AppMetadataTable.NAME); + db.execSQL(copyData(AppMetadataTable.Cols.ALL_COLS, AppMetadataTable.NAME, DB + "." + getTableName())); db.execSQL("CREATE INDEX IF NOT EXISTS " + DB + ".app_id ON " + getTableName() + " (" + AppMetadataTable.Cols.PACKAGE_NAME + ");"); db.execSQL("CREATE INDEX IF NOT EXISTS " + DB + ".app_upstreamVercode ON " + getTableName() + " (" + AppMetadataTable.Cols.UPSTREAM_VERSION_CODE + ");"); db.execSQL("CREATE INDEX IF NOT EXISTS " + DB + ".app_compatible ON " + getTableName() + " (" + AppMetadataTable.Cols.IS_COMPATIBLE + ");"); } + /** + * Constructs an INSERT INTO ... SELECT statement as a means from getting data from one table + * into another. The list of columns to copy are explicitly specified using colsToCopy. + */ + static String copyData(String[] colsToCopy, String fromTable, String toTable) { + String cols = TextUtils.join(", ", colsToCopy); + return "INSERT INTO " + toTable + " (" + cols + ") SELECT " + cols + " FROM " + fromTable; + } + private void commitTable() { final SQLiteDatabase db = db(); try { @@ -177,10 +186,10 @@ public class TempAppProvider extends AppProvider { final String tempApk = DB + "." + TempApkProvider.TABLE_TEMP_APK; db.execSQL("DELETE FROM " + AppMetadataTable.NAME + " WHERE 1"); - db.execSQL("INSERT INTO " + AppMetadataTable.NAME + " SELECT * FROM " + tempApp); + db.execSQL(copyData(AppMetadataTable.Cols.ALL_COLS, tempApp, AppMetadataTable.NAME)); db.execSQL("DELETE FROM " + ApkTable.NAME + " WHERE 1"); - db.execSQL("INSERT INTO " + ApkTable.NAME + " SELECT * FROM " + tempApk); + db.execSQL(copyData(ApkTable.Cols.ALL_COLS, tempApk, ApkTable.NAME)); db.setTransactionSuccessful();