Add test for device id v2 migration

components/brave_sync/brave_profile_sync_service_impl.cc

@@ -930,6 +930,8 @@ void BraveProfileSyncServiceImpl::OnResolvedPreferences(
     const RecordsList& records) {
   const std::string this_device_object_id =
       brave_sync_prefs_->GetThisDeviceObjectId();
+  const std::string this_device_id_v2 =
+      brave_sync_prefs_->GetThisDeviceIdV2();
   bool this_device_deleted = false;
 
   auto sync_devices = brave_sync_prefs_->GetSyncDevices();
@@ -942,9 +944,13 @@ void BraveProfileSyncServiceImpl::OnResolvedPreferences(
                                      record->deviceId, device.deviceIdV2,
                                      record->syncTimestamp.ToJsTime()),
                           record->action, &actually_merged);
+      // We check object id here specifically because device which doesn't have
+      // device id v2 also doesn't have this object id stored. So we use this
+      // trait for migration.
       this_device_deleted =
           this_device_deleted ||
           (record->objectId == this_device_object_id &&
+           device.deviceIdV2 == this_device_id_v2 &&
            record->action == SyncRecord::Action::A_DELETE && actually_merged);
     }
   }  // for each device

components/brave_sync/brave_profile_sync_service_impl.h

@@ -47,6 +47,8 @@ FORWARD_DECLARE_TEST(BraveSyncServiceTest, SetSyncDisabled);
 FORWARD_DECLARE_TEST(BraveSyncServiceTest, IsSyncReadyOnNewProfile);
 FORWARD_DECLARE_TEST(BraveSyncServiceTest, SetThisDeviceCreatedTime);
 FORWARD_DECLARE_TEST(BraveSyncServiceTest, InitialFetchesStartWithZero);
+FORWARD_DECLARE_TEST(BraveSyncServiceTest, DeviceIdV2Migration);
+FORWARD_DECLARE_TEST(BraveSyncServiceTest, DeviceIdV2MigrationDupDeviceId);
 
 class BraveSyncServiceTest;
 
@@ -163,6 +165,9 @@ class BraveProfileSyncServiceImpl
   FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, IsSyncReadyOnNewProfile);
   FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, SetThisDeviceCreatedTime);
   FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, InitialFetchesStartWithZero);
+  FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, DeviceIdV2Migration);
+  FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest,
+                           DeviceIdV2MigrationDupDeviceId);
 
   friend class ::BraveSyncServiceTest;
 

components/brave_sync/brave_sync_prefs.cc

@@ -97,8 +97,8 @@ std::string Prefs::GetThisDeviceIdV2() const {
 }
 
 void Prefs::SetThisDeviceIdV2(const std::string& device_id_v2) {
-  DCHECK(!device_id_v2.empty());
-  pref_service_->SetString(kSyncDeviceIdV2, device_id_v2);
+  if (!device_id_v2.empty())
+    pref_service_->SetString(kSyncDeviceIdV2, device_id_v2);
 }
 
 std::string Prefs::GetThisDeviceObjectId() const {

components/brave_sync/brave_sync_service_unittest.cc

@@ -527,7 +527,7 @@ TEST_F(BraveSyncServiceTest, OnDeleteDeviceWhenSelfDeleted) {
                          "beef01", "device1"));
   records.push_back(
       SimpleDeviceRecord(SyncRecord::Action::A_CREATE, object_id_2, "2",
-                         "beef02","device2"));
+                         "beef02", "device2"));
   EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(1);
   sync_service()->OnResolvedPreferences(records);
 
@@ -1145,3 +1145,117 @@ TEST_F(BraveSyncServiceTest, InitialFetchesStartWithZero) {
     sync_service()->FetchSyncRecords(true, false, false, 1000);
   }
 }
