Several improvements to RESTAPI interface #4001

This commit pack a couple of fixes and improvements for the RESTAPI: - Homogenization of GET/POST endpoint responses by ProxySQL. All responses should now be guaranteed to be valid JSON. - Fix JSON construction from parameters supplied to GET endpoint. - Add two new fields 'script_stdout' and 'script_stderr' to the JSON response when the target script fails to be executed. This is, when it exists with an error code other than zero. This makes the response homogeneous to when the scripts fails to produce a valid JSON output, and add extra information for debugging on client side. - Add a new debugging module 'PROXY_DEBUG_RESTAPI', to help tracing in debugging builds the requests to the endpoints.
sysown · Dec 9, 2022 · c83517c · c83517c
1 parent bcfc232
commit c83517c
Show file tree

Hide file tree

Showing 3 changed files with 128 additions and 91 deletions.
diff --git a/include/proxysql_structs.h b/include/proxysql_structs.h
@@ -128,6 +128,7 @@ enum debug_module {
 	PROXY_DEBUG_IPC,
 	PROXY_DEBUG_QUERY_CACHE,
 	PROXY_DEBUG_QUERY_STATISTICS,
+	PROXY_DEBUG_RESTAPI,
 	PROXY_DEBUG_UNKNOWN // this module doesn't exist. It is used only to define the last possible module
 };
 

diff --git a/lib/ProxySQL_RESTAPI_Server.cpp b/lib/ProxySQL_RESTAPI_Server.cpp
@@ -3,14 +3,10 @@
 #include "httpserver.hpp"
 
 #include <functional>
-#include <sstream>
 
 #include "ProxySQL_RESTAPI_Server.hpp"
 #include "proxysql_utils.h"
 
-using namespace httpserver;
-
-
 #ifdef DEBUG
 #define DEB "_DEBUG"
 #else
@@ -20,6 +16,9 @@ using namespace httpserver;
 
 extern ProxySQL_Admin *GloAdmin;
 
