From c203e07ee917f459122e267ec6e8fecb2a3f9d3d Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Mon, 18 Sep 2023 17:16:12 +0200 Subject: [PATCH] Make the library creatable only within a shared_ptr. --- include/library.h | 9 ++- test/library.cpp | 106 ++++++++++++++++++------------------ test/manager.cpp | 26 ++++----- test/name_mapper.cpp | 28 +++++----- test/server_testing_tools.h | 12 ++-- 5 files changed, 96 insertions(+), 85 deletions(-) diff --git a/include/library.h b/include/library.h index 602cfb9fd..b9ca15cea 100644 --- a/include/library.h +++ b/include/library.h @@ -176,7 +176,7 @@ class ZimSearcher : public zim::Searcher /** * A Library store several books. */ -class Library +class Library: public std::enable_shared_from_this { // all data fields must be added in LibraryBase mutable std::mutex m_mutex; @@ -187,8 +187,13 @@ class Library typedef std::map AttributeCounts; typedef std::set BookIdSet; - public: + private: Library(); + + public: + [[nodiscard]] static std::shared_ptr create() { + return std::shared_ptr(new Library()); + } ~Library(); /** diff --git a/test/library.cpp b/test/library.cpp index 4fb4b7dd1..aa75eb80a 100644 --- a/test/library.cpp +++ b/test/library.cpp @@ -238,14 +238,14 @@ typedef std::vector Langs; TEST(LibraryOpdsImportTest, allInOne) { - kiwix::Library lib; - kiwix::Manager manager(&lib); + auto lib = kiwix::Library::create(); + kiwix::Manager manager(lib.get()); manager.readOpds(sampleOpdsStream, "library-opds-import.unittests.dev"); - EXPECT_EQ(10U, lib.getBookCount(true, true)); + EXPECT_EQ(10U, lib->getBookCount(true, true)); { - const kiwix::Book& book1 = lib.getBookById("0c45160e-f917-760a-9159-dfe3c53cdcdd"); + const kiwix::Book& book1 = lib->getBookById("0c45160e-f917-760a-9159-dfe3c53cdcdd"); EXPECT_EQ(book1.getTitle(), "Encyclopédie de la Tunisie"); EXPECT_EQ(book1.getName(), "wikipedia_fr_tunisie_novid_2018-10"); @@ -271,7 +271,7 @@ TEST(LibraryOpdsImportTest, allInOne) } { - const kiwix::Book& book2 = lib.getBookById("0189d9be-2fd0-b4b6-7300-20fab0b5cdc8"); + const kiwix::Book& book2 = lib->getBookById("0189d9be-2fd0-b4b6-7300-20fab0b5cdc8"); EXPECT_EQ(book2.getTitle(), "TED talks - Business"); EXPECT_EQ(book2.getName(), ""); EXPECT_EQ(book2.getFlavour(), ""); @@ -301,8 +301,10 @@ class LibraryTest : public ::testing::Test { typedef kiwix::Library::BookIdCollection BookIdCollection; typedef std::vector TitleCollection; + LibraryTest(): lib(kiwix::Library::create()) {} + void SetUp() override { - kiwix::Manager manager(&lib); + kiwix::Manager manager(lib.get()); manager.readOpds(sampleOpdsStream, "foo.urlHost"); manager.readXml(sampleLibraryXML, false, "./test/library.xml", true); } @@ -316,25 +318,25 @@ class LibraryTest : public ::testing::Test { TitleCollection ids2Titles(const BookIdCollection& ids) { TitleCollection titles; for ( const auto& bookId : ids ) { - titles.push_back(lib.getBookById(bookId).getTitle()); + titles.push_back(lib->getBookById(bookId).getTitle()); } std::sort(titles.begin(), titles.end()); return titles; } - kiwix::Library lib; + std::shared_ptr lib; }; TEST_F(LibraryTest, getBookMarksTest) { - auto bookId1 = lib.getBooksIds()[0]; - auto bookId2 = lib.getBooksIds()[1]; + auto bookId1 = lib->getBooksIds()[0]; + auto bookId2 = lib->getBooksIds()[1]; - lib.addBookmark(createBookmark(bookId1)); - lib.addBookmark(createBookmark("invalid-bookmark-id")); - lib.addBookmark(createBookmark(bookId2)); - auto onlyValidBookmarks = lib.getBookmarks(); - auto allBookmarks = lib.getBookmarks(false); + lib->addBookmark(createBookmark(bookId1)); + lib->addBookmark(createBookmark("invalid-bookmark-id")); + lib->addBookmark(createBookmark(bookId2)); + auto onlyValidBookmarks = lib->getBookmarks(); + auto allBookmarks = lib->getBookmarks(false); EXPECT_EQ(onlyValidBookmarks[0].getBookId(), bookId1); EXPECT_EQ(onlyValidBookmarks[1].getBookId(), bookId2); @@ -346,11 +348,11 @@ TEST_F(LibraryTest, getBookMarksTest) TEST_F(LibraryTest, sanityCheck) { - EXPECT_EQ(lib.getBookCount(true, true), 12U); - EXPECT_EQ(lib.getBooksLanguages(), + EXPECT_EQ(lib->getBookCount(true, true), 12U); + EXPECT_EQ(lib->getBooksLanguages(), std::vector({"deu", "eng", "fra", "ita", "spa"}) ); - EXPECT_EQ(lib.getBooksCreators(), std::vector({ + EXPECT_EQ(lib->getBooksCreators(), std::vector({ "Islam Stack Exchange", "Movies & TV Stack Exchange", "Mythology & Folklore Stack Exchange", @@ -361,7 +363,7 @@ TEST_F(LibraryTest, sanityCheck) "Wikipedia", "Wikiquote" })); - EXPECT_EQ(lib.getBooksPublishers(), std::vector({ + EXPECT_EQ(lib->getBooksPublishers(), std::vector({ "", "Kiwix", "Kiwix & Some Enthusiasts", @@ -371,22 +373,22 @@ TEST_F(LibraryTest, sanityCheck) TEST_F(LibraryTest, categoryHandling) { - EXPECT_EQ("", lib.getBookById("0c45160e-f917-760a-9159-dfe3c53cdcdd").getCategory()); - EXPECT_EQ("category_defined_via_tags_only", lib.getBookById("0d0bcd57-d3f6-cb22-44cc-a723ccb4e1b2").getCategory()); - EXPECT_EQ("category_defined_via_category_element_only", lib.getBookById("0ea1cde6-441d-6c58-f2c7-21c2838e659f").getCategory()); - EXPECT_EQ("category_element_overrides_tags", lib.getBookById("1123e574-6eef-6d54-28fc-13e4caeae474").getCategory()); - EXPECT_EQ("category_element_overrides_tags", lib.getBookById("14829621-c490-c376-0792-9de558b57efa").getCategory()); + EXPECT_EQ("", lib->getBookById("0c45160e-f917-760a-9159-dfe3c53cdcdd").getCategory()); + EXPECT_EQ("category_defined_via_tags_only", lib->getBookById("0d0bcd57-d3f6-cb22-44cc-a723ccb4e1b2").getCategory()); + EXPECT_EQ("category_defined_via_category_element_only", lib->getBookById("0ea1cde6-441d-6c58-f2c7-21c2838e659f").getCategory()); + EXPECT_EQ("category_element_overrides_tags", lib->getBookById("1123e574-6eef-6d54-28fc-13e4caeae474").getCategory()); + EXPECT_EQ("category_element_overrides_tags", lib->getBookById("14829621-c490-c376-0792-9de558b57efa").getCategory()); } TEST_F(LibraryTest, emptyFilter) { - const auto bookIds = lib.filter(kiwix::Filter()); - EXPECT_EQ(bookIds, lib.getBooksIds()); + const auto bookIds = lib->filter(kiwix::Filter()); + EXPECT_EQ(bookIds, lib->getBooksIds()); } #define EXPECT_FILTER_RESULTS(f, ...) \ EXPECT_EQ( \ - ids2Titles(lib.filter(f)), \ + ids2Titles(lib->filter(f)), \ TitleCollection({ __VA_ARGS__ }) \ ) @@ -736,33 +738,33 @@ TEST_F(LibraryTest, filterByMultipleCriteria) TEST_F(LibraryTest, getBookByPath) { - kiwix::Book book = lib.getBookById(lib.getBooksIds()[0]); + kiwix::Book book = lib->getBookById(lib->getBooksIds()[0]); #ifdef _WIN32 auto path = "C:\\some\\abs\\path.zim"; #else auto path = "/some/abs/path.zim"; #endif book.setPath(path); - lib.addBook(book); - EXPECT_EQ(lib.getBookByPath(path).getId(), book.getId()); - EXPECT_THROW(lib.getBookByPath("non/existant/path.zim"), std::out_of_range); + lib->addBook(book); + EXPECT_EQ(lib->getBookByPath(path).getId(), book.getId()); + EXPECT_THROW(lib->getBookByPath("non/existant/path.zim"), std::out_of_range); } TEST_F(LibraryTest, removeBookByIdRemovesTheBook) { - const auto initialBookCount = lib.getBookCount(true, true); + const auto initialBookCount = lib->getBookCount(true, true); ASSERT_GT(initialBookCount, 0U); - EXPECT_NO_THROW(lib.getBookById("raycharles")); - lib.removeBookById("raycharles"); - EXPECT_EQ(initialBookCount - 1, lib.getBookCount(true, true)); - EXPECT_THROW(lib.getBookById("raycharles"), std::out_of_range); + EXPECT_NO_THROW(lib->getBookById("raycharles")); + lib->removeBookById("raycharles"); + EXPECT_EQ(initialBookCount - 1, lib->getBookCount(true, true)); + EXPECT_THROW(lib->getBookById("raycharles"), std::out_of_range); }; TEST_F(LibraryTest, removeBookByIdDropsTheReader) { - EXPECT_NE(nullptr, lib.getArchiveById("raycharles")); - lib.removeBookById("raycharles"); - EXPECT_THROW(lib.getArchiveById("raycharles"), std::out_of_range); + EXPECT_NE(nullptr, lib->getArchiveById("raycharles")); + lib->removeBookById("raycharles"); + EXPECT_THROW(lib->getArchiveById("raycharles"), std::out_of_range); }; TEST_F(LibraryTest, removeBookByIdUpdatesTheSearchDB) @@ -770,17 +772,17 @@ TEST_F(LibraryTest, removeBookByIdUpdatesTheSearchDB) kiwix::Filter f; f.local(true).valid(true).query(R"(title:"ray charles")", false); - EXPECT_NO_THROW(lib.getBookById("raycharles")); - EXPECT_EQ(1U, lib.filter(f).size()); + EXPECT_NO_THROW(lib->getBookById("raycharles")); + EXPECT_EQ(1U, lib->filter(f).size()); - lib.removeBookById("raycharles"); + lib->removeBookById("raycharles"); - EXPECT_THROW(lib.getBookById("raycharles"), std::out_of_range); - EXPECT_EQ(0U, lib.filter(f).size()); + EXPECT_THROW(lib->getBookById("raycharles"), std::out_of_range); + EXPECT_EQ(0U, lib->filter(f).size()); // make sure that Library::filter() doesn't add an empty book with // an id surviving in the search DB - EXPECT_THROW(lib.getBookById("raycharles"), std::out_of_range); + EXPECT_THROW(lib->getBookById("raycharles"), std::out_of_range); }; TEST_F(LibraryTest, removeBooksNotUpdatedSince) @@ -800,18 +802,18 @@ TEST_F(LibraryTest, removeBooksNotUpdatedSince) "Wikiquote" ); - const uint64_t rev = lib.getRevision(); - for ( const auto& id : lib.filter(kiwix::Filter().query("exchange")) ) { - lib.addBook(lib.getBookByIdThreadSafe(id)); + const uint64_t rev = lib->getRevision(); + for ( const auto& id : lib->filter(kiwix::Filter().query("exchange")) ) { + lib->addBook(lib->getBookByIdThreadSafe(id)); } - EXPECT_GT(lib.getRevision(), rev); + EXPECT_GT(lib->getRevision(), rev); - const uint64_t rev2 = lib.getRevision(); + const uint64_t rev2 = lib->getRevision(); - EXPECT_EQ(9u, lib.removeBooksNotUpdatedSince(rev)); + EXPECT_EQ(9u, lib->removeBooksNotUpdatedSince(rev)); - EXPECT_GT(lib.getRevision(), rev2); + EXPECT_GT(lib->getRevision(), rev2); EXPECT_FILTER_RESULTS(kiwix::Filter(), "Islam Stack Exchange", diff --git a/test/manager.cpp b/test/manager.cpp index e2b600644..21a492005 100644 --- a/test/manager.cpp +++ b/test/manager.cpp @@ -8,18 +8,18 @@ TEST(ManagerTest, addBookFromPathAndGetIdTest) { - kiwix::Library lib; - kiwix::Manager manager = kiwix::Manager(&lib); + auto lib = kiwix::Library::create(); + kiwix::Manager manager = kiwix::Manager(lib.get()); auto bookId = manager.addBookFromPathAndGetId("./test/example.zim"); ASSERT_NE(bookId, ""); - kiwix::Book book = lib.getBookById(bookId); + kiwix::Book book = lib->getBookById(bookId); EXPECT_EQ(book.getPath(), kiwix::computeAbsolutePath("", "./test/example.zim")); const std::string pathToSave = "./pathToSave"; const std::string url = "url"; bookId = manager.addBookFromPathAndGetId("./test/example.zim", pathToSave, url, true); - book = lib.getBookById(bookId); + book = lib->getBookById(bookId); auto savedPath = kiwix::computeAbsolutePath(kiwix::removeLastPathElement(manager.writableLibraryPath), pathToSave); EXPECT_EQ(book.getPath(), savedPath); EXPECT_EQ(book.getUrl(), url); @@ -48,11 +48,11 @@ const char sampleLibraryXML[] = R"( TEST(ManagerTest, readXml) { - kiwix::Library lib; - kiwix::Manager manager = kiwix::Manager(&lib); + auto lib = kiwix::Library::create(); + kiwix::Manager manager = kiwix::Manager(lib.get()); EXPECT_EQ(true, manager.readXml(sampleLibraryXML, true, "/data/lib.xml", true)); - kiwix::Book book = lib.getBookById("0d0bcd57-d3f6-cb22-44cc-a723ccb4e1b2"); + kiwix::Book book = lib->getBookById("0d0bcd57-d3f6-cb22-44cc-a723ccb4e1b2"); EXPECT_EQ("/data/zimfiles/unittest.zim", book.getPath()); EXPECT_EQ("https://example.com/zimfiles/unittest.zim", book.getUrl()); EXPECT_EQ("Unit Test", book.getTitle()); @@ -70,24 +70,24 @@ TEST(ManagerTest, readXml) TEST(Manager, reload) { - kiwix::Library lib; - kiwix::Manager manager(&lib); + auto lib = kiwix::Library::create(); + kiwix::Manager manager(lib.get()); manager.reload({ "./test/library.xml" }); - EXPECT_EQ(lib.getBooksIds(), (kiwix::Library::BookIdCollection{ + EXPECT_EQ(lib->getBooksIds(), (kiwix::Library::BookIdCollection{ "charlesray", "raycharles", "raycharles_uncategorized" })); - lib.removeBookById("raycharles"); - EXPECT_EQ(lib.getBooksIds(), (kiwix::Library::BookIdCollection{ + lib->removeBookById("raycharles"); + EXPECT_EQ(lib->getBooksIds(), (kiwix::Library::BookIdCollection{ "charlesray", "raycharles_uncategorized" })); manager.reload({ "./test/library.xml" }); - EXPECT_EQ(lib.getBooksIds(), kiwix::Library::BookIdCollection({ + EXPECT_EQ(lib->getBooksIds(), kiwix::Library::BookIdCollection({ "charlesray", "raycharles", "raycharles_uncategorized" diff --git a/test/name_mapper.cpp b/test/name_mapper.cpp index 0bc60254a..cb7af2686 100644 --- a/test/name_mapper.cpp +++ b/test/name_mapper.cpp @@ -18,18 +18,20 @@ const char libraryXML[] = R"( )"; class NameMapperTest : public ::testing::Test { + public: + NameMapperTest(): lib(kiwix::Library::create()) {} protected: void SetUp() override { - kiwix::Manager manager(&lib); + kiwix::Manager manager(lib.get()); manager.readXml(libraryXML, false, "./library.xml", true); - for ( const std::string& id : lib.getBooksIds() ) { - kiwix::Book bookCopy = lib.getBookById(id); + for ( const std::string& id : lib->getBooksIds() ) { + kiwix::Book bookCopy = lib->getBookById(id); bookCopy.setPathValid(true); - lib.addBook(bookCopy); + lib->addBook(bookCopy); } } - kiwix::Library lib; + std::shared_ptr lib; }; class CapturedStderr @@ -73,13 +75,13 @@ void checkUnaliasedEntriesInNameMapper(const kiwix::NameMapper& nm) TEST_F(NameMapperTest, HumanReadableNameMapperWithoutAliases) { CapturedStderr stderror; - kiwix::HumanReadableNameMapper nm(lib, false); + kiwix::HumanReadableNameMapper nm(*lib, false); EXPECT_EQ("", std::string(stderror)); checkUnaliasedEntriesInNameMapper(nm); EXPECT_THROW(nm.getIdForName("zero_four"), std::out_of_range); - lib.removeBookById("04-2021-10"); + lib->removeBookById("04-2021-10"); EXPECT_EQ("zero_four_2021-10", nm.getNameForId("04-2021-10")); EXPECT_EQ("04-2021-10", nm.getIdForName("zero_four_2021-10")); EXPECT_THROW(nm.getIdForName("zero_four"), std::out_of_range); @@ -88,7 +90,7 @@ TEST_F(NameMapperTest, HumanReadableNameMapperWithoutAliases) TEST_F(NameMapperTest, HumanReadableNameMapperWithAliases) { CapturedStderr stderror; - kiwix::HumanReadableNameMapper nm(lib, true); + kiwix::HumanReadableNameMapper nm(*lib, true); EXPECT_EQ( "Path collision: /data/zero_four_2021-10.zim and" " /data/zero_four_2021-11.zim can't share the same URL path 'zero_four'." @@ -99,7 +101,7 @@ TEST_F(NameMapperTest, HumanReadableNameMapperWithAliases) checkUnaliasedEntriesInNameMapper(nm); EXPECT_EQ("04-2021-10", nm.getIdForName("zero_four")); - lib.removeBookById("04-2021-10"); + lib->removeBookById("04-2021-10"); EXPECT_EQ("zero_four_2021-10", nm.getNameForId("04-2021-10")); EXPECT_EQ("04-2021-10", nm.getIdForName("zero_four_2021-10")); EXPECT_EQ("04-2021-10", nm.getIdForName("zero_four")); @@ -108,13 +110,13 @@ TEST_F(NameMapperTest, HumanReadableNameMapperWithAliases) TEST_F(NameMapperTest, UpdatableNameMapperWithoutAliases) { CapturedStderr stderror; - kiwix::UpdatableNameMapper nm(lib, false); + kiwix::UpdatableNameMapper nm(*lib, false); EXPECT_EQ("", std::string(stderror)); checkUnaliasedEntriesInNameMapper(nm); EXPECT_THROW(nm.getIdForName("zero_four"), std::out_of_range); - lib.removeBookById("04-2021-10"); + lib->removeBookById("04-2021-10"); nm.update(); EXPECT_THROW(nm.getNameForId("04-2021-10"), std::out_of_range); EXPECT_THROW(nm.getIdForName("zero_four_2021-10"), std::out_of_range); @@ -124,7 +126,7 @@ TEST_F(NameMapperTest, UpdatableNameMapperWithoutAliases) TEST_F(NameMapperTest, UpdatableNameMapperWithAliases) { CapturedStderr stderror; - kiwix::UpdatableNameMapper nm(lib, true); + kiwix::UpdatableNameMapper nm(*lib, true); EXPECT_EQ( "Path collision: /data/zero_four_2021-10.zim and" " /data/zero_four_2021-11.zim can't share the same URL path 'zero_four'." @@ -137,7 +139,7 @@ TEST_F(NameMapperTest, UpdatableNameMapperWithAliases) { CapturedStderr nmUpdateStderror; - lib.removeBookById("04-2021-10"); + lib->removeBookById("04-2021-10"); nm.update(); EXPECT_EQ("", std::string(nmUpdateStderror)); } diff --git a/test/server_testing_tools.h b/test/server_testing_tools.h index 777bb48e8..1974a0132 100644 --- a/test/server_testing_tools.h +++ b/test/server_testing_tools.h @@ -98,7 +98,7 @@ private: void run(int serverPort, std::string indexTemplateString = ""); private: // data - kiwix::Library library; + std::shared_ptr library; kiwix::Manager manager; std::unique_ptr nameMapper; std::unique_ptr server; @@ -107,7 +107,8 @@ private: // data }; ZimFileServer::ZimFileServer(int serverPort, Cfg _cfg, std::string libraryFilePath) -: manager(&this->library) +: library(kiwix::Library::create()) +, manager(this->library.get()) , cfg(_cfg) { if ( kiwix::isRelativePath(libraryFilePath) ) @@ -120,7 +121,8 @@ ZimFileServer::ZimFileServer(int serverPort, Cfg _cfg, const FilePathCollection& zimpaths, std::string indexTemplateString) -: manager(&this->library) +: library(kiwix::Library::create()) +, manager(this->library.get()) , cfg(_cfg) { for ( const auto& zimpath : zimpaths ) { @@ -136,9 +138,9 @@ void ZimFileServer::run(int serverPort, std::string indexTemplateString) if (cfg.options & NO_NAME_MAPPER) { nameMapper.reset(new kiwix::IdNameMapper()); } else { - nameMapper.reset(new kiwix::HumanReadableNameMapper(library, false)); + nameMapper.reset(new kiwix::HumanReadableNameMapper(*library, false)); } - server.reset(new kiwix::Server(&library, nameMapper.get())); + server.reset(new kiwix::Server(library.get(), nameMapper.get())); server->setRoot(cfg.root); server->setAddress(address); server->setPort(serverPort);