WIP Bug 1320806 - Add type column to ActivityStream blocklist table draft
authorAndrzej Hunt <ahunt@mozilla.com>
Thu, 19 Jan 2017 11:53:04 +0100
changeset 463577 60a06668d59994d93dd7c8b2a76bdaee39ccc0b7
parent 463077 cb5e0bd0160973d345bf54913e6f0b336ac2afb7
child 542725 f78a3fade72fb57bca83f32f1eb0c6230eb83e30
push id42117
push userahunt@mozilla.com
push dateThu, 19 Jan 2017 10:55:08 +0000
bugs1320806
milestone53.0a1
WIP Bug 1320806 - Add type column to ActivityStream blocklist table MozReview-Commit-ID: G9lPXr0xdZN
mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java
mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java
--- a/mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java
+++ b/mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java
@@ -635,16 +635,20 @@ public class BrowserContract {
     public static final class ActivityStreamBlocklist implements CommonColumns {
         private ActivityStreamBlocklist() {}
 
         public static final String TABLE_NAME = "activity_stream_blocklist";
         public static final Uri CONTENT_URI = Uri.withAppendedPath(AUTHORITY_URI, TABLE_NAME);
 
         public static final String URL = "url";
         public static final String CREATED = "created";
+        public static final String TYPE = "type";
+
+        public static final int TYPE_TOPSITES = 0;
+        public static final int TYPE_HIGHLIGHTS = 1;
     }
 
     @RobocopTarget
     public static final class UrlAnnotations implements CommonColumns, DateSyncColumns {
         private UrlAnnotations() {}
 
         public static final String TABLE_NAME = "urlannotations";
         public static final Uri CONTENT_URI = Uri.withAppendedPath(AUTHORITY_URI, TABLE_NAME);
--- a/mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java
+++ b/mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java
@@ -928,17 +928,19 @@ public final class BrowserDatabaseHelper
     }
 
     private void createActivityStreamBlocklistTable(final SQLiteDatabase db) {
         debug("Creating " + ActivityStreamBlocklist.TABLE_NAME + " table");
 
         db.execSQL("CREATE TABLE " + ActivityStreamBlocklist.TABLE_NAME + "(" +
                    ActivityStreamBlocklist._ID + " INTEGER PRIMARY KEY AUTOINCREMENT, " +
                    ActivityStreamBlocklist.URL + " TEXT UNIQUE NOT NULL, " +
-                   ActivityStreamBlocklist.CREATED + " INTEGER NOT NULL)");
+                   ActivityStreamBlocklist.CREATED + " INTEGER NOT NULL, " +
+                   ActivityStreamBlocklist.TYPE + " INTEGER NOT NULL " +
+                   ")");
     }
 
     private void createReadingListTable(final SQLiteDatabase db, final String tableName) {
         debug("Creating " + TABLE_READING_LIST + " table");
 
         db.execSQL("CREATE TABLE " + tableName + "(" +
                    ReadingListItems._ID + " INTEGER PRIMARY KEY AUTOINCREMENT, " +
                    ReadingListItems.GUID + " TEXT UNIQUE, " +                          // Server-assigned.
@@ -1967,22 +1969,42 @@ public final class BrowserDatabaseHelper
     }
 
     private void upgradeDatabaseFrom33to34(final SQLiteDatabase db) {
         updateHistoryTableAddVisitAggregates(db);
         calculateHistoryTableVisitAggregates(db);
         createV34CombinedView(db);
     }
 
-    private void upgradeDatabaseFrom34to35(final SQLiteDatabase db) {
-        createActivityStreamBlocklistTable(db);
+    private void upgradeDatabaseFrom35to36(final SQLiteDatabase db) {
+        createPageMetadataTable(db);
     }
 
-    private void upgradeDatabaseFrom35to36(final SQLiteDatabase db) {
-        createPageMetadataTable(db);
+    private void addTypeColumnToActivityStreamBlocklist(final SQLiteDatabase db) {
+        // We're merely adding a column here, however doing it manually is the best choice:
+        // Under the hood, sqlite performs this operation by creating a temp table with the new schema,
+        // followed by copying data over - doing this ourselves does not result in a loss of performance.
+        // We specifically want to add a new column that is NOT NULL, but that does not have a default.
+        // This is impossible with the sqlite ADD column syntax: we can either set NOT NULL, in which case
+        // we must supply a DEFAULT (which we don't want); or we don't set a DEFAULT, and we can't use NOT NULL.
+        // Instead we explicityly create our own new table with desired schema, and do copy from a temp table
+        // where we can stuff in the desired value for our new column:
+        final String BLOCKLIST_TMP = ActivityStreamBlocklist.TABLE_NAME + "_tmp";
+
+        db.execSQL("ALTER TABLE " + ActivityStreamBlocklist.TABLE_NAME +
+                " RENAME TO " + BLOCKLIST_TMP);
+
+        createActivityStreamBlocklistTable(db);
+
+        db.execSQL("INSERT INTO " + ActivityStreamBlocklist.TABLE_NAME +
+                " SELECT *, " +
+                ActivityStreamBlocklist.TYPE_HIGHLIGHTS + " AS " + ActivityStreamBlocklist.TYPE +
+                " FROM " + ActivityStreamBlocklist.TABLE_NAME);
+
+        db.execSQL("DROP TABLE " + BLOCKLIST_TMP);
     }
 
     private void createV33CombinedView(final SQLiteDatabase db) {
         db.execSQL("DROP VIEW IF EXISTS " + VIEW_COMBINED);
         db.execSQL("DROP VIEW IF EXISTS " + VIEW_COMBINED_WITH_FAVICONS);
 
         createCombinedViewOn33(db);
     }
@@ -2103,23 +2125,39 @@ public final class BrowserDatabaseHelper
                 case 33:
                     upgradeDatabaseFrom32to33(db);
                     break;
 
                 case 34:
                     upgradeDatabaseFrom33to34(db);
                     break;
 
-                case 35:
-                    upgradeDatabaseFrom34to35(db);
-                    break;
+                // case 35:
+                // created the ActivityStreamBlocklist table, is handled separately below in a
+                // table-by-table upgrade process
 
                 case 36:
                     upgradeDatabaseFrom35to36(db);
                     break;
+
+                // case 37:
+                // adds type column to ActivityStreamBlocklist, is handled separately below
+            }
+
+            // Activity Stream blocklist migrations: it's easier to reason about migrations, and
+            // to write cleaner migration code, if we take into account individual iterations
+            // of a given table. This also means we only need to run one migration for any given
+            // table to get it from the input state to desired state (e.g. if the table doesn't
+            // exist, create it once with an up-to-date schema, if we have v1 of a table run one
+            // migration to bring it to the current schema, if we have v2 run a different migration
+            // to update it to the current schema, etc.).
+            if (oldVersion < 35) {
+                createActivityStreamBlocklistTable(db);
+            } else if (oldVersion >= 35 && oldVersion < 37) {
+                addTypeColumnToActivityStreamBlocklist(db);
             }
         }
 
         for (Table table : BrowserProvider.sTables) {
             table.onUpgrade(db, oldVersion, newVersion);
         }
 
         // Delete the obsolete favicon database after all other upgrades complete.