Cleanup files uploaded on request deletion (#266)

LeSpocky · web-flow · commit acc3e6a3ad0b · 2022-04-12T13:33:36.000-07:00
* http_request: Remove uploaded files in destructor If the user needs those files, she should copy/move theme in the request handler. References: #264 Signed-off-by: Alexander Dahl <ada@thorsis.com> * test: integ: file_upload: Remove redundant file delete Not needed anymore. http_request destructor does cleanup now. * test: integ: file_upload: Check uploaded files are deleted * test: integ: file_upload: Stop webserver earlier The webserver runs in a different thread than the test, therefor deleting the uploaded files and checking if they are deleted might lead into a race condition, and thus into tests failing sometimes, but not always. Moving the webserver stop and destructor behind the curl action should be safe, every information needed for the test is copied to or in the resource handler.
diff --git a/src/http_request.cpp b/src/http_request.cpp
@@ -256,4 +256,13 @@ std::ostream &operator<< (std::ostream &os, const http_request &r) {
     return os;
 }
 
+http_request::~http_request() {
+    for ( const auto &file_key : this->get_files() ) {
+        for ( const auto &files : file_key.second ) {
+            // C++17 has std::filesystem::remove()
+            remove(files.second.get_file_system_file_name().c_str());
+        }
+    }
+}
+
 }  // namespace httpserver
