From 2733081b3a75110e1c7846aba72350ed8d72b413 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Thu, 23 Jun 2016 11:30:01 +1000 Subject: [PATCH 01/11] Change join from `app` to `apk` table to auto increment integers rather than Android package name strings. This is important for the ability to refactor the database for better performance in the future. See #511 for details. For those interested in the details, here is a query plan of selecting the "All" category of apps before this commit: * `SCAN TABLE fdroid_app USING INDEX app_id` * `SEARCH TABLE fdroid_apk USING INDEX apk_id (id=?)` * `SEARCH TABLE fdroid_repo USING INTEGER PRIMARY KEY (rowid=?)` * `SEARCH TABLE fdroid_installedApp AS installed USING INDEX sqlite_autoindex_fdroid_installedApp_1 (appId=?)` * `SEARCH TABLE fdroid_apk AS suggestedApk USING INDEX sqlite_autoindex_fdroid_apk_1 (id=? AND vercode=?)` * `USE TEMP B-TREE FOR ORDER BY` And here is a query plan of afterwards: * `SCAN TABLE fdroid_app` * `SEARCH TABLE fdroid_apk USING INDEX apk_appId (appId=?)` * `SEARCH TABLE fdroid_repo USING INTEGER PRIMARY KEY (rowid=?)` * `SEARCH TABLE fdroid_installedApp AS installed USING INDEX sqlite_autoindex_fdroid_installedApp_1 (appId=?)` * `SEARCH TABLE fdroid_apk AS suggestedApk USING INDEX apk_appId (appId=?)` * `USE TEMP B-TREE FOR ORDER BY` The things of note are: * `SCAN TABLE` doesn't use an index, which means that it is really using the rowid index. Shouldn't behave much differently. * The second item now uses an integer primary key index rather than a package name index. Should increase search speed marginally which was the goal of this commit. As more apks exist, the speed improvement will also increase. --- .../main/java/org/fdroid/fdroid/data/Apk.java | 13 ++- .../org/fdroid/fdroid/data/ApkProvider.java | 97 ++++++++++++++----- .../main/java/org/fdroid/fdroid/data/App.java | 11 +++ .../org/fdroid/fdroid/data/AppProvider.java | 51 ++++------ .../java/org/fdroid/fdroid/data/DBHelper.java | 25 ++++- .../org/fdroid/fdroid/data/RepoPersister.java | 38 +++++++- .../java/org/fdroid/fdroid/data/Schema.java | 28 ++++-- .../fdroid/fdroid/data/TempApkProvider.java | 7 +- .../fdroid/fdroid/data/TempAppProvider.java | 37 ++++++- .../test/java/org/fdroid/fdroid/Assert.java | 42 ++++++-- .../java/org/fdroid/fdroid/TestUtils.java | 25 +++++ .../fdroid/fdroid/data/ApkProviderTest.java | 49 +++++----- .../fdroid/data/FDroidProviderTest.java | 9 +- .../fdroid/fdroid/data/ProviderUriTests.java | 22 ++++- 14 files changed, 340 insertions(+), 114 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/data/Apk.java b/app/src/main/java/org/fdroid/fdroid/data/Apk.java index bb9d66df7..745050bd4 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/Apk.java +++ b/app/src/main/java/org/fdroid/fdroid/data/Apk.java @@ -61,6 +61,11 @@ public class Apk extends ValueObject implements Comparable { public String repoAddress; public String[] incompatibleReasons; + /** + * The numeric primary key of the App table, which is used to join apks. + */ + public long appId; + public Apk() { } @@ -74,6 +79,9 @@ public class Apk extends ValueObject implements Comparable { for (int i = 0; i < cursor.getColumnCount(); i++) { switch (cursor.getColumnName(i)) { + case Cols.APP_ID: + appId = cursor.getLong(i); + break; case Cols.HASH: hash = cursor.getString(i); break; @@ -131,10 +139,10 @@ public class Apk extends ValueObject implements Comparable { case Cols.VERSION_CODE: versionCode = cursor.getInt(i); break; - case Cols.REPO_VERSION: + case Cols.Repo.VERSION: repoVersion = cursor.getInt(i); break; - case Cols.REPO_ADDRESS: + case Cols.Repo.ADDRESS: repoAddress = cursor.getString(i); break; } @@ -192,6 +200,7 @@ public class Apk extends ValueObject implements Comparable { public ContentValues toContentValues() { ContentValues values = new ContentValues(); + values.put(Cols.APP_ID, appId); values.put(Cols.PACKAGE_NAME, packageName); values.put(Cols.VERSION_NAME, versionName); values.put(Cols.VERSION_CODE, versionCode); 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 cbb265a74..d777866a7 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/ApkProvider.java +++ b/app/src/main/java/org/fdroid/fdroid/data/ApkProvider.java @@ -11,6 +11,7 @@ import android.util.Log; import org.fdroid.fdroid.Utils; import org.fdroid.fdroid.data.Schema.ApkTable; import org.fdroid.fdroid.data.Schema.ApkTable.Cols; +import org.fdroid.fdroid.data.Schema.AppTable; import org.fdroid.fdroid.data.Schema.RepoTable; import java.util.ArrayList; @@ -135,7 +136,7 @@ public class ApkProvider extends FDroidProvider { String packageName, String[] projection) { ContentResolver resolver = context.getContentResolver(); final Uri uri = getAppUri(packageName); - final String sort = Cols.VERSION_CODE + " DESC"; + final String sort = "apk." + Cols.VERSION_CODE + " DESC"; Cursor cursor = resolver.query(uri, projection, null, null, sort); return cursorToList(cursor); } @@ -213,10 +214,12 @@ public class ApkProvider extends FDroidProvider { private static final UriMatcher MATCHER = new UriMatcher(-1); private static final Map REPO_FIELDS = new HashMap<>(); + private static final Map APP_FIELDS = new HashMap<>(); static { - REPO_FIELDS.put(Cols.REPO_VERSION, RepoTable.Cols.VERSION); - REPO_FIELDS.put(Cols.REPO_ADDRESS, RepoTable.Cols.ADDRESS); + REPO_FIELDS.put(Cols.Repo.VERSION, RepoTable.Cols.VERSION); + REPO_FIELDS.put(Cols.Repo.ADDRESS, RepoTable.Cols.ADDRESS); + APP_FIELDS.put(Cols.App.PACKAGE_NAME, AppTable.Cols.PACKAGE_NAME); MATCHER.addURI(getAuthority(), PATH_REPO + "/#", CODE_REPO); MATCHER.addURI(getAuthority(), PATH_APK + "/#/*", CODE_SINGLE); @@ -323,6 +326,10 @@ public class ApkProvider extends FDroidProvider { return ApkTable.NAME; } + protected String getAppTableName() { + return AppTable.NAME; + } + @Override protected String getProviderName() { return PROVIDER_NAME; @@ -333,30 +340,40 @@ public class ApkProvider extends FDroidProvider { return MATCHER; } - private static class Query extends QueryBuilder { + private class Query extends QueryBuilder { private boolean repoTableRequired; @Override protected String getRequiredTables() { - return ApkTable.NAME + " AS apk"; + final String apk = getTableName(); + final String app = getAppTableName(); + + return apk + " AS apk " + + " LEFT JOIN " + app + " AS app ON (app." + AppTable.Cols.ROW_ID + " = apk." + Cols.APP_ID + ")"; } @Override public void addField(String field) { - if (REPO_FIELDS.containsKey(field)) { + if (APP_FIELDS.containsKey(field)) { + addAppField(APP_FIELDS.get(field), field); + } else if (REPO_FIELDS.containsKey(field)) { addRepoField(REPO_FIELDS.get(field), field); } else if (field.equals(Cols._ID)) { appendField("rowid", "apk", "_id"); } else if (field.equals(Cols._COUNT)) { appendField("COUNT(*) AS " + Cols._COUNT); } else if (field.equals(Cols._COUNT_DISTINCT)) { - appendField("COUNT(DISTINCT apk." + Cols.PACKAGE_NAME + ") AS " + Cols._COUNT_DISTINCT); + appendField("COUNT(DISTINCT apk." + Cols.APP_ID + ") AS " + Cols._COUNT_DISTINCT); } else { appendField(field, "apk"); } } + private void addAppField(String field, String alias) { + appendField(field, "app", alias); + } + private void addRepoField(String field, String alias) { if (!repoTableRequired) { repoTableRequired = true; @@ -368,13 +385,23 @@ public class ApkProvider extends FDroidProvider { } private QuerySelection queryApp(String packageName) { - final String selection = Cols.PACKAGE_NAME + " = ? "; + return queryApp(packageName, true); + } + + private QuerySelection queryApp(String packageName, boolean includeTableAlias) { + String alias = includeTableAlias ? "apk." : ""; + final String selection = alias + Cols.APP_ID + " = (" + getAppIdFromPackageNameQuery() + ")"; final String[] args = {packageName}; return new QuerySelection(selection, args); } private QuerySelection querySingle(Uri uri) { - final String selection = Cols.VERSION_CODE + " = ? and " + Cols.PACKAGE_NAME + " = ? "; + return querySingle(uri, true); + } + + private QuerySelection querySingle(Uri uri, boolean includeAlias) { + String alias = includeAlias ? "apk." : ""; + final String selection = " " + alias + Cols.VERSION_CODE + " = ? and " + alias + Cols.PACKAGE_NAME + " = ? "; final String[] args = { // First (0th) path segment is the word "apk", // and we are not interested in it. @@ -385,22 +412,32 @@ public class ApkProvider extends FDroidProvider { } protected QuerySelection queryRepo(long repoId) { - final String selection = Cols.REPO_ID + " = ? "; + return queryRepo(repoId, true); + } + + protected QuerySelection queryRepo(long repoId, boolean includeAlias) { + String alias = includeAlias ? "apk." : ""; + final String selection = alias + Cols.REPO_ID + " = ? "; final String[] args = {Long.toString(repoId)}; return new QuerySelection(selection, args); } private QuerySelection queryRepoApps(long repoId, String packageNames) { - return queryRepo(repoId).add(AppProvider.queryApps(packageNames, Cols.PACKAGE_NAME)); + return queryRepo(repoId).add(AppProvider.queryApps(packageNames, "app." + AppTable.Cols.PACKAGE_NAME)); } protected QuerySelection queryApks(String apkKeys) { + return queryApks(apkKeys, true); + } + + protected QuerySelection queryApks(String apkKeys, boolean includeAlias) { final String[] apkDetails = apkKeys.split(","); if (apkDetails.length > MAX_APKS_TO_QUERY) { throw new IllegalArgumentException( "Cannot query more than " + MAX_APKS_TO_QUERY + ". " + "You tried to query " + apkDetails.length); } + String alias = includeAlias ? "apk." : ""; final String[] args = new String[apkDetails.length * 2]; StringBuilder sb = new StringBuilder(); for (int i = 0; i < apkDetails.length; i++) { @@ -412,11 +449,23 @@ public class ApkProvider extends FDroidProvider { if (i != 0) { sb.append(" OR "); } - sb.append(" ( " + Cols.PACKAGE_NAME + " = ? AND " + Cols.VERSION_CODE + " = ? ) "); + sb.append(" ( ") + .append(alias) + .append(Cols.APP_ID) + .append(" = (") + .append(getAppIdFromPackageNameQuery()) + .append(") AND ") + .append(alias) + .append(Cols.VERSION_CODE) + .append(" = ? ) "); } return new QuerySelection(sb.toString(), args); } + private String getAppIdFromPackageNameQuery() { + return "SELECT " + AppTable.Cols.ROW_ID + " FROM " + getAppTableName() + " WHERE " + AppTable.Cols.PACKAGE_NAME + " = ?"; + } + @Override public Cursor query(Uri uri, String[] projection, String selection, String[] selectionArgs, String sortOrder) { @@ -464,13 +513,17 @@ public class ApkProvider extends FDroidProvider { return cursor; } - private static void removeRepoFields(ContentValues values) { + private static void removeFieldsFromOtherTables(ContentValues values) { for (Map.Entry repoField : REPO_FIELDS.entrySet()) { final String field = repoField.getKey(); if (values.containsKey(field)) { - Utils.debugLog(TAG, "Cannot insert/update '" + field + "' field " + - "on apk table, as it belongs to the repo table. " + - "This field will be ignored."); + values.remove(field); + } + } + + for (Map.Entry appField : APP_FIELDS.entrySet()) { + final String field = appField.getKey(); + if (values.containsKey(field)) { values.remove(field); } } @@ -478,7 +531,7 @@ public class ApkProvider extends FDroidProvider { @Override public Uri insert(Uri uri, ContentValues values) { - removeRepoFields(values); + removeFieldsFromOtherTables(values); validateFields(Cols.ALL, values); db().insertOrThrow(getTableName(), null, values); if (!isApplyingBatch()) { @@ -498,15 +551,15 @@ public class ApkProvider extends FDroidProvider { switch (MATCHER.match(uri)) { case CODE_REPO: - query = query.add(queryRepo(Long.parseLong(uri.getLastPathSegment()))); + query = query.add(queryRepo(Long.parseLong(uri.getLastPathSegment()), false)); break; case CODE_APP: - query = query.add(queryApp(uri.getLastPathSegment())); + query = query.add(queryApp(uri.getLastPathSegment(), false)); break; case CODE_APKS: - query = query.add(queryApks(uri.getLastPathSegment())); + query = query.add(queryApks(uri.getLastPathSegment(), false)); break; // TODO: Add tests for this. @@ -542,10 +595,10 @@ public class ApkProvider extends FDroidProvider { protected int performUpdateUnchecked(Uri uri, ContentValues values, String where, String[] whereArgs) { validateFields(Cols.ALL, values); - removeRepoFields(values); + removeFieldsFromOtherTables(values); QuerySelection query = new QuerySelection(where, whereArgs); - query = query.add(querySingle(uri)); + query = query.add(querySingle(uri, false)); int numRows = db().update(getTableName(), values, query.getSelection(), query.getArgs()); if (!isApplyingBatch()) { diff --git a/app/src/main/java/org/fdroid/fdroid/data/App.java b/app/src/main/java/org/fdroid/fdroid/data/App.java index c9971f2b1..a071393b5 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/App.java +++ b/app/src/main/java/org/fdroid/fdroid/data/App.java @@ -130,6 +130,8 @@ public class App extends ValueObject implements Comparable { public String installedSig; + private long id; + public static String getIconName(String packageName, int versionCode) { return packageName + "_" + versionCode + ".png"; } @@ -153,6 +155,9 @@ public class App extends ValueObject implements Comparable { for (int i = 0; i < cursor.getColumnCount(); i++) { String n = cursor.getColumnName(i); switch (n) { + case Cols.ROW_ID: + id = cursor.getLong(i); + break; case Cols.IS_COMPATIBLE: compatible = cursor.getInt(i) == 1; break; @@ -440,6 +445,8 @@ public class App extends ValueObject implements Comparable { public ContentValues toContentValues() { final ContentValues values = new ContentValues(); + // Intentionally don't put "ROW_ID" in here, because we don't ever want to change that + // primary key generated by sqlite. values.put(Cols.PACKAGE_NAME, packageName); values.put(Cols.NAME, name); values.put(Cols.SUMMARY, summary); @@ -549,4 +556,8 @@ public class App extends ValueObject implements Comparable { } return new int[]{minSdkVersion, targetSdkVersion, maxSdkVersion}; } + + public long getId() { + return id; + } } 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 f16999bc8..1eab64e1b 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/AppProvider.java +++ b/app/src/main/java/org/fdroid/fdroid/data/AppProvider.java @@ -64,7 +64,7 @@ public class AppProvider extends FDroidProvider { return cursorToList(cursor); } - private static List cursorToList(Cursor cursor) { + static List cursorToList(Cursor cursor) { int knownAppCount = cursor != null ? cursor.getCount() : 0; List apps = new ArrayList<>(knownAppCount); if (cursor != null) { @@ -153,7 +153,6 @@ public class AppProvider extends FDroidProvider { final Uri fromUpstream = calcAppDetailsFromIndexUri(); context.getContentResolver().update(fromUpstream, null, null, null); } - } /** @@ -184,7 +183,7 @@ public class AppProvider extends FDroidProvider { * method from this class will return an instance of this class, that is aware of * the install apps table. */ - private static class AppQuerySelection extends QuerySelection { + protected static class AppQuerySelection extends QuerySelection { private boolean naturalJoinToInstalled; @@ -247,7 +246,7 @@ public class AppProvider extends FDroidProvider { final String repo = RepoTable.NAME; return app + - " LEFT JOIN " + apk + " ON (" + apk + "." + ApkTable.Cols.PACKAGE_NAME + " = " + app + "." + Cols.PACKAGE_NAME + ") " + + " LEFT JOIN " + apk + " ON (" + apk + "." + ApkTable.Cols.APP_ID + " = " + app + "." + Cols.ROW_ID + ") " + " LEFT JOIN " + repo + " ON (" + apk + "." + ApkTable.Cols.REPO_ID + " = " + repo + "." + RepoTable.Cols._ID + ") "; } @@ -259,7 +258,7 @@ public class AppProvider extends FDroidProvider { @Override protected String groupBy() { // If the count field has been requested, then we want to group all rows together. - return countFieldAppended ? null : getTableName() + "." + Cols.PACKAGE_NAME; + return countFieldAppended ? null : getTableName() + "." + Cols.ROW_ID; } public void addSelection(AppQuerySelection selection) { @@ -320,7 +319,7 @@ public class AppProvider extends FDroidProvider { private void appendCountField() { countFieldAppended = true; - appendField("COUNT( DISTINCT " + getTableName() + "." + Cols.PACKAGE_NAME + " ) AS " + Cols._COUNT); + appendField("COUNT( DISTINCT " + getTableName() + "." + Cols.ROW_ID + " ) AS " + Cols._COUNT); } private void addSuggestedApkVersionField() { @@ -335,7 +334,7 @@ public class AppProvider extends FDroidProvider { leftJoin( getApkTableName(), "suggestedApk", - getTableName() + "." + Cols.SUGGESTED_VERSION_CODE + " = suggestedApk." + ApkTable.Cols.VERSION_CODE + " AND " + getTableName() + "." + Cols.PACKAGE_NAME + " = suggestedApk." + ApkTable.Cols.PACKAGE_NAME); + getTableName() + "." + Cols.SUGGESTED_VERSION_CODE + " = suggestedApk." + ApkTable.Cols.VERSION_CODE + " AND " + getTableName() + "." + Cols.ROW_ID + " = suggestedApk." + ApkTable.Cols.APP_ID); } appendField(fieldName, "suggestedApk", alias); } @@ -378,7 +377,7 @@ public class AppProvider extends FDroidProvider { private static final String PATH_SEARCH_CAN_UPDATE = "searchCanUpdate"; private static final String PATH_SEARCH_REPO = "searchRepo"; private static final String PATH_NO_APKS = "noApks"; - private static final String PATH_APPS = "apps"; + protected static final String PATH_APPS = "apps"; private static final String PATH_RECENTLY_UPDATED = "recentlyUpdated"; private static final String PATH_NEWLY_ADDED = "newlyAdded"; private static final String PATH_CATEGORY = "category"; @@ -390,8 +389,7 @@ public class AppProvider extends FDroidProvider { private static final int INSTALLED = CAN_UPDATE + 1; private static final int SEARCH = INSTALLED + 1; private static final int NO_APKS = SEARCH + 1; - private static final int APPS = NO_APKS + 1; - private static final int RECENTLY_UPDATED = APPS + 1; + private static final int RECENTLY_UPDATED = NO_APKS + 1; private static final int NEWLY_ADDED = RECENTLY_UPDATED + 1; private static final int CATEGORY = NEWLY_ADDED + 1; private static final int IGNORED = CATEGORY + 1; @@ -416,7 +414,6 @@ public class AppProvider extends FDroidProvider { MATCHER.addURI(getAuthority(), PATH_CAN_UPDATE, CAN_UPDATE); MATCHER.addURI(getAuthority(), PATH_INSTALLED, INSTALLED); MATCHER.addURI(getAuthority(), PATH_NO_APKS, NO_APKS); - MATCHER.addURI(getAuthority(), PATH_APPS + "/*", APPS); MATCHER.addURI(getAuthority(), "*", CODE_SINGLE); } @@ -466,20 +463,6 @@ public class AppProvider extends FDroidProvider { .build(); } - public static Uri getContentUri(List apps) { - StringBuilder builder = new StringBuilder(); - for (int i = 0; i < apps.size(); i++) { - if (i != 0) { - builder.append(','); - } - builder.append(apps.get(i).packageName); - } - return getContentUri().buildUpon() - .appendPath(PATH_APPS) - .appendPath(builder.toString()) - .build(); - } - public static Uri getContentUri(App app) { return getContentUri(app.packageName); } @@ -684,10 +667,6 @@ public class AppProvider extends FDroidProvider { return new AppQuerySelection(selection, args); } - private AppQuerySelection queryApps(String packageNames) { - return queryApps(packageNames, getTableName() + "." + Cols.PACKAGE_NAME); - } - @Override public Cursor query(Uri uri, String[] projection, String customSelection, String[] selectionArgs, String sortOrder) { AppQuerySelection selection = new AppQuerySelection(customSelection, selectionArgs); @@ -742,10 +721,6 @@ public class AppProvider extends FDroidProvider { selection = selection.add(queryNoApks()); break; - case APPS: - selection = selection.add(queryApps(uri.getLastPathSegment())); - break; - case IGNORED: selection = selection.add(queryIgnored()); break; @@ -772,6 +747,14 @@ public class AppProvider extends FDroidProvider { throw new UnsupportedOperationException("Invalid URI for app content provider: " + uri); } + return runQuery(uri, selection, projection, includeSwap, sortOrder); + } + + /** + * Helper method used by both the genuine {@link AppProvider} and the temporary version used + * by the repo updater ({@link TempAppProvider}). + */ + protected Cursor runQuery(Uri uri, AppQuerySelection selection, String[] projection, boolean includeSwap, String sortOrder) { if (!includeSwap) { selection = selection.add(queryExcludeSwap()); } @@ -973,7 +956,7 @@ public class AppProvider extends FDroidProvider { apk + " JOIN " + repo + " ON (" + repo + "." + RepoTable.Cols._ID + " = " + apk + "." + ApkTable.Cols.REPO_ID + ") " + " WHERE " + - app + "." + Cols.PACKAGE_NAME + " = " + apk + "." + ApkTable.Cols.PACKAGE_NAME + " AND " + + app + "." + Cols.ROW_ID + " = " + apk + "." + ApkTable.Cols.APP_ID + " AND " + apk + "." + ApkTable.Cols.VERSION_CODE + " = " + app + "." + Cols.SUGGESTED_VERSION_CODE; return "UPDATE " + app + " SET " 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 3b682064e..c688ee01a 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/DBHelper.java +++ b/app/src/main/java/org/fdroid/fdroid/data/DBHelper.java @@ -47,6 +47,7 @@ class DBHelper extends SQLiteOpenHelper { private static final String CREATE_TABLE_APK = "CREATE TABLE " + ApkTable.NAME + " ( " + ApkTable.Cols.PACKAGE_NAME + " text not null, " + + ApkTable.Cols.APP_ID + " text not null, " + ApkTable.Cols.VERSION_NAME + " text not null, " + ApkTable.Cols.REPO_ID + " integer not null, " + ApkTable.Cols.HASH + " text not null, " @@ -114,7 +115,7 @@ class DBHelper extends SQLiteOpenHelper { + " );"; private static final String DROP_TABLE_INSTALLED_APP = "DROP TABLE " + InstalledAppTable.NAME + ";"; - private static final int DB_VERSION = 57; + private static final int DB_VERSION = 58; private final Context context; @@ -315,8 +316,29 @@ class DBHelper extends SQLiteOpenHelper { requireTimestampInRepos(db, oldVersion); recreateInstalledAppTable(db, oldVersion); addTargetSdkVersionToApk(db, oldVersion); + migrateAppPrimaryKeyToRowId(db, oldVersion); } + private void migrateAppPrimaryKeyToRowId(SQLiteDatabase db, int oldVersion) { + if (oldVersion < 58) { + final String alter = "ALTER TABLE " + ApkTable.NAME + " ADD COLUMN appId NUMERIC"; + Log.i(TAG, "Adding appId foreign key to fdroid_apk."); + Utils.debugLog(TAG, alter); + db.execSQL(alter); + + final String update = + "UPDATE " + ApkTable.NAME + " SET appId = ( " + + "SELECT app.rowid " + + "FROM " + ApkTable.NAME + " AS app " + + "WHERE " + ApkTable.NAME + ".id = app.id " + + ")"; + Log.i(TAG, "Updating foreign key from fdroid_apk to fdroid_app to use numeric foreign key."); + Utils.debugLog(TAG, update); + db.execSQL(update); + } + } + + /** * Migrate repo list to new structure. (No way to change primary * key in sqlite - table must be recreated). @@ -570,6 +592,7 @@ class DBHelper extends SQLiteOpenHelper { db.execSQL("create index app_id on " + AppTable.NAME + " (" + AppTable.Cols.PACKAGE_NAME + ");"); db.execSQL(CREATE_TABLE_APK); db.execSQL("create index apk_vercode on " + ApkTable.NAME + " (" + ApkTable.Cols.VERSION_CODE + ");"); + db.execSQL("create index apk_appId on " + ApkTable.NAME + " (" + ApkTable.Cols.APP_ID + ");"); db.execSQL("create index apk_id on " + ApkTable.NAME + " (" + AppTable.Cols.PACKAGE_NAME + ");"); } 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 ff34ffe53..98fdd849a 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/RepoPersister.java +++ b/app/src/main/java/org/fdroid/fdroid/data/RepoPersister.java @@ -98,16 +98,19 @@ 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."); - flushAppsToDbInBatch(); - flushApksToDbInBatch(); + Map appIds = flushAppsToDbInBatch(); + flushApksToDbInBatch(appIds); apksToSave.clear(); appsToSave.clear(); } } - private void flushApksToDbInBatch() throws RepoUpdater.UpdateException { + private void flushApksToDbInBatch(Map appIds) throws RepoUpdater.UpdateException { List apksToSaveList = new ArrayList<>(); for (Map.Entry> entries : apksToSave.entrySet()) { + for (Apk apk : entries.getValue()) { + apk.appId = appIds.get(apk.packageName); + } apksToSaveList.addAll(entries.getValue()); } @@ -127,16 +130,43 @@ public class RepoPersister { } } - private void flushAppsToDbInBatch() throws RepoUpdater.UpdateException { + /** + * Will first insert new or update existing rows in the database for each {@link RepoPersister#appsToSave}. + * Then, will query the database for the ID + packageName for each of these apps, so that they + * 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); try { context.getContentResolver().applyBatch(TempAppProvider.getAuthority(), appOperations); + return getIdsForPackages(appsToSave); } catch (RemoteException | OperationApplicationException e) { throw new RepoUpdater.UpdateException(repo, "An internal error occurred while updating the database", e); } } + /** + * Although this might seem counter intuitive - receiving a list of apps, then querying the + * database again for info about these apps, it is required because the apps came from the + * repo metadata, but we are really interested in their IDs from the database. These IDs only + * exist in SQLite and not the repo metadata. + */ + private Map getIdsForPackages(List apps) { + List packageNames = new ArrayList<>(appsToSave.size()); + for (App app : apps) { + packageNames.add(app.packageName); + } + String[] projection = {Schema.AppTable.Cols.ROW_ID, Schema.AppTable.Cols.PACKAGE_NAME}; + List fromDb = TempAppProvider.Helper.findByPackageNames(context, packageNames, projection); + + Map ids = new HashMap<>(fromDb.size()); + for (App app : fromDb) { + ids.put(app.packageName, app.getId()); + } + 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. 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 0d6571794..d9c12d7f0 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/Schema.java +++ b/app/src/main/java/org/fdroid/fdroid/data/Schema.java @@ -14,7 +14,12 @@ public interface Schema { String NAME = "fdroid_app"; interface Cols { - String _ID = "rowid as _id"; // Required for CursorLoaders + /** + * Same as the primary key {@link Cols#ROW_ID}, except aliased as "_id" instead + * of "rowid". Required for {@link android.content.CursorLoader}s. + */ + String _ID = "rowid as _id"; + String ROW_ID = "rowid"; String _COUNT = "_count"; String IS_COMPATIBLE = "compatible"; String PACKAGE_NAME = "id"; @@ -57,7 +62,7 @@ public interface Schema { } String[] ALL = { - _ID, IS_COMPATIBLE, PACKAGE_NAME, NAME, SUMMARY, ICON, DESCRIPTION, + _ID, 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, @@ -82,6 +87,10 @@ public interface Schema { interface Cols extends BaseColumns { String _COUNT_DISTINCT = "countDistinct"; + /** + * Foreign key to the {@link AppTable}. + */ + String APP_ID = "appId"; String PACKAGE_NAME = "id"; String VERSION_NAME = "version"; String REPO_ID = "repo"; @@ -101,14 +110,21 @@ public interface Schema { String ADDED_DATE = "added"; String IS_COMPATIBLE = "compatible"; String INCOMPATIBLE_REASONS = "incompatibleReasons"; - String REPO_VERSION = "repoVersion"; - String REPO_ADDRESS = "repoAddress"; + + interface Repo { + String VERSION = "repoVersion"; + String ADDRESS = "repoAddress"; + } + + interface App { + String PACKAGE_NAME = "appPackageName"; + } String[] ALL = { - _ID, PACKAGE_NAME, VERSION_NAME, REPO_ID, HASH, VERSION_CODE, NAME, + _ID, APP_ID, PACKAGE_NAME, 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, REPO_VERSION, REPO_ADDRESS, INCOMPATIBLE_REASONS, + IS_COMPATIBLE, Repo.VERSION, Repo.ADDRESS, INCOMPATIBLE_REASONS, }; } } 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 0033c353e..3f0335b69 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/TempApkProvider.java +++ b/app/src/main/java/org/fdroid/fdroid/data/TempApkProvider.java @@ -39,6 +39,11 @@ public class TempApkProvider extends ApkProvider { return TABLE_TEMP_APK; } + @Override + protected String getAppTableName() { + return TempAppProvider.TABLE_TEMP_APP; + } + public static String getAuthority() { return AUTHORITY + "." + PROVIDER_NAME; } @@ -111,7 +116,7 @@ public class TempApkProvider extends ApkProvider { switch (MATCHER.match(uri)) { case CODE_REPO_APK: List pathSegments = uri.getPathSegments(); - query = query.add(queryRepo(Long.parseLong(pathSegments.get(1)))).add(queryApks(pathSegments.get(2))); + query = query.add(queryRepo(Long.parseLong(pathSegments.get(1)), false)).add(queryApks(pathSegments.get(2), false)); break; default: 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 3290dfb88..e3907c480 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/TempAppProvider.java +++ b/app/src/main/java/org/fdroid/fdroid/data/TempAppProvider.java @@ -3,9 +3,13 @@ package org.fdroid.fdroid.data; import android.content.ContentValues; import android.content.Context; import android.content.UriMatcher; +import android.database.Cursor; import android.database.sqlite.SQLiteDatabase; import android.database.sqlite.SQLiteException; import android.net.Uri; +import android.text.TextUtils; + +import java.util.List; import org.fdroid.fdroid.data.Schema.ApkTable; import org.fdroid.fdroid.data.Schema.AppTable; @@ -22,19 +26,21 @@ public class TempAppProvider extends AppProvider { private static final String PROVIDER_NAME = "TempAppProvider"; - private static final String TABLE_TEMP_APP = "temp_" + AppTable.NAME; + static final String TABLE_TEMP_APP = "temp_" + AppTable.NAME; private static final String PATH_INIT = "init"; private static final String PATH_COMMIT = "commit"; private static final int CODE_INIT = 10000; private static final int CODE_COMMIT = CODE_INIT + 1; + private static final int APPS = CODE_COMMIT + 1; 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_APPS + "/*", APPS); MATCHER.addURI(getAuthority(), "*", CODE_SINGLE); } @@ -55,6 +61,17 @@ public class TempAppProvider extends AppProvider { return Uri.withAppendedPath(getContentUri(), app.packageName); } + public static Uri getAppsUri(List apps) { + return getContentUri().buildUpon() + .appendPath(PATH_APPS) + .appendPath(TextUtils.join(",", apps)) + .build(); + } + + private AppQuerySelection queryApps(String packageNames) { + return queryApps(packageNames, getTableName() + ".id"); + } + public static class Helper { /** @@ -67,6 +84,12 @@ public class TempAppProvider extends AppProvider { TempApkProvider.Helper.init(context); } + public static List findByPackageNames(Context context, List packageNames, String[] projection) { + Uri uri = getAppsUri(packageNames); + Cursor cursor = context.getContentResolver().query(uri, projection, null, null, null); + return AppProvider.Helper.cursorToList(cursor); + } + /** * 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. @@ -116,6 +139,18 @@ public class TempAppProvider extends AppProvider { return count; } + @Override + public Cursor query(Uri uri, String[] projection, String customSelection, String[] selectionArgs, String sortOrder) { + AppQuerySelection selection = new AppQuerySelection(customSelection, selectionArgs); + switch (MATCHER.match(uri)) { + case APPS: + selection = selection.add(queryApps(uri.getLastPathSegment())); + break; + } + + return super.runQuery(uri, selection, projection, true, sortOrder); + } + private void ensureTempTableDetached(SQLiteDatabase db) { try { db.execSQL("DETACH DATABASE " + DB); diff --git a/app/src/test/java/org/fdroid/fdroid/Assert.java b/app/src/test/java/org/fdroid/fdroid/Assert.java index cb6354b1f..97d3dff7c 100644 --- a/app/src/test/java/org/fdroid/fdroid/Assert.java +++ b/app/src/test/java/org/fdroid/fdroid/Assert.java @@ -1,12 +1,14 @@ package org.fdroid.fdroid; import android.content.ContentValues; +import android.content.Context; import android.database.Cursor; import android.net.Uri; import junit.framework.AssertionFailedError; import org.fdroid.fdroid.data.ApkProvider; +import org.fdroid.fdroid.data.App; import org.fdroid.fdroid.data.AppProvider; import org.fdroid.fdroid.data.InstalledAppProvider; import org.fdroid.fdroid.data.Schema.ApkTable; @@ -174,14 +176,14 @@ public class Assert { cursor.close(); } - public static void insertApp(ShadowContentResolver resolver, String appId, String name) { - insertApp(resolver, appId, name, new ContentValues()); + public static App insertApp(Context context, String packageName, String name) { + return insertApp(context, packageName, name, new ContentValues()); } - public static void insertApp(ShadowContentResolver resolver, String id, String name, ContentValues additionalValues) { + public static App insertApp(Context context, String packageName, String name, ContentValues additionalValues) { ContentValues values = new ContentValues(); - values.put(AppTable.Cols.PACKAGE_NAME, id); + values.put(AppTable.Cols.PACKAGE_NAME, packageName); values.put(AppTable.Cols.NAME, name); // Required fields (NOT NULL in the database). @@ -196,18 +198,38 @@ public class Assert { Uri uri = AppProvider.getContentUri(); - resolver.insert(uri, values); + context.getContentResolver().insert(uri, values); + return AppProvider.Helper.findByPackageName(context.getContentResolver(), packageName); } - public static Uri insertApk(ShadowContentResolver resolver, String id, int versionCode) { - return insertApk(resolver, id, versionCode, new ContentValues()); + private static App ensureApp(Context context, String packageName) { + App app = AppProvider.Helper.findByPackageName(context.getContentResolver(), packageName); + if (app == null) { + insertApp(context, packageName, packageName); + app = AppProvider.Helper.findByPackageName(context.getContentResolver(), packageName); + } + assertNotNull(app); + return app; } - public static Uri insertApk(ShadowContentResolver resolver, String id, int versionCode, ContentValues additionalValues) { + public static Uri insertApk(Context context, String packageName, int versionCode) { + return insertApk(context, ensureApp(context, packageName), versionCode); + } + + public static Uri insertApk(Context context, String packageName, int versionCode, ContentValues additionalValues) { + return insertApk(context, ensureApp(context, packageName), versionCode, additionalValues); + } + + public static Uri insertApk(Context context, App app, int versionCode) { + return insertApk(context, app, versionCode, new ContentValues()); + } + + public static Uri insertApk(Context context, App app, int versionCode, ContentValues additionalValues) { ContentValues values = new ContentValues(); - values.put(ApkTable.Cols.PACKAGE_NAME, id); + values.put(ApkTable.Cols.APP_ID, app.getId()); + values.put(ApkTable.Cols.PACKAGE_NAME, app.packageName); values.put(ApkTable.Cols.VERSION_CODE, versionCode); // Required fields (NOT NULL in the database). @@ -222,7 +244,7 @@ public class Assert { Uri uri = ApkProvider.getContentUri(); - return resolver.insert(uri, values); + return context.getContentResolver().insert(uri, values); } } diff --git a/app/src/test/java/org/fdroid/fdroid/TestUtils.java b/app/src/test/java/org/fdroid/fdroid/TestUtils.java index 2ced7a025..2cc05800c 100644 --- a/app/src/test/java/org/fdroid/fdroid/TestUtils.java +++ b/app/src/test/java/org/fdroid/fdroid/TestUtils.java @@ -1,5 +1,12 @@ package org.fdroid.fdroid; +import android.content.ContentResolver; +import android.content.ContextWrapper; + +import org.mockito.AdditionalAnswers; +import org.robolectric.RuntimeEnvironment; +import org.robolectric.shadows.ShadowContentResolver; + import java.io.File; import java.io.FileOutputStream; import java.io.IOException; @@ -8,6 +15,7 @@ import java.io.OutputStream; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import static org.mockito.Mockito.mock; public class TestUtils { @@ -37,4 +45,21 @@ public class TestUtils { return tempFile; } + /** + * The way that Robolectric has to implement shadows for Android classes such as {@link android.content.ContentProvider} + * is by using a special annotation that means the classes will implement the correct methods at runtime. + * However this means that the shadow of a content provider does not actually extend + * {@link android.content.ContentProvider}. As such, we need to do some special mocking using + * Mockito in order to provide a {@link ContextWrapper} which is able to return a proper + * content resolver that delegates to the Robolectric shadow object. + */ + public static ContextWrapper createContextWithContentResolver(ShadowContentResolver contentResolver) { + final ContentResolver resolver = mock(ContentResolver.class, AdditionalAnswers.delegatesTo(contentResolver)); + return new ContextWrapper(RuntimeEnvironment.application.getApplicationContext()) { + @Override + public ContentResolver getContentResolver() { + return resolver; + } + }; + } } 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 089706aab..20d9b609c 100644 --- a/app/src/test/java/org/fdroid/fdroid/data/ApkProviderTest.java +++ b/app/src/test/java/org/fdroid/fdroid/data/ApkProviderTest.java @@ -25,6 +25,7 @@ import java.util.List; import static org.fdroid.fdroid.Assert.assertCantDelete; import static org.fdroid.fdroid.Assert.assertContainsOnly; import static org.fdroid.fdroid.Assert.assertResultCount; +import static org.fdroid.fdroid.Assert.insertApp; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; @@ -38,9 +39,11 @@ public class ApkProviderTest extends FDroidProviderTest { @Test public void testAppApks() { + App fdroidApp = insertApp(context, "org.fdroid.fdroid", "F-Droid"); + App exampleApp = insertApp(context, "com.example", "Example"); for (int i = 1; i <= 10; i++) { - Assert.insertApk(contentResolver, "org.fdroid.fdroid", i); - Assert.insertApk(contentResolver, "com.example", i); + Assert.insertApk(context, fdroidApp, i); + Assert.insertApk(context, exampleApp, i); } assertTotalApkCount(20); @@ -188,7 +191,7 @@ public class ApkProviderTest extends FDroidProviderTest { Apk apk = new MockApk("org.fdroid.fdroid", 13); // Insert a new record... - Uri newUri = Assert.insertApk(contentResolver, apk.packageName, apk.versionCode); + Uri newUri = Assert.insertApk(context, apk.packageName, apk.versionCode); assertEquals(ApkProvider.getContentUri(apk).toString(), newUri.toString()); cursor = queryAllApks(); assertNotNull(cursor); @@ -206,7 +209,7 @@ public class ApkProviderTest extends FDroidProviderTest { @Test(expected = IllegalArgumentException.class) public void testCursorMustMoveToFirst() { - Assert.insertApk(contentResolver, "org.example.test", 12); + Assert.insertApk(context, "org.example.test", 12); Cursor cursor = queryAllApks(); new Apk(cursor); } @@ -216,7 +219,7 @@ public class ApkProviderTest extends FDroidProviderTest { String[] projectionCount = new String[] {Cols._COUNT}; for (int i = 0; i < 13; i++) { - Assert.insertApk(contentResolver, "com.example", i); + Assert.insertApk(context, "com.example", i); } Uri all = ApkProvider.getContentUri(); @@ -261,7 +264,7 @@ public class ApkProviderTest extends FDroidProviderTest { public void assertInvalidExtraField(String field) { ContentValues invalidRepo = new ContentValues(); invalidRepo.put(field, "Test data"); - Assert.insertApk(contentResolver, "org.fdroid.fdroid", 10, invalidRepo); + Assert.insertApk(context, "org.fdroid.fdroid", 10, invalidRepo); } @Test @@ -271,10 +274,10 @@ public class ApkProviderTest extends FDroidProviderTest { ContentValues values = new ContentValues(); values.put(Cols.REPO_ID, 10); - values.put(Cols.REPO_ADDRESS, "http://example.com"); - values.put(Cols.REPO_VERSION, 3); + values.put(Cols.Repo.ADDRESS, "http://example.com"); + values.put(Cols.Repo.VERSION, 3); values.put(Cols.FEATURES, "Some features"); - Uri uri = Assert.insertApk(contentResolver, "com.example.com", 1, values); + Uri uri = Assert.insertApk(context, "com.example.com", 1, values); assertResultCount(1, queryAllApks()); @@ -301,18 +304,18 @@ public class ApkProviderTest extends FDroidProviderTest { public void testKnownApks() { for (int i = 0; i < 7; i++) { - Assert.insertApk(contentResolver, "org.fdroid.fdroid", i); + Assert.insertApk(context, "org.fdroid.fdroid", i); } for (int i = 0; i < 9; i++) { - Assert.insertApk(contentResolver, "org.example", i); + Assert.insertApk(context, "org.example", i); } for (int i = 0; i < 3; i++) { - Assert.insertApk(contentResolver, "com.example", i); + Assert.insertApk(context, "com.example", i); } - Assert.insertApk(contentResolver, "com.apk.thingo", 1); + Assert.insertApk(context, "com.apk.thingo", 1); Apk[] known = { new MockApk("org.fdroid.fdroid", 1), @@ -359,18 +362,18 @@ public class ApkProviderTest extends FDroidProviderTest { public void testFindByApp() { for (int i = 0; i < 7; i++) { - Assert.insertApk(contentResolver, "org.fdroid.fdroid", i); + Assert.insertApk(context, "org.fdroid.fdroid", i); } for (int i = 0; i < 9; i++) { - Assert.insertApk(contentResolver, "org.example", i); + Assert.insertApk(context, "org.example", i); } for (int i = 0; i < 3; i++) { - Assert.insertApk(contentResolver, "com.example", i); + Assert.insertApk(context, "com.example", i); } - Assert.insertApk(contentResolver, "com.apk.thingo", 1); + Assert.insertApk(context, "com.apk.thingo", 1); assertTotalApkCount(7 + 9 + 3 + 1); @@ -394,7 +397,7 @@ public class ApkProviderTest extends FDroidProviderTest { @Test public void testUpdate() { - Uri apkUri = Assert.insertApk(contentResolver, "com.example", 10); + Uri apkUri = Assert.insertApk(context, "com.example", 10); String[] allFields = Cols.ALL; Cursor cursor = contentResolver.query(apkUri, allFields, null, null, null); @@ -453,18 +456,20 @@ public class ApkProviderTest extends FDroidProviderTest { // the Helper.find() method doesn't stumble upon the app we are interested // in by shear dumb luck... for (int i = 0; i < 10; i++) { - Assert.insertApk(contentResolver, "org.fdroid.apk." + i, i); + Assert.insertApk(context, "org.fdroid.apk." + i, i); } + App exampleApp = insertApp(context, "com.example", "Example"); + ContentValues values = new ContentValues(); values.put(Cols.VERSION_NAME, "v1.1"); values.put(Cols.HASH, "xxxxyyyy"); values.put(Cols.HASH_TYPE, "a hash type"); - Assert.insertApk(contentResolver, "com.example", 11, values); + Assert.insertApk(context, exampleApp, 11, values); // ...and a few more for good measure... for (int i = 15; i < 20; i++) { - Assert.insertApk(contentResolver, "com.other.thing." + i, i); + Assert.insertApk(context, "com.other.thing." + i, i); } Apk apk = ApkProvider.Helper.find(context, "com.example", 11); @@ -540,7 +545,7 @@ public class ApkProviderTest extends FDroidProviderTest { protected Apk insertApkForRepo(String id, int versionCode, long repoId) { ContentValues additionalValues = new ContentValues(); additionalValues.put(Cols.REPO_ID, repoId); - Uri uri = Assert.insertApk(contentResolver, id, versionCode, additionalValues); + Uri uri = Assert.insertApk(context, id, versionCode, additionalValues); return ApkProvider.Helper.get(context, uri); } } 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 adddf695c..713992d4b 100644 --- a/app/src/test/java/org/fdroid/fdroid/data/FDroidProviderTest.java +++ b/app/src/test/java/org/fdroid/fdroid/data/FDroidProviderTest.java @@ -3,6 +3,7 @@ package org.fdroid.fdroid.data; import android.content.ContentResolver; import android.content.ContextWrapper; +import org.fdroid.fdroid.TestUtils; import org.junit.After; import org.junit.Before; import org.mockito.AdditionalAnswers; @@ -20,13 +21,7 @@ public abstract class FDroidProviderTest { @Before public final void setupBase() { contentResolver = Shadows.shadowOf(RuntimeEnvironment.application.getContentResolver()); - final ContentResolver resolver = mock(ContentResolver.class, AdditionalAnswers.delegatesTo(contentResolver)); - context = new ContextWrapper(RuntimeEnvironment.application.getApplicationContext()) { - @Override - public ContentResolver getContentResolver() { - return resolver; - } - }; + context = TestUtils.createContextWithContentResolver(contentResolver); ShadowContentResolver.registerProvider(AppProvider.getAuthority(), new AppProvider()); } 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 d8b8f0599..945b5f646 100644 --- a/app/src/test/java/org/fdroid/fdroid/data/ProviderUriTests.java +++ b/app/src/test/java/org/fdroid/fdroid/data/ProviderUriTests.java @@ -1,6 +1,7 @@ package org.fdroid.fdroid.data; import org.fdroid.fdroid.BuildConfig; +import org.fdroid.fdroid.TestUtils; import org.fdroid.fdroid.data.Schema.InstalledAppTable; import org.fdroid.fdroid.mock.MockApk; import org.junit.After; @@ -93,14 +94,27 @@ public class ProviderUriTests { App app = new App(); app.packageName = "org.fdroid.fdroid"; - List apps = new ArrayList<>(1); - apps.add(app); - assertValidUri(resolver, AppProvider.getContentUri(app), "content://org.fdroid.fdroid.data.AppProvider/org.fdroid.fdroid", projection); - assertValidUri(resolver, AppProvider.getContentUri(apps), "content://org.fdroid.fdroid.data.AppProvider/apps/org.fdroid.fdroid", projection); assertValidUri(resolver, AppProvider.getContentUri("org.fdroid.fdroid"), "content://org.fdroid.fdroid.data.AppProvider/org.fdroid.fdroid", projection); } + @Test + public void validTempAppProviderUris() { + ShadowContentResolver.registerProvider(TempAppProvider.getAuthority(), new TempAppProvider()); + String[] projection = new String[]{Schema.AppTable.Cols._ID}; + + // Required so that the `assertValidUri` calls below will indeed have a real temp_fdroid_app + // table to query. + TempAppProvider.Helper.init(TestUtils.createContextWithContentResolver(resolver)); + + List packageNames = new ArrayList<>(2); + packageNames.add("org.fdroid.fdroid"); + packageNames.add("com.example.com"); + + assertValidUri(resolver, TempAppProvider.getAppsUri(packageNames), "content://org.fdroid.fdroid.data.TempAppProvider/apps/org.fdroid.fdroid%2Ccom.example.com", projection); + assertValidUri(resolver, TempAppProvider.getContentUri(), "content://org.fdroid.fdroid.data.TempAppProvider", projection); + } + @Test public void invalidApkProviderUris() { ShadowContentResolver.registerProvider(ApkProvider.getAuthority(), new ApkProvider()); From 6e3b1fde86319d1edd11858d1f34c5234ad2512e Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Mon, 27 Jun 2016 22:23:35 +1000 Subject: [PATCH 02/11] Implement `getLong` for `ContentValuesCursor`. This is required because SQLite "rowids" are 64 bit integers: > all rows within SQLite tables have a 64-bit signed integer > key that uniquely identifies the row within its table." https://sqlite.org/lang_createtable.html#rowid --- .../java/org/fdroid/fdroid/data/ContentValuesCursor.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/org/fdroid/fdroid/data/ContentValuesCursor.java b/app/src/main/java/org/fdroid/fdroid/data/ContentValuesCursor.java index 8bc350ce4..3c64c7957 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/ContentValuesCursor.java +++ b/app/src/main/java/org/fdroid/fdroid/data/ContentValuesCursor.java @@ -65,7 +65,10 @@ class ContentValuesCursor extends AbstractCursor { @Override public long getLong(int i) { - throw new IllegalArgumentException("unimplemented"); + if (values[i] instanceof Long) { + return (Long) values[i]; + } + throw new IllegalArgumentException("Value is not a Long"); } @Override From ec36f2a1cd36cfb0d3fd4d08b78c9d8e0406514e Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Mon, 27 Jun 2016 22:30:01 +1000 Subject: [PATCH 03/11] Remove some unused methods from providers. --- .../java/org/fdroid/fdroid/data/ApkProvider.java | 16 ---------------- .../java/org/fdroid/fdroid/data/AppProvider.java | 4 ---- 2 files changed, 20 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 d777866a7..303943ba2 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/ApkProvider.java +++ b/app/src/main/java/org/fdroid/fdroid/data/ApkProvider.java @@ -102,13 +102,6 @@ public class ApkProvider extends FDroidProvider { return cursorToList(cursor); } - /** - * @see org.fdroid.fdroid.data.ApkProvider.Helper#find(Context, Repo, List, String[]) - */ - public static List find(Context context, Repo repo, List apps) { - return find(context, repo, apps, Cols.ALL); - } - public static Apk find(Context context, String packageName, int versionCode, String[] projection) { final Uri uri = getContentUri(packageName, versionCode); return find(context, uri, projection); @@ -276,15 +269,6 @@ public class ApkProvider extends FDroidProvider { .build(); } - public static Uri getContentUriForApks(Repo repo, List apks) { - return getContentUri() - .buildUpon() - .appendPath(PATH_REPO_APK) - .appendPath(Long.toString(repo.id)) - .appendPath(buildApkString(apks)) - .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 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 1eab64e1b..a034fc01e 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/AppProvider.java +++ b/app/src/main/java/org/fdroid/fdroid/data/AppProvider.java @@ -201,10 +201,6 @@ public class AppProvider extends FDroidProvider { super(selection, args); } - AppQuerySelection(String selection, List args) { - super(selection, args); - } - public boolean naturalJoinToInstalled() { return naturalJoinToInstalled; } From d3f9cfbdfa0bc8b4354e2a0ae3b927f057642c73 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Mon, 27 Jun 2016 22:41:48 +1000 Subject: [PATCH 04/11] Remove need for `temorary b-tree for order by` in most cases by introducing two indexes. The fact that sqlite chose to do a "FULL TABLE SCAN" right off the bat when showing a list of apps results in it having to do extra work at the end of the query to sort. If the scan can be utilise an index, then the sorting is already done and a b-tree need not be constructed. Thus, this introduces indexes for both the `name` column (used to sort most lists of apps) and the `added` column (used to figure out the `Whats New` apps). --- .../java/org/fdroid/fdroid/data/DBHelper.java | 15 ++++++++++----- 1 file changed, 10 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 c688ee01a..677c207e0 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/DBHelper.java +++ b/app/src/main/java/org/fdroid/fdroid/data/DBHelper.java @@ -321,18 +321,21 @@ class DBHelper extends SQLiteOpenHelper { private void migrateAppPrimaryKeyToRowId(SQLiteDatabase db, int oldVersion) { if (oldVersion < 58) { - final String alter = "ALTER TABLE " + ApkTable.NAME + " ADD COLUMN appId NUMERIC"; - Log.i(TAG, "Adding appId foreign key to fdroid_apk."); + db.execSQL("CREATE INDEX IF NOT EXISTS name ON " + AppTable.NAME + " (" + AppTable.Cols.NAME + ")"); + db.execSQL("CREATE INDEX IF NOT EXISTS added ON " + AppTable.NAME + " (" + AppTable.Cols.ADDED + ")"); + + final String alter = "ALTER TABLE " + ApkTable.NAME + " ADD COLUMN " + ApkTable.Cols.APP_ID + " NUMERIC"; + Log.i(TAG, "Adding appId foreign key to " + ApkTable.NAME); Utils.debugLog(TAG, alter); db.execSQL(alter); final String update = "UPDATE " + ApkTable.NAME + " SET appId = ( " + - "SELECT app.rowid " + + "SELECT app." + AppTable.Cols.ROW_ID + " " + "FROM " + ApkTable.NAME + " AS app " + - "WHERE " + ApkTable.NAME + ".id = app.id " + + "WHERE " + ApkTable.NAME + "." + ApkTable.Cols.PACKAGE_NAME + " = app." + AppTable.Cols.PACKAGE_NAME + " " + ")"; - Log.i(TAG, "Updating foreign key from fdroid_apk to fdroid_app to use numeric foreign key."); + Log.i(TAG, "Updating foreign key from " + ApkTable.NAME + " to " + AppTable.NAME + " to use numeric foreign key."); Utils.debugLog(TAG, update); db.execSQL(update); } @@ -590,6 +593,8 @@ class DBHelper extends SQLiteOpenHelper { private static void createAppApk(SQLiteDatabase db) { db.execSQL(CREATE_TABLE_APP); db.execSQL("create index app_id on " + AppTable.NAME + " (" + AppTable.Cols.PACKAGE_NAME + ");"); + db.execSQL("create index name on " + AppTable.NAME + " (" + AppTable.Cols.NAME + ");"); // Used for sorting most lists + db.execSQL("create index added on " + AppTable.NAME + " (" + AppTable.Cols.ADDED + ");"); // Used for sorting "newly added" db.execSQL(CREATE_TABLE_APK); db.execSQL("create index apk_vercode on " + ApkTable.NAME + " (" + ApkTable.Cols.VERSION_CODE + ");"); db.execSQL("create index apk_appId on " + ApkTable.NAME + " (" + ApkTable.Cols.APP_ID + ");"); From 81910bf7499d6054d21a0b00a419c2d12e9eee18 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Wed, 29 Jun 2016 07:08:13 +1000 Subject: [PATCH 05/11] Added tests for "orphaned apks" that must be removed after disabling a repo. --- .../java/org/fdroid/fdroid/RepoUpdater.java | 10 ++-- .../AcceptableMultiRepoUpdaterTest.java | 52 +++++++++++++++++++ .../fdroid/fdroid/MultiRepoUpdaterTest.java | 20 +++---- 3 files changed, 66 insertions(+), 16 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/RepoUpdater.java b/app/src/main/java/org/fdroid/fdroid/RepoUpdater.java index 3df518df1..0cd108ac1 100644 --- a/app/src/main/java/org/fdroid/fdroid/RepoUpdater.java +++ b/app/src/main/java/org/fdroid/fdroid/RepoUpdater.java @@ -199,10 +199,12 @@ public class RepoUpdater { reader.setContentHandler(repoXMLHandler); reader.parse(new InputSource(indexInputStream)); - long timestamp = repoDetailsToSave.getAsLong(RepoTable.Cols.TIMESTAMP); - if (timestamp < repo.timestamp) { - throw new UpdateException(repo, "index.jar is older that current index! " - + timestamp + " < " + repo.timestamp); + if (repoDetailsToSave.containsKey(RepoTable.Cols.TIMESTAMP)) { + long timestamp = repoDetailsToSave.getAsLong(RepoTable.Cols.TIMESTAMP); + if (timestamp < repo.timestamp) { + throw new UpdateException(repo, "index.jar is older that current index! " + + timestamp + " < " + repo.timestamp); + } } signingCertFromJar = getSigningCertFromJar(indexEntry); diff --git a/app/src/test/java/org/fdroid/fdroid/AcceptableMultiRepoUpdaterTest.java b/app/src/test/java/org/fdroid/fdroid/AcceptableMultiRepoUpdaterTest.java index d159c6018..ef5a4f203 100644 --- a/app/src/test/java/org/fdroid/fdroid/AcceptableMultiRepoUpdaterTest.java +++ b/app/src/test/java/org/fdroid/fdroid/AcceptableMultiRepoUpdaterTest.java @@ -1,6 +1,9 @@ package org.fdroid.fdroid; +import android.content.ContentValues; +import android.net.Uri; +import android.support.annotation.NonNull; import android.util.Log; import org.fdroid.fdroid.RepoUpdater.UpdateException; @@ -14,6 +17,7 @@ import org.robolectric.annotation.Config; import java.util.List; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; @Config(constants = BuildConfig.class) @RunWith(RobolectricGradleTestRunner.class) @@ -79,4 +83,52 @@ public class AcceptableMultiRepoUpdaterTest extends MultiRepoUpdaterTest { } } + @NonNull + private Repo getMainRepo() { + Repo repo = RepoProvider.Helper.findByAddress(context, REPO_MAIN_URI); + assertNotNull(repo); + return repo; + } + + @NonNull + private Repo getArchiveRepo() { + Repo repo = RepoProvider.Helper.findByAddress(context, REPO_ARCHIVE_URI); + assertNotNull(repo); + return repo; + } + + @NonNull + private Repo getConflictingRepo() { + Repo repo = RepoProvider.Helper.findByAddress(context, REPO_CONFLICTING_URI); + assertNotNull(repo); + return repo; + } + + @Test + public void testOrphanedApps() throws UpdateException { + assertEmpty(); + + updateArchive(); + updateMain(); + updateConflicting(); + + assertSomewhatAcceptable(); + + disableRepo(getArchiveRepo()); + disableRepo(getMainRepo()); + disableRepo(getConflictingRepo()); + + RepoProvider.Helper.purgeApps(context, getArchiveRepo()); + RepoProvider.Helper.purgeApps(context, getMainRepo()); + RepoProvider.Helper.purgeApps(context, getConflictingRepo()); + + assertEmpty(); + } + + private void disableRepo(Repo repo) { + ContentValues values = new ContentValues(1); + values.put(RepoProvider.DataColumns.IN_USE, 0); + RepoProvider.Helper.update(context, repo, values); + } + } diff --git a/app/src/test/java/org/fdroid/fdroid/MultiRepoUpdaterTest.java b/app/src/test/java/org/fdroid/fdroid/MultiRepoUpdaterTest.java index 9aa0785e4..3b2468b1f 100644 --- a/app/src/test/java/org/fdroid/fdroid/MultiRepoUpdaterTest.java +++ b/app/src/test/java/org/fdroid/fdroid/MultiRepoUpdaterTest.java @@ -33,9 +33,9 @@ public abstract class MultiRepoUpdaterTest extends FDroidProviderTest { protected static final String REPO_ARCHIVE = "Test F-Droid repo (Archive)"; protected static final String REPO_CONFLICTING = "Test F-Droid repo with different apps"; - protected RepoUpdater conflictingRepoUpdater; - protected RepoUpdater mainRepoUpdater; - protected RepoUpdater archiveRepoUpdater; + protected static final String REPO_MAIN_URI = "https://f-droid.org/repo"; + protected static final String REPO_ARCHIVE_URI = "https://f-droid.org/archive"; + protected static final String REPO_CONFLICTING_URI = "https://example.com/conflicting/fdroid/repo"; private static final String PUB_KEY = "3082050b308202f3a003020102020420d8f212300d06092a864886f70d01010b050030363110300e0603" + @@ -79,10 +79,6 @@ public abstract class MultiRepoUpdaterTest extends FDroidProviderTest { RepoProvider.Helper.remove(context, 3); RepoProvider.Helper.remove(context, 4); - conflictingRepoUpdater = createUpdater(REPO_CONFLICTING, context); - mainRepoUpdater = createUpdater(REPO_MAIN, context); - archiveRepoUpdater = createUpdater(REPO_ARCHIVE, context); - Preferences.setup(context); } @@ -157,10 +153,10 @@ public abstract class MultiRepoUpdaterTest extends FDroidProviderTest { } } - private RepoUpdater createUpdater(String name, Context context) { + private RepoUpdater createUpdater(String name, String uri, Context context) { Repo repo = new Repo(); repo.signingCertificate = PUB_KEY; - repo.address = "https://fake.url/" + UUID.randomUUID().toString() + "/fdroid/repo"; + repo.address = uri; repo.name = name; ContentValues values = new ContentValues(2); @@ -176,15 +172,15 @@ public abstract class MultiRepoUpdaterTest extends FDroidProviderTest { } protected boolean updateConflicting() throws UpdateException { - return updateRepo(conflictingRepoUpdater, "multiRepo.conflicting.jar"); + return updateRepo(createUpdater(REPO_CONFLICTING, REPO_CONFLICTING_URI, context), "multiRepo.conflicting.jar"); } protected boolean updateMain() throws UpdateException { - return updateRepo(mainRepoUpdater, "multiRepo.normal.jar"); + return updateRepo(createUpdater(REPO_MAIN, REPO_MAIN_URI, context), "multiRepo.normal.jar"); } protected boolean updateArchive() throws UpdateException { - return updateRepo(archiveRepoUpdater, "multiRepo.archive.jar"); + return updateRepo(createUpdater(REPO_ARCHIVE, REPO_ARCHIVE_URI, context), "multiRepo.archive.jar"); } private boolean updateRepo(RepoUpdater updater, String indexJarPath) throws UpdateException { From b4e0bde57f530b78a27616b9a66fd4f5bea4ab1c Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Wed, 29 Jun 2016 07:11:57 +1000 Subject: [PATCH 06/11] Cleanup for checkstyle --- app/src/main/java/org/fdroid/fdroid/data/ApkProvider.java | 1 - app/src/main/java/org/fdroid/fdroid/data/DBHelper.java | 8 +++----- .../org/fdroid/fdroid/AcceptableMultiRepoUpdaterTest.java | 4 ++-- .../test/java/org/fdroid/fdroid/MultiRepoUpdaterTest.java | 1 - .../java/org/fdroid/fdroid/data/FDroidProviderTest.java | 4 ---- 5 files changed, 5 insertions(+), 13 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 303943ba2..ed3787c45 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/ApkProvider.java +++ b/app/src/main/java/org/fdroid/fdroid/data/ApkProvider.java @@ -8,7 +8,6 @@ import android.database.Cursor; import android.net.Uri; import android.util.Log; -import org.fdroid.fdroid.Utils; import org.fdroid.fdroid.data.Schema.ApkTable; import org.fdroid.fdroid.data.Schema.ApkTable.Cols; import org.fdroid.fdroid.data.Schema.AppTable; 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 677c207e0..b90cb1ad4 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/DBHelper.java +++ b/app/src/main/java/org/fdroid/fdroid/data/DBHelper.java @@ -329,12 +329,10 @@ class DBHelper extends SQLiteOpenHelper { Utils.debugLog(TAG, alter); db.execSQL(alter); - final String update = - "UPDATE " + ApkTable.NAME + " SET appId = ( " + + final String update = "UPDATE " + ApkTable.NAME + " SET " + ApkTable.Cols.APP_ID + " = ( " + "SELECT app." + AppTable.Cols.ROW_ID + " " + - "FROM " + ApkTable.NAME + " AS app " + - "WHERE " + ApkTable.NAME + "." + ApkTable.Cols.PACKAGE_NAME + " = app." + AppTable.Cols.PACKAGE_NAME + " " + - ")"; + "FROM " + AppTable.NAME + " AS app " + + "WHERE " + ApkTable.NAME + "." + ApkTable.Cols.PACKAGE_NAME + " = app." + AppTable.Cols.PACKAGE_NAME + ")"; Log.i(TAG, "Updating foreign key from " + ApkTable.NAME + " to " + AppTable.NAME + " to use numeric foreign key."); Utils.debugLog(TAG, update); db.execSQL(update); diff --git a/app/src/test/java/org/fdroid/fdroid/AcceptableMultiRepoUpdaterTest.java b/app/src/test/java/org/fdroid/fdroid/AcceptableMultiRepoUpdaterTest.java index ef5a4f203..00d3d889c 100644 --- a/app/src/test/java/org/fdroid/fdroid/AcceptableMultiRepoUpdaterTest.java +++ b/app/src/test/java/org/fdroid/fdroid/AcceptableMultiRepoUpdaterTest.java @@ -2,13 +2,13 @@ package org.fdroid.fdroid; import android.content.ContentValues; -import android.net.Uri; import android.support.annotation.NonNull; import android.util.Log; import org.fdroid.fdroid.RepoUpdater.UpdateException; import org.fdroid.fdroid.data.Repo; import org.fdroid.fdroid.data.RepoProvider; +import org.fdroid.fdroid.data.Schema; import org.junit.Test; import org.junit.runner.RunWith; import org.robolectric.RobolectricGradleTestRunner; @@ -127,7 +127,7 @@ public class AcceptableMultiRepoUpdaterTest extends MultiRepoUpdaterTest { private void disableRepo(Repo repo) { ContentValues values = new ContentValues(1); - values.put(RepoProvider.DataColumns.IN_USE, 0); + values.put(Schema.RepoTable.Cols.IN_USE, 0); RepoProvider.Helper.update(context, repo, values); } diff --git a/app/src/test/java/org/fdroid/fdroid/MultiRepoUpdaterTest.java b/app/src/test/java/org/fdroid/fdroid/MultiRepoUpdaterTest.java index 3b2468b1f..9b694e863 100644 --- a/app/src/test/java/org/fdroid/fdroid/MultiRepoUpdaterTest.java +++ b/app/src/test/java/org/fdroid/fdroid/MultiRepoUpdaterTest.java @@ -19,7 +19,6 @@ import org.junit.Before; import java.io.File; import java.util.List; -import java.util.UUID; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; 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 713992d4b..ca46c75a5 100644 --- a/app/src/test/java/org/fdroid/fdroid/data/FDroidProviderTest.java +++ b/app/src/test/java/org/fdroid/fdroid/data/FDroidProviderTest.java @@ -1,18 +1,14 @@ package org.fdroid.fdroid.data; -import android.content.ContentResolver; import android.content.ContextWrapper; import org.fdroid.fdroid.TestUtils; import org.junit.After; import org.junit.Before; -import org.mockito.AdditionalAnswers; import org.robolectric.RuntimeEnvironment; import org.robolectric.Shadows; import org.robolectric.shadows.ShadowContentResolver; -import static org.mockito.Mockito.mock; - public abstract class FDroidProviderTest { protected ShadowContentResolver contentResolver; From 94c91148627d043d229d07ce5f15feb4f45e32b7 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Thu, 7 Jul 2016 11:04:36 +1000 Subject: [PATCH 07/11] Made db migration more robust by wrapping in transaction. --- .../java/org/fdroid/fdroid/data/DBHelper.java | 34 +++++++++++-------- 1 file changed, 20 insertions(+), 14 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 b90cb1ad4..25ad35998 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/DBHelper.java +++ b/app/src/main/java/org/fdroid/fdroid/data/DBHelper.java @@ -320,22 +320,28 @@ class DBHelper extends SQLiteOpenHelper { } private void migrateAppPrimaryKeyToRowId(SQLiteDatabase db, int oldVersion) { - if (oldVersion < 58) { - db.execSQL("CREATE INDEX IF NOT EXISTS name ON " + AppTable.NAME + " (" + AppTable.Cols.NAME + ")"); - db.execSQL("CREATE INDEX IF NOT EXISTS added ON " + AppTable.NAME + " (" + AppTable.Cols.ADDED + ")"); + if (oldVersion < 58 && !columnExists(db, ApkTable.NAME, ApkTable.Cols.APP_ID)) { + db.beginTransaction(); + try { + db.execSQL("CREATE INDEX IF NOT EXISTS name ON " + AppTable.NAME + " (" + AppTable.Cols.NAME + ")"); + db.execSQL("CREATE INDEX IF NOT EXISTS added ON " + AppTable.NAME + " (" + AppTable.Cols.ADDED + ")"); - final String alter = "ALTER TABLE " + ApkTable.NAME + " ADD COLUMN " + ApkTable.Cols.APP_ID + " NUMERIC"; - Log.i(TAG, "Adding appId foreign key to " + ApkTable.NAME); - Utils.debugLog(TAG, alter); - db.execSQL(alter); + final String alter = "ALTER TABLE " + ApkTable.NAME + " ADD COLUMN " + ApkTable.Cols.APP_ID + " NUMERIC"; + Log.i(TAG, "Adding appId foreign key to " + ApkTable.NAME); + Utils.debugLog(TAG, alter); + db.execSQL(alter); - final String update = "UPDATE " + ApkTable.NAME + " SET " + ApkTable.Cols.APP_ID + " = ( " + - "SELECT app." + AppTable.Cols.ROW_ID + " " + - "FROM " + AppTable.NAME + " AS app " + - "WHERE " + ApkTable.NAME + "." + ApkTable.Cols.PACKAGE_NAME + " = app." + AppTable.Cols.PACKAGE_NAME + ")"; - Log.i(TAG, "Updating foreign key from " + ApkTable.NAME + " to " + AppTable.NAME + " to use numeric foreign key."); - Utils.debugLog(TAG, update); - db.execSQL(update); + final String update = "UPDATE " + ApkTable.NAME + " SET " + ApkTable.Cols.APP_ID + " = ( " + + "SELECT app." + AppTable.Cols.ROW_ID + " " + + "FROM " + AppTable.NAME + " AS app " + + "WHERE " + ApkTable.NAME + "." + ApkTable.Cols.PACKAGE_NAME + " = app." + AppTable.Cols.PACKAGE_NAME + ")"; + Log.i(TAG, "Updating foreign key from " + ApkTable.NAME + " to " + AppTable.NAME + " to use numeric foreign key."); + Utils.debugLog(TAG, update); + db.execSQL(update); + db.setTransactionSuccessful(); + } finally { + db.endTransaction(); + } } } From 131e7f9dbdfb601d4b126cc8cabeb5ebb536eb6e Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Thu, 7 Jul 2016 15:54:14 +1000 Subject: [PATCH 08/11] Make appId actually numeric on new database creations (was correct when migrating old databases) --- app/src/main/java/org/fdroid/fdroid/data/DBHelper.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 25ad35998..21b8fe8f0 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/DBHelper.java +++ b/app/src/main/java/org/fdroid/fdroid/data/DBHelper.java @@ -47,7 +47,7 @@ class DBHelper extends SQLiteOpenHelper { private static final String CREATE_TABLE_APK = "CREATE TABLE " + ApkTable.NAME + " ( " + ApkTable.Cols.PACKAGE_NAME + " text not null, " - + ApkTable.Cols.APP_ID + " text not null, " + + ApkTable.Cols.APP_ID + " integer not null, " + ApkTable.Cols.VERSION_NAME + " text not null, " + ApkTable.Cols.REPO_ID + " integer not null, " + ApkTable.Cols.HASH + " text not null, " From 1c8cba5692e74d76eb219a197c460a5dede86847 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Mon, 18 Jul 2016 22:12:44 +1000 Subject: [PATCH 09/11] Properly add indexes when migrating database. Moved index adding to a helper function, so that the same mistake isn't made again. That is, indexes should be the same whether upgrading or creating a database. Thus, the code to add indexes should always be the same regardless of the reason for an index being added. The `IF NOT EXISTS` syntax helps to allow the same queries to add during creatin and migration of database. --- .../java/org/fdroid/fdroid/data/DBHelper.java | 23 +++++++++++-------- 1 file changed, 14 insertions(+), 9 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 21b8fe8f0..c64a51b70 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/DBHelper.java +++ b/app/src/main/java/org/fdroid/fdroid/data/DBHelper.java @@ -323,9 +323,6 @@ class DBHelper extends SQLiteOpenHelper { if (oldVersion < 58 && !columnExists(db, ApkTable.NAME, ApkTable.Cols.APP_ID)) { db.beginTransaction(); try { - db.execSQL("CREATE INDEX IF NOT EXISTS name ON " + AppTable.NAME + " (" + AppTable.Cols.NAME + ")"); - db.execSQL("CREATE INDEX IF NOT EXISTS added ON " + AppTable.NAME + " (" + AppTable.Cols.ADDED + ")"); - final String alter = "ALTER TABLE " + ApkTable.NAME + " ADD COLUMN " + ApkTable.Cols.APP_ID + " NUMERIC"; Log.i(TAG, "Adding appId foreign key to " + ApkTable.NAME); Utils.debugLog(TAG, alter); @@ -338,6 +335,7 @@ class DBHelper extends SQLiteOpenHelper { Log.i(TAG, "Updating foreign key from " + ApkTable.NAME + " to " + AppTable.NAME + " to use numeric foreign key."); Utils.debugLog(TAG, update); db.execSQL(update); + ensureIndexes(db); db.setTransactionSuccessful(); } finally { db.endTransaction(); @@ -596,13 +594,20 @@ class DBHelper extends SQLiteOpenHelper { private static void createAppApk(SQLiteDatabase db) { db.execSQL(CREATE_TABLE_APP); - db.execSQL("create index app_id on " + AppTable.NAME + " (" + AppTable.Cols.PACKAGE_NAME + ");"); - db.execSQL("create index name on " + AppTable.NAME + " (" + AppTable.Cols.NAME + ");"); // Used for sorting most lists - db.execSQL("create index added on " + AppTable.NAME + " (" + AppTable.Cols.ADDED + ");"); // Used for sorting "newly added" db.execSQL(CREATE_TABLE_APK); - db.execSQL("create index apk_vercode on " + ApkTable.NAME + " (" + ApkTable.Cols.VERSION_CODE + ");"); - db.execSQL("create index apk_appId on " + ApkTable.NAME + " (" + ApkTable.Cols.APP_ID + ");"); - db.execSQL("create index apk_id on " + ApkTable.NAME + " (" + AppTable.Cols.PACKAGE_NAME + ");"); + ensureIndexes(db); + } + + private static void ensureIndexes(SQLiteDatabase db) { + Utils.debugLog(TAG, "Ensuring indexes exist for " + AppTable.NAME); + db.execSQL("CREATE INDEX IF NOT EXISTS app_id on " + AppTable.NAME + " (" + AppTable.Cols.PACKAGE_NAME + ");"); + db.execSQL("CREATE INDEX IF NOT EXISTS name on " + AppTable.NAME + " (" + AppTable.Cols.NAME + ");"); // Used for sorting most lists + db.execSQL("CREATE INDEX IF NOT EXISTS added on " + AppTable.NAME + " (" + AppTable.Cols.ADDED + ");"); // Used for sorting "newly added" + + Utils.debugLog(TAG, "Ensuring indexes exist for " + ApkTable.NAME); + db.execSQL("CREATE INDEX IF NOT EXISTS apk_vercode on " + ApkTable.NAME + " (" + ApkTable.Cols.VERSION_CODE + ");"); + db.execSQL("CREATE INDEX IF NOT EXISTS apk_appId on " + ApkTable.NAME + " (" + ApkTable.Cols.APP_ID + ");"); + db.execSQL("CREATE INDEX IF NOT EXISTS apk_id on " + ApkTable.NAME + " (" + AppTable.Cols.PACKAGE_NAME + ");"); } /** From 3d182d8e147eaae565f1418507045800255790ac Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Mon, 18 Jul 2016 22:28:49 +1000 Subject: [PATCH 10/11] Reinstate timestamp check as per CR comments --- app/src/main/java/org/fdroid/fdroid/RepoUpdater.java | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/RepoUpdater.java b/app/src/main/java/org/fdroid/fdroid/RepoUpdater.java index 0cd108ac1..3df518df1 100644 --- a/app/src/main/java/org/fdroid/fdroid/RepoUpdater.java +++ b/app/src/main/java/org/fdroid/fdroid/RepoUpdater.java @@ -199,12 +199,10 @@ public class RepoUpdater { reader.setContentHandler(repoXMLHandler); reader.parse(new InputSource(indexInputStream)); - if (repoDetailsToSave.containsKey(RepoTable.Cols.TIMESTAMP)) { - long timestamp = repoDetailsToSave.getAsLong(RepoTable.Cols.TIMESTAMP); - if (timestamp < repo.timestamp) { - throw new UpdateException(repo, "index.jar is older that current index! " - + timestamp + " < " + repo.timestamp); - } + long timestamp = repoDetailsToSave.getAsLong(RepoTable.Cols.TIMESTAMP); + if (timestamp < repo.timestamp) { + throw new UpdateException(repo, "index.jar is older that current index! " + + timestamp + " < " + repo.timestamp); } signingCertFromJar = getSigningCertFromJar(indexEntry); From 666e853c5c8606a8e3b4c5f018bbf25ecb2a3ab1 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Wed, 20 Jul 2016 06:35:44 +1000 Subject: [PATCH 11/11] Allow connected23 to fail, due to flakiness Right now there is only one test in there anyway, so hopefully this is a good tradeoff in terms of our time wasted vs not being able to run those tests. --- .gitlab-ci.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 746d1db75..3bc57ef60 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -70,6 +70,7 @@ connected23: # this file changes every time but should not be cached - rm -f $GRADLE_USER_HOME/caches/modules-2/modules-2.lock - exit $EXITVALUE + allow_failure: true # remove once install it runs reliably pmd: script: