From d6a2b9d19eacc6f929bbcd5b01daadacfdbffc3b Mon Sep 17 00:00:00 2001 From: Filip Leonarski Date: Thu, 17 Sep 2026 13:59:57 +0200 Subject: [PATCH] broker: error responses carry the content type the OpenAPI spec declares The 500 handlers built a correct error_message object but sent it through send_plain(), which labels the body text/plain. The spec declares 500 as application/json + error_message, and the generated clients key their deserialization off the content type: the Python client kept the body as a raw str, handed it to ErrorMessage.from_dict(), and pydantic raised - the exception swallowed by the finally: in response_deserialize, so the caller got a ServiceException with data=None and only the raw text in .body. send_plain becomes send_error, taking the content type alongside the code and the body; the two exception handlers now return it. 400 stays a plain-text exception string, exactly as the spec says; 500 is JSON. Two further 500s did not carry an error_message object at all and now do: the generic std::exception branch of handleParsingException, which returned a bare what(), and ProcessOutput's output-validation failure, which returned the validation dump. A catch-all set_exception_handler covers anything that escapes a route's own handler - httplib would otherwise answer with a bodyless 500, which fails to deserialize the same way. Verified against a running broker: POST /pedestal in the wrong state returns application/json and the generated Python client parses it into ErrorMessage(msg=..., reason='WrongDAQState'); a malformed body still returns 400 text/plain. Co-Authored-By: Claude Opus 5 (1M context) --- broker/JFJochBrokerHttp.cpp | 92 ++++++++++++++++++------------------- broker/JFJochBrokerHttp.h | 24 +++++++--- 2 files changed, 62 insertions(+), 54 deletions(-) diff --git a/broker/JFJochBrokerHttp.cpp b/broker/JFJochBrokerHttp.cpp index dbb9196b6..0943d42ec 100644 --- a/broker/JFJochBrokerHttp.cpp +++ b/broker/JFJochBrokerHttp.cpp @@ -74,15 +74,6 @@ namespace { return "application/octet-stream"; } - std::string error_message_json(const std::string &msg, const std::string &reason) { - Error_message m; - m.setMsg(msg); - m.setReason(reason); - nlohmann::json j; - to_json(j, m); - return j.dump(); - } - inline bool read_file_to_string(const std::string &path, std::string &out) { std::ifstream f(path, std::ios::binary); if (!f) @@ -94,6 +85,15 @@ namespace { } } +std::string JFJochBrokerHttp::error_message_json(const std::string &msg, const std::string &reason) { + Error_message m; + m.setMsg(msg); + m.setReason(reason); + nlohmann::json j; + to_json(j, m); + return j.dump(); +} + JFJochBrokerHttp::JFJochBrokerHttp(const DiffractionExperiment &experiment, const SpotFindingSettings &spot_finding_settings) : state_machine(experiment, services, logger, spot_finding_settings) { @@ -113,29 +113,41 @@ JFJochBrokerHttp &JFJochBrokerHttp::FrontendDirectory(const std::string &directo return *this; } -std::pair JFJochBrokerHttp::handleParsingException(const std::exception &ex) const noexcept { +JFJochBrokerHttp::HttpError JFJochBrokerHttp::handleParsingException(const std::exception &ex) const noexcept { try { throw; } catch (const nlohmann::detail::exception &e) { - return {400, e.what()}; + return {400, e.what(), "text/plain"}; } catch (const org::openapitools::server::helpers::ValidationException &e) { - return {400, e.what()}; + return {400, e.what(), "text/plain"}; } catch (const std::exception &e) { - return {500, e.what()}; + return {500, error_message_json(e.what(), "Other"), "application/json"}; } } -std::pair JFJochBrokerHttp::handleOperationException(const std::exception &ex) const noexcept { +JFJochBrokerHttp::HttpError JFJochBrokerHttp::handleOperationException(const std::exception &ex) const noexcept { try { throw; } catch (const WrongDAQStateException &) { - return {500, error_message_json(ex.what(), "WrongDAQState")}; + return {500, error_message_json(ex.what(), "WrongDAQState"), "application/json"}; } catch (const std::exception &) { - return {500, error_message_json(ex.what(), "Other")}; + return {500, error_message_json(ex.what(), "Other"), "application/json"}; } } void JFJochBrokerHttp::attach(httplib::Server &server) { + // Last resort for an exception that got past a route's own handler - httplib would otherwise + // answer with a bodyless 500, which no generated client can deserialize into an error_message. + server.set_exception_handler([this](const httplib::Request &, httplib::Response &res, + const std::exception_ptr &ep) { + try { + std::rethrow_exception(ep); + } catch (const std::exception &e) { + send_error(res, handleOperationException(e)); + } catch (...) { + send_error(res, {500, error_message_json("Unknown exception", "Other"), "application/json"}); + } + }); register_routes(server); } @@ -145,8 +157,7 @@ void JFJochBrokerHttp::register_routes(httplib::Server &server) { try { (this->*method)(res); } catch (const std::exception &e) { - auto [c, s] = handleOperationException(e); - send_plain(res, c, s); + send_error(res, handleOperationException(e)); } }; }; @@ -160,8 +171,7 @@ void JFJochBrokerHttp::register_routes(httplib::Server &server) { v.validate(); } catch (const std::exception &e) { // Malformed or invalid request body. - auto [c, s] = handleParsingException(e); - send_plain(res, c, s); + send_error(res, handleParsingException(e)); return; } try { @@ -170,8 +180,7 @@ void JFJochBrokerHttp::register_routes(httplib::Server &server) { // Operation failure (e.g. WrongDAQState, a failed synchronous start): return the // structured Error_message JSON with a reason field, like the no-arg endpoints, so // clients can distinguish the cause instead of receiving opaque plain text. - auto [c, s] = handleOperationException(e); - send_plain(res, c, s); + send_error(res, handleOperationException(e)); } }; }; @@ -181,8 +190,7 @@ void JFJochBrokerHttp::register_routes(httplib::Server &server) { try { (this->*method)(req, res); } catch (const std::exception &e) { - auto [c, s] = handleOperationException(e); - send_plain(res, c, s); + send_error(res, handleOperationException(e)); } }; }; @@ -210,16 +218,14 @@ void JFJochBrokerHttp::register_routes(httplib::Server &server) { try { config_internal_generator_image_put(parse_query_value(req, "id"), req, res); } catch (const std::exception &e) { - auto [c, s] = handleOperationException(e); - send_plain(res, c, s); + send_error(res, handleOperationException(e)); } }); server.Put("/config/internal_generator_image.tiff", [this](const httplib::Request &req, httplib::Response &res) { try { config_internal_generator_image_tiff_put(parse_query_value(req, "id"), req, res); } catch (const std::exception &e) { - auto [c, s] = handleOperationException(e); - send_plain(res, c, s); + send_error(res, handleOperationException(e)); } }); server.Get("/config/mask", bind_noarg(&JFJochBrokerHttp::config_mask_get)); @@ -250,8 +256,7 @@ void JFJochBrokerHttp::register_routes(httplib::Server &server) { try { image_buffer_image_cbor_get(parse_query_value(req, "id"), res); } catch (const std::exception &e) { - auto [c, s] = handleOperationException(e); - send_plain(res, c, s); + send_error(res, handleOperationException(e)); } }); @@ -272,8 +277,7 @@ void JFJochBrokerHttp::register_routes(httplib::Server &server) { res ); } catch (const std::exception &e) { - auto [c, s] = handleOperationException(e); - send_plain(res, c, s); + send_error(res, handleOperationException(e)); } }); @@ -281,8 +285,7 @@ void JFJochBrokerHttp::register_routes(httplib::Server &server) { try { image_buffer_image_tiff_get(parse_query_value(req, "id"), res); } catch (const std::exception &e) { - auto [c, s] = handleOperationException(e); - send_plain(res, c, s); + send_error(res, handleOperationException(e)); } }); @@ -298,8 +301,7 @@ void JFJochBrokerHttp::register_routes(httplib::Server &server) { parse_query_value(req, "sc"), res); } catch (const std::exception &e) { - auto [c, s] = handleOperationException(e); - send_plain(res, c, s); + send_error(res, handleOperationException(e)); } }); @@ -309,8 +311,7 @@ void JFJochBrokerHttp::register_routes(httplib::Server &server) { parse_query_string(req, "roi"), res); } catch (const std::exception &e) { - auto [c, s] = handleOperationException(e); - send_plain(res, c, s); + send_error(res, handleOperationException(e)); } }); @@ -323,8 +324,7 @@ void JFJochBrokerHttp::register_routes(httplib::Server &server) { parse_query_string(req, "azint_unit"), res); } catch (const std::exception &e) { - auto [c, s] = handleOperationException(e); - send_plain(res, c, s); + send_error(res, handleOperationException(e)); } }); @@ -337,8 +337,7 @@ void JFJochBrokerHttp::register_routes(httplib::Server &server) { try { statistics_get(res); } catch (const std::exception &e) { - auto [c, s] = handleOperationException(e); - send_plain(res, c, s); + send_error(res, handleOperationException(e)); } }); @@ -350,8 +349,7 @@ void JFJochBrokerHttp::register_routes(httplib::Server &server) { try { wait_till_done_post(parse_query_value(req, "timeout"), res); } catch (const std::exception &e) { - auto [c, s] = handleOperationException(e); - send_plain(res, c, s); + send_error(res, handleOperationException(e)); } }); @@ -359,8 +357,7 @@ void JFJochBrokerHttp::register_routes(httplib::Server &server) { try { wait_until_running_post(parse_query_value(req, "timeout"), res); } catch (const std::exception &e) { - auto [c, s] = handleOperationException(e); - send_plain(res, c, s); + send_error(res, handleOperationException(e)); } }); @@ -451,7 +448,8 @@ void JFJochBrokerHttp::wait_till_done_post(const std::optional &timeout // is never reported as an error. A Warning - a receiver that could not keep up - is // still a 200: the data is there, and the run is done. if (status.message_severity == BrokerStatus::MessageSeverity::Error) { - send_plain(response, 500, error_message_json(status.message.value_or("Unknown error"), "Other")); + send_error(response, {500, error_message_json(status.message.value_or("Unknown error"), "Other"), + "application/json"}); return; } response.status = 200; diff --git a/broker/JFJochBrokerHttp.h b/broker/JFJochBrokerHttp.h index 6d6ddc859..ebeb88baa 100644 --- a/broker/JFJochBrokerHttp.h +++ b/broker/JFJochBrokerHttp.h @@ -45,18 +45,28 @@ class JFJochBrokerHttp { JFJochStateMachine state_machine; std::string frontend_directory; - static void send_plain(httplib::Response &res, int code, const std::string &body) { - res.status = code; - res.set_content(body, "text/plain"); + // The API declares 400 as a plain-text exception string and 500 as an error_message object, + // so an error response has to carry the content type its body actually is - a JSON body + // labelled text/plain is what the generated clients refuse to deserialize. + struct HttpError { + int code; + std::string body; + const char *content_type; + }; + + static void send_error(httplib::Response &res, const HttpError &error) { + res.status = error.code; + res.set_content(error.body, error.content_type); } + static std::string error_message_json(const std::string &msg, const std::string &reason); + template void ProcessOutput(const T &output, httplib::Response &response) { std::stringstream s; if (!output.validate(s)) { logger.Error(s.str()); - response.status = 500; - response.set_content(s.str(), "text/plain"); + send_error(response, {500, error_message_json(s.str(), "Other"), "application/json"}); return; } @@ -65,8 +75,8 @@ class JFJochBrokerHttp { response.set_content(j.dump(), "application/json"); } - std::pair handleParsingException(const std::exception &ex) const noexcept; - std::pair handleOperationException(const std::exception &ex) const noexcept; + HttpError handleParsingException(const std::exception &ex) const noexcept; + HttpError handleOperationException(const std::exception &ex) const noexcept; void register_routes(httplib::Server &server);