summaryrefslogtreecommitdiffstats
path: root/sync
diff options
context:
space:
mode:
authormaniscalco@chromium.org <maniscalco@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>2014-05-02 19:00:11 +0000
committermaniscalco@chromium.org <maniscalco@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>2014-05-02 19:00:11 +0000
commit706d41dd9014de4a25e20069d8b607f291874fc1 (patch)
tree324309d4b9f7194d25a77036d7a2b1fa60eac6f4 /sync
parent92c7f876ef1041d01dc37750420513e19391874c (diff)
downloadchromium_src-706d41dd9014de4a25e20069d8b607f291874fc1.zip
chromium_src-706d41dd9014de4a25e20069d8b607f291874fc1.tar.gz
chromium_src-706d41dd9014de4a25e20069d8b607f291874fc1.tar.bz2
Keep track of which attachments are referenced by which sync entries.
Relanding https://codereview.chromium.org/247983002/ after fixing memory leak. PutAttachmentMetadata on MutableEntry now notifies the Directory when the attachments associated with an entry change. Give Directory::InitializeIndices a ScopedKernelLock to be consistent and make it easier to ensure we are locking when we need to. GenericChangeProcess passes new attachments to AttachmentService for storage and upload. Add an output parameter to GenericChangeProcessor's HandleActionAdd and HandleActionUpdate methods so they can keep track of potentially new attachments and pass them to AttachmentService for storage/upload. Convert AttachmentService's OnSyncDataAdd to StoreAttachments. Implement HasAttachmentNotOnServer. BUG=348625,353303,354530,356266 Review URL: https://codereview.chromium.org/264793007 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@267887 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'sync')
-rw-r--r--sync/api/attachments/attachment_id.cc7
-rw-r--r--sync/api/attachments/attachment_service.h23
-rw-r--r--sync/api/attachments/attachment_service_proxy.cc25
-rw-r--r--sync/api/attachments/attachment_service_proxy.h6
-rw-r--r--sync/api/attachments/attachment_service_proxy_unittest.cc24
-rw-r--r--sync/api/attachments/fake_attachment_service.cc23
-rw-r--r--sync/api/attachments/fake_attachment_service.h5
-rw-r--r--sync/api/sync_data.cc22
-rw-r--r--sync/api/sync_data.h15
-rw-r--r--sync/engine/get_commit_ids.cc10
-rw-r--r--sync/internal_api/base_node.cc11
-rw-r--r--sync/internal_api/public/base/attachment_id_proto.cc19
-rw-r--r--sync/internal_api/public/base/attachment_id_proto.h20
-rw-r--r--sync/internal_api/public/base_transaction.h3
-rw-r--r--sync/internal_api/public/write_node.h4
-rw-r--r--sync/internal_api/sync_manager_impl_unittest.cc239
-rw-r--r--sync/internal_api/write_node.cc5
-rw-r--r--sync/protocol/attachments.proto9
-rw-r--r--sync/sync_internal_api.gypi2
-rw-r--r--sync/syncable/directory.cc70
-rw-r--r--sync/syncable/directory.h44
-rw-r--r--sync/syncable/directory_backing_store.cc9
-rw-r--r--sync/syncable/directory_unittest.cc135
-rw-r--r--sync/syncable/directory_unittest.h19
-rw-r--r--sync/syncable/entry_kernel.h4
-rw-r--r--sync/syncable/mutable_entry.cc14
-rw-r--r--sync/syncable/mutable_entry.h3
27 files changed, 640 insertions, 130 deletions
diff --git a/sync/api/attachments/attachment_id.cc b/sync/api/attachments/attachment_id.cc
index 6f3e0a2..e61705e 100644
--- a/sync/api/attachments/attachment_id.cc
+++ b/sync/api/attachments/attachment_id.cc
@@ -5,7 +5,7 @@
#include "sync/api/attachments/attachment_id.h"
#include "base/logging.h"
-#include "base/rand_util.h"
+#include "sync/internal_api/public/base/attachment_id_proto.h"
#include "sync/protocol/sync.pb.h"
namespace syncer {
@@ -53,10 +53,7 @@ bool AttachmentId::operator<(const AttachmentId& other) const {
// Static.
AttachmentId AttachmentId::Create() {
- // Only requirement here is that this id must be globally unique.
- // TODO(maniscalco): Consider making this base64 encoded.
- sync_pb::AttachmentIdProto proto;
- proto.set_unique_id(base::RandBytesAsString(16));
+ sync_pb::AttachmentIdProto proto = CreateAttachmentIdProto();
return AttachmentId(&proto);
}
diff --git a/sync/api/attachments/attachment_service.h b/sync/api/attachments/attachment_service.h
index 81a1a63..9e5e25d 100644
--- a/sync/api/attachments/attachment_service.h
+++ b/sync/api/attachments/attachment_service.h
@@ -38,11 +38,22 @@ class SYNC_EXPORT AttachmentService {
// The result of a DropAttachments operation.
enum DropResult {
DROP_SUCCESS, // No error, all attachments dropped.
- DROP_UNSPECIFIED_ERROR, // An unspecified error occurred.
+ DROP_UNSPECIFIED_ERROR, // An unspecified error occurred. Some or all
+ // attachments may not have been dropped.
};
typedef base::Callback<void(const DropResult&)> DropCallback;
+ // The result of a StoreAttachments operation.
+ enum StoreResult {
+ STORE_SUCCESS, // No error, all attachments stored (at least
+ // locally).
+ STORE_UNSPECIFIED_ERROR, // An unspecified error occurred. Some or all
+ // attachments may not have been stored.
+ };
+
+ typedef base::Callback<void(const StoreResult&)> StoreCallback;
+
AttachmentService();
virtual ~AttachmentService();
@@ -55,10 +66,12 @@ class SYNC_EXPORT AttachmentService {
virtual void DropAttachments(const AttachmentIdList& attachment_ids,
const DropCallback& callback) = 0;
- // This method should be called when a SyncData is about to be added to the
- // sync database so we have a chance to persist the Attachment locally and
- // schedule it for upload to the sync server.
- virtual void OnSyncDataAdd(const SyncData& sync_data) = 0;
+ // Store |attachments| on device and (eventually) upload them to the server.
+ //
+ // Invokes |callback| once the attachments have been written to device
+ // storage.
+ virtual void StoreAttachments(const AttachmentList& attachments,
+ const StoreCallback& callback) = 0;
// This method should be called when a SyncData is about to be deleted from
// the sync database so we can remove any unreferenced attachments from local
diff --git a/sync/api/attachments/attachment_service_proxy.cc b/sync/api/attachments/attachment_service_proxy.cc
index 9bc40e5..6590033 100644
--- a/sync/api/attachments/attachment_service_proxy.cc
+++ b/sync/api/attachments/attachment_service_proxy.cc
@@ -35,6 +35,15 @@ void ProxyDropCallback(
task_runner->PostTask(FROM_HERE, base::Bind(callback, result));
}
+// Invokes |callback| with |result| and |attachments| in the |task_runner|
+// thread.
+void ProxyStoreCallback(
+ const scoped_refptr<base::SequencedTaskRunner>& task_runner,
+ const AttachmentService::StoreCallback& callback,
+ const AttachmentService::StoreResult& result) {
+ task_runner->PostTask(FROM_HERE, base::Bind(callback, result));
+}
+
} // namespace
AttachmentServiceProxy::AttachmentServiceProxy() {
@@ -85,11 +94,17 @@ void AttachmentServiceProxy::DropAttachments(
proxy_callback));
}
-void AttachmentServiceProxy::OnSyncDataAdd(const SyncData& sync_data) {
+void AttachmentServiceProxy::StoreAttachments(const AttachmentList& attachments,
+ const StoreCallback& callback) {
DCHECK(wrapped_task_runner_);
+ StoreCallback proxy_callback = base::Bind(
+ &ProxyStoreCallback, base::MessageLoopProxy::current(), callback);
wrapped_task_runner_->PostTask(
FROM_HERE,
- base::Bind(&AttachmentService::OnSyncDataAdd, core_, sync_data));
+ base::Bind(&AttachmentService::StoreAttachments,
+ core_,
+ attachments,
+ proxy_callback));
}
void AttachmentServiceProxy::OnSyncDataDelete(const SyncData& sync_data) {
@@ -137,11 +152,13 @@ void AttachmentServiceProxy::Core::DropAttachments(
wrapped_->DropAttachments(attachment_ids, callback);
}
-void AttachmentServiceProxy::Core::OnSyncDataAdd(const SyncData& sync_data) {
+void AttachmentServiceProxy::Core::StoreAttachments(
+ const AttachmentList& attachments,
+ const StoreCallback& callback) {
if (!wrapped_) {
return;
}
- wrapped_->OnSyncDataAdd(sync_data);
+ wrapped_->StoreAttachments(attachments, callback);
}
void AttachmentServiceProxy::Core::OnSyncDataDelete(const SyncData& sync_data) {
diff --git a/sync/api/attachments/attachment_service_proxy.h b/sync/api/attachments/attachment_service_proxy.h
index 7085625..462cdda 100644
--- a/sync/api/attachments/attachment_service_proxy.h
+++ b/sync/api/attachments/attachment_service_proxy.h
@@ -58,7 +58,8 @@ class SYNC_EXPORT AttachmentServiceProxy : public AttachmentService {
const GetOrDownloadCallback& callback) OVERRIDE;
virtual void DropAttachments(const AttachmentIdList& attachment_ids,
const DropCallback& callback) OVERRIDE;
- virtual void OnSyncDataAdd(const SyncData& sync_data) OVERRIDE;
+ virtual void StoreAttachments(const AttachmentList& attachment,
+ const StoreCallback& callback) OVERRIDE;
virtual void OnSyncDataDelete(const SyncData& sync_data) OVERRIDE;
virtual void OnSyncDataUpdate(const AttachmentIdList& old_attachment_ids,
const SyncData& updated_sync_data) OVERRIDE;
@@ -89,7 +90,8 @@ class SYNC_EXPORT AttachmentServiceProxy : public AttachmentService {
const GetOrDownloadCallback& callback) OVERRIDE;
virtual void DropAttachments(const AttachmentIdList& attachment_ids,
const DropCallback& callback) OVERRIDE;
- virtual void OnSyncDataAdd(const SyncData& sync_data) OVERRIDE;
+ virtual void StoreAttachments(const AttachmentList& attachment,
+ const StoreCallback& callback) OVERRIDE;
virtual void OnSyncDataDelete(const SyncData& sync_data) OVERRIDE;
virtual void OnSyncDataUpdate(const AttachmentIdList& old_attachment_ids,
const SyncData& updated_sync_data) OVERRIDE;
diff --git a/sync/api/attachments/attachment_service_proxy_unittest.cc b/sync/api/attachments/attachment_service_proxy_unittest.cc
index f6e3502..08c499e 100644
--- a/sync/api/attachments/attachment_service_proxy_unittest.cc
+++ b/sync/api/attachments/attachment_service_proxy_unittest.cc
@@ -56,9 +56,12 @@ class StubAttachmentService : public AttachmentService,
FROM_HERE, base::Bind(callback, AttachmentService::DROP_SUCCESS));
}
- virtual void OnSyncDataAdd(const SyncData& sync_data) OVERRIDE {
+ virtual void StoreAttachments(const AttachmentList& attachments,
+ const StoreCallback& callback) OVERRIDE {
CalledOnValidThread();
Increment();
+ base::MessageLoop::current()->PostTask(
+ FROM_HERE, base::Bind(callback, AttachmentService::STORE_SUCCESS));
}
virtual void OnSyncDataDelete(const SyncData& sync_data) OVERRIDE {
@@ -119,8 +122,11 @@ class AttachmentServiceProxyTest : public testing::Test,
base::Unretained(this));
callback_drop = base::Bind(&AttachmentServiceProxyTest::IncrementDrop,
base::Unretained(this));
+ callback_store = base::Bind(&AttachmentServiceProxyTest::IncrementStore,
+ base::Unretained(this));
count_callback_get_or_download = 0;
count_callback_drop = 0;
+ count_callback_store = 0;
}
virtual void TearDown()
@@ -147,6 +153,12 @@ class AttachmentServiceProxyTest : public testing::Test,
++count_callback_drop;
}
+ // a StoreCallback
+ void IncrementStore(const AttachmentService::StoreResult&) {
+ CalledOnValidThread();
+ ++count_callback_store;
+ }
+
void WaitForStubThread() {
base::WaitableEvent done(false, false);
stub_thread->message_loop()->PostTask(
@@ -165,21 +177,23 @@ class AttachmentServiceProxyTest : public testing::Test,
AttachmentService::GetOrDownloadCallback callback_get_or_download;
AttachmentService::DropCallback callback_drop;
+ AttachmentService::StoreCallback callback_store;
// number of times callback_get_or_download was invoked
int count_callback_get_or_download;
// number of times callback_drop was invoked
int count_callback_drop;
+ // number of times callback_store was invoked
+ int count_callback_store;
};
// Verify that each of AttachmentServiceProxy's regular methods (those that
// don't take callbacks) are invoked on the stub.
TEST_F(AttachmentServiceProxyTest, MethodsAreProxied) {
- proxy->OnSyncDataAdd(sync_data);
proxy->OnSyncDataDelete(sync_data_delete);
proxy->OnSyncDataUpdate(AttachmentIdList(), sync_data);
WaitForStubThread();
- EXPECT_EQ(3, stub->GetCallCount());
+ EXPECT_EQ(2, stub->GetCallCount());
}
// Verify that each of AttachmentServiceProxy's callback methods (those that
@@ -188,9 +202,10 @@ TEST_F(AttachmentServiceProxyTest, MethodsAreProxied) {
TEST_F(AttachmentServiceProxyTest, MethodsWithCallbacksAreProxied) {
proxy->GetOrDownloadAttachments(AttachmentIdList(), callback_get_or_download);
proxy->DropAttachments(AttachmentIdList(), callback_drop);
+ proxy->StoreAttachments(AttachmentList(), callback_store);
// Wait for the posted calls to execute in the stub thread.
WaitForStubThread();
- EXPECT_EQ(2, stub->GetCallCount());
+ EXPECT_EQ(3, stub->GetCallCount());
// At this point the stub thread has finished executed the calls. However, the
// result callbacks it has posted may not have executed yet. Wait a second
// time to ensure the stub thread has executed the posted result callbacks.
@@ -199,6 +214,7 @@ TEST_F(AttachmentServiceProxyTest, MethodsWithCallbacksAreProxied) {
loop.RunUntilIdle();
EXPECT_EQ(1, count_callback_get_or_download);
EXPECT_EQ(1, count_callback_drop);
+ EXPECT_EQ(1, count_callback_store);
}
// Verify that it's safe to use an AttachmentServiceProxy even after its wrapped
diff --git a/sync/api/attachments/fake_attachment_service.cc b/sync/api/attachments/fake_attachment_service.cc
index b9eb88e..5cb01e5 100644
--- a/sync/api/attachments/fake_attachment_service.cc
+++ b/sync/api/attachments/fake_attachment_service.cc
@@ -51,10 +51,15 @@ void FakeAttachmentService::DropAttachments(
callback));
}
-void FakeAttachmentService::OnSyncDataAdd(const SyncData& sync_data) {
+void FakeAttachmentService::StoreAttachments(const AttachmentList& attachments,
+ const StoreCallback& callback) {
DCHECK(CalledOnValidThread());
- // TODO(maniscalco): Ensure the linked attachments get persisted in local
- // storage and schedule them for upload to the server (bug 356351).
+ attachment_store_->Write(attachments,
+ base::Bind(&FakeAttachmentService::WriteDone,
+ weak_ptr_factory_.GetWeakPtr(),
+ callback));
+ // TODO(maniscalco): Ensure the linked attachments are schedule for upload to
+ // the server (bug 356351).
}
void FakeAttachmentService::OnSyncDataDelete(const SyncData& sync_data) {
@@ -99,4 +104,16 @@ void FakeAttachmentService::DropDone(const DropCallback& callback,
base::Bind(callback, drop_result));
}
+void FakeAttachmentService::WriteDone(const StoreCallback& callback,
+ const AttachmentStore::Result& result) {
+ AttachmentService::StoreResult store_result =
+ AttachmentService::STORE_UNSPECIFIED_ERROR;
+ if (result == AttachmentStore::SUCCESS) {
+ store_result = AttachmentService::STORE_SUCCESS;
+ }
+ // TODO(maniscalco): Deal with case where an error occurred (bug 361251).
+ base::MessageLoop::current()->PostTask(FROM_HERE,
+ base::Bind(callback, store_result));
+}
+
} // namespace syncer
diff --git a/sync/api/attachments/fake_attachment_service.h b/sync/api/attachments/fake_attachment_service.h
index eb97c52..5bc443a 100644
--- a/sync/api/attachments/fake_attachment_service.h
+++ b/sync/api/attachments/fake_attachment_service.h
@@ -29,7 +29,8 @@ class SYNC_EXPORT FakeAttachmentService : public AttachmentService,
OVERRIDE;
virtual void DropAttachments(const AttachmentIdList& attachment_ids,
const DropCallback& callback) OVERRIDE;
- virtual void OnSyncDataAdd(const SyncData& sync_data) OVERRIDE;
+ virtual void StoreAttachments(const AttachmentList& attachments,
+ const StoreCallback& callback) OVERRIDE;
virtual void OnSyncDataDelete(const SyncData& sync_data) OVERRIDE;
virtual void OnSyncDataUpdate(const AttachmentIdList& old_attachment_ids,
const SyncData& updated_sync_data) OVERRIDE;
@@ -40,6 +41,8 @@ class SYNC_EXPORT FakeAttachmentService : public AttachmentService,
scoped_ptr<AttachmentMap> attachments);
void DropDone(const DropCallback& callback,
const AttachmentStore::Result& result);
+ void WriteDone(const StoreCallback& callback,
+ const AttachmentStore::Result& result);
const scoped_ptr<AttachmentStore> attachment_store_;
// Must be last data member.
diff --git a/sync/api/sync_data.cc b/sync/api/sync_data.cc
index fa48ea5..35a21f9 100644
--- a/sync/api/sync_data.cc
+++ b/sync/api/sync_data.cc
@@ -36,6 +36,21 @@ syncer::AttachmentId ProtoToId(const sync_pb::AttachmentIdProto& proto) {
return syncer::AttachmentId::CreateFromProto(proto);
}
+// Return true iff |attachments| contains one or more elements with the same
+// AttachmentId.
+bool ContainsDuplicateAttachments(const syncer::AttachmentList& attachments) {
+ std::set<syncer::AttachmentId> id_set;
+ AttachmentList::const_iterator iter = attachments.begin();
+ AttachmentList::const_iterator end = attachments.end();
+ for (; iter != end; ++iter) {
+ if (id_set.find(iter->GetId()) != id_set.end()) {
+ return true;
+ }
+ id_set.insert(iter->GetId());
+ }
+ return false;
+}
+
} // namespace
namespace syncer {
@@ -96,15 +111,13 @@ SyncData SyncData::CreateLocalData(const std::string& sync_tag,
sync_tag, non_unique_title, specifics, attachments);
}
-// TODO(maniscalco): What should happen if the same Attachment appears in the
-// list twice? Document the behavior and add a test case.
-//
// Static.
SyncData SyncData::CreateLocalDataWithAttachments(
const std::string& sync_tag,
const std::string& non_unique_title,
const sync_pb::EntitySpecifics& specifics,
const AttachmentList& attachments) {
+ DCHECK(!ContainsDuplicateAttachments(attachments));
sync_pb::SyncEntity entity;
entity.set_client_defined_unique_tag(sync_tag);
entity.set_non_unique_name(non_unique_title);
@@ -113,9 +126,6 @@ SyncData SyncData::CreateLocalDataWithAttachments(
attachments.end(),
RepeatedFieldBackInserter(entity.mutable_attachment_id()),
AttachmentToProto);
- // TODO(maniscalco): Actually pass the attachments to the ctor and make them
- // available to the AttachmentService once this SyncData gets passed into
- // GenericChangeProcesso::ProcessSyncChanges (bug 354530).
AttachmentList copy_of_attachments(attachments);
return SyncData(kInvalidId,
&entity,
diff --git a/sync/api/sync_data.h b/sync/api/sync_data.h
index bb99b74..8795141 100644
--- a/sync/api/sync_data.h
+++ b/sync/api/sync_data.h
@@ -43,14 +43,19 @@ class SYNC_EXPORT SyncData {
// Default copy and assign welcome.
// Helper methods for creating SyncData objects for local data.
- // The sync tag must be a string unique to this datatype and is used as a node
+ //
+ // |sync_tag| Must be a string unique to this datatype and is used as a node
// identifier server-side.
+ //
// For deletes: |datatype| must specify the datatype who node is being
// deleted.
- // For adds/updates: the specifics must be valid and the non-unique title (can
- // be the same as sync tag) must be specfied.
- // Note: the non_unique_title is primarily for debug purposes, and will be
- // overwritten if the datatype is encrypted.
+ //
+ // For adds/updates: |specifics| must be valid and |non_unique_title| (can be
+ // the same as |sync_tag|) must be specfied. Note: |non_unique_title| is
+ // primarily for debug purposes, and will be overwritten if the datatype is
+ // encrypted.
+ //
+ // For data with attachments: |attachments| must not contain duplicates.
static SyncData CreateLocalDelete(
const std::string& sync_tag,
ModelType datatype);
diff --git a/sync/engine/get_commit_ids.cc b/sync/engine/get_commit_ids.cc
index 80222ee..5f91915 100644
--- a/sync/engine/get_commit_ids.cc
+++ b/sync/engine/get_commit_ids.cc
@@ -108,9 +108,13 @@ bool IsEntryInConflict(const syncable::Entry& entry) {
// Return true if this entry has any attachments that haven't yet been uploaded
// to the server.
bool HasAttachmentNotOnServer(const syncable::Entry& entry) {
- // TODO(maniscalco): Once AttachmentMetadata is fleshed out, implement this
- // function to return true if any of the attachments haven't been uploaded to
- // the server. Add test case (bug 356266).
+ // TODO(maniscalco): Add test case (bug 356266).
+ const sync_pb::AttachmentMetadata& metadata = entry.GetAttachmentMetadata();
+ for (int i = 0; i < metadata.record_size(); ++i) {
+ if (!metadata.record(i).is_on_server()) {
+ return true;
+ }
+ }
return false;
}
diff --git a/sync/internal_api/base_node.cc b/sync/internal_api/base_node.cc
index 0788395..57dbb22 100644
--- a/sync/internal_api/base_node.cc
+++ b/sync/internal_api/base_node.cc
@@ -291,10 +291,13 @@ ModelType BaseNode::GetModelType() const {
}
const syncer::AttachmentIdList BaseNode::GetAttachmentIds() const {
- // TODO(maniscalco): Once EntryKernel is capable of storing AttachmentIds,
- // update this method to retrieve the list of AttachmentIds from read_node and
- // pass it to CreateRemoteData (bug 348625).
- return AttachmentIdList();
+ AttachmentIdList result;
+ const sync_pb::AttachmentMetadata& metadata =
+ GetEntry()->GetAttachmentMetadata();
+ for (int i = 0; i < metadata.record_size(); ++i) {
+ result.push_back(AttachmentId::CreateFromProto(metadata.record(i).id()));
+ }
+ return result;
}
void BaseNode::SetUnencryptedSpecifics(
diff --git a/sync/internal_api/public/base/attachment_id_proto.cc b/sync/internal_api/public/base/attachment_id_proto.cc
new file mode 100644
index 0000000..d09c83e
--- /dev/null
+++ b/sync/internal_api/public/base/attachment_id_proto.cc
@@ -0,0 +1,19 @@
+// 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.
+
+#include "sync/internal_api/public/base/attachment_id_proto.h"
+
+#include "base/rand_util.h"
+
+namespace syncer {
+
+sync_pb::AttachmentIdProto CreateAttachmentIdProto() {
+ // Only requirement here is that this id must be globally unique.
+ // TODO(maniscalco): Consider making this base64 encoded.
+ sync_pb::AttachmentIdProto proto;
+ proto.set_unique_id(base::RandBytesAsString(16));
+ return proto;
+}
+
+} // namespace syncer
diff --git a/sync/internal_api/public/base/attachment_id_proto.h b/sync/internal_api/public/base/attachment_id_proto.h
new file mode 100644
index 0000000..3332d81
--- /dev/null
+++ b/sync/internal_api/public/base/attachment_id_proto.h
@@ -0,0 +1,20 @@
+// 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.
+
+#ifndef SYNC_INTERNAL_API_PUBLIC_BASE_ATTACHMENT_ID_PROTO_H_
+#define SYNC_INTERNAL_API_PUBLIC_BASE_ATTACHMENT_ID_PROTO_H_
+
+#include "sync/base/sync_export.h"
+#include "sync/protocol/sync.pb.h"
+
+namespace syncer {
+
+// Helper functions that are logically part of sync_pb::AttachmentIdProto.
+
+// Creates a unique AttachmentIdProto.
+SYNC_EXPORT_PRIVATE sync_pb::AttachmentIdProto CreateAttachmentIdProto();
+
+} // namespace syncer
+
+#endif // SYNC_INTERNAL_API_PUBLIC_BASE_ATTACHMENT_ID_PROTO_H_
diff --git a/sync/internal_api/public/base_transaction.h b/sync/internal_api/public/base_transaction.h
index 2f4fa3c..c75086e 100644
--- a/sync/internal_api/public/base_transaction.h
+++ b/sync/internal_api/public/base_transaction.h
@@ -24,6 +24,9 @@ class Directory;
// syncable, and are used in a similar way. Unlike syncable::BaseTransaction,
// whose construction requires an explicit syncable::Directory, a sync
// API BaseTransaction is created from a UserShare object.
+//
+// Note, these transactions are not atomic. Individual operations can
+// fail. There is no built-in rollback or undo mechanism.
class SYNC_EXPORT BaseTransaction {
public:
// Provide access to the underlying syncable objects from BaseNode.
diff --git a/sync/internal_api/public/write_node.h b/sync/internal_api/public/write_node.h
index e41c966..5b47bd6 100644
--- a/sync/internal_api/public/write_node.h
+++ b/sync/internal_api/public/write_node.h
@@ -173,6 +173,10 @@ class SYNC_EXPORT WriteNode : public BaseNode {
void SetPriorityPreferenceSpecifics(
const sync_pb::PriorityPreferenceSpecifics& specifics);
+ // Set the attachment metadata.
+ void SetAttachmentMetadata(
+ const sync_pb::AttachmentMetadata& attachment_metadata);
+
// Implementation of BaseNode's abstract virtual accessors.
virtual const syncable::Entry* GetEntry() const OVERRIDE;
diff --git a/sync/internal_api/sync_manager_impl_unittest.cc b/sync/internal_api/sync_manager_impl_unittest.cc
index a5c0c81..20b7685 100644
--- a/sync/internal_api/sync_manager_impl_unittest.cc
+++ b/sync/internal_api/sync_manager_impl_unittest.cc
@@ -210,22 +210,94 @@ class SyncApiTest : public testing::Test {
}
protected:
+ // Create an entry with the given |model_type|, |client_tag| and
+ // |attachment_metadata|.
+ void CreateEntryWithAttachmentMetadata(
+ const ModelType& model_type,
+ const std::string& client_tag,
+ const sync_pb::AttachmentMetadata& attachment_metadata);
+
+ // Attempts to load the entry specified by |model_type| and |client_tag| and
+ // returns the lookup result code.
+ BaseNode::InitByLookupResult LookupEntryByClientTag(
+ const ModelType& model_type,
+ const std::string& client_tag);
+
+ // Replace the entry specified by |model_type| and |client_tag| with a
+ // tombstone.
+ void ReplaceWithTombstone(const ModelType& model_type,
+ const std::string& client_tag);
+
+ // Save changes to the Directory, destroy it then reload it.
+ bool ReloadDir();
+
+ UserShare* user_share();
+ syncable::Directory* dir();
+ SyncEncryptionHandler* encryption_handler();
+
+ private:
base::MessageLoop message_loop_;
TestUserShare test_user_share_;
};
+UserShare* SyncApiTest::user_share() {
+ return test_user_share_.user_share();
+}
+
+syncable::Directory* SyncApiTest::dir() {
+ return test_user_share_.user_share()->directory.get();
+}
+
+SyncEncryptionHandler* SyncApiTest::encryption_handler() {
+ return test_user_share_.encryption_handler();
+}
+
+bool SyncApiTest::ReloadDir() {
+ return test_user_share_.Reload();
+}
+
+void SyncApiTest::CreateEntryWithAttachmentMetadata(
+ const ModelType& model_type,
+ const std::string& client_tag,
+ const sync_pb::AttachmentMetadata& attachment_metadata) {
+ syncer::WriteTransaction trans(FROM_HERE, user_share());
+ syncer::ReadNode root_node(&trans);
+ root_node.InitByRootLookup();
+ syncer::WriteNode node(&trans);
+ ASSERT_EQ(node.InitUniqueByCreation(model_type, root_node, client_tag),
+ syncer::WriteNode::INIT_SUCCESS);
+ node.SetAttachmentMetadata(attachment_metadata);
+}
+
+BaseNode::InitByLookupResult SyncApiTest::LookupEntryByClientTag(
+ const ModelType& model_type,
+ const std::string& client_tag) {
+ syncer::ReadTransaction trans(FROM_HERE, user_share());
+ syncer::ReadNode node(&trans);
+ return node.InitByClientTagLookup(model_type, client_tag);
+}
+
+void SyncApiTest::ReplaceWithTombstone(const ModelType& model_type,
+ const std::string& client_tag) {
+ syncer::WriteTransaction trans(FROM_HERE, user_share());
+ syncer::WriteNode node(&trans);
+ ASSERT_EQ(node.InitByClientTagLookup(model_type, client_tag),
+ syncer::WriteNode::INIT_OK);
+ node.Tombstone();
+}
+
TEST_F(SyncApiTest, SanityCheckTest) {
{
- ReadTransaction trans(FROM_HERE, test_user_share_.user_share());
+ ReadTransaction trans(FROM_HERE, user_share());
EXPECT_TRUE(trans.GetWrappedTrans());
}
{
- WriteTransaction trans(FROM_HERE, test_user_share_.user_share());
+ WriteTransaction trans(FROM_HERE, user_share());
EXPECT_TRUE(trans.GetWrappedTrans());
}
{
// No entries but root should exist
- ReadTransaction trans(FROM_HERE, test_user_share_.user_share());
+ ReadTransaction trans(FROM_HERE, user_share());
ReadNode node(&trans);
// Metahandle 1 can be root, sanity check 2
EXPECT_EQ(BaseNode::INIT_FAILED_ENTRY_NOT_GOOD, node.InitByIdLookup(2));
@@ -234,17 +306,16 @@ TEST_F(SyncApiTest, SanityCheckTest) {
TEST_F(SyncApiTest, BasicTagWrite) {
{
- ReadTransaction trans(FROM_HERE, test_user_share_.user_share());
+ ReadTransaction trans(FROM_HERE, user_share());
ReadNode root_node(&trans);
root_node.InitByRootLookup();
EXPECT_EQ(root_node.GetFirstChildId(), 0);
}
- ignore_result(MakeNode(test_user_share_.user_share(),
- BOOKMARKS, "testtag"));
+ ignore_result(MakeNode(user_share(), BOOKMARKS, "testtag"));
{
- ReadTransaction trans(FROM_HERE, test_user_share_.user_share());
+ ReadTransaction trans(FROM_HERE, user_share());
ReadNode node(&trans);
EXPECT_EQ(BaseNode::INIT_OK,
node.InitByClientTagLookup(BOOKMARKS, "testtag"));
@@ -258,21 +329,18 @@ TEST_F(SyncApiTest, BasicTagWrite) {
TEST_F(SyncApiTest, ModelTypesSiloed) {
{
- WriteTransaction trans(FROM_HERE, test_user_share_.user_share());
+ WriteTransaction trans(FROM_HERE, user_share());
ReadNode root_node(&trans);
root_node.InitByRootLookup();
EXPECT_EQ(root_node.GetFirstChildId(), 0);
}
- ignore_result(MakeNode(test_user_share_.user_share(),
- BOOKMARKS, "collideme"));
- ignore_result(MakeNode(test_user_share_.user_share(),
- PREFERENCES, "collideme"));
- ignore_result(MakeNode(test_user_share_.user_share(),
- AUTOFILL, "collideme"));
+ ignore_result(MakeNode(user_share(), BOOKMARKS, "collideme"));
+ ignore_result(MakeNode(user_share(), PREFERENCES, "collideme"));
+ ignore_result(MakeNode(user_share(), AUTOFILL, "collideme"));
{
- ReadTransaction trans(FROM_HERE, test_user_share_.user_share());
+ ReadTransaction trans(FROM_HERE, user_share());
ReadNode bookmarknode(&trans);
EXPECT_EQ(BaseNode::INIT_OK,
@@ -297,14 +365,14 @@ TEST_F(SyncApiTest, ModelTypesSiloed) {
TEST_F(SyncApiTest, ReadMissingTagsFails) {
{
- ReadTransaction trans(FROM_HERE, test_user_share_.user_share());
+ ReadTransaction trans(FROM_HERE, user_share());
ReadNode node(&trans);
EXPECT_EQ(BaseNode::INIT_FAILED_ENTRY_NOT_GOOD,
node.InitByClientTagLookup(BOOKMARKS,
"testtag"));
}
{
- WriteTransaction trans(FROM_HERE, test_user_share_.user_share());
+ WriteTransaction trans(FROM_HERE, user_share());
WriteNode node(&trans);
EXPECT_EQ(BaseNode::INIT_FAILED_ENTRY_NOT_GOOD,
node.InitByClientTagLookup(BOOKMARKS,
@@ -320,7 +388,7 @@ TEST_F(SyncApiTest, TestDeleteBehavior) {
std::string test_title("test1");
{
- WriteTransaction trans(FROM_HERE, test_user_share_.user_share());
+ WriteTransaction trans(FROM_HERE, user_share());
ReadNode root_node(&trans);
root_node.InitByRootLookup();
@@ -341,7 +409,7 @@ TEST_F(SyncApiTest, TestDeleteBehavior) {
// Ensure we can delete something with a tag.
{
- WriteTransaction trans(FROM_HERE, test_user_share_.user_share());
+ WriteTransaction trans(FROM_HERE, user_share());
WriteNode wnode(&trans);
EXPECT_EQ(BaseNode::INIT_OK,
wnode.InitByClientTagLookup(BOOKMARKS,
@@ -355,7 +423,7 @@ TEST_F(SyncApiTest, TestDeleteBehavior) {
// Lookup of a node which was deleted should return failure,
// but have found some data about the node.
{
- ReadTransaction trans(FROM_HERE, test_user_share_.user_share());
+ ReadTransaction trans(FROM_HERE, user_share());
ReadNode node(&trans);
EXPECT_EQ(BaseNode::INIT_FAILED_ENTRY_IS_DEL,
node.InitByClientTagLookup(BOOKMARKS,
@@ -366,7 +434,7 @@ TEST_F(SyncApiTest, TestDeleteBehavior) {
}
{
- WriteTransaction trans(FROM_HERE, test_user_share_.user_share());
+ WriteTransaction trans(FROM_HERE, user_share());
ReadNode folder_node(&trans);
EXPECT_EQ(BaseNode::INIT_OK, folder_node.InitByIdLookup(folder_id));
@@ -384,7 +452,7 @@ TEST_F(SyncApiTest, TestDeleteBehavior) {
// Now look up should work.
{
- ReadTransaction trans(FROM_HERE, test_user_share_.user_share());
+ ReadTransaction trans(FROM_HERE, user_share());
ReadNode node(&trans);
EXPECT_EQ(BaseNode::INIT_OK,
node.InitByClientTagLookup(BOOKMARKS,
@@ -397,11 +465,11 @@ TEST_F(SyncApiTest, TestDeleteBehavior) {
TEST_F(SyncApiTest, WriteAndReadPassword) {
KeyParams params = {"localhost", "username", "passphrase"};
{
- ReadTransaction trans(FROM_HERE, test_user_share_.user_share());
+ ReadTransaction trans(FROM_HERE, user_share());
trans.GetCryptographer()->AddKey(params);
}
{
- WriteTransaction trans(FROM_HERE, test_user_share_.user_share());
+ WriteTransaction trans(FROM_HERE, user_share());
ReadNode root_node(&trans);
root_node.InitByRootLookup();
@@ -415,7 +483,7 @@ TEST_F(SyncApiTest, WriteAndReadPassword) {
password_node.SetPasswordSpecifics(data);
}
{
- ReadTransaction trans(FROM_HERE, test_user_share_.user_share());
+ ReadTransaction trans(FROM_HERE, user_share());
ReadNode root_node(&trans);
root_node.InitByRootLookup();
@@ -431,13 +499,13 @@ TEST_F(SyncApiTest, WriteAndReadPassword) {
TEST_F(SyncApiTest, WriteEncryptedTitle) {
KeyParams params = {"localhost", "username", "passphrase"};
{
- ReadTransaction trans(FROM_HERE, test_user_share_.user_share());
+ ReadTransaction trans(FROM_HERE, user_share());
trans.GetCryptographer()->AddKey(params);
}
- test_user_share_.encryption_handler()->EnableEncryptEverything();
+ encryption_handler()->EnableEncryptEverything();
int bookmark_id;
{
- WriteTransaction trans(FROM_HERE, test_user_share_.user_share());
+ WriteTransaction trans(FROM_HERE, user_share());
ReadNode root_node(&trans);
root_node.InitByRootLookup();
@@ -453,7 +521,7 @@ TEST_F(SyncApiTest, WriteEncryptedTitle) {
pref_node.SetTitle("bar");
}
{
- ReadTransaction trans(FROM_HERE, test_user_share_.user_share());
+ ReadTransaction trans(FROM_HERE, user_share());
ReadNode root_node(&trans);
root_node.InitByRootLookup();
@@ -472,9 +540,8 @@ TEST_F(SyncApiTest, WriteEncryptedTitle) {
}
TEST_F(SyncApiTest, BaseNodeSetSpecifics) {
- int64 child_id = MakeNode(test_user_share_.user_share(),
- BOOKMARKS, "testtag");
- WriteTransaction trans(FROM_HERE, test_user_share_.user_share());
+ int64 child_id = MakeNode(user_share(), BOOKMARKS, "testtag");
+ WriteTransaction trans(FROM_HERE, user_share());
WriteNode node(&trans);
EXPECT_EQ(BaseNode::INIT_OK, node.InitByIdLookup(child_id));
@@ -489,9 +556,8 @@ TEST_F(SyncApiTest, BaseNodeSetSpecifics) {
}
TEST_F(SyncApiTest, BaseNodeSetSpecificsPreservesUnknownFields) {
- int64 child_id = MakeNode(test_user_share_.user_share(),
- BOOKMARKS, "testtag");
- WriteTransaction trans(FROM_HERE, test_user_share_.user_share());
+ int64 child_id = MakeNode(user_share(), BOOKMARKS, "testtag");
+ WriteTransaction trans(FROM_HERE, user_share());
WriteNode node(&trans);
EXPECT_EQ(BaseNode::INIT_OK, node.InitByIdLookup(child_id));
EXPECT_TRUE(node.GetEntitySpecifics().unknown_fields().empty());
@@ -508,7 +574,7 @@ TEST_F(SyncApiTest, BaseNodeSetSpecificsPreservesUnknownFields) {
}
TEST_F(SyncApiTest, EmptyTags) {
- WriteTransaction trans(FROM_HERE, test_user_share_.user_share());
+ WriteTransaction trans(FROM_HERE, user_share());
ReadNode root_node(&trans);
root_node.InitByRootLookup();
WriteNode node(&trans);
@@ -522,10 +588,9 @@ TEST_F(SyncApiTest, EmptyTags) {
// Test counting nodes when the type's root node has no children.
TEST_F(SyncApiTest, GetTotalNodeCountEmpty) {
- int64 type_root = MakeServerNodeForType(test_user_share_.user_share(),
- BOOKMARKS);
+ int64 type_root = MakeServerNodeForType(user_share(), BOOKMARKS);
{
- ReadTransaction trans(FROM_HERE, test_user_share_.user_share());
+ ReadTransaction trans(FROM_HERE, user_share());
ReadNode type_root_node(&trans);
EXPECT_EQ(BaseNode::INIT_OK,
type_root_node.InitByIdLookup(type_root));
@@ -535,14 +600,10 @@ TEST_F(SyncApiTest, GetTotalNodeCountEmpty) {
// Test counting nodes when there is one child beneath the type's root.
TEST_F(SyncApiTest, GetTotalNodeCountOneChild) {
- int64 type_root = MakeServerNodeForType(test_user_share_.user_share(),
- BOOKMARKS);
- int64 parent = MakeFolderWithParent(test_user_share_.user_share(),
- BOOKMARKS,
- type_root,
- NULL);
+ int64 type_root = MakeServerNodeForType(user_share(), BOOKMARKS);
+ int64 parent = MakeFolderWithParent(user_share(), BOOKMARKS, type_root, NULL);
{
- ReadTransaction trans(FROM_HERE, test_user_share_.user_share());
+ ReadTransaction trans(FROM_HERE, user_share());
ReadNode type_root_node(&trans);
EXPECT_EQ(BaseNode::INIT_OK,
type_root_node.InitByIdLookup(type_root));
@@ -557,32 +618,15 @@ TEST_F(SyncApiTest, GetTotalNodeCountOneChild) {
// Test counting nodes when there are multiple children beneath the type root,
// and one of those children has children of its own.
TEST_F(SyncApiTest, GetTotalNodeCountMultipleChildren) {
- int64 type_root = MakeServerNodeForType(test_user_share_.user_share(),
- BOOKMARKS);
- int64 parent = MakeFolderWithParent(test_user_share_.user_share(),
- BOOKMARKS,
- type_root,
- NULL);
- ignore_result(MakeFolderWithParent(test_user_share_.user_share(),
- BOOKMARKS,
- type_root,
- NULL));
- int64 child1 = MakeFolderWithParent(
- test_user_share_.user_share(),
- BOOKMARKS,
- parent,
- NULL);
- ignore_result(MakeBookmarkWithParent(
- test_user_share_.user_share(),
- parent,
- NULL));
- ignore_result(MakeBookmarkWithParent(
- test_user_share_.user_share(),
- child1,
- NULL));
-
- {
- ReadTransaction trans(FROM_HERE, test_user_share_.user_share());
+ int64 type_root = MakeServerNodeForType(user_share(), BOOKMARKS);
+ int64 parent = MakeFolderWithParent(user_share(), BOOKMARKS, type_root, NULL);
+ ignore_result(MakeFolderWithParent(user_share(), BOOKMARKS, type_root, NULL));
+ int64 child1 = MakeFolderWithParent(user_share(), BOOKMARKS, parent, NULL);
+ ignore_result(MakeBookmarkWithParent(user_share(), parent, NULL));
+ ignore_result(MakeBookmarkWithParent(user_share(), child1, NULL));
+
+ {
+ ReadTransaction trans(FROM_HERE, user_share());
ReadNode type_root_node(&trans);
EXPECT_EQ(BaseNode::INIT_OK,
type_root_node.InitByIdLookup(type_root));
@@ -594,6 +638,57 @@ TEST_F(SyncApiTest, GetTotalNodeCountMultipleChildren) {
}
}
+// Verify that Directory keeps track of which attachments are referenced by
+// which entries.
+TEST_F(SyncApiTest, AttachmentLinking) {
+ // Add an entry with an attachment.
+ std::string tag1("some tag");
+ syncer::AttachmentId attachment_id(syncer::AttachmentId::Create());
+ sync_pb::AttachmentMetadata attachment_metadata;
+ sync_pb::AttachmentMetadataRecord* record = attachment_metadata.add_record();
+ *record->mutable_id() = attachment_id.GetProto();
+ ASSERT_FALSE(dir()->IsAttachmentLinked(attachment_id.GetProto()));
+ CreateEntryWithAttachmentMetadata(PREFERENCES, tag1, attachment_metadata);
+
+ // See that the directory knows it's linked.
+ ASSERT_TRUE(dir()->IsAttachmentLinked(attachment_id.GetProto()));
+
+ // Add a second entry referencing the same attachment.
+ std::string tag2("some other tag");
+ CreateEntryWithAttachmentMetadata(PREFERENCES, tag2, attachment_metadata);
+
+ // See that the directory knows it's still linked.
+ ASSERT_TRUE(dir()->IsAttachmentLinked(attachment_id.GetProto()));
+
+ // Tombstone the first entry.
+ ReplaceWithTombstone(syncer::PREFERENCES, tag1);
+
+ // See that the attachment is still considered linked because the entry hasn't
+ // been purged from the Directory.
+ ASSERT_TRUE(dir()->IsAttachmentLinked(attachment_id.GetProto()));
+
+ // Save changes and see that the entry is truly gone.
+ ASSERT_TRUE(dir()->SaveChanges());
+ ASSERT_EQ(LookupEntryByClientTag(PREFERENCES, tag1),
+ syncer::WriteNode::INIT_FAILED_ENTRY_NOT_GOOD);
+
+ // However, the attachment is still linked.
+ ASSERT_TRUE(dir()->IsAttachmentLinked(attachment_id.GetProto()));
+
+ // Save, destroy, and recreate the directory. See that it's still linked.
+ ASSERT_TRUE(ReloadDir());
+ ASSERT_TRUE(dir()->IsAttachmentLinked(attachment_id.GetProto()));
+
+ // Tombstone the second entry, save changes, see that it's truly gone.
+ ReplaceWithTombstone(syncer::PREFERENCES, tag2);
+ ASSERT_TRUE(dir()->SaveChanges());
+ ASSERT_EQ(LookupEntryByClientTag(PREFERENCES, tag2),
+ syncer::WriteNode::INIT_FAILED_ENTRY_NOT_GOOD);
+
+ // Finally, the attachment is no longer linked.
+ ASSERT_FALSE(dir()->IsAttachmentLinked(attachment_id.GetProto()));
+}
+
namespace {
class TestHttpPostProviderInterface : public HttpPostProviderInterface {
diff --git a/sync/internal_api/write_node.cc b/sync/internal_api/write_node.cc
index 6a634eb..c3ef081 100644
--- a/sync/internal_api/write_node.cc
+++ b/sync/internal_api/write_node.cc
@@ -464,6 +464,11 @@ bool WriteNode::SetPosition(const BaseNode& new_parent,
return PutPredecessor(predecessor);
}
+void WriteNode::SetAttachmentMetadata(
+ const sync_pb::AttachmentMetadata& attachment_metadata) {
+ entry_->PutAttachmentMetadata(attachment_metadata);
+}
+
const syncable::Entry* WriteNode::GetEntry() const {
return entry_;
}
diff --git a/sync/protocol/attachments.proto b/sync/protocol/attachments.proto
index 50815d3..e08e2b0 100644
--- a/sync/protocol/attachments.proto
+++ b/sync/protocol/attachments.proto
@@ -21,7 +21,16 @@ message AttachmentIdProto {
optional string unique_id = 1;
}
+// Metadata for a single attachment.
+message AttachmentMetadataRecord {
+ optional AttachmentIdProto id = 1;
+ // Indicates we know this attachment exists on the server.
+ optional bool is_on_server = 2;
+}
+
// A collection of attachment metadata. This proto is part of EntryKernel's "on
// disk" representation. Private to sync.
message AttachmentMetadata {
+ // One record per attachment.
+ repeated AttachmentMetadataRecord record = 1;
}
diff --git a/sync/sync_internal_api.gypi b/sync/sync_internal_api.gypi
index 6078dd8..6f9af0328 100644
--- a/sync/sync_internal_api.gypi
+++ b/sync/sync_internal_api.gypi
@@ -45,6 +45,8 @@
'internal_api/protocol_event_buffer.h',
'internal_api/public/base/ack_handle.cc',
'internal_api/public/base/ack_handle.h',
+ 'internal_api/public/base/attachment_id_proto.cc',
+ 'internal_api/public/base/attachment_id_proto.h',
'internal_api/public/base/cancelation_observer.cc',
'internal_api/public/base/cancelation_observer.h',
'internal_api/public/base/cancelation_signal.cc',
diff --git a/sync/syncable/directory.cc b/sync/syncable/directory.cc
index 75c1bec..cebb2d4 100644
--- a/sync/syncable/directory.cc
+++ b/sync/syncable/directory.cc
@@ -10,6 +10,7 @@
#include "base/debug/trace_event.h"
#include "base/stl_util.h"
#include "base/strings/string_number_conversions.h"
+#include "sync/internal_api/public/base/attachment_id_proto.h"
#include "sync/internal_api/public/base/unique_position.h"
#include "sync/internal_api/public/util/unrecoverable_error_handler.h"
#include "sync/syncable/entry.h"
@@ -123,6 +124,7 @@ DirOpenResult Directory::Open(
}
void Directory::InitializeIndices(MetahandlesMap* handles_map) {
+ ScopedKernelLock lock(this);
kernel_->metahandles_map.swap(*handles_map);
for (MetahandlesMap::const_iterator it = kernel_->metahandles_map.begin();
it != kernel_->metahandles_map.end(); ++it) {
@@ -152,6 +154,7 @@ void Directory::InitializeIndices(MetahandlesMap* handles_map) {
kernel_->ids_map.end()) << "Unexpected duplicate use of ID";
kernel_->ids_map[entry->ref(ID).value()] = entry;
DCHECK(!entry->is_dirty());
+ AddToAttachmentIndex(metahandle, entry->ref(ATTACHMENT_METADATA), lock);
}
}
@@ -362,6 +365,8 @@ bool Directory::InsertEntry(BaseWriteTransaction* trans,
return false;
}
}
+ AddToAttachmentIndex(
+ entry->ref(META_HANDLE), entry->ref(ATTACHMENT_METADATA), *lock);
// Should NEVER be created with a client tag or server tag.
if (!SyncAssert(entry->ref(UNIQUE_SERVER_TAG).empty(), FROM_HERE,
@@ -408,6 +413,51 @@ bool Directory::ReindexParentId(BaseWriteTransaction* trans,
return true;
}
+void Directory::RemoveFromAttachmentIndex(
+ const int64 metahandle,
+ const sync_pb::AttachmentMetadata& attachment_metadata,
+ const ScopedKernelLock& lock) {
+ for (int i = 0; i < attachment_metadata.record_size(); ++i) {
+ AttachmentIdUniqueId unique_id =
+ attachment_metadata.record(i).id().unique_id();
+ IndexByAttachmentId::iterator iter =
+ kernel_->index_by_attachment_id.find(unique_id);
+ if (iter != kernel_->index_by_attachment_id.end()) {
+ iter->second.erase(metahandle);
+ if (iter->second.empty()) {
+ kernel_->index_by_attachment_id.erase(iter);
+ }
+ }
+ }
+}
+
+void Directory::AddToAttachmentIndex(
+ const int64 metahandle,
+ const sync_pb::AttachmentMetadata& attachment_metadata,
+ const ScopedKernelLock& lock) {
+ for (int i = 0; i < attachment_metadata.record_size(); ++i) {
+ AttachmentIdUniqueId unique_id =
+ attachment_metadata.record(i).id().unique_id();
+ IndexByAttachmentId::iterator iter =
+ kernel_->index_by_attachment_id.find(unique_id);
+ if (iter == kernel_->index_by_attachment_id.end()) {
+ iter = kernel_->index_by_attachment_id.insert(std::make_pair(
+ unique_id,
+ MetahandleSet())).first;
+ }
+ iter->second.insert(metahandle);
+ }
+}
+
+void Directory::UpdateAttachmentIndex(
+ const int64 metahandle,
+ const sync_pb::AttachmentMetadata& old_metadata,
+ const sync_pb::AttachmentMetadata& new_metadata) {
+ ScopedKernelLock lock(this);
+ RemoveFromAttachmentIndex(metahandle, old_metadata, lock);
+ AddToAttachmentIndex(metahandle, new_metadata, lock);
+}
+
bool Directory::unrecoverable_error_set(const BaseTransaction* trans) const {
DCHECK(trans != NULL);
return unrecoverable_error_set_;
@@ -548,6 +598,9 @@ bool Directory::VacuumAfterSaveChanges(const SaveChangesSnapshot& snapshot) {
"Deleted entry still present",
(&trans)))
return false;
+ RemoveFromAttachmentIndex(
+ entry->ref(META_HANDLE), entry->ref(ATTACHMENT_METADATA), lock);
+
delete entry;
}
if (trans.unrecoverable_error_set())
@@ -607,7 +660,8 @@ void Directory::UnapplyEntry(EntryKernel* entry) {
void Directory::DeleteEntry(bool save_to_journal,
EntryKernel* entry,
- EntryKernelSet* entries_to_journal) {
+ EntryKernelSet* entries_to_journal,
+ const ScopedKernelLock& lock) {
int64 handle = entry->ref(META_HANDLE);
ModelType server_type = GetModelTypeFromSpecifics(
entry->ref(SERVER_SPECIFICS));
@@ -637,6 +691,7 @@ void Directory::DeleteEntry(bool save_to_journal,
kernel_->server_tags_map.erase(entry->ref(UNIQUE_SERVER_TAG));
DCHECK_EQ(1u, num_erased);
}
+ RemoveFromAttachmentIndex(handle, entry->ref(ATTACHMENT_METADATA), lock);
if (save_to_journal) {
entries_to_journal->insert(entry);
@@ -704,7 +759,7 @@ bool Directory::PurgeEntriesWithTypeIn(ModelTypeSet disabled_types,
types_to_journal.Has(server_type)) &&
(delete_journal_->IsDeleteJournalEnabled(local_type) ||
delete_journal_->IsDeleteJournalEnabled(server_type));
- DeleteEntry(save_to_journal, entry, &entries_to_journal);
+ DeleteEntry(save_to_journal, entry, &entries_to_journal, lock);
}
}
@@ -762,6 +817,17 @@ bool Directory::ResetVersionsForType(BaseWriteTransaction* trans,
return true;
}
+bool Directory::IsAttachmentLinked(
+ const sync_pb::AttachmentIdProto& attachment_id_proto) const {
+ ScopedKernelLock lock(this);
+ IndexByAttachmentId::const_iterator iter =
+ kernel_->index_by_attachment_id.find(attachment_id_proto.unique_id());
+ if (iter != kernel_->index_by_attachment_id.end() && !iter->second.empty()) {
+ return true;
+ }
+ return false;
+}
+
void Directory::HandleSaveChangesFailure(const SaveChangesSnapshot& snapshot) {
WriteTransaction trans(FROM_HERE, HANDLE_SAVE_FAILURE, this);
ScopedKernelLock lock(this);
diff --git a/sync/syncable/directory.h b/sync/syncable/directory.h
index fdac9b4..5d26bbd 100644
--- a/sync/syncable/directory.h
+++ b/sync/syncable/directory.h
@@ -47,6 +47,9 @@ enum InvariantCheckLevel {
FULL_DB_VERIFICATION = 2 // Check every entry. This can be expensive.
};
+// Directory stores and manages EntryKernels.
+//
+// This class is tightly coupled to several other classes (see friends).
class SYNC_EXPORT Directory {
friend class BaseTransaction;
friend class Entry;
@@ -80,6 +83,9 @@ class SYNC_EXPORT Directory {
typedef base::hash_map<int64, EntryKernel*> MetahandlesMap;
typedef base::hash_map<std::string, EntryKernel*> IdsMap;
typedef base::hash_map<std::string, EntryKernel*> TagsMap;
+ typedef std::string AttachmentIdUniqueId;
+ typedef base::hash_map<AttachmentIdUniqueId, MetahandleSet>
+ IndexByAttachmentId;
static const base::FilePath::CharType kSyncDatabaseFilename[];
@@ -383,6 +389,14 @@ class SYNC_EXPORT Directory {
// WARNING! This can be slow, as it iterates over all entries for a type.
bool ResetVersionsForType(BaseWriteTransaction* trans, ModelType type);
+ // Returns true iff the attachment identified by |attachment_id_proto| is
+ // linked to an entry.
+ //
+ // An attachment linked to a deleted entry is still considered linked if the
+ // entry hasn't yet been purged.
+ bool IsAttachmentLinked(
+ const sync_pb::AttachmentIdProto& attachment_id_proto) const;
+
protected: // for friends, mainly used by Entry constructors
virtual EntryKernel* GetEntryByHandle(int64 handle);
virtual EntryKernel* GetEntryByHandle(int64 metahandle,
@@ -394,6 +408,11 @@ class SYNC_EXPORT Directory {
const Id& new_id);
bool ReindexParentId(BaseWriteTransaction* trans, EntryKernel* const entry,
const Id& new_parent_id);
+ // Update the attachment index for |metahandle| removing it from the index
+ // under |old_metadata| entries and add it under |new_metadata| entries.
+ void UpdateAttachmentIndex(const int64 metahandle,
+ const sync_pb::AttachmentMetadata& old_metadata,
+ const sync_pb::AttachmentMetadata& new_metadata);
void ClearDirtyMetahandles();
DirOpenResult OpenImpl(
@@ -451,6 +470,17 @@ class SYNC_EXPORT Directory {
// within parent. Protected by the ScopedKernelLock.
ParentChildIndex parent_child_index;
+ // This index keeps track of which metahandles refer to a given attachment.
+ // Think of it as the inverse of EntryKernel's AttachmentMetadata Records.
+ //
+ // Because entries can be undeleted (e.g. PutIsDel(false)), entries should
+ // not removed from the index until they are actually deleted from memory.
+ //
+ // All access should go through IsAttachmentLinked,
+ // RemoveFromAttachmentIndex, AddToAttachmentIndex, and
+ // UpdateAttachmentIndex methods to avoid iterator invalidation errors.
+ IndexByAttachmentId index_by_attachment_id;
+
// 3 in-memory indices on bits used extremely frequently by the syncer.
// |unapplied_update_metahandles| is keyed by the server model type.
MetahandleSet unapplied_update_metahandles[MODEL_TYPE_COUNT];
@@ -543,7 +573,19 @@ class SYNC_EXPORT Directory {
void UnapplyEntry(EntryKernel* entry);
void DeleteEntry(bool save_to_journal,
EntryKernel* entry,
- EntryKernelSet* entries_to_journal);
+ EntryKernelSet* entries_to_journal,
+ const ScopedKernelLock& lock);
+
+ // Remove each of |metahandle|'s attachment ids from index_by_attachment_id.
+ void RemoveFromAttachmentIndex(
+ const int64 metahandle,
+ const sync_pb::AttachmentMetadata& attachment_metadata,
+ const ScopedKernelLock& lock);
+ // Add each of |metahandle|'s attachment ids to the index_by_attachment_id.
+ void AddToAttachmentIndex(
+ const int64 metahandle,
+ const sync_pb::AttachmentMetadata& attachment_metadata,
+ const ScopedKernelLock& lock);
Kernel* kernel_;
diff --git a/sync/syncable/directory_backing_store.cc b/sync/syncable/directory_backing_store.cc
index e466548..ec28a53 100644
--- a/sync/syncable/directory_backing_store.cc
+++ b/sync/syncable/directory_backing_store.cc
@@ -70,6 +70,11 @@ void BindFields(const EntryKernel& entry,
entry.ref(static_cast<UniquePositionField>(i)).SerializeToString(&temp);
statement->BindBlob(index++, temp.data(), temp.length());
}
+ for (; i < ATTACHMENT_METADATA_FIELDS_END; ++i) {
+ std::string temp;
+ entry.ref(static_cast<AttachmentMetadataField>(i)).SerializeToString(&temp);
+ statement->BindBlob(index++, temp.data(), temp.length());
+ }
}
// The caller owns the returned EntryKernel*. Assumes the statement currently
@@ -114,6 +119,10 @@ scoped_ptr<EntryKernel> UnpackEntry(sql::Statement* statement) {
kernel->mutable_ref(static_cast<UniquePositionField>(i)) =
UniquePosition::FromProto(proto);
}
+ for (; i < ATTACHMENT_METADATA_FIELDS_END; ++i) {
+ kernel->mutable_ref(static_cast<AttachmentMetadataField>(i)).ParseFromArray(
+ statement->ColumnBlob(i), statement->ColumnByteLength(i));
+ }
return kernel.Pass();
}
diff --git a/sync/syncable/directory_unittest.cc b/sync/syncable/directory_unittest.cc
index b2bff36..f58f54f 100644
--- a/sync/syncable/directory_unittest.cc
+++ b/sync/syncable/directory_unittest.cc
@@ -6,6 +6,7 @@
#include "base/strings/stringprintf.h"
#include "base/test/values_test_util.h"
+#include "sync/internal_api/public/base/attachment_id_proto.h"
#include "sync/syncable/syncable_proto_util.h"
#include "sync/syncable/syncable_util.h"
#include "sync/syncable/syncable_write_transaction.h"
@@ -84,23 +85,45 @@ DirOpenResult SyncableDirectoryTest::ReopenDirectory() {
}
// Creates an empty entry and sets the ID field to a default one.
-void SyncableDirectoryTest::CreateEntry(const std::string& entryname) {
- CreateEntry(entryname, TestIdFactory::FromNumber(-99));
+void SyncableDirectoryTest::CreateEntry(const ModelType& model_type,
+ const std::string& entryname) {
+ CreateEntry(model_type, entryname, TestIdFactory::FromNumber(-99));
}
// Creates an empty entry and sets the ID field to id.
-void SyncableDirectoryTest::CreateEntry(const std::string& entryname,
+void SyncableDirectoryTest::CreateEntry(const ModelType& model_type,
+ const std::string& entryname,
const int id) {
- CreateEntry(entryname, TestIdFactory::FromNumber(id));
+ CreateEntry(model_type, entryname, TestIdFactory::FromNumber(id));
}
-void SyncableDirectoryTest::CreateEntry(const std::string& entryname, Id id) {
+
+void SyncableDirectoryTest::CreateEntry(const ModelType& model_type,
+ const std::string& entryname,
+ const Id& id) {
+ CreateEntryWithAttachmentMetadata(
+ model_type, entryname, id, sync_pb::AttachmentMetadata());
+}
+
+void SyncableDirectoryTest::CreateEntryWithAttachmentMetadata(
+ const ModelType& model_type,
+ const std::string& entryname,
+ const Id& id,
+ const sync_pb::AttachmentMetadata& attachment_metadata) {
WriteTransaction wtrans(FROM_HERE, UNITTEST, dir_.get());
- MutableEntry me(&wtrans, CREATE, BOOKMARKS, wtrans.root_id(), entryname);
+ MutableEntry me(&wtrans, CREATE, model_type, wtrans.root_id(), entryname);
ASSERT_TRUE(me.good());
me.PutId(id);
+ me.PutAttachmentMetadata(attachment_metadata);
me.PutIsUnsynced(true);
}
+void SyncableDirectoryTest::DeleteEntry(const Id& id) {
+ WriteTransaction trans(FROM_HERE, UNITTEST, dir().get());
+ MutableEntry entry(&trans, GET_BY_ID, id);
+ ASSERT_TRUE(entry.good());
+ entry.PutIsDel(true);
+}
+
DirOpenResult SyncableDirectoryTest::SimulateSaveAndReloadDir() {
if (!dir_->SaveChanges())
return FAILED_IN_UNITTEST;
@@ -411,8 +434,8 @@ TEST_F(SyncableDirectoryTest, ManageDeleteJournals) {
int64 handle2 = 0;
{
// Create two bookmark entries and save in database.
- CreateEntry("item1", id1);
- CreateEntry("item2", id2);
+ CreateEntry(BOOKMARKS, "item1", id1);
+ CreateEntry(BOOKMARKS, "item2", id2);
{
WriteTransaction trans(FROM_HERE, UNITTEST, dir().get());
MutableEntry item1(&trans, GET_BY_ID, id1);
@@ -529,7 +552,7 @@ TEST_F(SyncableDirectoryTest, TestBasicLookupNonExistantID) {
}
TEST_F(SyncableDirectoryTest, TestBasicLookupValidID) {
- CreateEntry("rtc");
+ CreateEntry(BOOKMARKS, "rtc");
ReadTransaction rtrans(FROM_HERE, dir().get());
Entry e(&rtrans, GET_BY_ID, TestIdFactory::FromNumber(-99));
ASSERT_TRUE(e.good());
@@ -1521,6 +1544,100 @@ TEST_F(SyncableDirectoryTest, StressTransactions) {
}
}
+// Verify that Directory is notifed when a MutableEntry's AttachmentMetadata
+// changes.
+TEST_F(SyncableDirectoryTest, MutableEntry_PutAttachmentMetadata) {
+ sync_pb::AttachmentMetadata attachment_metadata;
+ sync_pb::AttachmentMetadataRecord* record = attachment_metadata.add_record();
+ sync_pb::AttachmentIdProto attachment_id_proto =
+ syncer::CreateAttachmentIdProto();
+ *record->mutable_id() = attachment_id_proto;
+ ASSERT_FALSE(dir()->IsAttachmentLinked(attachment_id_proto));
+ {
+ WriteTransaction trans(FROM_HERE, UNITTEST, dir().get());
+
+ // Create an entry with attachment metadata and see that the attachment id
+ // is not linked.
+ MutableEntry entry(
+ &trans, CREATE, PREFERENCES, trans.root_id(), "some entry");
+ entry.PutId(TestIdFactory::FromNumber(-1));
+ entry.PutIsUnsynced(true);
+ ASSERT_FALSE(dir()->IsAttachmentLinked(attachment_id_proto));
+
+ // Now add the attachment metadata and see that Directory believes it is
+ // linked.
+ entry.PutAttachmentMetadata(attachment_metadata);
+ ASSERT_TRUE(dir()->IsAttachmentLinked(attachment_id_proto));
+
+ // Clear out the attachment metadata and see that it's no longer linked.
+ sync_pb::AttachmentMetadata empty_attachment_metadata;
+ entry.PutAttachmentMetadata(empty_attachment_metadata);
+ ASSERT_FALSE(dir()->IsAttachmentLinked(attachment_id_proto));
+ }
+ ASSERT_FALSE(dir()->IsAttachmentLinked(attachment_id_proto));
+}
+
+// Verify that deleted entries with attachments will retain the attachments.
+TEST_F(SyncableDirectoryTest, Directory_DeleteDoesNotUnlinkAttachments) {
+ sync_pb::AttachmentMetadata attachment_metadata;
+ sync_pb::AttachmentMetadataRecord* record = attachment_metadata.add_record();
+ sync_pb::AttachmentIdProto attachment_id_proto =
+ syncer::CreateAttachmentIdProto();
+ *record->mutable_id() = attachment_id_proto;
+ ASSERT_FALSE(dir()->IsAttachmentLinked(attachment_id_proto));
+ const Id id = TestIdFactory::FromNumber(-1);
+
+ // Create an entry with attachment metadata and see that the attachment id
+ // is linked.
+ CreateEntryWithAttachmentMetadata(
+ PREFERENCES, "some entry", id, attachment_metadata);
+ ASSERT_TRUE(dir()->IsAttachmentLinked(attachment_id_proto));
+
+ // Delete the entry and see that it's still linked because the entry hasn't
+ // yet been purged.
+ DeleteEntry(id);
+ ASSERT_TRUE(dir()->IsAttachmentLinked(attachment_id_proto));
+
+ // Reload the Directory, purging the deleted entry, and see that the
+ // attachment is no longer linked.
+ SimulateSaveAndReloadDir();
+ ASSERT_FALSE(dir()->IsAttachmentLinked(attachment_id_proto));
+}
+
+// Verify that a given attachment can be referenced by multiple entries and that
+// any one of the references is sufficient to ensure it remains linked.
+TEST_F(SyncableDirectoryTest, Directory_LastReferenceUnlinksAttachments) {
+ // Create one attachment.
+ sync_pb::AttachmentMetadata attachment_metadata;
+ sync_pb::AttachmentMetadataRecord* record = attachment_metadata.add_record();
+ sync_pb::AttachmentIdProto attachment_id_proto =
+ syncer::CreateAttachmentIdProto();
+ *record->mutable_id() = attachment_id_proto;
+
+ // Create two entries, each referencing the attachment.
+ const Id id1 = TestIdFactory::FromNumber(-1);
+ const Id id2 = TestIdFactory::FromNumber(-2);
+ CreateEntryWithAttachmentMetadata(
+ PREFERENCES, "some entry", id1, attachment_metadata);
+ CreateEntryWithAttachmentMetadata(
+ PREFERENCES, "some other entry", id2, attachment_metadata);
+
+ // See that the attachment is considered linked.
+ ASSERT_TRUE(dir()->IsAttachmentLinked(attachment_id_proto));
+
+ // Delete the first entry, reload the Directory, see that the attachment is
+ // still linked.
+ DeleteEntry(id1);
+ SimulateSaveAndReloadDir();
+ ASSERT_TRUE(dir()->IsAttachmentLinked(attachment_id_proto));
+
+ // Delete the second entry, reload the Directory, see that the attachment is
+ // no loner linked.
+ DeleteEntry(id2);
+ SimulateSaveAndReloadDir();
+ ASSERT_FALSE(dir()->IsAttachmentLinked(attachment_id_proto));
+}
+
} // namespace syncable
} // namespace syncer
diff --git a/sync/syncable/directory_unittest.h b/sync/syncable/directory_unittest.h
index 14e1a99..81e3888 100644
--- a/sync/syncable/directory_unittest.h
+++ b/sync/syncable/directory_unittest.h
@@ -31,7 +31,6 @@ class BaseTransaction;
//
// Serves as base class for several other test fixtures.
class SyncableDirectoryTest : public testing::Test {
- private:
protected:
static const char kDirectoryName[];
@@ -47,12 +46,24 @@ class SyncableDirectoryTest : public testing::Test {
DirOpenResult ReopenDirectory();
// Creates an empty entry and sets the ID field to a default one.
- void CreateEntry(const std::string& entryname);
+ void CreateEntry(const ModelType& model_type, const std::string& entryname);
// Creates an empty entry and sets the ID field to id.
- void CreateEntry(const std::string& entryname, const int id);
+ void CreateEntry(const ModelType& model_type,
+ const std::string& entryname,
+ const int id);
+
+ void CreateEntry(const ModelType& model_type,
+ const std::string& entryname,
+ const Id& id);
+
+ void CreateEntryWithAttachmentMetadata(
+ const ModelType& model_type,
+ const std::string& entryname,
+ const Id& id,
+ const sync_pb::AttachmentMetadata& attachment_metadata);
- void CreateEntry(const std::string& entryname, Id id);
+ void DeleteEntry(const Id& id);
// When a directory is saved then loaded from disk, it will pass through
// DropDeletedEntries(). This will remove some entries from the directory.
diff --git a/sync/syncable/entry_kernel.h b/sync/syncable/entry_kernel.h
index b6bc2ab..6380420 100644
--- a/sync/syncable/entry_kernel.h
+++ b/sync/syncable/entry_kernel.h
@@ -333,6 +333,10 @@ struct SYNC_EXPORT_PRIVATE EntryKernel {
inline UniquePosition& mutable_ref(UniquePositionField field) {
return unique_position_fields[field - UNIQUE_POSITION_FIELDS_BEGIN];
}
+ inline sync_pb::AttachmentMetadata& mutable_ref(
+ AttachmentMetadataField field) {
+ return attachment_metadata_fields[field - ATTACHMENT_METADATA_FIELDS_BEGIN];
+ }
ModelType GetModelType() const;
ModelType GetServerModelType() const;
diff --git a/sync/syncable/mutable_entry.cc b/sync/syncable/mutable_entry.cc
index 863e65b..d9d5daa 100644
--- a/sync/syncable/mutable_entry.cc
+++ b/sync/syncable/mutable_entry.cc
@@ -233,6 +233,20 @@ bool MutableEntry::PutPredecessor(const Id& predecessor_id) {
return true;
}
+void MutableEntry::PutAttachmentMetadata(
+ const sync_pb::AttachmentMetadata& attachment_metadata) {
+ DCHECK(kernel_);
+ write_transaction()->TrackChangesTo(kernel_);
+ if (kernel_->ref(ATTACHMENT_METADATA).SerializeAsString() !=
+ attachment_metadata.SerializeAsString()) {
+ dir()->UpdateAttachmentIndex(GetMetahandle(),
+ kernel_->ref(ATTACHMENT_METADATA),
+ attachment_metadata);
+ kernel_->put(ATTACHMENT_METADATA, attachment_metadata);
+ kernel_->mark_dirty(&dir()->kernel_->dirty_metahandles);
+ }
+}
+
// This function sets only the flags needed to get this entry to sync.
bool MarkForSyncing(MutableEntry* e) {
DCHECK_NE(static_cast<MutableEntry*>(NULL), e);
diff --git a/sync/syncable/mutable_entry.h b/sync/syncable/mutable_entry.h
index 8c2f2ab..c064358 100644
--- a/sync/syncable/mutable_entry.h
+++ b/sync/syncable/mutable_entry.h
@@ -59,6 +59,9 @@ class SYNC_EXPORT_PRIVATE MutableEntry : public ModelNeutralMutableEntry {
// ID to put the node in first position.
bool PutPredecessor(const Id& predecessor_id);
+ void PutAttachmentMetadata(
+ const sync_pb::AttachmentMetadata& attachment_metadata);
+
private:
// Kind of redundant. We should reduce the number of pointers
// floating around if at all possible. Could we store this in Directory?