[sync] Add support for bookmark client tags in loopback server

The general idea is that, if a client tag is provided upon *creation* of
a bookmark, it should be delivered back to clients during later
GetUpdates.

In order to do so, PersistentBookmarkEntity is extended with a new
member field to accommodate the client tag hash, and the corresponding
conversion to/from protos.

Bug: 1032052
Change-Id: Ie5fe597abb6b913da46a293a5f7bdee4446fcf6a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2624467Reviewed-by: Rushan Suleymanov <rushans@google.com>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#843151}

components/sync/engine_impl/loopback_server/loopback_server_unittest.cc

@@ -6,6 +6,7 @@
 #include "base/files/file_util.h"
 #include "base/guid.h"
 #include "base/test/task_environment.h"
+#include "components/sync/base/client_tag_hash.h"
 #include "components/sync/engine_impl/loopback_server/loopback_connection_manager.h"
 #include "components/sync/engine_impl/syncer_proto_util.h"
 #include "components/sync/protocol/sync.pb.h"
@@ -273,4 +274,54 @@ TEST_F(LoopbackServerTest, CommitCommandUpdate) {
   EXPECT_EQ(kUrl2, ResponseToMap(response)[id].specifics().bookmark().url());
 }
 
+TEST_F(LoopbackServerTest, CommitBookmarkCreationWithClientTag) {
+  const std::string kGuid = base::GenerateGUID();
+  const std::string kClientTagHash =
+      ClientTagHash::FromUnhashed(BOOKMARKS, kGuid).value();
+
+  SyncEntity entity;
+  entity.mutable_specifics()->mutable_bookmark()->set_url(kUrl1);
+  entity.set_parent_id_string(kBookmarkBar);
+  entity.set_id_string(kGuid);
+  entity.set_client_defined_unique_tag(kClientTagHash);
+
+  const std::string id = CommitVerifySuccess(entity);
+
+  std::map<std::string, SyncEntity> bookmarks =
+      ResponseToMap(GetUpdatesForType(EntitySpecifics::kBookmarkFieldNumber));
+  EXPECT_EQ(bookmarks[id].client_defined_unique_tag(), kClientTagHash);
+}
+
+// Verifies that a bookmark update (non-creation) does not populate the client
+// tag of a bookmark, if no client tag was provided upon creation.
+TEST_F(LoopbackServerTest, CommitBookmarkUpdateWithClientTag) {
+  const std::string kGuid = base::GenerateGUID();
+  const std::string kClientTagHash =
+      ClientTagHash::FromUnhashed(BOOKMARKS, kGuid).value();
+
+  SyncEntity entity;
+  entity.mutable_specifics()->mutable_bookmark()->set_url(kUrl1);
+  entity.set_parent_id_string(kBookmarkBar);
+  entity.set_id_string(kGuid);
+
+  const std::string id = CommitVerifySuccess(entity);
+
+  std::map<std::string, SyncEntity> bookmarks =
+      ResponseToMap(GetUpdatesForType(EntitySpecifics::kBookmarkFieldNumber));
+  ASSERT_EQ(bookmarks[id].specifics().bookmark().url(), kUrl1);
+  ASSERT_FALSE(bookmarks[id].has_client_defined_unique_tag());
+
+  // Issue an update, with the client tag being provided for the first time.
+  entity.set_id_string(id);
+  entity.set_client_defined_unique_tag(kClientTagHash);
+  entity.set_version(1);
+  entity.mutable_specifics()->mutable_bookmark()->set_url(kUrl2);
+  CommitVerifySuccess(entity);
+
+  bookmarks =
+      ResponseToMap(GetUpdatesForType(EntitySpecifics::kBookmarkFieldNumber));
+  ASSERT_EQ(bookmarks[id].specifics().bookmark().url(), kUrl2);
+  EXPECT_FALSE(bookmarks[id].has_client_defined_unique_tag());
+}
+
 }  // namespace syncer

components/sync/engine_impl/loopback_server/persistent_bookmark_entity.cc

@@ -8,6 +8,7 @@
 
 #include "base/guid.h"
 #include "base/logging.h"
+#include "components/sync/base/client_tag_hash.h"
 
 using std::string;
 
@@ -46,9 +47,10 @@ std::unique_ptr<LoopbackServerEntity> PersistentBookmarkEntity::CreateNew(
 
   return std::make_unique<PersistentBookmarkEntity>(
       id, 0, client_entity.name(), originator_cache_guid,
-      originator_client_item_id, client_entity.unique_position(),
-      client_entity.specifics(), client_entity.folder(), parent_id,
-      client_entity.ctime(), client_entity.mtime());
+      originator_client_item_id, client_entity.client_defined_unique_tag(),
+      client_entity.unique_position(), client_entity.specifics(),
+      client_entity.folder(), parent_id, client_entity.ctime(),
+      client_entity.mtime());
 }
 
 // static
