Introduce AttachmentStore metadata record

This change handles different errors during attachment store open:
- create record for empty database
- fail if record is corrupt or of newer version.

BUG=424304
R=maniscalco@chromium.org

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

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

sync/BUILD.gn

@@ -404,6 +404,7 @@ source_set("sync_core") {
     "//third_party/leveldatabase",
     "//third_party/zlib",
     "//url",
+    "//sync/internal_api/attachments/proto",
     "//sync/protocol",
   ]
 
@@ -632,10 +633,11 @@ test("sync_unit_tests") {
     "//net",
     "//net:test_support",
     "//sql",
+    "//sync",
     "//testing/gmock",
     "//testing/gtest",
+    "//third_party/leveldatabase",
     "//third_party/protobuf:protobuf_lite",
-    "//sync",
     ":test_support_sync_core",
     ":test_support_sync_internal_api",
   ]

sync/internal_api/attachments/on_disk_attachment_store.cc

@@ -9,6 +9,7 @@
 #include "base/location.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/sequenced_task_runner.h"
+#include "sync/internal_api/attachments/proto/attachment_store.pb.h"
 #include "sync/protocol/attachments.pb.h"
 #include "third_party/leveldatabase/src/include/leveldb/db.h"
 #include "third_party/leveldatabase/src/include/leveldb/options.h"
