Merge pull request #764 from kiwix/pre_multisearch

Preparatory work on multizim

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<std::string, Entry> m_books;
-  std::map<std::string, std::shared_ptr<Reader>> m_readers;
-  std::map<std::string, std::shared_ptr<zim::Archive>> m_archives;
-  std::vector<kiwix::Bookmark> m_bookmarks;
-  class BookDB;
-  std::unique_ptr<BookDB> 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<std::string> BookIdCollection;
   typedef std::map<std::string, int> 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<Impl> mp_impl;
 };
 
 }

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);
 
   /**

src/library.cpp

@@ -58,66 +58,98 @@ bool booksReferToTheSameArchive(const Book& book1, const Book& book2)
 
 } // unnamed namespace
 
-class LibraryBase::BookDB : public Xapian::WritableDatabase
+struct Library::Impl
 {
-public:
+  struct Entry : Book
-  BookDB() : Xapian::WritableDatabase("", Xapian::DB_BACKEND_INMEMORY) {}
+  {
+    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<std::string, Entry> m_books;
+  std::map<std::string, std::shared_ptr<Reader>> m_readers;
+  std::map<std::string, std::shared_ptr<zim::Archive>> m_archives;
+  std::vector<kiwix::Bookmark> m_bookmarks;
+  Xapian::WritableDatabase m_bookDB;
+
+  unsigned int getBookCount(const bool localBooks, const bool remoteBooks) const;
+
+  Impl();
+  ~Impl();
+
+  Impl(Impl&& );
+  Impl& operator=(Impl&& );
 };
 
-LibraryBase::LibraryBase()
+Library::Impl::Impl()
-  : m_bookDB(new BookDB)
+  : m_bookDB("", Xapian::DB_BACKEND_INMEMORY)
 {
 }
 
-LibraryBase::~LibraryBase()
+Library::Impl::~Impl()
 {
 }
 
-LibraryBase::LibraryBase(LibraryBase&& ) = default;
+Library::Impl::Impl(Library::Impl&& ) = default;
-LibraryBase& LibraryBase::operator=(LibraryBase&& ) = 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.getUrl().empty() && remoteBooks)) {
+      result++;
+    }
+  }
+  return result;
+}
 
 /* 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<std::mutex> 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<Book&>(newEntry) = book;
-    newEntry.lastUpdatedRevision = m_revision;
+    newEntry.lastUpdatedRevision = mp_impl->m_revision;
     return true;
   }
 }
@@ -125,15 +157,15 @@ bool Library::addBook(const Book& book)
 void Library::addBookmark(const Bookmark& bookmark)
 {
   std::lock_guard<std::mutex> 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<std::mutex> 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 +175,30 @@ bool Library::removeBookmark(const std::string& zimId, const std::string& url)
 
 void Library::dropReader(const std::string& id)
 {
-  m_readers.erase(id);
+  mp_impl->m_readers.erase(id);
-  m_archives.erase(id);
+  mp_impl->m_archives.erase(id);
 }
 
 bool Library::removeBookById(const std::string& id)
 {
   std::lock_guard<std::mutex> 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<std::mutex> 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<std::mutex> 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 +217,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 +230,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 +244,7 @@ std::shared_ptr<Reader> Library::getReaderById(const std::string& id)
 {
   try {
     std::lock_guard<std::mutex> 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 +253,7 @@ std::shared_ptr<Reader> Library::getReaderById(const std::string& id)
 
   const shared_ptr<Reader> reader(new Reader(archive, true));
   std::lock_guard<std::mutex> lock(m_mutex);
-  m_readers[id] = reader;
+  mp_impl->m_readers[id] = reader;
   return reader;
 }
 
@@ -229,7 +261,7 @@ std::shared_ptr<zim::Archive> Library::getArchiveById(const std::string& id)
 {
   std::lock_guard<std::mutex> 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 +269,7 @@ std::shared_ptr<zim::Archive> Library::getArchiveById(const std::string& id)
     return nullptr;
 
   auto sptr = make_shared<zim::Archive>(book.getPath());
-  m_archives[id] = sptr;
+  mp_impl->m_archives[id] = sptr;
   return sptr;
 }
 
@@ -245,15 +277,7 @@ unsigned int Library::getBookCount(const bool localBooks,
                                    const bool remoteBooks) const
 {
   std::lock_guard<std::mutex> lock(m_mutex);
-  unsigned int result = 0;
+  return mp_impl->getBookCount(localBooks, remoteBooks);
-  for (auto& pair: m_books) {
-    auto& book = pair.second;
-    if ((!book.getPath().empty() && localBooks)
-        || (book.getPath().empty() && remoteBooks)) {
-      result++;
-    }
-  }
-  return result;
 }
 
 bool Library::writeToFile(const std::string& path) const
@@ -284,7 +308,7 @@ Library::AttributeCounts Library::getBookAttributeCounts(BookStrPropMemFn p) con
   std::lock_guard<std::mutex> 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 +341,7 @@ std::vector<std::string> Library::getBooksCategories() const
   std::lock_guard<std::mutex> lock(m_mutex);
   std::set<std::string> 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 +365,12 @@ std::vector<std::string> Library::getBooksPublishers() const
 const std::vector<kiwix::Bookmark> Library::getBookmarks(bool onlyValidBookmarks) const
 {
   if (!onlyValidBookmarks) {
-    return m_bookmarks;
+    return mp_impl->m_bookmarks;
   }
   std::vector<kiwix::Bookmark> validBookmarks;
   auto booksId = getBooksIds();
   std::lock_guard<std::mutex> 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 +383,7 @@ Library::BookIdCollection Library::getBooksIds() const
   std::lock_guard<std::mutex> lock(m_mutex);
   BookIdCollection bookIds;
 
-  for (auto& pair: m_books) {
+  for (auto& pair: mp_impl->m_books) {
     bookIds.push_back(pair.first);
   }
 
@@ -405,7 +429,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 +562,9 @@ Library::BookIdCollection Library::filterViaBookDB(const Filter& filter) const
   BookIdCollection bookIds;
 
   std::lock_guard<std::mutex> 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 +578,7 @@ Library::BookIdCollection Library::filter(const Filter& filter) const
   const auto preliminaryResult = filterViaBookDB(filter);
   std::lock_guard<std::mutex> 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);
     }
   }

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,84 +86,133 @@ void SearchRenderer::setSearchProtocolPrefix(const std::string& prefix)
   this->searchProtocolPrefix = prefix;
 }
 
-std::string SearchRenderer::getHtml()
+kainjow::mustache::data buildQueryData
-{
+(
-  kainjow::mustache::data results{kainjow::mustache::data::type::list};
+  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;
+}
 
-  for (auto it = m_srs.begin(); it != m_srs.end(); it++) {
+kainjow::mustache::data buildPagination(
-    kainjow::mustache::data result;
+  unsigned int pageLength,
-    result.set("title", it.getTitle());
+  unsigned int resultsCount,
-    result.set("url", it.getPath());
+  unsigned int resultsStart
-    result.set("snippet", it.getSnippet());
+)
-    std::string zim_id(it.getZimId());
+{
-    result.set("resultContentId", mp_nameMapper->getNameForId(zim_id));
+  assert(pageLength!=0);
-    if (!mp_library) {
+  kainjow::mustache::data pagination;
-      result.set("bookTitle", kainjow::mustache::data(false));
+  kainjow::mustache::data pages{kainjow::mustache::data::type::list};
-    } else {
+
-      result.set("bookTitle", mp_library->getBookById(zim_id).getTitle());
+  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 items{kainjow::mustache::data::type::list};
+  for (auto it = m_srs.begin(); it != m_srs.end(); it++) {
+    kainjow::mustache::data result;
+    std::string zim_id(it.getZimId());
+    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)));
 
-  // pages
+  // pagination
-  kainjow::mustache::data pages{kainjow::mustache::data::type::list};
+  auto pagination = buildPagination(
+    pageLength,
+    estimatedResultCount,
+    resultStart
+  );
 
-  auto resultEnd = 0U;
+  kainjow::mustache::data query = buildQueryData(
-  auto currentPage = 0U;
+    searchProtocolPrefix,
-  auto pageStart = 0U;
+    searchPattern,
-  auto pageEnd = 0U;
+    searchContent
-  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;
-    }
-  }
-
-  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);
-  }
 
   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("pagination", pagination);
-  allData.set("hasResults", estimatedResultCount != 0);
+  allData.set("query", query);
-  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("protocolPrefix", this->protocolPrefix);
-  allData.set("searchProtocolPrefix", this->searchProtocolPrefix);
-  allData.set("contentId", this->searchContent);
 
   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();
 }
 

src/server/internalServer.cpp

@@ -633,12 +633,13 @@ std::unique_ptr<Response> 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,
+      HTTPErrorHtmlResponse response(*this, request, MHD_HTTP_NOT_FOUND,
                                      "fulltext-search-unavailable",
                                      "404-page-heading",
-                                   cssUrl)
+                                     cssUrl);
-           + nonParameterizedMessage("no-search-results")
+      response += nonParameterizedMessage("no-search-results");
-           + TaskbarInfo(searchInfo.bookName, archive.get());
+      response += TaskbarInfo(searchInfo.bookName, archive.get());
+      return response;
     }
 
 
@@ -664,7 +665,7 @@ std::unique_ptr<Response> 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());
@@ -673,10 +674,6 @@ std::unique_ptr<Response> 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();
   }
 }
 

src/server/request_context.h

@@ -28,6 +28,7 @@
 #include <stdexcept>
 
 #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<typename T=std::string>
     T get_argument(const std::string& name) const {
-        std::istringstream stream(arguments.at(name));
+        return extractFromString<T>(arguments.at(name));
-        T v;
-        stream >> v;
-        return v;
     }
 
     template<class T>

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> Response::build_416(const InternalServer& server, size_t resourceLength)
 {
   auto response = Response::build(server);

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 {};

src/tools/stringTools.cpp

@@ -405,3 +405,8 @@ std::vector<std::string> 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;
+}

src/tools/stringTools.h

@@ -25,6 +25,7 @@
 #include <string>
 #include <vector>
 #include <sstream>
+#include <stdexcept>
 
 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<std::string> getTitleVariants(const std::string& title);

static/templates/search_result.html

@@ -102,30 +102,30 @@
       }
 
     </style>
-    <title>Search: {{searchPattern}}</title>
+    <title>Search: {{query.pattern}}</title>
   </head>
   <body bgcolor="white">
     <div class="header">
-      {{#hasResults}}
+      {{#results.hasResults}}
         Results
         <b>
-          {{resultStart}}-{{resultEnd}}
+          {{results.start}}-{{results.end}}
         </b> of <b>
-          {{count}}
+          {{results.count}}
         </b> for <b>
-          "{{{searchPattern}}}"
+          "{{{query.pattern}}}"
         </b>
-      {{/hasResults}}
+      {{/results.hasResults}}
-      {{^hasResults}}
+      {{^results.hasResults}}
-        No results were found for <b>"{{{searchPattern}}}"</b>
+        No results were found for <b>"{{{query.pattern}}}"</b>
-      {{/hasResults}}
+      {{/results.hasResults}}
     </div>
 
     <div class="results">
       <ul>
-        {{#results}}
+        {{#results.items}}
           <li>
-            <a href="{{protocolPrefix}}{{resultContentId}}/{{url}}">
+            <a href="{{{absolutePath}}}">
               {{title}}
             </a>
             {{#snippet}}
@@ -138,39 +138,23 @@
               <div class="informations">{{wordCount}} words</div>
             {{/wordCount}}
           </li>
-        {{/results}}
+        {{/results.items}}
       </ul>
     </div>
 
     <div class="footer">
-      {{#hasPages}}
+      {{#pagination.hasPages}}
         <ul>
-          {{#resultLastPageStart}}
+          {{#pagination.pages}}
             <li>
-              <a {{! let the format of this tag be identical to the case below }}
+              <a {{#current}}class="selected"{{/current}}
-                 href="{{searchProtocolPrefix}}pattern={{searchPatternEncoded}}{{#contentId}}&content={{.}}{{/contentId}}&start=0&pageLength={{pageLength}}">
+                 href="{{{query.unpaginatedQuery}}}&start={{start}}&pageLength={{pagination.itemsPerPage}}">
-                ◀
-              </a>
-            </li>
-          {{/resultLastPageStart}}
-          {{#pages}}
-            <li>
-              <a {{#selected}}class="selected"{{/selected}}
-                 href="{{searchProtocolPrefix}}pattern={{searchPatternEncoded}}{{#contentId}}&content={{.}}{{/contentId}}&start={{start}}&pageLength={{pageLength}}">
                 {{label}}
               </a>
             </li>
-          {{/pages}}
+          {{/pagination.pages}}
-          {{#resultLastPageStart}}
-            <li>
-              <a {{! let the format of this tag be identical to the case above }}
-                 href="{{searchProtocolPrefix}}pattern={{searchPatternEncoded}}{{#contentId}}&content={{.}}{{/contentId}}&start={{resultLastPageStart}}&pageLength={{pageLength}}">
-                ▶
-              </a>
-            </li>
-          {{/resultLastPageStart}}
         </ul>
-      {{/hasPages}}
+      {{/pagination.hasPages}}
     </div>
   </body>
 </html>

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 <regex>' 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],

test/server.cpp

@@ -1158,7 +1158,7 @@ R"SEARCHRESULT(
 )SEARCHRESULT",
 
 R"SEARCHRESULT(
-            <a href="/ROOT/zimfile/A/Catchin&apos;_Some_Rays:_The_Music_of_Ray_Charles">
+            <a href="/ROOT/zimfile/A/Catchin'_Some_Rays:_The_Music_of_Ray_Charles">
               Catchin&apos; Some Rays: The Music of Ray Charles
             </a>
               <cite>...<b>jazz</b> singer Roseanna Vitro, released in August 1997 on the Telarc <b>Jazz</b> 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 <b>jazz</b> Length 61:00 Label Telarc <b>Jazz</b> 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......</cite>
@@ -1167,7 +1167,7 @@ R"SEARCHRESULT(
 )SEARCHRESULT",
 
 R"SEARCHRESULT(
-            <a href="/ROOT/zimfile/A/That&apos;s_What_I_Say:_John_Scofield_Plays_the_Music_of_Ray_Charles">
+            <a href="/ROOT/zimfile/A/That's_What_I_Say:_John_Scofield_Plays_the_Music_of_Ray_Charles">
               That&apos;s What I Say: John Scofield Plays the Music of Ray Charles
             </a>
               <cite>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 <b>Jazz</b> 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 <b>Jazz</b> All About <b>Jazz</b>...</cite>
@@ -1437,7 +1437,7 @@ R"SEARCHRESULT(
 )SEARCHRESULT",
 
 R"SEARCHRESULT(
-            <a href="/ROOT/zimfile/A/Don&apos;t_Let_the_Sun_Catch_You_Cryin&apos;">
+            <a href="/ROOT/zimfile/A/Don't_Let_the_Sun_Catch_You_Cryin'">
               Don&apos;t Let the Sun Catch You Cryin&apos;
             </a>
               <cite>...R&amp;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 <b>jazz</b> 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...</cite>
@@ -1446,7 +1446,7 @@ R"SEARCHRESULT(
 )SEARCHRESULT",
 
 R"SEARCHRESULT(
-            <a href="/ROOT/zimfile/A/I_Don&apos;t_Need_No_Doctor">
+            <a href="/ROOT/zimfile/A/I_Don't_Need_No_Doctor">
               I Don&apos;t Need No Doctor
             </a>
               <cite>...<b>jazz</b> 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 <b>jazz</b> singer Roseanna Vitro's......</cite>
@@ -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 },
       }
     },
   };

test/server_helper.cpp

@@ -0,0 +1,215 @@
+
+#include <mustache.hpp>
+#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<ExpectedPage>
+  {
+    public:
+      ExpectedPages(std::vector<ExpectedPage>&& v)
+        : std::vector<ExpectedPage>(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")));
+  }
+}

test/stringTools.cpp

@@ -18,11 +18,11 @@
  */
 
 #include "gtest/gtest.h"
