[5088] Proper documentation of the new HTTP classes.

Also, included a couple of modifications in the API.

src/lib/http/date_time.cc

@@ -5,13 +5,10 @@
 // file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 #include <http/date_time.h>
-#include <boost/algorithm/string.hpp>
-#include <boost/date_time/date_facet.hpp>
 #include <boost/date_time/time_facet.hpp>
 #include <boost/date_time/local_time/local_time.hpp>
 #include <sstream>
 
-using namespace boost::gregorian;
 using namespace boost::local_time;
 using namespace boost::posix_time;
 
@@ -57,6 +54,14 @@ HttpDateTime::fromRfc850(const std::string& time_string) {
 
 HttpDateTime
 HttpDateTime::fromAsctime(const std::string& time_string) {
+    // The asctime() puts space instead of leading 0 in days of
+    // month. The %e # formatter of time_input_facet doesn't deal
+    // with this. To deal with this, we make a copy of the string
+    // holding formatted time and replace a space preceding day
+    // number with 0. Thanks to this workaround we can use the
+    // %d formatter which seems to work fine. This has a side
+    // effect of accepting timestamps such as Sun Nov 06 08:49:37 1994,
+    // but it should be ok to be liberal in this case.
     std::string time_string_copy(time_string);
     boost::replace_all(time_string_copy, "  ", " 0");
     return (HttpDateTime(fromString(time_string_copy,
@@ -68,20 +73,25 @@ HttpDateTime::fromAsctime(const std::string& time_string) {
 HttpDateTime
 HttpDateTime::fromAny(const std::string& time_string) {
     HttpDateTime date_time;
+    // Try to parse as a timestamp specified in RFC 1123 format.
     try {
         date_time = fromRfc1123(time_string);
         return (date_time);
     } catch (...) {
+        // Ignore errors, simply try different format.
         ;
     }
 
+    // Try to parse as a timestamp specified in RFC 850 format.
     try {
         date_time = fromRfc850(time_string);
         return (date_time);
     } catch (...) {
+        // Ignore errors, simply try different format.
         ;
     }
 
+    // Try to parse as a timestamp output by asctime() function.
     try {
         date_time = fromAsctime(time_string);
     } catch (...) {
@@ -98,8 +108,12 @@ std::string
 HttpDateTime::toString(const std::string& format,
                        const std::string& method_name) const {
     std::ostringstream s;
+    // Create raw pointer. The output stream will take responsibility for
+    // deleting the object.
     time_facet* df(new time_facet(format.c_str()));
     s.imbue(std::locale(s.getloc(), df));
+
+    // Convert time value to a string.
     s << time_;
     if (s.fail()) {
         isc_throw(HttpTimeConversionError, "unable to convert "
@@ -116,14 +130,20 @@ HttpDateTime::fromString(const std::string& time_string,
                          const std::string& method_name,
                          const bool zone_check) {
     std::istringstream s(time_string);
+    // Create raw pointer. The input stream will take responsibility for
+    // deleting the object.
     time_input_facet* tif(new time_input_facet(format));
     s.imbue(std::locale(s.getloc(), tif));
 
     time_zone_ptr zone(new posix_time_zone("GMT"));
     local_date_time ldt = local_microsec_clock::local_time(zone);
+
+    // Parse the time value. The stream will not automatically detect whether
+    // the zone is GMT. We need to check it on our own.
     s >> ldt;
     if (s.fail() ||
-        (zone_check && (!ldt.zone() || ldt.zone()->std_zone_abbrev() != "GMT"))) {
+        (zone_check && (!ldt.zone() ||
+                        ldt.zone()->std_zone_abbrev() != "GMT"))) {
         isc_throw(HttpTimeConversionError, "unable to parse "
                   << method_name << " time value of '"
                   << time_string << "'");

src/lib/http/date_time.h

@@ -14,46 +14,142 @@
 namespace isc {
 namespace http {
 
+/// @brief Exception thrown when there is an error during time conversion.
+///
+/// The most common reason for this exception is that the unsupported time
+/// format was used as an input to the time parsing functions.
 class HttpTimeConversionError : public Exception {
 public:
     HttpTimeConversionError(const char* file, size_t line, const char* what) :
         isc::Exception(file, line, what) { };
 };
 
+/// @brief This class parses and generates time values used in HTTP.
+///
+/// The HTTP protocol have historically allowed 3 different date/time formats
+/// (see https://www.w3.org/Protocols/rfc2616/rfc2616-sec3.html). These are:
+/// - Sun, 06 Nov 1994 08:49:37 GMT
+/// - Sunday, 06-Nov-94 08:49:37 GMT
+/// - Sun Nov  6 08:49:37 1994
+///
+/// The first format is preferred but implementations must also support
+/// remaining two obsolete formats for compatibility. This class implements
+/// parsers and generators for all three formats. It uses @c boost::posix_time
+/// to represent time and date. It uses @ref boost::date_time::time_facet
+/// and @ref boost::date_time::time_input_facet to generate and parse the
+/// timestamps.
 class HttpDateTime {
 public:
 
+    /// @brief Default constructor.
+    ///
+    /// Sets current universal time as time value.
     HttpDateTime();
 
+    /// @brief Construct from @ref boost::posix_time::ptime object.
+    ///
+    /// @param t time value to be set.
     explicit HttpDateTime(const boost::posix_time::ptime& t);
 
+    /// @brief Returns time encapsulated by this class.
+    ///
+    /// @return @ref boost::posix_time::ptime value encapsulated by the instance
+    /// of this class.
     boost::posix_time::ptime getPtime() const {
         return (time_);
     }
 
+    /// @brief Returns time value formatted as specified in RFC 1123.
+    ///
+    /// @return A string containing time value formatted as
+    /// Sun, 06 Nov 1994 08:49:37 GMT.
     std::string rfc1123Format() const;
 
+    /// @brief Returns time value formatted as specified in RFC 850.
+    ///
+    /// @return A string containing time value formatted as
+    /// Sunday, 06-Nov-94 08:49:37 GMT.
     std::string rfc850Format() const;
 
+    /// @brief Returns time value formatted as output of ANSI C's
+    /// asctime().
+    ///
+    /// @return A string containing time value formatted as
+    /// Sun Nov  6 08:49:37 1994.
     std::string asctimeFormat() const;
 
+    /// @brief Creates an instance from a string containing time value
+    /// formatted as specified in RFC 1123.
+    ///
+    /// @param time_string Input string holding formatted time value.
+    /// @return Instance of @ref HttpDateTime.
+    /// @throw HttpTimeConversionError if provided timestamp has invalid
+    /// format.
     static HttpDateTime fromRfc1123(const std::string& time_string);
 
+    /// @brief Creates an instance from a string containing time value
+    /// formatted as specified in RFC 850.
+    ///
+    /// @param time_string Input string holding formatted time value.
+    /// @return Instance of @ref HttpDateTime.
+    /// @throw HttpTimeConversionError if provided timestamp has invalid
+    /// format.
     static HttpDateTime fromRfc850(const std::string& time_string);
 
+    /// @brief Creates an instance from a string containing time value
+    /// formatted as output from asctime() function.
+    ///
+    /// @param time_string Input string holding formatted time value.
+    /// @return Instance of @ref HttpDateTime.
+    /// @throw HttpTimeConversionError if provided timestamp has invalid
+    /// format.
     static HttpDateTime fromAsctime(const std::string& time_string);
 
+    /// @brief Creates an instance from a string containing time value
+    /// formatted in one of the supported formats.
+    ///
+    /// This method will detect the format of the time value and parse it.
+    /// It tries parsing the value in the following order:
+    /// - a format specified in RFC 1123,
+    /// - a format specified in RFC 850,
+    /// - a format of asctime output.
+    ///
+    /// @param time_string Input string holding formatted time value.
+    /// @return Instance of @ref HttpDateTime.
+    /// @throw HttpTimeConversionError if provided value doesn't match any
+    /// of the supported formats.
     static HttpDateTime fromAny(const std::string& time_string);
 
 private:
 
+    /// @brief Generic method formatting a time value to a specified format.
+    ////
+    /// @param format Time format as accepted by the
+    /// @c boost::date_time::time_facet.
     std::string toString(const std::string& format,
                          const std::string& method_name) const;
 
+    /// @brief Generic method parsing time value and converting it to the
+    /// instance of @c boost::posix_time::ptime.
+    ///
+    /// @param time_string Input string holding formatted time value.
+    /// @param format Time format as accepted by the
+    /// @c boost::date_time::time_input_facet.
+    /// @param method_name Name of the expected format to appear in the error
+    /// message if parsing fails, e.g. RFC 1123, RFC 850 or asctime.
+    /// @param zone_check Indicates if the time zone name should be validated
+    /// during parsing. This should be set to false for the formats which
+    /// lack time zones (e.g. asctime).
+    ///
+    /// @return Instance of the @ref boost::posix_time::ptime created from the
+    /// input string.
+    /// @throw HttpTimeConversionError if provided value doesn't match the
+    /// specified format.
     static boost::posix_time::ptime
     fromString(const std::string& time_string, const std::string& format,
                const std::string& method_name, const bool zone_check = true);
 
+    /// @brief Time value encapsulated by this class instance.
     boost::posix_time::ptime time_;
 
 };

src/lib/http/response.cc

@@ -4,6 +4,7 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
+#include <http/date_time.h>
 #include <http/response.h>
 #include <boost/date_time/local_time/local_time.hpp>
 #include <boost/date_time/time_facet.hpp>
@@ -14,6 +15,7 @@ using namespace isc::http;
 
 namespace {
 
+/// @brief A map of status codes to status names.
 const std::map<HttpStatusCode, std::string> status_code_to_description = {
     { HttpStatusCode::OK, "OK" },
     { HttpStatusCode::CREATED, "Created" },
@@ -33,6 +35,7 @@ const std::map<HttpStatusCode, std::string> status_code_to_description = {
     { HttpStatusCode::SERVICE_UNAVAILABLE, "Service Unavailable" }
 };
 
+/// @brief New line (CRLF).
 const std::string crlf = "\r\n";
 
 }
@@ -41,10 +44,15 @@ namespace isc {
 namespace http {
 
 HttpResponse::HttpResponse(const HttpVersion& version,
-                           const HttpStatusCode& status_code)
+                           const HttpStatusCode& status_code,
+                           const CallSetGenericBody& generic_body)
     : http_version_(version), status_code_(status_code), headers_(),
       body_() {
-    setGenericBody(status_code);
+    if (generic_body.set_) {
+        // This currently does nothing, but it is useful to have it here as
+        // an example how to implement it in the derived classes.
+        setGenericBody(status_code);
+    }
 }
 
 void
@@ -54,12 +62,14 @@ HttpResponse::setBody(const std::string& body) {
 
 bool
 HttpResponse::isClientError(const HttpStatusCode& status_code) {
+    // Client errors have status codes of 4XX.
     uint16_t c = statusCodeToNumber(status_code);
     return ((c >= 400) && (c < 500));
 }
 
 bool
 HttpResponse::isServerError(const HttpStatusCode& status_code) {
+    // Server errors have status codes of 5XX.
     uint16_t c = statusCodeToNumber(status_code);
     return ((c >= 500) && (c < 600));
 }
@@ -82,35 +92,43 @@ HttpResponse::statusCodeToNumber(const HttpStatusCode& status_code) {
 
 std::string
 HttpResponse::getDateHeaderValue() const {
-    local_date_time t(local_sec_clock::local_time(time_zone_ptr()));
-    std::stringstream s;
-    local_time_facet* lf(new local_time_facet("%a, %d %b %Y %H:%M:%S GMT"));
-    s.imbue(std::locale(s.getloc(), lf));
-    s << t;
-
-    return (s.str());
+    // This returns current time in the recommended format.
+    HttpDateTime date_time;
+    return (date_time.rfc1123Format());
 }
 
 std::string
 HttpResponse::toString() const {
     std::ostringstream s;
+    // HTTP version number and status code.
     s << "HTTP/" << http_version_.first << "." << http_version_.second;
     s << " " << static_cast<uint16_t>(status_code_);
     s << " " << statusCodeToString(status_code_) << crlf;
 
-    for (auto header = headers_.cbegin(); header != headers_.cend();
-         ++header) {
-        s << header->first << ": " << header->second << crlf;
-    }
+    // We need to at least insert "Date" header into the HTTP headers. This
+    // method is const thus we can't insert it into the headers_ map. We'll
+    // work on the copy of the map. Admittedly, we could just append "Date"
+    // into the generated string but we prefer that headers are ordered
+    // alphabetically.
+    std::map<std::string, std::string> headers(headers_);
 
-    s << "Date: " << getDateHeaderValue() << crlf;
+    // Update or add "Date" header.
+    addHeaderInternal("Date", getDateHeaderValue(), headers);
 
+    // Add "Content-Length" if body present.
     if (!body_.empty()) {
-        s << "Content-Length: " << body_.length() << crlf;
+        addHeaderInternal("Content-Length", body_.length(), headers);
+    }
+
+    // Include all headers.
+    for (auto header = headers.cbegin(); header != headers.cend();
+         ++header) {
+        s << header->first << ": " << header->second << crlf;
     }
 
     s << crlf;
 
+    // Include message body.
     s << body_;
 
     return (s.str());

src/lib/http/response.h

@@ -25,6 +25,7 @@ public:
         isc::Exception(file, line, what) { };
 };
 
+/// @brief HTTP status codes.
 enum class HttpStatusCode : std::uint16_t {
     OK = 200,
     CREATED = 201,
@@ -44,6 +45,34 @@ enum class HttpStatusCode : std::uint16_t {
     SERVICE_UNAVAILABLE = 503
 };
 
+/// @brief Encapsulates the boolean value indicating if the @ref HttpResponse
+/// constructor should call its @c setGenericBody method during construction.
+struct CallSetGenericBody {
+
+    /// @brief Contrsuctor.
+    ///
+    /// @param set A boolean value indicating if the method should be called
+    /// or not.
+    explicit CallSetGenericBody(const bool set)
+        : set_(set) {
+    }
+
+    /// @brief Returns encapsulated true.
+    static const CallSetGenericBody& yes() {
+        static CallSetGenericBody yes(true);
+        return (yes);
+    }
+
+    /// @brief Returns encapsulated false.
+    static const CallSetGenericBody& no() {
+        static CallSetGenericBody no(false);
+        return (no);
+    }
+
+    /// @brief A storage for the boolean flag.
+    bool set_;
+};
+
 class HttpResponse;
 
 /// @brief Pointer to the @ref HttpResponse object.
@@ -52,43 +81,153 @@ typedef boost::shared_ptr<HttpResponse> HttpResponsePtr;
 /// @brief Pointer to the const @ref HttpResponse object.
 typedef boost::shared_ptr<const HttpResponse> ConstHttpResponsePtr;
 
+/// @brief Represents HTTP response message.
+///
+/// This class represents HTTP response message. An instance of this object
+/// or its derivation is typically created by the implementation of the
+/// @ref HttpResponseCreator interface.
+///
+/// It contains @c toString method generating a textual representation of
+/// the HTTP response, which is send to the client over TCP socket.
 class HttpResponse {
 public:
 
-    HttpResponse(const HttpVersion& version, const HttpStatusCode& status_code);
-
+    /// @brief Constructor.
+    ///
+    /// Creates basic instance of the object. It sets the HTTP version and the
+    /// status code to be included in the response.
+    ///
+    /// @param version HTTP version.
+    /// @param status_code HTTP status code.
+    /// @param generic_body Indicates if the constructor should call
+    /// @c setGenericBody to create a generic content for the given
+    /// status code. This should be set to "no" when the constructor is
+    /// called by the derived class which provides its own implementation
+    /// of the @c setGenericBody method.
+    explicit HttpResponse(const HttpVersion& version,
+                          const HttpStatusCode& status_code,
+                          const CallSetGenericBody& generic_body =
+                          CallSetGenericBody::yes());
+
+    /// @brief Destructor.
+    ///
+    /// A class having virtual methods must have a virtual destructor.
     virtual ~HttpResponse() { }
 
-    void addHeader(const std::string& name, const std::string& value) {
-        headers_[name] = value;
+    /// @brief Adds HTTP header to the response.
+    ///
+    /// The "Content-Length" and "Date" headers should not be added using this
+    /// method because they are generated and added automatically when the
+    /// @c toString is called.
+    ///
+    /// @param name Header name.
+    /// @param value Header value.
+    /// @tparam ValueType Type of the header value.
+    template<typename ValueType>
+    void addHeader(const std::string& name, const ValueType& value) {
+        addHeaderInternal(name, value, headers_);
     }
 
-    virtual void setBody(const std::string& body);
+    /// @brief Assigns body/content to the message.
+    ///
+    /// @param body Body to be assigned.
+    void setBody(const std::string& body);
 
+    /// @brief Checks if the status code indicates client error.
+    ///
+    /// @param status_code HTTP status code.
+    /// @return true if the status code indicates client error.
     static bool isClientError(const HttpStatusCode& status_code);
 
+    /// @brief Checks if the status code indicates server error.
+    ///
+    /// @param status_code HTTP status code.
+    /// @return true if the status code indicates server error.
     static bool isServerError(const HttpStatusCode& status_code);
 
+    /// @brief Converts status code to string.
+    ///
+    /// @param status_code HTTP status code.
+    /// @return Textual representation of the status code.
     static std::string statusCodeToString(const HttpStatusCode& status_code);
 
-    std::string toString() const;
+    /// @brief Returns textual representation of the HTTP response.
+    ///
+    /// It includes the "Date" header with the current time in RFC 1123 format.
+    /// It also includes "Content-Length" when the response has a non-empty
+    /// body.
+    ///
+    /// @return Textual representation of the HTTP response.
+    std::string toString() const ;
 
 protected:
 
+    /// @brief Adds HTTP header to the map.
+    ///
+    /// @param name Header name.
+    /// @param value Header value.
+    /// @param [out] headers A map to which header value should be inserted.
+    /// @tparam ValueType Type of the header value.
+    template<typename ValueType>
+    void addHeaderInternal(const std::string& name, const ValueType& value,
+                           std::map<std::string, std::string>& headers) const {
+        try {
+            headers[name] = boost::lexical_cast<std::string>(value);
+
+        } catch (const boost::bad_lexical_cast& ex) {
+            isc_throw(HttpResponseError, "unable to convert the "
+                      << name << " header value to a string");
+        }
+    }
+
+    /// @brief Returns current time formatted as required by RFC 1123.
+    ///
+    /// This method is virtual so as it can be overriden in unit tests
+    /// to return a "predictable" value of time, e.g. constant value.
+    ///
+    /// @return Current time formatted as required by RFC 1123.
     virtual std::string getDateHeaderValue() const;
 
+    /// @brief Convenience method converting status code to numeric value.
+    ///
+    /// @param status_code Status code represented as enum.
+    /// @return Numeric representation of the status code.
     static uint16_t statusCodeToNumber(const HttpStatusCode& status_code);
 
 private:
 
+    /// @brief Sets generic body for the given status code.
+    ///
+    /// Most of the classes derived from @ref HttpResponse will expect
+    /// a certain content type. Depending on the content type used they
+    /// will use different body formats for error messages. For example,
+    /// a response using text/html will use HTML within the response
+    /// body. The application/json will use JSON body etc. There is a
+    /// need to implement class specific way of generating the body
+    /// for error messages. Thus, each derivation of this class is
+    /// required to implement class specific @ref setGenericBody function
+    /// which should be called in the class constructor.
+    ///
+    /// This is also the case for this class, though the implementation
+    /// of @c setGenericBody is currently no-op.
+    ///
+    /// Note that this class can't be declared virtual because it is
+    /// meant to be called from the class constructor.
+    ///
+    /// @param status_code Status code for which the body should be
+    /// generated.
     void setGenericBody(const HttpStatusCode& status_code) { };
 
+    /// @brief Holds HTTP version for the response.
     HttpVersion http_version_;
 
+    /// @brief Holds status code for the response.
     HttpStatusCode status_code_;
 
+    /// @brief Holds HTTP headers for the response.
     std::map<std::string, std::string> headers_;
 
+    /// @brief Holds the body/content.
     std::string body_;
 
 };

src/lib/http/response_creator.cc

@@ -11,14 +11,19 @@ namespace http {
 
 HttpResponsePtr
 HttpResponseCreator::createHttpResponse(const ConstHttpRequestPtr& request) {
+    // This should never happen. This method must only be called with a
+    // non null request, so we consider it unlikely internal server error.
     if (!request) {
         isc_throw(HttpResponseError, "internal server error: HTTP request is null");
     }
 
+    // If not finalized, the request parsing failed. Generate HTTP 400.
     if (!request->isFinalized()) {
         return (createStockBadRequest(request));
     }
 
+    // Message has been successfully parsed. Create implementation specific
+    // response to this request.
     return (createDynamicHttpResponse(request));
 }
 

src/lib/http/response_creator.h

@@ -13,18 +13,71 @@
 namespace isc {
 namespace http {
 
+/// @brief Specifies an interface for classes creating HTTP responses
+/// from HTTP requests.
+///
+/// HTTP is designed to carry various types of the content. Most commonly
+/// this is text/html. In Kea, the application/json content type is used
+/// to carry control commands in JSON format. The libkea-http library is
+/// meant to be generic and provide means for transferring different types
+/// of content, depending on the use case.
+///
+/// This class specifies a common interface for generating HTTP responses
+/// from HTTP requests using specific content type and being used in some
+/// specific context. Kea modules providing HTTP services need to
+/// implement their specific derivations of the @ref HttpResponseCreator
+/// class. These derivations use classes derived from @ref HttpRequest as
+/// an input and classes derived from @ref HttpResponse as an output of
+/// @c createHttpResponse method.
 class HttpResponseCreator {
 public:
 
+    /// @brief Destructor.
+    ///
+    /// Classes with virtual functions need virtual destructors.
     virtual ~HttpResponseCreator() { };
 
+    /// @brief Create HTTP response from HTTP request received.
+    ///
+    /// This class implements a generic logic for creating a HTTP response.
+    /// Derived classes do not override this method. They merely implement
+    /// the methods it calls.
+    ///
+    /// The request processing may generally fail at one of the two stages:
+    /// parsing or interpretation of the parsed request. During the former
+    /// stage the request's syntax is checked, i.e. HTTP version, URI,
+    /// headers etc. During the latter stage the HTTP server checks if the
+    /// request is valid within the specific context, e.g. valid HTTP version
+    /// used, expected content type etc.
+    ///
+    /// In the @ref HttpRequest terms, the request has gone through the
+    /// first stage if it is "finalized" (see @ref HttpRequest::finalize).
+    /// This method accepts instances of both finalized and not finalized
+    /// requests. If the request isn't finalized it indicates that
+    /// the request parsing has failed. In such case, this method calls
+    /// @c createStockBadRequest to generate a response with HTTP 400 status
+    /// code. If the request is finalized, this method calls
+    /// @c createDynamicHttpResponse to generate implementation specific
+    /// response to the received request.
+    ///
+    /// @param request Pointer to an object representing HTTP request.
+    /// @return Pointer to the object encapsulating generated HTTP response.
+    /// @throw HttpResponseError if request is a NULL pointer.
     HttpResponsePtr createHttpResponse(const ConstHttpRequestPtr& request);
 
 protected:
 
+    /// @brief Creates implentation specific HTTP 400 response..
+    ///
+    /// @param request Pointer to an object representing HTTP request.
+    /// @return Pointer to an object representing HTTP 400 response.
     virtual HttpResponsePtr
     createStockBadRequest(const ConstHttpRequestPtr& request) const = 0;
 
+    /// @brief Creates implementation specific HTTP response.
+    ///
+    /// @param request Pointer to an object representing HTTP request.
+    /// @return Pointer to an object representing HTTP response.
     virtual HttpResponsePtr
     createDynamicHttpResponse(const ConstHttpRequestPtr& request) = 0;
 

src/lib/http/response_json.cc

@@ -12,14 +12,24 @@ namespace isc {
 namespace http {
 
 HttpResponseJson::HttpResponseJson(const HttpVersion& version,
-                                   const HttpStatusCode& status_code)
-    : HttpResponse(version, status_code) {
+                                   const HttpStatusCode& status_code,
+                                   const CallSetGenericBody& generic_body)
+    : HttpResponse(version, status_code, CallSetGenericBody::no()) {
     addHeader("Content-Type", "application/json");
-    setGenericBody(status_code);
+    // This class provides its own implementation of the setGenericBody.
+    // We call it here unless the derived class calls this constructor
+    // from its own constructor and indicates that we shouldn't set the
+    // generic content in the body.
+    if (generic_body.set_) {
+        setGenericBody(status_code);
+    }
 }
 
 void
 HttpResponseJson::setGenericBody(const HttpStatusCode& status_code) {
+    // Only generate the content for the client or server errors. For
+    // other status codes (status OK in particular) the content should
+    // be created using setBodyAsJson or setBody.
     if (isClientError(status_code) || isServerError(status_code)) {
         std::map<std::string, ConstElementPtr> map_elements;
         map_elements["result"] =

src/lib/http/response_json.h

@@ -13,19 +13,52 @@
 namespace isc {
 namespace http {
 
+class HttpResponseJson;
+
+/// @brief Pointer to the @ref HttpResponseJson object.
+typedef boost::shared_ptr<HttpResponseJson> HttpResponseJsonPtr;
+
+/// @brief Represents HTTP response with JSON content.
+///
+/// This is a specialization of the @ref HttpResponse class which
+/// includes "Content-Type" equal to "application/json". It also provides
+/// methods to create JSON content within HTTP responses.
 class HttpResponseJson : public HttpResponse {
 public:
 
-    HttpResponseJson(const HttpVersion& version,
-                     const HttpStatusCode& status_code);
-
-    virtual void setGenericBody(const HttpStatusCode& status_code);
+    /// @brief Constructor.
+    ///
+    /// @param version HTTP version.
+    /// @param status_code HTTP status code.
+    /// @param generic_body Indicates if the constructor should call
+    /// @c setGenericBody to create a generic content for the given
+    /// status code. This should be set to "no" when the constructor is
+    /// called by the derived class which provides its own implementation
+    /// of the @c setGenericBody method.
+    explicit HttpResponseJson(const HttpVersion& version,
+                              const HttpStatusCode& status_code,
+                              const CallSetGenericBody& generic_body =
+                              CallSetGenericBody::yes());
 
+    /// @brief Generates JSON content from the data structures represented
+    /// as @ref data::ConstElementPtr.
+    ///
+    /// @param json_body A data structure representing JSON content.
     virtual void setBodyAsJson(const data::ConstElementPtr& json_body);
 
-};
+private:
 
-typedef boost::shared_ptr<HttpResponseJson> HttpResponseJsonPtr;
+    /// @brief Sets generic body for the given status code.
+    ///
+    /// This method generates JSON content for the HTTP client and server
+    /// errors. The generated JSON structure is a map containing "result"
+    /// value holding HTTP status code (e.g. 400) and the "text" string
+    /// holding a status code description.
+    ///
+    /// @param status_code Status code for which the body should be
+    /// generated.
+    void setGenericBody(const HttpStatusCode& status_code);
+};
 
 }
 }

src/lib/http/tests/date_time_unittests.cc

@@ -8,9 +8,6 @@
 #include <http/date_time.h>
 #include <boost/date_time/gregorian/gregorian.hpp>
 #include <boost/date_time/posix_time/posix_time.hpp>
-#include <boost/date_time/local_time/local_time.hpp>
-
-
 #include <gtest/gtest.h>
 
 using namespace boost::gregorian;
@@ -19,9 +16,21 @@ using namespace isc::http;
 
 namespace {
 
+/// @brief Test fixture class for @ref HttpDateTime.
 class HttpDateTimeTest : public ::testing::Test {
 public:
 
+    /// @brief Checks time value against expected values.
+    ///
+    /// This method uses value of @ref date_time_ for the test.
+    ///
+    /// @param exp_day_of_week Expected day of week.
+    /// @param exp_day Expected day of month.
+    /// @param exp_month Expected month.
+    /// @param exp_year Expected year.
+    /// @param exp_hours Expected hour value.
+    /// @param exp_minutes Expected minutes value.
+    /// @param exp_seconds Expected seconds value.
     void testDateTime(const unsigned short exp_day_of_week,
                       const unsigned short exp_day,
                       const unsigned short exp_month,
@@ -29,31 +38,40 @@ public:
                       const long exp_hours,
                       const long exp_minutes,
                       const long exp_seconds) {
+        // Retrieve @c boost::posix_time::ptime value.
         ptime as_ptime = date_time_.getPtime();
+        // Date is contained within this object.
         date date_part = as_ptime.date();
 
+        // Verify weekday.
         greg_weekday day_of_week = date_part.day_of_week();
         EXPECT_EQ(exp_day_of_week, day_of_week.as_number());
 
+        // Verify day of month.
         greg_day day = date_part.day();
         EXPECT_EQ(exp_day, day.as_number());
 
+        // Verify month.
         greg_month month = date_part.month();
         EXPECT_EQ(exp_month, month.as_number());
 
+        // Verify year.
         greg_year year = date_part.year();
         EXPECT_EQ(exp_year, static_cast<unsigned short>(year));
 
+        // Retrieve time of the day and verify hour, minute and second.
         time_duration time_of_day = as_ptime.time_of_day();
         EXPECT_EQ(exp_hours, time_of_day.hours());
         EXPECT_EQ(exp_minutes, time_of_day.minutes());
         EXPECT_EQ(exp_seconds, time_of_day.seconds());
     }
 
+    /// @brief Date/time value which should be set by the tests.
     HttpDateTime date_time_;
 
 };
 
+// Test formatting as specified in RFC 1123.
 TEST_F(HttpDateTimeTest, rfc1123Format) {
     date gdate(greg_year(2002), greg_month(1), greg_day(20));
     time_duration tm(23, 59, 59, 0);
@@ -64,6 +82,7 @@ TEST_F(HttpDateTimeTest, rfc1123Format) {
     EXPECT_EQ("Sun, 20 Jan 2002 23:59:59 GMT", formatted);
 }
 
+// Test formatting as specified in RFC 850.
 TEST_F(HttpDateTimeTest, rfc850Format) {
     date gdate(greg_year(1994), greg_month(8), greg_day(6));
     time_duration tm(11, 12, 13, 0);
@@ -75,6 +94,7 @@ TEST_F(HttpDateTimeTest, rfc850Format) {
     EXPECT_EQ("Saturday, 06-Aug-94 11:12:13 GMT", formatted);
 }
 
+// Test formatting as output of asctime().
 TEST_F(HttpDateTimeTest, asctimeFormat) {
     date gdate(greg_year(1999), greg_month(11), greg_day(2));
     time_duration tm(03, 57, 12, 0);
@@ -86,6 +106,7 @@ TEST_F(HttpDateTimeTest, asctimeFormat) {
     EXPECT_EQ("Tue Nov  2 03:57:12 1999", formatted);
 }
 
+// Test parsing time in RFC 1123 format.
 TEST_F(HttpDateTimeTest, fromRfc1123) {
     ASSERT_NO_THROW(
         date_time_ = HttpDateTime::fromRfc1123("Wed, 21 Dec 2016 18:53:45 GMT")
@@ -103,6 +124,7 @@ TEST_F(HttpDateTimeTest, fromRfc1123) {
                  HttpTimeConversionError);
 }
 
+// Test parsing time in RFC 850 format.
 TEST_F(HttpDateTimeTest, fromRfc850) {
     ASSERT_NO_THROW(
         date_time_ = HttpDateTime::fromRfc850("Wednesday, 21-Dec-16 18:53:45 GMT");
@@ -118,6 +140,7 @@ TEST_F(HttpDateTimeTest, fromRfc850) {
                  HttpTimeConversionError);
 }
 
+// Test parsing time in asctime() format.
 TEST_F(HttpDateTimeTest, fromRfcAsctime) {
     ASSERT_NO_THROW(
         date_time_ = HttpDateTime::fromAsctime("Wed Dec 21 08:49:37 2016");
@@ -133,6 +156,7 @@ TEST_F(HttpDateTimeTest, fromRfcAsctime) {
                  HttpTimeConversionError);
 }
 
+// Test parsing time in RFC 1123 format using HttpDateTime::fromAny().
 TEST_F(HttpDateTimeTest, fromAnyRfc1123) {
     ASSERT_NO_THROW(
         date_time_ = HttpDateTime::fromAny("Thu, 05 Jan 2017 09:15:06 GMT");
@@ -140,6 +164,7 @@ TEST_F(HttpDateTimeTest, fromAnyRfc1123) {
     testDateTime(4, 5, 1, 2017, 9, 15, 06);
 }
 
+// Test parsing time in RFC 850 format using HttpDateTime::fromAny().
 TEST_F(HttpDateTimeTest, fromAnyRfc850) {
     ASSERT_NO_THROW(
         date_time_ = HttpDateTime::fromAny("Saturday, 18-Feb-17 01:02:10 GMT");
@@ -147,6 +172,7 @@ TEST_F(HttpDateTimeTest, fromAnyRfc850) {
     testDateTime(6, 18, 2, 2017, 1, 2, 10);
 }
 
+// Test parsing time in asctime() format using HttpDateTime::fromAny().
 TEST_F(HttpDateTimeTest, fromAnyAsctime) {
     ASSERT_NO_THROW(
         date_time_ = HttpDateTime::fromAny("Wed Mar  1 15:45:07 2017 GMT");
@@ -154,6 +180,8 @@ TEST_F(HttpDateTimeTest, fromAnyAsctime) {
     testDateTime(3, 1, 3, 2017, 15, 45, 7);
 }
 
+// Test that HttpDateTime::fromAny throws exception if unsupported format is
+// used.
 TEST_F(HttpDateTimeTest, fromAnyInvalidFormat) {
     EXPECT_THROW(HttpDateTime::fromAsctime("20020131T235959"),
                  HttpTimeConversionError);

src/lib/http/tests/response_creator_unittests.cc

@@ -18,45 +18,77 @@ using namespace isc::http::test;
 
 namespace {
 
+/// @brief Test HTTP response.
 typedef TestHttpResponseBase<HttpResponseJson> Response;
+
+/// @brief Pointer to test HTTP response.
 typedef boost::shared_ptr<Response> ResponsePtr;
 
+/// @brief Implementation of the @ref HttpResponseCreator.
 class TestHttpResponseCreator : public HttpResponseCreator {
 private:
 
+    /// @brief Creates HTTP 400 response.
+    ///
+    /// @param request Pointer to the HTTP request.
+    /// @return Pointer to the generated HTTP 400 response.
     virtual HttpResponsePtr
     createStockBadRequest(const ConstHttpRequestPtr& request) const {
+        // The request hasn't been finalized so the request object
+        // doesn't contain any information about the HTTP version number
+        // used. But, the context should have this data (assuming the
+        // HTTP version is parsed ok).
         HttpVersion http_version(request->context()->http_version_major_,
                                  request->context()->http_version_minor_);
+        // This will generate the response holding JSON content.
         ResponsePtr response(new Response(http_version,
                                           HttpStatusCode::BAD_REQUEST));
         return (response);
     }
 
+    /// @brief Creates HTTP response.
+    ///
+    /// @param request Pointer to the HTTP request.
+    /// @return Pointer to the generated HTTP OK response with no content.
     virtual HttpResponsePtr
     createDynamicHttpResponse(const ConstHttpRequestPtr& request) {
+        // The simplest thing is to create a response with no content.
+        // We don't need content to test our class.
         ResponsePtr response(new Response(request->getHttpVersion(),
                                           HttpStatusCode::OK));
         return (response);
     }
 };
 
+// This test verifies that Bad Request status is generated when the request
+// hasn't been finalized.
 TEST(HttpResponseCreatorTest, badRequest) {
     HttpResponsePtr response;
+    // Create a request but do not finalize it.
     HttpRequestPtr request(new HttpRequest());
+    request->context()->http_version_major_ = 1;
+    request->context()->http_version_minor_ = 0;
+    request->context()->method_ = "GET";
+    request->context()->uri_ = "/foo";
+
+    // Use test specific implementation of Response Creator. It should
+    // generate HTTP error 400.
     TestHttpResponseCreator creator;
     ASSERT_NO_THROW(response = creator.createHttpResponse(request));
     ASSERT_TRUE(response);
 
-    EXPECT_EQ("HTTP/0.0 400 Bad Request\r\n"
+    EXPECT_EQ("HTTP/1.0 400 Bad Request\r\n"
+              "Content-Length: 40\r\n"
               "Content-Type: application/json\r\n"
-              "Date: Tue, 19 Dec 2016 18:53:35 GMT\r\n"
-              "Content-Length: 40\r\n\r\n"
+              "Date: Tue, 19 Dec 2016 18:53:35 GMT\r\n\r\n"
               "{ \"result\": 400, \"text\": \"Bad Request\" }", response->toString());
 }
 
+// This test verifies that response is generated successfully from the
+// finalized/parsed request.
 TEST(HttpResponseCreatorTest, goodRequest) {
     HttpResponsePtr response;
+    // Create request and finalize it.
     HttpRequestPtr request(new HttpRequest());
     request->context()->http_version_major_ = 1;
     request->context()->http_version_minor_ = 0;
@@ -64,6 +96,8 @@ TEST(HttpResponseCreatorTest, goodRequest) {
     request->context()->uri_ = "/foo";
     ASSERT_NO_THROW(request->finalize());
 
+    // Use test specific implementation of the Response Creator to generate
+    // a response.
     TestHttpResponseCreator creator;
     ASSERT_NO_THROW(response = creator.createHttpResponse(request));
     ASSERT_TRUE(response);

src/lib/http/tests/response_json_unittests.cc

@@ -17,11 +17,21 @@ using namespace isc::http::test;
 
 namespace {
 
+/// @brief Response type used in tests.
 typedef TestHttpResponseBase<HttpResponseJson> TestHttpResponseJson;
 
+/// @brief Test fixture class for @ref HttpResponseJson.
 class HttpResponseJsonTest : public ::testing::Test {
 public:
 
+    /// @brief Constructor.
+    ///
+    /// Initializes the following class members:
+    /// - json_string_ - which is a pretty formatted JSON content,
+    /// - json_ - A structure of Element objects representing the JSON,
+    /// - json_string_from_json_ - which is a JSON text converted back from
+    ///   the json_ data structure. It is the same content as json_string_
+    ///   but has different whitespaces.
     HttpResponseJsonTest()
         : json_(), json_string_(), json_string_from_json_() {
         json_string_ =
@@ -42,10 +52,16 @@ public:
         json_string_from_json_ = json_->str();
     }
 
+    /// @brief Test that the response format is correct.
+    ///
+    /// @param status_code HTTP status code for which the response should
+    /// be tested.
+    /// @param status_message HTTP status message.
     void testGenericResponse(const HttpStatusCode& status_code,
                              const std::string& status_message) {
         TestHttpResponseJson response(HttpVersion(1, 0), status_code);
         std::ostringstream status_message_json;
+        // Build the expected content.
         status_message_json << "{ \"result\": "
                             << static_cast<uint16_t>(status_code)
                             << ", \"text\": "
@@ -53,32 +69,40 @@ public:
         std::ostringstream response_string;
         response_string <<
             "HTTP/1.0 " << static_cast<uint16_t>(status_code) << " "
-            << status_message << "\r\n"
-            "Content-Type: application/json\r\n"
-            "Date: " << response.getDateHeaderValue() << "\r\n";
+            << status_message << "\r\n";
 
+        // The content must only be generated for error codes.
         if (HttpResponse::isClientError(status_code) ||
             HttpResponse::isServerError(status_code)) {
             response_string << "Content-Length: " << status_message_json.str().size()
                             << "\r\n";
         }
 
-        response_string << "\r\n";
+        // Content-Type and Date are automatically included.
+        response_string << "Content-Type: application/json\r\n"
+            "Date: " << response.getDateHeaderValue() << "\r\n\r\n";
 
         if (HttpResponse::isClientError(status_code) ||
             HttpResponse::isServerError(status_code)) {
             response_string << status_message_json.str();
         }
 
+        // Check that the output is as expected.
         EXPECT_EQ(response_string.str(), response.toString());
     }
 
+    /// @brief JSON content represented as structure of Element objects.
     ConstElementPtr json_;
+
+    /// @brief Pretty formatted JSON content.
     std::string json_string_;
+
+    /// @brief JSON content parsed back from json_ structure.
     std::string json_string_from_json_;
 
 };
 
+// Test that the response with custom JSON content is generated properly.
 TEST_F(HttpResponseJsonTest, responseWithContent) {
     TestHttpResponseJson response(HttpVersion(1, 1), HttpStatusCode::OK);
     ASSERT_NO_THROW(response.setBodyAsJson(json_));
@@ -86,13 +110,14 @@ TEST_F(HttpResponseJsonTest, responseWithContent) {
     std::ostringstream response_string;
     response_string <<
         "HTTP/1.1 200 OK\r\n"
+        "Content-Length: " << json_string_from_json_.length() << "\r\n"
         "Content-Type: application/json\r\n"
-        "Date: " << response.getDateHeaderValue() << "\r\n"
-        "Content-Length: " << json_string_from_json_.length()
-                    << "\r\n\r\n" << json_string_from_json_;
+        "Date: " << response.getDateHeaderValue() << "\r\n\r\n"
+                    << json_string_from_json_;
     EXPECT_EQ(response_string.str(), response.toString());
 }
 
+// Test that generic responses are created properly.
 TEST_F(HttpResponseJsonTest, genericResponse) {
     testGenericResponse(HttpStatusCode::OK, "OK");
     testGenericResponse(HttpStatusCode::CREATED, "Created");

src/lib/http/tests/response_unittests.cc

@@ -19,13 +19,22 @@ using namespace isc::http::test;
 
 namespace {
 
+/// @brief Response type used in tests.
 typedef TestHttpResponseBase<HttpResponse> TestHttpResponse;
 
+/// @brief Test fixture class for @ref HttpResponse.
 class HttpResponseTest : public ::testing::Test {
 public:
 
+    /// @brief Checks if the format of the response is correct.
+    ///
+    /// @param status_code HTTP status code in the response.
+    /// @param status_message HTTP status message in the response.
     void testResponse(const HttpStatusCode& status_code,
                       const std::string& status_message) {
+        // Create the response. Because we're using derived class
+        // it returns the fixed value of the Date header, which is
+        // very useful in unit tests.
         TestHttpResponse response(HttpVersion(1, 0), status_code);
         response.addHeader("Content-Type", "text/html");
         std::ostringstream response_string;
@@ -36,32 +45,38 @@ public:
 
         EXPECT_EQ(response_string.str(), response.toString());
     }
-
 };
 
+// Test the case of HTTP OK message.
 TEST_F(HttpResponseTest, responseOK) {
+    // Include HTML body.
     const std::string sample_body =
         "<html>"
         "<head><title>Kea page title</title></head>"
         "<body><h1>Some header</h1></body>"
         "</html>";
 
+    // Create the message and add some headers.
     TestHttpResponse response(HttpVersion(1, 0), HttpStatusCode::OK);
     response.addHeader("Content-Type", "text/html");
     response.addHeader("Host", "kea.example.org");
     response.setBody(sample_body);
 
+    // Create a string holding expected response. Note that the Date
+    // is a fixed value returned by the customized TestHttpResponse
+    // classs.
     std::ostringstream response_string;
     response_string <<
         "HTTP/1.0 200 OK\r\n"
+        "Content-Length: " << sample_body.length() << "\r\n"
         "Content-Type: text/html\r\n"
-        "Host: kea.example.org\r\n"
         "Date: " << response.getDateHeaderValue() << "\r\n"
-        "Content-Length: " << sample_body.length()
-                    << "\r\n\r\n" << sample_body;
+        "Host: kea.example.org\r\n\r\n" << sample_body;
+
     EXPECT_EQ(response_string.str(), response.toString());
 }
 
+// Test generic responses for various status codes.
 TEST_F(HttpResponseTest, genericResponse) {
     testResponse(HttpStatusCode::OK, "OK");
     testResponse(HttpStatusCode::CREATED, "Created");
@@ -81,6 +96,7 @@ TEST_F(HttpResponseTest, genericResponse) {
     testResponse(HttpStatusCode::SERVICE_UNAVAILABLE, "Service Unavailable");
 }
 
+// Test if the class correctly identifies client errors.
 TEST_F(HttpResponseTest, isClientError) {
     EXPECT_FALSE(HttpResponse::isClientError(HttpStatusCode::OK));
     EXPECT_FALSE(HttpResponse::isClientError(HttpStatusCode::CREATED));
@@ -100,6 +116,7 @@ TEST_F(HttpResponseTest, isClientError) {
     EXPECT_FALSE(HttpResponse::isClientError(HttpStatusCode::SERVICE_UNAVAILABLE));
 }
 
+// Test if the class correctly identifies server errors.
 TEST_F(HttpResponseTest, isServerError) {
     EXPECT_FALSE(HttpResponse::isServerError(HttpStatusCode::OK));
     EXPECT_FALSE(HttpResponse::isServerError(HttpStatusCode::CREATED));
@@ -119,10 +136,23 @@ TEST_F(HttpResponseTest, isServerError) {
     EXPECT_TRUE(HttpResponse::isServerError(HttpStatusCode::SERVICE_UNAVAILABLE));
 }
 
+// Test that the generated time value, being included in the Date
+// header, is correct.
 TEST_F(HttpResponseTest, getDateHeaderValue) {
+    // Create a response and retrieve the value to be included in the
+    // Date header. This value should hold a current time in the
+    // RFC1123 format.
     TestHttpResponse response(HttpVersion(1, 0), HttpStatusCode::OK);
     std::string generated_date = response.generateDateHeaderValue();
+
+    // Use our date/time utilities to parse this value into the ptime.
     HttpDateTime parsed_time = HttpDateTime::fromRfc1123(generated_date);
+
+    // Now that we have it converted back, we can check how far this
+    // value is from the current time. To be on the safe side, we check
+    // that it is not later than 10 seconds apart, rather than checking
+    // it for equality. In fact, checking it for equality would almost
+    // certainly cause an error. Especially on a virtual machine.
     time_duration parsed_to_current =
         microsec_clock::universal_time() - parsed_time.getPtime();
     EXPECT_LT(parsed_to_current.seconds(), 10);