Improve api error handling

refs #10194

lib/remote/actionshandler.cpp

@@ -33,22 +33,18 @@ REGISTER_URLHANDLER("/v1/actions", ActionsHandler);
 
 bool ActionsHandler::HandleRequest(const ApiUser::Ptr& user, HttpRequest& request, HttpResponse& response)
 {
-	if (request.RequestMethod != "POST") {
+	if (request.RequestUrl->GetPath().size() != 3)
-		HttpUtility::SendJsonError(response, 400, "Invalid request type. Must be POST.");
+		return false;
-		return true;
-	}
 
-	if (request.RequestUrl->GetPath().size() < 3) {
+	if (request.RequestMethod != "POST")
-		HttpUtility::SendJsonError(response, 400, "Action is missing.");
+		return false;
-		return true;
-	}
 
 	String actionName = request.RequestUrl->GetPath()[2];
 
 	ApiAction::Ptr action = ApiAction::GetByName(actionName);
 
 	if (!action) {
-		HttpUtility::SendJsonError(response, 404, "Action '" + actionName + "' could not be found.");
+		HttpUtility::SendJsonError(response, 404, "Action '" + actionName + "' does not exist.");
 		return true;
 	}
 

lib/remote/apilistener-configsync.cpp

@@ -44,10 +44,8 @@ void ApiListener::ConfigUpdateObjectHandler(const ConfigObject::Ptr& object, con
 {
 	ApiListener::Ptr listener = ApiListener::GetInstance();
 
-	if (!listener) {
+	if (!listener)
-		Log(LogCritical, "ApiListener", "No instance available.");
 		return;
-	}
 
 	if (object->IsActive()) {
 		/* Sync object config */
@@ -66,10 +64,8 @@ Value ApiListener::ConfigUpdateObjectAPIHandler(const MessageOrigin::Ptr& origin
 	/* check permissions */
 	ApiListener::Ptr listener = ApiListener::GetInstance();
 
-	if (!listener) {
+	if (!listener)
-		Log(LogCritical, "ApiListener", "No instance available.");
 		return Empty;
-	}
 
 	if (!listener->GetAcceptConfig()) {
 		Log(LogWarning, "ApiListener")
@@ -174,10 +170,8 @@ Value ApiListener::ConfigDeleteObjectAPIHandler(const MessageOrigin::Ptr& origin
 	/* check permissions */
 	ApiListener::Ptr listener = ApiListener::GetInstance();
 
-	if (!listener) {
+	if (!listener)
-		Log(LogCritical, "ApiListener", "No instance available.");
 		return Empty;
-	}
 
 	if (!listener->GetAcceptConfig()) {
 		Log(LogWarning, "ApiListener")

lib/remote/configfileshandler.cpp

@@ -31,16 +31,9 @@ REGISTER_URLHANDLER("/v1/config/files", ConfigFilesHandler);
 
 bool ConfigFilesHandler::HandleRequest(const ApiUser::Ptr& user, HttpRequest& request, HttpResponse& response)
 {
-	if (request.RequestMethod == "GET")
+	if (request.RequestMethod != "GET")
-		HandleGet(user, request, response);
+		return false;
-	else
-		HttpUtility::SendJsonError(response, 400, "Invalid request type. Must be GET.");
 
-	return true;
-}
-
-void ConfigFilesHandler::HandleGet(const ApiUser::Ptr& user, HttpRequest& request, HttpResponse& response)
-{
 	Dictionary::Ptr params = HttpUtility::FetchRequestParameters(request);
 
 	const std::vector<String>& urlPath = request.RequestUrl->GetPath();
@@ -61,21 +54,29 @@ void ConfigFilesHandler::HandleGet(const ApiUser::Ptr& user, HttpRequest& reques
 	String packageName = HttpUtility::GetLastParameter(params, "package");
 	String stageName = HttpUtility::GetLastParameter(params, "stage");
 
-	if (!ConfigPackageUtility::ValidateName(packageName))
+	if (!ConfigPackageUtility::ValidateName(packageName)) {
-		return HttpUtility::SendJsonError(response, 404, "Package is not valid or does not exist.");
+		HttpUtility::SendJsonError(response, 400, "Invalid package name.");
+		return true;
+	}
 
-	if (!ConfigPackageUtility::ValidateName(stageName))
+	if (!ConfigPackageUtility::ValidateName(stageName)) {
-		return HttpUtility::SendJsonError(response, 404, "Stage is not valid or does not exist.");
+		HttpUtility::SendJsonError(response, 400, "Invalid stage name.");
+		return true;
+	}
 
 	String relativePath = HttpUtility::GetLastParameter(params, "path");
 
-	if (ConfigPackageUtility::ContainsDotDot(relativePath))
+	if (ConfigPackageUtility::ContainsDotDot(relativePath)) {
-		return HttpUtility::SendJsonError(response, 403, "Path contains '..' (not allowed).");
+		HttpUtility::SendJsonError(response, 400, "Path contains '..' (not allowed).");
+		return true;
+	}
 
 	String path = ConfigPackageUtility::GetPackageDir() + "/" + packageName + "/" + stageName + "/" + relativePath;
 
-	if (!Utility::PathExists(path))
+	if (!Utility::PathExists(path)) {
-		return HttpUtility::SendJsonError(response, 404, "Path not found.");
+		HttpUtility::SendJsonError(response, 404, "Path not found.");
+		return true;
+	}
 
 	try {
 		std::ifstream fp(path.CStr(), std::ifstream::in | std::ifstream::binary);
@@ -86,9 +87,9 @@ void ConfigFilesHandler::HandleGet(const ApiUser::Ptr& user, HttpRequest& reques
 		response.AddHeader("Content-Type", "application/octet-stream");
 		response.WriteBody(content.CStr(), content.GetLength());
 	} catch (const std::exception& ex) {
-		return HttpUtility::SendJsonError(response, 500, "Could not read file.",
+		HttpUtility::SendJsonError(response, 500, "Could not read file.",
 		    request.GetVerboseErrors() ? DiagnosticInformation(ex) : "");
 	}
+
+	return true;
 }
-
-

lib/remote/configfileshandler.hpp

@@ -31,9 +31,6 @@ public:
 	DECLARE_PTR_TYPEDEFS(ConfigFilesHandler);
 
 	virtual bool HandleRequest(const ApiUser::Ptr& user, HttpRequest& request, HttpResponse& response) override;
-
-private:
-	void HandleGet(const ApiUser::Ptr& user, HttpRequest& request, HttpResponse& response);
 };
 
 }

lib/remote/configpackageshandler.cpp

@@ -30,11 +30,8 @@ REGISTER_URLHANDLER("/v1/config/packages", ConfigPackagesHandler);
 
 bool ConfigPackagesHandler::HandleRequest(const ApiUser::Ptr& user, HttpRequest& request, HttpResponse& response)
 {
-	if (request.RequestUrl->GetPath().size() > 4) {
+	if (request.RequestUrl->GetPath().size() > 4)
-		String path = boost::algorithm::join(request.RequestUrl->GetPath(), "/");
+		return false;
-		HttpUtility::SendJsonError(response, 404, "The requested path is too long to match any config package requests");
-		return true;
-	}
 
 	if (request.RequestMethod == "GET")
 		HandleGet(user, request, response);
@@ -43,7 +40,7 @@ bool ConfigPackagesHandler::HandleRequest(const ApiUser::Ptr& user, HttpRequest&
 	else if (request.RequestMethod == "DELETE")
 		HandleDelete(user, request, response);
 	else
-		HttpUtility::SendJsonError(response, 400, "Invalid request type. Must be GET, POST or DELETE.");
+		return false;
 
 	return true;
 }
@@ -83,7 +80,7 @@ void ConfigPackagesHandler::HandlePost(const ApiUser::Ptr& user, HttpRequest& re
 	String packageName = HttpUtility::GetLastParameter(params, "package");
 
 	if (!ConfigPackageUtility::ValidateName(packageName)) {
-		HttpUtility::SendJsonError(response, 404, "Package is not valid or does not exist.");
+		HttpUtility::SendJsonError(response, 400, "Invalid package name.");
 		return;
 	}
 
@@ -121,7 +118,7 @@ void ConfigPackagesHandler::HandleDelete(const ApiUser::Ptr& user, HttpRequest&
 	String packageName = HttpUtility::GetLastParameter(params, "package");
 
 	if (!ConfigPackageUtility::ValidateName(packageName)) {
-		HttpUtility::SendJsonError(response, 404, "Package is not valid or does not exist.");
+		HttpUtility::SendJsonError(response, 400, "Invalid package name.");
 		return;
 	}
 

lib/remote/configstageshandler.cpp

@@ -32,11 +32,8 @@ REGISTER_URLHANDLER("/v1/config/stages", ConfigStagesHandler);
 
 bool ConfigStagesHandler::HandleRequest(const ApiUser::Ptr& user, HttpRequest& request, HttpResponse& response)
 {
-	if (request.RequestUrl->GetPath().size() > 5) {
+	if (request.RequestUrl->GetPath().size() > 5)
-		String path = boost::algorithm::join(request.RequestUrl->GetPath(), "/");
+		return false;
-		HttpUtility::SendJsonError(response, 404, "The requested path is too long to match any config tag requests.");
-		return true;
-	}
 
 	if (request.RequestMethod == "GET")
 		HandleGet(user, request, response);
@@ -45,7 +42,7 @@ bool ConfigStagesHandler::HandleRequest(const ApiUser::Ptr& user, HttpRequest& r
 	else if (request.RequestMethod == "DELETE")
 		HandleDelete(user, request, response);
 	else
-		HttpUtility::SendJsonError(response, 400, "Invalid request type. Must be GET, POST or DELETE.");
+		return false;
 
 	return true;
 }
@@ -66,10 +63,10 @@ void ConfigStagesHandler::HandleGet(const ApiUser::Ptr& user, HttpRequest& reque
 	String stageName = HttpUtility::GetLastParameter(params, "stage");
 
 	if (!ConfigPackageUtility::ValidateName(packageName))
-		return HttpUtility::SendJsonError(response, 404, "Package is not valid or does not exist.");
+		return HttpUtility::SendJsonError(response, 400, "Invalid package name.");
 
 	if (!ConfigPackageUtility::ValidateName(stageName))
-		return HttpUtility::SendJsonError(response, 404, "Stage is not valid or does not exist.");
+		return HttpUtility::SendJsonError(response, 400, "Invalid stage name.");
 
 	Array::Ptr results = new Array();
 
@@ -104,7 +101,7 @@ void ConfigStagesHandler::HandlePost(const ApiUser::Ptr& user, HttpRequest& requ
 	String packageName = HttpUtility::GetLastParameter(params, "package");
 
 	if (!ConfigPackageUtility::ValidateName(packageName))
-		return HttpUtility::SendJsonError(response, 404, "Package is not valid or does not exist.");
+		return HttpUtility::SendJsonError(response, 400, "Invalid package name.");
 
 	Dictionary::Ptr files = params->Get("files");
 
@@ -155,10 +152,10 @@ void ConfigStagesHandler::HandleDelete(const ApiUser::Ptr& user, HttpRequest& re
 	String stageName = HttpUtility::GetLastParameter(params, "stage");
 
 	if (!ConfigPackageUtility::ValidateName(packageName))
-		return HttpUtility::SendJsonError(response, 404, "Package is not valid or does not exist.");
+		return HttpUtility::SendJsonError(response, 400, "Invalid package name.");
 
 	if (!ConfigPackageUtility::ValidateName(stageName))
-		return HttpUtility::SendJsonError(response, 404, "Stage is not valid or does not exist.");
+		return HttpUtility::SendJsonError(response, 400, "Invalid stage name.");
 
 	try {
 		ConfigPackageUtility::DeleteStage(packageName, stageName);
@@ -171,7 +168,7 @@ void ConfigStagesHandler::HandleDelete(const ApiUser::Ptr& user, HttpRequest& re
 	Dictionary::Ptr result1 = new Dictionary();
 
 	result1->Set("code", 200);
-	result1->Set("status", "Stage deleted");
+	result1->Set("status", "Stage deleted.");
 
 	Array::Ptr results = new Array();
 	results->Add(result1);

lib/remote/createobjecthandler.cpp

@@ -31,20 +31,16 @@ REGISTER_URLHANDLER("/v1/objects", CreateObjectHandler);
 
 bool CreateObjectHandler::HandleRequest(const ApiUser::Ptr& user, HttpRequest& request, HttpResponse& response)
 {
-	if (request.RequestMethod != "PUT") {
+	if (request.RequestUrl->GetPath().size() != 4)
-		/* there might be other request methods pending */
 		return false;
-	}
 
-	if (request.RequestUrl->GetPath().size() < 4) {
+	if (request.RequestMethod != "PUT")
-		HttpUtility::SendJsonError(response, 400, "Object name is missing.");
+		return false;
-		return true;
-	}
 
 	Type::Ptr type = FilterUtility::TypeFromPluralName(request.RequestUrl->GetPath()[2]);
 
 	if (!type) {
-		HttpUtility::SendJsonError(response, 403, "Erroneous type was supplied.");
+		HttpUtility::SendJsonError(response, 400, "Invalid type specified.");
 		return true;
 	}
 

lib/remote/deleteobjecthandler.cpp

@@ -34,21 +34,16 @@ REGISTER_URLHANDLER("/v1/objects", DeleteObjectHandler);
 
 bool DeleteObjectHandler::HandleRequest(const ApiUser::Ptr& user, HttpRequest& request, HttpResponse& response)
 {
-	if (request.RequestMethod != "DELETE") {
+	if (request.RequestUrl->GetPath().size() < 3 || request.RequestUrl->GetPath().size() > 4)
-		/* there might be other request methods pending */
 		return false;
-	}
 
-	if (request.RequestUrl->GetPath().size() < 3) {
+	if (request.RequestMethod != "DELETE")
-		String path = boost::algorithm::join(request.RequestUrl->GetPath(), "/");
+		return false;
-		HttpUtility::SendJsonError(response, 404, "The requested path is too long to match any config tag requests.");
-		return true;
-	}
 
 	Type::Ptr type = FilterUtility::TypeFromPluralName(request.RequestUrl->GetPath()[2]);
 
 	if (!type) {
-		HttpUtility::SendJsonError(response, 400, "Erroneous type was supplied.");
+		HttpUtility::SendJsonError(response, 400, "Invalid type specified.");
 		return true;
 	}
 

lib/remote/httphandler.cpp

@@ -103,8 +103,8 @@ void HttpHandler::ProcessRequest(const ApiUser::Ptr& user, HttpRequest& request,
 	}
 	if (!processed) {
 		String path = boost::algorithm::join(request.RequestUrl->GetPath(), "/");
-		HttpUtility::SendJsonError(response, 404, "The requested API '" +  path +
+		HttpUtility::SendJsonError(response, 404, "The requested path '" + path +
-				"' could not be found. Please check it for common errors like spelling and consult the docs.");
+				"' could not be found.");
 		return;
 	}
 }

lib/remote/modifyobjecthandler.cpp

@@ -32,18 +32,18 @@ REGISTER_URLHANDLER("/v1/objects", ModifyObjectHandler);
 
 bool ModifyObjectHandler::HandleRequest(const ApiUser::Ptr& user, HttpRequest& request, HttpResponse& response)
 {
-	if (request.RequestMethod != "POST") {
+	if (request.RequestUrl->GetPath().size() < 3 || request.RequestUrl->GetPath().size() > 4)
-		/* there might be other request methods pending */
 		return false;
-	}
 
-	if (request.RequestUrl->GetPath().size() < 3)
+	if (request.RequestMethod != "POST")
 		return false;
 
 	Type::Ptr type = FilterUtility::TypeFromPluralName(request.RequestUrl->GetPath()[1]);
 
-	if (!type)
+	if (!type) {
-		return false;
+		HttpUtility::SendJsonError(response, 400, "Invalid type specified.");
+		return true;
+	}
 
 	QueryDescription qd;
 	qd.Types.insert(type->GetName());
@@ -100,4 +100,3 @@ bool ModifyObjectHandler::HandleRequest(const ApiUser::Ptr& user, HttpRequest& r
 
 	return true;
 }
-

lib/remote/objectqueryhandler.cpp

@@ -32,16 +32,18 @@ REGISTER_URLHANDLER("/v1/objects", ObjectQueryHandler);
 
 bool ObjectQueryHandler::HandleRequest(const ApiUser::Ptr& user, HttpRequest& request, HttpResponse& response)
 {
-	if (request.RequestMethod != "GET")
+	if (request.RequestUrl->GetPath().size() < 3 || request.RequestUrl->GetPath().size() > 4)
 		return false;
 
-	if (request.RequestUrl->GetPath().size() < 3)
+	if (request.RequestMethod != "GET")
 		return false;
 
 	Type::Ptr type = FilterUtility::TypeFromPluralName(request.RequestUrl->GetPath()[2]);
 
-	if (!type)
+	if (!type) {
-		return false;
+		HttpUtility::SendJsonError(response, 400, "Invalid type specified.");
+		return true;
+	}
 
 	QueryDescription qd;
 	qd.Types.insert(type->GetName());

lib/remote/statushandler.cpp

@@ -71,15 +71,11 @@ public:
 
 bool StatusHandler::HandleRequest(const ApiUser::Ptr& user, HttpRequest& request, HttpResponse& response)
 {
-	Dictionary::Ptr result = new Dictionary();
+	if (request.RequestUrl->GetPath().size() > 3)
-
+		return false;
-	if (request.RequestMethod != "GET") {
-		response.SetStatus(400, "Bad request");
-		result->Set("info", "Request must be type GET");
-		HttpUtility::SendJsonBody(response, result);
-		return true;
-	}
 
+	if (request.RequestMethod != "GET")
+		return false;
 
 	QueryDescription qd;
 	qd.Types.insert("Status");
@@ -97,6 +93,7 @@ bool StatusHandler::HandleRequest(const ApiUser::Ptr& user, HttpRequest& request
 
 	Array::Ptr results = Array::FromVector(objs);
 
+	Dictionary::Ptr result = new Dictionary();
 	result->Set("results", results);
 
 	response.SetStatus(200, "OK");

lib/remote/typequeryhandler.cpp

@@ -77,14 +77,11 @@ public:
 
 bool TypeQueryHandler::HandleRequest(const ApiUser::Ptr& user, HttpRequest& request, HttpResponse& response)
 {
-	Dictionary::Ptr result = new Dictionary();
+	if (request.RequestUrl->GetPath().size() > 3)
+		return false;
 
-	if (request.RequestMethod != "GET") {
+	if (request.RequestMethod != "GET")
-		response.SetStatus(400, "Bad request");
+		return false;
-		result->Set("info", "Request must be type GET");
-		HttpUtility::SendJsonBody(response, result);
-		return true;
-	}
 
 	QueryDescription qd;
 	qd.Types.insert("Type");
@@ -157,6 +154,7 @@ bool TypeQueryHandler::HandleRequest(const ApiUser::Ptr& user, HttpRequest& requ
 		}
 	}
 
+	Dictionary::Ptr result = new Dictionary();
 	result->Set("results", results);
 
 	response.SetStatus(200, "OK");