Merge: Add write callback into SaveSettings function

Saving critical settings needs confirmation.
When command alters device config, it should be set "Done" only after
settings are actually saved.

BUG:25776798
Reviewed-on: https://weave-review.googlesource.com/2199
Reviewed-by: Alex Vakulenko <avakulenko@google.com>
(cherry picked from commit 42e508f2559e019d2fcc8f88adfd184b7a6bc3a4)

Change-Id: I693e3c17b3f2f707c8df7af29eefd48362980bce
Reviewed-on: https://weave-review.googlesource.com/2421
Reviewed-by: Vitaly Buka <vitalybuka@google.com>

8 files changed, 54 insertions, 29 deletions

diff --git a/examples/daemon/common/daemon.h b/examples/daemon/common/daemon.h
index 4cccff3..6dc021d 100644
--- a/examples/daemon/common/daemon.h
+++ b/examples/daemon/common/daemon.h
@@ -69,10 +69,11 @@ class Daemon {
   };
 
   Daemon(const Options& opts)
-      : config_store_{new weave::examples::FileConfigStore(
-            opts.disable_security_,
-            opts.model_id_)},
-        task_runner_{new weave::examples::EventTaskRunner},
+      : task_runner_{new weave::examples::EventTaskRunner},
+        config_store_{
+            new weave::examples::FileConfigStore(opts.disable_security_,
+                                                 opts.model_id_,
+                                                 task_runner_.get())},
         http_client_{new weave::examples::CurlHttpClient(task_runner_.get())},
         network_{new weave::examples::EventNetworkImpl(task_runner_.get())},
         bluetooth_{new weave::examples::BluetoothImpl} {
@@ -114,8 +115,8 @@ class Daemon {
       LOG(INFO) << "Device registered: " << device->GetSettings().cloud_id;
   }
 
-  std::unique_ptr<weave::examples::FileConfigStore> config_store_;
   std::unique_ptr<weave::examples::EventTaskRunner> task_runner_;
+  std::unique_ptr<weave::examples::FileConfigStore> config_store_;
   std::unique_ptr<weave::examples::CurlHttpClient> http_client_;
   std::unique_ptr<weave::examples::EventNetworkImpl> network_;
   std::unique_ptr<weave::examples::BluetoothImpl> bluetooth_;
diff --git a/examples/provider/file_config_store.cc b/examples/provider/file_config_store.cc
index af887a7..31efaa7 100644
--- a/examples/provider/file_config_store.cc
+++ b/examples/provider/file_config_store.cc
@@ -12,14 +12,19 @@
 #include <string>
 #include <vector>
 
+#include <base/bind.h>
+
 namespace weave {
 namespace examples {
 
 const char kSettingsDir[] = "/var/lib/weave/";
 
 FileConfigStore::FileConfigStore(bool disable_security,
-                                 const std::string& model_id)
-    : disable_security_{disable_security}, model_id_{model_id} {}
+                                 const std::string& model_id,
+                                 provider::TaskRunner* task_runner)
+    : disable_security_{disable_security},
+      model_id_{model_id},
+      task_runner_{task_runner} {}
 
 std::string FileConfigStore::GetPath(const std::string& name) const {
   std::string path{kSettingsDir};
@@ -72,11 +77,14 @@ std::string FileConfigStore::LoadSettings(const std::string& name) {
 }
 
 void FileConfigStore::SaveSettings(const std::string& name,
-                                   const std::string& settings) {
+                                   const std::string& settings,
+                                   const DoneCallback& callback) {
   CHECK(mkdir(kSettingsDir, S_IRWXU) == 0 || errno == EEXIST);
   LOG(INFO) << "Saving settings to " << GetPath(name);
   std::ofstream str(GetPath(name));
   str << settings;
+  if (!callback.is_null())
+    task_runner_->PostDelayedTask(FROM_HERE, base::Bind(callback, nullptr), {});
 }
 
 }  // namespace examples
diff --git a/examples/provider/file_config_store.h b/examples/provider/file_config_store.h
index 214194e..e7398d1 100644
--- a/examples/provider/file_config_store.h
+++ b/examples/provider/file_config_store.h
@@ -10,18 +10,22 @@
 #include <vector>
 
 #include <weave/provider/config_store.h>
+#include <weave/provider/task_runner.h>
 
 namespace weave {
 namespace examples {
 
 class FileConfigStore : public provider::ConfigStore {
  public:
-  FileConfigStore(bool disable_security, const std::string& model_id);
+  FileConfigStore(bool disable_security,
+                  const std::string& model_id,
+                  provider::TaskRunner* task_runner);
 
   bool LoadDefaults(Settings* settings) override;
   std::string LoadSettings(const std::string& name) override;
   void SaveSettings(const std::string& name,
-                    const std::string& settings) override;
+                    const std::string& settings,
+                    const DoneCallback& callback) override;
 
   std::string LoadSettings() override;
 
@@ -29,6 +33,7 @@ class FileConfigStore : public provider::ConfigStore {
   std::string GetPath(const std::string& name) const;
   const bool disable_security_;
   const std::string model_id_;
+  provider::TaskRunner* task_runner_{nullptr};
 };
 
 }  // namespace examples
diff --git a/include/weave/provider/config_store.h b/include/weave/provider/config_store.h
index 991d750..128eccc 100644
--- a/include/weave/provider/config_store.h
+++ b/include/weave/provider/config_store.h
@@ -13,6 +13,7 @@
 #include <base/callback.h>
 #include <base/time/time.h>
 #include <weave/enum_to_string.h>
+#include <weave/error.h>
 #include <weave/settings.h>
 
 namespace weave {
@@ -48,9 +49,13 @@ namespace provider {
 // persistent storage (file, flash, etc).
 // For example:
 //   void FileConfigStore::SaveSettings(const std::string& name,
-//                                      const std::string& settings) {
+//                                      const std::string& settings,
+//                                      const DoneCallback& callback) {
 //     std::ofstream str("/var/lib/weave/weave_" + name + ".json");
 //     str << settings;
+//     if (!callback.is_null())
+//       task_runner_->PostDelayedTask(FROM_HERE, base::Bind(callback, nullptr),
+//                                     {});
 //   }
 // It is highly recommended to protected data using encryption with
 // hardware backed key.
@@ -75,8 +80,10 @@ class ConfigStore {
   // modifications. Data stored in settings can be sensitive, so it's highly
   // recommended to protect data, e.g. using encryption.
   // |name| is the name of settings blob. Could be used as filename.
+  // Implementation must call or post callback
   virtual void SaveSettings(const std::string& name,
-                            const std::string& settings) = 0;
+                            const std::string& settings,
+                            const DoneCallback& callback) = 0;
 
   // Deprecated: only for migration of old configs to version with |name|.
   virtual std::string LoadSettings() = 0;
diff --git a/include/weave/provider/test/mock_config_store.h b/include/weave/provider/test/mock_config_store.h
index cdae693..e6411d6 100644
--- a/include/weave/provider/test/mock_config_store.h
+++ b/include/weave/provider/test/mock_config_store.h
@@ -40,11 +40,19 @@ class MockConfigStore : public ConfigStore {
           "device_id": "TEST_DEVICE_ID"
         })"));
     EXPECT_CALL(*this, LoadSettings("config")).WillRepeatedly(Return(""));
-    EXPECT_CALL(*this, SaveSettings("config", _)).WillRepeatedly(Return());
+    EXPECT_CALL(*this, SaveSettings("config", _, _))
+        .WillRepeatedly(testing::WithArgs<1, 2>(testing::Invoke(
+            [](const std::string& json, const DoneCallback& callback) {
+              if (!callback.is_null())
+                callback.Run(nullptr);
+            })));
   }
   MOCK_METHOD1(LoadDefaults, bool(Settings*));
   MOCK_METHOD1(LoadSettings, std::string(const std::string&));
-  MOCK_METHOD2(SaveSettings, void(const std::string&, const std::string&));
+  MOCK_METHOD3(SaveSettings,
+               void(const std::string&,
+                    const std::string&,
+                    const DoneCallback&));
   MOCK_METHOD0(LoadSettings, std::string());
 };
 
diff --git a/src/config.cc b/src/config.cc
index cf564c8..44d20dd 100644
--- a/src/config.cc
+++ b/src/config.cc
@@ -18,6 +18,7 @@
 #include "src/data_encoding.h"
 #include "src/privet/privet_types.h"
 #include "src/string_utils.h"
+#include "src/bind_lambda.h"
 
 namespace weave {
 
@@ -271,7 +272,9 @@ void Config::Save() {
   base::JSONWriter::WriteWithOptions(
       dict, base::JSONWriter::OPTIONS_PRETTY_PRINT, &json_string);
 
-  config_store_->SaveSettings(kConfigName, json_string);
+  config_store_->SaveSettings(
+      kConfigName, json_string,
+      base::Bind([](ErrorPtr error) { CHECK(!error); }));
 }
 
 Config::Transaction::~Transaction() {
diff --git a/src/config_unittest.cc b/src/config_unittest.cc
index 4517428..fbb558a 100644
--- a/src/config_unittest.cc
+++ b/src/config_unittest.cc
@@ -258,9 +258,10 @@ TEST_F(ConfigTest, Setters) {
 
   EXPECT_CALL(*this, OnConfigChanged(_)).Times(1);
 
-  EXPECT_CALL(config_store_, SaveSettings(kConfigName, _))
-      .WillOnce(WithArgs<1>(Invoke([](const std::string& json) {
-        auto expected = R"({
+  EXPECT_CALL(config_store_, SaveSettings(kConfigName, _, _))
+      .WillOnce(WithArgs<1, 2>(
+          Invoke([](const std::string& json, const DoneCallback& callback) {
+            auto expected = R"({
           'version': 1,
           'api_key': 'set_api_key',
           'client_id': 'set_client_id',
@@ -281,8 +282,9 @@ TEST_F(ConfigTest, Setters) {
           'secret': 'AQIDBAU=',
           'service_url': 'set_service_url'
         })";
-        EXPECT_JSON_EQ(expected, *test::CreateValue(json));
-      })));
+            EXPECT_JSON_EQ(expected, *test::CreateValue(json));
+            callback.Run(nullptr);
+          })));
 
   change.Commit();
 }
diff --git a/src/weave_unittest.cc b/src/weave_unittest.cc
index c22eaa1..ebc66cd 100644
--- a/src/weave_unittest.cc
+++ b/src/weave_unittest.cc
@@ -204,11 +204,6 @@ class WeaveTest : public ::testing::Test {
             })));
   }
 
