Refactored categories field from column in metadata table to join table.

When updating existing apps or inserting new apps, instead of splatting
a comma separated list into a single sqlite3 column, we now put it into
several rows in the `CatJoinTable`. This is done after deleting existing
categories, to make sure that if the app has been removed from a category,
then this is reflected during the update.

app/src/main/java/org/fdroid/fdroid/data/App.java

@@ -101,6 +101,8 @@ public class App extends ValueObject implements Comparable<App>, Parcelable {
 
     /**
      * List of categories (as defined in the metadata documentation) or null if there aren't any.
+     * This is only populated when parsing a repository. If you need to know about the categories
+     * an app is in any other part of F-Droid, use the {@link CategoryProvider}.
      */
     public String[] categories;
 
@@ -230,9 +232,6 @@ public class App extends ValueObject implements Comparable<App>, Parcelable {
                 case Cols.LAST_UPDATED:
                     lastUpdated = Utils.parseDate(cursor.getString(i), null);
                     break;
-                case Cols.CATEGORIES:
-                    categories = Utils.parseCommaSeparatedString(cursor.getString(i));
-                    break;
                 case Cols.ANTI_FEATURES:
                     antiFeatures = Utils.parseCommaSeparatedString(cursor.getString(i));
                     break;
@@ -504,7 +503,7 @@ public class App extends ValueObject implements Comparable<App>, Parcelable {
         values.put(Cols.SUGGESTED_VERSION_CODE, suggestedVersionCode);
         values.put(Cols.UPSTREAM_VERSION_NAME, upstreamVersionName);
         values.put(Cols.UPSTREAM_VERSION_CODE, upstreamVersionCode);
-        values.put(Cols.CATEGORIES, Utils.serializeCommaSeparatedString(categories));
+        values.put(Cols.Categories.CATEGORIES, Utils.serializeCommaSeparatedString(categories));
         values.put(Cols.ANTI_FEATURES, Utils.serializeCommaSeparatedString(antiFeatures));
         values.put(Cols.REQUIREMENTS, Utils.serializeCommaSeparatedString(requirements));
         values.put(Cols.IS_COMPATIBLE, compatible ? 1 : 0);

app/src/main/java/org/fdroid/fdroid/data/AppProvider.java

@@ -109,7 +109,7 @@ public class AppProvider extends FDroidProvider {
         public static List<String> categories(Context context) {
             final ContentResolver resolver = context.getContentResolver();
             final Uri uri = getContentUri();
-            final String[] projection = {Cols.CATEGORIES};
+            final String[] projection = {Cols.Categories.CATEGORIES};
             final Cursor cursor = resolver.query(uri, projection, null, null, null);
             final Set<String> categorySet = new HashSet<>();
             if (cursor != null) {
@@ -268,7 +268,6 @@ public class AppProvider extends FDroidProvider {
         private boolean isSuggestedApkTableAdded;
         private boolean requiresInstalledTable;
         private boolean requiresLeftJoinToPrefs;
-        private boolean categoryFieldAdded;
         private boolean countFieldAppended;
 
         @Override
@@ -288,14 +287,10 @@ public class AppProvider extends FDroidProvider {
                 " LEFT JOIN " + apk + " ON (" + apk + "." + ApkTable.Cols.APP_ID + " = " + app + "." + Cols.ROW_ID + ") ";
         }
 
-        @Override
-        protected boolean isDistinct() {
-            return fieldCount() == 1 && categoryFieldAdded;
-        }
-
         @Override
         protected String groupBy() {
-            // If the count field has been requested, then we want to group all rows together.
+            // If the count field has been requested, then we want to group all rows together. Otherwise
+            // we will only group all the rows belonging to a single app together.
             return countFieldAppended ? null : getTableName() + "." + Cols.ROW_ID;
         }
 
@@ -362,10 +357,10 @@ public class AppProvider extends FDroidProvider {
                 case Cols._COUNT:
                     appendCountField();
                     break;
+                case Cols.Categories.CATEGORIES:
+                    appendCategoriesField();
+                    break;
                 default:
-                    if (field.equals(Cols.CATEGORIES)) {
-                        categoryFieldAdded = true;
-                    }
                     appendField(field, getTableName());
                     break;
             }
@@ -393,6 +388,10 @@ public class AppProvider extends FDroidProvider {
             appendField(fieldName, "suggestedApk", alias);
         }
 
+        private void appendCategoriesField() {
+            appendField("GROUP_CONCAT(" + CategoryTable.NAME + "." + CategoryTable.Cols.NAME + ")", null, Cols.Categories.CATEGORIES);
+        }
+
         private void addInstalledAppVersionName() {
             addInstalledAppField(
                     InstalledAppTable.Cols.VERSION_NAME,
@@ -586,6 +585,10 @@ public class AppProvider extends FDroidProvider {
         return AppMetadataTable.NAME;
     }
 
+    protected String getCatJoinTableName() {
+        return CatJoinTable.NAME;
+    }
+
     protected String getApkTableName() {
         return ApkTable.NAME;
     }
@@ -904,13 +907,51 @@ public class AppProvider extends FDroidProvider {
         values.remove(Cols.Package.PACKAGE_NAME);
         values.put(Cols.PACKAGE_ID, packageId);
 
-        db().insertOrThrow(getTableName(), null, values);
+        String[] categories = null;
+        boolean saveCategories = false;
+        if (values.containsKey(Cols.Categories.CATEGORIES)) {
+            // Hold onto these categories, so that after we have an ID to reference the newly inserted
+            // app metadata we can then specify its categories.
+            saveCategories = true;
+            categories = Utils.parseCommaSeparatedString(values.getAsString(Cols.Categories.CATEGORIES));
+            values.remove(Cols.Categories.CATEGORIES);
+        }
+
+        long appMetadataId = db().insertOrThrow(getTableName(), null, values);
         if (!isApplyingBatch()) {
             getContext().getContentResolver().notifyChange(uri, null);
         }
+
+        if (saveCategories) {
+            ensureCategories(categories, appMetadataId);
+        }
+
         return getSpecificAppUri(values.getAsString(PackageTable.Cols.PACKAGE_NAME), values.getAsLong(Cols.REPO_ID));
     }
 
+    protected void ensureCategories(String[] categories, long appMetadataId) {
+        db().delete(getCatJoinTableName(), CatJoinTable.Cols.APP_METADATA_ID + " = ?", new String[] {Long.toString(appMetadataId)});
+        if (categories != null) {
+            Set<String> categoriesSet = new HashSet<>();
+            for (String categoryName : categories) {
+
+                // There is nothing stopping a server repeating a category name in the metadata of
+                // an app. In order to prevent unique constraint violations, only insert once into
+                // the join table.
+                if (categoriesSet.contains(categoryName)) {
+                    continue;
+                }
+
+                categoriesSet.add(categoryName);
+                long categoryId = CategoryProvider.Helper.ensureExists(getContext(), categoryName);
+                ContentValues categoryValues = new ContentValues(2);
+                categoryValues.put(CatJoinTable.Cols.APP_METADATA_ID, appMetadataId);
+                categoryValues.put(CatJoinTable.Cols.CATEGORY_ID, categoryId);
+                db().insert(getCatJoinTableName(), null, categoryValues);
+            }
+        }
+    }
+
     @Override
     public int update(Uri uri, ContentValues values, String where, String[] whereArgs) {
         if (MATCHER.match(uri) != CALC_APP_DETAILS_FROM_INDEX) {

app/src/main/java/org/fdroid/fdroid/data/DBHelper.java

@@ -131,7 +131,6 @@ class DBHelper extends SQLiteOpenHelper {
             + AppMetadataTable.Cols.LITECOIN_ADDR + " string,"
             + AppMetadataTable.Cols.FLATTR_ID + " string,"
             + AppMetadataTable.Cols.REQUIREMENTS + " string,"
-            + AppMetadataTable.Cols.CATEGORIES + " string,"
             + AppMetadataTable.Cols.ADDED + " string,"
             + AppMetadataTable.Cols.LAST_UPDATED + " string,"
             + AppMetadataTable.Cols.IS_COMPATIBLE + " int not null,"
@@ -151,11 +150,20 @@ class DBHelper extends SQLiteOpenHelper {
             + Schema.CategoryTable.Cols.NAME + " TEXT NOT NULL "
             + " );";
 
-    private static final String CREATE_TABLE_CAT_JOIN = "CREATE TABLE " + CatJoinTable.NAME
+    /**
+     * The order of the two columns in the primary key matters for this table. The index that is
+     * built for sqlite to quickly search the primary key will be sorted by app metadata id first,
+     * and category id second. This means that we don't need a separate individual index on the
+     * app metadata id, because it can instead look through the primary key index. This can be
+     * observed by flipping the order of the primary key columns, and noting the resulting sqlite
+     * logs along the lines of:
+     *   E/SQLiteLog(14164): (284) automatic index on fdroid_categoryAppMetadataJoin(appMetadataId)
+     */
+    static final String CREATE_TABLE_CAT_JOIN = "CREATE TABLE " + CatJoinTable.NAME
             + " ( "
             + CatJoinTable.Cols.APP_METADATA_ID + " INT NOT NULL, "
             + CatJoinTable.Cols.CATEGORY_ID + " INT NOT NULL, "
-            + "primary key(" + CatJoinTable.Cols.CATEGORY_ID + ", " + CatJoinTable.Cols.APP_METADATA_ID + ") "
+            + "primary key(" + CatJoinTable.Cols.APP_METADATA_ID + ", " + CatJoinTable.Cols.CATEGORY_ID + ") "
             + " );";
 
     private static final String CREATE_TABLE_INSTALLED_APP = "CREATE TABLE " + InstalledAppTable.NAME

app/src/main/java/org/fdroid/fdroid/data/Schema.java

@@ -96,6 +96,11 @@ public interface Schema {
              * @see CategoryTable
              */
             String CATEGORY_ID = "categoryId";
+
+            /**
+             * @see AppMetadataTable.Cols#ALL_COLS
+             */
+            String[] ALL_COLS = {APP_METADATA_ID, CATEGORY_ID};
         }
     }
 
@@ -134,7 +139,6 @@ public interface Schema {
             String UPSTREAM_VERSION_CODE = "upstreamVercode";
             String ADDED = "added";
             String LAST_UPDATED = "lastUpdated";
-            String CATEGORIES = "categories";
             String ANTI_FEATURES = "antiFeatures";
             String REQUIREMENTS = "requirements";
             String ICON_URL = "iconUrl";
@@ -154,6 +158,10 @@ public interface Schema {
                 String PACKAGE_NAME = "package_packageName";
             }
 
+            interface Categories {
+                String CATEGORIES = "categories_commaSeparatedCateogryNames";
+            }
+
             /**
              * Each of the physical columns in the sqlite table. Differs from {@link Cols#ALL} in
              * that it doesn't include fields which are aliases of other fields (e.g. {@link Cols#_ID}
@@ -164,7 +172,7 @@ public interface Schema {
                     LICENSE, AUTHOR, EMAIL, WEB_URL, TRACKER_URL, SOURCE_URL,
                     CHANGELOG_URL, DONATE_URL, BITCOIN_ADDR, LITECOIN_ADDR, FLATTR_ID,
                     UPSTREAM_VERSION_NAME, UPSTREAM_VERSION_CODE, ADDED, LAST_UPDATED,
-                    CATEGORIES, ANTI_FEATURES, REQUIREMENTS, ICON_URL, ICON_URL_LARGE,
+                    ANTI_FEATURES, REQUIREMENTS, ICON_URL, ICON_URL_LARGE,
                     SUGGESTED_VERSION_CODE,
             };
 
@@ -178,10 +186,10 @@ public interface Schema {
                     LICENSE, AUTHOR, EMAIL, WEB_URL, TRACKER_URL, SOURCE_URL,
                     CHANGELOG_URL, DONATE_URL, BITCOIN_ADDR, LITECOIN_ADDR, FLATTR_ID,
                     UPSTREAM_VERSION_NAME, UPSTREAM_VERSION_CODE, ADDED, LAST_UPDATED,
-                    CATEGORIES, ANTI_FEATURES, REQUIREMENTS, ICON_URL, ICON_URL_LARGE,
+                    ANTI_FEATURES, REQUIREMENTS, ICON_URL, ICON_URL_LARGE,
                     SUGGESTED_VERSION_CODE, SuggestedApk.VERSION_NAME,
                     InstalledApp.VERSION_CODE, InstalledApp.VERSION_NAME,
-                    InstalledApp.SIGNATURE, Package.PACKAGE_NAME,
+                    InstalledApp.SIGNATURE, Package.PACKAGE_NAME, Categories.CATEGORIES,
             };
         }
     }

app/src/main/java/org/fdroid/fdroid/data/TempAppProvider.java

@@ -11,9 +11,11 @@ import android.text.TextUtils;
 
 import java.util.List;
 
+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;
+import org.fdroid.fdroid.data.Schema.CatJoinTable;
 import org.fdroid.fdroid.data.Schema.PackageTable;
 
 /**
@@ -29,6 +31,7 @@ public class TempAppProvider extends AppProvider {
     private static final String PROVIDER_NAME = "TempAppProvider";
 
     static final String TABLE_TEMP_APP = "temp_" + AppMetadataTable.NAME;
+    static final String TABLE_TEMP_CAT_JOIN = "temp_" + CatJoinTable.NAME;
 
     private static final String PATH_INIT = "init";
     private static final String PATH_COMMIT = "commit";
@@ -51,6 +54,11 @@ public class TempAppProvider extends AppProvider {
         return TABLE_TEMP_APP;
     }
 
+    @Override
+    protected String getCatJoinTableName() {
+        return TABLE_TEMP_CAT_JOIN;
+    }
+
     public static String getAuthority() {
         return AUTHORITY + "." + PROVIDER_NAME;
     }
@@ -153,6 +161,12 @@ public class TempAppProvider extends AppProvider {
         // Package names for apps cannot change...
         values.remove(Cols.Package.PACKAGE_NAME);
 
+        if (values.containsKey(Cols.Categories.CATEGORIES)) {
+            String[] categories = Utils.parseCommaSeparatedString(values.getAsString(Cols.Categories.CATEGORIES));
+            ensureCategories(categories, packageName, repoId);
+            values.remove(Cols.Categories.CATEGORIES);
+        }
+
         int count = db().update(getTableName(), values, query.getSelection(), query.getArgs());
         if (!isApplyingBatch()) {
             getContext().getContentResolver().notifyChange(getHighestPriorityMetadataUri(packageName), null);
@@ -160,6 +174,16 @@ public class TempAppProvider extends AppProvider {
         return count;
     }
 
+    private void ensureCategories(String[] categories, String packageName, long repoId) {
+        QuerySelection forId = querySingle(packageName, repoId);
+        Cursor cursor = db().query(getTableName(), new String[] {Cols.ROW_ID}, forId.getSelection(), forId.getArgs(), null, null, null);
+        cursor.moveToFirst();
+        long appMetadataId = cursor.getLong(0);
+        cursor.close();
+
+        ensureCategories(categories, appMetadataId);
+    }
+
     @Override
     public Cursor query(Uri uri, String[] projection, String customSelection, String[] selectionArgs, String sortOrder) {
         AppQuerySelection selection = new AppQuerySelection(customSelection, selectionArgs);
@@ -188,7 +212,9 @@ public class TempAppProvider extends AppProvider {
         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 + ");");
@@ -208,8 +234,9 @@ public class TempAppProvider extends AppProvider {
         try {
             db.beginTransaction();
 
-            final String tempApp = DB + "." + TempAppProvider.TABLE_TEMP_APP;
+            final String tempApp = DB + "." + TABLE_TEMP_APP;
             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));
@@ -217,6 +244,9 @@ public class TempAppProvider extends AppProvider {
             db.execSQL("DELETE FROM " + ApkTable.NAME + " WHERE 1");
             db.execSQL(copyData(ApkTable.Cols.ALL_COLS, tempApk, ApkTable.NAME));
 
+            db.execSQL("DELETE FROM " + CatJoinTable.NAME + " WHERE 1");
+            db.execSQL(copyData(CatJoinTable.Cols.ALL_COLS, tempCatJoin, CatJoinTable.NAME));
+
             db.setTransactionSuccessful();
 
             getContext().getContentResolver().notifyChange(AppProvider.getContentUri(), null);

app/src/test/java/org/fdroid/fdroid/data/CategoryProviderTest.java

@@ -154,7 +154,7 @@ public class CategoryProviderTest extends FDroidProviderTest {
 
     private void insertAppWithCategory(String id, String name, String categories) {
         ContentValues values = new ContentValues(1);
-        values.put(Cols.CATEGORIES, categories);
+        values.put(Cols.Categories.CATEGORIES, categories);
         AppProviderTest.insertApp(contentResolver, context, id, name, values);
     }
 }