[http1] fix HttpRequest to support query params (#38099)
Also add trace logging for HTTP requests and responses.
Closes #38099
COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/38099 from markdroth:gcp_auth_trace_logging d3afa34b6bfdb153ed47bfdf058c5781160b333d
PiperOrigin-RevId: 695779969
diff --git a/src/core/util/http_client/httpcli.cc b/src/core/util/http_client/httpcli.cc
index 0eb2292..20e7768 100644
--- a/src/core/util/http_client/httpcli.cc
+++ b/src/core/util/http_client/httpcli.cc
@@ -79,8 +79,9 @@
}
std::string name =
absl::StrFormat("HTTP:GET:%s:%s", uri.authority(), uri.path());
- const grpc_slice request_text = grpc_httpcli_format_get_request(
- request, uri.authority().c_str(), uri.path().c_str());
+ const grpc_slice request_text =
+ grpc_httpcli_format_get_request(request, uri.authority().c_str(),
+ uri.EncodedPathAndQueryParams().c_str());
return MakeOrphanable<HttpRequest>(
std::move(uri), request_text, response, deadline, channel_args, on_done,
pollent, name.c_str(), std::move(test_only_generate_response),
@@ -103,8 +104,9 @@
}
std::string name =
absl::StrFormat("HTTP:POST:%s:%s", uri.authority(), uri.path());
- const grpc_slice request_text = grpc_httpcli_format_post_request(
- request, uri.authority().c_str(), uri.path().c_str());
+ const grpc_slice request_text =
+ grpc_httpcli_format_post_request(request, uri.authority().c_str(),
+ uri.EncodedPathAndQueryParams().c_str());
return MakeOrphanable<HttpRequest>(
std::move(uri), request_text, response, deadline, channel_args, on_done,
pollent, name.c_str(), std::move(test_only_generate_response),
@@ -127,8 +129,9 @@
}
std::string name =
absl::StrFormat("HTTP:PUT:%s:%s", uri.authority(), uri.path());
- const grpc_slice request_text = grpc_httpcli_format_put_request(
- request, uri.authority().c_str(), uri.path().c_str());
+ const grpc_slice request_text =
+ grpc_httpcli_format_put_request(request, uri.authority().c_str(),
+ uri.EncodedPathAndQueryParams().c_str());
return MakeOrphanable<HttpRequest>(
std::move(uri), request_text, response, deadline, channel_args, on_done,
pollent, name.c_str(), std::move(test_only_generate_response),
@@ -241,6 +244,8 @@
void HttpRequest::OnReadInternal(grpc_error_handle error) {
for (size_t i = 0; i < incoming_.count; i++) {
+ GRPC_TRACE_LOG(http1, INFO)
+ << "HTTP response data: " << StringViewFromSlice(incoming_.slices[i]);
if (GRPC_SLICE_LENGTH(incoming_.slices[i])) {
have_read_byte_ = 1;
grpc_error_handle err =
@@ -275,6 +280,8 @@
}
void HttpRequest::StartWrite() {
+ GRPC_TRACE_LOG(http1, INFO)
+ << "Sending HTTP1 request: " << StringViewFromSlice(request_text_);
CSliceRef(request_text_);
grpc_slice_buffer_add(&outgoing_, request_text_);
Ref().release(); // ref held by pending write
diff --git a/src/core/util/uri.cc b/src/core/util/uri.cc
index e6a94e5..e7b8224 100644
--- a/src/core/util/uri.cc
+++ b/src/core/util/uri.cc
@@ -352,6 +352,16 @@
parts.emplace_back("//");
parts.emplace_back(PercentEncode(authority_, IsAuthorityChar));
}
+ parts.emplace_back(EncodedPathAndQueryParams());
+ if (!fragment_.empty()) {
+ parts.push_back("#");
+ parts.push_back(PercentEncode(fragment_, IsQueryOrFragmentChar));
+ }
+ return absl::StrJoin(parts, "");
+}
+
+std::string URI::EncodedPathAndQueryParams() const {
+ std::vector<std::string> parts;
if (!path_.empty()) {
parts.emplace_back(PercentEncode(path_, IsPathChar));
}
@@ -360,10 +370,6 @@
parts.push_back(
absl::StrJoin(query_parameter_pairs_, "&", QueryParameterFormatter()));
}
- if (!fragment_.empty()) {
- parts.push_back("#");
- parts.push_back(PercentEncode(fragment_, IsQueryOrFragmentChar));
- }
return absl::StrJoin(parts, "");
}
diff --git a/src/core/util/uri.h b/src/core/util/uri.h
index 14e9274..3f9dc7d 100644
--- a/src/core/util/uri.h
+++ b/src/core/util/uri.h
@@ -76,7 +76,7 @@
return query_parameter_map_;
}
// A vector of key:value query parameter pairs, kept in order of appearance
- // within the URI search string. Repeated keys are represented as separate
+ // within the URI string. Repeated keys are represented as separate
// key:value elements.
const std::vector<QueryParam>& query_parameter_pairs() const {
return query_parameter_pairs_;
@@ -85,6 +85,10 @@
std::string ToString() const;
+ // Returns the encoded path and query params, such as would be used on
+ // the wire in an HTTP request.
+ std::string EncodedPathAndQueryParams() const;
+
private:
URI(std::string scheme, std::string authority, std::string path,
std::vector<QueryParam> query_parameter_pairs, std::string fragment);
diff --git a/test/core/http/httpcli_test.cc b/test/core/http/httpcli_test.cc
index 0a3c508..ba3dc9d 100644
--- a/test/core/http/httpcli_test.cc
+++ b/test/core/http/httpcli_test.cc
@@ -193,8 +193,10 @@
std::string host = absl::StrFormat("localhost:%d", g_server_port);
LOG(INFO) << "requesting from " << host;
memset(&req, 0, sizeof(req));
- auto uri = grpc_core::URI::Create("http", host, "/get", {} /* query params */,
- "" /* fragment */);
+ auto uri = grpc_core::URI::Create(
+ "http", host, "/get",
+ /*query_parameter_pairs=*/{{"foo", "bar"}, {"baz", "quux"}},
+ /*fragment=*/"");
CHECK(uri.ok());
grpc_core::OrphanablePtr<grpc_core::HttpRequest> http_request =
grpc_core::HttpRequest::Get(
@@ -219,8 +221,10 @@
memset(&req, 0, sizeof(req));
req.body = const_cast<char*>("hello");
req.body_length = 5;
- auto uri = grpc_core::URI::Create("http", host, "/post",
- {} /* query params */, "" /* fragment */);
+ auto uri = grpc_core::URI::Create(
+ "http", host, "/post",
+ /*query_parameter_pairs=*/{{"foo", "bar"}, {"mumble", "frotz"}},
+ /*fragment=*/"");
CHECK(uri.ok());
grpc_core::OrphanablePtr<grpc_core::HttpRequest> http_request =
grpc_core::HttpRequest::Post(
diff --git a/test/core/http/httpscli_test.cc b/test/core/http/httpscli_test.cc
index 88c764b..8c8c8bf 100644
--- a/test/core/http/httpscli_test.cc
+++ b/test/core/http/httpscli_test.cc
@@ -191,8 +191,10 @@
const_cast<char*>(GRPC_SSL_TARGET_NAME_OVERRIDE_ARG),
const_cast<char*>("foo.test.google.fr"));
grpc_channel_args args = {1, &ssl_override_arg};
- auto uri = grpc_core::URI::Create("https", host, "/get",
- {} /* query params */, "" /* fragment */);
+ auto uri = grpc_core::URI::Create(
+ "https", host, "/get",
+ /*query_parameter_pairs=*/{{"foo", "bar"}, {"baz", "quux"}},
+ /*fragment=*/"");
CHECK(uri.ok());
grpc_core::OrphanablePtr<grpc_core::HttpRequest> http_request =
grpc_core::HttpRequest::Get(
@@ -219,8 +221,10 @@
const_cast<char*>(GRPC_SSL_TARGET_NAME_OVERRIDE_ARG),
const_cast<char*>("foo.test.google.fr"));
grpc_channel_args args = {1, &ssl_override_arg};
- auto uri = grpc_core::URI::Create("https", host, "/post",
- {} /* query params */, "" /* fragment */);
+ auto uri = grpc_core::URI::Create(
+ "https", host, "/post",
+ /*query_parameter_pairs=*/{{"foo", "bar"}, {"mumble", "frotz"}},
+ /*fragment=*/"");
CHECK(uri.ok());
grpc_core::OrphanablePtr<grpc_core::HttpRequest> http_request =
grpc_core::HttpRequest::Post(
diff --git a/test/core/http/test_server.py b/test/core/http/test_server.py
index 6da64fc..34cb717 100755
--- a/test/core/http/test_server.py
+++ b/test/core/http/test_server.py
@@ -59,13 +59,13 @@
)
def do_GET(self):
- if self.path == "/get":
+ if self.path == "/get?foo=bar&baz=quux":
self.good()
def do_POST(self):
content_len = self.headers.get("content-length")
content = self.rfile.read(int(content_len)).decode("ascii")
- if self.path == "/post" and content == "hello":
+ if self.path == "/post?foo=bar&mumble=frotz" and content == "hello":
self.good()