@@ -70,12 +72,16 @@ PersistentBookmarkEntity::CreateUpdatedVersion(
 
   std::string originator_cache_guid;
   std::string originator_client_item_id;
+  std::string client_tag_hash;
 
   if (current_server_entity.IsDeleted()) {
     // Handle undeletions.
     originator_cache_guid = updating_client_cache_guid;
     originator_client_item_id =
         LoopbackServerEntity::GetInnerIdFromId(client_entity.id_string());
+    // An undeletion is similar to a creation, so let's honor the client tag
+    // provided by the client (if any).
+    client_tag_hash = client_entity.client_defined_unique_tag();
   } else {
     // Regular case (non-undeletion).
     const PersistentBookmarkEntity& current_bookmark_entity =
@@ -83,15 +89,19 @@ PersistentBookmarkEntity::CreateUpdatedVersion(
     originator_cache_guid = current_bookmark_entity.originator_cache_guid_;
     originator_client_item_id =
         current_bookmark_entity.originator_client_item_id_;
+    // Note that the client tag provided by the client in |client_entity| is
+    // ignored during non-creations updates, since it's meant to be immutable.
+    client_tag_hash = current_bookmark_entity.client_tag_hash_;
   }
 
   // Using a version of 0 is okay here as it'll be updated before this entity is
   // actually saved.
   return std::make_unique<PersistentBookmarkEntity>(
       client_entity.id_string(), 0, client_entity.name(), originator_cache_guid,
-      originator_client_item_id, client_entity.unique_position(),
-      client_entity.specifics(), client_entity.folder(), parent_id,
-      client_entity.ctime(), client_entity.mtime());
+      originator_client_item_id, client_tag_hash,
+      client_entity.unique_position(), client_entity.specifics(),
+      client_entity.folder(), parent_id, client_entity.ctime(),
+      client_entity.mtime());
 }
 
 // static
@@ -107,6 +117,7 @@ PersistentBookmarkEntity::CreateFromEntity(
       client_entity.id_string(), client_entity.version(), client_entity.name(),
       client_entity.originator_cache_guid(),
       client_entity.originator_client_item_id(),
+      client_entity.client_defined_unique_tag(),
       client_entity.unique_position(), client_entity.specifics(),
       client_entity.folder(), client_entity.parent_id_string(),
       client_entity.ctime(), client_entity.mtime());
@@ -117,6 +128,7 @@ PersistentBookmarkEntity::PersistentBookmarkEntity(
     const string& name,
     const string& originator_cache_guid,
     const string& originator_client_item_id,
+    const string& client_tag_hash,
     const sync_pb::UniquePosition& unique_position,
     const sync_pb::EntitySpecifics& specifics,
     bool is_folder,
@@ -126,11 +138,20 @@ PersistentBookmarkEntity::PersistentBookmarkEntity(
     : LoopbackServerEntity(id, syncer::BOOKMARKS, version, name),
       originator_cache_guid_(originator_cache_guid),
       originator_client_item_id_(originator_client_item_id),
+      client_tag_hash_(client_tag_hash),
       is_folder_(is_folder),
       unique_position_(unique_position),
       parent_id_(parent_id),
       creation_time_(creation_time),
       last_modified_time_(last_modified_time) {
+  if (!client_tag_hash.empty()) {
+    // This relies technically on a well-behaving client, but verifying here to
+    // avoid issues with Local Sync, which uses LoopbackServer.
+    DCHECK_EQ(
+        ClientTagHash::FromHashed(client_tag_hash),
+        ClientTagHash::FromUnhashed(BOOKMARKS, originator_client_item_id));
+  }
+
   SetSpecifics(specifics);
 }
 
@@ -156,6 +177,10 @@ void PersistentBookmarkEntity::SerializeAsProto(
     sync_pb::SyncEntity* proto) const {
   LoopbackServerEntity::SerializeBaseProtoFields(proto);
 
+  if (!client_tag_hash_.empty()) {
+    proto->set_client_defined_unique_tag(client_tag_hash_);
+  }
+
   proto->set_originator_cache_guid(originator_cache_guid_);
   proto->set_originator_client_item_id(originator_client_item_id_);
 

components/sync/engine_impl/loopback_server/persistent_bookmark_entity.h

@@ -50,6 +50,7 @@ class PersistentBookmarkEntity : public LoopbackServerEntity {
                            const std::string& name,
                            const std::string& originator_cache_guid,
                            const std::string& originator_client_item_id,
+                           const std::string& client_tag_hash,
                            const sync_pb::UniquePosition& unique_position,
                            const sync_pb::EntitySpecifics& specifics,
                            bool is_folder,
@@ -71,6 +72,10 @@ class PersistentBookmarkEntity : public LoopbackServerEntity {
   // All member values have equivalent fields in SyncEntity.
   const std::string originator_cache_guid_;
   const std::string originator_client_item_id_;
+  // Whether a client tag hash was provided upon *creation*, or an empty string
+  // otherwise. Takes precedence over the two fields above when exposing the
+  // entity in the protocol.
+  const std::string client_tag_hash_;
   const bool is_folder_;
   sync_pb::UniquePosition unique_position_;
   std::string parent_id_;

components/sync/test/fake_server/bookmark_entity_builder.cc

@@ -128,8 +128,8 @@ std::unique_ptr<LoopbackServerEntity> BookmarkEntityBuilder::Build(
   return base::WrapUnique<LoopbackServerEntity>(
       new syncer::PersistentBookmarkEntity(
           id_, kUnusedVersion, title_, originator_cache_guid_,
-          originator_client_item_id_, unique_position, entity_specifics,
-          is_folder, parent_id_, kDefaultTime, kDefaultTime));
+          originator_client_item_id_, /*client_tag_hash=*/"", unique_position,
+          entity_specifics, is_folder, parent_id_, kDefaultTime, kDefaultTime));
 }
 
 }  // namespace fake_server