peerd: Update existing services rather than recreating them

BUG=brillo:14
TEST=Existing integration tests pass.  Added more unittests.

Change-Id: Ia086b77a1093544fc72a59f64f51cd2069aff5a7
Reviewed-on: https://chromium-review.googlesource.com/253120
Tested-by: Christopher Wiley <wiley@chromium.org>
Reviewed-by: Alex Vakulenko <avakulenko@chromium.org>
Commit-Queue: Christopher Wiley <wiley@chromium.org>

8 files changed, 150 insertions, 38 deletions

diff --git a/discovered_peer.cc b/discovered_peer.cc
index 6ac7acf..1f5d820 100644
--- a/discovered_peer.cc
+++ b/discovered_peer.cc
@@ -56,7 +56,7 @@ void DiscoveredPeer::UpdateService(const std::string& service_id,
       LOG(WARNING) << "Discarding stale service update.";
       return;
     }
-    if (!service_it->second->Update(nullptr, addresses, info)) {
+    if (!service_it->second->Update(nullptr, addresses, info, {})) {
       LOG(WARNING) << "Discarding invalid service update.";
       return;
     }
diff --git a/manager.cc b/manager.cc
index 6d3afe9..2255a15 100644
--- a/manager.cc
+++ b/manager.cc
@@ -115,7 +115,7 @@ bool Manager::StartMonitoring(
     const vector<string>& requested_technologies,
     const map<string, Any>& options,
     std::string* monitoring_token) {
-  if (requested_technologies.empty())  {
+  if (requested_technologies.empty()) {
     Error::AddTo(error,
                  FROM_HERE,
                  kPeerdErrorDomain,
@@ -197,13 +197,15 @@ bool Manager::ExposeService(chromeos::ErrorPtr* error,
     return false;
   }
   auto it = exposed_services_.find(service_id);
-  // Regardless of whether |it| is valid, it will become invalid in this block.
   if (it != exposed_services_.end()) {
-    // TODO(wiley) We should be updating, but for now, remove and add again.
-    //             brbug.com/14 .
-    if (!RemoveExposedService(error, message, service_id)) {
+    if (it->second->connection_name() != message->GetSender()) {
+      Error::AddToPrintf(
+          error, FROM_HERE, kPeerdErrorDomain, errors::manager::kAlreadyExposed,
+          "Service %s was already exposed by another local process.",
+          service_id.c_str());
       return false;
     }
+    return self_->UpdateService(error, service_id, service_info, options);
   }
   if (!self_->AddPublishedService(error, service_id, service_info, options)) {
     return false;
diff --git a/manager_unittest.cc b/manager_unittest.cc
index 9da2e3d..06c6479 100644
--- a/manager_unittest.cc
+++ b/manager_unittest.cc
@@ -91,8 +91,8 @@ class ManagerTest : public testing::Test {
 TEST_F(ManagerTest, ShouldRejectSerbusServiceId) {
   chromeos::ErrorPtr error;
   dbus::MethodCall method_call{"org.chromium.peerd.Manager", "ExposeService"};
-  EXPECT_FALSE(manager_->ExposeService(&error, &method_call, kSerbusServiceId,
-                                       {}, {}));
+  EXPECT_FALSE(manager_->ExposeService(
+      &error, &method_call, kSerbusServiceId, {}, {}));
   EXPECT_NE(nullptr, error.get());
 }
 
diff --git a/published_peer.cc b/published_peer.cc
index 6f75957..04f7524 100644
--- a/published_peer.cc
+++ b/published_peer.cc
@@ -18,17 +18,8 @@ bool PublishedPeer::AddPublishedService(
   if (!Peer::AddService(error, service_id, {}, service_info, options)) {
     return false;
   }
-  bool success = true;
-  // Notify all the publishers we know about that we have a new service.
-  auto first_null = std::remove_if(
-      publishers_.begin(), publishers_.end(),
-      std::logical_not<base::WeakPtr<ServicePublisherInterface>>());
-  publishers_.erase(first_null, publishers_.end());
-  for (const auto& publisher : publishers_) {
-      success = publisher->OnServiceUpdated(
-          error, *services_[service_id]) && success;
-  }
-  return success;
+  CleanPublishers();
+  return PublishService(error, *services_[service_id]);
 }
 
 bool PublishedPeer::RemoveService(chromeos::ErrorPtr* error,
@@ -37,12 +28,9 @@ bool PublishedPeer::RemoveService(chromeos::ErrorPtr* error,
     // Didn't even have this service on this peer?
     return false;
   }
+  CleanPublishers();
   // Notify all the publishers we know about that we have removed a service.
   bool result = true;
-  auto first_null = std::remove_if(
-      publishers_.begin(), publishers_.end(),
-      std::logical_not<base::WeakPtr<ServicePublisherInterface>>());
-  publishers_.erase(first_null, publishers_.end());
   for (const auto& publisher : publishers_) {
       result = publisher->OnServiceRemoved(error, service_id) && result;
   }
@@ -51,6 +39,7 @@ bool PublishedPeer::RemoveService(chromeos::ErrorPtr* error,
 
 void PublishedPeer::RegisterServicePublisher(
     WeakPtr<ServicePublisherInterface> publisher) {
+  CleanPublishers();
   if (!publisher) { return; }
   for (const auto& kv : services_) {
     publisher->OnServiceUpdated(nullptr, *kv.second);
@@ -58,4 +47,40 @@ void PublishedPeer::RegisterServicePublisher(
   publishers_.push_back(publisher);
 }
 
+bool PublishedPeer::UpdateService(
+    chromeos::ErrorPtr* error,
+    const std::string& service_id,
+    const std::map<std::string, std::string>& service_info,
+    const std::map<std::string, chromeos::Any>& options) {
+  CleanPublishers();
+  auto it = services_.find(service_id);
+  if (it == services_.end()) {
+    chromeos::Error::AddToPrintf(
+        error, FROM_HERE, kPeerdErrorDomain, errors::peer::kUnknownService,
+        "Can't update service %s because it was not previously registered.",
+        service_id.c_str());
+    return false;
+  }
+  if (!it->second->Update(error, {}, service_info, options)) {
+    return false;
+  }
+  return PublishService(error, *it->second);
+}
+
+void PublishedPeer::CleanPublishers() {
+  auto first_null = std::remove_if(
+      publishers_.begin(), publishers_.end(),
+      std::logical_not<base::WeakPtr<ServicePublisherInterface>>());
+  publishers_.erase(first_null, publishers_.end());
+}
+
+bool PublishedPeer::PublishService(chromeos::ErrorPtr* error,
+                                   const Service& service) {
+  bool success = true;
+  for (const auto& publisher : publishers_) {
+      success = publisher->OnServiceUpdated(error, service) && success;
+  }
+  return success;
+}
+
 }  // namespace peerd
diff --git a/published_peer.h b/published_peer.h
index 199dea7..38685ad 100644
--- a/published_peer.h
+++ b/published_peer.h
@@ -42,8 +42,25 @@ class PublishedPeer : public Peer {
   virtual void RegisterServicePublisher(
       base::WeakPtr<ServicePublisherInterface> publisher);
 
+  // Updates an existing service by setting |service_info| and |options| to
+  // these new values.
+  virtual bool UpdateService(
+      chromeos::ErrorPtr* error,
+      const std::string& service_id,
+      const std::map<std::string, std::string>& service_info,
+      const std::map<std::string, chromeos::Any>& options);
+
  private:
+  // Removes invalid publishers.
+  void CleanPublishers();
+  // Notify all the publishers we know about that we have a new or updated
+  // service.
+  bool PublishService(chromeos::ErrorPtr* error, const Service& service);
+
   std::vector<base::WeakPtr<ServicePublisherInterface>> publishers_;
+
+  FRIEND_TEST(PublishedPeerTest, ShouldRejectInvalidUpdates);
+  FRIEND_TEST(PublishedPeerTest, ShouldAcceptValidUpdates);
   DISALLOW_COPY_AND_ASSIGN(PublishedPeer);
 };
 
diff --git a/published_peer_unittest.cc b/published_peer_unittest.cc
index 33ebc1e..1e208eb 100644
--- a/published_peer_unittest.cc
+++ b/published_peer_unittest.cc
@@ -16,9 +16,11 @@
 #include "peerd/service.h"
 #include "peerd/test_util.h"
 
+using chromeos::Any;
 using chromeos::ErrorPtr;
 using dbus::MockBus;
 using dbus::ObjectPath;
+using std::map;
 using std::string;
 using std::unique_ptr;
 using testing::AnyNumber;
@@ -100,4 +102,64 @@ TEST_F(PublishedPeerTest, ShouldPrunePublisherList) {
   EXPECT_EQ(nullptr, error.get());
 }
 
+TEST_F(PublishedPeerTest, ShouldRejectInvalidUpdates) {
+  const uint16_t kPort{9080};
+  const string kServiceId{"some-service"};
+  const Service::ServiceInfo kServiceInfo{
+      {"Legume_Duprix", "West Virginia University"},
+  };
+  const map<string, Any> kOptions{
+      {"mdns", map<string, Any>{ {"port", kPort}, },
+      },
+  };
+  const Service::ServiceInfo kUpdatedServiceInfo{
+      {"Splendiferous Finch", "Northwestern University"},
+  };
+  const map<string, Any> kInvalidOptions{
+      {"mdns", kPort + 1},
+  };
+  ErrorPtr error;
+  EXPECT_TRUE(peer_.AddPublishedService(
+        &error, kServiceId, kServiceInfo, kOptions));
+  // Unknown service ID is an error.
+  EXPECT_FALSE(peer_.UpdateService(
+        nullptr, "other-service", kUpdatedServiceInfo, kOptions));
+  // If the update contains invalid updated values, that's also an error.
+  EXPECT_FALSE(peer_.UpdateService(
+        nullptr, kServiceId, kUpdatedServiceInfo, kInvalidOptions));
+  // Since we've have no valid updates, we should see that the service
+  // is the same as before.
+  Service* service = peer_.services_[kServiceId].get();
+  EXPECT_EQ(kServiceInfo, service->GetServiceInfo());
+  EXPECT_EQ(kPort, service->GetMDnsOptions().port);
+}
+
+TEST_F(PublishedPeerTest, ShouldAcceptValidUpdates) {
+  const uint16_t kPort{9080};
+  const uint16_t kUpdatedPort{9081};
+  const string kServiceId{"some-service"};
+  const Service::ServiceInfo kServiceInfo{
+      {"Legume_Duprix", "West Virginia University"},
+  };
+  const map<string, Any> kOptions{
+      {"mdns", map<string, Any>{ {"port", kPort}, },
+      },
+  };
+  const Service::ServiceInfo kUpdatedServiceInfo{
+      {"Splendiferous_Finch", "Northwestern University"},
+  };
+  const map<string, Any> kUpdatedOptions{
+      {"mdns", map<string, Any>{ {"port", kUpdatedPort}, },
+      },
+  };
+  ErrorPtr error;
+  EXPECT_TRUE(peer_.AddPublishedService(
+        &error, kServiceId, kServiceInfo, kOptions));
+  EXPECT_TRUE(peer_.UpdateService(
+        &error, kServiceId, kUpdatedServiceInfo, kUpdatedOptions));
+  Service* service = peer_.services_[kServiceId].get();
+  EXPECT_EQ(kUpdatedServiceInfo, service->GetServiceInfo());
+  EXPECT_EQ(kUpdatedPort, service->GetMDnsOptions().port);
+}
+
 }  // namespace peerd
diff --git a/service.cc b/service.cc
index 78c2d58..0fb0b05 100644
--- a/service.cc
+++ b/service.cc
@@ -61,11 +61,8 @@ bool Service::RegisterAsync(chromeos::ErrorPtr* error,
                             const map<string, Any>& options,
                             const CompletionAction& completion_callback) {
   if (!IsValidServiceId(error, service_id)) { return false; }
-  if (!IsValidServiceInfo(error, service_info)) { return false; }
-  if (!ParseOptions(error, options)) { return false; }
+  if (!Update(error, addresses, service_info, options)) { return false;}
   dbus_adaptor_.SetServiceId(service_id);
-  dbus_adaptor_.SetIpInfos(addresses);
-  dbus_adaptor_.SetServiceInfo(service_info);
   dbus_adaptor_.RegisterWithDBusObject(dbus_object_.get());
   dbus_object_->RegisterAsync(completion_callback);
   return true;
@@ -88,12 +85,14 @@ const Service::MDnsOptions& Service::GetMDnsOptions() const {
 
 bool Service::Update(chromeos::ErrorPtr* error,
                      const IpAddresses& addresses,
-                     const ServiceInfo& info) {
-  if (!IsValidServiceInfo(error, info)) {
-    return false;
-  }
+                     const ServiceInfo& info,
+                     const map<string, Any>& options) {
+  MDnsOptions mdns_options;
+  if (!IsValidServiceInfo(error, info)) { return false; }
+  if (!ParseOptions(error, options, &mdns_options)) { return false; }
   dbus_adaptor_.SetIpInfos(addresses);
   dbus_adaptor_.SetServiceInfo(info);
+  parsed_mdns_options_ = mdns_options;
   return true;
 }
 
@@ -168,11 +167,14 @@ bool Service::IsValidServiceInfo(chromeos::ErrorPtr* error,
 }
 
 bool Service::ParseOptions(chromeos::ErrorPtr* error,
-                           const map<string, Any>& orig_options) {
+                           const map<string, Any>& orig_options,
+                           MDnsOptions* mdns_options_out) {
   map<string, Any> options{orig_options};
   auto mdns_it = options.find(kMDNSSectionName);
   if (mdns_it != options.end()) {
-    if (!ExtractMDnsOptions(error, &mdns_it->second)) { return false; }
+    if (!ExtractMDnsOptions(error, &mdns_it->second, mdns_options_out)) {
+      return false;
+    }
     options.erase(mdns_it);
   }
   if (!options.empty()) {
@@ -187,7 +189,8 @@ bool Service::ParseOptions(chromeos::ErrorPtr* error,
 }
 
 bool Service::ExtractMDnsOptions(chromeos::ErrorPtr* error,
-                                 Any* maybe_mdns_options) {
+                                 Any* maybe_mdns_options,
+                                 MDnsOptions* mdns_options_out) {
   map<string, Any>* mdns_options =
       maybe_mdns_options->GetPtr<map<string, Any>>();
   if (mdns_options == nullptr) {
@@ -211,7 +214,7 @@ bool Service::ExtractMDnsOptions(chromeos::ErrorPtr* error,
                    "Invalid entry for mDNS port.");
       return false;
     }
-    parsed_mdns_options_.port = static_cast<uint16_t>(port);
+    mdns_options_out->port = static_cast<uint16_t>(port);
     mdns_options->erase(port_it);
   }
   if (!mdns_options->empty()) {
diff --git a/service.h b/service.h
index 34cc629..68c6243 100644
--- a/service.h
+++ b/service.h
@@ -75,17 +75,20 @@ class Service : public org::chromium::peerd::ServiceInterface {
   // entire update is discarded.  Returns true if update is applied.
   bool Update(chromeos::ErrorPtr* error,
               const IpAddresses& addresses,
-              const ServiceInfo& info);
+              const ServiceInfo& info,
+              const std::map<std::string, chromeos::Any>& options);
 
  private:
   // Parses options for services being published by this device.
   bool ParseOptions(chromeos::ErrorPtr* error,
-                    const std::map<std::string, chromeos::Any>& options);
+                    const std::map<std::string, chromeos::Any>& options,
+                    MDnsOptions* mdns_options_out);
   // Checks that |maybe_mdns_options| is a map<string, Any> and then removes
   // values from that dictionary.  Stores the appropriate parsed values into
   // |mdns_options_|.
   bool ExtractMDnsOptions(chromeos::ErrorPtr* error,
-                          chromeos::Any* maybe_mdns_options);
+                          chromeos::Any* maybe_mdns_options,
+                          MDnsOptions* mdns_options_out);
 
 
   org::chromium::peerd::ServiceAdaptor dbus_adaptor_{this};