From a317877120a1b7b3ea2afcf5d048c3d77fd62fe2 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Tue, 18 Oct 2016 17:53:26 +1100 Subject: [PATCH 1/3] Be a little more concise about what to do when running migration for v64. Before it is not apparant that the migration introduced for v64 is associated with that particular version. This is because the guard condition used to bail out from the upgrade is more closely related to a previous migration. This is due to a flaw with the desigh of `resetTransient()`, whereby it always resets the database to the schema _of the current F-Droid version being run_, not of the tables as they stood at the time of the particular migration being introduced. This clarifies the guard condition for v64 by instead explicitly asking if the columns of interest exist yet in this particular invocation of `onUpgrade()`. --- .../java/org/fdroid/fdroid/data/DBHelper.java | 42 +++++++++++-------- 1 file changed, 24 insertions(+), 18 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 0a2125425..0cd82c8b2 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/DBHelper.java +++ b/app/src/main/java/org/fdroid/fdroid/data/DBHelper.java @@ -360,27 +360,33 @@ class DBHelper extends SQLiteOpenHelper { addObbFiles(db, oldVersion); } - /** - * For upgrades from earlier than 63, this is created in - * {@link #resetTransient(SQLiteDatabase)} in - * {@link #migrateToPackageTable(SQLiteDatabase, int)}, so it only needs - * to run when the database is at version 63. - */ private void addObbFiles(SQLiteDatabase db, int oldVersion) { - if (oldVersion != 63) { + if (oldVersion >= 64) { return; } - Utils.debugLog(TAG, "Adding " + ApkTable.Cols.OBB_MAIN_FILE - + ", " + ApkTable.Cols.OBB_PATCH_FILE - + ", and hash columns to " + ApkTable.NAME); - db.execSQL("alter table " + ApkTable.NAME + " add column " - + ApkTable.Cols.OBB_MAIN_FILE + " string"); - db.execSQL("alter table " + ApkTable.NAME + " add column " - + ApkTable.Cols.OBB_MAIN_FILE_SHA256 + " string"); - db.execSQL("alter table " + ApkTable.NAME + " add column " - + ApkTable.Cols.OBB_PATCH_FILE + " string"); - db.execSQL("alter table " + ApkTable.NAME + " add column " - + ApkTable.Cols.OBB_PATCH_FILE_SHA256 + " string"); + + Utils.debugLog(TAG, "Ensuring " + ApkTable.Cols.OBB_MAIN_FILE + ", " + + ApkTable.Cols.OBB_PATCH_FILE + ", and hash columns exist on " + ApkTable.NAME); + + if (!columnExists(db, ApkTable.NAME, ApkTable.Cols.OBB_MAIN_FILE)) { + db.execSQL("alter table " + ApkTable.NAME + " add column " + + ApkTable.Cols.OBB_MAIN_FILE + " string"); + } + + if (!columnExists(db, ApkTable.NAME, ApkTable.Cols.OBB_MAIN_FILE_SHA256)) { + db.execSQL("alter table " + ApkTable.NAME + " add column " + + ApkTable.Cols.OBB_MAIN_FILE_SHA256 + " string"); + } + + if (!columnExists(db, ApkTable.NAME, ApkTable.Cols.OBB_PATCH_FILE)) { + db.execSQL("alter table " + ApkTable.NAME + " add column " + + ApkTable.Cols.OBB_PATCH_FILE + " string"); + } + + if (!columnExists(db, ApkTable.NAME, ApkTable.Cols.OBB_PATCH_FILE_SHA256)) { + db.execSQL("alter table " + ApkTable.NAME + " add column " + + ApkTable.Cols.OBB_PATCH_FILE_SHA256 + " string"); + } } private void migrateToPackageTable(SQLiteDatabase db, int oldVersion) { From 63a609fbabc273f4fac72002f7e22551f12bfe94 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Tue, 18 Oct 2016 18:00:10 +1100 Subject: [PATCH 2/3] Moved methods away from top of `DBHelper` class. The fact there are arbitrary migrations at the top of the file (between `onCreate()` and `onUpdate()` makes it harder to scan this file. This changeset moves these methods verbatim, without changing any of the method bodies or signatures. --- .../java/org/fdroid/fdroid/data/DBHelper.java | 248 +++++++++--------- 1 file changed, 124 insertions(+), 124 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 0cd82c8b2..000b98b1e 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/DBHelper.java +++ b/app/src/main/java/org/fdroid/fdroid/data/DBHelper.java @@ -166,96 +166,6 @@ class DBHelper extends SQLiteOpenHelper { this.context = context; } - private void populateRepoNames(SQLiteDatabase db, int oldVersion) { - if (oldVersion >= 37) { - return; - } - Utils.debugLog(TAG, "Populating repo names from the url"); - final String[] columns = {RepoTable.Cols.ADDRESS, RepoTable.Cols._ID}; - Cursor cursor = db.query(RepoTable.NAME, columns, - RepoTable.Cols.NAME + " IS NULL OR " + RepoTable.Cols.NAME + " = ''", null, null, null, null); - if (cursor != null) { - if (cursor.getCount() > 0) { - cursor.moveToFirst(); - while (!cursor.isAfterLast()) { - String address = cursor.getString(0); - long id = cursor.getInt(1); - ContentValues values = new ContentValues(1); - String name = Repo.addressToName(address); - values.put(RepoTable.Cols.NAME, name); - final String[] args = {Long.toString(id)}; - Utils.debugLog(TAG, "Setting repo name to '" + name + "' for repo " + address); - db.update(RepoTable.NAME, values, RepoTable.Cols._ID + " = ?", args); - cursor.moveToNext(); - } - } - cursor.close(); - } - } - - private void renameRepoId(SQLiteDatabase db, int oldVersion) { - if (oldVersion >= 36 || columnExists(db, RepoTable.NAME, RepoTable.Cols._ID)) { - return; - } - - Utils.debugLog(TAG, "Renaming " + RepoTable.NAME + ".id to " + RepoTable.Cols._ID); - db.beginTransaction(); - - try { - // http://stackoverflow.com/questions/805363/how-do-i-rename-a-column-in-a-sqlite-database-table#805508 - String tempTableName = RepoTable.NAME + "__temp__"; - db.execSQL("ALTER TABLE " + RepoTable.NAME + " RENAME TO " + tempTableName + ";"); - - // I realise this is available in the CREATE_TABLE_REPO above, - // however I have a feeling that it will need to be the same as the - // current structure of the table as of DBVersion 36, or else we may - // get into strife. For example, if there was a field that - // got removed, then it will break the "insert select" - // statement. Therefore, I've put a copy of CREATE_TABLE_REPO - // here that is the same as it was at DBVersion 36. - String createTableDdl = "create table " + RepoTable.NAME + " (" - + RepoTable.Cols._ID + " integer not null primary key, " - + RepoTable.Cols.ADDRESS + " text not null, " - + RepoTable.Cols.NAME + " text, " - + RepoTable.Cols.DESCRIPTION + " text, " - + RepoTable.Cols.IN_USE + " integer not null, " - + RepoTable.Cols.PRIORITY + " integer not null, " - + RepoTable.Cols.SIGNING_CERT + " text, " - + RepoTable.Cols.FINGERPRINT + " text, " - + RepoTable.Cols.MAX_AGE + " integer not null default 0, " - + RepoTable.Cols.VERSION + " integer not null default 0, " - + RepoTable.Cols.LAST_ETAG + " text, " - + RepoTable.Cols.LAST_UPDATED + " string);"; - - db.execSQL(createTableDdl); - - String nonIdFields = TextUtils.join(", ", new String[] { - RepoTable.Cols.ADDRESS, - RepoTable.Cols.NAME, - RepoTable.Cols.DESCRIPTION, - RepoTable.Cols.IN_USE, - RepoTable.Cols.PRIORITY, - RepoTable.Cols.SIGNING_CERT, - RepoTable.Cols.FINGERPRINT, - RepoTable.Cols.MAX_AGE, - RepoTable.Cols.VERSION, - RepoTable.Cols.LAST_ETAG, - RepoTable.Cols.LAST_UPDATED, - }); - - String insertSql = "INSERT INTO " + RepoTable.NAME + - "(" + RepoTable.Cols._ID + ", " + nonIdFields + " ) " + - "SELECT id, " + nonIdFields + " FROM " + tempTableName + ";"; - - db.execSQL(insertSql); - db.execSQL("DROP TABLE " + tempTableName + ";"); - db.setTransactionSuccessful(); - } catch (Exception e) { - Log.e(TAG, "Error renaming id to " + RepoTable.Cols._ID, e); - } - db.endTransaction(); - } - @Override public void onCreate(SQLiteDatabase db) { @@ -288,40 +198,6 @@ class DBHelper extends SQLiteOpenHelper { } } - private void insertRepo(SQLiteDatabase db, String name, String address, - String description, String version, String enabled, - String priority, String pushRequests, String pubKey) { - ContentValues values = new ContentValues(); - values.put(RepoTable.Cols.ADDRESS, address); - values.put(RepoTable.Cols.NAME, name); - values.put(RepoTable.Cols.DESCRIPTION, description); - values.put(RepoTable.Cols.SIGNING_CERT, pubKey); - values.put(RepoTable.Cols.FINGERPRINT, Utils.calcFingerprint(pubKey)); - values.put(RepoTable.Cols.MAX_AGE, 0); - values.put(RepoTable.Cols.VERSION, Utils.parseInt(version, 0)); - values.put(RepoTable.Cols.IN_USE, Utils.parseInt(enabled, 0)); - values.put(RepoTable.Cols.PRIORITY, Utils.parseInt(priority, Integer.MAX_VALUE)); - values.put(RepoTable.Cols.LAST_ETAG, (String) null); - values.put(RepoTable.Cols.TIMESTAMP, 0); - - switch (pushRequests) { - case "ignore": - values.put(RepoTable.Cols.PUSH_REQUESTS, Repo.PUSH_REQUEST_IGNORE); - break; - case "prompt": - values.put(RepoTable.Cols.PUSH_REQUESTS, Repo.PUSH_REQUEST_PROMPT); - break; - case "always": - values.put(RepoTable.Cols.PUSH_REQUESTS, Repo.PUSH_REQUEST_ACCEPT_ALWAYS); - break; - default: - throw new IllegalArgumentException(pushRequests + " is not a supported option!"); - } - - Utils.debugLog(TAG, "Adding repository " + name + " with push requests as " + pushRequests); - db.insert(RepoTable.NAME, null, values); - } - @Override public void onUpgrade(SQLiteDatabase db, int oldVersion, int newVersion) { @@ -726,6 +602,96 @@ class DBHelper extends SQLiteOpenHelper { db.execSQL("Alter table " + RepoTable.NAME + " add column " + RepoTable.Cols.LAST_UPDATED + " string"); } + private void populateRepoNames(SQLiteDatabase db, int oldVersion) { + if (oldVersion >= 37) { + return; + } + Utils.debugLog(TAG, "Populating repo names from the url"); + final String[] columns = {RepoTable.Cols.ADDRESS, RepoTable.Cols._ID}; + Cursor cursor = db.query(RepoTable.NAME, columns, + RepoTable.Cols.NAME + " IS NULL OR " + RepoTable.Cols.NAME + " = ''", null, null, null, null); + if (cursor != null) { + if (cursor.getCount() > 0) { + cursor.moveToFirst(); + while (!cursor.isAfterLast()) { + String address = cursor.getString(0); + long id = cursor.getInt(1); + ContentValues values = new ContentValues(1); + String name = Repo.addressToName(address); + values.put(RepoTable.Cols.NAME, name); + final String[] args = {Long.toString(id)}; + Utils.debugLog(TAG, "Setting repo name to '" + name + "' for repo " + address); + db.update(RepoTable.NAME, values, RepoTable.Cols._ID + " = ?", args); + cursor.moveToNext(); + } + } + cursor.close(); + } + } + + private void renameRepoId(SQLiteDatabase db, int oldVersion) { + if (oldVersion >= 36 || columnExists(db, RepoTable.NAME, RepoTable.Cols._ID)) { + return; + } + + Utils.debugLog(TAG, "Renaming " + RepoTable.NAME + ".id to " + RepoTable.Cols._ID); + db.beginTransaction(); + + try { + // http://stackoverflow.com/questions/805363/how-do-i-rename-a-column-in-a-sqlite-database-table#805508 + String tempTableName = RepoTable.NAME + "__temp__"; + db.execSQL("ALTER TABLE " + RepoTable.NAME + " RENAME TO " + tempTableName + ";"); + + // I realise this is available in the CREATE_TABLE_REPO above, + // however I have a feeling that it will need to be the same as the + // current structure of the table as of DBVersion 36, or else we may + // get into strife. For example, if there was a field that + // got removed, then it will break the "insert select" + // statement. Therefore, I've put a copy of CREATE_TABLE_REPO + // here that is the same as it was at DBVersion 36. + String createTableDdl = "create table " + RepoTable.NAME + " (" + + RepoTable.Cols._ID + " integer not null primary key, " + + RepoTable.Cols.ADDRESS + " text not null, " + + RepoTable.Cols.NAME + " text, " + + RepoTable.Cols.DESCRIPTION + " text, " + + RepoTable.Cols.IN_USE + " integer not null, " + + RepoTable.Cols.PRIORITY + " integer not null, " + + RepoTable.Cols.SIGNING_CERT + " text, " + + RepoTable.Cols.FINGERPRINT + " text, " + + RepoTable.Cols.MAX_AGE + " integer not null default 0, " + + RepoTable.Cols.VERSION + " integer not null default 0, " + + RepoTable.Cols.LAST_ETAG + " text, " + + RepoTable.Cols.LAST_UPDATED + " string);"; + + db.execSQL(createTableDdl); + + String nonIdFields = TextUtils.join(", ", new String[] { + RepoTable.Cols.ADDRESS, + RepoTable.Cols.NAME, + RepoTable.Cols.DESCRIPTION, + RepoTable.Cols.IN_USE, + RepoTable.Cols.PRIORITY, + RepoTable.Cols.SIGNING_CERT, + RepoTable.Cols.FINGERPRINT, + RepoTable.Cols.MAX_AGE, + RepoTable.Cols.VERSION, + RepoTable.Cols.LAST_ETAG, + RepoTable.Cols.LAST_UPDATED, + }); + + String insertSql = "INSERT INTO " + RepoTable.NAME + + "(" + RepoTable.Cols._ID + ", " + nonIdFields + " ) " + + "SELECT id, " + nonIdFields + " FROM " + tempTableName + ";"; + + db.execSQL(insertSql); + db.execSQL("DROP TABLE " + tempTableName + ";"); + db.setTransactionSuccessful(); + } catch (Exception e) { + Log.e(TAG, "Error renaming id to " + RepoTable.Cols._ID, e); + } + db.endTransaction(); + } + private void addIsSwapToRepo(SQLiteDatabase db, int oldVersion) { if (oldVersion >= 47 || columnExists(db, RepoTable.NAME, RepoTable.Cols.IS_SWAP)) { return; @@ -990,4 +956,38 @@ class DBHelper extends SQLiteOpenHelper { return exists; } + private void insertRepo(SQLiteDatabase db, String name, String address, + String description, String version, String enabled, + String priority, String pushRequests, String pubKey) { + ContentValues values = new ContentValues(); + values.put(RepoTable.Cols.ADDRESS, address); + values.put(RepoTable.Cols.NAME, name); + values.put(RepoTable.Cols.DESCRIPTION, description); + values.put(RepoTable.Cols.SIGNING_CERT, pubKey); + values.put(RepoTable.Cols.FINGERPRINT, Utils.calcFingerprint(pubKey)); + values.put(RepoTable.Cols.MAX_AGE, 0); + values.put(RepoTable.Cols.VERSION, Utils.parseInt(version, 0)); + values.put(RepoTable.Cols.IN_USE, Utils.parseInt(enabled, 0)); + values.put(RepoTable.Cols.PRIORITY, Utils.parseInt(priority, Integer.MAX_VALUE)); + values.put(RepoTable.Cols.LAST_ETAG, (String) null); + values.put(RepoTable.Cols.TIMESTAMP, 0); + + switch (pushRequests) { + case "ignore": + values.put(RepoTable.Cols.PUSH_REQUESTS, Repo.PUSH_REQUEST_IGNORE); + break; + case "prompt": + values.put(RepoTable.Cols.PUSH_REQUESTS, Repo.PUSH_REQUEST_PROMPT); + break; + case "always": + values.put(RepoTable.Cols.PUSH_REQUESTS, Repo.PUSH_REQUEST_ACCEPT_ALWAYS); + break; + default: + throw new IllegalArgumentException(pushRequests + " is not a supported option!"); + } + + Utils.debugLog(TAG, "Adding repository " + name + " with push requests as " + pushRequests); + db.insert(RepoTable.NAME, null, values); + } + } From d65e72638bb26fd47c3921c5dfb5a704a2e5363b Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Wed, 19 Oct 2016 06:44:08 +1100 Subject: [PATCH 3/3] Reimplement `columnExists` using `PRAGMA table_info`. For some reason, the existing approach of "select * and then see if the column of interest is in the results set" didn't work as expected under tests. Perhaps SQLite is caching the list of columns for the purpose of `select *` even after running an `alter table add column` query? Either way, I couldn't figure out why it wasn't working as expected. This left us with two options: * Try to `select columnToCheck` and see if it throws an exception * Query columns using `PRAGMA table_info. The exception thrown when a column doesn't exist is not specific enough for our code to check that this is the exact exception that occured. It is not possible to say: `try { ... } catch (SQLiteColumnNotFound e) { ...}` unfotunately. Also, there is a cost associated with unwinding the stack to process an exception, which means exceptions probably shouldn't be used in unexceptional circumstances such as this. This change instead uses `PRAGMA table_info(tableOfInterest)` and then iterates over the cursor looking for the relevant column. Even if the performance is worse than the stack unwinding of an exception, it is more concise and less hacky. --- .../java/org/fdroid/fdroid/data/DBHelper.java | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 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 000b98b1e..5de7f6aea 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/DBHelper.java +++ b/app/src/main/java/org/fdroid/fdroid/data/DBHelper.java @@ -940,11 +940,20 @@ class DBHelper extends SQLiteOpenHelper { + Repo.PUSH_REQUEST_IGNORE); } - private static boolean columnExists(SQLiteDatabase db, String table, String column) { - Cursor cursor = db.rawQuery("select * from " + table + " limit 0,1", null); - boolean exists = cursor.getColumnIndex(column) != -1; + private static boolean columnExists(SQLiteDatabase db, String table, String field) { + boolean found = false; + Cursor cursor = db.rawQuery("PRAGMA table_info(" + table + ")", null); + cursor.moveToFirst(); + while (!cursor.isAfterLast()) { + String name = cursor.getString(cursor.getColumnIndex("name")); + if (name.equalsIgnoreCase(field)) { + found = true; + break; + } + cursor.moveToNext(); + } cursor.close(); - return exists; + return found; } private static boolean tableExists(SQLiteDatabase db, String table) {