Merge pull request #4710 from brave/sync_fix_obj_id_dup

Sync fix bookmarks object id duplication

Author: AlexeyBarabash

browser/profiles/brave_bookmark_model_loaded_observer.cc

@@ -31,8 +31,11 @@ void BraveBookmarkModelLoadedObserver::BookmarkModelLoaded(
   // it is handled in BraveProfileSyncServiceImpl::OnSyncReady
   if (brave_profile_service && !brave_profile_service->IsBraveSyncEnabled())
     BraveMigrateOtherNode(model);
+
+  BraveProfileSyncServiceImpl::AddNonClonedBookmarkKeys(model);
 #else
   BraveMigrateOtherNode(model);
 #endif
+
   BookmarkModelLoadedObserver::BookmarkModelLoaded(model, ids_reassigned);
 }

components/brave_sync/brave_profile_sync_service_impl.cc

@@ -516,6 +516,17 @@ void BraveProfileSyncServiceImpl::OnSyncReadyBookmarksModelLoaded() {
   BraveMigrateOtherNode(model_);
 }
 
+// static
+void BraveProfileSyncServiceImpl::AddNonClonedBookmarkKeys(
+    BookmarkModel* model) {
+  DCHECK(model);
+  DCHECK(model->loaded());
+  model->AddNonClonedKey("object_id");
+  model->AddNonClonedKey("order");
+  model->AddNonClonedKey("sync_timestamp");
+  model->AddNonClonedKey("version");
+}
+
 syncer::ModelTypeSet BraveProfileSyncServiceImpl::GetPreferredDataTypes()
     const {
   // Force DEVICE_INFO type to have nudge cycle each time to fetch

components/brave_sync/brave_profile_sync_service_impl.h

@@ -170,6 +170,8 @@ class BraveProfileSyncServiceImpl
   BraveSyncClient* GetBraveSyncClient() override;
 #endif
 
+  static void AddNonClonedBookmarkKeys(BookmarkModel* model);
+
   bool IsBraveSyncEnabled() const override;
 
   syncer::ModelTypeSet GetPreferredDataTypes() const override;

components/brave_sync/brave_sync_service_unittest.cc

@@ -99,6 +99,7 @@ using brave_sync::RecordsList;
 using brave_sync::SimpleBookmarkSyncRecord;
 using brave_sync::SimpleDeviceRecord;
 using brave_sync::jslib::SyncRecord;
+using brave_sync::tools::AsMutable;
 using testing::_;
 using testing::AtLeast;
 
@@ -1310,3 +1311,34 @@ TEST_F(BraveSyncServiceTest, DeviceIdV2MigrationDupDeviceId) {
       base::BindOnce(&OnGetRecordsStub);
   sync_service()->OnPollSyncCycle(std::move(on_get_records), &we);
 }
+
+TEST_F(BraveSyncServiceTest, AddNonClonedBookmarkKeys) {
+  sync_service()->AddNonClonedBookmarkKeys(model());
+  const bookmarks::BookmarkNode* bookmark_a1 =
+      model()->AddURL(model()->other_node(), 0, base::ASCIIToUTF16("A1"),
+                      GURL("https://a1.com"));
+
+  AsMutable(bookmark_a1)->SetMetaInfo("object_id", "object_id_value");
+  AsMutable(bookmark_a1)->SetMetaInfo("order", "order_value");
+  AsMutable(bookmark_a1)->SetMetaInfo("sync_timestamp", "sync_timestamp_value");
+  AsMutable(bookmark_a1)->SetMetaInfo("version", "version_value");
+
+  model()->Copy(bookmark_a1, model()->other_node(), 1);
+
+  const bookmarks::BookmarkNode* bookmark_copy =
+      model()->other_node()->children().at(1).get();
+
+  std::string meta_object_id;
+  EXPECT_FALSE(bookmark_copy->GetMetaInfo("object_id", &meta_object_id));
+  EXPECT_TRUE(meta_object_id.empty());
+  std::string meta_order;
+  EXPECT_FALSE(bookmark_copy->GetMetaInfo("order", &meta_order));
+  EXPECT_TRUE(meta_order.empty());
+  std::string meta_sync_timestamp;
+  EXPECT_FALSE(
+      bookmark_copy->GetMetaInfo("sync_timestamp", &meta_sync_timestamp));
+  EXPECT_TRUE(meta_sync_timestamp.empty());
+  std::string meta_version;
+  EXPECT_FALSE(bookmark_copy->GetMetaInfo("version", &meta_version));
+  EXPECT_TRUE(meta_version.empty());
+}

components/brave_sync/syncer_helper.cc

@@ -13,11 +13,6 @@
 namespace brave_sync {
 namespace {
 
-// Get mutable node to prevent BookmarkMetaInfoChanged from being triggered
-bookmarks::BookmarkNode* AsMutable(const bookmarks::BookmarkNode* node) {
-  return const_cast<bookmarks::BookmarkNode*>(node);
-}
-
 void SetOrder(const bookmarks::BookmarkNode* node,
               const std::string& parent_order) {
   DCHECK(!parent_order.empty());
@@ -41,7 +36,7 @@ void SetOrder(const bookmarks::BookmarkNode* node,
 
   std::string order =
       brave_sync::GetOrder(prev_order, next_order, parent_order);
-  AsMutable(node)->SetMetaInfo("order", order);
+  tools::AsMutable(node)->SetMetaInfo("order", order);
 }
 
 }  // namespace
@@ -92,17 +87,17 @@ void AddBraveMetaInfo(const bookmarks::BookmarkNode* node) {
   if (object_id.empty()) {
     object_id = tools::GenerateObjectId();
   }
-  AsMutable(node)->SetMetaInfo("object_id", object_id);
+  tools::AsMutable(node)->SetMetaInfo("object_id", object_id);
 
   std::string parent_object_id;
   node->parent()->GetMetaInfo("object_id", &parent_object_id);
-  AsMutable(node)->SetMetaInfo("parent_object_id", parent_object_id);
+  tools::AsMutable(node)->SetMetaInfo("parent_object_id", parent_object_id);
 
   std::string sync_timestamp;
   node->GetMetaInfo("sync_timestamp", &sync_timestamp);
   if (sync_timestamp.empty()) {
     sync_timestamp = std::to_string(base::Time::Now().ToJsTime());
-    AsMutable(node)->SetMetaInfo("sync_timestamp", sync_timestamp);
+    tools::AsMutable(node)->SetMetaInfo("sync_timestamp", sync_timestamp);
   }
   DCHECK(!sync_timestamp.empty());
 }

components/brave_sync/tools.cc

@@ -1,6 +1,8 @@
-/* This Source Code Form is subject to the terms of the Mozilla Public
- * License, v. 2.0. If a copy of the MPL was not distributed with this file,
- * You can obtain one at http://mozilla.org/MPL/2.0/. */
+/* Copyright 2020 The Brave Authors. All rights reserved.
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
 #include "brave/components/brave_sync/tools.h"
 
 #include <string>
@@ -15,21 +17,28 @@ namespace brave_sync {
 
 namespace tools {
 
-std::string GenerateObjectId() {
-  //16 random 8-bit unsigned numbers
-  const size_t length = 16;
-  uint8_t bytes[length];
-  crypto::RandBytes(bytes, sizeof(bytes));
+const size_t kIdSize = 16;
+
+namespace {
+std::string PrintObjectId(uint8_t* bytes) {
   std::stringstream ss;
-  for (size_t i = 0; i < length; ++i) {
-    const uint8_t &byte = bytes[i];
-    ss << std::dec << (int)byte;
-    if (i != length - 1) {
+  for (size_t i = 0; i < kIdSize; ++i) {
+    const uint8_t& byte = bytes[i];
+    ss << std::dec << static_cast<int>(byte);
+    if (i != kIdSize - 1) {
       ss << ", ";
     }
   }
   return ss.str();
 }
+}  // namespace
+
+std::string GenerateObjectId() {
+  // 16 random 8-bit unsigned numbers
+  uint8_t bytes[kIdSize];
+  crypto::RandBytes(bytes, sizeof(bytes));
+  return PrintObjectId(bytes);
+}
 
 std::string GetPlatformName() {
   #if defined(OS_ANDROID)
@@ -50,6 +59,11 @@ bool IsTimeEmpty(const base::Time &time) {
   return time.is_null() || base::checked_cast<int64_t>(time.ToJsTime()) == 0;
 }
 
-} // namespace tools
+// Get mutable node to prevent BookmarkMetaInfoChanged from being triggered
+bookmarks::BookmarkNode* AsMutable(const bookmarks::BookmarkNode* node) {
+  return const_cast<bookmarks::BookmarkNode*>(node);
+}
+
+}  // namespace tools
 
-} // namespace brave_sync
+}  // namespace brave_sync

components/brave_sync/tools.h

@@ -1,14 +1,20 @@
-/* This Source Code Form is subject to the terms of the Mozilla Public
- * License, v. 2.0. If a copy of the MPL was not distributed with this file,
- * You can obtain one at http://mozilla.org/MPL/2.0/. */
-#ifndef BRAVE_COMPONENTS_BRAVE_SYNC_BRAVE_SYNC_TOOLS_H_
-#define BRAVE_COMPONENTS_BRAVE_SYNC_BRAVE_SYNC_TOOLS_H_
+/* Copyright 2020 The Brave Authors. All rights reserved.
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#ifndef BRAVE_COMPONENTS_BRAVE_SYNC_TOOLS_H_
+#define BRAVE_COMPONENTS_BRAVE_SYNC_TOOLS_H_
 
 #include <string>
 
 namespace base {
-  class Time;
-} // namespace base
+class Time;
+}  // namespace base
+
+namespace bookmarks {
+class BookmarkNode;
+}
 
 namespace brave_sync {
 
@@ -19,8 +25,10 @@ std::string GetPlatformName();
 
 bool IsTimeEmpty(const base::Time &time);
 
-} // namespace tools
+bookmarks::BookmarkNode* AsMutable(const bookmarks::BookmarkNode* node);
+
+}  // namespace tools
 
-} // namespace brave_sync
+}  // namespace brave_sync
 
-#endif // BRAVE_COMPONENTS_BRAVE_SYNC_BRAVE_SYNC_TOOLS_H_
+#endif  // BRAVE_COMPONENTS_BRAVE_SYNC_TOOLS_H_