diff --git a/indexer/indexer_tests_support/test_with_custom_mwms.hpp b/indexer/indexer_tests_support/test_with_custom_mwms.hpp index a1066cdea3..9fbc81614b 100644 --- a/indexer/indexer_tests_support/test_with_custom_mwms.hpp +++ b/indexer/indexer_tests_support/test_with_custom_mwms.hpp @@ -69,9 +69,6 @@ public: return BuildMwm(name, feature::DataHeader::country, std::forward(fn)); } - Index & GetIndex() { return m_index; } - Index const & GetIndex() const { return m_index; } - protected: static void Cleanup(platform::LocalCountryFile const & file); diff --git a/map/booking_filter.cpp b/map/booking_filter.cpp index 96d1b541c9..70b6f2d12c 100644 --- a/map/booking_filter.cpp +++ b/map/booking_filter.cpp @@ -18,7 +18,7 @@ namespace { struct HotelToResult { - HotelToResult(search::Result const & result) : m_result(result) {} + explicit HotelToResult(search::Result const & result) : m_result(result) {} search::Result m_result; std::string m_hotelId; @@ -32,16 +32,16 @@ void FillAvailability(HotelToResult & hotelToResult, std::vector co { using availability::Cache; - if (hotelToResult.m_cacheStatus == Cache::HotelStatus::Unavailable) + switch (hotelToResult.m_cacheStatus) + { + case Cache::HotelStatus::Unavailable: return; - - if (hotelToResult.m_cacheStatus == Cache::HotelStatus::Available) + case Cache::HotelStatus::Available: { results.AddResult(std::move(hotelToResult.m_result)); return; } - - if (hotelToResult.m_cacheStatus == Cache::HotelStatus::NotReady) + case Cache::HotelStatus::NotReady: { auto hotelStatus = cache.Get(hotelToResult.m_hotelId); CHECK_NOT_EQUAL(hotelStatus, Cache::HotelStatus::Absent, ()); @@ -52,8 +52,7 @@ void FillAvailability(HotelToResult & hotelToResult, std::vector co return; } - - if (hotelToResult.m_cacheStatus == Cache::HotelStatus::Absent) + case Cache::HotelStatus::Absent: { if (std::binary_search(hotelIds.cbegin(), hotelIds.cend(), hotelToResult.m_hotelId)) { @@ -65,6 +64,7 @@ void FillAvailability(HotelToResult & hotelToResult, std::vector co cache.Insert(hotelToResult.m_hotelId, Cache::HotelStatus::Unavailable); } } + } } void PrepareData(Index const & index, search::Results const & results, @@ -98,10 +98,10 @@ void PrepareData(Index const & index, search::Results const & results, } auto it = std::find_if(hotelToResults.begin(), hotelToResults.end(), - [&featureId](HotelToResult const & item) - { - return item.m_result.GetFeatureID() == featureId; - }); + [&featureId](HotelToResult const & item) + { + return item.m_result.GetFeatureID() == featureId; + }); ASSERT(it != hotelToResults.cend(), ()); FeatureType ft; @@ -112,7 +112,7 @@ void PrepareData(Index const & index, search::Results const & results, continue; } - std::string hotelId = ft.GetMetadata().Get(feature::Metadata::FMD_SPONSORED_ID); + auto const hotelId = ft.GetMetadata().Get(feature::Metadata::FMD_SPONSORED_ID); auto const status = cache.Get(hotelId); it->m_hotelId = hotelId; @@ -147,7 +147,7 @@ void Filter::Availability(search::Results const & results, if (m_currentParams != params.m_params) { m_currentParams = std::move(params.m_params); - m_availabilityCache.Drop(); + m_availabilityCache.Clear(); ++m_cacheDropCounter; } diff --git a/map/booking_filter.hpp b/map/booking_filter.hpp index 4e95578a09..5a0041cf80 100644 --- a/map/booking_filter.hpp +++ b/map/booking_filter.hpp @@ -3,7 +3,7 @@ #include "map/booking_filter_availability_params.hpp" #include "map/booking_filter_cache.hpp" -#include "base/worker_thread.hpp" +#include class Index; @@ -33,7 +33,7 @@ private: std::mutex m_mutex; availability::Cache m_availabilityCache; - // |m_cacheDropCounter| and |m_currentParams| are used to identify request actuality. + // |m_cacheDropCounter| and |m_currentParams| are used to identify request freshness. uint32_t m_cacheDropCounter = {}; AvailabilityParams m_currentParams; }; diff --git a/map/booking_filter_cache.cpp b/map/booking_filter_cache.cpp index ba9cb1db6b..90fc248f92 100644 --- a/map/booking_filter_cache.cpp +++ b/map/booking_filter_cache.cpp @@ -15,36 +15,29 @@ Cache::Cache(size_t maxCount, size_t expiryPeriodSeconds) Cache::HotelStatus Cache::Get(std::string const & hotelId) { - auto const it = m_hotelToResult.find(hotelId); + HotelStatus result = Get(m_notReadyHotels, hotelId); - if (it == m_hotelToResult.cend()) - return HotelStatus::Absent; + if (result == HotelStatus::Absent) + result = Get(m_hotelToResult, hotelId); - if (m_expiryPeriodSeconds != 0) - { - auto const timeDiff = Clock::now() - it->second.m_timestamp; - if (timeDiff > seconds(m_expiryPeriodSeconds)) - { - m_hotelToResult.erase(it); - return HotelStatus::Absent; - } - } - - return it->second.m_status; + return result; } void Cache::Reserve(std::string const & hotelId) { - Item item; - m_hotelToResult.emplace(hotelId, std::move(item)); + m_notReadyHotels.emplace(hotelId, Item()); } void Cache::Insert(std::string const & hotelId, HotelStatus const s) { + ASSERT_NOT_EQUAL(s, HotelStatus::NotReady, + ("Please, use Cache::Reserve method for HotelStatus::NotReady")); + RemoveExtra(); Item item(s); m_hotelToResult[hotelId] = std::move(item); + m_notReadyHotels.erase(hotelId); } void Cache::RemoveOutdated() @@ -52,19 +45,14 @@ void Cache::RemoveOutdated() if (m_expiryPeriodSeconds == 0) return; - for (auto it = m_hotelToResult.begin(); it != m_hotelToResult.end();) - { - auto const timeDiff = Clock::now() - it->second.m_timestamp; - if (timeDiff > seconds(m_expiryPeriodSeconds)) - it = m_hotelToResult.erase(it); - else - ++it; - } + RemoveOutdated(m_hotelToResult); + RemoveOutdated(m_notReadyHotels); } -void Cache::Drop() +void Cache::Clear() { m_hotelToResult.clear(); + m_notReadyHotels.clear(); } void Cache::RemoveExtra() @@ -72,10 +60,39 @@ void Cache::RemoveExtra() if (m_maxCount == 0 || m_hotelToResult.size() < m_maxCount) return; - for (auto it = m_hotelToResult.begin(); it != m_hotelToResult.end();) + m_hotelToResult.clear(); +} + +bool Cache::IsExpired(Clock::time_point const & timestamp) const +{ + return (Clock::now() - timestamp) > seconds(m_expiryPeriodSeconds); +} + +Cache::HotelStatus Cache::Get(HotelsMap & src, std::string const & hotelId) +{ + auto const it = src.find(hotelId); + + if (it == src.cend()) + return HotelStatus::Absent; + + if (m_expiryPeriodSeconds != 0) { - if (it->second.m_status != HotelStatus::NotReady) - it = m_hotelToResult.erase(it); + if (IsExpired(it->second.m_timestamp)) + { + src.erase(it); + return HotelStatus::Absent; + } + } + + return it->second.m_status; +} + +void Cache::RemoveOutdated(HotelsMap & src) +{ + for (auto it = src.begin(); it != src.end();) + { + if (IsExpired(it->second.m_timestamp)) + it = src.erase(it); else ++it; } diff --git a/map/booking_filter_cache.hpp b/map/booking_filter_cache.hpp index 24057b21df..c641888185 100644 --- a/map/booking_filter_cache.hpp +++ b/map/booking_filter_cache.hpp @@ -3,7 +3,6 @@ #include "base/macros.hpp" #include -#include namespace booking { @@ -11,7 +10,6 @@ namespace filter { namespace availability { -// *NOTE* This class IS thread-safe. class Cache { public: @@ -47,11 +45,19 @@ public: void Insert(std::string const & hotelId, HotelStatus const s); void RemoveOutdated(); - void Drop(); -private: - void RemoveExtra(); + void Clear(); - std::map m_hotelToResult; +private: + using HotelsMap = std::map; + // In case when size >= |m_maxCount| removes items except of those who have the status + // HotelStatus::NotReady. + void RemoveExtra(); + bool IsExpired(Clock::time_point const & timestamp) const; + HotelStatus Get(HotelsMap & src, std::string const & hotelId); + void RemoveOutdated(HotelsMap & src); + + HotelsMap m_hotelToResult; + HotelsMap m_notReadyHotels; // Count is unlimited when |m_maxCount| is equal to zero. size_t const m_maxCount = 1000; // Do not use aging when |m_expiryPeriodSeconds| is equal to zero. diff --git a/map/map_tests/booking_filter_test.cpp b/map/map_tests/booking_filter_test.cpp index 488eacf1ce..c64240abdb 100644 --- a/map/map_tests/booking_filter_test.cpp +++ b/map/map_tests/booking_filter_test.cpp @@ -13,8 +13,6 @@ #include "storage/country_info_getter.hpp" -#include "base/scope_guard.hpp" - #include using namespace booking::filter; @@ -25,6 +23,16 @@ namespace class TestMwmEnvironment : public indexer::tests_support::TestWithCustomMwms { protected: + TestMwmEnvironment() + { + booking::SetBookingUrlForTesting("http://localhost:34568/booking/min_price"); + } + + ~TestMwmEnvironment() + { + booking::SetBookingUrlForTesting(""); + } + void OnMwmBuilt(MwmInfo const & info) override { m_infoGetter.AddCountry(storage::CountryDef(info.GetCountryName(), info.m_limitRect)); @@ -34,18 +42,14 @@ private: storage::CountryInfoGetterForTesting m_infoGetter; }; -UNIT_TEST(BookingFilter_AvailabilitySmoke) +UNIT_CLASS_TEST(TestMwmEnvironment, BookingFilter_AvailabilitySmoke) { - TestMwmEnvironment env; booking::Api api; - Filter filter(env.GetIndex(), api); - - booking::SetBookingUrlForTesting("http://localhost:34568/booking/min_price"); - MY_SCOPE_GUARD(cleanup, []() { booking::SetBookingUrlForTesting(""); }); + Filter filter(m_index, api); std::vector const kHotelIds = {"10623", "10624", "10625"}; - env.BuildCountry("TestMwm", [&kHotelIds](TestMwmBuilder & builder) + BuildCountry("TestMwm", [&kHotelIds](TestMwmBuilder & builder) { TestPOI hotel1(m2::PointD(1.0, 1.0), "hotel 1", "en"); hotel1.GetMetadata().Set(feature::Metadata::FMD_SPONSORED_ID, kHotelIds[0]); @@ -64,7 +68,7 @@ UNIT_TEST(BookingFilter_AvailabilitySmoke) search::Results results; results.AddResult({"suggest for testing", "suggest for testing"}); search::Results expectedResults; - env.GetIndex().ForEachInRect( + m_index.ForEachInRect( [&results, &expectedResults](FeatureType & ft) { search::Result::Metadata metadata; metadata.m_isSponsoredHotel = true; diff --git a/map/search_api.cpp b/map/search_api.cpp index 7d6831c39a..73d9ed4f1e 100644 --- a/map/search_api.cpp +++ b/map/search_api.cpp @@ -79,7 +79,8 @@ bool SearchAPI::SearchEverywhere(EverywhereSearchParams const & params) // TODO: delete me after Cian project is finished. if (IsCianMode(params.m_query)) m_sponsoredMode = SponsoredMode::Cian; - else if (!params.m_bookingFilterParams.IsEmpty()) + + if (!params.m_bookingFilterParams.IsEmpty()) m_sponsoredMode = SponsoredMode::Booking; SearchParams p; @@ -105,7 +106,8 @@ bool SearchAPI::SearchEverywhere(EverywhereSearchParams const & params) if (results.IsEndMarker() && results.IsEndedNormal() && !params.m_bookingFilterParams.IsEmpty()) { - m_delegate.FilterSearchResultsOnBooking(params.m_bookingFilterParams, results, false); + m_delegate.FilterSearchResultsOnBooking(params.m_bookingFilterParams, results, + false /* inViewport */); } }); @@ -117,7 +119,8 @@ bool SearchAPI::SearchInViewport(ViewportSearchParams const & params) // TODO: delete me after Cian project is finished. if (IsCianMode(params.m_query)) m_sponsoredMode = SponsoredMode::Cian; - else if (!params.m_bookingFilterParams.IsEmpty()) + + if (!params.m_bookingFilterParams.IsEmpty()) m_sponsoredMode = SponsoredMode::Booking; SearchParams p; @@ -146,7 +149,8 @@ bool SearchAPI::SearchInViewport(ViewportSearchParams const & params) if (results.IsEndMarker() && results.IsEndedNormal() && !params.m_bookingFilterParams.IsEmpty()) { - m_delegate.FilterSearchResultsOnBooking(params.m_bookingFilterParams, results, true); + m_delegate.FilterSearchResultsOnBooking(params.m_bookingFilterParams, results, + true /* inViewport */); } }); diff --git a/map/search_api.hpp b/map/search_api.hpp index 881607a89d..f7b204c3e7 100644 --- a/map/search_api.hpp +++ b/map/search_api.hpp @@ -46,7 +46,7 @@ class SearchAPI : public search::DownloaderSearchCallback::Delegate, public search::ViewportSearchCallback::Delegate { public: - enum SponsoredMode + enum class SponsoredMode { None, // TODO (@y, @m): delete me after Cian project is finished. diff --git a/partners_api/booking_availability_params.cpp b/partners_api/booking_availability_params.cpp index f3fda4e4c7..c9f67e5b14 100644 --- a/partners_api/booking_availability_params.cpp +++ b/partners_api/booking_availability_params.cpp @@ -45,4 +45,8 @@ bool AvailabilityParams::operator!=(AvailabilityParams const & rhs) return m_checkin != rhs.m_checkin || m_checkout != rhs.m_checkout || m_rooms != rhs.m_rooms || m_minReviewScore != m_minReviewScore || m_stars != rhs.m_stars; } +bool AvailabilityParams::operator==(AvailabilityParams const & rhs) +{ + return !this->operator!=(rhs); +} } // namespace booking diff --git a/partners_api/booking_availability_params.hpp b/partners_api/booking_availability_params.hpp index 5dfd9ae105..6be6a3d7d3 100644 --- a/partners_api/booking_availability_params.hpp +++ b/partners_api/booking_availability_params.hpp @@ -20,6 +20,7 @@ struct AvailabilityParams base::url::Params Get() const; bool IsEmpty() const; bool operator!=(AvailabilityParams const & rhs); + bool operator==(AvailabilityParams const & rhs); /// Limit the result list to the specified hotels where they have availability for the /// specified guests and dates.