5
\$\begingroup\$

I am writing an HTTP message coder/decoder. In my decoder I parse http messages into a http_request or http_response. In my encoder I encode one of these classes.

Representing the messages is the easy part, but I am sure I have made some mistakes. Please review and let me know what could be improved.

Some questions:

  1. I make the base class http_message available which is probably not ideal. Should I hide this away in a different header? Or hide some other way? any ideas?

  2. I don't think I need copy and assignment operators because i am not using pointers. Is that correct?

  3. Is the testing sufficient?

Header file, http_message.hpp:

#ifndef HTTP_MESSAGE_HPP_
#define HTTP_MESSAGE_HPP_

#include <string>
#include <unordered_map>
#include <iostream>

class http_message {
public:
    http_message();
    void set_version(int major, int minor);
    const std::string get_version() const;
    size_t get_body_length() const;
    void body(const std::string& data);
    std::string body() const;
    void add_header(const std::string& key, const std::string& value);
    const std::string get_header_value(const std::string& key) const;

    typedef const std::unordered_map<std::string, std::string>::const_iterator const_iterator;
    const_iterator begin() const { return headers_.begin(); }
    const_iterator end() const { return headers_.end(); }

protected:
    void set_body_length(size_t length);
    std::unordered_map<std::string, std::string> headers_;
    std::string body_;
    std::string version_;
};

class http_response : public http_message {
public:
    unsigned status = 0;
};

class http_request : public http_message {
public:
    std::string url;
    std::string method;
    std::string query;
};

std::ostream& operator<<(std::ostream& os, const http_request& request);
std::ostream& operator<<(std::ostream& os, const http_response& request);

#endif // HTTP_MESSAGE_HPP_

http_message.cpp:

#include <algorithm>

#include "http_message.hpp"

static const std::string content_length_string("Content-Length");

static bool case_insensitive_match(std::string s1, std::string s2) {
    // Convert complete given string to lower case
    std::transform(s1.begin(), s1.end(), s1.begin(), ::tolower);
    // Convert complete given sub-string to lower case
    std::transform(s2.begin(), s2.end(), s2.begin(), ::tolower);
    return s1.find(s2) == 0;  // must be found at start of string
}

http_message::http_message() : version_("HTTP/1.1") {}

void http_message::set_version(int major, int minor) {
    version_ = "HTTP/" + std::to_string(major) + '.' + std::to_string(minor);
}

size_t http_message::get_body_length() const {
    return body_.length();
}

void http_message::set_body_length(size_t length) {
    headers_[content_length_string] = std::to_string(length);
}

void http_message::add_header(const std::string& key, const std::string& value) {

    // check for Content-Length - fix header key as Content-Length to ease checking header
    if (case_insensitive_match(key, content_length_string)) {
        headers_[content_length_string] = value;
    }
    else {
        headers_[key] = value;
    }
}

const std::string http_message::get_version() const {
    return version_;
}

void http_message::body(const std::string& data) {
    body_ = data;
    if (body_.length() > 0) {
        headers_[content_length_string] = std::to_string(body_.length());
    }
    else {
        headers_.erase(content_length_string);
    }
}

std::string http_message::body() const {
    return body_;
}

const std::string http_message::get_header_value(const std::string& key) const {
    const auto it = headers_.find(key);
    return it != headers_.end() ? it->second : "";
}

std::ostream& operator<<(std::ostream& os, const http_request& request) {
    os << "Method=" << request.method << std::endl;
    os << "url: " << request.url << std::endl;
    if (request.get_body_length() > 0) {
        os << "body: " << request.body() << std::endl;
    }
    if (!request.query.empty()) {
        os << "query: " << request.query << std::endl;
    }

    // print all headers
    for (const auto& header : request) {
        os << header.first << ": " << header.second << std::endl;
    }
    return os;
}

std::ostream& operator<<(std::ostream& os, const http_response& response) {
    os << "Status=" << response.status << std::endl;
    if (response.get_body_length() > 0) {
        os << "body: " << response.body() << std::endl;
    }

    // print all headers
    for (const auto& header : response) {
        os << header.first << ": " << header.second << std::endl;
    }
    return os;
}

Some testing: test.cpp:

#include "gtest/gtest.h"

#include "http_message.hpp"

// consider having getter functions to give user no. headers
static size_t count_headers(const http_request& request) {
    int count(0);
    for (const auto& header : request) {
        count++;
    }
    return count;
}

TEST(http_message_tests, message_length_calculated_correctly) {
    http_request rq;
    rq.add_header("Content-Type", "text/plain");
    rq.body("Text message");
    EXPECT_EQ(rq.get_header_value("Content-Length"), "12");
    EXPECT_EQ(rq.get_body_length(), 12);
}

