From 090c2fd31a329be3de0ff5a1ce19af9fc5dcf829 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Mon, 21 Mar 2022 17:25:58 +0100 Subject: [PATCH 01/12] Move LibraryBase out of public API. We use composition instead of inheritance to implement Library. --- include/library.h | 47 ++++---------------- src/library.cpp | 110 ++++++++++++++++++++++++++++------------------ 2 files changed, 76 insertions(+), 81 deletions(-) diff --git a/include/library.h b/include/library.h index dc4463f6..1763de7d 100644 --- a/include/library.h +++ b/include/library.h @@ -140,51 +140,16 @@ private: // functions bool accept(const Book& book) const; }; -/** - * This class is not part of the libkiwix API. Its only purpose is - * to simplify the implementation of the Library's move operations - * and avoid bugs should new data members be added to Library. - */ -class LibraryBase -{ -protected: // types - typedef uint64_t LibraryRevision; - - struct Entry : Book - { - LibraryRevision lastUpdatedRevision = 0; - - // May also keep the Archive and Reader pointers here and get - // rid of the m_readers and m_archives data members in Library - }; - -protected: // data - LibraryRevision m_revision; - std::map m_books; - std::map> m_readers; - std::map> m_archives; - std::vector m_bookmarks; - class BookDB; - std::unique_ptr m_bookDB; - -protected: // functions - LibraryBase(); - ~LibraryBase(); - - LibraryBase(LibraryBase&& ); - LibraryBase& operator=(LibraryBase&& ); -}; - /** * A Library store several books. */ -class Library : private LibraryBase +class Library { // all data fields must be added in LibraryBase mutable std::mutex m_mutex; public: - typedef LibraryRevision Revision; + typedef uint64_t Revision; typedef std::vector BookIdCollection; typedef std::map AttributeCounts; @@ -351,7 +316,7 @@ class Library : private LibraryBase * * @return Current revision of the library. */ - LibraryRevision getRevision() const; + Revision getRevision() const; /** * Remove books that have not been updated since the specified revision. @@ -359,13 +324,14 @@ class Library : private LibraryBase * @param rev the library revision to use * @return Count of books that were removed by this operation. */ - uint32_t removeBooksNotUpdatedSince(LibraryRevision rev); + uint32_t removeBooksNotUpdatedSince(Revision rev); friend class OPDSDumper; friend class libXMLDumper; private: // types typedef const std::string& (Book::*BookStrPropMemFn)() const; + struct Impl; private: // functions AttributeCounts getBookAttributeCounts(BookStrPropMemFn p) const; @@ -373,6 +339,9 @@ private: // functions BookIdCollection filterViaBookDB(const Filter& filter) const; void updateBookDB(const Book& book); void dropReader(const std::string& bookId); + +private: //data + std::unique_ptr mp_impl; }; } diff --git a/src/library.cpp b/src/library.cpp index bf62c418..a1b66668 100644 --- a/src/library.cpp +++ b/src/library.cpp @@ -58,66 +58,92 @@ bool booksReferToTheSameArchive(const Book& book1, const Book& book2) } // unnamed namespace -class LibraryBase::BookDB : public Xapian::WritableDatabase +struct Library::Impl +{ + struct Entry : Book + { + Library::Revision lastUpdatedRevision = 0; + + // May also keep the Archive and Reader pointers here and get + // rid of the m_readers and m_archives data members in Library + }; + + Library::Revision m_revision; + std::map m_books; + std::map> m_readers; + std::map> m_archives; + std::vector m_bookmarks; + class BookDB; + std::unique_ptr m_bookDB; + + Impl(); + ~Impl(); + + Impl(Impl&& ); + Impl& operator=(Impl&& ); +}; + + +class Library::Impl::BookDB : public Xapian::WritableDatabase { public: BookDB() : Xapian::WritableDatabase("", Xapian::DB_BACKEND_INMEMORY) {} }; -LibraryBase::LibraryBase() +Library::Impl::Impl() : m_bookDB(new BookDB) { } -LibraryBase::~LibraryBase() +Library::Impl::~Impl() { } -LibraryBase::LibraryBase(LibraryBase&& ) = default; -LibraryBase& LibraryBase::operator=(LibraryBase&& ) = default; +Library::Impl::Impl(Library::Impl&& ) = default; +Library::Impl& Library::Impl::operator=(Library::Impl&& ) = default; + + /* Constructor */ Library::Library() + : mp_impl(new Library::Impl) { } Library::Library(Library&& other) - : LibraryBase(std::move(other)) + : mp_impl(std::move(other.mp_impl)) { } Library& Library::operator=(Library&& other) { - LibraryBase::operator=(std::move(other)); + mp_impl = std::move(other.mp_impl); return *this; } /* Destructor */ -Library::~Library() -{ -} - +Library::~Library() = default; bool Library::addBook(const Book& book) { std::lock_guard lock(m_mutex); - ++m_revision; + ++mp_impl->m_revision; /* Try to find it */ updateBookDB(book); try { - auto& oldbook = m_books.at(book.getId()); + auto& oldbook = mp_impl->m_books.at(book.getId()); if ( ! booksReferToTheSameArchive(oldbook, book) ) { dropReader(book.getId()); } oldbook.update(book); // XXX: This may have no effect if oldbook is readonly // XXX: Then m_bookDB will become out-of-sync with // XXX: the real contents of the library. - oldbook.lastUpdatedRevision = m_revision; + oldbook.lastUpdatedRevision = mp_impl->m_revision; return false; } catch (std::out_of_range&) { - Entry& newEntry = m_books[book.getId()]; + auto& newEntry = mp_impl->m_books[book.getId()]; static_cast(newEntry) = book; - newEntry.lastUpdatedRevision = m_revision; + newEntry.lastUpdatedRevision = mp_impl->m_revision; return true; } } @@ -125,15 +151,15 @@ bool Library::addBook(const Book& book) void Library::addBookmark(const Bookmark& bookmark) { std::lock_guard lock(m_mutex); - m_bookmarks.push_back(bookmark); + mp_impl->m_bookmarks.push_back(bookmark); } bool Library::removeBookmark(const std::string& zimId, const std::string& url) { std::lock_guard lock(m_mutex); - for(auto it=m_bookmarks.begin(); it!=m_bookmarks.end(); it++) { + for(auto it=mp_impl->m_bookmarks.begin(); it!=mp_impl->m_bookmarks.end(); it++) { if (it->getBookId() == zimId && it->getUrl() == url) { - m_bookmarks.erase(it); + mp_impl->m_bookmarks.erase(it); return true; } } @@ -143,30 +169,30 @@ bool Library::removeBookmark(const std::string& zimId, const std::string& url) void Library::dropReader(const std::string& id) { - m_readers.erase(id); - m_archives.erase(id); + mp_impl->m_readers.erase(id); + mp_impl->m_archives.erase(id); } bool Library::removeBookById(const std::string& id) { std::lock_guard lock(m_mutex); - m_bookDB->delete_document("Q" + id); + mp_impl->m_bookDB->delete_document("Q" + id); dropReader(id); - return m_books.erase(id) == 1; + return mp_impl->m_books.erase(id) == 1; } Library::Revision Library::getRevision() const { std::lock_guard lock(m_mutex); - return m_revision; + return mp_impl->m_revision; } -uint32_t Library::removeBooksNotUpdatedSince(LibraryRevision libraryRevision) +uint32_t Library::removeBooksNotUpdatedSince(Revision libraryRevision) { BookIdCollection booksToRemove; { std::lock_guard lock(m_mutex); - for ( const auto& entry : m_books) { + for ( const auto& entry : mp_impl->m_books) { if ( entry.second.lastUpdatedRevision <= libraryRevision ) { booksToRemove.push_back(entry.first); } @@ -185,7 +211,7 @@ const Book& Library::getBookById(const std::string& id) const { // XXX: Doesn't make sense to lock this operation since it cannot // XXX: guarantee thread-safety because of its return type - return m_books.at(id); + return mp_impl->m_books.at(id); } Book Library::getBookByIdThreadSafe(const std::string& id) const @@ -198,7 +224,7 @@ const Book& Library::getBookByPath(const std::string& path) const { // XXX: Doesn't make sense to lock this operation since it cannot // XXX: guarantee thread-safety because of its return type - for(auto& it: m_books) { + for(auto& it: mp_impl->m_books) { auto& book = it.second; if (book.getPath() == path) return book; @@ -212,7 +238,7 @@ std::shared_ptr Library::getReaderById(const std::string& id) { try { std::lock_guard lock(m_mutex); - return m_readers.at(id); + return mp_impl->m_readers.at(id); } catch (std::out_of_range& e) {} const auto archive = getArchiveById(id); @@ -221,7 +247,7 @@ std::shared_ptr Library::getReaderById(const std::string& id) const shared_ptr reader(new Reader(archive, true)); std::lock_guard lock(m_mutex); - m_readers[id] = reader; + mp_impl->m_readers[id] = reader; return reader; } @@ -229,7 +255,7 @@ std::shared_ptr Library::getArchiveById(const std::string& id) { std::lock_guard lock(m_mutex); try { - return m_archives.at(id); + return mp_impl->m_archives.at(id); } catch (std::out_of_range& e) {} auto book = getBookById(id); @@ -237,7 +263,7 @@ std::shared_ptr Library::getArchiveById(const std::string& id) return nullptr; auto sptr = make_shared(book.getPath()); - m_archives[id] = sptr; + mp_impl->m_archives[id] = sptr; return sptr; } @@ -246,7 +272,7 @@ unsigned int Library::getBookCount(const bool localBooks, { std::lock_guard lock(m_mutex); unsigned int result = 0; - for (auto& pair: m_books) { + for (auto& pair: mp_impl->m_books) { auto& book = pair.second; if ((!book.getPath().empty() && localBooks) || (book.getPath().empty() && remoteBooks)) { @@ -284,7 +310,7 @@ Library::AttributeCounts Library::getBookAttributeCounts(BookStrPropMemFn p) con std::lock_guard lock(m_mutex); AttributeCounts propValueCounts; - for (const auto& pair: m_books) { + for (const auto& pair: mp_impl->m_books) { const auto& book = pair.second; if (book.getOrigId().empty()) { propValueCounts[(book.*p)()] += 1; @@ -317,7 +343,7 @@ std::vector Library::getBooksCategories() const std::lock_guard lock(m_mutex); std::set categories; - for (const auto& pair: m_books) { + for (const auto& pair: mp_impl->m_books) { const auto& book = pair.second; const auto& c = book.getCategory(); if ( !c.empty() ) { @@ -341,12 +367,12 @@ std::vector Library::getBooksPublishers() const const std::vector Library::getBookmarks(bool onlyValidBookmarks) const { if (!onlyValidBookmarks) { - return m_bookmarks; + return mp_impl->m_bookmarks; } std::vector validBookmarks; auto booksId = getBooksIds(); std::lock_guard lock(m_mutex); - for(auto& bookmark:m_bookmarks) { + for(auto& bookmark:mp_impl->m_bookmarks) { if (std::find(booksId.begin(), booksId.end(), bookmark.getBookId()) != booksId.end()) { validBookmarks.push_back(bookmark); } @@ -359,7 +385,7 @@ Library::BookIdCollection Library::getBooksIds() const std::lock_guard lock(m_mutex); BookIdCollection bookIds; - for (auto& pair: m_books) { + for (auto& pair: mp_impl->m_books) { bookIds.push_back(pair.first); } @@ -405,7 +431,7 @@ void Library::updateBookDB(const Book& book) doc.set_data(book.getId()); - m_bookDB->replace_document(idterm, doc); + mp_impl->m_bookDB->replace_document(idterm, doc); } namespace @@ -538,9 +564,9 @@ Library::BookIdCollection Library::filterViaBookDB(const Filter& filter) const BookIdCollection bookIds; std::lock_guard lock(m_mutex); - Xapian::Enquire enquire(*m_bookDB); + Xapian::Enquire enquire(*mp_impl->m_bookDB); enquire.set_query(query); - const auto results = enquire.get_mset(0, m_books.size()); + const auto results = enquire.get_mset(0, mp_impl->m_books.size()); for ( auto it = results.begin(); it != results.end(); ++it ) { bookIds.push_back(it.get_document().get_data()); } @@ -554,7 +580,7 @@ Library::BookIdCollection Library::filter(const Filter& filter) const const auto preliminaryResult = filterViaBookDB(filter); std::lock_guard lock(m_mutex); for(auto id : preliminaryResult) { - if(filter.accept(m_books.at(id))) { + if(filter.accept(mp_impl->m_books.at(id))) { result.push_back(id); } } From aa1f73472dd498ee8d21d265b034503cd1553dbd Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Tue, 26 Apr 2022 17:15:39 +0200 Subject: [PATCH 02/12] Remove unecessary BookDB helper class. It was needed to not expose Xapian in public header. Now we can remove it and directly use a Xapian db. --- src/library.cpp | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/src/library.cpp b/src/library.cpp index a1b66668..95a3dbf0 100644 --- a/src/library.cpp +++ b/src/library.cpp @@ -73,8 +73,7 @@ struct Library::Impl std::map> m_readers; std::map> m_archives; std::vector m_bookmarks; - class BookDB; - std::unique_ptr m_bookDB; + Xapian::WritableDatabase m_bookDB; Impl(); ~Impl(); @@ -83,15 +82,8 @@ struct Library::Impl Impl& operator=(Impl&& ); }; - -class Library::Impl::BookDB : public Xapian::WritableDatabase -{ -public: - BookDB() : Xapian::WritableDatabase("", Xapian::DB_BACKEND_INMEMORY) {} -}; - Library::Impl::Impl() - : m_bookDB(new BookDB) + : m_bookDB("", Xapian::DB_BACKEND_INMEMORY) { } @@ -176,7 +168,7 @@ void Library::dropReader(const std::string& id) bool Library::removeBookById(const std::string& id) { std::lock_guard lock(m_mutex); - mp_impl->m_bookDB->delete_document("Q" + id); + mp_impl->m_bookDB.delete_document("Q" + id); dropReader(id); return mp_impl->m_books.erase(id) == 1; } @@ -431,7 +423,7 @@ void Library::updateBookDB(const Book& book) doc.set_data(book.getId()); - mp_impl->m_bookDB->replace_document(idterm, doc); + mp_impl->m_bookDB.replace_document(idterm, doc); } namespace @@ -564,7 +556,7 @@ Library::BookIdCollection Library::filterViaBookDB(const Filter& filter) const BookIdCollection bookIds; std::lock_guard lock(m_mutex); - Xapian::Enquire enquire(*mp_impl->m_bookDB); + Xapian::Enquire enquire(mp_impl->m_bookDB); enquire.set_query(query); const auto results = enquire.get_mset(0, mp_impl->m_books.size()); for ( auto it = results.begin(); it != results.end(); ++it ) { From 717c39f2ef26cf81909690cefb5fa115e168ca1d Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Tue, 26 Apr 2022 14:41:54 +0200 Subject: [PATCH 03/12] Better ExtractFromString - Throw a exception if we cannot extract from string. (We throw the same exception as `std::sto*`) - Add a specialization to extract string from string - Add some unit test --- src/tools/stringTools.cpp | 5 +++++ src/tools/stringTools.h | 7 +++++++ test/stringTools.cpp | 23 ++++++++++++++++++++++- 3 files changed, 34 insertions(+), 1 deletion(-) diff --git a/src/tools/stringTools.cpp b/src/tools/stringTools.cpp index 6d6e1d39..e7cb851e 100644 --- a/src/tools/stringTools.cpp +++ b/src/tools/stringTools.cpp @@ -405,3 +405,8 @@ std::vector kiwix::getTitleVariants(const std::string& title) { variants.push_back(kiwix::toTitle(title)); return variants; } + +template<> +std::string kiwix::extractFromString(const std::string& str) { + return str; +} diff --git a/src/tools/stringTools.h b/src/tools/stringTools.h index 9b5cf362..33754857 100644 --- a/src/tools/stringTools.h +++ b/src/tools/stringTools.h @@ -25,6 +25,7 @@ #include #include #include +#include namespace kiwix { @@ -65,9 +66,15 @@ T extractFromString(const std::string& str) { std::istringstream iss(str); T ret; iss >> ret; + if(iss.fail() || !iss.eof()) { + throw std::invalid_argument("no conversion"); + } return ret; } +template<> +std::string extractFromString(const std::string& str); + bool startsWith(const std::string& base, const std::string& start); std::vector getTitleVariants(const std::string& title); diff --git a/test/stringTools.cpp b/test/stringTools.cpp index af9c25bd..d9167093 100644 --- a/test/stringTools.cpp +++ b/test/stringTools.cpp @@ -18,11 +18,11 @@ */ #include "gtest/gtest.h" +#include "../src/tools/stringTools.h" #include #include namespace kiwix { -std::string join(const std::vector& list, const std::string& sep); std::vector split(const std::string& base, const std::string& sep, bool trimEmpty, bool keepDelim); }; @@ -58,4 +58,25 @@ TEST(stringTools, split) ASSERT_EQ(split(";a;b=;c=d;", ";=", false, true), list6); } +TEST(stringTools, extractFromString) +{ + ASSERT_EQ(extractFromString("55"), 55); + ASSERT_EQ(extractFromString("-55"), -55); + ASSERT_EQ(extractFromString("-55.0"), -55.0); + ASSERT_EQ(extractFromString("1"), true); + ASSERT_EQ(extractFromString("0"), false); + ASSERT_EQ(extractFromString("55"), "55"); + ASSERT_EQ(extractFromString("foo"), "foo"); + ASSERT_EQ(extractFromString("foo bar"), "foo bar"); + +// While spec says that >> operator should set the value to std::numeric_limits::max() +// and set the failbit of the stream (and so detect the error), the gcc implementation (?) +// set the value to (std::numeric_limits::max()-55) and doesn't set the failbit. +// ASSERT_THROW(extractFromString("-55"), std::invalid_argument); + + ASSERT_THROW(extractFromString("-55.0"), std::invalid_argument); + ASSERT_THROW(extractFromString("55 foo"), std::invalid_argument); + ASSERT_THROW(extractFromString("3.14.5"), std::invalid_argument); +} + }; From f42f6a60df9f200a11c8a9d38d75b908c61de4a1 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Wed, 30 Mar 2022 16:24:21 +0200 Subject: [PATCH 04/12] Use extractFromString to parse request argument. On top of reusing code, it throw a exception if we cannot convert given argument in the type we want. --- src/server/request_context.h | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/server/request_context.h b/src/server/request_context.h index 79b0a60e..2c4c8902 100644 --- a/src/server/request_context.h +++ b/src/server/request_context.h @@ -28,6 +28,7 @@ #include #include "byte_range.h" +#include "tools/stringTools.h" extern "C" { #include "microhttpd_wrapper.h" @@ -68,10 +69,7 @@ class RequestContext { std::string get_header(const std::string& name) const; template T get_argument(const std::string& name) const { - std::istringstream stream(arguments.at(name)); - T v; - stream >> v; - return v; + return extractFromString(arguments.at(name)); } template From 4695f47dd27a6f1f8a388298dc854987ffb9ee3f Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Wed, 27 Apr 2022 15:58:37 +0200 Subject: [PATCH 05/12] Introduce operator+= to simplify response creation. --- src/server/internalServer.cpp | 13 +++++++------ src/server/response.cpp | 13 +++++++++++++ src/server/response.h | 3 +++ 3 files changed, 23 insertions(+), 6 deletions(-) diff --git a/src/server/internalServer.cpp b/src/server/internalServer.cpp index 95f999f9..abafbba3 100644 --- a/src/server/internalServer.cpp +++ b/src/server/internalServer.cpp @@ -633,12 +633,13 @@ std::unique_ptr InternalServer::handle_search(const RequestContext& re // 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); - return HTTPErrorHtmlResponse(*this, request, MHD_HTTP_NOT_FOUND, - "fulltext-search-unavailable", - "404-page-heading", - cssUrl) - + nonParameterizedMessage("no-search-results") - + TaskbarInfo(searchInfo.bookName, archive.get()); + HTTPErrorHtmlResponse response(*this, request, MHD_HTTP_NOT_FOUND, + "fulltext-search-unavailable", + "404-page-heading", + cssUrl); + response += nonParameterizedMessage("no-search-results"); + response += TaskbarInfo(searchInfo.bookName, archive.get()); + return response; } diff --git a/src/server/response.cpp b/src/server/response.cpp index ae80a584..5543499f 100644 --- a/src/server/response.cpp +++ b/src/server/response.cpp @@ -194,6 +194,12 @@ HTTPErrorHtmlResponse& HTTPErrorHtmlResponse::operator+(const ParameterizedMessa return *this + details.getText(m_request.get_user_language()); } +HTTPErrorHtmlResponse& HTTPErrorHtmlResponse::operator+=(const ParameterizedMessage& details) +{ + // operator+() is already a state-modifying operator (akin to operator+=) + return *this + details; +} + HTTP400HtmlResponse::HTTP400HtmlResponse(const InternalServer& server, const RequestContext& request) @@ -246,6 +252,13 @@ ContentResponseBlueprint& ContentResponseBlueprint::operator+(const TaskbarInfo& return *this; } +ContentResponseBlueprint& ContentResponseBlueprint::operator+=(const TaskbarInfo& taskbarInfo) +{ + // operator+() is already a state-modifying operator (akin to operator+=) + return *this + taskbarInfo; +} + + std::unique_ptr Response::build_416(const InternalServer& server, size_t resourceLength) { auto response = Response::build(server); diff --git a/src/server/response.h b/src/server/response.h index 2eba46d2..5c14e778 100644 --- a/src/server/response.h +++ b/src/server/response.h @@ -165,6 +165,7 @@ public: // functions ContentResponseBlueprint& operator+(const TaskbarInfo& taskbarInfo); + ContentResponseBlueprint& operator+=(const TaskbarInfo& taskbarInfo); protected: // functions std::string getMessage(const std::string& msgId) const; @@ -190,8 +191,10 @@ struct HTTPErrorHtmlResponse : ContentResponseBlueprint const std::string& cssUrl = ""); using ContentResponseBlueprint::operator+; + using ContentResponseBlueprint::operator+=; HTTPErrorHtmlResponse& operator+(const std::string& msg); HTTPErrorHtmlResponse& operator+(const ParameterizedMessage& errorDetails); + HTTPErrorHtmlResponse& operator+=(const ParameterizedMessage& errorDetails); }; class UrlNotFoundMsg {}; From 52c12b0c2f44fbff613d11d30a8f3b710b73a5fc Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Wed, 20 Apr 2022 10:58:30 +0200 Subject: [PATCH 06/12] Introduce `Library::Impl::getBookCount` We simply introduce a `getBookCount` which is not protected by a lock. --- src/library.cpp | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/src/library.cpp b/src/library.cpp index 95a3dbf0..c5b68a40 100644 --- a/src/library.cpp +++ b/src/library.cpp @@ -75,6 +75,8 @@ struct Library::Impl std::vector m_bookmarks; Xapian::WritableDatabase m_bookDB; + unsigned int getBookCount(const bool localBooks, const bool remoteBooks) const; + Impl(); ~Impl(); @@ -94,7 +96,19 @@ Library::Impl::~Impl() Library::Impl::Impl(Library::Impl&& ) = default; Library::Impl& Library::Impl::operator=(Library::Impl&& ) = default; - +unsigned int +Library::Impl::getBookCount(const bool localBooks, const bool remoteBooks) const +{ + unsigned int result = 0; + for (auto& pair: m_books) { + auto& book = pair.second; + if ((!book.getPath().empty() && localBooks) + || (book.getPath().empty() && remoteBooks)) { + result++; + } + } + return result; +} /* Constructor */ Library::Library() @@ -263,15 +277,7 @@ unsigned int Library::getBookCount(const bool localBooks, const bool remoteBooks) const { std::lock_guard lock(m_mutex); - unsigned int result = 0; - for (auto& pair: mp_impl->m_books) { - auto& book = pair.second; - if ((!book.getPath().empty() && localBooks) - || (book.getPath().empty() && remoteBooks)) { - result++; - } - } - return result; + return mp_impl->getBookCount(localBooks, remoteBooks); } bool Library::writeToFile(const std::string& path) const From 288b4ae7dfaddcd9f11857461d2228f4d4ca374c Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Tue, 26 Apr 2022 17:23:39 +0200 Subject: [PATCH 07/12] Fix count of remote books in `Library::Impl::getBookCount` --- src/library.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/library.cpp b/src/library.cpp index c5b68a40..f7705940 100644 --- a/src/library.cpp +++ b/src/library.cpp @@ -103,7 +103,7 @@ Library::Impl::getBookCount(const bool localBooks, const bool remoteBooks) const for (auto& pair: m_books) { auto& book = pair.second; if ((!book.getPath().empty() && localBooks) - || (book.getPath().empty() && remoteBooks)) { + || (!book.getUrl().empty() && remoteBooks)) { result++; } } From cb62da65c3e6416603e2d9e9abd0f16ccd84980f Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Tue, 22 Mar 2022 18:44:01 +0100 Subject: [PATCH 08/12] Raise a exception if something went wrong in the template rendering. --- src/search_renderer.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/search_renderer.cpp b/src/search_renderer.cpp index f40b45e7..34521e3d 100644 --- a/src/search_renderer.cpp +++ b/src/search_renderer.cpp @@ -164,6 +164,9 @@ std::string SearchRenderer::getHtml() std::stringstream ss; tmpl.render(allData, [&ss](const std::string& str) { ss << str; }); + if (!tmpl.is_valid()) { + throw std::runtime_error("Error while rendering search results: " + tmpl.error_message()); + } return ss.str(); } From bbdde93f4927e55d4f12f28ce0bc627bf3b60a95 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Mon, 23 May 2022 19:12:17 +0200 Subject: [PATCH 09/12] Introduce a pagination object to render search result. --- src/search_renderer.cpp | 105 +++++++++----- static/templates/search_result.html | 28 +--- test/meson.build | 4 +- test/server.cpp | 11 -- test/server_helper.cpp | 215 ++++++++++++++++++++++++++++ 5 files changed, 294 insertions(+), 69 deletions(-) create mode 100644 test/server_helper.cpp diff --git a/src/search_renderer.cpp b/src/search_renderer.cpp index 34521e3d..34478dca 100644 --- a/src/search_renderer.cpp +++ b/src/search_renderer.cpp @@ -86,8 +86,69 @@ void SearchRenderer::setSearchProtocolPrefix(const std::string& prefix) this->searchProtocolPrefix = prefix; } +kainjow::mustache::data buildPagination( + unsigned int pageLength, + unsigned int resultsCount, + unsigned int resultsStart +) +{ + assert(pageLength!=0); + kainjow::mustache::data pagination; + kainjow::mustache::data pages{kainjow::mustache::data::type::list}; + + if (resultsCount == 0) { + // Easy case + pagination.set("itemsPerPage", to_string(pageLength)); + pagination.set("hasPages", false); + pagination.set("pages", pages); + return pagination; + } + + // First we want to display pages starting at a multiple of `pageLength` + // so, let's calculate the start index of the current page. + auto currentPage = resultsStart/pageLength; + auto lastPage = ((resultsCount-1)/pageLength); + auto lastPageStart = lastPage*pageLength; + auto nbPages = lastPage + 1; + + auto firstPageGenerated = currentPage > 4 ? currentPage-4 : 0; + auto lastPageGenerated = min(currentPage+4, lastPage); + + if (nbPages != 1) { + if (firstPageGenerated!=0) { + kainjow::mustache::data page; + page.set("label", "◀"); + page.set("start", to_string(0)); + page.set("current", false); + pages.push_back(page); + } + + for (auto i=firstPageGenerated; i<=lastPageGenerated; i++) { + kainjow::mustache::data page; + page.set("label", to_string(i+1)); + page.set("start", to_string(i*pageLength)); + page.set("current", bool(i == currentPage)); + pages.push_back(page); + } + + if (lastPageGenerated!=lastPage) { + kainjow::mustache::data page; + page.set("label", "▶"); + page.set("start", to_string(lastPageStart)); + page.set("current", false); + pages.push_back(page); + } + } + + pagination.set("itemsPerPage", to_string(pageLength)); + pagination.set("hasPages", firstPageGenerated < lastPageGenerated); + pagination.set("pages", pages); + return pagination; +} + std::string SearchRenderer::getHtml() { + // Build the results list kainjow::mustache::data results{kainjow::mustache::data::type::list}; for (auto it = m_srs.begin(); it != m_srs.end(); it++) { @@ -110,57 +171,31 @@ std::string SearchRenderer::getHtml() results.push_back(result); } - // pages - kainjow::mustache::data pages{kainjow::mustache::data::type::list}; - auto resultEnd = 0U; - auto currentPage = 0U; - auto pageStart = 0U; - auto pageEnd = 0U; - auto lastPageStart = 0U; - if (pageLength) { - currentPage = resultStart/pageLength; - pageStart = currentPage > 4 ? currentPage-4 : 0; - pageEnd = currentPage + 5; - if (pageEnd > estimatedResultCount / pageLength) { - pageEnd = (estimatedResultCount + pageLength - 1) / pageLength; - } - if (estimatedResultCount > pageLength) { - lastPageStart = ((estimatedResultCount-1)/pageLength)*pageLength; - } - } + // pagination + auto pagination = buildPagination( + pageLength, + estimatedResultCount, + resultStart + ); - resultEnd = resultStart+pageLength; //setting result end - - for (unsigned int i = pageStart; i < pageEnd; i++) { - kainjow::mustache::data page; - page.set("label", to_string(i + 1)); - page.set("start", to_string(i * pageLength)); - - if (i == currentPage) { - page.set("selected", true); - } - pages.push_back(page); - } + auto resultEnd = min(resultStart+pageLength, estimatedResultCount); std::string template_str = RESOURCE::templates::search_result_html; kainjow::mustache::mustache tmpl(template_str); kainjow::mustache::data allData; allData.set("results", results); - allData.set("pages", pages); allData.set("hasResults", estimatedResultCount != 0); - allData.set("hasPages", pageStart + 1 < pageEnd); allData.set("count", kiwix::beautifyInteger(estimatedResultCount)); allData.set("searchPattern", kiwix::encodeDiples(this->searchPattern)); allData.set("searchPatternEncoded", urlEncode(this->searchPattern)); allData.set("resultStart", to_string(resultStart + 1)); - allData.set("resultEnd", to_string(min(resultEnd, estimatedResultCount))); - allData.set("pageLength", to_string(pageLength)); - allData.set("resultLastPageStart", to_string(lastPageStart)); + allData.set("resultEnd", to_string(resultEnd)); allData.set("protocolPrefix", this->protocolPrefix); allData.set("searchProtocolPrefix", this->searchProtocolPrefix); allData.set("contentId", this->searchContent); + allData.set("pagination", pagination); std::stringstream ss; tmpl.render(allData, [&ss](const std::string& str) { ss << str; }); diff --git a/static/templates/search_result.html b/static/templates/search_result.html index ddf0ff2a..e252bacc 100644 --- a/static/templates/search_result.html +++ b/static/templates/search_result.html @@ -143,34 +143,18 @@
- {{#hasPages}} + {{#pagination.hasPages}}
    - {{#resultLastPageStart}} + {{#pagination.pages}}
  • - - ◀ - -
    • - {{/resultLastPageStart}} - {{#pages}} -
  • - + {{label}}
    • - {{/pages}} - {{#resultLastPageStart}} -
  • - - ▶ - -
    • - {{/resultLastPageStart}} + {{/pagination.pages}}
- {{/hasPages}} + {{/pagination.hasPages}}
diff --git a/test/meson.build b/test/meson.build index 1047ff0c..368144c5 100644 --- a/test/meson.build +++ b/test/meson.build @@ -9,7 +9,8 @@ tests = [ 'book', 'manager', 'name_mapper', - 'opds_catalog' + 'opds_catalog', + 'server_helper' ] if build_machine.system() != 'windows' @@ -59,6 +60,7 @@ if gtest_dep.found() and not meson.is_cross_build() # XXX: '#include ' includes the regex unit test binary test_exe = executable(test_name, [test_name+'.cpp'], implicit_include_directories: false, + include_directories : inc, link_with : kiwixlib, link_args: extra_link_args, dependencies : all_deps + [gtest_dep], diff --git a/test/server.cpp b/test/server.cpp index 72725a67..2a61db53 100644 --- a/test/server.cpp +++ b/test/server.cpp @@ -1849,7 +1849,6 @@ R"SEARCHRESULT( }, /* pagination */ { - { "◀", 0, false }, { "1", 0, true }, { "2", 5, false }, { "3", 10, false }, @@ -1874,7 +1873,6 @@ R"SEARCHRESULT( }, /* pagination */ { - { "◀", 0, false }, { "1", 0, false }, { "2", 5, true }, { "3", 10, false }, @@ -1900,7 +1898,6 @@ R"SEARCHRESULT( }, /* pagination */ { - { "◀", 0, false }, { "1", 0, false }, { "2", 5, false }, { "3", 10, true }, @@ -1927,7 +1924,6 @@ R"SEARCHRESULT( }, /* pagination */ { - { "◀", 0, false }, { "1", 0, false }, { "2", 5, false }, { "3", 10, false }, @@ -1955,7 +1951,6 @@ R"SEARCHRESULT( }, /* pagination */ { - { "◀", 0, false }, { "1", 0, false }, { "2", 5, false }, { "3", 10, false }, @@ -1965,7 +1960,6 @@ R"SEARCHRESULT( { "7", 30, false }, { "8", 35, false }, { "9", 40, false }, - { "▶", 40, false }, } }, @@ -1993,7 +1987,6 @@ R"SEARCHRESULT( { "7", 30, false }, { "8", 35, false }, { "9", 40, false }, - { "▶", 40, false }, } }, @@ -2020,7 +2013,6 @@ R"SEARCHRESULT( { "7", 30, true }, { "8", 35, false }, { "9", 40, false }, - { "▶", 40, false }, } }, @@ -2046,7 +2038,6 @@ R"SEARCHRESULT( { "7", 30, false }, { "8", 35, true }, { "9", 40, false }, - { "▶", 40, false }, } }, @@ -2070,7 +2061,6 @@ R"SEARCHRESULT( { "7", 30, false }, { "8", 35, false }, { "9", 40, true }, - { "▶", 40, false }, } }, @@ -2117,7 +2107,6 @@ R"SEARCHRESULT( { "7", 30, false }, { "8", 35, false }, { "9", 40, false }, - { "▶", 40, false }, } }, }; diff --git a/test/server_helper.cpp b/test/server_helper.cpp new file mode 100644 index 00000000..c8e04dea --- /dev/null +++ b/test/server_helper.cpp @@ -0,0 +1,215 @@ + +#include +#include "gtest/gtest.h" +#include "../src/tools/stringTools.h" + +namespace { + struct ExpectedPage { + ExpectedPage(const std::string& label, unsigned int start, bool current) + : label(label), + start(start), + current(current) + {} + + testing::AssertionResult isEqual(const kainjow::mustache::data& data) { + if (!data.is_object() + || data.get("label") == nullptr + || data.get("start") == nullptr + || data.get("current") == nullptr) { + return testing::AssertionFailure() << "data is not a valid object"; + } + + if (data.get("label")->string_value() != label) { + return testing::AssertionFailure() << data.get("label")->string_value() + << " is not equal to " + << label; + } + + if (data.get("start")->string_value() != kiwix::to_string(start)) { + return testing::AssertionFailure() << data.get("start")->string_value() + << " is not equal to " + << kiwix::to_string(start); + } + + if (current && !data.get("current")->is_true()) { + return testing::AssertionFailure() << "data is not true"; + } + if (!current && !data.get("current")->is_false()) { + return testing::AssertionFailure() << "data is not false"; + } + return testing::AssertionSuccess(); + } + + std::string label; + unsigned int start; + bool current; + }; + + class ExpectedPages : public std::vector + { + public: + ExpectedPages(std::vector&& v) + : std::vector(v) + {} + + testing::AssertionResult isEqual(const kainjow::mustache::data& data) { + if (empty()) { + if (data.is_empty_list()) { + return testing::AssertionSuccess(); + } else { + return testing::AssertionFailure() << "data is not an empty list."; + } + } + + if (! data.is_non_empty_list()) { + return testing::AssertionFailure() << "data is not a non empty list."; + } + auto& data_pages = data.list_value(); + if (size() != data_pages.size()) { + return testing::AssertionFailure() << "data (size " << data_pages.size() << ")" + << " and expected (size " << size() << ")" + << " must have the same size"; + } + auto it1 = begin(); + auto it2 = data_pages.begin(); + while (it1!=end()) { + auto result = it1->isEqual(*it2); + if(!result) { + return result; + } + it1++; it2++; + } + return testing::AssertionSuccess(); + } + }; +} + +namespace kiwix { +kainjow::mustache::data buildPagination( + unsigned int pageLength, + unsigned int resultsCount, + unsigned int resultsStart +); +} + +TEST(SearchRenderer, buildPagination) { + { + auto pagination = kiwix::buildPagination(10, 0, 0); + ASSERT_EQ(pagination.get("itemsPerPage")->string_value(), "10"); + ASSERT_TRUE(pagination.get("hasPages")->is_false()); + ASSERT_TRUE(ExpectedPages({}).isEqual(*pagination.get("pages"))); + } + { + auto pagination = kiwix::buildPagination(10, 1, 0); + ASSERT_EQ(pagination.get("itemsPerPage")->string_value(), "10"); + ASSERT_TRUE(pagination.get("hasPages")->is_false()); + ASSERT_TRUE(ExpectedPages({}).isEqual(*pagination.get("pages"))); + } + + { + auto pagination = kiwix::buildPagination(10, 10, 0); + ASSERT_EQ(pagination.get("itemsPerPage")->string_value(), "10"); + ASSERT_TRUE(pagination.get("hasPages")->is_false()); + ASSERT_TRUE(ExpectedPages({}).isEqual(*pagination.get("pages"))); + } + + { + auto pagination = kiwix::buildPagination(10, 11, 0); + ASSERT_EQ(pagination.get("itemsPerPage")->string_value(), "10"); + ASSERT_TRUE(pagination.get("hasPages")->is_true()); + ASSERT_TRUE(ExpectedPages({ + {"1", 0, true}, + {"2", 10, false}, + }).isEqual(*pagination.get("pages"))); + } + + { + auto pagination = kiwix::buildPagination(10, 20, 0); + ASSERT_EQ(pagination.get("itemsPerPage")->string_value(), "10"); + ASSERT_TRUE(pagination.get("hasPages")->is_true()); + ASSERT_TRUE(ExpectedPages({ + {"1", 0, true}, + {"2", 10, false}, + }).isEqual(*pagination.get("pages"))); + } + + { + auto pagination = kiwix::buildPagination(10, 21, 0); + ASSERT_EQ(pagination.get("itemsPerPage")->string_value(), "10"); + ASSERT_TRUE(pagination.get("hasPages")->is_true()); + ASSERT_TRUE(ExpectedPages({ + {"1", 0, true}, + {"2", 10, false}, + {"3", 20, false} + }).isEqual(*pagination.get("pages"))); + } + + { + auto pagination = kiwix::buildPagination(10, 21, 11); + ASSERT_EQ(pagination.get("itemsPerPage")->string_value(), "10"); + ASSERT_TRUE(pagination.get("hasPages")->is_true()); + ASSERT_TRUE(ExpectedPages({ + {"1", 0, false}, + {"2", 10, true}, + {"3", 20, false} + }).isEqual(*pagination.get("pages"))); + } + + { + auto pagination = kiwix::buildPagination(10, 21, 21); + ASSERT_EQ(pagination.get("itemsPerPage")->string_value(), "10"); + ASSERT_TRUE(pagination.get("hasPages")->is_true()); + ASSERT_TRUE(ExpectedPages({ + {"1", 0, false}, + {"2", 10, false}, + {"3", 20, true} + }).isEqual(*pagination.get("pages"))); + } + + { + auto pagination = kiwix::buildPagination(10, 200, 0); + ASSERT_EQ(pagination.get("itemsPerPage")->string_value(), "10"); + ASSERT_TRUE(pagination.get("hasPages")->is_true()); + ASSERT_TRUE(ExpectedPages({ + {"1", 0, true}, + {"2", 10, false}, + {"3", 20, false}, + {"4", 30, false}, + {"5", 40, false}, + {"▶", 190, false} + }).isEqual(*pagination.get("pages"))); + } + + { + auto pagination = kiwix::buildPagination(10, 200, 105); + ASSERT_EQ(pagination.get("itemsPerPage")->string_value(), "10"); + ASSERT_TRUE(pagination.get("hasPages")->is_true()); + ASSERT_TRUE(ExpectedPages({ + {"◀", 0, false}, + {"7", 60, false}, + {"8", 70, false}, + {"9", 80, false}, + {"10", 90, false}, + {"11", 100, true}, + {"12", 110, false}, + {"13", 120, false}, + {"14", 130, false}, + {"15", 140, false}, + {"▶", 190, false} + }).isEqual(*pagination.get("pages"))); + } + + { + auto pagination = kiwix::buildPagination(10, 200, 199); + ASSERT_EQ(pagination.get("itemsPerPage")->string_value(), "10"); + ASSERT_TRUE(pagination.get("hasPages")->is_true()); + ASSERT_TRUE(ExpectedPages({ + {"◀", 0, false}, + {"16", 150, false}, + {"17", 160, false}, + {"18", 170, false}, + {"19", 180, false}, + {"20", 190, true} + }).isEqual(*pagination.get("pages"))); + } +} From f0dd34b6db6844cbb6db021460968f31adb18d26 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Thu, 5 May 2022 17:42:16 +0200 Subject: [PATCH 10/12] Introduce buildQueryData helper in SearchRenderer --- include/search_renderer.h | 3 +++ src/search_renderer.cpp | 36 ++++++++++++++++++++++------- src/server/internalServer.cpp | 2 +- static/templates/search_result.html | 8 +++---- 4 files changed, 36 insertions(+), 13 deletions(-) diff --git a/include/search_renderer.h b/include/search_renderer.h index 627699a6..cd499d13 100644 --- a/include/search_renderer.h +++ b/include/search_renderer.h @@ -75,6 +75,9 @@ class SearchRenderer ~SearchRenderer(); + /** + * Set the search pattern used to do the search + */ void setSearchPattern(const std::string& pattern); /** diff --git a/src/search_renderer.cpp b/src/search_renderer.cpp index 34478dca..0381d424 100644 --- a/src/search_renderer.cpp +++ b/src/search_renderer.cpp @@ -58,7 +58,7 @@ SearchRenderer::SearchRenderer(zim::SearchResultSet srs, NameMapper* mapper, Lib mp_nameMapper(mapper), mp_library(library), protocolPrefix("zim://"), - searchProtocolPrefix("search://?"), + searchProtocolPrefix("search://"), estimatedResultCount(estimatedResultCount), resultStart(start) {} @@ -68,12 +68,12 @@ SearchRenderer::~SearchRenderer() = default; void SearchRenderer::setSearchPattern(const std::string& pattern) { - this->searchPattern = pattern; + searchPattern = pattern; } -void SearchRenderer::setSearchContent(const std::string& name) +void SearchRenderer::setSearchContent(const std::string& content) { - this->searchContent = name; + searchContent = content; } void SearchRenderer::setProtocolPrefix(const std::string& prefix) @@ -86,6 +86,21 @@ void SearchRenderer::setSearchProtocolPrefix(const std::string& prefix) this->searchProtocolPrefix = prefix; } +kainjow::mustache::data buildQueryData +( + const std::string& searchProtocolPrefix, + const std::string& pattern, + const std::string& searchContent +) { + kainjow::mustache::data query; + query.set("pattern", kiwix::encodeDiples(pattern)); + std::ostringstream ss; + ss << searchProtocolPrefix << "?pattern=" << urlEncode(pattern, true); + ss << "&content=" << urlEncode(searchContent, true); + query.set("unpaginatedQuery", ss.str()); + return query; +} + kainjow::mustache::data buildPagination( unsigned int pageLength, unsigned int resultsCount, @@ -181,6 +196,14 @@ std::string SearchRenderer::getHtml() auto resultEnd = min(resultStart+pageLength, estimatedResultCount); + kainjow::mustache::data query = buildQueryData( + searchProtocolPrefix, + searchPattern, + searchContent + ); + + + std::string template_str = RESOURCE::templates::search_result_html; kainjow::mustache::mustache tmpl(template_str); @@ -188,14 +211,11 @@ std::string SearchRenderer::getHtml() allData.set("results", results); allData.set("hasResults", estimatedResultCount != 0); allData.set("count", kiwix::beautifyInteger(estimatedResultCount)); - allData.set("searchPattern", kiwix::encodeDiples(this->searchPattern)); - allData.set("searchPatternEncoded", urlEncode(this->searchPattern)); allData.set("resultStart", to_string(resultStart + 1)); allData.set("resultEnd", to_string(resultEnd)); allData.set("protocolPrefix", this->protocolPrefix); - allData.set("searchProtocolPrefix", this->searchProtocolPrefix); - allData.set("contentId", this->searchContent); allData.set("pagination", pagination); + allData.set("query", query); std::stringstream ss; tmpl.render(allData, [&ss](const std::string& str) { ss << str; }); diff --git a/src/server/internalServer.cpp b/src/server/internalServer.cpp index abafbba3..efe39524 100644 --- a/src/server/internalServer.cpp +++ b/src/server/internalServer.cpp @@ -665,7 +665,7 @@ std::unique_ptr InternalServer::handle_search(const RequestContext& re renderer.setSearchPattern(searchInfo.pattern); renderer.setSearchContent(searchInfo.bookName); renderer.setProtocolPrefix(m_root + "/"); - renderer.setSearchProtocolPrefix(m_root + "/search?"); + renderer.setSearchProtocolPrefix(m_root + "/search"); renderer.setPageLength(pageLength); auto response = ContentResponse::build(*this, renderer.getHtml(), "text/html; charset=utf-8"); response->set_taskbar(searchInfo.bookName, archive.get()); diff --git a/static/templates/search_result.html b/static/templates/search_result.html index e252bacc..cca32af1 100644 --- a/static/templates/search_result.html +++ b/static/templates/search_result.html @@ -102,7 +102,7 @@ } - Search: {{searchPattern}} + Search: {{query.pattern}}
@@ -113,11 +113,11 @@ of {{count}} for - "{{{searchPattern}}}" + "{{{query.pattern}}}" {{/hasResults}} {{^hasResults}} - No results were found for "{{{searchPattern}}}" + No results were found for "{{{query.pattern}}}" {{/hasResults}}
@@ -148,7 +148,7 @@ {{#pagination.pages}}
  • + href="{{{query.unpaginatedQuery}}}&start={{start}}&pageLength={{pagination.itemsPerPage}}"> {{label}}
    • From aad95e34130bbac9d773ca45f9593a90154ba8f3 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Wed, 23 Mar 2022 12:01:39 +0100 Subject: [PATCH 11/12] Introduce a results intermediate object in the template rendering. Url in href must not be html encoded. As we already url encode the path, it is ok to have `'` in the url. --- src/search_renderer.cpp | 33 +++++++++++------------------ static/templates/search_result.html | 18 ++++++++-------- test/server.cpp | 8 +++---- 3 files changed, 25 insertions(+), 34 deletions(-) diff --git a/src/search_renderer.cpp b/src/search_renderer.cpp index 0381d424..8f968556 100644 --- a/src/search_renderer.cpp +++ b/src/search_renderer.cpp @@ -164,28 +164,28 @@ kainjow::mustache::data buildPagination( std::string SearchRenderer::getHtml() { // Build the results list - kainjow::mustache::data results{kainjow::mustache::data::type::list}; - + kainjow::mustache::data items{kainjow::mustache::data::type::list}; for (auto it = m_srs.begin(); it != m_srs.end(); it++) { kainjow::mustache::data result; - result.set("title", it.getTitle()); - result.set("url", it.getPath()); - result.set("snippet", it.getSnippet()); std::string zim_id(it.getZimId()); - result.set("resultContentId", mp_nameMapper->getNameForId(zim_id)); - if (!mp_library) { - result.set("bookTitle", kainjow::mustache::data(false)); - } else { + result.set("title", it.getTitle()); + result.set("absolutePath", protocolPrefix + urlEncode(mp_nameMapper->getNameForId(zim_id), true) + "/" + urlEncode(it.getPath())); + result.set("snippet", it.getSnippet()); + if (mp_library) { result.set("bookTitle", mp_library->getBookById(zim_id).getTitle()); } - if (it.getWordCount() >= 0) { result.set("wordCount", kiwix::beautifyInteger(it.getWordCount())); } - results.push_back(result); + items.push_back(result); } - + kainjow::mustache::data results; + results.set("items", items); + results.set("count", kiwix::beautifyInteger(estimatedResultCount)); + results.set("hasResults", estimatedResultCount != 0); + results.set("start", kiwix::beautifyInteger(resultStart+1)); + results.set("end", kiwix::beautifyInteger(min(resultStart+pageLength, estimatedResultCount))); // pagination auto pagination = buildPagination( @@ -194,26 +194,17 @@ std::string SearchRenderer::getHtml() resultStart ); - auto resultEnd = min(resultStart+pageLength, estimatedResultCount); - kainjow::mustache::data query = buildQueryData( searchProtocolPrefix, searchPattern, searchContent ); - - std::string template_str = RESOURCE::templates::search_result_html; kainjow::mustache::mustache tmpl(template_str); kainjow::mustache::data allData; allData.set("results", results); - allData.set("hasResults", estimatedResultCount != 0); - allData.set("count", kiwix::beautifyInteger(estimatedResultCount)); - allData.set("resultStart", to_string(resultStart + 1)); - allData.set("resultEnd", to_string(resultEnd)); - allData.set("protocolPrefix", this->protocolPrefix); allData.set("pagination", pagination); allData.set("query", query); diff --git a/static/templates/search_result.html b/static/templates/search_result.html index cca32af1..93133c58 100644 --- a/static/templates/search_result.html +++ b/static/templates/search_result.html @@ -106,26 +106,26 @@
    - {{#hasResults}} + {{#results.hasResults}} Results - {{resultStart}}-{{resultEnd}} + {{results.start}}-{{results.end}} of - {{count}} + {{results.count}} for "{{{query.pattern}}}" - {{/hasResults}} - {{^hasResults}} + {{/results.hasResults}} + {{^results.hasResults}} No results were found for "{{{query.pattern}}}" - {{/hasResults}} + {{/results.hasResults}}
      - {{#results}} + {{#results.items}}
    • - + {{title}} {{#snippet}} @@ -138,7 +138,7 @@
      {{wordCount}} words
      {{/wordCount}}
      • - {{/results}} + {{/results.items}}
    diff --git a/test/server.cpp b/test/server.cpp index 2a61db53..4054a49b 100644 --- a/test/server.cpp +++ b/test/server.cpp @@ -1158,7 +1158,7 @@ R"SEARCHRESULT( )SEARCHRESULT", R"SEARCHRESULT( - + Catchin' Some Rays: The Music of Ray Charles ...jazz singer Roseanna Vitro, released in August 1997 on the Telarc Jazz label. Catchin' Some Rays: The Music of Ray Charles Studio album by Roseanna Vitro Released August 1997 Recorded March 26, 1997 at Sound on Sound, NYC April 4,1997 at Quad Recording Studios, NYC Genre Vocal jazz Length 61:00 Label Telarc Jazz CD-83419 Producer Paul Wickliffe Roseanna Vitro chronology Passion Dance (1996) Catchin' Some Rays: The Music of Ray Charles (1997) The Time of My Life: Roseanna Vitro Sings the Songs of...... @@ -1167,7 +1167,7 @@ R"SEARCHRESULT( )SEARCHRESULT", R"SEARCHRESULT( - + That's What I Say: John Scofield Plays the Music of Ray Charles That's What I Say: John Scofield Plays the Music of Ray Charles Studio album by John Scofield Released June 7, 2005 (2005-06-07) Recorded December 2004 Studio Avatar Studios, New York City Genre Jazz Length 65:21 Label Verve Producer Steve Jordan John Scofield chronology EnRoute: John Scofield Trio LIVE (2004) That's What I Say: John Scofield Plays the Music of Ray Charles (2005) Out Louder (2006) Professional ratings Review scores Source Rating Allmusic All About Jazz All About Jazz... @@ -1437,7 +1437,7 @@ R"SEARCHRESULT( )SEARCHRESULT", R"SEARCHRESULT( - + Don't Let the Sun Catch You Cryin' ...R&B Sides" and No. 95 on the Billboard Hot 100. It was also recorded by Jackie DeShannon on her 1965 album This is Jackie De Shannon, Paul McCartney on his 1990 live album Tripping the Live Fantastic, Jex Saarelaht and Kate Ceberano on their album Open the Door - Live at Mietta's (1992) and jazz singer Roseanna Vitro on her 1997 album Catchin’ Some Rays: The Music of Ray Charles. Karin Krog and Steve Kuhn include it on their 2005 album, Together Again. Steve Alaimo released a version in 1963... @@ -1446,7 +1446,7 @@ R"SEARCHRESULT( )SEARCHRESULT", R"SEARCHRESULT( - + I Don't Need No Doctor ...jazz guitar player John Scofield recorded a version for his album That's What I Say: John Scofield Plays the Music of Ray Charles in 2005, featuring the blues guitarist John Mayer on additional guitar and vocals. Mayer covered the song again with his band during his tour in summer 2007. A recorded live version from a Los Angeles show during that tour is available on Mayer's CD/DVD release Where the Light Is. A Ray Charles tribute album also provided the impetus for jazz singer Roseanna Vitro's...... From 66b24498005486f8310a2cf86471d5538b5ae760 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Thu, 21 Apr 2022 15:43:00 +0200 Subject: [PATCH 12/12] Remove unnecessary catch Catch of std::exception is already made in `handle_request` --- src/server/internalServer.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/server/internalServer.cpp b/src/server/internalServer.cpp index efe39524..0290d79a 100644 --- a/src/server/internalServer.cpp +++ b/src/server/internalServer.cpp @@ -674,10 +674,6 @@ std::unique_ptr InternalServer::handle_search(const RequestContext& re return HTTP400HtmlResponse(*this, request) + invalidUrlMsg + std::string(e.what()); - } catch (const std::exception& e) { - std::cerr << e.what() << std::endl; - return HTTP500HtmlResponse(*this, request) - + e.what(); } }