sync: Stop supporting parent IDs for some types

Makes non-bookmarks nodes no longer send up a parent ID for each of
their commits.  This is part of an update to the sync protocol that will
eventually allow both the client and server to remove support for type
root nodes.

Note that the client still expects to receive type root nodes from the
server.  That expectation will be removed in a future CL.

BUG=373869

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@280148 0039d316-1c4b-4281-b951-d872f2087c98

12 files changed, 138 insertions, 16 deletions

diff --git a/sync/engine/commit_util.cc b/sync/engine/commit_util.cc
index 36b7c06..ad29e4f 100644
--- a/sync/engine/commit_util.cc
+++ b/sync/engine/commit_util.cc
@@ -128,7 +128,10 @@ void BuildCommitItem(
   } else {
     new_parent_id = meta_entry.GetParentId();
   }
-  sync_entry->set_parent_id_string(SyncableIdToProto(new_parent_id));
+
+  if (meta_entry.ShouldMaintainHierarchy()) {
+    sync_entry->set_parent_id_string(SyncableIdToProto(new_parent_id));
+  }
 
   // If our parent has changed, send up the old one so the server
   // can correctly deal with multiple parents.
diff --git a/sync/engine/directory_commit_contribution_unittest.cc b/sync/engine/directory_commit_contribution_unittest.cc
index b342b54..894db97 100644
--- a/sync/engine/directory_commit_contribution_unittest.cc
+++ b/sync/engine/directory_commit_contribution_unittest.cc
@@ -274,6 +274,68 @@ TEST_F(DirectoryCommitContributionTest, DeletedBookmarksWithSpecifics) {
   bm_cc->CleanUp();
 }
 
+// Test that bookmarks support hierarchy.
+TEST_F(DirectoryCommitContributionTest, HierarchySupport_Bookmark) {
+
+  // Create a normal-looking bookmark item.
+  int64 bm1;
+  {
+    syncable::WriteTransaction trans(FROM_HERE, syncable::UNITTEST, dir());
+    bm1 = CreateSyncedItem(&trans, BOOKMARKS, "bm1");
+    syncable::MutableEntry e(&trans, syncable::GET_BY_HANDLE, bm1);
+
+    sync_pb::EntitySpecifics specifics;
+    sync_pb::BookmarkSpecifics* bm_specifics = specifics.mutable_bookmark();
+    bm_specifics->set_url("http://www.chrome.com");
+    bm_specifics->set_title("Chrome");
+    e.PutSpecifics(specifics);
+
+    e.PutIsDel(false);
+    e.PutIsUnsynced(true);
+
+    EXPECT_TRUE(e.ShouldMaintainHierarchy());
+  }
+
+  DirectoryTypeDebugInfoEmitter emitter(BOOKMARKS, &type_observers_);
+  scoped_ptr<DirectoryCommitContribution> bm_cc(
+      DirectoryCommitContribution::Build(dir(), BOOKMARKS, 25, &emitter));
+
+  sync_pb::ClientToServerMessage message;
+  bm_cc->AddToCommitMessage(&message);
+  const sync_pb::CommitMessage& commit_message = message.commit();
+  bm_cc->CleanUp();
+
+  ASSERT_EQ(1, commit_message.entries_size());
+  EXPECT_TRUE(commit_message.entries(0).has_parent_id_string());
+  EXPECT_FALSE(commit_message.entries(0).parent_id_string().empty());
+}
+
+// Test that preferences do not support hierarchy.
+TEST_F(DirectoryCommitContributionTest, HierarchySupport_Preferences) {
+  // Create a normal-looking prefs item.
+  int64 pref1;
+  {
+    syncable::WriteTransaction trans(FROM_HERE, syncable::UNITTEST, dir());
+    pref1 = CreateUnsyncedItem(&trans, PREFERENCES, "pref1");
+    syncable::MutableEntry e(&trans, syncable::GET_BY_HANDLE, pref1);
+
+    EXPECT_FALSE(e.ShouldMaintainHierarchy());
+  }
+
+  DirectoryTypeDebugInfoEmitter emitter(PREFERENCES, &type_observers_);
+  scoped_ptr<DirectoryCommitContribution> pref_cc(
+      DirectoryCommitContribution::Build(dir(), PREFERENCES, 25, &emitter));
+
+  sync_pb::ClientToServerMessage message;
+  pref_cc->AddToCommitMessage(&message);
+  const sync_pb::CommitMessage& commit_message = message.commit();
+  pref_cc->CleanUp();
+
+  ASSERT_EQ(1, commit_message.entries_size());
+  EXPECT_FALSE(commit_message.entries(0).has_parent_id_string());
+  EXPECT_TRUE(commit_message.entries(0).parent_id_string().empty());
+}
+
 // Creates some unsynced items, pretends to commit them, and hands back a
 // specially crafted response to the syncer in order to test commit response
 // processing.  The response simulates a succesful commit scenario.
