diff --git a/src/slic3r/GUI/UserManager.cpp b/src/slic3r/GUI/UserManager.cpp index e874c2158d..f181f3e568 100644 --- a/src/slic3r/GUI/UserManager.cpp +++ b/src/slic3r/GUI/UserManager.cpp @@ -31,14 +31,15 @@ int UserManager::parse_json(std::string payload) { bool restored_json = false; json j; - json j_pre = json::parse(payload); - if (j_pre.empty()) { - return -1; - } //bind/unbind try { + json j_pre = json::parse(payload); + if (j_pre.empty()) { + return -1; + } + if (j_pre.contains("bind")) { if (j_pre["bind"].contains("command")) { diff --git a/src/slic3r/Utils/Duet.cpp b/src/slic3r/Utils/Duet.cpp index 37db6b6c33..e26a493131 100644 --- a/src/slic3r/Utils/Duet.cpp +++ b/src/slic3r/Utils/Duet.cpp @@ -274,13 +274,4 @@ bool Duet::start_print(wxString &msg, const std::string &filename, ConnectionTyp return res; } -int Duet::get_err_code_from_body(const std::string &body) const -{ - pt::ptree root; - std::istringstream iss (body); // wrap returned json to istringstream - pt::read_json(iss, root); - - return root.get("err", 0); -} - } diff --git a/src/slic3r/Utils/Duet.hpp b/src/slic3r/Utils/Duet.hpp index 2a91aa8536..7808c51fde 100644 --- a/src/slic3r/Utils/Duet.hpp +++ b/src/slic3r/Utils/Duet.hpp @@ -40,7 +40,6 @@ private: ConnectionType connect(wxString &msg) const; void disconnect(ConnectionType connectionType) const; bool start_print(wxString &msg, const std::string &filename, ConnectionType connectionType, bool simulationMode) const; - int get_err_code_from_body(const std::string &body) const; }; } diff --git a/src/slic3r/Utils/ESP3D.cpp b/src/slic3r/Utils/ESP3D.cpp index 6de41ebe55..5c10862118 100644 --- a/src/slic3r/Utils/ESP3D.cpp +++ b/src/slic3r/Utils/ESP3D.cpp @@ -146,15 +146,6 @@ bool ESP3D::start_print(wxString& msg, const std::string& filename) const return ret; } -int ESP3D::get_err_code_from_body(const std::string& body) const -{ - pt::ptree root; - std::istringstream iss(body); // wrap returned json to istringstream - pt::read_json(iss, root); - - return root.get("err", 0); -} - // ESP3D only accepts 8.3 filenames else it crashes marlin and other undefined behaviour std::string ESP3D::get_short_name(const std::string& filename) const { diff --git a/src/slic3r/Utils/ESP3D.hpp b/src/slic3r/Utils/ESP3D.hpp index 7ac3c66f48..44d206d238 100644 --- a/src/slic3r/Utils/ESP3D.hpp +++ b/src/slic3r/Utils/ESP3D.hpp @@ -33,7 +33,6 @@ private: std::string m_console_port; bool start_print(wxString& msg, const std::string& filename) const; - int get_err_code_from_body(const std::string& body) const; std::string get_short_name(const std::string& filename) const; std::string format_command(const std::string& path, const std::string& arg, const std::string& val) const; }; diff --git a/src/slic3r/Utils/Flashforge.cpp b/src/slic3r/Utils/Flashforge.cpp index 88b3eb3f69..a042a889ae 100644 --- a/src/slic3r/Utils/Flashforge.cpp +++ b/src/slic3r/Utils/Flashforge.cpp @@ -510,12 +510,22 @@ bool Flashforge::fetch_material_slots(std::vector& slots if (!request_local_api_json("detail", json{{"serialNumber", m_serial_number}, {"checkCode", m_check_code}}.dump(), body, msg)) return false; - const auto parsed = json::parse(body, nullptr, false, true); - if (parsed.is_discarded()) { + if (!parse_material_slots(body, slots, supports_material_station)) { msg = _(L("Flashforge returned an invalid JSON response.")); return false; } + return true; +} + +bool Flashforge::parse_material_slots(const std::string& body, std::vector& slots, bool* supports_material_station) +{ + slots.clear(); + + const auto parsed = json::parse(body, nullptr, false, true); + if (parsed.is_discarded()) + return false; + const auto& detail = parsed.contains("detail") ? parsed["detail"] : parsed; const auto& station = detail.contains("matlStationInfo") ? detail["matlStationInfo"] : detail.contains("MatlStationInfo") ? detail["MatlStationInfo"] : json(); @@ -542,12 +552,21 @@ bool Flashforge::fetch_material_slots(std::vector& slots if (supports_material_station != nullptr) *supports_material_station = reports_material_station; + // Fields are read leniently: firmware may send numbers as strings or flags as numbers. for (const auto& slot : slot_infos) { + if (!slot.is_object()) + continue; FlashforgeMaterialSlot info; - info.slot_id = slot.value("slotId", static_cast(slots.size()) + 1); - info.has_filament = slot.value("hasFilament", false); - info.material_name = slot.value("materialName", std::string()); - info.material_color = slot.value("materialColor", std::string()); + info.slot_id = static_cast(slots.size()) + 1; + if (const auto it = slot.find("slotId"); it != slot.end()) + try_parse_json_int(*it, info.slot_id); + int has_filament = 0; + if (const auto it = slot.find("hasFilament"); it != slot.end() && try_parse_json_int(*it, has_filament)) + info.has_filament = has_filament != 0; + if (const auto it = slot.find("materialName"); it != slot.end() && it->is_string()) + info.material_name = it->get(); + if (const auto it = slot.find("materialColor"); it != slot.end() && it->is_string()) + info.material_color = it->get(); slots.emplace_back(std::move(info)); } @@ -670,13 +689,4 @@ std::string Flashforge::extract_host_name() const return out; } -int Flashforge::get_err_code_from_body(const std::string& body) const -{ - pt::ptree root; - std::istringstream iss(body); // wrap returned json to istringstream - pt::read_json(iss, root); - - return root.get("err", 0); -} - } // namespace Slic3r diff --git a/src/slic3r/Utils/Flashforge.hpp b/src/slic3r/Utils/Flashforge.hpp index ea7acdcb11..237cf4c009 100644 --- a/src/slic3r/Utils/Flashforge.hpp +++ b/src/slic3r/Utils/Flashforge.hpp @@ -45,6 +45,8 @@ public: PrintHostPostUploadActions get_post_upload_actions() const override { return PrintHostPostUploadAction::StartPrint; } std::string get_host() const override { return m_host; } bool fetch_material_slots(std::vector& slots, bool* supports_material_station, wxString& msg) const; + // Parses a local API "detail" reply. Returns false when the body is not valid JSON. + static bool parse_material_slots(const std::string& body, std::vector& slots, bool* supports_material_station); static bool discover_printers(std::vector& printers, wxString& msg, int timeout_ms = 10000, int idle_timeout_ms = 1500, int max_retries = 3); private: @@ -68,7 +70,6 @@ private: bool request_local_api_json(const std::string& path, const std::string& body, std::string& response_body, wxString& error_msg) const; std::string make_http_url(const std::string& path) const; std::string extract_host_name() const; - int get_err_code_from_body(const std::string &body) const; bool connect(wxString& msg) const; bool start_print(wxString& msg, const std::string& filename) const; }; diff --git a/src/slic3r/Utils/MKS.cpp b/src/slic3r/Utils/MKS.cpp index c4aa4c7262..ced7af9c9f 100644 --- a/src/slic3r/Utils/MKS.cpp +++ b/src/slic3r/Utils/MKS.cpp @@ -141,13 +141,4 @@ bool MKS::start_print(wxString& msg, const std::string& filename) const return ret; } -int MKS::get_err_code_from_body(const std::string& body) const -{ - pt::ptree root; - std::istringstream iss(body); // wrap returned json to istringstream - pt::read_json(iss, root); - - return root.get("err", 0); -} - } // Slic3r diff --git a/src/slic3r/Utils/MKS.hpp b/src/slic3r/Utils/MKS.hpp index 79143fdd9a..01cdafc405 100644 --- a/src/slic3r/Utils/MKS.hpp +++ b/src/slic3r/Utils/MKS.hpp @@ -34,7 +34,6 @@ private: std::string get_upload_url(const std::string& filename) const; bool start_print(wxString& msg, const std::string& filename) const; - int get_err_code_from_body(const std::string& body) const; }; } diff --git a/src/slic3r/Utils/PrintHost.cpp b/src/slic3r/Utils/PrintHost.cpp index f3333c731d..41241abb10 100644 --- a/src/slic3r/Utils/PrintHost.cpp +++ b/src/slic3r/Utils/PrintHost.cpp @@ -3,10 +3,13 @@ #include #include #include +#include #include #include #include #include +#include +#include #include #include @@ -172,6 +175,20 @@ std::string moonraker_error_reason(const std::string &body) } // namespace +int PrintHost::get_err_code_from_body(const std::string &body) +{ + boost::property_tree::ptree root; + std::istringstream iss(body); + try { + boost::property_tree::read_json(iss, root); + } catch (const std::exception &ex) { + BOOST_LOG_TRIVIAL(error) << "PrintHost: response is not valid JSON: " << ex.what(); + return -1; + } + + return root.get("err", 0); +} + wxString PrintHost::format_error(const std::string &body, const std::string &error, unsigned status) const { if (status != 0) { @@ -303,7 +320,12 @@ void PrintHostJobQueue::priv::bg_thread_main() % job.cancelled; if (! job.cancelled) { - perform_job(std::move(job)); + // A failing job must not stop the worker, or later jobs would stay queued forever. + try { + perform_job(std::move(job)); + } catch (const std::exception &e) { + emit_error(e.what()); + } } remove_source(); diff --git a/src/slic3r/Utils/PrintHost.hpp b/src/slic3r/Utils/PrintHost.hpp index 87a8df8934..ed86f213e7 100644 --- a/src/slic3r/Utils/PrintHost.hpp +++ b/src/slic3r/Utils/PrintHost.hpp @@ -87,6 +87,8 @@ public: static PrintHost* get_print_host(DynamicPrintConfig *config); static std::string get_print_host_webui(DynamicPrintConfig *config); + // Reads the "err" field of a JSON reply, 0 when absent. Returns -1 when the body is not valid JSON. + static int get_err_code_from_body(const std::string &body); //Support for cloud webui login virtual bool is_cloud() const { return false; } diff --git a/src/slic3r/Utils/UltiMaker.cpp b/src/slic3r/Utils/UltiMaker.cpp index 0376578db0..badb69d209 100644 --- a/src/slic3r/Utils/UltiMaker.cpp +++ b/src/slic3r/Utils/UltiMaker.cpp @@ -654,13 +654,4 @@ bool UltiMaker::start_print(wxString &msg, const std::string &filename, Connecti return res; } -int UltiMaker::get_err_code_from_body(const std::string &body) const -{ - pt::ptree root; - std::istringstream iss (body); // wrap returned json to istringstream - pt::read_json(iss, root); - - return root.get("err", 0); -} - } diff --git a/src/slic3r/Utils/UltiMaker.hpp b/src/slic3r/Utils/UltiMaker.hpp index b32b87434f..c1f9ced304 100644 --- a/src/slic3r/Utils/UltiMaker.hpp +++ b/src/slic3r/Utils/UltiMaker.hpp @@ -64,7 +64,6 @@ private: void set_auth(Http& http) const; void disconnect(ConnectionType connectionType) const; bool start_print(wxString &msg, const std::string &filename, ConnectionType connectionType) const; - int get_err_code_from_body(const std::string &body) const; }; } diff --git a/tests/slic3rutils/CMakeLists.txt b/tests/slic3rutils/CMakeLists.txt index 94ebaed35e..a3863134d5 100644 --- a/tests/slic3rutils/CMakeLists.txt +++ b/tests/slic3rutils/CMakeLists.txt @@ -28,6 +28,7 @@ add_executable(${_TEST_NAME}_tests test_plugin_audit.cpp test_shortcuts.cpp test_file_url.cpp + test_user_manager.cpp ../fff_print/test_helpers.cpp ) diff --git a/tests/slic3rutils/test_printhost.cpp b/tests/slic3rutils/test_printhost.cpp index 557d4a5c89..a102d83770 100644 --- a/tests/slic3rutils/test_printhost.cpp +++ b/tests/slic3rutils/test_printhost.cpp @@ -1,8 +1,12 @@ #include +#include +#include + #include #include "slic3r/Utils/PrintHost.hpp" +#include "slic3r/Utils/Flashforge.hpp" using namespace Slic3r; @@ -46,6 +50,14 @@ std::string moonraker_error(int code, const std::string& message, const std::str constexpr const char* k_busy_file_403 = R"JSON({"error": {"code": 403, "message": "Forbidden", "traceback": "Traceback (most recent call last):\n\n File \"/home/lava/moonraker/moonraker/components/file_manager/file_manager.py\", line 1017, in _finish_gcode_upload\n can_start = self._handle_operation_check(check_path)\n ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n\nmoonraker.utils.exceptions.ServerError: File currently in use\n\nDuring handling of the above exception, another exception occurred:\n\nTraceback (most recent call last):\n\n File \"/home/lava/moonraker/moonraker/components/application.py\", line 1069, in post\n raise tornado.web.HTTPError(\ntornado.web.HTTPError: HTTP 403: Forbidden (File is loaded, upload not permitted)\n"}})JSON"; +// Replies a print host can send instead of JSON: a proxy or login page, nothing, a cut-off body. +const std::vector non_json_replies = { + "proxy login required", + "", + "{\"err\":", + "{\"detail\":{\"matlStationInfo\":{\"slotInfos\":[{\"slotId\":1,", +}; + } // namespace TEST_CASE("A Klipper upload error shows its reason instead of a Python traceback", "[PrintHost][Regression]") @@ -211,3 +223,94 @@ TEST_CASE("Error bodies that are not a Moonraker envelope are left unchanged", " CHECK(format_error("", "curl:Could not connect", 0) == "curl:Could not connect"); } } + +TEST_CASE("Print host error code is read from a JSON reply", "[PrintHost]") +{ + CHECK(PrintHost::get_err_code_from_body(R"({"err":0})") == 0); + CHECK(PrintHost::get_err_code_from_body(R"({"err":2})") == 2); + CHECK(PrintHost::get_err_code_from_body(R"({"status":"ok"})") == 0); +} + +TEST_CASE("Print host error code reports a reply that is not JSON as an error", "[PrintHost]") +{ + const std::string body = GENERATE(from_range(non_json_replies)); + int err = 0; + REQUIRE_NOTHROW(err = PrintHost::get_err_code_from_body(body)); + CHECK(err != 0); +} + +TEST_CASE("Print host error code tolerates a wrongly typed err field", "[PrintHost]") +{ + const std::string body = GENERATE(as{}, R"({"err":"busy"})", R"({"err":{"code":1}})", R"([1,2])"); + CHECK_NOTHROW(PrintHost::get_err_code_from_body(body)); +} + +TEST_CASE("Flashforge material slots are read from a well-formed reply", "[PrintHost][Flashforge]") +{ + const std::string body = R"({"code":0,"detail":{"hasMatlStation":true,"matlStationInfo":{"slotCnt":2,"slotInfos":[ + {"slotId":1,"hasFilament":true,"materialName":"PLA","materialColor":"#FFFFFF"}, + {"slotId":2,"hasFilament":false,"materialName":"","materialColor":""}]}}})"; + + std::vector slots; + bool supports_station = false; + REQUIRE(Flashforge::parse_material_slots(body, slots, &supports_station)); + CHECK(supports_station); + REQUIRE(slots.size() == 2); + CHECK(slots[0].slot_id == 1); + CHECK(slots[0].has_filament); + CHECK(slots[0].material_name == "PLA"); + CHECK(slots[0].material_color == "#FFFFFF"); + CHECK(slots[1].slot_id == 2); + CHECK_FALSE(slots[1].has_filament); +} + +TEST_CASE("Flashforge material slots accept numbers as strings and flags as numbers", "[PrintHost][Flashforge]") +{ + const std::string body = R"({"detail":{"matlStationInfo":{"slotInfos":[ + {"slotId":"3","hasFilament":1,"materialName":null,"materialColor":7}]}}})"; + + std::vector slots; + REQUIRE_NOTHROW(Flashforge::parse_material_slots(body, slots, nullptr)); + REQUIRE(slots.size() == 1); + CHECK(slots[0].slot_id == 3); + CHECK(slots[0].has_filament); + CHECK(slots[0].material_name.empty()); + CHECK(slots[0].material_color.empty()); +} + +TEST_CASE("Flashforge material slots skip entries that are not objects", "[PrintHost][Flashforge]") +{ + const std::string body = R"({"detail":{"matlStationInfo":{"slotInfos":[5,"slot",null,[], + {"slotId":4,"hasFilament":true,"materialName":"PETG"}]}}})"; + + std::vector slots; + REQUIRE_NOTHROW(Flashforge::parse_material_slots(body, slots, nullptr)); + REQUIRE(slots.size() == 1); + CHECK(slots[0].slot_id == 4); + CHECK(slots[0].material_name == "PETG"); +} + +TEST_CASE("Flashforge material slots tolerate slot info that is not a list", "[PrintHost][Flashforge]") +{ + const std::string body = GENERATE(as{}, + R"({"detail":{"matlStationInfo":{"slotInfos":5}}})", + R"({"detail":{"matlStationInfo":{"slotInfos":"none"}}})", + R"({"detail":{"matlStationInfo":7}})", + R"({"detail":"offline"})"); + + std::vector slots; + bool ok = false; + REQUIRE_NOTHROW(ok = Flashforge::parse_material_slots(body, slots, nullptr)); + CHECK(ok); + CHECK(slots.empty()); +} + +TEST_CASE("Flashforge material slots reject a reply that is not JSON", "[PrintHost][Flashforge]") +{ + const std::string body = GENERATE(from_range(non_json_replies)); + std::vector slots; + bool ok = true; + REQUIRE_NOTHROW(ok = Flashforge::parse_material_slots(body, slots, nullptr)); + CHECK_FALSE(ok); + CHECK(slots.empty()); +} diff --git a/tests/slic3rutils/test_user_manager.cpp b/tests/slic3rutils/test_user_manager.cpp new file mode 100644 index 0000000000..cfccebe1cc --- /dev/null +++ b/tests/slic3rutils/test_user_manager.cpp @@ -0,0 +1,23 @@ +#include + +#include + +#include "slic3r/GUI/UserManager.hpp" + +using namespace Slic3r; + +TEST_CASE("User message that is not JSON is rejected without throwing", "[UserManager]") +{ + const std::string payload = GENERATE(as{}, "not json", "", "", "{\"bind\":"); + UserManager manager; + int result = 0; + REQUIRE_NOTHROW(result = manager.parse_json(payload)); + CHECK(result == -1); +} + +TEST_CASE("User message without a successful bind is ignored", "[UserManager]") +{ + const std::string payload = GENERATE(as{}, "{}", R"({"bind":{"command":"unbind"}})", R"({"bind":"bind"})", "[1]"); + UserManager manager; + CHECK(manager.parse_json(payload) == -1); +}