http: improvement on http api (#296)

.travis.yml

@@ -18,19 +18,16 @@ addons:
             - clang-format-3.9
 
 before_install:
-    - wget https://raw.githubusercontent.com/xiaomi/pegasus-common/master/build-depends.tar.gz
+    - wget https://media.githubusercontent.com/media/XiaoMi/pegasus-common/master/build-depends.tar.gz
     - tar xfz build-depends.tar.gz
     - rm -f build-depends.tar.gz
     - cd packages
     - ls | xargs sudo dpkg -i --force-depends
     - cd ..
 
-install:
-    # - ./run.sh format
-
 before_script:
     - cd thirdparty
-    - wget https://raw.githubusercontent.com/xiaomi/pegasus-common/master/pegasus-thirdparty-prebuild.tar.gz
+    - wget https://media.githubusercontent.com/media/XiaoMi/pegasus-common/master/pegasus-thirdparty-prebuild.tar.gz
     - tar xfz pegasus-thirdparty-prebuild.tar.gz
     - rm -f pegasus-thirdparty-prebuild.tar.gz
     - cd ..

include/dsn/tool-api/http_server.h

@@ -24,7 +24,7 @@ struct http_request
     // http://ip:port/<service>/<method>
     std::pair<std::string, std::string> service_method;
     // <args_name, args_val>
-    std::map<std::string, std::string> query_args;
+    std::unordered_map<std::string, std::string> query_args;
     blob body;
     blob full_url;
     http_method method;
@@ -71,7 +71,7 @@ class http_service
         if (it != _cb_map.end()) {
             it->second(req, resp);
         } else {
-            resp.status_code = http_status_code::bad_request;
+            resp.status_code = http_status_code::not_found;
             resp.body = std::string("method not found for \"") + req.service_method.second + "\"";
         }
     }

include/dsn/utility/output_utils.h

@@ -62,7 +62,7 @@ void json_encode(Writer &out, const table_printer &tp);
 ///    tp.output(out, output_format::kTabular);
 ///
 /// Output looks like:
-///    sample_data
+///    [sample_data]
 ///    table_title  column_name1  column_name2
 ///    row_name_1   123           45.67
 ///    row_name_2   456           45.68
@@ -76,7 +76,7 @@ void json_encode(Writer &out, const table_printer &tp);
 ///    tp.output(out);
 ///
 /// Output looks like:
-///    sample_data_2
+///    [sample_data_2]
 ///    row_name_1 :  4567
 ///    row_name_2 :  hello
 ///

src/core/core/output_utils.cpp

@@ -37,10 +37,11 @@ void json_encode(Writer &writer, const table_printer &tp)
     if (tp._matrix_data.empty()) {
         return;
     }
-
-    dsn::json::json_encode(writer, tp._name); // table_printer name
-    if (tp._mode == table_printer::data_mode::kMultiColumns) {
+    if (!tp._name.empty()) {
+        json::json_encode(writer, tp._name); // table_printer name
         writer.StartObject();
+    }
+    if (tp._mode == table_printer::data_mode::kMultiColumns) {
         // The 1st row elements are column names, skip it.
         for (size_t row = 1; row < tp._matrix_data.size(); ++row) {
             dsn::json::json_encode(writer, tp._matrix_data[row][0]); // row name
@@ -51,17 +52,17 @@ void json_encode(Writer &writer, const table_printer &tp)
             }
             writer.EndObject();
         }
-        writer.EndObject();
     } else if (tp._mode == table_printer::data_mode::kSingleColumn) {
-        writer.StartObject();
         for (size_t row = 0; row < tp._matrix_data.size(); ++row) {
             dsn::json::json_encode(writer, tp._matrix_data[row][0]); // row name
             dsn::json::json_encode(writer, tp._matrix_data[row][1]); // row data
         }
-        writer.EndObject();
     } else {
         dassert(false, "Unknown mode");
     }
+    if (!tp._name.empty()) {
+        writer.EndObject();
+    }
 }
 
 void table_printer::add_title(const std::string &title, alignment align)

src/core/tests/output_utils_test.cpp

