[Offline pages] OPMStoreSQL: Upgrade to M55, Upgrade refactoring, conversion refactoring

components/offline_pages/offline_page_metadata_store_sql.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/offline_pages/offline_page_metadata_store_sql.h"
 
 #include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/location.h"
@@ skipping 15 matching lines @@
 
 // This is a macro instead of a const so that
 // it can be used inline in other SQL statements below.
 #define OFFLINE_PAGES_TABLE_NAME "offlinepages_v1"
 
 bool CreateOfflinePagesTable(sql::Connection* db) {
   const char kSql[] = "CREATE TABLE IF NOT EXISTS " OFFLINE_PAGES_TABLE_NAME
                       "(offline_id INTEGER PRIMARY KEY NOT NULL,"
                       " creation_time INTEGER NOT NULL,"
                       " file_size INTEGER NOT NULL,"
-                      " version INTEGER NOT NULL,"
                       " last_access_time INTEGER NOT NULL,"
                       " access_count INTEGER NOT NULL,"
-                      // TODO(fgorski): Remove. Never used.
-                      " status INTEGER NOT NULL DEFAULT 0,"
-                      // A note on this field:  It will be NULL for now and is
-                      // reserved for later use.  We will treat NULL as
-                      // "Unknown" in any subsequent queries for user_initiated
-                      // values.
-                      " user_initiated INTEGER,"  // this is actually a boolean
                       " expiration_time INTEGER NOT NULL DEFAULT 0,"
                       " client_namespace VARCHAR NOT NULL,"
                       " client_id VARCHAR NOT NULL,"
                       " online_url VARCHAR NOT NULL,"
-                      // TODO(fgorski): Remove. Never used.
-                      " offline_url VARCHAR NOT NULL DEFAULT '',"
                       " file_path VARCHAR NOT NULL,"
                       " title VARCHAR NOT NULL DEFAULT ''"
                       ")";
   return db->Execute(kSql);
 }
 
