From 5bde27daa84f2dd4ba8afa64e1812a10be5986a9 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Fri, 14 Jul 2017 15:24:22 +1000 Subject: [PATCH] Only copy the apps/apks for the current repo to temp tables. When preparing a temp database to write to, don't copy all apps/apks. Instead, only copy those _not_ belonging to the repo we are updating. In an ideal world, we'd not even need to copy them, but we need their IDs to be in the temp database so that we don't accidentally use the same auto-generated ID as the main database. This also means that we can drop the check for "does this app exist, and hence should we UPDATE it instead of INSERTing it?" and always just insert it. Then, when copying the temp table back to disk, first delete all apps/apks _belonging to the repo being updated_. Then, copy back the apks/apps we found in the repo. This again improves performance because we no longer need to bopy back and forth data which we know wont change (as evidenced by the fact it belongs to a differen trepo). I don't think this was possible earlier before we did the work to support repo priorities properly. That is because we had a single app which was serviced by several repositories. Now, we have multiple entries in the `fdroid_app` table, for each repo which supports that app. --- .../org/fdroid/fdroid/data/RepoPersister.java | 16 ++--- .../java/org/fdroid/fdroid/data/Schema.java | 4 +- .../fdroid/fdroid/data/TempApkProvider.java | 13 +++-- .../fdroid/fdroid/data/TempAppProvider.java | 58 ++++++++++++++----- .../updater/ProperMultiRepoUpdaterTest.java | 4 +- 5 files changed, 64 insertions(+), 31 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/data/RepoPersister.java b/app/src/main/java/org/fdroid/fdroid/data/RepoPersister.java index 979f2747d..d60a1b42a 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/RepoPersister.java +++ b/app/src/main/java/org/fdroid/fdroid/data/RepoPersister.java @@ -161,11 +161,11 @@ public class RepoPersister { private ArrayList insertOrUpdateApps(List apps) { ArrayList operations = new ArrayList<>(apps.size()); for (App app : apps) { - if (isAppInDatabase(app)) { + /*if (isAppInDatabase(app)) { operations.add(updateExistingApp(app)); } else { - operations.add(insertNewApp(app)); - } + */operations.add(insertNewApp(app)); + /*}*/ } return operations; } @@ -175,16 +175,16 @@ public class RepoPersister { * will queue up an update or an insert {@link ContentProviderOperation} for each package. */ private ArrayList insertOrUpdateApks(List packages) { - String[] projection = new String[]{ + /*String[] projection = new String[]{ Schema.ApkTable.Cols.Package.PACKAGE_NAME, Schema.ApkTable.Cols.VERSION_CODE, Schema.ApkTable.Cols.REPO_ID, Schema.ApkTable.Cols.APP_ID, }; - List existingApks = ApkProvider.Helper.knownApks(context, packages, projection); + List existingApks = ApkProvider.Helper.knownApks(context, packages, projection);*/ ArrayList operations = new ArrayList<>(packages.size()); for (Apk apk : packages) { - boolean exists = false; + /*boolean exists = false; for (Apk existing : existingApks) { if (existing.repoId == apk.repoId && existing.packageName.equals(apk.packageName) && existing.versionCode == apk.versionCode) { exists = true; @@ -195,8 +195,8 @@ public class RepoPersister { if (exists) { operations.add(updateExistingApk(apk)); } else { - operations.add(insertNewApk(apk)); - } + */operations.add(insertNewApk(apk)); + /*}*/ } return operations; 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 c10996b7c..da3e80565 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/Schema.java +++ b/app/src/main/java/org/fdroid/fdroid/data/Schema.java @@ -85,6 +85,8 @@ public interface Schema { String NAME = "fdroid_categoryAppMetadataJoin"; interface Cols { + String ROW_ID = "rowid"; + /** * Foreign key to {@link AppMetadataTable}. * @see AppMetadataTable @@ -100,7 +102,7 @@ public interface Schema { /** * @see AppMetadataTable.Cols#ALL_COLS */ - String[] ALL_COLS = {APP_METADATA_ID, CATEGORY_ID}; + String[] ALL_COLS = {ROW_ID, APP_METADATA_ID, CATEGORY_ID}; } } 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 683022797..5499e302f 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/TempApkProvider.java +++ b/app/src/main/java/org/fdroid/fdroid/data/TempApkProvider.java @@ -7,6 +7,7 @@ import android.database.sqlite.SQLiteDatabase; import android.net.Uri; import org.fdroid.fdroid.data.Schema.ApkTable; +import org.fdroid.fdroid.data.Schema.ApkTable.Cols; import java.util.List; @@ -130,10 +131,14 @@ public class TempApkProvider extends ApkProvider { private void initTable(long repoIdBeingUpdated) { final SQLiteDatabase db = db(); final String memoryDbName = TempAppProvider.DB; - 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_appId on " + getTableName() + " (" + ApkTable.Cols.APP_ID + ");"); - db.execSQL("CREATE INDEX IF NOT EXISTS " + memoryDbName + ".apk_compatible ON " + getTableName() + " (" + ApkTable.Cols.IS_COMPATIBLE + ");"); + db.execSQL(DBHelper.CREATE_TABLE_APK.replaceFirst(ApkTable.NAME, memoryDbName + "." + getTableName())); + + String where = ApkTable.NAME + "." + Cols.REPO_ID + " != ?"; + String[] whereArgs = new String[]{Long.toString(repoIdBeingUpdated)}; + db.execSQL(TempAppProvider.copyData(Cols.ALL_COLS, ApkTable.NAME, memoryDbName + "." + getTableName(), where), whereArgs); + + db.execSQL("CREATE INDEX IF NOT EXISTS " + memoryDbName + ".apk_appId on " + getTableName() + " (" + Cols.APP_ID + ");"); + db.execSQL("CREATE INDEX IF NOT EXISTS " + memoryDbName + ".apk_compatible ON " + getTableName() + " (" + 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 14d2c6147..4bf04dc10 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/TempAppProvider.java +++ b/app/src/main/java/org/fdroid/fdroid/data/TempAppProvider.java @@ -228,24 +228,40 @@ public class TempAppProvider extends AppProvider { private void initTable(long repoIdBeingUpdated) { final SQLiteDatabase db = db(); + + String mainApp = AppMetadataTable.NAME; + String tempApp = DB + "." + getTableName(); + String mainCat = CatJoinTable.NAME; + String tempCat = DB + "." + getCatJoinTableName(); + ensureTempTableDetached(db); db.execSQL("ATTACH DATABASE ':memory:' AS " + DB); - db.execSQL(DBHelper.CREATE_TABLE_APP_METADATA.replaceFirst(AppMetadataTable.NAME, DB + "." + getTableName())); - db.execSQL(DBHelper.CREATE_TABLE_CAT_JOIN.replaceFirst(CatJoinTable.NAME, DB + "." + getCatJoinTableName())); - db.execSQL(copyData(AppMetadataTable.Cols.ALL_COLS, AppMetadataTable.NAME, DB + "." + getTableName())); - db.execSQL(copyData(CatJoinTable.Cols.ALL_COLS, CatJoinTable.NAME, DB + "." + getCatJoinTableName())); - db.execSQL("CREATE INDEX IF NOT EXISTS " + DB + ".app_id ON " + getTableName() + " (" + AppMetadataTable.Cols.PACKAGE_ID + ");"); - 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 + ");"); + db.execSQL(DBHelper.CREATE_TABLE_APP_METADATA.replaceFirst(AppMetadataTable.NAME, tempApp)); + db.execSQL(DBHelper.CREATE_TABLE_CAT_JOIN.replaceFirst(CatJoinTable.NAME, tempCat)); + + String appWhere = mainApp + "." + Cols.REPO_ID + " != ?"; + String[] repoArgs = new String[]{Long.toString(repoIdBeingUpdated)}; + db.execSQL(copyData(Cols.ALL_COLS, mainApp, tempApp, appWhere), repoArgs); + + // TODO: String catWhere = mainCat + "." + CatJoinTable.Cols..Cols.REPO_ID + " != ?"; + db.execSQL(copyData(CatJoinTable.Cols.ALL_COLS, mainCat, tempCat, null)); + + db.execSQL("CREATE INDEX IF NOT EXISTS " + DB + ".app_id ON " + getTableName() + " (" + Cols.PACKAGE_ID + ");"); + db.execSQL("CREATE INDEX IF NOT EXISTS " + DB + ".app_upstreamVercode ON " + getTableName() + " (" + Cols.UPSTREAM_VERSION_CODE + ");"); + db.execSQL("CREATE INDEX IF NOT EXISTS " + DB + ".app_compatible ON " + getTableName() + " (" + 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) { + static String copyData(String[] colsToCopy, String fromTable, String toTable, String where) { String cols = TextUtils.join(", ", colsToCopy); - return "INSERT INTO " + toTable + " (" + cols + ") SELECT " + cols + " FROM " + fromTable; + String sql = "INSERT INTO " + toTable + " (" + cols + ") SELECT " + cols + " FROM " + fromTable; + if (!TextUtils.isEmpty(where)) { + sql += " WHERE " + where; + } + return sql; } private void commitTable(long repoIdToCommit) { @@ -257,14 +273,16 @@ public class TempAppProvider extends AppProvider { final String tempApk = DB + "." + TempApkProvider.TABLE_TEMP_APK; final String tempCatJoin = DB + "." + TABLE_TEMP_CAT_JOIN; - db.execSQL("DELETE FROM " + AppMetadataTable.NAME + " WHERE 1"); - db.execSQL(copyData(AppMetadataTable.Cols.ALL_COLS, tempApp, AppMetadataTable.NAME)); + final String[] repoArgs = new String[]{Long.toString(repoIdToCommit)}; - db.execSQL("DELETE FROM " + ApkTable.NAME + " WHERE 1"); - db.execSQL(copyData(ApkTable.Cols.ALL_COLS, tempApk, ApkTable.NAME)); + db.execSQL("DELETE FROM " + AppMetadataTable.NAME + " WHERE " + Cols.REPO_ID + " = ?", repoArgs); + db.execSQL(copyData(Cols.ALL_COLS, tempApp, AppMetadataTable.NAME, Cols.REPO_ID + " = ?"), repoArgs); - db.execSQL("DELETE FROM " + CatJoinTable.NAME + " WHERE 1"); - db.execSQL(copyData(CatJoinTable.Cols.ALL_COLS, tempCatJoin, CatJoinTable.NAME)); + db.execSQL("DELETE FROM " + ApkTable.NAME + " WHERE " + ApkTable.Cols.REPO_ID + " = ?", repoArgs); + db.execSQL(copyData(ApkTable.Cols.ALL_COLS, tempApk, ApkTable.NAME, ApkTable.Cols.REPO_ID + " = ?"), repoArgs); + + db.execSQL("DELETE FROM " + CatJoinTable.NAME + " WHERE " + getCatRepoWhere(CatJoinTable.NAME), repoArgs); + db.execSQL(copyData(CatJoinTable.Cols.ALL_COLS, tempCatJoin, CatJoinTable.NAME, getCatRepoWhere(tempCatJoin)), repoArgs); db.setTransactionSuccessful(); @@ -276,4 +294,14 @@ public class TempAppProvider extends AppProvider { db.execSQL("DETACH DATABASE " + DB); // Can't be done in a transaction. } } + + private String getCatRepoWhere(String categoryTable) { + String catRepoSubquery = + "SELECT DISTINCT innerCatJoin." + CatJoinTable.Cols.ROW_ID + " " + + "FROM " + categoryTable + " AS innerCatJoin " + + "JOIN " + getTableName() + " AS app ON (app." + Cols.ROW_ID + " = innerCatJoin." + CatJoinTable.Cols.APP_METADATA_ID + ") " + + "WHERE app." + Cols.REPO_ID + " = ?"; + + return CatJoinTable.Cols.ROW_ID + " IN (" + catRepoSubquery + ")"; + } } diff --git a/app/src/test/java/org/fdroid/fdroid/updater/ProperMultiRepoUpdaterTest.java b/app/src/test/java/org/fdroid/fdroid/updater/ProperMultiRepoUpdaterTest.java index d887fdb7b..2936110fa 100644 --- a/app/src/test/java/org/fdroid/fdroid/updater/ProperMultiRepoUpdaterTest.java +++ b/app/src/test/java/org/fdroid/fdroid/updater/ProperMultiRepoUpdaterTest.java @@ -45,8 +45,6 @@ public class ProperMultiRepoUpdaterTest extends MultiRepoUpdaterTest { @StringDef({"Conflicting", "Normal"}) public @interface RepoIdentifier { } - /* - *This test fails due to issue #568 (https://gitlab.com/fdroid/fdroidclient/issues/568). @Test public void appsRemovedFromRepo() throws RepoUpdater.UpdateException { assertEquals(0, AppProvider.Helper.all(context.getContentResolver()).size()); @@ -67,7 +65,7 @@ public class ProperMultiRepoUpdaterTest extends MultiRepoUpdaterTest { assertEquals(6, ApkProvider.Helper.findByRepo(context, repo, Schema.ApkTable.Cols.ALL).size()); assertEquals(4, ApkProvider.Helper.findByPackageName(context, "org.adaway").size()); assertEquals(2, ApkProvider.Helper.findByPackageName(context, "org.dgtale.icsimport").size()); - }*/ + } @Test public void mainRepo() throws RepoUpdater.UpdateException {