From cc849b31da4734a2cc7f8d65f290f519cf6d842e Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Tue, 18 Oct 2022 16:26:44 +0200 Subject: [PATCH 01/16] =?UTF-8?q?[New]=20Use=20a=20macro=C2=A0to=20define?= =?UTF-8?q?=20catalog's=20entries=20in=20test.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- test/library_server.cpp | 112 +++++++++++++++++++--------------------- 1 file changed, 53 insertions(+), 59 deletions(-) diff --git a/test/library_server.cpp b/test/library_server.cpp index d8f1ecacf..bed7806bc 100644 --- a/test/library_server.cpp +++ b/test/library_server.cpp @@ -70,20 +70,20 @@ std::string maskVariableOPDSFeedData(std::string s) " type=\"application/opensearchdescription+xml\"" \ " href=\"/ROOT/catalog/searchdescription.xml\" />\n" -#define CHARLES_RAY_CATALOG_ENTRY \ +#define CATALOG_ENTRY(UUID, TITLE, SUMMARY, LANG, NAME, CATEGORY, TAGS, EXTRA_LINK, CONTENT_NAME, FILE_NAME, LENGTH) \ " \n" \ - " urn:uuid:charlesray\n" \ - " Charles, Ray\n" \ + " urn:uuid:" UUID "\n" \ + " " TITLE "\n" \ " YYYY-MM-DDThh:mm:ssZ\n" \ - " Wikipedia articles about Ray Charles\n" \ - " fra\n" \ - " wikipedia_fr_ray_charles\n" \ + " " SUMMARY "\n" \ + " " LANG "\n" \ + " " NAME "\n" \ " \n" \ - " jazz\n" \ - " unittest;wikipedia;_category:jazz;_pictures:no;_videos:no;_details:no;_ftindex:yes\n" \ + " " CATEGORY "\n" \ + " " TAGS "\n" \ " 284\n" \ " 2\n" \ - " \n" \ + " " EXTRA_LINK "\n" \ " \n" \ " Wikipedia\n" \ " \n" \ @@ -91,59 +91,53 @@ std::string maskVariableOPDSFeedData(std::string s) " Kiwix\n" \ " \n" \ " 2020-03-31T00:00:00Z\n" \ - " \n" \ + " \n" \ " \n" -#define RAY_CHARLES_CATALOG_ENTRY \ - " \n" \ - " urn:uuid:raycharles\n" \ - " Ray Charles\n" \ - " YYYY-MM-DDThh:mm:ssZ\n" \ - " Wikipedia articles about Ray Charles\n" \ - " eng\n" \ - " wikipedia_en_ray_charles\n" \ - " \n" \ - " wikipedia\n" \ - " public_tag_without_a_value;_private_tag_without_a_value;wikipedia;_category:wikipedia;_pictures:no;_videos:no;_details:no;_ftindex:yes\n" \ - " 284\n" \ - " 2\n" \ - " \n" \ - " \n" \ - " \n" \ - " Wikipedia\n" \ - " \n" \ - " \n" \ - " Kiwix\n" \ - " \n" \ - " 2020-03-31T00:00:00Z\n" \ - " \n" \ - " \n" -#define UNCATEGORIZED_RAY_CHARLES_CATALOG_ENTRY \ - " \n" \ - " urn:uuid:raycharles_uncategorized\n" \ - " Ray (uncategorized) Charles\n" \ - " YYYY-MM-DDThh:mm:ssZ\n" \ - " No category is assigned to this library entry.\n" \ - " rus\n" \ - " wikipedia_ru_ray_charles\n" \ - " \n" \ - " \n" \ - " public_tag_with_a_value:value_of_a_public_tag;_private_tag_with_a_value:value_of_a_private_tag;wikipedia;_pictures:no;_videos:no;_details:no\n" \ - " 284\n" \ - " 2\n" \ - " \n" \ - " \n" \ - " Wikipedia\n" \ - " \n" \ - " \n" \ - " Kiwix\n" \ - " \n" \ - " 2020-03-31T00:00:00Z\n" \ - " \n" \ - " \n" +#define CHARLES_RAY_CATALOG_ENTRY CATALOG_ENTRY( \ + "charlesray", \ + "Charles, Ray", \ + "Wikipedia articles about Ray Charles", \ + "fra", \ + "wikipedia_fr_ray_charles",\ + "jazz",\ + "unittest;wikipedia;_category:jazz;_pictures:no;_videos:no;_details:no;_ftindex:yes",\ + "", \ + "zimfile%26other", \ + "zimfile%26other", \ + "569344" \ +) + +#define RAY_CHARLES_CATALOG_ENTRY CATALOG_ENTRY(\ + "raycharles",\ + "Ray Charles",\ + "Wikipedia articles about Ray Charles",\ + "eng",\ + "wikipedia_en_ray_charles",\ + "wikipedia",\ + "public_tag_without_a_value;_private_tag_without_a_value;wikipedia;_category:wikipedia;_pictures:no;_videos:no;_details:no;_ftindex:yes",\ + "\n ", \ + "zimfile", \ + "zimfile", \ + "569344"\ +) + +#define UNCATEGORIZED_RAY_CHARLES_CATALOG_ENTRY CATALOG_ENTRY(\ + "raycharles_uncategorized",\ + "Ray (uncategorized) Charles",\ + "No category is assigned to this library entry.",\ + "rus",\ + "wikipedia_ru_ray_charles",\ + "",\ + "public_tag_with_a_value:value_of_a_public_tag;_private_tag_with_a_value:value_of_a_private_tag;wikipedia;_pictures:no;_videos:no;_details:no",\ + "",\ + "zimfile", \ + "zimfile", \ + "125952"\ +) TEST_F(LibraryServerTest, catalog_root_xml) { From 96fb4236a63bd94cf76aadbf06bf7d2f7bdda0ee Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Wed, 5 Oct 2022 15:41:04 +0200 Subject: [PATCH 02/16] Make the opds_dumper respect the provided nameMapper used in the server. Fix #828 --- include/opds_dumper.h | 4 +- src/opds_dumper.cpp | 21 ++++---- src/server/internalServer.cpp | 2 +- src/server/internalServer_catalog_v2.cpp | 8 +-- test/library_server.cpp | 65 +++++++++++++++++++++++- test/server.cpp | 7 +++ test/server_testing_tools.h | 14 +++-- 7 files changed, 101 insertions(+), 20 deletions(-) diff --git a/include/opds_dumper.h b/include/opds_dumper.h index c824493de..cb6566f4f 100644 --- a/include/opds_dumper.h +++ b/include/opds_dumper.h @@ -27,6 +27,7 @@ #include #include "library.h" +#include "name_mapper.h" using namespace std; @@ -41,7 +42,7 @@ class OPDSDumper { public: OPDSDumper() = default; - OPDSDumper(Library* library); + OPDSDumper(Library* library, NameMapper* NameMapper); ~OPDSDumper(); /** @@ -110,6 +111,7 @@ class OPDSDumper protected: kiwix::Library* library; + kiwix::NameMapper* nameMapper; std::string libraryId; std::string rootLocation; int m_totalResults; diff --git a/src/opds_dumper.cpp b/src/opds_dumper.cpp index 7436ebc29..702f0a4f8 100644 --- a/src/opds_dumper.cpp +++ b/src/opds_dumper.cpp @@ -30,8 +30,9 @@ namespace kiwix { /* Constructor */ -OPDSDumper::OPDSDumper(Library* library) - : library(library) +OPDSDumper::OPDSDumper(Library* library, NameMapper* nameMapper) + : library(library), + nameMapper(nameMapper) { } /* Destructor */ @@ -71,7 +72,7 @@ IllustrationInfo getBookIllustrationInfo(const Book& book) return illustrations; } -std::string fullEntryXML(const Book& book, const std::string& rootLocation) +std::string fullEntryXML(const Book& book, const std::string& rootLocation, const std::string& bookName) { const auto bookDate = book.getDate() + "T00:00:00Z"; const kainjow::mustache::object data{ @@ -81,7 +82,7 @@ std::string fullEntryXML(const Book& book, const std::string& rootLocation) {"title", book.getTitle()}, {"description", book.getDescription()}, {"language", book.getLanguage()}, - {"content_id", urlEncode(book.getHumanReadableIdFromPath(), true)}, + {"content_id", urlEncode(bookName, true)}, {"updated", bookDate}, // XXX: this should be the entry update datetime {"book_date", bookDate}, {"category", book.getCategory()}, @@ -112,15 +113,16 @@ std::string partialEntryXML(const Book& book, const std::string& rootLocation) return render_template(xmlTemplate, data); } -BooksData getBooksData(const Library* library, const std::vector& bookIds, const std::string& rootLocation, bool partial) +BooksData getBooksData(const Library* library, const NameMapper& nameMapper, const std::vector& bookIds, const std::string& rootLocation, bool partial) { BooksData booksData; for ( const auto& bookId : bookIds ) { try { const Book book = library->getBookByIdThreadSafe(bookId); + const std::string bookName = nameMapper.getNameForId(bookId); const auto entryXML = partial ? partialEntryXML(book, rootLocation) - : fullEntryXML(book, rootLocation); + : fullEntryXML(book, rootLocation, bookName); booksData.push_back(kainjow::mustache::object{ {"entry", entryXML} }); } catch ( const std::out_of_range& ) { // the book was removed from the library since its id was obtained @@ -188,7 +190,7 @@ std::string getLanguageSelfName(const std::string& lang) { string OPDSDumper::dumpOPDSFeed(const std::vector& bookIds, const std::string& query) const { - const auto booksData = getBooksData(library, bookIds, rootLocation, false); + const auto booksData = getBooksData(library, *nameMapper, bookIds, rootLocation, false); const kainjow::mustache::object template_data{ {"date", gen_date_str()}, {"root", rootLocation}, @@ -206,7 +208,7 @@ string OPDSDumper::dumpOPDSFeed(const std::vector& bookIds, const s string OPDSDumper::dumpOPDSFeedV2(const std::vector& bookIds, const std::string& query, bool partial) const { const auto endpointRoot = rootLocation + "/catalog/v2"; - const auto booksData = getBooksData(library, bookIds, rootLocation, partial); + const auto booksData = getBooksData(library, *nameMapper, bookIds, rootLocation, partial); const char* const endpoint = partial ? "/partial_entries" : "/entries"; const kainjow::mustache::object template_data{ @@ -228,9 +230,10 @@ string OPDSDumper::dumpOPDSFeedV2(const std::vector& bookIds, const std::string OPDSDumper::dumpOPDSCompleteEntry(const std::string& bookId) const { const auto book = library->getBookById(bookId); + const std::string bookName = nameMapper->getNameForId(bookId); return XML_HEADER + "\n" - + fullEntryXML(book, rootLocation); + + fullEntryXML(book, rootLocation, bookName); } std::string OPDSDumper::categoriesOPDSFeed() const diff --git a/src/server/internalServer.cpp b/src/server/internalServer.cpp index d6e6a91a4..8c449ac35 100644 --- a/src/server/internalServer.cpp +++ b/src/server/internalServer.cpp @@ -967,7 +967,7 @@ std::unique_ptr InternalServer::handle_catalog(const RequestContext& r } zim::Uuid uuid; - kiwix::OPDSDumper opdsDumper(mp_library); + kiwix::OPDSDumper opdsDumper(mp_library, mp_nameMapper); opdsDumper.setRootLocation(m_root); opdsDumper.setLibraryId(m_library_id); std::vector bookIdsToDump; diff --git a/src/server/internalServer_catalog_v2.cpp b/src/server/internalServer_catalog_v2.cpp index b082dd1c1..93b400a4f 100644 --- a/src/server/internalServer_catalog_v2.cpp +++ b/src/server/internalServer_catalog_v2.cpp @@ -95,7 +95,7 @@ std::unique_ptr InternalServer::handle_catalog_v2_root(const RequestCo std::unique_ptr InternalServer::handle_catalog_v2_entries(const RequestContext& request, bool partial) { - OPDSDumper opdsDumper(mp_library); + OPDSDumper opdsDumper(mp_library, mp_nameMapper); opdsDumper.setRootLocation(m_root); opdsDumper.setLibraryId(m_library_id); const auto bookIds = search_catalog(request, opdsDumper); @@ -116,7 +116,7 @@ std::unique_ptr InternalServer::handle_catalog_v2_complete_entry(const + urlNotFoundMsg; } - OPDSDumper opdsDumper(mp_library); + OPDSDumper opdsDumper(mp_library, mp_nameMapper); opdsDumper.setRootLocation(m_root); opdsDumper.setLibraryId(m_library_id); const auto opdsFeed = opdsDumper.dumpOPDSCompleteEntry(entryId); @@ -129,7 +129,7 @@ std::unique_ptr InternalServer::handle_catalog_v2_complete_entry(const std::unique_ptr InternalServer::handle_catalog_v2_categories(const RequestContext& request) { - OPDSDumper opdsDumper(mp_library); + OPDSDumper opdsDumper(mp_library, mp_nameMapper); opdsDumper.setRootLocation(m_root); opdsDumper.setLibraryId(m_library_id); return ContentResponse::build( @@ -141,7 +141,7 @@ std::unique_ptr InternalServer::handle_catalog_v2_categories(const Req std::unique_ptr InternalServer::handle_catalog_v2_languages(const RequestContext& request) { - OPDSDumper opdsDumper(mp_library); + OPDSDumper opdsDumper(mp_library, mp_nameMapper); opdsDumper.setRootLocation(m_root); opdsDumper.setLibraryId(m_library_id); return ContentResponse::build( diff --git a/test/library_server.cpp b/test/library_server.cpp index bed7806bc..98b7f0b75 100644 --- a/test/library_server.cpp +++ b/test/library_server.cpp @@ -19,7 +19,7 @@ class LibraryServerTest : public ::testing::Test protected: void SetUp() override { - zfs1_.reset(new ZimFileServer(PORT, "./test/library.xml")); + zfs1_.reset(new ZimFileServer(PORT, ZimFileServer::DEFAULT_OPTIONS, "./test/library.xml")); } void TearDown() override { @@ -774,4 +774,67 @@ TEST_F(LibraryServerTest, catalog_search_excludes_hidden_tags) #undef EXPECT_ZERO_RESULTS } + +// Same as CHARLES_RAY_CATALOG_ENTRY but with link using the uuid (charlesray). +#define NO_MAPPER_CHARLES_RAY_CATALOG_ENTRY \ + " \n" \ + " urn:uuid:charlesray\n" \ + " Charles, Ray\n" \ + " YYYY-MM-DDThh:mm:ssZ\n" \ + " Wikipedia articles about Ray Charles\n" \ + " fra\n" \ + " wikipedia_fr_ray_charles\n" \ + " \n" \ + " jazz\n" \ + " unittest;wikipedia;_category:jazz;_pictures:no;_videos:no;_details:no;_ftindex:yes\n" \ + " 284\n" \ + " 2\n" \ + " \n" \ + " \n" \ + " Wikipedia\n" \ + " \n" \ + " \n" \ + " Kiwix\n" \ + " \n" \ + " 2020-03-31T00:00:00Z\n" \ + " \n" \ + " \n" + +class NoMapperLibraryServerTest : public ::testing::Test +{ +protected: + std::unique_ptr zfs1_; + + const int PORT = 8002; + +protected: + void SetUp() override { + zfs1_.reset(new ZimFileServer(PORT, ZimFileServer::NO_NAME_MAPPER, "./test/library.xml")); + } + + void TearDown() override { + zfs1_.reset(); + } +}; + +TEST_F(NoMapperLibraryServerTest, returned_catalog_use_uuid_in_link) +{ + const auto r = zfs1_->GET("/ROOT/catalog/search?tag=_category:jazz"); + EXPECT_EQ(r->status, 200); + EXPECT_EQ(maskVariableOPDSFeedData(r->body), + OPDS_FEED_TAG + " 12345678-90ab-cdef-1234-567890abcdef\n" + " Filtered zims (tag=_category:jazz)\n" + " YYYY-MM-DDThh:mm:ssZ\n" + " 1\n" + " 0\n" + " 1\n" + CATALOG_LINK_TAGS + NO_MAPPER_CHARLES_RAY_CATALOG_ENTRY + "\n" + ); +} + + + #undef EXPECT_SEARCH_RESULTS diff --git a/test/server.cpp b/test/server.cpp index 2d1e1068a..3da31e5b6 100644 --- a/test/server.cpp +++ b/test/server.cpp @@ -131,6 +131,13 @@ TEST_F(ServerTest, 200) EXPECT_EQ(200, zfs1_->GET(res.url)->status) << "res.url: " << res.url; } +TEST_F(ServerTest, 200_IdNameMapper) +{ + resetServer(ZimFileServer::NO_NAME_MAPPER); + EXPECT_EQ(200, zfs1_->GET("/ROOT/content/6f1d19d0-633f-087b-fb55-7ac324ff9baf/A/index")->status) << "url: /ROOT/content/6f1d19d0-633f-087b-fb55-7ac324ff9baf/A/index"; + EXPECT_EQ(404, zfs1_->GET("/ROOT/content/zimfile/A/index")->status) << "url: /ROOT/content/zimfile/A/index"; +} + TEST_F(ServerTest, CompressibleContentIsCompressedIfAcceptable) { for ( const Resource& res : resources200Compressible ) { diff --git a/test/server_testing_tools.h b/test/server_testing_tools.h index ab0b4a511..7af5834e6 100644 --- a/test/server_testing_tools.h +++ b/test/server_testing_tools.h @@ -61,6 +61,7 @@ class ZimFileServer WITH_TASKBAR = 1 << 1, WITH_LIBRARY_BUTTON = 1 << 2, BLOCK_EXTERNAL_LINKS = 1 << 3, + NO_NAME_MAPPER = 1 << 4, WITH_TASKBAR_AND_LIBRARY_BUTTON = WITH_TASKBAR | WITH_LIBRARY_BUTTON, @@ -68,7 +69,7 @@ class ZimFileServer }; public: // functions - ZimFileServer(int serverPort, std::string libraryFilePath); + ZimFileServer(int serverPort, Options options, std::string libraryFilePath); ZimFileServer(int serverPort, Options options, const FilePathCollection& zimpaths, @@ -91,14 +92,15 @@ class ZimFileServer private: // data kiwix::Library library; kiwix::Manager manager; - std::unique_ptr nameMapper; + std::unique_ptr nameMapper; std::unique_ptr server; std::unique_ptr client; const Options options = DEFAULT_OPTIONS; }; -ZimFileServer::ZimFileServer(int serverPort, std::string libraryFilePath) +ZimFileServer::ZimFileServer(int serverPort, Options _options, std::string libraryFilePath) : manager(&this->library) +, options(_options) { if ( kiwix::isRelativePath(libraryFilePath) ) libraryFilePath = kiwix::computeAbsolutePath(kiwix::getCurrentDirectory(), libraryFilePath); @@ -123,7 +125,11 @@ ZimFileServer::ZimFileServer(int serverPort, void ZimFileServer::run(int serverPort, std::string indexTemplateString) { const std::string address = "127.0.0.1"; - nameMapper.reset(new kiwix::HumanReadableNameMapper(library, false)); + if (options & NO_NAME_MAPPER) { + nameMapper.reset(new kiwix::IdNameMapper()); + } else { + nameMapper.reset(new kiwix::HumanReadableNameMapper(library, false)); + } server.reset(new kiwix::Server(&library, nameMapper.get())); server->setRoot("ROOT"); server->setAddress(address); From 24472e03ddf234626824955ddf53dab2531f6c4d Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Tue, 18 Oct 2022 16:22:01 +0200 Subject: [PATCH 03/16] fixup! Make the opds_dumper respect the provided nameMapper used in the server. --- test/library_server.cpp | 80 +++++++++++++++++------------------------ test/server.cpp | 4 +-- 2 files changed, 34 insertions(+), 50 deletions(-) diff --git a/test/library_server.cpp b/test/library_server.cpp index 98b7f0b75..277d139a2 100644 --- a/test/library_server.cpp +++ b/test/library_server.cpp @@ -18,6 +18,11 @@ class LibraryServerTest : public ::testing::Test const int PORT = 8002; protected: + void resetServer(ZimFileServer::Options options) { + zfs1_.reset(); + zfs1_.reset(new ZimFileServer(PORT, options, "./test/library.xml")); + } + void SetUp() override { zfs1_.reset(new ZimFileServer(PORT, ZimFileServer::DEFAULT_OPTIONS, "./test/library.xml")); } @@ -95,7 +100,7 @@ std::string maskVariableOPDSFeedData(std::string s) " \n" -#define CHARLES_RAY_CATALOG_ENTRY CATALOG_ENTRY( \ +#define _CHARLES_RAY_CATALOG_ENTRY(CONTENT_NAME) CATALOG_ENTRY( \ "charlesray", \ "Charles, Ray", \ "Wikipedia articles about Ray Charles", \ @@ -104,12 +109,14 @@ std::string maskVariableOPDSFeedData(std::string s) "jazz",\ "unittest;wikipedia;_category:jazz;_pictures:no;_videos:no;_details:no;_ftindex:yes",\ "", \ - "zimfile%26other", \ + CONTENT_NAME, \ "zimfile%26other", \ "569344" \ ) -#define RAY_CHARLES_CATALOG_ENTRY CATALOG_ENTRY(\ +#define CHARLES_RAY_CATALOG_ENTRY _CHARLES_RAY_CATALOG_ENTRY("zimfile%26other") + +#define _RAY_CHARLES_CATALOG_ENTRY(CONTENT_NAME) CATALOG_ENTRY(\ "raycharles",\ "Ray Charles",\ "Wikipedia articles about Ray Charles",\ @@ -120,11 +127,13 @@ std::string maskVariableOPDSFeedData(std::string s) "\n ", \ - "zimfile", \ + CONTENT_NAME, \ "zimfile", \ "569344"\ ) +#define RAY_CHARLES_CATALOG_ENTRY _RAY_CHARLES_CATALOG_ENTRY("zimfile") + #define UNCATEGORIZED_RAY_CHARLES_CATALOG_ENTRY CATALOG_ENTRY(\ "raycharles_uncategorized",\ "Ray (uncategorized) Charles",\ @@ -774,51 +783,11 @@ TEST_F(LibraryServerTest, catalog_search_excludes_hidden_tags) #undef EXPECT_ZERO_RESULTS } - -// Same as CHARLES_RAY_CATALOG_ENTRY but with link using the uuid (charlesray). -#define NO_MAPPER_CHARLES_RAY_CATALOG_ENTRY \ - " \n" \ - " urn:uuid:charlesray\n" \ - " Charles, Ray\n" \ - " YYYY-MM-DDThh:mm:ssZ\n" \ - " Wikipedia articles about Ray Charles\n" \ - " fra\n" \ - " wikipedia_fr_ray_charles\n" \ - " \n" \ - " jazz\n" \ - " unittest;wikipedia;_category:jazz;_pictures:no;_videos:no;_details:no;_ftindex:yes\n" \ - " 284\n" \ - " 2\n" \ - " \n" \ - " \n" \ - " Wikipedia\n" \ - " \n" \ - " \n" \ - " Kiwix\n" \ - " \n" \ - " 2020-03-31T00:00:00Z\n" \ - " \n" \ - " \n" - -class NoMapperLibraryServerTest : public ::testing::Test -{ -protected: - std::unique_ptr zfs1_; - - const int PORT = 8002; - -protected: - void SetUp() override { - zfs1_.reset(new ZimFileServer(PORT, ZimFileServer::NO_NAME_MAPPER, "./test/library.xml")); - } - - void TearDown() override { - zfs1_.reset(); - } -}; - -TEST_F(NoMapperLibraryServerTest, returned_catalog_use_uuid_in_link) +#define NO_MAPPER_CHARLES_RAY_CATALOG_ENTRY _CHARLES_RAY_CATALOG_ENTRY("charlesray") +#define NO_MAPPER_RAY_CHARLES_CATALOG_ENTRY _RAY_CHARLES_CATALOG_ENTRY("raycharles") +TEST_F(LibraryServerTest, no_name_mapper_returned_catalog_use_uuid_in_link) { + resetServer(ZimFileServer::NO_NAME_MAPPER); const auto r = zfs1_->GET("/ROOT/catalog/search?tag=_category:jazz"); EXPECT_EQ(r->status, 200); EXPECT_EQ(maskVariableOPDSFeedData(r->body), @@ -836,5 +805,20 @@ TEST_F(NoMapperLibraryServerTest, returned_catalog_use_uuid_in_link) } +TEST_F(LibraryServerTest, no_name_mapper_catalog_v2_individual_entry_access) +{ + resetServer(ZimFileServer::NO_NAME_MAPPER); + const auto r = zfs1_->GET("/ROOT/catalog/v2/entry/raycharles"); + EXPECT_EQ(r->status, 200); + EXPECT_EQ(maskVariableOPDSFeedData(r->body), + "\n" + NO_MAPPER_RAY_CHARLES_CATALOG_ENTRY + ); + + const auto r1 = zfs1_->GET("/ROOT/catalog/v2/entry/non-existent-entry"); + EXPECT_EQ(r1->status, 404); +} + + #undef EXPECT_SEARCH_RESULTS diff --git a/test/server.cpp b/test/server.cpp index 3da31e5b6..d03f9e6bb 100644 --- a/test/server.cpp +++ b/test/server.cpp @@ -134,8 +134,8 @@ TEST_F(ServerTest, 200) TEST_F(ServerTest, 200_IdNameMapper) { resetServer(ZimFileServer::NO_NAME_MAPPER); - EXPECT_EQ(200, zfs1_->GET("/ROOT/content/6f1d19d0-633f-087b-fb55-7ac324ff9baf/A/index")->status) << "url: /ROOT/content/6f1d19d0-633f-087b-fb55-7ac324ff9baf/A/index"; - EXPECT_EQ(404, zfs1_->GET("/ROOT/content/zimfile/A/index")->status) << "url: /ROOT/content/zimfile/A/index"; + EXPECT_EQ(200, zfs1_->GET("/ROOT/content/6f1d19d0-633f-087b-fb55-7ac324ff9baf/A/index")->status); + EXPECT_EQ(404, zfs1_->GET("/ROOT/content/zimfile/A/index")->status); } TEST_F(ServerTest, CompressibleContentIsCompressedIfAcceptable) From 85f58b8e0184df8153bbf62f873264b2242660f7 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Thu, 6 Oct 2022 11:29:24 +0200 Subject: [PATCH 04/16] Introduce a ServerConfiguration object. It is used to store the server configuration instead of passing (a lot of) arguments to functions/creators. Please note that this remove the thread protected on m_verbose. m_verbose is initialized once and never modified, be don't need to protect access. --- include/server.h | 99 ++++++++++++++----- src/server.cpp | 23 +---- src/server/internalServer.cpp | 117 ++++++++++------------- src/server/internalServer.h | 30 +----- src/server/internalServer_catalog_v2.cpp | 26 ++--- src/server/response.cpp | 8 +- test/server_testing_tools.h | 22 +++-- 7 files changed, 160 insertions(+), 165 deletions(-) diff --git a/include/server.h b/include/server.h index 0cd579c57..c9d2dcbc8 100644 --- a/include/server.h +++ b/include/server.h @@ -29,6 +29,77 @@ namespace kiwix class NameMapper; class InternalServer; + class ServerConfiguration { + public: + ServerConfiguration(Library* library, NameMapper* nameMapper=nullptr) + : mp_library(library), + mp_nameMapper(nameMapper) + {} + + ServerConfiguration& setRoot(const std::string& root); + ServerConfiguration& setAddress(const std::string& addr) { + m_addr = addr; + return *this; + } + + ServerConfiguration& setPort(int port) { + m_port = port; + return *this; + } + + ServerConfiguration& setNbThreads(int threads) { + m_nbThreads = threads; + return *this; + } + + ServerConfiguration& setMultiZimSearchLimit(unsigned int limit) { + m_multizimSearchLimit = limit; + return *this; + } + + ServerConfiguration& setIpConnectionLimit(int limit) { + m_ipConnectionLimit = limit; + return *this; + } + + ServerConfiguration& setVerbose(bool verbose) { + m_verbose = verbose; + return *this; + } + + ServerConfiguration& setIndexTemplateString(const std::string& indexTemplateString) { + m_indexTemplateString = indexTemplateString; + return *this; + } + + ServerConfiguration& setTaskbar(bool withTaskbar, bool withLibraryButton) { + m_withTaskbar = withTaskbar; + m_withLibraryButton = withLibraryButton; + return *this; + } + + ServerConfiguration& setBlockExternalLinks(bool blockExternalLinks) { + m_blockExternalLinks = blockExternalLinks; + return *this; + } + + Library* mp_library; + NameMapper* mp_nameMapper; + std::string m_root = ""; + std::string m_addr = ""; + std::string m_indexTemplateString = ""; + int m_port = 80; + int m_nbThreads = 1; + unsigned int m_multizimSearchLimit = 0; + bool m_verbose = false; + bool m_withTaskbar = true; + bool m_withLibraryButton = true; + bool m_blockExternalLinks = false; + int m_ipConnectionLimit = 0; + }; + + + class Server { public: /** @@ -36,7 +107,7 @@ namespace kiwix * * @param library The library to serve. */ - Server(Library* library, NameMapper* nameMapper=nullptr); + Server(const ServerConfiguration& configuration); virtual ~Server(); @@ -50,35 +121,11 @@ namespace kiwix */ void stop(); - void setRoot(const std::string& root); - void setAddress(const std::string& addr) { m_addr = addr; } - void setPort(int port) { m_port = port; } - void setNbThreads(int threads) { m_nbThreads = threads; } - void setMultiZimSearchLimit(unsigned int limit) { m_multizimSearchLimit = limit; } - void setIpConnectionLimit(int limit) { m_ipConnectionLimit = limit; } - void setVerbose(bool verbose) { m_verbose = verbose; } - void setIndexTemplateString(const std::string& indexTemplateString) { m_indexTemplateString = indexTemplateString; } - void setTaskbar(bool withTaskbar, bool withLibraryButton) - { m_withTaskbar = withTaskbar; m_withLibraryButton = withLibraryButton; } - void setBlockExternalLinks(bool blockExternalLinks) - { m_blockExternalLinks = blockExternalLinks; } int getPort(); std::string getAddress(); protected: - Library* mp_library; - NameMapper* mp_nameMapper; - std::string m_root = ""; - std::string m_addr = ""; - std::string m_indexTemplateString = ""; - int m_port = 80; - int m_nbThreads = 1; - unsigned int m_multizimSearchLimit = 0; - bool m_verbose = false; - bool m_withTaskbar = true; - bool m_withLibraryButton = true; - bool m_blockExternalLinks = false; - int m_ipConnectionLimit = 0; + ServerConfiguration m_configuration; std::unique_ptr mp_server; }; } diff --git a/src/server.cpp b/src/server.cpp index e9373417c..fa092edb0 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -29,9 +29,8 @@ namespace kiwix { -Server::Server(Library* library, NameMapper* nameMapper) : - mp_library(library), - mp_nameMapper(nameMapper), +Server::Server(const ServerConfiguration& configuration) : + m_configuration(configuration), mp_server(nullptr) { } @@ -39,20 +38,7 @@ Server::Server(Library* library, NameMapper* nameMapper) : Server::~Server() = default; bool Server::start() { - mp_server.reset(new InternalServer( - mp_library, - mp_nameMapper, - m_addr, - m_port, - m_root, - m_nbThreads, - m_multizimSearchLimit, - m_verbose, - m_withTaskbar, - m_withLibraryButton, - m_blockExternalLinks, - m_indexTemplateString, - m_ipConnectionLimit)); + mp_server.reset(new InternalServer(m_configuration)); return mp_server->start(); } @@ -63,7 +49,7 @@ void Server::stop() { } } -void Server::setRoot(const std::string& root) +ServerConfiguration& ServerConfiguration::setRoot(const std::string& root) { m_root = root; if (m_root[0] != '/') { @@ -72,6 +58,7 @@ void Server::setRoot(const std::string& root) if (m_root.back() == '/') { m_root.erase(m_root.size() - 1); } + return *this; } int Server::getPort() diff --git a/src/server/internalServer.cpp b/src/server/internalServer.cpp index 8c449ac35..86b208bc5 100644 --- a/src/server/internalServer.cpp +++ b/src/server/internalServer.cpp @@ -276,7 +276,7 @@ std::pair InternalServer::selectBooks(const Req // Check for filtering Filter filter = get_search_filter(request, "books.filter."); - auto id_vec = mp_library->filter(filter); + auto id_vec = m_configuration.mp_library->filter(filter); if (id_vec.empty()) { throw Error(nonParameterizedMessage("no-book-found")); } @@ -288,7 +288,7 @@ std::pair InternalServer::selectBooks(const Req SearchInfo InternalServer::getSearchInfo(const RequestContext& request) const { auto bookIds = selectBooks(request); - checkBookNumber(bookIds.second, m_multizimSearchLimit); + checkBookNumber(bookIds.second, m_configuration.m_multizimSearchLimit); auto pattern = request.get_optional_param("pattern", ""); GeoQuery geoQuery; @@ -366,35 +366,14 @@ class InternalServer::CustomizedResources : public std::map("KIWIX_SEARCH_CACHE_SIZE", DEFAULT_CACHE_SIZE)), - suggestionSearcherCache(getEnvVar("KIWIX_SUGGESTION_SEARCHER_CACHE_SIZE", std::max((unsigned int) (mp_library->getBookCount(true, true)*0.1), 1U))), + suggestionSearcherCache(getEnvVar("KIWIX_SUGGESTION_SEARCHER_CACHE_SIZE", std::max((unsigned int) (m_configuration.mp_library->getBookCount(true, true)*0.1), 1U))), m_customizedResources(new CustomizedResources) {} @@ -406,13 +385,13 @@ bool InternalServer::start() { #else int flags = MHD_USE_POLL_INTERNALLY; #endif - if (m_verbose.load()) + if (m_configuration.m_verbose) flags |= MHD_USE_DEBUG; struct sockaddr_in sockAddr; memset(&sockAddr, 0, sizeof(sockAddr)); sockAddr.sin_family = AF_INET; - sockAddr.sin_port = htons(m_port); + sockAddr.sin_port = htons(m_configuration.m_port); if (m_addr.empty()) { if (0 != INADDR_ANY) { sockAddr.sin_addr.s_addr = htonl(INADDR_ANY); @@ -425,17 +404,17 @@ bool InternalServer::start() { } } mp_daemon = MHD_start_daemon(flags, - m_port, + m_configuration.m_port, NULL, NULL, &staticHandlerCallback, this, MHD_OPTION_SOCK_ADDR, &sockAddr, - MHD_OPTION_THREAD_POOL_SIZE, m_nbThreads, - MHD_OPTION_PER_IP_CONNECTION_LIMIT, m_ipConnectionLimit, + MHD_OPTION_THREAD_POOL_SIZE, m_configuration.m_nbThreads, + MHD_OPTION_PER_IP_CONNECTION_LIMIT, m_configuration.m_ipConnectionLimit, MHD_OPTION_END); if (mp_daemon == nullptr) { - std::cerr << "Unable to instantiate the HTTP daemon. The port " << m_port + std::cerr << "Unable to instantiate the HTTP daemon. The port " << m_configuration.m_port << " is maybe already occupied or need more permissions to be open. " "Please try as root or with a port number higher or equal to 1024." << std::endl; @@ -481,14 +460,14 @@ MHD_Result InternalServer::handlerCallback(struct MHD_Connection* connection, void** cont_cls) { auto start_time = std::chrono::steady_clock::now(); - if (m_verbose.load() ) { + if (m_configuration.m_verbose) { printf("======================\n"); printf("Requesting : \n"); printf("full_url : %s\n", url); } RequestContext request(connection, m_root, url, method, version); - if (m_verbose.load() ) { + if (m_configuration.m_verbose) { request.print_debug_info(); } /* Unexpected method */ @@ -504,7 +483,7 @@ MHD_Result InternalServer::handlerCallback(struct MHD_Connection* connection, if (response->getReturnCode() == MHD_HTTP_INTERNAL_SERVER_ERROR) { printf("========== INTERNAL ERROR !! ============\n"); - if (!m_verbose.load()) { + if (!m_configuration.m_verbose) { printf("Requesting : \n"); printf("full_url : %s\n", url); request.print_debug_info(); @@ -517,7 +496,7 @@ MHD_Result InternalServer::handlerCallback(struct MHD_Connection* connection, auto ret = response->send(request, connection); auto end_time = std::chrono::steady_clock::now(); auto time_span = std::chrono::duration_cast>(end_time - start_time); - if (m_verbose.load()) { + if (m_configuration.m_verbose) { printf("Request time : %fs\n", time_span.count()); printf("----------------------\n"); } @@ -650,7 +629,7 @@ class InternalServer::LockableSuggestionSearcher : public zim::SuggestionSearche std::unique_ptr InternalServer::handle_suggest(const RequestContext& request) { - if (m_verbose.load()) { + if (m_configuration.m_verbose) { printf("** running handle_suggest\n"); } @@ -664,7 +643,7 @@ std::unique_ptr InternalServer::handle_suggest(const RequestContext& r try { bookName = request.get_argument("content"); bookId = mp_nameMapper->getIdForName(bookName); - archive = mp_library->getArchiveById(bookId); + archive = m_configuration.mp_library->getArchiveById(bookId); } catch (const std::out_of_range&) { // error handled by the archive == nullptr check below } @@ -681,7 +660,7 @@ std::unique_ptr InternalServer::handle_suggest(const RequestContext& r count = 10; } - if (m_verbose.load()) { + if (m_configuration.m_verbose) { printf("Searching suggestions for: \"%s\"\n", queryString.c_str()); } @@ -734,21 +713,21 @@ std::unique_ptr InternalServer::handle_suggest(const RequestContext& r std::unique_ptr InternalServer::handle_viewer_settings(const RequestContext& request) { - if (m_verbose.load()) { + if (m_configuration.m_verbose) { printf("** running handle_viewer_settings\n"); } const kainjow::mustache::object data{ - {"enable_toolbar", m_withTaskbar ? "true" : "false" }, - {"enable_link_blocking", m_blockExternalLinks ? "true" : "false" }, - {"enable_library_button", m_withLibraryButton ? "true" : "false" } + {"enable_toolbar", m_configuration.m_withTaskbar ? "true" : "false" }, + {"enable_link_blocking", m_configuration.m_blockExternalLinks ? "true" : "false" }, + {"enable_library_button", m_configuration.m_withLibraryButton ? "true" : "false" } }; return ContentResponse::build(*this, RESOURCE::templates::viewer_settings_js, data, "application/javascript; charset=utf-8"); } std::unique_ptr InternalServer::handle_skin(const RequestContext& request) { - if (m_verbose.load()) { + if (m_configuration.m_verbose) { printf("** running handle_skin\n"); } @@ -771,7 +750,7 @@ std::unique_ptr InternalServer::handle_skin(const RequestContext& requ std::unique_ptr InternalServer::handle_search(const RequestContext& request) { - if (m_verbose.load()) { + if (m_configuration.m_verbose) { printf("** running handle_search\n"); } @@ -793,14 +772,14 @@ std::unique_ptr InternalServer::handle_search(const RequestContext& re /* Make the search */ // Try to get a search from the searchInfo, else build it - auto searcher = mp_library->getSearcherByIds(bookIds); + auto searcher = m_configuration.mp_library->getSearcherByIds(bookIds); auto lock(searcher->getLock()); std::shared_ptr search; try { search = searchCache.getOrPut(searchInfo, [=](){ - return make_shared(searcher->search(searchInfo.getZimQuery(m_verbose.load()))); + return make_shared(searcher->search(searchInfo.getZimQuery(m_configuration.m_verbose))); } ); } catch(std::runtime_error& e) { @@ -818,7 +797,7 @@ std::unique_ptr InternalServer::handle_search(const RequestContext& re if(bookIds.size() == 1) { auto bookId = *bookIds.begin(); auto bookName = mp_nameMapper->getNameForId(bookId); - response += TaskbarInfo(bookName, mp_library->getArchiveById(bookId).get()); + response += TaskbarInfo(bookName, m_configuration.mp_library->getArchiveById(bookId).get()); } */ return response; @@ -842,7 +821,7 @@ std::unique_ptr InternalServer::handle_search(const RequestContext& re } /* Get the results */ - SearchRenderer renderer(search->getResults(start-1, pageLength), mp_nameMapper, mp_library, start, + SearchRenderer renderer(search->getResults(start-1, pageLength), mp_nameMapper, m_configuration.mp_library, start, search->getEstimatedMatches()); renderer.setSearchPattern(searchInfo.pattern); renderer.setSearchBookQuery(searchInfo.bookFilterQuery); @@ -859,7 +838,7 @@ std::unique_ptr InternalServer::handle_search(const RequestContext& re if(bookIds.size() == 1) { auto bookId = *bookIds.begin(); auto bookName = mp_nameMapper->getNameForId(bookId); - response->set_taskbar(bookName, mp_library->getArchiveById(bookId).get()); + response->set_taskbar(bookName, m_configuration.mp_library->getArchiveById(bookId).get()); } */ return std::move(response); @@ -872,7 +851,7 @@ std::unique_ptr InternalServer::handle_search(const RequestContext& re std::unique_ptr InternalServer::handle_random(const RequestContext& request) { - if (m_verbose.load()) { + if (m_configuration.m_verbose) { printf("** running handle_random\n"); } @@ -886,7 +865,7 @@ std::unique_ptr InternalServer::handle_random(const RequestContext& re try { bookName = request.get_argument("content"); const std::string bookId = mp_nameMapper->getIdForName(bookName); - archive = mp_library->getArchiveById(bookId); + archive = m_configuration.mp_library->getArchiveById(bookId); } catch (const std::out_of_range&) { // error handled by the archive == nullptr check below } @@ -924,7 +903,7 @@ std::unique_ptr InternalServer::handle_captured_external(const Request std::unique_ptr InternalServer::handle_catch(const RequestContext& request) { - if (m_verbose.load()) { + if (m_configuration.m_verbose) { printf("** running handle_catch\n"); } @@ -938,7 +917,7 @@ std::unique_ptr InternalServer::handle_catch(const RequestContext& req std::unique_ptr InternalServer::handle_catalog(const RequestContext& request) { - if (m_verbose.load()) { + if (m_configuration.m_verbose) { printf("** running handle_catalog"); } @@ -967,13 +946,13 @@ std::unique_ptr InternalServer::handle_catalog(const RequestContext& r } zim::Uuid uuid; - kiwix::OPDSDumper opdsDumper(mp_library, mp_nameMapper); + kiwix::OPDSDumper opdsDumper(m_configuration.mp_library, mp_nameMapper); opdsDumper.setRootLocation(m_root); opdsDumper.setLibraryId(m_library_id); std::vector bookIdsToDump; if (url == "root.xml") { uuid = zim::Uuid::generate(host); - bookIdsToDump = mp_library->filter(kiwix::Filter().valid(true).local(true).remote(true)); + bookIdsToDump = m_configuration.mp_library->filter(kiwix::Filter().valid(true).local(true).remote(true)); } else if (url == "search") { bookIdsToDump = search_catalog(request, opdsDumper); uuid = zim::Uuid::generate(); @@ -994,7 +973,7 @@ InternalServer::search_catalog(const RequestContext& request, const std::string q = filter.hasQuery() ? filter.getQuery() : ""; - std::vector bookIdsToDump = mp_library->filter(filter); + std::vector bookIdsToDump = m_configuration.mp_library->filter(filter); const auto totalResults = bookIdsToDump.size(); const size_t count = request.get_optional_param("count", 10UL); const size_t startIndex = request.get_optional_param("start", 0UL); @@ -1030,7 +1009,7 @@ std::unique_ptr InternalServer::handle_content(const RequestContext& r { const std::string url = request.get_url(); const std::string pattern = url.substr((url.find_last_of('/'))+1); - if (m_verbose.load()) { + if (m_configuration.m_verbose) { printf("** running handle_content\n"); } @@ -1042,7 +1021,7 @@ std::unique_ptr InternalServer::handle_content(const RequestContext& r std::shared_ptr archive; try { const std::string bookId = mp_nameMapper->getIdForName(bookName); - archive = mp_library->getArchiveById(bookId); + archive = m_configuration.mp_library->getArchiveById(bookId); } catch (const std::out_of_range& e) {} if (archive == nullptr) { @@ -1071,14 +1050,14 @@ std::unique_ptr InternalServer::handle_content(const RequestContext& r } auto response = ItemResponse::build(*this, request, entry.getItem()); - if (m_verbose.load()) { + if (m_configuration.m_verbose) { printf("Found %s\n", entry.getPath().c_str()); printf("mimeType: %s\n", entry.getItem(true).getMimetype().c_str()); } return response; } catch(zim::EntryNotFound& e) { - if (m_verbose.load()) + if (m_configuration.m_verbose) printf("Failed to find %s\n", urlStr.c_str()); std::string searchURL = m_root + "/search?content=" + bookName + "&pattern=" + kiwix::urlEncode(pattern, true); @@ -1091,7 +1070,7 @@ std::unique_ptr InternalServer::handle_content(const RequestContext& r std::unique_ptr InternalServer::handle_raw(const RequestContext& request) { - if (m_verbose.load()) { + if (m_configuration.m_verbose) { printf("** running handle_raw\n"); } @@ -1114,7 +1093,7 @@ std::unique_ptr InternalServer::handle_raw(const RequestContext& reque std::shared_ptr archive; try { const std::string bookId = mp_nameMapper->getIdForName(bookName); - archive = mp_library->getArchiveById(bookId); + archive = m_configuration.mp_library->getArchiveById(bookId); } catch (const std::out_of_range& e) {} if (archive == nullptr) { @@ -1141,7 +1120,7 @@ std::unique_ptr InternalServer::handle_raw(const RequestContext& reque return ItemResponse::build(*this, request, entry.getItem()); } } catch (zim::EntryNotFound& e ) { - if (m_verbose.load()) { + if (m_configuration.m_verbose) { printf("Failed to find %s\n", itemPath.c_str()); } return HTTP404Response(*this, request) @@ -1157,13 +1136,13 @@ bool InternalServer::isLocallyCustomizedResource(const std::string& url) const std::unique_ptr InternalServer::handle_locally_customized_resource(const RequestContext& request) { - if (m_verbose.load()) { + if (m_configuration.m_verbose) { printf("** running handle_locally_customized_resource\n"); } const CustomizedResourceData& crd = m_customizedResources->at(request.get_url()); - if (m_verbose.load()) { + if (m_configuration.m_verbose) { std::cout << "Reading " << crd.resourceFilePath << std::endl; } const auto resourceData = getFileContent(crd.resourceFilePath); diff --git a/src/server/internalServer.h b/src/server/internalServer.h index 6f523336e..33817c520 100644 --- a/src/server/internalServer.h +++ b/src/server/internalServer.h @@ -27,6 +27,7 @@ extern "C" { #include "library.h" #include "name_mapper.h" +#include "server.h" #include #include @@ -92,19 +93,7 @@ class OPDSDumper; class InternalServer { public: - InternalServer(Library* library, - NameMapper* nameMapper, - std::string addr, - int port, - std::string root, - int nbThreads, - unsigned int multizimSearchLimit, - bool verbose, - bool withTaskbar, - bool withLibraryButton, - bool blockExternalLinks, - std::string indexTemplateString, - int ipConnectionLimit); + InternalServer(const ServerConfiguration& configuration); virtual ~InternalServer(); MHD_Result handlerCallback(struct MHD_Connection* connection, @@ -117,7 +106,7 @@ class InternalServer { bool start(); void stop(); std::string getAddress() { return m_addr; } - int getPort() { return m_port; } + int getPort() { return m_configuration.m_port; } private: // functions std::unique_ptr handle_request(const RequestContext& request); @@ -160,21 +149,12 @@ class InternalServer { typedef ConcurrentCache> SuggestionSearcherCache; private: // data + ServerConfiguration m_configuration; std::string m_addr; - int m_port; std::string m_root; - int m_nbThreads; - unsigned int m_multizimSearchLimit; - std::atomic_bool m_verbose; - bool m_withTaskbar; - bool m_withLibraryButton; - bool m_blockExternalLinks; std::string m_indexTemplateString; - int m_ipConnectionLimit; - struct MHD_Daemon* mp_daemon; - - Library* mp_library; NameMapper* mp_nameMapper; + struct MHD_Daemon* mp_daemon; SearchCache searchCache; SuggestionSearcherCache suggestionSearcherCache; diff --git a/src/server/internalServer_catalog_v2.cpp b/src/server/internalServer_catalog_v2.cpp index 93b400a4f..a8122c828 100644 --- a/src/server/internalServer_catalog_v2.cpp +++ b/src/server/internalServer_catalog_v2.cpp @@ -35,7 +35,7 @@ namespace kiwix { std::unique_ptr InternalServer::handle_catalog_v2(const RequestContext& request) { - if (m_verbose.load()) { + if (m_configuration.m_verbose) { printf("** running handle_catalog_v2"); } @@ -50,7 +50,7 @@ std::unique_ptr InternalServer::handle_catalog_v2(const RequestContext if (url == "root.xml") { return handle_catalog_v2_root(request); } else if (url == "searchdescription.xml") { - const std::string endpoint_root = m_root + "/catalog/v2"; + const std::string endpoint_root = m_configuration.m_root + "/catalog/v2"; return ContentResponse::build(*this, RESOURCE::catalog_v2_searchdescription_xml, kainjow::mustache::object({{"endpoint_root", endpoint_root}}), @@ -82,7 +82,7 @@ std::unique_ptr InternalServer::handle_catalog_v2_root(const RequestCo RESOURCE::templates::catalog_v2_root_xml, kainjow::mustache::object{ {"date", gen_date_str()}, - {"endpoint_root", m_root + "/catalog/v2"}, + {"endpoint_root", m_configuration.m_root + "/catalog/v2"}, {"feed_id", gen_uuid(m_library_id)}, {"all_entries_feed_id", gen_uuid(m_library_id + "/entries")}, {"partial_entries_feed_id", gen_uuid(m_library_id + "/partial_entries")}, @@ -95,8 +95,8 @@ std::unique_ptr InternalServer::handle_catalog_v2_root(const RequestCo std::unique_ptr InternalServer::handle_catalog_v2_entries(const RequestContext& request, bool partial) { - OPDSDumper opdsDumper(mp_library, mp_nameMapper); - opdsDumper.setRootLocation(m_root); + OPDSDumper opdsDumper(m_configuration.mp_library, m_configuration.mp_nameMapper); + opdsDumper.setRootLocation(m_configuration.m_root); opdsDumper.setLibraryId(m_library_id); const auto bookIds = search_catalog(request, opdsDumper); const auto opdsFeed = opdsDumper.dumpOPDSFeedV2(bookIds, request.get_query(), partial); @@ -110,14 +110,14 @@ std::unique_ptr InternalServer::handle_catalog_v2_entries(const Reques std::unique_ptr InternalServer::handle_catalog_v2_complete_entry(const RequestContext& request, const std::string& entryId) { try { - mp_library->getBookById(entryId); + m_configuration.mp_library->getBookById(entryId); } catch (const std::out_of_range&) { return HTTP404Response(*this, request) + urlNotFoundMsg; } - OPDSDumper opdsDumper(mp_library, mp_nameMapper); - opdsDumper.setRootLocation(m_root); + OPDSDumper opdsDumper(m_configuration.mp_library, m_configuration.mp_nameMapper); + opdsDumper.setRootLocation(m_configuration.m_root); opdsDumper.setLibraryId(m_library_id); const auto opdsFeed = opdsDumper.dumpOPDSCompleteEntry(entryId); return ContentResponse::build( @@ -129,8 +129,8 @@ std::unique_ptr InternalServer::handle_catalog_v2_complete_entry(const std::unique_ptr InternalServer::handle_catalog_v2_categories(const RequestContext& request) { - OPDSDumper opdsDumper(mp_library, mp_nameMapper); - opdsDumper.setRootLocation(m_root); + OPDSDumper opdsDumper(m_configuration.mp_library, m_configuration.mp_nameMapper); + opdsDumper.setRootLocation(m_configuration.m_root); opdsDumper.setLibraryId(m_library_id); return ContentResponse::build( *this, @@ -141,8 +141,8 @@ std::unique_ptr InternalServer::handle_catalog_v2_categories(const Req std::unique_ptr InternalServer::handle_catalog_v2_languages(const RequestContext& request) { - OPDSDumper opdsDumper(mp_library, mp_nameMapper); - opdsDumper.setRootLocation(m_root); + OPDSDumper opdsDumper(m_configuration.mp_library, m_configuration.mp_nameMapper); + opdsDumper.setRootLocation(m_configuration.m_root); opdsDumper.setLibraryId(m_library_id); return ContentResponse::build( *this, @@ -155,7 +155,7 @@ std::unique_ptr InternalServer::handle_catalog_v2_illustration(const R { try { const auto bookId = request.get_url_part(3); - auto book = mp_library->getBookByIdThreadSafe(bookId); + auto book = m_configuration.mp_library->getBookByIdThreadSafe(bookId); auto size = request.get_argument("size"); auto illustration = book.getIllustration(size); return ContentResponse::build( diff --git a/src/server/response.cpp b/src/server/response.cpp index 7067ff207..064212353 100644 --- a/src/server/response.cpp +++ b/src/server/response.cpp @@ -114,7 +114,7 @@ Response::Response(bool verbose) std::unique_ptr Response::build(const InternalServer& server) { - return std::unique_ptr(new Response(server.m_verbose.load())); + return std::unique_ptr(new Response(server.m_configuration.m_verbose)); } std::unique_ptr Response::build_304(const InternalServer& server, const ETag& etag) @@ -389,8 +389,8 @@ std::unique_ptr ContentResponse::build( const std::string& mimetype) { return std::unique_ptr(new ContentResponse( - server.m_root, - server.m_verbose.load(), + server.m_configuration.m_root, + server.m_configuration.m_verbose, content, mimetype)); } @@ -435,7 +435,7 @@ std::unique_ptr ItemResponse::build(const InternalServer& server, cons } return std::unique_ptr(new ItemResponse( - server.m_verbose.load(), + server.m_configuration.m_verbose, item, mimetype, byteRange)); diff --git a/test/server_testing_tools.h b/test/server_testing_tools.h index 7af5834e6..77a9da64c 100644 --- a/test/server_testing_tools.h +++ b/test/server_testing_tools.h @@ -130,18 +130,20 @@ void ZimFileServer::run(int serverPort, std::string indexTemplateString) } else { nameMapper.reset(new kiwix::HumanReadableNameMapper(library, false)); } - server.reset(new kiwix::Server(&library, nameMapper.get())); - server->setRoot("ROOT"); - server->setAddress(address); - server->setPort(serverPort); - server->setNbThreads(2); - server->setVerbose(false); - server->setTaskbar(options & WITH_TASKBAR, options & WITH_LIBRARY_BUTTON); - server->setBlockExternalLinks(options & BLOCK_EXTERNAL_LINKS); - server->setMultiZimSearchLimit(3); + kiwix::ServerConfiguration configuration(&library, nameMapper.get()); + configuration.setRoot("ROOT") + .setAddress(address) + .setPort(serverPort) + .setNbThreads(2) + .setVerbose(false) + .setTaskbar(options & WITH_TASKBAR, options & WITH_LIBRARY_BUTTON) + .setBlockExternalLinks(options & BLOCK_EXTERNAL_LINKS) + .setMultiZimSearchLimit(3); + if (!indexTemplateString.empty()) { - server->setIndexTemplateString(indexTemplateString); + configuration.setIndexTemplateString(indexTemplateString); } + server.reset(new kiwix::Server(configuration)); if ( !server->start() ) throw std::runtime_error("ZimFileServer failed to start"); From 5896691b31146458370446f70e136598ad880623 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Wed, 12 Oct 2022 15:48:56 +0200 Subject: [PATCH 05/16] fixup! Introduce a ServerConfiguration object. Move from `ServerConfiguration` to `Server::Configuration`. --- include/server.h | 145 +++++++++++++++++----------------- src/server.cpp | 4 +- src/server/internalServer.cpp | 2 +- src/server/internalServer.h | 4 +- test/server_testing_tools.h | 2 +- 5 files changed, 78 insertions(+), 79 deletions(-) diff --git a/include/server.h b/include/server.h index c9d2dcbc8..1c98d19db 100644 --- a/include/server.h +++ b/include/server.h @@ -29,85 +29,84 @@ namespace kiwix class NameMapper; class InternalServer; - class ServerConfiguration { - public: - ServerConfiguration(Library* library, NameMapper* nameMapper=nullptr) - : mp_library(library), - mp_nameMapper(nameMapper) - {} - - ServerConfiguration& setRoot(const std::string& root); - ServerConfiguration& setAddress(const std::string& addr) { - m_addr = addr; - return *this; - } - - ServerConfiguration& setPort(int port) { - m_port = port; - return *this; - } - - ServerConfiguration& setNbThreads(int threads) { - m_nbThreads = threads; - return *this; - } - - ServerConfiguration& setMultiZimSearchLimit(unsigned int limit) { - m_multizimSearchLimit = limit; - return *this; - } - - ServerConfiguration& setIpConnectionLimit(int limit) { - m_ipConnectionLimit = limit; - return *this; - } - - ServerConfiguration& setVerbose(bool verbose) { - m_verbose = verbose; - return *this; - } - - ServerConfiguration& setIndexTemplateString(const std::string& indexTemplateString) { - m_indexTemplateString = indexTemplateString; - return *this; - } - - ServerConfiguration& setTaskbar(bool withTaskbar, bool withLibraryButton) { - m_withTaskbar = withTaskbar; - m_withLibraryButton = withLibraryButton; - return *this; - } - - ServerConfiguration& setBlockExternalLinks(bool blockExternalLinks) { - m_blockExternalLinks = blockExternalLinks; - return *this; - } - - Library* mp_library; - NameMapper* mp_nameMapper; - std::string m_root = ""; - std::string m_addr = ""; - std::string m_indexTemplateString = ""; - int m_port = 80; - int m_nbThreads = 1; - unsigned int m_multizimSearchLimit = 0; - bool m_verbose = false; - bool m_withTaskbar = true; - bool m_withLibraryButton = true; - bool m_blockExternalLinks = false; - int m_ipConnectionLimit = 0; - }; - - class Server { - public: + public: + class Configuration { + public: + explicit Configuration(Library* library, NameMapper* nameMapper=nullptr) + : mp_library(library), + mp_nameMapper(nameMapper) + {} + + Configuration& setRoot(const std::string& root); + Configuration& setAddress(const std::string& addr) { + m_addr = addr; + return *this; + } + + Configuration& setPort(int port) { + m_port = port; + return *this; + } + + Configuration& setNbThreads(int threads) { + m_nbThreads = threads; + return *this; + } + + Configuration& setMultiZimSearchLimit(unsigned int limit) { + m_multizimSearchLimit = limit; + return *this; + } + + Configuration& setIpConnectionLimit(int limit) { + m_ipConnectionLimit = limit; + return *this; + } + + Configuration& setVerbose(bool verbose) { + m_verbose = verbose; + return *this; + } + + Configuration& setIndexTemplateString(const std::string& indexTemplateString) { + m_indexTemplateString = indexTemplateString; + return *this; + } + + Configuration& setTaskbar(bool withTaskbar, bool withLibraryButton) { + m_withTaskbar = withTaskbar; + m_withLibraryButton = withLibraryButton; + return *this; + } + + Configuration& setBlockExternalLinks(bool blockExternalLinks) { + m_blockExternalLinks = blockExternalLinks; + return *this; + } + + Library* mp_library; + NameMapper* mp_nameMapper; + std::string m_root = ""; + std::string m_addr = ""; + std::string m_indexTemplateString = ""; + int m_port = 80; + int m_nbThreads = 1; + unsigned int m_multizimSearchLimit = 0; + bool m_verbose = false; + bool m_withTaskbar = true; + bool m_withLibraryButton = true; + bool m_blockExternalLinks = false; + int m_ipConnectionLimit = 0; + }; + /** * The default constructor. * * @param library The library to serve. */ - Server(const ServerConfiguration& configuration); + explicit Server(const Configuration& configuration); virtual ~Server(); @@ -125,7 +124,7 @@ namespace kiwix std::string getAddress(); protected: - ServerConfiguration m_configuration; + Configuration m_configuration; std::unique_ptr mp_server; }; } diff --git a/src/server.cpp b/src/server.cpp index fa092edb0..699301ade 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -29,7 +29,7 @@ namespace kiwix { -Server::Server(const ServerConfiguration& configuration) : +Server::Server(const Server::Configuration& configuration) : m_configuration(configuration), mp_server(nullptr) { @@ -49,7 +49,7 @@ void Server::stop() { } } -ServerConfiguration& ServerConfiguration::setRoot(const std::string& root) +Server::Configuration& Server::Configuration::setRoot(const std::string& root) { m_root = root; if (m_root[0] != '/') { diff --git a/src/server/internalServer.cpp b/src/server/internalServer.cpp index 86b208bc5..ecb3722b4 100644 --- a/src/server/internalServer.cpp +++ b/src/server/internalServer.cpp @@ -366,7 +366,7 @@ class InternalServer::CustomizedResources : public std::map> SuggestionSearcherCache; private: // data - ServerConfiguration m_configuration; + Server::Configuration m_configuration; std::string m_addr; std::string m_root; std::string m_indexTemplateString; diff --git a/test/server_testing_tools.h b/test/server_testing_tools.h index 77a9da64c..db79d5dae 100644 --- a/test/server_testing_tools.h +++ b/test/server_testing_tools.h @@ -130,7 +130,7 @@ void ZimFileServer::run(int serverPort, std::string indexTemplateString) } else { nameMapper.reset(new kiwix::HumanReadableNameMapper(library, false)); } - kiwix::ServerConfiguration configuration(&library, nameMapper.get()); + kiwix::Server::Configuration configuration(&library, nameMapper.get()); configuration.setRoot("ROOT") .setAddress(address) .setPort(serverPort) From 0bd5a5ec3b1583e990c21ebc01105caec5ceafc0 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Thu, 6 Oct 2022 12:00:27 +0200 Subject: [PATCH 06/16] Do not store raw pointer to Library. While it was "ok" to store raw pointer as, in our use case, the library always live longer than object using it; it is not safe to store raw pointer on object than may be deleted. All classes storing a library now store a shared_ptr. Functionr only using the library (as `HumanReadableNameMapper`) continue to use a (const) reference. --- include/manager.h | 8 +-- include/name_mapper.h | 6 +-- include/opds_dumper.h | 4 +- include/search_renderer.h | 4 +- include/server.h | 4 +- src/manager.cpp | 12 ++--- src/name_mapper.cpp | 6 +-- src/opds_dumper.cpp | 10 ++-- src/search_renderer.cpp | 2 +- test/library.cpp | 102 +++++++++++++++++++----------------- test/manager.cpp | 26 ++++----- test/name_mapper.cpp | 27 ++++++---- test/server_testing_tools.h | 16 +++--- 13 files changed, 119 insertions(+), 108 deletions(-) diff --git a/include/manager.h b/include/manager.h index d0addad70..91dfca5e9 100644 --- a/include/manager.h +++ b/include/manager.h @@ -37,10 +37,10 @@ namespace kiwix class LibraryManipulator { public: // functions - explicit LibraryManipulator(Library* library); + explicit LibraryManipulator(std::shared_ptr library); virtual ~LibraryManipulator(); - Library& getLibrary() const { return library; } + Library& getLibrary() const { return *library.get(); } bool addBookToLibrary(const Book& book); void addBookmarkToLibrary(const Bookmark& bookmark); @@ -52,7 +52,7 @@ class LibraryManipulator virtual void booksWereRemovedFromLibrary(); private: // data - kiwix::Library& library; + std::shared_ptr library; }; /** @@ -65,7 +65,7 @@ class Manager public: // functions explicit Manager(LibraryManipulator* manipulator); - explicit Manager(Library* library); + explicit Manager(std::shared_ptr library); /** * Read a `library.xml` and add book in the file to the library. diff --git a/include/name_mapper.h b/include/name_mapper.h index 4247c5c3c..13a428133 100644 --- a/include/name_mapper.h +++ b/include/name_mapper.h @@ -50,7 +50,7 @@ class HumanReadableNameMapper : public NameMapper { std::map m_nameToId; public: - HumanReadableNameMapper(kiwix::Library& library, bool withAlias); + HumanReadableNameMapper(const kiwix::Library& library, bool withAlias); virtual ~HumanReadableNameMapper() = default; virtual std::string getNameForId(const std::string& id) const; virtual std::string getIdForName(const std::string& name) const; @@ -59,7 +59,7 @@ class HumanReadableNameMapper : public NameMapper { class UpdatableNameMapper : public NameMapper { typedef std::shared_ptr NameMapperHandle; public: - UpdatableNameMapper(Library& library, bool withAlias); + UpdatableNameMapper(std::shared_ptr library, bool withAlias); virtual std::string getNameForId(const std::string& id) const; virtual std::string getIdForName(const std::string& name) const; @@ -71,7 +71,7 @@ class UpdatableNameMapper : public NameMapper { private: mutable std::mutex mutex; - Library& library; + std::shared_ptr library; NameMapperHandle nameMapper; const bool withAlias; }; diff --git a/include/opds_dumper.h b/include/opds_dumper.h index cb6566f4f..23cc94ae2 100644 --- a/include/opds_dumper.h +++ b/include/opds_dumper.h @@ -42,7 +42,7 @@ class OPDSDumper { public: OPDSDumper() = default; - OPDSDumper(Library* library, NameMapper* NameMapper); + OPDSDumper(std::shared_ptr library, NameMapper* NameMapper); ~OPDSDumper(); /** @@ -110,7 +110,7 @@ class OPDSDumper void setOpenSearchInfo(int totalResult, int startIndex, int count); protected: - kiwix::Library* library; + std::shared_ptr library; kiwix::NameMapper* nameMapper; std::string libraryId; std::string rootLocation; diff --git a/include/search_renderer.h b/include/search_renderer.h index 0190ee5f0..fec854a1d 100644 --- a/include/search_renderer.h +++ b/include/search_renderer.h @@ -58,7 +58,7 @@ class SearchRenderer * @param start The start offset used for the srs. * @param estimatedResultCount The estimatedResultCount of the whole search */ - SearchRenderer(zim::SearchResultSet srs, NameMapper* mapper, Library* library, + SearchRenderer(zim::SearchResultSet srs, NameMapper* mapper, std::shared_ptr library, unsigned int start, unsigned int estimatedResultCount); ~SearchRenderer(); @@ -107,7 +107,7 @@ class SearchRenderer std::string beautifyInteger(const unsigned int number); zim::SearchResultSet m_srs; NameMapper* mp_nameMapper; - Library* mp_library; + std::shared_ptr mp_library; std::string searchBookQuery; std::string searchPattern; std::string protocolPrefix; diff --git a/include/server.h b/include/server.h index 1c98d19db..39cacc994 100644 --- a/include/server.h +++ b/include/server.h @@ -34,7 +34,7 @@ namespace kiwix public: class Configuration { public: - explicit Configuration(Library* library, NameMapper* nameMapper=nullptr) + explicit Configuration(std::shared_ptr library, NameMapper* nameMapper=nullptr) : mp_library(library), mp_nameMapper(nameMapper) {} @@ -86,7 +86,7 @@ namespace kiwix return *this; } - Library* mp_library; + std::shared_ptr mp_library; NameMapper* mp_nameMapper; std::string m_root = ""; std::string m_addr = ""; diff --git a/src/manager.cpp b/src/manager.cpp index 3f24fad17..4feb0a96e 100644 --- a/src/manager.cpp +++ b/src/manager.cpp @@ -41,8 +41,8 @@ struct NoDelete // LibraryManipulator //////////////////////////////////////////////////////////////////////////////// -LibraryManipulator::LibraryManipulator(Library* library) - : library(*library) +LibraryManipulator::LibraryManipulator(std::shared_ptr library) + : library(library) {} LibraryManipulator::~LibraryManipulator() @@ -50,7 +50,7 @@ LibraryManipulator::~LibraryManipulator() bool LibraryManipulator::addBookToLibrary(const Book& book) { - const auto ret = library.addBook(book); + const auto ret = library->addBook(book); if ( ret ) { bookWasAddedToLibrary(book); } @@ -59,13 +59,13 @@ bool LibraryManipulator::addBookToLibrary(const Book& book) void LibraryManipulator::addBookmarkToLibrary(const Bookmark& bookmark) { - library.addBookmark(bookmark); + library->addBookmark(bookmark); bookmarkWasAddedToLibrary(bookmark); } uint32_t LibraryManipulator::removeBooksNotUpdatedSince(Library::Revision rev) { - const auto n = library.removeBooksNotUpdatedSince(rev); + const auto n = library->removeBooksNotUpdatedSince(rev); if ( n != 0 ) { booksWereRemovedFromLibrary(); } @@ -95,7 +95,7 @@ Manager::Manager(LibraryManipulator* manipulator): { } -Manager::Manager(Library* library) : +Manager::Manager(std::shared_ptr library) : writableLibraryPath(""), manipulator(new LibraryManipulator(library)) { diff --git a/src/name_mapper.cpp b/src/name_mapper.cpp index dccf40c9b..17c1cf23f 100644 --- a/src/name_mapper.cpp +++ b/src/name_mapper.cpp @@ -24,7 +24,7 @@ namespace kiwix { -HumanReadableNameMapper::HumanReadableNameMapper(kiwix::Library& library, bool withAlias) { +HumanReadableNameMapper::HumanReadableNameMapper(const kiwix::Library& library, bool withAlias) { for (auto& bookId: library.filter(kiwix::Filter().local(true).valid(true))) { auto& currentBook = library.getBookById(bookId); auto bookName = currentBook.getHumanReadableIdFromPath(); @@ -63,7 +63,7 @@ std::string HumanReadableNameMapper::getIdForName(const std::string& name) const // UpdatableNameMapper //////////////////////////////////////////////////////////////////////////////// -UpdatableNameMapper::UpdatableNameMapper(Library& lib, bool withAlias) +UpdatableNameMapper::UpdatableNameMapper(std::shared_ptr lib, bool withAlias) : library(lib) , withAlias(withAlias) { @@ -72,7 +72,7 @@ UpdatableNameMapper::UpdatableNameMapper(Library& lib, bool withAlias) void UpdatableNameMapper::update() { - const auto newNameMapper = new HumanReadableNameMapper(library, withAlias); + const auto newNameMapper = new HumanReadableNameMapper(*library, withAlias); std::lock_guard lock(mutex); nameMapper.reset(newNameMapper); } diff --git a/src/opds_dumper.cpp b/src/opds_dumper.cpp index 702f0a4f8..456b260b3 100644 --- a/src/opds_dumper.cpp +++ b/src/opds_dumper.cpp @@ -30,7 +30,7 @@ namespace kiwix { /* Constructor */ -OPDSDumper::OPDSDumper(Library* library, NameMapper* nameMapper) +OPDSDumper::OPDSDumper(std::shared_ptr library, NameMapper* nameMapper) : library(library), nameMapper(nameMapper) { @@ -113,12 +113,12 @@ std::string partialEntryXML(const Book& book, const std::string& rootLocation) return render_template(xmlTemplate, data); } -BooksData getBooksData(const Library* library, const NameMapper& nameMapper, const std::vector& bookIds, const std::string& rootLocation, bool partial) +BooksData getBooksData(const Library& library, const NameMapper& nameMapper, const std::vector& bookIds, const std::string& rootLocation, bool partial) { BooksData booksData; for ( const auto& bookId : bookIds ) { try { - const Book book = library->getBookByIdThreadSafe(bookId); + const Book book = library.getBookByIdThreadSafe(bookId); const std::string bookName = nameMapper.getNameForId(bookId); const auto entryXML = partial ? partialEntryXML(book, rootLocation) @@ -190,7 +190,7 @@ std::string getLanguageSelfName(const std::string& lang) { string OPDSDumper::dumpOPDSFeed(const std::vector& bookIds, const std::string& query) const { - const auto booksData = getBooksData(library, *nameMapper, bookIds, rootLocation, false); + const auto booksData = getBooksData(*library, *nameMapper, bookIds, rootLocation, false); const kainjow::mustache::object template_data{ {"date", gen_date_str()}, {"root", rootLocation}, @@ -208,7 +208,7 @@ string OPDSDumper::dumpOPDSFeed(const std::vector& bookIds, const s string OPDSDumper::dumpOPDSFeedV2(const std::vector& bookIds, const std::string& query, bool partial) const { const auto endpointRoot = rootLocation + "/catalog/v2"; - const auto booksData = getBooksData(library, *nameMapper, bookIds, rootLocation, partial); + const auto booksData = getBooksData(*library, *nameMapper, bookIds, rootLocation, partial); const char* const endpoint = partial ? "/partial_entries" : "/entries"; const kainjow::mustache::object template_data{ diff --git a/src/search_renderer.cpp b/src/search_renderer.cpp index 7b293f628..a8f2e31cb 100644 --- a/src/search_renderer.cpp +++ b/src/search_renderer.cpp @@ -41,7 +41,7 @@ SearchRenderer::SearchRenderer(zim::SearchResultSet srs, NameMapper* mapper, : SearchRenderer(srs, mapper, nullptr, start, estimatedResultCount) {} -SearchRenderer::SearchRenderer(zim::SearchResultSet srs, NameMapper* mapper, Library* library, +SearchRenderer::SearchRenderer(zim::SearchResultSet srs, NameMapper* mapper, std::shared_ptr library, unsigned int start, unsigned int estimatedResultCount) : m_srs(srs), mp_nameMapper(mapper), diff --git a/test/library.cpp b/test/library.cpp index ef41c4d7c..f119119ef 100644 --- a/test/library.cpp +++ b/test/library.cpp @@ -236,14 +236,14 @@ namespace TEST(LibraryOpdsImportTest, allInOne) { - kiwix::Library lib; - kiwix::Manager manager(&lib); + auto lib = std::make_shared(); + kiwix::Manager manager(lib); 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"); @@ -268,7 +268,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(), ""); @@ -297,8 +297,12 @@ class LibraryTest : public ::testing::Test { typedef kiwix::Library::BookIdCollection BookIdCollection; typedef std::vector TitleCollection; + explicit LibraryTest() + : lib(std::make_shared()) + {} + void SetUp() override { - kiwix::Manager manager(&lib); + kiwix::Manager manager(lib); manager.readOpds(sampleOpdsStream, "foo.urlHost"); manager.readXml(sampleLibraryXML, false, "./test/library.xml", true); } @@ -312,25 +316,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); @@ -342,11 +346,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"}) ); - EXPECT_EQ(lib.getBooksCreators(), std::vector({ + EXPECT_EQ(lib->getBooksCreators(), std::vector({ "Islam Stack Exchange", "Movies & TV Stack Exchange", "Mythology & Folklore Stack Exchange", @@ -357,7 +361,7 @@ TEST_F(LibraryTest, sanityCheck) "Wikipedia", "Wikiquote" })); - EXPECT_EQ(lib.getBooksPublishers(), std::vector({ + EXPECT_EQ(lib->getBooksPublishers(), std::vector({ "", "Kiwix", "Kiwix & Some Enthusiasts", @@ -367,22 +371,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__ }) \ ) @@ -732,33 +736,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) @@ -766,17 +770,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) @@ -796,12 +800,12 @@ 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_EQ(9u, lib.removeBooksNotUpdatedSince(rev)); + EXPECT_EQ(9u, lib->removeBooksNotUpdatedSince(rev)); EXPECT_FILTER_RESULTS(kiwix::Filter(), "Islam Stack Exchange", diff --git a/test/manager.cpp b/test/manager.cpp index 34774f630..41f2e3287 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 = std::make_shared(); + kiwix::Manager manager = kiwix::Manager(lib); 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 = std::make_shared(); + kiwix::Manager manager = kiwix::Manager(lib); 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 = std::make_shared(); + kiwix::Manager manager(lib); 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..b49c8bfa7 100644 --- a/test/name_mapper.cpp +++ b/test/name_mapper.cpp @@ -18,18 +18,23 @@ const char libraryXML[] = R"( )"; class NameMapperTest : public ::testing::Test { + public: + explicit NameMapperTest() : + lib(std::make_shared()) + {} + protected: void SetUp() override { - kiwix::Manager manager(&lib); + kiwix::Manager manager(lib); 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 +78,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 +93,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 +104,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")); @@ -114,7 +119,7 @@ TEST_F(NameMapperTest, UpdatableNameMapperWithoutAliases) 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); @@ -137,7 +142,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 db79d5dae..600fb6e4b 100644 --- a/test/server_testing_tools.h +++ b/test/server_testing_tools.h @@ -90,7 +90,7 @@ class ZimFileServer 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; @@ -99,8 +99,9 @@ class ZimFileServer }; ZimFileServer::ZimFileServer(int serverPort, Options _options, std::string libraryFilePath) -: manager(&this->library) -, options(_options) +: library(new kiwix::Library()), + manager(library), + options(_options) { if ( kiwix::isRelativePath(libraryFilePath) ) libraryFilePath = kiwix::computeAbsolutePath(kiwix::getCurrentDirectory(), libraryFilePath); @@ -112,8 +113,9 @@ ZimFileServer::ZimFileServer(int serverPort, Options _options, const FilePathCollection& zimpaths, std::string indexTemplateString) -: manager(&this->library) -, options(_options) +: library(new kiwix::Library()), + manager(library), + options(_options) { for ( const auto& zimpath : zimpaths ) { if (!manager.addBookFromPath(zimpath, zimpath, "", false)) @@ -128,9 +130,9 @@ void ZimFileServer::run(int serverPort, std::string indexTemplateString) if (options & NO_NAME_MAPPER) { nameMapper.reset(new kiwix::IdNameMapper()); } else { - nameMapper.reset(new kiwix::HumanReadableNameMapper(library, false)); + nameMapper.reset(new kiwix::HumanReadableNameMapper(*library, false)); } - kiwix::Server::Configuration configuration(&library, nameMapper.get()); + kiwix::Server::Configuration configuration(library, nameMapper.get()); configuration.setRoot("ROOT") .setAddress(address) .setPort(serverPort) From 4842fa03552920388d7f935fcea3fee5c2c883bb Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Wed, 12 Oct 2022 16:53:27 +0200 Subject: [PATCH 07/16] fixup! Do not store raw pointer to Library. --- test/library.cpp | 100 ++++++++++++++++++------------------ test/manager.cpp | 28 +++++----- test/name_mapper.cpp | 32 ++++++------ test/server_testing_tools.h | 14 ++--- test/testing_tools.h | 19 +++++++ 5 files changed, 108 insertions(+), 85 deletions(-) create mode 100644 test/testing_tools.h diff --git a/test/library.cpp b/test/library.cpp index f119119ef..598d5e16c 100644 --- a/test/library.cpp +++ b/test/library.cpp @@ -20,6 +20,7 @@ #include "gtest/gtest.h" #include +#include "testing_tools.h" const char * sampleOpdsStream = R"( (); - kiwix::Manager manager(lib); + kiwix::Library lib; + kiwix::Manager manager{NotOwned(lib)}; 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"); @@ -268,7 +269,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(), ""); @@ -298,11 +299,10 @@ class LibraryTest : public ::testing::Test { typedef std::vector TitleCollection; explicit LibraryTest() - : lib(std::make_shared()) {} void SetUp() override { - kiwix::Manager manager(lib); + kiwix::Manager manager{NotOwned(lib)}; manager.readOpds(sampleOpdsStream, "foo.urlHost"); manager.readXml(sampleLibraryXML, false, "./test/library.xml", true); } @@ -316,25 +316,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; } - std::shared_ptr lib; + kiwix::Library 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 +346,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"}) ); - 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 +361,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 +371,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 +736,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 +770,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,12 +800,12 @@ 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_EQ(9u, lib->removeBooksNotUpdatedSince(rev)); + EXPECT_EQ(9u, lib.removeBooksNotUpdatedSince(rev)); EXPECT_FILTER_RESULTS(kiwix::Filter(), "Islam Stack Exchange", diff --git a/test/manager.cpp b/test/manager.cpp index 41f2e3287..3b21ee7bc 100644 --- a/test/manager.cpp +++ b/test/manager.cpp @@ -6,20 +6,22 @@ #include #include +#include "testing_tools.h" + TEST(ManagerTest, addBookFromPathAndGetIdTest) { - auto lib = std::make_shared(); - kiwix::Manager manager = kiwix::Manager(lib); + kiwix::Library lib; + kiwix::Manager manager = kiwix::Manager(NotOwned(lib)); 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 +50,11 @@ const char sampleLibraryXML[] = R"( TEST(ManagerTest, readXml) { - auto lib = std::make_shared(); - kiwix::Manager manager = kiwix::Manager(lib); + kiwix::Library lib; + kiwix::Manager manager = kiwix::Manager(NotOwned(lib)); 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 +72,24 @@ TEST(ManagerTest, readXml) TEST(Manager, reload) { - auto lib = std::make_shared(); - kiwix::Manager manager(lib); + kiwix::Library lib; + kiwix::Manager manager = kiwix::Manager(NotOwned(lib)); 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 b49c8bfa7..351369900 100644 --- a/test/name_mapper.cpp +++ b/test/name_mapper.cpp @@ -4,9 +4,12 @@ #include "../include/manager.h" #include "gtest/gtest.h" +#include "testing_tools.h" + namespace { + const char libraryXML[] = R"( @@ -19,22 +22,21 @@ const char libraryXML[] = R"( class NameMapperTest : public ::testing::Test { public: - explicit NameMapperTest() : - lib(std::make_shared()) + explicit NameMapperTest() {} protected: void SetUp() override { - kiwix::Manager manager(lib); + kiwix::Manager manager{NotOwned(lib)}; 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); } } - std::shared_ptr lib; + kiwix::Library lib; }; class CapturedStderr @@ -78,13 +80,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); @@ -93,7 +95,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'." @@ -104,7 +106,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")); @@ -113,13 +115,13 @@ TEST_F(NameMapperTest, HumanReadableNameMapperWithAliases) TEST_F(NameMapperTest, UpdatableNameMapperWithoutAliases) { CapturedStderr stderror; - kiwix::UpdatableNameMapper nm(lib, false); + kiwix::UpdatableNameMapper nm(NotOwned(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); @@ -129,7 +131,7 @@ TEST_F(NameMapperTest, UpdatableNameMapperWithoutAliases) TEST_F(NameMapperTest, UpdatableNameMapperWithAliases) { CapturedStderr stderror; - kiwix::UpdatableNameMapper nm(lib, true); + kiwix::UpdatableNameMapper nm(NotOwned(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'." @@ -142,7 +144,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 600fb6e4b..6ecf02f06 100644 --- a/test/server_testing_tools.h +++ b/test/server_testing_tools.h @@ -6,6 +6,8 @@ #include #include +#include "testing_tools.h" + // Output generated via mustache templates sometimes contains end-of-line // whitespace. This complicates representing the expected output of a unit-test // as C++ raw strings in editors that are configured to delete EOL whitespace. @@ -90,7 +92,7 @@ class ZimFileServer void run(int serverPort, std::string indexTemplateString = ""); private: // data - std::shared_ptr library; + kiwix::Library library; kiwix::Manager manager; std::unique_ptr nameMapper; std::unique_ptr server; @@ -99,8 +101,7 @@ class ZimFileServer }; ZimFileServer::ZimFileServer(int serverPort, Options _options, std::string libraryFilePath) -: library(new kiwix::Library()), - manager(library), +: manager(NotOwned(library)), options(_options) { if ( kiwix::isRelativePath(libraryFilePath) ) @@ -113,8 +114,7 @@ ZimFileServer::ZimFileServer(int serverPort, Options _options, const FilePathCollection& zimpaths, std::string indexTemplateString) -: library(new kiwix::Library()), - manager(library), +: manager(NotOwned(library)), options(_options) { for ( const auto& zimpath : zimpaths ) { @@ -130,9 +130,9 @@ void ZimFileServer::run(int serverPort, std::string indexTemplateString) if (options & NO_NAME_MAPPER) { nameMapper.reset(new kiwix::IdNameMapper()); } else { - nameMapper.reset(new kiwix::HumanReadableNameMapper(*library, false)); + nameMapper.reset(new kiwix::HumanReadableNameMapper(library, false)); } - kiwix::Server::Configuration configuration(library, nameMapper.get()); + kiwix::Server::Configuration configuration(NotOwned(library), nameMapper.get()); configuration.setRoot("ROOT") .setAddress(address) .setPort(serverPort) diff --git a/test/testing_tools.h b/test/testing_tools.h new file mode 100644 index 000000000..ad3a8378e --- /dev/null +++ b/test/testing_tools.h @@ -0,0 +1,19 @@ + +#ifndef TESTING_TOOLS +#define TESTING_TOOLS + +#include + +struct NoDelete { + template void operator()(T*) {} +}; + +template +class NotOwned : public std::shared_ptr { + public: + NotOwned(T& object) : + std::shared_ptr(&object, NoDelete()) {}; +}; + + +#endif // TESTING_TOOLS From 3d75f24a299d644aca5d3d63ac22789fdbb7921a Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Fri, 7 Oct 2022 11:11:20 +0200 Subject: [PATCH 08/16] Do not store raw pointer to NameMapper. While it was "ok" to store raw pointer as, in our use case, the nameMapper always live longer than object using it; it is not safe to store raw pointer on object than may be deleted. All classes storing a NameMapper now store a shared_ptr. Functions only using the library (as `HumanReadableNameMapper`) continue to use a (const) reference. --- include/opds_dumper.h | 4 ++-- include/search_renderer.h | 6 +++--- include/server.h | 4 ++-- src/opds_dumper.cpp | 2 +- src/search_renderer.cpp | 4 ++-- src/server/internalServer.cpp | 4 ++-- src/server/internalServer.h | 2 +- test/server_testing_tools.h | 4 ++-- 8 files changed, 15 insertions(+), 15 deletions(-) diff --git a/include/opds_dumper.h b/include/opds_dumper.h index 23cc94ae2..b6ed2e85b 100644 --- a/include/opds_dumper.h +++ b/include/opds_dumper.h @@ -42,7 +42,7 @@ class OPDSDumper { public: OPDSDumper() = default; - OPDSDumper(std::shared_ptr library, NameMapper* NameMapper); + OPDSDumper(std::shared_ptr library, std::shared_ptr NameMapper); ~OPDSDumper(); /** @@ -111,7 +111,7 @@ class OPDSDumper protected: std::shared_ptr library; - kiwix::NameMapper* nameMapper; + std::shared_ptr nameMapper; std::string libraryId; std::string rootLocation; int m_totalResults; diff --git a/include/search_renderer.h b/include/search_renderer.h index fec854a1d..bd434517d 100644 --- a/include/search_renderer.h +++ b/include/search_renderer.h @@ -46,7 +46,7 @@ class SearchRenderer * @param start The start offset used for the srs. * @param estimatedResultCount The estimatedResultCount of the whole search */ - SearchRenderer(zim::SearchResultSet srs, NameMapper* mapper, + SearchRenderer(zim::SearchResultSet srs, std::shared_ptr mapper, unsigned int start, unsigned int estimatedResultCount); /** @@ -58,7 +58,7 @@ class SearchRenderer * @param start The start offset used for the srs. * @param estimatedResultCount The estimatedResultCount of the whole search */ - SearchRenderer(zim::SearchResultSet srs, NameMapper* mapper, std::shared_ptr library, + SearchRenderer(zim::SearchResultSet srs, std::shared_ptr mapper, std::shared_ptr library, unsigned int start, unsigned int estimatedResultCount); ~SearchRenderer(); @@ -106,7 +106,7 @@ class SearchRenderer protected: std::string beautifyInteger(const unsigned int number); zim::SearchResultSet m_srs; - NameMapper* mp_nameMapper; + std::shared_ptr mp_nameMapper; std::shared_ptr mp_library; std::string searchBookQuery; std::string searchPattern; diff --git a/include/server.h b/include/server.h index 39cacc994..19d776ef6 100644 --- a/include/server.h +++ b/include/server.h @@ -34,7 +34,7 @@ namespace kiwix public: class Configuration { public: - explicit Configuration(std::shared_ptr library, NameMapper* nameMapper=nullptr) + explicit Configuration(std::shared_ptr library, std::shared_ptr nameMapper=nullptr) : mp_library(library), mp_nameMapper(nameMapper) {} @@ -87,7 +87,7 @@ namespace kiwix } std::shared_ptr mp_library; - NameMapper* mp_nameMapper; + std::shared_ptr mp_nameMapper; std::string m_root = ""; std::string m_addr = ""; std::string m_indexTemplateString = ""; diff --git a/src/opds_dumper.cpp b/src/opds_dumper.cpp index 456b260b3..683e1af1e 100644 --- a/src/opds_dumper.cpp +++ b/src/opds_dumper.cpp @@ -30,7 +30,7 @@ namespace kiwix { /* Constructor */ -OPDSDumper::OPDSDumper(std::shared_ptr library, NameMapper* nameMapper) +OPDSDumper::OPDSDumper(std::shared_ptr library, std::shared_ptr nameMapper) : library(library), nameMapper(nameMapper) { diff --git a/src/search_renderer.cpp b/src/search_renderer.cpp index a8f2e31cb..91cafe4a5 100644 --- a/src/search_renderer.cpp +++ b/src/search_renderer.cpp @@ -36,12 +36,12 @@ namespace kiwix { /* Constructor */ -SearchRenderer::SearchRenderer(zim::SearchResultSet srs, NameMapper* mapper, +SearchRenderer::SearchRenderer(zim::SearchResultSet srs, std::shared_ptr mapper, unsigned int start, unsigned int estimatedResultCount) : SearchRenderer(srs, mapper, nullptr, start, estimatedResultCount) {} -SearchRenderer::SearchRenderer(zim::SearchResultSet srs, NameMapper* mapper, std::shared_ptr library, +SearchRenderer::SearchRenderer(zim::SearchResultSet srs, std::shared_ptr mapper, std::shared_ptr library, unsigned int start, unsigned int estimatedResultCount) : m_srs(srs), mp_nameMapper(mapper), diff --git a/src/server/internalServer.cpp b/src/server/internalServer.cpp index ecb3722b4..58dbc1d50 100644 --- a/src/server/internalServer.cpp +++ b/src/server/internalServer.cpp @@ -333,7 +333,7 @@ zim::Query SearchInfo::getZimQuery(bool verbose) const { return query; } -static IdNameMapper defaultNameMapper; +static std::shared_ptr defaultNameMapper = std::make_shared(); static MHD_Result staticHandlerCallback(void* cls, struct MHD_Connection* connection, @@ -370,7 +370,7 @@ InternalServer::InternalServer(const Server::Configuration& configuration) : m_configuration(configuration), m_root(normalizeRootUrl(configuration.m_root)), m_indexTemplateString(configuration.m_indexTemplateString.empty() ? RESOURCE::templates::index_html : configuration.m_indexTemplateString), - mp_nameMapper(configuration.mp_nameMapper ? configuration.mp_nameMapper : &defaultNameMapper), + mp_nameMapper(configuration.mp_nameMapper ? configuration.mp_nameMapper : defaultNameMapper), mp_daemon(nullptr), searchCache(getEnvVar("KIWIX_SEARCH_CACHE_SIZE", DEFAULT_CACHE_SIZE)), suggestionSearcherCache(getEnvVar("KIWIX_SUGGESTION_SEARCHER_CACHE_SIZE", std::max((unsigned int) (m_configuration.mp_library->getBookCount(true, true)*0.1), 1U))), diff --git a/src/server/internalServer.h b/src/server/internalServer.h index 9f9acfd1e..0b26fd9bb 100644 --- a/src/server/internalServer.h +++ b/src/server/internalServer.h @@ -153,7 +153,7 @@ class InternalServer { std::string m_addr; std::string m_root; std::string m_indexTemplateString; - NameMapper* mp_nameMapper; + std::shared_ptr mp_nameMapper; struct MHD_Daemon* mp_daemon; SearchCache searchCache; diff --git a/test/server_testing_tools.h b/test/server_testing_tools.h index 6ecf02f06..ad3099b26 100644 --- a/test/server_testing_tools.h +++ b/test/server_testing_tools.h @@ -94,7 +94,7 @@ class ZimFileServer private: // data kiwix::Library library; kiwix::Manager manager; - std::unique_ptr nameMapper; + std::shared_ptr nameMapper; std::unique_ptr server; std::unique_ptr client; const Options options = DEFAULT_OPTIONS; @@ -132,7 +132,7 @@ void ZimFileServer::run(int serverPort, std::string indexTemplateString) } else { nameMapper.reset(new kiwix::HumanReadableNameMapper(library, false)); } - kiwix::Server::Configuration configuration(NotOwned(library), nameMapper.get()); + kiwix::Server::Configuration configuration(NotOwned(library), nameMapper); configuration.setRoot("ROOT") .setAddress(address) .setPort(serverPort) From 92c0e145d4f4e98fddbd392133605e33b73d1a56 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Fri, 7 Oct 2022 11:48:08 +0200 Subject: [PATCH 09/16] Make internalServer use m_root from configuration instead of using its own. --- src/server.cpp | 9 ++------- src/server/internalServer.cpp | 29 +++++++++-------------------- src/server/internalServer.h | 1 - src/tools/otherTools.cpp | 10 ++++++++++ src/tools/otherTools.h | 2 ++ 5 files changed, 23 insertions(+), 28 deletions(-) diff --git a/src/server.cpp b/src/server.cpp index 699301ade..0ad3593af 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -26,6 +26,7 @@ #include #include "server/internalServer.h" +#include "tools/otherTools.h" namespace kiwix { @@ -51,13 +52,7 @@ void Server::stop() { Server::Configuration& Server::Configuration::setRoot(const std::string& root) { - m_root = root; - if (m_root[0] != '/') { - m_root = "/" + m_root; - } - if (m_root.back() == '/') { - m_root.erase(m_root.size() - 1); - } + m_root = normalizeRootUrl(root); return *this; } diff --git a/src/server/internalServer.cpp b/src/server/internalServer.cpp index 58dbc1d50..8abadf69c 100644 --- a/src/server/internalServer.cpp +++ b/src/server/internalServer.cpp @@ -85,16 +85,6 @@ namespace kiwix { namespace { -inline std::string normalizeRootUrl(std::string rootUrl) -{ - while ( !rootUrl.empty() && rootUrl.back() == '/' ) - rootUrl.pop_back(); - - while ( !rootUrl.empty() && rootUrl.front() == '/' ) - rootUrl = rootUrl.substr(1); - return rootUrl.empty() ? rootUrl : "/" + rootUrl; -} - Filter get_search_filter(const RequestContext& request, const std::string& prefix="") { auto filter = kiwix::Filter().valid(true).local(true); @@ -368,7 +358,6 @@ class InternalServer::CustomizedResources : public std::map InternalServer::handle_request(const RequestContext& r if (isEndpointUrl(url, "catch")) return handle_catch(request); - std::string contentUrl = m_root + "/content" + url; + std::string contentUrl = m_configuration.m_root + "/content" + url; const std::string query = request.get_query(); if ( ! query.empty() ) contentUrl += "?" + query; @@ -578,7 +567,7 @@ std::unique_ptr InternalServer::handle_request(const RequestContext& r MustacheData InternalServer::get_default_data() const { MustacheData data; - data.set("root", m_root); + data.set("root", m_configuration.m_root); return data; } @@ -785,7 +774,7 @@ std::unique_ptr InternalServer::handle_search(const RequestContext& re } catch(std::runtime_error& e) { // Searcher->search will throw a runtime error if there is no valid xapian database to do the search. // (in case of zim file not containing a index) - const auto cssUrl = renderUrl(m_root, RESOURCE::templates::url_of_search_results_css); + const auto cssUrl = renderUrl(m_configuration.m_root, RESOURCE::templates::url_of_search_results_css); HTTPErrorResponse response(*this, request, MHD_HTTP_NOT_FOUND, "fulltext-search-unavailable", "404-page-heading", @@ -825,8 +814,8 @@ std::unique_ptr InternalServer::handle_search(const RequestContext& re search->getEstimatedMatches()); renderer.setSearchPattern(searchInfo.pattern); renderer.setSearchBookQuery(searchInfo.bookFilterQuery); - renderer.setProtocolPrefix(m_root + "/content/"); - renderer.setSearchProtocolPrefix(m_root + "/search"); + renderer.setProtocolPrefix(m_configuration.m_root + "/content/"); + renderer.setSearchProtocolPrefix(m_configuration.m_root + "/search"); renderer.setPageLength(pageLength); if (request.get_requested_format() == "xml") { return ContentResponse::build(*this, renderer.getXml(), "application/rss+xml; charset=utf-8"); @@ -1001,7 +990,7 @@ std::unique_ptr InternalServer::build_redirect(const std::string& bookName, const zim::Item& item) const { const auto path = kiwix::urlEncode(item.getPath()); - const auto redirectUrl = m_root + "/content/" + bookName + "/" + path; + const auto redirectUrl = m_configuration.m_root + "/content/" + bookName + "/" + path; return Response::build_redirect(*this, redirectUrl); } @@ -1025,7 +1014,7 @@ std::unique_ptr InternalServer::handle_content(const RequestContext& r } catch (const std::out_of_range& e) {} if (archive == nullptr) { - const std::string searchURL = m_root + "/search?pattern=" + kiwix::urlEncode(pattern, true); + const std::string searchURL = m_configuration.m_root + "/search?pattern=" + kiwix::urlEncode(pattern, true); return HTTP404Response(*this, request) + urlNotFoundMsg + suggestSearchMsg(searchURL, kiwix::urlDecode(pattern)); @@ -1060,7 +1049,7 @@ std::unique_ptr InternalServer::handle_content(const RequestContext& r if (m_configuration.m_verbose) printf("Failed to find %s\n", urlStr.c_str()); - std::string searchURL = m_root + "/search?content=" + bookName + "&pattern=" + kiwix::urlEncode(pattern, true); + std::string searchURL = m_configuration.m_root + "/search?content=" + bookName + "&pattern=" + kiwix::urlEncode(pattern, true); return HTTP404Response(*this, request) + urlNotFoundMsg + suggestSearchMsg(searchURL, kiwix::urlDecode(pattern)); diff --git a/src/server/internalServer.h b/src/server/internalServer.h index 0b26fd9bb..64fc2f7aa 100644 --- a/src/server/internalServer.h +++ b/src/server/internalServer.h @@ -151,7 +151,6 @@ class InternalServer { private: // data Server::Configuration m_configuration; std::string m_addr; - std::string m_root; std::string m_indexTemplateString; std::shared_ptr mp_nameMapper; struct MHD_Daemon* mp_daemon; diff --git a/src/tools/otherTools.cpp b/src/tools/otherTools.cpp index ab5bf5874..6d6ff43b3 100644 --- a/src/tools/otherTools.cpp +++ b/src/tools/otherTools.cpp @@ -370,6 +370,16 @@ std::string kiwix::gen_uuid(const std::string& s) return kiwix::to_string(zim::Uuid::generate(s)); } +std::string kiwix::normalizeRootUrl(std::string rootUrl) +{ + while ( !rootUrl.empty() && rootUrl.back() == '/' ) + rootUrl.pop_back(); + + while ( !rootUrl.empty() && rootUrl.front() == '/' ) + rootUrl = rootUrl.substr(1); + return rootUrl.empty() ? rootUrl : "/" + rootUrl; +} + kainjow::mustache::data kiwix::onlyAsNonEmptyMustacheValue(const std::string& s) { return s.empty() diff --git a/src/tools/otherTools.h b/src/tools/otherTools.h index c0920d7bf..54d00c8eb 100644 --- a/src/tools/otherTools.h +++ b/src/tools/otherTools.h @@ -51,6 +51,8 @@ namespace kiwix std::string gen_date_str(); std::string gen_uuid(const std::string& s); + std::string normalizeRootUrl(std::string rootUrl); + // if s is empty then returns kainjow::mustache::data(false) // otherwise kainjow::mustache::data(value) kainjow::mustache::data onlyAsNonEmptyMustacheValue(const std::string& s); From 422e71a0177468a45607649333f7f1c4879975f5 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Fri, 7 Oct 2022 11:18:57 +0200 Subject: [PATCH 10/16] Make InternalServer use the NameMapper of the configuration. This move the defaulting to IdNameMapper in the configuration instead of in the InternalServer. This also create a default shared_ptr per Configuration instead of using a static default one. --- include/server.h | 5 +---- src/server.cpp | 17 +++++++++++------ src/server/internalServer.cpp | 27 ++++++++++++--------------- src/server/internalServer.h | 2 -- 4 files changed, 24 insertions(+), 27 deletions(-) diff --git a/include/server.h b/include/server.h index 19d776ef6..5d3677796 100644 --- a/include/server.h +++ b/include/server.h @@ -34,10 +34,7 @@ namespace kiwix public: class Configuration { public: - explicit Configuration(std::shared_ptr library, std::shared_ptr nameMapper=nullptr) - : mp_library(library), - mp_nameMapper(nameMapper) - {} + explicit Configuration(std::shared_ptr library, std::shared_ptr nameMapper=nullptr); Configuration& setRoot(const std::string& root); Configuration& setAddress(const std::string& addr) { diff --git a/src/server.cpp b/src/server.cpp index 0ad3593af..19d577b8f 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -30,6 +30,17 @@ namespace kiwix { +Server::Configuration::Configuration(std::shared_ptr library, std::shared_ptr nameMapper) : + mp_library(library), + mp_nameMapper(nameMapper ? nameMapper : std::make_shared()) +{} + +Server::Configuration& Server::Configuration::setRoot(const std::string& root) +{ + m_root = normalizeRootUrl(root); + return *this; +} + Server::Server(const Server::Configuration& configuration) : m_configuration(configuration), mp_server(nullptr) @@ -50,12 +61,6 @@ void Server::stop() { } } -Server::Configuration& Server::Configuration::setRoot(const std::string& root) -{ - m_root = normalizeRootUrl(root); - return *this; -} - int Server::getPort() { return mp_server->getPort(); diff --git a/src/server/internalServer.cpp b/src/server/internalServer.cpp index 8abadf69c..a99caa7a0 100644 --- a/src/server/internalServer.cpp +++ b/src/server/internalServer.cpp @@ -216,7 +216,7 @@ std::pair InternalServer::selectBooks(const Req try { auto bookName = request.get_argument("content"); try { - const auto bookIds = Library::BookIdSet{mp_nameMapper->getIdForName(bookName)}; + const auto bookIds = Library::BookIdSet{m_configuration.mp_nameMapper->getIdForName(bookName)}; const auto queryString = request.get_query([&](const std::string& key){return key == "content";}, true); return {queryString, bookIds}; } catch (const std::out_of_range&) { @@ -236,7 +236,7 @@ std::pair InternalServer::selectBooks(const Req for(const auto& bookId: id_vec) { try { // This is a silly way to check that bookId exists - mp_nameMapper->getNameForId(bookId); + m_configuration.mp_nameMapper->getNameForId(bookId); } catch (const std::out_of_range&) { throw Error(noSuchBookErrorMsg(bookId)); } @@ -255,7 +255,7 @@ std::pair InternalServer::selectBooks(const Req Library::BookIdSet bookIds; for(const auto& bookName: name_vec) { try { - bookIds.insert(mp_nameMapper->getIdForName(bookName)); + bookIds.insert(m_configuration.mp_nameMapper->getIdForName(bookName)); } catch(const std::out_of_range&) { throw Error(noSuchBookErrorMsg(bookName)); } @@ -323,8 +323,6 @@ zim::Query SearchInfo::getZimQuery(bool verbose) const { return query; } -static std::shared_ptr defaultNameMapper = std::make_shared(); - static MHD_Result staticHandlerCallback(void* cls, struct MHD_Connection* connection, const char* url, @@ -359,7 +357,6 @@ class InternalServer::CustomizedResources : public std::map("KIWIX_SEARCH_CACHE_SIZE", DEFAULT_CACHE_SIZE)), suggestionSearcherCache(getEnvVar("KIWIX_SUGGESTION_SEARCHER_CACHE_SIZE", std::max((unsigned int) (m_configuration.mp_library->getBookCount(true, true)*0.1), 1U))), @@ -631,7 +628,7 @@ std::unique_ptr InternalServer::handle_suggest(const RequestContext& r std::shared_ptr archive; try { bookName = request.get_argument("content"); - bookId = mp_nameMapper->getIdForName(bookName); + bookId = m_configuration.mp_nameMapper->getIdForName(bookName); archive = m_configuration.mp_library->getArchiveById(bookId); } catch (const std::out_of_range&) { // error handled by the archive == nullptr check below @@ -785,7 +782,7 @@ std::unique_ptr InternalServer::handle_search(const RequestContext& re /* if(bookIds.size() == 1) { auto bookId = *bookIds.begin(); - auto bookName = mp_nameMapper->getNameForId(bookId); + auto bookName = m_configuration.mp_nameMapper->getNameForId(bookId); response += TaskbarInfo(bookName, m_configuration.mp_library->getArchiveById(bookId).get()); } */ @@ -810,7 +807,7 @@ std::unique_ptr InternalServer::handle_search(const RequestContext& re } /* Get the results */ - SearchRenderer renderer(search->getResults(start-1, pageLength), mp_nameMapper, m_configuration.mp_library, start, + SearchRenderer renderer(search->getResults(start-1, pageLength), m_configuration.mp_nameMapper, m_configuration.mp_library, start, search->getEstimatedMatches()); renderer.setSearchPattern(searchInfo.pattern); renderer.setSearchBookQuery(searchInfo.bookFilterQuery); @@ -826,7 +823,7 @@ std::unique_ptr InternalServer::handle_search(const RequestContext& re /* if(bookIds.size() == 1) { auto bookId = *bookIds.begin(); - auto bookName = mp_nameMapper->getNameForId(bookId); + auto bookName = m_configuration.mp_nameMapper->getNameForId(bookId); response->set_taskbar(bookName, m_configuration.mp_library->getArchiveById(bookId).get()); } */ @@ -853,7 +850,7 @@ std::unique_ptr InternalServer::handle_random(const RequestContext& re std::shared_ptr archive; try { bookName = request.get_argument("content"); - const std::string bookId = mp_nameMapper->getIdForName(bookName); + const std::string bookId = m_configuration.mp_nameMapper->getIdForName(bookName); archive = m_configuration.mp_library->getArchiveById(bookId); } catch (const std::out_of_range&) { // error handled by the archive == nullptr check below @@ -935,8 +932,8 @@ std::unique_ptr InternalServer::handle_catalog(const RequestContext& r } zim::Uuid uuid; - kiwix::OPDSDumper opdsDumper(m_configuration.mp_library, mp_nameMapper); - opdsDumper.setRootLocation(m_root); + kiwix::OPDSDumper opdsDumper(m_configuration.mp_library, m_configuration.mp_nameMapper); + opdsDumper.setRootLocation(m_configuration.m_root); opdsDumper.setLibraryId(m_library_id); std::vector bookIdsToDump; if (url == "root.xml") { @@ -1009,7 +1006,7 @@ std::unique_ptr InternalServer::handle_content(const RequestContext& r std::shared_ptr archive; try { - const std::string bookId = mp_nameMapper->getIdForName(bookName); + const std::string bookId = m_configuration.mp_nameMapper->getIdForName(bookName); archive = m_configuration.mp_library->getArchiveById(bookId); } catch (const std::out_of_range& e) {} @@ -1081,7 +1078,7 @@ std::unique_ptr InternalServer::handle_raw(const RequestContext& reque std::shared_ptr archive; try { - const std::string bookId = mp_nameMapper->getIdForName(bookName); + const std::string bookId = m_configuration.mp_nameMapper->getIdForName(bookName); archive = m_configuration.mp_library->getArchiveById(bookId); } catch (const std::out_of_range& e) {} diff --git a/src/server/internalServer.h b/src/server/internalServer.h index 64fc2f7aa..91e6ce7da 100644 --- a/src/server/internalServer.h +++ b/src/server/internalServer.h @@ -26,7 +26,6 @@ extern "C" { } #include "library.h" -#include "name_mapper.h" #include "server.h" #include @@ -152,7 +151,6 @@ class InternalServer { Server::Configuration m_configuration; std::string m_addr; std::string m_indexTemplateString; - std::shared_ptr mp_nameMapper; struct MHD_Daemon* mp_daemon; SearchCache searchCache; From 3e54e5629118f412644a88e57c2f49b1d5febd4e Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Wed, 12 Oct 2022 16:19:56 +0200 Subject: [PATCH 11/16] [NEW] Make InternalServer inherite Server::Configuration. This avoid a lot of silly `m_configuration.foo`. --- src/server/internalServer.cpp | 126 +++++++++++------------ src/server/internalServer.h | 5 +- src/server/internalServer_catalog_v2.cpp | 26 ++--- src/server/response.cpp | 8 +- 4 files changed, 82 insertions(+), 83 deletions(-) diff --git a/src/server/internalServer.cpp b/src/server/internalServer.cpp index a99caa7a0..3c92db8d3 100644 --- a/src/server/internalServer.cpp +++ b/src/server/internalServer.cpp @@ -216,7 +216,7 @@ std::pair InternalServer::selectBooks(const Req try { auto bookName = request.get_argument("content"); try { - const auto bookIds = Library::BookIdSet{m_configuration.mp_nameMapper->getIdForName(bookName)}; + const auto bookIds = Library::BookIdSet{mp_nameMapper->getIdForName(bookName)}; const auto queryString = request.get_query([&](const std::string& key){return key == "content";}, true); return {queryString, bookIds}; } catch (const std::out_of_range&) { @@ -236,7 +236,7 @@ std::pair InternalServer::selectBooks(const Req for(const auto& bookId: id_vec) { try { // This is a silly way to check that bookId exists - m_configuration.mp_nameMapper->getNameForId(bookId); + mp_nameMapper->getNameForId(bookId); } catch (const std::out_of_range&) { throw Error(noSuchBookErrorMsg(bookId)); } @@ -255,7 +255,7 @@ std::pair InternalServer::selectBooks(const Req Library::BookIdSet bookIds; for(const auto& bookName: name_vec) { try { - bookIds.insert(m_configuration.mp_nameMapper->getIdForName(bookName)); + bookIds.insert(mp_nameMapper->getIdForName(bookName)); } catch(const std::out_of_range&) { throw Error(noSuchBookErrorMsg(bookName)); } @@ -266,7 +266,7 @@ std::pair InternalServer::selectBooks(const Req // Check for filtering Filter filter = get_search_filter(request, "books.filter."); - auto id_vec = m_configuration.mp_library->filter(filter); + auto id_vec = mp_library->filter(filter); if (id_vec.empty()) { throw Error(nonParameterizedMessage("no-book-found")); } @@ -278,7 +278,7 @@ std::pair InternalServer::selectBooks(const Req SearchInfo InternalServer::getSearchInfo(const RequestContext& request) const { auto bookIds = selectBooks(request); - checkBookNumber(bookIds.second, m_configuration.m_multizimSearchLimit); + checkBookNumber(bookIds.second, m_multizimSearchLimit); auto pattern = request.get_optional_param("pattern", ""); GeoQuery geoQuery; @@ -355,11 +355,11 @@ class InternalServer::CustomizedResources : public std::map("KIWIX_SEARCH_CACHE_SIZE", DEFAULT_CACHE_SIZE)), - suggestionSearcherCache(getEnvVar("KIWIX_SUGGESTION_SEARCHER_CACHE_SIZE", std::max((unsigned int) (m_configuration.mp_library->getBookCount(true, true)*0.1), 1U))), + suggestionSearcherCache(getEnvVar("KIWIX_SUGGESTION_SEARCHER_CACHE_SIZE", std::max((unsigned int) (mp_library->getBookCount(true, true)*0.1), 1U))), m_customizedResources(new CustomizedResources) {} @@ -371,13 +371,13 @@ bool InternalServer::start() { #else int flags = MHD_USE_POLL_INTERNALLY; #endif - if (m_configuration.m_verbose) + if (m_verbose) flags |= MHD_USE_DEBUG; struct sockaddr_in sockAddr; memset(&sockAddr, 0, sizeof(sockAddr)); sockAddr.sin_family = AF_INET; - sockAddr.sin_port = htons(m_configuration.m_port); + sockAddr.sin_port = htons(m_port); if (m_addr.empty()) { if (0 != INADDR_ANY) { sockAddr.sin_addr.s_addr = htonl(INADDR_ANY); @@ -390,17 +390,17 @@ bool InternalServer::start() { } } mp_daemon = MHD_start_daemon(flags, - m_configuration.m_port, + m_port, NULL, NULL, &staticHandlerCallback, this, MHD_OPTION_SOCK_ADDR, &sockAddr, - MHD_OPTION_THREAD_POOL_SIZE, m_configuration.m_nbThreads, - MHD_OPTION_PER_IP_CONNECTION_LIMIT, m_configuration.m_ipConnectionLimit, + MHD_OPTION_THREAD_POOL_SIZE, m_nbThreads, + MHD_OPTION_PER_IP_CONNECTION_LIMIT, m_ipConnectionLimit, MHD_OPTION_END); if (mp_daemon == nullptr) { - std::cerr << "Unable to instantiate the HTTP daemon. The port " << m_configuration.m_port + std::cerr << "Unable to instantiate the HTTP daemon. The port " << m_port << " is maybe already occupied or need more permissions to be open. " "Please try as root or with a port number higher or equal to 1024." << std::endl; @@ -446,14 +446,14 @@ MHD_Result InternalServer::handlerCallback(struct MHD_Connection* connection, void** cont_cls) { auto start_time = std::chrono::steady_clock::now(); - if (m_configuration.m_verbose) { + if (m_verbose) { printf("======================\n"); printf("Requesting : \n"); printf("full_url : %s\n", url); } - RequestContext request(connection, m_configuration.m_root, url, method, version); + RequestContext request(connection, m_root, url, method, version); - if (m_configuration.m_verbose) { + if (m_verbose) { request.print_debug_info(); } /* Unexpected method */ @@ -469,7 +469,7 @@ MHD_Result InternalServer::handlerCallback(struct MHD_Connection* connection, if (response->getReturnCode() == MHD_HTTP_INTERNAL_SERVER_ERROR) { printf("========== INTERNAL ERROR !! ============\n"); - if (!m_configuration.m_verbose) { + if (!m_verbose) { printf("Requesting : \n"); printf("full_url : %s\n", url); request.print_debug_info(); @@ -482,7 +482,7 @@ MHD_Result InternalServer::handlerCallback(struct MHD_Connection* connection, auto ret = response->send(request, connection); auto end_time = std::chrono::steady_clock::now(); auto time_span = std::chrono::duration_cast>(end_time - start_time); - if (m_configuration.m_verbose) { + if (m_verbose) { printf("Request time : %fs\n", time_span.count()); printf("----------------------\n"); } @@ -545,7 +545,7 @@ std::unique_ptr InternalServer::handle_request(const RequestContext& r if (isEndpointUrl(url, "catch")) return handle_catch(request); - std::string contentUrl = m_configuration.m_root + "/content" + url; + std::string contentUrl = m_root + "/content" + url; const std::string query = request.get_query(); if ( ! query.empty() ) contentUrl += "?" + query; @@ -564,7 +564,7 @@ std::unique_ptr InternalServer::handle_request(const RequestContext& r MustacheData InternalServer::get_default_data() const { MustacheData data; - data.set("root", m_configuration.m_root); + data.set("root", m_root); return data; } @@ -615,7 +615,7 @@ class InternalServer::LockableSuggestionSearcher : public zim::SuggestionSearche std::unique_ptr InternalServer::handle_suggest(const RequestContext& request) { - if (m_configuration.m_verbose) { + if (m_verbose) { printf("** running handle_suggest\n"); } @@ -628,8 +628,8 @@ std::unique_ptr InternalServer::handle_suggest(const RequestContext& r std::shared_ptr archive; try { bookName = request.get_argument("content"); - bookId = m_configuration.mp_nameMapper->getIdForName(bookName); - archive = m_configuration.mp_library->getArchiveById(bookId); + bookId = mp_nameMapper->getIdForName(bookName); + archive = mp_library->getArchiveById(bookId); } catch (const std::out_of_range&) { // error handled by the archive == nullptr check below } @@ -646,7 +646,7 @@ std::unique_ptr InternalServer::handle_suggest(const RequestContext& r count = 10; } - if (m_configuration.m_verbose) { + if (m_verbose) { printf("Searching suggestions for: \"%s\"\n", queryString.c_str()); } @@ -699,21 +699,21 @@ std::unique_ptr InternalServer::handle_suggest(const RequestContext& r std::unique_ptr InternalServer::handle_viewer_settings(const RequestContext& request) { - if (m_configuration.m_verbose) { + if (m_verbose) { printf("** running handle_viewer_settings\n"); } const kainjow::mustache::object data{ - {"enable_toolbar", m_configuration.m_withTaskbar ? "true" : "false" }, - {"enable_link_blocking", m_configuration.m_blockExternalLinks ? "true" : "false" }, - {"enable_library_button", m_configuration.m_withLibraryButton ? "true" : "false" } + {"enable_toolbar", m_withTaskbar ? "true" : "false" }, + {"enable_link_blocking", m_blockExternalLinks ? "true" : "false" }, + {"enable_library_button", m_withLibraryButton ? "true" : "false" } }; return ContentResponse::build(*this, RESOURCE::templates::viewer_settings_js, data, "application/javascript; charset=utf-8"); } std::unique_ptr InternalServer::handle_skin(const RequestContext& request) { - if (m_configuration.m_verbose) { + if (m_verbose) { printf("** running handle_skin\n"); } @@ -736,7 +736,7 @@ std::unique_ptr InternalServer::handle_skin(const RequestContext& requ std::unique_ptr InternalServer::handle_search(const RequestContext& request) { - if (m_configuration.m_verbose) { + if (m_verbose) { printf("** running handle_search\n"); } @@ -758,20 +758,20 @@ std::unique_ptr InternalServer::handle_search(const RequestContext& re /* Make the search */ // Try to get a search from the searchInfo, else build it - auto searcher = m_configuration.mp_library->getSearcherByIds(bookIds); + auto searcher = mp_library->getSearcherByIds(bookIds); auto lock(searcher->getLock()); std::shared_ptr search; try { search = searchCache.getOrPut(searchInfo, [=](){ - return make_shared(searcher->search(searchInfo.getZimQuery(m_configuration.m_verbose))); + return make_shared(searcher->search(searchInfo.getZimQuery(m_verbose))); } ); } catch(std::runtime_error& e) { // Searcher->search will throw a runtime error if there is no valid xapian database to do the search. // (in case of zim file not containing a index) - const auto cssUrl = renderUrl(m_configuration.m_root, RESOURCE::templates::url_of_search_results_css); + const auto cssUrl = renderUrl(m_root, RESOURCE::templates::url_of_search_results_css); HTTPErrorResponse response(*this, request, MHD_HTTP_NOT_FOUND, "fulltext-search-unavailable", "404-page-heading", @@ -782,8 +782,8 @@ std::unique_ptr InternalServer::handle_search(const RequestContext& re /* if(bookIds.size() == 1) { auto bookId = *bookIds.begin(); - auto bookName = m_configuration.mp_nameMapper->getNameForId(bookId); - response += TaskbarInfo(bookName, m_configuration.mp_library->getArchiveById(bookId).get()); + auto bookName = mp_nameMapper->getNameForId(bookId); + response += TaskbarInfo(bookName, mp_library->getArchiveById(bookId).get()); } */ return response; @@ -807,12 +807,12 @@ std::unique_ptr InternalServer::handle_search(const RequestContext& re } /* Get the results */ - SearchRenderer renderer(search->getResults(start-1, pageLength), m_configuration.mp_nameMapper, m_configuration.mp_library, start, + SearchRenderer renderer(search->getResults(start-1, pageLength), mp_nameMapper, mp_library, start, search->getEstimatedMatches()); renderer.setSearchPattern(searchInfo.pattern); renderer.setSearchBookQuery(searchInfo.bookFilterQuery); - renderer.setProtocolPrefix(m_configuration.m_root + "/content/"); - renderer.setSearchProtocolPrefix(m_configuration.m_root + "/search"); + renderer.setProtocolPrefix(m_root + "/content/"); + renderer.setSearchProtocolPrefix(m_root + "/search"); renderer.setPageLength(pageLength); if (request.get_requested_format() == "xml") { return ContentResponse::build(*this, renderer.getXml(), "application/rss+xml; charset=utf-8"); @@ -823,8 +823,8 @@ std::unique_ptr InternalServer::handle_search(const RequestContext& re /* if(bookIds.size() == 1) { auto bookId = *bookIds.begin(); - auto bookName = m_configuration.mp_nameMapper->getNameForId(bookId); - response->set_taskbar(bookName, m_configuration.mp_library->getArchiveById(bookId).get()); + auto bookName = mp_nameMapper->getNameForId(bookId); + response->set_taskbar(bookName, mp_library->getArchiveById(bookId).get()); } */ return std::move(response); @@ -837,7 +837,7 @@ std::unique_ptr InternalServer::handle_search(const RequestContext& re std::unique_ptr InternalServer::handle_random(const RequestContext& request) { - if (m_configuration.m_verbose) { + if (m_verbose) { printf("** running handle_random\n"); } @@ -850,8 +850,8 @@ std::unique_ptr InternalServer::handle_random(const RequestContext& re std::shared_ptr archive; try { bookName = request.get_argument("content"); - const std::string bookId = m_configuration.mp_nameMapper->getIdForName(bookName); - archive = m_configuration.mp_library->getArchiveById(bookId); + const std::string bookId = mp_nameMapper->getIdForName(bookName); + archive = mp_library->getArchiveById(bookId); } catch (const std::out_of_range&) { // error handled by the archive == nullptr check below } @@ -889,7 +889,7 @@ std::unique_ptr InternalServer::handle_captured_external(const Request std::unique_ptr InternalServer::handle_catch(const RequestContext& request) { - if (m_configuration.m_verbose) { + if (m_verbose) { printf("** running handle_catch\n"); } @@ -903,7 +903,7 @@ std::unique_ptr InternalServer::handle_catch(const RequestContext& req std::unique_ptr InternalServer::handle_catalog(const RequestContext& request) { - if (m_configuration.m_verbose) { + if (m_verbose) { printf("** running handle_catalog"); } @@ -932,13 +932,13 @@ std::unique_ptr InternalServer::handle_catalog(const RequestContext& r } zim::Uuid uuid; - kiwix::OPDSDumper opdsDumper(m_configuration.mp_library, m_configuration.mp_nameMapper); - opdsDumper.setRootLocation(m_configuration.m_root); + kiwix::OPDSDumper opdsDumper(mp_library, mp_nameMapper); + opdsDumper.setRootLocation(m_root); opdsDumper.setLibraryId(m_library_id); std::vector bookIdsToDump; if (url == "root.xml") { uuid = zim::Uuid::generate(host); - bookIdsToDump = m_configuration.mp_library->filter(kiwix::Filter().valid(true).local(true).remote(true)); + bookIdsToDump = mp_library->filter(kiwix::Filter().valid(true).local(true).remote(true)); } else if (url == "search") { bookIdsToDump = search_catalog(request, opdsDumper); uuid = zim::Uuid::generate(); @@ -959,7 +959,7 @@ InternalServer::search_catalog(const RequestContext& request, const std::string q = filter.hasQuery() ? filter.getQuery() : ""; - std::vector bookIdsToDump = m_configuration.mp_library->filter(filter); + std::vector bookIdsToDump = mp_library->filter(filter); const auto totalResults = bookIdsToDump.size(); const size_t count = request.get_optional_param("count", 10UL); const size_t startIndex = request.get_optional_param("start", 0UL); @@ -987,7 +987,7 @@ std::unique_ptr InternalServer::build_redirect(const std::string& bookName, const zim::Item& item) const { const auto path = kiwix::urlEncode(item.getPath()); - const auto redirectUrl = m_configuration.m_root + "/content/" + bookName + "/" + path; + const auto redirectUrl = m_root + "/content/" + bookName + "/" + path; return Response::build_redirect(*this, redirectUrl); } @@ -995,7 +995,7 @@ std::unique_ptr InternalServer::handle_content(const RequestContext& r { const std::string url = request.get_url(); const std::string pattern = url.substr((url.find_last_of('/'))+1); - if (m_configuration.m_verbose) { + if (m_verbose) { printf("** running handle_content\n"); } @@ -1006,12 +1006,12 @@ std::unique_ptr InternalServer::handle_content(const RequestContext& r std::shared_ptr archive; try { - const std::string bookId = m_configuration.mp_nameMapper->getIdForName(bookName); - archive = m_configuration.mp_library->getArchiveById(bookId); + const std::string bookId = mp_nameMapper->getIdForName(bookName); + archive = mp_library->getArchiveById(bookId); } catch (const std::out_of_range& e) {} if (archive == nullptr) { - const std::string searchURL = m_configuration.m_root + "/search?pattern=" + kiwix::urlEncode(pattern, true); + const std::string searchURL = m_root + "/search?pattern=" + kiwix::urlEncode(pattern, true); return HTTP404Response(*this, request) + urlNotFoundMsg + suggestSearchMsg(searchURL, kiwix::urlDecode(pattern)); @@ -1036,17 +1036,17 @@ std::unique_ptr InternalServer::handle_content(const RequestContext& r } auto response = ItemResponse::build(*this, request, entry.getItem()); - if (m_configuration.m_verbose) { + if (m_verbose) { printf("Found %s\n", entry.getPath().c_str()); printf("mimeType: %s\n", entry.getItem(true).getMimetype().c_str()); } return response; } catch(zim::EntryNotFound& e) { - if (m_configuration.m_verbose) + if (m_verbose) printf("Failed to find %s\n", urlStr.c_str()); - std::string searchURL = m_configuration.m_root + "/search?content=" + bookName + "&pattern=" + kiwix::urlEncode(pattern, true); + std::string searchURL = m_root + "/search?content=" + bookName + "&pattern=" + kiwix::urlEncode(pattern, true); return HTTP404Response(*this, request) + urlNotFoundMsg + suggestSearchMsg(searchURL, kiwix::urlDecode(pattern)); @@ -1056,7 +1056,7 @@ std::unique_ptr InternalServer::handle_content(const RequestContext& r std::unique_ptr InternalServer::handle_raw(const RequestContext& request) { - if (m_configuration.m_verbose) { + if (m_verbose) { printf("** running handle_raw\n"); } @@ -1078,8 +1078,8 @@ std::unique_ptr InternalServer::handle_raw(const RequestContext& reque std::shared_ptr archive; try { - const std::string bookId = m_configuration.mp_nameMapper->getIdForName(bookName); - archive = m_configuration.mp_library->getArchiveById(bookId); + const std::string bookId = mp_nameMapper->getIdForName(bookName); + archive = mp_library->getArchiveById(bookId); } catch (const std::out_of_range& e) {} if (archive == nullptr) { @@ -1106,7 +1106,7 @@ std::unique_ptr InternalServer::handle_raw(const RequestContext& reque return ItemResponse::build(*this, request, entry.getItem()); } } catch (zim::EntryNotFound& e ) { - if (m_configuration.m_verbose) { + if (m_verbose) { printf("Failed to find %s\n", itemPath.c_str()); } return HTTP404Response(*this, request) @@ -1122,13 +1122,13 @@ bool InternalServer::isLocallyCustomizedResource(const std::string& url) const std::unique_ptr InternalServer::handle_locally_customized_resource(const RequestContext& request) { - if (m_configuration.m_verbose) { + if (m_verbose) { printf("** running handle_locally_customized_resource\n"); } const CustomizedResourceData& crd = m_customizedResources->at(request.get_url()); - if (m_configuration.m_verbose) { + if (m_verbose) { std::cout << "Reading " << crd.resourceFilePath << std::endl; } const auto resourceData = getFileContent(crd.resourceFilePath); diff --git a/src/server/internalServer.h b/src/server/internalServer.h index 91e6ce7da..d44434e0a 100644 --- a/src/server/internalServer.h +++ b/src/server/internalServer.h @@ -90,7 +90,7 @@ class SearchInfo { typedef kainjow::mustache::data MustacheData; class OPDSDumper; -class InternalServer { +class InternalServer : Server::Configuration { public: InternalServer(const Server::Configuration& configuration); virtual ~InternalServer(); @@ -105,7 +105,7 @@ class InternalServer { bool start(); void stop(); std::string getAddress() { return m_addr; } - int getPort() { return m_configuration.m_port; } + int getPort() { return m_port; } private: // functions std::unique_ptr handle_request(const RequestContext& request); @@ -148,7 +148,6 @@ class InternalServer { typedef ConcurrentCache> SuggestionSearcherCache; private: // data - Server::Configuration m_configuration; std::string m_addr; std::string m_indexTemplateString; struct MHD_Daemon* mp_daemon; diff --git a/src/server/internalServer_catalog_v2.cpp b/src/server/internalServer_catalog_v2.cpp index a8122c828..c5b57c743 100644 --- a/src/server/internalServer_catalog_v2.cpp +++ b/src/server/internalServer_catalog_v2.cpp @@ -35,7 +35,7 @@ namespace kiwix { std::unique_ptr InternalServer::handle_catalog_v2(const RequestContext& request) { - if (m_configuration.m_verbose) { + if (m_verbose) { printf("** running handle_catalog_v2"); } @@ -50,7 +50,7 @@ std::unique_ptr InternalServer::handle_catalog_v2(const RequestContext if (url == "root.xml") { return handle_catalog_v2_root(request); } else if (url == "searchdescription.xml") { - const std::string endpoint_root = m_configuration.m_root + "/catalog/v2"; + const std::string endpoint_root = m_root + "/catalog/v2"; return ContentResponse::build(*this, RESOURCE::catalog_v2_searchdescription_xml, kainjow::mustache::object({{"endpoint_root", endpoint_root}}), @@ -82,7 +82,7 @@ std::unique_ptr InternalServer::handle_catalog_v2_root(const RequestCo RESOURCE::templates::catalog_v2_root_xml, kainjow::mustache::object{ {"date", gen_date_str()}, - {"endpoint_root", m_configuration.m_root + "/catalog/v2"}, + {"endpoint_root", m_root + "/catalog/v2"}, {"feed_id", gen_uuid(m_library_id)}, {"all_entries_feed_id", gen_uuid(m_library_id + "/entries")}, {"partial_entries_feed_id", gen_uuid(m_library_id + "/partial_entries")}, @@ -95,8 +95,8 @@ std::unique_ptr InternalServer::handle_catalog_v2_root(const RequestCo std::unique_ptr InternalServer::handle_catalog_v2_entries(const RequestContext& request, bool partial) { - OPDSDumper opdsDumper(m_configuration.mp_library, m_configuration.mp_nameMapper); - opdsDumper.setRootLocation(m_configuration.m_root); + OPDSDumper opdsDumper(mp_library, mp_nameMapper); + opdsDumper.setRootLocation(m_root); opdsDumper.setLibraryId(m_library_id); const auto bookIds = search_catalog(request, opdsDumper); const auto opdsFeed = opdsDumper.dumpOPDSFeedV2(bookIds, request.get_query(), partial); @@ -110,14 +110,14 @@ std::unique_ptr InternalServer::handle_catalog_v2_entries(const Reques std::unique_ptr InternalServer::handle_catalog_v2_complete_entry(const RequestContext& request, const std::string& entryId) { try { - m_configuration.mp_library->getBookById(entryId); + mp_library->getBookById(entryId); } catch (const std::out_of_range&) { return HTTP404Response(*this, request) + urlNotFoundMsg; } - OPDSDumper opdsDumper(m_configuration.mp_library, m_configuration.mp_nameMapper); - opdsDumper.setRootLocation(m_configuration.m_root); + OPDSDumper opdsDumper(mp_library, mp_nameMapper); + opdsDumper.setRootLocation(m_root); opdsDumper.setLibraryId(m_library_id); const auto opdsFeed = opdsDumper.dumpOPDSCompleteEntry(entryId); return ContentResponse::build( @@ -129,8 +129,8 @@ std::unique_ptr InternalServer::handle_catalog_v2_complete_entry(const std::unique_ptr InternalServer::handle_catalog_v2_categories(const RequestContext& request) { - OPDSDumper opdsDumper(m_configuration.mp_library, m_configuration.mp_nameMapper); - opdsDumper.setRootLocation(m_configuration.m_root); + OPDSDumper opdsDumper(mp_library, mp_nameMapper); + opdsDumper.setRootLocation(m_root); opdsDumper.setLibraryId(m_library_id); return ContentResponse::build( *this, @@ -141,8 +141,8 @@ std::unique_ptr InternalServer::handle_catalog_v2_categories(const Req std::unique_ptr InternalServer::handle_catalog_v2_languages(const RequestContext& request) { - OPDSDumper opdsDumper(m_configuration.mp_library, m_configuration.mp_nameMapper); - opdsDumper.setRootLocation(m_configuration.m_root); + OPDSDumper opdsDumper(mp_library, mp_nameMapper); + opdsDumper.setRootLocation(m_root); opdsDumper.setLibraryId(m_library_id); return ContentResponse::build( *this, @@ -155,7 +155,7 @@ std::unique_ptr InternalServer::handle_catalog_v2_illustration(const R { try { const auto bookId = request.get_url_part(3); - auto book = m_configuration.mp_library->getBookByIdThreadSafe(bookId); + auto book = mp_library->getBookByIdThreadSafe(bookId); auto size = request.get_argument("size"); auto illustration = book.getIllustration(size); return ContentResponse::build( diff --git a/src/server/response.cpp b/src/server/response.cpp index 064212353..46e3c86ea 100644 --- a/src/server/response.cpp +++ b/src/server/response.cpp @@ -114,7 +114,7 @@ Response::Response(bool verbose) std::unique_ptr Response::build(const InternalServer& server) { - return std::unique_ptr(new Response(server.m_configuration.m_verbose)); + return std::unique_ptr(new Response(server.m_verbose)); } std::unique_ptr Response::build_304(const InternalServer& server, const ETag& etag) @@ -389,8 +389,8 @@ std::unique_ptr ContentResponse::build( const std::string& mimetype) { return std::unique_ptr(new ContentResponse( - server.m_configuration.m_root, - server.m_configuration.m_verbose, + server.m_root, + server.m_verbose, content, mimetype)); } @@ -435,7 +435,7 @@ std::unique_ptr ItemResponse::build(const InternalServer& server, cons } return std::unique_ptr(new ItemResponse( - server.m_configuration.m_verbose, + server.m_verbose, item, mimetype, byteRange)); From 6815a4c6a939e10bf1bb648f24d600779c1001be Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Fri, 7 Oct 2022 11:40:15 +0200 Subject: [PATCH 12/16] Make the OPDSDumper use the ServerConfiguration. --- include/opds_dumper.h | 18 ++------- src/opds_dumper.cpp | 49 ++++++++++++------------ src/server/internalServer.cpp | 3 +- src/server/internalServer_catalog_v2.cpp | 12 ++---- 4 files changed, 34 insertions(+), 48 deletions(-) diff --git a/include/opds_dumper.h b/include/opds_dumper.h index b6ed2e85b..f16678a27 100644 --- a/include/opds_dumper.h +++ b/include/opds_dumper.h @@ -23,11 +23,11 @@ #include #include #include +#include #include -#include "library.h" -#include "name_mapper.h" +#include "server.h" using namespace std; @@ -41,8 +41,7 @@ namespace kiwix class OPDSDumper { public: - OPDSDumper() = default; - OPDSDumper(std::shared_ptr library, std::shared_ptr NameMapper); + OPDSDumper(Server::Configuration configuration); ~OPDSDumper(); /** @@ -93,13 +92,6 @@ class OPDSDumper */ void setLibraryId(const std::string& id) { this->libraryId = id;} - /** - * Set the root location used when generating url. - * - * @param rootLocation the root location to use. - */ - void setRootLocation(const std::string& rootLocation) { this->rootLocation = rootLocation; } - /** * Set some informations about the search results. * @@ -110,10 +102,8 @@ class OPDSDumper void setOpenSearchInfo(int totalResult, int startIndex, int count); protected: - std::shared_ptr library; - std::shared_ptr nameMapper; + Server::Configuration m_configuration; std::string libraryId; - std::string rootLocation; int m_totalResults; int m_startIndex; int m_count; diff --git a/src/opds_dumper.cpp b/src/opds_dumper.cpp index 683e1af1e..557d196e9 100644 --- a/src/opds_dumper.cpp +++ b/src/opds_dumper.cpp @@ -19,6 +19,8 @@ #include "opds_dumper.h" #include "book.h" +#include "library.h" +#include "name_mapper.h" #include "libkiwix-resources.h" #include @@ -30,9 +32,8 @@ namespace kiwix { /* Constructor */ -OPDSDumper::OPDSDumper(std::shared_ptr library, std::shared_ptr nameMapper) - : library(library), - nameMapper(nameMapper) +OPDSDumper::OPDSDumper(Server::Configuration configuration) + : m_configuration(configuration) { } /* Destructor */ @@ -72,11 +73,11 @@ IllustrationInfo getBookIllustrationInfo(const Book& book) return illustrations; } -std::string fullEntryXML(const Book& book, const std::string& rootLocation, const std::string& bookName) +std::string fullEntryXML(const Server::Configuration& configuration, const Book& book, const std::string& bookName) { const auto bookDate = book.getDate() + "T00:00:00Z"; const kainjow::mustache::object data{ - {"root", rootLocation}, + {"root", configuration.m_root}, {"id", book.getId()}, {"name", book.getName()}, {"title", book.getTitle()}, @@ -99,12 +100,12 @@ std::string fullEntryXML(const Book& book, const std::string& rootLocation, cons return render_template(RESOURCE::templates::catalog_v2_entry_xml, data); } -std::string partialEntryXML(const Book& book, const std::string& rootLocation) +std::string partialEntryXML(const Server::Configuration& configuration, const Book& book) { const auto bookDate = book.getDate() + "T00:00:00Z"; const kainjow::mustache::object data{ - {"root", rootLocation}, - {"endpoint_root", rootLocation + "/catalog/v2"}, + {"root", configuration.m_root}, + {"endpoint_root", configuration.m_root + "/catalog/v2"}, {"id", book.getId()}, {"title", book.getTitle()}, {"updated", bookDate}, // XXX: this should be the entry update datetime @@ -113,16 +114,16 @@ std::string partialEntryXML(const Book& book, const std::string& rootLocation) return render_template(xmlTemplate, data); } -BooksData getBooksData(const Library& library, const NameMapper& nameMapper, const std::vector& bookIds, const std::string& rootLocation, bool partial) +BooksData getBooksData(const Server::Configuration& configuration, const std::vector& bookIds, bool partial) { BooksData booksData; for ( const auto& bookId : bookIds ) { try { - const Book book = library.getBookByIdThreadSafe(bookId); - const std::string bookName = nameMapper.getNameForId(bookId); + const Book book = configuration.mp_library->getBookByIdThreadSafe(bookId); + const std::string bookName = configuration.mp_nameMapper->getNameForId(bookId); const auto entryXML = partial - ? partialEntryXML(book, rootLocation) - : fullEntryXML(book, rootLocation, bookName); + ? partialEntryXML(configuration, book) + : fullEntryXML(configuration, book, bookName); booksData.push_back(kainjow::mustache::object{ {"entry", entryXML} }); } catch ( const std::out_of_range& ) { // the book was removed from the library since its id was obtained @@ -190,10 +191,10 @@ std::string getLanguageSelfName(const std::string& lang) { string OPDSDumper::dumpOPDSFeed(const std::vector& bookIds, const std::string& query) const { - const auto booksData = getBooksData(*library, *nameMapper, bookIds, rootLocation, false); + const auto booksData = getBooksData(m_configuration, bookIds, false); const kainjow::mustache::object template_data{ {"date", gen_date_str()}, - {"root", rootLocation}, + {"root", m_configuration.m_root}, {"feed_id", gen_uuid(libraryId + "/catalog/search?"+query)}, {"filter", onlyAsNonEmptyMustacheValue(query)}, {"totalResults", to_string(m_totalResults)}, @@ -207,8 +208,8 @@ string OPDSDumper::dumpOPDSFeed(const std::vector& bookIds, const s string OPDSDumper::dumpOPDSFeedV2(const std::vector& bookIds, const std::string& query, bool partial) const { - const auto endpointRoot = rootLocation + "/catalog/v2"; - const auto booksData = getBooksData(*library, *nameMapper, bookIds, rootLocation, partial); + const auto endpointRoot = m_configuration.m_root + "/catalog/v2"; + const auto booksData = getBooksData(m_configuration, bookIds, partial); const char* const endpoint = partial ? "/partial_entries" : "/entries"; const kainjow::mustache::object template_data{ @@ -229,18 +230,18 @@ string OPDSDumper::dumpOPDSFeedV2(const std::vector& bookIds, const std::string OPDSDumper::dumpOPDSCompleteEntry(const std::string& bookId) const { - const auto book = library->getBookById(bookId); - const std::string bookName = nameMapper->getNameForId(bookId); + const auto book = m_configuration.mp_library->getBookById(bookId); + const std::string bookName = m_configuration.mp_nameMapper->getNameForId(bookId); return XML_HEADER + "\n" - + fullEntryXML(book, rootLocation, bookName); + + fullEntryXML(m_configuration, book, bookName); } std::string OPDSDumper::categoriesOPDSFeed() const { const auto now = gen_date_str(); kainjow::mustache::list categoryData; - for ( const auto& category : library->getBooksCategories() ) { + for ( const auto& category : m_configuration.mp_library->getBooksCategories() ) { const auto urlencodedCategoryName = urlEncode(category); categoryData.push_back(kainjow::mustache::object{ {"name", category}, @@ -254,7 +255,7 @@ std::string OPDSDumper::categoriesOPDSFeed() const RESOURCE::templates::catalog_v2_categories_xml, kainjow::mustache::object{ {"date", now}, - {"endpoint_root", rootLocation + "/catalog/v2"}, + {"endpoint_root", m_configuration.m_root + "/catalog/v2"}, {"feed_id", gen_uuid(libraryId + "/categories")}, {"categories", categoryData } } @@ -266,7 +267,7 @@ std::string OPDSDumper::languagesOPDSFeed() const const auto now = gen_date_str(); kainjow::mustache::list languageData; std::call_once(fillLanguagesFlag, fillLanguagesMap); - for ( const auto& langAndBookCount : library->getBooksLanguagesWithCounts() ) { + for ( const auto& langAndBookCount : m_configuration.mp_library->getBooksLanguagesWithCounts() ) { const std::string languageCode = langAndBookCount.first; const int bookCount = langAndBookCount.second; const auto languageSelfName = getLanguageSelfName(languageCode); @@ -283,7 +284,7 @@ std::string OPDSDumper::languagesOPDSFeed() const RESOURCE::templates::catalog_v2_languages_xml, kainjow::mustache::object{ {"date", now}, - {"endpoint_root", rootLocation + "/catalog/v2"}, + {"endpoint_root", m_configuration.m_root + "/catalog/v2"}, {"feed_id", gen_uuid(libraryId + "/languages")}, {"languages", languageData } } diff --git a/src/server/internalServer.cpp b/src/server/internalServer.cpp index 3c92db8d3..7eece05b5 100644 --- a/src/server/internalServer.cpp +++ b/src/server/internalServer.cpp @@ -932,8 +932,7 @@ std::unique_ptr InternalServer::handle_catalog(const RequestContext& r } zim::Uuid uuid; - kiwix::OPDSDumper opdsDumper(mp_library, mp_nameMapper); - opdsDumper.setRootLocation(m_root); + kiwix::OPDSDumper opdsDumper(*this); opdsDumper.setLibraryId(m_library_id); std::vector bookIdsToDump; if (url == "root.xml") { diff --git a/src/server/internalServer_catalog_v2.cpp b/src/server/internalServer_catalog_v2.cpp index c5b57c743..7820af46c 100644 --- a/src/server/internalServer_catalog_v2.cpp +++ b/src/server/internalServer_catalog_v2.cpp @@ -95,8 +95,7 @@ std::unique_ptr InternalServer::handle_catalog_v2_root(const RequestCo std::unique_ptr InternalServer::handle_catalog_v2_entries(const RequestContext& request, bool partial) { - OPDSDumper opdsDumper(mp_library, mp_nameMapper); - opdsDumper.setRootLocation(m_root); + OPDSDumper opdsDumper(*this); opdsDumper.setLibraryId(m_library_id); const auto bookIds = search_catalog(request, opdsDumper); const auto opdsFeed = opdsDumper.dumpOPDSFeedV2(bookIds, request.get_query(), partial); @@ -116,8 +115,7 @@ std::unique_ptr InternalServer::handle_catalog_v2_complete_entry(const + urlNotFoundMsg; } - OPDSDumper opdsDumper(mp_library, mp_nameMapper); - opdsDumper.setRootLocation(m_root); + OPDSDumper opdsDumper(*this); opdsDumper.setLibraryId(m_library_id); const auto opdsFeed = opdsDumper.dumpOPDSCompleteEntry(entryId); return ContentResponse::build( @@ -129,8 +127,7 @@ std::unique_ptr InternalServer::handle_catalog_v2_complete_entry(const std::unique_ptr InternalServer::handle_catalog_v2_categories(const RequestContext& request) { - OPDSDumper opdsDumper(mp_library, mp_nameMapper); - opdsDumper.setRootLocation(m_root); + OPDSDumper opdsDumper(*this); opdsDumper.setLibraryId(m_library_id); return ContentResponse::build( *this, @@ -141,8 +138,7 @@ std::unique_ptr InternalServer::handle_catalog_v2_categories(const Req std::unique_ptr InternalServer::handle_catalog_v2_languages(const RequestContext& request) { - OPDSDumper opdsDumper(mp_library, mp_nameMapper); - opdsDumper.setRootLocation(m_root); + OPDSDumper opdsDumper(*this); opdsDumper.setLibraryId(m_library_id); return ContentResponse::build( *this, From 37ccfb54edfd99ec0583fba783bf58f0c296bbf4 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Fri, 7 Oct 2022 11:51:34 +0200 Subject: [PATCH 13/16] Make `fullEntryXML` use the configuration to get the bookName. --- src/opds_dumper.cpp | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/opds_dumper.cpp b/src/opds_dumper.cpp index 557d196e9..40b2f87a1 100644 --- a/src/opds_dumper.cpp +++ b/src/opds_dumper.cpp @@ -73,7 +73,7 @@ IllustrationInfo getBookIllustrationInfo(const Book& book) return illustrations; } -std::string fullEntryXML(const Server::Configuration& configuration, const Book& book, const std::string& bookName) +std::string fullEntryXML(const Server::Configuration& configuration, const Book& book) { const auto bookDate = book.getDate() + "T00:00:00Z"; const kainjow::mustache::object data{ @@ -83,7 +83,7 @@ std::string fullEntryXML(const Server::Configuration& configuration, const Book& {"title", book.getTitle()}, {"description", book.getDescription()}, {"language", book.getLanguage()}, - {"content_id", urlEncode(bookName, true)}, + {"content_id", urlEncode(configuration.mp_nameMapper->getNameForId(book.getId()), true)}, {"updated", bookDate}, // XXX: this should be the entry update datetime {"book_date", bookDate}, {"category", book.getCategory()}, @@ -120,10 +120,9 @@ BooksData getBooksData(const Server::Configuration& configuration, const std::ve for ( const auto& bookId : bookIds ) { try { const Book book = configuration.mp_library->getBookByIdThreadSafe(bookId); - const std::string bookName = configuration.mp_nameMapper->getNameForId(bookId); const auto entryXML = partial ? partialEntryXML(configuration, book) - : fullEntryXML(configuration, book, bookName); + : fullEntryXML(configuration, book); booksData.push_back(kainjow::mustache::object{ {"entry", entryXML} }); } catch ( const std::out_of_range& ) { // the book was removed from the library since its id was obtained @@ -231,10 +230,9 @@ string OPDSDumper::dumpOPDSFeedV2(const std::vector& bookIds, const std::string OPDSDumper::dumpOPDSCompleteEntry(const std::string& bookId) const { const auto book = m_configuration.mp_library->getBookById(bookId); - const std::string bookName = m_configuration.mp_nameMapper->getNameForId(bookId); return XML_HEADER + "\n" - + fullEntryXML(m_configuration, book, bookName); + + fullEntryXML(m_configuration, book); } std::string OPDSDumper::categoriesOPDSFeed() const From 7d83127d58001adbeadf631e3796657c606b1e14 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Fri, 7 Oct 2022 12:06:58 +0200 Subject: [PATCH 14/16] Change the default NameMappe to use a `HumanReadableNameMapper` --- src/server.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/server.cpp b/src/server.cpp index 19d577b8f..425e88dba 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -32,7 +32,7 @@ namespace kiwix { Server::Configuration::Configuration(std::shared_ptr library, std::shared_ptr nameMapper) : mp_library(library), - mp_nameMapper(nameMapper ? nameMapper : std::make_shared()) + mp_nameMapper(nameMapper ? nameMapper : std::make_shared(*library, true)) {} Server::Configuration& Server::Configuration::setRoot(const std::string& root) From 1fcc2ad709ec7f0120d5bdad242bfcf20d7a6578 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Fri, 7 Oct 2022 16:20:24 +0200 Subject: [PATCH 15/16] Add a `Server::isRunning` to know if the server is running or not. --- include/server.h | 5 +++++ src/server.cpp | 7 +++++++ src/server/internalServer.cpp | 5 +++++ src/server/internalServer.h | 1 + 4 files changed, 18 insertions(+) diff --git a/include/server.h b/include/server.h index 5d3677796..0ce613f21 100644 --- a/include/server.h +++ b/include/server.h @@ -117,6 +117,11 @@ namespace kiwix */ void stop(); + /** + * Tell if the server is running or not. + */ + bool isRunning(); + int getPort(); std::string getAddress(); diff --git a/src/server.cpp b/src/server.cpp index 425e88dba..4388f471e 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -61,6 +61,13 @@ void Server::stop() { } } +bool Server::isRunning() { + if (!mp_server) { + return false; + } + return mp_server->isRunning(); +} + int Server::getPort() { return mp_server->getPort(); diff --git a/src/server/internalServer.cpp b/src/server/internalServer.cpp index 7eece05b5..c4d2ae02d 100644 --- a/src/server/internalServer.cpp +++ b/src/server/internalServer.cpp @@ -415,8 +415,13 @@ bool InternalServer::start() { void InternalServer::stop() { MHD_stop_daemon(mp_daemon); + mp_daemon = nullptr; } +bool InternalServer::isRunning() const +{ + return mp_daemon != nullptr; +} static MHD_Result staticHandlerCallback(void* cls, struct MHD_Connection* connection, const char* url, diff --git a/src/server/internalServer.h b/src/server/internalServer.h index d44434e0a..efd25a928 100644 --- a/src/server/internalServer.h +++ b/src/server/internalServer.h @@ -104,6 +104,7 @@ class InternalServer : Server::Configuration { void** cont_cls); bool start(); void stop(); + bool isRunning() const; std::string getAddress() { return m_addr; } int getPort() { return m_port; } From d1124704f98e1bf1994c91ab6c0110b88ed46377 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Fri, 7 Oct 2022 18:03:25 +0200 Subject: [PATCH 16/16] Make `kiwix::Server` a simple wrapper around `kiwix::InternalServer`. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `kiwix::Server` is now a simple exposition of a public API on top of the private `InternalServer`. --- include/server.h | 9 +++++++-- src/server.cpp | 12 ++---------- 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/include/server.h b/include/server.h index 0ce613f21..b79c24699 100644 --- a/include/server.h +++ b/include/server.h @@ -104,7 +104,6 @@ namespace kiwix * @param library The library to serve. */ explicit Server(const Configuration& configuration); - virtual ~Server(); /** @@ -122,11 +121,17 @@ namespace kiwix */ bool isRunning(); + /** + * Get the port of the server + */ int getPort(); + + /** + * Get the ipAddress of the server + */ std::string getAddress(); protected: - Configuration m_configuration; std::unique_ptr mp_server; }; } diff --git a/src/server.cpp b/src/server.cpp index 4388f471e..61668f4a5 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -42,29 +42,21 @@ Server::Configuration& Server::Configuration::setRoot(const std::string& root) } Server::Server(const Server::Configuration& configuration) : - m_configuration(configuration), - mp_server(nullptr) + mp_server(new InternalServer(configuration)) { } Server::~Server() = default; bool Server::start() { - mp_server.reset(new InternalServer(m_configuration)); return mp_server->start(); } void Server::stop() { - if (mp_server) { - mp_server->stop(); - mp_server.reset(nullptr); - } + mp_server->stop(); } bool Server::isRunning() { - if (!mp_server) { - return false; - } return mp_server->isRunning(); }