Add additional check to avoid duplicate subscriptions to Vehicle Data (#3512)

* Replace check for already existed subscriptions

* Delete unused enum

* Update unit tests and add new unit test

4 files changed, 94 insertions, 28 deletions

diff --git a/src/components/application_manager/rpc_plugins/vehicle_info_plugin/include/vehicle_info_plugin/vehicle_info_plugin.h b/src/components/application_manager/rpc_plugins/vehicle_info_plugin/include/vehicle_info_plugin/vehicle_info_plugin.h
index 5b597cfcab..1910a9efdf 100644
--- a/src/components/application_manager/rpc_plugins/vehicle_info_plugin/include/vehicle_info_plugin/vehicle_info_plugin.h
+++ b/src/components/application_manager/rpc_plugins/vehicle_info_plugin/include/vehicle_info_plugin/vehicle_info_plugin.h
@@ -42,7 +42,8 @@ class VehicleInfoAppExtension;
 namespace app_mngr = application_manager;
 namespace plugins = application_manager::plugin_manager;
 
-enum SubscribeStatus { SUBSCRIBE, UNSUBSCRIBE };
+bool IsSubscribedAppExist(const std::string& ivi,
+                          const app_mngr::ApplicationManager& app_manager);
 
 class VehicleInfoPlugin : public plugins::RPCPlugin {
  public:
@@ -96,7 +97,6 @@ class VehicleInfoPlugin : public plugins::RPCPlugin {
       const std::set<std::string>& list_of_subscriptions);
 
  private:
-  bool IsSubscribedAppExist(const std::string& ivi);
   bool IsAnyPendingSubscriptionExist(const std::string& ivi);
   void UnsubscribeFromRemovedVDItems();
   smart_objects::SmartObjectSPtr GetUnsubscribeIVIRequest(
diff --git a/src/components/application_manager/rpc_plugins/vehicle_info_plugin/src/vehicle_info_pending_resumption_handler.cc b/src/components/application_manager/rpc_plugins/vehicle_info_plugin/src/vehicle_info_pending_resumption_handler.cc
index d7b3f6ec8f..efbb81952b 100644
--- a/src/components/application_manager/rpc_plugins/vehicle_info_plugin/src/vehicle_info_pending_resumption_handler.cc
+++ b/src/components/application_manager/rpc_plugins/vehicle_info_plugin/src/vehicle_info_pending_resumption_handler.cc
@@ -38,6 +38,7 @@
 #include "application_manager/resumption/resumption_data_processor.h"
 #include "utils/helpers.h"
 #include "vehicle_info_plugin/custom_vehicle_data_manager.h"
+#include "vehicle_info_plugin/vehicle_info_plugin.h"
 
 namespace vehicle_info_plugin {
 SDL_CREATE_LOG_VARIABLE("VehicleInfoPlugin")
@@ -208,6 +209,7 @@ void VehicleInfoPendingResumptionHandler::on_event(
   SDL_LOG_AUTO_TRACE();
   sync_primitives::AutoLock lock(pending_resumption_lock_);
   using namespace application_manager;
+
   if (pending_requests_.empty()) {
     SDL_LOG_DEBUG("Not waiting for any response");
     return;
@@ -269,7 +271,17 @@ void VehicleInfoPendingResumptionHandler::HandleResumptionSubscriptionRequest(
   SDL_LOG_TRACE("app id " << app.app_id());
   auto& ext = dynamic_cast<VehicleInfoAppExtension&>(extension);
 
-  const auto subscriptions = ext.PendingSubscriptions().GetData();
+  auto subscriptions = ext.PendingSubscriptions().GetData();
+  for (auto ivi = subscriptions.begin(); ivi != subscriptions.end();) {
+    if (IsSubscribedAppExist(*ivi, application_manager_)) {
+      ext.RemovePendingSubscription(*ivi);
+      ext.subscribeToVehicleInfo(*ivi);
+      subscriptions.erase(ivi++);
+    } else {
+      ++ivi;
+    }
+  }
+
   if (subscriptions.empty()) {
     SDL_LOG_DEBUG("Subscriptions is empty");
     return;
diff --git a/src/components/application_manager/rpc_plugins/vehicle_info_plugin/src/vehicle_info_plugin.cc b/src/components/application_manager/rpc_plugins/vehicle_info_plugin/src/vehicle_info_plugin.cc
index 718814b201..1a357a86ee 100644
--- a/src/components/application_manager/rpc_plugins/vehicle_info_plugin/src/vehicle_info_plugin.cc
+++ b/src/components/application_manager/rpc_plugins/vehicle_info_plugin/src/vehicle_info_plugin.cc
@@ -48,6 +48,21 @@ namespace strings = application_manager::strings;
 namespace plugins = application_manager::plugin_manager;
 namespace commands = application_manager::commands;
 
+bool IsSubscribedAppExist(
+    const std::string& ivi,
+    const application_manager::ApplicationManager& app_manager) {
+  SDL_LOG_AUTO_TRACE();
+  auto apps_accessor = app_manager.applications();
+
+  for (auto& app : apps_accessor.GetData()) {
+    auto& ext = VehicleInfoAppExtension::ExtractVIExtension(*app);
+    if (ext.isSubscribedToVehicleInfo(ivi)) {
+      return true;
+    }
+  }
+  return false;
+}
+
 VehicleInfoPlugin::VehicleInfoPlugin()
     : application_manager_(nullptr), pending_resumption_handler_(nullptr) {}
 
@@ -164,13 +179,6 @@ void VehicleInfoPlugin::ProcessResumptionSubscription(
     application_manager::Application& app, VehicleInfoAppExtension& ext) {
   SDL_LOG_AUTO_TRACE();
 
-  auto pending = ext.PendingSubscriptions().GetData();
-  for (const auto& ivi : pending) {
-    if (IsSubscribedAppExist(ivi)) {
-      ext.RemovePendingSubscription(ivi);
-      ext.subscribeToVehicleInfo(ivi);
-    }
-  }
   pending_resumption_handler_->HandleResumptionSubscriptionRequest(ext, app);
 }
 
@@ -181,10 +189,9 @@ void VehicleInfoPlugin::RevertResumption(
   UNUSED(app);
 
   pending_resumption_handler_->OnResumptionRevert();
-
   std::set<std::string> subscriptions_to_revert;
   for (auto& ivi_data : list_of_subscriptions) {
-    if (!IsSubscribedAppExist(ivi_data) &&
+    if (!IsSubscribedAppExist(ivi_data, *application_manager_) &&
         !IsAnyPendingSubscriptionExist(ivi_data)) {
       subscriptions_to_revert.insert(ivi_data);
     }
@@ -230,19 +237,6 @@ smart_objects::SmartObjectSPtr VehicleInfoPlugin::CreateUnsubscriptionRequest(
   return request;
 }
 
-bool VehicleInfoPlugin::IsSubscribedAppExist(const std::string& ivi) {
-  SDL_LOG_AUTO_TRACE();
-  auto apps_accessor = application_manager_->applications();
-
-  for (auto& app : apps_accessor.GetData()) {
-    auto& ext = VehicleInfoAppExtension::ExtractVIExtension(*app);
-    if (ext.isSubscribedToVehicleInfo(ivi)) {
-      return true;
-    }
-  }
-  return false;
-}
-
 bool VehicleInfoPlugin::IsAnyPendingSubscriptionExist(const std::string& ivi) {
   SDL_LOG_AUTO_TRACE();
   auto apps_accessor = application_manager_->applications();
diff --git a/src/components/application_manager/rpc_plugins/vehicle_info_plugin/test/vehicle_info_pending_resumption_test.cc b/src/components/application_manager/rpc_plugins/vehicle_info_plugin/test/vehicle_info_pending_resumption_test.cc
index c2af7236f8..5b2f4d5e25 100644
--- a/src/components/application_manager/rpc_plugins/vehicle_info_plugin/test/vehicle_info_pending_resumption_test.cc
+++ b/src/components/application_manager/rpc_plugins/vehicle_info_plugin/test/vehicle_info_pending_resumption_test.cc
@@ -48,6 +48,7 @@ using namespace vehicle_info_plugin;
 
 using ::testing::_;
 using ::testing::DoAll;
+using ::testing::Mock;
 using ::testing::NiceMock;
 using ::testing::Return;
 using ::testing::ReturnRef;
@@ -226,8 +227,10 @@ class VehicleInfoPendingResumptionHandlerTest : public ::testing::Test {
   VehicleInfoPendingResumptionHandlerTest()
       : mock_message_helper_(
             *application_manager::MockMessageHelper::message_helper_mock())
-
-  {}
+      , applications_lock_(std::make_shared<sync_primitives::Lock>())
+      , applications_(application_set_, applications_lock_) {
+    Mock::VerifyAndClearExpectations(&mock_message_helper_);
+  }
 
   void SetUp() OVERRIDE {
     ON_CALL(app_manager_mock_, event_dispatcher())
@@ -238,6 +241,8 @@ class VehicleInfoPendingResumptionHandlerTest : public ::testing::Test {
         .WillByDefault(ReturnRef(resume_ctrl_mock_));
     ON_CALL(resume_ctrl_mock_, resumption_data_processor())
         .WillByDefault(ReturnRef(resumption_data_processor_mock_));
+    EXPECT_CALL(app_manager_mock_, applications())
+        .WillRepeatedly(Return(applications_));
 
     resumption_handler_.reset(
         new vehicle_info_plugin::VehicleInfoPendingResumptionHandler(
@@ -245,6 +250,11 @@ class VehicleInfoPendingResumptionHandlerTest : public ::testing::Test {
     MessageHelperResponseCreateExpectation();
   }
 
+  ~VehicleInfoPendingResumptionHandlerTest() {
+    Mock::VerifyAndClearExpectations(&mock_message_helper_);
+    Mock::VerifyAndClearExpectations(&app_manager_mock_);
+  }
+
   void MessageHelperResponseCreateExpectation() {
     const int default_corr_id = 0;
     static auto response = CreateHMIResponseMessage(
@@ -277,12 +287,15 @@ class VehicleInfoPendingResumptionHandlerTest : public ::testing::Test {
 
   MockMessageHelper& mock_message_helper_;
   MockApplicationManager app_manager_mock_;
-  MockResumeCtrl resume_ctrl_mock_;
+  NiceMock<MockResumeCtrl> resume_ctrl_mock_;
   MockResumptionDataProcessor resumption_data_processor_mock_;
   MockEventDispatcher event_dispatcher_mock_;
   MockRPCService mock_rpc_service_;
   NiceMock<MockCustomVehicleDataManager> custom_vehicle_data_manager_mock_;
   vehicle_info_plugin::VehicleInfoPlugin plugin_;
+  application_manager::ApplicationSet application_set_;
+  std::shared_ptr<sync_primitives::Lock> applications_lock_;
+  DataAccessor<application_manager::ApplicationSet> applications_;
 
   std::unique_ptr<vehicle_info_plugin::VehicleInfoPendingResumptionHandler>
       resumption_handler_;
@@ -300,6 +313,8 @@ TEST_F(VehicleInfoPendingResumptionHandlerTest, NoSubscriptionNoAction) {
 TEST_F(VehicleInfoPendingResumptionHandlerTest,
        OneAppSeveralVehicleDataSuccess) {
   auto mock_app = CreateApp(1);
+  application_set_.insert(mock_app);
+
   auto ext = CreateExtension(*mock_app);
   ext->AddPendingSubscription("gps");
   ext->AddPendingSubscription("speed");
@@ -327,6 +342,8 @@ TEST_F(VehicleInfoPendingResumptionHandlerTest,
 TEST_F(VehicleInfoPendingResumptionHandlerTest,
        OneAppSeveralVehicleDataResponseSuccess) {
   auto mock_app = CreateApp(1);
+  application_set_.insert(mock_app);
+
   auto ext = CreateExtension(*mock_app);
   ext->AddPendingSubscription("gps");
   ext->AddPendingSubscription("speed");
@@ -372,6 +389,8 @@ TEST_F(VehicleInfoPendingResumptionHandlerTest,
 TEST_F(VehicleInfoPendingResumptionHandlerTest,
        OneAppSeveralVehicleDataResponseOneFailed) {
   auto mock_app = CreateApp(1);
+  application_set_.insert(mock_app);
+
   auto ext = CreateExtension(*mock_app);
   ext->AddPendingSubscription("gps");
   ext->AddPendingSubscription("speed");
@@ -417,6 +436,8 @@ TEST_F(VehicleInfoPendingResumptionHandlerTest,
 TEST_F(VehicleInfoPendingResumptionHandlerTest,
        OneAppSeveralVehicleDataResponseAllFailed) {
   auto mock_app = CreateApp(1);
+  application_set_.insert(mock_app);
+
   auto ext = CreateExtension(*mock_app);
   ext->AddPendingSubscription("gps");
   ext->AddPendingSubscription("speed");
@@ -457,6 +478,8 @@ TEST_F(VehicleInfoPendingResumptionHandlerTest,
 TEST_F(VehicleInfoPendingResumptionHandlerTest,
        OneAppSeveralVehicleDataResponseOveralResultFailed) {
   auto mock_app = CreateApp(1);
+  application_set_.insert(mock_app);
+
   auto ext = CreateExtension(*mock_app);
   ext->AddPendingSubscription("gps");
   ext->AddPendingSubscription("speed");
@@ -491,7 +514,11 @@ TEST_F(VehicleInfoPendingResumptionHandlerTest,
 
 TEST_F(VehicleInfoPendingResumptionHandlerTest, TwoAppsOneSharedDataSuccess) {
   auto mock_app = CreateApp(1);
+  application_set_.insert(mock_app);
+
   auto mock_app2 = CreateApp(2);
+  application_set_.insert(mock_app2);
+
   auto ext = CreateExtension(*mock_app);
   auto ext2 = CreateExtension(*mock_app2);
   ext->AddPendingSubscription("gps");
@@ -533,7 +560,11 @@ TEST_F(VehicleInfoPendingResumptionHandlerTest, TwoAppsOneSharedDataSuccess) {
 TEST_F(VehicleInfoPendingResumptionHandlerTest,
        TwoAppsMultipleSharedDataSuccessResumption) {
   auto mock_app = CreateApp(1);
+  application_set_.insert(mock_app);
+
   auto mock_app2 = CreateApp(2);
+  application_set_.insert(mock_app2);
+
   auto ext = CreateExtension(*mock_app);
   auto ext2 = CreateExtension(*mock_app2);
   ext->AddPendingSubscription("gps");
@@ -580,7 +611,11 @@ TEST_F(VehicleInfoPendingResumptionHandlerTest,
 TEST_F(VehicleInfoPendingResumptionHandlerTest,
        TwoAppsOneSharedDataFailRetryforSecondApp) {
   auto mock_app = CreateApp(1);
+  application_set_.insert(mock_app);
+
   auto mock_app2 = CreateApp(2);
+  application_set_.insert(mock_app2);
+
   auto ext = CreateExtension(*mock_app);
   auto ext2 = CreateExtension(*mock_app2);
   ext->AddPendingSubscription("gps");
@@ -633,4 +668,29 @@ TEST_F(VehicleInfoPendingResumptionHandlerTest,
   EXPECT_EQ(ext2->PendingSubscriptions().GetData().size(), 0u);
 }
 
+TEST_F(VehicleInfoPendingResumptionHandlerTest,
+       TwoAppsOneSharedDataAlreadySubscribedByFirstAppNoRetryforSecondApp) {
+  auto mock_app = CreateApp(1);
+  application_set_.insert(mock_app);
+
+  auto mock_app2 = CreateApp(2);
+  application_set_.insert(mock_app2);
+
+  auto ext = CreateExtension(*mock_app);
+  auto ext2 = CreateExtension(*mock_app2);
+
+  ext->subscribeToVehicleInfo("gps");
+  ext2->AddPendingSubscription("gps");
+
+  EXPECT_CALL(resumption_data_processor_mock_, SubscribeToResponse(_, _))
+      .Times(0);
+  EXPECT_CALL(event_dispatcher_mock_, raise_event(_)).Times(0);
+  EXPECT_CALL(mock_rpc_service_, ManageHMICommand(_, _)).Times(0);
+
+  resumption_handler_->HandleResumptionSubscriptionRequest(*ext2, *mock_app2);
+
+  EXPECT_TRUE(ext->isSubscribedToVehicleInfo("gps"));
+  EXPECT_TRUE(ext2->isSubscribedToVehicleInfo("gps"));
+  EXPECT_EQ(ext2->PendingSubscriptions().GetData().size(), 0u);
+}
 }  // namespace vehicle_info_plugin_test