From 8c3441939f680eeb85cc22a1b767b2aa3c464f66 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Fri, 14 Jul 2017 15:00:29 +1000 Subject: [PATCH 1/4] Pass through the ID of the repo being updated to the temp tables. This will allow for more intelligent and efficient copying of data back and forth from temp to persistent tables. --- .../org/fdroid/fdroid/IndexV1Updater.java | 4 +-- .../java/org/fdroid/fdroid/RepoUpdater.java | 2 +- .../org/fdroid/fdroid/data/RepoPersister.java | 6 ++-- .../fdroid/fdroid/data/TempApkProvider.java | 15 ++++++---- .../fdroid/fdroid/data/TempAppProvider.java | 28 +++++++++++-------- .../fdroid/fdroid/data/ProviderUriTests.java | 2 +- 6 files changed, 33 insertions(+), 24 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/IndexV1Updater.java b/app/src/main/java/org/fdroid/fdroid/IndexV1Updater.java index e14344b2c..bf12dad1a 100644 --- a/app/src/main/java/org/fdroid/fdroid/IndexV1Updater.java +++ b/app/src/main/java/org/fdroid/fdroid/IndexV1Updater.java @@ -139,7 +139,7 @@ public class IndexV1Updater extends RepoUpdater { * and {@link Apk} instances. This uses {@link RepoPersister} to add the apps * and packages to the database in {@link RepoPersister#saveToDb(App, List)} * to write the {@link Repo}, and commit the whole thing in - * {@link RepoPersister#commit(ContentValues)}. One confusing thing about this + * {@link RepoPersister#commit(ContentValues, long)}. One confusing thing about this * whole process is that {@link RepoPersister} needs to first create and entry * in the database, then fetch the ID from the database to populate * {@link Repo#id}. That has to happen first, then the rest of the {@code Repo} @@ -265,7 +265,7 @@ public class IndexV1Updater extends RepoUpdater { if (repo.mirrors != null && repo.mirrors.length > 0) { contentValues.put(Schema.RepoTable.Cols.MIRRORS, Utils.serializeCommaSeparatedString(repo.mirrors)); } - repoPersister.commit(contentValues); + repoPersister.commit(contentValues, repo.getId()); profiler.log("Persited to database."); diff --git a/app/src/main/java/org/fdroid/fdroid/RepoUpdater.java b/app/src/main/java/org/fdroid/fdroid/RepoUpdater.java index f77548e33..b76475c03 100644 --- a/app/src/main/java/org/fdroid/fdroid/RepoUpdater.java +++ b/app/src/main/java/org/fdroid/fdroid/RepoUpdater.java @@ -280,7 +280,7 @@ public class RepoUpdater { private void commitToDb() throws UpdateException { Log.i(TAG, "Repo signature verified, saving app metadata to database."); notifyCommittingToDb(); - persister.commit(repoDetailsToSave); + persister.commit(repoDetailsToSave, repo.getId()); } private void assertSigningCertFromXmlCorrect() throws SigningException { 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 abe30a51d..979f2747d 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/RepoPersister.java +++ b/app/src/main/java/org/fdroid/fdroid/data/RepoPersister.java @@ -66,9 +66,9 @@ public class RepoPersister { } } - public void commit(ContentValues repoDetailsToSave) throws RepoUpdater.UpdateException { + public void commit(ContentValues repoDetailsToSave, long repoIdToCommit) throws RepoUpdater.UpdateException { flushBufferToDb(); - TempAppProvider.Helper.commitAppsAndApks(context); + TempAppProvider.Helper.commitAppsAndApks(context, repoIdToCommit); RepoProvider.Helper.update(context, repo, repoDetailsToSave); } @@ -79,7 +79,7 @@ public class RepoPersister { // the index was signed with until we've finished reading it - and we don't // want to put stuff in the real database until we are sure it is from a // trusted source. It also helps performance as it is done via an in-memory database. - TempAppProvider.Helper.init(context); + TempAppProvider.Helper.init(context, repo.getId()); hasBeenInitialized = true; } 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 412688121..683022797 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/TempApkProvider.java +++ b/app/src/main/java/org/fdroid/fdroid/data/TempApkProvider.java @@ -27,7 +27,7 @@ public class TempApkProvider extends ApkProvider { private static final UriMatcher MATCHER = new UriMatcher(-1); static { - MATCHER.addURI(getAuthority(), PATH_INIT, CODE_INIT); + MATCHER.addURI(getAuthority(), PATH_INIT + "/#", CODE_INIT); MATCHER.addURI(getAuthority(), PATH_APK_FROM_ANY_REPO + "/#/*", CODE_APK_FROM_ANY_REPO); MATCHER.addURI(getAuthority(), PATH_APK_FROM_REPO + "/#/#", CODE_APK_FROM_REPO); MATCHER.addURI(getAuthority(), PATH_REPO_APK + "/#/*", CODE_REPO_APK); @@ -76,12 +76,15 @@ public class TempApkProvider extends ApkProvider { * table and populates it with all the data from the real apk provider table. * * This is package local because it must be invoked after - * {@link org.fdroid.fdroid.data.TempAppProvider.Helper#init(Context)}. Due to this + * {@link org.fdroid.fdroid.data.TempAppProvider.Helper#init(Context, long)}. Due to this * dependence, that method invokes this one itself, rather than leaving it to the * {@link RepoPersister}. */ - static void init(Context context) { - Uri uri = Uri.withAppendedPath(getContentUri(), PATH_INIT); + static void init(Context context, long repoIdToUpdate) { + Uri uri = getContentUri().buildUpon() + .appendPath(PATH_INIT) + .appendPath(Long.toString(repoIdToUpdate)) + .build(); context.getContentResolver().insert(uri, new ContentValues()); } } @@ -89,7 +92,7 @@ public class TempApkProvider extends ApkProvider { @Override public Uri insert(Uri uri, ContentValues values) { if (MATCHER.match(uri) == CODE_INIT) { - initTable(); + initTable(Long.parseLong(uri.getLastPathSegment())); return null; } @@ -124,7 +127,7 @@ public class TempApkProvider extends ApkProvider { } - private void initTable() { + 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())); 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 79d61817c..14d2c6147 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/TempAppProvider.java +++ b/app/src/main/java/org/fdroid/fdroid/data/TempAppProvider.java @@ -43,8 +43,8 @@ public class TempAppProvider extends AppProvider { private static final UriMatcher MATCHER = new UriMatcher(-1); static { - MATCHER.addURI(getAuthority(), PATH_INIT, CODE_INIT); - MATCHER.addURI(getAuthority(), PATH_COMMIT, CODE_COMMIT); + MATCHER.addURI(getAuthority(), PATH_INIT + "/#", CODE_INIT); + MATCHER.addURI(getAuthority(), PATH_COMMIT + "/#", CODE_COMMIT); MATCHER.addURI(getAuthority(), PATH_APPS + "/#/*", APPS); MATCHER.addURI(getAuthority(), PATH_SPECIFIC_APP + "/#/*", CODE_SINGLE); } @@ -105,10 +105,13 @@ public class TempAppProvider extends AppProvider { * Deletes the old temporary table (if it exists). Then creates a new temporary apk provider * table and populates it with all the data from the real apk provider table. */ - public static void init(Context context) { - Uri uri = Uri.withAppendedPath(getContentUri(), PATH_INIT); + public static void init(Context context, long repoIdToUpdate) { + Uri uri = getContentUri().buildUpon() + .appendPath(PATH_INIT) + .appendPath(Long.toString(repoIdToUpdate)) + .build(); context.getContentResolver().insert(uri, new ContentValues()); - TempApkProvider.Helper.init(context); + TempApkProvider.Helper.init(context, repoIdToUpdate); } public static List findByPackageNames(Context context, @@ -122,8 +125,11 @@ public class TempAppProvider extends AppProvider { * Saves data from the temp table to the apk table, by removing _EVERYTHING_ from the real * apk table and inserting all of the records from here. The temporary table is then removed. */ - public static void commitAppsAndApks(Context context) { - Uri uri = Uri.withAppendedPath(getContentUri(), PATH_COMMIT); + public static void commitAppsAndApks(Context context, long repoIdToCommit) { + Uri uri = getContentUri().buildUpon() + .appendPath(PATH_COMMIT) + .appendPath(Long.toString(repoIdToCommit)) + .build(); context.getContentResolver().insert(uri, new ContentValues()); } } @@ -137,11 +143,11 @@ public class TempAppProvider extends AppProvider { public Uri insert(Uri uri, ContentValues values) { switch (MATCHER.match(uri)) { case CODE_INIT: - initTable(); + initTable(Long.parseLong(uri.getLastPathSegment())); return null; case CODE_COMMIT: updateAllAppDetails(); - commitTable(); + commitTable(Long.parseLong(uri.getLastPathSegment())); return null; default: return super.insert(uri, values); @@ -220,7 +226,7 @@ public class TempAppProvider extends AppProvider { } } - private void initTable() { + private void initTable(long repoIdBeingUpdated) { final SQLiteDatabase db = db(); ensureTempTableDetached(db); db.execSQL("ATTACH DATABASE ':memory:' AS " + DB); @@ -242,7 +248,7 @@ public class TempAppProvider extends AppProvider { return "INSERT INTO " + toTable + " (" + cols + ") SELECT " + cols + " FROM " + fromTable; } - private void commitTable() { + private void commitTable(long repoIdToCommit) { final SQLiteDatabase db = db(); try { db.beginTransaction(); diff --git a/app/src/test/java/org/fdroid/fdroid/data/ProviderUriTests.java b/app/src/test/java/org/fdroid/fdroid/data/ProviderUriTests.java index 93ec81e7b..cd72e4523 100644 --- a/app/src/test/java/org/fdroid/fdroid/data/ProviderUriTests.java +++ b/app/src/test/java/org/fdroid/fdroid/data/ProviderUriTests.java @@ -110,7 +110,7 @@ public class ProviderUriTests { // Required so that the `assertValidUri` calls below will indeed have a real temp_fdroid_app // table to query. - TempAppProvider.Helper.init(TestUtils.createContextWithContentResolver(resolver)); + TempAppProvider.Helper.init(TestUtils.createContextWithContentResolver(resolver), 123); List packageNames = new ArrayList<>(2); packageNames.add("org.fdroid.fdroid"); From 5bde27daa84f2dd4ba8afa64e1812a10be5986a9 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Fri, 14 Jul 2017 15:24:22 +1000 Subject: [PATCH 2/4] 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 { From e26748e0e09c6ab050acba426dcf354a31b19100 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Mon, 17 Jul 2017 13:55:37 +1000 Subject: [PATCH 3/4] Remove now-unneccesary "update" code from repo updater. Now that we need only "insert" new apps rather than" * Identify if an app exists * If so, update * If not, insert There is much less code required for all of this stuff. --- .../org/fdroid/fdroid/data/ApkProvider.java | 112 +------------ .../org/fdroid/fdroid/data/RepoPersister.java | 147 ++---------------- .../fdroid/fdroid/data/TempApkProvider.java | 43 +---- .../fdroid/fdroid/data/TempAppProvider.java | 56 +------ .../fdroid/fdroid/data/ApkProviderTest.java | 63 -------- .../fdroid/data/FDroidProviderTest.java | 1 + .../fdroid/fdroid/data/ProviderUriTests.java | 22 --- 7 files changed, 22 insertions(+), 422 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/data/ApkProvider.java b/app/src/main/java/org/fdroid/fdroid/data/ApkProvider.java index d3f3e8ddc..41988a378 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/ApkProvider.java +++ b/app/src/main/java/org/fdroid/fdroid/data/ApkProvider.java @@ -94,17 +94,6 @@ public class ApkProvider extends FDroidProvider { return findApkFromAnyRepo(context, packageName, versionCode, signature, Cols.ALL); } - /** - * Find all apks for a particular app, but limit it to those originating from the - * specified repo. - */ - public static List findByUri(Context context, Repo repo, List apps, String[] projection) { - ContentResolver resolver = context.getContentResolver(); - final Uri uri = getContentUriForApps(repo, apps); - Cursor cursor = resolver.query(uri, projection, null, null, null); - return cursorToList(cursor); - } - public static Apk findApkFromAnyRepo(Context context, String packageName, int versionCode, @Nullable String signature, String[] projection) { final Uri uri = getApkFromAnyRepoUri(packageName, versionCode, signature); @@ -137,36 +126,6 @@ public class ApkProvider extends FDroidProvider { return cursorToList(cursor); } - /** - * Returns apks in the database, which have the same packageName and version as - * one of the apks in the "apks" argument. - */ - public static List knownApks(Context context, List apks, String[] fields) { - if (apks.isEmpty()) { - return new ArrayList<>(); - } - - List knownApks = new ArrayList<>(); - if (apks.size() > ApkProvider.MAX_APKS_TO_QUERY) { - int middle = apks.size() / 2; - List apks1 = apks.subList(0, middle); - List apks2 = apks.subList(middle, apks.size()); - knownApks.addAll(knownApks(context, apks1, fields)); - knownApks.addAll(knownApks(context, apks2, fields)); - } else { - knownApks.addAll(knownApksSafe(context, apks, fields)); - } - return knownApks; - - } - - private static List knownApksSafe(final Context context, final List apks, final String[] fields) { - ContentResolver resolver = context.getContentResolver(); - final Uri uri = getContentUri(apks); - Cursor cursor = resolver.query(uri, fields, null, null, null); - return cursorToList(cursor); - } - public static List findByRepo(Context context, Repo repo, String[] fields) { ContentResolver resolver = context.getContentResolver(); final Uri uri = getRepoUri(repo.getId()); @@ -206,9 +165,7 @@ public class ApkProvider extends FDroidProvider { private static final int CODE_PACKAGE = CODE_SINGLE + 1; private static final int CODE_REPO = CODE_PACKAGE + 1; private static final int CODE_APKS = CODE_REPO + 1; - private static final int CODE_REPO_APPS = CODE_APKS + 1; - protected static final int CODE_REPO_APK = CODE_REPO_APPS + 1; - private static final int CODE_APK_ROW_ID = CODE_REPO_APK + 1; + private static final int CODE_APK_ROW_ID = CODE_APKS + 1; static final int CODE_APK_FROM_ANY_REPO = CODE_APK_ROW_ID + 1; static final int CODE_APK_FROM_REPO = CODE_APK_FROM_ANY_REPO + 1; @@ -218,8 +175,6 @@ public class ApkProvider extends FDroidProvider { private static final String PATH_APKS = "apks"; private static final String PATH_APP = "app"; private static final String PATH_REPO = "repo"; - private static final String PATH_REPO_APPS = "repo-apps"; - protected static final String PATH_REPO_APK = "repo-apk"; private static final String PATH_APK_ROW_ID = "apk-rowId"; private static final UriMatcher MATCHER = new UriMatcher(-1); @@ -238,8 +193,6 @@ public class ApkProvider extends FDroidProvider { MATCHER.addURI(getAuthority(), PATH_APK_FROM_REPO + "/#/#", CODE_APK_FROM_REPO); MATCHER.addURI(getAuthority(), PATH_APKS + "/*", CODE_APKS); MATCHER.addURI(getAuthority(), PATH_APP + "/*", CODE_PACKAGE); - MATCHER.addURI(getAuthority(), PATH_REPO_APPS + "/#/*", CODE_REPO_APPS); - MATCHER.addURI(getAuthority(), PATH_REPO_APK + "/#/*", CODE_REPO_APK); MATCHER.addURI(getAuthority(), PATH_APK_ROW_ID + "/#", CODE_APK_ROW_ID); MATCHER.addURI(getAuthority(), null, CODE_LIST); } @@ -293,51 +246,6 @@ public class ApkProvider extends FDroidProvider { return builder.build(); } - public static Uri getContentUriForApps(Repo repo, List apps) { - return getContentUri() - .buildUpon() - .appendPath(PATH_REPO_APPS) - .appendPath(Long.toString(repo.id)) - .appendPath(buildAppString(apps)) - .build(); - } - - /** - * Intentionally left protected because it will break if apks is larger than - * {@link org.fdroid.fdroid.data.ApkProvider#MAX_APKS_TO_QUERY}. Instead of using - * this directly, think about using - * {@link ApkProvider.Helper#knownApks(android.content.Context, java.util.List, String[])} - */ - static Uri getContentUri(List apks) { - return getContentUri().buildUpon() - .appendPath(PATH_APKS) - .appendPath(buildApkString(apks)) - .build(); - } - - protected static String buildApkString(List apks) { - StringBuilder builder = new StringBuilder(); - for (int i = 0; i < apks.size(); i++) { - if (i != 0) { - builder.append(','); - } - final Apk apk = apks.get(i); - builder.append(apk.appId).append(':').append(apk.versionCode); - } - return builder.toString(); - } - - private static String buildAppString(List apks) { - StringBuilder builder = new StringBuilder(); - for (int i = 0; i < apks.size(); i++) { - if (i != 0) { - builder.append(','); - } - builder.append(apks.get(i).packageName); - } - return builder.toString(); - } - @Override protected String getTableName() { return ApkTable.NAME; @@ -470,10 +378,6 @@ public class ApkProvider extends FDroidProvider { return new QuerySelection(selection, args); } - private QuerySelection queryRepoApps(long repoId, String packageNames) { - return queryRepo(repoId).add(AppProvider.queryPackageNames(packageNames, "pkg." + PackageTable.Cols.PACKAGE_NAME)); - } - protected QuerySelection queryApks(String apkKeys) { return queryApks(apkKeys, true); } @@ -547,11 +451,6 @@ public class ApkProvider extends FDroidProvider { query = query.add(queryRepo(Long.parseLong(uri.getLastPathSegment()))); break; - case CODE_REPO_APPS: - List pathSegments = uri.getPathSegments(); - query = query.add(queryRepoApps(Long.parseLong(pathSegments.get(1)), pathSegments.get(2))); - break; - default: Log.e(TAG, "Invalid URI for apk content provider: " + uri); throw new UnsupportedOperationException("Invalid URI for apk content provider: " + uri); @@ -611,12 +510,6 @@ public class ApkProvider extends FDroidProvider { query = query.add(queryApks(uri.getLastPathSegment(), false)); break; - // TODO: Add tests for this. - case CODE_REPO_APK: - List pathSegments = uri.getPathSegments(); - query = query.add(queryRepo(Long.parseLong(pathSegments.get(1)))).add(queryApks(pathSegments.get(2))); - break; - default: Log.e(TAG, "Invalid URI for apk content provider: " + uri); throw new UnsupportedOperationException("Invalid URI for apk content provider: " + uri); @@ -633,10 +526,7 @@ public class ApkProvider extends FDroidProvider { if (MATCHER.match(uri) != CODE_APK_FROM_REPO) { throw new UnsupportedOperationException("Cannot update anything other than a single apk."); } - return performUpdateUnchecked(uri, values, where, whereArgs); - } - protected int performUpdateUnchecked(Uri uri, ContentValues values, String where, String[] whereArgs) { validateFields(Cols.ALL, values); removeFieldsFromOtherTables(values); 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 d60a1b42a..f278de932 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/RepoPersister.java +++ b/app/src/main/java/org/fdroid/fdroid/data/RepoPersister.java @@ -7,7 +7,6 @@ import android.content.OperationApplicationException; import android.net.Uri; import android.os.RemoteException; import android.support.annotation.NonNull; -import android.support.annotation.Nullable; import org.fdroid.fdroid.CompatibilityChecker; import org.fdroid.fdroid.RepoUpdater; @@ -18,7 +17,6 @@ import java.util.HashMap; import java.util.List; import java.util.Map; -@SuppressWarnings("LineLength") public class RepoPersister { private static final String TAG = "RepoPersister"; @@ -84,7 +82,7 @@ public class RepoPersister { } if (apksToSave.size() > 0 || appsToSave.size() > 0) { - Utils.debugLog(TAG, "Flushing details of up to " + MAX_APP_BUFFER + " apps and their packages to the database."); + Utils.debugLog(TAG, "Flushing details of up to " + MAX_APP_BUFFER + " apps/packages to the database."); Map appIds = flushAppsToDbInBatch(); flushApksToDbInBatch(appIds); apksToSave.clear(); @@ -103,12 +101,7 @@ public class RepoPersister { calcApkCompatibilityFlags(apksToSaveList); - ArrayList apkOperations = new ArrayList<>(); - ContentProviderOperation clearOrphans = deleteOrphanedApks(appsToSave, apksToSave); - if (clearOrphans != null) { - apkOperations.add(clearOrphans); - } - apkOperations.addAll(insertOrUpdateApks(apksToSaveList)); + ArrayList apkOperations = insertApks(apksToSaveList); try { context.getContentResolver().applyBatch(TempApkProvider.getAuthority(), apkOperations); @@ -123,7 +116,7 @@ public class RepoPersister { * can be returned and the relevant apks can be joined to the app table correctly. */ private Map flushAppsToDbInBatch() throws RepoUpdater.UpdateException { - ArrayList appOperations = insertOrUpdateApps(appsToSave); + ArrayList appOperations = insertApps(appsToSave); try { context.getContentResolver().applyBatch(TempAppProvider.getAuthority(), appOperations); @@ -144,7 +137,12 @@ public class RepoPersister { for (App app : apps) { packageNames.add(app.packageName); } - String[] projection = {Schema.AppMetadataTable.Cols.ROW_ID, Schema.AppMetadataTable.Cols.Package.PACKAGE_NAME}; + + String[] projection = { + Schema.AppMetadataTable.Cols.ROW_ID, + Schema.AppMetadataTable.Cols.Package.PACKAGE_NAME, + }; + List fromDb = TempAppProvider.Helper.findByPackageNames(context, packageNames, repo.id, projection); Map ids = new HashMap<>(fromDb.size()); @@ -154,138 +152,27 @@ public class RepoPersister { return ids; } - /** - * Depending on whether the {@link App}s have been added to the database previously, this - * will queue up an update or an insert {@link ContentProviderOperation} for each app. - */ - private ArrayList insertOrUpdateApps(List apps) { + private ArrayList insertApps(List apps) { ArrayList operations = new ArrayList<>(apps.size()); for (App app : apps) { - /*if (isAppInDatabase(app)) { - operations.add(updateExistingApp(app)); - } else { - */operations.add(insertNewApp(app)); - /*}*/ + ContentValues values = app.toContentValues(); + Uri uri = TempAppProvider.getContentUri(); + operations.add(ContentProviderOperation.newInsert(uri).withValues(values).build()); } return operations; } - /** - * Depending on whether the .apks have been added to the database previously, this - * will queue up an update or an insert {@link ContentProviderOperation} for each package. - */ - private ArrayList insertOrUpdateApks(List packages) { - /*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);*/ + private ArrayList insertApks(List packages) { ArrayList operations = new ArrayList<>(packages.size()); for (Apk apk : packages) { - /*boolean exists = false; - for (Apk existing : existingApks) { - if (existing.repoId == apk.repoId && existing.packageName.equals(apk.packageName) && existing.versionCode == apk.versionCode) { - exists = true; - break; - } - } - - if (exists) { - operations.add(updateExistingApk(apk)); - } else { - */operations.add(insertNewApk(apk)); - /*}*/ + ContentValues values = apk.toContentValues(); + Uri uri = TempApkProvider.getContentUri(); + operations.add(ContentProviderOperation.newInsert(uri).withValues(values).build()); } return operations; } - /** - * Creates an update {@link ContentProviderOperation} for the {@link App} in question. - * Does not do any checks to see if the app already exists or not. - */ - private ContentProviderOperation updateExistingApp(App app) { - Uri uri = TempAppProvider.getSpecificTempAppUri(app.packageName, app.repoId); - return ContentProviderOperation.newUpdate(uri).withValues(app.toContentValues()).build(); - } - - /** - * Creates an insert {@link ContentProviderOperation} for the {@link App} in question. - * Does not do any checks to see if the app already exists or not. - */ - private ContentProviderOperation insertNewApp(App app) { - ContentValues values = app.toContentValues(); - Uri uri = TempAppProvider.getContentUri(); - return ContentProviderOperation.newInsert(uri).withValues(values).build(); - } - - /** - * Looks in the database to see which apps we already know about. Only - * returns ids of apps that are in the database if they are in the "apps" - * array. - */ - private boolean isAppInDatabase(App app) { - String[] fields = {Schema.AppMetadataTable.Cols.Package.PACKAGE_NAME}; - App found = AppProvider.Helper.findSpecificApp(context.getContentResolver(), app.packageName, repo.id, fields); - return found != null; - } - - /** - * Creates an update {@link ContentProviderOperation} for the {@link Apk} in question. - * Does not do any checks to see if the apk already exists or not. - */ - private ContentProviderOperation updateExistingApk(final Apk apk) { - Uri uri = TempApkProvider.getApkUri(apk); - ContentValues values = apk.toContentValues(); - return ContentProviderOperation.newUpdate(uri).withValues(values).build(); - } - - /** - * Creates an insert {@link ContentProviderOperation} for the {@link Apk} in question. - * Does not do any checks to see if the apk already exists or not. - */ - private ContentProviderOperation insertNewApk(final Apk apk) { - ContentValues values = apk.toContentValues(); - Uri uri = TempApkProvider.getContentUri(); - return ContentProviderOperation.newInsert(uri).withValues(values).build(); - } - - /** - * Finds all apks from the repo we are currently updating, that belong to the specified app, - * and delete them as they are no longer provided by that repo. - */ - @Nullable - private ContentProviderOperation deleteOrphanedApks(List apps, Map> packages) { - String[] projection = new String[]{Schema.ApkTable.Cols.Package.PACKAGE_NAME, Schema.ApkTable.Cols.VERSION_CODE}; - List existing = ApkProvider.Helper.findByUri(context, repo, apps, projection); - List toDelete = new ArrayList<>(); - - for (Apk existingApk : existing) { - boolean shouldStay = false; - - if (packages.containsKey(existingApk.packageName)) { - for (Apk newApk : packages.get(existingApk.packageName)) { - if (newApk.versionCode == existingApk.versionCode) { - shouldStay = true; - break; - } - } - } - - if (!shouldStay) { - toDelete.add(existingApk); - } - } - - if (toDelete.size() == 0) { - return null; - } - Uri uri = TempApkProvider.getApksUri(repo, toDelete); - return ContentProviderOperation.newDelete(uri).build(); - } - /** * This cannot be offloaded to the database (as we did with the query which * updates apps, depending on whether their apks are compatible or not). 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 5499e302f..5a45c827b 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/TempApkProvider.java +++ b/app/src/main/java/org/fdroid/fdroid/data/TempApkProvider.java @@ -9,8 +9,6 @@ import android.net.Uri; import org.fdroid.fdroid.data.Schema.ApkTable; import org.fdroid.fdroid.data.Schema.ApkTable.Cols; -import java.util.List; - /** * This class does all of its operations in a temporary sqlite table. */ @@ -31,7 +29,6 @@ public class TempApkProvider extends ApkProvider { MATCHER.addURI(getAuthority(), PATH_INIT + "/#", CODE_INIT); MATCHER.addURI(getAuthority(), PATH_APK_FROM_ANY_REPO + "/#/*", CODE_APK_FROM_ANY_REPO); MATCHER.addURI(getAuthority(), PATH_APK_FROM_REPO + "/#/#", CODE_APK_FROM_REPO); - MATCHER.addURI(getAuthority(), PATH_REPO_APK + "/#/*", CODE_REPO_APK); } @Override @@ -52,24 +49,6 @@ public class TempApkProvider extends ApkProvider { return Uri.parse("content://" + getAuthority()); } - public static Uri getApkUri(Apk apk) { - return getContentUri() - .buildUpon() - .appendPath(PATH_APK_FROM_REPO) - .appendPath(Long.toString(apk.appId)) - .appendPath(Integer.toString(apk.versionCode)) - .build(); - } - - public static Uri getApksUri(Repo repo, List apks) { - return getContentUri() - .buildUpon() - .appendPath(PATH_REPO_APK) - .appendPath(Long.toString(repo.id)) - .appendPath(buildApkString(apks)) - .build(); - } - public static class Helper { /** @@ -102,30 +81,12 @@ public class TempApkProvider extends ApkProvider { @Override public int update(Uri uri, ContentValues values, String where, String[] whereArgs) { - if (MATCHER.match(uri) != CODE_APK_FROM_REPO) { - throw new UnsupportedOperationException("Cannot update anything other than a single apk."); - } - - return performUpdateUnchecked(uri, values, where, whereArgs); + throw new UnsupportedOperationException("Invalid URI for apk content provider: " + uri); } @Override public int delete(Uri uri, String where, String[] whereArgs) { - if (MATCHER.match(uri) != CODE_REPO_APK) { - throw new UnsupportedOperationException("Invalid URI for apk content provider: " + uri); - } - - List pathSegments = uri.getPathSegments(); - QuerySelection query = new QuerySelection(where, whereArgs) - .add(queryRepo(Long.parseLong(pathSegments.get(1)), false)) - .add(queryApks(pathSegments.get(2), false)); - - int rowsAffected = db().delete(getTableName(), query.getSelection(), query.getArgs()); - if (!isApplyingBatch()) { - getContext().getContentResolver().notifyChange(uri, null); - } - return rowsAffected; - + throw new UnsupportedOperationException("Invalid URI for apk content provider: " + uri); } private void initTable(long repoIdBeingUpdated) { 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 4bf04dc10..1f5ebb3a4 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/TempAppProvider.java +++ b/app/src/main/java/org/fdroid/fdroid/data/TempAppProvider.java @@ -8,7 +8,6 @@ import android.database.sqlite.SQLiteDatabase; import android.database.sqlite.SQLiteException; import android.net.Uri; import android.text.TextUtils; -import org.fdroid.fdroid.Utils; import org.fdroid.fdroid.data.Schema.ApkTable; import org.fdroid.fdroid.data.Schema.AppMetadataTable; import org.fdroid.fdroid.data.Schema.AppMetadataTable.Cols; @@ -67,19 +66,6 @@ public class TempAppProvider extends AppProvider { return Uri.parse("content://" + getAuthority()); } - /** - * Same as {@link AppProvider#getSpecificAppUri(String, long)}, except loads data from the temp - * table being used during a repo update rather than the persistent table. - */ - public static Uri getSpecificTempAppUri(String packageName, long repoId) { - return getContentUri() - .buildUpon() - .appendPath(PATH_SPECIFIC_APP) - .appendPath(Long.toString(repoId)) - .appendPath(packageName) - .build(); - } - public static Uri getAppsUri(List apps, long repoId) { return getContentUri().buildUpon() .appendPath(PATH_APPS) @@ -156,47 +142,7 @@ public class TempAppProvider extends AppProvider { @Override public int update(Uri uri, ContentValues values, String where, String[] whereArgs) { - if (MATCHER.match(uri) != CODE_SINGLE) { - throw new UnsupportedOperationException("Update not supported for " + uri + "."); - } - - if (values.containsKey(Cols.DESCRIPTION) && values.getAsString(Cols.DESCRIPTION) == null) { - // the database does not let a description be set as null - values.put(Cols.DESCRIPTION, ""); - } - - List pathParts = uri.getPathSegments(); - String packageName = pathParts.get(2); - long repoId = Long.parseLong(pathParts.get(1)); - QuerySelection query = new QuerySelection(where, whereArgs).add(querySingleForUpdate(packageName, repoId)); - - // Package names for apps cannot change... - values.remove(Cols.Package.PACKAGE_NAME); - - if (values.containsKey(Cols.ForWriting.Categories.CATEGORIES)) { - String[] categories = Utils.parseCommaSeparatedString( - values.getAsString(Cols.ForWriting.Categories.CATEGORIES)); - ensureCategories(categories, packageName, repoId); - values.remove(Cols.ForWriting.Categories.CATEGORIES); - } - - int count = db().update(getTableName(), values, query.getSelection(), query.getArgs()); - if (!isApplyingBatch()) { - getContext().getContentResolver().notifyChange(getHighestPriorityMetadataUri(packageName), null); - } - return count; - } - - private void ensureCategories(String[] categories, String packageName, long repoId) { - Query query = new AppProvider.Query(); - query.addField(Cols.ROW_ID); - query.addSelection(querySingle(packageName, repoId)); - Cursor cursor = db().rawQuery(query.toString(), query.getArgs()); - cursor.moveToFirst(); - long appMetadataId = cursor.getLong(0); - cursor.close(); - - ensureCategories(categories, appMetadataId); + throw new UnsupportedOperationException("Update not supported for " + uri + "."); } @Override diff --git a/app/src/test/java/org/fdroid/fdroid/data/ApkProviderTest.java b/app/src/test/java/org/fdroid/fdroid/data/ApkProviderTest.java index 8553b0509..11833cf9e 100644 --- a/app/src/test/java/org/fdroid/fdroid/data/ApkProviderTest.java +++ b/app/src/test/java/org/fdroid/fdroid/data/ApkProviderTest.java @@ -238,69 +238,6 @@ public class ApkProviderTest extends FDroidProviderTest { assertEquals(10, apk.repoId); } - @Test - public void testKnownApks() { - - App fdroid = Assert.ensureApp(context, "org.fdroid.fdroid"); - for (int i = 0; i < 7; i++) { - Assert.insertApk(context, fdroid, i); - } - - App exampleOrg = Assert.ensureApp(context, "org.example"); - for (int i = 0; i < 9; i++) { - Assert.insertApk(context, exampleOrg, i); - } - - App exampleCom = Assert.ensureApp(context, "com.example"); - for (int i = 0; i < 3; i++) { - Assert.insertApk(context, exampleCom, i); - } - - App thingo = Assert.ensureApp(context, "com.apk.thingo"); - Assert.insertApk(context, thingo, 1); - - Apk[] known = { - new MockApk(fdroid, 1), - new MockApk(fdroid, 3), - new MockApk(fdroid, 5), - - new MockApk(exampleCom, 1), - new MockApk(exampleCom, 2), - }; - - Apk[] unknown = { - new MockApk(fdroid, 7), - new MockApk(fdroid, 9), - new MockApk(fdroid, 11), - new MockApk(fdroid, 13), - - new MockApk(exampleCom, 3), - new MockApk(exampleCom, 4), - new MockApk(exampleCom, 5), - - new MockApk(-10, 1), - new MockApk(-10, 2), - }; - - List apksToCheck = new ArrayList<>(known.length + unknown.length); - Collections.addAll(apksToCheck, known); - Collections.addAll(apksToCheck, unknown); - - String[] projection = { - Cols.Package.PACKAGE_NAME, - Cols.APP_ID, - Cols.VERSION_CODE, - }; - - List knownApks = ApkProvider.Helper.knownApks(context, apksToCheck, projection); - - assertResultCount(known.length, knownApks); - - for (Apk knownApk : knownApks) { - assertContains(knownApks, knownApk); - } - } - @Test public void testFindByApp() { diff --git a/app/src/test/java/org/fdroid/fdroid/data/FDroidProviderTest.java b/app/src/test/java/org/fdroid/fdroid/data/FDroidProviderTest.java index c1f2463ce..d03b610a5 100644 --- a/app/src/test/java/org/fdroid/fdroid/data/FDroidProviderTest.java +++ b/app/src/test/java/org/fdroid/fdroid/data/FDroidProviderTest.java @@ -23,6 +23,7 @@ public abstract class FDroidProviderTest { @After public final void tearDownBase() { + CategoryProvider.Helper.clearCategoryIdCache(); FDroidProvider.clearDbHelperSingleton(); } diff --git a/app/src/test/java/org/fdroid/fdroid/data/ProviderUriTests.java b/app/src/test/java/org/fdroid/fdroid/data/ProviderUriTests.java index cd72e4523..05cb646d4 100644 --- a/app/src/test/java/org/fdroid/fdroid/data/ProviderUriTests.java +++ b/app/src/test/java/org/fdroid/fdroid/data/ProviderUriTests.java @@ -140,29 +140,7 @@ public class ProviderUriTests { assertValidUri(resolver, ApkProvider.getContentUri(), "content://org.fdroid.fdroid.data.ApkProvider", projection); assertValidUri(resolver, ApkProvider.getAppUri("org.fdroid.fdroid"), "content://org.fdroid.fdroid.data.ApkProvider/app/org.fdroid.fdroid", projection); assertValidUri(resolver, ApkProvider.getApkFromAnyRepoUri(new MockApk("org.fdroid.fdroid", 100)), "content://org.fdroid.fdroid.data.ApkProvider/apk-any-repo/100/org.fdroid.fdroid", projection); - assertValidUri(resolver, ApkProvider.getContentUri(apks), projection); assertValidUri(resolver, ApkProvider.getApkFromAnyRepoUri("org.fdroid.fdroid", 100, null), "content://org.fdroid.fdroid.data.ApkProvider/apk-any-repo/100/org.fdroid.fdroid", projection); assertValidUri(resolver, ApkProvider.getRepoUri(1000), "content://org.fdroid.fdroid.data.ApkProvider/repo/1000", projection); } - - @Test(expected = IllegalArgumentException.class) - public void invalidApkUrisWithTooManyApks() { - String[] projection = Schema.ApkTable.Cols.ALL; - - List manyApks = new ArrayList<>(ApkProvider.MAX_APKS_TO_QUERY - 5); - for (int i = 0; i < ApkProvider.MAX_APKS_TO_QUERY - 1; i++) { - manyApks.add(new MockApk("com.example." + i, i)); - } - assertValidUri(resolver, ApkProvider.getContentUri(manyApks), projection); - - manyApks.add(new MockApk("org.fdroid.fdroid.1", 1)); - manyApks.add(new MockApk("org.fdroid.fdroid.2", 2)); - - // Technically, it is a valid URI, because it doesn't - // throw an UnsupportedOperationException. However it - // is still not okay (we run out of bindable parameters - // in the sqlite query. - assertValidUri(resolver, ApkProvider.getContentUri(manyApks), projection); - } - } From 1bd9a73dbc563b0ec042ef5487cda8cba16d1401 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Mon, 17 Jul 2017 12:45:16 +1000 Subject: [PATCH 4/4] Cache results of category-id query. Each app insert required asking the database for the ID of each category an app is in. Given the categories don't change (ever) but are only appended to, we can cache the results in a static Java variable for increased performance. This reduced the "repo persister" logic for me from 50 seconds for main F-Droid and 100 seconds for Archive, down to 15 seconds and 30 seconds respectively. --- .../org/fdroid/fdroid/data/AppProvider.java | 2 ++ .../fdroid/fdroid/data/CategoryProvider.java | 31 ++++++++++++++++++- 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/org/fdroid/fdroid/data/AppProvider.java b/app/src/main/java/org/fdroid/fdroid/data/AppProvider.java index 36426fc0d..4b7f74c1b 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/AppProvider.java +++ b/app/src/main/java/org/fdroid/fdroid/data/AppProvider.java @@ -24,8 +24,10 @@ import org.fdroid.fdroid.data.Schema.RepoTable; import java.util.ArrayList; import java.util.Arrays; +import java.util.HashMap; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Set; /** diff --git a/app/src/main/java/org/fdroid/fdroid/data/CategoryProvider.java b/app/src/main/java/org/fdroid/fdroid/data/CategoryProvider.java index 6810fdd48..3997d43dd 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/CategoryProvider.java +++ b/app/src/main/java/org/fdroid/fdroid/data/CategoryProvider.java @@ -12,13 +12,39 @@ import org.fdroid.fdroid.data.Schema.CategoryTable; import org.fdroid.fdroid.data.Schema.CategoryTable.Cols; import org.fdroid.fdroid.data.Schema.PackageTable; +import java.util.HashMap; +import java.util.Map; + public class CategoryProvider extends FDroidProvider { public static final class Helper { private Helper() { } + /** + * During repo updates, each app needs to know the ID of each category it belongs to. + * This results in lots of database lookups, usually at least one for each app, sometimes more. + * To improve performance, this caches the association between categories and their database IDs. + * + * It can stay around for the entire F-Droid process, even across multiple repo updates, as we + * don't actually remove data from the categories table. + */ + private static final Map KNOWN_CATEGORIES = new HashMap<>(); + + /** + * Used by tests to clear that the "Category -> ID" cache (used to prevent excessive disk reads). + */ + static void clearCategoryIdCache() { + KNOWN_CATEGORIES.clear(); + } + public static long ensureExists(Context context, String category) { + // Check our in-memory cache to potentially prevent a trip to the database (and hence disk). + String lowerCaseCategory = category.toLowerCase(); + if (KNOWN_CATEGORIES.containsKey(lowerCaseCategory)) { + return KNOWN_CATEGORIES.get(lowerCaseCategory); + } + long id = getCategoryId(context, category); if (id <= 0) { ContentValues values = new ContentValues(1); @@ -26,10 +52,13 @@ public class CategoryProvider extends FDroidProvider { Uri uri = context.getContentResolver().insert(getContentUri(), values); id = Long.parseLong(uri.getLastPathSegment()); } + + KNOWN_CATEGORIES.put(lowerCaseCategory, id); + return id; } - public static long getCategoryId(Context context, String category) { + private static long getCategoryId(Context context, String category) { String[] projection = new String[]{Cols.ROW_ID}; Cursor cursor = context.getContentResolver().query(getCategoryUri(category), projection, null, null, null);