initial add of SQL based storage

components/offline_pages/offline_page_metadata_store_sql.cc

+// Copyright 2016 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 "components/offline_pages/offline_page_metadata_store_sql.h"
+
+#include "base/bind.h"
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/location.h"
+#include "base/logging.h"
+#include "base/metrics/histogram_macros.h"
+#include "base/sequenced_task_runner.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/thread_task_runner_handle.h"
+#include "components/offline_pages/offline_page_item.h"
+#include "sql/connection.h"
+#include "sql/error_delegate_util.h"
+#include "sql/meta_table.h"
+#include "sql/statement.h"
+#include "sql/transaction.h"
+
+namespace offline_pages {
+
+namespace {
+
+const int kCurrentVersion = 1;
+const int kCompatibleVersion = 1;
+
+// This is a macro instead of a const so that
+// it can be used inline in other SQL statements below.
+#define OFFLINE_PAGES_TABLE_NAME "offlinepages"

[Scott Hess - ex-Googler, 2016/04/12 21:49:21]

Are you actually planning to ever use this to somehow redirect table names? 
When debugging things from the sql/ level, it's generally helpful if clients
just code statements in searchable form rather than doing a partial ORM layer. 
If it will be useful, go for it, but if you're imagining this is best practice
or something, reconsider.

[bburns, 2016/04/19 17:59:45]

Its repeated in several places, so I have a constant to prevent typos, etc. 
It's not intendeded for ORM.

I'm happy to inline the string if you'd rather.

[Scott Hess - ex-Googler, 2016/04/20 18:22:37]

It's really your call.  I'd be more averse if you were dynamically constructing
statements (a half-implemented ORM is generally materially worse than a complete
ORM).

[bburns, 2016/04/20 20:34:24]

Thanks, I think I prefer this way of doing things.

+
+const char kOfflinePagesColumns[] =
+    "(offline_id INTEGER NOT NULL,"
+    " client_namespace VARCHAR(256),"
+    " client_id VARCHAR(256),"
+    " online_url VARCHAR(2048),"
+    " offline_url VARCHAR(2048),"

[Scott Hess - ex-Googler, 2016/04/12 21:49:22]

VARCHAR(N) has no meaning to SQLite, it will all be TEXT.  Unless you feel this
is self-documenting or something, I'd just go with TEXT or VARCHAR.

[bburns, 2016/04/19 17:59:45]

Done.

+    " version INTEGER,"
+    " creation_time INTEGER,"
+    " file_path VARCHAR(1024),"
+    " file_size INTEGER,"
+    " last_access_time INTEGER,"
+    " access_count INTEGER,"
+    " status INTEGER,"
+    " user_initiated BOOLEAN,"

[Scott Hess - ex-Googler, 2016/04/12 21:49:21]

BOOLEAN has no meaning to SQLite.

[Scott Hess - ex-Googler, 2016/04/14 18:18:53]

SQLite stores most types as a type code plus the type data.  INTEGER 0 and
INTEGER 1 have an optimized encoding of 1 byte each, whereas INTEGER 2 or
INTEGER -1 would need 2 bytes, so that's the most sensible way to encode a
boolean.

[SQLite added this encoding in 2006, so you might also see suggestions to encode
booleans as VARCHAR column with NULL or "".  IMHO that's more contrived, don't
use that.]

[bburns, 2016/04/19 17:59:45]

Done.

[bburns, 2016/04/19 17:59:45]

Acknowledged.

+    " UNIQUE(offline_id))";

[Scott Hess - ex-Googler, 2016/04/12 21:49:21]

Why not just "offline_id INTEGER PRIMARY KEY" in the first place?

[bburns, 2016/04/19 17:59:45]

I had this.  For some reason it didn't work correctly with the INSERT OR REPLACE
that I'm doing for update.  But I'm def. not an expert, I could have had some
other error.

[Scott Hess - ex-Googler, 2016/04/20 18:22:36]

AFAICT, it should work fine.  I mean when I play around with sqlite3, I can do
INSERT OR REPLACE with NULL or an assigned value for the primary key, and it
works as I'd expect it to.

Actually, you should probably even say "INTEGER PRIMARY KEY NOT NULL".  SQLite
allows NULL in primary keys, but "INTEGER PRIMARY KEY" is an alias for rowid,
which cannot be NULL.  So SQLite will just ignore the NOT NULL, but it provides
a tidbit of documentation.

It's probably worth figuring out.  As-is, this creates an additional btree
versus just using a primary key.

[bburns, 2016/04/20 20:34:24]

Hrm, it works now.  I blame the cosmic rays...

Done.

+
+// This is cloned from //content/browser/appcache/appcache_database.cc
+struct TableInfo {
+  const char* table_name;
+  const char* columns;
+};
+
+const TableInfo kOfflinePagesTable{OFFLINE_PAGES_TABLE_NAME,
+                                   kOfflinePagesColumns};
+
+// This enum is used to define the indices for the columns in each row
+// that hold the different pieces of offline page.

[Scott Hess - ex-Googler, 2016/04/12 21:49:21]

This feels all indexy and static-compile-checking, but in reality it's a
hard-coded mapping which only happens to coincidentally match, and has no actual
automated checking.  The bind case isn't abstracted in this way, and has the
same basic problem.  The only solution is thorough tests, but once you have that
this abstraction really doesn't buy much.

That said, your preference.

[bburns, 2016/04/19 17:59:45]

This is really just to declare constants for the column indices (e.g. Column #0,
etc)

Previous reviewers preferred an enum to a #define.

[Scott Hess - ex-Googler, 2016/04/20 18:22:36]

I'd probably prefer an enum to a #define, too.  But my point was more that there
is no language support or tooling for keeping this list in sync with SQLite's
list.  So you need to manually fix things up as needed.  [I am not asserting
that SQLite will arbitrarily reorder the columns returned by *.]

In the end this is your call as to whether you think it will gain you anything. 
If you later want to just inline the values, feel free to loop me in to review
it, I would support that.

[bburns, 2016/04/20 20:34:24]

Acknowledged.  I think I prefer the descriptive names to random constants in the
code, but I agree it's a little brittle.

+enum : int {
+  OP_OFFLINE_ID = 0,
+  OP_CLIENT_NAMESPACE,
+  OP_CLIENT_ID,
+  OP_ONLINE_URL,
+  OP_OFFLINE_URL,
+  OP_VERSION,
+  OP_CREATION_TIME,
+  OP_FILE_PATH,
+  OP_FILE_SIZE,
+  OP_LAST_ACCESS_TIME,
+  OP_ACCESS_COUNT,
+  OP_STATUS,
+  OP_USER_INITIATED
+};
+
+bool CreateTable(sql::Connection* db, const TableInfo& table_info) {
+  std::string sql("CREATE TABLE ");
+  sql += table_info.table_name;
+  sql += table_info.columns;
+  return db->Execute(sql.c_str());
+}
+
+bool CreateSchema(sql::MetaTable* meta_table, sql::Connection* db) {
+  // If you create a transaction but don't Commit() it is automatically
+  // rolled back by its destructor when it falls out of scope.
+  sql::Transaction transaction(db);
+  if (!transaction.Begin())
+    return false;
+
+  if (!meta_table->Init(db, kCurrentVersion, kCompatibleVersion))
+    return false;
+
+  if (!CreateTable(db, kOfflinePagesTable))
+    return false;
+
+  // TODO(bburns): Add indices here.
+  return transaction.Commit();
+}
+
+bool DeleteByOfflineId(sql::Connection* db, int64_t offline_id) {
+  const char kSql[] =
+      "DELETE FROM " OFFLINE_PAGES_TABLE_NAME " WHERE offline_id=?";
+  sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
+  statement.BindInt64(0, offline_id);
+  return statement.Run();
+}
+
+// Create an offline page item from a SQL result.  Expects complete rows with
+// all columns present.
+OfflinePageItem MakeOfflinePageItem(sql::Statement* statement) {
+  int64_t id = statement->ColumnInt64(OP_OFFLINE_ID);
+  GURL url(statement->ColumnString(OP_ONLINE_URL));
+  ClientId client_id(statement->ColumnString(OP_CLIENT_NAMESPACE),
+                     statement->ColumnString(OP_CLIENT_ID));
+#if defined(OS_POSIX)
+  base::FilePath path(statement->ColumnString(OP_FILE_PATH));
+#elif defined(OS_WIN)
+  base::FilePath path(base::UTF8ToWide(statement->ColumnString(OP_FILE_PATH)));
+#endif
+  int64_t file_size = statement->ColumnInt64(OP_FILE_SIZE);
+  base::Time creation_time =
+      base::Time::FromInternalValue(statement->ColumnInt64(OP_CREATION_TIME));
+
+  OfflinePageItem item(url, id, client_id, path, file_size, creation_time);
+  item.last_access_time = base::Time::FromInternalValue(
+      statement->ColumnInt64(OP_LAST_ACCESS_TIME));
+  item.version = statement->ColumnInt(OP_VERSION);
+  item.access_count = statement->ColumnInt(OP_ACCESS_COUNT);
+  return item;
+}
+
+bool InsertOrReplace(sql::Connection* db, const OfflinePageItem& item) {
+  const char kSql[] =
+      "INSERT OR REPLACE INTO " OFFLINE_PAGES_TABLE_NAME
+      " (offline_id, online_url, client_namespace, client_id, file_path, "
+      "file_size, creation_time, last_access_time, version, access_count)"
+      " VALUES "
+      " (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)";
+
+  sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
+  statement.BindInt64(0, item.offline_id);
+  statement.BindString(1, item.url.spec());
+  statement.BindString(2, item.client_id.name_space);
+  statement.BindString(3, item.client_id.id);
+  std::string path_string;
+#if defined(OS_POSIX)
+  path_string = item.file_path.value();
+#elif defined(OS_WIN)
+  path_string = base::WideToUTF8(item.file_path.value());
+#endif

[Scott Hess - ex-Googler, 2016/04/12 21:49:22]

Declaring the variable inline in the conditional would have the useful side
effect of not working accidentally with empty paths for new platforms.

[bburns, 2016/04/19 17:59:45]

Done.

+  statement.BindString(4, path_string);
+  statement.BindInt64(5, item.file_size);
+  statement.BindInt64(6, item.creation_time.ToInternalValue());
+  statement.BindInt64(7, item.last_access_time.ToInternalValue());
+  statement.BindInt(8, item.version);
+  statement.BindInt(9, item.access_count);
+  return statement.Run();
+}
+
+}  // anonymous namespace
+
+OfflinePageMetadataStoreSQL::OfflinePageMetadataStoreSQL(
+    scoped_refptr<base::SequencedTaskRunner> background_task_runner,
+    const base::FilePath& path)
+    : background_task_runner_(background_task_runner),
+      db_file_path_(path.AppendASCII("OfflinePages.db")),
+      use_in_memory_(false),
+      weak_ptr_factory_(this) {}
+
+OfflinePageMetadataStoreSQL::~OfflinePageMetadataStoreSQL() {
+  if (db_.get() &&
+      !background_task_runner_->DeleteSoon(FROM_HERE, db_.release())) {
+    DLOG(WARNING) << "SQL database will not be deleted.";
+  }
+}
+
+void OfflinePageMetadataStoreSQL::LoadSync(
+    scoped_refptr<base::SingleThreadTaskRunner> runner,
+    const LoadCallback& callback) {
+  bool opened = false;
+  db_.reset(new sql::Connection);
+  meta_table_.reset(new sql::MetaTable);

[Scott Hess - ex-Googler, 2016/04/12 21:49:21]

You never use meta_table_ outside of this method.  Maybe it could be stack
local?  Note that it maintains a pointer to the db, so it might still make sense
as a unique_ptr<> rather than being constructed directly on the stack.

[bburns, 2016/04/19 17:59:45]

Done.

+
+  if (use_in_memory_) {
+    opened = db_->OpenInMemory();
+  } else {
+    base::File::Error err;
+    if (!base::CreateDirectoryAndGetError(db_file_path_.DirName(), &err)) {

[Scott Hess - ex-Googler, 2016/04/12 21:49:21]

Are there lots of these?

[bburns, 2016/04/19 17:59:45]

Lots of errors?  Probably not, but still worth handling, right?

[Scott Hess - ex-Googler, 2016/04/20 18:22:36]

Apologies - I mean are there lots of distinct databases involved?  I'm wondering
why you have a separate directory rather than just having the database stored
wherever the directory is stored.  And if you do have a directory of databases,
I'm wondering why you're intuiting where to put things based on a pathname
rather than explicitly coding "Here is the directory to store things, here is
the name to use within that directory."

[bburns, 2016/04/20 20:34:24]

Ah, I'm basically just mirroring the existing LevelDB implementation that passes
in a directory name to the constructor.

I'm not entirely sure why it is done that way, but I'd prefer the consistency
for easily switching the code over.

[Scott Hess - ex-Googler, 2016/04/21 03:25:11]

It totally makes sense with leveldb, since that's a federation of files.

Is it using the _same_ directory?  If so, that maybe presents an eventual
migration issue, since the obvious way to delete the leveldb directory is a
recursive delete, rather than deleting the individual files in the directory.

+      LOG(ERROR) << "Failed to create offline pages db directory: "
+                 << base::File::ErrorToString(err);
+    } else {
+      opened = db_->Open(db_file_path_);
+      if (opened)
+        db_->Preload();
+    }
+  }
+  if (!opened) {
+    LOG(ERROR) << "Failed to open database";
+    NotifyLoadResult(runner, callback, STORE_INIT_FAILED,
+                     std::vector<OfflinePageItem>());
+    return;
+  }
+
+  if (!sql::MetaTable::DoesTableExist(db_.get())) {
+    if (!CreateSchema(meta_table_.get(), db_.get())) {
+      LOG(ERROR) << "Failed to create schema";
+      NotifyLoadResult(runner, callback, STORE_INIT_FAILED,
+                       std::vector<OfflinePageItem>());
+      return;
+    }
+  } else if (!meta_table_->Init(db_.get(), kCurrentVersion,
+                                kCompatibleVersion)) {
+    LOG(ERROR) << "Failed to initialize database";
+    NotifyLoadResult(runner, callback, STORE_INIT_FAILED,
+                     std::vector<OfflinePageItem>());
+    return;
+  }
+
+  if (meta_table_->GetCompatibleVersionNumber() > kCurrentVersion) {
+    LOG(WARNING) << "Offline database is too new.";
+    NotifyLoadResult(runner, callback, STORE_INIT_FAILED,
+                     std::vector<OfflinePageItem>());
+    return;
+  }
+
+  const char kSql[] = "SELECT * FROM " OFFLINE_PAGES_TABLE_NAME;
+
+  sql::Statement statement(db_->GetCachedStatement(SQL_FROM_HERE, kSql));
+
+  std::vector<OfflinePageItem> result;
+  while (statement.Step()) {
+    result.push_back(MakeOfflinePageItem(&statement));
+  }
+
+  if (statement.Succeeded()) {
+    NotifyLoadResult(runner, callback, LOAD_SUCCEEDED, result);
+  } else {
+    NotifyLoadResult(runner, callback, STORE_LOAD_FAILED,
+                     std::vector<OfflinePageItem>());
+  }
+}
+
+void OfflinePageMetadataStoreSQL::AddOrUpdateOfflinePageSync(
+    const OfflinePageItem& offline_page,
+    scoped_refptr<base::SingleThreadTaskRunner> runner,
+    const UpdateCallback& callback) {
+  DCHECK(db_.get());
+  // TODO(bburns): add UMA metrics here (and for levelDB).

[Scott Hess - ex-Googler, 2016/04/12 21:49:21]

What does this mean?  It sounds like you're building dueling implementations? 
The tracking bug really doesn't explain it.

[bburns, 2016/04/19 17:59:44]

There's an existing implementation:

https://code.google.com/p/chromium/codesearch#chromium/src/components/offline...

This is part of deprecating that version and migrating to SQLite.

[Scott Hess - ex-Googler, 2016/04/20 18:22:36]

OK, we had an offline discussion.  Reasoning for changing from leveldb to SQLite
seems legit, and I've expressed my concerns WRT places where such a change won't
magically fix problems.

+  bool ok = InsertOrReplace(db_.get(), offline_page);
+  runner->PostTask(FROM_HERE, base::Bind(callback, ok));
+}
+
+void OfflinePageMetadataStoreSQL::RemoveOfflinePagesSync(
+    const std::vector<int64_t>& offline_ids,
+    scoped_refptr<base::SingleThreadTaskRunner> runner,
+    const UpdateCallback& callback) {
+  DCHECK(db_.get());
+  // TODO(bburns): add UMA metrics here (and for levelDB).
+
+  // If you create a transaction but don't Commit() it is automatically
+  // rolled back by its destructor when it falls out of scope.
+  sql::Transaction transaction(db_.get());
+  if (!transaction.Begin()) {
+    runner->PostTask(FROM_HERE, base::Bind(callback, false));
+    return;
+  }
+  for (auto offline_id : offline_ids) {
+    if (!DeleteByOfflineId(db_.get(), offline_id)) {
+      runner->PostTask(FROM_HERE, base::Bind(callback, false));
+      return;
+    }
+  }
+
+  bool success = transaction.Commit();
+  runner->PostTask(FROM_HERE, base::Bind(callback, success));
+}
+
+void OfflinePageMetadataStoreSQL::ResetSync(
+    scoped_refptr<base::SingleThreadTaskRunner> runner,
+    const ResetCallback& callback) {
+  const char kSql[] = "DELETE * FROM " OFFLINE_PAGES_TABLE_NAME;
+  sql::Statement statement(db_->GetCachedStatement(SQL_FROM_HERE, kSql));
+  if (!statement.Run()) {
+    runner->PostTask(FROM_HERE, base::Bind(callback, false));
+    return;
+  }
+  meta_table_.reset();
+  db_.reset();
+  weak_ptr_factory_.InvalidateWeakPtrs();
+
+  runner->PostTask(FROM_HERE, base::Bind(callback, true));
+}
+
+void OfflinePageMetadataStoreSQL::NotifyLoadResult(
+    scoped_refptr<base::SingleThreadTaskRunner> runner,
+    const LoadCallback& callback,
+    LoadStatus status,
+    const std::vector<OfflinePageItem>& result) {
+  // TODO(bburns): Switch to SQL specific UMA metrics.
+  UMA_HISTOGRAM_ENUMERATION("OfflinePages.LoadStatus", status,
+                            OfflinePageMetadataStore::LOAD_STATUS_COUNT);
+  if (status == LOAD_SUCCEEDED) {
+    UMA_HISTOGRAM_COUNTS("OfflinePages.SavedPageCount", result.size());
+  } else {
+    DVLOG(1) << "Offline pages database loading failed: " << status;
+    meta_table_.reset();
+    db_.reset();
+  }
+  runner->PostTask(FROM_HERE, base::Bind(callback, status, result));
+}
+
+void OfflinePageMetadataStoreSQL::Load(const LoadCallback& callback) {
+  background_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&OfflinePageMetadataStoreSQL::LoadSync,
+                            weak_ptr_factory_.GetWeakPtr(),
+                            base::ThreadTaskRunnerHandle::Get(), callback));
+}
+
+void OfflinePageMetadataStoreSQL::AddOrUpdateOfflinePage(
+    const OfflinePageItem& offline_page,
+    const UpdateCallback& callback) {
+  background_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&OfflinePageMetadataStoreSQL::AddOrUpdateOfflinePageSync,
+                 weak_ptr_factory_.GetWeakPtr(), offline_page,
+                 base::ThreadTaskRunnerHandle::Get(), callback));
+}
+
+void OfflinePageMetadataStoreSQL::RemoveOfflinePages(
+    const std::vector<int64_t>& offline_ids,
+    const UpdateCallback& callback) {
+  if (offline_ids.size() == 0) {

[Scott Hess - ex-Googler, 2016/04/12 21:49:21]

.empty() may be more robust.

[bburns, 2016/04/19 17:59:45]

Done.

+    // Nothing to do, but post a callback instead of calling directly
+    // to preserve the async style behavior to prevent bugs.
+    base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                  base::Bind(callback, true));
+    return;
+  }
+
+  background_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&OfflinePageMetadataStoreSQL::RemoveOfflinePagesSync,
+                 weak_ptr_factory_.GetWeakPtr(), offline_ids,
+                 base::ThreadTaskRunnerHandle::Get(), callback));
+}
+
+void OfflinePageMetadataStoreSQL::Reset(const ResetCallback& callback) {
+  background_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&OfflinePageMetadataStoreSQL::ResetSync,
+                            weak_ptr_factory_.GetWeakPtr(),
+                            base::ThreadTaskRunnerHandle::Get(), callback));
+}
+
+void OfflinePageMetadataStoreSQL::SetInMemoryDatabaseForTesting(
+    bool in_memory) {
+  // Must be called prior to Load(...)
+  DCHECK(db_.get() == NULL);
+
+  use_in_memory_ = in_memory;
+}
+
+}  // namespace offline_pages