Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix #292 change get_args to return mutliple values per key #293

Closed
wants to merge 1 commit into from
Closed

fix #292 change get_args to return mutliple values per key #293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 22 additions & 5 deletions src/http_request.cpp
View file
Open in desktop
Expand Up @@ -105,20 +105,31 @@ const http::header_view_map http_request::get_cookies() const {
}

std::string_view http_request::get_arg(std::string_view key) const {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since get_arg() now returns only the first matching item (which does keep the API the same), should there also be a get_args() method that returns a std::span<const std::string> or whatever type makes the least memory allocations? IIRC a span<const T> can be made from a vector<T> implicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

afaik span is C++20, so this would require bumping the min version from 17.

std::map<std::string, std::string>::const_iterator it = cache->unescaped_args.find(std::string(key));
auto const it = cache->unescaped_args.find(std::string(key));

if (it != cache->unescaped_args.end()) {
return it->second;
return it->second.front();
}

return get_connection_value(key, MHD_GET_ARGUMENT_KIND);
}

static void fill_arg_view_map(http::arg_view_map *arguments, const http::arg_map& cached_map) {
for (const auto& kv : cached_map) {
std::vector<std::string_view> vec;
vec.reserve(kv.second.size());
for (const auto& value : kv.second) {
vec.push_back(value);
}
arguments->emplace(std::string_view(kv.first), std::move(vec));
}
}

const http::arg_view_map http_request::get_args() const {
http::arg_view_map arguments;

if (!cache->unescaped_args.empty()) {
arguments.insert(cache->unescaped_args.begin(), cache->unescaped_args.end());
fill_arg_view_map(&arguments, cache->unescaped_args);
return arguments;
}

Expand All @@ -128,7 +139,7 @@ const http::arg_view_map http_request::get_args() const {

MHD_get_connection_values(underlying_connection, MHD_GET_ARGUMENT_KIND, &build_request_args, reinterpret_cast<void*>(&aa));

arguments.insert(cache->unescaped_args.begin(), cache->unescaped_args.end());
fill_arg_view_map(&arguments, cache->unescaped_args);

return arguments;
}
Expand All @@ -155,7 +166,13 @@ MHD_Result http_request::build_request_args(void *cls, enum MHD_ValueKind kind,
std::string value = ((arg_value == nullptr) ? "" : arg_value);

http::base_unescaper(&value, aa->unescaper);
(*aa->arguments)[key] = value;
auto existing_iter = aa->arguments->find(key);
if (existing_iter != aa->arguments->end()) {
// Key exists, add value to collection instead of overwriting previous value.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whether the key exists or not, I think this if/else equivalent to just:

(*aa->arguments)[key].push_back(value);

existing_iter->second.push_back(value);
} else {
(*aa->arguments)[key] = {value};
}
return MHD_YES;
}

Expand Down
28 changes: 26 additions & 2 deletions src/http_utils.cpp
View file
Open in desktop
Expand Up @@ -525,11 +525,35 @@ 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<http::header_view_map>(os, prefix, map);
auto it = map.begin();
auto end = map.end();

if (map.size()) {
os << " " << prefix << " [";
for (; it != end; ++it) {
os << (*it).first << ":\"" << (*it).second << "\" ";
}
os << "]" << std::endl;
}
}

void dump_arg_map(std::ostream& os, const std::string& prefix, const http::arg_view_map &map) {
dump_map<http::arg_view_map>(os, prefix, map);
auto it = map.begin();
auto end = map.end();

if (map.size()) {
os << " " << prefix << " [";
for (; it != end; ++it) {
os << (*it).first << ":[";
std::string delim = "";
for (const auto & v : it->second) {
os << delim << "\"" << v << "\"";
delim = ", ";
}
os << "] ";
}
os << "]" << std::endl;
}
}

