Create client tags for Wallet data in the processor

The client_tag_based_model_type_processor requires incoming data to
have client tag hash field set on each entity. Wallet data does not
have this field set, because the data comes from the Wallet server, as
opposed to from other clients. This makes the processor drop all
incoming updates on the ground (this code introduced here), which means
there's no data for the user.

This CL makes the bridge to create client_tags for Wallet items that
don't have the tag set yet.

A future, longer term solutions would be to either make the server set
the client tags, or to use a different processor for wallet data.

BUG=874001

Change-Id: Ie4331c3bd601a43c59fc35ea94d271db2db66ec8
Reviewed-on: https://chromium-review.googlesource.com/1174435
Commit-Queue: Florian Uunk <feuunk@chromium.org>
Reviewed-by: Tim Schumann <tschumann@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584766}

components/sync/model/entity_data.cc

@@ -76,6 +76,15 @@ EntityDataPtr EntityData::UpdateId(const std::string& new_id) const {
   return target;
 }
 
+EntityDataPtr EntityData::UpdateClientTagHash(
+    const std::string& new_client_tag_hash) const {
+  EntityData entity_data(*this);
+  entity_data.client_tag_hash = new_client_tag_hash;
+  EntityDataPtr target;
+  target.swap_value(&entity_data);
+  return target;
+}
+
 EntityDataPtr EntityData::UpdateSpecifics(
     const sync_pb::EntitySpecifics& new_specifics) const {
   EntityData entity_data(*this);

components/sync/model/entity_data.h

@@ -94,6 +94,14 @@ struct EntityData {
   // local change is cached in ProcessorEntityTracker.
   EntityDataPtr UpdateId(const std::string& new_id) const WARN_UNUSED_RESULT;
 
+  // Makes a copy of EntityData and updates its client tag hash to
+  // |new_client_tag_hash|. This is needed when a Wallet entity id is updated
+  // and it has no client_tag_hash.
+  // TODO(crbug.com/874001): Remove this when we have a longer term fix for
+  // wallet data.
+  EntityDataPtr UpdateClientTagHash(
+      const std::string& new_client_tag_hash) const WARN_UNUSED_RESULT;
+
   // Makes a copy of EntityData and updates its specifics to |new_specifics|.
   // This is needed when specifics is updated after decryption in the
   // ModelTypeWorker::DecryptStoredEntities().

components/sync/model/entity_data_unittest.cc

@@ -66,4 +66,12 @@ TEST_F(EntityDataTest, Swap) {
   EXPECT_EQ(false, data.is_folder);
 }
 
+TEST_F(EntityDataTest, UpdateClientTagHash) {
+  EntityData data;
+  ASSERT_TRUE(data.client_tag_hash.empty());
+
+  EntityDataPtr new_data(data.UpdateClientTagHash("test!"));
+  EXPECT_EQ("test!", new_data->client_tag_hash);
+}
+
 }  // namespace syncer

components/sync/model_impl/client_tag_based_model_type_processor.cc

@@ -530,7 +530,34 @@ void ClientTagBasedModelTypeProcessor::OnCommitCompleted(
 void ClientTagBasedModelTypeProcessor::OnUpdateReceived(
     const sync_pb::ModelTypeState& model_type_state,
     const UpdateResponseDataList& updates) {
+  if (type_ == AUTOFILL_WALLET_DATA) {
+    // The client tag based processor requires client tags to function properly.
+    // However, the wallet data type does not have any client tags. This hacky
+    // code manually asks the bridge to create the client tags for each update,
+    // so that we can still use this processor. A proper fix would be to either
+    // fully use client tags, or to use a different processor.
+    // TODO(crbug.com/874001): Remove this feature-specific logic when the right
+    // solution for Wallet data has been decided.
+    // TODO(crbug.com/874001): Restructure this method so we don't need the
+    // OnUpdateReceivedInternal method if creating tags on the client is the way
+    // we want to move forward.
+    UpdateResponseDataList updates_with_client_tags;
+    for (UpdateResponseData update : updates) {
+      update.entity = update.entity->UpdateClientTagHash(
+          GetHashForTag(bridge_->GetClientTag(update.entity.value())));
+      updates_with_client_tags.push_back(update);
+    }
+    OnUpdateReceivedInternal(model_type_state, updates_with_client_tags);
+  } else {
+    OnUpdateReceivedInternal(model_type_state, updates);
+  }
+}
+
+void ClientTagBasedModelTypeProcessor::OnUpdateReceivedInternal(
+    const sync_pb::ModelTypeState& model_type_state,
+    const UpdateResponseDataList& updates) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
   if (!model_type_state_.initial_sync_done()) {
     OnInitialUpdateReceived(model_type_state, updates);
     return;

components/sync/model_impl/client_tag_based_model_type_processor.h

@@ -129,6 +129,12 @@ class ClientTagBasedModelTypeProcessor : public ModelTypeProcessor,
   void RecommitAllForEncryption(std::unordered_set<std::string> already_updated,
                                 MetadataChangeList* metadata_changes);
 
+  // Does the actual handling of updates, without changing client tags.
+  // TODO(crbug.com/874001): Merge this back into OnUpdateReceived when the
+  // right solution for Wallet data has been decided.
+  void OnUpdateReceivedInternal(const sync_pb::ModelTypeState& type_state,
+                                const UpdateResponseDataList& updates);
+
   // Handle the first update received from the server after being enabled.
   void OnInitialUpdateReceived(const sync_pb::ModelTypeState& type_state,
                                const UpdateResponseDataList& updates);

components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc

@@ -71,16 +71,17 @@ void CaptureStatusCounters(StatusCounters* dst,
 
 class TestModelTypeSyncBridge : public FakeModelTypeSyncBridge {
  public:
-  explicit TestModelTypeSyncBridge(bool commit_only)
+  explicit TestModelTypeSyncBridge(bool commit_only, ModelType model_type)
       : FakeModelTypeSyncBridge(
             std::make_unique<ClientTagBasedModelTypeProcessor>(
-                PREFERENCES,
+                model_type,
                 /*dump_stack=*/base::RepeatingClosure(),
                 commit_only)) {}
 
   TestModelTypeSyncBridge(std::unique_ptr<TestModelTypeSyncBridge> other,
-                          bool commit_only)
-      : TestModelTypeSyncBridge(commit_only) {
+                          bool commit_only,
+                          ModelType model_type)
+      : TestModelTypeSyncBridge(commit_only, model_type) {
     std::swap(db_, other->db_);
   }
 
@@ -185,7 +186,8 @@ class ClientTagBasedModelTypeProcessorTest : public ::testing::Test {
   ~ClientTagBasedModelTypeProcessorTest() override { CheckPostConditions(); }
 
   void SetUp() override {
-    bridge_ = std::make_unique<TestModelTypeSyncBridge>(IsCommitOnly());
+    bridge_ = std::make_unique<TestModelTypeSyncBridge>(IsCommitOnly(),
+                                                        GetModelType());
   }
 
   void InitializeToMetadataLoaded() {
@@ -251,16 +253,18 @@ class ClientTagBasedModelTypeProcessorTest : public ::testing::Test {
   }
 
   void ResetState(bool keep_db) {
-    bridge_ = keep_db
-                  ? std::make_unique<TestModelTypeSyncBridge>(
-                        std::move(bridge_), IsCommitOnly())
-                  : std::make_unique<TestModelTypeSyncBridge>(IsCommitOnly());
+    bridge_ = keep_db ? std::make_unique<TestModelTypeSyncBridge>(
+                            std::move(bridge_), IsCommitOnly(), GetModelType())
+                      : std::make_unique<TestModelTypeSyncBridge>(
+                            IsCommitOnly(), GetModelType());
     worker_ = nullptr;
     CheckPostConditions();
   }
 
   virtual bool IsCommitOnly() { return false; }
 
+  virtual ModelType GetModelType() { return PREFERENCES; }
+
   // Wipes existing DB and simulates a pending update of a server-known item.
   EntitySpecifics ResetStateWriteItem(const std::string& name,
                                       const std::string& value) {
@@ -1431,6 +1435,46 @@ TEST_F(ClientTagBasedModelTypeProcessorTest, IgnoreLocalEncryption) {
   EXPECT_EQ(1U, worker()->GetNumPendingCommits());
 }
 
+// Tests that updates without client tags get dropped.
+TEST_F(ClientTagBasedModelTypeProcessorTest, DropsClientTags) {
+  InitializeToReadyState();
+  UpdateResponseDataList updates;
+  updates.push_back(worker()->GenerateUpdateData(
+      /*tag_hash=*/"", GenerateSpecifics(kKey1, kValue1), 1, "k1"));
+
+  worker()->UpdateFromServer(updates);
+
+  // Verify that the data wasn't actually stored.
+  EXPECT_EQ(0U, db().metadata_count());
+  EXPECT_EQ(0U, db().data_count());
+}
+
+class WalletDataClientTagBasedModelTypeProcessorTest
+    : public ClientTagBasedModelTypeProcessorTest {
+ protected:
+  ModelType GetModelType() override { return AUTOFILL_WALLET_DATA; }
+};
+
+// Tests that updates for Wallet data without client tags get client tags
+// assigned, and not dropped.
+// TODO(crbug.com/874001): Remove this feature-specific logic when the right
+// solution for Wallet data has been decided.
+TEST_F(WalletDataClientTagBasedModelTypeProcessorTest,
+       CreatesClientTagsForWallet) {
+  InitializeToReadyState();
+
+  // Commit an item.
+  UpdateResponseDataList updates;
+  updates.push_back(worker()->GenerateUpdateData(
+      /*tag_hash=*/"", GenerateSpecifics(kKey1, kValue1), 1, "k1"));
+  worker()->UpdateFromServer(updates);
+
+  // Verify that the data was stored.
+  EXPECT_EQ(1U, db().metadata_count());
+  EXPECT_EQ(1U, db().data_count());
+  EXPECT_FALSE(db().GetMetadata(kKey1).client_tag_hash().empty());
+}
+
 // Tests that a real local change wins over a remote encryption-only change.
 TEST_F(ClientTagBasedModelTypeProcessorTest, IgnoreRemoteEncryption) {
   InitializeToReadyState();