+
+TEST_F(BraveSyncServiceTest, DeviceIdV2Migration) {
+  EXPECT_CALL(*sync_client(), OnSyncEnabledChanged);
+  EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service()))
+      .Times(AtLeast(2));
+  sync_service()->OnSetupSyncNewToSync("device0");
+  EXPECT_TRUE(
+      profile()->GetPrefs()->GetBoolean(brave_sync::prefs::kSyncEnabled));
+
+  // Service gets seed from client via BraveSyncServiceImpl::OnSaveInitData
+  const auto binary_seed = brave_sync::Uint8Array(16, 77);
+
+  // emulate device without device id v2
+  sync_service()->OnSaveInitData(binary_seed, {0}, "");
+  std::string expected_seed = brave_sync::StrFromUint8Array(binary_seed);
+  EXPECT_EQ(sync_service()->GetSeed(), expected_seed);
+  EXPECT_EQ(brave_sync_prefs()->GetThisDeviceIdV2(), "");
+
+  RecordsList records;
+  records.push_back(
+      SimpleDeviceRecord(SyncRecord::Action::A_CREATE, "", "0", "",
+                         "device0"));
+  records.push_back(
+      SimpleDeviceRecord(SyncRecord::Action::A_CREATE, "", "1", "",
+                         "device1"));
+  sync_service()->OnResolvedPreferences(records);
+  brave_sync_prefs()->SetLastFetchTime(base::Time::Now());
+
+  auto devices = brave_sync_prefs()->GetSyncDevices();
+
+  EXPECT_TRUE(DevicesContains(devices.get(), "0", "", "device0"));
+  EXPECT_TRUE(DevicesContains(devices.get(), "1", "", "device1"));
+
+  const std::string device_id_v2 = "beef00";
+  // sync library will call SAVE_INIT_DATA to propagate device id v2
+  sync_service()->OnSaveInitData(binary_seed, {0}, device_id_v2);
+  EXPECT_EQ(brave_sync_prefs()->GetThisDeviceIdV2(), device_id_v2);
+
+  EXPECT_CALL(*sync_client(),
+              SendSyncRecords("PREFERENCES",
+                              ContainsDeviceRecord(SyncRecord::Action::A_DELETE,
+                                                   "device0", device_id_v2)))
+  .Times(1);
+  EXPECT_CALL(*sync_client(),
+              SendSyncRecords("PREFERENCES",
+                              ContainsDeviceRecord(SyncRecord::Action::A_CREATE,
+                                                   "device0", device_id_v2)))
+  .Times(1);
+
+  base::WaitableEvent we;
+  brave_sync::GetRecordsCallback on_get_records =
+      base::BindOnce(&OnGetRecordsStub);
+  sync_service()->OnPollSyncCycle(std::move(on_get_records), &we);
+}
+
+TEST_F(BraveSyncServiceTest, DeviceIdV2MigrationDupDeviceId) {
+  EXPECT_CALL(*sync_client(), OnSyncEnabledChanged);
+  EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service()))
+      .Times(AtLeast(2));
+  sync_service()->OnSetupSyncHaveCode(kTestWords1, "device1");
+  EXPECT_TRUE(
+      profile()->GetPrefs()->GetBoolean(brave_sync::prefs::kSyncEnabled));
+
+  // Service gets seed from client via BraveSyncServiceImpl::OnSaveInitData
+  const auto binary_seed = brave_sync::Uint8Array(16, 77);
+
+  // emulate device with dup device id and without device id v2
+  sync_service()->OnSaveInitData(binary_seed, {1}, "");
+  std::string expected_seed = brave_sync::StrFromUint8Array(binary_seed);
+  EXPECT_EQ(sync_service()->GetSeed(), expected_seed);
+  EXPECT_EQ(brave_sync_prefs()->GetThisDeviceIdV2(), "");
+
+  RecordsList records;
+  records.push_back(
+      SimpleDeviceRecord(SyncRecord::Action::A_CREATE, "", "0", "",
+                         "device0"));
+  records.push_back(
+      SimpleDeviceRecord(SyncRecord::Action::A_CREATE, "", "1", "",
+                         "device1"));
+  records.push_back(
+      SimpleDeviceRecord(SyncRecord::Action::A_CREATE, "", "1", "",
+                         "device2"));
+  sync_service()->OnResolvedPreferences(records);
+  brave_sync_prefs()->SetLastFetchTime(base::Time::Now());
+
+  auto devices = brave_sync_prefs()->GetSyncDevices();
+
+  EXPECT_TRUE(DevicesContains(devices.get(), "0", "", "device0"));
+  EXPECT_TRUE(DevicesContains(devices.get(), "1", "", "device1"));
+  EXPECT_TRUE(DevicesContains(devices.get(), "1", "", "device2"));
+
+  const std::string device_id_v2 = "beef01";
+  // sync library will call SAVE_INIT_DATA to propagate device id v2
+  sync_service()->OnSaveInitData(binary_seed, {1}, device_id_v2);
+  EXPECT_EQ(brave_sync_prefs()->GetThisDeviceIdV2(), device_id_v2);
+
+  // It will send DELETE record twice because two device records has same device
+  // id
+  EXPECT_CALL(*sync_client(),
+              SendSyncRecords("PREFERENCES",
+                              ContainsDeviceRecord(SyncRecord::Action::A_DELETE,
+                                                   "device1", device_id_v2)))
+  .Times(2);
+  EXPECT_CALL(*sync_client(),
+              SendSyncRecords("PREFERENCES",
+                              ContainsDeviceRecord(SyncRecord::Action::A_CREATE,
+                                                   "device1", device_id_v2)))
+  .Times(1);
+
+  base::WaitableEvent we;
+  brave_sync::GetRecordsCallback on_get_records =
+      base::BindOnce(&OnGetRecordsStub);
+  sync_service()->OnPollSyncCycle(std::move(on_get_records), &we);
+}