From 90623594499337d359b1c35154d07f02a0e7e0af Mon Sep 17 00:00:00 2001 From: Craig Edwards Date: Wed, 4 Oct 2023 14:59:01 +0000 Subject: [PATCH 1/3] feat: improved human readable error messages to default logger, with unit test --- include/dpp/restresults.h | 8 ++++++++ src/dpp/cluster/confirmation.cpp | 17 +++++++++++++++++ src/dpp/utility.cpp | 4 ++-- src/unittest/test.cpp | 19 +++++++++++++++++++ src/unittest/test.h | 1 + 5 files changed, 47 insertions(+), 2 deletions(-) diff --git a/include/dpp/restresults.h b/include/dpp/restresults.h index 0691b41869..d6ad8bfa57 100644 --- a/include/dpp/restresults.h +++ b/include/dpp/restresults.h @@ -216,6 +216,10 @@ struct DPP_EXPORT error_detail { * @brief Error reason (full message) */ std::string reason; + /** + * @brief Object field index + */ + int index = 0; }; /** @@ -235,6 +239,10 @@ struct DPP_EXPORT error_info { * @brief Field specific error descriptions */ std::vector errors; + /** + * @brief Human readable error message constructed from the above + */ + std::string human_readable; }; /** diff --git a/src/dpp/cluster/confirmation.cpp b/src/dpp/cluster/confirmation.cpp index b2744f8d8f..a02a886443 100644 --- a/src/dpp/cluster/confirmation.cpp +++ b/src/dpp/cluster/confirmation.cpp @@ -82,7 +82,9 @@ error_info confirmation_callback_t::get_error() const { json& errors = j["errors"]; for (auto obj = errors.begin(); obj != errors.end(); ++obj) { + int array_index = 0; if (obj->find("0") != obj->end()) { + array_index = 0; /* An array of error messages */ for (auto index = obj->begin(); index != obj->end(); ++index) { if (index->find("_errors") != index->end()) { @@ -92,6 +94,7 @@ error_info confirmation_callback_t::get_error() const { detail.reason = (*errordetails)["message"].get(); detail.object.clear(); detail.field = obj.key(); + detail.index = array_index; e.errors.emplace_back(detail); } } else { @@ -102,10 +105,13 @@ error_info confirmation_callback_t::get_error() const { detail.reason = (*errordetails)["message"].get(); detail.field = fields.key(); detail.object = obj.key(); + detail.index = array_index; e.errors.emplace_back(detail); } } } + /* Index only increments per field, not per error*/ + array_index++; } } else if (obj->find("_errors") != obj->end()) { @@ -117,11 +123,22 @@ error_info confirmation_callback_t::get_error() const { detail.reason = (*errordetails)["message"].get(); detail.object.clear(); detail.field = obj.key(); + detail.index = 0; e.errors.emplace_back(detail); } } } + e.human_readable = std::to_string(e.code) + ": " + e.message; + std::string prefix = e.errors.size() == 1 ? " " : "\n\t"; + for (const auto& error : e.errors) { + if (error.object.empty()) { + e.human_readable += prefix + "- " + error.field + ": " + error.reason + " (" + error.code + ")"; + } else { + e.human_readable += prefix + "- " + error.object + "[" + std::to_string(error.index) + "]." + error.field + ": " + error.reason + " (" + error.code + ")"; + } + } + return e; } return error_info(); diff --git a/src/dpp/utility.cpp b/src/dpp/utility.cpp index 3c526df04c..1ca4751ca3 100644 --- a/src/dpp/utility.cpp +++ b/src/dpp/utility.cpp @@ -499,10 +499,10 @@ namespace dpp { return [](const dpp::confirmation_callback_t& detail) { if (detail.is_error()) { if (detail.bot) { + error_info e = detail.get_error(); detail.bot->log( dpp::ll_error, - "Error " + std::to_string(detail.get_error().code) + " [" + - detail.get_error().message + "] on API request, returned content was: " + detail.http_info.body + "Error: " + e.human_readable ); } } diff --git a/src/unittest/test.cpp b/src/unittest/test.cpp index 8173c6842c..488367d4bd 100644 --- a/src/unittest/test.cpp +++ b/src/unittest/test.cpp @@ -58,6 +58,25 @@ Markdown lol ||spoiler|| ~~strikethrough~~ `small *code* block`\n"; set_test(COMPARISON, u1 == u2 && u1 != u3); + dpp::confirmation_callback_t error_test; + set_test(ERRORS, false); + bool error_message_success = false; + error_test.http_info.status = 400; + + error_test.http_info.body = "{\"message\": \"Invalid Form Body\", \"code\": 50035, \"errors\": {\"options\": {\"0\": {\"name\": {\"_errors\": [{\"code\": \"STRING_TYPE_REGEX\", \"message\": \"String value did not match validation regex.\"}, {\"code\": \"APPLICATION_COMMAND_INVALID_NAME\", \"message\": \"Command name is invalid\"}]}}}}}"; + error_message_success = (error_test.get_error().human_readable == "50035: Invalid Form Body\n\t- options[0].name: String value did not match validation regex. (STRING_TYPE_REGEX)\n\t- options[0].name: Command name is invalid (APPLICATION_COMMAND_INVALID_NAME)"); + + error_test.http_info.body = "{\"message\": \"Invalid Form Body\", \"code\": 50035, \"errors\": {\"type\": {\"_errors\": [{\"code\": \"BASE_TYPE_CHOICES\", \"message\": \"Value must be one of {4, 5, 9, 10, 11}.\"}]}}}"; + error_message_success = (error_message_success && error_test.get_error().human_readable == "50035: Invalid Form Body - type: Value must be one of {4, 5, 9, 10, 11}. (BASE_TYPE_CHOICES)"); + + error_test.http_info.body = "{\"message\": \"Unknown Guild\", \"code\": 10004}"; + error_message_success = (error_message_success && error_test.get_error().human_readable == "10004: Unknown Guild"); + + error_test.http_info.body = "{\"message\": \"Invalid Form Body\", \"code\": 50035, \"errors\": {\"allowed_mentions\": {\"_errors\": [{\"code\": \"MESSAGE_ALLOWED_MENTIONS_PARSE_EXCLUSIVE\", \"message\": \"parse:[\\\"users\\\"] and users: [ids...] are mutually exclusive.\"}]}}}"; + error_message_success = (error_message_success && error_test.get_error().human_readable == "50035: Invalid Form Body - allowed_mentions: parse:[\"users\"] and users: [ids...] are mutually exclusive. (MESSAGE_ALLOWED_MENTIONS_PARSE_EXCLUSIVE)"); + + set_test(ERRORS, error_message_success); + set_test(MD_ESC_1, false); set_test(MD_ESC_2, false); std::string escaped1 = dpp::utility::markdown_escape(test_to_escape); diff --git a/src/unittest/test.h b/src/unittest/test.h index c3b295ae45..df6a98d34a 100644 --- a/src/unittest/test.h +++ b/src/unittest/test.h @@ -144,6 +144,7 @@ DPP_TEST(CHANNELTYPES, "channel type flags", tf_online); DPP_TEST(FORUM_CREATION, "create a forum channel", tf_online); DPP_TEST(FORUM_CHANNEL_GET, "retrieve the created forum channel", tf_online); DPP_TEST(FORUM_CHANNEL_DELETE, "delete the created forum channel", tf_online); +DPP_TEST(ERRORS, "Human readable error translation", tf_offline); DPP_TEST(GUILD_BAN_CREATE, "cluster::guild_ban_add ban three deleted discord accounts", tf_online); DPP_TEST(GUILD_BAN_GET, "cluster::guild_get_ban getting one of the banned accounts", tf_online); From d0a5748daa7f805fc2ccb9ec8465fb4124434ff1 Mon Sep 17 00:00:00 2001 From: Craig Edwards Date: Wed, 4 Oct 2023 16:36:46 +0000 Subject: [PATCH 2/3] improvements to pick up the index properly and subobjects of subobjects --- .vscode/settings.json | 3 +- src/dpp/cluster/confirmation.cpp | 67 ++++++++++++++++++++----- src/unittest/test.cpp | 84 ++++++++++++++++++++++++++++++-- 3 files changed, 135 insertions(+), 19 deletions(-) diff --git a/.vscode/settings.json b/.vscode/settings.json index f00d7bf8ee..21ab674e7d 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -79,6 +79,7 @@ "coroutine": "cpp", "numbers": "cpp", "semaphore": "cpp", - "stop_token": "cpp" + "stop_token": "cpp", + "charconv": "cpp" } } \ No newline at end of file diff --git a/src/dpp/cluster/confirmation.cpp b/src/dpp/cluster/confirmation.cpp index a02a886443..009d6c71b2 100644 --- a/src/dpp/cluster/confirmation.cpp +++ b/src/dpp/cluster/confirmation.cpp @@ -83,11 +83,14 @@ error_info confirmation_callback_t::get_error() const { for (auto obj = errors.begin(); obj != errors.end(); ++obj) { int array_index = 0; - if (obj->find("0") != obj->end()) { - array_index = 0; + + /* Arrays in the error report are numerically indexed with a number in a string. Ugh. */ + if (isdigit(*(obj.key().c_str()))) { /* An array of error messages */ + array_index = std::atoll(obj.key().c_str()); for (auto index = obj->begin(); index != obj->end(); ++index) { if (index->find("_errors") != index->end()) { + /* A single object where one or more fields generated an error */ for (auto errordetails = (*index)["_errors"].begin(); errordetails != (*index)["_errors"].end(); ++errordetails) { error_detail detail; detail.code = (*errordetails)["code"].get(); @@ -98,24 +101,38 @@ error_info confirmation_callback_t::get_error() const { e.errors.emplace_back(detail); } } else { + /* An object where one or more fields within it generated an error, e.g. slash command */ for (auto fields = index->begin(); fields != index->end(); ++fields) { - for (auto errordetails = (*fields)["_errors"].begin(); errordetails != (*fields)["_errors"].end(); ++errordetails) { - error_detail detail; - detail.code = (*errordetails)["code"].get(); - detail.reason = (*errordetails)["message"].get(); - detail.field = fields.key(); - detail.object = obj.key(); - detail.index = array_index; - e.errors.emplace_back(detail); + if (fields->find("_errors") != fields->end()) { + for (auto errordetails = (*fields)["_errors"].begin(); errordetails != (*fields)["_errors"].end(); ++errordetails) { + error_detail detail; + detail.code = (*errordetails)["code"].get(); + detail.reason = (*errordetails)["message"].get(); + detail.field = fields.key(); + detail.object = obj.key(); + detail.index = array_index; + e.errors.emplace_back(detail); + } + } else { + /* An array of objects where one or more generated an error, e.g. slash command bulk registration */ + for (auto fields2 = fields->begin(); fields2 != fields->end(); ++fields2) { + for (auto errordetails = (*fields2)["_errors"].begin(); errordetails != (*fields2)["_errors"].end(); ++errordetails) { + error_detail detail; + detail.code = (*errordetails)["code"].get(); + detail.reason = (*errordetails)["message"].get(); + detail.field = index.key() + "[" + fields.key() + "]." + fields2.key(); + detail.object = obj.key(); + detail.index = array_index; + e.errors.emplace_back(detail); + } + } } } } - /* Index only increments per field, not per error*/ - array_index++; } } else if (obj->find("_errors") != obj->end()) { - /* An object of error messages */ + /* An object of error messages (rare) */ e.errors.reserve((*obj)["_errors"].size()); for (auto errordetails = (*obj)["_errors"].begin(); errordetails != (*obj)["_errors"].end(); ++errordetails) { error_detail detail; @@ -126,6 +143,25 @@ error_info confirmation_callback_t::get_error() const { detail.index = 0; e.errors.emplace_back(detail); } + } else { + /* An object that has a subobject with errors */ + for (auto index = obj->begin(); index != obj->end(); ++index) { + array_index = std::atoll(index.key().c_str()); + for (auto index2 = index->begin(); index2 != index->end(); ++index2) { + if (index2->find("_errors") != index2->end()) { + /* A single object where one or more fields generated an error */ + for (auto errordetails = (*index2)["_errors"].begin(); errordetails != (*index2)["_errors"].end(); ++errordetails) { + error_detail detail; + detail.code = (*errordetails)["code"].get(); + detail.reason = (*errordetails)["message"].get(); + detail.object = obj.key(); + detail.field = index2.key(); + detail.index = array_index; + e.errors.emplace_back(detail); + } + } + } + } } } @@ -133,8 +169,13 @@ error_info confirmation_callback_t::get_error() const { std::string prefix = e.errors.size() == 1 ? " " : "\n\t"; for (const auto& error : e.errors) { if (error.object.empty()) { + /* A singular field with an error in an unnamed object */ e.human_readable += prefix + "- " + error.field + ": " + error.reason + " (" + error.code + ")"; + } else if (isdigit(*(error.object.c_str()))) { + /* An unnamed array of objects where one or more generated an error, e.g. slash command bulk registration */ + e.human_readable += prefix + "- [" + error.object + "]." + error.field + ": " + error.reason + " (" + error.code + ")"; } else { + /* A named array of objects whre a field in the object has an error */ e.human_readable += prefix + "- " + error.object + "[" + std::to_string(error.index) + "]." + error.field + ": " + error.reason + " (" + error.code + ")"; } } diff --git a/src/unittest/test.cpp b/src/unittest/test.cpp index 488367d4bd..9c82ee7863 100644 --- a/src/unittest/test.cpp +++ b/src/unittest/test.cpp @@ -58,23 +58,97 @@ Markdown lol ||spoiler|| ~~strikethrough~~ `small *code* block`\n"; set_test(COMPARISON, u1 == u2 && u1 != u3); - dpp::confirmation_callback_t error_test; set_test(ERRORS, false); + + /* Prepare a confirmation_callback_t in error state (400) */ + dpp::confirmation_callback_t error_test; bool error_message_success = false; error_test.http_info.status = 400; - error_test.http_info.body = "{\"message\": \"Invalid Form Body\", \"code\": 50035, \"errors\": {\"options\": {\"0\": {\"name\": {\"_errors\": [{\"code\": \"STRING_TYPE_REGEX\", \"message\": \"String value did not match validation regex.\"}, {\"code\": \"APPLICATION_COMMAND_INVALID_NAME\", \"message\": \"Command name is invalid\"}]}}}}}"; + error_test.http_info.body = "{\ + \"message\": \"Invalid Form Body\",\ + \"code\": 50035,\ + \"errors\": {\ + \"options\": {\ + \"0\": {\ + \"name\": {\ + \"_errors\": [\ + {\ + \"code\": \"STRING_TYPE_REGEX\",\ + \"message\": \"String value did not match validation regex.\"\ + },\ + {\ + \"code\": \"APPLICATION_COMMAND_INVALID_NAME\",\ + \"message\": \"Command name is invalid\"\ + }\ + ]\ + }\ + }\ + }\ + }\ + }"; error_message_success = (error_test.get_error().human_readable == "50035: Invalid Form Body\n\t- options[0].name: String value did not match validation regex. (STRING_TYPE_REGEX)\n\t- options[0].name: Command name is invalid (APPLICATION_COMMAND_INVALID_NAME)"); - error_test.http_info.body = "{\"message\": \"Invalid Form Body\", \"code\": 50035, \"errors\": {\"type\": {\"_errors\": [{\"code\": \"BASE_TYPE_CHOICES\", \"message\": \"Value must be one of {4, 5, 9, 10, 11}.\"}]}}}"; + error_test.http_info.body = "{\ + \"message\": \"Invalid Form Body\",\ + \"code\": 50035,\ + \"errors\": {\ + \"type\": {\ + \"_errors\": [\ + {\ + \"code\": \"BASE_TYPE_CHOICES\",\ + \"message\": \"Value must be one of {4, 5, 9, 10, 11}.\"\ + }\ + ]\ + }\ + }\ + }"; error_message_success = (error_message_success && error_test.get_error().human_readable == "50035: Invalid Form Body - type: Value must be one of {4, 5, 9, 10, 11}. (BASE_TYPE_CHOICES)"); - error_test.http_info.body = "{\"message\": \"Unknown Guild\", \"code\": 10004}"; + error_test.http_info.body = "{\ + \"message\": \"Unknown Guild\",\ + \"code\": 10004\ + }"; error_message_success = (error_message_success && error_test.get_error().human_readable == "10004: Unknown Guild"); - error_test.http_info.body = "{\"message\": \"Invalid Form Body\", \"code\": 50035, \"errors\": {\"allowed_mentions\": {\"_errors\": [{\"code\": \"MESSAGE_ALLOWED_MENTIONS_PARSE_EXCLUSIVE\", \"message\": \"parse:[\\\"users\\\"] and users: [ids...] are mutually exclusive.\"}]}}}"; + error_test.http_info.body = "{\ + \"message\": \"Invalid Form Body\",\ + \"code\": 50035,\ + \"errors\": {\ + \"allowed_mentions\": {\ + \"_errors\": [\ + {\ + \"code\": \"MESSAGE_ALLOWED_MENTIONS_PARSE_EXCLUSIVE\",\ + \"message\": \"parse:[\\\"users\\\"] and users: [ids...] are mutually exclusive.\"\ + }\ + ]\ + }\ + }\ + }"; error_message_success = (error_message_success && error_test.get_error().human_readable == "50035: Invalid Form Body - allowed_mentions: parse:[\"users\"] and users: [ids...] are mutually exclusive. (MESSAGE_ALLOWED_MENTIONS_PARSE_EXCLUSIVE)"); + error_test.http_info.body = "{\ + \"message\": \"Invalid Form Body\",\ + \"code\": 50035,\ + \"errors\": {\ + \"1\": {\ + \"options\": {\ + \"1\": {\ + \"description\": {\ + \"_errors\": [\ + {\ + \"code\": \"BASE_TYPE_BAD_LENGTH\",\ + \"message\": \"Must be between 1 and 100 in length.\"\ + }\ + ]\ + }\ + }\ + }\ + }\ + }\ + }"; + error_message_success = (error_message_success && error_test.get_error().human_readable == "50035: Invalid Form Body - [1].options[1].description: Must be between 1 and 100 in length. (BASE_TYPE_BAD_LENGTH)"); + set_test(ERRORS, error_message_success); set_test(MD_ESC_1, false); From 68fd22a34f9be7359395065bad24c9bb33f60896 Mon Sep 17 00:00:00 2001 From: Craig Edwards Date: Wed, 4 Oct 2023 17:02:25 +0000 Subject: [PATCH 3/3] shut up codacy --- src/dpp/cluster/confirmation.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/dpp/cluster/confirmation.cpp b/src/dpp/cluster/confirmation.cpp index 009d6c71b2..41cc6915a1 100644 --- a/src/dpp/cluster/confirmation.cpp +++ b/src/dpp/cluster/confirmation.cpp @@ -82,12 +82,10 @@ error_info confirmation_callback_t::get_error() const { json& errors = j["errors"]; for (auto obj = errors.begin(); obj != errors.end(); ++obj) { - int array_index = 0; - /* Arrays in the error report are numerically indexed with a number in a string. Ugh. */ if (isdigit(*(obj.key().c_str()))) { /* An array of error messages */ - array_index = std::atoll(obj.key().c_str()); + int array_index = std::atoll(obj.key().c_str()); for (auto index = obj->begin(); index != obj->end(); ++index) { if (index->find("_errors") != index->end()) { /* A single object where one or more fields generated an error */ @@ -146,7 +144,7 @@ error_info confirmation_callback_t::get_error() const { } else { /* An object that has a subobject with errors */ for (auto index = obj->begin(); index != obj->end(); ++index) { - array_index = std::atoll(index.key().c_str()); + int array_index = std::atoll(index.key().c_str()); for (auto index2 = index->begin(); index2 != index->end(); ++index2) { if (index2->find("_errors") != index2->end()) { /* A single object where one or more fields generated an error */