Merge pull request #4940 from brave/sync_dup_obj_id_migration2

Migration of duplicated bookmarks for sync (2)

components/brave_sync/brave_profile_sync_service_impl.cc

@@ -7,6 +7,7 @@
 
 #include <algorithm>
 #include <map>
+#include <set>
 #include <utility>
 #include <vector>
 
@@ -167,18 +168,19 @@ SyncRecordPtr PrepareResolvedDevice(SyncDevice* device,
   return record;
 }
 
-struct BookmarkByDateAddedComparator {
+struct IndexInParentComparator {
   bool operator()(const bookmarks::BookmarkNode* lhs,
                   const bookmarks::BookmarkNode* rhs) const {
     DCHECK(lhs);
     DCHECK(rhs);
-    DCHECK(!tools::IsTimeEmpty(lhs->date_added()));
-    DCHECK(!tools::IsTimeEmpty(rhs->date_added()));
-    return lhs->date_added() < rhs->date_added();
+    // Nodes equal by object_id we want to sort by index in parent
+    // We need that to make easier the assigning of orders in the case
+    // when orders would be re-calculated on migration of object_id
+    return lhs->parent()->GetIndexOf(lhs) < rhs->parent()->GetIndexOf(rhs);
   }
 };
-using SortedNodes =
-    std::set<const bookmarks::BookmarkNode*, BookmarkByDateAddedComparator>;
+using SortedNodes = std::multiset<const bookmarks::BookmarkNode*,
+                                  IndexInParentComparator>;
 using ObjectIdToNodes = std::map<std::string, SortedNodes>;
 
 void FillObjectsMap(const bookmarks::BookmarkNode* parent,
@@ -202,17 +204,15 @@ void ClearDuplicatedNodes(ObjectIdToNodes* object_id_nodes,
        it_object_id != object_id_nodes->end(); ++it_object_id) {
     const SortedNodes& nodes = it_object_id->second;
     if (nodes.size() > 1) {
-      // Nodes are sorted from oldest to newest, go to the second by age
-      SortedNodes::iterator it_nodes = nodes.begin();
-      ++it_nodes;
-      for (; it_nodes != nodes.end(); ++it_nodes) {
+      // Re-create all group of nodes which have the same object_id
+      for (SortedNodes::iterator it_nodes = nodes.begin();
+           it_nodes != nodes.end(); ++it_nodes) {
         const bookmarks::BookmarkNode* node = *it_nodes;
         // Copy and delete node
         const auto* parent = node->parent();
         size_t original_index = parent->GetIndexOf(node);
         model->Copy(node, parent, original_index);
         model->Remove(node);
-        brave_sync::AddBraveMetaInfo(parent->children()[original_index].get());
       }
     }
   }
@@ -815,9 +815,10 @@ void BraveProfileSyncServiceImpl::MigrateDuplicatedBookmarksObjectIds(
   DCHECK(model);
   DCHECK(model->loaded());
 
-  bool duplicated_bookmarks_recovered =
-      profile->GetPrefs()->GetBoolean(prefs::kDuplicatedBookmarksRecovered);
-  if (duplicated_bookmarks_recovered) {
+  int migrated_version = profile->GetPrefs()->GetInteger(
+      prefs::kDuplicatedBookmarksMigrateVersion);
+
+  if (migrated_version >= 1) {
     return;
   }
 
@@ -829,7 +830,7 @@ void BraveProfileSyncServiceImpl::MigrateDuplicatedBookmarksObjectIds(
   FillObjectsMap(model->root_node(), &object_id_nodes);
   ClearDuplicatedNodes(&object_id_nodes, model);
 
-  profile->GetPrefs()->SetBoolean(prefs::kDuplicatedBookmarksRecovered, true);
+  profile->GetPrefs()->SetInteger(prefs::kDuplicatedBookmarksMigrateVersion, 1);
 }
 
 std::unique_ptr<SyncRecord>

components/brave_sync/brave_sync_prefs.cc

@@ -15,6 +15,7 @@
 
 void MigrateBraveSyncPrefs(PrefService* prefs) {
   prefs->ClearPref(brave_sync::prefs::kSyncPrevSeed);
+  prefs->ClearPref(brave_sync::prefs::kDuplicatedBookmarksRecovered);
 }
 
 namespace brave_sync {
@@ -45,6 +46,8 @@ const char kSyncRecordsToResend[] = "brave_sync_records_to_resend";
 const char kSyncRecordsToResendMeta[] = "brave_sync_records_to_resend_meta";
 const char kDuplicatedBookmarksRecovered[] =
     "brave_sync_duplicated_bookmarks_recovered";
+const char kDuplicatedBookmarksMigrateVersion[] =
+    "brave_sync_duplicated_bookmarks_migrate_version";
 
 Prefs::Prefs(PrefService* pref_service) : pref_service_(pref_service) {}
 
@@ -75,6 +78,7 @@ void Prefs::RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) {
   registry->RegisterListPref(prefs::kSyncRecordsToResend);
   registry->RegisterDictionaryPref(prefs::kSyncRecordsToResendMeta);
   registry->RegisterBooleanPref(kDuplicatedBookmarksRecovered, false);
+  registry->RegisterIntegerPref(prefs::kDuplicatedBookmarksMigrateVersion, 0);
 }
 
 std::string Prefs::GetSeed() const {

components/brave_sync/brave_sync_prefs.h

@@ -72,9 +72,14 @@ extern const char kSyncMigrateBookmarksVersion[];
 extern const char kSyncRecordsToResend[];
 // Meta info of kSyncRecordsToResend
 extern const char kSyncRecordsToResendMeta[];
-// Flag indicates we had recovered duplicated bookmarks object ids
+// Flag indicates we had recovered duplicated bookmarks object ids (deprecated)
 extern const char kDuplicatedBookmarksRecovered[];
 
+// Version indicates had recovered duplicated bookmarks object ids:
+// 1 - we had migrated object ids
+// 2 - we have migrated broken bookmarks orders
+extern const char kDuplicatedBookmarksMigrateVersion[];
+
 class Prefs {
  public:
   explicit Prefs(PrefService* pref_service);

components/brave_sync/brave_sync_service_unittest.cc

@@ -1647,36 +1647,50 @@ TEST_F(BraveSyncServiceTest, MigrateDuplicatedBookmarksObjectIds) {
 
   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_TRUE(bookmark_copy->GetMetaInfo("object_id", &meta_object_id));
-  EXPECT_EQ(meta_object_id, "object_id_value");
-  std::string meta_order;
-  EXPECT_TRUE(bookmark_copy->GetMetaInfo("order", &meta_order));
-  EXPECT_EQ(meta_order, "255.255.255.3");
-  std::string meta_sync_timestamp;
-  EXPECT_TRUE(
-      bookmark_copy->GetMetaInfo("sync_timestamp", &meta_sync_timestamp));
-  EXPECT_EQ(meta_sync_timestamp, "sync_timestamp_value");
-  std::string meta_version;
-  EXPECT_TRUE(bookmark_copy->GetMetaInfo("version", &meta_version));
-  EXPECT_EQ(meta_version, "version_value");
+  model()->Copy(bookmark_a1, model()->other_node(), 2);
+
+  for (size_t i = 1; i <= 2; ++i) {
+    const bookmarks::BookmarkNode* bookmark_copy =
+        model()->other_node()->children().at(i).get();
+
+    // Verify fields after copying
+    std::string meta_object_id;
+    EXPECT_TRUE(bookmark_copy->GetMetaInfo("object_id", &meta_object_id));
+    EXPECT_EQ(meta_object_id, "object_id_value");
+    std::string meta_order;
+    EXPECT_TRUE(bookmark_copy->GetMetaInfo("order", &meta_order));
+    EXPECT_EQ(meta_order, "255.255.255.3");
+    std::string meta_sync_timestamp;
+    EXPECT_TRUE(
+        bookmark_copy->GetMetaInfo("sync_timestamp", &meta_sync_timestamp));
+    EXPECT_EQ(meta_sync_timestamp, "sync_timestamp_value");
+    std::string meta_version;
+    EXPECT_TRUE(bookmark_copy->GetMetaInfo("version", &meta_version));
+    EXPECT_EQ(meta_version, "version_value");
+
+    // Simulate all bookmarks don`t have added time, as a worse case,
+    // but happened on live profile
+    AsMutable(bookmark_copy)->set_date_added(base::Time());
+  }
 
   sync_service()->AddNonClonedBookmarkKeys(model());
 
   // Do the migration
   BraveProfileSyncServiceImpl::MigrateDuplicatedBookmarksObjectIds(profile(),
                                                                    model());
-  bookmark_copy = model()->other_node()->children().at(1).get();
 
-  std::string meta_migrated_object_id;
-  EXPECT_TRUE(
-      bookmark_copy->GetMetaInfo("object_id", &meta_migrated_object_id));
-  EXPECT_NE(meta_migrated_object_id, "object_id_value");
-
-  std::string meta_migrated_order;
-  EXPECT_TRUE(bookmark_copy->GetMetaInfo("order", &meta_migrated_order));
-  EXPECT_NE(meta_migrated_order, "255.255.255.3");
+  // All the bookmarks after migration must not have sync meta info
+  for (size_t i = 0; i <= 2; ++i) {
+    const bookmarks::BookmarkNode* bookmark =
+        model()->other_node()->children().at(i).get();
+    std::string migrated_object_id;
+    std::string migrated_order;
+    std::string migrated_sync_timestamp;
+    std::string migrated_version;
+    EXPECT_FALSE(bookmark->GetMetaInfo("object_id", &migrated_object_id));
+    EXPECT_FALSE(bookmark->GetMetaInfo("order", &migrated_order));
+    EXPECT_FALSE(
+        bookmark->GetMetaInfo("sync_timestamp", &migrated_sync_timestamp));
+    EXPECT_FALSE(bookmark->GetMetaInfo("version", &migrated_version));
+  }
 }

components/brave_sync/syncer_helper.cc

@@ -103,8 +103,8 @@ void AddBraveMetaInfo(const bookmarks::BookmarkNode* node) {
   DCHECK(!sync_timestamp.empty());
   // Set other_node to have same sync_timestamp as least added child
   if (node->parent()->type() == bookmarks::BookmarkNode::OTHER_NODE) {
-    tools::AsMutable(node->parent())->SetMetaInfo("sync_timestamp",
-                                                  sync_timestamp);
+    tools::AsMutable(node->parent())
+        ->SetMetaInfo("sync_timestamp", sync_timestamp);
   }
 }