diff options
30 files changed, 853 insertions, 167 deletions
diff --git a/components/sync_driver/generic_change_processor.cc b/components/sync_driver/generic_change_processor.cc index adc4c90..49889a6 100644 --- a/components/sync_driver/generic_change_processor.cc +++ b/components/sync_driver/generic_change_processor.cc @@ -36,6 +36,27 @@ void SetNodeSpecifics(const sync_pb::EntitySpecifics& entity_specifics, } } +// Helper function to convert AttachmentId to AttachmentMetadataRecord. +sync_pb::AttachmentMetadataRecord AttachmentIdToRecord( + const syncer::AttachmentId& attachment_id) { + sync_pb::AttachmentMetadataRecord record; + *record.mutable_id() = attachment_id.GetProto(); + return record; +} + +// Replace |write_nodes|'s attachment ids with |attachment_ids|. +void SetAttachmentMetadata(const syncer::AttachmentIdList& attachment_ids, + syncer::WriteNode* write_node) { + DCHECK(write_node); + sync_pb::AttachmentMetadata attachment_metadata; + std::transform( + attachment_ids.begin(), + attachment_ids.end(), + RepeatedFieldBackInserter(attachment_metadata.mutable_record()), + AttachmentIdToRecord); + write_node->SetAttachmentMetadata(attachment_metadata); +} + syncer::SyncData BuildRemoteSyncData( int64 sync_id, const syncer::BaseNode& read_node, @@ -319,12 +340,11 @@ syncer::SyncError LogLookupFailure( } } -syncer::SyncError AttemptDelete( - const syncer::SyncChange& change, - syncer::ModelType type, - const std::string& type_str, - syncer::WriteNode* node, - DataTypeErrorHandler* error_handler) { +syncer::SyncError AttemptDelete(const syncer::SyncChange& change, + syncer::ModelType type, + const std::string& type_str, + syncer::WriteNode* node, + DataTypeErrorHandler* error_handler) { DCHECK_EQ(change.change_type(), syncer::SyncChange::ACTION_DELETE); if (change.sync_data().IsLocal()) { const std::string& tag = syncer::SyncDataLocal(change.sync_data()).GetTag(); @@ -368,12 +388,36 @@ syncer::SyncError AttemptDelete( return syncer::SyncError(); } +// A callback invoked on completion of AttachmentService::StoreAttachment. +void IgnoreStoreResult(const syncer::AttachmentService::StoreResult&) { + // TODO(maniscalco): Here is where we're going to update the in-directory + // entry to indicate that the attachments have been successfully stored on + // disk. Why do we care? Because we might crash after persisting the + // directory to disk, but before we have persisted its attachments, leaving us + // with danging attachment ids. Having a flag that indicates we've stored the + // entry will allow us to detect and filter entries with dangling attachment + // ids (bug 368353). +} + +void StoreAttachments(syncer::AttachmentService* attachment_service, + const syncer::AttachmentList& attachments) { + DCHECK(attachment_service); + syncer::AttachmentService::StoreCallback ignore_store_result = + base::Bind(&IgnoreStoreResult); + attachment_service->StoreAttachments(attachments, ignore_store_result); +} + } // namespace syncer::SyncError GenericChangeProcessor::ProcessSyncChanges( const tracked_objects::Location& from_here, const syncer::SyncChangeList& list_of_changes) { DCHECK(CalledOnValidThread()); + + // Keep track of brand new attachments so we can persist them on this device + // and upload them to the server. + syncer::AttachmentList new_attachments; + syncer::WriteTransaction trans(from_here, share_handle()); for (syncer::SyncChangeList::const_iterator iter = list_of_changes.begin(); @@ -391,20 +435,19 @@ syncer::SyncError GenericChangeProcessor::ProcessSyncChanges( NOTREACHED(); return error; } - attachment_service_->OnSyncDataDelete(change.sync_data()); if (merge_result_.get()) { merge_result_->set_num_items_deleted( merge_result_->num_items_deleted() + 1); } } else if (change.change_type() == syncer::SyncChange::ACTION_ADD) { - syncer::SyncError error = - HandleActionAdd(change, type_str, type, trans, &sync_node); + syncer::SyncError error = HandleActionAdd( + change, type_str, type, trans, &sync_node, &new_attachments); if (error.IsSet()) { return error; } } else if (change.change_type() == syncer::SyncChange::ACTION_UPDATE) { - syncer::SyncError error = - HandleActionUpdate(change, type_str, type, trans, &sync_node); + syncer::SyncError error = HandleActionUpdate( + change, type_str, type, trans, &sync_node, &new_attachments); if (error.IsSet()) { return error; } @@ -422,6 +465,11 @@ syncer::SyncError GenericChangeProcessor::ProcessSyncChanges( return error; } } + + if (!new_attachments.empty()) { + StoreAttachments(attachment_service_.get(), new_attachments); + } + return syncer::SyncError(); } @@ -434,12 +482,14 @@ syncer::SyncError GenericChangeProcessor::HandleActionAdd( const std::string& type_str, const syncer::ModelType& type, const syncer::WriteTransaction& trans, - syncer::WriteNode* sync_node) { + syncer::WriteNode* sync_node, + syncer::AttachmentList* new_attachments) { // TODO(sync): Handle other types of creation (custom parents, folders, // etc.). syncer::ReadNode root_node(&trans); + const syncer::SyncDataLocal sync_data_local(change.sync_data()); if (root_node.InitByTagLookup(syncer::ModelTypeToRootTag( - change.sync_data().GetDataType())) != syncer::BaseNode::INIT_OK) { + sync_data_local.GetDataType())) != syncer::BaseNode::INIT_OK) { syncer::SyncError error(FROM_HERE, syncer::SyncError::DATATYPE_ERROR, "Failed to look up root node for type " + type_str, @@ -452,9 +502,7 @@ syncer::SyncError GenericChangeProcessor::HandleActionAdd( } syncer::WriteNode::InitUniqueByCreationResult result = sync_node->InitUniqueByCreation( - change.sync_data().GetDataType(), - root_node, - syncer::SyncDataLocal(change.sync_data()).GetTag()); + sync_data_local.GetDataType(), root_node, sync_data_local.GetTag()); if (result != syncer::WriteNode::INIT_SUCCESS) { std::string error_prefix = "Failed to create " + type_str + " node: " + change.location().ToString() + ", "; @@ -503,8 +551,18 @@ syncer::SyncError GenericChangeProcessor::HandleActionAdd( } } sync_node->SetTitle(change.sync_data().GetTitle()); - SetNodeSpecifics(change.sync_data().GetSpecifics(), sync_node); - attachment_service_->OnSyncDataAdd(change.sync_data()); + SetNodeSpecifics(sync_data_local.GetSpecifics(), sync_node); + + syncer::AttachmentIdList attachment_ids = sync_data_local.GetAttachmentIds(); + SetAttachmentMetadata(attachment_ids, sync_node); + + // Return any newly added attachments. + const syncer::AttachmentList& local_attachments_for_upload = + sync_data_local.GetLocalAttachmentsForUpload(); + new_attachments->insert(new_attachments->end(), + local_attachments_for_upload.begin(), + local_attachments_for_upload.end()); + if (merge_result_.get()) { merge_result_->set_num_items_added(merge_result_->num_items_added() + 1); } @@ -519,12 +577,14 @@ syncer::SyncError GenericChangeProcessor::HandleActionUpdate( const std::string& type_str, const syncer::ModelType& type, const syncer::WriteTransaction& trans, - syncer::WriteNode* sync_node) { + syncer::WriteNode* sync_node, + syncer::AttachmentList* new_attachments) { // TODO(zea): consider having this logic for all possible changes? + + const syncer::SyncDataLocal sync_data_local(change.sync_data()); syncer::BaseNode::InitByLookupResult result = - sync_node->InitByClientTagLookup( - change.sync_data().GetDataType(), - syncer::SyncDataLocal(change.sync_data()).GetTag()); + sync_node->InitByClientTagLookup(sync_data_local.GetDataType(), + sync_data_local.GetTag()); if (result != syncer::BaseNode::INIT_OK) { std::string error_prefix = "Failed to load " + type_str + " node. " + change.location().ToString() + ", "; @@ -606,9 +666,16 @@ syncer::SyncError GenericChangeProcessor::HandleActionUpdate( } sync_node->SetTitle(change.sync_data().GetTitle()); - SetNodeSpecifics(change.sync_data().GetSpecifics(), sync_node); - attachment_service_->OnSyncDataUpdate(sync_node->GetAttachmentIds(), - change.sync_data()); + SetNodeSpecifics(sync_data_local.GetSpecifics(), sync_node); + SetAttachmentMetadata(sync_data_local.GetAttachmentIds(), sync_node); + + // Return any newly added attachments. + const syncer::AttachmentList& local_attachments_for_upload = + sync_data_local.GetLocalAttachmentsForUpload(); + new_attachments->insert(new_attachments->end(), + local_attachments_for_upload.begin(), + local_attachments_for_upload.end()); + if (merge_result_.get()) { merge_result_->set_num_items_modified(merge_result_->num_items_modified() + 1); diff --git a/components/sync_driver/generic_change_processor.h b/components/sync_driver/generic_change_processor.h index a5e33cb..9f55a53 100644 --- a/components/sync_driver/generic_change_processor.h +++ b/components/sync_driver/generic_change_processor.h @@ -98,18 +98,27 @@ class GenericChangeProcessor : public ChangeProcessor, virtual syncer::UserShare* share_handle() const OVERRIDE; private: - // Helper methods for acting on changes coming from the datatype. These are - // logically part of ProcessSyncChanges. + // Logically part of ProcessSyncChanges. + // + // |new_attachments| is an output parameter containing newly added attachments + // that need to be stored. This method will append to it. syncer::SyncError HandleActionAdd(const syncer::SyncChange& change, const std::string& type_str, const syncer::ModelType& type, const syncer::WriteTransaction& trans, - syncer::WriteNode* sync_node); + syncer::WriteNode* sync_node, + syncer::AttachmentList* new_attachments); + + // Logically part of ProcessSyncChanges. + // + // |new_attachments| is an output parameter containing newly added attachments + // that need to be stored. This method will append to it. syncer::SyncError HandleActionUpdate(const syncer::SyncChange& change, const std::string& type_str, const syncer::ModelType& type, const syncer::WriteTransaction& trans, - syncer::WriteNode* sync_node); + syncer::WriteNode* sync_node, + syncer::AttachmentList* new_attachments); // The SyncableService this change processor will forward changes on to. const base::WeakPtr<syncer::SyncableService> local_service_; diff --git a/components/sync_driver/generic_change_processor_unittest.cc b/components/sync_driver/generic_change_processor_unittest.cc index 0fcce29..0875908 100644 --- a/components/sync_driver/generic_change_processor_unittest.cc +++ b/components/sync_driver/generic_change_processor_unittest.cc @@ -10,6 +10,7 @@ #include "base/strings/stringprintf.h" #include "components/sync_driver/data_type_error_handler_mock.h" #include "sync/api/attachments/fake_attachment_service.h" +#include "sync/api/attachments/fake_attachment_store.h" #include "sync/api/fake_syncable_service.h" #include "sync/api/sync_change.h" #include "sync/api/sync_merge_result.h" @@ -27,16 +28,50 @@ namespace browser_sync { namespace { +const char kTestData[] = "some data"; + +// A mock that keeps track of attachments passed to StoreAttachments. +class MockAttachmentService : public syncer::FakeAttachmentService { + public: + MockAttachmentService(); + virtual ~MockAttachmentService(); + virtual void StoreAttachments(const syncer::AttachmentList& attachments, + const StoreCallback& callback) OVERRIDE; + std::vector<syncer::AttachmentList>* attachment_lists(); + + private: + std::vector<syncer::AttachmentList> attachment_lists_; +}; + +MockAttachmentService::MockAttachmentService() + : FakeAttachmentService(scoped_ptr<syncer::AttachmentStore>( + new syncer::FakeAttachmentStore(base::MessageLoopProxy::current()))) { +} + +MockAttachmentService::~MockAttachmentService() { +} + +void MockAttachmentService::StoreAttachments( + const syncer::AttachmentList& attachments, + const StoreCallback& callback) { + attachment_lists_.push_back(attachments); + FakeAttachmentService::StoreAttachments(attachments, callback); +} + +std::vector<syncer::AttachmentList>* MockAttachmentService::attachment_lists() { + return &attachment_lists_; +} + class SyncGenericChangeProcessorTest : public testing::Test { public: // It doesn't matter which type we use. Just pick one. static const syncer::ModelType kType = syncer::PREFERENCES; - SyncGenericChangeProcessorTest() : - sync_merge_result_(kType), - merge_result_ptr_factory_(&sync_merge_result_), - syncable_service_ptr_factory_(&fake_syncable_service_) { - } + SyncGenericChangeProcessorTest() + : sync_merge_result_(kType), + merge_result_ptr_factory_(&sync_merge_result_), + syncable_service_ptr_factory_(&fake_syncable_service_), + mock_attachment_service_(NULL) {} virtual void SetUp() OVERRIDE { test_user_share_.SetUp(); @@ -47,15 +82,23 @@ class SyncGenericChangeProcessorTest : public testing::Test { test_user_share_.user_share()); } test_user_share_.encryption_handler()->Init(); + scoped_ptr<MockAttachmentService> mock_attachment_service( + new MockAttachmentService); + // GenericChangeProcessor takes ownership of the AttachmentService, but we + // need to have a pointer to it so we can see that it was used properly. + // Take a pointer and trust that GenericChangeProcessor does not prematurely + // destroy it. + mock_attachment_service_ = mock_attachment_service.get(); change_processor_.reset(new GenericChangeProcessor( &data_type_error_handler_, syncable_service_ptr_factory_.GetWeakPtr(), merge_result_ptr_factory_.GetWeakPtr(), test_user_share_.user_share(), - syncer::FakeAttachmentService::CreateForTest())); + mock_attachment_service.PassAs<syncer::AttachmentService>())); } virtual void TearDown() OVERRIDE { + mock_attachment_service_ = NULL; test_user_share_.TearDown(); } @@ -78,6 +121,10 @@ class SyncGenericChangeProcessorTest : public testing::Test { return test_user_share_.user_share(); } + MockAttachmentService* mock_attachment_service() { + return mock_attachment_service_; + } + private: base::MessageLoopForUI loop_; @@ -90,6 +137,7 @@ class SyncGenericChangeProcessorTest : public testing::Test { DataTypeErrorHandlerMock data_type_error_handler_; syncer::TestUserShare test_user_share_; + MockAttachmentService* mock_attachment_service_; scoped_ptr<GenericChangeProcessor> change_processor_; }; @@ -239,8 +287,60 @@ TEST_F(SyncGenericChangeProcessorTest, UpdatePasswords) { } } -// TODO(maniscalco): Add test cases that verify GenericChangeProcessor calls the -// right methods on its AttachmentService at the right times (bug 353303). +// Verify that attachments on newly added or updated SyncData are passed to the +// AttachmentService. +TEST_F(SyncGenericChangeProcessorTest, + ProcessSyncChanges_AddUpdateWithAttachment) { + std::string tag = "client_tag"; + std::string title = "client_title"; + sync_pb::EntitySpecifics specifics; + sync_pb::PreferenceSpecifics* pref_specifics = specifics.mutable_preference(); + pref_specifics->set_name("test"); + syncer::AttachmentList attachments; + scoped_refptr<base::RefCountedString> attachment_data = + new base::RefCountedString; + attachment_data->data() = kTestData; + attachments.push_back(syncer::Attachment::Create(attachment_data)); + attachments.push_back(syncer::Attachment::Create(attachment_data)); + + // Add a SyncData with two attachments. + syncer::SyncChangeList change_list; + change_list.push_back( + syncer::SyncChange(FROM_HERE, + syncer::SyncChange::ACTION_ADD, + syncer::SyncData::CreateLocalDataWithAttachments( + tag, title, specifics, attachments))); + ASSERT_FALSE( + change_processor()->ProcessSyncChanges(FROM_HERE, change_list).IsSet()); + + // Check that the AttachmentService received the new attachments. + ASSERT_EQ(mock_attachment_service()->attachment_lists()->size(), 1U); + const syncer::AttachmentList& attachments_added = + mock_attachment_service()->attachment_lists()->front(); + ASSERT_EQ(attachments_added.size(), 2U); + ASSERT_EQ(attachments_added[0].GetId(), attachments[0].GetId()); + ASSERT_EQ(attachments_added[1].GetId(), attachments[1].GetId()); + + // Update the SyncData, replacing its two attachments with one new attachment. + syncer::AttachmentList new_attachments; + new_attachments.push_back(syncer::Attachment::Create(attachment_data)); + mock_attachment_service()->attachment_lists()->clear(); + change_list.clear(); + change_list.push_back( + syncer::SyncChange(FROM_HERE, + syncer::SyncChange::ACTION_UPDATE, + syncer::SyncData::CreateLocalDataWithAttachments( + tag, title, specifics, new_attachments))); + ASSERT_FALSE( + change_processor()->ProcessSyncChanges(FROM_HERE, change_list).IsSet()); + + // Check that the AttachmentService received it. + ASSERT_EQ(mock_attachment_service()->attachment_lists()->size(), 1U); + const syncer::AttachmentList& new_attachments_added = + mock_attachment_service()->attachment_lists()->front(); + ASSERT_EQ(new_attachments_added.size(), 1U); + ASSERT_EQ(new_attachments_added[0].GetId(), new_attachments[0].GetId()); +} } // namespace 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? |