[sql] Database recovery system for Shortcuts.

sql/recovery_unittest.cc

Index: sql/recovery_unittest.cc
diff --git a/sql/recovery_unittest.cc b/sql/recovery_unittest.cc
index d3b04b1eb900a888cb955aa5fdfab36a83038c92..0da129014baf822d534997b39fd88f89408faf0c 100644
--- a/sql/recovery_unittest.cc
+++ b/sql/recovery_unittest.cc
@@ -747,4 +747,71 @@ TEST_F(SQLRecoveryTest, NoMmap) {
   EXPECT_TRUE(!s.Step() || !s.ColumnInt64(0));
 }
 
+TEST_F(SQLRecoveryTest, RecoverDatabaseOrRaze) {
+  // This table needs sqlite_sequence to work.

[Mark P, 2016/04/08 20:12:06]

... because of the autoincrement.

[Scott Hess - ex-Googler, 2016/04/15 00:38:16]

Done.

+  ASSERT_TRUE(db().Execute(
+      "CREATE TABLE a (id INTEGER PRIMARY KEY AUTOINCREMENT, v TEXT)"));

[Mark P, 2016/04/08 20:12:06]

Everywhere else in this file uses tables x and y.  Any reason not to use those
names?

[Scott Hess - ex-Googler, 2016/04/15 00:38:15]

Eh, it was just [a]utoincrement and [i]ndex and [v]alue.  Done.

+  EXPECT_TRUE(db().Execute("INSERT INTO a (v) VALUES ('turtle')"));
+  EXPECT_TRUE(db().Execute("INSERT INTO a (v) VALUES ('truck')"));
+  EXPECT_TRUE(db().Execute("INSERT INTO a (v) VALUES ('trailer')"));
+
+  // This table needs index and a unique index to work.
+  ASSERT_TRUE(db().Execute("CREATE TABLE i (name TEXT, v TEXT)"));
+  ASSERT_TRUE(db().Execute("CREATE UNIQUE INDEX i_name ON i(name)"));
+  ASSERT_TRUE(db().Execute("CREATE INDEX i_v ON i(v)"));
+  EXPECT_TRUE(db().Execute("INSERT INTO i VALUES ('jim', 'telephone')"));
+  EXPECT_TRUE(db().Execute("INSERT INTO i VALUES ('bob', 'truck')"));
+  EXPECT_TRUE(db().Execute("INSERT INTO i VALUES ('dean', 'trailer')"));
+
+  // View which is the intersection of [a.v] and [i.v].
+  ASSERT_TRUE(db().Execute(
+      "CREATE VIEW v AS SELECT a.v FROM a, i WHERE a.v = i.v"));
+
+  // When an element is deleted from [a], trigger a delete on [i].  Between the
+  // BEGIN and END, [old] stands for the deleted rows from [a].
+  ASSERT_TRUE(db().Execute("CREATE TRIGGER t AFTER DELETE ON a "
+                           "BEGIN DELETE FROM i WHERE i.v = old.v; END"));
+
+  // Save aside a copy of the original schema and data.
+  const std::string orig_schema(GetSchema(&db()));
+  const char kASql[] = "SELECT * FROM a ORDER BY 1";
+  const char kISql[] = "SELECT * FROM i ORDER BY 1";
+  const std::string orig_data_a(ExecuteWithResults(&db(), kASql, "|", "\n"));
+  const std::string orig_data_i(ExecuteWithResults(&db(), kISql, "|", "\n"));
+

[Mark P, 2016/04/08 20:12:07]

I think this blank line is unnecessary.

[Scott Hess - ex-Googler, 2016/04/15 00:38:15]

Done.

+  const char kVSql[] = "SELECT * FROM v ORDER BY 1";
+  const std::string orig_data_v(ExecuteWithResults(&db(), kVSql, "|", "\n"));
+

[Mark P, 2016/04/08 20:12:06]

ditto

[Scott Hess - ex-Googler, 2016/04/15 00:38:15]

Done.

+  const char kTrivialSql[] = "SELECT COUNT(*) FROM sqlite_master";
+  EXPECT_TRUE(db().IsSQLValid(kTrivialSql));

[Mark P, 2016/04/08 20:12:06]

Comment such as
// Verify the database handle is valid.

[Scott Hess - ex-Googler, 2016/04/15 00:38:16]

Done.

[Scott Hess - ex-Googler, 2016/04/15 00:38:16]

Done.

+
+  sql::Recovery::RecoverDatabaseOrRaze(&db(), db_path());
+
+  // Database handle has been poisoned.
+  EXPECT_FALSE(db().IsSQLValid(kTrivialSql));
+
+  // Since the database was not corrupt, the entire schema and all
+  // data should be recovered.
+  ASSERT_TRUE(Reopen());

[Mark P, 2016/04/08 20:12:07]

This block of tests makes me nervous because you're comparing the new data with
the old data, yet you have no check that the old data is plausible.  Suppose
there was an uncaught error earlier that prevented any data from being inserted
into the database.  This test would still pass.  I suggest either testing the
results of the original data calls or removing that original data test and
simply compare the new results with the actual hard-coded values you'd expect.

[Scott Hess - ex-Googler, 2016/04/15 00:38:15]

Done.

In testing the schema, it seemed like a fair bit of annoying text to format, and
a little brittle (the actual format of the schema seems like a SQLite internal
kind of thing, to me).  So I tested that the expected number of schema items was
present.  That's still a bit specific, since sqlite_sequence seems like an
internal thing clients have no reason to see, but explicitly omitting it is also
unnatural.

+  ASSERT_EQ(orig_schema, GetSchema(&db()));
+  EXPECT_EQ(orig_data_a, ExecuteWithResults(&db(), kASql, "|", "\n"));
+  EXPECT_EQ(orig_data_i, ExecuteWithResults(&db(), kISql, "|", "\n"));
+  EXPECT_EQ(orig_data_v, ExecuteWithResults(&db(), kVSql, "|", "\n"));
+
+  // Test that the view works.

[Mark P, 2016/04/08 20:12:07]

I don't understand this block.  I don't see how this checks that the view works.
 There is no expect or assert statements in here, and ExecuteWithResults()
doesn't have any expected or assert inside it that would trigger the view was
broken.

If nothing else, this block is more correctly combined with the trigger block
below, with no comment making an explicit claim about checking the view.

[Scott Hess - ex-Googler, 2016/04/15 00:38:15]

It's possible that the block is dumb.  It's gone now.

+  const char kAWithoutSql[] = "SELECT * FROM a WHERE v<>'truck' ORDER BY 1";
+  const char kIWithoutSql[] = "SELECT * FROM i WHERE v<>'truck' ORDER BY 1";
+  const std::string orig_data_a_without(
+      ExecuteWithResults(&db(), kAWithoutSql, "|", "\n"));
+  const std::string orig_data_i_without(
+      ExecuteWithResults(&db(), kIWithoutSql, "|", "\n"));
+
+  // Test that the trigger works.
+  ASSERT_TRUE(db().Execute("DELETE FROM a WHERE v = 'truck'"));
+  EXPECT_EQ(orig_data_a_without,
+            ExecuteWithResults(&db(), kAWithoutSql, "|", "\n"));

[Mark P, 2016/04/08 20:12:07]

Shouldn't this be kASql, i.e., you're checking that after the above deletion, a
_normal_ select statement will return the same result as a select statement that
excludes 'truck' explicitly?
ditto with kISql below

[Scott Hess - ex-Googler, 2016/04/15 00:38:15]

Sorry, the above capture from the view _is_ the same result excluding 'truck'. 
The trigger has the side effect of changing the view's results.

I'll just go through and make the expectations static.

+  EXPECT_EQ(orig_data_i_without,
+            ExecuteWithResults(&db(), kIWithoutSql, "|", "\n"));
+}
+
 }  // namespace