diff --git a/ugc/api.cpp b/ugc/api.cpp index 7ea6954324..d6b42c4071 100644 --- a/ugc/api.cpp +++ b/ugc/api.cpp @@ -1,6 +1,6 @@ #include "ugc/api.hpp" -#include "platform/platform.hpp" +#include "base/assert.hpp" #include @@ -14,19 +14,24 @@ Api::Api(Index const & index) : m_storage(index), m_loader(index) m_thread.Push([this] { m_storage.Load(); }); } -void Api::GetUGC(FeatureID const & id, UGCCallback callback) +void Api::GetUGC(FeatureID const & id, UGCCallback const & callback) { m_thread.Push([=] { GetUGCImpl(id, callback); }); } -void Api::SetUGCUpdate(FeatureID const & id, UGCUpdate const & ugc) +void Api::SetUGCUpdate(FeatureID const & id, UGCUpdate const & ugc, + OnResultCallback const & callback /* nullptr */) { - m_thread.Push([=] { SetUGCUpdateImpl(id, ugc); }); + m_thread.Push([=] { + auto const res = SetUGCUpdateImpl(id, ugc); + if (callback) + callback(res); + }); } -void Api::GetUGCToSend(UGCJsonToSendCallback const & fn) +void Api::GetUGCToSend(UGCJsonToSendCallback const & callback) { - m_thread.Push([fn, this] { GetUGCToSendImpl(fn); }); + m_thread.Push([callback, this] { GetUGCToSendImpl(callback); }); } void Api::SendingCompleted() @@ -39,29 +44,31 @@ void Api::SaveUGCOnDisk() m_thread.Push([this] { SaveUGCOnDiskImpl(); }); } -void Api::GetUGCImpl(FeatureID const & id, UGCCallback callback) +void Api::GetUGCImpl(FeatureID const & id, UGCCallback const & callback) { + CHECK(callback, ()); if (!id.IsValid()) { - GetPlatform().RunOnGuiThread([callback] { callback({}, {}); }); + callback({}, {}); return; } auto const update = m_storage.GetUGCUpdate(id); auto const ugc = m_loader.GetUGC(id); - GetPlatform().RunOnGuiThread([ugc, update, callback] { callback(ugc, update); }); + callback(ugc, update); } -void Api::SetUGCUpdateImpl(FeatureID const & id, UGCUpdate const & ugc) +Storage::SettingResult Api::SetUGCUpdateImpl(FeatureID const & id, UGCUpdate const & ugc) { - m_storage.SetUGCUpdate(id, ugc); + return m_storage.SetUGCUpdate(id, ugc); } -void Api::GetUGCToSendImpl(UGCJsonToSendCallback const & fn) +void Api::GetUGCToSendImpl(UGCJsonToSendCallback const & callback) { + CHECK(callback, ()); auto json = m_storage.GetUGCToSend(); - fn(move(json)); + callback(move(json)); } void Api::SendingCompletedImpl() diff --git a/ugc/api.hpp b/ugc/api.hpp index 8eec8c54c5..b507ac1f95 100644 --- a/ugc/api.hpp +++ b/ugc/api.hpp @@ -1,11 +1,13 @@ #pragma once -#include "base/worker_thread.hpp" - #include "ugc/loader.hpp" #include "ugc/storage.hpp" #include "ugc/types.hpp" +#include "platform/safe_callback.hpp" + +#include "base/worker_thread.hpp" + #include class Index; @@ -16,21 +18,23 @@ namespace ugc class Api { public: - using UGCCallback = std::function; + using UGCCallback = platform::SafeCallback; using UGCJsonToSendCallback = std::function; + using OnResultCallback = platform::SafeCallback; explicit Api(Index const & index); - void GetUGC(FeatureID const & id, UGCCallback callback); - void SetUGCUpdate(FeatureID const & id, UGCUpdate const & ugc); - void GetUGCToSend(UGCJsonToSendCallback const & fn); + void GetUGC(FeatureID const & id, UGCCallback const & callback); + void SetUGCUpdate(FeatureID const & id, UGCUpdate const & ugc, + OnResultCallback const & callback = nullptr); + void GetUGCToSend(UGCJsonToSendCallback const & callback); void SendingCompleted(); void SaveUGCOnDisk(); private: - void GetUGCImpl(FeatureID const & id, UGCCallback callback); - void SetUGCUpdateImpl(FeatureID const & id, UGCUpdate const & ugc); - void GetUGCToSendImpl(UGCJsonToSendCallback const & fn); + void GetUGCImpl(FeatureID const & id, UGCCallback const & callback); + Storage::SettingResult SetUGCUpdateImpl(FeatureID const & id, UGCUpdate const & ugc); + void GetUGCToSendImpl(UGCJsonToSendCallback const & callback); void SendingCompletedImpl(); void SaveUGCOnDiskImpl(); diff --git a/ugc/storage.cpp b/ugc/storage.cpp index a9544879ef..9608d9ad84 100644 --- a/ugc/storage.cpp +++ b/ugc/storage.cpp @@ -116,8 +116,11 @@ UGCUpdate Storage::GetUGCUpdate(FeatureID const & id) const return update; } -void Storage::SetUGCUpdate(FeatureID const & id, UGCUpdate const & ugc) +Storage::SettingResult Storage::SetUGCUpdate(FeatureID const & id, UGCUpdate const & ugc) { + if (!ugc.IsValid()) + return Storage::SettingResult::InvalidUGC; + auto const feature = GetFeature(id); auto const mercator = feature::GetCenter(*feature); feature::TypesHolder th(*feature); @@ -159,8 +162,10 @@ void Storage::SetUGCUpdate(FeatureID const & id, UGCUpdate const & ugc) catch (FileWriter::Exception const & exception) { LOG(LERROR, ("Exception while writing file:", ugcFilePath, "reason:", exception.Msg())); - return; + return Storage::SettingResult::WritingError; } + + return Storage::SettingResult::Success; } void Storage::Load() diff --git a/ugc/storage.hpp b/ugc/storage.hpp index 958df79cd3..3c9f484120 100644 --- a/ugc/storage.hpp +++ b/ugc/storage.hpp @@ -42,7 +42,15 @@ public: explicit Storage(Index const & index) : m_index(index) {} UGCUpdate GetUGCUpdate(FeatureID const & id) const; - void SetUGCUpdate(FeatureID const & id, UGCUpdate const & ugc); + + enum class SettingResult + { + Success, + InvalidUGC, + WritingError + }; + + SettingResult SetUGCUpdate(FeatureID const & id, UGCUpdate const & ugc); void SaveIndex() const; std::string GetUGCToSend() const; void MarkAllAsSynchronized(); @@ -61,4 +69,14 @@ private: std::vector m_UGCIndexes; size_t m_numberOfDeleted = 0; }; + +inline std::string DebugPrint(Storage::SettingResult const & result) +{ + switch (result) + { + case Storage::SettingResult::Success: return "Success"; + case Storage::SettingResult::InvalidUGC: return "Invalid UGC"; + case Storage::SettingResult::WritingError: return "Writing Error"; + } +} } // namespace ugc diff --git a/ugc/types.hpp b/ugc/types.hpp index e588ce7a2c..d2457e95b4 100644 --- a/ugc/types.hpp +++ b/ugc/types.hpp @@ -259,6 +259,25 @@ struct UGCUpdate return !((!m_ratings.empty() || !m_text.m_text.empty()) && m_time != Time()); } + bool IsValid() const + { + bool const timeIsValid = m_time != Time(); + if (!m_text.m_text.empty()) + return timeIsValid; + + bool ratingIsValid = false; + for (auto const & r : m_ratings) + { + if (static_cast(r.m_value) > 0) + { + ratingIsValid = true; + break; + } + } + + return ratingIsValid && timeIsValid; + } + Ratings m_ratings; Text m_text; Time m_time{}; diff --git a/ugc/ugc_tests/storage_tests.cpp b/ugc/ugc_tests/storage_tests.cpp index 19f9fda6a7..76b5c5d5a7 100644 --- a/ugc/ugc_tests/storage_tests.cpp +++ b/ugc/ugc_tests/storage_tests.cpp @@ -182,7 +182,7 @@ UNIT_CLASS_TEST(StorageTest, Smoke) auto const original = MakeTestUGCUpdate(Time(chrono::hours(24 * 300))); Storage storage(builder.GetIndex()); storage.Load(); - storage.SetUGCUpdate(id, original); + TEST_EQUAL(storage.SetUGCUpdate(id, original), Storage::SettingResult::Success, ()); auto const actual = storage.GetUGCUpdate(id); TEST_EQUAL(original, actual, ()); TEST(!storage.GetUGCToSend().empty(), ()); @@ -204,11 +204,11 @@ UNIT_CLASS_TEST(StorageTest, DuplicatesAndDefragmentationSmoke) auto const last = MakeTestUGCUpdate(Time(chrono::hours(24 * 100))); Storage storage(builder.GetIndex()); storage.Load(); - storage.SetUGCUpdate(cafeId, first); - storage.SetUGCUpdate(cafeId, second); - storage.SetUGCUpdate(cafeId, third); - storage.SetUGCUpdate(cafeId, last); - storage.SetUGCUpdate(railwayId, first); + TEST_EQUAL(storage.SetUGCUpdate(cafeId, first), Storage::SettingResult::Success, ()); + TEST_EQUAL(storage.SetUGCUpdate(cafeId, second), Storage::SettingResult::Success, ()); + TEST_EQUAL(storage.SetUGCUpdate(cafeId, third), Storage::SettingResult::Success, ()); + TEST_EQUAL(storage.SetUGCUpdate(cafeId, last), Storage::SettingResult::Success, ()); + TEST_EQUAL(storage.SetUGCUpdate(railwayId, first), Storage::SettingResult::Success, ()); TEST_EQUAL(last, storage.GetUGCUpdate(cafeId), ()); TEST_EQUAL(storage.GetIndexesForTesting().size(), 5, ()); TEST_EQUAL(storage.GetNumberOfDeletedForTesting(), 3, ()); @@ -230,8 +230,8 @@ UNIT_CLASS_TEST(StorageTest, DifferentTypes) auto const railwayUGC = MakeTestUGCUpdate(Time(chrono::hours(24 * 300))); Storage storage(builder.GetIndex()); storage.Load(); - storage.SetUGCUpdate(cafeId, cafeUGC); - storage.SetUGCUpdate(railwayId, railwayUGC); + TEST_EQUAL(storage.SetUGCUpdate(cafeId, cafeUGC), Storage::SettingResult::Success, ()); + TEST_EQUAL(storage.SetUGCUpdate(railwayId, railwayUGC), Storage::SettingResult::Success, ()); TEST_EQUAL(railwayUGC, storage.GetUGCUpdate(railwayId), ()); TEST_EQUAL(cafeUGC, storage.GetUGCUpdate(cafeId), ()); } @@ -250,8 +250,8 @@ UNIT_CLASS_TEST(StorageTest, LoadIndex) { Storage storage(builder.GetIndex()); storage.Load(); - storage.SetUGCUpdate(cafeId, cafeUGC); - storage.SetUGCUpdate(railwayId, railwayUGC); + TEST_EQUAL(storage.SetUGCUpdate(cafeId, cafeUGC), Storage::SettingResult::Success, ()); + TEST_EQUAL(storage.SetUGCUpdate(railwayId, railwayUGC), Storage::SettingResult::Success, ()); storage.SaveIndex(); } @@ -265,7 +265,7 @@ UNIT_CLASS_TEST(StorageTest, LoadIndex) TEST(!i.m_deleted, ()); } - storage.SetUGCUpdate(cafeId, cafeUGC); + TEST_EQUAL(storage.SetUGCUpdate(cafeId, cafeUGC), Storage::SettingResult::Success, ()); TEST_EQUAL(indexArray.size(), 3, ()); TEST(DeleteIndexFile(), ()); } @@ -280,8 +280,8 @@ UNIT_CLASS_TEST(StorageTest, ContentTest) auto const newUGC = MakeTestUGCUpdate(Time(chrono::hours(24 * 300))); Storage storage(builder.GetIndex()); storage.Load(); - storage.SetUGCUpdate(cafeId, oldUGC); - storage.SetUGCUpdate(cafeId, newUGC); + TEST_EQUAL(storage.SetUGCUpdate(cafeId, oldUGC), Storage::SettingResult::Success, ()); + TEST_EQUAL(storage.SetUGCUpdate(cafeId, newUGC), Storage::SettingResult::Success, ()); TEST_EQUAL(storage.GetIndexesForTesting().size(), 2, ()); auto const toSendActual = storage.GetUGCToSend(); @@ -322,3 +322,28 @@ UNIT_CLASS_TEST(StorageTest, ContentTest) TEST(lastIndex.m_synchronized, ()); TEST(DeleteIndexFile(), ()); } + +UNIT_CLASS_TEST(StorageTest, InvalidUGC) +{ + auto & builder = MwmBuilder::Builder(); + m2::PointD const cafePoint(1.0, 1.0); + builder.Build({TestCafe(cafePoint)}); + auto const cafeId = builder.FeatureIdForCafeAtPoint(cafePoint); + Storage storage(builder.GetIndex()); + storage.Load(); + + UGCUpdate first; + TEST_EQUAL(storage.SetUGCUpdate(cafeId, first), Storage::SettingResult::InvalidUGC, ()); + first.m_time = chrono::system_clock::now(); + TEST_EQUAL(storage.SetUGCUpdate(cafeId, first), Storage::SettingResult::InvalidUGC, ()); + TEST(storage.GetIndexesForTesting().empty(), ()); + TEST(storage.GetUGCToSend().empty(), ()); + first.m_text = Text("a", 1); + TEST_EQUAL(storage.SetUGCUpdate(cafeId, first), Storage::SettingResult::Success, ()); + UGCUpdate second; + second.m_time = chrono::system_clock::now(); + second.m_ratings.emplace_back("a", 0); + TEST_EQUAL(storage.SetUGCUpdate(cafeId, second), Storage::SettingResult::InvalidUGC, ()); + second.m_ratings.emplace_back("b", 1); + TEST_EQUAL(storage.SetUGCUpdate(cafeId, second), Storage::SettingResult::Success, ()); +}