Allow remote SyncData without valid ID

SyncableServices don't actually consume this field, the only caller
to GetId() being GenericChangeProcessor. Changing the API towards
SyncableServices would require a huge refactoring, so let's instead
allow that SyncData may not be populated, and DCHECK-fail lazily on
read (in case a SyncableService implementation is changed in the
future to consume this field).

The rationale behind is that future patches will integrate
SyncableService instantes within the USS framework, where the ID
doesn't have any semantics.

As implementation detail: because local vs remote was previously
implemented verifying id_ != kInvalidId, we need to instead
introduce a dedicated bool member.

Bug: 870624
Change-Id: I75aa92e69a861ea6af58327a14406bc086233043
Reviewed-on: https://chromium-review.googlesource.com/1205393
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594646}

components/sync/model/sync_data.cc

@@ -39,14 +39,16 @@ void SyncData::ImmutableSyncEntityTraits::Swap(sync_pb::SyncEntity* t1,
   t1->Swap(t2);
 }
 
-SyncData::SyncData() : id_(kInvalidId), is_valid_(false) {}
+SyncData::SyncData() : id_(kInvalidId), is_local_(false), is_valid_(false) {}
 
-SyncData::SyncData(int64_t id,
+SyncData::SyncData(bool is_local,
+                   int64_t id,
                    sync_pb::SyncEntity* entity,
                    const base::Time& remote_modification_time)
     : id_(id),
       remote_modification_time_(remote_modification_time),
       immutable_entity_(entity),
+      is_local_(is_local),
       is_valid_(true) {}
 
 SyncData::SyncData(const SyncData& other) = default;
@@ -69,7 +71,7 @@ SyncData SyncData::CreateLocalData(const std::string& sync_tag,
   entity.set_client_defined_unique_tag(sync_tag);
   entity.set_non_unique_name(non_unique_title);
   entity.mutable_specifics()->CopyFrom(specifics);
-  return SyncData(kInvalidId, &entity, base::Time());
+  return SyncData(/*is_local=*/true, kInvalidId, &entity, base::Time());
 }
 
 // Static.
@@ -78,11 +80,10 @@ SyncData SyncData::CreateRemoteData(
     const sync_pb::EntitySpecifics& specifics,
     const base::Time& modification_time,
     const std::string& client_tag_hash) {
-  DCHECK_NE(id, kInvalidId);
   sync_pb::SyncEntity entity;
   entity.mutable_specifics()->CopyFrom(specifics);
   entity.set_client_defined_unique_tag(client_tag_hash);
-  return SyncData(id, &entity, modification_time);
+  return SyncData(/*is_local=*/false, id, &entity, modification_time);
 }
 
 bool SyncData::IsValid() const {
@@ -104,7 +105,7 @@ const std::string& SyncData::GetTitle() const {
 }
 
 bool SyncData::IsLocal() const {
-  return id_ == kInvalidId;
+  return is_local_;
 }
 
 std::string SyncData::ToString() const {
@@ -125,7 +126,7 @@ std::string SyncData::ToString() const {
   }
 
   SyncDataRemote sync_data_remote(*this);
-  std::string id = base::Int64ToString(sync_data_remote.GetId());
+  std::string id = base::Int64ToString(sync_data_remote.id_);
   return "{ isLocal: false, type: " + type + ", specifics: " + specifics +
          ", id: " + id + "}";
 }
@@ -156,6 +157,8 @@ const base::Time& SyncDataRemote::GetModifiedTime() const {
 }
 
 int64_t SyncDataRemote::GetId() const {
+  DCHECK(!IsLocal());
+  DCHECK_NE(id_, kInvalidId);
   return id_;
 }
 

components/sync/model/sync_data.h

@@ -109,21 +109,26 @@ class SyncData {
   using ImmutableSyncEntity =
       Immutable<sync_pb::SyncEntity, ImmutableSyncEntityTraits>;
 
-  // Equal to kInvalidId iff this is local.
   int64_t id_;
 
   // This may be null if the SyncData represents a deleted item.
+  // TODO(crbug.com/681921): Remove when directory-based sessions sync is
+  // removed, the only remaining reader.
   base::Time remote_modification_time_;
 
   // The actual shared sync entity being held.
   ImmutableSyncEntity immutable_entity_;
 
  private:
+  // Whether this SyncData represents a local change.
+  bool is_local_;
+
   // Whether this SyncData holds valid data.
   bool is_valid_;
 
   // Clears |entity|.
-  SyncData(int64_t id,
+  SyncData(bool is_local_,
+           int64_t id,
            sync_pb::SyncEntity* entity,
            const base::Time& remote_modification_time);
 };
@@ -153,13 +158,17 @@ class SyncDataRemote : public SyncData {
 
   // Return the last motification time according to the server. This may be null
   // if the SyncData represents a deleted item.
+  // TODO(crbug.com/681921): Remove when directory-based sessions sync is
+  // removed, the only remaining reader.
   const base::Time& GetModifiedTime() const;
 
-  int64_t GetId() const;
-
   // Returns the tag hash value. May not always be present, in which case an
   // empty string will be returned.
   const std::string& GetClientTagHash() const;
+
+  // Deprecated: might not be populated in SyncableService API.
+  // TODO(crbug.com/870624): Remove when directory is removed.
+  int64_t GetId() const;
 };
 
 // gmock printer helper.

components/sync/model/sync_data_unittest.cc

@@ -10,6 +10,7 @@
 #include "base/message_loop/message_loop.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/sync/protocol/sync.pb.h"
+#include "components/sync/syncable/base_node.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using std::string;
@@ -66,6 +67,16 @@ TEST_F(SyncDataTest, CreateRemoteData) {
   EXPECT_TRUE(data.GetSpecifics().has_preference());
 }
 
+TEST_F(SyncDataTest, CreateRemoteDataWithInvalidId) {
+  specifics.mutable_preference();
+  SyncData data =
+      SyncData::CreateRemoteData(kInvalidId, specifics, kLastModifiedTime);
+  EXPECT_TRUE(data.IsValid());
+  EXPECT_FALSE(data.IsLocal());
+  EXPECT_EQ(kLastModifiedTime, SyncDataRemote(data).GetModifiedTime());
+  EXPECT_TRUE(data.GetSpecifics().has_preference());
+}
+
 }  // namespace
 
 }  // namespace syncer