Skip to content

Commit

Permalink
Merge pull request #4710 from brave/sync_fix_obj_id_dup
Browse files Browse the repository at this point in the history
Sync fix bookmarks object id duplication
  • Loading branch information
AlexeyBarabash committed Mar 3, 2020
1 parent 701b8ec commit 1712048
Show file tree
Hide file tree
Showing 7 changed files with 98 additions and 33 deletions.
3 changes: 3 additions & 0 deletions browser/profiles/brave_bookmark_model_loaded_observer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
11 changes: 11 additions & 0 deletions components/brave_sync/brave_profile_sync_service_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions components/brave_sync/brave_profile_sync_service_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,8 @@ class BraveProfileSyncServiceImpl
BraveSyncClient* GetBraveSyncClient() override;
#endif

static void AddNonClonedBookmarkKeys(BookmarkModel* model);

bool IsBraveSyncEnabled() const override;

syncer::ModelTypeSet GetPreferredDataTypes() const override;
Expand Down
32 changes: 32 additions & 0 deletions components/brave_sync/brave_sync_service_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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());
}
13 changes: 4 additions & 9 deletions components/brave_sync/syncer_helper.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand All @@ -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
Expand Down Expand Up @@ -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());
}
Expand Down
42 changes: 28 additions & 14 deletions components/brave_sync/tools.cc
Original file line number Diff line number Diff line change
@@ -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>
Expand All @@ -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)
Expand All @@ -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
28 changes: 18 additions & 10 deletions components/brave_sync/tools.h
Original file line number Diff line number Diff line change
@@ -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 {

Expand All @@ -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_

0 comments on commit 1712048

Please sign in to comment.