From ccdc31621722d3b45135fe14d1fb389a5b341647 Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Fri, 9 Apr 2021 15:34:38 +0400 Subject: [PATCH 1/5] Two unit-tests for Library::removeBookById The `XmlLibraryTest.removeBookByIdDropsTheReader` unit-test fails, demonstrating a bug in `kiwix::Library::removeBookById()`. --- test/library.cpp | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/test/library.cpp b/test/library.cpp index 7f34e008b..beb6c2e6a 100644 --- a/test/library.cpp +++ b/test/library.cpp @@ -283,4 +283,31 @@ TEST_F(LibraryTest, getBookByPath) EXPECT_EQ(lib.getBookByPath(path).getId(), book.getId()); EXPECT_THROW(lib.getBookByPath("non/existant/path.zim"), std::out_of_range); } + +class XmlLibraryTest : public ::testing::Test { + protected: + void SetUp() override { + kiwix::Manager manager(&lib); + manager.readFile( "./test/library.xml", true, true); + } + + kiwix::Library lib; +}; + +TEST_F(XmlLibraryTest, removeBookByIdRemovesTheBook) +{ + EXPECT_EQ(3U, lib.getBookCount(true, true)); + EXPECT_NO_THROW(lib.getBookById("raycharles")); + lib.removeBookById("raycharles"); + EXPECT_EQ(2U, lib.getBookCount(true, true)); + EXPECT_THROW(lib.getBookById("raycharles"), std::out_of_range); +}; + +TEST_F(XmlLibraryTest, removeBookByIdDropsTheReader) +{ + EXPECT_NE(nullptr, lib.getReaderById("raycharles")); + lib.removeBookById("raycharles"); + EXPECT_THROW(lib.getReaderById("raycharles"), std::out_of_range); +}; + }; From 24ed96a38c1181f8a22ad3b9f2c6a2119b7d877e Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Fri, 9 Apr 2021 15:36:53 +0400 Subject: [PATCH 2/5] Library.removeBookById() drops the reader too This fix makes the `XmlLibraryTest.removeBookByIdDropsTheReader` unit-test pass. --- src/library.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/library.cpp b/src/library.cpp index 5c66ffb51..f088ace46 100644 --- a/src/library.cpp +++ b/src/library.cpp @@ -106,6 +106,7 @@ bool Library::removeBookmark(const std::string& zimId, const std::string& url) bool Library::removeBookById(const std::string& id) { + m_readers.erase(id); return m_books.erase(id) == 1; } From 49940a30d0b7a1dda6a39981a37232d1271b1c57 Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Fri, 9 Apr 2021 16:15:59 +0400 Subject: [PATCH 3/5] XmlLibraryTest.removeBookByIdUpdatesTheSearchDB The new unit-test fails with a reason not expected before it was written. The `Library::filter()` operation returns a correct result after the call to `removeBookById()` (this was a surprise!) but it has a side-effect of re-adding an empty book with the id still surviving in the search DB (the emptiness of this re-created book doesn't allow it to pass the other filtering criteria, which explains why the result of `Library::filter()` is correct). Had to add a special check to the new unit-test against that hidden side-effect of `Library::removeBookById()` + `Library::filter()` combination. --- test/library.cpp | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/library.cpp b/test/library.cpp index beb6c2e6a..c19514163 100644 --- a/test/library.cpp +++ b/test/library.cpp @@ -310,4 +310,22 @@ TEST_F(XmlLibraryTest, removeBookByIdDropsTheReader) EXPECT_THROW(lib.getReaderById("raycharles"), std::out_of_range); }; +TEST_F(XmlLibraryTest, removeBookByIdUpdatesTheSearchDB) +{ + kiwix::Filter f; + f.local(true).valid(true).query(R"(title:"ray charles")", false); + + EXPECT_NO_THROW(lib.getBookById("raycharles")); + EXPECT_EQ(1U, lib.filter(f).size()); + + lib.removeBookById("raycharles"); + + EXPECT_THROW(lib.getBookById("raycharles"), std::out_of_range); + EXPECT_EQ(0U, lib.filter(f).size()); + + // make sure that Library::filter() doesn't add an empty book with + // an id surviving in the search DB + EXPECT_THROW(lib.getBookById("raycharles"), std::out_of_range); +}; + }; From aaaa5a637eb04e5a4dc6a8cc98388ff50f9e08ef Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Fri, 9 Apr 2021 16:50:05 +0400 Subject: [PATCH 4/5] Library::filter() doesn't create empty books This changes how the `XmlLibraryTest.removeBookByIdUpdatesTheSearchDB` unit-test fails. --- src/library.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/library.cpp b/src/library.cpp index f088ace46..1ec844099 100644 --- a/src/library.cpp +++ b/src/library.cpp @@ -331,7 +331,7 @@ Library::BookIdCollection Library::filter(const Filter& filter) { BookIdCollection result; for(auto id : getBooksByTitleOrDescription(filter)) { - if(filter.acceptByNonQueryCriteria(m_books[id])) { + if(filter.acceptByNonQueryCriteria(m_books.at(id))) { result.push_back(id); } } From ec9186b17425ee8f6c70ba5cfa2df772e828939e Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Fri, 9 Apr 2021 16:55:39 +0400 Subject: [PATCH 5/5] Library::removeBookById() updates the search DB This fix makes the `XmlLibraryTest.removeBookByIdUpdatesTheSearchDB` unit-test pass. --- src/library.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/library.cpp b/src/library.cpp index 1ec844099..6a2bc5db8 100644 --- a/src/library.cpp +++ b/src/library.cpp @@ -106,6 +106,7 @@ bool Library::removeBookmark(const std::string& zimId, const std::string& url) bool Library::removeBookById(const std::string& id) { + m_bookDB->delete_document("Q" + id); m_readers.erase(id); return m_books.erase(id) == 1; }