diff --git a/sync/syncable/entry.cc b/sync/syncable/entry.cc
index 0a4a44f..5bee5ee 100644
--- a/sync/syncable/entry.cc
+++ b/sync/syncable/entry.cc
@@ -121,6 +121,10 @@ bool Entry::ShouldMaintainPosition() const {
   return kernel_->ShouldMaintainPosition();
 }
 
+bool Entry::ShouldMaintainHierarchy() const {
+  return kernel_->ShouldMaintainHierarchy();
+}
+
 std::ostream& operator<<(std::ostream& s, const Blob& blob) {
   for (Blob::const_iterator i = blob.begin(); i != blob.end(); ++i)
     s << std::hex << std::setw(2)
diff --git a/sync/syncable/entry.h b/sync/syncable/entry.h
index 619c84b..5faa6e8 100644
--- a/sync/syncable/entry.h
+++ b/sync/syncable/entry.h
@@ -245,6 +245,10 @@ class SYNC_EXPORT Entry {
   // sort ordering relative to its siblings under the same parent.
   bool ShouldMaintainPosition() const;
 
+  // Returns true if this is an entry that is expected to maintain hierarchy.
+  // ie. Whether or not the PARENT_ID field contains useful information.
+  bool ShouldMaintainHierarchy() const;
+
   Directory* dir() const;
 
   const EntryKernel GetKernelCopy() const {
diff --git a/sync/syncable/entry_kernel.cc b/sync/syncable/entry_kernel.cc
index 0821999..d945d50 100644
--- a/sync/syncable/entry_kernel.cc
+++ b/sync/syncable/entry_kernel.cc
@@ -56,6 +56,14 @@ bool EntryKernel::ShouldMaintainPosition() const {
       && !(!ref(UNIQUE_SERVER_TAG).empty() && ref(IS_DIR));
 }
 
+bool EntryKernel::ShouldMaintainHierarchy() const {
+  // We maintain hierarchy for bookmarks, device info, and top-level folders,
+  // but no other types.  Note that the Nigori node consists of a single
+  // top-level folder, so it's included in this set.
+  return (GetModelTypeFromSpecifics(ref(SPECIFICS)) == syncer::BOOKMARKS)
+      || (!ref(UNIQUE_SERVER_TAG).empty());
+}
+
 namespace {
 
 // Utility function to loop through a set of enum values and add the
diff --git a/sync/syncable/entry_kernel.h b/sync/syncable/entry_kernel.h
index 6380420..d270c4b 100644
--- a/sync/syncable/entry_kernel.h
+++ b/sync/syncable/entry_kernel.h
@@ -341,6 +341,7 @@ struct SYNC_EXPORT_PRIVATE EntryKernel {
   ModelType GetModelType() const;
   ModelType GetServerModelType() const;
   bool ShouldMaintainPosition() const;
+  bool ShouldMaintainHierarchy() const;
 
   // Dumps all kernel info into a DictionaryValue and returns it.
   // Transfers ownership of the DictionaryValue to the caller.
diff --git a/sync/test/fake_server/fake_server_entity.cc b/sync/test/fake_server/fake_server_entity.cc
index b2aa6f9..602a266 100644
--- a/sync/test/fake_server/fake_server_entity.cc
+++ b/sync/test/fake_server/fake_server_entity.cc
@@ -64,6 +64,13 @@ string FakeServerEntity::CreateId(const ModelType& model_type,
 }
 
 // static
+std::string FakeServerEntity::GetTopLevelId(const ModelType& model_type) {
+  return FakeServerEntity::CreateId(
+      model_type,
+      syncer::ModelTypeToRootTag(model_type));
+}
+
+// static
 ModelType FakeServerEntity::GetModelTypeFromId(const string& id) {
   vector<string> tokens;
   size_t token_count = Tokenize(id, kIdSeparator, &tokens);
@@ -80,8 +87,8 @@ FakeServerEntity::FakeServerEntity(const string& id,
                                    const ModelType& model_type,
                                    int64 version,
                                    const string& name)
-      : id_(id),
-        model_type_(model_type),
+      : model_type_(model_type),
+        id_(id),
         version_(version),
         name_(name) {}
 
diff --git a/sync/test/fake_server/fake_server_entity.h b/sync/test/fake_server/fake_server_entity.h
index 7f30bb0..b7c5990 100644
--- a/sync/test/fake_server/fake_server_entity.h
+++ b/sync/test/fake_server/fake_server_entity.h
@@ -26,6 +26,9 @@ class FakeServerEntity {
   static std::string CreateId(const syncer::ModelType& model_type,
                               const std::string& inner_id);
 
+  // Returns the ID string of the top level node for the specified type.
+  static std::string GetTopLevelId(const syncer::ModelType& model_type);
+
   virtual ~FakeServerEntity();
   const std::string& GetId() const;
   syncer::ModelType GetModelType() const;
@@ -51,13 +54,13 @@ class FakeServerEntity {
 
   void SerializeBaseProtoFields(sync_pb::SyncEntity* sync_entity);
 
+  // The ModelType that categorizes this entity.
+  syncer::ModelType model_type_;
+
  private:
   // The entity's ID.
   std::string id_;
 
-  // The ModelType that categorizes this entity.
-  syncer::ModelType model_type_;
-
   // The version of this entity.
   int64 version_;
 
diff --git a/sync/test/fake_server/permanent_entity.cc b/sync/test/fake_server/permanent_entity.cc
index 3ceb5c8..5b1631c 100644
--- a/sync/test/fake_server/permanent_entity.cc
+++ b/sync/test/fake_server/permanent_entity.cc
@@ -56,7 +56,7 @@ FakeServerEntity* PermanentEntity::CreateTopLevel(const ModelType& model_type) {
                                            << "invalid.";
   string server_tag = syncer::ModelTypeToRootTag(model_type);
   string name = syncer::ModelTypeToString(model_type);
-  string id = FakeServerEntity::CreateId(model_type, server_tag);
+  string id = FakeServerEntity::GetTopLevelId(model_type);
   sync_pb::EntitySpecifics entity_specifics;
   AddDefaultFieldValue(model_type, &entity_specifics);
   return new PermanentEntity(id,
diff --git a/sync/test/fake_server/unique_client_entity.cc b/sync/test/fake_server/unique_client_entity.cc
index e9ce2e9..39d3a4c 100644
--- a/sync/test/fake_server/unique_client_entity.cc
+++ b/sync/test/fake_server/unique_client_entity.cc
@@ -11,6 +11,7 @@
 #include "sync/internal_api/public/base/model_type.h"
 #include "sync/protocol/sync.pb.h"
 #include "sync/test/fake_server/fake_server_entity.h"
+#include "sync/test/fake_server/permanent_entity.h"
 
 using std::string;
 
@@ -34,7 +35,6 @@ FakeServerEntity* UniqueClientEntity::CreateNew(
                                 model_type,
                                 client_entity.version(),
                                 client_entity.name(),
-                                client_entity.parent_id_string(),
                                 client_entity.client_defined_unique_tag(),
                                 client_entity.specifics(),
                                 client_entity.ctime(),
@@ -49,7 +49,6 @@ FakeServerEntity* UniqueClientEntity::CreateUpdatedVersion(
                                 current_server_entity->GetModelType(),
                                 client_entity.version(),
                                 client_entity.name(),
-                                client_entity.parent_id_string(),
                                 client_entity.client_defined_unique_tag(),
                                 client_entity.specifics(),
                                 client_entity.ctime(),
@@ -61,13 +60,11 @@ UniqueClientEntity::UniqueClientEntity(
     const ModelType& model_type,
     int64 version,
     const string& name,
-    const string& parent_id,
     const string& client_defined_unique_tag,
     const sync_pb::EntitySpecifics& specifics,
     int64 creation_time,
     int64 last_modified_time)
     : FakeServerEntity(id, model_type, version, name),
-      parent_id_(parent_id),
       client_defined_unique_tag_(client_defined_unique_tag),
       specifics_(specifics),
       creation_time_(creation_time),
@@ -76,7 +73,7 @@ UniqueClientEntity::UniqueClientEntity(
 string UniqueClientEntity::GetParentId() const {
   // The parent ID for this type of entity should always be its ModelType's
   // root node.
-  return parent_id_;
+  return FakeServerEntity::GetTopLevelId(model_type_);
 }
 
 sync_pb::SyncEntity* UniqueClientEntity::SerializeAsProto() {
@@ -86,7 +83,7 @@ sync_pb::SyncEntity* UniqueClientEntity::SerializeAsProto() {
   sync_pb::EntitySpecifics* specifics = sync_entity->mutable_specifics();
   specifics->CopyFrom(specifics_);
 
-  sync_entity->set_parent_id_string(parent_id_);
+  sync_entity->set_parent_id_string(GetParentId());
   sync_entity->set_client_defined_unique_tag(client_defined_unique_tag_);
   sync_entity->set_ctime(creation_time_);
   sync_entity->set_mtime(last_modified_time_);
diff --git a/sync/test/fake_server/unique_client_entity.h b/sync/test/fake_server/unique_client_entity.h
index 6e876e3..fecf044 100644
--- a/sync/test/fake_server/unique_client_entity.h
+++ b/sync/test/fake_server/unique_client_entity.h
@@ -39,14 +39,12 @@ class UniqueClientEntity : public FakeServerEntity {
                      const syncer::ModelType& model_type,
                      int64 version,
                      const std::string& name,
-                     const std::string& parent_id,
                      const std::string& client_defined_unique_tag,
                      const sync_pb::EntitySpecifics& specifics,
                      int64 creation_time,
                      int64 last_modified_time);
 
-  // All member values have equivalent fields in SyncEntity.
-  std::string parent_id_;
+  // These member values have equivalent fields in SyncEntity.
   std::string client_defined_unique_tag_;
   sync_pb::EntitySpecifics specifics_;
   int64 creation_time_;
diff --git a/sync/tools/testserver/chromiumsync.py b/sync/tools/testserver/chromiumsync.py
index 661366a..2930059 100644
--- a/sync/tools/testserver/chromiumsync.py
+++ b/sync/tools/testserver/chromiumsync.py
@@ -624,6 +624,12 @@ class SyncDataModel(object):
     spec = [x for x in self._PERMANENT_ITEM_SPECS if x.tag == tag][0]
     return self._MakeCurrentId(spec.sync_type, '<server tag>%s' % tag)
 
+  def _TypeToTypeRootId(self, model_type):
+    """Returns the server ID for the type root node of the given type."""
+    tag = [x.tag for x in self._PERMANENT_ITEM_SPECS
+           if x.sync_type == model_type][0]
+    return self._ServerTagToId(tag)
+
   def _ClientTagToId(self, datatype, tag):
     """Determine the server ID from a client-unique tag.
 
@@ -840,6 +846,9 @@ class SyncDataModel(object):
     if entry.parent_id_string == ROOT_ID:
       # This is generally allowed.
       return True
+    if (not entry.HasField('parent_id_string') and
+        entry.HasField('client_defined_unique_tag')):
+      return True  # Unique client tag items do not need to specify a parent.
     if entry.parent_id_string not in self._entries:
       print 'Warning: Client sent unknown ID.  Should never happen.'
       return False
@@ -939,6 +948,11 @@ class SyncDataModel(object):
     # ID generation state is stored in 'commit_session'.
     self._RewriteIdsAsServerIds(entry, cache_guid, commit_session)
 
+    # Sets the parent ID field for a client-tagged item.  The client is allowed
+    # to not specify parents for these types of items.  The server can figure
+    # out on its own what the parent ID for this entry should be.
+    self._RewriteParentIdForUniqueClientEntry(entry)
+
     # Perform the optimistic concurrency check on the entry's version number.
     # Clients are not allowed to commit unless they indicate that they've seen
     # the most recent version of an object.
@@ -1035,6 +1049,27 @@ class SyncDataModel(object):
     datatype, old_migration_version, inner_id = parsed_id
     return self._MakeCurrentId(datatype, inner_id)
 
+  def _RewriteParentIdForUniqueClientEntry(self, entry):
+    """Sets the entry's parent ID field to the appropriate value.
+
+    The client must always set enough of the specifics of the entries it sends
+    up such that the server can identify its type.  (See crbug.com/373859)
+
+    The client is under no obligation to set the parent ID field.  The server
+    can always infer what the appropriate parent for this model type should be.
+    Having the client not send the parent ID is a step towards the removal of
+    type root nodes.  (See crbug.com/373869)
+
+    This server implements these features by "faking" the existing of a parent
+    ID early on in the commit processing.
+
+    This function has no effect on non-client-tagged items.
+    """
+    if not entry.HasField('client_defined_unique_tag'):
+      return  # Skip this processing for non-client-tagged types.
+    data_type = GetEntryType(entry)
+    entry.parent_id_string = self._TypeToTypeRootId(data_type)
+
   def TriggerMigration(self, datatypes):
     """Cause a migration to occur for a set of datatypes on this account.