Autofill database migration to clean up bogus profiles.

chrome/browser/webdata/web_database_unittest.cc

Index: chrome/browser/webdata/web_database_unittest.cc
diff --git a/chrome/browser/webdata/web_database_unittest.cc b/chrome/browser/webdata/web_database_unittest.cc
index c37f75c20cbdfa306a4740f96e4f44f98239b3c2..61b6fa7c9de2155c15c07f8979fc8c1810869866 100644
--- a/chrome/browser/webdata/web_database_unittest.cc
+++ b/chrome/browser/webdata/web_database_unittest.cc
@@ -1535,6 +1535,95 @@ TEST_F(WebDatabaseTest, AutofillProfile) {
   EXPECT_FALSE(db.GetAutofillProfile(billing_profile.guid(), &db_profile));
 }
 
+TEST_F(WebDatabaseTest, AutofillProfileTrash) {
+  WebDatabase db;
+
+  ASSERT_EQ(sql::INIT_OK, db.Init(file_));
+
+  std::vector<std::string> guids;
+  db.GetAutofillProfilesInTrash(&guids);
+  EXPECT_TRUE(guids.empty());
+
+  ASSERT_TRUE(
+      db.AddAutofillGUIDToTrash("00000000-0000-0000-0000-000000000000"));
+  ASSERT_TRUE(
+      db.AddAutofillGUIDToTrash("00000000-0000-0000-0000-000000000001"));
+  ASSERT_TRUE(db.GetAutofillProfilesInTrash(&guids));
+  EXPECT_EQ(2UL, guids.size());
+  EXPECT_EQ("00000000-0000-0000-0000-000000000000", guids[0]);
+  EXPECT_EQ("00000000-0000-0000-0000-000000000001", guids[1]);
+
+  ASSERT_TRUE(db.EmptyAutofillProfilesTrash());
+  ASSERT_TRUE(db.GetAutofillProfilesInTrash(&guids));
+  EXPECT_TRUE(guids.empty());
+}
+
+TEST_F(WebDatabaseTest, AutofillProfileTrashInteraction) {
+  WebDatabase db;
+
+  ASSERT_EQ(sql::INIT_OK, db.Init(file_));
+
+  std::vector<std::string> guids;
+  db.GetAutofillProfilesInTrash(&guids);
+  EXPECT_TRUE(guids.empty());
+
+  AutofillProfile profile;
+  profile.SetInfo(AutofillType(NAME_FIRST), ASCIIToUTF16("John"));
+  profile.SetInfo(AutofillType(NAME_MIDDLE), ASCIIToUTF16("Q."));
+  profile.SetInfo(AutofillType(NAME_LAST), ASCIIToUTF16("Smith"));
+  profile.SetInfo(AutofillType(EMAIL_ADDRESS),ASCIIToUTF16("js@smith.xyz"));
+  profile.SetInfo(AutofillType(ADDRESS_HOME_LINE1), ASCIIToUTF16("1 Main St"));
+  profile.SetInfo(AutofillType(ADDRESS_HOME_CITY), ASCIIToUTF16("Los Angeles"));
+  profile.SetInfo(AutofillType(ADDRESS_HOME_STATE), ASCIIToUTF16("CA"));
+  profile.SetInfo(AutofillType(ADDRESS_HOME_ZIP), ASCIIToUTF16("90025"));
+  profile.SetInfo(AutofillType(ADDRESS_HOME_COUNTRY), ASCIIToUTF16("US"));
+
+  // Mark this profile as in the trash.  This stops |AddAutofillProfile| from
+  // adding it.
+  EXPECT_TRUE(db.AddAutofillGUIDToTrash(profile.guid()));
+  EXPECT_TRUE(db.AddAutofillProfile(profile));
+  AutofillProfile* added_profile = NULL;
+  EXPECT_FALSE(db.GetAutofillProfile(profile.guid(), &added_profile));
+  EXPECT_EQ(static_cast<AutofillProfile*>(NULL), added_profile);
+
+  // Add the profile for real this time.
+  EXPECT_TRUE(db.EmptyAutofillProfilesTrash());
+  EXPECT_TRUE(db.GetAutofillProfilesInTrash(&guids));
+  EXPECT_TRUE(guids.empty());
+  EXPECT_TRUE(db.AddAutofillProfile(profile));
+  EXPECT_TRUE(db.GetAutofillProfile(profile.guid(), &added_profile));
+  ASSERT_NE(static_cast<AutofillProfile*>(NULL), added_profile);
+  delete added_profile;
+
+  // Mark this profile as in the trash.  This stops |UpdateAutofillProfile| from
+  // updating it.
+  EXPECT_TRUE(db.AddAutofillGUIDToTrash(profile.guid()));

[Ilya Sherman, 2011/03/16 04:58:57]

I would consider this a database error -- we shouldn't ever have a profile both
in the profiles table and in the trash table... should we?

[dhollowa, 2011/03/16 16:13:35]

That is correct.  This is just a way to test the interaction.  I've added
comment.

On 2011/03/16 04:58:57, Ilya Sherman wrote:

+  profile.SetInfo(AutofillType(NAME_FIRST), ASCIIToUTF16("Jane"));
+  EXPECT_TRUE(db.UpdateAutofillProfile(profile));
+  AutofillProfile* updated_profile = NULL;
+  EXPECT_TRUE(db.GetAutofillProfile(profile.guid(), &updated_profile));
+  ASSERT_NE(static_cast<AutofillProfile*>(NULL), added_profile);
+  EXPECT_EQ(ASCIIToUTF16("John"),
+            updated_profile->GetFieldText(AutofillType(NAME_FIRST)));
+  delete updated_profile;
+
+  // Try to delete the trashed profile.  This stops |RemoveAutofillProfile| from
+  // deleting it.  In normal operation deletion is done by migration step, and
+  // removal from trash is done by |WebDataService|.
+  EXPECT_TRUE(db.RemoveAutofillProfile(profile.guid()));
+  AutofillProfile* removed_profile = NULL;
+  EXPECT_TRUE(db.GetAutofillProfile(profile.guid(), &removed_profile));
+  ASSERT_NE(static_cast<AutofillProfile*>(NULL), removed_profile);
+  delete removed_profile;
+
+  // Check that emptying the trash now allows removal to occur.
+  EXPECT_TRUE(db.EmptyAutofillProfilesTrash());
+  EXPECT_TRUE(db.RemoveAutofillProfile(profile.guid()));
+  removed_profile = NULL;
+  EXPECT_FALSE(db.GetAutofillProfile(profile.guid(), &removed_profile));
+  EXPECT_EQ(static_cast<AutofillProfile*>(NULL), removed_profile);
+}
+
 TEST_F(WebDatabaseTest, CreditCard) {
   WebDatabase db;
 
@@ -2104,7 +2193,7 @@ class WebDatabaseMigrationTest : public testing::Test {
   DISALLOW_COPY_AND_ASSIGN(WebDatabaseMigrationTest);
 };
 
-const int WebDatabaseMigrationTest::kCurrentTestedVersionNumber = 35;
+const int WebDatabaseMigrationTest::kCurrentTestedVersionNumber = 36;
 
 void WebDatabaseMigrationTest::LoadDatabase(const FilePath::StringType& file) {
   std::string contents;
@@ -2983,16 +3072,7 @@ TEST_F(WebDatabaseMigrationTest, MigrateVersion32ToCurrent) {
     EXPECT_EQ(1297882100L, s1.ColumnInt64(8));
 
     // Alfred E Newman.
-    ASSERT_TRUE(s1.Step());
-    EXPECT_EQ("584282AC-5D21-8D73-A2DB-4F892EF61F3F", s1.ColumnString(0));
-    EXPECT_EQ(ASCIIToUTF16(""), s1.ColumnString16(1));
-    EXPECT_EQ(ASCIIToUTF16(""), s1.ColumnString16(2));
-    EXPECT_EQ(ASCIIToUTF16(""), s1.ColumnString16(3));
-    EXPECT_EQ(ASCIIToUTF16(""), s1.ColumnString16(4));
-    EXPECT_EQ(ASCIIToUTF16(""), s1.ColumnString16(5));
-    EXPECT_EQ(ASCIIToUTF16(""), s1.ColumnString16(6));
-    EXPECT_EQ(ASCIIToUTF16(""), s1.ColumnString16(7));
-    EXPECT_EQ(1297882100L, s1.ColumnInt64(8));
+    // Gets culled during migration from 35 to 36 due to incomplete address.
 
     // 3 Main St.
     ASSERT_TRUE(s1.Step());
@@ -3043,11 +3123,7 @@ TEST_F(WebDatabaseMigrationTest, MigrateVersion32ToCurrent) {
     EXPECT_EQ(ASCIIToUTF16("Smith"), s2.ColumnString16(3));
 
     // Alfred E Newman.
-    ASSERT_TRUE(s2.Step());
-    EXPECT_EQ("584282AC-5D21-8D73-A2DB-4F892EF61F3F", s2.ColumnString(0));
-    EXPECT_EQ(ASCIIToUTF16("Alfred"), s2.ColumnString16(1));
-    EXPECT_EQ(ASCIIToUTF16("E"), s2.ColumnString16(2));
-    EXPECT_EQ(ASCIIToUTF16("Newman"), s2.ColumnString16(3));
+    // Gets culled during migration from 35 to 36 due to incomplete address.
 
     // Note no name for 3 Main St.
 
@@ -3073,9 +3149,7 @@ TEST_F(WebDatabaseMigrationTest, MigrateVersion32ToCurrent) {
     // Note no email for 2 Main St.
 
     // Alfred E Newman.
-    ASSERT_TRUE(s3.Step());
-    EXPECT_EQ("584282AC-5D21-8D73-A2DB-4F892EF61F3F", s3.ColumnString(0));
-    EXPECT_EQ(ASCIIToUTF16("a@e.com"), s3.ColumnString16(1));
+    // Gets culled during migration from 35 to 36 due to incomplete address.
 
     // Note no email for 3 Main St.
 
@@ -3231,3 +3305,94 @@ TEST_F(WebDatabaseMigrationTest, MigrateVersion34ToCurrent) {
     ASSERT_FALSE(s.Step());
   }
 }
+
+// Cleans up invalid profiles based on more agressive merging.  Filters out
+// profiles that are subsets of other profiles, and profiles with invalid email,
+// state, and incomplete address.
+TEST_F(WebDatabaseMigrationTest, MigrateVersion35ToCurrent) {
+  // Initialize the database.
+  ASSERT_NO_FATAL_FAILURE(LoadDatabase(FILE_PATH_LITERAL("version_35.sql")));
+
+  // Verify pre-conditions. These are expectations for version 34 of the
+  // database.
+  {
+    sql::Connection connection;
+    ASSERT_TRUE(connection.Open(GetDatabasePath()));
+
+    EXPECT_FALSE(connection.DoesTableExist("autofill_profiles_trash"));
+    ASSERT_TRUE(connection.DoesColumnExist("autofill_profiles", "guid"));
+
+    // Check that there are 6 profiles prior to merge.
+    sql::Statement s(
+        connection.GetUniqueStatement("SELECT guid FROM autofill_profiles"));
+    int i = 0;
+    while (s.Step())
+      ++i;
+    EXPECT_EQ(6, i);
+  }
+
+  // Load the database via the WebDatabase class and migrate the database to
+  // the current version.
+  {
+    WebDatabase db;
+    ASSERT_EQ(sql::INIT_OK, db.Init(GetDatabasePath()));
+  }
+
+  // Verify post-conditions.  These are expectations for current version of the
+  // database.
+  {
+    sql::Connection connection;
+    ASSERT_TRUE(connection.Open(GetDatabasePath()));
+
+    // Check version.
+    EXPECT_EQ(kCurrentTestedVersionNumber, VersionFromConnection(&connection));
+
+    ASSERT_TRUE(connection.DoesTableExist("autofill_profiles_trash"));
+    ASSERT_TRUE(connection.DoesColumnExist("autofill_profiles_trash", "guid"));
+    ASSERT_TRUE(connection.DoesColumnExist("autofill_profiles", "guid"));
+
+    // Verify data in the database after the migration.
+    sql::Statement s1(
+        connection.GetUniqueStatement(
+            "SELECT guid, company_name, address_line_1, address_line_2, "
+            "city, state, zipcode, country, date_modified "
+            "FROM autofill_profiles"));
+
+    // John Doe.
+    ASSERT_TRUE(s1.Step());
+    EXPECT_EQ("00000000-0000-0000-0000-000000000001", s1.ColumnString(0));
+    EXPECT_EQ(ASCIIToUTF16("Acme Inc."), s1.ColumnString16(1));
+    EXPECT_EQ(ASCIIToUTF16("1 Main Street"), s1.ColumnString16(2));
+    EXPECT_EQ(ASCIIToUTF16("Apt 2"), s1.ColumnString16(3));
+    EXPECT_EQ(ASCIIToUTF16("San Francisco"), s1.ColumnString16(4));
+    EXPECT_EQ(ASCIIToUTF16("CA"), s1.ColumnString16(5));
+    EXPECT_EQ(ASCIIToUTF16("94102"), s1.ColumnString16(6));
+    EXPECT_EQ(ASCIIToUTF16("United States"), s1.ColumnString16(7));
+    EXPECT_EQ(1300131704, s1.ColumnInt64(8));
+
+    // That should be it.
+    ASSERT_FALSE(s1.Step());
+
+    // Check that there 6 trashed profile after the merge.

[Ilya Sherman, 2011/03/16 04:58:57]

nit: 5 trashed profiles

[dhollowa, 2011/03/16 16:13:35]

Done.

+    sql::Statement s2(
+        connection.GetUniqueStatement("SELECT guid "
+                                      "FROM autofill_profiles_trash"));
+    ASSERT_TRUE(s2.Step());
+    EXPECT_EQ("00000000-0000-0000-0000-000000000002", s2.ColumnString(0));
+
+    ASSERT_TRUE(s2.Step());
+    EXPECT_EQ("00000000-0000-0000-0000-000000000003", s2.ColumnString(0));
+
+    ASSERT_TRUE(s2.Step());
+    EXPECT_EQ("00000000-0000-0000-0000-000000000004", s2.ColumnString(0));
+
+    ASSERT_TRUE(s2.Step());
+    EXPECT_EQ("00000000-0000-0000-0000-000000000005", s2.ColumnString(0));
+
+    ASSERT_TRUE(s2.Step());
+    EXPECT_EQ("00000000-0000-0000-0000-000000000006", s2.ColumnString(0));
+
+    // That should be it.
+    ASSERT_FALSE(s2.Step());
+  }
+}