From a29020d568148b614342e7783faa7f89a371eeb2 Mon Sep 17 00:00:00 2001 From: Stuart Byma Date: Sat, 29 Oct 2022 14:04:04 +0000 Subject: [PATCH 01/14] Issue #286: use string_view in API methods where applicable This change reduces required copying of immutable data primarily in the HTTP request API. Methods returning strings now return string_view to avoid copying immutable data from the underlying MHD impl. get_*s methods still return a std::map which must be allocated and constructed, but does not copy underlying strings. Fix a few instances of these method calls in the code base. Introduce some type aliases for maps from string_view to string_view, string to string to reduce code verbosity. Overload the comparator operator()s as required. Provide overloads for dump_*_map functions and a template impl to avoid code duplication. Bump C++ version to 17 required to use string_view. Remove CI configurations using ancient compilers that don't support string_view or C++17 --- .github/workflows/verify-build.yml | 36 ------------------------------ README.md | 16 ++++++------- configure.ac | 4 ++-- examples/hello_with_get_arg.cpp | 2 +- examples/hello_world.cpp | 4 ++-- examples/url_registration.cpp | 2 +- src/http_request.cpp | 32 +++++++++++++------------- src/http_utils.cpp | 30 ++++++++++++++----------- src/httpserver/http_request.hpp | 24 ++++++++++---------- src/httpserver/http_utils.hpp | 21 +++++++++++++++-- src/webserver.cpp | 2 +- test/integ/basic.cpp | 8 +++---- 12 files changed, 83 insertions(+), 98 deletions(-) diff --git a/.github/workflows/verify-build.yml b/.github/workflows/verify-build.yml index 97b081e5..ee2e0f8e 100644 --- a/.github/workflows/verify-build.yml +++ b/.github/workflows/verify-build.yml @@ -98,24 +98,6 @@ jobs: cc-compiler: clang++-6.0 debug: debug coverage: nocoverage - - test-group: extra - os: ubuntu-18.04 - os-type: ubuntu - build-type: none - compiler-family: gcc - c-compiler: gcc-5 - cc-compiler: g++-5 - debug: nodebug - coverage: nocoverage - - test-group: extra - os: ubuntu-18.04 - os-type: ubuntu - build-type: none - compiler-family: gcc - c-compiler: gcc-6 - cc-compiler: g++-6 - debug: nodebug - coverage: nocoverage - test-group: extra os: ubuntu-latest os-type: ubuntu @@ -152,24 +134,6 @@ jobs: cc-compiler: g++-10 debug: nodebug coverage: nocoverage - - test-group: extra - os: ubuntu-18.04 - os-type: ubuntu - build-type: none - compiler-family: clang - c-compiler: clang-3.9 - cc-compiler: clang++-3.9 - debug: nodebug - coverage: nocoverage - - test-group: extra - os: ubuntu-18.04 - os-type: ubuntu - build-type: none - compiler-family: clang - c-compiler: clang-4.0 - cc-compiler: clang++-4.0 - debug: nodebug - coverage: nocoverage - test-group: extra os: ubuntu-18.04 os-type: ubuntu diff --git a/README.md b/README.md index ca1ac442..c4610498 100644 --- a/README.md +++ b/README.md @@ -559,14 +559,14 @@ The `http_request` class has a set of methods you will have access to when imple * _**const std::vector\&** get_path_pieces() **const**:_ Returns the components of the path requested by the HTTP client (each piece of the path split by `'/'`. * _**const std::string&** get_path_piece(int index) **const**:_ Returns one piece of the path requested by the HTTP client. The piece is selected through the `index` parameter (0-indexed). * _**const std::string&** get_method() **const**:_ Returns the method requested by the HTTP client. -* _**const std::string** get_header(**const std::string&** key) **const**:_ Returns the header with name equal to `key` if present in the HTTP request. Returns an `empty string` otherwise. -* _**const std::string** get_cookie(**const std::string&** key) **const**:_ Returns the cookie with name equal to `key` if present in the HTTP request. Returns an `empty string` otherwise. -* _**const std::string** get_footer(**const std::string&** key) **const**:_ Returns the footer with name equal to `key` if present in the HTTP request (only for http 1.1 chunked encodings). Returns an `empty string` otherwise. -* _**const std::string** get_arg(**const std::string&** key) **const**:_ Returns the argument with name equal to `key` if present in the HTTP request. Arguments can be (1) querystring parameters, (2) path argument (in case of parametric endpoint, (3) parameters parsed from the HTTP request body if the body is in `application/x-www-form-urlencoded` or `multipart/form-data` formats and the postprocessor is enabled in the webserver (enabled by default). -* _**const std::map** get_headers() **const**:_ Returns a map containing all the headers present in the HTTP request. -* _**const std::map** get_cookies() **const**:_ Returns a map containing all the cookies present in the HTTP request. -* _**const std::map** get_footers() **const**:_ Returns a map containing all the footers present in the HTTP request (only for http 1.1 chunked encodings). -* _**const std::map** get_args() **const**:_ Returns all the arguments present in the HTTP request. Arguments can be (1) querystring parameters, (2) path argument (in case of parametric endpoint, (3) parameters parsed from the HTTP request body if the body is in `application/x-www-form-urlencoded` or `multipart/form-data` formats and the postprocessor is enabled in the webserver (enabled by default). +* _**std::string_view** get_header(**std::string_view** key) **const**:_ Returns the header with name equal to `key` if present in the HTTP request. Returns an `empty string` otherwise. +* _**std::string_view** get_cookie(**std::string_view** key) **const**:_ Returns the cookie with name equal to `key` if present in the HTTP request. Returns an `empty string` otherwise. +* _**std::string_view** get_footer(**std::string_view** key) **const**:_ Returns the footer with name equal to `key` if present in the HTTP request (only for http 1.1 chunked encodings). Returns an `empty string` otherwise. +* _**std::string_view** get_arg(**std::string_view** key) **const**:_ Returns the argument with name equal to `key` if present in the HTTP request. Arguments can be (1) querystring parameters, (2) path argument (in case of parametric endpoint, (3) parameters parsed from the HTTP request body if the body is in `application/x-www-form-urlencoded` or `multipart/form-data` formats and the postprocessor is enabled in the webserver (enabled by default). +* _**const std::map** get_headers() **const**:_ Returns a map containing all the headers present in the HTTP request. +* _**const std::map** get_cookies() **const**:_ Returns a map containing all the cookies present in the HTTP request. +* _**const std::map** get_footers() **const**:_ Returns a map containing all the footers present in the HTTP request (only for http 1.1 chunked encodings). +* _**const std::map** get_args() **const**:_ Returns all the arguments present in the HTTP request. Arguments can be (1) querystring parameters, (2) path argument (in case of parametric endpoint, (3) parameters parsed from the HTTP request body if the body is in `application/x-www-form-urlencoded` or `multipart/form-data` formats and the postprocessor is enabled in the webserver (enabled by default). * _**const std::map>** get_files() **const**:_ Returns information about all the uploaded files (if the files are stored to disk). This information includes the key (as identifier of the outer map), the original file name (as identifier of the inner map) and a class `file_info`, which includes the size of the file and the path to the file in the file system. * _**const std::string&** get_content() **const**:_ Returns the body of the HTTP request. * _**bool** content_too_large() **const**:_ Returns `true` if the body length of the HTTP request sent by the client is longer than the max allowed on the server. diff --git a/configure.ac b/configure.ac index 23adba36..b40e3067 100644 --- a/configure.ac +++ b/configure.ac @@ -44,7 +44,7 @@ AC_LANG([C++]) AC_SYS_LARGEFILE # Minimal feature-set required -AX_CXX_COMPILE_STDCXX([14]) +AX_CXX_COMPILE_STDCXX([17]) native_srcdir=$srcdir @@ -197,7 +197,7 @@ AM_LDFLAGS="-lstdc++" if test x"$debugit" = x"yes"; then AC_DEFINE([DEBUG],[],[Debug Mode]) - AM_CXXFLAGS="$AM_CXXFLAGS -DDEBUG -g -Wall -Wextra -Werror -pedantic -std=c++14 -Wno-unused-command-line-argument -O0" + AM_CXXFLAGS="$AM_CXXFLAGS -DDEBUG -g -Wall -Wextra -Werror -pedantic -std=c++17 -Wno-unused-command-line-argument -O0" AM_CFLAGS="$AM_CXXFLAGS -DDEBUG -g -Wall -Wextra -Werror -pedantic -Wno-unused-command-line-argument -O0" else AC_DEFINE([NDEBUG],[],[No-debug Mode]) diff --git a/examples/hello_with_get_arg.cpp b/examples/hello_with_get_arg.cpp index d8a3dfa0..268d00c3 100644 --- a/examples/hello_with_get_arg.cpp +++ b/examples/hello_with_get_arg.cpp @@ -23,7 +23,7 @@ class hello_world_resource : public httpserver::http_resource { public: std::shared_ptr render(const httpserver::http_request& req) { - return std::shared_ptr(new httpserver::string_response("Hello: " + req.get_arg("name"))); + return std::shared_ptr(new httpserver::string_response("Hello: " + std::string(req.get_arg("name")))); } }; diff --git a/examples/hello_world.cpp b/examples/hello_world.cpp index b23d35c0..391a600f 100755 --- a/examples/hello_world.cpp +++ b/examples/hello_world.cpp @@ -33,8 +33,8 @@ class hello_world_resource : public httpserver::http_resource { std::shared_ptr hello_world_resource::render(const httpserver::http_request& req) { // It is possible to store data inside the resource object that can be altered through the requests std::cout << "Data was: " << data << std::endl; - std::string datapar = req.get_arg("data"); - set_some_data(datapar == "" ? "no data passed!!!" : datapar); + std::string_view datapar = req.get_arg("data"); + set_some_data(datapar == "" ? "no data passed!!!" : std::string(datapar)); std::cout << "Now data is:" << data << std::endl; // It is possible to send a response initializing an http_string_response that reads the content to send in response from a string. diff --git a/examples/url_registration.cpp b/examples/url_registration.cpp index 1b504bdb..4d96f80b 100644 --- a/examples/url_registration.cpp +++ b/examples/url_registration.cpp @@ -37,7 +37,7 @@ class handling_multiple_resource : public httpserver::http_resource { class url_args_resource : public httpserver::http_resource { public: std::shared_ptr render(const httpserver::http_request& req) { - return std::shared_ptr(new httpserver::string_response("ARGS: " + req.get_arg("arg1") + " and " + req.get_arg("arg2"))); + return std::shared_ptr(new httpserver::string_response("ARGS: " + std::string(req.get_arg("arg1")) + " and " + std::string(req.get_arg("arg2")))); } }; diff --git a/src/http_request.cpp b/src/http_request.cpp index 195dddbc..ba3ec323 100644 --- a/src/http_request.cpp +++ b/src/http_request.cpp @@ -32,7 +32,7 @@ const char http_request::EMPTY[] = ""; struct arguments_accumulator { unescaper_ptr unescaper; - std::map* arguments; + http::arg_map* arguments; }; void http_request::set_method(const std::string& method) { @@ -55,8 +55,8 @@ bool http_request::check_digest_auth(const std::string& realm, const std::string return true; } -const std::string http_request::get_connection_value(const std::string& key, enum MHD_ValueKind kind) const { - const char* header_c = MHD_lookup_connection_value(underlying_connection, kind, key.c_str()); +std::string_view http_request::get_connection_value(std::string_view key, enum MHD_ValueKind kind) const { + const char* header_c = MHD_lookup_connection_value(underlying_connection, kind, key.data()); if (header_c == nullptr) return EMPTY; @@ -67,45 +67,45 @@ MHD_Result http_request::build_request_header(void *cls, enum MHD_ValueKind kind // Parameters needed to respect MHD interface, but not used in the implementation. std::ignore = kind; - std::map* dhr = static_cast*>(cls); + http::value_map_view* dhr = static_cast(cls); (*dhr)[key] = value; return MHD_YES; } -const std::map http_request::get_headerlike_values(enum MHD_ValueKind kind) const { - std::map headers; +const http::value_map_view http_request::get_headerlike_values(enum MHD_ValueKind kind) const { + http::value_map_view headers; MHD_get_connection_values(underlying_connection, kind, &build_request_header, reinterpret_cast(&headers)); return headers; } -const std::string http_request::get_header(const std::string& key) const { +std::string_view http_request::get_header(std::string_view key) const { return get_connection_value(key, MHD_HEADER_KIND); } -const std::map http_request::get_headers() const { +const http::value_map_view http_request::get_headers() const { return get_headerlike_values(MHD_HEADER_KIND); } -const std::string http_request::get_footer(const std::string& key) const { +std::string_view http_request::get_footer(std::string_view key) const { return get_connection_value(key, MHD_FOOTER_KIND); } -const std::map http_request::get_footers() const { +const http::value_map_view http_request::get_footers() const { return get_headerlike_values(MHD_FOOTER_KIND); } -const std::string http_request::get_cookie(const std::string& key) const { +std::string_view http_request::get_cookie(std::string_view key) const { return get_connection_value(key, MHD_COOKIE_KIND); } -const std::map http_request::get_cookies() const { +const http::value_map_view http_request::get_cookies() const { return get_headerlike_values(MHD_COOKIE_KIND); } -const std::string http_request::get_arg(const std::string& key) const { - std::map::const_iterator it = args.find(key); +std::string_view http_request::get_arg(std::string_view key) const { + std::map::const_iterator it = args.find(std::string(key)); if (it != args.end()) { return it->second; @@ -114,8 +114,8 @@ const std::string http_request::get_arg(const std::string& key) const { return get_connection_value(key, MHD_GET_ARGUMENT_KIND); } -const std::map http_request::get_args() const { - std::map arguments; +const http::arg_map http_request::get_args() const { + http::arg_map arguments; arguments.insert(args.begin(), args.end()); arguments_accumulator aa; diff --git a/src/http_utils.cpp b/src/http_utils.cpp index 552bfc19..9519c3e4 100644 --- a/src/http_utils.cpp +++ b/src/http_utils.cpp @@ -510,9 +510,10 @@ const std::string load_file(const std::string& filename) { } } -void dump_header_map(std::ostream &os, const std::string &prefix, const std::map &map) { - std::map::const_iterator it = map.begin(); - std::map::const_iterator end = map.end(); +template +void dump_map(std::ostream &os, const std::string &prefix, const map_t &map) { + auto it = map.begin(); + auto end = map.end(); if (map.size()) { os << " " << prefix << " ["; @@ -523,17 +524,20 @@ void dump_header_map(std::ostream &os, const std::string &prefix, const std::map } } -void dump_arg_map(std::ostream &os, const std::string &prefix, const std::map &map) { - std::map::const_iterator it = map.begin(); - std::map::const_iterator end = map.end(); +void dump_header_map(std::ostream &os, const std::string &prefix, const http::value_map &map) { + dump_map(os, prefix, map); +} - if (map.size()) { - os << " " << prefix << " ["; - for (; it != end; ++it) { - os << (*it).first << ":\"" << (*it).second << "\" "; - } - os << "]" << std::endl; - } +void dump_header_map(std::ostream &os, const std::string &prefix, const http::value_map_view &map) { + dump_map(os, prefix, map); +} + +void dump_arg_map(std::ostream &os, const std::string &prefix, const http::arg_map &map) { + dump_map(os, prefix, map); +} + +void dump_arg_map(std::ostream &os, const std::string &prefix, const http::arg_map_view &map) { + dump_map(os, prefix, map); } size_t base_unescaper(std::string* s, unescaper_ptr unescaper) { diff --git a/src/httpserver/http_request.hpp b/src/httpserver/http_request.hpp index 560f76c5..0b66fa46 100644 --- a/src/httpserver/http_request.hpp +++ b/src/httpserver/http_request.hpp @@ -115,28 +115,28 @@ class http_request { * @param result a map > that will be filled with all headers * @result the size of the map **/ - const std::map get_headers() const; + const http::value_map_view get_headers() const; /** * Method used to get all footers passed with the request. * @param result a map > that will be filled with all footers * @result the size of the map **/ - const std::map get_footers() const; + const http::value_map_view get_footers() const; /** * Method used to get all cookies passed with the request. * @param result a map > that will be filled with all cookies * @result the size of the map **/ - const std::map get_cookies() const; + const http::value_map_view get_cookies() const; /** * Method used to get all args passed with the request. * @param result a map > that will be filled with all args * @result the size of the map **/ - const std::map get_args() const; + const http::arg_map get_args() const; /** * Method to get or create a file info struct in the map if the provided filename is already in the map @@ -159,29 +159,29 @@ class http_request { * @param key the specific header to get the value from * @return the value of the header. **/ - const std::string get_header(const std::string& key) const; + std::string_view get_header(std::string_view key) const; - const std::string get_cookie(const std::string& key) const; + std::string_view get_cookie(std::string_view key) const; /** * Method used to get a specific footer passed with the request. * @param key the specific footer to get the value from * @return the value of the footer. **/ - const std::string get_footer(const std::string& key) const; + std::string_view get_footer(std::string_view key) const; /** * Method used to get a specific argument passed with the request. * @param ket the specific argument to get the value from * @return the value of the arg. **/ - const std::string get_arg(const std::string& key) const; + std::string_view get_arg(std::string_view key) const; /** * Method used to get the content of the request. * @return the content in string representation **/ - const std::string& get_content() const { + std::string_view get_content() const { return content; } @@ -260,7 +260,7 @@ class http_request { std::string path; std::string method; - std::map args; + http::arg_map args; std::map> files; std::string content = ""; size_t content_size_limit = static_cast(-1); @@ -356,8 +356,8 @@ class http_request { } } - const std::string get_connection_value(const std::string& key, enum MHD_ValueKind kind) const; - const std::map get_headerlike_values(enum MHD_ValueKind kind) const; + std::string_view get_connection_value(std::string_view key, enum MHD_ValueKind kind) const; + const http::value_map_view get_headerlike_values(enum MHD_ValueKind kind) const; friend class webserver; friend struct details::modded_request; diff --git a/src/httpserver/http_utils.hpp b/src/httpserver/http_utils.hpp index 6de523ea..cee5436a 100644 --- a/src/httpserver/http_utils.hpp +++ b/src/httpserver/http_utils.hpp @@ -284,6 +284,9 @@ class header_comparator { * @param first string * @param second string **/ + bool operator()(std::string_view x, std::string_view y) const { + COMPARATOR(x, y, std::toupper); + } bool operator()(const std::string& x, const std::string& y) const { COMPARATOR(x, y, std::toupper); } @@ -301,6 +304,13 @@ class arg_comparator { * @param first string * @param second string **/ + bool operator()(std::string_view x, std::string_view y) const { +#ifdef CASE_INSENSITIVE + COMPARATOR(x, y, std::toupper); +#else + COMPARATOR(x, y,); // NOLINT(whitespace/comma) +#endif + } bool operator()(const std::string& x, const std::string& y) const { #ifdef CASE_INSENSITIVE COMPARATOR(x, y, std::toupper); @@ -310,6 +320,11 @@ class arg_comparator { } }; +using value_map = std::map; +using arg_map = std::map; +using value_map_view = std::map; +using arg_map_view = std::map; + struct ip_representation { http_utils::IP_version_T ip_version; uint16_t pieces[16]; @@ -356,7 +371,8 @@ uint16_t get_port(const struct sockaddr* sa); * @param prefix Prefix to identify the map * @param map **/ -void dump_header_map(std::ostream &os, const std::string &prefix, const std::map &map); +void dump_header_map(std::ostream &os, const std::string &prefix, const http::value_map &map); +void dump_header_map(std::ostream &os, const std::string &prefix, const http::value_map_view &map); /** * Method to output the contents of an arguments map to a std::ostream @@ -364,7 +380,8 @@ void dump_header_map(std::ostream &os, const std::string &prefix, const std::map * @param prefix Prefix to identify the map * @param map **/ -void dump_arg_map(std::ostream &os, const std::string &prefix, const std::map &map); +void dump_arg_map(std::ostream &os, const std::string &prefix, const http::arg_map &map); +void dump_arg_map(std::ostream &os, const std::string &prefix, const http::arg_map_view &map); /** * Process escape sequences ('+'=space, %HH) Updates val in place; the diff --git a/src/webserver.cpp b/src/webserver.cpp index 46f1082a..5a1f4324 100644 --- a/src/webserver.cpp +++ b/src/webserver.cpp @@ -463,7 +463,7 @@ MHD_Result webserver::post_iterator(void *cls, enum MHD_ValueKind kind, try { if (filename == nullptr || mr->ws->file_upload_target != FILE_UPLOAD_DISK_ONLY) { - mr->dhr->set_arg(key, mr->dhr->get_arg(key) + std::string(data, size)); + mr->dhr->set_arg(key, std::string(mr->dhr->get_arg(key)) + std::string(data, size)); } if (filename && *filename != '\0' && mr->ws->file_upload_target != FILE_UPLOAD_MEMORY_ONLY) { diff --git a/test/integ/basic.cpp b/test/integ/basic.cpp index c34ec599..a61335b6 100644 --- a/test/integ/basic.cpp +++ b/test/integ/basic.cpp @@ -63,14 +63,14 @@ class simple_resource : public http_resource { return shared_ptr(new string_response("OK", 200, "text/plain")); } shared_ptr render_POST(const http_request& req) { - return shared_ptr(new string_response(req.get_arg("arg1")+req.get_arg("arg2"), 200, "text/plain")); + return shared_ptr(new string_response(std::string(req.get_arg("arg1")) + std::string(req.get_arg("arg2")), 200, "text/plain")); } }; class args_resource : public http_resource { public: shared_ptr render_GET(const http_request& req) { - return shared_ptr(new string_response(req.get_arg("arg") + req.get_arg("arg2"), 200, "text/plain")); + return shared_ptr(new string_response(std::string(req.get_arg("arg")) + std::string(req.get_arg("arg2")), 200, "text/plain")); } }; @@ -102,14 +102,14 @@ class cookie_set_test_resource : public http_resource { class cookie_reading_resource : public http_resource { public: shared_ptr render_GET(const http_request& req) { - return shared_ptr(new string_response(req.get_cookie("name"), 200, "text/plain")); + return shared_ptr(new string_response(std::string(req.get_cookie("name")), 200, "text/plain")); } }; class header_reading_resource : public http_resource { public: shared_ptr render_GET(const http_request& req) { - return shared_ptr(new string_response(req.get_header("MyHeader"), 200, "text/plain")); + return shared_ptr(new string_response(std::string(req.get_header("MyHeader")), 200, "text/plain")); } }; From 2d0f77f7e9644bb87452d4ce142cffdf3ba5f6c4 Mon Sep 17 00:00:00 2001 From: Stuart Byma Date: Sat, 29 Oct 2022 14:04:04 +0000 Subject: [PATCH 02/14] Issue #286: use string_view in API methods where applicable This change reduces required copying of immutable data primarily in the HTTP request API. Methods returning strings now return string_view to avoid copying immutable data from the underlying MHD impl. get_*s methods still return a std::map which must be allocated and constructed, but does not copy underlying strings. Fix a few instances of these method calls in the code base. Introduce some type aliases for maps from string_view to string_view, string to string to reduce code verbosity. Overload the comparator operator()s as required. Provide overloads for dump_*_map functions and a template impl to avoid code duplication. Bump C++ version to 17 required to use string_view. Remove CI configurations using ancient compilers that don't support string_view or C++17 --- src/httpserver/http_response.hpp | 6 +++--- src/httpserver/http_utils.hpp | 6 +----- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/src/httpserver/http_response.hpp b/src/httpserver/http_response.hpp index 0ab1ec87..88efb821 100644 --- a/src/httpserver/http_response.hpp +++ b/src/httpserver/http_response.hpp @@ -130,9 +130,9 @@ class http_response { private: int response_code = -1; - std::map headers; - std::map footers; - std::map cookies; + http::value_map headers; + http::value_map footers; + http::value_map cookies; protected: friend std::ostream &operator<< (std::ostream &os, const http_response &r); diff --git a/src/httpserver/http_utils.hpp b/src/httpserver/http_utils.hpp index cee5436a..9479722e 100644 --- a/src/httpserver/http_utils.hpp +++ b/src/httpserver/http_utils.hpp @@ -312,11 +312,7 @@ class arg_comparator { #endif } bool operator()(const std::string& x, const std::string& y) const { -#ifdef CASE_INSENSITIVE - COMPARATOR(x, y, std::toupper); -#else - COMPARATOR(x, y,); // NOLINT(whitespace/comma) -#endif + return operator()(std::string_view(x), std::string_view(y)); } }; From f795be947b2839f9c7485e814e08c922ca7b2f01 Mon Sep 17 00:00:00 2001 From: Stuart Byma Date: Sat, 29 Oct 2022 14:04:04 +0000 Subject: [PATCH 03/14] Issue #286: use string_view in API methods where applicable This change reduces required copying of immutable data primarily in the HTTP request API. Methods returning strings now return string_view to avoid copying immutable data from the underlying MHD impl. get_*s methods still return a std::map which must be allocated and constructed, but does not copy underlying strings. Fix a few instances of these method calls in the code base. Introduce some type aliases for maps from string_view to string_view, string to string to reduce code verbosity. Overload the comparator operator()s as required. Bump C++ version to 17 required to use string_view. Remove CI configurations using ancient compilers that don't support string_view or C++17 --- README.md | 6 ++++-- src/http_request.cpp | 18 +++++++++--------- src/http_response.cpp | 16 +++++++++++++--- src/http_utils.cpp | 12 ++---------- src/httpserver/http_request.hpp | 10 +++++----- src/httpserver/http_response.hpp | 6 +++--- src/httpserver/http_utils.hpp | 12 +++++------- test/integ/basic.cpp | 2 +- test/integ/file_upload.cpp | 31 +++++++++++++++++-------------- test/unit/http_utils_test.cpp | 8 ++++---- 10 files changed, 63 insertions(+), 58 deletions(-) diff --git a/README.md b/README.md index c4610498..8d21640c 100644 --- a/README.md +++ b/README.md @@ -511,7 +511,9 @@ There are essentially four ways to specify an endpoint string: class url_args_resource : public http_resource { public: std::shared_ptr render(const http_request& req) { - return std::shared_ptr(new string_response("ARGS: " + req.get_arg("arg1") + " and " + req.get_arg("arg2"))); + std::string arg1(req.get_arg("arg1")); + std::string arg2(req.get_arg("arg2")); + return std::shared_ptr(new string_response("ARGS: " + arg1 + " and " + arg2)); } }; @@ -597,7 +599,7 @@ Details on the `http::file_info` structure. class hello_world_resource : public http_resource { public: std::shared_ptr render(const http_request& req) { - return std::shared_ptr(new string_response("Hello: " + req.get_arg("name"))); + return std::shared_ptr(new string_response("Hello: " + std::string(req.get_arg("name")))); } }; diff --git a/src/http_request.cpp b/src/http_request.cpp index ba3ec323..337d0655 100644 --- a/src/http_request.cpp +++ b/src/http_request.cpp @@ -32,7 +32,7 @@ const char http_request::EMPTY[] = ""; struct arguments_accumulator { unescaper_ptr unescaper; - http::arg_map* arguments; + http::arg_view_map* arguments; }; void http_request::set_method(const std::string& method) { @@ -67,13 +67,13 @@ MHD_Result http_request::build_request_header(void *cls, enum MHD_ValueKind kind // Parameters needed to respect MHD interface, but not used in the implementation. std::ignore = kind; - http::value_map_view* dhr = static_cast(cls); + http::header_view_map* dhr = static_cast(cls); (*dhr)[key] = value; return MHD_YES; } -const http::value_map_view http_request::get_headerlike_values(enum MHD_ValueKind kind) const { - http::value_map_view headers; +const http::header_view_map http_request::get_headerlike_values(enum MHD_ValueKind kind) const { + http::header_view_map headers; MHD_get_connection_values(underlying_connection, kind, &build_request_header, reinterpret_cast(&headers)); @@ -84,7 +84,7 @@ std::string_view http_request::get_header(std::string_view key) const { return get_connection_value(key, MHD_HEADER_KIND); } -const http::value_map_view http_request::get_headers() const { +const http::header_view_map http_request::get_headers() const { return get_headerlike_values(MHD_HEADER_KIND); } @@ -92,7 +92,7 @@ std::string_view http_request::get_footer(std::string_view key) const { return get_connection_value(key, MHD_FOOTER_KIND); } -const http::value_map_view http_request::get_footers() const { +const http::header_view_map http_request::get_footers() const { return get_headerlike_values(MHD_FOOTER_KIND); } @@ -100,7 +100,7 @@ std::string_view http_request::get_cookie(std::string_view key) const { return get_connection_value(key, MHD_COOKIE_KIND); } -const http::value_map_view http_request::get_cookies() const { +const http::header_view_map http_request::get_cookies() const { return get_headerlike_values(MHD_COOKIE_KIND); } @@ -114,8 +114,8 @@ std::string_view http_request::get_arg(std::string_view key) const { return get_connection_value(key, MHD_GET_ARGUMENT_KIND); } -const http::arg_map http_request::get_args() const { - http::arg_map arguments; +const http::arg_view_map http_request::get_args() const { + http::arg_view_map arguments; arguments.insert(args.begin(), args.end()); arguments_accumulator aa; diff --git a/src/http_response.cpp b/src/http_response.cpp index c801fa66..e042d4a6 100644 --- a/src/http_response.cpp +++ b/src/http_response.cpp @@ -54,12 +54,22 @@ void http_response::shoutCAST() { response_code |= http::http_utils::shoutcast_response; } +namespace { +static inline http::header_view_map to_view_map(const http::header_map & hdr_map) { + http::header_view_map view_map; + for (const auto & item : hdr_map) { + view_map[std::string_view(item.first)] = std::string_view(item.second); + } + return view_map; +} +} + std::ostream &operator<< (std::ostream &os, const http_response &r) { os << "Response [response_code:" << r.response_code << "]" << std::endl; - http::dump_header_map(os, "Headers", r.headers); - http::dump_header_map(os, "Footers", r.footers); - http::dump_header_map(os, "Cookies", r.cookies); + http::dump_header_map(os, "Headers", to_view_map(r.headers)); + http::dump_header_map(os, "Footers", to_view_map(r.footers)); + http::dump_header_map(os, "Cookies", to_view_map(r.cookies)); return os; } diff --git a/src/http_utils.cpp b/src/http_utils.cpp index 9519c3e4..7dd2ccd7 100644 --- a/src/http_utils.cpp +++ b/src/http_utils.cpp @@ -524,19 +524,11 @@ void dump_map(std::ostream &os, const std::string &prefix, const map_t &map) { } } -void dump_header_map(std::ostream &os, const std::string &prefix, const http::value_map &map) { +void dump_header_map(std::ostream &os, const std::string &prefix, const http::header_view_map &map) { dump_map(os, prefix, map); } -void dump_header_map(std::ostream &os, const std::string &prefix, const http::value_map_view &map) { - dump_map(os, prefix, map); -} - -void dump_arg_map(std::ostream &os, const std::string &prefix, const http::arg_map &map) { - dump_map(os, prefix, map); -} - -void dump_arg_map(std::ostream &os, const std::string &prefix, const http::arg_map_view &map) { +void dump_arg_map(std::ostream &os, const std::string &prefix, const http::arg_view_map &map) { dump_map(os, prefix, map); } diff --git a/src/httpserver/http_request.hpp b/src/httpserver/http_request.hpp index 0b66fa46..df6de8f9 100644 --- a/src/httpserver/http_request.hpp +++ b/src/httpserver/http_request.hpp @@ -115,28 +115,28 @@ class http_request { * @param result a map > that will be filled with all headers * @result the size of the map **/ - const http::value_map_view get_headers() const; + const http::header_view_map get_headers() const; /** * Method used to get all footers passed with the request. * @param result a map > that will be filled with all footers * @result the size of the map **/ - const http::value_map_view get_footers() const; + const http::header_view_map get_footers() const; /** * Method used to get all cookies passed with the request. * @param result a map > that will be filled with all cookies * @result the size of the map **/ - const http::value_map_view get_cookies() const; + const http::header_view_map get_cookies() const; /** * Method used to get all args passed with the request. * @param result a map > that will be filled with all args * @result the size of the map **/ - const http::arg_map get_args() const; + const http::arg_view_map get_args() const; /** * Method to get or create a file info struct in the map if the provided filename is already in the map @@ -357,7 +357,7 @@ class http_request { } std::string_view get_connection_value(std::string_view key, enum MHD_ValueKind kind) const; - const http::value_map_view get_headerlike_values(enum MHD_ValueKind kind) const; + const http::header_view_map get_headerlike_values(enum MHD_ValueKind kind) const; friend class webserver; friend struct details::modded_request; diff --git a/src/httpserver/http_response.hpp b/src/httpserver/http_response.hpp index 88efb821..4c7988ea 100644 --- a/src/httpserver/http_response.hpp +++ b/src/httpserver/http_response.hpp @@ -130,9 +130,9 @@ class http_response { private: int response_code = -1; - http::value_map headers; - http::value_map footers; - http::value_map cookies; + http::header_map headers; + http::header_map footers; + http::header_map cookies; protected: friend std::ostream &operator<< (std::ostream &os, const http_response &r); diff --git a/src/httpserver/http_utils.hpp b/src/httpserver/http_utils.hpp index 9479722e..a23359fc 100644 --- a/src/httpserver/http_utils.hpp +++ b/src/httpserver/http_utils.hpp @@ -316,10 +316,10 @@ class arg_comparator { } }; -using value_map = std::map; +using header_map = std::map; using arg_map = std::map; -using value_map_view = std::map; -using arg_map_view = std::map; +using header_view_map = std::map; +using arg_view_map = std::map; struct ip_representation { http_utils::IP_version_T ip_version; @@ -367,8 +367,7 @@ uint16_t get_port(const struct sockaddr* sa); * @param prefix Prefix to identify the map * @param map **/ -void dump_header_map(std::ostream &os, const std::string &prefix, const http::value_map &map); -void dump_header_map(std::ostream &os, const std::string &prefix, const http::value_map_view &map); +void dump_header_map(std::ostream &os, const std::string &prefix, const http::header_view_map &map); /** * Method to output the contents of an arguments map to a std::ostream @@ -376,8 +375,7 @@ void dump_header_map(std::ostream &os, const std::string &prefix, const http::va * @param prefix Prefix to identify the map * @param map **/ -void dump_arg_map(std::ostream &os, const std::string &prefix, const http::arg_map &map); -void dump_arg_map(std::ostream &os, const std::string &prefix, const http::arg_map_view &map); +void dump_arg_map(std::ostream &os, const std::string &prefix, const http::arg_view_map &map); /** * Process escape sequences ('+'=space, %HH) Updates val in place; the diff --git a/test/integ/basic.cpp b/test/integ/basic.cpp index a61335b6..4992f55c 100644 --- a/test/integ/basic.cpp +++ b/test/integ/basic.cpp @@ -116,7 +116,7 @@ class header_reading_resource : public http_resource { class full_args_resource : public http_resource { public: shared_ptr render_GET(const http_request& req) { - return shared_ptr(new string_response(req.get_args().at("arg"), 200, "text/plain")); + return shared_ptr(new string_response(std::string(req.get_args().at("arg")), 200, "text/plain")); } }; diff --git a/test/integ/file_upload.cpp b/test/integ/file_upload.cpp index 643608fb..380dd52e 100644 --- a/test/integ/file_upload.cpp +++ b/test/integ/file_upload.cpp @@ -29,6 +29,7 @@ #include "./littletest.hpp" using std::string; +using std::string_view; using std::map; using std::shared_ptr; using std::vector; @@ -41,6 +42,7 @@ using httpserver::string_response; using httpserver::file_response; using httpserver::webserver; using httpserver::create_webserver; +using httpserver::http::arg_view_map; static const char* TEST_CONTENT_FILENAME = "test_content"; static const char* TEST_CONTENT_FILEPATH = "./test_content"; @@ -153,7 +155,7 @@ class print_file_upload_resource : public http_resource { return hresp; } - const map get_args() const { + const arg_view_map get_args() const { return args; } @@ -166,7 +168,7 @@ class print_file_upload_resource : public http_resource { } private: - map args; + arg_view_map args; map> files; string content; }; @@ -204,11 +206,12 @@ LT_BEGIN_AUTO_TEST(file_upload_suite, file_upload_memory_and_disk) LT_CHECK_EQ(actual_content.find(FILENAME_IN_GET_CONTENT) != string::npos, true); LT_CHECK_EQ(actual_content.find(TEST_CONTENT) != string::npos, true); - map args = resource.get_args(); + auto args = resource.get_args(); LT_CHECK_EQ(args.size(), 1); - map::iterator arg = args.begin(); + auto arg = args.begin(); LT_CHECK_EQ(arg->first, TEST_KEY); LT_CHECK_EQ(arg->second, TEST_CONTENT); + std::string test(arg->second); map> files = resource.get_files(); LT_CHECK_EQ(files.size(), 1); @@ -249,7 +252,7 @@ LT_BEGIN_AUTO_TEST(file_upload_suite, file_upload_memory_and_disk_via_put) string actual_content = resource.get_content(); LT_CHECK_EQ(actual_content, TEST_CONTENT); - map args = resource.get_args(); + auto args = resource.get_args(); LT_CHECK_EQ(args.size(), 0); map> files = resource.get_files(); @@ -286,9 +289,9 @@ LT_BEGIN_AUTO_TEST(file_upload_suite, file_upload_memory_and_disk_additional_par LT_CHECK_EQ(actual_content.find(TEST_PARAM_KEY) != string::npos, true); LT_CHECK_EQ(actual_content.find(TEST_PARAM_VALUE) != string::npos, true); - map args = resource.get_args(); + auto args = resource.get_args(); LT_CHECK_EQ(args.size(), 2); - map::iterator arg = args.begin(); + auto arg = args.begin(); LT_CHECK_EQ(arg->first, TEST_KEY); LT_CHECK_EQ(arg->second, TEST_CONTENT); arg++; @@ -340,9 +343,9 @@ LT_BEGIN_AUTO_TEST(file_upload_suite, file_upload_memory_and_disk_two_files) LT_CHECK_EQ(actual_content.find(FILENAME_IN_GET_CONTENT_2) != string::npos, true); LT_CHECK_EQ(actual_content.find(TEST_CONTENT_2) != string::npos, true); - map args = resource.get_args(); + auto args = resource.get_args(); LT_CHECK_EQ(args.size(), 2); - map::iterator arg = args.begin(); + auto arg = args.begin(); LT_CHECK_EQ(arg->first, TEST_KEY); LT_CHECK_EQ(arg->second, TEST_CONTENT); arg++; @@ -406,7 +409,7 @@ LT_BEGIN_AUTO_TEST(file_upload_suite, file_upload_disk_only) string actual_content = resource.get_content(); LT_CHECK_EQ(actual_content.size(), 0); - map args = resource.get_args(); + auto args = resource.get_args(); LT_CHECK_EQ(args.size(), 0); map> files = resource.get_files(); @@ -447,9 +450,9 @@ LT_BEGIN_AUTO_TEST(file_upload_suite, file_upload_memory_only_incl_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); - map args = resource.get_args(); + auto args = resource.get_args(); LT_CHECK_EQ(args.size(), 1); - map::iterator arg = args.begin(); + auto arg = args.begin(); LT_CHECK_EQ(arg->first, TEST_KEY); LT_CHECK_EQ(arg->second, TEST_CONTENT); @@ -479,9 +482,9 @@ LT_BEGIN_AUTO_TEST(file_upload_suite, file_upload_memory_only_excl_content) string actual_content = resource.get_content(); LT_CHECK_EQ(actual_content.size(), 0); - map args = resource.get_args(); + auto args = resource.get_args(); LT_CHECK_EQ(args.size(), 1); - map::iterator arg = args.begin(); + auto arg = args.begin(); LT_CHECK_EQ(arg->first, TEST_KEY); LT_CHECK_EQ(arg->second, TEST_CONTENT); diff --git a/test/unit/http_utils_test.cpp b/test/unit/http_utils_test.cpp index 71c1a655..1b7486ac 100644 --- a/test/unit/http_utils_test.cpp +++ b/test/unit/http_utils_test.cpp @@ -596,7 +596,7 @@ LT_BEGIN_AUTO_TEST(http_utils_suite, ip_representation_less_than_with_masks) LT_END_AUTO_TEST(ip_representation_less_than_with_masks) LT_BEGIN_AUTO_TEST(http_utils_suite, dump_header_map) - std::map header_map; + std::map header_map; header_map["HEADER_ONE"] = "VALUE_ONE"; header_map["HEADER_TWO"] = "VALUE_TWO"; header_map["HEADER_THREE"] = "VALUE_THREE"; @@ -607,7 +607,7 @@ LT_BEGIN_AUTO_TEST(http_utils_suite, dump_header_map) LT_END_AUTO_TEST(dump_header_map) LT_BEGIN_AUTO_TEST(http_utils_suite, dump_header_map_no_prefix) - std::map header_map; + std::map header_map; header_map["HEADER_ONE"] = "VALUE_ONE"; header_map["HEADER_TWO"] = "VALUE_TWO"; header_map["HEADER_THREE"] = "VALUE_THREE"; @@ -618,7 +618,7 @@ LT_BEGIN_AUTO_TEST(http_utils_suite, dump_header_map_no_prefix) LT_END_AUTO_TEST(dump_header_map_no_prefix) LT_BEGIN_AUTO_TEST(http_utils_suite, dump_arg_map) - std::map arg_map; + std::map arg_map; arg_map["ARG_ONE"] = "VALUE_ONE"; arg_map["ARG_TWO"] = "VALUE_TWO"; arg_map["ARG_THREE"] = "VALUE_THREE"; @@ -629,7 +629,7 @@ LT_BEGIN_AUTO_TEST(http_utils_suite, dump_arg_map) LT_END_AUTO_TEST(dump_arg_map) LT_BEGIN_AUTO_TEST(http_utils_suite, dump_arg_map_no_prefix) - std::map arg_map; + std::map arg_map; arg_map["ARG_ONE"] = "VALUE_ONE"; arg_map["ARG_TWO"] = "VALUE_TWO"; arg_map["ARG_THREE"] = "VALUE_THREE"; From 4d1776352e9b4da2ac5fc07b0fffc5aa55cdc9d1 Mon Sep 17 00:00:00 2001 From: Stuart Byma Date: Wed, 9 Nov 2022 09:23:22 +0000 Subject: [PATCH 04/14] remove accidentally added line of code --- test/integ/file_upload.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/test/integ/file_upload.cpp b/test/integ/file_upload.cpp index 380dd52e..123c1f57 100644 --- a/test/integ/file_upload.cpp +++ b/test/integ/file_upload.cpp @@ -211,7 +211,6 @@ LT_BEGIN_AUTO_TEST(file_upload_suite, file_upload_memory_and_disk) auto arg = args.begin(); LT_CHECK_EQ(arg->first, TEST_KEY); LT_CHECK_EQ(arg->second, TEST_CONTENT); - std::string test(arg->second); map> files = resource.get_files(); LT_CHECK_EQ(files.size(), 1); From dbef2a651cc35437cc22ff54760c5455a0e612b8 Mon Sep 17 00:00:00 2001 From: Stuart Byma Date: Wed, 9 Nov 2022 09:50:07 +0000 Subject: [PATCH 05/14] fix data scope issue In the file upload test, it was saving the arg_view_map from the request, which went out of scope, leaving dangling refs. Fix by making a deep copy. --- test/integ/file_upload.cpp | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/test/integ/file_upload.cpp b/test/integ/file_upload.cpp index 123c1f57..1efbb13b 100644 --- a/test/integ/file_upload.cpp +++ b/test/integ/file_upload.cpp @@ -42,7 +42,7 @@ using httpserver::string_response; using httpserver::file_response; using httpserver::webserver; using httpserver::create_webserver; -using httpserver::http::arg_view_map; +using httpserver::http::arg_map; static const char* TEST_CONTENT_FILENAME = "test_content"; static const char* TEST_CONTENT_FILEPATH = "./test_content"; @@ -141,7 +141,11 @@ class print_file_upload_resource : public http_resource { public: shared_ptr render_POST(const http_request& req) { content = req.get_content(); - args = req.get_args(); + auto args_view = req.get_args(); + // req may go out of scope, so we need to copy the values. + for (auto const & item : args_view) { + args[std::string(item.first)] = std::string(item.second); + } files = req.get_files(); shared_ptr hresp(new string_response("OK", 201, "text/plain")); return hresp; @@ -149,13 +153,17 @@ class print_file_upload_resource : public http_resource { shared_ptr render_PUT(const http_request& req) { content = req.get_content(); - args = req.get_args(); + auto args_view = req.get_args(); + // req may go out of scope, so we need to copy the values. + for (auto const & item : args_view) { + args[std::string(item.first)] = std::string(item.second); + } files = req.get_files(); shared_ptr hresp(new string_response("OK", 200, "text/plain")); return hresp; } - const arg_view_map get_args() const { + const arg_map get_args() const { return args; } @@ -168,7 +176,7 @@ class print_file_upload_resource : public http_resource { } private: - arg_view_map args; + arg_map args; map> files; string content; }; From a391d147567c1fc7ff11960bc271fb1b4a3cb6da Mon Sep 17 00:00:00 2001 From: Stuart Byma Date: Wed, 9 Nov 2022 10:17:00 +0000 Subject: [PATCH 06/14] undo change from arg_map to arg_view_map Because build_request_args makes a temp copy of underlying data and mutates it, it is not feasible currently to return a view over that data. --- src/http_request.cpp | 6 +++--- src/http_utils.cpp | 2 +- src/httpserver/http_request.hpp | 2 +- src/httpserver/http_utils.hpp | 5 ++--- test/unit/http_utils_test.cpp | 4 ++-- 5 files changed, 9 insertions(+), 10 deletions(-) diff --git a/src/http_request.cpp b/src/http_request.cpp index 337d0655..7389db22 100644 --- a/src/http_request.cpp +++ b/src/http_request.cpp @@ -32,7 +32,7 @@ const char http_request::EMPTY[] = ""; struct arguments_accumulator { unescaper_ptr unescaper; - http::arg_view_map* arguments; + http::arg_map* arguments; }; void http_request::set_method(const std::string& method) { @@ -114,8 +114,8 @@ std::string_view http_request::get_arg(std::string_view key) const { return get_connection_value(key, MHD_GET_ARGUMENT_KIND); } -const http::arg_view_map http_request::get_args() const { - http::arg_view_map arguments; +const http::arg_map http_request::get_args() const { + http::arg_map arguments; arguments.insert(args.begin(), args.end()); arguments_accumulator aa; diff --git a/src/http_utils.cpp b/src/http_utils.cpp index 7dd2ccd7..0e822724 100644 --- a/src/http_utils.cpp +++ b/src/http_utils.cpp @@ -528,7 +528,7 @@ void dump_header_map(std::ostream &os, const std::string &prefix, const http::he dump_map(os, prefix, map); } -void dump_arg_map(std::ostream &os, const std::string &prefix, const http::arg_view_map &map) { +void dump_arg_map(std::ostream &os, const std::string &prefix, const http::arg_map &map) { dump_map(os, prefix, map); } diff --git a/src/httpserver/http_request.hpp b/src/httpserver/http_request.hpp index df6de8f9..9b8d55b7 100644 --- a/src/httpserver/http_request.hpp +++ b/src/httpserver/http_request.hpp @@ -136,7 +136,7 @@ class http_request { * @param result a map > that will be filled with all args * @result the size of the map **/ - const http::arg_view_map get_args() const; + const http::arg_map get_args() const; /** * Method to get or create a file info struct in the map if the provided filename is already in the map diff --git a/src/httpserver/http_utils.hpp b/src/httpserver/http_utils.hpp index a23359fc..798fb6a5 100644 --- a/src/httpserver/http_utils.hpp +++ b/src/httpserver/http_utils.hpp @@ -317,9 +317,8 @@ class arg_comparator { }; using header_map = std::map; -using arg_map = std::map; using header_view_map = std::map; -using arg_view_map = std::map; +using arg_map = std::map; struct ip_representation { http_utils::IP_version_T ip_version; @@ -375,7 +374,7 @@ void dump_header_map(std::ostream &os, const std::string &prefix, const http::he * @param prefix Prefix to identify the map * @param map **/ -void dump_arg_map(std::ostream &os, const std::string &prefix, const http::arg_view_map &map); +void dump_arg_map(std::ostream &os, const std::string &prefix, const http::arg_map &map); /** * Process escape sequences ('+'=space, %HH) Updates val in place; the diff --git a/test/unit/http_utils_test.cpp b/test/unit/http_utils_test.cpp index 1b7486ac..90f872f4 100644 --- a/test/unit/http_utils_test.cpp +++ b/test/unit/http_utils_test.cpp @@ -618,7 +618,7 @@ LT_BEGIN_AUTO_TEST(http_utils_suite, dump_header_map_no_prefix) LT_END_AUTO_TEST(dump_header_map_no_prefix) LT_BEGIN_AUTO_TEST(http_utils_suite, dump_arg_map) - std::map arg_map; + std::map arg_map; arg_map["ARG_ONE"] = "VALUE_ONE"; arg_map["ARG_TWO"] = "VALUE_TWO"; arg_map["ARG_THREE"] = "VALUE_THREE"; @@ -629,7 +629,7 @@ LT_BEGIN_AUTO_TEST(http_utils_suite, dump_arg_map) LT_END_AUTO_TEST(dump_arg_map) LT_BEGIN_AUTO_TEST(http_utils_suite, dump_arg_map_no_prefix) - std::map arg_map; + std::map arg_map; arg_map["ARG_ONE"] = "VALUE_ONE"; arg_map["ARG_TWO"] = "VALUE_TWO"; arg_map["ARG_THREE"] = "VALUE_THREE"; From dbe1aab045d58e9d39915bb717cfd4b2072c2171 Mon Sep 17 00:00:00 2001 From: Stuart Byma Date: Wed, 9 Nov 2022 10:47:23 +0000 Subject: [PATCH 07/14] fix readme after changing get_args signature --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 8d21640c..8e8ab233 100644 --- a/README.md +++ b/README.md @@ -568,7 +568,7 @@ The `http_request` class has a set of methods you will have access to when imple * _**const std::map** get_headers() **const**:_ Returns a map containing all the headers present in the HTTP request. * _**const std::map** get_cookies() **const**:_ Returns a map containing all the cookies present in the HTTP request. * _**const std::map** get_footers() **const**:_ Returns a map containing all the footers present in the HTTP request (only for http 1.1 chunked encodings). -* _**const std::map** get_args() **const**:_ Returns all the arguments present in the HTTP request. Arguments can be (1) querystring parameters, (2) path argument (in case of parametric endpoint, (3) parameters parsed from the HTTP request body if the body is in `application/x-www-form-urlencoded` or `multipart/form-data` formats and the postprocessor is enabled in the webserver (enabled by default). +* _**const std::map** get_args() **const**:_ Returns all the arguments present in the HTTP request. Arguments can be (1) querystring parameters, (2) path argument (in case of parametric endpoint, (3) parameters parsed from the HTTP request body if the body is in `application/x-www-form-urlencoded` or `multipart/form-data` formats and the postprocessor is enabled in the webserver (enabled by default). * _**const std::map>** get_files() **const**:_ Returns information about all the uploaded files (if the files are stored to disk). This information includes the key (as identifier of the outer map), the original file name (as identifier of the inner map) and a class `file_info`, which includes the size of the file and the path to the file in the file system. * _**const std::string&** get_content() **const**:_ Returns the body of the HTTP request. * _**bool** content_too_large() **const**:_ Returns `true` if the body length of the HTTP request sent by the client is longer than the max allowed on the server. From 8c6231b84159f4a280137ce803ad6a5c184e933e Mon Sep 17 00:00:00 2001 From: Stuart Byma Date: Mon, 14 Nov 2022 09:29:53 +0000 Subject: [PATCH 08/14] cache transformed data to return views over data Add a data_cache type to the http request type, and a unique_ptr instance member. This allows caching of transformed and/or copied data from MHD, while preserving const-ness of all accessor methods. Change all applicable methods to string_view, fix a few call sites. --- src/http_request.cpp | 97 +++++++++++++++++++-------------- src/http_utils.cpp | 2 +- src/httpserver/http_request.hpp | 44 +++++++++++---- src/httpserver/http_utils.hpp | 3 +- test/integ/authentication.cpp | 2 +- test/integ/basic.cpp | 2 +- test/unit/http_utils_test.cpp | 4 +- 7 files changed, 97 insertions(+), 57 deletions(-) diff --git a/src/http_request.cpp b/src/http_request.cpp index 7389db22..fff2dade 100644 --- a/src/http_request.cpp +++ b/src/http_request.cpp @@ -40,9 +40,9 @@ void http_request::set_method(const std::string& method) { } bool http_request::check_digest_auth(const std::string& realm, const std::string& password, int nonce_timeout, bool* reload_nonce) const { - std::string digested_user = get_digested_user(); + auto digested_user = get_digested_user(); - int val = MHD_digest_auth_check(underlying_connection, realm.c_str(), digested_user.c_str(), password.c_str(), nonce_timeout); + int val = MHD_digest_auth_check(underlying_connection, realm.c_str(), digested_user.data(), password.c_str(), nonce_timeout); if (val == MHD_INVALID_NONCE) { *reload_nonce = true; @@ -114,16 +114,23 @@ std::string_view http_request::get_arg(std::string_view key) const { return get_connection_value(key, MHD_GET_ARGUMENT_KIND); } -const http::arg_map http_request::get_args() const { - http::arg_map arguments; +const http::arg_view_map http_request::get_args() const { + http::arg_view_map arguments; arguments.insert(args.begin(), args.end()); + if (!cache->unescaped_mhd_args.empty()) { + arguments.insert(cache->unescaped_mhd_args.begin(), cache->unescaped_mhd_args.end()); + return arguments; + } + arguments_accumulator aa; aa.unescaper = unescaper; - aa.arguments = &arguments; + aa.arguments = &cache->unescaped_mhd_args; MHD_get_connection_values(underlying_connection, MHD_GET_ARGUMENT_KIND, &build_request_args, reinterpret_cast(&aa)); + arguments.insert(cache->unescaped_mhd_args.begin(), cache->unescaped_mhd_args.end()); + return arguments; } @@ -131,12 +138,14 @@ http::file_info& http_request::get_or_create_file_info(const std::string& key, c return files[key][upload_file_name]; } -const std::string http_request::get_querystring() const { - std::string querystring = ""; +std::string_view http_request::get_querystring() const { + if (!cache->query_str.empty()) { + return cache->query_str; + } - MHD_get_connection_values(underlying_connection, MHD_GET_ARGUMENT_KIND, &build_request_querystring, reinterpret_cast(&querystring)); + MHD_get_connection_values(underlying_connection, MHD_GET_ARGUMENT_KIND, &build_request_querystring, reinterpret_cast(&cache->query_str)); - return querystring; + return cache->query_str; } MHD_Result http_request::build_request_args(void *cls, enum MHD_ValueKind kind, const char *key, const char *arg_value) { @@ -173,47 +182,50 @@ MHD_Result http_request::build_request_querystring(void *cls, enum MHD_ValueKind return MHD_YES; } -const std::string http_request::get_user() const { - char* username = nullptr; +void http_request::fetch_user_pass() const { char* password = nullptr; + auto* username = MHD_basic_auth_get_username_password(underlying_connection, &password); - username = MHD_basic_auth_get_username_password(underlying_connection, &password); - if (password != nullptr) free(password); - - std::string user; - if (username != nullptr) user = username; - - free(username); - - return user; + if (username != nullptr) { + cache->username = username; + MHD_free(username); + } + if (password != nullptr) { + cache->password = password; + MHD_free(password); + } } -const std::string http_request::get_pass() const { - char* username = nullptr; - char* password = nullptr; - - username = MHD_basic_auth_get_username_password(underlying_connection, &password); - if (username != nullptr) free(username); - - std::string pass; - if (password != nullptr) pass = password; - - free(password); +std::string_view http_request::get_user() const { + if (!cache->username.empty()) { + return cache->username; + } + fetch_user_pass(); + return cache->username; +} - return pass; +std::string_view http_request::get_pass() const { + if (!cache->password.empty()) { + return cache->password; + } + fetch_user_pass(); + return cache->password; } -const std::string http_request::get_digested_user() const { - char* digested_user_c = nullptr; - digested_user_c = MHD_digest_auth_get_username(underlying_connection); +std::string_view http_request::get_digested_user() const { + if (!cache->digested_user.empty()) { + return cache->digested_user; + } + + auto* digested_user_c = MHD_digest_auth_get_username(underlying_connection); - std::string digested_user = EMPTY; + cache->digested_user = EMPTY; if (digested_user_c != nullptr) { - digested_user = digested_user_c; + cache->digested_user = digested_user_c; free(digested_user_c); } - return digested_user; + return cache->digested_user; } #ifdef HAVE_GNUTLS @@ -229,10 +241,15 @@ gnutls_session_t http_request::get_tls_session() const { } #endif // HAVE_GNUTLS -const std::string http_request::get_requestor() const { +std::string_view http_request::get_requestor() const { + if (!cache->requestor_ip.empty()) { + return cache->requestor_ip; + } + const MHD_ConnectionInfo * conninfo = MHD_get_connection_info(underlying_connection, MHD_CONNECTION_INFO_CLIENT_ADDRESS); - return http::get_ip_str(conninfo->client_addr); + cache->requestor_ip = http::get_ip_str(conninfo->client_addr); + return cache->requestor_ip; } uint16_t http_request::get_requestor_port() const { diff --git a/src/http_utils.cpp b/src/http_utils.cpp index 0e822724..7dd2ccd7 100644 --- a/src/http_utils.cpp +++ b/src/http_utils.cpp @@ -528,7 +528,7 @@ void dump_header_map(std::ostream &os, const std::string &prefix, const http::he dump_map(os, prefix, map); } -void dump_arg_map(std::ostream &os, const std::string &prefix, const http::arg_map &map) { +void dump_arg_map(std::ostream &os, const std::string &prefix, const http::arg_view_map &map) { dump_map(os, prefix, map); } diff --git a/src/httpserver/http_request.hpp b/src/httpserver/http_request.hpp index 9b8d55b7..b971c0c7 100644 --- a/src/httpserver/http_request.hpp +++ b/src/httpserver/http_request.hpp @@ -35,6 +35,7 @@ #include #include #include +#include #include #include #include @@ -59,25 +60,25 @@ class http_request { * Method used to get the username eventually passed through basic authentication. * @return string representation of the username. **/ - const std::string get_user() const; + std::string_view get_user() const; /** * Method used to get the username extracted from a digest authentication * @return the username **/ - const std::string get_digested_user() const; + std::string_view get_digested_user() const; /** * Method used to get the password eventually passed through basic authentication. * @return string representation of the password. **/ - const std::string get_pass() const; + std::string_view get_pass() const; /** * Method used to get the path requested * @return string representing the path requested. **/ - const std::string& get_path() const { + std::string_view get_path() const { return path; } @@ -106,7 +107,7 @@ class http_request { * Method used to get the METHOD used to make the request. * @return string representing the method. **/ - const std::string& get_method() const { + std::string_view get_method() const { return method; } @@ -136,7 +137,7 @@ class http_request { * @param result a map > that will be filled with all args * @result the size of the map **/ - const http::arg_map get_args() const; + const http::arg_view_map get_args() const; /** * Method to get or create a file info struct in the map if the provided filename is already in the map @@ -196,13 +197,13 @@ class http_request { * Method used to get the content of the query string.. * @return the query string in string representation **/ - const std::string get_querystring() const; + std::string_view get_querystring() const; /** * Method used to get the version of the request. * @return the version in string representation **/ - const std::string& get_version() const { + std::string_view get_version() const { return version; } @@ -224,7 +225,7 @@ class http_request { * Method used to get the requestor. * @return the requestor **/ - const std::string get_requestor() const; + std::string_view get_requestor() const; /** * Method used to get the requestor port used. @@ -240,11 +241,15 @@ class http_request { /** * Default constructor of the class. It is a specific responsibility of apis to initialize this type of objects. **/ - http_request() = default; + http_request() { + cache = std::make_unique(); + } http_request(MHD_Connection* underlying_connection, unescaper_ptr unescaper): underlying_connection(underlying_connection), - unescaper(unescaper) { } + unescaper(unescaper) { + cache = std::make_unique(); + } /** * Copy constructor. @@ -276,6 +281,8 @@ class http_request { static MHD_Result build_request_querystring(void *cls, enum MHD_ValueKind kind, const char *key, const char *value); + void fetch_user_pass() const; + /** * Method used to set an argument value by key. * @param key The name identifying the argument @@ -356,6 +363,21 @@ class http_request { } } + // Cache certain data items on demand so we can consistently return views + // over the data. Some things we transform before returning to the user for + // simplicity (e.g. query_str, requestor), others out of necessity (arg unescaping). + // Others (username, password, digested_user) MHD returns as char* that we need + // to make a copy of and free anyway. + struct data_cache { + std::string username; + std::string password; + std::string query_str; + std::string requestor_ip; + std::string digested_user; + http::arg_map unescaped_mhd_args; + }; + std::unique_ptr cache; + std::string_view get_connection_value(std::string_view key, enum MHD_ValueKind kind) const; const http::header_view_map get_headerlike_values(enum MHD_ValueKind kind) const; diff --git a/src/httpserver/http_utils.hpp b/src/httpserver/http_utils.hpp index 798fb6a5..f4871b17 100644 --- a/src/httpserver/http_utils.hpp +++ b/src/httpserver/http_utils.hpp @@ -319,6 +319,7 @@ class arg_comparator { using header_map = std::map; using header_view_map = std::map; using arg_map = std::map; +using arg_view_map = std::map; struct ip_representation { http_utils::IP_version_T ip_version; @@ -374,7 +375,7 @@ void dump_header_map(std::ostream &os, const std::string &prefix, const http::he * @param prefix Prefix to identify the map * @param map **/ -void dump_arg_map(std::ostream &os, const std::string &prefix, const http::arg_map &map); +void dump_arg_map(std::ostream &os, const std::string &prefix, const http::arg_view_map &map); /** * Process escape sequences ('+'=space, %HH) Updates val in place; the diff --git a/test/integ/authentication.cpp b/test/integ/authentication.cpp index f7277d6e..c17a15c7 100644 --- a/test/integ/authentication.cpp +++ b/test/integ/authentication.cpp @@ -58,7 +58,7 @@ class user_pass_resource : public http_resource { if (req.get_user() != "myuser" || req.get_pass() != "mypass") { return shared_ptr(new basic_auth_fail_response("FAIL", "examplerealm")); } - return shared_ptr(new string_response(req.get_user() + " " + req.get_pass(), 200, "text/plain")); + return shared_ptr(new string_response(std::string(req.get_user()) + " " + std::string(req.get_pass()), 200, "text/plain")); } }; diff --git a/test/integ/basic.cpp b/test/integ/basic.cpp index 4992f55c..c6e61d85 100644 --- a/test/integ/basic.cpp +++ b/test/integ/basic.cpp @@ -123,7 +123,7 @@ class full_args_resource : public http_resource { class querystring_resource : public http_resource { public: shared_ptr render_GET(const http_request& req) { - return shared_ptr(new string_response(req.get_querystring(), 200, "text/plain")); + return shared_ptr(new string_response(std::string(req.get_querystring()), 200, "text/plain")); } }; diff --git a/test/unit/http_utils_test.cpp b/test/unit/http_utils_test.cpp index 90f872f4..1b7486ac 100644 --- a/test/unit/http_utils_test.cpp +++ b/test/unit/http_utils_test.cpp @@ -618,7 +618,7 @@ LT_BEGIN_AUTO_TEST(http_utils_suite, dump_header_map_no_prefix) LT_END_AUTO_TEST(dump_header_map_no_prefix) LT_BEGIN_AUTO_TEST(http_utils_suite, dump_arg_map) - std::map arg_map; + std::map arg_map; arg_map["ARG_ONE"] = "VALUE_ONE"; arg_map["ARG_TWO"] = "VALUE_TWO"; arg_map["ARG_THREE"] = "VALUE_THREE"; @@ -629,7 +629,7 @@ LT_BEGIN_AUTO_TEST(http_utils_suite, dump_arg_map) LT_END_AUTO_TEST(dump_arg_map) LT_BEGIN_AUTO_TEST(http_utils_suite, dump_arg_map_no_prefix) - std::map arg_map; + std::map arg_map; arg_map["ARG_ONE"] = "VALUE_ONE"; arg_map["ARG_TWO"] = "VALUE_TWO"; arg_map["ARG_THREE"] = "VALUE_THREE"; From 1771e1328f420f2d71d51b72df04728d278c6f87 Mon Sep 17 00:00:00 2001 From: Stuart Byma Date: Mon, 14 Nov 2022 09:29:53 +0000 Subject: [PATCH 09/14] cache transformed data to return views over data Add a data_cache type to the http request type, and a unique_ptr instance member. This allows caching of transformed and/or copied data from MHD, while preserving const-ness of all accessor methods. Change all applicable methods to string_view, fix a few call sites. --- examples/basic_authentication.cpp | 2 +- examples/url_registration.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/examples/basic_authentication.cpp b/examples/basic_authentication.cpp index 2661806f..7fb82340 100644 --- a/examples/basic_authentication.cpp +++ b/examples/basic_authentication.cpp @@ -27,7 +27,7 @@ class user_pass_resource : public httpserver::http_resource { return std::shared_ptr(new httpserver::basic_auth_fail_response("FAIL", "test@example.com")); } - return std::shared_ptr(new httpserver::string_response(req.get_user() + " " + req.get_pass(), 200, "text/plain")); + return std::shared_ptr(new httpserver::string_response(std::string(req.get_user()) + " " + std::string(req.get_pass()), 200, "text/plain")); } }; diff --git a/examples/url_registration.cpp b/examples/url_registration.cpp index 4d96f80b..be6d1ce3 100644 --- a/examples/url_registration.cpp +++ b/examples/url_registration.cpp @@ -30,7 +30,7 @@ class hello_world_resource : public httpserver::http_resource { class handling_multiple_resource : public httpserver::http_resource { public: std::shared_ptr render(const httpserver::http_request& req) { - return std::shared_ptr(new httpserver::string_response("Your URL: " + req.get_path())); + return std::shared_ptr(new httpserver::string_response("Your URL: " + std::string(req.get_path()))); } }; From f50b953c305fcfc13c06bfe5869968380ed797ae Mon Sep 17 00:00:00 2001 From: Stuart Byma Date: Mon, 14 Nov 2022 09:29:53 +0000 Subject: [PATCH 10/14] cache transformed data to return views over data Add a data_cache type to the http request type, and a unique_ptr instance member. This allows caching of transformed and/or copied data from MHD, while preserving const-ness of all accessor methods. Change all applicable methods to string_view, fix a few call sites. --- src/httpserver/http_request.hpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/httpserver/http_request.hpp b/src/httpserver/http_request.hpp index b971c0c7..a85d1ea7 100644 --- a/src/httpserver/http_request.hpp +++ b/src/httpserver/http_request.hpp @@ -251,14 +251,14 @@ class http_request { cache = std::make_unique(); } + http_request(const http_request& b) = delete; /** - * Copy constructor. - * @param b http_request b to copy attributes from. + * Move constructor. + * @param b http_request b to move attributes from. **/ - http_request(const http_request& b) = default; http_request(http_request&& b) noexcept = default; - http_request& operator=(const http_request& b) = default; + http_request& operator=(const http_request& b) = delete; http_request& operator=(http_request&& b) = default; ~http_request(); From 539496dd84a112610ea8d360f190789375fa079c Mon Sep 17 00:00:00 2001 From: Stuart Byma Date: Mon, 14 Nov 2022 09:29:53 +0000 Subject: [PATCH 11/14] cache transformed data to return views over data Add a data_cache type to the http request type, and a unique_ptr instance member. This allows caching of transformed and/or copied data from MHD, while preserving const-ness of all accessor methods. Change all applicable methods to string_view, fix a few call sites. --- src/httpserver/http_request.hpp | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/httpserver/http_request.hpp b/src/httpserver/http_request.hpp index a85d1ea7..5cbd284b 100644 --- a/src/httpserver/http_request.hpp +++ b/src/httpserver/http_request.hpp @@ -241,15 +241,11 @@ class http_request { /** * Default constructor of the class. It is a specific responsibility of apis to initialize this type of objects. **/ - http_request() { - cache = std::make_unique(); - } + http_request() : cache(std::make_unique()) {} http_request(MHD_Connection* underlying_connection, unescaper_ptr unescaper): underlying_connection(underlying_connection), - unescaper(unescaper) { - cache = std::make_unique(); - } + unescaper(unescaper), cache(std::make_unique()) {} http_request(const http_request& b) = delete; /** From 52fb1f0c3a16e80b81a4eb6337ef92b013bd2e63 Mon Sep 17 00:00:00 2001 From: Stuart Byma Date: Mon, 14 Nov 2022 09:29:53 +0000 Subject: [PATCH 12/14] cache transformed data to return views over data Add a http_request_data_cache type to the http request type, and a unique_ptr instance member. This allows caching of transformed and/or copied data from MHD, while preserving const-ness of all accessor methods. Change all applicable methods to string_view, fix a few call sites. Remove the existing http_request::args member, use the cache instead. Make the http_request class move-only. --- src/http_request.cpp | 21 ++++++++++----------- src/httpserver/http_request.hpp | 33 +++++++++++++++++++-------------- 2 files changed, 29 insertions(+), 25 deletions(-) diff --git a/src/http_request.cpp b/src/http_request.cpp index fff2dade..6e85f408 100644 --- a/src/http_request.cpp +++ b/src/http_request.cpp @@ -105,9 +105,9 @@ const http::header_view_map http_request::get_cookies() const { } std::string_view http_request::get_arg(std::string_view key) const { - std::map::const_iterator it = args.find(std::string(key)); + std::map::const_iterator it = cache->unescaped_args.find(std::string(key)); - if (it != args.end()) { + if (it != cache->unescaped_args.end()) { return it->second; } @@ -116,20 +116,19 @@ std::string_view http_request::get_arg(std::string_view key) const { const http::arg_view_map http_request::get_args() const { http::arg_view_map arguments; - arguments.insert(args.begin(), args.end()); - if (!cache->unescaped_mhd_args.empty()) { - arguments.insert(cache->unescaped_mhd_args.begin(), cache->unescaped_mhd_args.end()); + if (!cache->unescaped_args.empty()) { + arguments.insert(cache->unescaped_args.begin(), cache->unescaped_args.end()); return arguments; } arguments_accumulator aa; aa.unescaper = unescaper; - aa.arguments = &cache->unescaped_mhd_args; + aa.arguments = &cache->unescaped_args; MHD_get_connection_values(underlying_connection, MHD_GET_ARGUMENT_KIND, &build_request_args, reinterpret_cast(&aa)); - arguments.insert(cache->unescaped_mhd_args.begin(), cache->unescaped_mhd_args.end()); + arguments.insert(cache->unescaped_args.begin(), cache->unescaped_args.end()); return arguments; } @@ -139,13 +138,13 @@ http::file_info& http_request::get_or_create_file_info(const std::string& key, c } std::string_view http_request::get_querystring() const { - if (!cache->query_str.empty()) { - return cache->query_str; + if (!cache->querystring.empty()) { + return cache->querystring; } - MHD_get_connection_values(underlying_connection, MHD_GET_ARGUMENT_KIND, &build_request_querystring, reinterpret_cast(&cache->query_str)); + MHD_get_connection_values(underlying_connection, MHD_GET_ARGUMENT_KIND, &build_request_querystring, reinterpret_cast(&cache->querystring)); - return cache->query_str; + return cache->querystring; } MHD_Result http_request::build_request_args(void *cls, enum MHD_ValueKind kind, const char *key, const char *arg_value) { diff --git a/src/httpserver/http_request.hpp b/src/httpserver/http_request.hpp index 5cbd284b..7c2850c8 100644 --- a/src/httpserver/http_request.hpp +++ b/src/httpserver/http_request.hpp @@ -241,12 +241,15 @@ class http_request { /** * Default constructor of the class. It is a specific responsibility of apis to initialize this type of objects. **/ - http_request() : cache(std::make_unique()) {} + http_request() : cache(std::make_unique()) {} http_request(MHD_Connection* underlying_connection, unescaper_ptr unescaper): underlying_connection(underlying_connection), - unescaper(unescaper), cache(std::make_unique()) {} + unescaper(unescaper), cache(std::make_unique()) {} + /** + * Copy constructor. Deleted to make class move-only. Also required due to presence of unique_ptr member. + **/ http_request(const http_request& b) = delete; /** * Move constructor. @@ -254,6 +257,9 @@ class http_request { **/ http_request(http_request&& b) noexcept = default; + /** + * Copy assign. Deleted to make class move-only. Also required due to presence of unique_ptr member. + **/ http_request& operator=(const http_request& b) = delete; http_request& operator=(http_request&& b) = default; @@ -261,7 +267,6 @@ class http_request { std::string path; std::string method; - http::arg_map args; std::map> files; std::string content = ""; size_t content_size_limit = static_cast(-1); @@ -277,7 +282,7 @@ class http_request { static MHD_Result build_request_querystring(void *cls, enum MHD_ValueKind kind, const char *key, const char *value); - void fetch_user_pass() const; + void fetch_user_pass() const; /** * Method used to set an argument value by key. @@ -285,7 +290,7 @@ class http_request { * @param value The value assumed by the argument **/ void set_arg(const std::string& key, const std::string& value) { - args[key] = value.substr(0, content_size_limit); + cache->unescaped_args[key] = value.substr(0, content_size_limit); } /** @@ -295,7 +300,7 @@ class http_request { * @param size The size in number of char of the value parameter. **/ void set_arg(const char* key, const char* value, size_t size) { - args[key] = std::string(value, std::min(size, content_size_limit)); + cache->unescaped_args[key] = std::string(value, std::min(size, content_size_limit)); } /** @@ -355,27 +360,27 @@ class http_request { void set_args(const std::map& args) { std::map::const_iterator it; for (it = args.begin(); it != args.end(); ++it) { - this->args[it->first] = it->second.substr(0, content_size_limit); + this->cache->unescaped_args[it->first] = it->second.substr(0, content_size_limit); } } + std::string_view get_connection_value(std::string_view key, enum MHD_ValueKind kind) const; + const http::header_view_map get_headerlike_values(enum MHD_ValueKind kind) const; + // Cache certain data items on demand so we can consistently return views // over the data. Some things we transform before returning to the user for // simplicity (e.g. query_str, requestor), others out of necessity (arg unescaping). // Others (username, password, digested_user) MHD returns as char* that we need // to make a copy of and free anyway. - struct data_cache { + struct http_request_data_cache { std::string username; std::string password; - std::string query_str; + std::string querystring; std::string requestor_ip; std::string digested_user; - http::arg_map unescaped_mhd_args; + http::arg_map unescaped_args; }; - std::unique_ptr cache; - - std::string_view get_connection_value(std::string_view key, enum MHD_ValueKind kind) const; - const http::header_view_map get_headerlike_values(enum MHD_ValueKind kind) const; + std::unique_ptr cache; friend class webserver; friend struct details::modded_request; From 40c0c54086f2eb6bd650cbe85f4579e6eb9e09b0 Mon Sep 17 00:00:00 2001 From: Stuart Byma Date: Mon, 14 Nov 2022 09:29:53 +0000 Subject: [PATCH 13/14] cache transformed data to return views over data Add a http_request_data_cache type to the http request type, and a unique_ptr instance member. This allows caching of transformed and/or copied data from MHD, while preserving const-ness of all accessor methods. Change all applicable methods to string_view, fix a few call sites. Remove the existing http_request::args member, use the cache instead. Make the http_request class move-only. --- src/http_request.cpp | 4 ++-- src/http_response.cpp | 2 +- src/http_utils.cpp | 4 ++-- src/httpserver/http_request.hpp | 12 ++++++++++-- 4 files changed, 15 insertions(+), 7 deletions(-) diff --git a/src/http_request.cpp b/src/http_request.cpp index 6e85f408..b0cd46b3 100644 --- a/src/http_request.cpp +++ b/src/http_request.cpp @@ -40,7 +40,7 @@ void http_request::set_method(const std::string& method) { } bool http_request::check_digest_auth(const std::string& realm, const std::string& password, int nonce_timeout, bool* reload_nonce) const { - auto digested_user = get_digested_user(); + std::string_view digested_user = get_digested_user(); int val = MHD_digest_auth_check(underlying_connection, realm.c_str(), digested_user.data(), password.c_str(), nonce_timeout); @@ -216,7 +216,7 @@ std::string_view http_request::get_digested_user() const { return cache->digested_user; } - auto* digested_user_c = MHD_digest_auth_get_username(underlying_connection); + char* digested_user_c = MHD_digest_auth_get_username(underlying_connection); cache->digested_user = EMPTY; if (digested_user_c != nullptr) { diff --git a/src/http_response.cpp b/src/http_response.cpp index e042d4a6..1fd56c69 100644 --- a/src/http_response.cpp +++ b/src/http_response.cpp @@ -55,7 +55,7 @@ void http_response::shoutCAST() { } namespace { -static inline http::header_view_map to_view_map(const http::header_map & hdr_map) { +static inline http::header_view_map to_view_map(const http::header_map& hdr_map) { http::header_view_map view_map; for (const auto & item : hdr_map) { view_map[std::string_view(item.first)] = std::string_view(item.second); diff --git a/src/http_utils.cpp b/src/http_utils.cpp index 7dd2ccd7..6366209c 100644 --- a/src/http_utils.cpp +++ b/src/http_utils.cpp @@ -525,11 +525,11 @@ void dump_map(std::ostream &os, const std::string &prefix, const map_t &map) { } void dump_header_map(std::ostream &os, const std::string &prefix, const http::header_view_map &map) { - dump_map(os, prefix, map); + dump_map(os, prefix, map); } void dump_arg_map(std::ostream &os, const std::string &prefix, const http::arg_view_map &map) { - dump_map(os, prefix, map); + dump_map(os, prefix, map); } size_t base_unescaper(std::string* s, unescaper_ptr unescaper) { diff --git a/src/httpserver/http_request.hpp b/src/httpserver/http_request.hpp index 7c2850c8..6228c82f 100644 --- a/src/httpserver/http_request.hpp +++ b/src/httpserver/http_request.hpp @@ -248,7 +248,11 @@ class http_request { unescaper(unescaper), cache(std::make_unique()) {} /** - * Copy constructor. Deleted to make class move-only. Also required due to presence of unique_ptr member. + * Copy constructor. Deleted to make class move-only. The class is move-only for several reasons: + * - Internal cache structure is expensive to copy + * - Various string members are expensive to copy + * - The destructor removes transient files from disk, which must only happen once. + * - unique_ptr members are not copyable. **/ http_request(const http_request& b) = delete; /** @@ -258,7 +262,11 @@ class http_request { http_request(http_request&& b) noexcept = default; /** - * Copy assign. Deleted to make class move-only. Also required due to presence of unique_ptr member. + * Copy-assign. Deleted to make class move-only. The class is move-only for several reasons: + * - Internal cache structure is expensive to copy + * - Various string members are expensive to copy + * - The destructor removes transient files from disk, which must only happen once. + * - unique_ptr members are not copyable. **/ http_request& operator=(const http_request& b) = delete; http_request& operator=(http_request&& b) = default; From 5aa7eb9dedf4488314dccbe92a4de031baf74334 Mon Sep 17 00:00:00 2001 From: Stuart Byma Date: Mon, 14 Nov 2022 09:29:53 +0000 Subject: [PATCH 14/14] cache transformed data to return views over data Add a http_request_data_cache type to the http request type, and a unique_ptr instance member. This allows caching of transformed and/or copied data from MHD, while preserving const-ness of all accessor methods. Change all applicable methods to string_view, fix a few call sites. Remove the existing http_request::args member, use the cache instead. Make the http_request class move-only. --- src/http_response.cpp | 4 ++-- src/http_utils.cpp | 6 +++--- src/httpserver/http_utils.hpp | 4 ++-- test/integ/file_upload.cpp | 4 ++-- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/http_response.cpp b/src/http_response.cpp index 1fd56c69..cb357b24 100644 --- a/src/http_response.cpp +++ b/src/http_response.cpp @@ -57,14 +57,14 @@ void http_response::shoutCAST() { namespace { static inline http::header_view_map to_view_map(const http::header_map& hdr_map) { http::header_view_map view_map; - for (const auto & item : hdr_map) { + for (const auto& item : hdr_map) { view_map[std::string_view(item.first)] = std::string_view(item.second); } return view_map; } } -std::ostream &operator<< (std::ostream &os, const http_response &r) { +std::ostream &operator<< (std::ostream& os, const http_response& r) { os << "Response [response_code:" << r.response_code << "]" << std::endl; http::dump_header_map(os, "Headers", to_view_map(r.headers)); diff --git a/src/http_utils.cpp b/src/http_utils.cpp index 6366209c..d34e196b 100644 --- a/src/http_utils.cpp +++ b/src/http_utils.cpp @@ -511,7 +511,7 @@ const std::string load_file(const std::string& filename) { } template -void dump_map(std::ostream &os, const std::string &prefix, const map_t &map) { +void dump_map(std::ostream& os, const std::string& prefix, const map_t& map) { auto it = map.begin(); auto end = map.end(); @@ -524,11 +524,11 @@ void dump_map(std::ostream &os, const std::string &prefix, const map_t &map) { } } -void dump_header_map(std::ostream &os, const std::string &prefix, const http::header_view_map &map) { +void dump_header_map(std::ostream& os, const std::string& prefix, const http::header_view_map &map) { dump_map(os, prefix, map); } -void dump_arg_map(std::ostream &os, const std::string &prefix, const http::arg_view_map &map) { +void dump_arg_map(std::ostream& os, const std::string& prefix, const http::arg_view_map &map) { dump_map(os, prefix, map); } diff --git a/src/httpserver/http_utils.hpp b/src/httpserver/http_utils.hpp index f4871b17..e6b5411f 100644 --- a/src/httpserver/http_utils.hpp +++ b/src/httpserver/http_utils.hpp @@ -367,7 +367,7 @@ uint16_t get_port(const struct sockaddr* sa); * @param prefix Prefix to identify the map * @param map **/ -void dump_header_map(std::ostream &os, const std::string &prefix, const http::header_view_map &map); +void dump_header_map(std::ostream& os, const std::string& prefix, const http::header_view_map& map); /** * Method to output the contents of an arguments map to a std::ostream @@ -375,7 +375,7 @@ void dump_header_map(std::ostream &os, const std::string &prefix, const http::he * @param prefix Prefix to identify the map * @param map **/ -void dump_arg_map(std::ostream &os, const std::string &prefix, const http::arg_view_map &map); +void dump_arg_map(std::ostream& os, const std::string& prefix, const http::arg_view_map& map); /** * Process escape sequences ('+'=space, %HH) Updates val in place; the diff --git a/test/integ/file_upload.cpp b/test/integ/file_upload.cpp index 1efbb13b..2082f9c7 100644 --- a/test/integ/file_upload.cpp +++ b/test/integ/file_upload.cpp @@ -143,7 +143,7 @@ class print_file_upload_resource : public http_resource { content = req.get_content(); auto args_view = req.get_args(); // req may go out of scope, so we need to copy the values. - for (auto const & item : args_view) { + for (auto const& item : args_view) { args[std::string(item.first)] = std::string(item.second); } files = req.get_files(); @@ -155,7 +155,7 @@ class print_file_upload_resource : public http_resource { content = req.get_content(); auto args_view = req.get_args(); // req may go out of scope, so we need to copy the values. - for (auto const & item : args_view) { + for (auto const& item : args_view) { args[std::string(item.first)] = std::string(item.second); } files = req.get_files();