[Passwords] Add post migration callback to SQLTableBuilder::MigrateFrom

This is to allow the calling site to introduce custom post migration
logic after each migration step.

Bug: 1032992
Change-Id: Id03ee9ee0a5a1a679f19ea7f7fbe572a6977f77a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2139726
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Dominic Battré <battre@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#758883}

components/password_manager/core/browser/login_database.cc

@@ -392,12 +392,60 @@ void InitializeBuilders(SQLTableBuilders builders) {
          "here.";
 }
 
+// Callback called upon each migration step of the logins table. It's used to
+// inject custom schema migration logic not covered by the generic
+// SQLTableBuilder migration. |new_version| indicates how far
+// SQLTableBuilder is in the migration process.
+bool LoginsTablePostMigrationStepCallback(sql::Database* db,
+                                          unsigned new_version) {
+  // In version 26, the primary key of the logins table became an
+  // AUTOINCREMENT field. Since SQLite doesn't allow changing the column type,
+  // the only way is to actually create a temp table with the primary key
+  // properly set as an AUTOINCREMENT field, and move the data there. The code
+  // has been adjusted such that newly created tables have the primary key
+  // properly set as AUTOINCREMENT.
+  if (new_version == 26) {
+    // This statement creates the logins database similar to version 26 with
+    // the primary key column set to AUTOINCREMENT.
+    const char temp_table_create_statement_version_26[] =
+        "CREATE TABLE logins_temp (origin_url VARCHAR NOT NULL,action_url "
+        "VARCHAR,username_element VARCHAR,username_value "
+        "VARCHAR,password_element VARCHAR,password_value BLOB,submit_element "
+        "VARCHAR,signon_realm VARCHAR NOT NULL,preferred INTEGER NOT "
+        "NULL,date_created INTEGER NOT NULL,blacklisted_by_user INTEGER NOT "
+        "NULL,scheme INTEGER NOT NULL,password_type INTEGER,times_used "
+        "INTEGER,form_data BLOB,date_synced INTEGER,display_name "
+        "VARCHAR,icon_url VARCHAR,federation_url VARCHAR,skip_zero_click "
+        "INTEGER,generation_upload_status INTEGER,possible_username_pairs "
+        "BLOB,id INTEGER PRIMARY KEY AUTOINCREMENT,date_last_used "
+        "INTEGER,UNIQUE (origin_url, username_element, username_value, "
+        "password_element, signon_realm))";
+    const char move_data_statement[] =
+        "INSERT INTO logins_temp SELECT * from logins";
+    const char drop_table_statement[] = "DROP TABLE logins";
+    const char rename_table_statement[] =
+        "ALTER TABLE logins_temp RENAME TO logins";
+
+    sql::Transaction transaction(db);
+    if (!(transaction.Begin() &&
+          db->Execute(temp_table_create_statement_version_26) &&
+          db->Execute(move_data_statement) &&
+          db->Execute(drop_table_statement) &&
+          db->Execute(rename_table_statement) && transaction.Commit())) {
+      return false;
+    }
+  }
+  return true;
+}
+
 // Call this after having called InitializeBuilders(), to migrate the database
 // from the current version to kCurrentVersionNumber.
 bool MigrateLogins(unsigned current_version,
                    SQLTableBuilders builders,
                    sql::Database* db) {
-  if (!builders.logins->MigrateFrom(current_version, db))
+  if (!builders.logins->MigrateFrom(
+          current_version, db,
+          base::BindRepeating(&LoginsTablePostMigrationStepCallback)))
     return false;
 
   if (!builders.sync_entities_metadata->MigrateFrom(current_version, db))
@@ -453,45 +501,6 @@ bool MigrateLogins(unsigned current_version,
       return false;
   }
 
-  // In version 26, the primary key of the logins table became an AUTOINCREMENT
-  // field. Since SQLite doesn't allow changing the column type, the only way is
-  // to actually create a temp table with the primary key propely set as an
-  // AUTOINCREMENT field, and move the data there. The code has been adjusted
-  // such that newly created tables have the primary key properly set as
-  // AUTOINCREMENT.
-  if (current_version < 26) {
-    // This statement creates the logins database similar to version 26 with the
-    // primary key column set to AUTOINCREMENT.
-    std::string temp_table_create_statement_version_26 =
-        "CREATE TABLE logins_temp (origin_url VARCHAR NOT NULL,action_url "
-        "VARCHAR,username_element VARCHAR,username_value "
-        "VARCHAR,password_element VARCHAR,password_value BLOB,submit_element "
-        "VARCHAR,signon_realm VARCHAR NOT NULL,preferred INTEGER NOT "
-        "NULL,date_created INTEGER NOT NULL,blacklisted_by_user INTEGER NOT "
-        "NULL,scheme INTEGER NOT NULL,password_type INTEGER,times_used "
-        "INTEGER,form_data BLOB,date_synced INTEGER,display_name "
-        "VARCHAR,icon_url VARCHAR,federation_url VARCHAR,skip_zero_click "
-        "INTEGER,generation_upload_status INTEGER,possible_username_pairs "
-        "BLOB,id INTEGER PRIMARY KEY AUTOINCREMENT,date_last_used "
-        "INTEGER,UNIQUE (origin_url, username_element, username_value, "
-        "password_element, signon_realm))";
-    std::string move_data_statement =
-        "INSERT INTO logins_temp SELECT * from logins";
-    std::string drop_table_statement = "DROP TABLE logins";
-    std::string rename_table_statement =
-        "ALTER TABLE logins_temp RENAME TO logins";
-
-    sql::Transaction transaction(db);
-    if (!(transaction.Begin() &&
-          db->Execute(temp_table_create_statement_version_26.c_str()) &&
-          db->Execute(move_data_statement.c_str()) &&
-          db->Execute(drop_table_statement.c_str()) &&
-          db->Execute(rename_table_statement.c_str()) &&
-          transaction.Commit())) {
-      return false;
-    }
-  }
-
   return true;
 }
 

