Create a class to represent a DOM Storage Database.

webkit/dom_storage/dom_storage_database_unittest.cc

Index: webkit/dom_storage/dom_storage_database_unittest.cc
diff --git a/webkit/dom_storage/dom_storage_database_unittest.cc b/webkit/dom_storage/dom_storage_database_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..61cda075d3fcf23d291683a435909657bddedccf
--- /dev/null
+++ b/webkit/dom_storage/dom_storage_database_unittest.cc
@@ -0,0 +1,362 @@
+// Copyright (c) 2012 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 "webkit/dom_storage/dom_storage_database.h"
+
+#include "base/file_path.h"
+#include "base/file_util.h"
+#include "base/path_service.h"
+#include "base/scoped_temp_dir.h"
+#include "base/utf_string_conversions.h"
+#include "sql/statement.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace dom_storage {
+
+class DomStorageDatabaseTest : public testing::Test {
+ protected:
+  virtual void SetUp() {
+    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+    file_name_ = temp_dir_.path().AppendASCII("TestDomStorageDatabase.db");
+  }
+
+  void CreateV1Table(sql::Connection* db) {
+    ASSERT_FALSE(db->is_open());
+    ASSERT_TRUE(db->Open(file_name_));
+    ASSERT_TRUE(db->Execute("DROP TABLE IF EXISTS ItemTable"));
+    ASSERT_TRUE(db->Execute(
+        "CREATE TABLE IF NOT EXISTS ItemTable ("
+        "key TEXT UNIQUE ON CONFLICT REPLACE, "
+        "value TEXT NOT NULL ON CONFLICT FAIL)"));
+  }
+
+  void CreateInvalidTable(sql::Connection* db) {
+    // Create a table with the value type as FLOAT - this is "invalid"

[michaeln, 2012/02/03 04:45:15]

a more interesting test case would be trying to open a file that isn't a
database file at all, i think the invalid table case and that non-a-database
case could/should be handled in the same way?

[benm (inactive), 2012/02/03 11:46:40]

Yes, I think so. An file that isn't a database would be a good test. I can check
in a blob to use for that, but given that the trybots aren't happy with adding
files for testing, I'll leave the test DISABLED until the next CL, like the one
that verifies we can read a WebCore DB.

[benm (inactive), 2012/02/03 14:21:36]

Writing this test exposed some interesting behavior with regard to sqlite's Open
function call. It will quite happily open a connection to any file that you like
and return success, but later dies when you try and do something with that
connection. So I've added another layer of checks inside LazyOpen to cover this.
And the test data file I have is ASCII not binary, so patch should be happy and
I needn't check it in DISABLED. \o/

+    // as far as the DOM Storage db is concerned.
+    ASSERT_FALSE(db->is_open());
+    ASSERT_TRUE(db->Open(file_name_));
+    ASSERT_TRUE(db->Execute("DROP TABLE IF EXISTS ItemTable"));
+    ASSERT_TRUE(db->Execute(
+        "CREATE TABLE IF NOT EXISTS ItemTable ("
+        "key TEXT UNIQUE ON CONFLICT REPLACE, "
+        "value FLOAT NOT NULL ON CONFLICT FAIL)"));
+  }
+
+
+  void InsertDataV1(sql::Connection* db,
+                    const string16& key,
+                    const string16& value) {
+    sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE,
+        "INSERT INTO ItemTable VALUES (?,?)"));
+    statement.BindString16(0, key);
+    statement.BindString16(1, value);
+    ASSERT_TRUE(statement.is_valid());
+    statement.Run();
+  }
+
+  void InsertDataV2(sql::Connection* db,

[michaeln, 2012/02/03 04:45:15]

Would it make sense for callsites to directly use the
DomStorageDatabase::CommitChanges method instead of replicating the logic here?
Having knowlege of the schema here makes it more difficult to change the schema
later (w/o having to revisit this helper).

I think these tests need to have explicit knowlege of the v2 schema in
VerifySchema(), but otherwise, mutating or accessing the data can be
accomplished by calling ReadValues or CommitChanges directly.

[benm (inactive), 2012/02/03 11:46:40]

I added the helpers up here so that we can test the read and commit methods
independently, i.e. we test that values were committed correctly by pulling them
out of the DB directly with SQL without regard to what ReadAllValues does. wdyt?

Maybe this method would be better named just InsertData, and then in the future
we would just update it to match the new schema if it changes, and then add
InsertDataV2 to test the upgrade from v2->v3.

+                    const string16& key,
+                    const string16& value) {
+    sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE,
+        "INSERT INTO ItemTable VALUES (?,?)"));
+    statement.BindString16(0, key);
+    statement.BindBlob(1, value.data(), value.length() * sizeof(char16));
+    ASSERT_TRUE(statement.is_valid());
+    statement.Run();
+  }
+
+  DomStorageDatabase::ValuesMap ReadAllRows(sql::Connection* db) {

[michaeln, 2012/02/03 04:45:15]

similarly, would it make sense to directly use the DomStorageDatabase method
instead of replicating it here

[benm (inactive), 2012/02/03 11:46:40]

See above.

+      DomStorageDatabase::ValuesMap values;
+    sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE,
+                                               "SELECT * from ItemTable"));
+    EXPECT_TRUE(statement.is_valid());
+    while (statement.Step()) {
+      string16 key = statement.ColumnString16(0);
+      string16 value;
+      statement.ColumnBlobAsString16(1, &value);
+      values[key] = NullableString16(value, false);
+    }
+    return values;
+  }
+
+  void CheckValuesMatch(sql::Connection* db,
+                        const DomStorageDatabase::ValuesMap& expected) {
+    const DomStorageDatabase::ValuesMap values_read = ReadAllRows(db);
+    EXPECT_EQ(expected.size(), values_read.size());
+
+    DomStorageDatabase::ValuesMap::const_iterator it = values_read.begin();
+    for (; it != values_read.end(); ++it) {
+      string16 key = it->first;
+      NullableString16 value = it->second;
+      EXPECT_EQ((expected.find(key)->second).string(), value.string());
+    }
+  }
+
+  void VerifySchema(sql::Connection* db) {
+    sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE,
+        "SELECT * from ItemTable LIMIT 1"));
+    EXPECT_EQ(sql::COLUMN_TYPE_TEXT, statement.DeclaredColumnType(0));
+    EXPECT_EQ(sql::COLUMN_TYPE_BLOB, statement.DeclaredColumnType(1));
+  }
+
+  ScopedTempDir temp_dir_;
+  FilePath file_name_;
+};
+
+TEST_F(DomStorageDatabaseTest, SimpleOpenAndClose) {

[michaeln, 2012/02/03 04:45:15]

this test could use an in-memory db

[benm (inactive), 2012/02/03 11:46:40]

Agreed, I've added a TODO about this but as it will need more work in the
DomStorageClass to implement, I'd like to leave the actual implementation out
for now. I'll go over all the tests here and replace with an in-mem db where
appropriate when fixing the TODO.

+  DomStorageDatabase db(file_name_);
+  ASSERT_TRUE(db.LazyOpen(true));
+  EXPECT_TRUE(db.db_->DoesTableExist("ItemTable"));
+  // Ensure that we've got the colums we expect.
+  EXPECT_TRUE(db.db_->DoesColumnExist("ItemTable", "key"));
+  EXPECT_TRUE(db.db_->DoesColumnExist("ItemTable", "value"));

[michaeln, 2012/02/03 04:45:15]

Would it make sense to put these three statements be in VerifySchema too?

 110   EXPECT_TRUE(db.db_->DoesTableExist("ItemTable"));
 111   // Ensure that we've got the colums we expect.
 112   EXPECT_TRUE(db.db_->DoesColumnExist("ItemTable", "key"));
 113   EXPECT_TRUE(db.db_->DoesColumnExist("ItemTable", "value"));

[benm (inactive), 2012/02/03 11:46:40]

Done.

+  VerifySchema(db.db_.get());
+  db.Close();
+  EXPECT_FALSE(db.IsOpen());
+}
+
+TEST_F(DomStorageDatabaseTest, TestLazyOpenIsLazy) {
+  DomStorageDatabase db(file_name_);
+  EXPECT_FALSE(db.IsOpen());
+  DomStorageDatabase::ValuesMap values;
+  db.ReadAllValues(&values);
+  EXPECT_FALSE(db.IsOpen());
+  values[ASCIIToUTF16("key")] = NullableString16(ASCIIToUTF16("value"), false);
+  db.CommitChanges(false, values);
+  EXPECT_TRUE(db.IsOpen());
+
+  db.Close();
+  ASSERT_FALSE(db.IsOpen());
+
+  db.ReadAllValues(&values);
+  EXPECT_TRUE(db.IsOpen());
+}
+
+TEST_F(DomStorageDatabaseTest, TestUpgradesV1TableToV2) {

[michaeln, 2012/02/03 04:45:15]

this one maybe could be done out of an in-mem db by setting up a connection
which a v1 schema and calling the upgrade method directly similar to the test
below

+  DomStorageDatabase db(file_name_);
+  db.db_.reset(new sql::Connection());
+  CreateV1Table(db.db_.get());
+  db.Close();
+
+  db.LazyOpen(true);
+  VerifySchema(db.db_.get());
+}
+
+TEST_F(DomStorageDatabaseTest, TestFailedUpgrade) {
+  DomStorageDatabase db(file_name_);
+  db.db_.reset(new sql::Connection());
+  CreateInvalidTable(db.db_.get());
+
+  EXPECT_FALSE(db.UpgradeVersion1To2IfNeeded());
+
+  db.Close();
+
+  // LazyOpen should be able to deal with the failed upgrade.
+  EXPECT_TRUE(db.LazyOpen(true));
+  VerifySchema(db.db_.get());
+}
+
+
+TEST_F(DomStorageDatabaseTest, TestIsOpen) {

[michaeln, 2012/02/03 04:45:15]

could be done with an in-mem db

+  DomStorageDatabase db(file_name_);
+  EXPECT_FALSE(db.IsOpen());
+  ASSERT_TRUE(db.LazyOpen(true));
+  EXPECT_TRUE(db.IsOpen());
+  db.Close();
+  EXPECT_FALSE(db.IsOpen());
+}
+
+TEST_F(DomStorageDatabaseTest, SimpleRead) {
+  DomStorageDatabase db(file_name_);
+  db.LazyOpen(true);
+
+  const string16 kCannedKey = ASCIIToUTF16("name");
+  const string16 kCannedValue = ASCIIToUTF16("Joe Bloggs");
+  InsertDataV2(db.db_.get(), kCannedKey, kCannedValue);
+  DomStorageDatabase::ValuesMap values;
+  db.ReadAllValues(&values);
+
+  EXPECT_EQ(1u, values.size());
+  EXPECT_TRUE(values.find(kCannedKey) != values.end());
+  EXPECT_EQ(kCannedValue, values[kCannedKey].string());
+}
+
+TEST_F(DomStorageDatabaseTest, SimpleWrite) {

[michaeln, 2012/02/03 04:45:15]

Since SimpleWrite actually also reads, maybe call it SimpleReadWrite and delete
SimpleRead?

[benm (inactive), 2012/02/03 11:46:40]

So the idea here was that we're only testing the CommitChanges method of
DomStorageDatabase - that's why I've got the helpers up top to pull out of the
underlying DB with SQL, regardless of what the ReadAllValues method on the
DomStorageDatabase does.

[michaeln, 2012/02/04 00:21:34]

I see that, but my point is that it makes the tests less resilient to changes to
the DomStorageDatabase class. Eventually a very similar class (maybe the same
class name) will utilize levelDB under the covers (with bits and pieces of the
existing impl to help with migration). It'd be nice to be able to throw the very
similar tests at it for the basic functionality. The more the tests stick to
utilizing class interfaces to perform those tests, the easier it is to migrate
the impl.

[benm (inactive), 2012/02/06 14:02:36]

OK, done

+  DomStorageDatabase db(file_name_);
+
+  DomStorageDatabase::ValuesMap storage;
+  string16 kCannedKeys[] = {
+      ASCIIToUTF16("test"),
+      ASCIIToUTF16("company"),
+      ASCIIToUTF16("date")
+  };
+  NullableString16 kCannedValues[] = {
+      NullableString16(ASCIIToUTF16("123"), false),
+      NullableString16(ASCIIToUTF16("Google"), false),
+      NullableString16(ASCIIToUTF16("18-01-2012"), false)
+  };
+  for (int i = 0; i < 3; i++) {
+    storage[kCannedKeys[i]] = kCannedValues[i];
+  }
+
+  ASSERT_TRUE(db.CommitChanges(false, storage));
+
+  CheckValuesMatch(db.db_.get(), storage);
+}
+
+TEST_F(DomStorageDatabaseTest, WriteWithClear) {
+  DomStorageDatabase db(file_name_);
+
+  DomStorageDatabase::ValuesMap storage;
+  string16 kCannedKeys[] = {
+      ASCIIToUTF16("test"),
+      ASCIIToUTF16("company"),
+      ASCIIToUTF16("date")
+  };
+  NullableString16 kCannedValues[] = {
+      NullableString16(ASCIIToUTF16("123"), false),
+      NullableString16(ASCIIToUTF16("Google"), false),
+      NullableString16(ASCIIToUTF16("18-01-2012"), false)
+  };
+  for (int i = 0; i < 3; i++) {
+    storage[kCannedKeys[i]] = kCannedValues[i];

[michaeln, 2012/02/03 04:45:15]

this is replicated in the previous test too, feels like a helper function would
be good

[benm (inactive), 2012/02/03 11:46:40]

Done.

+  }
+
+  ASSERT_TRUE(db.CommitChanges(false, storage));
+  CheckValuesMatch(db.db_.get(), storage);
+
+  // Insert some values, clearing the database first.
+  storage.clear();
+  storage[ASCIIToUTF16("another_key")] =
+      NullableString16(ASCIIToUTF16("test"), false);
+  ASSERT_TRUE(db.CommitChanges(true, storage));
+  CheckValuesMatch(db.db_.get(), storage);
+
+  // Now clear the values without inserting any new ones.
+  storage.clear();
+  ASSERT_TRUE(db.CommitChanges(true, storage));
+  CheckValuesMatch(db.db_.get(), storage);
+}
+
+TEST_F(DomStorageDatabaseTest, UpgradeFromV1ToV2NoData) {

[michaeln, 2012/02/03 04:45:15]

there are multiple upgrade tests, can any of them be merged, how is this any
different than TestUpgradesV1TableToV2?

[benm (inactive), 2012/02/03 11:46:40]

I think what I was thinking is that this test verifies that
db.UpgradeVersion1To2IfNeeded() works, and TestUpgradesV1TableToV2 verifies that
LazyOpen triggers the upgrade logic. I can rename the other test to be more
explicit about that.

[michaeln, 2012/02/04 00:21:34]

Got it. Since that test also verifies the resulting schema in the new db (given
an empty v1 schema as input), its essentially performing this test as a part of
that test. So can we remove this one since its already covered by that one?

I'm a big believer in less is more ;)

[benm (inactive), 2012/02/06 14:02:36]

sounds good. It's nice to have such a powerful testing framework, I want to
write ALL the tests!! :P

+  DomStorageDatabase db(file_name_);
+  db.db_.reset(new sql::Connection());
+  CreateV1Table(db.db_.get());
+
+  // The database has V1 structure, try to update it to V2.
+  sql::Statement statement(db.db_->GetCachedStatement(SQL_FROM_HERE,
+      "SELECT value from ItemTable LIMIT 1"));

[michaeln, 2012/02/03 04:45:15]

can this statement be removed, what purpose does it serve?

[benm (inactive), 2012/02/03 11:46:40]

I think you need a valid SQL statement to be able to call DeclaredColoumType
below. I believe it can be any statement you like, I picked a SELECT :)