-bool RefreshColumnsFrom52To54(sql::Connection* db) {
+bool UpgradeWithQuery(sql::Connection* db, const char* upgrade_sql) {
   if (!db->Execute("ALTER TABLE " OFFLINE_PAGES_TABLE_NAME
                    " RENAME TO temp_" OFFLINE_PAGES_TABLE_NAME)) {
     return false;
   }
   if (!CreateOfflinePagesTable(db))
     return false;
-  if (!db->Execute(
-          "INSERT INTO " OFFLINE_PAGES_TABLE_NAME
-          " (offline_id, creation_time, file_size, version, last_access_time, "
-          "access_count, status, user_initiated, client_namespace, client_id, "
-          "online_url, offline_url, file_path) "
-          "SELECT offline_id, creation_time, file_size, version, "
-          "last_access_time, "
-          "access_count, status, user_initiated, client_namespace, client_id, "
-          "online_url, offline_url, file_path "
-          "FROM temp_" OFFLINE_PAGES_TABLE_NAME)) {
+  if (!db->Execute(upgrade_sql))
     return false;
-  }
   if (!db->Execute("DROP TABLE IF EXISTS temp_" OFFLINE_PAGES_TABLE_NAME))
     return false;
-
   return true;
 }
 
-bool RefreshColumnsFrom53To54(sql::Connection* db) {
-  if (!db->Execute("ALTER TABLE " OFFLINE_PAGES_TABLE_NAME
-                   " RENAME TO temp_" OFFLINE_PAGES_TABLE_NAME)) {
-    return false;
-  }
-  if (!CreateOfflinePagesTable(db))
-    return false;
-  if (!db->Execute(
-          "INSERT INTO " OFFLINE_PAGES_TABLE_NAME
-          " (offline_id, creation_time, file_size, version, last_access_time, "
-          "access_count, status, user_initiated, client_namespace, client_id, "
-          "online_url, offline_url, file_path, expiration_time) "
-          "SELECT offline_id, creation_time, file_size, version, "
-          "last_access_time, "
-          "access_count, status, user_initiated, client_namespace, client_id, "
-          "online_url, offline_url, file_path, expiration_time "
-          "FROM temp_" OFFLINE_PAGES_TABLE_NAME)) {
-    return false;
-  }
-  if (!db->Execute("DROP TABLE IF EXISTS temp_" OFFLINE_PAGES_TABLE_NAME))
-    return false;
+bool UpgradeFrom52(sql::Connection* db) {
+  const char kSql[] =
+      "INSERT INTO " OFFLINE_PAGES_TABLE_NAME
+      " (offline_id, creation_time, file_size, last_access_time, "
+      "access_count, client_namespace, client_id, "
+      "online_url, file_path) "
+      "SELECT "
+      "offline_id, creation_time, file_size, last_access_time, "
+      "access_count, client_namespace, client_id, "
+      "online_url, file_path "
+      "FROM temp_" OFFLINE_PAGES_TABLE_NAME;
+  return UpgradeWithQuery(db, kSql);
+}
 
-  return true;
+bool UpgradeFrom53(sql::Connection* db) {
+  const char kSql[] =
+      "INSERT INTO " OFFLINE_PAGES_TABLE_NAME
+      " (offline_id, creation_time, file_size, last_access_time, "
+      "access_count, expiration_time, client_namespace, client_id, "
+      "online_url, file_path) "
+      "SELECT "
+      "offline_id, creation_time, file_size, last_access_time, "
+      "access_count, expiration_time, client_namespace, client_id, "
+      "online_url, file_path "
+      "FROM temp_" OFFLINE_PAGES_TABLE_NAME;
+  return UpgradeWithQuery(db, kSql);
+}
+
+bool UpgradeFrom54(sql::Connection* db) {
+  const char kSql[] =
+      "INSERT INTO " OFFLINE_PAGES_TABLE_NAME
+      " (offline_id, creation_time, file_size, last_access_time, "
+      "access_count, expiration_time, client_namespace, client_id, "
+      "online_url, file_path, title) "
+      "SELECT "
+      "offline_id, creation_time, file_size, last_access_time, "
+      "access_count, expiration_time, client_namespace, client_id, "
+      "online_url, file_path, title "
+      "FROM temp_" OFFLINE_PAGES_TABLE_NAME;
+  return UpgradeWithQuery(db, kSql);
 }
 
 bool CreateSchema(sql::Connection* db) {
   // If you create a transaction but don't Commit() it is automatically
   // rolled back by its destructor when it falls out of scope.
   sql::Transaction transaction(db);
   if (!transaction.Begin())
     return false;
 
   if (!db->DoesTableExist(OFFLINE_PAGES_TABLE_NAME)) {
     if (!CreateOfflinePagesTable(db))
       return false;
   }
 
+  // Upgrade section. Details are described in the header file.
   if (!db->DoesColumnExist(OFFLINE_PAGES_TABLE_NAME, "expiration_time")) {
-    if (!RefreshColumnsFrom52To54(db))
+    if (!UpgradeFrom52(db))
       return false;
   } else if (!db->DoesColumnExist(OFFLINE_PAGES_TABLE_NAME, "title")) {
-    if (!RefreshColumnsFrom53To54(db))
+    if (!UpgradeFrom53(db))
+      return false;
+  } else if (db->DoesColumnExist(OFFLINE_PAGES_TABLE_NAME, "offline_url")) {
+    if (!UpgradeFrom54(db))
       return false;
   }
 
   // TODO(bburns): Add indices here.
   return transaction.Commit();
 }
 
 bool DeleteByOfflineId(sql::Connection* db, int64_t offline_id) {
   static const char kSql[] =
       "DELETE FROM " OFFLINE_PAGES_TABLE_NAME " WHERE offline_id=?";
   sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
   statement.BindInt64(0, offline_id);
   return statement.Run();
 }
 
+base::FilePath GetPathFromUTF8String(const std::string& path_string) {
+#if defined(OS_POSIX)
+  return base::FilePath(path_string);
+#elif defined(OS_WIN)
+  return base::FilePath(base::UTF8ToWide(path_string));
+#else
+#error Unknown OS
+#endif
+}
+
+std::string GetUTF8StringFromPath(const base::FilePath& path) {
+#if defined(OS_POSIX)
+  return path.value();
+#elif defined(OS_WIN)
+  return base::WideToUTF8(path.value());
+#else
+#error Unknown OS
+#endif
+}
+
 // Create an offline page item from a SQL result.  Expects complete rows with
 // all columns present.
 OfflinePageItem MakeOfflinePageItem(sql::Statement* statement) {
   int64_t id = statement->ColumnInt64(0);
   base::Time creation_time =
       base::Time::FromInternalValue(statement->ColumnInt64(1));
   int64_t file_size = statement->ColumnInt64(2);
-  ClientId client_id(statement->ColumnString(9),
-                     statement->ColumnString(10));
-  GURL url(statement->ColumnString(11));
-#if defined(OS_POSIX)
-  base::FilePath path(statement->ColumnString(13));
-#elif defined(OS_WIN)
-  base::FilePath path(base::UTF8ToWide(statement->ColumnString(13)));
-#else
-#error Unknown OS
-#endif
+  base::Time last_access_time =
+      base::Time::FromInternalValue(statement->ColumnInt64(3));
+  int access_count = statement->ColumnInt(4);
+  base::Time expiration_time =
+      base::Time::FromInternalValue(statement->ColumnInt64(5));
+  ClientId client_id(statement->ColumnString(6), statement->ColumnString(7));
+  GURL url(statement->ColumnString(8));
+  base::FilePath path(GetPathFromUTF8String(statement->ColumnString(9)));
+  base::string16 title = statement->ColumnString16(10);
 
   OfflinePageItem item(url, id, client_id, path, file_size, creation_time);
-  item.version = statement->ColumnInt(3);
-  item.last_access_time = base::Time::FromInternalValue(
-      statement->ColumnInt64(4));
-  item.access_count = statement->ColumnInt(5);
-  item.expiration_time =
-      base::Time::FromInternalValue(statement->ColumnInt64(8));
-  item.title = statement->ColumnString16(14);
+  item.last_access_time = last_access_time;
+  item.access_count = access_count;
+  item.expiration_time = expiration_time;
+  item.title = title;
   return item;
 }
 
 bool InsertOrReplace(sql::Connection* db, const OfflinePageItem& item) {
   const char kSql[] =
       "INSERT OR REPLACE INTO " OFFLINE_PAGES_TABLE_NAME
       " (offline_id, online_url, client_namespace, client_id, file_path, "
-      "file_size, creation_time, last_access_time, version, access_count, "
+      "file_size, creation_time, last_access_time, access_count, "
       "expiration_time, title)"
       " VALUES "
-      " (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)";
+      " (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)";
 
   sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
   statement.BindInt64(0, item.offline_id);
   statement.BindString(1, item.url.spec());
   statement.BindString(2, item.client_id.name_space);
   statement.BindString(3, item.client_id.id);
-#if defined(OS_POSIX)
-  std::string path_string = item.file_path.value();
-#elif defined(OS_WIN)
-  std::string path_string = base::WideToUTF8(item.file_path.value());
-#else
-#error Unknown OS
-#endif
-  statement.BindString(4, path_string);
+  statement.BindString(4, GetUTF8StringFromPath(item.file_path));
   statement.BindInt64(5, item.file_size);
   statement.BindInt64(6, item.creation_time.ToInternalValue());
   statement.BindInt64(7, item.last_access_time.ToInternalValue());
-  statement.BindInt(8, item.version);
-  statement.BindInt(9, item.access_count);
-  statement.BindInt64(10, item.expiration_time.ToInternalValue());
-  statement.BindString16(11, item.title);
+  statement.BindInt(8, item.access_count);
+  statement.BindInt64(9, item.expiration_time.ToInternalValue());
+  statement.BindString16(10, item.title);
   return statement.Run();
 }
 
 bool InitDatabase(sql::Connection* db, base::FilePath path) {
   db->set_page_size(4096);
   db->set_cache_size(500);
   db->set_histogram_tag("OfflinePageMetadata");
   db->set_exclusive_locking();
 
   base::File::Error err;
@@ skipping 52 matching lines @@
   if (statement.Succeeded()) {
     NotifyLoadResult(runner, callback, OfflinePageMetadataStore::LOAD_SUCCEEDED,
                      result);
   } else {
     result.clear();
     NotifyLoadResult(runner, callback,
                      OfflinePageMetadataStore::STORE_LOAD_FAILED, result);
   }
 }
 
 void AddOrUpdateOfflinePageSync(

[dewittj, 2016/09/13 21:43:51]

nit: I'd avoid moving these functions just because, screws up blaming.  (not a
strong opinion so feel free to ignore)

[fgorski, 2016/09/13 22:08:59]

Move had to happen to clean up the interface and blaming bburns@ for anything
wouldn't work much anyway ;)

     const OfflinePageItem& offline_page,
     sql::Connection* db,
     scoped_refptr<base::SingleThreadTaskRunner> runner,
     const OfflinePageMetadataStore::UpdateCallback& callback) {
   // TODO(bburns): add UMA metrics here (and for levelDB).

[dewittj, 2016/09/13 21:43:51]

But if you do move the functions, it might be wise to retarget the TODO :)

[fgorski, 2016/09/13 22:08:59]

I'll get to that later.

   bool ok = InsertOrReplace(db, offline_page);
   runner->PostTask(FROM_HERE, base::Bind(callback, ok));
 }
 
 void RemoveOfflinePagesSync(
     const std::vector<int64_t>& offline_ids,
     sql::Connection* db,
     scoped_refptr<base::SingleThreadTaskRunner> runner,
     const OfflinePageMetadataStore::UpdateCallback& callback) {
   // TODO(bburns): add UMA metrics here (and for levelDB).
@@ skipping 139 matching lines @@
   DCHECK(db_.get());
   if (!db_.get()) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
                                                   base::Bind(callback));
     return false;
   }
   return true;
 }
 
 }  // namespace offline_pages