@@ -145,6 +145,19 @@ TEST(table_printer_test, empty_content_test)
     ASSERT_NO_FATAL_FAILURE(check_output(tp, {"", "{}\n", "{}\n"}));
 }
 
+TEST(table_printer_test, empty_name_test)
+{
+    utils::table_printer tp;
+    tp.add_row_name_and_data("row1", 1.234);
+    ASSERT_NO_FATAL_FAILURE(check_output(tp,
+                                         {"row1  : 1.23\n",
+                                          R"*({"row1":"1.23"})*"
+                                          "\n",
+                                          "{\n"
+                                          R"*(    "row1": "1.23")*"
+                                          "\n}\n"}));
+}
+
 TEST(table_printer_test, single_column_test)
 {
     utils::table_printer tp(generate_single_column_tp());

src/dist/http/CMakeLists.txt

@@ -4,7 +4,7 @@ set(MY_PROJ_SRC "")
 
 set(MY_SRC_SEARCH_MODE "GLOB")
 
-set(MY_PROJ_LIBS "")
+set(MY_PROJ_LIBS fmt)
 
 dsn_add_static_library()
 

src/dist/http/http_server.cpp

@@ -5,6 +5,7 @@
 #include <dsn/tool-api/http_server.h>
 #include <dsn/tool_api.h>
 #include <boost/algorithm/string.hpp>
+#include <fmt/ostream.h>
 
 #include "http_message_parser.h"
 #include "root_http_service.h"
@@ -50,17 +51,16 @@ void http_server::serve(message_ex *msg)
     error_with<http_request> res = http_request::parse(msg);
     http_response resp;
     if (!res.is_ok()) {
-        derror("failed to parse request: %s", res.get_error().description().c_str());
         resp.status_code = http_status_code::bad_request;
-        resp.body = "failed to parse request";
+        resp.body = fmt::format("failed to parse request: {}", res.get_error());
     } else {
         const http_request &req = res.get_value();
         auto it = _service_map.find(req.service_method.first);
         if (it != _service_map.end()) {
             it->second->call(req, resp);
         } else {
-            resp.status_code = http_status_code::bad_request;
-            resp.body = "service not found";
+            resp.status_code = http_status_code::not_found;
+            resp.body = fmt::format("service not found for \"{}\"", req.service_method.first);
         }
     }
 
@@ -117,29 +117,34 @@ void http_server::add_service(http_service *service)
         return error_s::make(ERR_INVALID_PARAMETERS);
     }
     if (real_args.size() == 1) {
-        ret.service_method = std::make_pair(std::string(real_args[0]), std::string(""));
-        return ret;
-    }
-    if (real_args.size() == 0) {
-        ret.service_method = std::make_pair(std::string(""), std::string(""));
-        return ret;
+        ret.service_method = {std::string(real_args[0]), ""};
+    } else if (real_args.size() == 0) {
+        ret.service_method = {"", ""};
+    } else {
+        ret.service_method = {std::string(real_args[0]), std::string(real_args[1])};
     }
 
