Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/21920 )
Change subject: Implement path parameter handling in webserver ...................................................................... Patch Set 4: (2 comments) http://gerrit.cloudera.org:8080/#/c/21920/4/src/kudu/server/webserver-test.cc File src/kudu/server/webserver-test.cc: http://gerrit.cloudera.org:8080/#/c/21920/4/src/kudu/server/webserver-test.cc@668 PS4, Line 668: void PathParamHandler(const Webserver::WebRequest& req, Webserver::WebResponse* resp) { : EasyJson* output = &resp->output; : for (const auto& param : req.path_params) { : (*output)[param.first] = param.second; : } : for (const auto& param : req.parsed_args) { : (*output)[param.first] = param.second; : } : } Can you please put some comments for this section. For first time test readers it might be useful. Something like: "this path param handler just gives back the params in .... this format etc" http://gerrit.cloudera.org:8080/#/c/21920/4/src/kudu/server/webserver-test.cc@719 PS4, Line 719: TEST_F(PathParamWebserverTest, TestPathWithQueryString) { : ASSERT_OK(curl_.FetchURL( : Substitute("$0/api/tables/45dc8d192549427b8dca871dbbb20bb3?foo=bar", url_), &buf_)); : ASSERT_STR_CONTAINS(buf_.ToString(), "\"table_id\":\"45dc8d192549427b8dca871dbbb20bb3\""); : ASSERT_STR_CONTAINS(buf_.ToString(), "\"foo\":\"bar\""); : } Is this what we are aiming for? I mean this way what you get back is a map where you have path params and query params mixed, right? -- To view, visit http://gerrit.cloudera.org:8080/21920 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id2085c55d3b86985c87c5fd5a8b48568f8e6fa63 Gerrit-Change-Number: 21920 Gerrit-PatchSet: 4 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Thu, 17 Oct 2024 15:12:27 +0000 Gerrit-HasComments: Yes
