From f313245afd6b6568fc7456c8ee71d4dd187f8214 Mon Sep 17 00:00:00 2001 From: "maniscalco@chromium.org" Date: Thu, 17 Jul 2014 22:29:16 +0000 Subject: Update Commit and GetUpdatesResponse messages to include attachment ids. Private function HasAttachmentNotOnServer is now tested (albeit indirectly) by directory_commit_contribution_unittest.cc via DirectoryCommitContribution::Build. When displayed in about:sync, SyncEntity instances will now show any attachment ids they contain. This CL depends on issue 395913003. BUG=356266,394023 Review URL: https://codereview.chromium.org/393083004 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@283908 0039d316-1c4b-4281-b951-d872f2087c98 --- sync/engine/commit_util.cc | 16 ++ sync/engine/conflict_resolver.cc | 36 ++++- .../directory_commit_contribution_unittest.cc | 107 ++++++++++++- sync/engine/directory_update_handler_unittest.cc | 165 +++++++++++++++++++++ sync/engine/get_commit_ids.cc | 1 - sync/engine/syncer_util.cc | 4 + 6 files changed, 324 insertions(+), 5 deletions(-) (limited to 'sync/engine') diff --git a/sync/engine/commit_util.cc b/sync/engine/commit_util.cc index ad29e4f..c913edd 100644 --- a/sync/engine/commit_util.cc +++ b/sync/engine/commit_util.cc @@ -11,6 +11,7 @@ #include "base/strings/string_util.h" #include "sync/engine/syncer_proto_util.h" +#include "sync/internal_api/public/base/attachment_id_proto.h" #include "sync/internal_api/public/base/unique_position.h" #include "sync/protocol/bookmark_specifics.pb.h" #include "sync/protocol/sync.pb.h" @@ -83,6 +84,7 @@ void AddClientConfigParamsToMessage( } namespace { + void SetEntrySpecifics(const Entry& meta_entry, sync_pb::SyncEntity* sync_entry) { // Add the new style extension and the folder bit. @@ -92,6 +94,16 @@ void SetEntrySpecifics(const Entry& meta_entry, CHECK(!sync_entry->specifics().password().has_client_only_encrypted_data()); DCHECK_EQ(meta_entry.GetModelType(), GetModelType(*sync_entry)); } + +void SetAttachmentIds(const Entry& meta_entry, + sync_pb::SyncEntity* sync_entry) { + const sync_pb::AttachmentMetadata& attachment_metadata = + meta_entry.GetAttachmentMetadata(); + for (int i = 0; i < attachment_metadata.record_size(); ++i) { + *sync_entry->add_attachment_id() = attachment_metadata.record(i).id(); + } +} + } // namespace void BuildCommitItem( @@ -161,6 +173,8 @@ void BuildCommitItem( sync_entry->set_ctime(TimeToProtoTime(meta_entry.GetCtime())); sync_entry->set_mtime(TimeToProtoTime(meta_entry.GetMtime())); + SetAttachmentIds(meta_entry, sync_entry); + // Handle bookmarks separately. if (meta_entry.GetSpecifics().has_bookmark()) { if (meta_entry.GetIsDel()) { @@ -305,6 +319,8 @@ void UpdateServerFieldsAfterCommit( (committed_entry.folder() || committed_entry.bookmarkdata().bookmark_folder())); local_entry->PutServerSpecifics(committed_entry.specifics()); + local_entry->PutServerAttachmentMetadata( + CreateAttachmentMetadata(committed_entry.attachment_id())); local_entry->PutServerMtime(ProtoTimeToTime(committed_entry.mtime())); local_entry->PutServerCtime(ProtoTimeToTime(committed_entry.ctime())); if (committed_entry.has_unique_position()) { diff --git a/sync/engine/conflict_resolver.cc b/sync/engine/conflict_resolver.cc index 1630620..f914403 100644 --- a/sync/engine/conflict_resolver.cc +++ b/sync/engine/conflict_resolver.cc @@ -30,6 +30,39 @@ using syncable::Id; using syncable::MutableEntry; using syncable::WriteTransaction; +namespace { + +// Returns true iff the set of attachment ids contained in attachment_metadata +// matches the set of ids contained in server_attachment_metadata. +bool AttachmentMetadataMatches(const MutableEntry& entity) { + const sync_pb::AttachmentMetadata& local = entity.GetAttachmentMetadata(); + const sync_pb::AttachmentMetadata& server = + entity.GetServerAttachmentMetadata(); + if (local.record_size() != server.record_size()) { + return false; + } + + // The order of records in local and server may be different so use a std::set + // to determine if they are equivalent. + std::set local_ids; + for (int i = 0; i < local.record_size(); ++i) { + const sync_pb::AttachmentMetadataRecord& record = local.record(i); + DCHECK(record.is_on_server()); + local_ids.insert(record.id().SerializeAsString()); + } + for (int i = 0; i < server.record_size(); ++i) { + const sync_pb::AttachmentMetadataRecord& record = server.record(i); + DCHECK(record.is_on_server()); + if (local_ids.find(record.id().SerializeAsString()) == local_ids.end()) { + return false; + } + } + + return true; +} + +} // namespace + ConflictResolver::ConflictResolver() { } @@ -148,8 +181,9 @@ void ConflictResolver::ProcessSimpleConflict(WriteTransaction* trans, base_server_specifics_match = true; } + bool attachment_metadata_matches = AttachmentMetadataMatches(entry); if (!entry_deleted && name_matches && parent_matches && specifics_match && - position_matches) { + position_matches && attachment_metadata_matches) { DVLOG(1) << "Resolving simple conflict, everything matches, ignoring " << "changes for: " << entry; conflict_util::IgnoreConflict(&entry); diff --git a/sync/engine/directory_commit_contribution_unittest.cc b/sync/engine/directory_commit_contribution_unittest.cc index 894db97..7c12f50 100644 --- a/sync/engine/directory_commit_contribution_unittest.cc +++ b/sync/engine/directory_commit_contribution_unittest.cc @@ -5,6 +5,7 @@ #include "sync/engine/directory_commit_contribution.h" #include "base/message_loop/message_loop.h" +#include "sync/internal_api/public/base/attachment_id_proto.h" #include "sync/sessions/status_controller.h" #include "sync/syncable/entry.h" #include "sync/syncable/mutable_entry.h" @@ -25,6 +26,7 @@ class DirectoryCommitContributionTest : public ::testing::Test { syncable::WriteTransaction trans(FROM_HERE, syncable::UNITTEST, dir()); CreateTypeRoot(&trans, dir(), PREFERENCES); CreateTypeRoot(&trans, dir(), EXTENSIONS); + CreateTypeRoot(&trans, dir(), ARTICLES); CreateTypeRoot(&trans, dir(), BOOKMARKS); } @@ -33,9 +35,11 @@ class DirectoryCommitContributionTest : public ::testing::Test { } protected: - int64 CreateUnsyncedItem(syncable::WriteTransaction* trans, - ModelType type, - const std::string& tag) { + int64 CreateUnsyncedItemWithAttachments( + syncable::WriteTransaction* trans, + ModelType type, + const std::string& tag, + const sync_pb::AttachmentMetadata& attachment_metadata) { syncable::Entry parent_entry(trans, syncable::GET_TYPE_ROOT, type); syncable::MutableEntry entry( trans, @@ -43,10 +47,20 @@ class DirectoryCommitContributionTest : public ::testing::Test { type, parent_entry.GetId(), tag); + if (attachment_metadata.record_size() > 0) { + entry.PutAttachmentMetadata(attachment_metadata); + } entry.PutIsUnsynced(true); return entry.GetMetahandle(); } + int64 CreateUnsyncedItem(syncable::WriteTransaction* trans, + ModelType type, + const std::string& tag) { + return CreateUnsyncedItemWithAttachments( + trans, type, tag, sync_pb::AttachmentMetadata()); + } + int64 CreateSyncedItem(syncable::WriteTransaction* trans, ModelType type, const std::string& tag) { @@ -336,6 +350,13 @@ TEST_F(DirectoryCommitContributionTest, HierarchySupport_Preferences) { EXPECT_TRUE(commit_message.entries(0).parent_id_string().empty()); } +void AddAttachment(sync_pb::AttachmentMetadata* metadata, bool is_on_server) { + sync_pb::AttachmentMetadataRecord record; + *record.mutable_id() = CreateAttachmentIdProto(); + record.set_is_on_server(is_on_server); + *metadata->add_record() = record; +} + // Creates some unsynced items, pretends to commit them, and hands back a // specially crafted response to the syncer in order to test commit response // processing. The response simulates a succesful commit scenario. @@ -400,4 +421,84 @@ TEST_F(DirectoryCommitContributionTest, ProcessCommitResponse) { ext_cc->CleanUp(); } +// Creates some unsynced items with attachments and verifies that only items +// where all attachments have been uploaded to the server are eligible to be +// committed. +TEST_F(DirectoryCommitContributionTest, ProcessCommitResponseWithAttachments) { + int64 art1_handle; + int64 art2_handle; + int64 art3_handle; + { + syncable::WriteTransaction trans(FROM_HERE, syncable::UNITTEST, dir()); + + // art1 has two attachments, both have been uploaded to the server. art1 is + // eligible to be committed. + sync_pb::AttachmentMetadata art1_attachments; + AddAttachment(&art1_attachments, true /* is_on_server */); + AddAttachment(&art1_attachments, true /* is_on_server */); + art1_handle = CreateUnsyncedItemWithAttachments( + &trans, ARTICLES, "art1", art1_attachments); + + // art2 has two attachments, one of which has been uploaded to the + // server. art2 is not eligible to be committed. + sync_pb::AttachmentMetadata art2_attachments; + AddAttachment(&art2_attachments, false /* is_on_server */); + AddAttachment(&art2_attachments, true /* is_on_server */); + art2_handle = CreateUnsyncedItemWithAttachments( + &trans, ARTICLES, "art2", art2_attachments); + + // art3 has two attachments, neither of which have been uploaded to the + // server. art2 is not eligible to be committed. + sync_pb::AttachmentMetadata art3_attachments; + AddAttachment(&art3_attachments, false /* is_on_server */); + AddAttachment(&art3_attachments, false /* is_on_server */); + art3_handle = CreateUnsyncedItemWithAttachments( + &trans, ARTICLES, "art3", art3_attachments); + } + + DirectoryTypeDebugInfoEmitter emitter(ARTICLES, &type_observers_); + scoped_ptr art_cc( + DirectoryCommitContribution::Build(dir(), ARTICLES, 25, &emitter)); + + // Only art1 is ready. + EXPECT_EQ(1U, art_cc->GetNumEntries()); + + sync_pb::ClientToServerMessage message; + art_cc->AddToCommitMessage(&message); + + const sync_pb::CommitMessage& commit_message = message.commit(); + ASSERT_EQ(1, commit_message.entries_size()); + + sync_pb::ClientToServerResponse response; + for (int i = 0; i < commit_message.entries_size(); ++i) { + sync_pb::SyncEntity entity = commit_message.entries(i); + sync_pb::CommitResponse_EntryResponse* entry_response = + response.mutable_commit()->add_entryresponse(); + CreateSuccessfulCommitResponse(entity, entry_response); + } + + sessions::StatusController status; + art_cc->ProcessCommitResponse(response, &status); + { + syncable::ReadTransaction trans(FROM_HERE, dir()); + + syncable::Entry a1(&trans, syncable::GET_BY_HANDLE, art1_handle); + EXPECT_TRUE(a1.GetId().ServerKnows()); + EXPECT_FALSE(a1.GetSyncing()); + EXPECT_LT(0, a1.GetServerVersion()); + + syncable::Entry a2(&trans, syncable::GET_BY_HANDLE, art2_handle); + EXPECT_FALSE(a2.GetId().ServerKnows()); + EXPECT_FALSE(a2.GetSyncing()); + EXPECT_EQ(0, a2.GetServerVersion()); + + syncable::Entry a3(&trans, syncable::GET_BY_HANDLE, art3_handle); + EXPECT_FALSE(a3.GetId().ServerKnows()); + EXPECT_FALSE(a3.GetSyncing()); + EXPECT_EQ(0, a3.GetServerVersion()); + } + + art_cc->CleanUp(); +} + } // namespace syncer diff --git a/sync/engine/directory_update_handler_unittest.cc b/sync/engine/directory_update_handler_unittest.cc index 837e9f4..6052da19 100644 --- a/sync/engine/directory_update_handler_unittest.cc +++ b/sync/engine/directory_update_handler_unittest.cc @@ -8,6 +8,7 @@ #include "base/message_loop/message_loop.h" #include "base/stl_util.h" #include "sync/engine/syncer_proto_util.h" +#include "sync/internal_api/public/base/attachment_id_proto.h" #include "sync/internal_api/public/base/model_type.h" #include "sync/internal_api/public/test/test_entry_factory.h" #include "sync/protocol/sync.pb.h" @@ -396,6 +397,69 @@ TEST_F(DirectoryUpdateHandlerProcessUpdateTest, ContextVersion) { } } +// See that updates containing attachment metadata are applied +// (i.e. server_attachment_metadata is copied to attachment_metadata). +TEST_F(DirectoryUpdateHandlerProcessUpdateTest, + ProcessAndApplyUpdatesWithAttachments) { + DirectoryTypeDebugInfoEmitter emitter(ARTICLES, &type_observers_); + DirectoryUpdateHandler handler(dir(), ARTICLES, ui_worker(), &emitter); + sessions::StatusController status; + + sync_pb::DataTypeProgressMarker progress; + progress.set_data_type_id(GetSpecificsFieldNumberFromModelType(ARTICLES)); + progress.set_token("token"); + progress.mutable_gc_directive()->set_version_watermark(kDefaultVersion + 10); + + sync_pb::DataTypeContext context; + context.set_data_type_id(GetSpecificsFieldNumberFromModelType(ARTICLES)); + context.set_context("context"); + context.set_version(1); + + scoped_ptr type_root = + CreateUpdate(SyncableIdToProto(syncable::Id::CreateFromServerId("root")), + syncable::GetNullId().GetServerId(), + ARTICLES); + type_root->set_server_defined_unique_tag(ModelTypeToRootTag(ARTICLES)); + type_root->set_folder(true); + + scoped_ptr e1 = + CreateUpdate(SyncableIdToProto(syncable::Id::CreateFromServerId("e1")), + type_root->id_string(), + ARTICLES); + sync_pb::AttachmentIdProto* attachment_id = e1->add_attachment_id(); + *attachment_id = CreateAttachmentIdProto(); + + SyncEntityList updates; + updates.push_back(type_root.get()); + updates.push_back(e1.get()); + + // Process and apply updates. + EXPECT_EQ( + SYNCER_OK, + handler.ProcessGetUpdatesResponse(progress, context, updates, &status)); + handler.ApplyUpdates(&status); + + ASSERT_TRUE(EntryExists(type_root->id_string())); + ASSERT_TRUE(EntryExists(e1->id_string())); + { + syncable::ReadTransaction trans(FROM_HERE, dir()); + syncable::Entry e(&trans, + syncable::GET_BY_ID, + syncable::Id::CreateFromServerId(e1->id_string())); + + // See that the attachment_metadata is correct. + sync_pb::AttachmentMetadata attachment_metadata = e.GetAttachmentMetadata(); + ASSERT_EQ(1, attachment_metadata.record_size()); + ASSERT_EQ(attachment_id->SerializeAsString(), + attachment_metadata.record(0).id().SerializeAsString()); + ASSERT_TRUE(attachment_metadata.record(0).is_on_server()); + + // See that attachment_metadata and server_attachment_metadata are equal. + ASSERT_EQ(attachment_metadata.SerializeAsString(), + e.GetServerAttachmentMetadata().SerializeAsString()); + } +} + // A test harness for tests that focus on applying updates. // // Update application is performed when we want to take updates that were @@ -419,6 +483,7 @@ class DirectoryUpdateHandlerApplyUpdateTest : public ::testing::Test { passive_worker_(new FakeModelWorker(GROUP_PASSIVE)), bookmarks_emitter_(BOOKMARKS, &type_observers_), passwords_emitter_(PASSWORDS, &type_observers_), + articles_emitter_(ARTICLES, &type_observers_), update_handler_map_deleter_(&update_handler_map_) {} virtual void SetUp() OVERRIDE { @@ -435,6 +500,10 @@ class DirectoryUpdateHandlerApplyUpdateTest : public ::testing::Test { PASSWORDS, password_worker_, &passwords_emitter_))); + update_handler_map_.insert(std::make_pair( + ARTICLES, + new DirectoryUpdateHandler( + directory(), ARTICLES, ui_worker_, &articles_emitter_))); } virtual void TearDown() OVERRIDE { @@ -449,6 +518,10 @@ class DirectoryUpdateHandlerApplyUpdateTest : public ::testing::Test { return passwords_emitter_.GetUpdateCounters(); } + const UpdateCounters& GetArticlesUpdateCounters() { + return articles_emitter_.GetUpdateCounters(); + } + protected: void ApplyBookmarkUpdates(sessions::StatusController* status) { update_handler_map_[BOOKMARKS]->ApplyUpdates(status); @@ -458,6 +531,10 @@ class DirectoryUpdateHandlerApplyUpdateTest : public ::testing::Test { update_handler_map_[PASSWORDS]->ApplyUpdates(status); } + void ApplyArticlesUpdates(sessions::StatusController* status) { + update_handler_map_[ARTICLES]->ApplyUpdates(status); + } + TestEntryFactory* entry_factory() { return entry_factory_.get(); } @@ -480,6 +557,7 @@ class DirectoryUpdateHandlerApplyUpdateTest : public ::testing::Test { ObserverList type_observers_; DirectoryTypeDebugInfoEmitter bookmarks_emitter_; DirectoryTypeDebugInfoEmitter passwords_emitter_; + DirectoryTypeDebugInfoEmitter articles_emitter_; UpdateHandlerMap update_handler_map_; STLValueDeleter update_handler_map_deleter_; @@ -1024,4 +1102,91 @@ TEST_F(DirectoryUpdateHandlerApplyUpdateTest, SomeUndecryptablePassword) { } } +TEST_F(DirectoryUpdateHandlerApplyUpdateTest, + SimpleConflictDifferentAttachmentMetadata) { + const bool is_folder = false; + sync_pb::EntitySpecifics specifics; + *specifics.mutable_article() = sync_pb::ArticleSpecifics(); + int64 handle = entry_factory()->CreateSyncedItem("art1", ARTICLES, is_folder); + + sync_pb::AttachmentIdProto local_attachment_id = CreateAttachmentIdProto(); + sync_pb::AttachmentIdProto server_attachment_id = CreateAttachmentIdProto(); + + // Add an attachment to the local attachment metadata. + sync_pb::AttachmentMetadata local_metadata; + sync_pb::AttachmentMetadataRecord* local_record = local_metadata.add_record(); + *local_record->mutable_id() = local_attachment_id; + local_record->set_is_on_server(true); + entry_factory()->SetLocalAttachmentMetadataForItem(handle, local_metadata); + + // Add a different attachment to the server attachment metadata. + sync_pb::AttachmentMetadata server_metadata; + sync_pb::AttachmentMetadataRecord* server_record = + server_metadata.add_record(); + *server_record->mutable_id() = server_attachment_id; + server_record->set_is_on_server(true); + entry_factory()->SetServerAttachmentMetadataForItem(handle, server_metadata); + + // At this point we have a simple conflict. The server says art1 should have + // server_attachment_id, but the local sync engine says it should have + // local_attachment_id. + + sessions::StatusController status; + ApplyArticlesUpdates(&status); + + // See that the server won. + const UpdateCounters& counters = GetArticlesUpdateCounters(); + EXPECT_EQ(1, counters.num_updates_applied); + EXPECT_EQ(1, counters.num_local_overwrites); + EXPECT_EQ(0, counters.num_server_overwrites); + local_metadata = entry_factory()->GetLocalAttachmentMetadataForItem(handle); + EXPECT_EQ(server_metadata.SerializeAsString(), + local_metadata.SerializeAsString()); +} + +TEST_F(DirectoryUpdateHandlerApplyUpdateTest, + SimpleConflictSameAttachmentMetadataDifferentOrder) { + const bool is_folder = false; + sync_pb::EntitySpecifics specifics; + *specifics.mutable_article() = sync_pb::ArticleSpecifics(); + int64 handle = entry_factory()->CreateSyncedItem("art1", ARTICLES, is_folder); + + sync_pb::AttachmentIdProto id1 = CreateAttachmentIdProto(); + sync_pb::AttachmentIdProto id2 = CreateAttachmentIdProto(); + + // Add id1, then id2 to the local attachment metadata. + sync_pb::AttachmentMetadata local_metadata; + sync_pb::AttachmentMetadataRecord* record = local_metadata.add_record(); + *record->mutable_id() = id1; + record->set_is_on_server(true); + record = local_metadata.add_record(); + *record->mutable_id() = id2; + record->set_is_on_server(true); + entry_factory()->SetLocalAttachmentMetadataForItem(handle, local_metadata); + + // Add id1 and id2 to the server attachment metadata, but in reverse order. + sync_pb::AttachmentMetadata server_metadata; + record = server_metadata.add_record(); + *record->mutable_id() = id2; + record->set_is_on_server(true); + record = local_metadata.add_record(); + *record->mutable_id() = id1; + record->set_is_on_server(true); + entry_factory()->SetServerAttachmentMetadataForItem(handle, server_metadata); + + // At this point we have a (false) conflict. + + sessions::StatusController status; + ApplyArticlesUpdates(&status); + + // See that the server won. + const UpdateCounters& counters = GetArticlesUpdateCounters(); + EXPECT_EQ(1, counters.num_updates_applied); + EXPECT_EQ(1, counters.num_local_overwrites); + EXPECT_EQ(0, counters.num_server_overwrites); + local_metadata = entry_factory()->GetLocalAttachmentMetadataForItem(handle); + EXPECT_EQ(server_metadata.SerializeAsString(), + local_metadata.SerializeAsString()); +} + } // namespace syncer diff --git a/sync/engine/get_commit_ids.cc b/sync/engine/get_commit_ids.cc index 5f91915..5718cec 100644 --- a/sync/engine/get_commit_ids.cc +++ b/sync/engine/get_commit_ids.cc @@ -108,7 +108,6 @@ 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): 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()) { diff --git a/sync/engine/syncer_util.cc b/sync/engine/syncer_util.cc index c7d93c1..3cb9bca 100644 --- a/sync/engine/syncer_util.cc +++ b/sync/engine/syncer_util.cc @@ -17,6 +17,7 @@ #include "sync/engine/conflict_resolver.h" #include "sync/engine/syncer_proto_util.h" #include "sync/engine/syncer_types.h" +#include "sync/internal_api/public/base/attachment_id_proto.h" #include "sync/internal_api/public/base/model_type.h" #include "sync/internal_api/public/base/unique_position.h" #include "sync/protocol/bookmark_specifics.pb.h" @@ -407,6 +408,8 @@ void UpdateServerFieldsFromUpdate( bookmark.bookmark_favicon(), target); } + target->PutServerAttachmentMetadata( + CreateAttachmentMetadata(update.attachment_id())); if (SyncerProtoUtil::ShouldMaintainPosition(update)) { UpdateBookmarkPositioning(update, target); } @@ -463,6 +466,7 @@ void UpdateLocalDataFromServerData( entry->PutBaseVersion(entry->GetServerVersion()); entry->PutIsDel(entry->GetServerIsDel()); entry->PutIsUnappliedUpdate(false); + entry->PutAttachmentMetadata(entry->GetServerAttachmentMetadata()); } VerifyCommitResult ValidateCommitEntry(syncable::Entry* entry) { -- cgit v1.1