From 6957721479ecefacf1e95e12343afc9a9f4fb304 Mon Sep 17 00:00:00 2001 From: Peter Serwylo <peter@serwylo.com> Date: Mon, 16 Nov 2015 17:32:46 +1100 Subject: [PATCH 1/5] CR: Leave old db update code as it was. This may not have caused any trouble, but the principle behind the old behaviour is that at the point that that was required, the fdroid_repo table had that particular structure. There is a small chance that it _may_ have some unintended consequences when upgrading clients with very old database versions. Probably not, but may as well leave it as is. --- F-Droid/src/org/fdroid/fdroid/data/DBHelper.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/F-Droid/src/org/fdroid/fdroid/data/DBHelper.java b/F-Droid/src/org/fdroid/fdroid/data/DBHelper.java index 3d301f62e..dc8f717e3 100644 --- a/F-Droid/src/org/fdroid/fdroid/data/DBHelper.java +++ b/F-Droid/src/org/fdroid/fdroid/data/DBHelper.java @@ -169,14 +169,12 @@ public class DBHelper extends SQLiteOpenHelper { + "maxage integer not null default 0, " + "version integer not null default 0, " + "lastetag text, " - + "lastUpdated string," - + "username string, password string" - + ");"; + + "lastUpdated string);"; db.execSQL(createTableDdl); String nonIdFields = "address, name, description, inuse, priority, " + - "pubkey, fingerprint, maxage, version, lastetag, lastUpdated, username, password"; + "pubkey, fingerprint, maxage, version, lastetag, lastUpdated"; String insertSql = "INSERT INTO " + TABLE_REPO + "(_id, " + nonIdFields + " ) " + From 97b60d937d63fd34a7cdd29f07ed81da47f95208 Mon Sep 17 00:00:00 2001 From: Peter Serwylo <peter@serwylo.com> Date: Mon, 16 Nov 2015 17:38:12 +1100 Subject: [PATCH 2/5] CR: Apply database changes in chronological order. As with the previous commit, there is probably not any harm doing this in the way it was done. However it helps reason about the code if changes are applied in the order that they were introduced. Especially because each of them does something depending on the version of the database at that point. With this change, you always know that at the point that the function is run, the database version will be 51 (and hence the structure of the database will be predictable). --- F-Droid/src/org/fdroid/fdroid/data/DBHelper.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/F-Droid/src/org/fdroid/fdroid/data/DBHelper.java b/F-Droid/src/org/fdroid/fdroid/data/DBHelper.java index dc8f717e3..1173517a9 100644 --- a/F-Droid/src/org/fdroid/fdroid/data/DBHelper.java +++ b/F-Droid/src/org/fdroid/fdroid/data/DBHelper.java @@ -283,11 +283,11 @@ public class DBHelper extends SQLiteOpenHelper { populateRepoNames(db, oldVersion); if (oldVersion < 43) createInstalledApp(db); addIsSwapToRepo(db, oldVersion); - addCredentialsToRepo(db, oldVersion); addChangelogToApp(db, oldVersion); addIconUrlLargeToApp(db, oldVersion); updateIconUrlLarge(db, oldVersion); recreateInstalledCache(db, oldVersion); + addCredentialsToRepo(db, oldVersion); } /** From 3426b3bb2d6c8326a6368ce1e110e8be4236b594 Mon Sep 17 00:00:00 2001 From: Peter Serwylo <peter@serwylo.com> Date: Mon, 16 Nov 2015 17:51:32 +1100 Subject: [PATCH 3/5] CR: Better deal with the possibility of crashes during database update. This is for an abundance of caution. If the guard condition checks for the presence of both username _and_ password fields, then a crash or some sort of force close during the update (after adding username but before password) will mean that next time the app runs, this condition will evaluate to false and the password field will never get added. --- F-Droid/src/org/fdroid/fdroid/data/DBHelper.java | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/F-Droid/src/org/fdroid/fdroid/data/DBHelper.java b/F-Droid/src/org/fdroid/fdroid/data/DBHelper.java index 1173517a9..1b89b0ab8 100644 --- a/F-Droid/src/org/fdroid/fdroid/data/DBHelper.java +++ b/F-Droid/src/org/fdroid/fdroid/data/DBHelper.java @@ -423,11 +423,16 @@ public class DBHelper extends SQLiteOpenHelper { } private void addCredentialsToRepo(SQLiteDatabase db, int oldVersion) { - if (oldVersion < 52 && !columnExists(db, TABLE_REPO, "username") && !columnExists(db, TABLE_REPO, "password")) { - Utils.debugLog(TAG, "Adding username field to " + TABLE_REPO + " table in db."); - db.execSQL("alter table " + TABLE_REPO + " add column username string;"); - Utils.debugLog(TAG, "Adding password field to " + TABLE_REPO + " table in db."); - db.execSQL("alter table " + TABLE_REPO + " add column password string;"); + if (oldVersion < 52) { + if (!columnExists(db, TABLE_REPO, "username")) { + Utils.debugLog(TAG, "Adding username field to " + TABLE_REPO + " table in db."); + db.execSQL("alter table " + TABLE_REPO + " add column username string;"); + } + + if (!columnExists(db, TABLE_REPO, "password")) { + Utils.debugLog(TAG, "Adding password field to " + TABLE_REPO + " table in db."); + db.execSQL("alter table " + TABLE_REPO + " add column password string;"); + } } } From 0b3b32dab3535b241fcd0002497bd92930ac9563 Mon Sep 17 00:00:00 2001 From: Peter Serwylo <peter@serwylo.com> Date: Mon, 16 Nov 2015 18:00:58 +1100 Subject: [PATCH 4/5] CR: Add overloaded `createNewRepo` method which provides default arguments. Instead of passing in `null` each time you don't have a username/password, this change provides those as meaningful default values in an overloaded version of the method. This takes care of Java's lack of default argument support. --- .../org/fdroid/fdroid/views/ManageReposActivity.java | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/F-Droid/src/org/fdroid/fdroid/views/ManageReposActivity.java b/F-Droid/src/org/fdroid/fdroid/views/ManageReposActivity.java index 96d664c85..c9c6e55b4 100644 --- a/F-Droid/src/org/fdroid/fdroid/views/ManageReposActivity.java +++ b/F-Droid/src/org/fdroid/fdroid/views/ManageReposActivity.java @@ -553,7 +553,7 @@ public class ManageReposActivity extends ActionBarActivity { } else { // create repo without username/password - createNewRepo(newAddress, fingerprint, null, null); + createNewRepo(newAddress, fingerprint); } } } @@ -570,7 +570,7 @@ public class ManageReposActivity extends ActionBarActivity { // or their internet is playing up, then you'd have to wait for several // connection timeouts before being able to proceed. - createNewRepo(originalAddress, fingerprint, null, null); + createNewRepo(originalAddress, fingerprint); checker.cancel(false); } }); @@ -606,6 +606,13 @@ public class ManageReposActivity extends ActionBarActivity { path, uri.getQuery(), uri.getFragment()).toString(); } + /** + * Create a repository without a username or password. + */ + private void createNewRepo(String address, String fingerprint) { + createNewRepo(address, fingerprint, null, null); + } + private void createNewRepo(String address, String fingerprint, final String username, final String password) { try { address = normalizeUrl(address); From ee7761e1af94f4d93dc72ee3d59c29168ffa55c2 Mon Sep 17 00:00:00 2001 From: Peter Serwylo <peter@serwylo.com> Date: Mon, 16 Nov 2015 18:03:08 +1100 Subject: [PATCH 5/5] CR: Replace String.isEmpty() with TextUtils.isEmpty(String). String.isEmpty() is only supported in API v9, whereas we target API v8 for now. --- F-Droid/src/org/fdroid/fdroid/views/RepoDetailsActivity.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/F-Droid/src/org/fdroid/fdroid/views/RepoDetailsActivity.java b/F-Droid/src/org/fdroid/fdroid/views/RepoDetailsActivity.java index e7c3d8223..dcfa63cf6 100644 --- a/F-Droid/src/org/fdroid/fdroid/views/RepoDetailsActivity.java +++ b/F-Droid/src/org/fdroid/fdroid/views/RepoDetailsActivity.java @@ -390,7 +390,7 @@ public class RepoDetailsActivity extends ActionBarActivity { final String name = nameInput.getText().toString(); final String password = passwordInput.getText().toString(); - if (name != null && !name.isEmpty()) { + if (!TextUtils.isEmpty(name)) { final ContentValues values = new ContentValues(2); values.put(RepoProvider.DataColumns.USERNAME, name);