From 8d97686b81a32212f8d0ce10bb886e1f1d695ddc Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Fri, 19 Jan 2024 11:04:20 +0100 Subject: [PATCH] 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) {