-  void InitConfigStore() {
-    EXPECT_CALL(config_store_, SaveSettings("config", _))
-        .WillRepeatedly(Return());
-  }
-
   void InitNetwork() {
     EXPECT_CALL(network_, AddConnectionChangedCallback(_))
         .WillRepeatedly(Invoke(
@@ -268,7 +263,6 @@ class WeaveTest : public ::testing::Test {
   }
 
   void InitDefaultExpectations() {
-    InitConfigStore();
     InitNetwork();
     EXPECT_CALL(wifi_, StartAccessPoint(MatchesRegex("TEST_NAME.*prv")))
         .WillOnce(Return());
@@ -361,13 +355,11 @@ TEST_F(WeaveTest, Mocks) {
 }
 
 TEST_F(WeaveTest, StartMinimal) {
-  InitConfigStore();
   device_ = weave::Device::Create(&config_store_, &task_runner_, &http_client_,
                                   &network_, nullptr, nullptr, &wifi_, nullptr);
 }
 
 TEST_F(WeaveTest, StartNoWifi) {
-  InitConfigStore();
   InitNetwork();
   InitHttpServer();
   InitDnsSd();
@@ -451,7 +443,6 @@ class WeaveWiFiSetupTest : public WeaveTest {
   void SetUp() override {
     WeaveTest::SetUp();
 
-    InitConfigStore();
     InitHttpServer();
     InitNetwork();
     InitDnsSd();