From 9b9c61a19498edb7f01c2eb0c66b14ecd1e9decf Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Fri, 19 Jan 2024 11:06:17 +0100 Subject: [PATCH 01/18] Use a `recursive_mutex` instead of a `mutex`. This allow us to internally call thread_safe function from already locked context. --- include/library.h | 2 +- src/library.cpp | 34 +++++++++++++++++----------------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/include/library.h b/include/library.h index 8ed86f324..89514590b 100644 --- a/include/library.h +++ b/include/library.h @@ -408,7 +408,7 @@ private: // functions void dropCache(const std::string& bookId); private: //data - mutable std::mutex m_mutex; + mutable std::recursive_mutex m_mutex; Library::Revision m_revision; std::map m_books; using ArchiveCache = ConcurrentCache>; diff --git a/src/library.cpp b/src/library.cpp index 98482d87d..a62d4dac6 100644 --- a/src/library.cpp +++ b/src/library.cpp @@ -110,7 +110,7 @@ Library::~Library() = default; bool Library::addBook(const Book& book) { - std::lock_guard lock(m_mutex); + std::lock_guard lock(m_mutex); ++m_revision; /* Try to find it */ updateBookDB(book); @@ -141,13 +141,13 @@ bool Library::addBook(const Book& book) void Library::addBookmark(const Bookmark& bookmark) { - std::lock_guard lock(m_mutex); + std::lock_guard lock(m_mutex); m_bookmarks.push_back(bookmark); } bool Library::removeBookmark(const std::string& zimId, const std::string& url) { - std::lock_guard lock(m_mutex); + std::lock_guard lock(m_mutex); for(auto it=m_bookmarks.begin(); it!=m_bookmarks.end(); it++) { if (it->getBookId() == zimId && it->getUrl() == url) { m_bookmarks.erase(it); @@ -166,7 +166,7 @@ void Library::dropCache(const std::string& id) bool Library::removeBookById(const std::string& id) { - std::lock_guard lock(m_mutex); + std::lock_guard lock(m_mutex); m_bookDB->delete_document("Q" + id); dropCache(id); // We do not change the cache size here @@ -184,7 +184,7 @@ bool Library::removeBookById(const std::string& id) Library::Revision Library::getRevision() const { - std::lock_guard lock(m_mutex); + std::lock_guard lock(m_mutex); return m_revision; } @@ -192,7 +192,7 @@ uint32_t Library::removeBooksNotUpdatedSince(Revision libraryRevision) { BookIdCollection booksToRemove; { - std::lock_guard lock(m_mutex); + std::lock_guard lock(m_mutex); for ( const auto& entry : m_books) { if ( entry.second.lastUpdatedRevision <= libraryRevision ) { booksToRemove.push_back(entry.first); @@ -217,7 +217,7 @@ const Book& Library::getBookById(const std::string& id) const Book Library::getBookByIdThreadSafe(const std::string& id) const { - std::lock_guard lock(m_mutex); + std::lock_guard lock(m_mutex); return getBookById(id); } @@ -275,7 +275,7 @@ std::shared_ptr Library::getSearcherByIds(const BookIdSet& ids) unsigned int Library::getBookCount(const bool localBooks, const bool remoteBooks) const { - std::lock_guard lock(m_mutex); + std::lock_guard lock(m_mutex); return getBookCount_not_protected(localBooks, remoteBooks); } @@ -288,7 +288,7 @@ bool Library::writeToFile(const std::string& path) const dumper.setBaseDir(baseDir); std::string xml; { - std::lock_guard lock(m_mutex); + std::lock_guard lock(m_mutex); xml = dumper.dumpLibXMLContent(allBookIds); }; return writeTextFile(path, xml); @@ -304,7 +304,7 @@ bool Library::writeBookmarksToFile(const std::string& path) const Library::AttributeCounts Library::getBookAttributeCounts(BookStrPropMemFn p) const { - std::lock_guard lock(m_mutex); + std::lock_guard lock(m_mutex); AttributeCounts propValueCounts; for (const auto& pair: m_books) { @@ -336,7 +336,7 @@ std::vector Library::getBooksLanguages() const Library::AttributeCounts Library::getBooksLanguagesWithCounts() const { - std::lock_guard lock(m_mutex); + std::lock_guard lock(m_mutex); AttributeCounts langsWithCounts; for (const auto& pair: m_books) { @@ -352,7 +352,7 @@ Library::AttributeCounts Library::getBooksLanguagesWithCounts() const std::vector Library::getBooksCategories() const { - std::lock_guard lock(m_mutex); + std::lock_guard lock(m_mutex); std::set categories; for (const auto& pair: m_books) { @@ -383,7 +383,7 @@ const std::vector Library::getBookmarks(bool onlyValidBookmarks } std::vector validBookmarks; auto booksId = getBooksIds(); - std::lock_guard lock(m_mutex); + std::lock_guard lock(m_mutex); for(auto& bookmark:m_bookmarks) { if (std::find(booksId.begin(), booksId.end(), bookmark.getBookId()) != booksId.end()) { validBookmarks.push_back(bookmark); @@ -394,7 +394,7 @@ const std::vector Library::getBookmarks(bool onlyValidBookmarks Library::BookIdCollection Library::getBooksIds() const { - std::lock_guard lock(m_mutex); + std::lock_guard lock(m_mutex); BookIdCollection bookIds; for (auto& pair: m_books) { @@ -600,7 +600,7 @@ Library::BookIdCollection Library::filterViaBookDB(const Filter& filter) const BookIdCollection bookIds; - std::lock_guard lock(m_mutex); + std::lock_guard lock(m_mutex); Xapian::Enquire enquire(*m_bookDB); enquire.set_query(query); const auto results = enquire.get_mset(0, m_books.size()); @@ -615,7 +615,7 @@ Library::BookIdCollection Library::filter(const Filter& filter) const { BookIdCollection result; const auto preliminaryResult = filterViaBookDB(filter); - std::lock_guard lock(m_mutex); + std::lock_guard lock(m_mutex); for(auto id : preliminaryResult) { if(filter.accept(m_books.at(id))) { result.push_back(id); @@ -689,7 +689,7 @@ void Library::sort(BookIdCollection& bookIds, supportedListSortBy sort, bool asc // NOTE: for the entire duration of the sort. Will need to obtain (under a // NOTE: lock) the required atributes from the books once, and then the // NOTE: sorting will run on a copy of data without locking. - std::lock_guard lock(m_mutex); + std::lock_guard lock(m_mutex); switch(sort) { case TITLE: std::sort(bookIds.begin(), bookIds.end(), Comparator(this, ascending)); From 82cb1133e5fa88fb6a1f1bbc539a81f5851524ff Mon Sep 17 00:00:00 2001 From: Matthieu Gautier <mgautier@kymeria.fr> Date: Fri, 19 Jan 2024 11:45:28 +0100 Subject: [PATCH 02/18] [Test] Add missing name in sample library.xml --- test/library.cpp | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/test/library.cpp b/test/library.cpp index 7f364e42f..6e37dcb71 100644 --- a/test/library.cpp +++ b/test/library.cpp @@ -28,8 +28,8 @@ const char * sampleOpdsStream = R"( <id>00000000-0000-0000-0000-000000000000</id> <entry> <title>Encyclopédie de la Tunisie - wikipedia_fr_tunisie_novid_2018-10 - unforgettable + wikipedia_fr_tunisie + novid urn:uuid:0c45160e-f917-760a-9159-dfe3c53cdcdd /meta?name=favicon&content=wikipedia_fr_tunisie_novid_2018-10 2018-10-08T00:00::00:Z @@ -52,6 +52,7 @@ const char * sampleOpdsStream = R"( Tania Louis urn:uuid:0d0bcd57-d3f6-cb22-44cc-a723ccb4e1b2 + biologie-tout-compris_fr_all /meta?name=favicon&content=biologie-tout-compris_fr_all_2018-06 2018-06-23T00:00::00:Z fra @@ -67,6 +68,7 @@ const char * sampleOpdsStream = R"( Wikiquote urn:uuid:0ea1cde6-441d-6c58-f2c7-21c2838e659f + wikiquote_fr_all /meta?name=favicon&content=wikiquote_fr_all_nopic_2019-06 2019-06-05T00:00::00:Z fra,ita @@ -83,6 +85,7 @@ const char * sampleOpdsStream = R"( Géographie par Wikipédia urn:uuid:1123e574-6eef-6d54-28fc-13e4caeae474 + wikipedia_fr_geography /meta?name=favicon&content=wikipedia_fr_geography_nopic_2019-06 2019-06-02T00:00::00:Z Une sélection d'articles de Wikipédia sur la géographie @@ -99,6 +102,7 @@ const char * sampleOpdsStream = R"( Mathématiques urn:uuid:14829621-c490-c376-0792-9de558b57efa + wikipedia_fr_mathematics /meta?name=favicon&content=wikipedia_fr_mathematics_nopic_2019-05 2019-05-13T00:00::00:Z fra @@ -115,6 +119,7 @@ const char * sampleOpdsStream = R"( Granblue Fantasy Wiki urn:uuid:006cbd1b-16d8-b00d-a584-c1ae110a94ed + grandbluefantasy_en_all /meta?name=favicon&content=granbluefantasy_en_all_all_nopic_2018-10 2018-10-14T00:00::00:Z eng @@ -130,6 +135,7 @@ const char * sampleOpdsStream = R"( Movies & TV Stack Exchange urn:uuid:00f37b00-f4da-0675-995a-770f9c72903e + movies.stackexchange.com_en_all /meta?name=favicon&content=movies.stackexchange.com_en_all_2019-02 2019-02-03T00:00::00:Z eng @@ -145,6 +151,7 @@ const char * sampleOpdsStream = R"( TED talks - Business urn:uuid:0189d9be-2fd0-b4b6-7300-20fab0b5cdc8 + ted_en_business /meta?name=favicon&content=ted_en_business_2018-07 2018-07-23T00:00::00:Z eng @@ -160,6 +167,7 @@ const char * sampleOpdsStream = R"( Mythology & Folklore Stack Exchange urn:uuid:028055ac-4acc-1d54-65e0-a96de45e1b22 + mythology.stackexchange.com_en_all /meta?name=favicon&content=mythology.stackexchange.com_en_all_2019-02 2019-02-03T00:00::00:Z eng @@ -175,6 +183,7 @@ const char * sampleOpdsStream = R"( Islam Stack Exchange urn:uuid:02e9c7ff-36fc-9c6e-6ac7-cd7085989029 + islam.stackexchange.com_en_all /meta?name=favicon&content=islam.stackexchange.com_en_all_2019-01 2019-01-31T00:00::00:Z eng @@ -248,8 +257,8 @@ TEST(LibraryOpdsImportTest, allInOne) const kiwix::Book& book1 = lib->getBookById("0c45160e-f917-760a-9159-dfe3c53cdcdd"); EXPECT_EQ(book1.getTitle(), "Encyclopédie de la Tunisie"); - EXPECT_EQ(book1.getName(), "wikipedia_fr_tunisie_novid_2018-10"); - EXPECT_EQ(book1.getFlavour(), "unforgettable"); + EXPECT_EQ(book1.getName(), "wikipedia_fr_tunisie"); + EXPECT_EQ(book1.getFlavour(), "novid"); EXPECT_EQ(book1.getLanguages(), Langs{ "fra" }); EXPECT_EQ(book1.getCommaSeparatedLanguages(), "fra"); EXPECT_EQ(book1.getDate(), "8 Oct 2018"); @@ -273,7 +282,7 @@ TEST(LibraryOpdsImportTest, allInOne) { const kiwix::Book& book2 = lib->getBookById("0189d9be-2fd0-b4b6-7300-20fab0b5cdc8"); EXPECT_EQ(book2.getTitle(), "TED talks - Business"); - EXPECT_EQ(book2.getName(), ""); + EXPECT_EQ(book2.getName(), "ted_en_business"); EXPECT_EQ(book2.getFlavour(), ""); EXPECT_EQ(book2.getLanguages(), Langs{ "eng" }); EXPECT_EQ(book2.getCommaSeparatedLanguages(), "eng"); From bf1ab03332b5f0e20e71d588e6cd74d1c6f83f56 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Fri, 19 Jan 2024 12:03:15 +0100 Subject: [PATCH 03/18] [Test] Add missing flavour in books. --- test/library.cpp | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/test/library.cpp b/test/library.cpp index 6e37dcb71..6f4e62a52 100644 --- a/test/library.cpp +++ b/test/library.cpp @@ -53,6 +53,7 @@ const char * sampleOpdsStream = R"( Tania Louis urn:uuid:0d0bcd57-d3f6-cb22-44cc-a723ccb4e1b2 biologie-tout-compris_fr_all + full /meta?name=favicon&content=biologie-tout-compris_fr_all_2018-06 2018-06-23T00:00::00:Z fra @@ -69,6 +70,7 @@ const char * sampleOpdsStream = R"( Wikiquote urn:uuid:0ea1cde6-441d-6c58-f2c7-21c2838e659f wikiquote_fr_all + full /meta?name=favicon&content=wikiquote_fr_all_nopic_2019-06 2019-06-05T00:00::00:Z fra,ita @@ -86,6 +88,7 @@ const char * sampleOpdsStream = R"( Géographie par Wikipédia urn:uuid:1123e574-6eef-6d54-28fc-13e4caeae474 wikipedia_fr_geography + full /meta?name=favicon&content=wikipedia_fr_geography_nopic_2019-06 2019-06-02T00:00::00:Z Une sélection d'articles de Wikipédia sur la géographie @@ -103,6 +106,7 @@ const char * sampleOpdsStream = R"( Mathématiques urn:uuid:14829621-c490-c376-0792-9de558b57efa wikipedia_fr_mathematics + novid /meta?name=favicon&content=wikipedia_fr_mathematics_nopic_2019-05 2019-05-13T00:00::00:Z fra @@ -120,6 +124,7 @@ const char * sampleOpdsStream = R"( Granblue Fantasy Wiki urn:uuid:006cbd1b-16d8-b00d-a584-c1ae110a94ed grandbluefantasy_en_all + novid /meta?name=favicon&content=granbluefantasy_en_all_all_nopic_2018-10 2018-10-14T00:00::00:Z eng @@ -136,6 +141,7 @@ const char * sampleOpdsStream = R"( Movies & TV Stack Exchange urn:uuid:00f37b00-f4da-0675-995a-770f9c72903e movies.stackexchange.com_en_all + novid /meta?name=favicon&content=movies.stackexchange.com_en_all_2019-02 2019-02-03T00:00::00:Z eng @@ -152,6 +158,7 @@ const char * sampleOpdsStream = R"( TED talks - Business urn:uuid:0189d9be-2fd0-b4b6-7300-20fab0b5cdc8 ted_en_business + nodet /meta?name=favicon&content=ted_en_business_2018-07 2018-07-23T00:00::00:Z eng @@ -168,6 +175,7 @@ const char * sampleOpdsStream = R"( Mythology & Folklore Stack Exchange urn:uuid:028055ac-4acc-1d54-65e0-a96de45e1b22 mythology.stackexchange.com_en_all + novid /meta?name=favicon&content=mythology.stackexchange.com_en_all_2019-02 2019-02-03T00:00::00:Z eng @@ -184,6 +192,7 @@ const char * sampleOpdsStream = R"( Islam Stack Exchange urn:uuid:02e9c7ff-36fc-9c6e-6ac7-cd7085989029 islam.stackexchange.com_en_all + novid /meta?name=favicon&content=islam.stackexchange.com_en_all_2019-01 2019-01-31T00:00::00:Z eng @@ -283,7 +292,7 @@ TEST(LibraryOpdsImportTest, allInOne) const kiwix::Book& book2 = lib->getBookById("0189d9be-2fd0-b4b6-7300-20fab0b5cdc8"); EXPECT_EQ(book2.getTitle(), "TED talks - Business"); EXPECT_EQ(book2.getName(), "ted_en_business"); - EXPECT_EQ(book2.getFlavour(), ""); + EXPECT_EQ(book2.getFlavour(), "nodet"); EXPECT_EQ(book2.getLanguages(), Langs{ "eng" }); EXPECT_EQ(book2.getCommaSeparatedLanguages(), "eng"); EXPECT_EQ(book2.getDate(), "2018-07-23"); From 903f476f77c222c3c3663d7ff5b42c4926647a26 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Fri, 19 Jan 2024 11:56:10 +0100 Subject: [PATCH 04/18] Test bookmarks serializations. --- src/libxml_dumper.cpp | 2 +- test/library.cpp | 66 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 64 insertions(+), 4 deletions(-) diff --git a/src/libxml_dumper.cpp b/src/libxml_dumper.cpp index 722b9dac9..125694d11 100644 --- a/src/libxml_dumper.cpp +++ b/src/libxml_dumper.cpp @@ -135,7 +135,7 @@ std::string LibXMLDumper::dumpLibXMLBookmark() pugi::xml_node bookmarksNode = doc.append_child("bookmarks"); if (library) { - for (auto& bookmark: library->getBookmarks()) { + for (auto& bookmark: library->getBookmarks(false)) { handleBookmark(bookmark, bookmarksNode); } } diff --git a/test/library.cpp b/test/library.cpp index 6f4e62a52..5fcde072b 100644 --- a/test/library.cpp +++ b/test/library.cpp @@ -327,9 +327,11 @@ class LibraryTest : public ::testing::Test { manager.readXml(sampleLibraryXML, false, "./test/library.xml", true); } - kiwix::Bookmark createBookmark(const std::string &id) { + kiwix::Bookmark createBookmark(const std::string &id, const std::string& url="", const std::string& title="") { kiwix::Bookmark bookmark; bookmark.setBookId(id); + bookmark.setUrl(url); + bookmark.setTitle(title); return bookmark; }; @@ -351,7 +353,7 @@ TEST_F(LibraryTest, getBookMarksTest) auto bookId2 = lib->getBooksIds()[1]; lib->addBookmark(createBookmark(bookId1)); - lib->addBookmark(createBookmark("invalid-bookmark-id")); + lib->addBookmark(createBookmark("invalid-book-id")); lib->addBookmark(createBookmark(bookId2)); auto onlyValidBookmarks = lib->getBookmarks(); auto allBookmarks = lib->getBookmarks(false); @@ -360,10 +362,68 @@ TEST_F(LibraryTest, getBookMarksTest) EXPECT_EQ(onlyValidBookmarks[1].getBookId(), bookId2); EXPECT_EQ(allBookmarks[0].getBookId(), bookId1); - EXPECT_EQ(allBookmarks[1].getBookId(), "invalid-bookmark-id"); + EXPECT_EQ(allBookmarks[1].getBookId(), "invalid-book-id"); EXPECT_EQ(allBookmarks[2].getBookId(), bookId2); } +TEST_F(LibraryTest, bookmarksSerializationTest) +{ + auto bookId1 = lib->getBooksIds()[0]; + auto bookId2 = lib->getBooksIds()[1]; + + auto book1 = lib->getBookById(bookId1); + auto book2 = lib->getBookById(bookId2); + + lib->addBookmark(createBookmark(bookId1, "a/url", "Article title1")); + lib->addBookmark(createBookmark("invalid-book-id", "another/url", "Unknown title")); + lib->addBookmark(createBookmark(bookId2, "a/url/2", "Article title2")); + + lib->writeBookmarksToFile("__test__bookmarks.xml"); + + // Build a new library + auto new_lib = kiwix::Library::create(); + { + kiwix::Manager manager(new_lib); + manager.readOpds(sampleOpdsStream, "foo.urlHost"); + manager.readXml(sampleLibraryXML, false, "./test/library.xml", true); + manager.readBookmarkFile("__test__bookmarks.xml"); + } + std::remove("__test__bookmarks.xml"); + + auto onlyValidBookmarks = new_lib->getBookmarks(); + auto allBookmarks = new_lib->getBookmarks(false); + + ASSERT_EQ(onlyValidBookmarks.size(), 2); + EXPECT_EQ(onlyValidBookmarks[0].getBookId(), bookId1); + EXPECT_EQ(onlyValidBookmarks[1].getBookId(), bookId2); + + ASSERT_EQ(allBookmarks.size(), 3); + auto bookmark1 = allBookmarks[0]; + EXPECT_EQ(bookmark1.getBookId(), bookId1); + EXPECT_EQ(bookmark1.getBookTitle(), book1.getTitle()); + EXPECT_EQ(bookmark1.getUrl(), "a/url"); + EXPECT_EQ(bookmark1.getTitle(), "Article title1"); + EXPECT_EQ(bookmark1.getLanguage(), book1.getCommaSeparatedLanguages()); + EXPECT_EQ(bookmark1.getDate(), book1.getDate()); + + auto bookmark2 = allBookmarks[1]; + EXPECT_EQ(bookmark2.getBookId(), "invalid-book-id"); + EXPECT_EQ(bookmark2.getBookTitle(), ""); + EXPECT_EQ(bookmark2.getUrl(), "another/url"); + EXPECT_EQ(bookmark2.getTitle(), "Unknown title"); + EXPECT_EQ(bookmark2.getLanguage(), ""); + EXPECT_EQ(bookmark2.getDate(), ""); + + auto bookmark3 = allBookmarks[2]; + EXPECT_EQ(bookmark3.getBookId(), bookId2); + EXPECT_EQ(bookmark3.getBookTitle(), book2.getTitle()); + EXPECT_EQ(bookmark3.getUrl(), "a/url/2"); + EXPECT_EQ(bookmark3.getTitle(), "Article title2"); + EXPECT_EQ(bookmark3.getLanguage(), book2.getCommaSeparatedLanguages()); + EXPECT_EQ(bookmark3.getDate(), book2.getDate()); +} + + TEST_F(LibraryTest, sanityCheck) { EXPECT_EQ(lib->getBookCount(true, true), 12U); From 5a0644d32b5e22b4367da7c16211f06259c1ec2c Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Tue, 16 Jan 2024 17:22:01 +0100 Subject: [PATCH 05/18] Also store book's name in bookmark. --- include/bookmark.h | 3 +++ src/bookmark.cpp | 1 + src/libxml_dumper.cpp | 2 ++ test/library.cpp | 3 +++ 4 files changed, 9 insertions(+) diff --git a/include/bookmark.h b/include/bookmark.h index 7861fee31..815e728d7 100644 --- a/include/bookmark.h +++ b/include/bookmark.h @@ -42,6 +42,7 @@ class Bookmark const std::string& getBookId() const { return m_bookId; } const std::string& getBookTitle() const { return m_bookTitle; } + const std::string& getBookName() const { return m_bookName; } const std::string& getUrl() const { return m_url; } const std::string& getTitle() const { return m_title; } const std::string& getLanguage() const { return m_language; } @@ -49,6 +50,7 @@ class Bookmark void setBookId(const std::string& bookId) { m_bookId = bookId; } void setBookTitle(const std::string& bookTitle) { m_bookTitle = bookTitle; } + void setBookName(const std::string& bookName) { m_bookName = bookName; } void setUrl(const std::string& url) { m_url = url; } void setTitle(const std::string& title) { m_title = title; } void setLanguage(const std::string& language) { m_language = language; } @@ -57,6 +59,7 @@ class Bookmark protected: std::string m_bookId; std::string m_bookTitle; + std::string m_bookName; std::string m_url; std::string m_title; std::string m_language; diff --git a/src/bookmark.cpp b/src/bookmark.cpp index 2ca1b623f..54a65deb9 100644 --- a/src/bookmark.cpp +++ b/src/bookmark.cpp @@ -38,6 +38,7 @@ void Bookmark::updateFromXml(const pugi::xml_node& node) auto bookNode = node.child("book"); m_bookId = bookNode.child("id").child_value(); m_bookTitle = bookNode.child("title").child_value(); + m_bookName = bookNode.child("name").child_value(); m_language = bookNode.child("language").child_value(); m_date = bookNode.child("date").child_value(); m_title = node.child("title").child_value(); diff --git a/src/libxml_dumper.cpp b/src/libxml_dumper.cpp index 125694d11..1e22b5b41 100644 --- a/src/libxml_dumper.cpp +++ b/src/libxml_dumper.cpp @@ -97,11 +97,13 @@ void LibXMLDumper::handleBookmark(Bookmark bookmark, pugi::xml_node root_node) { auto book = library->getBookByIdThreadSafe(bookmark.getBookId()); ADD_TEXT_ENTRY(book_node, "id", book.getId()); ADD_TEXT_ENTRY(book_node, "title", book.getTitle()); + ADD_TEXT_ENTRY(book_node, "name", book.getName()); ADD_TEXT_ENTRY(book_node, "language", book.getCommaSeparatedLanguages()); ADD_TEXT_ENTRY(book_node, "date", book.getDate()); } catch (...) { ADD_TEXT_ENTRY(book_node, "id", bookmark.getBookId()); ADD_TEXT_ENTRY(book_node, "title", bookmark.getBookTitle()); + ADD_TEXT_ENTRY(book_node, "name", bookmark.getBookName()); ADD_TEXT_ENTRY(book_node, "language", bookmark.getLanguage()); ADD_TEXT_ENTRY(book_node, "date", bookmark.getDate()); } diff --git a/test/library.cpp b/test/library.cpp index 5fcde072b..21733209c 100644 --- a/test/library.cpp +++ b/test/library.cpp @@ -401,6 +401,7 @@ TEST_F(LibraryTest, bookmarksSerializationTest) auto bookmark1 = allBookmarks[0]; EXPECT_EQ(bookmark1.getBookId(), bookId1); EXPECT_EQ(bookmark1.getBookTitle(), book1.getTitle()); + EXPECT_EQ(bookmark1.getBookName(), book1.getName()); EXPECT_EQ(bookmark1.getUrl(), "a/url"); EXPECT_EQ(bookmark1.getTitle(), "Article title1"); EXPECT_EQ(bookmark1.getLanguage(), book1.getCommaSeparatedLanguages()); @@ -409,6 +410,7 @@ TEST_F(LibraryTest, bookmarksSerializationTest) auto bookmark2 = allBookmarks[1]; EXPECT_EQ(bookmark2.getBookId(), "invalid-book-id"); EXPECT_EQ(bookmark2.getBookTitle(), ""); + EXPECT_EQ(bookmark2.getBookName(), ""); EXPECT_EQ(bookmark2.getUrl(), "another/url"); EXPECT_EQ(bookmark2.getTitle(), "Unknown title"); EXPECT_EQ(bookmark2.getLanguage(), ""); @@ -417,6 +419,7 @@ TEST_F(LibraryTest, bookmarksSerializationTest) auto bookmark3 = allBookmarks[2]; EXPECT_EQ(bookmark3.getBookId(), bookId2); EXPECT_EQ(bookmark3.getBookTitle(), book2.getTitle()); + EXPECT_EQ(bookmark3.getBookName(), book2.getName()); EXPECT_EQ(bookmark3.getUrl(), "a/url/2"); EXPECT_EQ(bookmark3.getTitle(), "Article title2"); EXPECT_EQ(bookmark3.getLanguage(), book2.getCommaSeparatedLanguages()); From 699f96ca0d751dfbeb9f541849cdde24d54c9e3f Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Wed, 17 Jan 2024 17:25:34 +0100 Subject: [PATCH 06/18] Add book's flavour in bookmark. --- include/bookmark.h | 3 +++ src/bookmark.cpp | 1 + src/libxml_dumper.cpp | 2 ++ test/library.cpp | 3 +++ 4 files changed, 9 insertions(+) diff --git a/include/bookmark.h b/include/bookmark.h index 815e728d7..456fef0f8 100644 --- a/include/bookmark.h +++ b/include/bookmark.h @@ -43,6 +43,7 @@ class Bookmark const std::string& getBookId() const { return m_bookId; } const std::string& getBookTitle() const { return m_bookTitle; } const std::string& getBookName() const { return m_bookName; } + const std::string& getBookFlavour() const { return m_bookFlavour; } const std::string& getUrl() const { return m_url; } const std::string& getTitle() const { return m_title; } const std::string& getLanguage() const { return m_language; } @@ -51,6 +52,7 @@ class Bookmark void setBookId(const std::string& bookId) { m_bookId = bookId; } void setBookTitle(const std::string& bookTitle) { m_bookTitle = bookTitle; } void setBookName(const std::string& bookName) { m_bookName = bookName; } + void setBookFlavour(const std::string& bookFlavour) { m_bookFlavour = bookFlavour; } void setUrl(const std::string& url) { m_url = url; } void setTitle(const std::string& title) { m_title = title; } void setLanguage(const std::string& language) { m_language = language; } @@ -60,6 +62,7 @@ class Bookmark std::string m_bookId; std::string m_bookTitle; std::string m_bookName; + std::string m_bookFlavour; std::string m_url; std::string m_title; std::string m_language; diff --git a/src/bookmark.cpp b/src/bookmark.cpp index 54a65deb9..6bfe79bbb 100644 --- a/src/bookmark.cpp +++ b/src/bookmark.cpp @@ -39,6 +39,7 @@ void Bookmark::updateFromXml(const pugi::xml_node& node) m_bookId = bookNode.child("id").child_value(); m_bookTitle = bookNode.child("title").child_value(); m_bookName = bookNode.child("name").child_value(); + m_bookFlavour = bookNode.child("flavour").child_value(); m_language = bookNode.child("language").child_value(); m_date = bookNode.child("date").child_value(); m_title = node.child("title").child_value(); diff --git a/src/libxml_dumper.cpp b/src/libxml_dumper.cpp index 1e22b5b41..88a2fd054 100644 --- a/src/libxml_dumper.cpp +++ b/src/libxml_dumper.cpp @@ -98,12 +98,14 @@ void LibXMLDumper::handleBookmark(Bookmark bookmark, pugi::xml_node root_node) { ADD_TEXT_ENTRY(book_node, "id", book.getId()); ADD_TEXT_ENTRY(book_node, "title", book.getTitle()); ADD_TEXT_ENTRY(book_node, "name", book.getName()); + ADD_TEXT_ENTRY(book_node, "flavour", book.getFlavour()); ADD_TEXT_ENTRY(book_node, "language", book.getCommaSeparatedLanguages()); ADD_TEXT_ENTRY(book_node, "date", book.getDate()); } catch (...) { ADD_TEXT_ENTRY(book_node, "id", bookmark.getBookId()); ADD_TEXT_ENTRY(book_node, "title", bookmark.getBookTitle()); ADD_TEXT_ENTRY(book_node, "name", bookmark.getBookName()); + ADD_TEXT_ENTRY(book_node, "flavour", bookmark.getBookFlavour()); ADD_TEXT_ENTRY(book_node, "language", bookmark.getLanguage()); ADD_TEXT_ENTRY(book_node, "date", bookmark.getDate()); } diff --git a/test/library.cpp b/test/library.cpp index 21733209c..c684e1d68 100644 --- a/test/library.cpp +++ b/test/library.cpp @@ -402,6 +402,7 @@ TEST_F(LibraryTest, bookmarksSerializationTest) EXPECT_EQ(bookmark1.getBookId(), bookId1); EXPECT_EQ(bookmark1.getBookTitle(), book1.getTitle()); EXPECT_EQ(bookmark1.getBookName(), book1.getName()); + EXPECT_EQ(bookmark1.getBookFlavour(), book1.getFlavour()); EXPECT_EQ(bookmark1.getUrl(), "a/url"); EXPECT_EQ(bookmark1.getTitle(), "Article title1"); EXPECT_EQ(bookmark1.getLanguage(), book1.getCommaSeparatedLanguages()); @@ -411,6 +412,7 @@ TEST_F(LibraryTest, bookmarksSerializationTest) EXPECT_EQ(bookmark2.getBookId(), "invalid-book-id"); EXPECT_EQ(bookmark2.getBookTitle(), ""); EXPECT_EQ(bookmark2.getBookName(), ""); + EXPECT_EQ(bookmark2.getBookFlavour(), ""); EXPECT_EQ(bookmark2.getUrl(), "another/url"); EXPECT_EQ(bookmark2.getTitle(), "Unknown title"); EXPECT_EQ(bookmark2.getLanguage(), ""); @@ -420,6 +422,7 @@ TEST_F(LibraryTest, bookmarksSerializationTest) EXPECT_EQ(bookmark3.getBookId(), bookId2); EXPECT_EQ(bookmark3.getBookTitle(), book2.getTitle()); EXPECT_EQ(bookmark3.getBookName(), book2.getName()); + EXPECT_EQ(bookmark3.getBookFlavour(), book2.getFlavour()); EXPECT_EQ(bookmark3.getUrl(), "a/url/2"); EXPECT_EQ(bookmark3.getTitle(), "Article title2"); EXPECT_EQ(bookmark3.getLanguage(), book2.getCommaSeparatedLanguages()); From a546effa15aad4e39d6d1bc77e40c345a513c303 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Tue, 16 Jan 2024 17:15:02 +0100 Subject: [PATCH 07/18] Allow bookmark to be created from a Book and url/title. --- include/bookmark.h | 12 +++++++++++ src/bookmark.cpp | 12 +++++++++++ test/library.cpp | 53 ++++++++++++++++++++++++++++++++++------------ 3 files changed, 64 insertions(+), 13 deletions(-) diff --git a/include/bookmark.h b/include/bookmark.h index 456fef0f8..a76dac50a 100644 --- a/include/bookmark.h +++ b/include/bookmark.h @@ -29,13 +29,25 @@ class xml_node; namespace kiwix { +class Book; /** * A class to store information about a bookmark (an article in a book) */ class Bookmark { public: + /** + * Create an empty bookmark. + * + * Bookmark must be populated with `set*` methods + */ Bookmark(); + + /** + * Create a bookmark given a Book, a path and a title. + */ + Bookmark(const Book& book, const std::string& path, const std::string& title); + ~Bookmark(); void updateFromXml(const pugi::xml_node& node); diff --git a/src/bookmark.cpp b/src/bookmark.cpp index 6bfe79bbb..918db2125 100644 --- a/src/bookmark.cpp +++ b/src/bookmark.cpp @@ -18,6 +18,7 @@ */ #include "bookmark.h" +#include "book.h" #include @@ -28,6 +29,17 @@ Bookmark::Bookmark() { } +Bookmark::Bookmark(const Book& book, const std::string& path, const std::string& title): + m_bookId(book.getId()), + m_bookTitle(book.getTitle()), + m_bookName(book.getName()), + m_bookFlavour(book.getFlavour()), + m_url(path), + m_title(title), + m_language(book.getCommaSeparatedLanguages()), + m_date(book.getDate()) +{} + /* Destructor */ Bookmark::~Bookmark() { diff --git a/test/library.cpp b/test/library.cpp index c684e1d68..f75a4dae9 100644 --- a/test/library.cpp +++ b/test/library.cpp @@ -20,7 +20,6 @@ #include "gtest/gtest.h" #include - const char * sampleOpdsStream = R"( lib; }; +TEST_F(LibraryTest, createBookMark) +{ + auto bookId = "0c45160e-f917-760a-9159-dfe3c53cdcdd"; + auto book = lib->getBookById(bookId); + + auto bookmark = createBookmark(book, "/a/url", "A title"); + + EXPECT_EQ(bookmark.getUrl(), "/a/url"); + EXPECT_EQ(bookmark.getTitle(), "A title"); + EXPECT_EQ(bookmark.getBookId(), bookId); + EXPECT_EQ(bookmark.getBookName(), book.getName()); + EXPECT_EQ(bookmark.getBookName(), "wikipedia_fr_tunisie"); + EXPECT_EQ(bookmark.getBookTitle(), book.getTitle()); + EXPECT_EQ(bookmark.getDate(), book.getDate()); + EXPECT_EQ(bookmark.getBookFlavour(), book.getFlavour()); + EXPECT_EQ(bookmark.getLanguage(), book.getCommaSeparatedLanguages()); +} + TEST_F(LibraryTest, getBookMarksTest) { - auto bookId1 = lib->getBooksIds()[0]; - auto bookId2 = lib->getBooksIds()[1]; + auto bookId1 = "0c45160e-f917-760a-9159-dfe3c53cdcdd"; + auto bookId2 = "0189d9be-2fd0-b4b6-7300-20fab0b5cdc8"; - lib->addBookmark(createBookmark(bookId1)); + auto book1 = lib->getBookById(bookId1); + auto book2 = lib->getBookById(bookId2); + + lib->addBookmark(createBookmark(book1)); lib->addBookmark(createBookmark("invalid-book-id")); - lib->addBookmark(createBookmark(bookId2)); + lib->addBookmark(createBookmark(book2)); auto onlyValidBookmarks = lib->getBookmarks(); auto allBookmarks = lib->getBookmarks(false); @@ -374,9 +400,10 @@ TEST_F(LibraryTest, bookmarksSerializationTest) auto book1 = lib->getBookById(bookId1); auto book2 = lib->getBookById(bookId2); + // Create bookmarks using three different ways. lib->addBookmark(createBookmark(bookId1, "a/url", "Article title1")); lib->addBookmark(createBookmark("invalid-book-id", "another/url", "Unknown title")); - lib->addBookmark(createBookmark(bookId2, "a/url/2", "Article title2")); + lib->addBookmark(createBookmark(book2, "a/url/2", "Article title2")); lib->writeBookmarksToFile("__test__bookmarks.xml"); From b16f6b9561535dcc6d5433d5335ff05fb6ec2e5b Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Wed, 17 Jan 2024 17:27:01 +0100 Subject: [PATCH 08/18] Allow to filter books by flavour. --- include/library.h | 5 +++++ src/library.cpp | 25 +++++++++++++++++++++++++ test/library.cpp | 19 +++++++++++++++++++ 3 files changed, 49 insertions(+) diff --git a/include/library.h b/include/library.h index 89514590b..f44b31ee1 100644 --- a/include/library.h +++ b/include/library.h @@ -71,6 +71,7 @@ class Filter { std::string _query; bool _queryIsPartial; std::string _name; + std::string _flavour; public: // functions Filter(); @@ -130,6 +131,7 @@ class Filter { Filter& maxSize(size_t size); Filter& query(std::string query, bool partial=true); Filter& name(std::string name); + Filter& flavour(std::string flavour); Filter& clearLang(); Filter& clearCategory(); @@ -152,6 +154,9 @@ class Filter { bool hasCreator() const; const std::string& getCreator() const { return _creator; } + bool hasFlavour() const; + const std::string& getFlavour() const { return _flavour; } + const Tags& getAcceptTags() const { return _acceptTags; } const Tags& getRejectTags() const { return _rejectTags; } diff --git a/src/library.cpp b/src/library.cpp index a62d4dac6..bc16dc184 100644 --- a/src/library.cpp +++ b/src/library.cpp @@ -437,6 +437,7 @@ void Library::updateBookDB(const Book& book) indexer.index_text(normalizeText(book.getCreator()), 1, "A"); indexer.index_text(normalizeText(book.getPublisher()), 1, "XP"); doc.add_term("XN"+normalizeText(book.getName())); + indexer.index_text(normalizeText(book.getFlavour()), 1, "XF"); indexer.index_text(normalizeText(book.getCategory()), 1, "XC"); for ( const auto& tag : split(normalizeText(book.getTags()), ";") ) { @@ -477,6 +478,7 @@ Xapian::Query buildXapianQueryFromFilterQuery(const Filter& filter) queryParser.add_prefix("title", "S"); queryParser.add_prefix("description", "XD"); queryParser.add_prefix("name", "XN"); + queryParser.add_prefix("flavour", "XF"); queryParser.add_prefix("category", "XC"); queryParser.add_prefix("lang", "L"); queryParser.add_prefix("publisher", "XP"); @@ -503,6 +505,12 @@ Xapian::Query nameQuery(const std::string& name) return Xapian::Query("XN" + normalizeText(name)); } +Xapian::Query flavourQuery(const std::string& name) +{ + return Xapian::Query("XF" + normalizeText(name)); +} + + Xapian::Query multipleParamQuery(const std::string& commaSeparatedList, const std::string& prefix) { Xapian::Query q; @@ -570,6 +578,9 @@ Xapian::Query buildXapianQuery(const Filter& filter) if ( filter.hasName() ) { q = Xapian::Query(Xapian::Query::OP_AND, q, nameQuery(filter.getName())); } + if ( filter.hasFlavour() ) { + q = Xapian::Query(Xapian::Query::OP_AND, q, flavourQuery(filter.getFlavour())); + } if ( filter.hasCategory() ) { q = Xapian::Query(Xapian::Query::OP_AND, q, categoryQuery(filter.getCategory())); } @@ -735,6 +746,7 @@ enum filterTypes { QUERY = FLAG(12), NAME = FLAG(13), CATEGORY = FLAG(14), + FLAVOUR = FLAG(15), }; Filter& Filter::local(bool accept) @@ -836,6 +848,13 @@ Filter& Filter::name(std::string name) activeFilters |= NAME; return *this; } + +Filter& Filter::flavour(std::string flavour) +{ + _flavour = flavour; + activeFilters |= FLAVOUR; + return *this; +} Filter& Filter::clearLang() { @@ -881,6 +900,12 @@ bool Filter::hasCreator() const return ACTIVE(_CREATOR); } +bool Filter::hasFlavour() const +{ + return ACTIVE(FLAVOUR); +} + + bool Filter::accept(const Book& book) const { auto local = !book.getPath().empty(); diff --git a/test/library.cpp b/test/library.cpp index f75a4dae9..734351216 100644 --- a/test/library.cpp +++ b/test/library.cpp @@ -570,6 +570,25 @@ TEST_F(LibraryTest, filterByLanguage) ); } +TEST_F(LibraryTest, filterByFlavour) +{ + EXPECT_FILTER_RESULTS(kiwix::Filter().flavour("full"), + "Géographie par Wikipédia", + "Tania Louis", + "Wikiquote" + ); + + EXPECT_FILTER_RESULTS(kiwix::Filter().query("flavour:full"), + "Géographie par Wikipédia", + "Tania Louis", + "Wikiquote" + ); + + EXPECT_FILTER_RESULTS(kiwix::Filter().query("full"), + /* no results */ + ); +} + TEST_F(LibraryTest, filterByTags) { EXPECT_FILTER_RESULTS(kiwix::Filter().acceptTags({"stackexchange"}), From 8d97686b81a32212f8d0ce10bb886e1f1d695ddc Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Fri, 19 Jan 2024 11:04:20 +0100 Subject: [PATCH 09/18] Introduce `migrateBookmarks` to move (invalid) bookmarks to new books. --- include/library.h | 36 +++++++++++++++++- src/library.cpp | 88 ++++++++++++++++++++++++++++++++++++++++++ test/library.cpp | 97 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 220 insertions(+), 1 deletion(-) diff --git a/include/library.h b/include/library.h index f44b31ee1..52f4185f3 100644 --- a/include/library.h +++ b/include/library.h @@ -255,7 +255,7 @@ class Library: public std::enable_shared_from_this void addBookmark(const Bookmark& bookmark); /** - * Remove a bookmarkk + * Remove a bookmark * * @param zimId The zimId of the bookmark. * @param url The url of the bookmark. @@ -263,6 +263,39 @@ class Library: public std::enable_shared_from_this */ bool removeBookmark(const std::string& zimId, const std::string& url); + /** + * Migrate all invalid bookmarks. + * + * All invalid bookmarks (ie pointing to unknown books, no check is made on bookmark pointing to + * invalid articles of valid book) will be migrated (if possible) to a better book. + * "Better book", will be determined using heuristics based on book name, flavour and date. + * + * @return A tuple: , . + */ + std::tuple migrateBookmarks(); + + /** + * Migrate all bookmarks associated to a specific book. + * + * All bookmarks associated to `sourceBookId` book will be migrated to a better book. + * "Better book", will be determined using heuristics based on book name, flavour and date. + + * @param source the source bookId of the bookmarks to migrate. + * @return The number of bookmarks updated. + */ + int migrateBookmarks(const std::string& sourceBookId); + + /** + * Migrate bookmarks + * + * Migrate all bookmarks pointing to `source` to `destination`. + * + * @param sourceBookId the source bookId of the bookmarks to migrate. + * @param targetBookId the destination bookId to migrate the bookmarks to. + * @return The number of bookmarks updated. + */ + int migrateBookmarks(const std::string& sourceBookId, const std::string& targetBookId); + // XXX: This is a non-thread-safe operation const Book& getBookById(const std::string& id) const; // XXX: This is a non-thread-safe operation @@ -408,6 +441,7 @@ private: // functions AttributeCounts getBookAttributeCounts(BookStrPropMemFn p) const; std::vector getBookPropValueSet(BookStrPropMemFn p) const; BookIdCollection filterViaBookDB(const Filter& filter) const; + std::string getBestTargetBookId(const Bookmark& bookmark) const; unsigned int getBookCount_not_protected(const bool localBooks, const bool remoteBooks) const; void updateBookDB(const Book& book); void dropCache(const std::string& bookId); diff --git a/src/library.cpp b/src/library.cpp index bc16dc184..a8336fe9f 100644 --- a/src/library.cpp +++ b/src/library.cpp @@ -157,6 +157,94 @@ bool Library::removeBookmark(const std::string& zimId, const std::string& url) return false; } +std::tuple Library::migrateBookmarks() { + std::set sourceBooks; + int invalidBookmarks = 0; + { + std::lock_guard lock(m_mutex); + for(auto& bookmark:m_bookmarks) { + if (m_books.find(bookmark.getBookId()) == m_books.end()) { + invalidBookmarks += 1; + sourceBooks.insert(bookmark.getBookId()); + } + } + } + int changed = 0; + for(auto& sourceBook:sourceBooks) { + changed += migrateBookmarks(sourceBook); + } + return std::make_tuple(changed, invalidBookmarks); +} + +std::string Library::getBestTargetBookId(const Bookmark& bookmark) const { + // Search for a existing book with the same name + auto book_filter = Filter(); + if (!bookmark.getBookName().empty()) { + book_filter.name(bookmark.getBookName()); + } else { + // We don't have a name stored (older bookmarks) + // Fallback on title (All bookmarks should have one, but let's be safe against wrongly filled bookmark) + if (bookmark.getBookTitle().empty()) { + // No bookName nor bookTitle, no way to find target book. + return ""; + } + book_filter.query("title:\""+bookmark.getBookTitle() + "\""); + } + auto targetBooks = filter(book_filter); + // Remove source book + auto foundIT = std::find(targetBooks.begin(), targetBooks.end(), bookmark.getBookId()); + if (foundIT != targetBooks.end()) { + targetBooks.erase(foundIT); + } + + if (targetBooks.empty()) { + // No existing book found for the target, or the same than source. + return ""; + } + if (targetBooks.size() != 1) { + sort(targetBooks, DATE, false); + } + return targetBooks[0]; +} + +int Library::migrateBookmarks(const std::string& sourceBookId) { + std::lock_guard lock(m_mutex); + + Bookmark firstBookmarkToChange; + for(auto& bookmark:m_bookmarks) { + if (bookmark.getBookId() == sourceBookId) { + firstBookmarkToChange = bookmark; + break; + } + } + + if (firstBookmarkToChange.getBookId().empty()) { + return 0; + } + + std::string betterBook = getBestTargetBookId(firstBookmarkToChange); + + if (betterBook.empty()) { + return 0; + } + + return migrateBookmarks(sourceBookId, betterBook); +} + +int Library::migrateBookmarks(const std::string& sourceBookId, const std::string& targetBookId) { + if (sourceBookId == targetBookId) { + return 0; + } + int changed = 0; + for (auto& bookmark:m_bookmarks) { + if (bookmark.getBookId() == sourceBookId) { + bookmark.setBookId(targetBookId); + changed += 1; + } + } + return changed; +} + void Library::dropCache(const std::string& id) { diff --git a/test/library.cpp b/test/library.cpp index 734351216..9b3d17a95 100644 --- a/test/library.cpp +++ b/test/library.cpp @@ -456,6 +456,103 @@ TEST_F(LibraryTest, bookmarksSerializationTest) EXPECT_EQ(bookmark3.getDate(), book2.getDate()); } +TEST_F(LibraryTest, MigrateBookmark) +{ + auto bookId1 = "0c45160e-f917-760a-9159-dfe3c53cdcdd"; + auto bookId2 = "0189d9be-2fd0-b4b6-7300-20fab0b5cdc8"; + + auto book1 = lib->getBookById(bookId1); + auto book2 = lib->getBookById(bookId2); + + lib->addBookmark(createBookmark(book1)); + lib->addBookmark(createBookmark("invalid-book-id")); + lib->addBookmark(createBookmark(book2)); + + auto wrongIdBookmark = createBookmark(book1); + wrongIdBookmark.setBookId("wrong-book-id"); + lib->addBookmark(wrongIdBookmark); + + auto wrongIdBookmarkNoName = createBookmark(book2); + wrongIdBookmarkNoName.setBookId("wrong-book-id-noname"); + wrongIdBookmarkNoName.setBookName(""); + lib->addBookmark(wrongIdBookmarkNoName); + + auto onlyValidBookmarks = lib->getBookmarks(); + auto allBookmarks = lib->getBookmarks(false); + + ASSERT_EQ(onlyValidBookmarks.size(), 2); + EXPECT_EQ(onlyValidBookmarks[0].getBookId(), bookId1); + EXPECT_EQ(onlyValidBookmarks[1].getBookId(), bookId2); + + ASSERT_EQ(allBookmarks.size(), 5); + EXPECT_EQ(allBookmarks[0].getBookId(), bookId1); + EXPECT_EQ(allBookmarks[1].getBookId(), "invalid-book-id"); + EXPECT_EQ(allBookmarks[2].getBookId(), bookId2); + EXPECT_EQ(allBookmarks[3].getBookId(), "wrong-book-id"); + EXPECT_EQ(allBookmarks[4].getBookId(), "wrong-book-id-noname"); + + ASSERT_EQ(lib->migrateBookmarks("no-existant-book"), 0); + + ASSERT_EQ(lib->migrateBookmarks(), std::make_tuple(2, 3)); + + onlyValidBookmarks = lib->getBookmarks(); + allBookmarks = lib->getBookmarks(false); + + ASSERT_EQ(onlyValidBookmarks.size(), 4); + EXPECT_EQ(onlyValidBookmarks[0].getBookId(), bookId1); + EXPECT_EQ(onlyValidBookmarks[1].getBookId(), bookId2); + EXPECT_EQ(onlyValidBookmarks[2].getBookId(), bookId1); + EXPECT_EQ(onlyValidBookmarks[3].getBookId(), bookId2); + + ASSERT_EQ(allBookmarks.size(), 5); + EXPECT_EQ(allBookmarks[0].getBookId(), bookId1); + EXPECT_EQ(allBookmarks[1].getBookId(), "invalid-book-id"); + EXPECT_EQ(allBookmarks[2].getBookId(), bookId2); + EXPECT_EQ(allBookmarks[3].getBookId(), bookId1); + EXPECT_EQ(allBookmarks[4].getBookId(), bookId2); + + ASSERT_EQ(lib->migrateBookmarks(), std::make_tuple(0, 1)); + ASSERT_EQ(lib->migrateBookmarks(bookId1), 0); + ASSERT_EQ(lib->migrateBookmarks(bookId1, bookId2), 2); + + onlyValidBookmarks = lib->getBookmarks(); + allBookmarks = lib->getBookmarks(false); + + ASSERT_EQ(onlyValidBookmarks.size(), 4); + EXPECT_EQ(onlyValidBookmarks[0].getBookId(), bookId2); + EXPECT_EQ(onlyValidBookmarks[1].getBookId(), bookId2); + EXPECT_EQ(onlyValidBookmarks[2].getBookId(), bookId2); + EXPECT_EQ(onlyValidBookmarks[3].getBookId(), bookId2); + + + ASSERT_EQ(allBookmarks.size(), 5); + EXPECT_EQ(allBookmarks[0].getBookId(), bookId2); + EXPECT_EQ(allBookmarks[1].getBookId(), "invalid-book-id"); + EXPECT_EQ(allBookmarks[2].getBookId(), bookId2); + EXPECT_EQ(allBookmarks[3].getBookId(), bookId2); + EXPECT_EQ(allBookmarks[4].getBookId(), bookId2); + + ASSERT_EQ(lib->migrateBookmarks("invalid-book-id", bookId1), 1); + + onlyValidBookmarks = lib->getBookmarks(); + allBookmarks = lib->getBookmarks(false); + + ASSERT_EQ(onlyValidBookmarks.size(), 5); + EXPECT_EQ(onlyValidBookmarks[0].getBookId(), bookId2); + EXPECT_EQ(onlyValidBookmarks[1].getBookId(), bookId1); + EXPECT_EQ(onlyValidBookmarks[2].getBookId(), bookId2); + EXPECT_EQ(onlyValidBookmarks[3].getBookId(), bookId2); + EXPECT_EQ(onlyValidBookmarks[4].getBookId(), bookId2); + + + ASSERT_EQ(allBookmarks.size(), 5); + EXPECT_EQ(allBookmarks[0].getBookId(), bookId2); + EXPECT_EQ(allBookmarks[1].getBookId(), bookId1); + EXPECT_EQ(allBookmarks[2].getBookId(), bookId2); + EXPECT_EQ(allBookmarks[3].getBookId(), bookId2); + EXPECT_EQ(allBookmarks[4].getBookId(), bookId2); +} + TEST_F(LibraryTest, sanityCheck) { From 14c9530afaeb570b255f8ea1d5d712b9da0e1654 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Fri, 19 Jan 2024 15:16:39 +0100 Subject: [PATCH 10/18] [Test] Introduce variant books in sample library. We will need them to test flavour/date bookmarks migration. --- test/library.cpp | 100 ++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 91 insertions(+), 9 deletions(-) diff --git a/test/library.cpp b/test/library.cpp index 9b3d17a95..5c55f2057 100644 --- a/test/library.cpp +++ b/test/library.cpp @@ -48,6 +48,48 @@ const char * sampleOpdsStream = R"( 1100 172 + + Encyclopédie de la Tunisie + wikipedia_fr_tunisie + novid + urn:uuid:0c45160e-f917-760a-9159-dfe3c53cdcdd_updated1yearlater + 2019-10-08T00:00::00:Z + 8 Oct 2019 + fra + Le meilleur de Wikipédia sur la Tunisie. Updated in 2019 + + Wikipedia + + + + + Encyclopédie de la Tunisie + wikipedia_fr_tunisie + other_flavour + urn:uuid:0c45160e-f917-760a-9159-dfe3c53cdcdd_flavour + 2018-10-08T00:00::00:Z + 8 Oct 2018 + fra + Le meilleur de Wikipédia sur la Tunisie. With another flavour + + Wikipedia + + + + + Encyclopédie de la Tunisie + wikipedia_fr_tunisie + other_flavour + urn:uuid:0c45160e-f917-760a-9159-dfe3c53cdcdd_updated1yearlater_flavour + 2019-10-08T00:00::00:Z + 8 Oct 2019 + fra + Le meilleur de Wikipédia sur la Tunisie. Updated in 2019, and other flavour + + Wikipedia + + + Tania Louis urn:uuid:0d0bcd57-d3f6-cb22-44cc-a723ccb4e1b2 @@ -260,7 +302,7 @@ TEST(LibraryOpdsImportTest, allInOne) kiwix::Manager manager(lib); manager.readOpds(sampleOpdsStream, "library-opds-import.unittests.dev"); - EXPECT_EQ(10U, lib->getBookCount(true, true)); + EXPECT_EQ(13U, lib->getBookCount(true, true)); { const kiwix::Book& book1 = lib->getBookById("0c45160e-f917-760a-9159-dfe3c53cdcdd"); @@ -458,8 +500,8 @@ TEST_F(LibraryTest, bookmarksSerializationTest) TEST_F(LibraryTest, MigrateBookmark) { - auto bookId1 = "0c45160e-f917-760a-9159-dfe3c53cdcdd"; - auto bookId2 = "0189d9be-2fd0-b4b6-7300-20fab0b5cdc8"; + std::string bookId1 = "0c45160e-f917-760a-9159-dfe3c53cdcdd"; + std::string bookId2 = "0189d9be-2fd0-b4b6-7300-20fab0b5cdc8"; auto book1 = lib->getBookById(bookId1); auto book2 = lib->getBookById(bookId2); @@ -501,20 +543,30 @@ TEST_F(LibraryTest, MigrateBookmark) ASSERT_EQ(onlyValidBookmarks.size(), 4); EXPECT_EQ(onlyValidBookmarks[0].getBookId(), bookId1); EXPECT_EQ(onlyValidBookmarks[1].getBookId(), bookId2); - EXPECT_EQ(onlyValidBookmarks[2].getBookId(), bookId1); + EXPECT_EQ(onlyValidBookmarks[2].getBookId(), bookId1+"_updated1yearlater"); EXPECT_EQ(onlyValidBookmarks[3].getBookId(), bookId2); ASSERT_EQ(allBookmarks.size(), 5); EXPECT_EQ(allBookmarks[0].getBookId(), bookId1); EXPECT_EQ(allBookmarks[1].getBookId(), "invalid-book-id"); EXPECT_EQ(allBookmarks[2].getBookId(), bookId2); - EXPECT_EQ(allBookmarks[3].getBookId(), bookId1); + EXPECT_EQ(allBookmarks[3].getBookId(), bookId1+"_updated1yearlater"); EXPECT_EQ(allBookmarks[4].getBookId(), bookId2); ASSERT_EQ(lib->migrateBookmarks(), std::make_tuple(0, 1)); - ASSERT_EQ(lib->migrateBookmarks(bookId1), 0); - ASSERT_EQ(lib->migrateBookmarks(bookId1, bookId2), 2); + ASSERT_EQ(lib->migrateBookmarks(bookId1), 1); + allBookmarks = lib->getBookmarks(false); + ASSERT_EQ(allBookmarks.size(), 5); + EXPECT_EQ(allBookmarks[0].getBookId(), bookId1+"_updated1yearlater"); + EXPECT_EQ(allBookmarks[1].getBookId(), "invalid-book-id"); + EXPECT_EQ(allBookmarks[2].getBookId(), bookId2); + EXPECT_EQ(allBookmarks[3].getBookId(), bookId1+"_updated1yearlater"); + EXPECT_EQ(allBookmarks[4].getBookId(), bookId2); + + ASSERT_EQ(lib->migrateBookmarks(bookId1, bookId2), 0); // No more bookId1 bookmark + + ASSERT_EQ(lib->migrateBookmarks(bookId1+"_updated1yearlater", bookId2), 2); onlyValidBookmarks = lib->getBookmarks(); allBookmarks = lib->getBookmarks(false); @@ -556,7 +608,7 @@ TEST_F(LibraryTest, MigrateBookmark) TEST_F(LibraryTest, sanityCheck) { - EXPECT_EQ(lib->getBookCount(true, true), 12U); + EXPECT_EQ(lib->getBookCount(true, true), 15U); EXPECT_EQ(lib->getBooksLanguages(), std::vector({"deu", "eng", "fra", "ita", "spa"}) ); @@ -608,6 +660,9 @@ TEST_F(LibraryTest, filterLocal) ); EXPECT_FILTER_RESULTS(kiwix::Filter().local(false), + "Encyclopédie de la Tunisie", + "Encyclopédie de la Tunisie", + "Encyclopédie de la Tunisie", "Encyclopédie de la Tunisie", "Granblue Fantasy Wiki", "Géographie par Wikipédia", @@ -624,6 +679,9 @@ TEST_F(LibraryTest, filterLocal) TEST_F(LibraryTest, filterRemote) { EXPECT_FILTER_RESULTS(kiwix::Filter().remote(true), + "Encyclopédie de la Tunisie", + "Encyclopédie de la Tunisie", + "Encyclopédie de la Tunisie", "Encyclopédie de la Tunisie", "Granblue Fantasy Wiki", "Géographie par Wikipédia", @@ -785,6 +843,9 @@ TEST_F(LibraryTest, filterByQuery) EXPECT_FILTER_RESULTS(kiwix::Filter().query("Wiki"), "An example ZIM archive", // due to the "wikibooks" tag "Encyclopédie de la Tunisie", + "Encyclopédie de la Tunisie", + "Encyclopédie de la Tunisie", + "Encyclopédie de la Tunisie", "Granblue Fantasy Wiki", "Géographie par Wikipédia", "Mathématiques", // due to the "wikipedia" tag @@ -804,6 +865,9 @@ TEST_F(LibraryTest, filteringByEmptyQueryReturnsAllEntries) EXPECT_FILTER_RESULTS(kiwix::Filter().query(""), "An example ZIM archive", "Encyclopédie de la Tunisie", + "Encyclopédie de la Tunisie", + "Encyclopédie de la Tunisie", + "Encyclopédie de la Tunisie", "Granblue Fantasy Wiki", "Géographie par Wikipédia", "Islam Stack Exchange", @@ -820,6 +884,9 @@ TEST_F(LibraryTest, filteringByEmptyQueryReturnsAllEntries) TEST_F(LibraryTest, filterByCreator) { EXPECT_FILTER_RESULTS(kiwix::Filter().creator("Wikipedia"), + "Encyclopédie de la Tunisie", + "Encyclopédie de la Tunisie", + "Encyclopédie de la Tunisie", "Encyclopédie de la Tunisie", "Géographie par Wikipédia", "Mathématiques", @@ -861,6 +928,9 @@ TEST_F(LibraryTest, filterByCreator) ); EXPECT_FILTER_RESULTS(kiwix::Filter().query("creator:Wikipedia"), + "Encyclopédie de la Tunisie", + "Encyclopédie de la Tunisie", + "Encyclopédie de la Tunisie", "Encyclopédie de la Tunisie", "Géographie par Wikipédia", "Mathématiques", @@ -968,6 +1038,9 @@ TEST_F(LibraryTest, filterByMaxSize) TEST_F(LibraryTest, filterByMultipleCriteria) { EXPECT_FILTER_RESULTS(kiwix::Filter().query("Wiki").creator("Wikipedia"), + "Encyclopédie de la Tunisie", + "Encyclopédie de la Tunisie", + "Encyclopédie de la Tunisie", "Encyclopédie de la Tunisie", "Géographie par Wikipédia", "Mathématiques", // due to the "wikipedia" tag @@ -975,11 +1048,17 @@ TEST_F(LibraryTest, filterByMultipleCriteria) ); EXPECT_FILTER_RESULTS(kiwix::Filter().query("Wiki").creator("Wikipedia").maxSize(100000000UL), + "Encyclopédie de la Tunisie", + "Encyclopédie de la Tunisie", + "Encyclopédie de la Tunisie", "Encyclopédie de la Tunisie", "Ray Charles" ); EXPECT_FILTER_RESULTS(kiwix::Filter().query("Wiki").creator("Wikipedia").maxSize(100000000UL).local(false), + "Encyclopédie de la Tunisie", + "Encyclopédie de la Tunisie", + "Encyclopédie de la Tunisie", "Encyclopédie de la Tunisie" ); } @@ -1038,6 +1117,9 @@ TEST_F(LibraryTest, removeBooksNotUpdatedSince) EXPECT_FILTER_RESULTS(kiwix::Filter(), "An example ZIM archive", "Encyclopédie de la Tunisie", + "Encyclopédie de la Tunisie", + "Encyclopédie de la Tunisie", + "Encyclopédie de la Tunisie", "Granblue Fantasy Wiki", "Géographie par Wikipédia", "Islam Stack Exchange", @@ -1059,7 +1141,7 @@ TEST_F(LibraryTest, removeBooksNotUpdatedSince) const uint64_t rev2 = lib->getRevision(); - EXPECT_EQ(9u, lib->removeBooksNotUpdatedSince(rev)); + EXPECT_EQ(12u, lib->removeBooksNotUpdatedSince(rev)); EXPECT_GT(lib->getRevision(), rev2); From 167e0dc4b3995ba759388407c8be88db5ffe24a0 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Fri, 19 Jan 2024 16:57:18 +0100 Subject: [PATCH 11/18] Only migrate bookmarks to books with the same flavour. If there is no book with the same flavour, but book with same name and different flavour, we do the migration to the other book. --- src/library.cpp | 7 ++++++- test/library.cpp | 32 ++++++++++++++++++++++---------- 2 files changed, 28 insertions(+), 11 deletions(-) diff --git a/src/library.cpp b/src/library.cpp index a8336fe9f..3aa6157a1 100644 --- a/src/library.cpp +++ b/src/library.cpp @@ -202,6 +202,11 @@ std::string Library::getBestTargetBookId(const Bookmark& bookmark) const { return ""; } if (targetBooks.size() != 1) { + // We have several, reduce to same flavour + auto flavouredTargetBooks = filter(book_filter.flavour(bookmark.getBookFlavour())); + if (!flavouredTargetBooks.empty()) { + targetBooks = flavouredTargetBooks; + } sort(targetBooks, DATE, false); } return targetBooks[0]; @@ -223,7 +228,7 @@ int Library::migrateBookmarks(const std::string& sourceBookId) { } std::string betterBook = getBestTargetBookId(firstBookmarkToChange); - + if (betterBook.empty()) { return 0; } diff --git a/test/library.cpp b/test/library.cpp index 5c55f2057..5ed3ff498 100644 --- a/test/library.cpp +++ b/test/library.cpp @@ -504,6 +504,7 @@ TEST_F(LibraryTest, MigrateBookmark) std::string bookId2 = "0189d9be-2fd0-b4b6-7300-20fab0b5cdc8"; auto book1 = lib->getBookById(bookId1); + auto book1Flavour = lib->getBookById(bookId1+"_flavour"); auto book2 = lib->getBookById(bookId2); lib->addBookmark(createBookmark(book1)); @@ -519,6 +520,10 @@ TEST_F(LibraryTest, MigrateBookmark) wrongIdBookmarkNoName.setBookName(""); lib->addBookmark(wrongIdBookmarkNoName); + auto wrongIdFlavourBookmark = createBookmark(book1Flavour); + wrongIdFlavourBookmark.setBookId("wrong-book-flavour-id"); + lib->addBookmark(wrongIdFlavourBookmark); + auto onlyValidBookmarks = lib->getBookmarks(); auto allBookmarks = lib->getBookmarks(false); @@ -526,43 +531,47 @@ TEST_F(LibraryTest, MigrateBookmark) EXPECT_EQ(onlyValidBookmarks[0].getBookId(), bookId1); EXPECT_EQ(onlyValidBookmarks[1].getBookId(), bookId2); - ASSERT_EQ(allBookmarks.size(), 5); + ASSERT_EQ(allBookmarks.size(), 6); EXPECT_EQ(allBookmarks[0].getBookId(), bookId1); EXPECT_EQ(allBookmarks[1].getBookId(), "invalid-book-id"); EXPECT_EQ(allBookmarks[2].getBookId(), bookId2); EXPECT_EQ(allBookmarks[3].getBookId(), "wrong-book-id"); EXPECT_EQ(allBookmarks[4].getBookId(), "wrong-book-id-noname"); + EXPECT_EQ(allBookmarks[5].getBookId(), "wrong-book-flavour-id"); ASSERT_EQ(lib->migrateBookmarks("no-existant-book"), 0); - ASSERT_EQ(lib->migrateBookmarks(), std::make_tuple(2, 3)); + ASSERT_EQ(lib->migrateBookmarks(), std::make_tuple(3, 4)); onlyValidBookmarks = lib->getBookmarks(); allBookmarks = lib->getBookmarks(false); - ASSERT_EQ(onlyValidBookmarks.size(), 4); + ASSERT_EQ(onlyValidBookmarks.size(), 5); EXPECT_EQ(onlyValidBookmarks[0].getBookId(), bookId1); EXPECT_EQ(onlyValidBookmarks[1].getBookId(), bookId2); EXPECT_EQ(onlyValidBookmarks[2].getBookId(), bookId1+"_updated1yearlater"); EXPECT_EQ(onlyValidBookmarks[3].getBookId(), bookId2); + EXPECT_EQ(onlyValidBookmarks[4].getBookId(), bookId1+"_updated1yearlater_flavour"); - ASSERT_EQ(allBookmarks.size(), 5); + ASSERT_EQ(allBookmarks.size(), 6); EXPECT_EQ(allBookmarks[0].getBookId(), bookId1); EXPECT_EQ(allBookmarks[1].getBookId(), "invalid-book-id"); EXPECT_EQ(allBookmarks[2].getBookId(), bookId2); EXPECT_EQ(allBookmarks[3].getBookId(), bookId1+"_updated1yearlater"); EXPECT_EQ(allBookmarks[4].getBookId(), bookId2); + EXPECT_EQ(allBookmarks[5].getBookId(), bookId1+"_updated1yearlater_flavour"); ASSERT_EQ(lib->migrateBookmarks(), std::make_tuple(0, 1)); ASSERT_EQ(lib->migrateBookmarks(bookId1), 1); allBookmarks = lib->getBookmarks(false); - ASSERT_EQ(allBookmarks.size(), 5); + ASSERT_EQ(allBookmarks.size(), 6); EXPECT_EQ(allBookmarks[0].getBookId(), bookId1+"_updated1yearlater"); EXPECT_EQ(allBookmarks[1].getBookId(), "invalid-book-id"); EXPECT_EQ(allBookmarks[2].getBookId(), bookId2); EXPECT_EQ(allBookmarks[3].getBookId(), bookId1+"_updated1yearlater"); EXPECT_EQ(allBookmarks[4].getBookId(), bookId2); + EXPECT_EQ(allBookmarks[5].getBookId(), bookId1+"_updated1yearlater_flavour"); ASSERT_EQ(lib->migrateBookmarks(bookId1, bookId2), 0); // No more bookId1 bookmark @@ -570,39 +579,42 @@ TEST_F(LibraryTest, MigrateBookmark) onlyValidBookmarks = lib->getBookmarks(); allBookmarks = lib->getBookmarks(false); - ASSERT_EQ(onlyValidBookmarks.size(), 4); + ASSERT_EQ(onlyValidBookmarks.size(), 5); EXPECT_EQ(onlyValidBookmarks[0].getBookId(), bookId2); EXPECT_EQ(onlyValidBookmarks[1].getBookId(), bookId2); EXPECT_EQ(onlyValidBookmarks[2].getBookId(), bookId2); EXPECT_EQ(onlyValidBookmarks[3].getBookId(), bookId2); + EXPECT_EQ(onlyValidBookmarks[4].getBookId(), bookId1+"_updated1yearlater_flavour"); - - ASSERT_EQ(allBookmarks.size(), 5); + ASSERT_EQ(allBookmarks.size(), 6); EXPECT_EQ(allBookmarks[0].getBookId(), bookId2); EXPECT_EQ(allBookmarks[1].getBookId(), "invalid-book-id"); EXPECT_EQ(allBookmarks[2].getBookId(), bookId2); EXPECT_EQ(allBookmarks[3].getBookId(), bookId2); EXPECT_EQ(allBookmarks[4].getBookId(), bookId2); + EXPECT_EQ(allBookmarks[5].getBookId(), bookId1+"_updated1yearlater_flavour"); ASSERT_EQ(lib->migrateBookmarks("invalid-book-id", bookId1), 1); onlyValidBookmarks = lib->getBookmarks(); allBookmarks = lib->getBookmarks(false); - ASSERT_EQ(onlyValidBookmarks.size(), 5); + ASSERT_EQ(onlyValidBookmarks.size(), 6); EXPECT_EQ(onlyValidBookmarks[0].getBookId(), bookId2); EXPECT_EQ(onlyValidBookmarks[1].getBookId(), bookId1); EXPECT_EQ(onlyValidBookmarks[2].getBookId(), bookId2); EXPECT_EQ(onlyValidBookmarks[3].getBookId(), bookId2); EXPECT_EQ(onlyValidBookmarks[4].getBookId(), bookId2); + EXPECT_EQ(onlyValidBookmarks[5].getBookId(), bookId1+"_updated1yearlater_flavour"); - ASSERT_EQ(allBookmarks.size(), 5); + ASSERT_EQ(allBookmarks.size(), 6); EXPECT_EQ(allBookmarks[0].getBookId(), bookId2); EXPECT_EQ(allBookmarks[1].getBookId(), bookId1); EXPECT_EQ(allBookmarks[2].getBookId(), bookId2); EXPECT_EQ(allBookmarks[3].getBookId(), bookId2); EXPECT_EQ(allBookmarks[4].getBookId(), bookId2); + EXPECT_EQ(allBookmarks[5].getBookId(), bookId1+"_updated1yearlater_flavour"); } From f3a604380c8047655d6256e8ef34a7651206a728 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Mon, 22 Jan 2024 15:51:42 +0100 Subject: [PATCH 12/18] Do not migrate bookmarks to an older book. At least, it must be explicitly asked by the user. --- include/library.h | 26 +++++++++++++++++++++----- src/library.cpp | 37 ++++++++++++++++++++++++------------- test/library.cpp | 17 +++++++++++++++++ 3 files changed, 62 insertions(+), 18 deletions(-) diff --git a/include/library.h b/include/library.h index 52f4185f3..895c20166 100644 --- a/include/library.h +++ b/include/library.h @@ -55,6 +55,18 @@ enum supportedListMode { NOVALID = 1 << 5 }; +enum MigrationMode { + /** When migrating bookmarks, do not allow to migrate to an older book than the currently pointed one. + * + * This is enforce only if the bookmark point to a valid books. If bookmark point to a invalid book, + * it will be migrated to an existing one, even if older. + */ + UPGRADE_ONLY = 0, + + /** Allow to migrate the bookmark to an older book. */ + ALLOW_DOWNGRADE = 1, +}; + class Filter { public: // types using Tags = std::vector; @@ -272,18 +284,21 @@ class Library: public std::enable_shared_from_this * * @return A tuple: , . */ - std::tuple migrateBookmarks(); + std::tuple migrateBookmarks(MigrationMode migrationMode = ALLOW_DOWNGRADE); /** * Migrate all bookmarks associated to a specific book. * * All bookmarks associated to `sourceBookId` book will be migrated to a better book. * "Better book", will be determined using heuristics based on book name, flavour and date. - - * @param source the source bookId of the bookmarks to migrate. + * Be carrefull, when using with `migrationModde == ALLOW_DOWGRADE`, the bookmark will be migrated to a different book + * even if the bookmark points to an existing book and the other book is older than the current one. + * + * @param sourceBookId the source bookId of the bookmarks to migrate. + * @param migrationMode how we will find the best book. * @return The number of bookmarks updated. */ - int migrateBookmarks(const std::string& sourceBookId); + int migrateBookmarks(const std::string& sourceBookId, MigrationMode migrationMode = UPGRADE_ONLY); /** * Migrate bookmarks @@ -441,7 +456,8 @@ private: // functions AttributeCounts getBookAttributeCounts(BookStrPropMemFn p) const; std::vector getBookPropValueSet(BookStrPropMemFn p) const; BookIdCollection filterViaBookDB(const Filter& filter) const; - std::string getBestTargetBookId(const Bookmark& bookmark) const; + void cleanupBookCollection(BookIdCollection& books, const std::string& sourceBookId, MigrationMode migrationMode) const; + std::string getBestTargetBookId(const Bookmark& bookmark, MigrationMode migrationMode) const; unsigned int getBookCount_not_protected(const bool localBooks, const bool remoteBooks) const; void updateBookDB(const Book& book); void dropCache(const std::string& bookId); diff --git a/src/library.cpp b/src/library.cpp index 3aa6157a1..e5cc989cc 100644 --- a/src/library.cpp +++ b/src/library.cpp @@ -157,7 +157,7 @@ bool Library::removeBookmark(const std::string& zimId, const std::string& url) return false; } -std::tuple Library::migrateBookmarks() { +std::tuple Library::migrateBookmarks(MigrationMode migrationMode) { std::set sourceBooks; int invalidBookmarks = 0; { @@ -171,12 +171,27 @@ std::tuple Library::migrateBookmarks() { } int changed = 0; for(auto& sourceBook:sourceBooks) { - changed += migrateBookmarks(sourceBook); + changed += migrateBookmarks(sourceBook, migrationMode); } return std::make_tuple(changed, invalidBookmarks); } -std::string Library::getBestTargetBookId(const Bookmark& bookmark) const { +void Library::cleanupBookCollection(BookIdCollection& books, const std::string& sourceBookId, MigrationMode migrationMode) const { + sort(books, DATE, false); + + // Remove source book + auto foundIT = std::find(books.begin(), books.end(), sourceBookId); + if (foundIT != books.end()) { + if (migrationMode == ALLOW_DOWNGRADE) { + books.erase(foundIT); + } else { + // books is sorted by date, remove the source and all older books + books.erase(foundIT, books.end()); + } + } +} + +std::string Library::getBestTargetBookId(const Bookmark& bookmark, MigrationMode migrationMode) const { // Search for a existing book with the same name auto book_filter = Filter(); if (!bookmark.getBookName().empty()) { @@ -191,11 +206,7 @@ std::string Library::getBestTargetBookId(const Bookmark& bookmark) const { book_filter.query("title:\""+bookmark.getBookTitle() + "\""); } auto targetBooks = filter(book_filter); - // Remove source book - auto foundIT = std::find(targetBooks.begin(), targetBooks.end(), bookmark.getBookId()); - if (foundIT != targetBooks.end()) { - targetBooks.erase(foundIT); - } + cleanupBookCollection(targetBooks, bookmark.getBookId(), migrationMode); if (targetBooks.empty()) { // No existing book found for the target, or the same than source. @@ -204,15 +215,15 @@ std::string Library::getBestTargetBookId(const Bookmark& bookmark) const { if (targetBooks.size() != 1) { // We have several, reduce to same flavour auto flavouredTargetBooks = filter(book_filter.flavour(bookmark.getBookFlavour())); + cleanupBookCollection(flavouredTargetBooks, bookmark.getBookId(), migrationMode); if (!flavouredTargetBooks.empty()) { targetBooks = flavouredTargetBooks; } - sort(targetBooks, DATE, false); } return targetBooks[0]; } -int Library::migrateBookmarks(const std::string& sourceBookId) { +int Library::migrateBookmarks(const std::string& sourceBookId, MigrationMode migrationMode) { std::lock_guard lock(m_mutex); Bookmark firstBookmarkToChange; @@ -227,8 +238,8 @@ int Library::migrateBookmarks(const std::string& sourceBookId) { return 0; } - std::string betterBook = getBestTargetBookId(firstBookmarkToChange); - + std::string betterBook = getBestTargetBookId(firstBookmarkToChange, migrationMode); + if (betterBook.empty()) { return 0; } @@ -244,7 +255,7 @@ int Library::migrateBookmarks(const std::string& sourceBookId, const std::string for (auto& bookmark:m_bookmarks) { if (bookmark.getBookId() == sourceBookId) { bookmark.setBookId(targetBookId); - changed += 1; + changed +=1; } } return changed; diff --git a/test/library.cpp b/test/library.cpp index 5ed3ff498..d63a44865 100644 --- a/test/library.cpp +++ b/test/library.cpp @@ -617,6 +617,23 @@ TEST_F(LibraryTest, MigrateBookmark) EXPECT_EQ(allBookmarks[5].getBookId(), bookId1+"_updated1yearlater_flavour"); } +TEST_F(LibraryTest, MigrateBookmarkOlder) +{ + auto bookId1 = "0c45160e-f917-760a-9159-dfe3c53cdcdd_updated1yearlater"; + + auto book1 = lib->getBookById(bookId1); + + lib->addBookmark(createBookmark(book1)); + + auto onlyValidBookmarks = lib->getBookmarks(); + + ASSERT_EQ(onlyValidBookmarks.size(), 1); + EXPECT_EQ(onlyValidBookmarks[0].getBookId(), bookId1); + + ASSERT_EQ(lib->migrateBookmarks(bookId1), 0); + ASSERT_EQ(lib->migrateBookmarks(bookId1, kiwix::ALLOW_DOWNGRADE), 1); +} + TEST_F(LibraryTest, sanityCheck) { From 3e9d50fecb7cf7c2f0ee996723d39964a21db256 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Wed, 7 Feb 2024 16:26:35 +0100 Subject: [PATCH 13/18] Make `getBestTargetBookId` public. --- include/library.h | 19 ++++++++++++++----- src/library.cpp | 1 + 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/include/library.h b/include/library.h index 895c20166..354876456 100644 --- a/include/library.h +++ b/include/library.h @@ -280,7 +280,7 @@ class Library: public std::enable_shared_from_this * * All invalid bookmarks (ie pointing to unknown books, no check is made on bookmark pointing to * invalid articles of valid book) will be migrated (if possible) to a better book. - * "Better book", will be determined using heuristics based on book name, flavour and date. + * "Better book", will be determined using method `getBestTargetBookId`. * * @return A tuple: , . */ @@ -290,9 +290,7 @@ class Library: public std::enable_shared_from_this * Migrate all bookmarks associated to a specific book. * * All bookmarks associated to `sourceBookId` book will be migrated to a better book. - * "Better book", will be determined using heuristics based on book name, flavour and date. - * Be carrefull, when using with `migrationModde == ALLOW_DOWGRADE`, the bookmark will be migrated to a different book - * even if the bookmark points to an existing book and the other book is older than the current one. + * "Better book", will be determined using method `getBestTargetBookId`. * * @param sourceBookId the source bookId of the bookmarks to migrate. * @param migrationMode how we will find the best book. @@ -311,6 +309,18 @@ class Library: public std::enable_shared_from_this */ int migrateBookmarks(const std::string& sourceBookId, const std::string& targetBookId); + /** + * Get the best available bookId for a bookmark. + * + * Given a bookmark, return the best available bookId. + * "best available bookId" is determined using heuristitcs based on book name, flavour and date. + * + * @param bookmark The bookmark to search the bookId for. + * @param migrationMode The migration mode to use. + * @return A bookId. Potentially empty string if no suitable book found. + */ + std::string getBestTargetBookId(const Bookmark& bookmark, MigrationMode migrationMode) const; + // XXX: This is a non-thread-safe operation const Book& getBookById(const std::string& id) const; // XXX: This is a non-thread-safe operation @@ -457,7 +467,6 @@ private: // functions std::vector getBookPropValueSet(BookStrPropMemFn p) const; BookIdCollection filterViaBookDB(const Filter& filter) const; void cleanupBookCollection(BookIdCollection& books, const std::string& sourceBookId, MigrationMode migrationMode) const; - std::string getBestTargetBookId(const Bookmark& bookmark, MigrationMode migrationMode) const; unsigned int getBookCount_not_protected(const bool localBooks, const bool remoteBooks) const; void updateBookDB(const Book& book); void dropCache(const std::string& bookId); diff --git a/src/library.cpp b/src/library.cpp index e5cc989cc..0e88867d3 100644 --- a/src/library.cpp +++ b/src/library.cpp @@ -192,6 +192,7 @@ void Library::cleanupBookCollection(BookIdCollection& books, const std::string& } std::string Library::getBestTargetBookId(const Bookmark& bookmark, MigrationMode migrationMode) const { + std::lock_guard lock(m_mutex); // Search for a existing book with the same name auto book_filter = Filter(); if (!bookmark.getBookName().empty()) { From 7a0ab3a429c5c239cd633845b47f44cf82aecea9 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Wed, 7 Feb 2024 17:28:55 +0100 Subject: [PATCH 14/18] Update tests to check book's title with double quotes (") On top of modifying the existing test, the commit also make `MigrateBookmark` test fails as `migrateBookmarks` now migrates from `wrong-book-id-noname` to `Dummy id`. Fix will be provided in next commit. --- test/library.cpp | 45 ++++++++++++++++++++++++++++++++++----------- 1 file changed, 34 insertions(+), 11 deletions(-) diff --git a/test/library.cpp b/test/library.cpp index d63a44865..6aa2b00a6 100644 --- a/test/library.cpp +++ b/test/library.cpp @@ -196,7 +196,7 @@ const char * sampleOpdsStream = R"( - TED talks - Business + TED"talks" - Business urn:uuid:0189d9be-2fd0-b4b6-7300-20fab0b5cdc8 ted_en_business nodet @@ -212,6 +212,23 @@ const char * sampleOpdsStream = R"( + + Business talks about TED + Dummy id + speak_business + nodet + /meta?name=favicon&content=ted_en_business_2018-07 + 2018-08-23T00:00::00:Z + eng + Ideas worth spreading + + + + TED + + + + Mythology & Folklore Stack Exchange urn:uuid:028055ac-4acc-1d54-65e0-a96de45e1b22 @@ -302,7 +319,7 @@ TEST(LibraryOpdsImportTest, allInOne) kiwix::Manager manager(lib); manager.readOpds(sampleOpdsStream, "library-opds-import.unittests.dev"); - EXPECT_EQ(13U, lib->getBookCount(true, true)); + EXPECT_EQ(14U, lib->getBookCount(true, true)); { const kiwix::Book& book1 = lib->getBookById("0c45160e-f917-760a-9159-dfe3c53cdcdd"); @@ -332,7 +349,7 @@ TEST(LibraryOpdsImportTest, allInOne) { const kiwix::Book& book2 = lib->getBookById("0189d9be-2fd0-b4b6-7300-20fab0b5cdc8"); - EXPECT_EQ(book2.getTitle(), "TED talks - Business"); + EXPECT_EQ(book2.getTitle(), "TED\"talks\" - Business"); EXPECT_EQ(book2.getName(), "ted_en_business"); EXPECT_EQ(book2.getFlavour(), "nodet"); EXPECT_EQ(book2.getLanguages(), Langs{ "eng" }); @@ -637,7 +654,7 @@ TEST_F(LibraryTest, MigrateBookmarkOlder) TEST_F(LibraryTest, sanityCheck) { - EXPECT_EQ(lib->getBookCount(true, true), 15U); + EXPECT_EQ(lib->getBookCount(true, true), 16U); EXPECT_EQ(lib->getBooksLanguages(), std::vector({"deu", "eng", "fra", "ita", "spa"}) ); @@ -689,6 +706,7 @@ TEST_F(LibraryTest, filterLocal) ); EXPECT_FILTER_RESULTS(kiwix::Filter().local(false), + "Business talks about TED", "Encyclopédie de la Tunisie", "Encyclopédie de la Tunisie", "Encyclopédie de la Tunisie", @@ -699,7 +717,7 @@ TEST_F(LibraryTest, filterLocal) "Mathématiques", "Movies & TV Stack Exchange", "Mythology & Folklore Stack Exchange", - "TED talks - Business", + "TED\"talks\" - Business", "Tania Louis", "Wikiquote" ); @@ -708,6 +726,7 @@ TEST_F(LibraryTest, filterLocal) TEST_F(LibraryTest, filterRemote) { EXPECT_FILTER_RESULTS(kiwix::Filter().remote(true), + "Business talks about TED", "Encyclopédie de la Tunisie", "Encyclopédie de la Tunisie", "Encyclopédie de la Tunisie", @@ -719,7 +738,7 @@ TEST_F(LibraryTest, filterRemote) "Movies & TV Stack Exchange", "Mythology & Folklore Stack Exchange", "Ray Charles", - "TED talks - Business", + "TED\"talks\" - Business", "Tania Louis", "Wikiquote" ); @@ -732,21 +751,23 @@ TEST_F(LibraryTest, filterRemote) TEST_F(LibraryTest, filterByLanguage) { EXPECT_FILTER_RESULTS(kiwix::Filter().lang("eng"), + "Business talks about TED", "Granblue Fantasy Wiki", "Islam Stack Exchange", "Movies & TV Stack Exchange", "Mythology & Folklore Stack Exchange", "Ray Charles", - "TED talks - Business" + "TED\"talks\" - Business" ); EXPECT_FILTER_RESULTS(kiwix::Filter().query("lang:eng"), + "Business talks about TED", "Granblue Fantasy Wiki", "Islam Stack Exchange", "Movies & TV Stack Exchange", "Mythology & Folklore Stack Exchange", "Ray Charles", - "TED talks - Business" + "TED\"talks\" - Business" ); EXPECT_FILTER_RESULTS(kiwix::Filter().query("eng"), @@ -893,6 +914,7 @@ TEST_F(LibraryTest, filteringByEmptyQueryReturnsAllEntries) { EXPECT_FILTER_RESULTS(kiwix::Filter().query(""), "An example ZIM archive", + "Business talks about TED", "Encyclopédie de la Tunisie", "Encyclopédie de la Tunisie", "Encyclopédie de la Tunisie", @@ -904,7 +926,7 @@ TEST_F(LibraryTest, filteringByEmptyQueryReturnsAllEntries) "Movies & TV Stack Exchange", "Mythology & Folklore Stack Exchange", "Ray Charles", - "TED talks - Business", + "TED\"talks\" - Business", "Tania Louis", "Wikiquote" ); @@ -1145,6 +1167,7 @@ TEST_F(LibraryTest, removeBooksNotUpdatedSince) { EXPECT_FILTER_RESULTS(kiwix::Filter(), "An example ZIM archive", + "Business talks about TED", "Encyclopédie de la Tunisie", "Encyclopédie de la Tunisie", "Encyclopédie de la Tunisie", @@ -1156,7 +1179,7 @@ TEST_F(LibraryTest, removeBooksNotUpdatedSince) "Movies & TV Stack Exchange", "Mythology & Folklore Stack Exchange", "Ray Charles", - "TED talks - Business", + "TED\"talks\" - Business", "Tania Louis", "Wikiquote" ); @@ -1170,7 +1193,7 @@ TEST_F(LibraryTest, removeBooksNotUpdatedSince) const uint64_t rev2 = lib->getRevision(); - EXPECT_EQ(12u, lib->removeBooksNotUpdatedSince(rev)); + EXPECT_EQ(13u, lib->removeBooksNotUpdatedSince(rev)); EXPECT_GT(lib->getRevision(), rev2); From 6efdc4396450c7e2ca66ac5d4ee14f0c9b72b7a4 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Wed, 7 Feb 2024 17:34:08 +0100 Subject: [PATCH 15/18] Correcly search for book's title with double quote ("). At indexation time, double quote are ignored, so a title as `TED "talks" - Business` is indexed as `ted talks business`. By removing the quotes, we ensure that our title "phrase" is not closed too early and we correctly search for `ted PHRASE talks PHRASE business` instead of `ted AND talks AND business`. --- src/library.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/library.cpp b/src/library.cpp index 0e88867d3..8facc89b2 100644 --- a/src/library.cpp +++ b/src/library.cpp @@ -191,6 +191,11 @@ void Library::cleanupBookCollection(BookIdCollection& books, const std::string& } } +std::string remove_quote(std::string input) { + std::replace(input.begin(), input.end(), '"', ' '); + return input; +} + std::string Library::getBestTargetBookId(const Bookmark& bookmark, MigrationMode migrationMode) const { std::lock_guard lock(m_mutex); // Search for a existing book with the same name @@ -204,7 +209,7 @@ std::string Library::getBestTargetBookId(const Bookmark& bookmark, MigrationMode // No bookName nor bookTitle, no way to find target book. return ""; } - book_filter.query("title:\""+bookmark.getBookTitle() + "\""); + book_filter.query("title:\"" + remove_quote(bookmark.getBookTitle()) + "\""); } auto targetBooks = filter(book_filter); cleanupBookCollection(targetBooks, bookmark.getBookId(), migrationMode); From eaca7010bc05b0b67618880c2c7c580de65a1c81 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Tue, 13 Feb 2024 16:33:53 +0100 Subject: [PATCH 16/18] Fix definition of `UPGRADE_ONLY` and `ALLOW_DOWNGRADE`. `MigrationMode` was kind of defined in the context of an internal mode used by `migrateBookmark(...)`. But now, with `getBestTargetBookId`, it is broken. This commit fix that and the associated implementation. Now `UPGRADE_ONLY` will make `getBestTargetBookId` return only newer books. and `ALLOW_DOWNGRADE` will return older books only if current book is invalid. --- include/library.h | 14 ++++++--- src/library.cpp | 76 ++++++++++++++++++++++++++++++--------------- test/library.cpp | 78 +++++++++++++++++++++++++++++++++++++---------- 3 files changed, 122 insertions(+), 46 deletions(-) diff --git a/include/library.h b/include/library.h index 354876456..9807d3984 100644 --- a/include/library.h +++ b/include/library.h @@ -56,14 +56,18 @@ enum supportedListMode { }; enum MigrationMode { - /** When migrating bookmarks, do not allow to migrate to an older book than the currently pointed one. + /** When migrating bookmarks, do not allow to migrate to an older book than the currently pointed one + * (or date stored in the bookmark if book is invalid) * - * This is enforce only if the bookmark point to a valid books. If bookmark point to a invalid book, - * it will be migrated to an existing one, even if older. + * If no newer books are found, no upgrade is made. */ UPGRADE_ONLY = 0, - /** Allow to migrate the bookmark to an older book. */ + /** Try hard to do a migration. This mostly does: + * - Try to find a newer book. + * - If book is invalid: find a best book, potentially older. + * Older book will never be returned if current book is a valid one. + */ ALLOW_DOWNGRADE = 1, }; @@ -466,7 +470,7 @@ private: // functions AttributeCounts getBookAttributeCounts(BookStrPropMemFn p) const; std::vector getBookPropValueSet(BookStrPropMemFn p) const; BookIdCollection filterViaBookDB(const Filter& filter) const; - void cleanupBookCollection(BookIdCollection& books, const std::string& sourceBookId, MigrationMode migrationMode) const; + std::string getBestFromBookCollection(BookIdCollection books, const Bookmark& bookmark, MigrationMode migrationMode) const; unsigned int getBookCount_not_protected(const bool localBooks, const bool remoteBooks) const; void updateBookDB(const Book& book); void dropCache(const std::string& bookId); diff --git a/src/library.cpp b/src/library.cpp index 8facc89b2..596556299 100644 --- a/src/library.cpp +++ b/src/library.cpp @@ -176,19 +176,53 @@ std::tuple Library::migrateBookmarks(MigrationMode migrationMode) { return std::make_tuple(changed, invalidBookmarks); } -void Library::cleanupBookCollection(BookIdCollection& books, const std::string& sourceBookId, MigrationMode migrationMode) const { - sort(books, DATE, false); +std::string Library::getBestFromBookCollection(BookIdCollection books, const Bookmark& bookmark, MigrationMode migrationMode) const { + // This function try to get the best book for a bookmark from a book collection. + // It assumes that all books in the collection are "acceptable". + // (this definiton is not clear but for now it is book's name is equal to bookmark's bookName) + // + // The algorithm first sort the colletion by "flavour equality" and date. + // "flavour equality" is if book's flavour is same that bookmark's flavour (let's say "flavourA" here) + // So we have the sorted collection: + // - flavourA, date 5 + // - flavourA, date 4 + // - flavourB, date 6 + // - flavourC, date 5 + // - flavourB, date 3 + // + // Then, depending of migrationMode: + // - If ALLOW_DOWNGRADE => take the first one + // - If UPGRADE_ONLY => loop on books until we find a book newer than bookmark. + // So if bookmark date is 5 => flavourB, date 6 + // if bookmark date is 4 => flavourA, date 5 + // if bookmark date is 7 => No book - // Remove source book - auto foundIT = std::find(books.begin(), books.end(), sourceBookId); - if (foundIT != books.end()) { - if (migrationMode == ALLOW_DOWNGRADE) { - books.erase(foundIT); - } else { - // books is sorted by date, remove the source and all older books - books.erase(foundIT, books.end()); + if (books.empty()) { + return ""; + } + + sort(books, DATE, false); + stable_sort(books.begin(), books.end(), [&](const std::string& bookId1, const std::string& bookId2) { + const auto& book1 = getBookById(bookId1); + const auto& book2 = getBookById(bookId2); + bool same_flavour1 = book1.getFlavour() == bookmark.getBookFlavour(); + bool same_flavour2 = book2.getFlavour() == bookmark.getBookFlavour(); + // return True if bookId1 is before bookId2, ie if same_flavour1 and not same_flavour2 + return same_flavour1 > same_flavour2; + }); + + if (migrationMode == ALLOW_DOWNGRADE) { + return books[0]; + } else { + for (const auto& bookId: books) { + const auto& book = getBookById(bookId); + if (book.getDate() >= bookmark.getDate()) { + return bookId; + } } } + + return ""; } std::string remove_quote(std::string input) { @@ -212,21 +246,14 @@ std::string Library::getBestTargetBookId(const Bookmark& bookmark, MigrationMode book_filter.query("title:\"" + remove_quote(bookmark.getBookTitle()) + "\""); } auto targetBooks = filter(book_filter); - cleanupBookCollection(targetBooks, bookmark.getBookId(), migrationMode); - - if (targetBooks.empty()) { - // No existing book found for the target, or the same than source. - return ""; + auto bestBook = getBestFromBookCollection(targetBooks, bookmark, migrationMode); + if (bestBook.empty()) { + try { + getBookById(bookmark.getBookId()); + return bookmark.getBookId(); + } catch (std::out_of_range&) {} } - if (targetBooks.size() != 1) { - // We have several, reduce to same flavour - auto flavouredTargetBooks = filter(book_filter.flavour(bookmark.getBookFlavour())); - cleanupBookCollection(flavouredTargetBooks, bookmark.getBookId(), migrationMode); - if (!flavouredTargetBooks.empty()) { - targetBooks = flavouredTargetBooks; - } - } - return targetBooks[0]; + return bestBook; } int Library::migrateBookmarks(const std::string& sourceBookId, MigrationMode migrationMode) { @@ -620,7 +647,6 @@ Xapian::Query flavourQuery(const std::string& name) return Xapian::Query("XF" + normalizeText(name)); } - Xapian::Query multipleParamQuery(const std::string& commaSeparatedList, const std::string& prefix) { Xapian::Query q; diff --git a/test/library.cpp b/test/library.cpp index 6aa2b00a6..2be7b5130 100644 --- a/test/library.cpp +++ b/test/library.cpp @@ -32,7 +32,7 @@ const char * sampleOpdsStream = R"( urn:uuid:0c45160e-f917-760a-9159-dfe3c53cdcdd /meta?name=favicon&content=wikipedia_fr_tunisie_novid_2018-10 2018-10-08T00:00::00:Z - 8 Oct 2018 + 2018-10-08T00:00::00:Z fra Le meilleur de Wikipédia sur la Tunisie wikipedia;novid;_ftindex @@ -54,7 +54,7 @@ const char * sampleOpdsStream = R"( novid urn:uuid:0c45160e-f917-760a-9159-dfe3c53cdcdd_updated1yearlater 2019-10-08T00:00::00:Z - 8 Oct 2019 + 2019-10-08T00:00::00:Z fra Le meilleur de Wikipédia sur la Tunisie. Updated in 2019 @@ -68,7 +68,7 @@ const char * sampleOpdsStream = R"( other_flavour urn:uuid:0c45160e-f917-760a-9159-dfe3c53cdcdd_flavour 2018-10-08T00:00::00:Z - 8 Oct 2018 + 2018-10-08T00:00::00:Z fra Le meilleur de Wikipédia sur la Tunisie. With another flavour @@ -82,7 +82,7 @@ const char * sampleOpdsStream = R"( other_flavour urn:uuid:0c45160e-f917-760a-9159-dfe3c53cdcdd_updated1yearlater_flavour 2019-10-08T00:00::00:Z - 8 Oct 2019 + 2019-10-08T00:00::00:Z fra Le meilleur de Wikipédia sur la Tunisie. Updated in 2019, and other flavour @@ -329,7 +329,7 @@ TEST(LibraryOpdsImportTest, allInOne) EXPECT_EQ(book1.getFlavour(), "novid"); EXPECT_EQ(book1.getLanguages(), Langs{ "fra" }); EXPECT_EQ(book1.getCommaSeparatedLanguages(), "fra"); - EXPECT_EQ(book1.getDate(), "8 Oct 2018"); + EXPECT_EQ(book1.getDate(), "2018-10-08"); EXPECT_EQ(book1.getDescription(), "Le meilleur de Wikipédia sur la Tunisie"); EXPECT_EQ(book1.getCreator(), "Wikipedia"); EXPECT_EQ(book1.getPublisher(), "Wikipedia Publishing House"); @@ -634,23 +634,69 @@ TEST_F(LibraryTest, MigrateBookmark) EXPECT_EQ(allBookmarks[5].getBookId(), bookId1+"_updated1yearlater_flavour"); } -TEST_F(LibraryTest, MigrateBookmarkOlder) +TEST_F(LibraryTest, GetBestTargetBookIdOlder) { - auto bookId1 = "0c45160e-f917-760a-9159-dfe3c53cdcdd_updated1yearlater"; + auto bookId = std::string("0c45160e-f917-760a-9159-dfe3c53cdcdd"); - auto book1 = lib->getBookById(bookId1); + auto book = lib->getBookById(bookId); - lib->addBookmark(createBookmark(book1)); + auto validBookmark = createBookmark(book); + lib->addBookmark(validBookmark); - auto onlyValidBookmarks = lib->getBookmarks(); - - ASSERT_EQ(onlyValidBookmarks.size(), 1); - EXPECT_EQ(onlyValidBookmarks[0].getBookId(), bookId1); - - ASSERT_EQ(lib->migrateBookmarks(bookId1), 0); - ASSERT_EQ(lib->migrateBookmarks(bookId1, kiwix::ALLOW_DOWNGRADE), 1); + ASSERT_EQ(lib->getBestTargetBookId(validBookmark, kiwix::UPGRADE_ONLY), bookId+"_updated1yearlater"); + ASSERT_EQ(lib->getBestTargetBookId(validBookmark, kiwix::ALLOW_DOWNGRADE), bookId+"_updated1yearlater"); } +TEST_F(LibraryTest, GetBestTargetBookIdNewer) +{ + auto bookId = std::string("0c45160e-f917-760a-9159-dfe3c53cdcdd_updated1yearlater"); + + auto book = lib->getBookById(bookId); + EXPECT_EQ(book.getDate(), "2019-10-08"); + + auto validBookmark = createBookmark(book); + // Make the bookmark more recent than any books in the library. + // (But still pointing to existing book) + validBookmark.setDate("2020-10-08"); + lib->addBookmark(validBookmark); + + // The best book for the bookmark is bookId... + ASSERT_EQ(lib->getBestTargetBookId(validBookmark, kiwix::UPGRADE_ONLY), bookId); + // but there is not migration to do as the bookmark already point to it. + ASSERT_EQ(lib->migrateBookmarks(bookId, kiwix::UPGRADE_ONLY), 0); + + ASSERT_EQ(lib->getBestTargetBookId(validBookmark, kiwix::ALLOW_DOWNGRADE), bookId); +} + +TEST_F(LibraryTest, GetBestTargetBookIdInvalidOlder) +{ + auto bookId = std::string("0c45160e-f917-760a-9159-dfe3c53cdcdd"); + + auto book = lib->getBookById(bookId); + + auto invalidBookmark = createBookmark(book); + invalidBookmark.setBookId("invalid-book-id"); + lib->addBookmark(invalidBookmark); + + ASSERT_EQ(lib->getBestTargetBookId(invalidBookmark, kiwix::UPGRADE_ONLY), bookId+"_updated1yearlater"); + ASSERT_EQ(lib->getBestTargetBookId(invalidBookmark, kiwix::ALLOW_DOWNGRADE), bookId+"_updated1yearlater"); +} + +TEST_F(LibraryTest, GetBestTargetBookIdInvalidNewer) +{ + auto bookId = std::string("0c45160e-f917-760a-9159-dfe3c53cdcdd"); + + auto book = lib->getBookById(bookId); + EXPECT_EQ(book.getDate(), "2018-10-08"); + + auto invalidBookmark = createBookmark(book); + invalidBookmark.setBookId("invalid-book-id"); + invalidBookmark.setDate("2020-10-08"); + lib->addBookmark(invalidBookmark); + + ASSERT_EQ(lib->getBestTargetBookId(invalidBookmark, kiwix::UPGRADE_ONLY), ""); + ASSERT_EQ(lib->getBestTargetBookId(invalidBookmark, kiwix::ALLOW_DOWNGRADE), bookId+"_updated1yearlater"); +} TEST_F(LibraryTest, sanityCheck) { From 73b855ce6bd30751d25e73d13f1c156335677987 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Thu, 15 Feb 2024 12:01:57 +0100 Subject: [PATCH 17/18] Add a `getBestTargetBookId` directly taking bookName, flavour and date. --- include/library.h | 14 ++++++++++++++ src/library.cpp | 16 ++++++++++++++++ test/library.cpp | 8 ++++++++ 3 files changed, 38 insertions(+) diff --git a/include/library.h b/include/library.h index 9807d3984..d7b187970 100644 --- a/include/library.h +++ b/include/library.h @@ -325,6 +325,20 @@ class Library: public std::enable_shared_from_this */ std::string getBestTargetBookId(const Bookmark& bookmark, MigrationMode migrationMode) const; + /** + * Get the best bookId for a combination of book's name, flavour and date. + * + * Given a bookName (mandatory), try to find the best book. + * If preferedFlavour is given, will try to find a book with the same flavour. If not found, return a book with a different flavour. + * If minDate is given, return a book newer than minDate. If not found, return a empty bookId. + * + * @param bookName The name of the book + * @param preferedFlavour The prefered flavour. + * @param minDate the minimal book date acceptable. Must be a string in the format "YYYY-MM-DD". + * @return A bookId corresponding to the query, or empty string if not found. + */ + std::string getBestTargetBookId(const std::string& bookName, const std::string& preferedFlavour="", const std::string& minDate="") const; + // XXX: This is a non-thread-safe operation const Book& getBookById(const std::string& id) const; // XXX: This is a non-thread-safe operation diff --git a/src/library.cpp b/src/library.cpp index 596556299..038052a36 100644 --- a/src/library.cpp +++ b/src/library.cpp @@ -230,6 +230,22 @@ std::string remove_quote(std::string input) { return input; } +std::string Library::getBestTargetBookId(const std::string& bookName, const std::string& preferedFlavour, const std::string& minDate) const { + // Let's reuse our algorithm based on bookmark. + MigrationMode migrationMode = UPGRADE_ONLY; + auto bookmark = Bookmark(); + bookmark.setBookName(bookName); + bookmark.setBookFlavour(preferedFlavour); + + if (minDate.empty()) { + migrationMode = ALLOW_DOWNGRADE; + } else { + bookmark.setDate(minDate); + } + + return getBestTargetBookId(bookmark, migrationMode); +} + std::string Library::getBestTargetBookId(const Bookmark& bookmark, MigrationMode migrationMode) const { std::lock_guard lock(m_mutex); // Search for a existing book with the same name diff --git a/test/library.cpp b/test/library.cpp index 2be7b5130..4f9c234a5 100644 --- a/test/library.cpp +++ b/test/library.cpp @@ -698,6 +698,14 @@ TEST_F(LibraryTest, GetBestTargetBookIdInvalidNewer) ASSERT_EQ(lib->getBestTargetBookId(invalidBookmark, kiwix::ALLOW_DOWNGRADE), bookId+"_updated1yearlater"); } +TEST_F(LibraryTest, GetBestTargetBookIdName) +{ + ASSERT_EQ(lib->getBestTargetBookId("wikipedia_fr_tunisie"), "0c45160e-f917-760a-9159-dfe3c53cdcdd_updated1yearlater"); + ASSERT_EQ(lib->getBestTargetBookId("wikipedia_fr_tunisie", "novid"), "0c45160e-f917-760a-9159-dfe3c53cdcdd_updated1yearlater"); + ASSERT_EQ(lib->getBestTargetBookId("wikipedia_fr_tunisie", "other_flavour"), "0c45160e-f917-760a-9159-dfe3c53cdcdd_updated1yearlater_flavour"); + ASSERT_EQ(lib->getBestTargetBookId("wikipedia_fr_tunisie", "other_flavour", "2020-12-12"), ""); +} + TEST_F(LibraryTest, sanityCheck) { EXPECT_EQ(lib->getBookCount(true, true), 16U); From 6b05eeb24b31cb9cc8543d96b5d1214b8de5bee7 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Thu, 15 Feb 2024 12:17:01 +0100 Subject: [PATCH 18/18] Add a small test on getBestTargetBookId and flavour. --- test/library.cpp | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/library.cpp b/test/library.cpp index 4f9c234a5..593c4ada9 100644 --- a/test/library.cpp +++ b/test/library.cpp @@ -698,6 +698,22 @@ TEST_F(LibraryTest, GetBestTargetBookIdInvalidNewer) ASSERT_EQ(lib->getBestTargetBookId(invalidBookmark, kiwix::ALLOW_DOWNGRADE), bookId+"_updated1yearlater"); } +TEST_F(LibraryTest, GetBestTargetBookIdFlavour) +{ + auto bookId = std::string("0c45160e-f917-760a-9159-dfe3c53cdcdd_flavour"); + + auto book = lib->getBookById(bookId); + EXPECT_EQ(book.getDate(), "2018-10-08"); + + auto invalidBookmark = createBookmark(book); + invalidBookmark.setBookId("invalid-book-id"); + invalidBookmark.setDate("2020-10-08"); + lib->addBookmark(invalidBookmark); + + ASSERT_EQ(lib->getBestTargetBookId(invalidBookmark, kiwix::UPGRADE_ONLY), ""); + ASSERT_EQ(lib->getBestTargetBookId(invalidBookmark, kiwix::ALLOW_DOWNGRADE), "0c45160e-f917-760a-9159-dfe3c53cdcdd_updated1yearlater_flavour"); +} + TEST_F(LibraryTest, GetBestTargetBookIdName) { ASSERT_EQ(lib->getBestTargetBookId("wikipedia_fr_tunisie"), "0c45160e-f917-760a-9159-dfe3c53cdcdd_updated1yearlater");