diff --git a/src/httpserver/http_request.hpp b/src/httpserver/http_request.hpp
@@ -46,6 +46,8 @@ struct MHD_Connection;
 
 namespace httpserver {
 
+namespace details { struct modded_request; }
+
 /**
  * Class representing an abstraction for an Http Request. It is used from classes using these apis to receive information through http protocol.
 **/
@@ -254,6 +256,8 @@ class http_request {
      http_request& operator=(const http_request& b) = default;
      http_request& operator=(http_request&& b) = default;
 
+     ~http_request();
+
      std::string path;
      std::string method;
      std::map<std::string, std::string, http::arg_comparator> args;
@@ -356,6 +360,7 @@ class http_request {
      const std::map<std::string, std::string, http::header_comparator> get_headerlike_values(enum MHD_ValueKind kind) const;
 
      friend class webserver;
+     friend struct details::modded_request;
 };
 
 std::ostream &operator<< (std::ostream &os, const http_request &r);
diff --git a/test/integ/file_upload.cpp b/test/integ/file_upload.cpp
@@ -59,6 +59,12 @@ static size_t TEST_CONTENT_SIZE_2 = 28;
 static const char* TEST_PARAM_KEY = "param_key";
 static const char* TEST_PARAM_VALUE = "Value of test param";
 
+static bool file_exists(const string &path) {
+    struct stat sb;
+
+    return (stat(path.c_str(), &sb) == 0);
+}
+
 static CURLcode send_file_to_webserver(bool add_second_file, bool append_parameters) {
     curl_global_init(CURL_GLOBAL_ALL);
 
@@ -191,6 +197,9 @@ LT_BEGIN_AUTO_TEST(file_upload_suite, file_upload_memory_and_disk)
     CURLcode res = send_file_to_webserver(false, false);
     LT_ASSERT_EQ(res, 0);
 
+    ws->stop();
+    delete ws;
+
     string actual_content = resource.get_content();
     LT_CHECK_EQ(actual_content.find(FILENAME_IN_GET_CONTENT) != string::npos, true);
     LT_CHECK_EQ(actual_content.find(TEST_CONTENT) != string::npos, true);
@@ -216,10 +225,7 @@ LT_BEGIN_AUTO_TEST(file_upload_suite, file_upload_memory_and_disk)
                                httpserver::http::http_utils::upload_filename_template;
     LT_CHECK_EQ(file->second.get_file_system_file_name().substr(0, file->second.get_file_system_file_name().size() - 6),
                 expected_filename.substr(0, expected_filename.size() - 6));
-    unlink(file->second.get_file_system_file_name().c_str());
-
-    ws->stop();
-    delete ws;
+    LT_CHECK_EQ(file_exists(file->second.get_file_system_file_name()), false);
 LT_END_AUTO_TEST(file_upload_memory_and_disk)
 
 LT_BEGIN_AUTO_TEST(file_upload_suite, file_upload_memory_and_disk_via_put)
@@ -271,6 +277,9 @@ LT_BEGIN_AUTO_TEST(file_upload_suite, file_upload_memory_and_disk_additional_par
     CURLcode res = send_file_to_webserver(false, true);
     LT_ASSERT_EQ(res, 0);
 
+    ws->stop();
+    delete ws;
+
     string actual_content = resource.get_content();
     LT_CHECK_EQ(actual_content.find(FILENAME_IN_GET_CONTENT) != string::npos, true);
     LT_CHECK_EQ(actual_content.find(TEST_CONTENT) != string::npos, true);
@@ -301,10 +310,7 @@ LT_BEGIN_AUTO_TEST(file_upload_suite, file_upload_memory_and_disk_additional_par
                                httpserver::http::http_utils::upload_filename_template;
     LT_CHECK_EQ(file->second.get_file_system_file_name().substr(0, file->second.get_file_system_file_name().size() - 6),
                 expected_filename.substr(0, expected_filename.size() - 6));
-    unlink(file->second.get_file_system_file_name().c_str());
-
-    ws->stop();
-    delete ws;
+    LT_CHECK_EQ(file_exists(file->second.get_file_system_file_name()), false);
 LT_END_AUTO_TEST(file_upload_memory_and_disk_additional_params)
 
 LT_BEGIN_AUTO_TEST(file_upload_suite, file_upload_memory_and_disk_two_files)
@@ -325,6 +331,9 @@ LT_BEGIN_AUTO_TEST(file_upload_suite, file_upload_memory_and_disk_two_files)
     CURLcode res = send_file_to_webserver(true, false);
     LT_ASSERT_EQ(res, 0);
 
+    ws->stop();
+    delete ws;
+
     string actual_content = resource.get_content();
     LT_CHECK_EQ(actual_content.find(FILENAME_IN_GET_CONTENT) != string::npos, true);
     LT_CHECK_EQ(actual_content.find(TEST_CONTENT) != string::npos, true);
@@ -355,7 +364,7 @@ LT_BEGIN_AUTO_TEST(file_upload_suite, file_upload_memory_and_disk_two_files)
                                httpserver::http::http_utils::upload_filename_template;
     LT_CHECK_EQ(file->second.get_file_system_file_name().substr(0, file->second.get_file_system_file_name().size() - 6),
                 expected_filename.substr(0, expected_filename.size() - 6));
-    unlink(file->second.get_file_system_file_name().c_str());
+    LT_CHECK_EQ(file_exists(file->second.get_file_system_file_name()), false);
 
     file_key++;
     LT_CHECK_EQ(file_key->first, TEST_KEY_2);
@@ -370,11 +379,7 @@ LT_BEGIN_AUTO_TEST(file_upload_suite, file_upload_memory_and_disk_two_files)
                                httpserver::http::http_utils::upload_filename_template;
     LT_CHECK_EQ(file->second.get_file_system_file_name().substr(0, file->second.get_file_system_file_name().size() - 6),
                 expected_filename.substr(0, expected_filename.size() - 6));
-    unlink(file->second.get_file_system_file_name().c_str());
-
-
-    ws->stop();
-    delete ws;
+    LT_CHECK_EQ(file_exists(file->second.get_file_system_file_name()), false);
 LT_END_AUTO_TEST(file_upload_memory_and_disk_two_files)
 
 LT_BEGIN_AUTO_TEST(file_upload_suite, file_upload_disk_only)
@@ -395,6 +400,9 @@ LT_BEGIN_AUTO_TEST(file_upload_suite, file_upload_disk_only)
     CURLcode res = send_file_to_webserver(false, false);
     LT_ASSERT_EQ(res, 0);
 
+    ws->stop();
+    delete ws;
+
     string actual_content = resource.get_content();
     LT_CHECK_EQ(actual_content.size(), 0);
 
@@ -416,10 +424,7 @@ LT_BEGIN_AUTO_TEST(file_upload_suite, file_upload_disk_only)
                                httpserver::http::http_utils::upload_filename_template;
     LT_CHECK_EQ(file->second.get_file_system_file_name().substr(0, file->second.get_file_system_file_name().size() - 6),
                 expected_filename.substr(0, expected_filename.size() - 6));
-    unlink(file->second.get_file_system_file_name().c_str());
-
-    ws->stop();
-    delete ws;
+    LT_CHECK_EQ(file_exists(file->second.get_file_system_file_name()), false);
 LT_END_AUTO_TEST(file_upload_disk_only)
 
 LT_BEGIN_AUTO_TEST(file_upload_suite, file_upload_memory_only_incl_content)
