Basic implementation of the Sync C++ fake server

sync/internal_api/public/test/fake_server.h

Index: sync/internal_api/public/test/fake_server.h
diff --git a/sync/internal_api/public/test/fake_server.h b/sync/internal_api/public/test/fake_server.h
new file mode 100644
index 0000000000000000000000000000000000000000..25edf210c9d70f80efa3cfafe5ba077133b65b9c
--- /dev/null
+++ b/sync/internal_api/public/test/fake_server.h
@@ -0,0 +1,132 @@
+// 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.
+
+#ifndef SYNC_INTERNAL_API_PUBLIC_TEST_FAKE_SERVER_H_
+#define SYNC_INTERNAL_API_PUBLIC_TEST_FAKE_SERVER_H_
+
+#include <map>
+#include <string>
+
+#include "base/basictypes.h"
+#include "base/memory/scoped_ptr.h"
+#include "sync/internal_api/public/base/model_type.h"
+#include "sync/protocol/sync.pb.h"
+
+namespace syncer {
+
+// An in-process implementation of the Sync server used for testing.

[Raghu Simha, 2013/12/18 22:37:12]

Desktop in-process browser tests already have the notion of an embedded test
server. It may be worth elaborating on why we're adding yet another test server
class instead of reusing the infrastructure in
net/test/embedded_test_server/embedded_test_server.h.

[pval...(no longer on Chromium), 2013/12/19 01:16:56]

I modified this comment to remove the mention of running it in-process;
FakeServer is agnostic to how its being used (e.g., in-process as in this CL,
EmbeddedTestServer, separate binary). 

I've added a TODO in sync_test.cc to use EmbeddedTestServer for desktop tests.
After our discussions, I think that is the right direction going forward. We
need this new in-process method for iOS; where do you think I should mention
that?

+class FakeServer {

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

General comment:

I think you'll find it easier in the long run to keep up with server and client
features if you split up functionality for different types.  Certain types, like
experiments or push messaging, are not like the others.  If you continue down
the current path, you might one day find your code littered with if (type ==
PUSH_MESSAGING) or (type == BOOKMARKS) conditions all over the place.

If you implement one class (or at least one instance of some common class) per
type, you might have an easier time handling each type's special features.

You don't need to act on this comment in this CL, or at all.  However, if you
agree that splitting up functionality per type is more future proof, you'll
probably want to start with that design from the start rather than trying to
refactor it in later.

[pval...(no longer on Chromium), 2013/12/19 01:16:56]

This makes sense. For this CL, I'll keep this as is and revisit the idea in the
next CL.

+ public:
+  FakeServer();
+  virtual ~FakeServer();
+
+  // Initializes the fake server. The server is not able to process requests
+  // before this is called and returns true.
+  bool Init();
+
+  // Handles a /command POST to the server and returns the response as a string.
+  // |error_code| and |response_code| are also set.
+  std::string HandleCommand(std::string request, int* error_code,

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

nit: one parameter per line.

http://www.chromium.org/developers/coding-style#Code_Formatting

Also, it seems a little odd to me that this combines both out parameters and a
return value in this way.  Normally the pattern would be something like

int error = DoFailableAction(&result);

where there is some sort of guarantee that result is not modified when the
return value indicates an error.

[pval...(no longer on Chromium), 2013/12/19 01:16:56]

Done.

+                            int* response_code);
+
+ private:
+  // A filter used during GetUpdates calls to determine what information to
+  // send back to the client. There is a 1:1 correspondence between any given
+  // GetUpdates call and an UpdateSieve instance.
+  class UpdateSieve {

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

I see few advantages to using inner-classes in C++.

Since you declared it private, I'm guessing your intention is to hide it.  If
that's the case, you're probably better off declaring it in an anonymous
namespace in the .cpp file.

[pval...(no longer on Chromium), 2013/12/19 01:16:56]

Done.

+   public:
+    // Populates |state_| based on |get_updates_message|.
+    UpdateSieve(const sync_pb::GetUpdatesMessage& get_updates_message);
+
+    ~UpdateSieve();
+
+    // Sets the progress markers in |get_updates_response| given the progress
+    // markers from |get_updates_message_| and |new_version| (the latest
+    // version in the entries sent back).
+    void UpdateProgressMarkers(
+        int64 new_version,
+        sync_pb::GetUpdatesResponse* get_updates_response);
+
+    // Determines whether the server should send |entity| to the client based
+    // on its type and version.
+    bool ClientWantsItem(const sync_pb::SyncEntity& entity);
+
+    // Returns the mininum version seen across all types.
+    int64 GetMinVersion();
+
+    // Returns the data type IDs of types being synced for the first time.
+    std::vector<int> GetFirstTimeTypes();
+
+   private:
+    typedef std::map<int, int64> TypeIdToVersionMap;

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

Since we're in C++ land and you have the classes available, you'd might as well
make this a map from ModelType to int64.

[pval...(no longer on Chromium), 2013/12/19 01:16:56]

Done.

+
+    // The GetUpdatesMessage associated with this instance.
+    const sync_pb::GetUpdatesMessage& get_updates_message_;
+
+    // Maps data type IDs to the latest version seen for that version.

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

"version seen for that version" -> typo?

[pval...(no longer on Chromium), 2013/12/19 01:16:56]

Done.

+    TypeIdToVersionMap state_;

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

state_ is not a very informative name.  How about:
- type_progress_
- type_min_versions_

other suggestions welcome.

[pval...(no longer on Chromium), 2013/12/19 01:16:56]

how about type_latest_version_?

+
+    // The minimum version seen among all data types.
+    int min_version_;
+  };
+
+  // The data necessary for the server to create a permanent item.
+  struct PermanentItemSpec {
+    std::string tag;
+    std::string name;
+    std::string parent_tag;
+    ModelType sync_type;
+    bool create_by_default;
+  };
+
+  typedef std::map<std::string, sync_pb::SyncEntity> EntityMap;
+
+  // Processes a GetUpdates call.
+  sync_pb::ClientToServerResponse HandleGetUpdatesRequest(
+      const sync_pb::ClientToServerMessage& message);
+
+  // Processes a Commit call.
+  sync_pb::ClientToServerResponse HandleCommitRequest(
+      const sync_pb::ClientToServerMessage& message);
+
+  // Initializes |permanent_item_specs_|.
+  bool PopulatePermanentItemSpecs();
+
+  // Inserts the appropriate permanent items in |entities_|.
+  void CreateDefaultPermanentItems(const std::vector<int>& first_time_type_ids);
+
+  // Creates and saves a permanent item based on |spec|.
+  void CreatePermanentItem(const PermanentItemSpec& spec);
+
+  // Saves a |entity| to |entities_|.
+  void SaveEntity(sync_pb::SyncEntity entity);
+
+  // Commits a client-side |entity| to |entities_|.
+  sync_pb::SyncEntity CommitEntity(sync_pb::SyncEntity entity,
+                                   std::string guid);
+
+  // Whether the server has been initialized and is ready to process requests.
+  bool initialized_;
+
+  // The collection of PermanentItemSpecs created at runtime.
+  std::vector<PermanentItemSpec> permanent_item_specs_;
+
+  // This is the last version number assigned to an entity. The next entity will
+  // have a version number of version_ + 1.
+  int64 version_;
+
+  // The current birthday value.
+  std::string birthday_;
+
+  // All SyncEntity objects saved by the server. The key value is the entity's
+  // id string.
+  EntityMap entities_;
+
+  // All Keystore keys knwon to the server.
+  std::vector<std::string> keystore_keys_;
+};
+
+}  // namespace syncer
+
+#endif  // SYNC_INTERNAL_API_PUBLIC_TEST_FAKE_SERVER_H_