Introduce `migrateBookmarks` to move (invalid) bookmarks to new books.

include/library.h

@@ -255,7 +255,7 @@ class Library: public std::enable_shared_from_this<Library>
   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<Library>
    */
   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<int, int>: <The number of bookmarks updated>, <Number of invalid bookmarks before migration was performed>.
+   */
+  std::tuple<int, int> 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<std::string> 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);

src/library.cpp

@@ -157,6 +157,94 @@ bool Library::removeBookmark(const std::string& zimId, const std::string& url)
   return false;
 }
 
+std::tuple<int, int> Library::migrateBookmarks() {
+  std::set<std::string> sourceBooks;
+  int invalidBookmarks = 0;
+  {
+    std::lock_guard<std::recursive_mutex> 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<std::recursive_mutex> 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)
 {

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)
 {