-    ret.service_method = std::make_pair(std::string(real_args[0]), std::string(real_args[1]));
-
     // find if there are method args (<ip>:<port>/<service>/<method>?<arg>=<val>&<arg>=<val>)
     if (!unresolved_query.empty()) {
         std::vector<std::string> method_arg_val;
         boost::split(method_arg_val, unresolved_query, boost::is_any_of("&"));
-        for (std::string &arg_val : method_arg_val) {
-            std::vector<std::string> arg_vals;
-            boost::split(arg_vals, arg_val, boost::is_any_of("="));
-            if (arg_vals.size() != 2)
-                return error_s::make(ERR_INVALID_PARAMETERS);
-            auto iter = ret.query_args.find(arg_vals[0]);
-            if (iter != ret.query_args.end())
-                return error_s::make(ERR_INVALID_PARAMETERS, std::string("repeat parameters"));
-            ret.query_args.emplace(arg_vals[0], arg_vals[1]);
+        for (const std::string &arg_val : method_arg_val) {
+            size_t sep = arg_val.find_first_of('=');
+            if (sep == std::string::npos) {
+                // assume this as a bool flag
+                ret.query_args.emplace(arg_val, "");
+                continue;
+            }
+            std::string name = arg_val.substr(0, sep);
+            std::string value;
+            if (sep + 1 < arg_val.size()) {
+                value = arg_val.substr(sep + 1, arg_val.size() - sep);
+            }
+            auto iter = ret.query_args.find(name);
+            if (iter != ret.query_args.end()) {
+                return FMT_ERR(ERR_INVALID_PARAMETERS, "duplicate parameter: {}", name);
+            }
+            ret.query_args.emplace(std::move(name), std::move(value));
         }
     }
 
@@ -154,7 +159,9 @@ message_ptr http_response::to_message(message_ex *req) const
     os << "HTTP/1.1 " << http_status_code_to_string(status_code) << "\r\n";
     os << "Content-Type: " << content_type << "\r\n";
     os << "Content-Length: " << body.length() << "\r\n";
-    os << "Location: " << location << "\r\n";
+    if (!location.empty()) {
+        os << "Location: " << location << "\r\n";
+    }
     os << "\r\n";
     os << body;
 

src/dist/http/server_info_http_services.cpp

@@ -11,7 +11,7 @@ namespace dsn {
 void version_http_service::get_version_handler(const http_request &req, http_response &resp)
 {
     std::ostringstream out;
-    dsn::utils::table_printer tp("Version");
+    dsn::utils::table_printer tp;
     tp.add_row_name_and_data("Version", _version);
     tp.add_row_name_and_data("GitCommit", _git_commit);
     tp.output(out, dsn::utils::table_printer::output_format::kJsonCompact);
@@ -26,7 +26,7 @@ void recent_start_time_http_service::get_recent_start_time_handler(const http_re
     char start_time[100];
     dsn::utils::time_ms_to_date_time(dsn::utils::process_start_millis(), start_time, 100);
     std::ostringstream out;
-    dsn::utils::table_printer tp("RecentStartTime");
+    dsn::utils::table_printer tp;
     tp.add_row_name_and_data("RecentStartTime", start_time);
     tp.output(out, dsn::utils::table_printer::output_format::kJsonCompact);
 

src/dist/http/server_info_http_services.h

@@ -47,7 +47,7 @@ class recent_start_time_http_service : public http_service
                                    std::placeholders::_2));
     }
 
-    std::string path() const override { return "startTime"; }
+    std::string path() const override { return "recentStartTime"; }
 
     void get_recent_start_time_handler(const http_request &req, http_response &resp);
 };

src/dist/http/test/CMakeLists.txt

@@ -9,6 +9,7 @@ set(MY_PROJ_LIBS
     dsn_runtime
     gtest
     gtest_main
+    fmt
     )
 
 set(MY_BOOST_LIBS Boost::system Boost::filesystem)

src/dist/http/test/http_server_test.cpp

@@ -228,4 +228,42 @@ TEST_F(http_message_parser_test, parse_long_url)
     ASSERT_EQ(msg->buffers[2].size(), 4097); // url
 }
 
+TEST_F(http_message_parser_test, parse_query_params)
+{
+    struct test_case
+    {
+        std::string url;
+
+        error_code err;
+        std::unordered_map<std::string, std::string> result;
+    } tests[] = {
+        {"http://127.0.0.1:34601?query1", ERR_OK, {{"query1", ""}}},
+        {"http://127.0.0.1:34601?query1=", ERR_OK, {{"query1", ""}}},
+        {"http://127.0.0.1:34601?query1=value1", ERR_OK, {{"query1", "value1"}}},
+        {"http://127.0.0.1:34601?=", ERR_OK, {{"", ""}}},
+        {"http://127.0.0.1:34601?", ERR_OK, {}},
+
+        {"http://127.0.0.1:34601?query1=value1&query2=value2",
+         ERR_OK,
+         {{"query1", "value1"}, {"query2", "value2"}}},
+
+        {"http://127.0.0.1:34601?query1=value1&query2",
+         ERR_OK,
+         {{"query1", "value1"}, {"query2", ""}}},
+    };
+
+    for (auto tt : tests) {
+        ref_ptr<message_ex> m = message_ex::create_receive_message_with_standalone_header(
+            blob::create_from_bytes(std::string("POST")));
+        m->buffers.emplace_back(blob::create_from_bytes(std::string(tt.url)));
+
+        auto res = http_request::parse(m.get());
+        if (res.is_ok()) {
+            ASSERT_EQ(res.get_value().query_args, tt.result) << tt.url;
+        } else {
+            ASSERT_EQ(res.get_error().code(), tt.err);
+        }
+    }
+}
+
 } // namespace dsn

