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..75cbfe05bd8f113ff8431a60b1e14efe24c6b569
--- /dev/null
+++ b/webkit/dom_storage/dom_storage_database_unittest.cc
@@ -0,0 +1,377 @@
+// 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 {
+
+// TODO(benm): Rather than operating on files on disk, these tests could
+// run much faster if they were operating on in-memory databases.

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

faster and more reliably too, i think it'd be worth doing this prior to the
initial checkin, probably pay for itself in time time saved not having to
respond to test hiccups that will come along with actually fileio

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

Is there a runtime flag or an #ifdef I can use to determine when we're
running/building tests? Would be nice to be sure I'm only creating an in memory
database when we're under test and not in real life?

[benm (inactive), 2012/02/06 15:41:33]

I found #ifdef UNIT_TEST. I think that should do the trick.

[benm (inactive), 2012/02/06 17:03:21]

Done - the variance in time to run is quite remarkable between tests that hit
the disk and those that stay in memory. 

[michaeln, 2012/02/08 06:11:33]

yes, how many human lifetimes will be saved in try runs :)

+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"
+    // 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,
+                    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) {
+      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());

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

what about is_null()?

if lhs is_null() with an empty string, it will equate with a rhs thats
!is_null() but also with a empty string

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

Done.

+    }
+  }
+
+  void VerifySchema(sql::Connection* db) {
+    sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE,
+        "SELECT * from ItemTable LIMIT 1"));
+    EXPECT_TRUE(db->DoesTableExist("ItemTable"));
+    // Ensure that we've got the colums we expect and they are of the
+    // correct type.
+    EXPECT_TRUE(db->DoesColumnExist("ItemTable", "key"));
+    EXPECT_TRUE(db->DoesColumnExist("ItemTable", "value"));
+    EXPECT_EQ(sql::COLUMN_TYPE_TEXT, statement.DeclaredColumnType(0));
+    EXPECT_EQ(sql::COLUMN_TYPE_BLOB, statement.DeclaredColumnType(1));
+  }
+
+  void CreateMapWith3Values(DomStorageDatabase::ValuesMap* values) {
+    string16 kCannedKeys[] = {
+        ASCIIToUTF16("test"),
+        ASCIIToUTF16("company"),
+        ASCIIToUTF16("date")
+    };
+    NullableString16 kCannedValues[] = {
+        NullableString16(ASCIIToUTF16("123"), false),
+        NullableString16(ASCIIToUTF16("Google"), false),
+        NullableString16(ASCIIToUTF16("18-01-2012"), false)

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

Maybe add a empty string as a value to ensure that's not confused with 'null' on
a round trip in and out of the DB.

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

Done.

+    };
+    for (int i = 0; i < 3; i++)
+      (*values)[kCannedKeys[i]] = kCannedValues[i];
+  }
+
+  ScopedTempDir temp_dir_;
+  FilePath file_name_;
+};
+
+TEST_F(DomStorageDatabaseTest, SimpleOpenAndClose) {
+  DomStorageDatabase db(file_name_);
+  ASSERT_TRUE(db.LazyOpen(true));
+  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, TestLazyOpenUpgradesV1TableToV2) {
+  DomStorageDatabase db(file_name_);
+  db.db_.reset(new sql::Connection());
+  CreateV1Table(db.db_.get());
+  // Open a scope to run the statements to ensure that statements are cleaned
+  // up before we close the database.
+  {
+    sql::Statement statement(db.db_->GetCachedStatement(SQL_FROM_HERE,
+        "SELECT value from ItemTable LIMIT 1"));
+    ASSERT_EQ(sql::COLUMN_TYPE_TEXT, statement.DeclaredColumnType(0));

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

Not sure this block adds value? Its testing the results of the test setup
(CreateV1Table) and not the upgrade method. Not having it in the unittest would
enhance readability. It might make more sense to put this in CreateV1Table
directly, but i'm not sure its really needed in this file?

Looks like its testing the stmt.DeclaredColumnType() method moreso than anything
else, maybe put tests for that method in statement_unittest.cc instead?

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

Good point. I put it as an ASSERT as there's no point in continuing if we
weren't able to create the table at V1. But we already ASSERT the creation
statements inside CreateTableV1... so I guess it's OK to remove this.

+  }
+  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 by
+  // dropping the table and creating a new one.
+  EXPECT_TRUE(db.LazyOpen(true));
+  VerifySchema(db.db_.get());
+}
+
+
+TEST_F(DomStorageDatabaseTest, TestIsOpen) {
+  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) {
+  DomStorageDatabase db(file_name_);
+
+  DomStorageDatabase::ValuesMap storage;
+  CreateMapWith3Values(&storage);
+  ASSERT_TRUE(db.CommitChanges(false, storage));
+
+  CheckValuesMatch(db.db_.get(), storage);
+}
+
+TEST_F(DomStorageDatabaseTest, WriteWithClear) {
+  DomStorageDatabase db(file_name_);
+
+  DomStorageDatabase::ValuesMap storage;
+  CreateMapWith3Values(&storage);
+
+  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) {
+  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));

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

ditto

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

Done.

+
+  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));

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

ditto

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

Done.

+
+  InsertDataV1(db.db_.get(), kCannedKey, kCannedValue.string());
+
+  ASSERT_TRUE(db.UpgradeVersion1To2IfNeeded());
+
+  VerifySchema(db.db_.get());
+
+  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. (try-bots don't like adding binary files like test databases :) )
+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());
+}
+
+TEST_F(DomStorageDatabaseTest, TestCanOpenFileThatIsNotADatabase) {
+  FilePath not_a_database;
+  PathService::Get(base::DIR_SOURCE_ROOT, &not_a_database);

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

would be more hermetic to not rely on an external data file for this one, should
we create the file in the scoped temp dir instead, file_util::WriteFile would
work i think

(makes sense to include the sqlite3 db file for the test above because to create
that hermetically would mean dragging all of sqlite3 into the unittest)

also a similar test where the input is a directory instead of a file would be
good

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

Done.
The directory test causes the call to Connection::Open to ASSERT, as we hit the
NOTREACHED() in DiagnosticErrorDelegate::OnSqlError (as the open actually fails
on a directory). I think that means then that there isn't anyone out there that
is testing this sort of thing (as otherwise they'd be ASSERTing in the test)? I
can demote the ASSERT in DiagnosticErrorDelegate to a log message, or not test
this... wdyt?

+  not_a_database = not_a_database.AppendASCII("webkit");
+  not_a_database = not_a_database.AppendASCII("data");
+  not_a_database = not_a_database.AppendASCII("dom_storage");
+  not_a_database =
+      not_a_database.AppendASCII("not_a_database.localstorage");
+
+  ASSERT_TRUE(file_util::PathExists(not_a_database));
+
+  DomStorageDatabase db(not_a_database);
+  DomStorageDatabase::ValuesMap values;
+  CreateMapWith3Values(&values);
+  EXPECT_FALSE(db.CommitChanges(true, values));

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

we want the behavior to be such that this commit succeeds after deleting the bad
file and creating a new, right?

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

Right, but don't have the code to do the delete and recreate yet :)

Makes the directory test case even more useful, because we can't create anything
in that case.

+  EXPECT_FALSE(db.CommitChanges(false, values));
+  EXPECT_FALSE(db.IsOpen());
+
+  values.clear();
+
+  db.ReadAllValues(&values);
+  EXPECT_EQ(0u, values.size());
+  EXPECT_FALSE(db.IsOpen());
+}
+
+}  // namespace dom_storage