[Sync] USS: Enforce tag consistency in SMTP.

We have decided to move forward assuming the invariant that the hash of
a tag will always match its tag hash. This may be revisited in the
future if necessary for other data types (specifically bookmarks).

BUG=561818

Review URL: https://codereview.chromium.org/1744443002

Cr-Commit-Position: refs/heads/master@{#378077}

6 files changed, 68 insertions, 93 deletions

diff --git a/sync/internal_api/model_type_entity.cc b/sync/internal_api/model_type_entity.cc
index 19be3e6..d73a8f6 100644
--- a/sync/internal_api/model_type_entity.cc
+++ b/sync/internal_api/model_type_entity.cc
@@ -45,6 +45,7 @@ ModelTypeEntity::ModelTypeEntity(const std::string& client_tag,
     : client_tag_(client_tag),
       commit_requested_sequence_number_(metadata->acked_sequence_number()) {
   DCHECK(metadata->has_client_tag_hash());
+  DCHECK(metadata->has_creation_time());
   metadata_.Swap(metadata);
 }
 
@@ -87,7 +88,6 @@ void ModelTypeEntity::ApplyUpdateFromServer(
     const UpdateResponseData& response_data) {
   DCHECK(metadata_.has_client_tag_hash());
   DCHECK(!metadata_.client_tag_hash().empty());
-  DCHECK(metadata_.has_creation_time());
   DCHECK(metadata_.has_sequence_number());
 
   // TODO(stanisc): crbug/561829: Filter out update if specifics hash hasn't
@@ -109,24 +109,20 @@ void ModelTypeEntity::ApplyUpdateFromServer(
   encryption_key_name_ = response_data.encryption_key_name;
 }
 
-void ModelTypeEntity::MakeLocalChange(scoped_ptr<EntityData> entity_data,
-                                      base::Time modification_time) {
-  DCHECK(metadata_.has_client_tag_hash());
+void ModelTypeEntity::MakeLocalChange(scoped_ptr<EntityData> data) {
   DCHECK(!metadata_.client_tag_hash().empty());
-  DCHECK(metadata_.has_creation_time());
+  DCHECK_EQ(metadata_.client_tag_hash(), data->client_tag_hash);
+  DCHECK(!data->modification_time.is_null());
 
-  metadata_.set_modification_time(syncer::TimeToProtoTime(modification_time));
+  metadata_.set_modification_time(
+      syncer::TimeToProtoTime(data->modification_time));
   metadata_.set_is_deleted(false);
   IncrementSequenceNumber();
-  UpdateSpecificsHash(entity_data->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;
+  UpdateSpecificsHash(data->specifics);
 
-  CacheCommitData(entity_data.get());
+  data->id = metadata_.server_id();
+  data->creation_time = syncer::ProtoTimeToTime(metadata_.creation_time());
+  CacheCommitData(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 c1d0035..6f2856c 100644
--- a/sync/internal_api/model_type_entity_unittest.cc
+++ b/sync/internal_api/model_type_entity_unittest.cc
@@ -60,9 +60,11 @@ class ModelTypeEntityTest : public ::testing::Test {
   void MakeLocalChange(ModelTypeEntity* entity,
                        const sync_pb::EntitySpecifics& specifics) {
     scoped_ptr<EntityData> entity_data = make_scoped_ptr(new EntityData());
+    entity_data->client_tag_hash = entity->metadata().client_tag_hash();
     entity_data->specifics = specifics;
     entity_data->non_unique_name = "foo";
-    entity->MakeLocalChange(std::move(entity_data), kMtime);
+    entity_data->modification_time = kMtime;
+    entity->MakeLocalChange(std::move(entity_data));
   }
 
   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 f8204a2..733a148 100644
--- a/sync/internal_api/public/model_type_entity.h
+++ b/sync/internal_api/public/model_type_entity.h
@@ -77,8 +77,7 @@ class SYNC_EXPORT ModelTypeEntity {
   void ApplyUpdateFromServer(const UpdateResponseData& response_data);
 
   // Applies a local change to this item.
-  void MakeLocalChange(scoped_ptr<EntityData> entity_data,
-                       base::Time modification_time);
+  void MakeLocalChange(scoped_ptr<EntityData> data);
 
   // Schedule a commit if the |name| does not match this item's last known
   // encryption key.  The worker that performs the commit is expected to
diff --git a/sync/internal_api/public/shared_model_type_processor.h b/sync/internal_api/public/shared_model_type_processor.h
index d365f25..7ca4572 100644
--- a/sync/internal_api/public/shared_model_type_processor.h
+++ b/sync/internal_api/public/shared_model_type_processor.h
@@ -117,6 +117,9 @@ class SYNC_EXPORT SharedModelTypeProcessor : public ModelTypeProcessor,
   // Requires that no entity for |tag| already exists in the map.
   ModelTypeEntity* CreateEntity(const std::string& tag, const EntityData& data);
 
+  // Version of the above that generates a tag for |data|.
+  ModelTypeEntity* CreateEntity(const EntityData& data);
+
   syncer::ModelType type_;
   sync_pb::DataTypeState data_type_state_;
 
diff --git a/sync/internal_api/shared_model_type_processor.cc b/sync/internal_api/shared_model_type_processor.cc
index 0956151..757e12d 100644
--- a/sync/internal_api/shared_model_type_processor.cc
+++ b/sync/internal_api/shared_model_type_processor.cc
@@ -213,55 +213,46 @@ void SharedModelTypeProcessor::ConnectSync(scoped_ptr<CommitQueue> worker) {
   FlushPendingCommitRequests();
 }
 
-void SharedModelTypeProcessor::Put(const std::string& client_tag,
-                                   scoped_ptr<EntityData> entity_data,
+void SharedModelTypeProcessor::Put(const std::string& tag,
+                                   scoped_ptr<EntityData> data,
                                    MetadataChangeList* metadata_change_list) {
   DCHECK(IsAllowingChanges());
-  DCHECK(entity_data.get());
-  DCHECK(!entity_data->is_deleted());
-  DCHECK(!entity_data->non_unique_name.empty());
-  DCHECK_EQ(type_, syncer::GetModelTypeFromSpecifics(entity_data->specifics));
+  DCHECK(data.get());
+  DCHECK(!data->is_deleted());
+  DCHECK(!data->non_unique_name.empty());
+  DCHECK_EQ(type_, syncer::GetModelTypeFromSpecifics(data->specifics));
 
   if (!data_type_state_.initial_sync_done()) {
     // Ignore changes before the initial sync is done.
     return;
   }
 
-  // If the service specified an overriding hash, use that, otherwise generate
-  // one from the tag.
-  // TODO(skym): This behavior should be delayed, once crbug.com/561818 is fixed
-  // we will only perform this logic in the create case.
-  const std::string client_tag_hash(
-      entity_data->client_tag_hash.empty()
-          ? syncer::syncable::GenerateSyncableHash(type_, client_tag)
-          : entity_data->client_tag_hash);
+  // Fill in some data.
+  data->client_tag_hash = GetHashForTag(tag);
+  if (data->modification_time.is_null()) {
+    data->modification_time = base::Time::Now();
+  }
 
-  base::Time now = base::Time::Now();
+  ModelTypeEntity* entity = GetEntityForTagHash(data->client_tag_hash);
 
-  // TODO(stanisc): crbug.com/561818: Search by client_tag rather than
-  // client_tag_hash.
-  ModelTypeEntity* entity = GetEntityForTagHash(client_tag_hash);
   if (entity == nullptr) {
     // 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_[client_tag_hash] = std::move(scoped_entity);
-  } else {
-    // The service is updating an existing entity.
-    DCHECK_EQ(client_tag, entity->client_tag());
+    if (data->creation_time.is_null()) {
+      data->creation_time = data->modification_time;
+    }
+    entity = CreateEntity(tag, *data);
   }
 
   // TODO(stanisc): crbug.com/561829: Avoid committing a change if there is no
   // actual change.
-  entity->MakeLocalChange(std::move(entity_data), now);
-  metadata_change_list->UpdateMetadata(client_tag, entity->metadata());
+  entity->MakeLocalChange(std::move(data));
+  metadata_change_list->UpdateMetadata(tag, entity->metadata());
 
   FlushPendingCommitRequests();
 }
 
 void SharedModelTypeProcessor::Delete(
-    const std::string& client_tag,
+    const std::string& tag,
     MetadataChangeList* metadata_change_list) {
   DCHECK(IsAllowingChanges());
 
@@ -270,24 +261,17 @@ void SharedModelTypeProcessor::Delete(
     return;
   }
 
-  const std::string client_tag_hash(
-      syncer::syncable::GenerateSyncableHash(type_, client_tag));
-
-  // TODO(skym): crbug.com/561818: Search by client_tag rather than
-  // client_tag_hash.
-  ModelTypeEntity* entity = GetEntityForTagHash(client_tag_hash);
+  ModelTypeEntity* entity = GetEntityForTag(tag);
   if (entity == nullptr) {
     // That's unusual, but not necessarily a bad thing.
     // Missing is as good as deleted as far as the model is concerned.
     DLOG(WARNING) << "Attempted to delete missing item."
-                  << " client tag: " << client_tag;
+                  << " client tag: " << tag;
     return;
   }
 
   entity->Delete();
-
-  metadata_change_list->UpdateMetadata(client_tag, entity->metadata());
-
+  metadata_change_list->UpdateMetadata(tag, entity->metadata());
   FlushPendingCommitRequests();
 }
 
@@ -382,17 +366,9 @@ void SharedModelTypeProcessor::OnUpdateReceived(
         continue;
       }
 
-      // Let the service define |client_tag| based on the entity data.
-      const std::string client_tag = service_->GetClientTag(data);
-
-      scoped_ptr<ModelTypeEntity> scoped_entity = ModelTypeEntity::CreateNew(
-          client_tag, client_tag_hash, data.id, data.creation_time);
-      entity = scoped_entity.get();
-      entities_[client_tag_hash] = std::move(scoped_entity);
-
+      entity = CreateEntity(data);
       entity_changes.push_back(
-          EntityChange::CreateAdd(client_tag, update.entity));
-
+          EntityChange::CreateAdd(entity->client_tag(), update.entity));
     } else {
       if (data.is_deleted()) {
         entity_changes.push_back(
@@ -458,12 +434,8 @@ void SharedModelTypeProcessor::OnInitialUpdateReceived(
   metadata_changes->UpdateDataTypeState(data_type_state_);
 
   for (const UpdateResponseData& update : updates) {
-    const EntityData& data = update.entity.value();
-
-    // Let the service define a client |tag| based on the entity data.
-    std::string tag = service_->GetClientTag(data);
-
-    ModelTypeEntity* entity = CreateEntity(tag, data);
+    ModelTypeEntity* entity = CreateEntity(update.entity.value());
+    const std::string& tag = entity->client_tag();
     entity->ApplyUpdateFromServer(update);
     metadata_changes->UpdateMetadata(tag, entity->metadata());
     data_map[tag] = update.entity;
@@ -503,4 +475,13 @@ ModelTypeEntity* SharedModelTypeProcessor::CreateEntity(
   return entity_ptr;
 }
 
+ModelTypeEntity* SharedModelTypeProcessor::CreateEntity(
+    const EntityData& data) {
+  // Let the service define |client_tag| based on the entity data.
+  const std::string tag = service_->GetClientTag(data);
+  // This constraint may be relaxed in the future.
+  DCHECK_EQ(data.client_tag_hash, GetHashForTag(tag));
+  return CreateEntity(tag, data);
+}
+
 }  // namespace syncer_v2
diff --git a/sync/internal_api/shared_model_type_processor_unittest.cc b/sync/internal_api/shared_model_type_processor_unittest.cc
index 8e1c730..fb5363b 100644
--- a/sync/internal_api/shared_model_type_processor_unittest.cc
+++ b/sync/internal_api/shared_model_type_processor_unittest.cc
@@ -803,8 +803,6 @@ TEST_F(SharedModelTypeProcessorTest, CreateLocalItem) {
 
 // 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());
@@ -818,48 +816,44 @@ TEST_F(SharedModelTypeProcessorTest, CreateAndModifyWithOverrides) {
   entity_data->id = "cid1";
   WriteItem("tag1", std::move(entity_data));
 
-  // Don't access through tag because we forced a specific hash.
   EXPECT_EQ(1U, GetNumCommitRequestLists());
-  ASSERT_TRUE(mock_queue()->HasCommitRequestForTagHash("hash"));
+  ASSERT_FALSE(mock_queue()->HasCommitRequestForTagHash("hash"));
+  ASSERT_TRUE(HasCommitRequestForTag("tag1"));
+  EXPECT_EQ(1U, db().MetadataCount());
   const EntityData& out_entity1 =
-      mock_queue()->GetLatestCommitRequestForTagHash("hash").entity.value();
+      GetLatestCommitRequestForTag("tag1").entity.value();
+  const sync_pb::EntityMetadata metadata_v1 = db().GetMetadata("tag1");
 
-  EXPECT_EQ("hash", out_entity1.client_tag_hash);
   EXPECT_EQ("cid1", out_entity1.id);
+  EXPECT_NE("hash", out_entity1.client_tag_hash);
   EXPECT_EQ("value1", out_entity1.specifics.preference().value());
-
-  EXPECT_EQ(1U, db().MetadataCount());
-  const sync_pb::EntityMetadata metadata_v1 = db().GetMetadata("tag1");
   EXPECT_EQ("cid1", metadata_v1.server_id());
-  EXPECT_EQ("hash", metadata_v1.client_tag_hash());
+  EXPECT_EQ(metadata_v1.client_tag_hash(), out_entity1.client_tag_hash);
 
   entity_data.reset(new EntityData());
   entity_data->specifics.mutable_preference()->set_name("name2");
   entity_data->specifics.mutable_preference()->set_value("value2");
   entity_data->non_unique_name = "name2";
   entity_data->client_tag_hash = "hash";
-  // TODO(skym): Consider removing this. The ID should never be changed by the
-  // client once established.
+  // Make sure ID isn't overwritten either.
   entity_data->id = "cid2";
-
   WriteItem("tag1", std::move(entity_data));
 
   EXPECT_EQ(2U, GetNumCommitRequestLists());
-  ASSERT_TRUE(mock_queue()->HasCommitRequestForTagHash("hash"));
+  ASSERT_FALSE(mock_queue()->HasCommitRequestForTagHash("hash"));
+  ASSERT_TRUE(HasCommitRequestForTag("tag1"));
+  EXPECT_EQ(1U, db().MetadataCount());
   const EntityData& out_entity2 =
-      mock_queue()->GetLatestCommitRequestForTagHash("hash").entity.value();
+      GetLatestCommitRequestForTag("tag1").entity.value();
+  const sync_pb::EntityMetadata metadata_v2 = db().GetMetadata("tag1");
 
+  EXPECT_EQ("value2", out_entity2.specifics.preference().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());
-
-  EXPECT_EQ(1U, db().MetadataCount());
-  const sync_pb::EntityMetadata metadata_v2 = db().GetMetadata("tag1");
-  // TODO(skym): Is this correct?
   EXPECT_EQ("cid1", metadata_v2.server_id());
-  EXPECT_EQ("hash", metadata_v2.client_tag_hash());
+  EXPECT_EQ(metadata_v2.client_tag_hash(), out_entity2.client_tag_hash);
 
+  // Specifics have changed so the hashes should not match.
   EXPECT_NE(metadata_v1.specifics_hash(), metadata_v2.specifics_hash());
 }