Check serialized temporary files memory is non-empty

libdnf5/base/transaction.cpp

@@ -882,7 +882,7 @@ Transaction::TransactionRunResult Transaction::Impl::_run(
         auto keepcache = config.get_keepcache_option().get_value();
         auto any_inbound_action_present = contains_any_inbound_package(packages);
         if (!keepcache && any_inbound_action_present) {
-            libdnf5::repo::TempFilesMemory temp_files_memory(config.get_cachedir_option().get_value());
+            libdnf5::repo::TempFilesMemory temp_files_memory(base, config.get_cachedir_option().get_value());
             auto temp_files = temp_files_memory.get_files();
             for (auto & file : temp_files) {
                 try {

libdnf5/repo/package_downloader.cpp

@@ -209,7 +209,7 @@ void PackageDownloader::download() try {
             });
 
         auto & cachedir = config.get_cachedir_option().get_value();
-        TempFilesMemory temp_files_memory(cachedir);
+        TempFilesMemory temp_files_memory(p_impl->base, cachedir);
         temp_files_memory.add_files(package_paths);
     }
 

libdnf5/repo/temp_files_memory.cpp

@@ -20,7 +20,9 @@ along with libdnf.  If not, see <https://www.gnu.org/licenses/>.
 #include "temp_files_memory.hpp"
 
 #include "utils/fs/file.hpp"
+#include "utils/string.hpp"
 
+#include "libdnf5/base/base.hpp"
 #include "libdnf5/common/exception.hpp"
 #include "libdnf5/utils/bgettext/bgettext-mark-domain.h"
 
@@ -29,8 +31,9 @@ along with libdnf.  If not, see <https://www.gnu.org/licenses/>.
 
 namespace libdnf5::repo {
 
-TempFilesMemory::TempFilesMemory(const std::string & parent_dir)
-    : full_memory_path(std::filesystem::path(parent_dir) / MEMORY_FILENAME) {
+TempFilesMemory::TempFilesMemory(const BaseWeakPtr & base, const std::string & parent_dir)
+    : base(base),
+      full_memory_path(std::filesystem::path(parent_dir) / MEMORY_FILENAME) {
     std::filesystem::create_directories(parent_dir);
 }
 
@@ -62,7 +65,17 @@ void TempFilesMemory::add_files(const std::vector<std::string> & paths) {
 
     // write new contents to a temporary file and then move the new file atomically
     auto temporary_path = full_memory_path.string() + ".temp";
-    utils::fs::File(temporary_path, "w").write(toml::format(toml::value({{FILE_PATHS_TOML_KEY, files}})));
+    auto new_data = toml::format(toml::value({{FILE_PATHS_TOML_KEY, files}}));
+    // Although it's not clear for the documentation if it is possible,
+    // it occurred, that the file was empty, which results in parsing error,
+    // see https://github.com/rpm-software-management/dnf5/issues/1001
+    if (new_data.empty()) {
+        auto & logger = *base->get_logger();
+        logger.error(
+            "Serializing temporary file paths failed for the input vector: \"{}\"", utils::string::join(files, ", "));
+        return;
+    }
+    utils::fs::File(temporary_path, "w").write(new_data);
     std::filesystem::rename(temporary_path, full_memory_path);
 }
 

libdnf5/repo/temp_files_memory.hpp

@@ -20,6 +20,8 @@ along with libdnf.  If not, see <https://www.gnu.org/licenses/>.
 #ifndef LIBDNF5_REPO_TEMP_FILES_MEMORY_HPP
 #define LIBDNF5_REPO_TEMP_FILES_MEMORY_HPP
 
+#include "libdnf5/base/base_weak.hpp"
+
 #include <filesystem>
 #include <vector>
 
@@ -38,10 +40,11 @@ class TempFilesMemory {
     static constexpr const char * FILE_PATHS_TOML_KEY = "files";
 
     /// @brief Create the object for managing temporary files' paths.
+    /// @param base       A weak pointer to Base object.
     /// @param parent_dir Path to a directory where the memory file is or will be stored.
     ///                   If the directory doesn't exist yet, it will be created.
     /// @exception std::filesystem::filesystem_error When an error occurs during creating the parent directory.
-    TempFilesMemory(const std::string & parent_dir);
+    TempFilesMemory(const BaseWeakPtr & base, const std::string & parent_dir);
     ~TempFilesMemory();
 
     /// @brief Retrieve stored paths of temporary files.
@@ -64,6 +67,7 @@ class TempFilesMemory {
     void clear();
 
 private:
+    BaseWeakPtr base;
     std::filesystem::path full_memory_path;
 };
 

test/libdnf5/repo/test_package_downloader.cpp

@@ -107,7 +107,7 @@ void PackageDownloaderTest::test_package_downloader_temp_files_memory() {
     config.get_keepcache_option().set(false);
 
     auto & cachedir = config.get_cachedir_option().get_value();
-    libdnf5::repo::TempFilesMemory memory(cachedir);
+    libdnf5::repo::TempFilesMemory memory(base.get_weak_ptr(), cachedir);
 
     // check memory is empty before downloading
     CPPUNIT_ASSERT_EQUAL((size_t)0, memory.get_files().size());

test/libdnf5/repo/test_temp_files_memory.cpp

@@ -32,36 +32,30 @@ using namespace libdnf5::repo;
 
 
 void TempFilesMemoryTest::setUp() {
-    CppUnit::TestCase::setUp();
-    temp_dir = std::make_unique<libdnf5::utils::fs::TempDir>("libdnf_test_filesmemory");
-    parent_dir_path = temp_dir->get_path();
+    BaseTestCase::setUp();
+    parent_dir_path = temp->get_path();
     full_path = parent_dir_path / TempFilesMemory::MEMORY_FILENAME;
 }
 
-void TempFilesMemoryTest::tearDown() {
-    temp_dir.reset();
-    CppUnit::TestCase::tearDown();
-}
-
 void TempFilesMemoryTest::test_directory_is_created_when_not_exists() {
-    auto test_path = temp_dir->get_path() / "some/new/path";
+    auto test_path = parent_dir_path / "some/new/path";
     CPPUNIT_ASSERT(!std::filesystem::exists(test_path));
-    TempFilesMemory memory(test_path);
+    TempFilesMemory memory(base.get_weak_ptr(), test_path);
     CPPUNIT_ASSERT(std::filesystem::exists(test_path));
 }
 
 void TempFilesMemoryTest::test_get_files_when_empty_storage() {
-    TempFilesMemory memory(temp_dir->get_path() / "unknown/path");
+    TempFilesMemory memory(base.get_weak_ptr(), parent_dir_path / "unknown/path");
     CPPUNIT_ASSERT(memory.get_files().empty());
 }
 
 void TempFilesMemoryTest::test_get_files_throws_exception_when_invalid_format() {
     libdnf5::utils::fs::File(full_path, "w").write("");
-    TempFilesMemory memory_empty(parent_dir_path);
+    TempFilesMemory memory_empty(base.get_weak_ptr(), parent_dir_path);
     CPPUNIT_ASSERT_THROW(memory_empty.get_files(), libdnf5::Error);
 
     libdnf5::utils::fs::File(full_path, "w").write("[\"path1\", \"path2\", \"path3\"]");
-    TempFilesMemory memory_invalid(parent_dir_path);
+    TempFilesMemory memory_invalid(base.get_weak_ptr(), parent_dir_path);
     CPPUNIT_ASSERT_THROW(memory_invalid.get_files(), libdnf5::Error);
 }
 
@@ -70,27 +64,38 @@ void TempFilesMemoryTest::test_get_files_returns_stored_values() {
         .write(fmt::format(
             "{} = [\"path/to/package1.rpm\", \"different/path/to/package2.rpm\"]",
             TempFilesMemory::FILE_PATHS_TOML_KEY));
-    TempFilesMemory memory(parent_dir_path);
+    TempFilesMemory memory(base.get_weak_ptr(), parent_dir_path);
     std::vector<std::string> expected = {"path/to/package1.rpm", "different/path/to/package2.rpm"};
     CPPUNIT_ASSERT_EQUAL(expected, memory.get_files());
 }
 
 void TempFilesMemoryTest::test_add_files_when_empty_storage() {
     std::vector<std::string> new_paths = {"path1", "path2"};
 
-    TempFilesMemory memory(parent_dir_path);
+    TempFilesMemory memory(base.get_weak_ptr(), parent_dir_path);
     CPPUNIT_ASSERT(memory.get_files().empty());
 
     memory.add_files(new_paths);
     CPPUNIT_ASSERT_EQUAL(new_paths, memory.get_files());
 }
 
+void TempFilesMemoryTest::test_add_no_files_when_empty_storage() {
+    // make sure that adding an empty vector will not cause crashing
+
+    TempFilesMemory memory(base.get_weak_ptr(), parent_dir_path);
+    CPPUNIT_ASSERT(memory.get_files().empty());
+
+    std::vector<std::string> empty_paths = {};
+    memory.add_files(empty_paths);
+    CPPUNIT_ASSERT(memory.get_files().empty());
+}
+
 void TempFilesMemoryTest::test_add_files_when_existing_storage() {
     std::vector<std::string> new_paths = {"path3", "path4"};
     libdnf5::utils::fs::File(full_path, "w")
         .write(fmt::format("{} = [\"path1\", \"path2\"]", TempFilesMemory::FILE_PATHS_TOML_KEY));
 
-    TempFilesMemory memory(parent_dir_path);
+    TempFilesMemory memory(base.get_weak_ptr(), parent_dir_path);
     memory.add_files(new_paths);
     std::vector<std::string> expected = {"path1", "path2", "path3", "path4"};
     CPPUNIT_ASSERT_EQUAL(expected, memory.get_files());
@@ -101,7 +106,7 @@ void TempFilesMemoryTest::test_add_files_deduplicates_and_sorts_data() {
     libdnf5::utils::fs::File(full_path, "w")
         .write(fmt::format("{} = [\"path4\", \"path1\", \"path4\", \"path3\"]", TempFilesMemory::FILE_PATHS_TOML_KEY));
 
-    TempFilesMemory memory(parent_dir_path);
+    TempFilesMemory memory(base.get_weak_ptr(), parent_dir_path);
     memory.add_files(new_paths);
     std::vector<std::string> expected = {"path1", "path2", "path3", "path4"};
     CPPUNIT_ASSERT_EQUAL(expected, memory.get_files());
@@ -113,7 +118,7 @@ void TempFilesMemoryTest::test_clear_deletes_storage_content() {
             "{} = [\"/path/to/package1.rpm\", \"/different-path/to/package2.rpm\", "
             "\"/another-path/leading/to/pkg3.rpm\"]",
             TempFilesMemory::FILE_PATHS_TOML_KEY));
-    TempFilesMemory memory(parent_dir_path);
+    TempFilesMemory memory(base.get_weak_ptr(), parent_dir_path);
     std::vector<std::string> expected = {
         "/path/to/package1.rpm", "/different-path/to/package2.rpm", "/another-path/leading/to/pkg3.rpm"};
     CPPUNIT_ASSERT_EQUAL(expected, memory.get_files());
@@ -123,6 +128,6 @@ void TempFilesMemoryTest::test_clear_deletes_storage_content() {
 }
 
 void TempFilesMemoryTest::test_clear_when_empty_storage() {
-    TempFilesMemory memory(temp_dir->get_path() / "non-existing/path");
+    TempFilesMemory memory(base.get_weak_ptr(), parent_dir_path / "non-existing/path");
     memory.clear();
 }

test/libdnf5/repo/test_temp_files_memory.hpp

@@ -20,21 +20,21 @@ along with libdnf.  If not, see <https://www.gnu.org/licenses/>.
 #ifndef LIBDNF5_TEST_REPO_TEMP_FILES_MEMORY_HPP
 #define LIBDNF5_TEST_REPO_TEMP_FILES_MEMORY_HPP
 
-#include "utils/fs/temp.hpp"
+#include "../shared/base_test_case.hpp"
 
-#include <cppunit/TestCase.h>
 #include <cppunit/extensions/HelperMacros.h>
 
 #include <filesystem>
 
 
-class TempFilesMemoryTest : public CppUnit::TestCase {
+class TempFilesMemoryTest : public BaseTestCase {
     CPPUNIT_TEST_SUITE(TempFilesMemoryTest);
     CPPUNIT_TEST(test_directory_is_created_when_not_exists);
     CPPUNIT_TEST(test_get_files_when_empty_storage);
     CPPUNIT_TEST(test_get_files_throws_exception_when_invalid_format);
     CPPUNIT_TEST(test_get_files_returns_stored_values);
     CPPUNIT_TEST(test_add_files_when_empty_storage);
+    CPPUNIT_TEST(test_add_no_files_when_empty_storage);
     CPPUNIT_TEST(test_add_files_when_existing_storage);
     CPPUNIT_TEST(test_add_files_deduplicates_and_sorts_data);
     CPPUNIT_TEST(test_clear_deletes_storage_content);
@@ -43,20 +43,19 @@ class TempFilesMemoryTest : public CppUnit::TestCase {
 
 public:
     void setUp() override;
-    void tearDown() override;
 
     void test_directory_is_created_when_not_exists();
     void test_get_files_when_empty_storage();
     void test_get_files_throws_exception_when_invalid_format();
     void test_get_files_returns_stored_values();
     void test_add_files_when_empty_storage();
+    void test_add_no_files_when_empty_storage();
     void test_add_files_when_existing_storage();
     void test_add_files_deduplicates_and_sorts_data();
     void test_clear_deletes_storage_content();
     void test_clear_when_empty_storage();
 
 private:
-    std::unique_ptr<libdnf5::utils::fs::TempDir> temp_dir;
     std::filesystem::path parent_dir_path;
     std::filesystem::path full_path;
 };