Refactor FakeServer and LoopbackServer

LoopbackServer doesn't need to deal with strings in its API, since it
deals with protos. It can be trivially refactored without behavioral
changes (relevant for local sync, i.e. enterprise).

This allows various simplifications in FakeServer.

Bug: None
Change-Id: Iffebffae9af844d27df53fe46a6b5f61c4016ce7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1883516
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#710066}

components/sync/engine_impl/loopback_server/loopback_connection_manager.cc

@@ -4,6 +4,7 @@
 #include "components/sync/engine_impl/loopback_server/loopback_connection_manager.h"
 
 #include "components/sync/engine_impl/net/server_connection_manager.h"
+#include "components/sync/protocol/sync.pb.h"
 #include "net/http/http_status_code.h"
 
 namespace syncer {
@@ -20,8 +21,20 @@ bool LoopbackConnectionManager::PostBufferToPath(
     PostBufferParams* params,
     const std::string& path,
     const std::string& access_token) {
+  params->buffer_out.clear();
+
+  sync_pb::ClientToServerMessage message;
+  bool parsed = message.ParseFromString(params->buffer_in);
+  DCHECK(parsed) << "Unable to parse the ClientToServerMessage.";
+
+  sync_pb::ClientToServerResponse response;
   params->response.http_status_code =
-      loopback_server_.HandleCommand(params->buffer_in, &params->buffer_out);
+      loopback_server_.HandleCommand(message, &response);
+
+  if (response.IsInitialized()) {
+    params->buffer_out = response.SerializeAsString();
+  }
+
   DCHECK_GE(params->response.http_status_code, 0);
 
   if (params->response.http_status_code != net::HTTP_OK) {

components/sync/engine_impl/loopback_server/loopback_server.cc

@@ -305,24 +305,21 @@ void LoopbackServer::SaveEntity(std::unique_ptr<LoopbackServerEntity> entity) {
   entities_[entity->GetId()] = std::move(entity);
 }
 
-net::HttpStatusCode LoopbackServer::HandleCommand(const string& request,
-                                                  std::string* response) {
+net::HttpStatusCode LoopbackServer::HandleCommand(
+    const sync_pb::ClientToServerMessage& message,
+    sync_pb::ClientToServerResponse* response) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  response->clear();
+  DCHECK(response);
 
-  sync_pb::ClientToServerMessage message;
-  bool parsed = message.ParseFromString(request);
-  DCHECK(parsed) << "Unable to parse the ClientToServerMessage.";
-
-  sync_pb::ClientToServerResponse response_proto;
+  response->Clear();
 
   if (bag_of_chips_.has_value()) {
-    *response_proto.mutable_new_bag_of_chips() = *bag_of_chips_;
+    *response->mutable_new_bag_of_chips() = *bag_of_chips_;
   }
 
   if (message.has_store_birthday() &&
       message.store_birthday() != GetStoreBirthday()) {
-    response_proto.set_error_code(sync_pb::SyncEnums::NOT_MY_BIRTHDAY);
+    response->set_error_code(sync_pb::SyncEnums::NOT_MY_BIRTHDAY);
   } else {
     bool success = false;
     std::vector<ModelType> datatypes_to_migrate;
@@ -330,25 +327,26 @@ net::HttpStatusCode LoopbackServer::HandleCommand(const string& request,
       case sync_pb::ClientToServerMessage::GET_UPDATES:
         success = HandleGetUpdatesRequest(
             message.get_updates(), message.store_birthday(),
-            message.invalidator_client_id(),
-            response_proto.mutable_get_updates(), &datatypes_to_migrate);
+            message.invalidator_client_id(), response->mutable_get_updates(),
+            &datatypes_to_migrate);
         break;
       case sync_pb::ClientToServerMessage::COMMIT:
         success = HandleCommitRequest(message.commit(),
                                       message.invalidator_client_id(),
-                                      response_proto.mutable_commit());
+                                      response->mutable_commit());
         break;
       case sync_pb::ClientToServerMessage::CLEAR_SERVER_DATA:
         ClearServerData();
-        response_proto.mutable_clear_server_data();
+        response->mutable_clear_server_data();
         success = true;
         break;
       default:
+        response->Clear();
         return net::HTTP_BAD_REQUEST;
     }
 
     if (success) {
-      response_proto.set_error_code(sync_pb::SyncEnums::SUCCESS);
+      response->set_error_code(sync_pb::SyncEnums::SUCCESS);
     } else if (datatypes_to_migrate.empty()) {
       UMA_HISTOGRAM_ENUMERATION(
           "Sync.Local.RequestTypeOnError", message.message_contents(),
@@ -358,15 +356,14 @@ net::HttpStatusCode LoopbackServer::HandleCommand(const string& request,
       DLOG(WARNING) << "Migration required for " << datatypes_to_migrate.size()
                     << " datatypes";
       for (ModelType type : datatypes_to_migrate) {
-        response_proto.add_migrated_data_type_id(
+        response->add_migrated_data_type_id(
             GetSpecificsFieldNumberFromModelType(type));
       }
-      response_proto.set_error_code(sync_pb::SyncEnums::MIGRATION_DONE);
+      response->set_error_code(sync_pb::SyncEnums::MIGRATION_DONE);
     }
   }
 
-  response_proto.set_store_birthday(GetStoreBirthday());
-  *response = response_proto.SerializeAsString();
+  response->set_store_birthday(GetStoreBirthday());
 
   // TODO(pastarmovj): This should be done asynchronously.
   SaveStateToFile(persistent_file_);

components/sync/engine_impl/loopback_server/loopback_server.h

@@ -50,10 +50,11 @@ class LoopbackServer {
   explicit LoopbackServer(const base::FilePath& persistent_file);
   virtual ~LoopbackServer();
 
-  // Handles a /command POST (with the given |request|) to the server.
-  // |*response| must not be null.
-  net::HttpStatusCode HandleCommand(const std::string& request,
-                                    std::string* response);
+  // Handles a /command POST (with the given |message|) to the server.
+  // |response| must not be null.
+  net::HttpStatusCode HandleCommand(
+      const sync_pb::ClientToServerMessage& message,
+      sync_pb::ClientToServerResponse* response);
 
   // Enables strong consistency model (i.e. server detects conflicts).
   void EnableStrongConsistencyWithConflictDetectionModel();

components/sync/test/fake_server/fake_server.cc

@@ -200,98 +200,89 @@ net::HttpStatusCode FakeServer::HandleCommand(const std::string& request,
                         message, /*include_specifics=*/true)));
 
   sync_pb::ClientToServerResponse response_proto;
-  if (message.message_contents() == sync_pb::ClientToServerMessage::COMMIT &&
-      commit_error_type_ != sync_pb::SyncEnums::SUCCESS &&
-      ShouldSendTriggeredError()) {
-    response_proto.set_error_code(commit_error_type_);
-  } else if (error_type_ != sync_pb::SyncEnums::SUCCESS &&
-             ShouldSendTriggeredError()) {
-    response_proto.set_error_code(error_type_);
-  } else if (triggered_actionable_error_.get() && ShouldSendTriggeredError()) {
-    sync_pb::ClientToServerResponse_Error* error =
-        response_proto.mutable_error();
-    error->CopyFrom(*(triggered_actionable_error_.get()));
-  } else {
-    switch (message.message_contents()) {
-      case sync_pb::ClientToServerMessage::GET_UPDATES:
-        last_getupdates_message_ = message;
-        break;
-      case sync_pb::ClientToServerMessage::COMMIT:
-        last_commit_message_ = message;
-        break;
-      default:
-        break;
-        // Don't care.
-    }
-    // The loopback server does not know how to handle Wallet requests -- and
-    // should not. The FakeServer is handling those instead. The loopback server
-    // has a strong expectations about how progress tokens are structured. To
-    // not interfere with this, we remove wallet progress markers before passing
-    // the request to the loopback server and add them back again afterwards
-    // before handling those requests.
-    std::unique_ptr<sync_pb::DataTypeProgressMarker> wallet_marker =
-        RemoveWalletProgressMarkerIfExists(&message);
-    net::HttpStatusCode http_status_code =
-        SendToLoopbackServer(message.SerializeAsString(), response);
-    if (wallet_marker != nullptr) {
-      *message.mutable_get_updates()->add_from_progress_marker() =
-          *wallet_marker;
-      if (http_status_code == net::HTTP_OK) {
-        HandleWalletRequest(message, *wallet_marker, response);
-      }
-    }
-    if (http_status_code == net::HTTP_OK) {
-      InjectClientCommand(response);
-    }
-
-    // Parse the proto for logging purposes.
-    response_proto.ParseFromString(*response);
-    LogForTestFailure(FROM_HERE, "RESPONSE",
-                      PrettyPrintValue(syncer::ClientToServerResponseToValue(
-                          response_proto, /*include_specifics=*/true)));
-
-    return http_status_code;
-  }
+  net::HttpStatusCode http_status_code =
+      HandleParsedCommand(message, &response_proto);
 
   LogForTestFailure(FROM_HERE, "RESPONSE",
                     PrettyPrintValue(syncer::ClientToServerResponseToValue(
                         response_proto,
                         /*include_specifics=*/true)));
 
-  response_proto.set_store_birthday(loopback_server_->GetStoreBirthday());
   *response = response_proto.SerializeAsString();
-  return net::HTTP_OK;
+  return http_status_code;
 }
 
-void FakeServer::HandleWalletRequest(
-    const sync_pb::ClientToServerMessage& request,
-    const sync_pb::DataTypeProgressMarker& old_wallet_marker,
-    std::string* response_string) {
-  if (request.message_contents() !=
-      sync_pb::ClientToServerMessage::GET_UPDATES) {
-    return;
+net::HttpStatusCode FakeServer::HandleParsedCommand(
+    const sync_pb::ClientToServerMessage& message,
+    sync_pb::ClientToServerResponse* response) {
+  DCHECK(response);
+  response->Clear();
+
+  if (message.message_contents() == sync_pb::ClientToServerMessage::COMMIT &&
+      commit_error_type_ != sync_pb::SyncEnums::SUCCESS &&
+      ShouldSendTriggeredError()) {
+    response->set_error_code(commit_error_type_);
+    response->set_store_birthday(loopback_server_->GetStoreBirthday());
+    return net::HTTP_OK;
   }
-  sync_pb::ClientToServerResponse response_proto;
-  CHECK(response_proto.ParseFromString(*response_string));
-  PopulateWalletResults(wallet_entities_, old_wallet_marker,
-                        response_proto.mutable_get_updates());
-  *response_string = response_proto.SerializeAsString();
-}
 
-net::HttpStatusCode FakeServer::SendToLoopbackServer(const std::string& request,
-                                                     std::string* response) {
-  base::ThreadRestrictions::SetIOAllowed(true);
-  return loopback_server_->HandleCommand(request, response);
-}
+  if (error_type_ != sync_pb::SyncEnums::SUCCESS &&
+      ShouldSendTriggeredError()) {
+    response->set_error_code(error_type_);
+    response->set_store_birthday(loopback_server_->GetStoreBirthday());
+    return net::HTTP_OK;
+  }
 
-void FakeServer::InjectClientCommand(std::string* response) {
-  sync_pb::ClientToServerResponse response_proto;
-  bool parse_ok = response_proto.ParseFromString(*response);
-  DCHECK(parse_ok) << "Unable to parse-back the server response";
-  if (response_proto.error_code() == sync_pb::SyncEnums::SUCCESS) {
-    *response_proto.mutable_client_command() = client_command_;
-    *response = response_proto.SerializeAsString();
+  if (triggered_actionable_error_.get() && ShouldSendTriggeredError()) {
+    *response->mutable_error() = *triggered_actionable_error_;
+    response->set_store_birthday(loopback_server_->GetStoreBirthday());
+    return net::HTTP_OK;
+  }
+
+  switch (message.message_contents()) {
+    case sync_pb::ClientToServerMessage::GET_UPDATES:
+      last_getupdates_message_ = message;
+      break;
+    case sync_pb::ClientToServerMessage::COMMIT:
+      last_commit_message_ = message;
+      break;
+    default:
+      break;
+      // Don't care.
   }
+
+  // The loopback server does not know how to handle Wallet requests -- and
+  // should not. The FakeServer is handling those instead. The loopback server
+  // has a strong expectations about how progress tokens are structured. To
+  // not interfere with this, we remove wallet progress markers before passing
+  // the request to the loopback server.
+  sync_pb::ClientToServerMessage message_without_wallet = message;
+  std::unique_ptr<sync_pb::DataTypeProgressMarker> wallet_marker =
+      RemoveWalletProgressMarkerIfExists(&message_without_wallet);
+
+  net::HttpStatusCode http_status_code =
+      SendToLoopbackServer(message_without_wallet, response);
+
+  if (wallet_marker != nullptr && http_status_code == net::HTTP_OK &&
+      message.message_contents() ==
+          sync_pb::ClientToServerMessage::GET_UPDATES) {
+    PopulateWalletResults(wallet_entities_, *wallet_marker,
+                          response->mutable_get_updates());
+  }
+
+  if (http_status_code == net::HTTP_OK &&
+      response->error_code() == sync_pb::SyncEnums::SUCCESS) {
+    *response->mutable_client_command() = client_command_;
+  }
+
+  return http_status_code;
+}
+
+net::HttpStatusCode FakeServer::SendToLoopbackServer(
+    const sync_pb::ClientToServerMessage& message,
+    sync_pb::ClientToServerResponse* response) {
+  base::ThreadRestrictions::SetIOAllowed(true);
+  return loopback_server_->HandleCommand(message, response);
 }
 
 bool FakeServer::GetLastCommitMessage(sync_pb::ClientToServerMessage* message) {

components/sync/test/fake_server/fake_server.h

@@ -208,16 +208,17 @@ class FakeServer : public syncer::LoopbackServer::ObserverForTests {
       syncer::LoopbackServer::ResponseTypeProvider response_type_override);
 
  private:
+  // Analogous to HandleCommand() but deals with parsed protos.
+  net::HttpStatusCode HandleParsedCommand(
+      const sync_pb::ClientToServerMessage& message,
+      sync_pb::ClientToServerResponse* response);
+
   // Returns whether a triggered error should be sent for the request.
   bool ShouldSendTriggeredError() const;
   bool HasTriggeredError() const;
-  net::HttpStatusCode SendToLoopbackServer(const std::string& request,
-                                           std::string* response);
-  void InjectClientCommand(std::string* response);
-  void HandleWalletRequest(
-      const sync_pb::ClientToServerMessage& request,
-      const sync_pb::DataTypeProgressMarker& old_wallet_marker,
-      std::string* response_string);
+  net::HttpStatusCode SendToLoopbackServer(
+      const sync_pb::ClientToServerMessage& message,
+      sync_pb::ClientToServerResponse* response);
 
   // Logs a string that is meant to be shown in case the running test fails.
   void LogForTestFailure(const base::Location& location,