size_t base_unescaper(std::string* s, unescaper_ptr unescaper) {
Expand Down
30 changes: 22 additions & 8 deletions src/httpserver/http_request.hpp
View file
Open in desktop
Expand Up @@ -173,8 +173,8 @@ class http_request {

/**
* 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.
* @param key the specific argument to get the value from
* @return the value of the first arg matching the key.
**/
std::string_view get_arg(std::string_view key) const;

Expand Down Expand Up @@ -298,7 +298,12 @@ class http_request {
* @param value The value assumed by the argument
**/
void set_arg(const std::string& key, const std::string& value) {
cache->unescaped_args[key] = value.substr(0, content_size_limit);
auto existing_iter = cache->unescaped_args.find(key);
if (existing_iter != cache->unescaped_args.end()) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly I think this is also just cache->unescaped_args[key].push_back(value.substr(0, content_size_limit));, and below.

existing_iter->second.push_back(value.substr(0, content_size_limit));
} else {
cache->unescaped_args[key] = {value.substr(0, content_size_limit)};
}
}

/**
Expand All @@ -308,7 +313,13 @@ 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) {
cache->unescaped_args[key] = std::string(value, std::min(size, content_size_limit));
auto value_str = std::string(value, std::min(size, content_size_limit));
auto existing_iter = cache->unescaped_args.find(key);
if (existing_iter != cache->unescaped_args.end()) {
existing_iter->second.push_back(value_str);
} else {
cache->unescaped_args[key] = {value_str};
}
}

/**
Expand Down Expand Up @@ -365,10 +376,13 @@ class http_request {
* Method used to set all arguments of the request.
* @param args The args key-value map to set for the request.
**/
void set_args(const std::map<std::string, std::string>& args) {
std::map<std::string, std::string>::const_iterator it;
for (it = args.begin(); it != args.end(); ++it) {
this->cache->unescaped_args[it->first] = it->second.substr(0, content_size_limit);
void set_args(const std::map<std::string, std::vector<std::string>>& args) {
for (auto it = args.begin(); it != args.end(); ++it) {
auto value_vec = it->second;
std::transform(value_vec.begin(), value_vec.end(), value_vec.begin(), [this](const std::string& s) {
return s.substr(0, content_size_limit);
});
this->cache->unescaped_args[it->first] = std::move(value_vec);
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/httpserver/http_utils.hpp
View file
Open in desktop
Expand Up @@ -318,8 +318,8 @@ class arg_comparator {

using header_map = std::map<std::string, std::string, http::header_comparator>;
using header_view_map = std::map<std::string_view, std::string_view, http::header_comparator>;
using arg_map = std::map<std::string, std::string, http::arg_comparator>;
using arg_view_map = std::map<std::string_view, std::string_view, http::arg_comparator>;
using arg_map = std::map<std::string, std::vector<std::string>, http::arg_comparator>;
using arg_view_map = std::map<std::string_view, std::vector<std::string_view>, http::arg_comparator>;

struct ip_representation {
http_utils::IP_version_T ip_version;
Expand Down
9 changes: 6 additions & 3 deletions test/integ/basic.cpp
View file
Open in desktop
Expand Up @@ -116,7 +116,10 @@ class header_reading_resource : public http_resource {
class full_args_resource : public http_resource {
public:
shared_ptr<http_response> render_GET(const http_request& req) {
return shared_ptr<string_response>(new string_response(std::string(req.get_args().at("arg")), 200, "text/plain"));
const auto args = req.get_args();
const auto arg = args.at("arg");
const auto resp = std::string(arg[0]) + std::string(arg[1]);
return shared_ptr<string_response>(new string_response(resp, 200, "text/plain"));
}
};

Expand Down Expand Up @@ -772,13 +775,13 @@ LT_BEGIN_AUTO_TEST(basic_suite, full_arguments_processing)
string s;
CURL *curl = curl_easy_init();
CURLcode res;
curl_easy_setopt(curl, CURLOPT_URL, "localhost:8080/this/captures/args/passed/in/the/querystring?arg=argument");
curl_easy_setopt(curl, CURLOPT_URL, "localhost:8080/this/captures/args/passed/in/the/querystring?arg=argument&arg=another");
curl_easy_setopt(curl, CURLOPT_HTTPGET, 1L);
curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, writefunc);
curl_easy_setopt(curl, CURLOPT_WRITEDATA, &s);
res = curl_easy_perform(curl);
LT_ASSERT_EQ(res, 0);
LT_CHECK_EQ(s, "argument");
LT_CHECK_EQ(s, "argumentanother");
curl_easy_cleanup(curl);
LT_END_AUTO_TEST(full_arguments_processing)

Expand Down
18 changes: 9 additions & 9 deletions test/integ/file_upload.cpp
View file
Open in desktop
Expand Up @@ -144,7 +144,7 @@ class print_file_upload_resource : public http_resource {
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);
args[std::string(item.first)] = {std::string(item.second.front())};
}
files = req.get_files();
shared_ptr<string_response> hresp(new string_response("OK", 201, "text/plain"));
Expand All @@ -156,7 +156,7 @@ class print_file_upload_resource : public http_resource {
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);
args[std::string(item.first)] = {std::string(item.second.front())};
}
files = req.get_files();
shared_ptr<string_response> hresp(new string_response("OK", 200, "text/plain"));
Expand Down Expand Up @@ -218,7 +218,7 @@ LT_BEGIN_AUTO_TEST(file_upload_suite, file_upload_memory_and_disk)
LT_CHECK_EQ(args.size(), 1);
auto arg = args.begin();
LT_CHECK_EQ(arg->first, TEST_KEY);
LT_CHECK_EQ(arg->second, TEST_CONTENT);
LT_CHECK_EQ(arg->second.front(), TEST_CONTENT);

map<string, map<string, httpserver::http::file_info>> files = resource.get_files();
LT_CHECK_EQ(files.size(), 1);
Expand Down Expand Up @@ -300,10 +300,10 @@ LT_BEGIN_AUTO_TEST(file_upload_suite, file_upload_memory_and_disk_additional_par
LT_CHECK_EQ(args.size(), 2);
auto arg = args.begin();
LT_CHECK_EQ(arg->first, TEST_KEY);
LT_CHECK_EQ(arg->second, TEST_CONTENT);
LT_CHECK_EQ(arg->second.front(), TEST_CONTENT);
arg++;
LT_CHECK_EQ(arg->first, TEST_PARAM_KEY);
LT_CHECK_EQ(arg->second, TEST_PARAM_VALUE);
LT_CHECK_EQ(arg->second.front(), TEST_PARAM_VALUE);

map<string, map<string, httpserver::http::file_info>> files = resource.get_files();
LT_CHECK_EQ(files.size(), 1);
Expand Down Expand Up @@ -354,10 +354,10 @@ LT_BEGIN_AUTO_TEST(file_upload_suite, file_upload_memory_and_disk_two_files)
LT_CHECK_EQ(args.size(), 2);
auto arg = args.begin();
LT_CHECK_EQ(arg->first, TEST_KEY);
LT_CHECK_EQ(arg->second, TEST_CONTENT);
LT_CHECK_EQ(arg->second.front(), TEST_CONTENT);
arg++;
LT_CHECK_EQ(arg->first, TEST_KEY_2);
LT_CHECK_EQ(arg->second, TEST_CONTENT_2);
LT_CHECK_EQ(arg->second.front(), TEST_CONTENT_2);

map<string, map<string, httpserver::http::file_info>> files = resource.get_files();
LT_CHECK_EQ(files.size(), 2);
Expand Down Expand Up @@ -461,7 +461,7 @@ LT_BEGIN_AUTO_TEST(file_upload_suite, file_upload_memory_only_incl_content)
LT_CHECK_EQ(args.size(), 1);
auto arg = args.begin();
LT_CHECK_EQ(arg->first, TEST_KEY);
LT_CHECK_EQ(arg->second, TEST_CONTENT);
LT_CHECK_EQ(arg->second.front(), TEST_CONTENT);

map<string, map<string, httpserver::http::file_info>> files = resource.get_files();
LT_CHECK_EQ(resource.get_files().size(), 0);
Expand Down Expand Up @@ -493,7 +493,7 @@ LT_BEGIN_AUTO_TEST(file_upload_suite, file_upload_memory_only_excl_content)
LT_CHECK_EQ(args.size(), 1);
auto arg = args.begin();
LT_CHECK_EQ(arg->first, TEST_KEY);
LT_CHECK_EQ(arg->second, TEST_CONTENT);
LT_CHECK_EQ(arg->second.front(), TEST_CONTENT);

map<string, map<string, httpserver::http::file_info>> files = resource.get_files();
LT_CHECK_EQ(files.size(), 0);
Expand Down
20 changes: 10 additions & 10 deletions test/unit/http_utils_test.cpp
View file
Open in desktop
Expand Up @@ -618,25 +618,25 @@ 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<std::string_view, std::string_view, httpserver::http::arg_comparator> arg_map;
arg_map["ARG_ONE"] = "VALUE_ONE";
arg_map["ARG_TWO"] = "VALUE_TWO";
arg_map["ARG_THREE"] = "VALUE_THREE";
std::map<std::string_view, std::vector<std::string_view>, httpserver::http::arg_comparator> arg_map;
arg_map["ARG_ONE"] = {"VALUE_ONE", "VALUE_ONE_2"};
arg_map["ARG_TWO"] = {"VALUE_TWO"};
arg_map["ARG_THREE"] = {"VALUE_THREE"};

std::stringstream ss;
httpserver::http::dump_arg_map(ss, "prefix", arg_map);
LT_CHECK_EQ(ss.str(), " prefix [ARG_ONE:\"VALUE_ONE\" ARG_TWO:\"VALUE_TWO\" ARG_THREE:\"VALUE_THREE\" ]\n");
LT_CHECK_EQ(ss.str(), " prefix [ARG_ONE:[\"VALUE_ONE\", \"VALUE_ONE_2\"] ARG_TWO:[\"VALUE_TWO\"] ARG_THREE:[\"VALUE_THREE\"] ]\n");
LT_END_AUTO_TEST(dump_arg_map)

LT_BEGIN_AUTO_TEST(http_utils_suite, dump_arg_map_no_prefix)
std::map<std::string_view, std::string_view, httpserver::http::arg_comparator> arg_map;
arg_map["ARG_ONE"] = "VALUE_ONE";
arg_map["ARG_TWO"] = "VALUE_TWO";
arg_map["ARG_THREE"] = "VALUE_THREE";
std::map<std::string_view, std::vector<std::string_view>, httpserver::http::arg_comparator> arg_map;
arg_map["ARG_ONE"] = {"VALUE_ONE", "VALUE_ONE_2"};
arg_map["ARG_TWO"] = {"VALUE_TWO"};
arg_map["ARG_THREE"] = {"VALUE_THREE"};

std::stringstream ss;
httpserver::http::dump_arg_map(ss, "", arg_map);
LT_CHECK_EQ(ss.str(), " [ARG_ONE:\"VALUE_ONE\" ARG_TWO:\"VALUE_TWO\" ARG_THREE:\"VALUE_THREE\" ]\n");
LT_CHECK_EQ(ss.str(), " [ARG_ONE:[\"VALUE_ONE\", \"VALUE_ONE_2\"] ARG_TWO:[\"VALUE_TWO\"] ARG_THREE:[\"VALUE_THREE\"] ]\n");
LT_END_AUTO_TEST(dump_arg_map_no_prefix)

LT_BEGIN_AUTO_TEST_ENV()
Expand Down