sync: Initialize entries with a valid model type

sync/engine/process_commit_response_command_unittest.cc

Index: sync/engine/process_commit_response_command_unittest.cc
diff --git a/sync/engine/process_commit_response_command_unittest.cc b/sync/engine/process_commit_response_command_unittest.cc
index f96ea5e29749b67b60c64b2ca7c10cebe4496b93..fd4fb284a57f694480e43bd30a154b27290c8dbb 100644
--- a/sync/engine/process_commit_response_command_unittest.cc
+++ b/sync/engine/process_commit_response_command_unittest.cc
@@ -8,6 +8,7 @@
 
 #include "base/location.h"
 #include "base/stringprintf.h"
+#include "sync/internal_api/public/test/test_entry_factory.h"
 #include "sync/protocol/bookmark_specifics.pb.h"
 #include "sync/protocol/sync.pb.h"
 #include "sync/sessions/sync_session.h"
@@ -55,13 +56,14 @@ class ProcessCommitResponseCommandTest : public SyncerCommandTest {
     (*mutable_routing_info())[AUTOFILL] = GROUP_DB;
 
     SyncerCommandTest::SetUp();
+
+    test_entry_factory_.reset(new TestEntryFactory(directory()));
   }
 
  protected:
 
   ProcessCommitResponseCommandTest()
-      : next_old_revision_(1),
-        next_new_revision_(4000),
+      : next_new_revision_(4000),
         next_server_position_(10000) {
   }
 
@@ -75,59 +77,21 @@ class ProcessCommitResponseCommandTest : public SyncerCommandTest {
          << "Item should have a valid (positive) server base revision";
   }
 
