Added URL within http::Request. Review: https://reviews.apache.org/r/38597
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/d347bf56 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/d347bf56 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/d347bf56 Branch: refs/heads/master Commit: d347bf56c807dad2c7966546819d03f9a7686eb4 Parents: 2fd5ed9 Author: Benjamin Mahler <[email protected]> Authored: Thu Sep 17 16:27:51 2015 -0700 Committer: Benjamin Mahler <[email protected]> Committed: Mon Sep 28 10:50:50 2015 -0700 ---------------------------------------------------------------------- .../libprocess/include/process/firewall.hpp | 4 +- 3rdparty/libprocess/include/process/http.hpp | 129 ++++++++++--------- 3rdparty/libprocess/include/process/system.hpp | 2 +- 3rdparty/libprocess/src/decoder.hpp | 19 ++- 3rdparty/libprocess/src/help.cpp | 2 +- 3rdparty/libprocess/src/logging.cpp | 4 +- 3rdparty/libprocess/src/metrics/metrics.cpp | 6 +- 3rdparty/libprocess/src/process.cpp | 45 +++---- 3rdparty/libprocess/src/tests/benchmarks.cpp | 8 +- 3rdparty/libprocess/src/tests/decoder_tests.cpp | 16 +-- 3rdparty/libprocess/src/tests/http_tests.cpp | 24 ++-- 11 files changed, 129 insertions(+), 130 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/d347bf56/3rdparty/libprocess/include/process/firewall.hpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/include/process/firewall.hpp b/3rdparty/libprocess/include/process/firewall.hpp index b1abfb2..b5c7d99 100644 --- a/3rdparty/libprocess/include/process/firewall.hpp +++ b/3rdparty/libprocess/include/process/firewall.hpp @@ -80,8 +80,8 @@ public: const network::Socket&, const http::Request& request) { - if (paths.contains(request.path)) { - return http::Forbidden("Endpoint '" + request.path + "' is disabled"); + if (paths.contains(request.url.path)) { + return http::Forbidden("Endpoint '" + request.url.path + "' is disabled"); } return None(); http://git-wip-us.apache.org/repos/asf/mesos/blob/d347bf56/3rdparty/libprocess/include/process/http.hpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/include/process/http.hpp b/3rdparty/libprocess/include/process/http.hpp index c71efcc..5932af4 100644 --- a/3rdparty/libprocess/include/process/http.hpp +++ b/3rdparty/libprocess/include/process/http.hpp @@ -62,6 +62,59 @@ extern hashmap<uint16_t, std::string> statuses; void initialize(); +// Represents a Uniform Resource Locator: +// scheme://domain|ip:port/path?query#fragment +// +// This is actually a URI-reference (see 4.1 of RFC 3986). +// +// TODO(bmahler): The default port should depend on the scheme! +struct URL +{ + URL() = default; + + URL(const std::string& _scheme, + const std::string& _domain, + const uint16_t _port = 80, + const std::string& _path = "/", + const hashmap<std::string, std::string>& _query = + (hashmap<std::string, std::string>()), + const Option<std::string>& _fragment = None()) + : scheme(_scheme), + domain(_domain), + port(_port), + path(_path), + query(_query), + fragment(_fragment) {} + + URL(const std::string& _scheme, + const net::IP& _ip, + const uint16_t _port = 80, + const std::string& _path = "/", + const hashmap<std::string, std::string>& _query = + (hashmap<std::string, std::string>()), + const Option<std::string>& _fragment = None()) + : scheme(_scheme), + ip(_ip), + port(_port), + path(_path), + query(_query), + fragment(_fragment) {} + + Option<std::string> scheme; + + // TODO(benh): Consider using unrestricted union for 'domain' and 'ip'. + Option<std::string> domain; + Option<net::IP> ip; + Option<uint16_t> port; + std::string path; + hashmap<std::string, std::string> query; + Option<std::string> fragment; +}; + + +std::ostream& operator<<(std::ostream& stream, const URL& url); + + struct CaseInsensitiveHash { size_t operator()(const std::string& key) const @@ -94,31 +147,30 @@ struct CaseInsensitiveEqual struct Request { - // Contains the client's address. Note that this may - // correspond to a proxy or load balancer address. - network::Address client; + std::string method; // TODO(benh): Add major/minor version. + + // For client requests, the URL should be a URI. + // For server requests, the URL may be a URI or a relative reference. + URL url; + hashmap<std::string, std::string, CaseInsensitiveHash, CaseInsensitiveEqual> headers; - std::string method; - - // TODO(benh): Replace 'url', 'path', 'query', and 'fragment' with URL. - std::string url; // (path?query#fragment) - - // TODO(vinod): Make this a 'Path' instead of 'string'. - std::string path; - - hashmap<std::string, std::string> query; - std::string fragment; + // TODO(bmahler): Add a 'query' field which contains both + // the URL query and the parsed form data from the body. std::string body; bool keepAlive; + // For server requests, this contains the address of the client. + // Note that this may correspond to a proxy or load balancer address. + network::Address client; + /** * Returns whether the encoding is considered acceptable in the * response. See RFC 2616 section 14.3 for details. @@ -661,57 +713,6 @@ std::string encode(const hashmap<std::string, std::string>& query); } // namespace query { -// Represents a Uniform Resource Locator: -// scheme://domain|ip:port/path?query#fragment -// -// This is actually a URI-reference (see 4.1 of RFC 3986). -// -// TODO(bmahler): The default port should depend on the scheme! -struct URL -{ - URL(const std::string& _scheme, - const std::string& _domain, - const uint16_t _port = 80, - const std::string& _path = "/", - const hashmap<std::string, std::string>& _query = - (hashmap<std::string, std::string>()), - const Option<std::string>& _fragment = None()) - : scheme(_scheme), - domain(_domain), - port(_port), - path(_path), - query(_query), - fragment(_fragment) {} - - URL(const std::string& _scheme, - const net::IP& _ip, - const uint16_t _port = 80, - const std::string& _path = "/", - const hashmap<std::string, std::string>& _query = - (hashmap<std::string, std::string>()), - const Option<std::string>& _fragment = None()) - : scheme(_scheme), - ip(_ip), - port(_port), - path(_path), - query(_query), - fragment(_fragment) {} - - Option<std::string> scheme; - - // TODO(benh): Consider using unrestricted union for 'domain' and 'ip'. - Option<std::string> domain; - Option<net::IP> ip; - Option<uint16_t> port; - std::string path; - hashmap<std::string, std::string> query; - Option<std::string> fragment; -}; - - -std::ostream& operator<<(std::ostream& stream, const URL& url); - - // TODO(bmahler): Consolidate these functions into a single // http::request function that takes a 'Request' object. http://git-wip-us.apache.org/repos/asf/mesos/blob/d347bf56/3rdparty/libprocess/include/process/system.hpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/include/process/system.hpp b/3rdparty/libprocess/include/process/system.hpp index 264d948..9b49c95 100644 --- a/3rdparty/libprocess/include/process/system.hpp +++ b/3rdparty/libprocess/include/process/system.hpp @@ -182,7 +182,7 @@ private: object.values["mem_free_bytes"] = memory.get().free.bytes(); } - return http::OK(object, request.query.get("jsonp")); + return http::OK(object, request.url.query.get("jsonp")); } metrics::Gauge load_1min; http://git-wip-us.apache.org/repos/asf/mesos/blob/d347bf56/3rdparty/libprocess/src/decoder.hpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/src/decoder.hpp b/3rdparty/libprocess/src/decoder.hpp index 67a5135..d348463 100644 --- a/3rdparty/libprocess/src/decoder.hpp +++ b/3rdparty/libprocess/src/decoder.hpp @@ -108,13 +108,6 @@ private: CHECK(decoder->request == NULL); decoder->request = new http::Request(); - decoder->request->headers.clear(); - decoder->request->method.clear(); - decoder->request->path.clear(); - decoder->request->url.clear(); - decoder->request->fragment.clear(); - decoder->request->query.clear(); - decoder->request->body.clear(); return 0; } @@ -124,7 +117,7 @@ private: { DataDecoder* decoder = (DataDecoder*) p->data; CHECK_NOTNULL(decoder->request); - decoder->request->path.append(data, length); + decoder->request->url.path.append(data, length); return 0; } @@ -140,7 +133,12 @@ private: { DataDecoder* decoder = (DataDecoder*) p->data; CHECK_NOTNULL(decoder->request); - decoder->request->fragment.append(data, length); + + if (decoder->request->url.fragment.isNone()) { + decoder->request->url.fragment = ""; + } + + decoder->request->url.fragment->append(data, length); return 0; } #endif // !(HTTP_PARSER_VERSION_MAJOR >= 2) @@ -149,7 +147,6 @@ private: { DataDecoder* decoder = (DataDecoder*) p->data; CHECK_NOTNULL(decoder->request); - decoder->request->url.append(data, length); int result = 0; #if (HTTP_PARSER_VERSION_MAJOR >= 2) @@ -248,7 +245,7 @@ private: CHECK_NOTNULL(decoder->request); - decoder->request->query = decoded.get(); + decoder->request->url.query = decoded.get(); Option<std::string> encoding = decoder->request->headers.get("Content-Encoding"); http://git-wip-us.apache.org/repos/asf/mesos/blob/d347bf56/3rdparty/libprocess/src/help.cpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/src/help.cpp b/3rdparty/libprocess/src/help.cpp index d358b93..822c084 100644 --- a/3rdparty/libprocess/src/help.cpp +++ b/3rdparty/libprocess/src/help.cpp @@ -109,7 +109,7 @@ void Help::initialize() Future<http::Response> Help::help(const http::Request& request) { // Split the path by '/'. - vector<string> tokens = strings::tokenize(request.path, "/"); + vector<string> tokens = strings::tokenize(request.url.path, "/"); Option<string> id = None(); Option<string> name = None(); http://git-wip-us.apache.org/repos/asf/mesos/blob/d347bf56/3rdparty/libprocess/src/logging.cpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/src/logging.cpp b/3rdparty/libprocess/src/logging.cpp index db76abe..bb1131f 100644 --- a/3rdparty/libprocess/src/logging.cpp +++ b/3rdparty/libprocess/src/logging.cpp @@ -29,8 +29,8 @@ namespace process { Future<http::Response> Logging::toggle(const http::Request& request) { - Option<std::string> level = request.query.get("level"); - Option<std::string> duration = request.query.get("duration"); + Option<std::string> level = request.url.query.get("level"); + Option<std::string> duration = request.url.query.get("duration"); if (level.isNone() && duration.isNone()) { return http::OK(stringify(FLAGS_v) + "\n"); http://git-wip-us.apache.org/repos/asf/mesos/blob/d347bf56/3rdparty/libprocess/src/metrics/metrics.cpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/src/metrics/metrics.cpp b/3rdparty/libprocess/src/metrics/metrics.cpp index 943ba63..0cf844c 100644 --- a/3rdparty/libprocess/src/metrics/metrics.cpp +++ b/3rdparty/libprocess/src/metrics/metrics.cpp @@ -115,8 +115,8 @@ Future<http::Response> MetricsProcess::_snapshot(const http::Request& request) // Parse the 'timeout' parameter. Option<Duration> timeout; - if (request.query.contains("timeout")) { - string parameter = request.query.get("timeout").get(); + if (request.url.query.contains("timeout")) { + string parameter = request.url.query.get("timeout").get(); Try<Duration> duration = Duration::parse(parameter); @@ -192,7 +192,7 @@ Future<http::Response> MetricsProcess::__snapshot( } } - return http::OK(object, request.query.get("jsonp")); + return http::OK(object, request.url.query.get("jsonp")); } } // namespace internal { http://git-wip-us.apache.org/repos/asf/mesos/blob/d347bf56/3rdparty/libprocess/src/process.cpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/src/process.cpp b/3rdparty/libprocess/src/process.cpp index 7a2b911..d1c81f1 100644 --- a/3rdparty/libprocess/src/process.cpp +++ b/3rdparty/libprocess/src/process.cpp @@ -556,11 +556,11 @@ static Message* parse(Request* request) } // Now determine 'to'. - size_t index = request->path.find('/', 1); + size_t index = request->url.path.find('/', 1); index = index != string::npos ? index - 1 : string::npos; // Decode possible percent-encoded 'to'. - Try<string> decode = http::decode(request->path.substr(1, index)); + Try<string> decode = http::decode(request->url.path.substr(1, index)); if (decode.isError()) { VLOG(2) << "Failed to decode URL path: " << decode.get(); @@ -570,8 +570,8 @@ static Message* parse(Request* request) const UPID to(decode.get(), __address__); // And now determine 'name'. - index = index != string::npos ? index + 2: request->path.size(); - const string name = request->path.substr(index); + index = index != string::npos ? index + 2: request->url.path.size(); + const string name = request->url.path.substr(index); VLOG(2) << "Parsed message name '" << name << "' for " << to << " from " << from.get(); @@ -2226,11 +2226,11 @@ bool ProcessManager::handle( Option<string> agent = request->headers.get("User-Agent"); if (agent.getOrElse("").find("libprocess/") == string::npos) { if (accepted) { - VLOG(2) << "Accepted libprocess message to " << request->path; + VLOG(2) << "Accepted libprocess message to " << request->url.path; dispatch(proxy, &HttpProxy::enqueue, Accepted(), *request); } else { VLOG(1) << "Failed to handle libprocess message to " - << request->path << ": not found"; + << request->url.path << ": not found"; dispatch(proxy, &HttpProxy::enqueue, NotFound(), *request); } } @@ -2240,7 +2240,7 @@ bool ProcessManager::handle( } VLOG(1) << "Failed to handle libprocess message: " - << request->method << " " << request->path + << request->method << " " << request->url.path << " (User-Agent: " << request->headers["User-Agent"] << ")"; delete request; @@ -2249,8 +2249,8 @@ bool ProcessManager::handle( // Treat this as an HTTP request. Start by checking that the path // starts with a '/' (since the code below assumes as much). - if (request->path.find('/') != 0) { - VLOG(1) << "Returning '400 Bad Request' for '" << request->path << "'"; + if (request->url.path.find('/') != 0) { + VLOG(1) << "Returning '400 Bad Request' for '" << request->url.path << "'"; // Get the HttpProxy pid for this socket. PID<HttpProxy> proxy = socket_manager->proxy(socket); @@ -2265,8 +2265,8 @@ bool ProcessManager::handle( } // Ignore requests with relative paths (i.e., contain "/.."). - if (request->path.find("/..") != string::npos) { - VLOG(1) << "Returning '404 Not Found' for '" << request->path + if (request->url.path.find("/..") != string::npos) { + VLOG(1) << "Returning '404 Not Found' for '" << request->url.path << "' (ignoring requests with relative paths)"; // Get the HttpProxy pid for this socket. @@ -2282,13 +2282,13 @@ bool ProcessManager::handle( } // Split the path by '/'. - vector<string> tokens = strings::tokenize(request->path, "/"); + vector<string> tokens = strings::tokenize(request->url.path, "/"); // Try and determine a receiver, otherwise try and delegate. ProcessReference receiver; if (tokens.size() == 0 && delegate != "") { - request->path = "/" + delegate; + request->url.path = "/" + delegate; receiver = use(UPID(delegate, __address__)); } else if (tokens.size() > 0) { // Decode possible percent-encoded path. @@ -2302,7 +2302,7 @@ bool ProcessManager::handle( if (!receiver && delegate != "") { // Try and delegate the request. - request->path = "/" + delegate + request->path; + request->url.path = "/" + delegate + request->url.path; receiver = use(UPID(delegate, __address__)); } @@ -2313,7 +2313,7 @@ bool ProcessManager::handle( Option<Response> rejection = rule->apply(socket, *request); if (rejection.isSome()) { VLOG(1) << "Returning '"<< rejection.get().status << "' for '" - << request->path << "' (firewall rule forbids request)"; + << request->url.path << "' (firewall rule forbids request)"; // TODO(arojas): Get rid of the duplicated code to return an // error. @@ -2343,7 +2343,7 @@ bool ProcessManager::handle( } // This has no receiver, send error response. - VLOG(1) << "Returning '404 Not Found' for '" << request->path << "'"; + VLOG(1) << "Returning '404 Not Found' for '" << request->url.path << "'"; // Get the HttpProxy pid for this socket. PID<HttpProxy> proxy = socket_manager->proxy(socket); @@ -2924,7 +2924,7 @@ Future<Response> ProcessManager::__processes__(const Request&) const Request& request = *event.request; object.values["method"] = request.method; - object.values["url"] = request.url; + object.values["url"] = stringify(request.url); events->values.push_back(object); } @@ -3073,12 +3073,12 @@ void ProcessBase::visit(const DispatchEvent& event) void ProcessBase::visit(const HttpEvent& event) { VLOG(1) << "Handling HTTP event for process '" << pid.id << "'" - << " with path: '" << event.request->path << "'"; + << " with path: '" << event.request->url.path << "'"; - CHECK(event.request->path.find('/') == 0); // See ProcessManager::handle. + CHECK(event.request->url.path.find('/') == 0); // See ProcessManager::handle. // Split the path by '/'. - vector<string> tokens = strings::tokenize(event.request->path, "/"); + vector<string> tokens = strings::tokenize(event.request->url.path, "/"); CHECK(tokens.size() >= 1); CHECK_EQ(pid.id, http::decode(tokens[0]).get()); @@ -3087,7 +3087,7 @@ void ProcessBase::visit(const HttpEvent& event) // Remove the 'id' prefix from the path. string name = strings::remove( - event.request->path, "/" + tokens[0], strings::PREFIX); + event.request->url.path, "/" + tokens[0], strings::PREFIX); name = strings::trim(name, strings::PREFIX, "/"); @@ -3151,7 +3151,8 @@ void ProcessBase::visit(const HttpEvent& event) return; } - VLOG(1) << "Returning '404 Not Found' for '" << event.request->path << "'"; + VLOG(1) << "Returning '404 Not Found' for" + << " '" << event.request->url.path << "'"; // Get the HttpProxy pid for this socket. PID<HttpProxy> proxy = socket_manager->proxy(event.socket); http://git-wip-us.apache.org/repos/asf/mesos/blob/d347bf56/3rdparty/libprocess/src/tests/benchmarks.cpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/src/tests/benchmarks.cpp b/3rdparty/libprocess/src/tests/benchmarks.cpp index 97c81b8..bb9ced8 100644 --- a/3rdparty/libprocess/src/tests/benchmarks.cpp +++ b/3rdparty/libprocess/src/tests/benchmarks.cpp @@ -95,10 +95,10 @@ private: } hashmap<string, Option<string>> parameters { - {"server", request.query.get("server")}, - {"messageSize", request.query.get("messageSize")}, - {"requests", request.query.get("requests")}, - {"concurrency", request.query.get("concurrency")}, + {"server", request.url.query.get("server")}, + {"messageSize", request.url.query.get("messageSize")}, + {"requests", request.url.query.get("requests")}, + {"concurrency", request.url.query.get("concurrency")}, }; // Ensure all parameters were provided. http://git-wip-us.apache.org/repos/asf/mesos/blob/d347bf56/3rdparty/libprocess/src/tests/decoder_tests.cpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/src/tests/decoder_tests.cpp b/3rdparty/libprocess/src/tests/decoder_tests.cpp index 6994fa9..858b40f 100644 --- a/3rdparty/libprocess/src/tests/decoder_tests.cpp +++ b/3rdparty/libprocess/src/tests/decoder_tests.cpp @@ -50,21 +50,21 @@ TEST(DecoderTest, Request) Request* request = requests[0]; EXPECT_EQ("GET", request->method); - EXPECT_EQ("/path/file.json", request->path); - EXPECT_EQ("/path/file.json?key1=value1&key2=value2#fragment", request->url); - EXPECT_EQ("fragment", request->fragment); + + EXPECT_EQ("/path/file.json", request->url.path); + EXPECT_SOME_EQ("fragment", request->url.fragment); + EXPECT_EQ(2u, request->url.query.size()); + EXPECT_SOME_EQ("value1", request->url.query.get("key1")); + EXPECT_SOME_EQ("value2", request->url.query.get("key2")); + EXPECT_TRUE(request->body.empty()); EXPECT_FALSE(request->keepAlive); - EXPECT_EQ(3, request->headers.size()); + EXPECT_EQ(3u, request->headers.size()); EXPECT_SOME_EQ("localhost", request->headers.get("Host")); EXPECT_SOME_EQ("close", request->headers.get("Connection")); EXPECT_SOME_EQ("compress, gzip", request->headers.get("Accept-Encoding")); - EXPECT_EQ(2, request->query.size()); - EXPECT_SOME_EQ("value1", request->query.get("key1")); - EXPECT_SOME_EQ("value2", request->query.get("key2")); - delete request; } http://git-wip-us.apache.org/repos/asf/mesos/blob/d347bf56/3rdparty/libprocess/src/tests/http_tests.cpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/src/tests/http_tests.cpp b/3rdparty/libprocess/src/tests/http_tests.cpp index d0b9399..a52561e 100644 --- a/3rdparty/libprocess/src/tests/http_tests.cpp +++ b/3rdparty/libprocess/src/tests/http_tests.cpp @@ -395,10 +395,10 @@ http::Response validateGetWithoutQuery(const http::Request& request) { EXPECT_NE(process::address(), request.client); EXPECT_EQ("GET", request.method); - EXPECT_THAT(request.path, EndsWith("get")); + EXPECT_THAT(request.url.path, EndsWith("get")); EXPECT_EQ("", request.body); - EXPECT_EQ("", request.fragment); - EXPECT_TRUE(request.query.empty()); + EXPECT_NONE(request.url.fragment); + EXPECT_TRUE(request.url.query.empty()); return http::OK(); } @@ -408,11 +408,11 @@ http::Response validateGetWithQuery(const http::Request& request) { EXPECT_NE(process::address(), request.client); EXPECT_EQ("GET", request.method); - EXPECT_THAT(request.path, EndsWith("get")); + EXPECT_THAT(request.url.path, EndsWith("get")); EXPECT_EQ("", request.body); - EXPECT_EQ("", request.fragment); - EXPECT_EQ("bar", request.query.at("foo")); - EXPECT_EQ(1, request.query.size()); + EXPECT_NONE(request.url.fragment); + EXPECT_EQ("bar", request.url.query.at("foo")); + EXPECT_EQ(1, request.url.query.size()); return http::OK(); } @@ -573,10 +573,10 @@ TEST(HTTPTest, PipeEquality) http::Response validatePost(const http::Request& request) { EXPECT_EQ("POST", request.method); - EXPECT_THAT(request.path, EndsWith("post")); + EXPECT_THAT(request.url.path, EndsWith("post")); EXPECT_EQ("This is the payload.", request.body); - EXPECT_EQ("", request.fragment); - EXPECT_TRUE(request.query.empty()); + EXPECT_NONE(request.url.fragment); + EXPECT_TRUE(request.url.query.empty()); return http::OK(); } @@ -623,9 +623,9 @@ TEST(HTTPTest, Post) http::Response validateDelete(const http::Request& request) { EXPECT_EQ("DELETE", request.method); - EXPECT_THAT(request.path, EndsWith("delete")); + EXPECT_THAT(request.url.path, EndsWith("delete")); EXPECT_TRUE(request.body.empty()); - EXPECT_TRUE(request.query.empty()); + EXPECT_TRUE(request.url.query.empty()); return http::OK(); }
