Merge pull request #3727 from ygorshenin/fix-features-loading-guard

[indexer] Fixed FeaturesLoaderGuard.

4 files changed, 108 insertions, 30 deletions

diff --git a/indexer/index.cpp b/indexer/index.cpp
index 830faa1adf..6caaa521f7 100644
--- a/indexer/index.cpp
+++ b/indexer/index.cpp
@@ -83,36 +83,52 @@ bool Index::DeregisterMap(CountryFile const & countryFile) { return Deregister(c
 // Index::FeaturesLoaderGuard implementation
 //////////////////////////////////////////////////////////////////////////////////
 
-Index::FeaturesLoaderGuard::FeaturesLoaderGuard(Index const & parent, MwmId const & id)
-  : m_handle(parent.GetMwmHandleById(id))
-  , m_vector(m_handle.GetValue<MwmValue>()->m_cont, m_handle.GetValue<MwmValue>()->GetHeader(),
-             m_handle.GetValue<MwmValue>()->m_table.get())
+Index::FeaturesLoaderGuard::FeaturesLoaderGuard(Index const & index, MwmId const & id)
+  : m_handle(index.GetMwmHandleById(id))
 {
+  if (!m_handle.IsAlive())
+    return;
+
+  auto const & value = *m_handle.GetValue<MwmValue>();
+  m_vector = make_unique<FeaturesVector>(value.m_cont, value.GetHeader(), value.m_table.get());
 }
 
 string Index::FeaturesLoaderGuard::GetCountryFileName() const
 {
   if (!m_handle.IsAlive())
     return string();
+
   return m_handle.GetValue<MwmValue>()->GetCountryFileName();
 }
 
 bool Index::FeaturesLoaderGuard::IsWorld() const
 {
+  if (!m_handle.IsAlive())
+    return false;
+
   return m_handle.GetValue<MwmValue>()->GetHeader().GetType() == feature::DataHeader::world;
 }
 
-void Index::FeaturesLoaderGuard::GetFeatureByIndex(uint32_t index, FeatureType & ft) const
+bool Index::FeaturesLoaderGuard::GetFeatureByIndex(uint32_t index, FeatureType & ft) const
 {
+  if (!m_handle.IsAlive())
+    return false;
+
   MwmId const & id = m_handle.GetId();
   ASSERT_NOT_EQUAL(osm::Editor::FeatureStatus::Deleted, m_editor.GetFeatureStatus(id, index),
                    ("Deleted feature was cached. It should not be here. Please review your code."));
-  if (!m_editor.Instance().GetEditedFeature(id, index, ft))
-    GetOriginalFeatureByIndex(index, ft);
+  if (m_editor.Instance().GetEditedFeature(id, index, ft))
+    return true;
+  return GetOriginalFeatureByIndex(index, ft);
 }
 
-void Index::FeaturesLoaderGuard::GetOriginalFeatureByIndex(uint32_t index, FeatureType & ft) const
+bool Index::FeaturesLoaderGuard::GetOriginalFeatureByIndex(uint32_t index, FeatureType & ft) const
 {
-  m_vector.GetByIndex(index, ft);
+  if (!m_handle.IsAlive())
+    return false;
+
+  ASSERT(m_vector != nullptr, ());
+  m_vector->GetByIndex(index, ft);
   ft.SetID(FeatureID(m_handle.GetId(), index));
+  return true;
 }
diff --git a/indexer/index.hpp b/indexer/index.hpp
index 8c8467e49e..6979d7af10 100644
--- a/indexer/index.hpp
+++ b/indexer/index.hpp
@@ -272,20 +272,21 @@ public:
   class FeaturesLoaderGuard
   {
   public:
-    FeaturesLoaderGuard(Index const & parent, MwmId const & id);
+    FeaturesLoaderGuard(Index const & index, MwmId const & id);
 
     inline MwmSet::MwmId const & GetId() const { return m_handle.GetId(); }
     string GetCountryFileName() const;
     bool IsWorld() const;
+
     /// Everyone, except Editor core, should use this method.
-    void GetFeatureByIndex(uint32_t index, FeatureType & ft) const;
+    WARN_UNUSED_RESULT bool GetFeatureByIndex(uint32_t index, FeatureType & ft) const;
+
     /// Editor core only method, to get 'untouched', original version of feature.
-    void GetOriginalFeatureByIndex(uint32_t index, FeatureType & ft) const;
-    inline FeaturesVector const & GetFeaturesVector() const { return m_vector; }
+    WARN_UNUSED_RESULT bool GetOriginalFeatureByIndex(uint32_t index, FeatureType & ft) const;
 
   private:
     MwmHandle m_handle;
-    FeaturesVector m_vector;
+    unique_ptr<FeaturesVector> m_vector;
     osm::Editor & m_editor = osm::Editor::Instance();
   };
 
diff --git a/indexer/osm_editor.cpp b/indexer/osm_editor.cpp
index 243b3097c0..00ae1161b2 100644
--- a/indexer/osm_editor.cpp
+++ b/indexer/osm_editor.cpp
@@ -97,18 +97,17 @@ bool AreFeaturesEqualButStreet(FeatureType const & a, FeatureType const & b)
   return true;
 }
 
-XMLFeature GetMatchingFeatureFromOSM(osm::ChangesetWrapper & cw,
-                                     unique_ptr<FeatureType const> featurePtr)
+XMLFeature GetMatchingFeatureFromOSM(osm::ChangesetWrapper & cw, FeatureType const & ft)
 {
-  ASSERT_NOT_EQUAL(featurePtr->GetFeatureType(), feature::GEOM_LINE,
+  ASSERT_NOT_EQUAL(ft.GetFeatureType(), feature::GEOM_LINE,
                    ("Line features are not supported yet."));
-  if (featurePtr->GetFeatureType() == feature::GEOM_POINT)
-    return cw.GetMatchingNodeFeatureFromOSM(featurePtr->GetCenter());
+  if (ft.GetFeatureType() == feature::GEOM_POINT)
+    return cw.GetMatchingNodeFeatureFromOSM(ft.GetCenter());
 
   // Warning: geometry is cached in FeatureType. So if it wasn't BEST_GEOMETRY,
   // we can never have it. Features here came from editors loader and should
   // have BEST_GEOMETRY geometry.
-  auto geometry = featurePtr->GetTriangesAsPoints(FeatureType::BEST_GEOMETRY);
+  auto geometry = ft.GetTriangesAsPoints(FeatureType::BEST_GEOMETRY);
 
   // Filters out duplicate points for closed ways or triangles' vertices.
   my::SortUnique(geometry);
@@ -222,7 +221,20 @@ void Editor::LoadMapEdits()
           }
           else
           {
-            fti.m_feature = *m_getOriginalFeatureFn(fid);
+            auto const originalFeaturePtr = m_getOriginalFeatureFn(fid);
+            if (!originalFeaturePtr)
+            {
+              LOG(LERROR, ("A feature with id", fid, "cannot be loaded."));
+              // TODO: alohalytics::LogEvent in this function leads to a linker error
+              // with complains on alohalytics::Stats::Instance() reference is missing.
+              // The problem remains even when the whole code but alohalytics::LogEvent
+              // is removed in this function. There are no problems with this call in
+              // other functions.
+              // alohalytics::LogEvent("Editor_MissingFeature_Error");
+              goto SECTION_END;
+            }
+
+            fti.m_feature = *originalFeaturePtr;
             fti.m_feature.ApplyPatch(xml);
           }
 
@@ -450,12 +462,20 @@ Editor::SaveResult Editor::SaveEditedFeature(EditableMapObject const & emo)
   {
     ASSERT_NOT_EQUAL(featureStatus, FeatureStatus::Deleted, ("Unexpected feature status."));
 
+    auto const originalFeaturePtr = m_getOriginalFeatureFn(fid);
+    if (!originalFeaturePtr)
+    {
+      LOG(LERROR, ("A feature with id", fid, "cannot be loaded."));
+      alohalytics::LogEvent("Editor_MissingFeature_Error");
+      return SaveResult::SavingError;
+    }
+
     fti.m_feature = featureStatus == FeatureStatus::Untouched
-        ? *m_getOriginalFeatureFn(fid)
+        ? *originalFeaturePtr
         : m_features[fid.m_mwmId][fid.m_index].m_feature;
     fti.m_feature.ReplaceBy(emo);
     bool const sameAsInMWM = featureStatus != FeatureStatus::Created &&
-        AreFeaturesEqualButStreet(fti.m_feature, *m_getOriginalFeatureFn(fid)) &&
+        AreFeaturesEqualButStreet(fti.m_feature, *originalFeaturePtr) &&
         emo.GetStreet().m_defaultName == m_getOriginalFeatureStreetFn(fti.m_feature);
 
     if (featureStatus != FeatureStatus::Untouched)
@@ -615,8 +635,15 @@ EditableProperties Editor::GetEditableProperties(FeatureType const & feature) co
   // Disable opening hours editing if opening hours cannot be parsed.
   if (GetFeatureStatus(feature.GetID()) != FeatureStatus::Created)
   {
-    auto const & originalFeature = m_getOriginalFeatureFn(feature.GetID());
-    auto const & metadata = originalFeature->GetMetadata();
+    auto const originalFeaturePtr = m_getOriginalFeatureFn(feature.GetID());
+    if (originalFeaturePtr)
+    {
+      LOG(LERROR, ("A feature with id", feature.GetID(), "cannot be loaded."));
+      alohalytics::LogEvent("Editor_MissingFeature_Error");
+      return {};
+    }
+
+    auto const & metadata = originalFeaturePtr->GetMetadata();
     auto const & featureOpeningHours = metadata.Get(feature::Metadata::FMD_OPEN_HOURS);
     // Note: empty string is parsed as a valid opening hours rule.
     if (!osmoh::OpeningHours(featureOpeningHours).IsValid())
@@ -770,8 +797,17 @@ void Editor::UploadChanges(string const & key, string const & secret, TChangeset
                 feature.SetTagValue(kAddrStreetTag, fti.m_street);
               ourDebugFeatureString = DebugPrint(feature);
 
-              XMLFeature osmFeature =
-                  GetMatchingFeatureFromOSM(changeset, m_getOriginalFeatureFn(fti.m_feature.GetID()));
+              auto const originalFeaturePtr = m_getOriginalFeatureFn(fti.m_feature.GetID());
+              if (!originalFeaturePtr)
+              {
+                LOG(LERROR, ("A feature with id", fti.m_feature.GetID(), "cannot be loaded."));
+                alohalytics::LogEvent("Editor_MissingFeature_Error");
+                RemoveFeatureFromStorageIfExists(fti.m_feature.GetID());
+                continue;
+              }
+
+              XMLFeature osmFeature = GetMatchingFeatureFromOSM(
+                  changeset, *originalFeaturePtr);
               XMLFeature const osmFeatureCopy = osmFeature;
               osmFeature.ApplyPatch(feature);
               // Check to avoid uploading duplicates into OSM.
@@ -790,8 +826,16 @@ void Editor::UploadChanges(string const & key, string const & secret, TChangeset
             break;
 
           case FeatureStatus::Deleted:
+            auto const originalFeaturePtr = m_getOriginalFeatureFn(fti.m_feature.GetID());
+            if (!originalFeaturePtr)
+            {
+              LOG(LERROR, ("A feature with id", fti.m_feature.GetID(), "cannot be loaded."));
+              alohalytics::LogEvent("Editor_MissingFeature_Error");
+              RemoveFeatureFromStorageIfExists(fti.m_feature.GetID());
+              continue;
+            }
             changeset.Delete(GetMatchingFeatureFromOSM(
-                changeset, m_getOriginalFeatureFn(fti.m_feature.GetID())));
+                changeset, *originalFeaturePtr));
             break;
           }
           fti.m_uploadStatus = kUploaded;
@@ -925,6 +969,11 @@ void Editor::RemoveFeatureFromStorageIfExists(MwmSet::MwmId const & mwmId, uint3
     m_features.erase(matchedMwm);
 }
 
+void Editor::RemoveFeatureFromStorageIfExists(FeatureID const & fid)
+{
+  return RemoveFeatureFromStorageIfExists(fid.m_mwmId, fid.m_index);
+}
+
 void Editor::Invalidate()
 {
   if (m_invalidateFn)
@@ -942,7 +991,16 @@ void Editor::MarkFeatureAsObsolete(FeatureID const & fid)
 
   auto & fti = m_features[fid.m_mwmId][fid.m_index];
   // If a feature was modified we can drop all changes since it's now obsolete.
-  fti.m_feature = *m_getOriginalFeatureFn(fid);
+  auto const originalFeaturePtr = m_getOriginalFeatureFn(fid);
+
+  if (originalFeaturePtr)
+  {
+    LOG(LERROR, ("A feature with id", fid, "cannot be loaded."));
+    alohalytics::LogEvent("Editor_MissingFeature_Error");
+    return;
+  }
+
+  fti.m_feature = *originalFeaturePtr;
   fti.m_status = FeatureStatus::Obsolete;
   fti.m_modificationTimestamp = time(nullptr);
 
diff --git a/indexer/osm_editor.hpp b/indexer/osm_editor.hpp
index e545470051..109b089600 100644
--- a/indexer/osm_editor.hpp
+++ b/indexer/osm_editor.hpp
@@ -113,7 +113,9 @@ public:
   {
     NothingWasChanged,
     SavedSuccessfully,
-    NoFreeSpaceError
+    NoFreeSpaceError,
+    NoUnderlyingMapError,
+    SavingError
   };
   /// Editor checks internally if any feature params were actually edited.
   SaveResult SaveEditedFeature(EditableMapObject const & emo);
@@ -171,6 +173,7 @@ private:
   /// @returns false if fails.
   bool Save(string const & fullFilePath) const;
   void RemoveFeatureFromStorageIfExists(MwmSet::MwmId const & mwmId, uint32_t index);
+  void RemoveFeatureFromStorageIfExists(FeatureID const & fid);
   /// Notify framework that something has changed and should be redisplayed.
   void Invalidate();