Basic implementation of the Sync C++ fake server

sync/internal_api/test/fake_server.cc

Index: sync/internal_api/test/fake_server.cc
diff --git a/sync/internal_api/test/fake_server.cc b/sync/internal_api/test/fake_server.cc
new file mode 100644
index 0000000000000000000000000000000000000000..713fcddfe2b9e9c0ab248c3d12e532eb0a8ed1cf
--- /dev/null
+++ b/sync/internal_api/test/fake_server.cc
@@ -0,0 +1,311 @@
+// Copyright 2013 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/test/fake_server.h"
+
+#include <limits>
+#include <string>
+#include <vector>
+
+#include "base/basictypes.h"
+#include "base/logging.h"
+#include "base/strings/string_number_conversions.h"
+#include "sync/internal_api/public/base/model_type.h"
+#include "sync/protocol/sync.pb.h"
+
+using std::string;
+
+// The PermanentItemSpec parent_tag for a root node.
+static const std::string kRootParentTag = "0";

[rlarocque, 2013/12/18 20:22:01]

I'm not 100% sure about this, but I think this line creates a static
initializer.

Declaring this as a char[] would be a bit uglier, but would almost certainly
prevent the creation of a static initializer.

On the other hand, this is test code that only gets compiled into a test binary,
so I don't know if we care either way.

[pval...(no longer on Chromium), 2014/01/03 23:25:31]

I'm fine with leaving it here if there are no awful consequences. Should this be
in the syncer namespace instead?

[rlarocque, 2014/01/04 00:06:24]

The namespace doesn't matter too much.  Because it's declared file static it
will be hidden from all contexts outside of this file anyway.

