diff options
| -rw-r--r-- | sync/engine/process_updates_command.cc | 214 | ||||
| -rw-r--r-- | sync/engine/process_updates_command.h | 11 | ||||
| -rw-r--r-- | sync/engine/process_updates_command_unittest.cc | 62 | ||||
| -rw-r--r-- | sync/engine/syncer.cc | 8 | ||||
| -rw-r--r-- | sync/engine/syncer.h | 1 | ||||
| -rw-r--r-- | sync/engine/verify_updates_command.cc | 210 | ||||
| -rw-r--r-- | sync/engine/verify_updates_command.h | 48 | ||||
| -rw-r--r-- | sync/engine/verify_updates_command_unittest.cc | 109 | ||||
| -rw-r--r-- | sync/sessions/sync_session.cc | 18 | ||||
| -rw-r--r-- | sync/sessions/sync_session.h | 3 | ||||
| -rw-r--r-- | sync/sync.gyp | 3 |
11 files changed, 251 insertions, 436 deletions
diff --git a/sync/engine/process_updates_command.cc b/sync/engine/process_updates_command.cc index 0bf03f1..22d465c 100644 --- a/sync/engine/process_updates_command.cc +++ b/sync/engine/process_updates_command.cc @@ -32,50 +32,214 @@ using sessions::SyncSession; using sessions::StatusController; using sessions::UpdateProgress; +using syncable::GET_BY_ID; + ProcessUpdatesCommand::ProcessUpdatesCommand() {} ProcessUpdatesCommand::~ProcessUpdatesCommand() {} std::set<ModelSafeGroup> ProcessUpdatesCommand::GetGroupsToChange( const sessions::SyncSession& session) const { - return session.GetEnabledGroupsWithVerifiedUpdates(); + std::set<ModelSafeGroup> groups_with_updates; + + const sync_pb::GetUpdatesResponse& updates = + session.status_controller().updates_response().get_updates(); + for (int i = 0; i < updates.entries().size(); i++) { + groups_with_updates.insert( + GetGroupForModelType(GetModelType(updates.entries(i)), + session.routing_info())); + } + + return groups_with_updates; +} + +namespace { + +// This function attempts to determine whether or not this update is genuinely +// new, or if it is a reflection of one of our own commits. +// +// There is a known inaccuracy in its implementation. If this update ends up +// being applied to a local item with a different ID, we will count the change +// as being a non-reflection update. Fortunately, the server usually updates +// our IDs correctly in its commit response, so a new ID during GetUpdate should +// be rare. +// +// The only secnarios I can think of where this might happen are: +// - We commit a new item to the server, but we don't persist the +// server-returned new ID to the database before we shut down. On the GetUpdate +// following the next restart, we will receive an update from the server that +// updates its local ID. +// - When two attempts to create an item with identical UNIQUE_CLIENT_TAG values +// collide at the server. I have seen this in testing. When it happens, the +// test server will send one of the clients a response to upate its local ID so +// that both clients will refer to the item using the same ID going forward. In +// this case, we're right to assume that the update is not a reflection. +// +// For more information, see FindLocalIdToUpdate(). +bool UpdateContainsNewVersion(syncable::BaseTransaction *trans, + const sync_pb::SyncEntity &update) { + int64 existing_version = -1; // The server always sends positive versions. + syncable::Entry existing_entry(trans, GET_BY_ID, + SyncableIdFromProto(update.id_string())); + if (existing_entry.good()) + existing_version = existing_entry.Get(syncable::BASE_VERSION); + + if (!existing_entry.good() && update.deleted()) { + // There are several possible explanations for this. The most common cases + // will be first time sync and the redelivery of deletions we've already + // synced, accepted, and purged from our database. In either case, the + // update is useless to us. Let's count them all as "not new", even though + // that may not always be entirely accurate. + return false; + } + + if (existing_entry.good() && + !existing_entry.Get(syncable::UNIQUE_CLIENT_TAG).empty() && + existing_entry.Get(syncable::IS_DEL) && + update.deleted()) { + // Unique client tags will have their version set to zero when they're + // deleted. The usual version comparison logic won't be able to detect + // reflections of these items. Instead, we assume any received tombstones + // are reflections. That should be correct most of the time. + return false; + } + + return existing_version < update.version(); } +} // namespace + SyncerError ProcessUpdatesCommand::ModelChangingExecuteImpl( SyncSession* session) { syncable::Directory* dir = session->context()->directory(); - const sessions::UpdateProgress* progress = - session->status_controller().update_progress(); - if (!progress) - return SYNCER_OK; // Nothing to do. - syncable::WriteTransaction trans(FROM_HERE, syncable::SYNCER, dir); - vector<sessions::VerifiedUpdate>::const_iterator it; - for (it = progress->VerifiedUpdatesBegin(); - it != progress->VerifiedUpdatesEnd(); - ++it) { - const sync_pb::SyncEntity& update = it->second; - if (it->first != VERIFY_SUCCESS && it->first != VERIFY_UNDELETE) + sessions::StatusController* status = session->mutable_status_controller(); + const sync_pb::GetUpdatesResponse& updates = + status->updates_response().get_updates(); + int update_count = updates.entries().size(); + + ModelTypeSet requested_types = GetRoutingInfoTypes( + session->routing_info()); + + DVLOG(1) << update_count << " entries to verify"; + for (int i = 0; i < update_count; i++) { + const sync_pb::SyncEntity& update = updates.entries(i); + + // The current function gets executed on several different threads, but + // every call iterates over the same list of items that the server returned + // to us. We're not allowed to process items unless we're on the right + // thread for that type. This check will ensure we only touch the items + // that live on our current thread. + // TODO(tim): Don't allow access to objects in other ModelSafeGroups. + // See crbug.com/121521 . + ModelSafeGroup g = GetGroupForModelType(GetModelType(update), + session->routing_info()); + if (g != status->group_restriction()) continue; - switch (ProcessUpdate(update, - dir->GetCryptographer(&trans), - &trans)) { - case SUCCESS_PROCESSED: - case SUCCESS_STORED: - break; - default: - NOTREACHED(); - break; - } + + VerifyResult verify_result = VerifyUpdate(&trans, update, + requested_types, + session->routing_info()); + status->mutable_update_progress()->AddVerifyResult(verify_result, update); + status->increment_num_updates_downloaded_by(1); + if (!UpdateContainsNewVersion(&trans, update)) + status->increment_num_reflected_updates_downloaded_by(1); + if (update.deleted()) + status->increment_num_tombstone_updates_downloaded_by(1); + + if (verify_result != VERIFY_SUCCESS && verify_result != VERIFY_UNDELETE) + continue; + + ServerUpdateProcessingResult process_result = + ProcessUpdate(update, dir->GetCryptographer(&trans), &trans); + + DCHECK(process_result == SUCCESS_PROCESSED || + process_result == SUCCESS_STORED); } - StatusController* status = session->mutable_status_controller(); status->mutable_update_progress()->ClearVerifiedUpdates(); return SYNCER_OK; } namespace { + +// In the event that IDs match, but tags differ AttemptReuniteClient tag +// will have refused to unify the update. +// We should not attempt to apply it at all since it violates consistency +// rules. +VerifyResult VerifyTagConsistency(const sync_pb::SyncEntity& entry, + const syncable::MutableEntry& same_id) { + if (entry.has_client_defined_unique_tag() && + entry.client_defined_unique_tag() != + same_id.Get(syncable::UNIQUE_CLIENT_TAG)) { + return VERIFY_FAIL; + } + return VERIFY_UNDECIDED; +} + +} // namespace + +VerifyResult ProcessUpdatesCommand::VerifyUpdate( + syncable::WriteTransaction* trans, const sync_pb::SyncEntity& entry, + ModelTypeSet requested_types, + const ModelSafeRoutingInfo& routes) { + syncable::Id id = SyncableIdFromProto(entry.id_string()); + VerifyResult result = VERIFY_FAIL; + + const bool deleted = entry.has_deleted() && entry.deleted(); + const bool is_directory = IsFolder(entry); + const ModelType model_type = GetModelType(entry); + + if (!id.ServerKnows()) { + LOG(ERROR) << "Illegal negative id in received updates"; + return result; + } + { + const std::string name = SyncerProtoUtil::NameFromSyncEntity(entry); + if (name.empty() && !deleted) { + LOG(ERROR) << "Zero length name in non-deleted update"; + return result; + } + } + + syncable::MutableEntry same_id(trans, GET_BY_ID, id); + result = VerifyNewEntry(entry, &same_id, deleted); + + ModelType placement_type = !deleted ? GetModelType(entry) + : same_id.good() ? same_id.GetModelType() : UNSPECIFIED; + + if (VERIFY_UNDECIDED == result) { + result = VerifyTagConsistency(entry, same_id); + } + + if (VERIFY_UNDECIDED == result) { + if (deleted) { + // For deletes the server could send tombostones for items that + // the client did not request. If so ignore those items. + if (IsRealDataType(placement_type) && + !requested_types.Has(placement_type)) { + result = VERIFY_SKIP; + } else { + result = VERIFY_SUCCESS; + } + } + } + + // If we have an existing entry, we check here for updates that break + // consistency rules. + if (VERIFY_UNDECIDED == result) { + result = VerifyUpdateConsistency(trans, entry, &same_id, + deleted, is_directory, model_type); + } + + if (VERIFY_UNDECIDED == result) + result = VERIFY_SUCCESS; // No news is good news. + + return result; // This might be VERIFY_SUCCESS as well +} + +namespace { // Returns true if the entry is still ok to process. bool ReverifyEntry(syncable::WriteTransaction* trans, const sync_pb::SyncEntity& entry, @@ -115,10 +279,10 @@ ServerUpdateProcessingResult ProcessUpdatesCommand::ProcessUpdate( // We take a two step approach. First we store the entries data in the // server fields of a local entry and then move the data to the local fields - syncable::MutableEntry target_entry(trans, syncable::GET_BY_ID, local_id); + syncable::MutableEntry target_entry(trans, GET_BY_ID, local_id); // We need to run the Verify checks again; the world could have changed - // since VerifyUpdatesCommand. + // since we last verified. if (!ReverifyEntry(trans, update, &target_entry)) { return SUCCESS_PROCESSED; // The entry has become irrelevant. } diff --git a/sync/engine/process_updates_command.h b/sync/engine/process_updates_command.h index 7604d11..9f89a63 100644 --- a/sync/engine/process_updates_command.h +++ b/sync/engine/process_updates_command.h @@ -21,14 +21,12 @@ class WriteTransaction; class Cryptographer; -// A syncer command for processing updates. +// A syncer command for verifying and processing updates. // -// Preconditions - updates in the SyncerSesssion have been downloaded -// and verified. +// Preconditions - Updates in the SyncerSesssion have been downloaded. // // Postconditions - All of the verified SyncEntity data will be copied to // the server fields of the corresponding syncable entries. -// TODO(tim): This should not be ModelChanging (bug 36592). class ProcessUpdatesCommand : public ModelChangingSyncerCommand { public: ProcessUpdatesCommand(); @@ -42,6 +40,11 @@ class ProcessUpdatesCommand : public ModelChangingSyncerCommand { sessions::SyncSession* session) OVERRIDE; private: + VerifyResult VerifyUpdate( + syncable::WriteTransaction* trans, + const sync_pb::SyncEntity& entry, + ModelTypeSet requested_types, + const ModelSafeRoutingInfo& routes); ServerUpdateProcessingResult ProcessUpdate( const sync_pb::SyncEntity& proto_update, const Cryptographer* cryptographer, diff --git a/sync/engine/process_updates_command_unittest.cc b/sync/engine/process_updates_command_unittest.cc index 59f2210..fc543fb 100644 --- a/sync/engine/process_updates_command_unittest.cc +++ b/sync/engine/process_updates_command_unittest.cc @@ -3,11 +3,11 @@ // found in the LICENSE file. #include "base/basictypes.h" -#include "base/memory/ref_counted.h" #include "sync/engine/process_updates_command.h" #include "sync/internal_api/public/base/model_type.h" #include "sync/sessions/session_state.h" #include "sync/sessions/sync_session.h" +#include "sync/syncable/mutable_entry.h" #include "sync/syncable/syncable_id.h" #include "sync/test/engine/fake_model_worker.h" #include "sync/test/engine/syncer_command_test.h" @@ -15,6 +15,11 @@ namespace syncer { +using syncable::Id; +using syncable::MutableEntry; +using syncable::UNITTEST; +using syncable::WriteTransaction; + namespace { class ProcessUpdatesCommandTest : public SyncerCommandTest { @@ -27,24 +32,67 @@ class ProcessUpdatesCommandTest : public SyncerCommandTest { make_scoped_refptr(new FakeModelWorker(GROUP_UI))); workers()->push_back( make_scoped_refptr(new FakeModelWorker(GROUP_DB))); + (*mutable_routing_info())[PREFERENCES] = GROUP_UI; (*mutable_routing_info())[BOOKMARKS] = GROUP_UI; (*mutable_routing_info())[AUTOFILL] = GROUP_DB; SyncerCommandTest::SetUp(); } + void CreateLocalItem(const std::string& item_id, + const std::string& parent_id, + const ModelType& type) { + WriteTransaction trans(FROM_HERE, UNITTEST, directory()); + MutableEntry entry(&trans, syncable::CREATE_NEW_UPDATE_ITEM, + Id::CreateFromServerId(item_id)); + ASSERT_TRUE(entry.good()); + + entry.Put(syncable::BASE_VERSION, 1); + entry.Put(syncable::SERVER_VERSION, 1); + entry.Put(syncable::NON_UNIQUE_NAME, item_id); + entry.Put(syncable::PARENT_ID, Id::CreateFromServerId(parent_id)); + sync_pb::EntitySpecifics default_specifics; + AddDefaultFieldValue(type, &default_specifics); + entry.Put(syncable::SERVER_SPECIFICS, default_specifics); + } + + void AddUpdate(sync_pb::GetUpdatesResponse* updates, + const std::string& id, const std::string& parent, + const ModelType& type) { + sync_pb::SyncEntity* e = updates->add_entries(); + e->set_id_string("b1"); + e->set_parent_id_string(parent); + e->set_non_unique_name("b1"); + e->set_name("b1"); + AddDefaultFieldValue(type, e->mutable_specifics()); + } + ProcessUpdatesCommand command_; private: DISALLOW_COPY_AND_ASSIGN(ProcessUpdatesCommandTest); }; -TEST_F(ProcessUpdatesCommandTest, GetGroupsToChange) { +TEST_F(ProcessUpdatesCommandTest, GroupsToChange) { + std::string root = syncable::GetNullId().GetServerId(); + + CreateLocalItem("b1", root, BOOKMARKS); + CreateLocalItem("b2", root, BOOKMARKS); + CreateLocalItem("p1", root, PREFERENCES); + CreateLocalItem("a1", root, AUTOFILL); + ExpectNoGroupsToChange(command_); - // Add a verified update for GROUP_DB. - session()->mutable_status_controller()-> - GetUnrestrictedMutableUpdateProgressForTest(GROUP_DB)-> - AddVerifyResult(VerifyResult(), sync_pb::SyncEntity()); - ExpectGroupToChange(command_, GROUP_DB); + + sync_pb::GetUpdatesResponse* updates = + session()->mutable_status_controller()-> + mutable_updates_response()->mutable_get_updates(); + AddUpdate(updates, "b1", root, BOOKMARKS); + AddUpdate(updates, "b2", root, BOOKMARKS); + AddUpdate(updates, "p1", root, PREFERENCES); + AddUpdate(updates, "a1", root, AUTOFILL); + + ExpectGroupsToChange(command_, GROUP_UI, GROUP_DB); + + command_.ExecuteImpl(session()); } } // namespace diff --git a/sync/engine/syncer.cc b/sync/engine/syncer.cc index 7b09c3f6..ffb2fca 100644 --- a/sync/engine/syncer.cc +++ b/sync/engine/syncer.cc @@ -23,7 +23,6 @@ #include "sync/engine/store_timestamps_command.h" #include "sync/engine/syncer_types.h" #include "sync/engine/throttled_data_type_tracker.h" -#include "sync/engine/verify_updates_command.h" #include "sync/syncable/mutable_entry.h" #include "sync/syncable/syncable-inl.h" @@ -58,7 +57,6 @@ const char* SyncerStepToString(const SyncerStep step) switch (step) { ENUM_CASE(SYNCER_BEGIN); ENUM_CASE(DOWNLOAD_UPDATES); - ENUM_CASE(VERIFY_UPDATES); ENUM_CASE(PROCESS_UPDATES); ENUM_CASE(STORE_TIMESTAMPS); ENUM_CASE(APPLY_UPDATES); @@ -121,12 +119,6 @@ void Syncer::SyncShare(sessions::SyncSession* session, DownloadUpdatesCommand download_updates(kCreateMobileBookmarksFolder); session->mutable_status_controller()->set_last_download_updates_result( download_updates.Execute(session)); - next_step = VERIFY_UPDATES; - break; - } - case VERIFY_UPDATES: { - VerifyUpdatesCommand verify_updates; - verify_updates.Execute(session); next_step = PROCESS_UPDATES; break; } diff --git a/sync/engine/syncer.h b/sync/engine/syncer.h index 8fadde9..3c4c68e 100644 --- a/sync/engine/syncer.h +++ b/sync/engine/syncer.h @@ -27,7 +27,6 @@ class MutableEntry; enum SyncerStep { SYNCER_BEGIN, DOWNLOAD_UPDATES, - VERIFY_UPDATES, PROCESS_UPDATES, STORE_TIMESTAMPS, APPLY_UPDATES, diff --git a/sync/engine/verify_updates_command.cc b/sync/engine/verify_updates_command.cc deleted file mode 100644 index b8bc043..0000000 --- a/sync/engine/verify_updates_command.cc +++ /dev/null @@ -1,210 +0,0 @@ -// Copyright (c) 2012 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/engine/verify_updates_command.h" - -#include <string> - -#include "base/location.h" -#include "sync/engine/syncer.h" -#include "sync/engine/syncer_proto_util.h" -#include "sync/engine/syncer_types.h" -#include "sync/engine/syncer_util.h" -#include "sync/internal_api/public/engine/model_safe_worker.h" -#include "sync/protocol/bookmark_specifics.pb.h" -#include "sync/protocol/sync.pb.h" -#include "sync/syncable/entry.h" -#include "sync/syncable/mutable_entry.h" -#include "sync/syncable/syncable_proto_util.h" -#include "sync/syncable/write_transaction.h" - -namespace syncer { - -using syncable::GET_BY_ID; -using syncable::SYNCER; -using syncable::WriteTransaction; - -namespace { - -// This function attempts to determine whether or not this update is genuinely -// new, or if it is a reflection of one of our own commits. -// -// There is a known inaccuracy in its implementation. If this update ends up -// being applied to a local item with a different ID, we will count the change -// as being a non-reflection update. Fortunately, the server usually updates -// our IDs correctly in its commit response, so a new ID during GetUpdate should -// be rare. -// -// The only secnarios I can think of where this might happen are: -// - We commit a new item to the server, but we don't persist the -// server-returned new ID to the database before we shut down. On the GetUpdate -// following the next restart, we will receive an update from the server that -// updates its local ID. -// - When two attempts to create an item with identical UNIQUE_CLIENT_TAG values -// collide at the server. I have seen this in testing. When it happens, the -// test server will send one of the clients a response to upate its local ID so -// that both clients will refer to the item using the same ID going forward. In -// this case, we're right to assume that the update is not a reflection. -// -// For more information, see FindLocalIdToUpdate(). -bool UpdateContainsNewVersion(syncable::BaseTransaction *trans, - const sync_pb::SyncEntity &update) { - int64 existing_version = -1; // The server always sends positive versions. - syncable::Entry existing_entry(trans, GET_BY_ID, - SyncableIdFromProto(update.id_string())); - if (existing_entry.good()) - existing_version = existing_entry.Get(syncable::BASE_VERSION); - - if (!existing_entry.good() && update.deleted()) { - // There are several possible explanations for this. The most common cases - // will be first time sync and the redelivery of deletions we've already - // synced, accepted, and purged from our database. In either case, the - // update is useless to us. Let's count them all as "not new", even though - // that may not always be entirely accurate. - return false; - } - - if (existing_entry.good() && - !existing_entry.Get(syncable::UNIQUE_CLIENT_TAG).empty() && - existing_entry.Get(syncable::IS_DEL) && - update.deleted()) { - // Unique client tags will have their version set to zero when they're - // deleted. The usual version comparison logic won't be able to detect - // reflections of these items. Instead, we assume any received tombstones - // are reflections. That should be correct most of the time. - return false; - } - - return existing_version < update.version(); -} - -// In the event that IDs match, but tags differ AttemptReuniteClient tag -// will have refused to unify the update. -// We should not attempt to apply it at all since it violates consistency -// rules. -VerifyResult VerifyTagConsistency(const sync_pb::SyncEntity& entry, - const syncable::MutableEntry& same_id) { - if (entry.has_client_defined_unique_tag() && - entry.client_defined_unique_tag() != - same_id.Get(syncable::UNIQUE_CLIENT_TAG)) { - return VERIFY_FAIL; - } - return VERIFY_UNDECIDED; -} -} // namespace - -VerifyUpdatesCommand::VerifyUpdatesCommand() {} -VerifyUpdatesCommand::~VerifyUpdatesCommand() {} - -std::set<ModelSafeGroup> VerifyUpdatesCommand::GetGroupsToChange( - const sessions::SyncSession& session) const { - std::set<ModelSafeGroup> groups_with_updates; - - const sync_pb::GetUpdatesResponse& updates = - session.status_controller().updates_response().get_updates(); - for (int i = 0; i < updates.entries().size(); i++) { - groups_with_updates.insert( - GetGroupForModelType(GetModelType(updates.entries(i)), - session.routing_info())); - } - - return groups_with_updates; -} - -SyncerError VerifyUpdatesCommand::ModelChangingExecuteImpl( - sessions::SyncSession* session) { - DVLOG(1) << "Beginning Update Verification"; - syncable::Directory* dir = session->context()->directory(); - WriteTransaction trans(FROM_HERE, SYNCER, dir); - sessions::StatusController* status = session->mutable_status_controller(); - const sync_pb::GetUpdatesResponse& updates = - status->updates_response().get_updates(); - int update_count = updates.entries().size(); - - ModelTypeSet requested_types = GetRoutingInfoTypes( - session->routing_info()); - - DVLOG(1) << update_count << " entries to verify"; - for (int i = 0; i < update_count; i++) { - const sync_pb::SyncEntity& update = updates.entries(i); - ModelSafeGroup g = GetGroupForModelType(GetModelType(update), - session->routing_info()); - if (g != status->group_restriction()) - continue; - - VerifyUpdateResult result = VerifyUpdate(&trans, update, - requested_types, - session->routing_info()); - status->mutable_update_progress()->AddVerifyResult(result.value, update); - status->increment_num_updates_downloaded_by(1); - if (!UpdateContainsNewVersion(&trans, update)) - status->increment_num_reflected_updates_downloaded_by(1); - if (update.deleted()) - status->increment_num_tombstone_updates_downloaded_by(1); - } - - return SYNCER_OK; -} - -VerifyUpdatesCommand::VerifyUpdateResult VerifyUpdatesCommand::VerifyUpdate( - syncable::WriteTransaction* trans, const sync_pb::SyncEntity& entry, - ModelTypeSet requested_types, - const ModelSafeRoutingInfo& routes) { - syncable::Id id = SyncableIdFromProto(entry.id_string()); - VerifyUpdateResult result = {VERIFY_FAIL, GROUP_PASSIVE}; - - const bool deleted = entry.has_deleted() && entry.deleted(); - const bool is_directory = IsFolder(entry); - const ModelType model_type = GetModelType(entry); - - if (!id.ServerKnows()) { - LOG(ERROR) << "Illegal negative id in received updates"; - return result; - } - { - const std::string name = SyncerProtoUtil::NameFromSyncEntity(entry); - if (name.empty() && !deleted) { - LOG(ERROR) << "Zero length name in non-deleted update"; - return result; - } - } - - syncable::MutableEntry same_id(trans, GET_BY_ID, id); - result.value = VerifyNewEntry(entry, &same_id, deleted); - - ModelType placement_type = !deleted ? GetModelType(entry) - : same_id.good() ? same_id.GetModelType() : UNSPECIFIED; - result.placement = GetGroupForModelType(placement_type, routes); - - if (VERIFY_UNDECIDED == result.value) { - result.value = VerifyTagConsistency(entry, same_id); - } - - if (VERIFY_UNDECIDED == result.value) { - if (deleted) { - // For deletes the server could send tombostones for items that - // the client did not request. If so ignore those items. - if (IsRealDataType(placement_type) && - !requested_types.Has(placement_type)) { - result.value = VERIFY_SKIP; - } else { - result.value = VERIFY_SUCCESS; - } - } - } - - // If we have an existing entry, we check here for updates that break - // consistency rules. - if (VERIFY_UNDECIDED == result.value) { - result.value = VerifyUpdateConsistency(trans, entry, &same_id, - deleted, is_directory, model_type); - } - - if (VERIFY_UNDECIDED == result.value) - result.value = VERIFY_SUCCESS; // No news is good news. - - return result; // This might be VERIFY_SUCCESS as well -} - -} // namespace syncer diff --git a/sync/engine/verify_updates_command.h b/sync/engine/verify_updates_command.h deleted file mode 100644 index 1788b77..0000000 --- a/sync/engine/verify_updates_command.h +++ /dev/null @@ -1,48 +0,0 @@ -// Copyright (c) 2012 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_ENGINE_VERIFY_UPDATES_COMMAND_H_ -#define SYNC_ENGINE_VERIFY_UPDATES_COMMAND_H_ - -#include "base/basictypes.h" -#include "base/compiler_specific.h" -#include "sync/engine/model_changing_syncer_command.h" -#include "sync/engine/syncer_types.h" -#include "sync/internal_api/public/engine/model_safe_worker.h" - -namespace syncer { - -namespace syncable { -class WriteTransaction; -} - -// Verifies the response from a GetUpdates request. All invalid updates will be -// noted in the SyncSession after this command is executed. -class VerifyUpdatesCommand : public ModelChangingSyncerCommand { - public: - VerifyUpdatesCommand(); - virtual ~VerifyUpdatesCommand(); - - protected: - // ModelChangingSyncerCommand implementation. - virtual std::set<ModelSafeGroup> GetGroupsToChange( - const sessions::SyncSession& session) const OVERRIDE; - virtual SyncerError ModelChangingExecuteImpl( - sessions::SyncSession* session) OVERRIDE; - - private: - struct VerifyUpdateResult { - VerifyResult value; - ModelSafeGroup placement; - }; - VerifyUpdateResult VerifyUpdate(syncable::WriteTransaction* trans, - const sync_pb::SyncEntity& entry, - ModelTypeSet requested_types, - const ModelSafeRoutingInfo& routes); - DISALLOW_COPY_AND_ASSIGN(VerifyUpdatesCommand); -}; - -} // namespace syncer - -#endif // SYNC_ENGINE_VERIFY_UPDATES_COMMAND_H_ diff --git a/sync/engine/verify_updates_command_unittest.cc b/sync/engine/verify_updates_command_unittest.cc deleted file mode 100644 index c0ea363..0000000 --- a/sync/engine/verify_updates_command_unittest.cc +++ /dev/null @@ -1,109 +0,0 @@ -// Copyright (c) 2012 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 "base/location.h" -#include "sync/engine/verify_updates_command.h" -#include "sync/protocol/bookmark_specifics.pb.h" -#include "sync/sessions/session_state.h" -#include "sync/sessions/sync_session.h" -#include "sync/syncable/mutable_entry.h" -#include "sync/syncable/syncable_id.h" -#include "sync/test/engine/fake_model_worker.h" -#include "sync/test/engine/syncer_command_test.h" -#include "testing/gtest/include/gtest/gtest.h" - -namespace syncer { - -using sessions::StatusController; -using std::string; -using syncable::Id; -using syncable::MutableEntry; -using syncable::UNITTEST; -using syncable::WriteTransaction; - -class VerifyUpdatesCommandTest : public SyncerCommandTest { - public: - virtual void SetUp() { - workers()->clear(); - mutable_routing_info()->clear(); - workers()->push_back(make_scoped_refptr(new FakeModelWorker(GROUP_DB))); - workers()->push_back(make_scoped_refptr(new FakeModelWorker(GROUP_UI))); - (*mutable_routing_info())[PREFERENCES] = GROUP_UI; - (*mutable_routing_info())[BOOKMARKS] = GROUP_UI; - (*mutable_routing_info())[AUTOFILL] = GROUP_DB; - SyncerCommandTest::SetUp(); - } - - void CreateLocalItem(const std::string& item_id, - const std::string& parent_id, - const ModelType& type) { - WriteTransaction trans(FROM_HERE, UNITTEST, directory()); - MutableEntry entry(&trans, syncable::CREATE_NEW_UPDATE_ITEM, - Id::CreateFromServerId(item_id)); - ASSERT_TRUE(entry.good()); - - entry.Put(syncable::BASE_VERSION, 1); - entry.Put(syncable::SERVER_VERSION, 1); - entry.Put(syncable::NON_UNIQUE_NAME, item_id); - entry.Put(syncable::PARENT_ID, Id::CreateFromServerId(parent_id)); - sync_pb::EntitySpecifics default_specifics; - AddDefaultFieldValue(type, &default_specifics); - entry.Put(syncable::SERVER_SPECIFICS, default_specifics); - } - - void AddUpdate(sync_pb::GetUpdatesResponse* updates, - const std::string& id, const std::string& parent, - const ModelType& type) { - sync_pb::SyncEntity* e = updates->add_entries(); - e->set_id_string("b1"); - e->set_parent_id_string(parent); - e->set_non_unique_name("b1"); - e->set_name("b1"); - AddDefaultFieldValue(type, e->mutable_specifics()); - } - - VerifyUpdatesCommand command_; - -}; - -TEST_F(VerifyUpdatesCommandTest, AllVerified) { - string root = syncable::GetNullId().GetServerId(); - - CreateLocalItem("b1", root, BOOKMARKS); - CreateLocalItem("b2", root, BOOKMARKS); - CreateLocalItem("p1", root, PREFERENCES); - CreateLocalItem("a1", root, AUTOFILL); - - ExpectNoGroupsToChange(command_); - - sync_pb::GetUpdatesResponse* updates = - session()->mutable_status_controller()-> - mutable_updates_response()->mutable_get_updates(); - AddUpdate(updates, "b1", root, BOOKMARKS); - AddUpdate(updates, "b2", root, BOOKMARKS); - AddUpdate(updates, "p1", root, PREFERENCES); - AddUpdate(updates, "a1", root, AUTOFILL); - - ExpectGroupsToChange(command_, GROUP_UI, GROUP_DB); - - command_.ExecuteImpl(session()); - - StatusController* status = session()->mutable_status_controller(); - { - sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI); - ASSERT_TRUE(status->update_progress()); - EXPECT_EQ(3, status->update_progress()->VerifiedUpdatesSize()); - } - { - sessions::ScopedModelSafeGroupRestriction r(status, GROUP_DB); - ASSERT_TRUE(status->update_progress()); - EXPECT_EQ(1, status->update_progress()->VerifiedUpdatesSize()); - } - { - sessions::ScopedModelSafeGroupRestriction r(status, GROUP_PASSIVE); - EXPECT_FALSE(status->update_progress()); - } -} - -} // namespace syncer diff --git a/sync/sessions/sync_session.cc b/sync/sessions/sync_session.cc index 5586819..5a2b572 100644 --- a/sync/sessions/sync_session.cc +++ b/sync/sessions/sync_session.cc @@ -219,24 +219,6 @@ std::set<ModelSafeGroup> SyncSession::GetEnabledGroupsWithConflicts() const { return enabled_groups_with_conflicts; } -std::set<ModelSafeGroup> - SyncSession::GetEnabledGroupsWithVerifiedUpdates() const { - const std::set<ModelSafeGroup>& enabled_groups = GetEnabledGroups(); - std::set<ModelSafeGroup> enabled_groups_with_verified_updates; - for (std::set<ModelSafeGroup>::const_iterator it = - enabled_groups.begin(); it != enabled_groups.end(); ++it) { - const UpdateProgress* update_progress = - status_controller_->GetUnrestrictedUpdateProgress(*it); - if (update_progress && - (update_progress->VerifiedUpdatesBegin() != - update_progress->VerifiedUpdatesEnd())) { - enabled_groups_with_verified_updates.insert(*it); - } - } - - return enabled_groups_with_verified_updates; -} - namespace { // Returns false iff one of the command results had an error. diff --git a/sync/sessions/sync_session.h b/sync/sessions/sync_session.h index 461d98e..93d7f41 100644 --- a/sync/sessions/sync_session.h +++ b/sync/sessions/sync_session.h @@ -177,9 +177,6 @@ class SyncSession { // Returns the set of enabled groups that have conflicts. std::set<ModelSafeGroup> GetEnabledGroupsWithConflicts() const; - // Returns the set of enabled groups that have verified updates. - std::set<ModelSafeGroup> GetEnabledGroupsWithVerifiedUpdates() const; - // Mark the session has having finished all the sync steps it needed. void SetFinished(); diff --git a/sync/sync.gyp b/sync/sync.gyp index 7347348..0d32ffb 100644 --- a/sync/sync.gyp +++ b/sync/sync.gyp @@ -137,8 +137,6 @@ 'engine/traffic_recorder.h', 'engine/update_applicator.cc', 'engine/update_applicator.h', - 'engine/verify_updates_command.cc', - 'engine/verify_updates_command.h', 'js/js_arg_list.cc', 'js/js_arg_list.h', 'js/js_backend.h', @@ -616,7 +614,6 @@ 'engine/syncer_unittest.cc', 'engine/throttled_data_type_tracker_unittest.cc', 'engine/traffic_recorder_unittest.cc', - 'engine/verify_updates_command_unittest.cc', 'js/js_arg_list_unittest.cc', 'js/js_event_details_unittest.cc', 'js/sync_js_controller_unittest.cc', |