Add per attachment metadata records.

Currently metadata record only contains size and it is not used anywhere.
In the future size will be used for storage management by datatypes.

Next step is to add crc to metadata record.

BUG=424304
R=maniscalco@chromium.org

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

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

sync/internal_api/attachments/on_disk_attachment_store.cc

@@ -15,6 +15,7 @@
 #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"
+#include "third_party/leveldatabase/src/include/leveldb/write_batch.h"
 
 namespace syncer {
 
@@ -23,6 +24,9 @@ namespace {
 // Prefix for records containing attachment data.
 const char kDataPrefix[] = "data-";
 
+// Prefix for records containing attachment metadata.
+const char kMetadataPrefix[] = "metadata-";
+
 const char kDatabaseMetadataKey[] = "database-metadata";
 
 const int32 kCurrentSchemaVersion = 1;
@@ -36,16 +40,29 @@ leveldb::WriteOptions MakeWriteOptions() {
   return write_options;
 }
 
-leveldb::Status ReadStoreMetadata(
-    leveldb::DB* db,
-    attachment_store_pb::AttachmentStoreMetadata* metadata) {
-  std::string data_str;
+leveldb::ReadOptions MakeDataReadOptions() {
   leveldb::ReadOptions read_options;
+  // Attachment content is typically large and only read once. Don't cache it on
+  // db level.
   read_options.fill_cache = false;
   read_options.verify_checksums = true;
+  return read_options;
+}
+
+leveldb::ReadOptions MakeMetadataReadOptions() {
+  leveldb::ReadOptions read_options;
+  read_options.fill_cache = true;
+  read_options.verify_checksums = true;
+  return read_options;
+}
+
+leveldb::Status ReadStoreMetadata(
+    leveldb::DB* db,
+    attachment_store_pb::StoreMetadata* metadata) {
+  std::string data_str;
 
   leveldb::Status status =
-      db->Get(read_options, kDatabaseMetadataKey, &data_str);
+      db->Get(MakeMetadataReadOptions(), kDatabaseMetadataKey, &data_str);
   if (!status.ok())
     return status;
   if (!metadata->ParseFromString(data_str))
@@ -55,7 +72,7 @@ leveldb::Status ReadStoreMetadata(
 
 leveldb::Status WriteStoreMetadata(
     leveldb::DB* db,
-    const attachment_store_pb::AttachmentStoreMetadata& metadata) {
+    const attachment_store_pb::StoreMetadata& metadata) {
   std::string data_str;
 
   metadata.SerializeToString(&data_str);
@@ -79,27 +96,15 @@ void OnDiskAttachmentStore::Read(const AttachmentIdList& ids,
   scoped_ptr<AttachmentMap> result_map(new AttachmentMap());
   scoped_ptr<AttachmentIdList> unavailable_attachments(new AttachmentIdList());
 
-  leveldb::ReadOptions read_options;
-  // Attachment content is typically large and only read once. Don't cache it on
-  // db level.
-  read_options.fill_cache = false;
-  read_options.verify_checksums = true;
-
   AttachmentIdList::const_iterator iter = ids.begin();
   const AttachmentIdList::const_iterator end = ids.end();
   for (; iter != end; ++iter) {
-    const std::string key = MakeDataKeyFromAttachmentId(*iter);
-    std::string data_str;
-    leveldb::Status status = db_->Get(read_options, key, &data_str);
-    if (!status.ok()) {
-      DVLOG(1) << "DB::Get failed: status=" << status.ToString();
+    scoped_ptr<Attachment> attachment = ReadSingleAttachment(*iter);
+    if (attachment) {
+      result_map->insert(std::make_pair(*iter, *attachment));
+    } else {
       unavailable_attachments->push_back(*iter);
-      continue;
     }
-    scoped_refptr<base::RefCountedMemory> data =
-        base::RefCountedString::TakeString(&data_str);
-    Attachment attachment = Attachment::CreateWithId(*iter, data);
-    result_map->insert(std::make_pair(*iter, attachment));
   }
 
   Result result_code =
@@ -118,40 +123,11 @@ void OnDiskAttachmentStore::Write(const AttachmentList& attachments,
   DCHECK(db_);
   Result result_code = SUCCESS;
 
-  leveldb::ReadOptions read_options;
-  read_options.fill_cache = false;
-  read_options.verify_checksums = true;
-
-  leveldb::WriteOptions write_options = MakeWriteOptions();
-
   AttachmentList::const_iterator iter = attachments.begin();
   const AttachmentList::const_iterator end = attachments.end();
   for (; iter != end; ++iter) {
-    const std::string key = MakeDataKeyFromAttachmentId(iter->GetId());
-
-    std::string data_str;
-    // TODO(pavely): crbug/424304 This read is expensive. When I add metadata
-    // records this read will target metadata record instead of payload record.
-    leveldb::Status status = db_->Get(read_options, key, &data_str);
-    if (status.ok()) {
-      // Entry exists, don't overwrite.
-      continue;
-    } else if (!status.IsNotFound()) {
-      // Entry exists but failed to read.
-      DVLOG(1) << "DB::Get failed: status=" << status.ToString();
+    if (!WriteSingleAttachment(*iter))
       result_code = UNSPECIFIED_ERROR;
-      continue;
-    }
-    DCHECK(status.IsNotFound());
-
-    scoped_refptr<base::RefCountedMemory> data = iter->GetData();
-    leveldb::Slice data_slice(data->front_as<char>(), data->size());
-    status = db_->Put(write_options, key, data_slice);
-    if (!status.ok()) {
-      // Failed to write.
-      DVLOG(1) << "DB::Put failed: status=" << status.ToString();
-      result_code = UNSPECIFIED_ERROR;
-    }
   }
   callback_task_runner_->PostTask(FROM_HERE, base::Bind(callback, result_code));
 }
@@ -165,12 +141,15 @@ void OnDiskAttachmentStore::Drop(const AttachmentIdList& ids,
   AttachmentIdList::const_iterator iter = ids.begin();
   const AttachmentIdList::const_iterator end = ids.end();
   for (; iter != end; ++iter) {
-    const std::string key = MakeDataKeyFromAttachmentId(*iter);
-    leveldb::Status status = db_->Delete(write_options, key);
+    leveldb::WriteBatch write_batch;
+    write_batch.Delete(MakeDataKeyFromAttachmentId(*iter));
+    write_batch.Delete(MakeMetadataKeyFromAttachmentId(*iter));
+
+    leveldb::Status status = db_->Write(write_options, &write_batch);
     if (!status.ok()) {
       // DB::Delete doesn't check if record exists, it returns ok just like
       // AttachmentStore::Drop should.
-      DVLOG(1) << "DB::Delete failed: status=" << status.ToString();
+      DVLOG(1) << "DB::Write failed: status=" << status.ToString();
       result_code = UNSPECIFIED_ERROR;
     }
   }
@@ -199,7 +178,7 @@ AttachmentStore::Result OnDiskAttachmentStore::OpenOrCreate(
 
   db.reset(db_raw);
 
-  attachment_store_pb::AttachmentStoreMetadata metadata;
+  attachment_store_pb::StoreMetadata metadata;
   status = ReadStoreMetadata(db.get(), &metadata);
   if (!status.ok() && !status.IsNotFound()) {
     DVLOG(1) << "ReadStoreMetadata failed: status=" << status.ToString();
@@ -227,10 +206,73 @@ AttachmentStore::Result OnDiskAttachmentStore::OpenOrCreate(
   return SUCCESS;
 }
 
+scoped_ptr<Attachment> OnDiskAttachmentStore::ReadSingleAttachment(
+    const AttachmentId& attachment_id) {
+  scoped_ptr<Attachment> attachment;
+
+  const std::string key = MakeDataKeyFromAttachmentId(attachment_id);
+  std::string data_str;
+  leveldb::Status status = db_->Get(MakeDataReadOptions(), key, &data_str);
+  if (status.ok()) {
+    scoped_refptr<base::RefCountedMemory> data =
+        base::RefCountedString::TakeString(&data_str);
+    attachment.reset(
+        new Attachment(Attachment::CreateWithId(attachment_id, data)));
+  } else {
+    DVLOG(1) << "DB::Get failed: status=" << status.ToString();
+  }
+  return attachment.Pass();
+}
+
+bool OnDiskAttachmentStore::WriteSingleAttachment(
+    const Attachment& attachment) {
+  const std::string metadata_key =
+      MakeMetadataKeyFromAttachmentId(attachment.GetId());
+  const std::string data_key = MakeDataKeyFromAttachmentId(attachment.GetId());
+
+  std::string metadata_str;
+  leveldb::Status status =
+      db_->Get(MakeMetadataReadOptions(), metadata_key, &metadata_str);
+  if (status.ok()) {
+    // Entry exists, don't overwrite.
+    return true;
+  } else if (!status.IsNotFound()) {
+    // Entry exists but failed to read.
+    DVLOG(1) << "DB::Get failed: status=" << status.ToString();
+    return false;
+  }
+  DCHECK(status.IsNotFound());
+
+  leveldb::WriteBatch write_batch;
+  // Write metadata.
+  attachment_store_pb::RecordMetadata metadata;
+  metadata.set_attachment_size(attachment.GetData()->size());
+  metadata_str = metadata.SerializeAsString();
+  write_batch.Put(metadata_key, metadata_str);
+  // Write data.
+  scoped_refptr<base::RefCountedMemory> data = attachment.GetData();
+  leveldb::Slice data_slice(data->front_as<char>(), data->size());
+  write_batch.Put(data_key, data_slice);
+
+  status = db_->Write(MakeWriteOptions(), &write_batch);
+  if (!status.ok()) {
+    // Failed to write.
+    DVLOG(1) << "DB::Write failed: status=" << status.ToString();
+    return false;
+  }
+  return true;
+}
+
 std::string OnDiskAttachmentStore::MakeDataKeyFromAttachmentId(
     const AttachmentId& attachment_id) {
   std::string key = kDataPrefix + attachment_id.GetProto().unique_id();
   return key;
 }
 
+std::string OnDiskAttachmentStore::MakeMetadataKeyFromAttachmentId(
+    const AttachmentId& attachment_id) {
+  std::string key = kMetadataPrefix + attachment_id.GetProto().unique_id();
+  return key;
+}
+
 }  // namespace syncer

sync/internal_api/attachments/on_disk_attachment_store_unittest.cc

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "sync/api/attachments/attachment_store.h"
+#include "sync/internal_api/public/attachments/on_disk_attachment_store.h"
 
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
@@ -109,6 +109,28 @@ class OnDiskAttachmentStoreSpecificTest : public testing::Test {
     return content;
   }
 
+  void VerifyAttachmentRecordsPresent(const base::FilePath& path,
+                                      const AttachmentId& attachment_id,
+                                      bool expect_records_present) {
+    scoped_ptr<leveldb::DB> db = OpenLevelDB(path);
+
+    std::string metadata_key =
+        OnDiskAttachmentStore::MakeMetadataKeyFromAttachmentId(attachment_id);
+    std::string data_key =
+        OnDiskAttachmentStore::MakeDataKeyFromAttachmentId(attachment_id);
+    std::string data;
+    leveldb::Status s = db->Get(leveldb::ReadOptions(), data_key, &data);
+    if (expect_records_present)
+      EXPECT_TRUE(s.ok());
+    else
+      EXPECT_TRUE(s.IsNotFound());
+    s = db->Get(leveldb::ReadOptions(), metadata_key, &data);
+    if (expect_records_present)
+      EXPECT_TRUE(s.ok());
+    else
+      EXPECT_TRUE(s.IsNotFound());
+  }
+
   void RunLoop() {
     base::RunLoop run_loop;
     run_loop.RunUntilIdle();
@@ -211,7 +233,7 @@ TEST_F(OnDiskAttachmentStoreSpecificTest, StoreMetadata) {
 
   // AttachmentStore should create metadata record.
   std::string data = ReadStoreMetadataRecord(db_path);
-  attachment_store_pb::AttachmentStoreMetadata metadata;
+  attachment_store_pb::StoreMetadata metadata;
   EXPECT_TRUE(metadata.ParseFromString(data));
   EXPECT_EQ(metadata.schema_version(), 1);
 
@@ -244,4 +266,51 @@ TEST_F(OnDiskAttachmentStoreSpecificTest, StoreMetadata) {
   EXPECT_EQ(store_.get(), nullptr);
 }
 
+TEST_F(OnDiskAttachmentStoreSpecificTest, RecordMetadata) {
+  ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+  base::FilePath db_path =
+      temp_dir_.path().Append(FILE_PATH_LITERAL("leveldb"));
+
+  // Create attachment store.
+  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);
+
+  // Write two attachments.
+  std::string some_data;
+  AttachmentList attachments;
+  some_data = "data1";
+  attachments.push_back(
+      Attachment::Create(base::RefCountedString::TakeString(&some_data)));
+  some_data = "data2";
+  attachments.push_back(
+      Attachment::Create(base::RefCountedString::TakeString(&some_data)));
+  store_->Write(attachments,
+                base::Bind(&OnDiskAttachmentStoreSpecificTest::CopyResult,
+                           base::Unretained(this),
+                           &result));
+  RunLoop();
+  EXPECT_EQ(result, AttachmentStore::SUCCESS);
+
+  // Delete one of written attachments.
+  AttachmentIdList attachment_ids;
+  attachment_ids.push_back(attachments[0].GetId());
+  store_->Drop(attachment_ids,
+               base::Bind(&OnDiskAttachmentStoreSpecificTest::CopyResult,
+                          base::Unretained(this),
+                          &result));
+  RunLoop();
+  EXPECT_EQ(result, AttachmentStore::SUCCESS);
+  store_ = nullptr;
+  RunLoop();
+
+  // Verify that attachment store contains only records for second attachment.
+  VerifyAttachmentRecordsPresent(db_path, attachments[0].GetId(), false);
+  VerifyAttachmentRecordsPresent(db_path, attachments[1].GetId(), true);
+}
+
 }  // namespace syncer

sync/internal_api/attachments/proto/attachment_store.proto

@@ -10,9 +10,18 @@ option retain_unknown_fields = true;
 package attachment_store_pb;
 
 // Metadata for leveldb attachment store database.
-message AttachmentStoreMetadata {
+message StoreMetadata {
   // |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;
 }
+
+// Metadata for attachment in attachment store. Storing metadata in separate
+// record from actual data allows us to enumerate attachments in the store
+// without incurring cost to read actual data. It also allows us to update
+// attachment metadata independent of the data.
+message RecordMetadata {
+  // Size of attachment data. Useful for attachment store space management.
+  optional int64 attachment_size = 1;
+}

sync/internal_api/public/attachments/on_disk_attachment_store.h

@@ -47,7 +47,18 @@ class SYNC_EXPORT OnDiskAttachmentStore : public AttachmentStoreBase,
   Result OpenOrCreate(const base::FilePath& path);
 
  private:
-  std::string MakeDataKeyFromAttachmentId(const AttachmentId& attachment_id);
+  friend class OnDiskAttachmentStoreSpecificTest;
+
+  // Reads single attachment from store. Returns nullptr in case of errors.
+  scoped_ptr<Attachment> ReadSingleAttachment(
+      const AttachmentId& attachment_id);
+  // Writes single attachment to store. Returns false in case of errors.
+  bool WriteSingleAttachment(const Attachment& attachment);
+
+  static std::string MakeDataKeyFromAttachmentId(
+      const AttachmentId& attachment_id);
+  static std::string MakeMetadataKeyFromAttachmentId(
+      const AttachmentId& attachment_id);
 
   scoped_refptr<base::SequencedTaskRunner> callback_task_runner_;
   scoped_ptr<leveldb::DB> db_;