diff options
| -rw-r--r-- | http/http_response.hpp | 5 | ||||
| -rw-r--r-- | redfish-core/include/error_messages.hpp | 5 | ||||
| -rw-r--r-- | redfish-core/include/utils/query_param.hpp | 84 | ||||
| -rw-r--r-- | redfish-core/include/utils/query_param_test.cpp | 140 | ||||
| -rw-r--r-- | redfish-core/src/error_messages.cpp | 35 |
5 files changed, 268 insertions, 1 deletions
diff --git a/http/http_response.hpp b/http/http_response.hpp index dd1f37aea2..ddc808cb29 100644 --- a/http/http_response.hpp +++ b/http/http_response.hpp @@ -92,6 +92,11 @@ struct Response return *this; } + void result(unsigned v) + { + stringResponse->result(v); + } + void result(boost::beast::http::status v) { stringResponse->result(v); diff --git a/redfish-core/include/error_messages.hpp b/redfish-core/include/error_messages.hpp index 50bcfa02bc..d686b5e9e0 100644 --- a/redfish-core/include/error_messages.hpp +++ b/redfish-core/include/error_messages.hpp @@ -38,6 +38,11 @@ constexpr const char* messageVersionPrefix = "Base.1.11.0."; constexpr const char* messageAnnotation = "@Message.ExtendedInfo"; /** + * @brief Moves all error messages from the |source| JSON to |target| + */ +void moveErrorsToErrorJson(nlohmann::json& target, nlohmann::json& source); + +/** * @brief Formats ResourceInUse message into JSON * Message body: "The change to the requested resource failed because the * resource is in use or in transition." diff --git a/redfish-core/include/utils/query_param.hpp b/redfish-core/include/utils/query_param.hpp index 45cb43bb48..fec5384b7e 100644 --- a/redfish-core/include/utils/query_param.hpp +++ b/redfish-core/include/utils/query_param.hpp @@ -659,6 +659,84 @@ inline std::optional<std::string> formatQueryForExpand(const Query& query) return str; } +// Propogates the worst error code to the final response. +// The order of error code is (from high to low) +// 500 Internal Server Error +// 511 Network Authentication Required +// 510 Not Extended +// 508 Loop Detected +// 507 Insufficient Storage +// 506 Variant Also Negotiates +// 505 HTTP Version Not Supported +// 504 Gateway Timeout +// 503 Service Unavailable +// 502 Bad Gateway +// 501 Not Implemented +// 401 Unauthorized +// 451 - 409 Error codes (not listed explictly) +// 408 Request Timeout +// 407 Proxy Authentication Required +// 406 Not Acceptable +// 405 Method Not Allowed +// 404 Not Found +// 403 Forbidden +// 402 Payment Required +// 400 Bad Request +inline unsigned propogateErrorCode(unsigned finalCode, unsigned subResponseCode) +{ + // We keep a explicit list for error codes that this project often uses + // Higer priority codes are in lower indexes + constexpr std::array<unsigned, 13> orderedCodes = { + 500, 507, 503, 502, 501, 401, 412, 409, 406, 405, 404, 403, 400}; + size_t finalCodeIndex = std::numeric_limits<size_t>::max(); + size_t subResponseCodeIndex = std::numeric_limits<size_t>::max(); + for (size_t i = 0; i < orderedCodes.size(); ++i) + { + if (orderedCodes[i] == finalCode) + { + finalCodeIndex = i; + } + if (orderedCodes[i] == subResponseCode) + { + subResponseCodeIndex = i; + } + } + if (finalCodeIndex != std::numeric_limits<size_t>::max() && + subResponseCodeIndex != std::numeric_limits<size_t>::max()) + { + return finalCodeIndex <= subResponseCodeIndex ? finalCode + : subResponseCode; + } + if (subResponseCode == 500 || finalCode == 500) + { + return 500; + } + if (subResponseCode > 500 || finalCode > 500) + { + return std::max(finalCode, subResponseCode); + } + if (subResponseCode == 401) + { + return subResponseCode; + } + return std::max(finalCode, subResponseCode); +} + +// Propogates all error messages into |finalResponse| +inline void propogateError(crow::Response& finalResponse, + crow::Response& subResponse) +{ + // no errors + if (subResponse.resultInt() >= 200 && subResponse.resultInt() < 400) + { + return; + } + messages::moveErrorsToErrorJson(finalResponse.jsonValue, + subResponse.jsonValue); + finalResponse.result( + propogateErrorCode(finalResponse.resultInt(), subResponse.resultInt())); +} + class MultiAsyncResp : public std::enable_shared_from_this<MultiAsyncResp> { public: @@ -683,6 +761,12 @@ class MultiAsyncResp : public std::enable_shared_from_this<MultiAsyncResp> void placeResult(const nlohmann::json::json_pointer& locationToPlace, crow::Response& res) { + BMCWEB_LOG_DEBUG << "placeResult for " << locationToPlace; + propogateError(finalRes->res, res); + if (!res.jsonValue.is_object() || res.jsonValue.empty()) + { + return; + } nlohmann::json& finalObj = finalRes->res.jsonValue[locationToPlace]; finalObj = std::move(res.jsonValue); } diff --git a/redfish-core/include/utils/query_param_test.cpp b/redfish-core/include/utils/query_param_test.cpp index 965b6adf9c..4b89b2e1a1 100644 --- a/redfish-core/include/utils/query_param_test.cpp +++ b/redfish-core/include/utils/query_param_test.cpp @@ -364,6 +364,146 @@ TEST(RecursiveSelect, ReservedPropertiesAreSelected) EXPECT_EQ(root, expected); } +TEST(PropogateErrorCode, 500IsWorst) +{ + constexpr std::array<unsigned, 7> codes = {100, 200, 300, 400, + 401, 500, 501}; + for (auto code : codes) + { + EXPECT_EQ(propogateErrorCode(500, code), 500); + EXPECT_EQ(propogateErrorCode(code, 500), 500); + } +} + +TEST(PropogateErrorCode, 5xxAreWorseThanOthers) +{ + constexpr std::array<unsigned, 7> codes = {100, 200, 300, 400, + 401, 501, 502}; + for (auto code : codes) + { + EXPECT_EQ(propogateErrorCode(code, 505), 505); + EXPECT_EQ(propogateErrorCode(505, code), 505); + } + EXPECT_EQ(propogateErrorCode(502, 501), 502); + EXPECT_EQ(propogateErrorCode(501, 502), 502); + EXPECT_EQ(propogateErrorCode(503, 502), 503); +} + +TEST(PropogateErrorCode, 401IsWorseThanOthers) +{ + constexpr std::array<unsigned, 7> codes = {100, 200, 300, 400, 401}; + for (auto code : codes) + { + EXPECT_EQ(propogateErrorCode(code, 401), 401); + EXPECT_EQ(propogateErrorCode(401, code), 401); + } +} + +TEST(PropogateErrorCode, 4xxIsWorseThanOthers) +{ + constexpr std::array<unsigned, 7> codes = {100, 200, 300, 400, 402}; + for (auto code : codes) + { + EXPECT_EQ(propogateErrorCode(code, 405), 405); + EXPECT_EQ(propogateErrorCode(405, code), 405); + } + EXPECT_EQ(propogateErrorCode(400, 402), 402); + EXPECT_EQ(propogateErrorCode(402, 403), 403); + EXPECT_EQ(propogateErrorCode(403, 402), 403); +} + +TEST(PropogateError, IntermediateNoErrorMessageMakesNoChange) +{ + crow::Response intermediate; + intermediate.result(boost::beast::http::status::ok); + + crow::Response finalRes; + finalRes.result(boost::beast::http::status::ok); + propogateError(finalRes, intermediate); + EXPECT_EQ(finalRes.result(), boost::beast::http::status::ok); + EXPECT_EQ(finalRes.jsonValue.find("error"), finalRes.jsonValue.end()); +} + +TEST(PropogateError, ErrorsArePropergatedWithErrorInRoot) +{ + nlohmann::json root = R"( +{ + "@odata.type": "#Message.v1_1_1.Message", + "Message": "The request failed due to an internal service error. The service is still operational.", + "MessageArgs": [], + "MessageId": "Base.1.13.0.InternalError", + "MessageSeverity": "Critical", + "Resolution": "Resubmit the request. If the problem persists, consider resetting the service." +} +)"_json; + crow::Response intermediate; + intermediate.result(boost::beast::http::status::internal_server_error); + intermediate.jsonValue = root; + + crow::Response final; + final.result(boost::beast::http::status::ok); + + propogateError(final, intermediate); + + EXPECT_EQ(final.jsonValue["error"]["code"].get<std::string>(), + "Base.1.13.0.InternalError"); + EXPECT_EQ( + final.jsonValue["error"]["message"].get<std::string>(), + "The request failed due to an internal service error. The service is still operational."); + EXPECT_EQ(intermediate.jsonValue, R"({})"_json); + EXPECT_EQ(final.result(), + boost::beast::http::status::internal_server_error); +} + +TEST(PropogateError, ErrorsArePropergatedWithErrorCode) +{ + crow::Response intermediate; + intermediate.result(boost::beast::http::status::internal_server_error); + + nlohmann::json error = R"( +{ + "error": { + "@Message.ExtendedInfo": [], + "code": "Base.1.13.0.InternalError", + "message": "The request failed due to an internal service error. The service is still operational." + } +} +)"_json; + nlohmann::json extendedInfo = R"( +{ + "@odata.type": "#Message.v1_1_1.Message", + "Message": "The request failed due to an internal service error. The service is still operational.", + "MessageArgs": [], + "MessageId": "Base.1.13.0.InternalError", + "MessageSeverity": "Critical", + "Resolution": "Resubmit the request. If the problem persists, consider resetting the service." +} +)"_json; + + for (int i = 0; i < 10; ++i) + { + error["error"][messages::messageAnnotation].push_back(extendedInfo); + } + intermediate.jsonValue = error; + crow::Response final; + final.result(boost::beast::http::status::ok); + + propogateError(final, intermediate); + EXPECT_EQ(final.jsonValue["error"][messages::messageAnnotation], + error["error"][messages::messageAnnotation]); + std::string errorCode = messages::messageVersionPrefix; + errorCode += "GeneralError"; + std::string errorMessage = + "A general error has occurred. See Resolution for " + "information on how to resolve the error."; + EXPECT_EQ(final.jsonValue["error"]["code"].get<std::string>(), errorCode); + EXPECT_EQ(final.jsonValue["error"]["message"].get<std::string>(), + errorMessage); + EXPECT_EQ(intermediate.jsonValue, R"({})"_json); + EXPECT_EQ(final.result(), + boost::beast::http::status::internal_server_error); +} + TEST(QueryParams, ParseParametersOnly) { auto ret = boost::urls::parse_relative_ref("/redfish/v1?only"); diff --git a/redfish-core/src/error_messages.cpp b/redfish-core/src/error_messages.cpp index 198e54425d..1bfce24504 100644 --- a/redfish-core/src/error_messages.cpp +++ b/redfish-core/src/error_messages.cpp @@ -74,7 +74,7 @@ static void addMessageToErrorJson(nlohmann::json& target, "information on how to resolve the error."; } - // This check could technically be done in in the default construction + // This check could technically be done in the default construction // branch above, but because we need the pointer to the extended info field // anyway, it's more efficient to do it here. auto& extendedInfo = error[messages::messageAnnotation]; @@ -86,6 +86,39 @@ static void addMessageToErrorJson(nlohmann::json& target, extendedInfo.push_back(message); } +void moveErrorsToErrorJson(nlohmann::json& target, nlohmann::json& source) +{ + if (!source.is_object()) + { + return; + } + auto errorIt = source.find("error"); + if (errorIt == source.end()) + { + // caller puts error message in root + messages::addMessageToErrorJson(target, source); + source.clear(); + return; + } + auto extendedInfoIt = errorIt->find(messages::messageAnnotation); + if (extendedInfoIt == errorIt->end()) + { + return; + } + const nlohmann::json::array_t* extendedInfo = + (*extendedInfoIt).get_ptr<const nlohmann::json::array_t*>(); + if (extendedInfo == nullptr) + { + source.erase(errorIt); + return; + } + for (const nlohmann::json& message : *extendedInfo) + { + addMessageToErrorJson(target, message); + } + source.erase(errorIt); +} + static void addMessageToJsonRoot(nlohmann::json& target, const nlohmann::json& message) { |