Skip to content

Commit

Permalink
Fix definition of UPGRADE_ONLY and ALLOW_DOWNGRADE.
Browse files Browse the repository at this point in the history 
`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.
  • Loading branch information
@mgautierfr
mgautierfr committed Feb 14, 2024
1 parent 32deb29 commit 39134f6
Show file tree
Hide file tree
Showing 3 changed files with 94 additions and 35 deletions.
14 changes: 9 additions & 5 deletions include/library.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};

Expand Down Expand Up @@ -466,7 +470,7 @@ class Library: public std::enable_shared_from_this<Library>
AttributeCounts getBookAttributeCounts(BookStrPropMemFn p) const;
std::vector<std::string> getBookPropValueSet(BookStrPropMemFn p) const;
BookIdCollection filterViaBookDB(const Filter& filter) const;
void cleanupBookCollection(BookIdCollection& books, const std::string& sourceBookId, MigrationMode migrationMode) const;
void removeOlderBooksFromBookCollection(BookIdCollection& books, 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);
Expand Down
43 changes: 27 additions & 16 deletions src/library.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,18 +176,17 @@ std::tuple<int, int> Library::migrateBookmarks() {
return std::make_tuple(changed, invalidBookmarks);
}

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());
}
void Library::removeOlderBooksFromBookCollection(BookIdCollection& books, const Bookmark& bookmark) const {
auto foundIt = std::find(books.begin(), books.end(), bookmark.getBookId());
if (foundIt == books.end()) {
foundIt = std::find_if(books.begin(), books.end(), [&](const std::string& bookId) {
auto& book = getBookById(bookId);
return book.getDate() <= bookmark.getDate();
});
}
if (foundIt != books.end()) {
// books is sorted by date, remove the source and all older books
books.erase(foundIt, books.end());
}
}

Expand All @@ -212,16 +211,28 @@ 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);
sort(targetBooks, DATE, false);
if (migrationMode == UPGRADE_ONLY) {
removeOlderBooksFromBookCollection(targetBooks, bookmark);
}

if (targetBooks.empty()) {
// No existing book found for the target, or the same than source.
return "";
// No better book to migrate to.
// If bookmark is valid, best is the current one.
try {
getBookById(bookmark.getBookId());
return bookmark.getBookId();
} catch (std::out_of_range& e) {
return "";
}
}
if (targetBooks.size() != 1) {
// We have several, reduce to same flavour
auto flavouredTargetBooks = filter(book_filter.flavour(bookmark.getBookFlavour()));
cleanupBookCollection(flavouredTargetBooks, bookmark.getBookId(), migrationMode);
sort(flavouredTargetBooks, DATE, false);
if (migrationMode == UPGRADE_ONLY) {
removeOlderBooksFromBookCollection(flavouredTargetBooks, bookmark);
}
if (!flavouredTargetBooks.empty()) {
targetBooks = flavouredTargetBooks;
}
Expand Down
72 changes: 58 additions & 14 deletions test/library.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ const char * sampleOpdsStream = R"(
<id>urn:uuid:0c45160e-f917-760a-9159-dfe3c53cdcdd</id>
<icon>/meta?name=favicon&amp;content=wikipedia_fr_tunisie_novid_2018-10</icon>
<updated>2018-10-08T00:00::00:Z</updated>
<dc:issued>8 Oct 2018</dc:issued>
<dc:issued>2018-10-08T00:00::00:Z</dc:issued>
<language>fra</language>
<summary>Le meilleur de Wikipédia sur la Tunisie</summary>
<tags>wikipedia;novid;_ftindex</tags>
Expand All @@ -54,7 +54,7 @@ const char * sampleOpdsStream = R"(
<flavour>novid</flavour>
<id>urn:uuid:0c45160e-f917-760a-9159-dfe3c53cdcdd_date</id>
<updated>2019-10-08T00:00::00:Z</updated>
<dc:issued>8 Oct 2019</dc:issued>
<dc:issued>2019-10-08T00:00::00:Z</dc:issued>
<language>fra</language>
<summary>Le meilleur de Wikipédia sur la Tunisie. Updated in 2019</summary>
<author>
Expand All @@ -68,7 +68,7 @@ const char * sampleOpdsStream = R"(
<flavour>other_flavour</flavour>
<id>urn:uuid:0c45160e-f917-760a-9159-dfe3c53cdcdd_flavour</id>
<updated>2018-10-08T00:00::00:Z</updated>
<dc:issued>8 Oct 2018</dc:issued>
<dc:issued>2018-10-08T00:00::00:Z</dc:issued>
<language>fra</language>
<summary>Le meilleur de Wikipédia sur la Tunisie. With another flavour</summary>
<author>
Expand All @@ -82,7 +82,7 @@ const char * sampleOpdsStream = R"(
<flavour>other_flavour</flavour>
<id>urn:uuid:0c45160e-f917-760a-9159-dfe3c53cdcdd_date_flavour</id>
<updated>2019-10-08T00:00::00:Z</updated>
<dc:issued>8 Oct 2019</dc:issued>
<dc:issued>2019-10-08T00:00::00:Z</dc:issued>
<language>fra</language>
<summary>Le meilleur de Wikipédia sur la Tunisie. Updated in 2019, and other flavour</summary>
<author>
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -634,23 +634,67 @@ TEST_F(LibraryTest, MigrateBookmark)
EXPECT_EQ(allBookmarks[5].getBookId(), bookId1+"_date_flavour");
}

TEST_F(LibraryTest, MigrateBookmarkOlder)
TEST_F(LibraryTest, GetBestTargetBookIdOlder)
{
auto bookId1 = "0c45160e-f917-760a-9159-dfe3c53cdcdd_date";
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(lib->getBestTargetBookId(validBookmark, kiwix::UPGRADE_ONLY), bookId+"_date");
ASSERT_EQ(lib->getBestTargetBookId(validBookmark, kiwix::ALLOW_DOWNGRADE), bookId+"_date");
}

ASSERT_EQ(onlyValidBookmarks.size(), 1);
EXPECT_EQ(onlyValidBookmarks[0].getBookId(), bookId1);
TEST_F(LibraryTest, GetBestTargetBookIdNewer)
{
auto bookId = std::string("0c45160e-f917-760a-9159-dfe3c53cdcdd_date");

auto book = lib->getBookById(bookId);
EXPECT_EQ(book.getDate(), "2019-10-08");

auto validBookmark = createBookmark(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->migrateBookmarks(bookId1), 0);
ASSERT_EQ(lib->migrateBookmarks(bookId1, kiwix::ALLOW_DOWNGRADE), 1);
ASSERT_EQ(lib->getBestTargetBookId(invalidBookmark, kiwix::UPGRADE_ONLY), bookId+"_date");
ASSERT_EQ(lib->getBestTargetBookId(invalidBookmark, kiwix::ALLOW_DOWNGRADE), bookId+"_date");
}

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+"_date");
}

TEST_F(LibraryTest, sanityCheck)
{
Expand Down

0 comments on commit 39134f6

Please sign in to comment.