Commit 5f97c454 authored by Sophie Wenzel-Teuber's avatar Sophie Wenzel-Teuber
Browse files

Fix a bug in S3 header

get body length was throwing an exception when the Content-Length header was missing

Changelog:
* Add more error checks to s3 header class
* Add unit tests for s3 header class covering all typical use cases
* Add check for Markdown documentation to gitlab CI
parent 2238f58c
Pipeline #1863 passed with stages
in 8 minutes and 31 seconds
......@@ -110,6 +110,10 @@ Documentation:
script:
- *configure
- make -C build/doc
&& git status
&& git diff-index --quiet HEAD
|| { echo "This commit contains changes to the documentation! Run doxygen-XML-parser on the project update the documentation."; false; }
Default:
<<: *default_job
......
......@@ -21,7 +21,6 @@ class to extract the S3 specific information from proxygens HTTPMessage headers
| [is_bucket_only_request](#deimos-s3_utilities-S3_header-is_bucket_only_request) | checks if the message we got is only concerning a bucket or a key inside a bucket |
| [get_body_length](#deimos-s3_utilities-S3_header-get_body_length) | gets the total length of all body chunks that is expected |
| [get_meta_data](#deimos-s3_utilities-S3_header-get_meta_data) | gets the S3 metadata stored in the HTTP headers |
| [print_all_headers](#deimos-s3_utilities-S3_header-print_all_headers) | print all headers to std out |
| [get_header_by_name](#deimos-s3_utilities-S3_header-get_header_by_name) | extract one header value from the headers |
| [get_queries](#deimos-s3_utilities-S3_header-get_queries) | extract the query string from the headers |
| [get_method](#deimos-s3_utilities-S3_header-get_method) | get the HTTP method the header belongs to |
......@@ -245,24 +244,6 @@ In S3 arbitrary metadata can be defined. This has the form x-amx-meta-KEY: VALUE
#### Qualifiers:
* const
* inline
[Go to Top](#deimos-s3_utilities-S3_header)
### <a name='deimos-s3_utilities-S3_header-print_all_headers' /> public void deimos::s3_utilities::S3_header::print_all_headers () const
print all headers to std out
#### Qualifiers:
* const
* inline
......
......@@ -17,6 +17,7 @@ class for handling command line arguments and configuration files
| [m_hostname](#deimos-util-Config-m_hostname) | IP/Hostname to bind to. |
| [m_thread_count](#deimos-util-Config-m_thread_count) | Number of threads to listen on. |
| [m_config_filename](#deimos-util-Config-m_config_filename) | filename to write configuration options to |
| [m_log_filename](#deimos-util-Config-m_log_filename) | filename to write logs to |
## Public Functions
......@@ -162,6 +163,19 @@ filename to write configuration options to
[Go to Top](#deimos-util-Config)
### <a name='deimos-util-Config-m_log_filename' /> private deimos::util::Config::m_log_filename = "/tmp/deimos_log.txt"
filename to write logs to
[Go to Top](#deimos-util-Config)
## Public Functions
......
......@@ -28,7 +28,6 @@ void GetRequestHandler::onRequest(
spdlog::info("bucket: {}", m_s3_header.get_bucket());
spdlog::info("key: {}", m_s3_header.get_key_without_bucket());
// spdlog::debug("body length: {}", m_s3_header.get_body_length());
spdlog::debug("queries: {}", m_s3_header.get_queries());
#ifndef DEIMOS_NO_AUTHORISATION
......
......@@ -33,7 +33,7 @@ void PutRequestHandler::onRequest(
spdlog::info("bucket: {}", m_s3_header.get_bucket());
spdlog::info("key: {}", m_s3_header.get_key_without_bucket());
// spdlog::debug("body length: {}", m_s3_header.get_body_length());
spdlog::debug("body length: {}", m_s3_header.get_body_length());
spdlog::debug("queries: {}", m_s3_header.get_queries());
#ifndef DEIMOS_NO_AUTHORISATION
......
......@@ -9,6 +9,8 @@
#include <string>
#include <utility>
#include <spdlog/spdlog.h>
namespace deimos {
namespace s3_utilities {
......@@ -122,10 +124,24 @@ class S3_header {
///
size_t get_body_length() const
{
// TODO fixme
std::string content_length =
if (!m_headers) {
return 0;
}
const std::string content_length =
m_headers->getHeaders().getSingleOrEmpty("Content-Length");
return static_cast<size_t>(std::stoi(content_length));
if (content_length.empty()) {
return 0;
}
try {
int content_length_int = std::stoi(content_length);
return static_cast<size_t>(content_length_int);
}
catch (...) {
spdlog::warn("Could not convert body length {}", content_length);
return 0;
}
}
///
......@@ -139,6 +155,10 @@ class S3_header {
///
std::map<std::string, std::string> get_meta_data() const
{
if (!m_headers) {
return std::map<std::string, std::string>();
}
std::map<std::string, std::string> metadata;
std::string meta_search_string = "x-amz-meta-";
......@@ -163,6 +183,9 @@ class S3_header {
///
std::string get_header_by_name(const std::string header_name) const
{
if (!m_headers) {
return "";
}
return m_headers->getHeaders().getSingleOrEmpty(header_name);
}
......@@ -172,14 +195,26 @@ class S3_header {
/// @returns the query string of the request or an empty_string if it
/// doesn't have one
///
std::string get_queries() const { return m_headers->getQueryString(); }
std::string get_queries() const
{
if (!m_headers) {
return "";
}
return m_headers->getQueryString();
}
///
/// @brief get the HTTP method the header belongs to
///
/// @returns the method as a string
///
std::string get_method() const { return m_headers->getMethodString(); }
std::string get_method() const
{
if (!m_headers) {
return "";
}
return m_headers->getMethodString();
}
private:
/// @brief inner pointer to the proxygen header object
......
......@@ -9,6 +9,7 @@ add_executable(
phobos_file.cc
fifo.cc
s3_utilities.cc
s3_header.cc
storage/disk.cc
utils.cc
......
#include <catch2/catch.hpp>
#include <fstream>
#include "../../src/server/s3_utilities/s3_header.h"
namespace deimos {
namespace s3_utilities {
SCENARIO("S3 header", "[header]")
{
std::unique_ptr<proxygen::HTTPMessage> message =
std::make_unique<proxygen::HTTPMessage>();
proxygen::HTTPHeaders& proxygen_headers = message->getHeaders();
S3_header header;
GIVEN("an empty s3 header object")
{
WHEN("No proxygen header is set")
{
THEN("get_bucket returns an empty string")
{
REQUIRE(header.get_bucket().empty());
}
THEN("get_key_without_bucket returns an empty string")
{
REQUIRE(header.get_key_without_bucket().empty());
}
THEN("get_key returns an empty string")
{
REQUIRE(header.get_key().empty());
}
THEN("get_header_by_name returns an empty string")
{
REQUIRE(header.get_header_by_name("some-name").empty());
}
THEN("get_queries returns an empty string")
{
REQUIRE(header.get_queries().empty());
}
THEN("get_method returns an empty string")
{
REQUIRE(header.get_method().empty());
}
THEN("is_bucket_only_request returns false")
{
REQUIRE(!header.is_bucket_only_request());
}
THEN("get_body_length returns 0")
{
REQUIRE(header.get_body_length() == 0);
}
THEN("get_meta_data returns an empty map")
{
REQUIRE(header.get_meta_data().empty());
}
}
}
GIVEN("an object with a URL")
{
WHEN("There is no key")
{
std::string bucketname = "bucket";
message->setURL("/bucket");
header.set_headers(std::move(message));
THEN("get_bucket returns the bucket")
{
REQUIRE(header.get_bucket() == bucketname);
}
THEN("get_key_without_bucket returns an empty string")
{
REQUIRE(header.get_key_without_bucket().empty());
}
THEN("get_key returns the bucket")
{
REQUIRE(header.get_key() == bucketname);
}
THEN("is_bucket_only_request returns true")
{
REQUIRE(header.is_bucket_only_request());
}
}
WHEN("There is a bucket and a key")
{
std::string bucketname = "bucket";
std::string keyname = "key";
message->setURL("/bucket/key");
header.set_headers(std::move(message));
THEN("get_bucket returns the bucket")
{
REQUIRE(header.get_bucket() == bucketname);
}
THEN("get_key_without_bucket returns the key")
{
REQUIRE(header.get_key_without_bucket() == keyname);
}
THEN("get_key returns the bucket and the key")
{
std::stringstream ss;
ss << bucketname << '/' << keyname;
REQUIRE(header.get_key() == ss.str());
}
THEN("is_bucket_only_request returns false")
{
REQUIRE(!header.is_bucket_only_request());
}
}
}
GIVEN("an s3 header object with empty proxygen headers")
{
header.set_headers(std::move(message));
WHEN("Content-Length is accessed")
{
THEN("get_body_length returns 0")
{
REQUIRE(header.get_body_length() == 0);
}
}
WHEN("metadata is accessed")
{
THEN("an empty map is returned")
{
REQUIRE(header.get_meta_data().empty());
}
}
WHEN("a named header is accessed")
{
std::string requested_header = "requested";
THEN("get_header_by_name returns an empty string")
{
REQUIRE(header.get_header_by_name(requested_header).empty());
}
}
WHEN("Query string is accessed")
{
THEN("get_queries returns an empty string")
{
REQUIRE(header.get_queries().empty());
}
}
WHEN("the method is accessed")
{
THEN("get_method returns an empty string")
{
REQUIRE(header.get_method().empty());
}
}
}
GIVEN("a completely filled s3 header object")
{
size_t content_length = 32;
proxygen_headers.add("Content-Length", std::to_string(content_length));
proxygen_headers.add("x-amz-meta-a", "a");
proxygen_headers.add("x-amz-meta-b", "b");
std::string requested_header = "requested";
std::string header_value = "header_value";
proxygen_headers.add(requested_header, header_value);
message->setURL("testURL");
REQUIRE(message->setQueryParam("query_one", "1"));
REQUIRE(message->setQueryParam("query_two", "2"));
message->setMethod(proxygen::HTTPMethod::GET);
header.set_headers(std::move(message));
WHEN("Content-Length is accessed")
{
THEN("get_body_length returns the correct value")
{
REQUIRE(header.get_body_length() == content_length);
}
}
WHEN("Metadata is accessed")
{
THEN("a corresponding map is returned")
{
std::map<std::string, std::string> metadata =
header.get_meta_data();
REQUIRE(metadata["a"] == "a");
REQUIRE(metadata["b"] == "b");
}
}
WHEN("a named header is accessed")
{
THEN("get_header_by_name returns an empty string")
{
REQUIRE(
header.get_header_by_name(requested_header)
== header_value);
}
}
WHEN("Query string is accessed")
{
THEN("get_queries returns a query string")
{
REQUIRE(header.get_queries() == "query_one=1&query_two=2");
}
}
WHEN("the method is accessed")
{
THEN("get_queries returns GET")
{
REQUIRE(header.get_method() == "GET");
}
}
}
GIVEN("headers are inconvertable")
{
proxygen_headers.add("Content-Length", "inconveritble");
header.set_headers(std::move(message));
WHEN("Content-Length is accessed")
{
THEN("get_body_length returns 0")
{
REQUIRE(header.get_body_length() == 0);
}
}
}
}
} // namespace s3_utilities
} // namespace deimos
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment