Make the `Manager` keep a `shared_ptr` instead of a raw Library reference.

We want to be sure that `Library` actually exists when we modify it.
While it is not a silver bullet (user can still create a shared_ptr on
a raw pointer), making the `Manager` keep `shared_ptr` on the library
help us a lot here.

include/manager.h

@@ -37,10 +37,10 @@ namespace kiwix
 class LibraryManipulator
 {
  public: // functions
-  explicit LibraryManipulator(Library* library);
+  explicit LibraryManipulator(std::shared_ptr<Library> library);
   virtual ~LibraryManipulator();
 
-  Library& getLibrary() const { return library; }
+  std::shared_ptr<Library> getLibrary() const { return library; }
 
   bool addBookToLibrary(const Book& book);
   void addBookmarkToLibrary(const Bookmark& bookmark);
@@ -52,7 +52,7 @@ class LibraryManipulator
   virtual void booksWereRemovedFromLibrary();
 
  private: // data
-  kiwix::Library& library;
+  std::shared_ptr<kiwix::Library> library;
 };
 
 /**
@@ -64,8 +64,8 @@ class Manager
   typedef std::vector<std::string> Paths;
 
  public: // functions
-  explicit Manager(LibraryManipulator* manipulator);
-  explicit Manager(Library* library);
+  explicit Manager(LibraryManipulator manipulator);
+  explicit Manager(std::shared_ptr<Library> library);
 
   /**
    * Read a `library.xml` and add book in the file to the library.
@@ -163,7 +163,7 @@ class Manager
   uint64_t m_itemsPerPage = 0;
 
  protected:
-  std::shared_ptr<kiwix::LibraryManipulator> manipulator;
+  kiwix::LibraryManipulator manipulator;
 
   bool readBookFromPath(const std::string& path, Book* book);
   bool parseXmlDom(const pugi::xml_document& doc,

src/manager.cpp

@@ -27,22 +27,12 @@
 namespace kiwix
 {
 
-namespace
-{
-
-struct NoDelete
-{
-  template<class T> void operator()(T*) {}
-};
-
-} // unnamed namespace
-
 ////////////////////////////////////////////////////////////////////////////////
 // LibraryManipulator
 ////////////////////////////////////////////////////////////////////////////////
 
-LibraryManipulator::LibraryManipulator(Library* library)
-  : library(*library)
+LibraryManipulator::LibraryManipulator(std::shared_ptr<Library> library)
+  : library(library)
 {}
 
 LibraryManipulator::~LibraryManipulator()
@@ -50,7 +40,7 @@ LibraryManipulator::~LibraryManipulator()
 
 bool LibraryManipulator::addBookToLibrary(const Book& book)
 {
-  const auto ret = library.addBook(book);
+  const auto ret = library->addBook(book);
   if ( ret ) {
     bookWasAddedToLibrary(book);
   }
@@ -59,13 +49,13 @@ bool LibraryManipulator::addBookToLibrary(const Book& book)
 
 void LibraryManipulator::addBookmarkToLibrary(const Bookmark& bookmark)
 {
-  library.addBookmark(bookmark);
+  library->addBookmark(bookmark);
   bookmarkWasAddedToLibrary(bookmark);
 }
 
 uint32_t LibraryManipulator::removeBooksNotUpdatedSince(Library::Revision rev)
 {
-  const auto n = library.removeBooksNotUpdatedSince(rev);
+  const auto n = library->removeBooksNotUpdatedSince(rev);
   if ( n != 0 ) {
     booksWereRemovedFromLibrary();
   }
@@ -89,15 +79,15 @@ void LibraryManipulator::booksWereRemovedFromLibrary()
 ////////////////////////////////////////////////////////////////////////////////
 
 /* Constructor */
-Manager::Manager(LibraryManipulator* manipulator):
+Manager::Manager(LibraryManipulator manipulator):
   writableLibraryPath(""),
-  manipulator(manipulator, NoDelete())
+  manipulator(manipulator)
 {
 }
 
-Manager::Manager(Library* library) :
+Manager::Manager(std::shared_ptr<Library> library) :
   writableLibraryPath(""),
-  manipulator(new LibraryManipulator(library))
+  manipulator(LibraryManipulator(library))
 {
 }
 
@@ -121,7 +111,7 @@ bool Manager::parseXmlDom(const pugi::xml_document& doc,
     if (!trustLibrary && !book.getPath().empty()) {
       this->readBookFromPath(book.getPath(), &book);
     }
-    manipulator->addBookToLibrary(book);
+    manipulator.addBookToLibrary(book);
   }
 
   return true;
@@ -166,7 +156,7 @@ bool Manager::parseOpdsDom(const pugi::xml_document& doc, const std::string& url
     book.updateFromOpds(entryNode, urlHost);
 
     /* Update the book properties with the new importer */
-    manipulator->addBookToLibrary(book);
+    manipulator.addBookToLibrary(book);
   }
 
   return true;
