From eaca7010bc05b0b67618880c2c7c580de65a1c81 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Tue, 13 Feb 2024 16:33:53 +0100 Subject: [PATCH] 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) {