@@ -22,8 +23,45 @@ namespace {
 // Prefix for records containing attachment data.
 const char kDataPrefix[] = "data-";
 
+const char kDatabaseMetadataKey[] = "database-metadata";
+
+const int32 kCurrentSchemaVersion = 1;
+
 const base::FilePath::CharType kLeveldbDirectory[] =
     FILE_PATH_LITERAL("leveldb");
+
+leveldb::WriteOptions MakeWriteOptions() {
+  leveldb::WriteOptions write_options;
+  write_options.sync = true;
+  return write_options;
+}
+
+leveldb::Status ReadStoreMetadata(
+    leveldb::DB* db,
+    attachment_store_pb::AttachmentStoreMetadata* metadata) {
+  std::string data_str;
+  leveldb::ReadOptions read_options;
+  read_options.fill_cache = false;
+  read_options.verify_checksums = true;
+
+  leveldb::Status status =
+      db->Get(read_options, kDatabaseMetadataKey, &data_str);
+  if (!status.ok())
+    return status;
+  if (!metadata->ParseFromString(data_str))
+    return leveldb::Status::Corruption("Metadata record corruption");
+  return leveldb::Status::OK();
+}
+
+leveldb::Status WriteStoreMetadata(
+    leveldb::DB* db,
+    const attachment_store_pb::AttachmentStoreMetadata& metadata) {
+  std::string data_str;
+
+  metadata.SerializeToString(&data_str);
+  return db->Put(MakeWriteOptions(), kDatabaseMetadataKey, data_str);
+}
+
 }  // namespace
 
 OnDiskAttachmentStore::OnDiskAttachmentStore(
@@ -50,7 +88,7 @@ void OnDiskAttachmentStore::Read(const AttachmentIdList& ids,
   AttachmentIdList::const_iterator iter = ids.begin();
   const AttachmentIdList::const_iterator end = ids.end();
   for (; iter != end; ++iter) {
-    const std::string key = CreateDataKeyFromAttachmentId(*iter);
+    const std::string key = MakeDataKeyFromAttachmentId(*iter);
     std::string data_str;
     leveldb::Status status = db_->Get(read_options, key, &data_str);
     if (!status.ok()) {
@@ -84,13 +122,12 @@ void OnDiskAttachmentStore::Write(const AttachmentList& attachments,
   read_options.fill_cache = false;
   read_options.verify_checksums = true;
 
-  leveldb::WriteOptions write_options;
+  leveldb::WriteOptions write_options = MakeWriteOptions();
-  write_options.sync = true;
 
   AttachmentList::const_iterator iter = attachments.begin();
   const AttachmentList::const_iterator end = attachments.end();
   for (; iter != end; ++iter) {
-    const std::string key = CreateDataKeyFromAttachmentId(iter->GetId());
+    const std::string key = MakeDataKeyFromAttachmentId(iter->GetId());
 
     std::string data_str;
     // TODO(pavely): crbug/424304 This read is expensive. When I add metadata
@@ -124,12 +161,11 @@ void OnDiskAttachmentStore::Drop(const AttachmentIdList& ids,
   DCHECK(CalledOnValidThread());
   DCHECK(db_);
   Result result_code = SUCCESS;
-  leveldb::WriteOptions write_options;
+  leveldb::WriteOptions write_options = MakeWriteOptions();
-  write_options.sync = true;
   AttachmentIdList::const_iterator iter = ids.begin();
   const AttachmentIdList::const_iterator end = ids.end();
   for (; iter != end; ++iter) {
-    const std::string key = CreateDataKeyFromAttachmentId(*iter);
+    const std::string key = MakeDataKeyFromAttachmentId(*iter);
     leveldb::Status status = db_->Delete(write_options, key);
     if (!status.ok()) {
       // DB::Delete doesn't check if record exists, it returns ok just like
@@ -145,27 +181,53 @@ AttachmentStore::Result OnDiskAttachmentStore::OpenOrCreate(
     const base::FilePath& path) {
   DCHECK(CalledOnValidThread());
   DCHECK(!db_);
-  Result result_code = UNSPECIFIED_ERROR;
   base::FilePath leveldb_path = path.Append(kLeveldbDirectory);
 
-  leveldb::DB* db;
+  leveldb::DB* db_raw;
+  scoped_ptr<leveldb::DB> db;
   leveldb::Options options;
   options.create_if_missing = true;
   // TODO(pavely): crbug/424287 Consider adding info_log, block_cache and
   // filter_policy to options.
   leveldb::Status status =
-      leveldb::DB::Open(options, leveldb_path.AsUTF8Unsafe(), &db);
+      leveldb::DB::Open(options, leveldb_path.AsUTF8Unsafe(), &db_raw);
   if (!status.ok()) {
     DVLOG(1) << "DB::Open failed: status=" << status.ToString()
              << ", path=" << path.AsUTF8Unsafe();
-  } else {
+    return UNSPECIFIED_ERROR;
-    db_.reset(db);
-    result_code = SUCCESS;
   }
-  return result_code;
+
+  db.reset(db_raw);
+
+  attachment_store_pb::AttachmentStoreMetadata metadata;
+  status = ReadStoreMetadata(db.get(), &metadata);
+  if (!status.ok() && !status.IsNotFound()) {
+    DVLOG(1) << "ReadStoreMetadata failed: status=" << status.ToString();
+    return UNSPECIFIED_ERROR;
+  }
+  if (status.IsNotFound()) {
+    // Brand new database.
+    metadata.set_schema_version(kCurrentSchemaVersion);
+    status = WriteStoreMetadata(db.get(), metadata);
+    if (!status.ok()) {
+      DVLOG(1) << "WriteStoreMetadata failed: status=" << status.ToString();
+      return UNSPECIFIED_ERROR;
+    }
+  }
+  DCHECK(status.ok());
+
+  // Upgrade code goes here.
+
+  if (metadata.schema_version() != kCurrentSchemaVersion) {
+    DVLOG(1) << "Unknown schema version: " << metadata.schema_version();
+    return UNSPECIFIED_ERROR;
+  }
+
+  db_ = db.Pass();
+  return SUCCESS;
 }
 
-std::string OnDiskAttachmentStore::CreateDataKeyFromAttachmentId(
+std::string OnDiskAttachmentStore::MakeDataKeyFromAttachmentId(
     const AttachmentId& attachment_id) {
   std::string key = kDataPrefix + attachment_id.GetProto().unique_id();
   return key;

sync/internal_api/attachments/on_disk_attachment_store_unittest.cc

@@ -12,7 +12,12 @@
 #include "base/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "sync/internal_api/attachments/attachment_store_test_template.h"
+#include "sync/internal_api/attachments/proto/attachment_store.pb.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/leveldatabase/src/include/leveldb/db.h"
+#include "third_party/leveldatabase/src/include/leveldb/options.h"
+#include "third_party/leveldatabase/src/include/leveldb/slice.h"
+#include "third_party/leveldatabase/src/include/leveldb/status.h"
 
 namespace syncer {
 
@@ -78,6 +83,32 @@ class OnDiskAttachmentStoreSpecificTest : public testing::Test {
     CopyResult(destination_result, source_result);
   }
 
+  scoped_ptr<leveldb::DB> OpenLevelDB(const base::FilePath& path) {
+    leveldb::DB* db;
+    leveldb::Options options;
+    options.create_if_missing = true;
+    leveldb::Status s = leveldb::DB::Open(options, path.AsUTF8Unsafe(), &db);
+    EXPECT_TRUE(s.ok());
+    return make_scoped_ptr(db);
+  }
+
+  void UpdateStoreMetadataRecord(const base::FilePath& path,
+                                 const std::string& content) {
+    scoped_ptr<leveldb::DB> db = OpenLevelDB(path);
+    leveldb::Status s =
+        db->Put(leveldb::WriteOptions(), "database-metadata", content);
+    EXPECT_TRUE(s.ok());
+  }
+
+  std::string ReadStoreMetadataRecord(const base::FilePath& path) {
+    scoped_ptr<leveldb::DB> db = OpenLevelDB(path);
+    std::string content;
+    leveldb::Status s =
+        db->Get(leveldb::ReadOptions(), "database-metadata", &content);
+    EXPECT_TRUE(s.ok());
+    return content;
+  }
+
   void RunLoop() {
     base::RunLoop run_loop;
     run_loop.RunUntilIdle();
@@ -140,8 +171,8 @@ TEST_F(OnDiskAttachmentStoreSpecificTest, FailToOpen) {
       temp_dir_.path().Append(FILE_PATH_LITERAL("leveldb"));
   base::CreateDirectory(db_path);
 
-  // To simulate corrupt database write garbage to CURRENT file.
+  // To simulate corrupt database write empty CURRENT file.
-  std::string current_file_content = "abra.cadabra";
+  std::string current_file_content = "";
   base::WriteFile(db_path.Append(FILE_PATH_LITERAL("CURRENT")),
                   current_file_content.c_str(),
                   current_file_content.size());
@@ -156,4 +187,61 @@ TEST_F(OnDiskAttachmentStoreSpecificTest, FailToOpen) {
   EXPECT_EQ(store_.get(), nullptr);
 }
 
+// Ensure that attachment store works correctly when store metadata is missing,
+// corrupt or has unknown schema version.
+TEST_F(OnDiskAttachmentStoreSpecificTest, StoreMetadata) {
+  ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+  base::FilePath db_path =
+      temp_dir_.path().Append(FILE_PATH_LITERAL("leveldb"));
+  base::CreateDirectory(db_path);
+
+  // Create and close empty database.
+  OpenLevelDB(db_path);
+  // Open database with AttachmentStore.
+  AttachmentStore::Result result = AttachmentStore::UNSPECIFIED_ERROR;
+  AttachmentStore::CreateOnDiskStore(
+      temp_dir_.path(),
+      base::ThreadTaskRunnerHandle::Get(),
+      base::Bind(&AttachmentStoreCreated, &store_, &result));
+  RunLoop();
+  EXPECT_EQ(result, AttachmentStore::SUCCESS);
+  // Close AttachmentStore so that test can check content.
+  store_ = nullptr;
+  RunLoop();
+
+  // AttachmentStore should create metadata record.
+  std::string data = ReadStoreMetadataRecord(db_path);
+  attachment_store_pb::AttachmentStoreMetadata metadata;
+  EXPECT_TRUE(metadata.ParseFromString(data));
+  EXPECT_EQ(metadata.schema_version(), 1);
+
+  // Set unknown future schema version.
+  metadata.set_schema_version(2);
+  data = metadata.SerializeAsString();
+  UpdateStoreMetadataRecord(db_path, data);
+
+  // AttachmentStore should fail to load.
+  result = AttachmentStore::SUCCESS;
+  AttachmentStore::CreateOnDiskStore(
+      temp_dir_.path(),
+      base::ThreadTaskRunnerHandle::Get(),
+      base::Bind(&AttachmentStoreCreated, &store_, &result));
+  RunLoop();
+  EXPECT_EQ(result, AttachmentStore::UNSPECIFIED_ERROR);
+  EXPECT_EQ(store_.get(), nullptr);
+
+  // Write garbage into metadata record.
+  UpdateStoreMetadataRecord(db_path, "abra.cadabra");
+
+  // AttachmentStore should fail to load.
+  result = AttachmentStore::SUCCESS;
+  AttachmentStore::CreateOnDiskStore(
+      temp_dir_.path(),
+      base::ThreadTaskRunnerHandle::Get(),
+      base::Bind(&AttachmentStoreCreated, &store_, &result));
+  RunLoop();
+  EXPECT_EQ(result, AttachmentStore::UNSPECIFIED_ERROR);
+  EXPECT_EQ(store_.get(), nullptr);
+}
+
 }  // namespace syncer

sync/internal_api/attachments/proto/BUILD.gn

@@ -0,0 +1,16 @@
+# Copyright 2014 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.
+
+import("//third_party/protobuf/proto_library.gni")
+
+proto_library("proto") {
+  sources = [
+    "attachment_store.proto",
+  ]
+  cc_generator_options = "dllexport_decl=SYNC_EXPORT_PRIVATE:"
+  cc_include = "sync/base/sync_export.h"
+
+  defines = [ "SYNC_IMPLEMENTATION" ]
+
+}

sync/internal_api/attachments/proto/attachment_store.proto

@@ -0,0 +1,18 @@
+// Copyright 2014 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.
+
+syntax = "proto2";
+
+option optimize_for = LITE_RUNTIME;
+option retain_unknown_fields = true;
+
+package attachment_store_pb;
+
+// Metadata for leveldb attachment store database.
+message AttachmentStoreMetadata {
+  // |schema_version| indicates format in which data is written in attachment
+  // store. Needed for upgrade and to prevent newer data from being loaded by
+  // older code that doesn't understand it.
+  optional int32 schema_version = 1;
+}

sync/internal_api/public/attachments/on_disk_attachment_store.h

@@ -47,7 +47,7 @@ class SYNC_EXPORT OnDiskAttachmentStore : public AttachmentStoreBase,
   Result OpenOrCreate(const base::FilePath& path);
 
  private:
-  std::string CreateDataKeyFromAttachmentId(const AttachmentId& attachment_id);
+  std::string MakeDataKeyFromAttachmentId(const AttachmentId& attachment_id);
 
   scoped_refptr<base::SequencedTaskRunner> callback_task_runner_;
   scoped_ptr<leveldb::DB> db_;

sync/sync.gyp

@@ -34,7 +34,7 @@
     {
       'target_name': 'sync_core',
       'type': '<(component)',
-      'variables': { 'enable_wexit_time_desctructors': 1, },
+      'variables': { 'enable_wexit_time_destructors': 1, },
       'defines': [
         'SYNC_IMPLEMENTATION',
       ],
@@ -52,6 +52,7 @@
         '../third_party/protobuf/protobuf.gyp:protobuf_lite',
         '../third_party/zlib/zlib.gyp:zlib',
         '../url/url.gyp:url_lib',
+        'attachment_store_proto',
         'sync_proto',
       ],
       'export_dependent_settings': [
@@ -508,7 +509,7 @@
         'protocol/unique_position.proto',
       ],
       'variables': {
-        'enable_wexit_time_desctructors': 1,
+        'enable_wexit_time_destructors': 1,
         'proto_in_dir': './protocol',
         'proto_out_dir': 'sync/protocol',
         'cc_generator_options': 'dllexport_decl=SYNC_PROTO_EXPORT:',
@@ -518,5 +519,32 @@
         '../build/protoc.gypi'
       ],
     },
+    {
+      # Contains attachment_store protobuf definitions.  Do not depend on this
+      # directly.
+      # Depend on the 'sync' target to get the relevant C++ code, too.
+      #
+      # GN version: //sync/internal_api/attachments/proto
+      'target_name': 'attachment_store_proto',
+      'type': 'static_library',
+      'sources': [
+        # NOTE: If you add a file to this list, also add it to
+        # sync/internal_api/attachments/proto/BUILD.gn
+        'internal_api/attachments/proto/attachment_store.proto',
+      ],
+      'variables': {
+        'enable_wexit_time_destructors': 1,
+        'proto_in_dir': 'internal_api/attachments/proto',
+        'proto_out_dir': 'sync/internal_api/attachments/proto',
+        'cc_generator_options': 'dllexport_decl=SYNC_EXPORT_PRIVATE:',
+        'cc_include': 'sync/base/sync_export.h',
+      },
+      'includes': [
+        '../build/protoc.gypi'
+      ],
+      'defines': [
+        'SYNC_IMPLEMENTATION'
+      ],
+    },
   ],
 }

sync/sync_tests.gypi

@@ -249,6 +249,7 @@
         '../sql/sql.gyp:sql',
         '../testing/gmock.gyp:gmock',
         '../testing/gtest.gyp:gtest',
+        '../third_party/leveldatabase/leveldatabase.gyp:leveldatabase',
         '../third_party/protobuf/protobuf.gyp:protobuf_lite',
         'sync',
         'test_support_sync_core',