Sync: Support directory entries with unset (implicit) parent ID

This is a part of a larger workitem on supporting datatypes that have implicit
permanent folders. This change prepares Sync directory to handle entries with
unset parent ID. That includes:
1) ParentChildIndex gets ability to resolve an entry without Parent ID to a
datatype root folder based on the entry type.
2) MutableEntity accepts a null Id and gets a constructor overload without
parent ID.
3) Similarly WriteNode accepts initialization with a null parent ID.
4) Added a few extra conditions and checks in the directory validation
routines to handle entries with unset parent ID.
5) Added a few new unit tests.

For now directory entries with implicit parent ID can be created only in test
code. All existing datatypes continue creating entries the old way, with parent ID.

BUG=438313

Review URL: https://codereview.chromium.org/843613002

Cr-Commit-Position: refs/heads/master@{#311090}

5 files changed, 118 insertions, 48 deletions

diff --git a/sync/internal_api/base_node.cc b/sync/internal_api/base_node.cc
index b43c6ca..4d6d56f 100644
--- a/sync/internal_api/base_node.cc
+++ b/sync/internal_api/base_node.cc
@@ -34,6 +34,8 @@ using syncable::SPECIFICS;
 // string.
 static int64 IdToMetahandle(syncable::BaseTransaction* trans,
                             const syncable::Id& id) {
+  if (id.IsNull())
+    return kInvalidId;
   syncable::Entry entry(trans, syncable::GET_BY_ID, id);
   if (!entry.good())
     return kInvalidId;
diff --git a/sync/internal_api/public/write_node.h b/sync/internal_api/public/write_node.h
index 2682d017..2d84aae 100644
--- a/sync/internal_api/public/write_node.h
+++ b/sync/internal_api/public/write_node.h
@@ -34,6 +34,7 @@ class Cryptographer;
 class WriteTransaction;
 
 namespace syncable {
+class Id;
 class Entry;
 class MutableEntry;
 }
@@ -84,6 +85,12 @@ class SYNC_EXPORT WriteNode : public BaseNode {
       const BaseNode& parent,
       const std::string& client_tag);
 
+  // InitUniqueByCreation overload for model types without hierarchy.
+  // The parent node isn't stored but is assumed to be the type root folder.
+  InitUniqueByCreationResult InitUniqueByCreation(
+      ModelType model_type,
+      const std::string& client_tag);
+
   // Looks up the type's root folder.  This is usually created by the sync
   // server during initial sync, though we do eventually wish to remove it from
   // the protocol and have the client "fake it" instead.
@@ -188,6 +195,11 @@ class SYNC_EXPORT WriteNode : public BaseNode {
 
   void* operator new(size_t size);  // Node is meant for stack use only.
 
+  InitUniqueByCreationResult InitUniqueByCreationImpl(
+      ModelType model_type,
+      const syncable::Id& parent_id,
+      const std::string& client_tag);
+
   // Helper to set the previous node.
   bool PutPredecessor(const BaseNode* predecessor) WARN_UNUSED_RESULT;
 
diff --git a/sync/internal_api/sync_backup_manager.cc b/sync/internal_api/sync_backup_manager.cc
index d626229..00615a8 100644
--- a/sync/internal_api/sync_backup_manager.cc
+++ b/sync/internal_api/sync_backup_manager.cc
@@ -83,7 +83,7 @@ void SyncBackupManager::NormalizeEntries() {
 
     if (!entry.GetId().ServerKnows())
       entry.PutId(syncable::Id::CreateFromServerId(entry.GetId().value()));
-    if (!entry.GetParentId().ServerKnows()) {
+    if (!entry.GetParentId().IsNull() && !entry.GetParentId().ServerKnows()) {
       entry.PutParentIdPropertyOnly(syncable::Id::CreateFromServerId(
           entry.GetParentId().value()));
     }
diff --git a/sync/internal_api/sync_manager_impl_unittest.cc b/sync/internal_api/sync_manager_impl_unittest.cc
index e6a1fa9..8e77e70 100644
--- a/sync/internal_api/sync_manager_impl_unittest.cc
+++ b/sync/internal_api/sync_manager_impl_unittest.cc
@@ -95,12 +95,26 @@ using syncable::kEncryptedString;
 
 namespace {
 
-// Makes a non-folder child of the root node.  Returns the id of the
+// Makes a child node under the type root folder.  Returns the id of the
 // newly-created node.
 int64 MakeNode(UserShare* share,
                ModelType model_type,
                const std::string& client_tag) {
   WriteTransaction trans(FROM_HERE, share);
+  WriteNode node(&trans);
+  WriteNode::InitUniqueByCreationResult result =
+      node.InitUniqueByCreation(model_type, client_tag);
+  EXPECT_EQ(WriteNode::INIT_SUCCESS, result);
+  node.SetIsFolder(false);
+  return node.GetId();
+}
+
+// Makes a non-folder child of the root node.  Returns the id of the
+// newly-created node.
+int64 MakeNodeWithRoot(UserShare* share,
+                       ModelType model_type,
+                       const std::string& client_tag) {
+  WriteTransaction trans(FROM_HERE, share);
   ReadNode root_node(&trans);
   root_node.InitByRootLookup();
   WriteNode node(&trans);
@@ -140,8 +154,7 @@ int64 MakeBookmarkWithParent(UserShare* share,
 // Creates the "synced" root node for a particular datatype. We use the syncable
 // methods here so that the syncer treats these nodes as if they were already
 // received from the server.
-int64 MakeServerNodeForType(UserShare* share,
-                            ModelType model_type) {
+int64 MakeTypeRoot(UserShare* share, ModelType model_type) {
   sync_pb::EntitySpecifics specifics;
   AddDefaultFieldValue(model_type, &specifics);
   syncable::WriteTransaction trans(
@@ -155,8 +168,6 @@ int64 MakeServerNodeForType(UserShare* share,
   entry.PutBaseVersion(1);
   entry.PutServerVersion(1);
   entry.PutIsUnappliedUpdate(false);
-  // TODO(stanisc): setting parent ID might be unnecessary once
-  // empty parent ID is supported.
   entry.PutParentId(syncable::Id::GetRoot());
   entry.PutServerParentId(syncable::Id::GetRoot());
   entry.PutServerIsDir(true);
@@ -306,24 +317,49 @@ TEST_F(SyncApiTest, BasicTagWrite) {
     ReadTransaction trans(FROM_HERE, user_share());
     ReadNode root_node(&trans);
     root_node.InitByRootLookup();
-    EXPECT_EQ(root_node.GetFirstChildId(), 0);
+    EXPECT_EQ(kInvalidId, root_node.GetFirstChildId());
   }
 
-  ignore_result(MakeNode(user_share(), BOOKMARKS, "testtag"));
+  ignore_result(MakeNodeWithRoot(user_share(), BOOKMARKS, "testtag"));
 
   {
     ReadTransaction trans(FROM_HERE, user_share());
     ReadNode node(&trans);
     EXPECT_EQ(BaseNode::INIT_OK,
               node.InitByClientTagLookup(BOOKMARKS, "testtag"));
+    EXPECT_NE(0, node.GetId());
 
     ReadNode root_node(&trans);
     root_node.InitByRootLookup();
-    EXPECT_NE(node.GetId(), 0);
     EXPECT_EQ(node.GetId(), root_node.GetFirstChildId());
   }
 }
 
+TEST_F(SyncApiTest, BasicTagWriteWithImplicitParent) {
+  int64 type_root = MakeTypeRoot(user_share(), PREFERENCES);
+
+  {
+    ReadTransaction trans(FROM_HERE, user_share());
+    ReadNode type_root_node(&trans);
+    EXPECT_EQ(BaseNode::INIT_OK, type_root_node.InitByIdLookup(type_root));
+    EXPECT_EQ(kInvalidId, type_root_node.GetFirstChildId());
+  }
+
+  ignore_result(MakeNode(user_share(), PREFERENCES, "testtag"));
+
+  {
+    ReadTransaction trans(FROM_HERE, user_share());
+    ReadNode node(&trans);
+    EXPECT_EQ(BaseNode::INIT_OK,
+              node.InitByClientTagLookup(PREFERENCES, "testtag"));
+    EXPECT_EQ(kInvalidId, node.GetParentId());
+
+    ReadNode type_root_node(&trans);
+    EXPECT_EQ(BaseNode::INIT_OK, type_root_node.InitByIdLookup(type_root));
+    EXPECT_EQ(node.GetId(), type_root_node.GetFirstChildId());
+  }
+}
+
 TEST_F(SyncApiTest, ModelTypesSiloed) {
   {
     WriteTransaction trans(FROM_HERE, user_share());
@@ -332,9 +368,9 @@ TEST_F(SyncApiTest, ModelTypesSiloed) {
     EXPECT_EQ(root_node.GetFirstChildId(), 0);
   }
 
-  ignore_result(MakeNode(user_share(), BOOKMARKS, "collideme"));
-  ignore_result(MakeNode(user_share(), PREFERENCES, "collideme"));
-  ignore_result(MakeNode(user_share(), AUTOFILL, "collideme"));
+  ignore_result(MakeNodeWithRoot(user_share(), BOOKMARKS, "collideme"));
+  ignore_result(MakeNodeWithRoot(user_share(), PREFERENCES, "collideme"));
+  ignore_result(MakeNodeWithRoot(user_share(), AUTOFILL, "collideme"));
 
   {
     ReadTransaction trans(FROM_HERE, user_share());
@@ -537,7 +573,7 @@ TEST_F(SyncApiTest, WriteEncryptedTitle) {
 }
 
 TEST_F(SyncApiTest, BaseNodeSetSpecifics) {
-  int64 child_id = MakeNode(user_share(), BOOKMARKS, "testtag");
+  int64 child_id = MakeNodeWithRoot(user_share(), BOOKMARKS, "testtag");
   WriteTransaction trans(FROM_HERE, user_share());
   WriteNode node(&trans);
   EXPECT_EQ(BaseNode::INIT_OK, node.InitByIdLookup(child_id));
@@ -553,7 +589,7 @@ TEST_F(SyncApiTest, BaseNodeSetSpecifics) {
 }
 
 TEST_F(SyncApiTest, BaseNodeSetSpecificsPreservesUnknownFields) {
-  int64 child_id = MakeNode(user_share(), BOOKMARKS, "testtag");
+  int64 child_id = MakeNodeWithRoot(user_share(), BOOKMARKS, "testtag");
   WriteTransaction trans(FROM_HERE, user_share());
   WriteNode node(&trans);
   EXPECT_EQ(BaseNode::INIT_OK, node.InitByIdLookup(child_id));
@@ -585,7 +621,7 @@ TEST_F(SyncApiTest, EmptyTags) {
 
 // Test counting nodes when the type's root node has no children.
 TEST_F(SyncApiTest, GetTotalNodeCountEmpty) {
-  int64 type_root = MakeServerNodeForType(user_share(), BOOKMARKS);
+  int64 type_root = MakeTypeRoot(user_share(), BOOKMARKS);
   {
     ReadTransaction trans(FROM_HERE, user_share());
     ReadNode type_root_node(&trans);
@@ -597,7 +633,7 @@ TEST_F(SyncApiTest, GetTotalNodeCountEmpty) {
 
 // Test counting nodes when there is one child beneath the type's root.
 TEST_F(SyncApiTest, GetTotalNodeCountOneChild) {
-  int64 type_root = MakeServerNodeForType(user_share(), BOOKMARKS);
+  int64 type_root = MakeTypeRoot(user_share(), BOOKMARKS);
   int64 parent = MakeFolderWithParent(user_share(), BOOKMARKS, type_root, NULL);
   {
     ReadTransaction trans(FROM_HERE, user_share());
@@ -615,7 +651,7 @@ TEST_F(SyncApiTest, GetTotalNodeCountOneChild) {
 // Test counting nodes when there are multiple children beneath the type root,
 // and one of those children has children of its own.
 TEST_F(SyncApiTest, GetTotalNodeCountMultipleChildren) {
-  int64 type_root = MakeServerNodeForType(user_share(), BOOKMARKS);
+  int64 type_root = MakeTypeRoot(user_share(), BOOKMARKS);
   int64 parent = MakeFolderWithParent(user_share(), BOOKMARKS, type_root, NULL);
   ignore_result(MakeFolderWithParent(user_share(), BOOKMARKS, type_root, NULL));
   int64 child1 = MakeFolderWithParent(user_share(), BOOKMARKS, parent, NULL);
@@ -834,8 +870,8 @@ class SyncManagerTest : public testing::Test,
     if (initialization_succeeded_) {
       for (ModelSafeRoutingInfo::iterator i = routing_info.begin();
            i != routing_info.end(); ++i) {
-        type_roots_[i->first] = MakeServerNodeForType(
-            sync_manager_.GetUserShare(), i->first);
+        type_roots_[i->first] =
+            MakeTypeRoot(sync_manager_.GetUserShare(), i->first);
       }
     }
 
@@ -1126,13 +1162,13 @@ TEST_F(SyncManagerTest, EncryptDataTypesWithData) {
   }
   // Next batch_size nodes are a different type and on their own.
   for (; i < 2*batch_size; ++i) {
-    MakeNode(sync_manager_.GetUserShare(), SESSIONS,
-             base::StringPrintf("%" PRIuS "", i));
+    MakeNodeWithRoot(sync_manager_.GetUserShare(), SESSIONS,
+                     base::StringPrintf("%" PRIuS "", i));
   }
   // Last batch_size nodes are a third type that will not need encryption.
   for (; i < 3*batch_size; ++i) {
-    MakeNode(sync_manager_.GetUserShare(), THEMES,
-             base::StringPrintf("%" PRIuS "", i));
+    MakeNodeWithRoot(sync_manager_.GetUserShare(), THEMES,
+                     base::StringPrintf("%" PRIuS "", i));
   }
 
   {
@@ -1615,12 +1651,10 @@ TEST_F(SyncManagerTest, EncryptBookmarksWithLegacyData) {
   std::string url2 = "http://www.bla.com";
 
   // Create a bookmark using the legacy format.
-  int64 node_id1 = MakeNode(sync_manager_.GetUserShare(),
-      BOOKMARKS,
-      "testtag");
-  int64 node_id2 = MakeNode(sync_manager_.GetUserShare(),
-      BOOKMARKS,
-      "testtag2");
+  int64 node_id1 =
+      MakeNodeWithRoot(sync_manager_.GetUserShare(), BOOKMARKS, "testtag");
+  int64 node_id2 =
+      MakeNodeWithRoot(sync_manager_.GetUserShare(), BOOKMARKS, "testtag2");
   {
     WriteTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
     WriteNode node(&trans);
@@ -2690,7 +2724,7 @@ TEST_F(SyncManagerTest, PurgeUnappliedTypes) {
   AddDefaultFieldValue(BOOKMARKS, &bm_specifics);
   int pref1_meta = MakeServerNode(
       share, PREFERENCES, "pref1", "hash1", pref_specifics);
-  int64 pref2_meta = MakeNode(share, PREFERENCES, "pref2");
+  int64 pref2_meta = MakeNodeWithRoot(share, PREFERENCES, "pref2");
   int pref3_meta = MakeServerNode(
       share, PREFERENCES, "pref3", "hash3", pref_specifics);
   int pref4_meta = MakeServerNode(
diff --git a/sync/internal_api/write_node.cc b/sync/internal_api/write_node.cc
index 4aab598..cc445e4 100644
--- a/sync/internal_api/write_node.cc
+++ b/sync/internal_api/write_node.cc
@@ -337,6 +337,7 @@ bool WriteNode::InitBookmarkByCreation(const BaseNode& parent,
   }
 
   syncable::Id parent_id = parent.GetEntry()->GetId();
+  DCHECK(!parent_id.IsNull());
 
   // Start out with a dummy name.  We expect
   // the caller to set a meaningful name after creation.
@@ -352,8 +353,26 @@ bool WriteNode::InitBookmarkByCreation(const BaseNode& parent,
   // Entries are untitled folders by default.
   entry_->PutIsDir(true);
 
-  // Now set the predecessor, which sets IS_UNSYNCED as necessary.
-  return PutPredecessor(predecessor);
+  if (!PutPredecessor(predecessor)) {
+    return false;
+  }
+
+  // Mark this entry as unsynced, to wake up the syncer.
+  MarkForSyncing();
+  return true;
+}
+
+WriteNode::InitUniqueByCreationResult WriteNode::InitUniqueByCreation(
+    ModelType model_type,
+    const BaseNode& parent,
+    const std::string& tag) {
+  return InitUniqueByCreationImpl(model_type, parent.GetEntry()->GetId(), tag);
+}
+
+WriteNode::InitUniqueByCreationResult WriteNode::InitUniqueByCreation(
+    ModelType model_type,
+    const std::string& tag) {
+  return InitUniqueByCreationImpl(model_type, syncable::Id(), tag);
 }
 
 // Create a new node with default properties and a client defined unique tag,
@@ -362,9 +381,9 @@ bool WriteNode::InitBookmarkByCreation(const BaseNode& parent,
 // we will attempt to undelete the node.
 // TODO(chron): Code datatype into hash tag.
 // TODO(chron): Is model type ever lost?
-WriteNode::InitUniqueByCreationResult WriteNode::InitUniqueByCreation(
+WriteNode::InitUniqueByCreationResult WriteNode::InitUniqueByCreationImpl(
     ModelType model_type,
-    const BaseNode& parent,
+    const syncable::Id& parent_id,
     const std::string& tag) {
   // This DCHECK will only fail if init is called twice.
   DCHECK(!entry_);
@@ -375,8 +394,6 @@ WriteNode::InitUniqueByCreationResult WriteNode::InitUniqueByCreation(
 
   const std::string hash = syncable::GenerateSyncableHash(model_type, tag);
 
-  syncable::Id parent_id = parent.GetEntry()->GetId();
-
   // Start out with a dummy name.  We expect
   // the caller to set a meaningful name after creation.
   string dummy(kDefaultNameForNewNodes);
@@ -440,10 +457,13 @@ WriteNode::InitUniqueByCreationResult WriteNode::InitUniqueByCreation(
   // We don't support directory and tag combinations.
   entry_->PutIsDir(false);
 
-  // Now set the predecessor, which sets IS_UNSYNCED as necessary.
-  bool success = PutPredecessor(NULL);
-  if (!success)
-    return INIT_FAILED_SET_PREDECESSOR;
+  if (!parent_id.IsNull()) {
+    if (!PutPredecessor(NULL))
+      return INIT_FAILED_SET_PREDECESSOR;
+  }
+
+  // Mark this entry as unsynced, to wake up the syncer.
+  MarkForSyncing();
 
   return INIT_SUCCESS;
 }
@@ -457,6 +477,7 @@ bool WriteNode::SetPosition(const BaseNode& new_parent,
   }
 
   syncable::Id new_parent_id = new_parent.GetEntry()->GetId();
+  DCHECK(!new_parent_id.IsNull());
 
   // Filter out redundant changes if both the parent and the predecessor match.
   if (new_parent_id == entry_->GetParentId()) {
@@ -469,8 +490,13 @@ bool WriteNode::SetPosition(const BaseNode& new_parent,
 
   entry_->PutParentId(new_parent_id);
 
-  // Now set the predecessor, which sets IS_UNSYNCED as necessary.
-  return PutPredecessor(predecessor);
+  if (!PutPredecessor(predecessor)) {
+    return false;
+  }
+
+  // Mark this entry as unsynced, to wake up the syncer.
+  MarkForSyncing();
+  return true;
 }
 
 void WriteNode::SetAttachmentMetadata(
@@ -505,14 +531,10 @@ void WriteNode::Drop() {
 }
 
 bool WriteNode::PutPredecessor(const BaseNode* predecessor) {
+  DCHECK(!entry_->GetParentId().IsNull());
   syncable::Id predecessor_id = predecessor ?
       predecessor->GetEntry()->GetId() : syncable::Id();
-  if (!entry_->PutPredecessor(predecessor_id))
-    return false;
-  // Mark this entry as unsynced, to wake up the syncer.
-  MarkForSyncing();
-
-  return true;
+  return entry_->PutPredecessor(predecessor_id);
 }
 
 void WriteNode::MarkForSyncing() {