+
+namespace syncer {
+
+FakeServer::UpdateSieve::UpdateSieve(
+    const sync_pb::GetUpdatesMessage& get_updates_message)
+      : get_updates_message_(get_updates_message) {
+  DCHECK_GT(get_updates_message.from_progress_marker_size(), 0);
+
+  int64 min_version = std::numeric_limits<int64>::max();
+  for (int i = 0; i < get_updates_message_.from_progress_marker_size(); i++) {
+    sync_pb::DataTypeProgressMarker marker =
+        get_updates_message_.from_progress_marker(i);
+
+    int64 version;
+    if (!base::StringToInt64(marker.token(), &version)) {
+      version = 0;
+    }
+
+    state_[marker.data_type_id()] = version;
+
+    if (version < min_version)
+      min_version = version;
+  }
+
+  min_version_ = min_version;
+  state_[ModelTypeFromInt(TOP_LEVEL_FOLDER)] = min_version_;

[rlarocque, 2013/12/18 20:22:01]

This looks sketchy to me.

TOP_LEVEL_FOLDER is enough of an exception that it might be better keep it out
of the map and (if necessary) store its version in a separate member variable.

You might need more if statements in ClientWantsItem(), but you would need less
special case code here and in UpdateProgressMarkers.  This seems less
error-prone.

FWIW, the client is slightly inconsistent about this, but in general it tries to
treat TOP_LEVEL_FOLDER items as having the same type as their children.  So the
node with SERVER_UNIQUE_TAG of google_chrome_preferences would often be observed
as having type PREFERENCES.

[pval...(no longer on Chromium), 2014/01/03 23:25:31]

I've removed handling of TOP_LEVEL_FOLDER since it never goes over the wire and,
AFAIK, will never show up here.

+}
+
+FakeServer::UpdateSieve::~UpdateSieve() { }
+
+void FakeServer::UpdateSieve::UpdateProgressMarkers(
+    int64 new_version,
+    sync_pb::GetUpdatesResponse* get_updates_response) {
+  for (TypeIdToVersionMap::iterator it = state_.begin(); it != state_.end();
+       ++it) {
+    if (it->first == TOP_LEVEL_FOLDER)
+      continue;
+
+    sync_pb::DataTypeProgressMarker* new_marker =
+        get_updates_response->add_new_progress_marker();
+    new_marker->set_data_type_id(it->first);
+
+    int64 version = new_version > state_[new_marker->data_type_id()] ?
+        new_version : state_[new_marker->data_type_id()];
+    new_marker->set_token(base::Int64ToString(version));
+  }
+}
+
+bool FakeServer::UpdateSieve::ClientWantsItem(
+  const sync_pb::SyncEntity& entity) {
+  ModelType model_type = GetModelType(entity);
+  int data_type_id = GetSpecificsFieldNumberFromModelType(model_type);
+  return state_[data_type_id] < entity.version();
+}
+
+int64 FakeServer::UpdateSieve::GetMinVersion() {
+  return min_version_;
+}
+
+std::vector<int> FakeServer::UpdateSieve::GetFirstTimeTypes() {
+  std::vector<int> types;
+
+  for (TypeIdToVersionMap::iterator it = state_.begin(); it != state_.end();
+       ++it) {
+    if (state_[it->first] == 0)

[rlarocque, 2013/12/18 20:22:01]

Could TOP_LEVEL_FOLDER wind up in this list of types?  Would that make sense?

[pval...(no longer on Chromium), 2014/01/03 23:25:31]

Not anymore (see comment above about removing it entirely).

+      types.push_back(it->first);
+  }
+
+  return types;
+}
+
+FakeServer::FakeServer() : initialized_(false), version_(0) { }
+
+FakeServer::~FakeServer() { }
+
+bool FakeServer::Init() {
+  // Use dummy values for these fields.
+  keystore_keys_.push_back("1111111111111111");
+  birthday_ = "1234567890";
+
+  initialized_ = PopulatePermanentItemSpecs();
+  return initialized_;
+}
+
+bool FakeServer::PopulatePermanentItemSpecs() {
+  ModelTypeSet protocol_types = ProtocolTypes();
+  for (ModelTypeSet::Iterator it = protocol_types.First(); it.Good();
+       it.Inc()) {
+    ModelType model_type = it.Get();
+
+    PermanentItemSpec spec;
+    spec.tag = ModelTypeToRootTag(model_type);
+    spec.name = ModelTypeToString(model_type);
+    spec.parent_tag = kRootParentTag;

[rlarocque, 2013/12/18 20:22:01]

It seems you're using tags as IDs?

The root node's ID should be "r".

[pval...(no longer on Chromium), 2014/01/03 23:25:31]

Is it only "r" on the client side?

It appears that "0" is used for the server:
https://cs.corp.google.com/#piper///depot/google3/java/com/google/personaliza...

https://code.google.com/p/chromium/codesearch#chromium/src/sync/syncable/sync...

The current Python test server also uses "0":
https://code.google.com/p/chromium/codesearch#chromium/src/sync/tools/testser...

[rlarocque, 2014/01/04 00:06:24]

Looks like you're right.  The server refers to is as "0".

+    spec.sync_type = model_type;
+    spec.create_by_default = true;
+    permanent_item_specs_.push_back(spec);
+  }
+
+  PermanentItemSpec bookmark_bar_spec;
+  bookmark_bar_spec.tag = "bookmark_bar";
+  bookmark_bar_spec.name = "Bookmark Bar";
+  bookmark_bar_spec.parent_tag = "google_chrome_bookmarks";
+  bookmark_bar_spec.sync_type = BOOKMARKS;
+  bookmark_bar_spec.create_by_default = true;
+  permanent_item_specs_.push_back(bookmark_bar_spec);
+
+  PermanentItemSpec other_bookmarks_spec;
+  other_bookmarks_spec.tag = "other_bookmarks";
+  other_bookmarks_spec.name = "Other Bookmarks";
+  other_bookmarks_spec.parent_tag = "google_chrome_bookmarks";
+  other_bookmarks_spec.sync_type = BOOKMARKS;
+  other_bookmarks_spec.create_by_default = true;
+  permanent_item_specs_.push_back(other_bookmarks_spec);
+

[rlarocque, 2013/12/18 20:22:01]

In certain situations, you should create the mobile bookmarks folder, too.  If
you see your tests crashing in strange ways on mobile clients, this may be the
reason why.  If they don't crash, feel free to ignore this comment and implement
that feature later.

[albertb, 2013/12/18 20:45:30]

Specifically, the situation when you need to create the mobile bookmark folder
is when ClientToServerMessage.get_updates.create_mobile_bookmarks_folder ==
true.

[pval...(no longer on Chromium), 2014/01/03 23:25:31]

Thanks; I added a TODO for this.

+  return true;
+}
+
+void FakeServer::CreateDefaultPermanentItems(
+    const std::vector<int>& first_time_type_ids) {
+  for (size_t i = 0; i < permanent_item_specs_.size(); i++) {

[rlarocque, 2013/12/18 20:22:01]

Why is the creation of permanent items a two-step process?

Rather than creating a bunch of PermanentItemSpecs then iterating over them to
transform them into SyncEntities, couldn't you just create them as SyncEntities
from the start?

[pval...(no longer on Chromium), 2014/01/03 23:25:31]

This was a result of following the Python server's implementation too closely.
Your idea is great and it's cleaned up this code immensely. As a result, I've
also removed the Init() method.

+    PermanentItemSpec spec = permanent_item_specs_[i];
+
+    int spec_id = GetSpecificsFieldNumberFromModelType(spec.sync_type);
+    bool first_time = std::find(first_time_type_ids.begin(),
+        first_time_type_ids.end(), spec_id) != first_time_type_ids.end();
+
+    if (first_time && spec.create_by_default) {
+      CreatePermanentItem(spec);
+    }
+  }
+}
+
+void FakeServer::CreatePermanentItem(const PermanentItemSpec& spec) {
+  sync_pb::SyncEntity entity;
+  entity.set_id_string(spec.tag);
+  entity.set_non_unique_name(spec.name);
+  entity.set_name(spec.name);
+  entity.set_server_defined_unique_tag(spec.tag);
+  entity.set_folder(true);
+  entity.set_deleted(false);
+
+  if (spec.parent_tag == kRootParentTag) {
+    entity.set_parent_id_string(kRootParentTag);
+  } else {
+    for (size_t i = 0; i < permanent_item_specs_.size(); i++) {

[rlarocque, 2013/12/18 20:22:01]

nit: You should prefer to use STL iterators rather than indexes where possible.

[pval...(no longer on Chromium), 2014/01/03 23:25:31]

Done.

+      if (spec.parent_tag == permanent_item_specs_[i].tag) {
+        entity.set_parent_id_string(permanent_item_specs_[i].tag);

[rlarocque, 2013/12/18 20:22:01]

This looks like it's redundant.  Can't you just call
entity.set_parent_id_string(spec.parent_tag) and skip this iteration?

[pval...(no longer on Chromium), 2014/01/03 23:25:31]

Yes. I've made this change in the new version.

+        // Use a dummy value here.
+        entity.set_position_in_parent(1337);

[rlarocque, 2013/12/18 20:22:01]

I think this should be unset on all items that are neither bookmarks nor
bookmark folders.

[pval...(no longer on Chromium), 2014/01/03 23:25:31]

Done.

+      }
+    }
+
+    if (entity.parent_id_string().empty()) {
+      NOTREACHED() << "The permanent item's parent node was not found.";
+    }
+  }
+
+  sync_pb::EntitySpecifics* specifics = entity.mutable_specifics();
+  AddDefaultFieldValue(spec.sync_type, specifics);
+
+  SaveEntity(entity);
+}
+
+void FakeServer::SaveEntity(sync_pb::SyncEntity entity) {
+  version_++;
+  entity.set_version(version_);
+  entity.set_sync_timestamp(version_);
+
+  sync_pb::SyncEntity original_entity = entities_[entity.id_string()];
+  entity.set_originator_cache_guid(original_entity.originator_cache_guid());

[albertb, 2013/12/18 20:45:30]

The client should be setting originator_cache_guid, we never modify that field
on the server.

[rlarocque, 2013/12/18 21:26:49]

Are you sure?  I'm pretty sure we're not setting it on the client.

"git grep set_originator_cache_guid" turns up nothing but unit tests on the
client.

[albertb, 2013/12/18 21:47:30]

Ah, I was slightly confused. Looking at the code:

- Both originator_cache_guid are only relevant for CLIENT types (eg. bookmarks)
- On commit, we grab the client_id parameter from the URL and use it to derive
IdString
- On GetUpdates, we read client_id from the IdString and set
originator_cache_guid on the SyncEntity

The same applies to originator_client_item_id.

What this means is that for bookmarks, on the first commit (ie. version==0),
FakeServer should:
- Set originator_cache_guid to the client_id parameter from the request URL
- Set originator_client_item_id to the IdString sent up by the client.

For all other commits, FakeServer shouldn't touch that field.

[pval...(no longer on Chromium), 2014/01/03 23:25:31]

Thanks for the explanation.

I will follow this commit logic when I implement the commit command.

I based this code on the Python test server. It appears this is done so that the
server always maintains both fields. Is this right, wrong, or simply
unnecessary?

https://code.google.com/p/chromium/codesearch#chromium/src/sync/tools/testser...

[albertb, 2014/01/06 17:25:16]

That does look correct. The server should preserve the initial value (ie.
version==0) of those fields, regardless of whether further commits modify or
omit them.

+  entity.set_originator_client_item_id(

[albertb, 2013/12/18 20:45:30]

Same comment about originator_client_item_id.

+      original_entity.originator_client_item_id());
+
+  entities_[entity.id_string()] = entity;
+}
+
+string FakeServer::HandleCommand(string request, int* error_code,
+                                 int* response_code) {
+  DCHECK(initialized_);
+  sync_pb::ClientToServerMessage message;
+  DCHECK(message.ParseFromString(request));
+
+  switch (message.message_contents()) {
+    case sync_pb::ClientToServerMessage::GET_UPDATES:
+      *error_code = 0;
+      *response_code = 200;
+      return HandleGetUpdatesRequest(message).SerializeAsString();
+    case sync_pb::ClientToServerMessage::COMMIT:
+      *error_code = 0;
+      *response_code = 200;
+      return HandleCommitRequest(message).SerializeAsString();
+    default:
+      *error_code = -1;
+      *response_code = 400;
+      return "";
+  }
+}
+
+bool SyncEntityVersionComparator(const sync_pb::SyncEntity& first,
+                                 const sync_pb::SyncEntity& second) {
+  return first.version() < second.version();
+}
+
+sync_pb::ClientToServerResponse FakeServer::HandleGetUpdatesRequest(
+    const sync_pb::ClientToServerMessage& message) {
+  sync_pb::ClientToServerResponse response;
+  response.set_error_code(sync_pb::SyncEnums::SUCCESS);
+  response.set_store_birthday(birthday_);
+
+  sync_pb::GetUpdatesResponse* get_updates_response =
+      response.mutable_get_updates();
+  get_updates_response->set_changes_remaining(0);

[albertb, 2013/12/18 20:45:30]

Will all SyncEntities always fit in a single response? Shouldn't FakeServer
respect GetUpdatesMessage.batch_size?

[pval...(no longer on Chromium), 2014/01/03 23:25:31]

I've added a TODO for this.

+
+  UpdateSieve sieve(message.get_updates());
+  CreateDefaultPermanentItems(sieve.GetFirstTimeTypes());
+
+  int64 min_version = sieve.GetMinVersion();
+
+  // use std::transform instead?
+  std::vector<sync_pb::SyncEntity> filtered_entities;
+  for (EntityMap::iterator it = entities_.begin(); it != entities_.end();
+       ++it) {
+    sync_pb::SyncEntity entity = it->second;
+    if (entity.version() > min_version) {
+      filtered_entities.push_back(entity);
+    }
+  }
+
+  std::sort(filtered_entities.begin(), filtered_entities.end(),
+            SyncEntityVersionComparator);
+
+  bool sending_nigori = false;
+  for (size_t i = 0; i < filtered_entities.size(); i++) {
+    if (sieve.ClientWantsItem(filtered_entities[i])) {
+      sync_pb::SyncEntity* entity = get_updates_response->add_entries();
+      entity->CopyFrom(filtered_entities[i]);
+
+      if (entity->name() == "Nigori")
+        sending_nigori = true;
+    }
+  }
+
+  if (sending_nigori || message.get_updates().need_encryption_key()) {

[albertb, 2013/12/18 20:45:30]

Note that sending_nigori does not necessarily imply that you should add
encryption_keys to the response. For custom passphrase users, we never send back
encryption keys.

[pval...(no longer on Chromium), 2014/01/03 23:25:31]

Thanks, I added some logic here for this.

+    for (size_t i = 0; i < keystore_keys_.size(); i++) {
+      get_updates_response->add_encryption_keys(keystore_keys_[i]);
+    }
+  }
+
+  int new_version = filtered_entities.empty() ?
+      0 : filtered_entities.back().version();
+
+  sieve.UpdateProgressMarkers(new_version, get_updates_response);
+
+  return response;
+}
+
+sync_pb::SyncEntity FakeServer::CommitEntity(sync_pb::SyncEntity entity,
+                                             string guid) {
+  // TODO(pvalenzuela): Implement this. Right now this method cheats and
+  // doesn't actually commit.
+  return entity;
+}
+
+sync_pb::ClientToServerResponse FakeServer::HandleCommitRequest(
+    const sync_pb::ClientToServerMessage& message) {
+  sync_pb::ClientToServerResponse response;
+  response.set_error_code(sync_pb::SyncEnums::SUCCESS);
+  response.set_store_birthday(birthday_);
+
+  sync_pb::CommitMessage commit = message.commit();
+  string guid = commit.cache_guid();
+
+  sync_pb::CommitResponse* commit_response = response.mutable_commit();
+
+  ::google::protobuf::RepeatedPtrField<sync_pb::SyncEntity>::const_iterator it;
+  for (it = commit.entries().begin(); it != commit.entries().end(); ++it) {
+    sync_pb::CommitResponse_EntryResponse* entry_response =
+        commit_response->add_entryresponse();
+
+    sync_pb::SyncEntity server_entity = CommitEntity(*it, guid);
+
+    entry_response->set_id_string(server_entity.id_string());
+    entry_response->set_response_type(sync_pb::CommitResponse::SUCCESS);
+    entry_response->set_version(it->version() + 1);
+  }
+
+  return response;
+}
+
+}  // namespace syncer