From 943444a7c34653d0e3db009711d5b38a30c22b48 Mon Sep 17 00:00:00 2001 From: Yuri Gorshenin Date: Fri, 6 Nov 2015 13:32:21 +0300 Subject: [PATCH] [platform, storage] Implemented cleanup of absent countries indexes. --- map/framework.cpp | 6 ++- platform/local_country_file_utils.cpp | 43 ++++++++++++++++--- platform/local_country_file_utils.hpp | 8 +++- .../local_country_file_tests.cpp | 35 +++++++++++---- .../platform_tests_support/scoped_dir.cpp | 5 +++ .../platform_tests_support/scoped_dir.hpp | 2 + storage/storage.cpp | 14 +++--- storage/storage_tests/storage_tests.cpp | 27 ++++++++++++ 8 files changed, 117 insertions(+), 23 deletions(-) diff --git a/map/framework.cpp b/map/framework.cpp index 530cbcc67f..e003c9a7ec 100644 --- a/map/framework.cpp +++ b/map/framework.cpp @@ -477,7 +477,11 @@ void Framework::RegisterAllMaps() ("Registering maps while map downloading leads to removing downloading maps from " "ActiveMapsListener::m_items.")); - platform::CleanupMapsDirectory(m_storage.GetCurrentDataVersion()); + auto const isCountryName = [this](string const & filename) + { + return m_storage.FindIndexByFile(filename).IsValid(); + }; + platform::CleanupMapsDirectory(m_storage.GetCurrentDataVersion(), isCountryName); m_storage.RegisterAllLocalMaps(); int minFormat = numeric_limits::max(); diff --git a/platform/local_country_file_utils.cpp b/platform/local_country_file_utils.cpp index 966d292a69..56525a9e0e 100644 --- a/platform/local_country_file_utils.cpp +++ b/platform/local_country_file_utils.cpp @@ -1,6 +1,8 @@ -#include "local_country_file_utils.hpp" -#include "mwm_version.hpp" -#include "platform.hpp" +#include "platform/local_country_file_utils.hpp" + +#include "platform/country_file.hpp" +#include "platform/mwm_version.hpp" +#include "platform/platform.hpp" #include "coding/file_name_utils.hpp" #include "coding/internal/file_data.hpp" @@ -15,6 +17,7 @@ #include "std/cctype.hpp" #include "std/sstream.hpp" #include "std/unique_ptr.hpp" +#include "std/unordered_set.hpp" #include "defines.hpp" @@ -82,6 +85,31 @@ void DeleteDownloaderFilesForAllCountries(string const & directory) for (auto const & file : files) my::DeleteFileX(my::JoinFoldersToPath(directory, file)); } + +void DeleteIndexesForAbsentCountries(string const & directory, + int64_t version, + function const & isCountryName) +{ + vector files; + FindAllLocalMapsInDirectory(directory, version, files); + + unordered_set names; + for (auto const & file : files) + names.insert(file.GetCountryName()); + + Platform::FilesList subdirs; + Platform::GetFilesByType(directory, Platform::FILE_TYPE_DIRECTORY, subdirs); + for (auto const & subdir : subdirs) + { + if (subdir == "." || subdir == "..") + continue; + if (!isCountryName(subdir) || names.count(subdir) != 0) + continue; + + LocalCountryFile absentCountry(directory, CountryFile(subdir), version); + CountryIndexes::DeleteFromDisk(absentCountry); + } +} } // namespace void DeleteDownloaderFilesForCountry(CountryFile const & countryFile, int64_t version) @@ -96,7 +124,8 @@ void DeleteDownloaderFilesForCountry(CountryFile const & countryFile, int64_t ve } } -void CleanupMapsDirectory(int64_t latestVersion) +void CleanupMapsDirectory(int64_t latestVersion, + function const & isCountryName) { Platform & platform = GetPlatform(); @@ -137,6 +166,9 @@ void CleanupMapsDirectory(int64_t latestVersion) if (version != latestVersion) DeleteDownloaderFilesForAllCountries(subdirPath); + // It's OK to remove indexes for absent countries. + DeleteIndexesForAbsentCountries(subdirPath, version, isCountryName); + // Remove subdirectory if it does not contain any files except "." and "..". if (subdir != "." && Platform::IsDirectoryEmpty(subdirPath)) { @@ -146,9 +178,6 @@ void CleanupMapsDirectory(int64_t latestVersion) UNUSED_VALUE(ret); } } - - /// @todo Cleanup temporary index files for already absent mwm files. - /// https://trello.com/c/PKiiOsB4/28-- } void FindAllLocalMapsInDirectory(string const & directory, int64_t version, diff --git a/platform/local_country_file_utils.hpp b/platform/local_country_file_utils.hpp index a283b3e830..e6795ea063 100644 --- a/platform/local_country_file_utils.hpp +++ b/platform/local_country_file_utils.hpp @@ -3,6 +3,7 @@ #include "platform/country_defines.hpp" #include "platform/local_country_file.hpp" +#include "std/function.hpp" #include "std/shared_ptr.hpp" #include "std/utility.hpp" #include "std/vector.hpp" @@ -17,8 +18,11 @@ void DeleteDownloaderFilesForCountry(CountryFile const & countryFile, int64_t ve // Removes partially downloaded maps, empty directories and old // (format v1) maps. Also, removes old (splitted) Japan and Brazil // maps. |version| must be set to the latest data version this app can -// work with. -void CleanupMapsDirectory(int64_t latestVersion); +// work with. |isCountryName| predicate is used to detect and remove +// indexes for absent countries. It must return true for file names +// corresponding to a country. +void CleanupMapsDirectory(int64_t latestVersion, + function const & isCountryName); // Finds all local map files in a directory. Version of these files is // passed as an argument. diff --git a/platform/platform_tests/local_country_file_tests.cpp b/platform/platform_tests/local_country_file_tests.cpp index fa320355b8..b25ab73f20 100644 --- a/platform/platform_tests/local_country_file_tests.cpp +++ b/platform/platform_tests/local_country_file_tests.cpp @@ -137,6 +137,13 @@ UNIT_TEST(LocalCountryFile_CleanupMapFiles) Platform & platform = GetPlatform(); string const mapsDir = platform.WritableDir(); + // Two fake directories for test country files and indexes. + ScopedDir dir3("3"); + ScopedDir dir4("4"); + + ScopedDir absentCountryIndexesDir(dir4, "Absent"); + ScopedDir irelandIndexesDir(dir4, "Ireland"); + CountryFile japanFile("Japan"); CountryFile brazilFile("Brazil"); CountryFile irelandFile("Ireland"); @@ -147,10 +154,8 @@ UNIT_TEST(LocalCountryFile_CleanupMapFiles) LocalCountryFile brazilLocalFile(mapsDir, brazilFile, 0 /* version */); ScopedFile brazilMapFile("Brazil.mwm", "Brazil"); - LocalCountryFile irelandLocalFile(mapsDir, irelandFile, 0 /* version */); - ScopedFile irelandMapFile("Ireland.mwm", "Ireland"); - - ScopedDir emptyDir("3"); + LocalCountryFile irelandLocalFile(dir4.GetFullPath(), irelandFile, 4 /* version */); + ScopedFile irelandMapFile(dir4, irelandFile, MapOptions::Map, "Ireland"); // Check that FindAllLocalMaps() vector localFiles; @@ -159,7 +164,10 @@ UNIT_TEST(LocalCountryFile_CleanupMapFiles) TEST(Contains(localFiles, brazilLocalFile), (brazilLocalFile, localFiles)); TEST(Contains(localFiles, irelandLocalFile), (irelandLocalFile, localFiles)); - CleanupMapsDirectory(0 /* latestVersion */); + CleanupMapsDirectory(0 /* latestVersion */, [](string const filename) + { + return filename == "Ireland" || filename == "Absent"; + }); japanLocalFile.SyncWithDisk(); TEST_EQUAL(MapOptions::Nothing, japanLocalFile.GetFiles(), ()); @@ -177,8 +185,15 @@ UNIT_TEST(LocalCountryFile_CleanupMapFiles) TEST(!irelandMapFile.Exists(), (irelandMapFile)); irelandMapFile.Reset(); - TEST(!emptyDir.Exists(), ("Empty directory", emptyDir, "wasn't removed.")); - emptyDir.Reset(); + TEST(!dir3.Exists(), ("Empty directory", dir3, "wasn't removed.")); + dir3.Reset(); + + TEST(dir4.Exists(), ()); + + TEST(!absentCountryIndexesDir.Exists(), ("Indexes for absent country weren't deleted.")); + absentCountryIndexesDir.Reset(); + + TEST(irelandIndexesDir.Exists(), ()); } UNIT_TEST(LocalCountryFile_CleanupPartiallyDownloadedFiles) @@ -200,7 +215,11 @@ UNIT_TEST(LocalCountryFile_CleanupPartiallyDownloadedFiles) {my::JoinFoldersToPath(latestDir.GetRelativePath(), "Russia_Southern.mwm.downloading"), "Southern Russia map"}}; - CleanupMapsDirectory(101010 /* latestVersion */); + auto const isCountryName = [&](string const & /* filename */) + { + return false; + }; + CleanupMapsDirectory(101010 /* latestVersion */, isCountryName); for (ScopedFile & file : toBeDeleted) { diff --git a/platform/platform_tests_support/scoped_dir.cpp b/platform/platform_tests_support/scoped_dir.cpp index 1f27f44e9d..611909b936 100644 --- a/platform/platform_tests_support/scoped_dir.cpp +++ b/platform/platform_tests_support/scoped_dir.cpp @@ -34,6 +34,11 @@ ScopedDir::ScopedDir(string const & relativePath) } } +ScopedDir::ScopedDir(ScopedDir const & parent, string const & name) + : ScopedDir(my::JoinFoldersToPath(parent.GetRelativePath(), name)) +{ +} + ScopedDir::~ScopedDir() { if (m_reset) diff --git a/platform/platform_tests_support/scoped_dir.hpp b/platform/platform_tests_support/scoped_dir.hpp index 8451c0d76a..4095a4777d 100644 --- a/platform/platform_tests_support/scoped_dir.hpp +++ b/platform/platform_tests_support/scoped_dir.hpp @@ -18,6 +18,8 @@ public: /// @param path Path for a testing directory, should be relative to writable-dir. ScopedDir(string const & relativePath); + ScopedDir(ScopedDir const & parent, string const & name); + ~ScopedDir(); inline void Reset() { m_reset = true; } diff --git a/storage/storage.cpp b/storage/storage.cpp index 6c2c331b24..5cadadce8a 100644 --- a/storage/storage.cpp +++ b/storage/storage.cpp @@ -66,6 +66,12 @@ void DeleteCountryIndexes(LocalCountryFile const & localFile) platform::CountryIndexes::DeleteFromDisk(localFile); } +void DeleteFromDiskWithIndexes(LocalCountryFile const & localFile, MapOptions options) +{ + DeleteCountryIndexes(localFile); + localFile.DeleteFromDisk(options); +} + class EqualFileName { string const & m_name; @@ -130,7 +136,7 @@ void Storage::RegisterAllLocalMaps() LocalCountryFile & localFile = *j; LOG(LINFO, ("Removing obsolete", localFile)); localFile.SyncWithDisk(); - localFile.DeleteFromDisk(MapOptions::MapWithCarRouting); + DeleteFromDiskWithIndexes(localFile, MapOptions::MapWithCarRouting); ++j; } @@ -340,8 +346,7 @@ void Storage::DeleteCountry(TIndex const & index, MapOptions opt) void Storage::DeleteCustomCountryVersion(LocalCountryFile const & localFile) { CountryFile const countryFile = localFile.GetCountryFile(); - localFile.DeleteFromDisk(MapOptions::MapWithCarRouting); - CountryIndexes::DeleteFromDisk(localFile); + DeleteFromDiskWithIndexes(localFile, MapOptions::MapWithCarRouting); { auto it = m_localFilesForFakeCountries.find(countryFile); @@ -844,9 +849,8 @@ void Storage::DeleteCountryFiles(TIndex const & index, MapOptions opt) auto & localFiles = it->second; for (auto & localFile : localFiles) { - localFile->DeleteFromDisk(opt); + DeleteFromDiskWithIndexes(*localFile, opt); localFile->SyncWithDisk(); - DeleteCountryIndexes(*localFile); if (localFile->GetFiles() == MapOptions::Nothing) localFile.reset(); } diff --git a/storage/storage_tests/storage_tests.cpp b/storage/storage_tests/storage_tests.cpp index baefeac4d2..304b5e3daa 100644 --- a/storage/storage_tests/storage_tests.cpp +++ b/storage/storage_tests/storage_tests.cpp @@ -12,6 +12,7 @@ #include "platform/local_country_file.hpp" #include "platform/local_country_file_utils.hpp" #include "platform/platform.hpp" +#include "platform/platform_tests_support/scoped_dir.hpp" #include "platform/platform_tests_support/scoped_file.hpp" #include "coding/file_name_utils.hpp" @@ -732,4 +733,30 @@ UNIT_TEST(StorageTest_EmptyRoutingFile) checker->StartDownload(); runner.Run(); } + +UNIT_TEST(StorageTest_ObsoleteMapsRemoval) +{ + CountryFile country("Azerbaijan"); + + tests_support::ScopedDir dir1("1"); + tests_support::ScopedFile map1(dir1, country, MapOptions::Map, "map1"); + LocalCountryFile file1(dir1.GetFullPath(), country, 1 /* version */); + CountryIndexes::PreparePlaceOnDisk(file1); + + tests_support::ScopedDir dir2("2"); + tests_support::ScopedFile map2(dir2, country, MapOptions::Map, "map2"); + LocalCountryFile file2(dir2.GetFullPath(), country, 2 /* version */); + CountryIndexes::PreparePlaceOnDisk(file2); + + TEST(map1.Exists(), ()); + TEST(map2.Exists(), ()); + + Storage storage; + storage.RegisterAllLocalMaps(); + + TEST(!map1.Exists(), ()); + map1.Reset(); + + TEST(map2.Exists(), ()); +} } // namespace storage