sync: Move unique_client_tag hashing code

This is one of many changes that will lead up to the introduction of the bookmark UniquePosition algorithm. We eventually want to use the hashing code to create a unique tag for bookmarks. This tag will be initialized at bookmark creation time in MutableEntry. This patch moves the hashing code out of BaseNode and into syncable_util so it can be accessed from the syncable layer. BUG=145412,126505 Review URL: https://chromiumcodereview.appspot.com/11571092 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@174449 0039d316-1c4b-4281-b951-d872f2087c98
qweri0p · Dec 21, 2012 · b2bf91a · b2bf91a
1 parent 859566a
commit b2bf91a
Show file tree

Hide file tree

Showing 9 changed files with 74 additions and 53 deletions.
diff --git a/sync/internal_api/base_node.cc b/sync/internal_api/base_node.cc
@@ -6,8 +6,6 @@
 
 #include <stack>
 
-#include "base/base64.h"
-#include "base/sha1.h"
 #include "base/string_number_conversions.h"
 #include "base/utf_string_conversions.h"
 #include "base/values.h"
@@ -65,21 +63,6 @@ BaseNode::BaseNode() : password_data_(new sync_pb::PasswordSpecificsData) {}
 
 BaseNode::~BaseNode() {}
 
-std::string BaseNode::GenerateSyncableHash(
-    ModelType model_type, const std::string& client_tag) {
-  // Blank PB with just the field in it has termination symbol,
-  // handy for delimiter.
-  sync_pb::EntitySpecifics serialized_type;
-  AddDefaultFieldValue(model_type, &serialized_type);
-  std::string hash_input;
-  serialized_type.AppendToString(&hash_input);
-  hash_input.append(client_tag);
-
-  std::string encode_output;
-  CHECK(base::Base64Encode(base::SHA1HashString(hash_input), &encode_output));
-  return encode_output;
-}
-
 bool BaseNode::DecryptIfNecessary() {
   if (!GetEntry()->Get(syncable::UNIQUE_SERVER_TAG).empty())
       return true;  // Ignore unique folders.

diff --git a/sync/internal_api/public/base_node.h b/sync/internal_api/public/base_node.h
@@ -200,11 +200,6 @@ class SYNC_EXPORT BaseNode {
  protected:
   BaseNode();
   virtual ~BaseNode();
-  // The server has a size limit on client tags, so we generate a fixed length
-  // hash locally. This also ensures that ModelTypes have unique namespaces.
-  static std::string GenerateSyncableHash(
-      ModelType model_type,
-      const std::string& client_tag);
 
   // Determines whether part of the entry is encrypted, and if so attempts to
   // decrypt it. Unless decryption is necessary and fails, this will always

diff --git a/sync/internal_api/read_node.cc b/sync/internal_api/read_node.cc
@@ -8,6 +8,7 @@
 #include "sync/internal_api/public/base_transaction.h"
 #include "sync/syncable/entry.h"
 #include "sync/syncable/syncable_base_transaction.h"
+#include "sync/syncable/syncable_util.h"
 
 namespace syncer {
 
@@ -57,7 +58,7 @@ BaseNode::InitByLookupResult ReadNode::InitByClientTagLookup(
   if (tag.empty())
     return INIT_FAILED_PRECONDITION;
 
-  const std::string hash = GenerateSyncableHash(model_type, tag);
+  const std::string hash = syncable::GenerateSyncableHash(model_type, tag);
 
   entry_ = new syncable::Entry(transaction_->GetWrappedTrans(),
                                syncable::GET_BY_CLIENT_TAG, hash);

diff --git a/sync/internal_api/sync_manager_impl_unittest.cc b/sync/internal_api/sync_manager_impl_unittest.cc
@@ -63,6 +63,7 @@
 #include "sync/syncable/nigori_util.h"
 #include "sync/syncable/syncable_id.h"
 #include "sync/syncable/syncable_read_transaction.h"
+#include "sync/syncable/syncable_util.h"
 #include "sync/syncable/syncable_write_transaction.h"
 #include "sync/test/callback_counter.h"
 #include "sync/test/engine/fake_sync_scheduler.h"
@@ -279,22 +280,6 @@ TEST_F(SyncApiTest, BasicTagWrite) {
   }
 }
 
-TEST_F(SyncApiTest, GenerateSyncableHash) {
-  EXPECT_EQ("OyaXV5mEzrPS4wbogmtKvRfekAI=",
-      BaseNode::GenerateSyncableHash(BOOKMARKS, "tag1"));
-  EXPECT_EQ("iNFQtRFQb+IZcn1kKUJEZDDkLs4=",
-      BaseNode::GenerateSyncableHash(PREFERENCES, "tag1"));
-  EXPECT_EQ("gO1cPZQXaM73sHOvSA+tKCKFs58=",
-      BaseNode::GenerateSyncableHash(AUTOFILL, "tag1"));
-
-  EXPECT_EQ("A0eYIHXM1/jVwKDDp12Up20IkKY=",
-      BaseNode::GenerateSyncableHash(BOOKMARKS, "tag2"));
-  EXPECT_EQ("XYxkF7bhS4eItStFgiOIAU23swI=",
-      BaseNode::GenerateSyncableHash(PREFERENCES, "tag2"));
-  EXPECT_EQ("GFiWzo5NGhjLlN+OyCfhy28DJTQ=",
-      BaseNode::GenerateSyncableHash(AUTOFILL, "tag2"));
-}
-
 TEST_F(SyncApiTest, ModelTypesSiloed) {
   {
     WriteTransaction trans(FROM_HERE, test_user_share_.user_share());
@@ -956,7 +941,7 @@ class SyncManagerTest : public testing::Test,
     UserShare* share = sync_manager_.GetUserShare();
     syncable::WriteTransaction trans(
         FROM_HERE, syncable::UNITTEST, share->directory.get());
-    const std::string hash = BaseNode::GenerateSyncableHash(type, client_tag);
+    const std::string hash = syncable::GenerateSyncableHash(type, client_tag);
     syncable::MutableEntry entry(&trans, syncable::GET_BY_CLIENT_TAG,
                                  hash);
     EXPECT_TRUE(entry.good());
@@ -2197,7 +2182,7 @@ TEST_F(SyncManagerTest, UpdateEntryWithEncryption) {
   entity_specifics.mutable_bookmark()->set_url("url");
   entity_specifics.mutable_bookmark()->set_title("title");
   MakeServerNode(sync_manager_.GetUserShare(), BOOKMARKS, client_tag,
-                 BaseNode::GenerateSyncableHash(BOOKMARKS,
+                 syncable::GenerateSyncableHash(BOOKMARKS,
                                                 client_tag),
                  entity_specifics);
   // New node shouldn't start off unsynced.
@@ -2346,7 +2331,7 @@ TEST_F(SyncManagerTest, UpdatePasswordSetEntitySpecificsNoChange) {
             mutable_encrypted());
   }
   MakeServerNode(sync_manager_.GetUserShare(), PASSWORDS, client_tag,
-                 BaseNode::GenerateSyncableHash(PASSWORDS,
+                 syncable::GenerateSyncableHash(PASSWORDS,
                                                 client_tag),
                  entity_specifics);
   // New node shouldn't start off unsynced.
@@ -2381,7 +2366,7 @@ TEST_F(SyncManagerTest, UpdatePasswordSetPasswordSpecifics) {
             mutable_encrypted());
   }
   MakeServerNode(sync_manager_.GetUserShare(), PASSWORDS, client_tag,
-                 BaseNode::GenerateSyncableHash(PASSWORDS,
+                 syncable::GenerateSyncableHash(PASSWORDS,
                                                 client_tag),
                  entity_specifics);
   // New node shouldn't start off unsynced.
@@ -2432,7 +2417,7 @@ TEST_F(SyncManagerTest, UpdatePasswordNewPassphrase) {
         entity_specifics.mutable_password()->mutable_encrypted());
   }
   MakeServerNode(sync_manager_.GetUserShare(), PASSWORDS, client_tag,
-                 BaseNode::GenerateSyncableHash(PASSWORDS,
+                 syncable::GenerateSyncableHash(PASSWORDS,
                                                 client_tag),
                  entity_specifics);
   // New node shouldn't start off unsynced.
@@ -2471,7 +2456,7 @@ TEST_F(SyncManagerTest, UpdatePasswordReencryptEverything) {
         entity_specifics.mutable_password()->mutable_encrypted());
   }
   MakeServerNode(sync_manager_.GetUserShare(), PASSWORDS, client_tag,
-                 BaseNode::GenerateSyncableHash(PASSWORDS,
+                 syncable::GenerateSyncableHash(PASSWORDS,
                                                 client_tag),
                  entity_specifics);
   // New node shouldn't start off unsynced.
@@ -2495,7 +2480,7 @@ TEST_F(SyncManagerTest, SetBookmarkTitle) {
   entity_specifics.mutable_bookmark()->set_url("url");
   entity_specifics.mutable_bookmark()->set_title("title");
   MakeServerNode(sync_manager_.GetUserShare(), BOOKMARKS, client_tag,
-                 BaseNode::GenerateSyncableHash(BOOKMARKS,
+                 syncable::GenerateSyncableHash(BOOKMARKS,
                                                 client_tag),
                  entity_specifics);
   // New node shouldn't start off unsynced.
@@ -2531,7 +2516,7 @@ TEST_F(SyncManagerTest, SetBookmarkTitleWithEncryption) {
   entity_specifics.mutable_bookmark()->set_url("url");
   entity_specifics.mutable_bookmark()->set_title("title");
   MakeServerNode(sync_manager_.GetUserShare(), BOOKMARKS, client_tag,
-                 BaseNode::GenerateSyncableHash(BOOKMARKS,
+                 syncable::GenerateSyncableHash(BOOKMARKS,
                                                 client_tag),
                  entity_specifics);
   // New node shouldn't start off unsynced.
@@ -2590,7 +2575,7 @@ TEST_F(SyncManagerTest, SetNonBookmarkTitle) {
   MakeServerNode(sync_manager_.GetUserShare(),
                  PREFERENCES,
                  client_tag,
-                 BaseNode::GenerateSyncableHash(PREFERENCES,
+                 syncable::GenerateSyncableHash(PREFERENCES,
                                                 client_tag),
                  entity_specifics);
   // New node shouldn't start off unsynced.
@@ -2628,7 +2613,7 @@ TEST_F(SyncManagerTest, SetNonBookmarkTitleWithEncryption) {
   MakeServerNode(sync_manager_.GetUserShare(),
                  PREFERENCES,
                  client_tag,
-                 BaseNode::GenerateSyncableHash(PREFERENCES,
+                 syncable::GenerateSyncableHash(PREFERENCES,
                                                 client_tag),
                  entity_specifics);
   // New node shouldn't start off unsynced.
@@ -2699,7 +2684,7 @@ TEST_F(SyncManagerTest, SetPreviouslyEncryptedSpecifics) {
     AddDefaultFieldValue(BOOKMARKS, &entity_specifics);
   }
   MakeServerNode(sync_manager_.GetUserShare(), BOOKMARKS, client_tag,
-                 BaseNode::GenerateSyncableHash(BOOKMARKS,
+                 syncable::GenerateSyncableHash(BOOKMARKS,
                                                 client_tag),
                  entity_specifics);
 
@@ -2764,7 +2749,7 @@ TEST_F(SyncManagerTest, IncrementTransactionVersion) {
   entity_specifics.mutable_bookmark()->set_url("url");
   entity_specifics.mutable_bookmark()->set_title("title");
   MakeServerNode(sync_manager_.GetUserShare(), BOOKMARKS, client_tag,
-                 BaseNode::GenerateSyncableHash(BOOKMARKS,
+                 syncable::GenerateSyncableHash(BOOKMARKS,
                                                 client_tag),
                  entity_specifics);
 

diff --git a/sync/internal_api/write_node.cc b/sync/internal_api/write_node.cc
@@ -19,6 +19,7 @@
 #include "sync/protocol/typed_url_specifics.pb.h"
 #include "sync/syncable/mutable_entry.h"
 #include "sync/syncable/nigori_util.h"
+#include "sync/syncable/syncable_util.h"
 #include "sync/util/cryptographer.h"
 
 using std::string;
@@ -292,7 +293,7 @@ BaseNode::InitByLookupResult WriteNode::InitByClientTagLookup(
   if (tag.empty())
     return INIT_FAILED_PRECONDITION;
 
-  const std::string hash = GenerateSyncableHash(model_type, tag);
+  const std::string hash = syncable::GenerateSyncableHash(model_type, tag);
 
   entry_ = new syncable::MutableEntry(transaction_->GetWrappedWriteTrans(),
                                       syncable::GET_BY_CLIENT_TAG, hash);
@@ -380,7 +381,7 @@ WriteNode::InitUniqueByCreationResult WriteNode::InitUniqueByCreation(
     return INIT_FAILED_EMPTY_TAG;
   }
 
-  const std::string hash = GenerateSyncableHash(model_type, tag);
+  const std::string hash = syncable::GenerateSyncableHash(model_type, tag);
 
   syncable::Id parent_id = parent.GetEntry()->Get(syncable::ID);
 

diff --git a/sync/sync.gyp b/sync/sync.gyp
@@ -687,6 +687,7 @@
           'syncable/syncable_enum_conversions_unittest.cc',
           'syncable/syncable_id_unittest.cc',
           'syncable/syncable_unittest.cc',
+          'syncable/syncable_util_unittest.cc',
           'util/cryptographer_unittest.cc',
           'util/data_encryption_win_unittest.cc',
           'util/data_type_histogram_unittest.cc',

diff --git a/sync/syncable/syncable_util.cc b/sync/syncable/syncable_util.cc
@@ -4,8 +4,10 @@
 
 #include "sync/syncable/syncable_util.h"
 
+#include "base/base64.h"
 #include "base/location.h"
 #include "base/logging.h"
+#include "base/sha1.h"
 #include "sync/syncable/directory.h"
 #include "sync/syncable/entry.h"
 #include "sync/syncable/mutable_entry.h"
@@ -102,5 +104,20 @@ bool SyncAssert(bool condition,
   return true;
 }
 
+std::string GenerateSyncableHash(
+    ModelType model_type, const std::string& client_tag) {
+  // Blank PB with just the field in it has termination symbol,
+  // handy for delimiter.
+  sync_pb::EntitySpecifics serialized_type;
+  AddDefaultFieldValue(model_type, &serialized_type);
+  std::string hash_input;
+  serialized_type.AppendToString(&hash_input);
+  hash_input.append(client_tag);
+
+  std::string encode_output;
+  CHECK(base::Base64Encode(base::SHA1HashString(hash_input), &encode_output));
+  return encode_output;
+}
+
 }  // namespace syncable
 }  // namespace syncer
diff --git a/sync/syncable/syncable_util.h b/sync/syncable/syncable_util.h
@@ -5,9 +5,11 @@
 #ifndef SYNC_SYNCABLE_SYNCABLE_UTIL_H_
 #define SYNC_SYNCABLE_SYNCABLE_UTIL_H_
 
+#include <string>
 #include <vector>
 
 #include "base/basictypes.h"
+#include "sync/internal_api/public/base/model_type.h"
 
 namespace tracked_objects {
 class Location;
@@ -35,6 +37,10 @@ bool SyncAssert(bool condition,
 int GetUnsyncedEntries(BaseTransaction* trans,
                        std::vector<int64> *handles);
 
+// Generates a fixed-length tag for the given string under the given model_type.
+std::string GenerateSyncableHash(
+    ModelType model_type, const std::string& client_tag);
+
 }  // namespace syncable
 }  // namespace syncer
 

diff --git a/sync/syncable/syncable_util_unittest.cc b/sync/syncable/syncable_util_unittest.cc
@@ -0,0 +1,32 @@
+// Copyright 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "sync/internal_api/public/base/model_type.h"
+#include "sync/syncable/syncable_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace syncer {
+namespace syncable {
+namespace {
+
+// Tests that the hashing algorithm has not changed.
+TEST(SyncableUtilTest, GenerateSyncableHash) {
+  EXPECT_EQ("OyaXV5mEzrPS4wbogmtKvRfekAI=",
+            GenerateSyncableHash(BOOKMARKS, "tag1"));
+  EXPECT_EQ("iNFQtRFQb+IZcn1kKUJEZDDkLs4=",
+            GenerateSyncableHash(PREFERENCES, "tag1"));
+  EXPECT_EQ("gO1cPZQXaM73sHOvSA+tKCKFs58=",
+            GenerateSyncableHash(AUTOFILL, "tag1"));
+
+  EXPECT_EQ("A0eYIHXM1/jVwKDDp12Up20IkKY=",
+            GenerateSyncableHash(BOOKMARKS, "tag2"));
+  EXPECT_EQ("XYxkF7bhS4eItStFgiOIAU23swI=",
+            GenerateSyncableHash(PREFERENCES, "tag2"));
+  EXPECT_EQ("GFiWzo5NGhjLlN+OyCfhy28DJTQ=",
+            GenerateSyncableHash(AUTOFILL, "tag2"));
+}
+
+}  // namespace
+}  // namespace syncer
+}  // namespace syncable