-  // Create an unsynced item in the database.  If item_id is a local ID, it
-  // will be treated as a create-new.  Otherwise, if it's a server ID, we'll
-  // fake the server data so that it looks like it exists on the server.
-  // Returns the methandle of the created item in |metahandle_out| if not NULL.
-  void CreateUnsyncedItem(const Id& item_id,
-                          const Id& parent_id,
-                          const string& name,
-                          bool is_folder,
-                          ModelType model_type,
-                          int64* metahandle_out) {
-    WriteTransaction trans(FROM_HERE, UNITTEST, directory());
-    Id predecessor_id;
-    ASSERT_TRUE(
-        directory()->GetLastChildIdForTest(&trans, parent_id, &predecessor_id));
-    MutableEntry entry(&trans, syncable::CREATE, parent_id, name);
-    ASSERT_TRUE(entry.good());
-    entry.Put(syncable::ID, item_id);
-    entry.Put(syncable::BASE_VERSION,
-        item_id.ServerKnows() ? next_old_revision_++ : 0);
-    entry.Put(syncable::IS_UNSYNCED, true);
-    entry.Put(syncable::IS_DIR, is_folder);
-    entry.Put(syncable::IS_DEL, false);
-    entry.Put(syncable::PARENT_ID, parent_id);
-    entry.PutPredecessor(predecessor_id);
-    sync_pb::EntitySpecifics default_specifics;
-    AddDefaultFieldValue(model_type, &default_specifics);
-    entry.Put(syncable::SPECIFICS, default_specifics);
-    if (item_id.ServerKnows()) {
-      entry.Put(syncable::SERVER_SPECIFICS, default_specifics);
-      entry.Put(syncable::SERVER_IS_DIR, is_folder);
-      entry.Put(syncable::SERVER_PARENT_ID, parent_id);
-      entry.Put(syncable::SERVER_IS_DEL, false);
-    }
-    if (metahandle_out)
-      *metahandle_out = entry.Get(syncable::META_HANDLE);
-  }
-
-  // Create a new unsynced item in the database, and synthesize a commit
-  // record and a commit response for it in the syncer session.  If item_id
-  // is a local ID, the item will be a create operation.  Otherwise, it
-  // will be an edit.
-  void CreateUnprocessedCommitResult(
+  // Create a new unsynced item in the database, and synthesize a commit record
+  // and a commit response for it in the syncer session.  If item_id is a local
+  // ID, the item will be a create operation.  Otherwise, it will be an edit.
+  int CreateUnprocessedCommitResult(

[tim (not reviewing), 2013/01/10 20:16:43]

Comment that the return value is the created metahandle.

       const Id& item_id,
       const Id& parent_id,
       const string& name,
+      bool is_folder,
       ModelType model_type,
       sessions::OrderedCommitSet *commit_set,
       sync_pb::ClientToServerMessage *commit,
       sync_pb::ClientToServerResponse *response) {
-    bool is_folder = true;
     int64 metahandle = 0;
-    CreateUnsyncedItem(item_id, parent_id, name, is_folder, model_type,
-                       &metahandle);
+    test_entry_factory_->CreateUnsyncedItem(item_id, parent_id, name,
+                                            is_folder, model_type, &metahandle);
 
     // ProcessCommitResponseCommand consumes commit_ids from the session
     // state, so we need to update that.  O(n^2) because it's a test.
@@ -135,19 +99,26 @@ class ProcessCommitResponseCommandTest : public SyncerCommandTest {
 
     WriteTransaction trans(FROM_HERE, UNITTEST, directory());
     MutableEntry entry(&trans, syncable::GET_BY_ID, item_id);
-    ASSERT_TRUE(entry.good());
+    EXPECT_TRUE(entry.good());
     entry.Put(syncable::SYNCING, true);
 
     // Add to the commit message.
+    // TODO(sync): Use the real commit-building code to construct this.
     commit->set_message_contents(ClientToServerMessage::COMMIT);
     sync_pb::SyncEntity* entity = commit->mutable_commit()->add_entries();
-    entity->set_non_unique_name(name);
-    entity->set_folder(is_folder);
-    entity->set_parent_id_string(SyncableIdToProto(parent_id));
+    entity->set_non_unique_name(entry.Get(syncable::NON_UNIQUE_NAME));
+    entity->set_folder(entry.Get(syncable::IS_DIR));
+    entity->set_parent_id_string(
+        SyncableIdToProto(entry.Get(syncable::PARENT_ID)));
     entity->set_version(entry.Get(syncable::BASE_VERSION));
     entity->mutable_specifics()->CopyFrom(entry.Get(syncable::SPECIFICS));
     entity->set_id_string(SyncableIdToProto(item_id));
 
+    if (!entry.Get(syncable::UNIQUE_CLIENT_TAG).empty()) {
+      entity->set_client_defined_unique_tag(
+          entry.Get(syncable::UNIQUE_CLIENT_TAG));
+    }
+
     // Add to the response message.
     response->set_error_code(sync_pb::SyncEnums::SUCCESS);
     sync_pb::CommitResponse_EntryResponse* entry_response =
@@ -173,6 +144,8 @@ class ProcessCommitResponseCommandTest : public SyncerCommandTest {
             response->commit().entryresponse(i).id_string());
       }
     }
+
+    return metahandle;
   }
 
   void SetLastErrorCode(sync_pb::CommitResponse::ResponseType error_code,
@@ -184,8 +157,8 @@ class ProcessCommitResponseCommandTest : public SyncerCommandTest {
   }
 
   TestIdFactory id_factory_;
+  scoped_ptr<TestEntryFactory> test_entry_factory_;
  private:
-  int64 next_old_revision_;
   int64 next_new_revision_;
   int64 next_server_position_;
   DISALLOW_COPY_AND_ASSIGN(ProcessCommitResponseCommandTest);
@@ -197,69 +170,63 @@ TEST_F(ProcessCommitResponseCommandTest, MultipleCommitIdProjections) {
   sync_pb::ClientToServerResponse response;
 
   Id bookmark_folder_id = id_factory_.NewLocalId();
-  Id bookmark_id1 = id_factory_.NewLocalId();
-  Id bookmark_id2 = id_factory_.NewLocalId();
-  Id pref_id1 = id_factory_.NewLocalId(), pref_id2 = id_factory_.NewLocalId();
-  Id autofill_id1 = id_factory_.NewLocalId();
-  Id autofill_id2 = id_factory_.NewLocalId();
-  CreateUnprocessedCommitResult(bookmark_folder_id, id_factory_.root(),
-                                "A bookmark folder", BOOKMARKS,
-                                &commit_set, &request, &response);
-  CreateUnprocessedCommitResult(bookmark_id1, bookmark_folder_id,
-                                "bookmark 1", BOOKMARKS,
-                                &commit_set, &request, &response);
-  CreateUnprocessedCommitResult(bookmark_id2, bookmark_folder_id,
-                                "bookmark 2", BOOKMARKS,
-                                &commit_set, &request, &response);
-  CreateUnprocessedCommitResult(pref_id1, id_factory_.root(),
-                                "Pref 1", PREFERENCES,
-                                &commit_set, &request, &response);
-  CreateUnprocessedCommitResult(pref_id2, id_factory_.root(),
-                                "Pref 2", PREFERENCES,
-                                &commit_set, &request, &response);
-  CreateUnprocessedCommitResult(autofill_id1, id_factory_.root(),
-                                "Autofill 1", AUTOFILL,
-                                &commit_set, &request, &response);
-  CreateUnprocessedCommitResult(autofill_id2, id_factory_.root(),
-                                "Autofill 2", AUTOFILL,
-                                &commit_set, &request, &response);
+  int bookmark_folder_handle = CreateUnprocessedCommitResult(
+      bookmark_folder_id, id_factory_.root(), "A bookmark folder", true,
+      BOOKMARKS, &commit_set, &request, &response);
+  int bookmark1_handle = CreateUnprocessedCommitResult(
+      id_factory_.NewLocalId(), bookmark_folder_id, "bookmark 1", false,
+      BOOKMARKS, &commit_set, &request, &response);
+  int bookmark2_handle = CreateUnprocessedCommitResult(
+      id_factory_.NewLocalId(), bookmark_folder_id, "bookmark 2", false,
+      BOOKMARKS, &commit_set, &request, &response);
+  int pref1_handle = CreateUnprocessedCommitResult(
+      id_factory_.NewLocalId(), id_factory_.root(), "Pref 1", false,
+      PREFERENCES, &commit_set, &request, &response);
+  int pref2_handle = CreateUnprocessedCommitResult(
+      id_factory_.NewLocalId(), id_factory_.root(), "Pref 2", false,
+      PREFERENCES, &commit_set, &request, &response);
+  int autofill1_handle = CreateUnprocessedCommitResult(
+      id_factory_.NewLocalId(), id_factory_.root(), "Autofill 1", false,
+      AUTOFILL, &commit_set, &request, &response);
+  int autofill2_handle = CreateUnprocessedCommitResult(
+      id_factory_.NewLocalId(), id_factory_.root(), "Autofill 2", false,
+      AUTOFILL, &commit_set, &request, &response);
 
   ProcessCommitResponseCommand command(commit_set, request, response);
   ExpectGroupsToChange(command, GROUP_UI, GROUP_DB);
   command.ExecuteImpl(session());
 
   syncable::ReadTransaction trans(FROM_HERE, directory());
-  Id new_fid;
-  ASSERT_TRUE(directory()->GetFirstChildId(
-          &trans, id_factory_.root(), &new_fid));
+
+  Entry b_folder(&trans, syncable::GET_BY_HANDLE, bookmark_folder_handle);

[tim (not reviewing), 2013/01/10 20:16:43]

Is there a practical advantage here to returning / using the handle instead of
the ID?  The code already passes the IDs in to CreateUnprocessedCommitResult,
seems like it'd be slightly easier to stick to IDs and not bring handles into
it?

[rlarocque, 2013/01/10 21:32:09]

The ID is renamed when the commit response is processed.

+  ASSERT_TRUE(b_folder.good());
+
+  Id new_fid = b_folder.Get(syncable::ID);
   ASSERT_FALSE(new_fid.IsRoot());
   EXPECT_TRUE(new_fid.ServerKnows());
   EXPECT_FALSE(bookmark_folder_id.ServerKnows());
   EXPECT_FALSE(new_fid == bookmark_folder_id);
-  Entry b_folder(&trans, syncable::GET_BY_ID, new_fid);
-  ASSERT_TRUE(b_folder.good());
+
   ASSERT_EQ("A bookmark folder", b_folder.Get(NON_UNIQUE_NAME))
       << "Name of bookmark folder should not change.";
   ASSERT_LT(0, b_folder.Get(BASE_VERSION))
       << "Bookmark folder should have a valid (positive) server base revision";
 
   // Look at the two bookmarks in bookmark_folder.
-  Id cid;
-  ASSERT_TRUE(directory()->GetFirstChildId(&trans, new_fid, &cid));
-  Entry b1(&trans, syncable::GET_BY_ID, cid);
-  Entry b2(&trans, syncable::GET_BY_ID, b1.GetSuccessorId());
+  Entry b1(&trans, syncable::GET_BY_HANDLE, bookmark1_handle);
+  Entry b2(&trans, syncable::GET_BY_HANDLE, bookmark2_handle);
   CheckEntry(&b1, "bookmark 1", BOOKMARKS, new_fid);
   CheckEntry(&b2, "bookmark 2", BOOKMARKS, new_fid);
   ASSERT_TRUE(b2.GetSuccessorId().IsRoot());
 
   // Look at the prefs and autofill items.
-  Entry p1(&trans, syncable::GET_BY_ID, b_folder.GetSuccessorId());
-  Entry p2(&trans, syncable::GET_BY_ID, p1.GetSuccessorId());
+  Entry p1(&trans, syncable::GET_BY_HANDLE, pref1_handle);
+  Entry p2(&trans, syncable::GET_BY_HANDLE, pref2_handle);
   CheckEntry(&p1, "Pref 1", PREFERENCES, id_factory_.root());
   CheckEntry(&p2, "Pref 2", PREFERENCES, id_factory_.root());
 
-  Entry a1(&trans, syncable::GET_BY_ID, p2.GetSuccessorId());
-  Entry a2(&trans, syncable::GET_BY_ID, a1.GetSuccessorId());
+  Entry a1(&trans, syncable::GET_BY_HANDLE, autofill1_handle);
+  Entry a2(&trans, syncable::GET_BY_HANDLE, autofill2_handle);
   CheckEntry(&a1, "Autofill 1", AUTOFILL, id_factory_.root());
   CheckEntry(&a2, "Autofill 2", AUTOFILL, id_factory_.root());
   ASSERT_TRUE(a2.GetSuccessorId().IsRoot());
@@ -282,16 +249,18 @@ TEST_F(ProcessCommitResponseCommandTest, NewFolderCommitKeepsChildOrder) {
 
   // Create the parent folder, a new item whose ID will change on commit.
   Id folder_id = id_factory_.NewLocalId();
-  CreateUnprocessedCommitResult(folder_id, id_factory_.root(), "A",
-                                BOOKMARKS,
+  CreateUnprocessedCommitResult(folder_id, id_factory_.root(),
+                                "A", true, BOOKMARKS,
                                 &commit_set, &request, &response);
 
   // Verify that the item is reachable.
   {
-    syncable::ReadTransaction trans(FROM_HERE, directory());
     Id child_id;
+    syncable::ReadTransaction trans(FROM_HERE, directory());
+    syncable::Entry root(&trans, syncable::GET_BY_ID, id_factory_.root());
+    ASSERT_TRUE(root.good());
     ASSERT_TRUE(directory()->GetFirstChildId(
-            &trans, id_factory_.root(), &child_id));
+        &trans, id_factory_.root(), &child_id));
     ASSERT_EQ(folder_id, child_id);
   }
 
@@ -303,8 +272,8 @@ TEST_F(ProcessCommitResponseCommandTest, NewFolderCommitKeepsChildOrder) {
     // Alternate between new and old child items, just for kicks.
     Id id = (i % 4 < 2) ? id_factory_.NewLocalId() : id_factory_.NewServerId();
     CreateUnprocessedCommitResult(
-        id, folder_id, base::StringPrintf("Item %d", i), BOOKMARKS,
-        &commit_set, &request, &response);
+        id, folder_id, base::StringPrintf("Item %d", i), false,
+        BOOKMARKS, &commit_set, &request, &response);
   }
   // The second 25 children will be unsynced items but NOT part of the commit
   // batch.  When the ID of the parent folder changes during the commit,
@@ -313,8 +282,9 @@ TEST_F(ProcessCommitResponseCommandTest, NewFolderCommitKeepsChildOrder) {
   for (; i < 2*batch_size; ++i) {
     // Alternate between new and old child items, just for kicks.
     Id id = (i % 4 < 2) ? id_factory_.NewLocalId() : id_factory_.NewServerId();
-    CreateUnsyncedItem(id, folder_id, base::StringPrintf("Item %d", i),
-                       false, BOOKMARKS, NULL);
+    test_entry_factory_->CreateUnsyncedItem(
+        id, folder_id, base::StringPrintf("Item %d", i),
+        false, BOOKMARKS, NULL);
   }
 
   // Process the commit response for the parent folder and the first
@@ -413,15 +383,17 @@ TEST_P(MixedResult, ExtensionActivity) {
       << "To not be lame, this test requires more than one active group.";
 
   // Bookmark item setup.
-  CreateUnprocessedCommitResult(id_factory_.NewServerId(),
-      id_factory_.root(), "Some bookmark", BOOKMARKS,
-      &commit_set, &request, &response);
+  CreateUnprocessedCommitResult(
+      id_factory_.NewServerId(),
+      id_factory_.root(), "Some bookmark", false,
+      BOOKMARKS, &commit_set, &request, &response);
   if (ShouldFailBookmarkCommit())
     SetLastErrorCode(CommitResponse::TRANSIENT_ERROR, &response);
   // Autofill item setup.
-  CreateUnprocessedCommitResult(id_factory_.NewServerId(),
-      id_factory_.root(), "Some autofill", AUTOFILL,
-      &commit_set, &request, &response);
+  CreateUnprocessedCommitResult(
+      id_factory_.NewServerId(),
+      id_factory_.root(), "Some autofill", false,
+      AUTOFILL, &commit_set, &request, &response);
   if (ShouldFailAutofillCommit())
     SetLastErrorCode(CommitResponse::TRANSIENT_ERROR, &response);