TEST(http_message_tests, message_length_with_no_body_zero) {
    http_request rq;
    rq.add_header("Content-Type", "text/plain");
    EXPECT_EQ(rq.get_header_value("Content-Length"), "");
    EXPECT_EQ(rq.get_body_length(), 0);
}

TEST(http_message_tests, message_without_version_set_defaults_to_http_v1_1) {
    http_request rq;
    EXPECT_EQ(rq.get_version(), "HTTP/1.1");
}

TEST(http_message_tests, message_version_correctly_set) {
    http_request rq;
    rq.set_version(1, 0);
    EXPECT_EQ(rq.get_version(), "HTTP/1.0");
}

TEST(http_message_tests, message_header_correctly_set) {
    http_request rq;
    rq.add_header("Content-Type", "text/plain");
    EXPECT_EQ(rq.get_header_value("Content-Type"), "text/plain");
}

TEST(http_message_tests, http_response_correctly_default_initialised) {
    http_response rs;
    EXPECT_EQ(rs.get_header_value("Content-Length"), "");
    EXPECT_EQ(rs.status, 0u);
    EXPECT_EQ(rs.get_version(), "HTTP/1.1");
}

TEST(http_message_tests, http_request_correctly_default_initialised) {
    http_request rq;
    EXPECT_EQ(rq.get_header_value("Content-Length"), "");
    EXPECT_EQ(rq.get_version(), "HTTP/1.1");
    EXPECT_EQ(rq.get_body_length(), 0);
    EXPECT_EQ(count_headers(rq), 0);
}

TEST(http_message_tests, http_response_headers_added_correctly) {
    http_response rs;
    EXPECT_EQ(rs.get_header_value("Content-Length"), "");
    EXPECT_EQ(rs.status, 0u);
    EXPECT_EQ(rs.get_version(), "HTTP/1.1");
}

TEST(http_message_tests, http_request_headers_added_correctly) {
    http_request rq;
    rq.method = "GET";
    rq.url = "/";
    rq.set_version(1, 1);
    rq.add_header("Host", "localhost");
    rq.add_header("Connection", "keep-alive");

    rq.add_header("Upgrade-Insecure-Requests", "1");
    rq.add_header("User-Agent", "Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML,like Gecko) Chrome/76.0.3809.132 Safari/537.36");
    rq.add_header("Sec-Fetch-Mode", "navigate");
    rq.add_header("Sec-Fetch-User", "?1");
    rq.add_header("Accept", "text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3");
    rq.add_header("Sec-Fetch-Site", "none");
    rq.add_header("Accept-Encoding", "gzip, deflate, br");
    rq.add_header("Accept-Language", "en-US,en;q=0.9");

    EXPECT_EQ(rq.url, "/");
    EXPECT_EQ(rq.get_version(), "HTTP/1.1");
    EXPECT_EQ(rq.method, "GET");
    EXPECT_EQ(rq.query, "");
    EXPECT_EQ(rq.get_header_value("Content-Length"), "");
    EXPECT_EQ(rq.get_header_value("Host"), "localhost");
    EXPECT_EQ(rq.get_header_value("Connection"), "keep-alive");
    EXPECT_EQ(rq.get_header_value("Upgrade-Insecure-Requests"), "1");
    EXPECT_EQ(rq.get_header_value("User-Agent"), "Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML,like Gecko) Chrome/76.0.3809.132 Safari/537.36");
    EXPECT_EQ(rq.get_header_value("Sec-Fetch-Mode"), "navigate");
    EXPECT_EQ(rq.get_header_value("Sec-Fetch-User"), "?1");
    EXPECT_EQ(rq.get_header_value("Accept"), "text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3");
    EXPECT_EQ(rq.get_header_value("Sec-Fetch-Site"), "none");
    EXPECT_EQ(rq.get_header_value("Accept-Encoding"), "gzip, deflate, br");
    EXPECT_EQ(rq.get_header_value("Accept-Language"), "en-US,en;q=0.9");
    EXPECT_EQ(rq.body(), "");

    EXPECT_EQ(count_headers(rq), 10);
}

TEST(http_message_tests, http_request_headers_length_header_automatically_added) {
    http_request rq;
    rq.method = "POST";
    rq.url = "/welcome.php";
    rq.set_version(1, 0);
    rq.add_header("Host", "www.iteloffice.com");
    rq.add_header("Content-Type", "application/x-www-form-urlencoded");
    std::string body("name=Joe+Bloggs&email=joe%40bloggs.com");
    rq.body(body);

    EXPECT_EQ(rq.url, "/welcome.php");
    EXPECT_EQ(rq.get_version(), "HTTP/1.0");
    EXPECT_EQ(rq.method, "POST");
    EXPECT_EQ(rq.query, "");
    EXPECT_EQ(rq.get_header_value("Content-Length"), std::to_string(body.length()));
    EXPECT_EQ(rq.get_header_value("Host"), "www.iteloffice.com");
    EXPECT_EQ(rq.get_header_value("Content-Type"), "application/x-www-form-urlencoded");
    EXPECT_EQ(rq.body(), body);

    EXPECT_EQ(count_headers(rq), 3);  // Content-Length automatically added
}
\$\endgroup\$
2
  • \$\begingroup\$ Did you know the existence of boost beast? \$\endgroup\$ Commented Nov 16, 2019 at 23:21
  • \$\begingroup\$ Is there any possibility you could add "gtest/gtest.h" to the question? \$\endgroup\$ Commented Nov 18, 2019 at 16:42

