From 4c23ca085f17b83ceb500574f1befcb83d7a0867 Mon Sep 17 00:00:00 2001 From: Tomek Mrugalski Date: Tue, 16 Jan 2018 21:58:37 +0100 Subject: [PATCH] [5451] Various changes after review. --- src/lib/asiolink/tcp_socket.h | 3 +- src/lib/http/client.cc | 4 +- .../http/tests/response_parser_unittests.cc | 88 ++++++++++------- src/lib/http/tests/server_client_unittests.cc | 2 +- src/lib/http/url.cc | 94 +++++++++---------- src/lib/http/url.h | 10 +- 6 files changed, 115 insertions(+), 86 deletions(-) diff --git a/src/lib/asiolink/tcp_socket.h b/src/lib/asiolink/tcp_socket.h index 8465c0705e..05a8b1497f 100644 --- a/src/lib/asiolink/tcp_socket.h +++ b/src/lib/asiolink/tcp_socket.h @@ -92,7 +92,8 @@ public: /// \brief Checks if the connection is usable. /// - /// The connection is usable if the peer has closed it. + /// The connection is usable if the socket is open and the peer has not + /// closed its connection. /// /// \return true if the connection is usable. bool isUsable() const { diff --git a/src/lib/http/client.cc b/src/lib/http/client.cc index b1a2b9ed23..2691735312 100644 --- a/src/lib/http/client.cc +++ b/src/lib/http/client.cc @@ -269,7 +269,7 @@ public: HttpResponsePtr& response, long& request_timeout, HttpClient::RequestHandler& callback) { - // Check if the is a queue for this URL. If there is no queue, there + // Check if there is a queue for this URL. If there is no queue, there // is no request queued either. auto it = queue_.find(url); if (it != queue_.end()) { @@ -580,7 +580,7 @@ Connection::sendCallback(const boost::system::error_code& ec, size_t length) { } // If there is no more data to be sent, start receiving a response. Otherwise, - // continue receiving. + // continue sending. if (buf_.empty()) { doReceive(); diff --git a/src/lib/http/tests/response_parser_unittests.cc b/src/lib/http/tests/response_parser_unittests.cc index b4dacfe890..dd21c33260 100644 --- a/src/lib/http/tests/response_parser_unittests.cc +++ b/src/lib/http/tests/response_parser_unittests.cc @@ -68,6 +68,58 @@ public: EXPECT_FALSE(parser.getErrorMessage().empty()); } + /// @brief Tests that the response specified with (header, body) can + /// be parsed properly. + /// + /// @param header specifies the header of the response to be parsed + /// @param json specifies the body of the response (JSON in text format) to be parsed + /// @param expect_success whether the parsing is expected to be successful + /// + /// @return a parser that parsed the response for further inspection + HttpResponseJson testResponseWithJson(const std::string& header, + const std::string& json, + bool expect_success = true) { + std::string http_resp = createResponseString(header, json); + + // Create HTTP response which accepts JSON as a body. + HttpResponseJson response; + + // Create a parser and make it use the response we created. + HttpResponseParser parser(response); + EXPECT_NO_THROW(parser.initModel()); + + // Simulate receiving HTTP response in chunks. + const unsigned chunk_size = 10; + while (!http_resp.empty()) { + size_t chunk = http_resp.size() % chunk_size; + if (chunk == 0) { + chunk = chunk_size; + } + + parser.postBuffer(&http_resp[0], chunk); + http_resp.erase(0, chunk); + parser.poll(); + if (chunk < chunk_size) { + EXPECT_TRUE(parser.needData()); + if (!parser.needData()) { + ADD_FAILURE() << "Parser completed prematurely"; + return (response); + } + } + } + + if (expect_success) { + // Parser should have parsed the response and should expect no more data. + EXPECT_FALSE(parser.needData()); + // Parsing should be successful. + EXPECT_TRUE(parser.httpParseOk()) << parser.getErrorMessage(); + // There should be no error message. + EXPECT_TRUE(parser.getErrorMessage().empty()); + } + + return (response); + } + /// @brief Instance of the HttpResponse used by the unit tests. HttpResponse response_; }; @@ -79,37 +131,7 @@ TEST_F(HttpResponseParserTest, responseWithJson) { "Content-Type: application/json\r\n"; std::string json = "{ \"result\": 0, \"text\": \"All ok\" }"; - http_resp = createResponseString(http_resp, json); - - // Create HTTP response which accepts JSON as a body. - HttpResponseJson response; - - // Create a parser and make it use the response we created. - HttpResponseParser parser(response); - ASSERT_NO_THROW(parser.initModel()); - - // Simulate receiving HTTP response in chunks. - const unsigned chunk_size = 10; - while (!http_resp.empty()) { - size_t chunk = http_resp.size() % chunk_size; - if (chunk == 0) { - chunk = chunk_size; - } - - parser.postBuffer(&http_resp[0], chunk); - http_resp.erase(0, chunk); - parser.poll(); - if (chunk < chunk_size) { - ASSERT_TRUE(parser.needData()); - } - } - - // Parser should have parsed the response and should expect no more data. - ASSERT_FALSE(parser.needData()); - // Parsing should be successful. - ASSERT_TRUE(parser.httpParseOk()) << parser.getErrorMessage(); - // There should be no error message. - EXPECT_TRUE(parser.getErrorMessage().empty()); + HttpResponseJson response = testResponseWithJson(http_resp, json); // Verify HTTP version, status code and phrase. EXPECT_EQ(1, response.getHttpVersion().major_); @@ -166,8 +188,8 @@ TEST_F(HttpResponseParserTest, extraneousDataInResponse) { EXPECT_TRUE(parser.getErrorMessage().empty()); } -// This test verifies that LWS is parsed correctly. The LWS marks line breaks -// in the HTTP header values. +// This test verifies that LWS is parsed correctly. The LWS (linear white +// space) marks line breaks in the HTTP header values. TEST_F(HttpResponseParserTest, getLWS) { // "User-Agent" header contains line breaks with whitespaces in the new // lines to mark continuation of the header value. diff --git a/src/lib/http/tests/server_client_unittests.cc b/src/lib/http/tests/server_client_unittests.cc index d8979a08dd..ac5def3b31 100644 --- a/src/lib/http/tests/server_client_unittests.cc +++ b/src/lib/http/tests/server_client_unittests.cc @@ -1236,7 +1236,7 @@ TEST_F(HttpClientTest, clientRequestTimeout) { PostHttpRequestJsonPtr request2 = createRequest("sequence", 1); HttpResponseJsonPtr response2(new HttpResponseJson()); ASSERT_NO_THROW(client.asyncSendRequest(url, request2, response2, - [this, &cb_num](const boost::system::error_code& ec, const HttpResponsePtr&, + [this, &cb_num](const boost::system::error_code& /*ec*/, const HttpResponsePtr&, const std::string&) { if (++cb_num > 1) { io_service_.stop(); diff --git a/src/lib/http/url.cc b/src/lib/http/url.cc index 196bcdbec6..4f12ab5643 100644 --- a/src/lib/http/url.cc +++ b/src/lib/http/url.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2017 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2017-2018 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -80,18 +80,18 @@ Url::parse() { port_ = 0; path_.clear(); - std::ostringstream e; + std::ostringstream error; // Retrieve scheme - size_t p = url_.find(":"); - if ((p == 0) || (p == std::string::npos)) { - e << "url " << url_ << " lacks http or https scheme"; - error_message_ = e.str(); + size_t offset = url_.find(":"); + if ((offset == 0) || (offset == std::string::npos)) { + error << "url " << url_ << " lacks http or https scheme"; + error_message_ = error.str(); return; } // Validate scheme. - std::string scheme = url_.substr(0, p); + std::string scheme = url_.substr(0, offset); if (scheme == "http") { scheme_ = Url::HTTP; @@ -99,87 +99,87 @@ Url::parse() { scheme_ = Url::HTTPS; } else { - e << "invalid scheme " << scheme << " in " << url_; - error_message_ = e.str(); + error << "invalid scheme " << scheme << " in " << url_; + error_message_ = error.str(); return; } // Colon and two slashes should follow the scheme - if (url_.substr(p, 3) != "://") { - e << "expected :// after scheme in " << url_; - error_message_ = e.str(); + if (url_.substr(offset, 3) != "://") { + error << "expected :// after scheme in " << url_; + error_message_ = error.str(); return; } // Move forward to hostname. - p += 3; - if (p >= url_.length()) { - e << "hostname missing in " << url_; - error_message_ = e.str(); + offset += 3; + if (offset >= url_.length()) { + error << "hostname missing in " << url_; + error_message_ = error.str(); return; } - size_t h = 0; + size_t offset2 = 0; // IPv6 address is specified within [ ]. - if (url_.at(p) == '[') { - h = url_.find(']', p); - if (h == std::string::npos) { - e << "expected ] after IPv6 address in " << url_; - error_message_ = e.str(); + if (url_.at(offset) == '[') { + offset2 = url_.find(']', offset); + if (offset2 == std::string::npos) { + error << "expected ] after IPv6 address in " << url_; + error_message_ = error.str(); return; - } else if (h == p + 1) { - e << "expected IPv6 address within [] in " << url_; - error_message_ = e.str(); + } else if (offset2 == offset + 1) { + error << "expected IPv6 address within [] in " << url_; + error_message_ = error.str(); return; } // Move one character beyond the ]. - ++h; + ++offset2; } else { // There is a normal hostname or IPv4 address. It is terminated // by the colon (for port number), a slash (if no port number) or // goes up to the end of the URL. - h = url_.find(":", p); - if (h == std::string::npos) { - h = url_.find("/", p); - if (h == std::string::npos) { + offset2 = url_.find(":", offset); + if (offset2 == std::string::npos) { + offset2 = url_.find("/", offset); + if (offset2 == std::string::npos) { // No port number and no slash. - h = url_.length(); + offset2 = url_.length(); } } } // Extract the hostname. - hostname_ = url_.substr(p, h - p); + hostname_ = url_.substr(offset, offset2 - offset); // If there is no port number and no path, simply return and mark the // URL as valid. - if (h == url_.length()) { + if (offset2 == url_.length()) { valid_ = true; return; } // If there is a port number, we need to read it and convert to // numeric value. - if (url_.at(h) == ':') { - if (h == url_.length() - 1) { - e << "expected port number after : in " << url_; - error_message_ = e.str(); + if (url_.at(offset2) == ':') { + if (offset2 == url_.length() - 1) { + error << "expected port number after : in " << url_; + error_message_ = error.str(); return; } // Move to the port number. - ++h; + ++offset2; // Port number may be terminated by a slash or by the end of URL. - size_t s = url_.find('/', h); + size_t slash_offset = url_.find('/', offset2); std::string port_str; - if (s == std::string::npos) { - port_str = url_.substr(h); + if (slash_offset == std::string::npos) { + port_str = url_.substr(offset2); } else { - port_str = url_.substr(h, s - h); + port_str = url_.substr(offset2, slash_offset - offset2); } try { @@ -187,18 +187,18 @@ Url::parse() { port_ = boost::lexical_cast(port_str); } catch (...) { - e << "invalid port number " << port_str << " in " << url_; - error_message_ = e.str(); + error << "invalid port number " << port_str << " in " << url_; + error_message_ = error.str(); return; } // Go to the end of the port section. - h = s; + offset2 = slash_offset; } // If there is anything left in the URL, we consider it a path. - if (h != std::string::npos) { - path_ = url_.substr(h); + if (offset2 != std::string::npos) { + path_ = url_.substr(offset2); } valid_ = true; diff --git a/src/lib/http/url.h b/src/lib/http/url.h index a688cdcd9d..0d9b509941 100644 --- a/src/lib/http/url.h +++ b/src/lib/http/url.h @@ -1,4 +1,4 @@ -// Copyright (C) 2017 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2017-2018 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -13,7 +13,7 @@ namespace isc { namespace http { -/// @brief Represents URL. +/// @brief Represents an URL. /// /// It parses the provided URL and allows for retrieving the parts /// of it after parsing. @@ -33,6 +33,12 @@ public: /// @param url URL. explicit Url(const std::string& url); + /// @brief compares URLs lexically. + /// + /// Both URLs are compared as text. + /// + /// @param url URL to be compared with + /// @return true if the other operator is larger (in lexical sense) bool operator<(const Url& url) const; /// @brief Checks if the URL is valid. -- 2.47.2