From 1712048e90e63fbf8732aea27d23735b67061812 Mon Sep 17 00:00:00 2001 From: AlexeyBarabash Date: Thu, 27 Feb 2020 14:19:53 -0700 Subject: [PATCH] Merge pull request #4710 from brave/sync_fix_obj_id_dup Sync fix bookmarks object id duplication --- .../brave_bookmark_model_loaded_observer.cc | 3 ++ .../brave_profile_sync_service_impl.cc | 11 +++++ .../brave_profile_sync_service_impl.h | 2 + .../brave_sync/brave_sync_service_unittest.cc | 32 ++++++++++++++ components/brave_sync/syncer_helper.cc | 13 ++---- components/brave_sync/tools.cc | 42 ++++++++++++------- components/brave_sync/tools.h | 28 ++++++++----- 7 files changed, 98 insertions(+), 33 deletions(-) diff --git a/browser/profiles/brave_bookmark_model_loaded_observer.cc b/browser/profiles/brave_bookmark_model_loaded_observer.cc index 59e4f191809a..f76f5ca20e83 100644 --- a/browser/profiles/brave_bookmark_model_loaded_observer.cc +++ b/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); } diff --git a/components/brave_sync/brave_profile_sync_service_impl.cc b/components/brave_sync/brave_profile_sync_service_impl.cc index 5b24a7df4cdb..b61394e0e588 100644 --- a/components/brave_sync/brave_profile_sync_service_impl.cc +++ b/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 diff --git a/components/brave_sync/brave_profile_sync_service_impl.h b/components/brave_sync/brave_profile_sync_service_impl.h index 2582116779f7..01a4b228c684 100644 --- a/components/brave_sync/brave_profile_sync_service_impl.h +++ b/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; diff --git a/components/brave_sync/brave_sync_service_unittest.cc b/components/brave_sync/brave_sync_service_unittest.cc index 92bf3431d621..1983541b12f0 100644 --- a/components/brave_sync/brave_sync_service_unittest.cc +++ b/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()); +} diff --git a/components/brave_sync/syncer_helper.cc b/components/brave_sync/syncer_helper.cc index 391eb8460491..2ca5e7cd79a3 100644 --- a/components/brave_sync/syncer_helper.cc +++ b/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(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()); } diff --git a/components/brave_sync/tools.cc b/components/brave_sync/tools.cc index 8671df9a157c..c49d31647ad9 100644 --- a/components/brave_sync/tools.cc +++ b/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 @@ -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(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(time.ToJsTime()) == 0; } -} // namespace tools +// Get mutable node to prevent BookmarkMetaInfoChanged from being triggered +bookmarks::BookmarkNode* AsMutable(const bookmarks::BookmarkNode* node) { + return const_cast(node); +} + +} // namespace tools -} // namespace brave_sync +} // namespace brave_sync diff --git a/components/brave_sync/tools.h b/components/brave_sync/tools.h index 537e94215dac..b3ca40241ed5 100644 --- a/components/brave_sync/tools.h +++ b/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 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_