Fix onsystemrequest retry logic (#3400)

* Fix onsystemrequest retry logic

* Cache policy update file from HMI for retries

* Fix issues with previous commit

Co-authored-by: jacobkeeler <jacob.keeler@livioradio.com>

11 files changed, 100 insertions, 25 deletions

diff --git a/src/components/application_manager/include/application_manager/message_helper.h b/src/components/application_manager/include/application_manager/message_helper.h
index 5d95a2468d..09d225a120 100644
--- a/src/components/application_manager/include/application_manager/message_helper.h
+++ b/src/components/application_manager/include/application_manager/message_helper.h
@@ -491,9 +491,23 @@ class MessageHelper {
    * @brief Send notification for Update of Policy Table
    * with PT Snapshot.
    * @param connection_key Id of application to send message to
+   * @param snapshot_file_path path to PT Snapshot
+   * @param url If empty string, no URL is provided
+   * @param timeout If -1 no timeout is provided
+   */
+  static void SendPolicySnapshotNotification(
+      uint32_t connection_key,
+      const std::string& snapshot_file_path,
+      const std::string& url,
+      ApplicationManager& app_mngr);
+
+  /*
+   * @brief Send notification for Update of Policy Table
+   * with PT Snapshot.
+   * @param connection_key Id of application to send message to
    * @param policy_data PT Snapshot
    * @param url If empty string, no URL is provided
-   * @param timeout If -1 no timeout is provdied
+   * @param timeout If -1 no timeout is provided
    */
   static void SendPolicySnapshotNotification(
       uint32_t connection_key,
diff --git a/src/components/application_manager/include/application_manager/policies/policy_handler.h b/src/components/application_manager/include/application_manager/policies/policy_handler.h
index 2c7b878023..8559e77061 100644
--- a/src/components/application_manager/include/application_manager/policies/policy_handler.h
+++ b/src/components/application_manager/include/application_manager/policies/policy_handler.h
@@ -419,7 +419,9 @@ class PolicyHandler : public PolicyHandlerInterface,
   uint32_t ChoosePTUApplication(
       const PTUIterationType iteration_type =
           PTUIterationType::DefaultIteration) OVERRIDE;
-  void CacheRetryInfo(const uint32_t app_id, const std::string url) OVERRIDE;
+  void CacheRetryInfo(const uint32_t app_id = 0,
+                      const std::string url = std::string(),
+                      const std::string snapshot_path = std::string()) OVERRIDE;
 #endif  // EXTERNAL_PROPRIETARY_MODE
 
   uint32_t GetAppIdForSending() const OVERRIDE;
@@ -903,6 +905,7 @@ class PolicyHandler : public PolicyHandlerInterface,
   // PTU retry information
   uint32_t last_ptu_app_id_;
   std::string retry_update_url_;
+  std::string policy_snapshot_path_;
 #endif  // EXTERNAL_PROPRIETARY_MODE
 
   /**
diff --git a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/on_system_request_notification.cc b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/on_system_request_notification.cc
index d96d2bb90e..4f83e3e653 100644
--- a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/on_system_request_notification.cc
+++ b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/on_system_request_notification.cc
@@ -84,10 +84,12 @@ void OnSystemRequestNotification::Run() {
       policy_handler_.CacheRetryInfo(msg_params.keyExists(strings::app_id)
                                          ? msg_params[strings::app_id].asUInt()
                                          : 0,
-                                     msg_params[strings::url].asString());
+                                     msg_params[strings::url].asString(),
+                                     msg_params[strings::file_name].asString());
     } else {
       // Clear cached retry info
-      policy_handler_.CacheRetryInfo(0, std::string());
+      policy_handler_.CacheRetryInfo(
+          0, std::string(), msg_params[strings::file_name].asString());
 
       // URL and app are chosen by Core for PROPRIETARY mode normally
       uint32_t app_id = 0;
diff --git a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/on_system_request_notification.cc b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/on_system_request_notification.cc
index 991144d7cf..9c868172f5 100644
--- a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/on_system_request_notification.cc
+++ b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/on_system_request_notification.cc
@@ -118,10 +118,14 @@ void OnSystemRequestNotification::Run() {
           mobile_apis::RequestType::OEM_SPECIFIC);
 
   BinaryMessage binary_data;
-  if (binary_data_is_required) {
+  if (binary_data_is_required &&
+      (*message_)[strings::msg_params].keyExists(strings::file_name)) {
     const std::string filename =
         (*message_)[strings::msg_params][strings::file_name].asString();
     file_system::ReadBinaryFile(filename, binary_data);
+  } else if ((*message_)[strings::params].keyExists(strings::binary_data)) {
+    // Binary data may already be attached to the message
+    binary_data = (*message_)[strings::params][strings::binary_data].asBinary();
   }
 
   if (mobile_apis::RequestType::OEM_SPECIFIC == request_type) {
diff --git a/src/components/application_manager/src/message_helper/message_helper.cc b/src/components/application_manager/src/message_helper/message_helper.cc
index 9b12eb2889..d6fbed115b 100644
--- a/src/components/application_manager/src/message_helper/message_helper.cc
+++ b/src/components/application_manager/src/message_helper/message_helper.cc
@@ -2526,6 +2526,33 @@ bool MessageHelper::SendUnsubscribedWayPoints(ApplicationManager& app_mngr) {
 
 void MessageHelper::SendPolicySnapshotNotification(
     uint32_t connection_key,
+    const std::string& snapshot_file_path,
+    const std::string& url,
+    ApplicationManager& app_mngr) {
+  LOG4CXX_AUTO_TRACE(logger_);
+  smart_objects::SmartObject content(smart_objects::SmartType_Map);
+  const auto request_type =
+#if defined(PROPRIETARY_MODE) || defined(EXTERNAL_PROPRIETARY_MODE)
+      mobile_apis::RequestType::PROPRIETARY;
+#else
+      mobile_apis::RequestType::HTTP;
+#endif  // PROPRIETARY || EXTERNAL_PROPRIETARY_MODE
+
+  content[strings::msg_params][strings::request_type] = request_type;
+
+  if (!url.empty()) {
+    content[strings::msg_params][strings::url] = url;
+  } else {
+    LOG4CXX_WARN(logger_, "No service URLs");
+  }
+
+  content[strings::msg_params][strings::file_name] = snapshot_file_path;
+
+  SendSystemRequestNotification(connection_key, content, app_mngr);
+}
+
+void MessageHelper::SendPolicySnapshotNotification(
+    uint32_t connection_key,
     const std::vector<uint8_t>& policy_data,
     const std::string& url,
     ApplicationManager& app_mngr) {
diff --git a/src/components/application_manager/src/policies/policy_handler.cc b/src/components/application_manager/src/policies/policy_handler.cc
index 678916f7eb..5032c2884c 100644
--- a/src/components/application_manager/src/policies/policy_handler.cc
+++ b/src/components/application_manager/src/policies/policy_handler.cc
@@ -449,9 +449,11 @@ uint32_t PolicyHandler::ChoosePTUApplication(
 }
 
 void PolicyHandler::CacheRetryInfo(const uint32_t app_id,
-                                   const std::string url) {
+                                   const std::string url,
+                                   const std::string snapshot_path) {
   last_ptu_app_id_ = app_id;
   retry_update_url_ = url;
+  policy_snapshot_path_ = snapshot_path;
 }
 #endif  // EXTERNAL_PROPRIETARY_MODE
 
@@ -1627,11 +1629,10 @@ void PolicyHandler::OnSnapshotCreated(const BinaryMessage& pt_string,
     uint32_t app_id_for_sending = 0;
     const std::string& url =
         GetNextUpdateUrl(PTUIterationType::RetryIteration, app_id_for_sending);
-    if (0 != url.length()) {
+    if (0 != url.length() && !policy_snapshot_path_.empty()) {
       MessageHelper::SendPolicySnapshotNotification(
-          app_id_for_sending, pt_string, url, application_manager_);
+          app_id_for_sending, policy_snapshot_path_, url, application_manager_);
     }
-
   } else {
     std::string policy_snapshot_full_path;
     if (!SaveSnapshot(pt_string, policy_snapshot_full_path)) {
diff --git a/src/components/application_manager/test/include/application_manager/mock_message_helper.h b/src/components/application_manager/test/include/application_manager/mock_message_helper.h
index eb3c97bfff..5f6e613123 100644
--- a/src/components/application_manager/test/include/application_manager/mock_message_helper.h
+++ b/src/components/application_manager/test/include/application_manager/mock_message_helper.h
@@ -162,6 +162,11 @@ class MockMessageHelper {
                     const bool require_encryption));
   MOCK_METHOD4(SendPolicySnapshotNotification,
                void(uint32_t connection_key,
+                    const std::string& snapshot_file_path,
+                    const std::string& url,
+                    ApplicationManager& app_mngr));
+  MOCK_METHOD4(SendPolicySnapshotNotification,
+               void(uint32_t connection_key,
                     const std::vector<uint8_t>& policy_data,
                     const std::string& url,
                     ApplicationManager& app_mngr));
diff --git a/src/components/application_manager/test/mock_message_helper.cc b/src/components/application_manager/test/mock_message_helper.cc
index 959bbf5bd0..a001e852b8 100644
--- a/src/components/application_manager/test/mock_message_helper.cc
+++ b/src/components/application_manager/test/mock_message_helper.cc
@@ -220,6 +220,15 @@ void MessageHelper::SendOnPermissionsChangeNotification(
 
 void MessageHelper::SendPolicySnapshotNotification(
     uint32_t connection_key,
+    const std::string& snapshot_file_path,
+    const std::string& url,
+    ApplicationManager& app_mngr) {
+  MockMessageHelper::message_helper_mock()->SendPolicySnapshotNotification(
+      connection_key, snapshot_file_path, url, app_mngr);
+}
+
+void MessageHelper::SendPolicySnapshotNotification(
+    uint32_t connection_key,
     const std::vector<uint8_t>& policy_data,
     const std::string& url,
     ApplicationManager& app_mngr) {
diff --git a/src/components/include/application_manager/policies/policy_handler_interface.h b/src/components/include/application_manager/policies/policy_handler_interface.h
index 735f44a314..1c039d6a88 100644
--- a/src/components/include/application_manager/policies/policy_handler_interface.h
+++ b/src/components/include/application_manager/policies/policy_handler_interface.h
@@ -375,8 +375,13 @@ class PolicyHandlerInterface : public VehicleDataItemProvider {
    * @param url The URL provided by the HMI to be used for performing PTU
    * retries. If empty, the existing cached URL will be cleared and Core will
    * choose which URLs to use on retry
+   * @param snapshot_path The PT snapshot path provided by the HMI. If empty,
+   * the existing cached snapshot path will be cleared.
    */
-  virtual void CacheRetryInfo(const uint32_t app_id, const std::string url) = 0;
+  virtual void CacheRetryInfo(
+      const uint32_t app_id = 0,
+      const std::string url = std::string(),
+      const std::string snapshot_path = std::string()) = 0;
 #endif  // EXTERNAL_PROPRIETARY_MODE
 
   /**
diff --git a/src/components/include/test/application_manager/policies/mock_policy_handler_interface.h b/src/components/include/test/application_manager/policies/mock_policy_handler_interface.h
index 0dde38bc79..38c1cca69e 100644
--- a/src/components/include/test/application_manager/policies/mock_policy_handler_interface.h
+++ b/src/components/include/test/application_manager/policies/mock_policy_handler_interface.h
@@ -193,8 +193,10 @@ class MockPolicyHandlerInterface : public policy::PolicyHandlerInterface {
 #ifndef EXTERNAL_PROPRIETARY_MODE
   MOCK_METHOD1(ChoosePTUApplication,
                uint32_t(const policy::PTUIterationType iteration_type));
-  MOCK_METHOD2(CacheRetryInfo,
-               void(const uint32_t app_id, const std::string url));
+  MOCK_METHOD3(CacheRetryInfo,
+               void(const uint32_t app_id,
+                    const std::string url,
+                    const std::string snapshot_path));
 #endif
   MOCK_CONST_METHOD0(GetAppIdForSending, uint32_t());
   MOCK_METHOD1(
diff --git a/src/components/policy/policy_regular/src/policy_manager_impl.cc b/src/components/policy/policy_regular/src/policy_manager_impl.cc
index c6389b503b..0cdca26f2e 100644
--- a/src/components/policy/policy_regular/src/policy_manager_impl.cc
+++ b/src/components/policy/policy_regular/src/policy_manager_impl.cc
@@ -610,23 +610,26 @@ void PolicyManagerImpl::GetUpdateUrls(const uint32_t service_type,
 
 bool PolicyManagerImpl::RequestPTUpdate(const PTUIterationType iteration_type) {
   LOG4CXX_AUTO_TRACE(logger_);
-  std::shared_ptr<policy_table::Table> policy_table_snapshot =
-      cache_->GenerateSnapshot();
-  if (!policy_table_snapshot) {
-    LOG4CXX_ERROR(logger_, "Failed to create snapshot of policy table");
-    return false;
-  }
+  BinaryMessage update;
+  if (PTUIterationType::DefaultIteration == iteration_type) {
+    std::shared_ptr<policy_table::Table> policy_table_snapshot =
+        cache_->GenerateSnapshot();
+    if (!policy_table_snapshot) {
+      LOG4CXX_ERROR(logger_, "Failed to create snapshot of policy table");
+      return false;
+    }
 
-  IsPTValid(policy_table_snapshot, policy_table::PT_SNAPSHOT);
+    IsPTValid(policy_table_snapshot, policy_table::PT_SNAPSHOT);
 
-  Json::Value value = policy_table_snapshot->ToJsonValue();
-  Json::StreamWriterBuilder writer_builder;
-  writer_builder["indentation"] = "";
-  std::string message_string = Json::writeString(writer_builder, value);
+    Json::Value value = policy_table_snapshot->ToJsonValue();
+    Json::StreamWriterBuilder writer_builder;
+    writer_builder["indentation"] = "";
+    std::string message_string = Json::writeString(writer_builder, value);
 
-  LOG4CXX_DEBUG(logger_, "Snapshot contents is : " << message_string);
+    LOG4CXX_DEBUG(logger_, "Snapshot contents is : " << message_string);
 
-  BinaryMessage update(message_string.begin(), message_string.end());
+    update = BinaryMessage(message_string.begin(), message_string.end());
+  }
   ptu_requested_ = true;
   listener_->OnSnapshotCreated(update, iteration_type);
   return true;