From b86ec8860e1a1bd3ec12f137b7c9ad0cd63ab687 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Wed, 5 Oct 2022 15:41:04 +0200 Subject: [PATCH] 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 | 57 +++++++++++++++++++++--- test/server.cpp | 7 +++ test/server_testing_tools.h | 14 ++++-- 7 files changed, 89 insertions(+), 24 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..807b1ca31 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..277d139a2 100644 --- a/test/library_server.cpp +++ b/test/library_server.cpp @@ -18,8 +18,13 @@ 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, "./test/library.xml")); + zfs1_.reset(new ZimFileServer(PORT, ZimFileServer::DEFAULT_OPTIONS, "./test/library.xml")); } void TearDown() override { @@ -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,4 +783,42 @@ TEST_F(LibraryServerTest, catalog_search_excludes_hidden_tags) #undef EXPECT_ZERO_RESULTS } +#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), + 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" + ); +} + + +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 2d1e1068a..d03f9e6bb 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); + EXPECT_EQ(404, zfs1_->GET("/ROOT/content/zimfile/A/index")->status); +} + 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);