diff options
Diffstat (limited to 'sync/internal_api')
-rw-r--r-- | sync/internal_api/model_type_entity.cc | 23 | ||||
-rw-r--r-- | sync/internal_api/model_type_entity_unittest.cc | 5 | ||||
-rw-r--r-- | sync/internal_api/public/model_type_entity.h | 3 | ||||
-rw-r--r-- | sync/internal_api/public/shared_model_type_processor.h | 3 | ||||
-rw-r--r-- | sync/internal_api/shared_model_type_processor.cc | 31 | ||||
-rw-r--r-- | sync/internal_api/shared_model_type_processor_unittest.cc | 66 |
6 files changed, 96 insertions, 35 deletions
diff --git a/sync/internal_api/model_type_entity.cc b/sync/internal_api/model_type_entity.cc index eabdd06..cbfe00c 100644 --- a/sync/internal_api/model_type_entity.cc +++ b/sync/internal_api/model_type_entity.cc @@ -92,8 +92,7 @@ void ModelTypeEntity::ApplyUpdateFromServer( encryption_key_name_ = response_data.encryption_key_name; } -void ModelTypeEntity::MakeLocalChange(const std::string& non_unique_name, - const sync_pb::EntitySpecifics& specifics, +void ModelTypeEntity::MakeLocalChange(scoped_ptr<EntityData> entity_data, base::Time modification_time) { DCHECK(metadata_.has_client_tag_hash()); DCHECK(!metadata_.client_tag_hash().empty()); @@ -102,21 +101,15 @@ void ModelTypeEntity::MakeLocalChange(const std::string& non_unique_name, metadata_.set_modification_time(syncer::TimeToProtoTime(modification_time)); metadata_.set_is_deleted(false); IncrementSequenceNumber(); - UpdateSpecificsHash(specifics); + UpdateSpecificsHash(entity_data->specifics); - // Build and cache commit data. - // TODO(stanisc): Consider moving this out to make caching the data - // optional. - EntityData data; - data.client_tag_hash = metadata_.client_tag_hash(); - data.id = metadata_.server_id(); - data.non_unique_name = non_unique_name; - data.creation_time = syncer::ProtoTimeToTime(metadata_.creation_time()); - data.modification_time = modification_time; - // TODO(stanisc): Consider taking over specifics value without copying. - data.specifics.CopyFrom(specifics); + entity_data->client_tag_hash = metadata_.client_tag_hash(); + entity_data->id = metadata_.server_id(); + entity_data->creation_time = + syncer::ProtoTimeToTime(metadata_.creation_time()); + entity_data->modification_time = modification_time; - CacheCommitData(&data); + CacheCommitData(entity_data.get()); } void ModelTypeEntity::UpdateDesiredEncryptionKey(const std::string& name) { diff --git a/sync/internal_api/model_type_entity_unittest.cc b/sync/internal_api/model_type_entity_unittest.cc index 0af2b58..08de2fc 100644 --- a/sync/internal_api/model_type_entity_unittest.cc +++ b/sync/internal_api/model_type_entity_unittest.cc @@ -57,7 +57,10 @@ class ModelTypeEntityTest : public ::testing::Test { void MakeLocalChange(ModelTypeEntity* entity, const sync_pb::EntitySpecifics& specifics) { - entity->MakeLocalChange("foo", specifics, kMtime); + scoped_ptr<EntityData> entity_data = make_scoped_ptr(new EntityData()); + entity_data->specifics = specifics; + entity_data->non_unique_name = "foo"; + entity->MakeLocalChange(entity_data.Pass(), kMtime); } scoped_ptr<ModelTypeEntity> NewServerItem() { diff --git a/sync/internal_api/public/model_type_entity.h b/sync/internal_api/public/model_type_entity.h index 3d0a074..551f1bd 100644 --- a/sync/internal_api/public/model_type_entity.h +++ b/sync/internal_api/public/model_type_entity.h @@ -65,8 +65,7 @@ class SYNC_EXPORT ModelTypeEntity { void ApplyUpdateFromServer(const UpdateResponseData& response_data); // Applies a local change to this item. - void MakeLocalChange(const std::string& non_unique_name, - const sync_pb::EntitySpecifics& specifics, + void MakeLocalChange(scoped_ptr<EntityData> entity_data, base::Time modification_time); // Schedule a commit if the |name| does not match this item's last known diff --git a/sync/internal_api/public/shared_model_type_processor.h b/sync/internal_api/public/shared_model_type_processor.h index 88adbcc..eb47d93 100644 --- a/sync/internal_api/public/shared_model_type_processor.h +++ b/sync/internal_api/public/shared_model_type_processor.h @@ -65,8 +65,7 @@ class SYNC_EXPORT SharedModelTypeProcessor : public ModelTypeProcessor, // ModelTypeChangeProcessor implementation. void Put(const std::string& client_key, - const std::string& non_unique_name, - const sync_pb::EntitySpecifics& specifics, + scoped_ptr<EntityData> entity_data, MetadataChangeList* metadata_change_list) override; void Delete(const std::string& client_key, MetadataChangeList* metadata_change_list) override; diff --git a/sync/internal_api/shared_model_type_processor.cc b/sync/internal_api/shared_model_type_processor.cc index 97cf8e5..15edd30 100644 --- a/sync/internal_api/shared_model_type_processor.cc +++ b/sync/internal_api/shared_model_type_processor.cc @@ -149,16 +149,23 @@ void SharedModelTypeProcessor::OnConnect(scoped_ptr<CommitQueue> worker) { } void SharedModelTypeProcessor::Put(const std::string& client_tag, - const std::string& non_unique_name, - const sync_pb::EntitySpecifics& specifics, + scoped_ptr<EntityData> entity_data, MetadataChangeList* metadata_change_list) { - // TODO(skym): Update for new approach. Different objects, different caching, - // different loopups, metadata_change_list, etc. + // TODO(skym): Add metadata to persist to MetadataChangeList, crbug/569636. - DCHECK_EQ(type_, syncer::GetModelTypeFromSpecifics(specifics)); + DCHECK(entity_data.get()); + DCHECK(!entity_data->is_deleted()); + DCHECK(!entity_data->non_unique_name.empty()); + DCHECK_EQ(type_, syncer::GetModelTypeFromSpecifics(entity_data->specifics)); + // If the service specified an overriding hash, use that, otherwise generate + // one from the tag. TODO(skym): This behavior should be delayed, once + // crbug/561818 is fixed we will only perform this logic in the create case. const std::string client_tag_hash( - syncer::syncable::GenerateSyncableHash(type_, client_tag)); + entity_data->client_tag_hash.empty() + ? syncer::syncable::GenerateSyncableHash(type_, client_tag) + : entity_data->client_tag_hash); + base::Time now = base::Time::Now(); ModelTypeEntity* entity = nullptr; @@ -166,28 +173,30 @@ void SharedModelTypeProcessor::Put(const std::string& client_tag, // client_tag_hash. EntityMap::const_iterator it = entities_.find(client_tag_hash); if (it == entities_.end()) { - scoped_ptr<ModelTypeEntity> scoped_entity = - ModelTypeEntity::CreateNew(client_tag, client_tag_hash, "", now); + // The service is creating a new entity. + scoped_ptr<ModelTypeEntity> scoped_entity = ModelTypeEntity::CreateNew( + client_tag, client_tag_hash, entity_data->id, now); entity = scoped_entity.get(); entities_.insert( std::make_pair(client_tag_hash, std::move(scoped_entity))); } else { + // The service is updating an existing entity. entity = it->second.get(); } - entity->MakeLocalChange(non_unique_name, specifics, now); + entity->MakeLocalChange(entity_data.Pass(), now); FlushPendingCommitRequests(); } void SharedModelTypeProcessor::Delete( const std::string& client_key, MetadataChangeList* metadata_change_list) { - // TODO(skym): Update for new approach. Different caching, different lookup, - // metadata changes. + // TODO(skym): Add metadata to persist to MetadataChangeList, crbug/569636. const std::string client_tag_hash( syncer::syncable::GenerateSyncableHash(type_, client_key)); + // TODO(skym): crbug/561818: Search by client_tag rather than client_tag_hash. EntityMap::const_iterator it = entities_.find(client_tag_hash); if (it == entities_.end()) { // That's unusual, but not necessarily a bad thing. diff --git a/sync/internal_api/shared_model_type_processor_unittest.cc b/sync/internal_api/shared_model_type_processor_unittest.cc index 6bbed17..23506a82 100644 --- a/sync/internal_api/shared_model_type_processor_unittest.cc +++ b/sync/internal_api/shared_model_type_processor_unittest.cc @@ -116,6 +116,9 @@ class SharedModelTypeProcessorTest : public ::testing::Test, // when receiving items. void SetServerEncryptionKey(const std::string& key_name); + MockCommitQueue* mock_queue(); + SharedModelTypeProcessor* type_processor(); + private: static std::string GenerateTagHash(const std::string& tag); static sync_pb::EntitySpecifics GenerateSpecifics(const std::string& tag, @@ -197,16 +200,17 @@ void SharedModelTypeProcessorTest::StartDone( // Hand off ownership of |mock_queue_ptr_|, while keeping // an unsafe pointer to it. This is why we can only connect once. DCHECK(mock_queue_ptr_); - context->type_processor->OnConnect(mock_queue_ptr_.Pass()); + context->type_processor->OnConnect(std::move(mock_queue_ptr_)); // The context's type processor is a proxy; run the task it posted. sync_loop_.RunUntilIdle(); } void SharedModelTypeProcessorTest::WriteItem(const std::string& tag, const std::string& value) { - sync_pb::EntitySpecifics specifics = GenerateSpecifics(tag, value); - // Use the tag as non-unique name. - type_processor_->Put(tag, tag, specifics, nullptr); + scoped_ptr<EntityData> entity_data = make_scoped_ptr(new EntityData()); + entity_data->specifics = GenerateSpecifics(tag, value); + entity_data->non_unique_name = tag; + type_processor_->Put(tag, std::move(entity_data), nullptr); } void SharedModelTypeProcessorTest::DeleteItem(const std::string& tag) { @@ -313,6 +317,14 @@ void SharedModelTypeProcessorTest::SetServerEncryptionKey( mock_queue_->SetServerEncryptionKey(key_name); } +MockCommitQueue* SharedModelTypeProcessorTest::mock_queue() { + return mock_queue_; +} + +SharedModelTypeProcessor* SharedModelTypeProcessorTest::type_processor() { + return type_processor_.get(); +} + std::string SharedModelTypeProcessorTest::GenerateTagHash( const std::string& tag) { return syncer::syncable::GenerateSyncableHash(kModelType, tag); @@ -393,6 +405,52 @@ TEST_F(SharedModelTypeProcessorTest, CreateLocalItem) { EXPECT_EQ("value1", tag1_data.specifics.preference().value()); } +// The purpose of this test case is to test setting |client_tag_hash| and |id| +// on the EntityData object as we pass it into the Put method of the processor. +// TODO(skym): Update this test to verify that specifying a |client_tag_hash| +// on update has no effect once crbug/561818 is completed. +TEST_F(SharedModelTypeProcessorTest, CreateAndModifyWithOverrides) { + InitializeToReadyState(); + EXPECT_EQ(0U, GetNumCommitRequestLists()); + + scoped_ptr<EntityData> entity_data = make_scoped_ptr(new EntityData()); + entity_data->specifics.mutable_preference()->set_name("tag1"); + entity_data->specifics.mutable_preference()->set_value("value1"); + + entity_data->non_unique_name = "tag1"; + entity_data->client_tag_hash = "hash"; + entity_data->id = "cid1"; + type_processor()->Put("tag1", std::move(entity_data), nullptr); + + // Don't access through tag because we forced a specific hash. + EXPECT_EQ(1U, GetNumCommitRequestLists()); + ASSERT_TRUE(mock_queue()->HasCommitRequestForTagHash("hash")); + const EntityData& out_entity1 = + mock_queue()->GetLatestCommitRequestForTagHash("hash").entity.value(); + + EXPECT_EQ("hash", out_entity1.client_tag_hash); + EXPECT_EQ("cid1", out_entity1.id); + EXPECT_EQ("value1", out_entity1.specifics.preference().value()); + + entity_data.reset(new EntityData()); + entity_data->specifics.mutable_preference()->set_name("tag2"); + entity_data->specifics.mutable_preference()->set_value("value2"); + entity_data->non_unique_name = "tag2"; + entity_data->client_tag_hash = "hash"; + entity_data->id = "cid2"; + type_processor()->Put("tag2", std::move(entity_data), nullptr); + + EXPECT_EQ(2U, GetNumCommitRequestLists()); + ASSERT_TRUE(mock_queue()->HasCommitRequestForTagHash("hash")); + const EntityData& out_entity2 = + mock_queue()->GetLatestCommitRequestForTagHash("hash").entity.value(); + + // Should still see old cid1 value, override is not respected on update. + EXPECT_EQ("hash", out_entity2.client_tag_hash); + EXPECT_EQ("cid1", out_entity2.id); + EXPECT_EQ("value2", out_entity2.specifics.preference().value()); +} + // Creates a new local item then modifies it. // Thoroughly tests data generated by modification of server-unknown item. TEST_F(SharedModelTypeProcessorTest, CreateAndModifyLocalItem) { |