src/dist/replication/meta_server/meta_http_service.cpp

@@ -38,28 +38,25 @@ void meta_http_service::get_app_handler(const http_request &req, http_response &
         if (p.first == "name") {
             app_name = p.second;
         } else if (p.first == "detail") {
-            if (p.second == "false")
-                detailed = false;
-            else if (p.second == "true")
-                detailed = true;
-            else {
-                resp.status_code = http_status_code::bad_request;
-                return;
-            }
+            detailed = true;
         } else {
             resp.status_code = http_status_code::bad_request;
             return;
         }
     }
-    if (!is_primary(req, resp))
+    if (!redirect_if_not_primary(req, resp))
         return;
 
     configuration_query_by_index_request request;
     configuration_query_by_index_response response;
 
     request.app_name = app_name;
     _service->_state->query_configuration_by_index(request, response);
-
+    if (response.err == ERR_OBJECT_NOT_FOUND) {
+        resp.status_code = http_status_code::not_found;
+        resp.body = fmt::format("table not found: \"{}\"", app_name);
+        return;
+    }
     if (response.err != dsn::ERR_OK) {
         resp.body = response.err.to_string();
         resp.status_code = http_status_code::internal_server_error;
@@ -170,20 +167,13 @@ void meta_http_service::list_app_handler(const http_request &req, http_response
     bool detailed = false;
     for (const auto &p : req.query_args) {
         if (p.first == "detail") {
-            if (p.second == "false")
-                detailed = false;
-            else if (p.second == "true")
-                detailed = true;
-            else {
-                resp.status_code = http_status_code::bad_request;
-                return;
-            }
+            detailed = true;
         } else {
             resp.status_code = http_status_code::bad_request;
             return;
         }
     }
-    if (!is_primary(req, resp))
+    if (!redirect_if_not_primary(req, resp))
         return;
     configuration_list_apps_response response;
     configuration_list_apps_request request;
@@ -348,20 +338,13 @@ void meta_http_service::list_node_handler(const http_request &req, http_response
     bool detailed = false;
     for (const auto &p : req.query_args) {
         if (p.first == "detail") {
-            if (p.second == "false")
-                detailed = false;
-            else if (p.second == "true")
-                detailed = true;
-            else {
-                resp.status_code = http_status_code::bad_request;
-                return;
-            }
+            detailed = true;
         } else {
             resp.status_code = http_status_code::bad_request;
             return;
         }
     }
-    if (!is_primary(req, resp))
+    if (!redirect_if_not_primary(req, resp))
         return;
 
     std::map<dsn::rpc_address, list_nodes_helper> tmp_map;
@@ -446,9 +429,10 @@ void meta_http_service::list_node_handler(const http_request &req, http_response
 
 void meta_http_service::get_cluster_info_handler(const http_request &req, http_response &resp)
 {
-    if (!is_primary(req, resp))
+    if (!redirect_if_not_primary(req, resp))
         return;
-    dsn::utils::table_printer tp("cluster_info");
+
+    dsn::utils::table_printer tp;
     std::ostringstream out;
     std::string meta_servers_str;
     int ms_size = _service->_opts.meta_servers.size();
@@ -483,7 +467,7 @@ void meta_http_service::get_cluster_info_handler(const http_request &req, http_r
     resp.status_code = http_status_code::ok;
 }
 
-bool meta_http_service::is_primary(const http_request &req, http_response &resp)
+bool meta_http_service::redirect_if_not_primary(const http_request &req, http_response &resp)
 {
 #ifdef DSN_MOCK_TEST
     return true;