Implement AttachmentStore metadata enumeration API.

Added implementation of ReadMetadata and ReadAllMetadata for both
OnDiskAttachmentStore and InMemoryAttachmentStore.
Added new unit tests for these two APIs.
Fixed order of arguments in all EXPECT_EQ macros in all AttachmentStore tests.
Please note that the expected value should go first and the tested value - second.
The order is important to make sense of the error message when the
condition test fails.

BUG=435205

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

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

5 files changed, 405 insertions, 106 deletions

diff --git a/sync/internal_api/attachments/attachment_store_test_template.h b/sync/internal_api/attachments/attachment_store_test_template.h
index cd1c016..b47f07e 100644
--- a/sync/internal_api/attachments/attachment_store_test_template.h
+++ b/sync/internal_api/attachments/attachment_store_test_template.h
@@ -16,6 +16,7 @@
 #include "sync/api/attachments/attachment.h"
 #include "sync/internal_api/public/attachments/attachment_util.h"
 #include "sync/protocol/sync.pb.h"
+#include "testing/gmock/include/gmock/gmock-matchers.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 // AttachmentStoreTest defines tests for AttachmentStore. To instantiate these
@@ -50,17 +51,19 @@ class AttachmentStoreTest : public testing::Test {
   AttachmentStore::Result result;
   scoped_ptr<AttachmentMap> attachments;
   scoped_ptr<AttachmentIdList> failed_attachment_ids;
+  scoped_ptr<AttachmentMetadataList> attachment_metadata;
 
   AttachmentStore::ReadCallback read_callback;
   AttachmentStore::WriteCallback write_callback;
   AttachmentStore::DropCallback drop_callback;
+  AttachmentStore::ReadMetadataCallback read_metadata_callback;
 
   scoped_refptr<base::RefCountedString> some_data1;
   scoped_refptr<base::RefCountedString> some_data2;
 
   AttachmentStoreTest() {}
 
-  virtual void SetUp() {
+  void SetUp() override {
     store = attachment_store_factory.CreateAttachmentStore();
 
     Clear();
@@ -72,6 +75,9 @@ class AttachmentStoreTest : public testing::Test {
     write_callback = base::Bind(
         &AttachmentStoreTest::CopyResult, base::Unretained(this), &result);
     drop_callback = write_callback;
+    read_metadata_callback =
+        base::Bind(&AttachmentStoreTest::CopyResultMetadata,
+                   base::Unretained(this), &result, &attachment_metadata);
 
     some_data1 = new base::RefCountedString;
     some_data1->data() = kTestData1;
@@ -80,7 +86,7 @@ class AttachmentStoreTest : public testing::Test {
     some_data2->data() = kTestData2;
   }
 
-  virtual void ClearAndPumpLoop() {
+  void ClearAndPumpLoop() {
     Clear();
     message_loop.RunUntilIdle();
   }
@@ -90,6 +96,7 @@ class AttachmentStoreTest : public testing::Test {
     result = AttachmentStore::UNSPECIFIED_ERROR;
     attachments.reset();
     failed_attachment_ids.reset();
+    attachment_metadata.reset();
   }
 
   void CopyResult(AttachmentStore::Result* destination_result,
@@ -108,6 +115,15 @@ class AttachmentStoreTest : public testing::Test {
     *destination_attachments = source_attachments.Pass();
     *destination_failed_attachment_ids = source_failed_attachment_ids.Pass();
   }
+
+  void CopyResultMetadata(
+      AttachmentStore::Result* destination_result,
+      scoped_ptr<AttachmentMetadataList>* destination_metadata,
+      const AttachmentStore::Result& source_result,
+      scoped_ptr<AttachmentMetadataList> source_metadata) {
+    CopyResult(destination_result, source_result);
+    *destination_metadata = source_metadata.Pass();
+  }
 };
 
 TYPED_TEST_CASE_P(AttachmentStoreTest);
@@ -126,23 +142,23 @@ TYPED_TEST_P(AttachmentStoreTest, Write_NoOverwriteNoError) {
   some_attachments.push_back(attachment1);
   this->store->Write(some_attachments, this->write_callback);
   this->ClearAndPumpLoop();
-  EXPECT_EQ(this->result, AttachmentStore::SUCCESS);
+  EXPECT_EQ(AttachmentStore::SUCCESS, this->result);
 
   // Write the second one.
   some_attachments.clear();
   some_attachments.push_back(attachment2);
   this->store->Write(some_attachments, this->write_callback);
   this->ClearAndPumpLoop();
-  EXPECT_EQ(this->result, AttachmentStore::SUCCESS);
+  EXPECT_EQ(AttachmentStore::SUCCESS, this->result);
 
   // Read it back and see that it was not overwritten.
   AttachmentIdList some_attachment_ids;
   some_attachment_ids.push_back(attachment1.GetId());
   this->store->Read(some_attachment_ids, this->read_callback);
   this->ClearAndPumpLoop();
-  EXPECT_EQ(this->result, AttachmentStore::SUCCESS);
-  EXPECT_EQ(this->attachments->size(), 1U);
-  EXPECT_EQ(this->failed_attachment_ids->size(), 0U);
+  EXPECT_EQ(AttachmentStore::SUCCESS, this->result);
+  EXPECT_EQ(1U, this->attachments->size());
+  EXPECT_EQ(0U, this->failed_attachment_ids->size());
   AttachmentMap::const_iterator a1 =
       this->attachments->find(attachment1.GetId());
   EXPECT_TRUE(a1 != this->attachments->end());
@@ -159,16 +175,16 @@ TYPED_TEST_P(AttachmentStoreTest, Write_RoundTrip) {
 
   this->store->Write(some_attachments, this->write_callback);
   this->ClearAndPumpLoop();
-  EXPECT_EQ(this->result, AttachmentStore::SUCCESS);
+  EXPECT_EQ(AttachmentStore::SUCCESS, this->result);
 
   AttachmentIdList some_attachment_ids;
   some_attachment_ids.push_back(attachment1.GetId());
   some_attachment_ids.push_back(attachment2.GetId());
   this->store->Read(some_attachment_ids, this->read_callback);
   this->ClearAndPumpLoop();
-  EXPECT_EQ(this->result, AttachmentStore::SUCCESS);
-  EXPECT_EQ(this->attachments->size(), 2U);
-  EXPECT_EQ(this->failed_attachment_ids->size(), 0U);
+  EXPECT_EQ(AttachmentStore::SUCCESS, this->result);
+  EXPECT_EQ(2U, this->attachments->size());
+  EXPECT_EQ(0U, this->failed_attachment_ids->size());
 
   AttachmentMap::const_iterator a1 =
       this->attachments->find(attachment1.GetId());
@@ -191,7 +207,7 @@ TYPED_TEST_P(AttachmentStoreTest, Read_OneNotFound) {
   some_attachments.push_back(attachment1);
   this->store->Write(some_attachments, this->write_callback);
   this->ClearAndPumpLoop();
-  EXPECT_EQ(this->result, AttachmentStore::SUCCESS);
+  EXPECT_EQ(AttachmentStore::SUCCESS, this->result);
 
   // Try to read both attachment1 and attachment2.
   AttachmentIdList ids;
@@ -201,9 +217,9 @@ TYPED_TEST_P(AttachmentStoreTest, Read_OneNotFound) {
   this->ClearAndPumpLoop();
 
   // See that only attachment1 was read.
-  EXPECT_EQ(this->result, AttachmentStore::UNSPECIFIED_ERROR);
-  EXPECT_EQ(this->attachments->size(), 1U);
-  EXPECT_EQ(this->failed_attachment_ids->size(), 1U);
+  EXPECT_EQ(AttachmentStore::UNSPECIFIED_ERROR, this->result);
+  EXPECT_EQ(1U, this->attachments->size());
+  EXPECT_EQ(1U, this->failed_attachment_ids->size());
 }
 
 // Try to drop two attachments when only one exists. Verify that no error occurs
@@ -217,22 +233,22 @@ TYPED_TEST_P(AttachmentStoreTest, Drop_DropTwoButOnlyOneExists) {
   some_attachments.push_back(attachment2);
   this->store->Write(some_attachments, this->write_callback);
   this->ClearAndPumpLoop();
-  EXPECT_EQ(this->result, AttachmentStore::SUCCESS);
+  EXPECT_EQ(AttachmentStore::SUCCESS, this->result);
 
   // Drop attachment1 only.
   AttachmentIdList ids;
   ids.push_back(attachment1.GetId());
   this->store->Drop(ids, this->drop_callback);
   this->ClearAndPumpLoop();
-  EXPECT_EQ(this->result, AttachmentStore::SUCCESS);
+  EXPECT_EQ(AttachmentStore::SUCCESS, this->result);
 
   // See that attachment1 is gone.
   this->store->Read(ids, this->read_callback);
   this->ClearAndPumpLoop();
-  EXPECT_EQ(this->result, AttachmentStore::UNSPECIFIED_ERROR);
-  EXPECT_EQ(this->attachments->size(), 0U);
-  EXPECT_EQ(this->failed_attachment_ids->size(), 1U);
-  EXPECT_EQ((*this->failed_attachment_ids)[0], attachment1.GetId());
+  EXPECT_EQ(AttachmentStore::UNSPECIFIED_ERROR, this->result);
+  EXPECT_EQ(0U, this->attachments->size());
+  EXPECT_EQ(1U, this->failed_attachment_ids->size());
+  EXPECT_EQ(attachment1.GetId(), (*this->failed_attachment_ids)[0]);
 
   // Drop both attachment1 and attachment2.
   ids.clear();
@@ -240,17 +256,17 @@ TYPED_TEST_P(AttachmentStoreTest, Drop_DropTwoButOnlyOneExists) {
   ids.push_back(attachment2.GetId());
   this->store->Drop(ids, this->drop_callback);
   this->ClearAndPumpLoop();
-  EXPECT_EQ(this->result, AttachmentStore::SUCCESS);
+  EXPECT_EQ(AttachmentStore::SUCCESS, this->result);
 
   // See that attachment2 is now gone.
   ids.clear();
   ids.push_back(attachment2.GetId());
   this->store->Read(ids, this->read_callback);
   this->ClearAndPumpLoop();
-  EXPECT_EQ(this->result, AttachmentStore::UNSPECIFIED_ERROR);
-  EXPECT_EQ(this->attachments->size(), 0U);
-  EXPECT_EQ(this->failed_attachment_ids->size(), 1U);
-  EXPECT_EQ((*this->failed_attachment_ids)[0], attachment2.GetId());
+  EXPECT_EQ(AttachmentStore::UNSPECIFIED_ERROR, this->result);
+  EXPECT_EQ(0U, this->attachments->size());
+  EXPECT_EQ(1U, this->failed_attachment_ids->size());
+  EXPECT_EQ(attachment2.GetId(), (*this->failed_attachment_ids)[0]);
 }
 
 // Verify that attempting to drop an attachment that does not exist is not an
@@ -261,29 +277,113 @@ TYPED_TEST_P(AttachmentStoreTest, Drop_DoesNotExist) {
   some_attachments.push_back(attachment1);
   this->store->Write(some_attachments, this->write_callback);
   this->ClearAndPumpLoop();
-  EXPECT_EQ(this->result, AttachmentStore::SUCCESS);
+  EXPECT_EQ(AttachmentStore::SUCCESS, this->result);
 
   // Drop the attachment.
   AttachmentIdList ids;
   ids.push_back(attachment1.GetId());
   this->store->Drop(ids, this->drop_callback);
   this->ClearAndPumpLoop();
-  EXPECT_EQ(this->result, AttachmentStore::SUCCESS);
+  EXPECT_EQ(AttachmentStore::SUCCESS, this->result);
 
   // See that it's gone.
   this->store->Read(ids, this->read_callback);
   this->ClearAndPumpLoop();
-  EXPECT_EQ(this->result, AttachmentStore::UNSPECIFIED_ERROR);
-  EXPECT_EQ(this->attachments->size(), 0U);
-  EXPECT_EQ(this->failed_attachment_ids->size(), 1U);
-  EXPECT_EQ((*this->failed_attachment_ids)[0], attachment1.GetId());
+  EXPECT_EQ(AttachmentStore::UNSPECIFIED_ERROR, this->result);
+  EXPECT_EQ(0U, this->attachments->size());
+  EXPECT_EQ(1U, this->failed_attachment_ids->size());
+  EXPECT_EQ(attachment1.GetId(), (*this->failed_attachment_ids)[0]);
 
   // Drop again, see that no error occurs.
   ids.clear();
   ids.push_back(attachment1.GetId());
   this->store->Drop(ids, this->drop_callback);
   this->ClearAndPumpLoop();
-  EXPECT_EQ(this->result, AttachmentStore::SUCCESS);
+  EXPECT_EQ(AttachmentStore::SUCCESS, this->result);
+}
+
+// Verify getting metadata for specific attachments.
+TYPED_TEST_P(AttachmentStoreTest, ReadMetadata) {
+  Attachment attachment1 = Attachment::Create(this->some_data1);
+  Attachment attachment2 = Attachment::Create(this->some_data2);
+
+  AttachmentList some_attachments;
+  // Write attachment1 only.
+  some_attachments.push_back(attachment1);
+  this->store->Write(some_attachments, this->write_callback);
+  this->ClearAndPumpLoop();
+  EXPECT_EQ(AttachmentStore::SUCCESS, this->result);
+
+  // Try to read metadata for both attachment1 and attachment2.
+  AttachmentIdList ids;
+  ids.push_back(attachment1.GetId());
+  ids.push_back(attachment2.GetId());
+  this->store->ReadMetadata(ids, this->read_metadata_callback);
+  this->ClearAndPumpLoop();
+
+  // See that only one entry was read.
+  EXPECT_EQ(AttachmentStore::UNSPECIFIED_ERROR, this->result);
+  EXPECT_EQ(1U, this->attachment_metadata->size());
+
+  // Now write attachment2.
+  some_attachments[0] = attachment2;
+  this->store->Write(some_attachments, this->write_callback);
+  this->ClearAndPumpLoop();
+  EXPECT_EQ(AttachmentStore::SUCCESS, this->result);
+
+  // Try to read metadata for both attachment1 and attachment2 again.
+  this->store->ReadMetadata(ids, this->read_metadata_callback);
+  this->ClearAndPumpLoop();
+  EXPECT_EQ(AttachmentStore::SUCCESS, this->result);
+  EXPECT_EQ(2U, this->attachment_metadata->size());
+
+  // Verify that we've got both entries back in the right order.
+  AttachmentMetadataList::const_iterator iter =
+      this->attachment_metadata->begin();
+  EXPECT_EQ(attachment1.GetId(), iter->GetId());
+  ++iter;
+  EXPECT_EQ(attachment2.GetId(), iter->GetId());
+}
+
+// Verify getting metadata for all attachments.
+TYPED_TEST_P(AttachmentStoreTest, ReadAllMetadata) {
+  // Try to read all metadata from an empty store.
+  this->store->ReadAllMetadata(this->read_metadata_callback);
+  this->ClearAndPumpLoop();
+  EXPECT_EQ(AttachmentStore::SUCCESS, this->result);
+  EXPECT_EQ(0U, this->attachment_metadata->size());
+
+  // Create and write two attachments.
+  Attachment attachment1 = Attachment::Create(this->some_data1);
+  Attachment attachment2 = Attachment::Create(this->some_data2);
+
+  AttachmentList some_attachments;
+  some_attachments.push_back(attachment1);
+  some_attachments.push_back(attachment2);
+  this->store->Write(some_attachments, this->write_callback);
+  this->ClearAndPumpLoop();
+  EXPECT_EQ(AttachmentStore::SUCCESS, this->result);
+
+  // Read all metadata again.
+  this->store->ReadAllMetadata(this->read_metadata_callback);
+  this->ClearAndPumpLoop();
+  EXPECT_EQ(AttachmentStore::SUCCESS, this->result);
+  EXPECT_EQ(2U, this->attachment_metadata->size());
+
+  // Verify that we get all attachments back (the order is undefined).
+  AttachmentIdSet ids;
+  ids.insert(attachment1.GetId());
+  ids.insert(attachment2.GetId());
+
+  AttachmentMetadataList::const_iterator iter =
+      this->attachment_metadata->begin();
+  const AttachmentMetadataList::const_iterator end =
+      this->attachment_metadata->end();
+  for (; iter != end; ++iter) {
+    EXPECT_THAT(ids, testing::Contains(iter->GetId()));
+    ids.erase(iter->GetId());
+  }
+  EXPECT_TRUE(ids.empty());
 }
 
 REGISTER_TYPED_TEST_CASE_P(AttachmentStoreTest,
@@ -291,7 +391,9 @@ REGISTER_TYPED_TEST_CASE_P(AttachmentStoreTest,
                            Write_RoundTrip,
                            Read_OneNotFound,
                            Drop_DropTwoButOnlyOneExists,
-                           Drop_DoesNotExist);
+                           Drop_DoesNotExist,
+                           ReadMetadata,
+                           ReadAllMetadata);
 
 }  // namespace syncer
 
diff --git a/sync/internal_api/attachments/in_memory_attachment_store.cc b/sync/internal_api/attachments/in_memory_attachment_store.cc
index f5000c7..9ce6c3e 100644
--- a/sync/internal_api/attachments/in_memory_attachment_store.cc
+++ b/sync/internal_api/attachments/in_memory_attachment_store.cc
@@ -11,6 +11,16 @@
 
 namespace syncer {
 
+namespace {
+
+void AppendMetadata(AttachmentMetadataList* list,
+                    const Attachment& attachment) {
+  list->push_back(
+      AttachmentMetadata(attachment.GetId(), attachment.GetData()->size()));
+}
+
+}  // namespace
+
 InMemoryAttachmentStore::InMemoryAttachmentStore(
     const scoped_refptr<base::SingleThreadTaskRunner>& callback_task_runner)
     : callback_task_runner_(callback_task_runner) {
@@ -85,14 +95,40 @@ void InMemoryAttachmentStore::Drop(const AttachmentIdList& ids,
 void InMemoryAttachmentStore::ReadMetadata(
     const AttachmentIdList& ids,
     const ReadMetadataCallback& callback) {
-  // TODO(stanisc): implement this.
-  NOTIMPLEMENTED();
+  DCHECK(CalledOnValidThread());
+  Result result_code = SUCCESS;
+  scoped_ptr<AttachmentMetadataList> metadata_list(
+      new AttachmentMetadataList());
+  AttachmentIdList::const_iterator ids_iter = ids.begin();
+  AttachmentIdList::const_iterator ids_end = ids.end();
+
+  for (; ids_iter != ids_end; ++ids_iter) {
+    AttachmentMap::iterator attachments_iter = attachments_.find(*ids_iter);
+    if (attachments_iter != attachments_.end()) {
+      AppendMetadata(metadata_list.get(), attachments_iter->second);
+    } else {
+      result_code = UNSPECIFIED_ERROR;
+    }
+  }
+  callback_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(callback, result_code, base::Passed(&metadata_list)));
 }
 
 void InMemoryAttachmentStore::ReadAllMetadata(
     const ReadMetadataCallback& callback) {
-  // TODO(stanisc): implement this.
-  NOTIMPLEMENTED();
+  DCHECK(CalledOnValidThread());
+  Result result_code = SUCCESS;
+  scoped_ptr<AttachmentMetadataList> metadata_list(
+      new AttachmentMetadataList());
+
+  for (AttachmentMap::const_iterator iter = attachments_.begin();
+       iter != attachments_.end(); ++iter) {
+    AppendMetadata(metadata_list.get(), iter->second);
+  }
+  callback_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(callback, result_code, base::Passed(&metadata_list)));
 }
 
 }  // namespace syncer
diff --git a/sync/internal_api/attachments/on_disk_attachment_store.cc b/sync/internal_api/attachments/on_disk_attachment_store.cc
index 16fa0c0..309e610 100644
--- a/sync/internal_api/attachments/on_disk_attachment_store.cc
+++ b/sync/internal_api/attachments/on_disk_attachment_store.cc
@@ -41,16 +41,14 @@ leveldb::WriteOptions MakeWriteOptions() {
   return write_options;
 }
 
-leveldb::ReadOptions MakeDataReadOptions() {
+leveldb::ReadOptions MakeNonCachingReadOptions() {
   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 MakeCachingReadOptions() {
   leveldb::ReadOptions read_options;
   read_options.fill_cache = true;
   read_options.verify_checksums = true;
@@ -63,7 +61,7 @@ leveldb::Status ReadStoreMetadata(
   std::string data_str;
 
   leveldb::Status status =
-      db->Get(MakeMetadataReadOptions(), kDatabaseMetadataKey, &data_str);
+      db->Get(MakeCachingReadOptions(), kDatabaseMetadataKey, &data_str);
   if (!status.ok())
     return status;
   if (!metadata->ParseFromString(data_str))
@@ -177,14 +175,72 @@ void OnDiskAttachmentStore::Drop(const AttachmentIdList& ids,
 
 void OnDiskAttachmentStore::ReadMetadata(const AttachmentIdList& ids,
                                          const ReadMetadataCallback& callback) {
-  // TODO(stanisc): implement this.
-  NOTIMPLEMENTED();
+  DCHECK(CalledOnValidThread());
+  Result result_code = STORE_INITIALIZATION_FAILED;
+  scoped_ptr<AttachmentMetadataList> metadata_list(
+      new AttachmentMetadataList());
+  if (db_) {
+    result_code = SUCCESS;
+    AttachmentIdList::const_iterator iter = ids.begin();
+    const AttachmentIdList::const_iterator end = ids.end();
+    for (; iter != end; ++iter) {
+      attachment_store_pb::RecordMetadata record_metadata;
+      if (ReadSingleRecordMetadata(*iter, &record_metadata)) {
+        metadata_list->push_back(
+            MakeAttachmentMetadata(*iter, record_metadata));
+      } else {
+        result_code = UNSPECIFIED_ERROR;
+      }
+    }
+  }
+  callback_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(callback, result_code, base::Passed(&metadata_list)));
 }
 
 void OnDiskAttachmentStore::ReadAllMetadata(
     const ReadMetadataCallback& callback) {
-  // TODO(stanisc): implement this.
-  NOTIMPLEMENTED();
+  DCHECK(CalledOnValidThread());
+  Result result_code = STORE_INITIALIZATION_FAILED;
+  scoped_ptr<AttachmentMetadataList> metadata_list(
+      new AttachmentMetadataList());
+
+  if (db_) {
+    result_code = SUCCESS;
+    scoped_ptr<leveldb::Iterator> db_iterator(
+        db_->NewIterator(MakeNonCachingReadOptions()));
+    DCHECK(db_iterator);
+    for (db_iterator->Seek(kMetadataPrefix); db_iterator->Valid();
+         db_iterator->Next()) {
+      leveldb::Slice key = db_iterator->key();
+      if (!key.starts_with(kMetadataPrefix)) {
+        break;
+      }
+      // Make AttachmentId from levelDB key.
+      key.remove_prefix(strlen(kMetadataPrefix));
+      sync_pb::AttachmentIdProto id_proto;
+      id_proto.set_unique_id(key.ToString());
+      AttachmentId id = AttachmentId::CreateFromProto(id_proto);
+      // Parse metadata record.
+      attachment_store_pb::RecordMetadata record_metadata;
+      if (!record_metadata.ParseFromString(db_iterator->value().ToString())) {
+        DVLOG(1) << "RecordMetadata::ParseFromString failed";
+        result_code = UNSPECIFIED_ERROR;
+        continue;
+      }
+      metadata_list->push_back(MakeAttachmentMetadata(id, record_metadata));
+    }
+
+    if (!db_iterator->status().ok()) {
+      DVLOG(1) << "DB Iterator failed: status="
+               << db_iterator->status().ToString();
+      result_code = UNSPECIFIED_ERROR;
+    }
+  }
+
+  callback_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(callback, result_code, base::Passed(&metadata_list)));
 }
 
 AttachmentStore::Result OnDiskAttachmentStore::OpenOrCreate(
@@ -239,24 +295,14 @@ AttachmentStore::Result OnDiskAttachmentStore::OpenOrCreate(
 scoped_ptr<Attachment> OnDiskAttachmentStore::ReadSingleAttachment(
     const AttachmentId& attachment_id) {
   scoped_ptr<Attachment> attachment;
-
-  const std::string key = MakeDataKeyFromAttachmentId(attachment_id);
-  const std::string metadata_key =
-      MakeMetadataKeyFromAttachmentId(attachment_id);
-  leveldb::Status status;
-  std::string metadata_str;
-  status = db_->Get(MakeMetadataReadOptions(), metadata_key, &metadata_str);
-  if (!status.ok()) {
-    DVLOG(1) << "DB::Get for metadata failed: status=" << status.ToString();
-    return attachment.Pass();
-  }
   attachment_store_pb::RecordMetadata record_metadata;
-  if (!record_metadata.ParseFromString(metadata_str)) {
-    DVLOG(1) << "RecordMetadata::ParseFromString failed";
+  if (!ReadSingleRecordMetadata(attachment_id, &record_metadata)) {
     return attachment.Pass();
   }
+  const std::string key = MakeDataKeyFromAttachmentId(attachment_id);
   std::string data_str;
-  status = db_->Get(MakeDataReadOptions(), key, &data_str);
+  leveldb::Status status = db_->Get(
+      MakeNonCachingReadOptions(), key, &data_str);
   if (!status.ok()) {
     DVLOG(1) << "DB::Get for data failed: status=" << status.ToString();
     return attachment.Pass();
@@ -281,7 +327,7 @@ bool OnDiskAttachmentStore::WriteSingleAttachment(
 
   std::string metadata_str;
   leveldb::Status status =
-      db_->Get(MakeMetadataReadOptions(), metadata_key, &metadata_str);
+      db_->Get(MakeCachingReadOptions(), metadata_key, &metadata_str);
   if (status.ok()) {
     // Entry exists, don't overwrite.
     return true;
@@ -313,6 +359,26 @@ bool OnDiskAttachmentStore::WriteSingleAttachment(
   return true;
 }
 
+bool OnDiskAttachmentStore::ReadSingleRecordMetadata(
+    const AttachmentId& attachment_id,
+    attachment_store_pb::RecordMetadata* record_metadata) {
+  DCHECK(record_metadata);
+  const std::string metadata_key =
+      MakeMetadataKeyFromAttachmentId(attachment_id);
+  std::string metadata_str;
+  leveldb::Status status =
+      db_->Get(MakeCachingReadOptions(), metadata_key, &metadata_str);
+  if (!status.ok()) {
+    DVLOG(1) << "DB::Get for metadata failed: status=" << status.ToString();
+    return false;
+  }
+  if (!record_metadata->ParseFromString(metadata_str)) {
+    DVLOG(1) << "RecordMetadata::ParseFromString failed";
+    return false;
+  }
+  return true;
+}
+
 std::string OnDiskAttachmentStore::MakeDataKeyFromAttachmentId(
     const AttachmentId& attachment_id) {
   std::string key = kDataPrefix + attachment_id.GetProto().unique_id();
@@ -325,4 +391,10 @@ std::string OnDiskAttachmentStore::MakeMetadataKeyFromAttachmentId(
   return key;
 }
 
+AttachmentMetadata OnDiskAttachmentStore::MakeAttachmentMetadata(
+    const AttachmentId& attachment_id,
+    const attachment_store_pb::RecordMetadata& record_metadata) {
+  return AttachmentMetadata(attachment_id, record_metadata.attachment_size());
+}
+
 }  // namespace syncer
diff --git a/sync/internal_api/attachments/on_disk_attachment_store_unittest.cc b/sync/internal_api/attachments/on_disk_attachment_store_unittest.cc
index 620ff74..e46f060 100644
--- a/sync/internal_api/attachments/on_disk_attachment_store_unittest.cc
+++ b/sync/internal_api/attachments/on_disk_attachment_store_unittest.cc
@@ -62,11 +62,18 @@ INSTANTIATE_TYPED_TEST_CASE_P(OnDisk,
 class OnDiskAttachmentStoreSpecificTest : public testing::Test {
  public:
   base::ScopedTempDir temp_dir_;
+  base::FilePath db_path_;
   base::MessageLoop message_loop_;
   scoped_refptr<AttachmentStore> store_;
 
   OnDiskAttachmentStoreSpecificTest() {}
 
+  void SetUp() override {
+    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+    db_path_ = temp_dir_.path().Append(FILE_PATH_LITERAL("leveldb"));
+    base::CreateDirectory(db_path_);
+  }
+
   void CopyResult(AttachmentStore::Result* destination_result,
                   const AttachmentStore::Result& source_result) {
     *destination_result = source_result;
@@ -82,25 +89,44 @@ class OnDiskAttachmentStoreSpecificTest : public testing::Test {
     *destination_failed_attachment_ids = *source_failed_attachment_ids;
   }
 
-  scoped_ptr<leveldb::DB> OpenLevelDB(const base::FilePath& path) {
+  void CopyResultMetadata(
+      AttachmentStore::Result* destination_result,
+      scoped_ptr<AttachmentMetadataList>* destination_metadata,
+      const AttachmentStore::Result& source_result,
+      scoped_ptr<AttachmentMetadataList> source_metadata) {
+    CopyResult(destination_result, source_result);
+    *destination_metadata = source_metadata.Pass();
+  }
+
+  scoped_ptr<leveldb::DB> OpenLevelDB() {
     leveldb::DB* db;
     leveldb::Options options;
     options.create_if_missing = true;
-    leveldb::Status s = leveldb::DB::Open(options, path.AsUTF8Unsafe(), &db);
+    leveldb::Status s =
+        leveldb::DB::Open(options, db_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);
+  void UpdateRecord(const std::string& key, const std::string& content) {
+    scoped_ptr<leveldb::DB> db = OpenLevelDB();
+    leveldb::Status s = db->Put(leveldb::WriteOptions(), key, content);
     EXPECT_TRUE(s.ok());
   }
 
-  std::string ReadStoreMetadataRecord(const base::FilePath& path) {
-    scoped_ptr<leveldb::DB> db = OpenLevelDB(path);
+  void UpdateStoreMetadataRecord(const std::string& content) {
+    UpdateRecord("database-metadata", content);
+  }
+
+  void UpdateAttachmentMetadataRecord(const AttachmentId& attachment_id,
+                                      const std::string& content) {
+    std::string metadata_key =
+        OnDiskAttachmentStore::MakeMetadataKeyFromAttachmentId(attachment_id);
+    UpdateRecord(metadata_key, content);
+  }
+
+  std::string ReadStoreMetadataRecord() {
+    scoped_ptr<leveldb::DB> db = OpenLevelDB();
     std::string content;
     leveldb::Status s =
         db->Get(leveldb::ReadOptions(), "database-metadata", &content);
@@ -108,10 +134,9 @@ class OnDiskAttachmentStoreSpecificTest : public testing::Test {
     return content;
   }
 
-  void VerifyAttachmentRecordsPresent(const base::FilePath& path,
-                                      const AttachmentId& attachment_id,
+  void VerifyAttachmentRecordsPresent(const AttachmentId& attachment_id,
                                       bool expect_records_present) {
-    scoped_ptr<leveldb::DB> db = OpenLevelDB(path);
+    scoped_ptr<leveldb::DB> db = OpenLevelDB();
 
     std::string metadata_key =
         OnDiskAttachmentStore::MakeMetadataKeyFromAttachmentId(attachment_id);
@@ -139,7 +164,6 @@ class OnDiskAttachmentStoreSpecificTest : public testing::Test {
 // Ensure that store can be closed and reopen while retaining stored
 // attachments.
 TEST_F(OnDiskAttachmentStoreSpecificTest, CloseAndReopen) {
-  ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
   AttachmentStore::Result result;
 
   result = AttachmentStore::UNSPECIFIED_ERROR;
@@ -186,16 +210,10 @@ TEST_F(OnDiskAttachmentStoreSpecificTest, CloseAndReopen) {
 
 // Ensure loading corrupt attachment store fails.
 TEST_F(OnDiskAttachmentStoreSpecificTest, FailToOpen) {
-  ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
-  base::FilePath db_path =
-      temp_dir_.path().Append(FILE_PATH_LITERAL("leveldb"));
-  base::CreateDirectory(db_path);
-
   // To simulate corrupt database write empty CURRENT file.
   std::string current_file_content = "";
-  base::WriteFile(db_path.Append(FILE_PATH_LITERAL("CURRENT")),
-                  current_file_content.c_str(),
-                  current_file_content.size());
+  base::WriteFile(db_path_.Append(FILE_PATH_LITERAL("CURRENT")),
+                  current_file_content.c_str(), current_file_content.size());
 
   AttachmentStore::Result result = AttachmentStore::SUCCESS;
   store_ = AttachmentStore::CreateOnDiskStore(
@@ -208,13 +226,8 @@ TEST_F(OnDiskAttachmentStoreSpecificTest, FailToOpen) {
 // 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);
+  OpenLevelDB();
   // Open database with AttachmentStore.
   AttachmentStore::Result result = AttachmentStore::UNSPECIFIED_ERROR;
   store_ = AttachmentStore::CreateOnDiskStore(
@@ -227,7 +240,7 @@ TEST_F(OnDiskAttachmentStoreSpecificTest, StoreMetadata) {
   RunLoop();
 
   // AttachmentStore should create metadata record.
-  std::string data = ReadStoreMetadataRecord(db_path);
+  std::string data = ReadStoreMetadataRecord();
   attachment_store_pb::StoreMetadata metadata;
   EXPECT_TRUE(metadata.ParseFromString(data));
   EXPECT_EQ(1, metadata.schema_version());
@@ -235,7 +248,7 @@ TEST_F(OnDiskAttachmentStoreSpecificTest, StoreMetadata) {
   // Set unknown future schema version.
   metadata.set_schema_version(2);
   data = metadata.SerializeAsString();
-  UpdateStoreMetadataRecord(db_path, data);
+  UpdateStoreMetadataRecord(data);
 
   // AttachmentStore should fail to load.
   result = AttachmentStore::SUCCESS;
@@ -246,7 +259,7 @@ TEST_F(OnDiskAttachmentStoreSpecificTest, StoreMetadata) {
   EXPECT_EQ(AttachmentStore::UNSPECIFIED_ERROR, result);
 
   // Write garbage into metadata record.
-  UpdateStoreMetadataRecord(db_path, "abra.cadabra");
+  UpdateStoreMetadataRecord("abra.cadabra");
 
   // AttachmentStore should fail to load.
   result = AttachmentStore::SUCCESS;
@@ -260,10 +273,6 @@ TEST_F(OnDiskAttachmentStoreSpecificTest, StoreMetadata) {
 // Ensure that attachment store correctly maintains metadata records for
 // attachments.
 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 create_result = AttachmentStore::UNSPECIFIED_ERROR;
   store_ = AttachmentStore::CreateOnDiskStore(
@@ -298,14 +307,12 @@ TEST_F(OnDiskAttachmentStoreSpecificTest, RecordMetadata) {
   EXPECT_EQ(AttachmentStore::SUCCESS, drop_result);
 
   // Verify that attachment store contains only records for second attachment.
-  VerifyAttachmentRecordsPresent(db_path, attachments[0].GetId(), false);
-  VerifyAttachmentRecordsPresent(db_path, attachments[1].GetId(), true);
+  VerifyAttachmentRecordsPresent(attachments[0].GetId(), false);
+  VerifyAttachmentRecordsPresent(attachments[1].GetId(), true);
 }
 
 // Ensure that attachment store fails to load attachment with mismatched crc.
 TEST_F(OnDiskAttachmentStoreSpecificTest, MismatchedCrc) {
-  ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
-
   // Create attachment store.
   AttachmentStore::Result create_result = AttachmentStore::UNSPECIFIED_ERROR;
   store_ = AttachmentStore::CreateOnDiskStore(
@@ -344,14 +351,9 @@ TEST_F(OnDiskAttachmentStoreSpecificTest, MismatchedCrc) {
 // Ensure that after store initialization failure ReadWrite/Drop operations fail
 // with correct error.
 TEST_F(OnDiskAttachmentStoreSpecificTest, OpsAfterInitializationFailed) {
-  ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
-  base::FilePath db_path =
-      temp_dir_.path().Append(FILE_PATH_LITERAL("leveldb"));
-  base::CreateDirectory(db_path);
-
   // To simulate corrupt database write empty CURRENT file.
   std::string current_file_content = "";
-  base::WriteFile(db_path.Append(FILE_PATH_LITERAL("CURRENT")),
+  base::WriteFile(db_path_.Append(FILE_PATH_LITERAL("CURRENT")),
                   current_file_content.c_str(), current_file_content.size());
 
   AttachmentStore::Result create_result = AttachmentStore::SUCCESS;
@@ -397,4 +399,79 @@ TEST_F(OnDiskAttachmentStoreSpecificTest, OpsAfterInitializationFailed) {
   EXPECT_EQ(AttachmentStore::STORE_INITIALIZATION_FAILED, write_result);
 }
 
+// Ensure that attachment store handles the case of having an unexpected
+// record at the end without crashing.
+TEST_F(OnDiskAttachmentStoreSpecificTest, ReadAllMetadataWithUnexpectedRecord) {
+  // Write a bogus entry at the end of the database.
+  UpdateRecord("zzz", "foobar");
+
+  // Create attachment store.
+  AttachmentStore::Result create_result = AttachmentStore::UNSPECIFIED_ERROR;
+  store_ = AttachmentStore::CreateOnDiskStore(
+      temp_dir_.path(), base::ThreadTaskRunnerHandle::Get(),
+      base::Bind(&AttachmentStoreCreated, &create_result));
+
+  // Read all metadata. Should be getting no error and zero entries.
+  AttachmentStore::Result metadata_result = AttachmentStore::UNSPECIFIED_ERROR;
+  scoped_ptr<AttachmentMetadataList> metadata_list;
+  store_->ReadAllMetadata(
+      base::Bind(&OnDiskAttachmentStoreSpecificTest::CopyResultMetadata,
+                 base::Unretained(this), &metadata_result, &metadata_list));
+  RunLoop();
+  EXPECT_EQ(AttachmentStore::SUCCESS, create_result);
+  EXPECT_EQ(AttachmentStore::SUCCESS, metadata_result);
+  EXPECT_EQ(0U, metadata_list->size());
+  metadata_list.reset();
+
+  // Write 3 attachments to the store
+  AttachmentList attachments;
+
+  for (int i = 0; i < 3; i++) {
+    std::string some_data = "data";
+    Attachment attachment =
+        Attachment::Create(base::RefCountedString::TakeString(&some_data));
+    attachments.push_back(attachment);
+  }
+  AttachmentStore::Result write_result = AttachmentStore::UNSPECIFIED_ERROR;
+  store_->Write(attachments,
+                base::Bind(&OnDiskAttachmentStoreSpecificTest::CopyResult,
+                           base::Unretained(this), &write_result));
+
+  // Read all metadata back. We should be getting 3 entries.
+  store_->ReadAllMetadata(
+      base::Bind(&OnDiskAttachmentStoreSpecificTest::CopyResultMetadata,
+                 base::Unretained(this), &metadata_result, &metadata_list));
+  RunLoop();
+  EXPECT_EQ(AttachmentStore::SUCCESS, write_result);
+  EXPECT_EQ(AttachmentStore::SUCCESS, metadata_result);
+  EXPECT_EQ(3U, metadata_list->size());
+  // Get Id of the attachment in the middle.
+  AttachmentId id = (*metadata_list.get())[1].GetId();
+  metadata_list.reset();
+
+  // Close the store.
+  store_ = nullptr;
+  RunLoop();
+
+  // Modify the middle attachment metadata entry so that it isn't valid anymore.
+  UpdateAttachmentMetadataRecord(id, "foobar");
+
+  // Reopen the store.
+  create_result = AttachmentStore::UNSPECIFIED_ERROR;
+  metadata_result = AttachmentStore::UNSPECIFIED_ERROR;
+  store_ = AttachmentStore::CreateOnDiskStore(
+      temp_dir_.path(), base::ThreadTaskRunnerHandle::Get(),
+      base::Bind(&AttachmentStoreCreated, &create_result));
+
+  // Read all metadata back. We should be getting a failure and
+  // only 2 entries now.
+  store_->ReadAllMetadata(
+      base::Bind(&OnDiskAttachmentStoreSpecificTest::CopyResultMetadata,
+                 base::Unretained(this), &metadata_result, &metadata_list));
+  RunLoop();
+  EXPECT_EQ(AttachmentStore::SUCCESS, create_result);
+  EXPECT_EQ(AttachmentStore::UNSPECIFIED_ERROR, metadata_result);
+  EXPECT_EQ(2U, metadata_list->size());
+}
+
 }  // namespace syncer
diff --git a/sync/internal_api/public/attachments/on_disk_attachment_store.h b/sync/internal_api/public/attachments/on_disk_attachment_store.h
index ed6eefb..cb809a0 100644
--- a/sync/internal_api/public/attachments/on_disk_attachment_store.h
+++ b/sync/internal_api/public/attachments/on_disk_attachment_store.h
@@ -13,6 +13,10 @@
 #include "sync/api/attachments/attachment_store.h"
 #include "sync/base/sync_export.h"
 
+namespace attachment_store_pb {
+class RecordMetadata;
+}  // namespace attachment_store_pb
+
 namespace base {
 class SequencedTaskRunner;
 }  // namespace base
@@ -56,11 +60,19 @@ class SYNC_EXPORT OnDiskAttachmentStore : public AttachmentStoreBase,
       const AttachmentId& attachment_id);
   // Writes single attachment to store. Returns false in case of errors.
   bool WriteSingleAttachment(const Attachment& attachment);
+  // Reads single store_pb::RecordMetadata from levelDB into the provided
+  // buffer. Returns false in case of an error.
+  bool ReadSingleRecordMetadata(
+      const AttachmentId& attachment_id,
+      attachment_store_pb::RecordMetadata* record_metadata);
 
   static std::string MakeDataKeyFromAttachmentId(
       const AttachmentId& attachment_id);
   static std::string MakeMetadataKeyFromAttachmentId(
       const AttachmentId& attachment_id);
+  static AttachmentMetadata MakeAttachmentMetadata(
+      const AttachmentId& attachment_id,
+      const attachment_store_pb::RecordMetadata& record_metadata);
 
   scoped_refptr<base::SequencedTaskRunner> callback_task_runner_;
   const base::FilePath path_;