2 Answers 2

Reset to default
1
\$\begingroup\$

I'll just go from top to bottom, not in the order of importance:

  1. size_t get_body_length() const; does not make much sense. Also not clear what it does. If it is the length of body, why not just body().size()? If this is Content-Length, why is it called body length and what's wrong with getting it from the headers?
  2. void body(const std::string& data) name is inconsistent with other set_* functions. In general, prefer verb-named functions
  3. In get_header_value prefer optional to returning a special value like an empty string
  4. The ability to iterate a http_message as a list of headers is counter-intuitive to say the least.
  5. You might want to consider adding named constants for response codes and HTTP verbs
  6. http_message::add_header treats Content-length as case-insensitive, while other headers as case-sensitive, which violates HTTP standard and is counter-intuitive to anyone not familiar with the internal implementation of your class
  7. case_insensitive_match, though probably unneeded, I'd still like to point out that it can be simplified in several ways. One of them is using std::equal/std::mismatch with a predicate which would give you a no-copy solution
  8. Probably most important is that I don't see a use-case for this class as it is because it lacks a lot of features like serializing/deserializing to wire format.
\$\endgroup\$
2
  • \$\begingroup\$ great feedback, thanks. I am writing a separate encoder/decoder. Any more hints on how to write the case-insensitive matching with std::equal/std::mismatch? \$\endgroup\$ Commented Nov 19, 2019 at 13:09
  • \$\begingroup\$ @arcomber, something like this: return (s1.size() == s2.size()) && std::equal(s1.begin(), s1.end(), s2.begin(), [](char x, char y){ return std::tolower(x) == std::tolower(y); }); \$\endgroup\$ Commented Nov 19, 2019 at 13:22
2
\$\begingroup\$

I make the base class http_message available which is probably not ideal. Should I hide this away in a different header? Or hide some other way? any ideas?

Each class should have it's own header file. Right now if any of the 3 classes are edited all files need to recompile, if each class had it's own header file, the only time every file needs to recompile is when the http_message class is modified. The overrides for the << operator should be defined for http_response in the http_response header file and for the http_request in the http_request header file.

Is the testing sufficient?

Since the "gtest/gtest.h" file was not provided I was unable to run the unit tests, however, there are some logical extensions: Since http_message is not an abstract class there should be some unit tests for http_message that are independent of the http_request and http_response unit tests. The http_message unit tests should precede the other unit tests so that if the http_message unit tests fail the other tests are not executed.

Unused Protected Function

Neither http_request nor http_response are currently using the protected functionhttp_message::set_body_length(size_t length)so it is unclear that this function is needed, since it also isn't used byhttp_message`.

Avoid Using Macros in C++ When Possible.

It is unclear that macros are used in the unit tests since "gtest/gtest.h" is not provided, however, TEST() and EXPECT_EQ() appears to be macros. Macros are not type safe and can might not be the best option. This may cause the unit testing to be less accurate than it could be.

Perhaps a unit test class might be better.

You might also want to look into using cppunit. This would tell you how much of the source code is getting test coverage.

Possible Optimizations

There are many functions in http_message that could be inlined. That means that the body of the functions could also be in http_message.hpp. In line function may be optized and speed up the code or make the executable code smaller. Candidates for inlining are - void set_version(int major, int minor)
- const std::string get_version()
- std::string body()
- get_header_value(const std::string& key)

\$\endgroup\$
3
  • 2
    \$\begingroup\$ For what it's worth, gtest/gtest.h, TEST and EXPECT_EQ are almost certainly referring to Google Test. I generally prefer it over cppunit (though I prefer Catch2 over either). Most unit tests use macros so (among other things) __FILE__ and __LINE__ will expand to the location of a failing test. \$\endgroup\$ Commented Nov 19, 2019 at 6:33
  • \$\begingroup\$ @JerryCoffin Thanks for the clarification. \$\endgroup\$ Commented Nov 19, 2019 at 15:18
  • \$\begingroup\$ Modern compilers can inline functions across compilation units. Moving the source to the header file is less desirable then simply allowing the compiler to do its job. \$\endgroup\$ Commented Nov 20, 2019 at 9:52

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.