[michaeln, 2012/02/04 00:21:34]

sorry, i meant what purpose does assertion part serve... see earlier comment
about testing the results test setup in the test body.

[benm (inactive), 2012/02/06 14:02:36]

Got it - removed now.

+  ASSERT_EQ(sql::COLUMN_TYPE_TEXT, statement.DeclaredColumnType(0));
+
+  ASSERT_TRUE(db.UpgradeVersion1To2IfNeeded());
+
+  // Verify the db now has V2 structure.
+  VerifySchema(db.db_.get());
+}
+
+TEST_F(DomStorageDatabaseTest, UpgradeFromV1ToV2WithData) {
+  const string16 kCannedKey = ASCIIToUTF16("foo");
+  const NullableString16 kCannedValue(ASCIIToUTF16("bar"), false);
+  DomStorageDatabase::ValuesMap expected;
+  expected[kCannedKey] = kCannedValue;
+
+  {
+    DomStorageDatabase db(file_name_);
+    db.db_.reset(new sql::Connection());
+    CreateV1Table(db.db_.get());
+
+    // The database has V1 structure, try to update it to V2.
+    sql::Statement statement(db.db_->GetCachedStatement(SQL_FROM_HERE,
+        "SELECT value from ItemTable LIMIT 1"));
+    ASSERT_EQ(sql::COLUMN_TYPE_TEXT, statement.DeclaredColumnType(0));
+
+    InsertDataV1(db.db_.get(), kCannedKey, kCannedValue.string());
+
+    ASSERT_TRUE(db.UpgradeVersion1To2IfNeeded());
+
+    VerifySchema(db.db_.get());
+
+    CheckValuesMatch(db.db_.get(), expected);
+  }
+
+  // Now open the db again and check that the data is consistent.

[michaeln, 2012/02/03 04:45:15]

provided other tests ensure that files containing data are actually opened by
LazyOpen(), not sure you need this block in this test. And then, the upgrade
logic could be tested in-memory (much faster).

[benm (inactive), 2012/02/03 11:46:40]

Yes, it should be covered by the next test. will remove

+  {
+    DomStorageDatabase db(file_name_);
+    ASSERT_TRUE(db.LazyOpen(true));
+    CheckValuesMatch(db.db_.get(), expected);
+  }
+}
+
+TEST_F(DomStorageDatabaseTest, TestOpenCloseDataPreserved) {
+  DomStorageDatabase db(file_name_);
+
+  ASSERT_TRUE(db.LazyOpen(true));
+
+  const string16 kCannedKey = ASCIIToUTF16("test");
+  const NullableString16 kCannedValue(ASCIIToUTF16("data"), false);
+  InsertDataV2(db.db_.get(), kCannedKey, kCannedValue.string());
+  db.Close();
+
+  ASSERT_TRUE(db.LazyOpen(true));
+  DomStorageDatabase::ValuesMap expected;
+  expected[kCannedKey] = kCannedValue;
+  CheckValuesMatch(db.db_.get(), expected);
+}
+
+TEST_F(DomStorageDatabaseTest, TestSimpleRemoveOneValue) {
+  DomStorageDatabase db(file_name_);
+
+  ASSERT_TRUE(db.LazyOpen(true));
+  const string16 kCannedKey = ASCIIToUTF16("test");
+  const NullableString16 kCannedValue(ASCIIToUTF16("data"), false);
+  InsertDataV2(db.db_.get(), kCannedKey, kCannedValue.string());
+
+  DomStorageDatabase::ValuesMap expected;
+  expected[kCannedKey] = kCannedValue;
+  CheckValuesMatch(db.db_.get(), expected);
+
+  DomStorageDatabase::ValuesMap values;
+  values[kCannedKey] = NullableString16(true);
+
+  db.CommitChanges(false, values);
+
+  expected.clear();
+  CheckValuesMatch(db.db_.get(), expected);
+}
+
+// TODO(benm): Enable this test in follow up patch once the test data has
+// landed.
+TEST_F(DomStorageDatabaseTest, DISABLED_TestCanOpenAndReadWebCoreDatabase) {
+  {
+    FilePath webcore_database;
+    PathService::Get(base::DIR_SOURCE_ROOT, &webcore_database);
+    webcore_database = webcore_database.AppendASCII("webkit");
+    webcore_database = webcore_database.AppendASCII("data");
+    webcore_database = webcore_database.AppendASCII("dom_storage");
+    webcore_database =
+        webcore_database.AppendASCII("webcore_test_database.localstorage");
+
+    ASSERT_TRUE(file_util::PathExists(webcore_database));
+
+    DomStorageDatabase db(webcore_database);
+    DomStorageDatabase::ValuesMap values;
+    db.ReadAllValues(&values);
+    EXPECT_TRUE(db.IsOpen());
+    EXPECT_EQ(2u, values.size());
+
+    DomStorageDatabase::ValuesMap::const_iterator it =
+        values.find(ASCIIToUTF16("value"));
+    EXPECT_TRUE(it != values.end());
+    EXPECT_EQ(ASCIIToUTF16("I am in local storage!"), it->second.string());
+
+    it = values.find(ASCIIToUTF16("timestamp"));
+    EXPECT_TRUE(it != values.end());
+    EXPECT_EQ(ASCIIToUTF16("1326738338841"), it->second.string());
+
+    it = values.find(ASCIIToUTF16("not_there"));
+    EXPECT_TRUE(it == values.end());
+  }
+}
+
+}  // namespace dom_storage