query: propogate errors for expand
The existing code doesn't propogate errors of subqueries correctly.
This commit corrects the behavior, so that the final response gets all
error message of subqueries and the "highest priority" HTTP code.
DMTF doesn't specify how expand queries handle error codes, since using
subqueries is an implementation choice that we made in this project.
What we did here follows existing behavior of this project, and follows
the error message section of the Redfish spec;
[1] https://redfish.dmtf.org/schemas/DSP0266_1.15.1.html#error-responses
As for now, this commit uses the worst HTTP code among all the error
code. See query_param.hpp, function |propogateErrorCode| for detailed
order of the errror codes.
Tested:
1. this is difficult to test, but I hijacked the code so it returns
errors in TaskServices, then I verified that "/redfish/v1?$expand=."
correctly returns 500 and the gets the error message set.
2. unit test so that when there are multiple errors, the final response
gets a generate error message.
Signed-off-by: Nan Zhou <nanzhoumails@gmail.com>
Change-Id: I0c1ebdd9015f389801db9150d687027485f1203c
diff --git a/http/http_response.hpp b/http/http_response.hpp
index dd1f37a..ddc808c 100644
--- a/http/http_response.hpp
+++ b/http/http_response.hpp
@@ -92,6 +92,11 @@
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 50bcfa0..d686b5e 100644
--- a/redfish-core/include/error_messages.hpp
+++ b/redfish-core/include/error_messages.hpp
@@ -38,6 +38,11 @@
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 45cb43b..fec5384 100644
--- a/redfish-core/include/utils/query_param.hpp
+++ b/redfish-core/include/utils/query_param.hpp
@@ -659,6 +659,84 @@
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 @@
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 965b6ad..4b89b2e 100644
--- a/redfish-core/include/utils/query_param_test.cpp
+++ b/redfish-core/include/utils/query_param_test.cpp
@@ -364,6 +364,146 @@
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 198e544..1bfce24 100644
--- a/redfish-core/src/error_messages.cpp
+++ b/redfish-core/src/error_messages.cpp
@@ -74,7 +74,7 @@
"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 @@
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)
{