[sql] Remove misleading AutoRecoverTable() parameter.

|extend_columns| was intended to be used in the case where the target
schema differed slightly from the schema of the table being recovered.
Unfortunately, it actually implemented part of the solution for when the
target has fewer columns and described it as the solution for when the
target has more columns.  Remove the unnecessary code and parameter.

[In general, table schema only add new columns, removing is more
infrequent.]

BUG=none

Review URL: https://codereview.chromium.org/1666473003

Cr-Commit-Position: refs/heads/master@{#373586}

3 files changed, 33 insertions, 34 deletions

diff --git a/sql/recovery.cc b/sql/recovery.cc
index 5e101b1..5c4223c 100644
--- a/sql/recovery.cc
+++ b/sql/recovery.cc
@@ -349,7 +349,6 @@ void Recovery::Shutdown(Recovery::Disposition raze) {
 }
 
 bool Recovery::AutoRecoverTable(const char* table_name,
-                                size_t extend_columns,
                                 size_t* rows_recovered) {
   // Query the info for the recovered table in database [main].
   std::string query(
@@ -459,14 +458,6 @@ bool Recovery::AutoRecoverTable(const char* table_name,
   if (pk_column_count == 1 && !rowid_decl.empty())
     create_column_decls[rowid_ofs] = rowid_decl;
 
-  // Additional columns accept anything.
-  // TODO(shess): ignoreN isn't well namespaced.  But it will fail to
-  // execute in case of conflicts.
-  for (size_t i = 0; i < extend_columns; ++i) {
-    create_column_decls.push_back(
-        base::StringPrintf("ignore%" PRIuS " ANY", i));
-  }
-
   std::string recover_create(base::StringPrintf(
       "CREATE VIRTUAL TABLE temp.recover_%s USING recover(corrupt.%s, %s)",
       table_name,
diff --git a/sql/recovery.h b/sql/recovery.h
index c03ebb2..2c191cf 100644
--- a/sql/recovery.h
+++ b/sql/recovery.h
@@ -118,9 +118,8 @@ class SQL_EXPORT Recovery {
   // in database [main].  Data is copied using INSERT OR REPLACE, so
   // duplicates overwrite each other.
   //
-  // |extend_columns| allows recovering tables which have excess
-  // columns relative to the target schema.  The recover virtual table
-  // treats more data than specified as a sign of corruption.
+  // If the source table has fewer columns than the target, the target
+  // DEFAULT value will be used for those columns.
   //
   // Returns true if all operations succeeded, with the number of rows
   // recovered in |*rows_recovered|.
@@ -134,9 +133,7 @@ class SQL_EXPORT Recovery {
   //
   // TODO(shess): Flag for INSERT OR REPLACE vs IGNORE.
   // TODO(shess): Handle extended table names.
-  bool AutoRecoverTable(const char* table_name,
-                        size_t extend_columns,
-                        size_t* rows_recovered);
+  bool AutoRecoverTable(const char* table_name, size_t* rows_recovered);
 
   // Setup a recover virtual table at temp.recover_meta, reading from
   // corrupt.meta.  Returns true if created.
diff --git a/sql/recovery_unittest.cc b/sql/recovery_unittest.cc
index 11f2a87..73d146f 100644
--- a/sql/recovery_unittest.cc
+++ b/sql/recovery_unittest.cc
@@ -429,7 +429,7 @@ TEST_F(SQLRecoveryTest, AutoRecoverTable) {
         ExecuteWithResults(recovery->db(), kTempSchemaSql, "|", "\n"));
 
     size_t rows = 0;
-    EXPECT_TRUE(recovery->AutoRecoverTable("x", 0, &rows));
+    EXPECT_TRUE(recovery->AutoRecoverTable("x", &rows));
     EXPECT_EQ(2u, rows);
 
     // Test that any additional temp tables were cleaned up.
@@ -452,7 +452,7 @@ TEST_F(SQLRecoveryTest, AutoRecoverTable) {
 
     // TODO(shess): Should this failure implicitly lead to Raze()?
     size_t rows = 0;
-    EXPECT_FALSE(recovery->AutoRecoverTable("y", 0, &rows));
+    EXPECT_FALSE(recovery->AutoRecoverTable("y", &rows));
 
     sql::Recovery::Unrecoverable(std::move(recovery));
   }
@@ -509,7 +509,7 @@ TEST_F(SQLRecoveryTest, AutoRecoverTableWithDefault) {
     ASSERT_TRUE(recovery->db()->Execute(final_schema.c_str()));
 
     size_t rows = 0;
-    EXPECT_TRUE(recovery->AutoRecoverTable("x", 0, &rows));
+    EXPECT_TRUE(recovery->AutoRecoverTable("x", &rows));
     EXPECT_EQ(4u, rows);
 
     ASSERT_TRUE(sql::Recovery::Recovered(std::move(recovery)));
@@ -545,7 +545,7 @@ TEST_F(SQLRecoveryTest, AutoRecoverTableNullFilter) {
     ASSERT_TRUE(recovery->db()->Execute(kFinalSchema));
 
     size_t rows = 0;
-    EXPECT_TRUE(recovery->AutoRecoverTable("x", 0, &rows));
+    EXPECT_TRUE(recovery->AutoRecoverTable("x", &rows));
     EXPECT_EQ(1u, rows);
 
     ASSERT_TRUE(sql::Recovery::Recovered(std::move(recovery)));
@@ -584,7 +584,7 @@ TEST_F(SQLRecoveryTest, AutoRecoverTableWithRowid) {
     ASSERT_TRUE(recovery->db()->Execute(kCreateSql));
 
     size_t rows = 0;
-    EXPECT_TRUE(recovery->AutoRecoverTable("x", 0, &rows));
+    EXPECT_TRUE(recovery->AutoRecoverTable("x", &rows));
     EXPECT_EQ(2u, rows);
 
     ASSERT_TRUE(sql::Recovery::Recovered(std::move(recovery)));
@@ -629,7 +629,7 @@ TEST_F(SQLRecoveryTest, AutoRecoverTableWithCompoundKey) {
     ASSERT_TRUE(recovery->db()->Execute(kCreateSql));
 
     size_t rows = 0;
-    EXPECT_TRUE(recovery->AutoRecoverTable("x", 0, &rows));
+    EXPECT_TRUE(recovery->AutoRecoverTable("x", &rows));
     EXPECT_EQ(3u, rows);
 
     ASSERT_TRUE(sql::Recovery::Recovered(std::move(recovery)));
@@ -642,29 +642,40 @@ TEST_F(SQLRecoveryTest, AutoRecoverTableWithCompoundKey) {
   ASSERT_EQ(orig_data, ExecuteWithResults(&db(), kXSql, "|", "\n"));
 }
 
-// Test |extend_columns| support.
-TEST_F(SQLRecoveryTest, AutoRecoverTableExtendColumns) {
+// Test recovering from a table with fewer columns than the target.
+TEST_F(SQLRecoveryTest, AutoRecoverTableMissingColumns) {
   const char kCreateSql[] = "CREATE TABLE x (id INTEGER PRIMARY KEY, t0 TEXT)";
+  const char kAlterSql[] = "ALTER TABLE x ADD COLUMN t1 TEXT DEFAULT 't'";
   ASSERT_TRUE(db().Execute(kCreateSql));
   ASSERT_TRUE(db().Execute("INSERT INTO x VALUES (1, 'This is')"));
   ASSERT_TRUE(db().Execute("INSERT INTO x VALUES (2, 'That was')"));
 
-  // Save aside a copy of the original schema and data.
+  // Generate the expected info by faking a table to match what recovery will
+  // create.
   const std::string orig_schema(GetSchema(&db()));
   const char kXSql[] = "SELECT * FROM x ORDER BY 1";
-  const std::string orig_data(ExecuteWithResults(&db(), kXSql, "|", "\n"));
+  std::string expected_schema;
+  std::string expected_data;
+  {
+    ASSERT_TRUE(db().BeginTransaction());
+    ASSERT_TRUE(db().Execute(kAlterSql));
 
-  // Modify the table to add a column, and add data to that column.
-  ASSERT_TRUE(db().Execute("ALTER TABLE x ADD COLUMN t1 TEXT"));
-  ASSERT_TRUE(db().Execute("UPDATE x SET t1 = 'a test'"));
-  ASSERT_NE(orig_schema, GetSchema(&db()));
-  ASSERT_NE(orig_data, ExecuteWithResults(&db(), kXSql, "|", "\n"));
+    expected_schema = GetSchema(&db());
+    expected_data = ExecuteWithResults(&db(), kXSql, "|", "\n");
 
+    db().RollbackTransaction();
+  }
+
+  // Following tests are pointless if the rollback didn't work.
+  ASSERT_EQ(orig_schema, GetSchema(&db()));
+
+  // Recover the previous version of the table into the altered version.
   {
     scoped_ptr<sql::Recovery> recovery = sql::Recovery::Begin(&db(), db_path());
     ASSERT_TRUE(recovery->db()->Execute(kCreateSql));
+    ASSERT_TRUE(recovery->db()->Execute(kAlterSql));
     size_t rows = 0;
-    EXPECT_TRUE(recovery->AutoRecoverTable("x", 1, &rows));
+    EXPECT_TRUE(recovery->AutoRecoverTable("x", &rows));
     EXPECT_EQ(2u, rows);
     ASSERT_TRUE(sql::Recovery::Recovered(std::move(recovery)));
   }
@@ -672,8 +683,8 @@ TEST_F(SQLRecoveryTest, AutoRecoverTableExtendColumns) {
   // Since the database was not corrupt, the entire schema and all
   // data should be recovered.
   ASSERT_TRUE(Reopen());
-  ASSERT_EQ(orig_schema, GetSchema(&db()));
-  ASSERT_EQ(orig_data, ExecuteWithResults(&db(), kXSql, "|", "\n"));
+  ASSERT_EQ(expected_schema, GetSchema(&db()));
+  ASSERT_EQ(expected_data, ExecuteWithResults(&db(), kXSql, "|", "\n"));
 }
 
 // Recover a golden file where an interior page has been manually modified so
@@ -697,7 +708,7 @@ TEST_F(SQLRecoveryTest, Bug387868) {
     ASSERT_TRUE(recovery->db()->Execute(kCreateSql));
 
     size_t rows = 0;
-    EXPECT_TRUE(recovery->AutoRecoverTable("x", 0, &rows));
+    EXPECT_TRUE(recovery->AutoRecoverTable("x", &rows));
     EXPECT_EQ(43u, rows);
 
     // Successfully recovered.