+#include "../src/tools/stringTools.h"
 #include <string>
 #include <vector>
 
 namespace kiwix {
-std::string join(const std::vector<std::string>& list, const std::string& sep);
 std::vector<std::string> 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<int>("55"), 55);
+  ASSERT_EQ(extractFromString<int>("-55"), -55);
+  ASSERT_EQ(extractFromString<float>("-55.0"), -55.0);
+  ASSERT_EQ(extractFromString<bool>("1"), true);
+  ASSERT_EQ(extractFromString<bool>("0"), false);
+  ASSERT_EQ(extractFromString<std::string>("55"), "55");
+  ASSERT_EQ(extractFromString<std::string>("foo"), "foo");
+  ASSERT_EQ(extractFromString<std::string>("foo bar"), "foo bar");
+
+// While spec says that >> operator should set the value to std::numeric_limits<uint>::max()
+// and set the failbit of the stream (and so detect the error), the gcc implementation (?)
+// set the value to (std::numeric_limits<uint>::max()-55) and doesn't set the failbit.
+//  ASSERT_THROW(extractFromString<uint>("-55"), std::invalid_argument);
+
+  ASSERT_THROW(extractFromString<int>("-55.0"), std::invalid_argument);
+  ASSERT_THROW(extractFromString<int>("55 foo"), std::invalid_argument);
+  ASSERT_THROW(extractFromString<float>("3.14.5"), std::invalid_argument);
+}
+
 };