components/password_manager/core/browser/sql_table_builder.cc

@@ -196,11 +196,17 @@ unsigned SQLTableBuilder::SealVersion() {
   return ++sealed_version_;
 }
 
-bool SQLTableBuilder::MigrateFrom(unsigned old_version,
-                                  sql::Database* db) const {
+bool SQLTableBuilder::MigrateFrom(
+    unsigned old_version,
+    sql::Database* db,
+    const base::RepeatingCallback<bool(sql::Database*, unsigned)>&
+        post_migration_step_callback) const {
   for (; old_version < sealed_version_; ++old_version) {
     if (!MigrateToNextFrom(old_version, db))
       return false;
+    if (post_migration_step_callback &&
+        !post_migration_step_callback.Run(db, old_version + 1))
+      return false;
   }
 
   return true;

components/password_manager/core/browser/sql_table_builder.h

-// 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.
+// 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.
 
 #ifndef COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_SQL_TABLE_BUILDER_H_
 #define COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_SQL_TABLE_BUILDER_H_
@@ -9,6 +9,7 @@
 #include <string>
 #include <vector>
 
+#include "base/bind_helpers.h"
 #include "base/macros.h"
 #include "base/strings/string_piece.h"
 
@@ -87,7 +88,17 @@ class SQLTableBuilder {
   // Assuming that the database connected through |db| contains a table called
   // |table_name_| in a state described by version |old_version|, migrates it to
   // the current version, which must be sealed. Returns true on success.
-  bool MigrateFrom(unsigned old_version, sql::Database* db) const;
+  // |post_migration_step_callback| is executed after each migration step in to
+  // allow the calling site to inject custom logic to run upon each migration
+  // step from |old_version| to the current version. The passed parameter
+  // corresponds to database to be migrated and the new version number that has
+  // been reached after the migration step. |post_migration_step_callback|
+  // returns true on success, otherwise the migration is aborted.
+  bool MigrateFrom(
+      unsigned old_version,
+      sql::Database* db,
+      const base::RepeatingCallback<bool(sql::Database*, unsigned)>&
+          post_migration_step_callback = base::NullCallback()) const;
 
   // If |db| connects to a database where table |table_name_| already exists,
   // this is a no-op and returns true. Otherwise, |table_name_| is created in a

components/password_manager/core/browser/sql_table_builder_unittest.cc

@@ -7,11 +7,13 @@
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/macros.h"
+#include "base/test/mock_callback.h"
 #include "sql/database.h"
 #include "sql/statement.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using ::testing::Return;
 using ::testing::UnorderedElementsAre;
 
 namespace password_manager {
@@ -310,4 +312,26 @@ TEST_F(SQLTableBuilderTest, MigrateFrom_AddPrimaryKey) {
       std::string::npos);
 }
 
+TEST_F(SQLTableBuilderTest, MigrateFromWithSuccessfulCallback) {
+  EXPECT_EQ(0u, builder()->SealVersion());
+  EXPECT_EQ(1u, builder()->SealVersion());
+
+  base::MockCallback<base::RepeatingCallback<bool(sql::Database*, unsigned)>>
+      migation_callback;
+
+  EXPECT_CALL(migation_callback, Run(db(), 1u)).WillOnce(Return(true));
+  EXPECT_TRUE(builder()->MigrateFrom(0, db(), migation_callback.Get()));
+}
+
+TEST_F(SQLTableBuilderTest, MigrateFromWithUnsuccessfulCallback) {
+  EXPECT_EQ(0u, builder()->SealVersion());
+  EXPECT_EQ(1u, builder()->SealVersion());
+
+  base::MockCallback<base::RepeatingCallback<bool(sql::Database*, unsigned)>>
+      migation_callback;
+
+  EXPECT_CALL(migation_callback, Run(db(), 1u)).WillOnce(Return(false));
+  EXPECT_FALSE(builder()->MigrateFrom(0, db(), migation_callback.Get()));
+}
+
 }  // namespace password_manager