[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 d3afa34b6b
PiperOrigin-RevId: 695779969

src/core/util/http_client/httpcli.cc

@@ -79,8 +79,9 @@ OrphanablePtr<HttpRequest> HttpRequest::Get(
   }
   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 @@ OrphanablePtr<HttpRequest> HttpRequest::Post(
   }
   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 @@ OrphanablePtr<HttpRequest> HttpRequest::Put(
   }
   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::AppendError(grpc_error_handle error) {
 
 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::ContinueDoneWriteAfterScheduleOnExecCtx(
 }
 
 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

src/core/util/uri.cc

@@ -352,6 +352,16 @@ std::string URI::ToString() const {
     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 @@ std::string URI::ToString() const {
     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, "");
 }
 

src/core/util/uri.h

@@ -76,7 +76,7 @@ class URI {
     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 @@ class URI {
 
   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);

test/core/http/httpcli_test.cc

@@ -193,8 +193,10 @@ TEST_F(HttpRequestTest, Get) {
   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 @@ TEST_F(HttpRequestTest, Post) {
   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(

test/core/http/httpscli_test.cc

@@ -191,8 +191,10 @@ TEST_F(HttpsCliTest, Get) {
       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 @@ TEST_F(HttpsCliTest, Post) {
       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(

test/core/http/test_server.py

@@ -59,13 +59,13 @@ class Handler(BaseHTTPRequestHandler):
         )
 
     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()