+using namespace httpserver;
+using nlohmann::json;
+
 class sync_resource : public http_resource {
 private:
 	void add_headers(std::shared_ptr<http_response> &response) {
@@ -29,61 +28,90 @@ class sync_resource : public http_resource {
 
 	const std::shared_ptr<http_response> find_script(const http_request& req, std::string& script, int &interval_ms) {
 		char *error=NULL;
-		std::stringstream ss;
-		ss << "SELECT * FROM runtime_restapi_routes WHERE uri='" << req.get_path_piece(1) << "' and method='" << req.get_method() << "' and active=1";
-		std::unique_ptr<SQLite3_result> resultset = std::unique_ptr<SQLite3_result>(GloAdmin->admindb->execute_statement(ss.str().c_str(), &error));
+		const string req_uri { req.get_path_piece(1) };
+		const string req_path { req.get_path() };
+		const string select_query {
+			"SELECT * FROM runtime_restapi_routes WHERE uri='" + req_uri + "' and"
+				" method='" + req.get_method() + "' and active=1"
+		};
+
+		std::unique_ptr<SQLite3_result> resultset {
+			std::unique_ptr<SQLite3_result>(GloAdmin->admindb->execute_statement(select_query.c_str(), &error))
+		};
+
 		if (!resultset) {
-			proxy_error("Cannot query script for given method [%s] and uri [%s]\n", req.get_method().c_str(), req.get_path_piece(1).c_str());
-			std::stringstream ss;
+			proxy_error(
+				"Cannot query script for given method [%s] and uri [%s]\n", req.get_method().c_str(), req_uri.c_str()
+			);
+			const string not_found_err_msg {
+				"The script for method [" + req.get_method() + "] and route [" + req.get_path() + "] was not found."
+			};
+			json j_err_resp {};
+
 			if (error) {
-				ss << "{\"error\":\"The script for method [" << req.get_method() << "] and route [" << req.get_path() << "] was not found. Error: " << error << "Error: \"}";
-				proxy_error("Path %s, error %s\n", req.get_path().c_str(), error);
-			}
-			else {
-				ss << "{\"error\":\"The script for method [" << req.get_method() << "] and route [" << req.get_path() << "] was not found.\"}";
-				proxy_error("Path %s\n", req.get_path().c_str());
+				j_err_resp = json { { "error", not_found_err_msg + " Error:" + error } };
+				proxy_error("Path '%s', error '%s'\n", req_path.c_str(), error);
+			} else {
+				j_err_resp = json { { "error", not_found_err_msg } };
+				proxy_error("Path '%s', error 'Path failed to be found on 'runtime_restapi_routes'\n", req_path.c_str());
 			}
-			auto response = std::shared_ptr<http_response>(new string_response(ss.str(), http::http_utils::http_bad_request));
+
+			auto response =
+				std::shared_ptr<http_response>(new string_response(j_err_resp.dump(), http::http_utils::http_bad_request));
 			add_headers(response);
+
 			return response;
-		}
-		if (resultset && resultset->rows_count != 1) {
-			std::stringstream ss;
-			ss << "{\"error\":\"The script for method [" << req.get_method() << "] and route [" << req.get_path() << "] was not found. Rows count returned [" << resultset->rows_count << "]\" }";
-			proxy_error("Script for method [%s] and route [%s] was not found\n", req.get_method().c_str(), req.get_path().c_str());
-			auto response = std::shared_ptr<http_response>(new string_response(ss.str(), http::http_utils::http_bad_request));
+		} else if (resultset && resultset->rows_count != 1) {
+			const string not_found_err_msg {
+				"The script for method [" + req.get_method() + "] and route [" + req_path + "] was not found."
+					" Rows count returned [" + std::to_string(resultset->rows_count) + "]"
+			};
+			json j_err_resp { { "error", not_found_err_msg } };
+
+			proxy_error(
+				"Script for method [%s] and uri [%s] was not found\n", req.get_method().c_str(), req_uri.c_str()
+			);
+			auto response =
+				std::shared_ptr<http_response>(new string_response(j_err_resp.dump(), http::http_utils::http_bad_request));
 			add_headers(response);
+
 			return response;
+		} else {
+			script = resultset->rows[0]->fields[5];
+			interval_ms = atoi(resultset->rows[0]->fields[2]);
+
+			return std::shared_ptr<http_response>(nullptr);
 		}
-		script = resultset->rows[0]->fields[5];
-		interval_ms = atoi(resultset->rows[0]->fields[2]);
-		return std::shared_ptr<http_response>(nullptr);
 	}
 
     const std::shared_ptr<http_response> process_request(const http_request& req, const std::string& _params) {
 		std::string params = req.get_content();
+		const string req_path { req.get_path() };
+
 		if (params.empty())
 			params = _params;
 		if (params.empty()) {
-			proxy_error("Empty parameters\n");
-
+			proxy_error("Path '%s', error 'Supplied empty parameters'\n", req_path.c_str());
+			json j_err_resp { { "type", "in" }, { "error", "Empty parameters" } };
 			auto response = std::shared_ptr<http_response>(
-				new string_response("{\"error\":\"Empty parameters\"}", http::http_utils::http_bad_request)
+				new string_response(j_err_resp.dump(), http::http_utils::http_bad_request)
 			);
+
 			add_headers(response);
 			return response;
 		}
 
 		try {
 			nlohmann::json valid=nlohmann::json::parse(params);
-		}
-		catch(nlohmann::json::exception& e) {
-			std::stringstream ss;
-			ss << "{\"type\":\"in\", \"error\":\"" << e.what() << "\"}";
-			proxy_error("Error parsing input json parameters. %s\n", ss.str().c_str());
+		} catch(nlohmann::json::exception& e) {
+			const string p_err_msg {
+				"parsing input JSON parameters - params: `" + params + "`, error: '" + e.what() + "'"
+			};
+			json j_err_resp { { "type", "in" }, { "error", "Error " + p_err_msg } };
+			proxy_error("Path '%s', error %s\n", req_path.c_str(), p_err_msg.c_str());
 
 			auto response = std::shared_ptr<http_response>(
-				new string_response(ss.str(), http::http_utils::http_bad_request)
+				new string_response(j_err_resp.dump(), http::http_utils::http_bad_request)
 			);
 			add_headers(response);
 			return response;
@@ -107,18 +135,24 @@ class sync_resource : public http_resource {
 		std::string script_stderr {""};
 
 		const std::vector<const char*> args { const_cast<const char*>(params.c_str()), NULL};
+		proxy_debug(PROXY_DEBUG_RESTAPI, 2, "Starting script exec - script: '%s', params: `%s`\n", script.c_str(), params.c_str());
 		int script_err = wexecvp(script.c_str(), args, wexecvp_opts, script_stdout, script_stderr);
+		proxy_debug(PROXY_DEBUG_RESTAPI, 2, "Finished script exec - script: '%s', params: `%s`\n", script.c_str(), params.c_str());
 		int script_errno = errno;
 
 		std::string str_response_err {};
 		bool internal_error = false;
 
 		// 'execvp' failed to be executed, the error code comes directly from the child process
 		if (script_err > 255) {
-			str_response_err =
-				std::string { "{\"type\":\"out\", \"error\":\"Script failed to be executed.\", \"error_code\":\"" }
-				+ std::to_string(script_err / 256)
-				+ "\"}";
+			json j_err_resp {
+				{ "type", "out" },
+				{ "error", "Script failed to be executed" },
+				{ "error_code", std::to_string(script_err / 256) },
+				{ "script_stdout", script_stdout },
+				{ "script_stderr", script_stderr }
+			};
+			str_response_err = j_err_resp.dump();
 			proxy_error(
 				"Script '%s' exited with errcode '%d': \n- script_stdout:\n'''\n%s'''\n- script_stderr:\n'''\n%s'''\n",
 				script.c_str(),
@@ -130,11 +164,12 @@ class sync_resource : public http_resource {
 		// there was an internal error while calling the executable, or request timedout.
 		else if (script_err < 256 && script_err != 0) {
 			if (script_err == ETIME) {
-				str_response_err =
-					std::string { "{\"type\":\"out\", \"error\":\"Script execution timed out.\", \"error_code\":\"" }
-					+ std::to_string(ETIME)
-					+ "\"}";
-
+				json j_err_resp {
+					{ "type", "out" },
+					{ "error", "Script execution timed out" },
+					{ "error_code", std::to_string(ETIME) }
+				};
+				str_response_err = j_err_resp.dump();
 				proxy_error("Request to execute script '%s' timed out.\n", script.c_str());
 			} else if (script_err < 0) {
 				// there was an internal error unrelated to script execution
@@ -159,56 +194,54 @@ class sync_resource : public http_resource {
 						failed_syscall = "unknown"; break;
 				}
 
-				str_response_err =
-					std::string {
-						"{\"type\":\"out\"," } +
-							"\"error\":\"Internal error while executing script, '" + failed_syscall + "' syscall failed.\", " +
-							"\"error_code\":\"" + std::to_string(script_errno)
-						+ "\"}";
+				json j_err_resp {
+					{ "type", "out" },
+					{ "error", "Internal error while executing script, '" + failed_syscall + "' syscall failed" },
+					{ "error_code", std::to_string(script_errno) }
+				};
+				str_response_err = j_err_resp.dump();
 				proxy_error(
 					"Internal error while executing script '%s'. '%s' syscall failed with error code: '%d'.\n",
 					script.c_str(),
 					failed_syscall.c_str(),
 					script_errno
 				);
 			} else {
-				str_response_err =
-					"{\"type\":\"out\", \"error\":\"Terminated without exit code. Child exit status reported in"
-						" 'error_code'\", \"error_code\":\"" + std::to_string(script_err) + "\"}";
-				proxy_error(
-					"Error while executing script '%s'. Child exit status: '%d'.\n", script.c_str(), script_err
-				);
+				json j_err_resp {
+					{ "type", "out" },
+					{ "error", "Terminated without exit code. Child exit status reported in 'error_code'" },
+					{ "error_code", std::to_string(script_err) }
+				};
+				str_response_err = j_err_resp.dump();
+				proxy_error("Error while executing script '%s'. Child exit status: '%d'.\n", script.c_str(), script_err);
 			}
 		}
 		// script returned and empty output, invalid output, no need to parse it.
 		else if (script_stdout.empty()) {
-			str_response_err =
-				std::string {
-					"{\"type\":\"out\"," } +
-						"\"error\":\"Script response is empty, only valid JSONs are accepted.\", " +
-						"\"error_code\":\"" + std::to_string(0)
-					+ "\"}";
+			json j_err_resp {
+				{ "type", "out" },
+				{ "error", "Script response is empty, only valid JSONs are accepted" },
+				{ "error_code", std::to_string(0) }
+			};
+			str_response_err = j_err_resp.dump();
 			proxy_error("Invalid empty response from script: '%s'\n", script.c_str());
 		}
 		// execution completed successfully without timing out.
 		else {
 			try {
 				nlohmann::json j { nlohmann::json::parse(script_stdout.c_str()) };
-			}
-			catch(nlohmann::json::exception& e) {
-				std::string escaped_stdout { replace_str(script_stdout, std::string { '"' }, "\\\"") };
-
-				str_response_err =
-					std::string { "{" } +
-						std::string { "\"type\":\"out\", \"error\":\"" } + e.what() +
-						std::string { ", \"error_code\":\"" } + std::to_string(script_err / 256) + "\"" +
-						std::string { ", \"script_stdout\":\"" } + escaped_stdout + "\"" +
-						std::string { ", \"script_stderr\":\"" } + script_stderr + "\"" +
-					"}";
+			} catch(nlohmann::json::exception& e) {
+				json j_err_resp {
+					{ "type", "out" },
+					{ "error", e.what() },
+					{ "error_code", std::to_string(script_err / 256) },
+					{ "script_stdout", script_stdout },
+					{ "script_stderr", script_stderr }
+				};
+				str_response_err = j_err_resp.dump();
 				proxy_error(
 					"Error parsing script output from script: '%s'\n- script_stdout:\n'''\n%s'''\n",
-					script.c_str(),
-					script_stdout.c_str()
+					script.c_str(), script_stdout.c_str()
 				);
 			}
 		}
@@ -240,35 +273,37 @@ class sync_resource : public http_resource {
 public:
 	const std::shared_ptr<http_response> render(const http_request& req) {
 		proxy_info("Render generic request with method %s for uri %s\n", req.get_method().c_str(), req.get_path().c_str());
-		std::stringstream ss;
-		ss << "{\"error\":\"HTTP method " << req.get_method().c_str() << " is not implemented\"}";
-
-        auto response = std::shared_ptr<http_response>(new string_response(ss.str().c_str()));
+		json j_err_resp {{ "error", "HTTP method " + req.get_method() + " is not implemented" }};
+        auto response = std::shared_ptr<http_response>(new string_response(j_err_resp.dump()));
         response->with_header("Content-Type", "application/json");
         response->with_header("Access-Control-Allow-Origin", "*");
+
         return response;
     }
 
 	const std::shared_ptr<http_response> render_GET(const http_request& req) {
-		auto args = req.get_args();
-
-		size_t last = 0;
-		std::stringstream params;
-		params << "{";
-		for (auto arg : args) {
-			params << "\"" << arg.first << "\":\"" << arg.second << "\"";
-			if (last < args.size()-1) {
-				params << ",";
-				last++;
-			}
+		const auto args = req.get_args();
+
+		// Explicit object creation, otherwise 'array' is initialized
+		json input_params = json::object();
+		for (const auto& arg : args) {
+			input_params.push_back({arg.first, arg.second});
 		}
-		params << "}";
 
-		return process_request(req, params.str());
+		const char* req_path { req.get_path().c_str() };
+		const string s_params { input_params.dump() };
+		const char* p_params { s_params.c_str() };
+		proxy_debug(PROXY_DEBUG_RESTAPI, 1, "Processing GET - req: '%s', params: `%s`\n", req_path, p_params);
+
+		return process_request(req, s_params);
 	}
 
 	const std::shared_ptr<http_response> render_POST(const http_request& req) {
 		std::string params=req.get_content();
+		const char* req_path { req.get_path().c_str() };
+		const char* p_params { params.c_str() };
+		proxy_debug(PROXY_DEBUG_RESTAPI, 1, "Processing POST - req: '%s', params: `%s`\n", req_path, p_params);
+
 		return process_request(req, params);
 	}
 

diff --git a/lib/debug.cpp b/lib/debug.cpp
@@ -401,6 +401,7 @@ void init_debug_struct() {
 	GloVars.global.gdbg_lvl[PROXY_DEBUG_IPC].name=(char *)"debug_ipc";
 	GloVars.global.gdbg_lvl[PROXY_DEBUG_QUERY_CACHE].name=(char *)"debug_query_cache";
 	GloVars.global.gdbg_lvl[PROXY_DEBUG_QUERY_STATISTICS].name=(char *)"debug_query_statistics";
+	GloVars.global.gdbg_lvl[PROXY_DEBUG_RESTAPI].name=(char *)"debug_restapi";
 
 	for (i=0;i<PROXY_DEBUG_UNKNOWN;i++) {
 		// if this happen, the above table is not populated correctly