@@ -241,7 +231,7 @@ std::string Manager::addBookFromPathAndGetId(const std::string& pathToOpen,
         || (!book.getTitle().empty() && !book.getLanguages().empty()
             && !book.getDate().empty())) {
       book.setUrl(url);
-      manipulator->addBookToLibrary(book);
+      manipulator.addBookToLibrary(book);
       return book.getId();
     }
   }
@@ -296,7 +286,7 @@ bool Manager::readBookmarkFile(const std::string& path)
 
     bookmark.updateFromXml(node);
 
-    manipulator->addBookmarkToLibrary(bookmark);
+    manipulator.addBookmarkToLibrary(bookmark);
   }
 
   return true;
@@ -304,7 +294,7 @@ bool Manager::readBookmarkFile(const std::string& path)
 
 void Manager::reload(const Paths& paths)
 {
-  const auto libRevision = manipulator->getLibrary().getRevision();
+  const auto libRevision = manipulator.getLibrary()->getRevision();
   for (std::string path : paths) {
     if (!path.empty()) {
       if ( kiwix::isRelativePath(path) )
@@ -316,7 +306,7 @@ void Manager::reload(const Paths& paths)
     }
   }
 
-  manipulator->removeBooksNotUpdatedSince(libRevision);
+  manipulator.removeBooksNotUpdatedSince(libRevision);
 }
 
 }

test/library.cpp

@@ -239,7 +239,7 @@ typedef std::vector<std::string> Langs;
 TEST(LibraryOpdsImportTest, allInOne)
 {
   auto lib = kiwix::Library::create();
-  kiwix::Manager manager(lib.get());
+  kiwix::Manager manager(lib);
   manager.readOpds(sampleOpdsStream, "library-opds-import.unittests.dev");
 
   EXPECT_EQ(10U, lib->getBookCount(true, true));
@@ -304,7 +304,7 @@ class LibraryTest : public ::testing::Test {
   LibraryTest(): lib(kiwix::Library::create()) {}
 
   void SetUp() override {
-     kiwix::Manager manager(lib.get());
+     kiwix::Manager manager(lib);
      manager.readOpds(sampleOpdsStream, "foo.urlHost");
      manager.readXml(sampleLibraryXML, false, "./test/library.xml", true);
   }

test/manager.cpp

@@ -9,7 +9,7 @@
 TEST(ManagerTest, addBookFromPathAndGetIdTest)
 {
     auto lib = kiwix::Library::create();
-    kiwix::Manager manager = kiwix::Manager(lib.get());
+    kiwix::Manager manager = kiwix::Manager(lib);
 
     auto bookId = manager.addBookFromPathAndGetId("./test/example.zim");
     ASSERT_NE(bookId, "");
@@ -49,7 +49,7 @@ const char sampleLibraryXML[] = R"(
 TEST(ManagerTest, readXml)
 {
     auto lib = kiwix::Library::create();
-    kiwix::Manager manager = kiwix::Manager(lib.get());
+    kiwix::Manager manager = kiwix::Manager(lib);
 
     EXPECT_EQ(true, manager.readXml(sampleLibraryXML, true, "/data/lib.xml", true));
     kiwix::Book book = lib->getBookById("0d0bcd57-d3f6-cb22-44cc-a723ccb4e1b2");
@@ -71,7 +71,7 @@ TEST(ManagerTest, readXml)
 TEST(Manager, reload)
 {
   auto lib = kiwix::Library::create();
-  kiwix::Manager manager(lib.get());
+  kiwix::Manager manager(lib);
 
   manager.reload({ "./test/library.xml" });
   EXPECT_EQ(lib->getBooksIds(), (kiwix::Library::BookIdCollection{

test/name_mapper.cpp

@@ -22,7 +22,7 @@ class NameMapperTest : public ::testing::Test {
   NameMapperTest(): lib(kiwix::Library::create()) {}
  protected:
   void SetUp() override {
-     kiwix::Manager manager(lib.get());
+     kiwix::Manager manager(lib);
      manager.readXml(libraryXML, false, "./library.xml", true);
      for ( const std::string& id : lib->getBooksIds() ) {
        kiwix::Book bookCopy = lib->getBookById(id);

test/server_testing_tools.h

@@ -108,7 +108,7 @@ private: // data
 
 ZimFileServer::ZimFileServer(int serverPort, Cfg _cfg, std::string libraryFilePath)
 : library(kiwix::Library::create())
-, manager(this->library.get())
+, manager(this->library)
 , cfg(_cfg)
 {
   if ( kiwix::isRelativePath(libraryFilePath) )
@@ -122,7 +122,7 @@ ZimFileServer::ZimFileServer(int serverPort,
                              const FilePathCollection& zimpaths,
                              std::string indexTemplateString)
 : library(kiwix::Library::create())
-, manager(this->library.get())
+, manager(this->library)
 , cfg(_cfg)
 {
   for ( const auto& zimpath : zimpaths ) {