From 45b2f3f895f032fca2b3b627ebe6cd1feeecd965 Mon Sep 17 00:00:00 2001 From: Will Sobel Date: Wed, 27 Aug 2025 12:10:09 +0200 Subject: [PATCH 1/5] Fixed request info for invalid paramater --- src/mtconnect/agent.cpp | 15 ++- src/mtconnect/agent.hpp | 3 +- src/mtconnect/sink/rest_sink/error.hpp | 113 +++++++++++++----- src/mtconnect/sink/rest_sink/request.hpp | 21 ++-- src/mtconnect/sink/rest_sink/rest_service.cpp | 17 ++- src/mtconnect/sink/rest_sink/rest_service.hpp | 2 +- src/mtconnect/sink/rest_sink/server.hpp | 4 +- test_package/agent_asset_test.cpp | 76 ++++++------ test_package/agent_test.cpp | 5 +- test_package/test_utilities.hpp | 9 +- 10 files changed, 165 insertions(+), 100 deletions(-) diff --git a/src/mtconnect/agent.cpp b/src/mtconnect/agent.cpp index 5263e336d..bac1512c9 100644 --- a/src/mtconnect/agent.cpp +++ b/src/mtconnect/agent.cpp @@ -372,7 +372,8 @@ namespace mtconnect { else if (old || !added) { di = device->getAssetChanged(); - /// If we have changed the asset that is currently recorded as added. Make added unavailable. + /// If we have changed the asset that is currently recorded as added. Make added + /// unavailable. if (added) { auto last = getLatest(added); @@ -782,10 +783,11 @@ namespace mtconnect { auto last = getLatest(changed); if (last && asset->getAssetId() == last->getValue()) { - m_loopback->receive(changed, {{"assetType", asset->getName()}, {"VALUE", g_unavailable}}); + m_loopback->receive(changed, + {{"assetType", asset->getName()}, {"VALUE", g_unavailable}}); } } - + auto added = dev->getAssetAdded(); if (added) { @@ -798,7 +800,7 @@ namespace mtconnect { } } } - + // --------------------------------------- // Agent Device // --------------------------------------- @@ -934,8 +936,9 @@ namespace mtconnect { // Create asset removed data item and add it to the device. entity::ErrorList errors; auto di = DataItem::make({{"type", "ASSET_ADDED"s}, - {"id", device->getId() + "_asset_add"}, {"discrete", true}, - {"category", "EVENT"s}}, + {"id", device->getId() + "_asset_add"}, + {"discrete", true}, + {"category", "EVENT"s}}, errors); device->addDataItem(di, errors); } diff --git a/src/mtconnect/agent.hpp b/src/mtconnect/agent.hpp index 8c08c915a..cd11c975f 100644 --- a/src/mtconnect/agent.hpp +++ b/src/mtconnect/agent.hpp @@ -380,7 +380,8 @@ namespace mtconnect { asset::AssetList &list); /// @brief Send asset changed and added observation when an asset is removed. /// - /// Also sets asset changed and added to `UNAVAILABLE` if the asset removed asset was the last changed. + /// Also sets asset changed and added to `UNAVAILABLE` if the asset removed asset was the last + /// changed. /// /// @param device The device related to the asset /// @param asset The asset diff --git a/src/mtconnect/sink/rest_sink/error.hpp b/src/mtconnect/sink/rest_sink/error.hpp index 32b9be5a4..0a0edacea 100644 --- a/src/mtconnect/sink/rest_sink/error.hpp +++ b/src/mtconnect/sink/rest_sink/error.hpp @@ -28,9 +28,13 @@ namespace mtconnect::sink::rest_sink { using status = boost::beast::http::status; + /// @brief An Error entity for error reporting + /// Builds an Error entity with errorCode, URI, Request, and ErrorMessage. + /// Subclasses can add additional information class AGENT_LIB_API Error : public mtconnect::entity::Entity { public: + /// @brief Error codes for MTConnect REST API enum class ErrorCode { ASSET_NOT_FOUND, @@ -69,9 +73,17 @@ namespace mtconnect::sink::rest_sink { return error; } + /// @brief Get the name for an error code + /// @param code the error code static const std::string nameForCode(ErrorCode code); + /// @brief Get the controlled vocabulary enumeration string for an error code + /// @param code the error code static const std::string enumForCode(ErrorCode code); + /// @brief static error factory method + /// @param code the error code + /// @param errorMessage optional error message + /// @param request optional request string static entity::EntityPtr make(const ErrorCode code, std::optional errorMessage = std::nullopt, std::optional request = std::nullopt) @@ -97,6 +109,7 @@ namespace mtconnect::sink::rest_sink { using ErrorPtr = std::shared_ptr; + /// @brief A QueryParameter entity for error reporting class AGENT_LIB_API QueryParameter : public entity::Entity { public: @@ -123,12 +136,16 @@ namespace mtconnect::sink::rest_sink { return qp; } + /// @brief static factory method + /// @param properties the properties for the QueryParameter static entity::EntityPtr make(const entity::Properties &properties) { return std::make_shared(properties); } }; + /// @brief An InvalidParameterValue error + /// Builds a QueryParameter with name, value, type, and format class AGENT_LIB_API InvalidParameterValue : public Error { public: @@ -158,6 +175,13 @@ namespace mtconnect::sink::rest_sink { return factory; } + /// @brief static factory method + /// @param name the name of the parameter + /// @param value the value of the parameter + /// @param type the type of the parameter + /// @param format the format of the parameter + /// @param errorMessage optional error message + /// @param request optional request string static entity::EntityPtr make(const std::string &name, const std::string &value, const std::string &type, const std::string &format, std::optional errorMessage = std::nullopt, @@ -180,6 +204,8 @@ namespace mtconnect::sink::rest_sink { } }; + /// @brief An OutOfRange error + /// Builds a QueryParameter with name, value, min, and max class AGENT_LIB_API OutOfRange : public Error { public: @@ -205,6 +231,13 @@ namespace mtconnect::sink::rest_sink { return factory; } + /// @brief static factory method + /// @param name the name of the parameter + /// @param value the value of the parameter + /// @param min the minimum value of the parameter + /// @param max the maximum value of the parameter + /// @param errorMessage optional error message + /// @param request optional request string static entity::EntityPtr make(const std::string &name, int64_t value, int64_t min, int64_t max, std::optional errorMessage = std::nullopt, std::optional request = std::nullopt) @@ -225,6 +258,7 @@ namespace mtconnect::sink::rest_sink { } }; + /// @brief An AssetNotFound error class AGENT_LIB_API AssetNotFound : public Error { public: @@ -249,6 +283,10 @@ namespace mtconnect::sink::rest_sink { return factory; } + /// @brief static factory method + /// @param assetId the asset ID that was not found + /// @param errorMessage optional error message + /// @param request optional request string static entity::EntityPtr make(const std::string &assetId, std::optional errorMessage = std::nullopt, std::optional request = std::nullopt) @@ -266,6 +304,7 @@ namespace mtconnect::sink::rest_sink { } }; + /// @brief An exception that gets thrown during REST processing with an error and a status class AGENT_LIB_API RestError { public: @@ -276,12 +315,13 @@ namespace mtconnect::sink::rest_sink { /// @param format optional format for the error RestError(entity::EntityPtr error, std::string accepts = "application/xml", status st = status::bad_request, std::optional format = std::nullopt, - std::optional request = std::nullopt) - : m_errors({error}), m_accepts(accepts), m_status(st), m_format(format) - { - if (request) - setRequest(*request); - } + std::optional requestId = std::nullopt) + : m_errors({error}), + m_accepts(accepts), + m_status(st), + m_format(format), + m_requestId(requestId) + {} /// @brief An exception that gets thrown during REST processing with an error and a status /// @param errors a list of errors @@ -290,12 +330,9 @@ namespace mtconnect::sink::rest_sink { /// @param format optional format for the error RestError(entity::EntityList &errors, std::string accepts = "application/xml", status st = status::bad_request, std::optional format = std::nullopt, - std::optional request = std::nullopt) - : m_errors(errors), m_accepts(accepts), m_status(st), m_format(format) - { - if (request) - setRequest(*request); - } + std::optional requestId = std::nullopt) + : m_errors(errors), m_accepts(accepts), m_status(st), m_format(format), m_requestId(requestId) + {} /// @brief An exception that gets thrown during REST processing with an error and a status /// @param error the error entity @@ -304,12 +341,14 @@ namespace mtconnect::sink::rest_sink { /// @param format optional format for the error RestError(entity::EntityPtr error, const printer::Printer *printer = nullptr, status st = status::bad_request, std::optional format = std::nullopt, - std::optional request = std::nullopt) - : m_errors({error}), m_status(st), m_format(format), m_printer(printer) - { - if (request) - setRequest(*request); - } + std::optional requestId = std::nullopt) + : m_errors({error}), + m_status(st), + m_format(format), + m_requestId(requestId), + m_printer(printer) + + {} /// @brief An exception that gets thrown during REST processing with an error and a status /// @param errors a list of errors @@ -318,12 +357,9 @@ namespace mtconnect::sink::rest_sink { /// @param format optional format for the error RestError(entity::EntityList &errors, const printer::Printer *printer = nullptr, status st = status::bad_request, std::optional format = std::nullopt, - std::optional request = std::nullopt) - : m_errors(errors), m_status(st), m_format(format), m_printer(printer) - { - if (request) - setRequest(*request); - } + std::optional requestId = std::nullopt) + : m_errors(errors), m_status(st), m_format(format), m_requestId(requestId), m_printer(printer) + {} ~RestError() = default; RestError(const RestError &o) = default; @@ -336,24 +372,33 @@ namespace mtconnect::sink::rest_sink { e->setProperty("URI", uri); } - /// @brief set the Request for all errors + /// @brief set the Request ID for the websocket responses + /// @param requestId the Request ID + void setRequestId(const std::string &requestId) { m_requestId = requestId; } + const auto &getRequestId() const { return m_requestId; } + + /// @brief The response document type for the request (e.g. MTConnectDevices) /// @param request the Request void setRequest(const std::string &request) { - m_request = request; for (auto &e : m_errors) e->setProperty("Request", request); } const auto &getErrors() const { return m_errors; } + void setStatus(status st) { m_status = st; } const auto &getStatus() const { return m_status; } + void setFormat(const std::string &format) { m_format = format; } const auto &getFormat() const { return m_format; } + const auto &getAccepts() const { return m_accepts; } - const auto &getRequest() const { return m_request; } + auto getPrinter() const { return m_printer; } + /// @brief Get a string representation of the error(s) concating the error messages. + /// @return a string representation of the error(s) std::string what() const { std::stringstream ss; @@ -371,12 +416,14 @@ namespace mtconnect::sink::rest_sink { } protected: - entity::EntityList m_errors; - std::string m_accepts {"application/xml"}; - status m_status; - std::optional m_format; - std::optional m_request; - const printer::Printer *m_printer {nullptr}; + entity::EntityList m_errors; ///< The list of errors to be reported + std::string m_accepts {"application/xml"}; ///< The accepted mime types for the response + status m_status; ///< The HTTP status code + std::optional m_format; ///< The format for the error response overriding accepts + std::optional m_requestId; ///< The request id for the response + const printer::Printer *m_printer { + nullptr}; ///< The printer to use for the response. If the printer is not specified it will + ///< be inferred from the accepts or format. }; } // namespace mtconnect::sink::rest_sink diff --git a/src/mtconnect/sink/rest_sink/request.hpp b/src/mtconnect/sink/rest_sink/request.hpp index 1d51060bb..7edd1422a 100644 --- a/src/mtconnect/sink/rest_sink/request.hpp +++ b/src/mtconnect/sink/rest_sink/request.hpp @@ -38,16 +38,17 @@ namespace mtconnect::sink::rest_sink { Request() = default; Request(const Request &request) = default; - boost::beast::http::verb m_verb; ///< GET, PUT, POST, or DELETE - std::string m_body; ///< The body of the request - std::string m_accepts; ///< The accepts header - std::string m_acceptsEncoding; ///< Encodings that can be returned - std::string m_contentType; ///< The content type for the body - std::string m_path; ///< The URI for the request - std::string m_foreignIp; ///< The requestors IP Address - uint16_t m_foreignPort; ///< The requestors Port - QueryMap m_query; ///< The parsed query parameters - ParameterMap m_parameters; ///< The parsed path parameters + boost::beast::http::verb m_verb; ///< GET, PUT, POST, or DELETE + std::string m_body; ///< The body of the request + std::string m_accepts; ///< The accepts header + std::string m_acceptsEncoding; ///< Encodings that can be returned + std::string m_contentType; ///< The content type for the body + std::string m_path; ///< The URI for the request + std::string m_foreignIp; ///< The requestors IP Address + uint16_t m_foreignPort; ///< The requestors Port + QueryMap m_query; ///< The parsed query parameters + ParameterMap m_parameters; ///< The parsed path parameters + std::optional m_request; ///< The request type for error reporting /// @name Websocket related properties ///@{ diff --git a/src/mtconnect/sink/rest_sink/rest_service.cpp b/src/mtconnect/sink/rest_sink/rest_service.cpp index 5722a4b4f..779508eeb 100644 --- a/src/mtconnect/sink/rest_sink/rest_service.cpp +++ b/src/mtconnect/sink/rest_sink/rest_service.cpp @@ -113,10 +113,10 @@ namespace mtconnect { "Time in ms between publishing a empty document when no data has changed"}, {"id", PATH, "webservice request id"}}); - createProbeRoutings(); createCurrentRoutings(); createSampleRoutings(); createAssetRoutings(); + createProbeRoutings(); createPutObservationRoutings(); createFileRoutings(); m_server->addCommands(); @@ -469,12 +469,14 @@ namespace mtconnect { using namespace rest_sink; // Probe auto handler = [&](SessionPtr session, const RequestPtr request) -> bool { + request->m_request = "MTConnectDevices"; + auto device = request->parameter("device"); auto pretty = *request->parameter("pretty"); auto deviceType = request->parameter("deviceType"); auto format = request->parameter("format"); auto printer = getPrinter(request->m_accepts, format); - + if (device && !ends_with(request->m_path, string("probe")) && m_sinkContract->findDeviceByUUIDorName(*device) == nullptr) return false; @@ -529,6 +531,8 @@ namespace mtconnect { auto pretty = request->parameter("pretty").value_or(false); auto format = request->parameter("format"); auto printer = getPrinter(request->m_accepts, format); + + request->m_request = "MTConnectAssets"; respond(session, assetRequest(printer, count, removed, request->parameter("type"), @@ -539,6 +543,7 @@ namespace mtconnect { auto idHandler = [&](SessionPtr session, RequestPtr request) -> bool { auto asset = request->parameter("assetIds"); + request->m_request = "MTConnectAssets"; if (asset) { @@ -679,6 +684,8 @@ namespace mtconnect { { using namespace rest_sink; auto handler = [&](SessionPtr session, RequestPtr request) -> bool { + request->m_request = "MTConnectStreams"; + auto interval = request->parameter("interval"); if (interval) { @@ -725,6 +732,8 @@ namespace mtconnect { { using namespace rest_sink; auto handler = [&](SessionPtr session, RequestPtr request) -> bool { + request->m_request = "MTConnectStreams"; + auto interval = request->parameter("interval"); if (interval) { @@ -1101,6 +1110,8 @@ namespace mtconnect { << ": Error processing request: " << re.what(); if (asyncResponse->m_session) { + if (asyncResponse->getRequestId()) + re.setRequestId(*asyncResponse->getRequestId()); writeErrorResponse(asyncResponse->m_session, re); asyncResponse->m_session->close(); } @@ -1232,6 +1243,8 @@ namespace mtconnect { << ": Error processing request: " << re.what(); if (asyncResponse->m_session) { + if (asyncResponse->getRequestId()) + re.setRequestId(*asyncResponse->getRequestId()); writeErrorResponse(asyncResponse->m_session, re); asyncResponse->m_session->close(); } diff --git a/src/mtconnect/sink/rest_sink/rest_service.hpp b/src/mtconnect/sink/rest_sink/rest_service.hpp index 887aeb110..8a917b8cb 100644 --- a/src/mtconnect/sink/rest_sink/rest_service.hpp +++ b/src/mtconnect/sink/rest_sink/rest_service.hpp @@ -308,7 +308,7 @@ namespace mtconnect { auto body = prnt->printErrors(m_instanceId, m_sinkContract->getCircularBuffer().getBufferSize(), m_sinkContract->getCircularBuffer().getSequence(), - error.getErrors(), false, error.getRequest()); + error.getErrors(), false, error.getRequestId()); ResponsePtr resp = std::make_unique(error.getStatus(), body, prnt->mimeType()); diff --git a/src/mtconnect/sink/rest_sink/server.hpp b/src/mtconnect/sink/rest_sink/server.hpp index 8b5ee4d29..e1da8e1cb 100644 --- a/src/mtconnect/sink/rest_sink/server.hpp +++ b/src/mtconnect/sink/rest_sink/server.hpp @@ -198,9 +198,11 @@ namespace mtconnect::sink::rest_sink { { LOG(error) << session->getRemote().address() << ": Error processing request: " << request->m_path; + if (request->m_request) + re.setRequest(*request->m_request); re.setUri(request->getUri()); if (request->m_requestId) - re.setRequest(*request->m_requestId); + re.setRequestId(*request->m_requestId); m_errorFunction(session, re); } catch (std::logic_error &le) diff --git a/test_package/agent_asset_test.cpp b/test_package/agent_asset_test.cpp index 07bf35843..e28b50ac1 100644 --- a/test_package/agent_asset_test.cpp +++ b/test_package/agent_asset_test.cpp @@ -127,30 +127,30 @@ TEST_F(AgentAssetTest, should_store_assets_in_buffer) TEST_F(AgentAssetTest, should_store_assets_in_buffer_and_generate_asset_added) { auto agent = m_agentTestHelper->createAgent("/samples/test_config.xml", 8, 4, "2.6", 4, true); - + auto rest = m_agentTestHelper->getRestService(); ASSERT_TRUE(rest->getServer()->arePutsAllowed()); string body = "TEST"; QueryMap queries; - + queries["type"] = "Part"; queries["device"] = "LinuxCNC"; - + ASSERT_EQ((unsigned int)4, agent->getAssetStorage()->getMaxAssets()); ASSERT_EQ((unsigned int)0, agent->getAssetStorage()->getCount()); - + { PARSE_XML_RESPONSE_PUT("/asset/123", body, queries); ASSERT_EQ((unsigned int)1, agent->getAssetStorage()->getCount()); } - + { PARSE_XML_RESPONSE("/asset/123"); ASSERT_XML_PATH_EQUAL(doc, "//m:Header@assetCount", "1"); ASSERT_XML_PATH_EQUAL(doc, "//m:Header@assetBufferSize", "4"); ASSERT_XML_PATH_EQUAL(doc, "//m:Part", "TEST"); } - + // The device should generate an asset added event as well. { PARSE_XML_RESPONSE("/LinuxCNC/current"); @@ -159,7 +159,6 @@ TEST_F(AgentAssetTest, should_store_assets_in_buffer_and_generate_asset_added) } } - TEST_F(AgentAssetTest, should_handle_asset_buffer_and_buffer_limits) { auto agent = m_agentTestHelper->createAgent("/samples/test_config.xml", 8, 4, "1.3", 4, true); @@ -700,13 +699,13 @@ TEST_F(AgentAssetTest, should_remove_changed_observation_asset_in_2_6) addAdapter(); auto agent = m_agentTestHelper->getAgent(); const auto &storage = agent->getAssetStorage(); - + ASSERT_EQ((unsigned int)4, storage->getMaxAssets()); - + m_agentTestHelper->m_adapter->processData( - "2021-02-01T12:00:00Z|@ASSET@|P1|Part|TEST 1"); + "2021-02-01T12:00:00Z|@ASSET@|P1|Part|TEST 1"); ASSERT_EQ((unsigned int)1, storage->getCount()); - + { PARSE_XML_RESPONSE("/LinuxCNC/current"); ASSERT_XML_PATH_EQUAL(doc, "//m:AssetAdded", "P1"); @@ -715,9 +714,9 @@ TEST_F(AgentAssetTest, should_remove_changed_observation_asset_in_2_6) } m_agentTestHelper->m_adapter->processData( - "2021-02-01T12:00:00Z|@ASSET@|P1|Part|TEST 2"); + "2021-02-01T12:00:00Z|@ASSET@|P1|Part|TEST 2"); ASSERT_EQ((unsigned int)1, storage->getCount()); - + { PARSE_XML_RESPONSE("/LinuxCNC/current"); ASSERT_XML_PATH_EQUAL(doc, "//m:AssetChanged", "P1"); @@ -730,26 +729,26 @@ TEST_F(AgentAssetTest, should_remove_changed_observation_asset_in_2_6) TEST_F(AgentAssetTest, should_remove_added_asset_observation_in_2_6) { m_agentTestHelper->createAgent("/samples/min_config.xml", 8, 4, "2.6", 25); - + addAdapter(); auto agent = m_agentTestHelper->getAgent(); const auto &storage = agent->getAssetStorage(); - + ASSERT_EQ((unsigned int)4, storage->getMaxAssets()); - + m_agentTestHelper->m_adapter->processData( - "2021-02-01T12:00:00Z|@ASSET@|P1|Part|TEST 1"); + "2021-02-01T12:00:00Z|@ASSET@|P1|Part|TEST 1"); ASSERT_EQ((unsigned int)1, storage->getCount()); - + { PARSE_XML_RESPONSE("/LinuxCNC/current"); ASSERT_XML_PATH_EQUAL(doc, "//m:AssetAdded", "P1"); ASSERT_XML_PATH_EQUAL(doc, "//m:AssetAdded@assetType", "Part"); } - + m_agentTestHelper->m_adapter->processData("2021-02-01T12:00:00Z|@REMOVE_ASSET@|P1"); ASSERT_EQ((unsigned int)1, storage->getCount(false)); - + { PARSE_XML_RESPONSE("/LinuxCNC/current"); ASSERT_XML_PATH_EQUAL(doc, "//m:AssetRemoved", "P1"); @@ -759,8 +758,6 @@ TEST_F(AgentAssetTest, should_remove_added_asset_observation_in_2_6) } } - - TEST_F(AgentAssetTest, should_remove_asset_using_http_delete) { auto agent = m_agentTestHelper->createAgent("/samples/test_config.xml", 8, 4, "1.3", 4, true); @@ -805,7 +802,7 @@ TEST_F(AgentAssetTest, in_2_6_asset_changed_removed_and_added_should_be_defaulte { auto agent = m_agentTestHelper->createAgent("/samples/test_config.xml", 8, 4, "2.6", 4, true); addAdapter(); - + { PARSE_XML_RESPONSE("/LinuxCNC/current"); ASSERT_XML_PATH_EQUAL(doc, "//m:AssetChanged", "UNAVAILABLE"); @@ -1037,14 +1034,14 @@ TEST_F(AgentAssetTest, update_asset_count_data_item_v2_0) TEST_F(AgentAssetTest, asset_count_should_not_occur_in_header_post_20) { auto agent = m_agentTestHelper->createAgent("/samples/test_config.xml", 8, 4, "2.0", 4, true); - + string body = "TEST 1"; QueryMap queries; const auto &storage = agent->getAssetStorage(); - + queries["device"] = "LinuxCNC"; queries["type"] = "Part"; - + { PARSE_XML_RESPONSE_PUT("/asset", body, queries); ASSERT_EQ((unsigned int)1, storage->getCount()); @@ -1053,7 +1050,7 @@ TEST_F(AgentAssetTest, asset_count_should_not_occur_in_header_post_20) PARSE_XML_RESPONSE_PUT("/asset/P2", body, queries); ASSERT_EQ((unsigned int)2, storage->getCount()); } - + { PARSE_XML_RESPONSE("/probe"); ASSERT_XML_PATH_COUNT(doc, "//m:Header/*", 0); @@ -1063,38 +1060,38 @@ TEST_F(AgentAssetTest, asset_count_should_not_occur_in_header_post_20) TEST_F(AgentAssetTest, asset_count_should_track_asset_additions_by_type) { auto agent = m_agentTestHelper->createAgent("/samples/test_config.xml", 8, 4, "2.0", 4, true); - + string body1 = "TEST 1"; QueryMap queries; const auto &storage = agent->getAssetStorage(); - + queries["device"] = "LinuxCNC"; queries["type"] = "Part"; - + { PARSE_XML_RESPONSE_PUT("/asset", body1, queries); ASSERT_EQ(1u, storage->getCount()); } - + { PARSE_XML_RESPONSE("/LinuxCNC/current"); ASSERT_XML_PATH_EQUAL(doc, "//m:AssetCountDataSet/m:Entry[@key='Part']", "1"); } - + string body2 = "TEST 2"; queries["type"] = "PartThing"; - + { PARSE_XML_RESPONSE_PUT("/asset", body2, queries); ASSERT_EQ(2u, storage->getCount()); } - + { PARSE_XML_RESPONSE("/LinuxCNC/current"); ASSERT_XML_PATH_EQUAL(doc, "//m:AssetCountDataSet/m:Entry[@key='Part']", "1"); ASSERT_XML_PATH_EQUAL(doc, "//m:AssetCountDataSet/m:Entry[@key='PartThing']", "1"); } - + { PARSE_XML_RESPONSE_PUT("/asset/P3", body2, queries); ASSERT_EQ(3u, storage->getCount()); @@ -1104,9 +1101,9 @@ TEST_F(AgentAssetTest, asset_count_should_track_asset_additions_by_type) ASSERT_XML_PATH_EQUAL(doc, "//m:AssetCountDataSet/m:Entry[@key='Part']", "1"); ASSERT_XML_PATH_EQUAL(doc, "//m:AssetCountDataSet/m:Entry[@key='PartThing']", "2"); } - + body2 = "TEST 2"; - + { PARSE_XML_RESPONSE_PUT("/asset/P3", body2, queries); ASSERT_EQ(2u, storage->getCount()); @@ -1121,11 +1118,11 @@ TEST_F(AgentAssetTest, asset_count_should_track_asset_additions_by_type) TEST_F(AgentAssetTest, asset_should_also_work_using_post_with_assets) { auto agent = m_agentTestHelper->createAgent("/samples/test_config.xml", 8, 4, "2.0", 4, true); - + string body = "TEST 1"; QueryMap queries; const auto &storage = agent->getAssetStorage(); - + { PARSE_XML_RESPONSE_PUT("/assets", body, queries); ASSERT_EQ((unsigned int)1, storage->getCount()); @@ -1135,4 +1132,3 @@ TEST_F(AgentAssetTest, asset_should_also_work_using_post_with_assets) ASSERT_EQ((unsigned int)2, storage->getCount()); } } - diff --git a/test_package/agent_test.cpp b/test_package/agent_test.cpp index 5966404e8..4bf95ad9b 100644 --- a/test_package/agent_test.cpp +++ b/test_package/agent_test.cpp @@ -165,6 +165,7 @@ TEST_F(AgentTest, should_return_2_6_error_for_unknown_device) string message = (string) "Could not find the device 'LinuxCN'"; ASSERT_XML_PATH_EQUAL(doc, "//m:NoDevice@errorCode", "NO_DEVICE"); ASSERT_XML_PATH_EQUAL(doc, "//m:NoDevice/m:ErrorMessage", message.c_str()); + ASSERT_XML_PATH_EQUAL(doc, "//m:NoDevice/m:Request", "MTConnectDevices"); ASSERT_XML_PATH_EQUAL(doc, "//m:NoDevice/m:URI", "/LinuxCN/probe"); ASSERT_EQ(status::not_found, m_agentTestHelper->session()->m_code); } @@ -492,6 +493,7 @@ TEST_F(AgentTest, should_report_2_6_out_of_range_for_current_at) PARSE_XML_RESPONSE_QUERY("/current", query); ASSERT_XML_PATH_EQUAL(doc, "//m:OutOfRange@errorCode", "OUT_OF_RANGE"); ASSERT_XML_PATH_EQUAL(doc, "//m:OutOfRange/m:ErrorMessage", line); + ASSERT_XML_PATH_EQUAL(doc, "//m:OutOfRange/m:Request", "MTConnectStreams"); ASSERT_XML_PATH_EQUAL(doc, "//m:OutOfRange/m:URI", ("/current?at=" + s).c_str()); ASSERT_XML_PATH_EQUAL(doc, "//m:OutOfRange/m:QueryParameter@name", "at"); ASSERT_XML_PATH_EQUAL(doc, "//m:OutOfRange/m:QueryParameter/m:Value", s.c_str()); @@ -507,6 +509,7 @@ TEST_F(AgentTest, should_report_2_6_out_of_range_for_current_at) PARSE_XML_RESPONSE_QUERY("/current", query); ASSERT_XML_PATH_EQUAL(doc, "//m:OutOfRange@errorCode", "OUT_OF_RANGE"); ASSERT_XML_PATH_EQUAL(doc, "//m:OutOfRange/m:ErrorMessage", line); + ASSERT_XML_PATH_EQUAL(doc, "//m:OutOfRange/m:Request", "MTConnectStreams"); ASSERT_XML_PATH_EQUAL(doc, "//m:OutOfRange/m:URI", "/current?at=0"); ASSERT_XML_PATH_EQUAL(doc, "//m:OutOfRange/m:QueryParameter@name", "at"); ASSERT_XML_PATH_EQUAL(doc, "//m:OutOfRange/m:QueryParameter/m:Value", "0"); @@ -685,6 +688,7 @@ TEST_F(AgentTest, should_report_a_2_6_error_when_the_count_is_out_of_range) ASSERT_XML_PATH_EQUAL(doc, "//m:OutOfRange@errorCode", "OUT_OF_RANGE"); string value("'count' must be greater than "); value += to_string(-size); + ASSERT_XML_PATH_EQUAL(doc, "//m:OutOfRange/m:Request", "MTConnectStreams"); ASSERT_XML_PATH_EQUAL(doc, "//m:OutOfRange/m:ErrorMessage", value.c_str()); ASSERT_XML_PATH_EQUAL(doc, "//m:OutOfRange/m:URI", "/sample?count=-500"); ASSERT_XML_PATH_EQUAL(doc, "//m:OutOfRange/m:QueryParameter@name", "count"); @@ -2109,7 +2113,6 @@ TEST_F(AgentTest, should_handle_uuid_change) } } - /// @name Streaming Tests /// Tests that validate HTTP long poll behavior of the agent diff --git a/test_package/test_utilities.hpp b/test_package/test_utilities.hpp index 921f2cfd4..c40b39f42 100644 --- a/test_package/test_utilities.hpp +++ b/test_package/test_utilities.hpp @@ -111,8 +111,7 @@ inline void fillAttribute(std::string &xmlString, const std::string &attribute, #define ASSERT_XML_PATH_COUNT(doc, path, expected) \ xpathTestCount(doc, path, expected, __FILE__, __LINE__) -#define DUMP_XML(doc) \ - dumpXml(doc) +#define DUMP_XML(doc) dumpXml(doc) inline void failIf(bool condition, const std::string &message, const std::string &file, int line) { @@ -139,10 +138,10 @@ inline std::string dumpXml(xmlDocPtr doc) int size; xmlChar *memory; xmlDocDumpFormatMemory(doc, &memory, &size, 1); - - std::string s((const char *) memory, size); + + std::string s((const char *)memory, size); xmlFree(memory); - + return s; } From a4fb41a8d9a07f1fd21de0a994a180cedafb4bbe Mon Sep 17 00:00:00 2001 From: Will Sobel Date: Thu, 28 Aug 2025 19:15:33 +0200 Subject: [PATCH 2/5] Improved error handling for websockets --- src/mtconnect/sink/rest_sink/rest_service.hpp | 4 +- src/mtconnect/sink/rest_sink/routing.hpp | 175 ++++++++++++-- src/mtconnect/sink/rest_sink/server.hpp | 39 ++- .../sink/rest_sink/websocket_session.hpp | 227 ++++++++++++------ 4 files changed, 330 insertions(+), 115 deletions(-) diff --git a/src/mtconnect/sink/rest_sink/rest_service.hpp b/src/mtconnect/sink/rest_sink/rest_service.hpp index 8a917b8cb..b96dbaccc 100644 --- a/src/mtconnect/sink/rest_sink/rest_service.hpp +++ b/src/mtconnect/sink/rest_sink/rest_service.hpp @@ -311,7 +311,9 @@ namespace mtconnect { error.getErrors(), false, error.getRequestId()); ResponsePtr resp = std::make_unique(error.getStatus(), body, prnt->mimeType()); - + if (error.getRequestId()) + resp->m_requestId = error.getRequestId(); + session->writeFailureResponse(std::move(resp)); } } diff --git a/src/mtconnect/sink/rest_sink/routing.hpp b/src/mtconnect/sink/rest_sink/routing.hpp index 2aea117b7..075b9f071 100644 --- a/src/mtconnect/sink/rest_sink/routing.hpp +++ b/src/mtconnect/sink/rest_sink/routing.hpp @@ -157,17 +157,35 @@ namespace mtconnect::sink::rest_sink { const ParameterList &getPathParameters() const { return m_pathParameters; } /// @brief get the unordered set of query parameters const QuerySet &getQueryParameters() const { return m_queryParameters; } - - /// @brief match the session's request against the this routing + + /// @brief run the session's request if this routing matches /// /// Call the associated lambda when matched /// /// @param[in] session the session making the request to pass to the Routing if matched /// @param[in,out] request the incoming request with a verb and a path /// @return `true` if the request was matched + bool run(SessionPtr session, RequestPtr request) + { + if (matches(session, request) && validateRequest(session, request)) + return m_function(session, request); + else + return false; + } + + /// @brief check if the routing matches the request + /// + /// @param[in] session the session making the request to pass to the Routing if matched + /// @param[in,out] request the incoming request with a verb and a path + /// @return `true` if the request was matched + /// @throws `RestError` if there are any parameter errors bool matches(SessionPtr session, RequestPtr request) { - if (!request->m_command) + if (request->m_command) + { + return m_command == *request->m_command && m_verb == request->m_verb; + } + else { request->m_parameters.clear(); std::smatch m; @@ -184,46 +202,93 @@ namespace mtconnect::sink::rest_sink { s++; } } + + entity::EntityList errors; + for (auto &p : m_queryParameters) + { + auto q = request->m_query.find(p.m_name); + if (q != request->m_query.end()) + { + try + { + auto v = convertValue(q->second, p.m_type); + request->m_parameters.emplace(make_pair(p.m_name, v)); + } + catch (ParameterError &e) + { + std::string msg = std::string("query parameter '") + p.m_name + "': " + e.what(); + + LOG(warning) << "Parameter error: " << msg; + auto error = InvalidParameterValue::make(p.m_name, q->second, p.getTypeName(), + p.getTypeFormat(), msg); + errors.emplace_back(error); + } + } + else if (!std::holds_alternative(p.m_default)) + { + request->m_parameters.emplace(make_pair(p.m_name, p.m_default)); + } + } + + if (!errors.empty()) + throw RestError(errors, request->m_accepts); + + return true; } else { return false; } } - + } + + /// @brief Validate the request parameters without matching the path + /// @param[in] session the session making the request to pass to the Routing if matched + /// @param[in,out] request the incoming request with a verb and a path + /// @return `true` if the request was matched + /// @throws `RestError` if there are any parameter errors + bool validateRequest(SessionPtr session, RequestPtr request) + { entity::EntityList errors; - for (auto &p : m_queryParameters) + /// Just validate the types of the parameters + for (auto &p : m_pathParameters) { - auto q = request->m_query.find(p.m_name); - if (q != request->m_query.end()) + auto it = request->m_parameters.find(p.m_name); + if (it != request->m_parameters.end()) { - try - { - auto v = convertValue(q->second, p.m_type); - request->m_parameters.emplace(make_pair(p.m_name, v)); - } - catch (ParameterError &e) + if (!validateValueType(p.m_type, it->second)) { - std::string msg = std::string("query parameter '") + p.m_name + "': " + e.what(); - + std::string msg = std::string("path parameter '") + p.m_name + "': invalid type, expected " + p.getTypeName(); LOG(warning) << "Parameter error: " << msg; - auto error = InvalidParameterValue::make(p.m_name, q->second, p.getTypeName(), + auto error = InvalidParameterValue::make(p.m_name, toString(it->second), p.getTypeName(), p.getTypeFormat(), msg); errors.emplace_back(error); } } - else if (!std::holds_alternative(p.m_default)) + } + + for (auto &p : m_queryParameters) + { + auto it = request->m_parameters.find(p.m_name); + if (it != request->m_parameters.end()) { - request->m_parameters.emplace(make_pair(p.m_name, p.m_default)); + if (!validateValueType(p.m_type, it->second)) + { + std::string msg = std::string("query parameter '") + p.m_name + "': invalid type, expected " + p.getTypeName(); + LOG(warning) << "Parameter error: " << msg; + auto error = InvalidParameterValue::make(p.m_name, toString(it->second), p.getTypeName(), + p.getTypeFormat(), msg); + errors.emplace_back(error); + } } } if (!errors.empty()) throw RestError(errors, request->m_accepts); - else - return m_function(session, request); + + return true; } - + /// @brief check if this is related to a swagger API /// @returns `true` if related to swagger auto isSwagger() const { return m_swagger; } @@ -372,6 +437,74 @@ namespace mtconnect::sink::rest_sink { return ParameterValue(); } + + bool validateValueType(ParameterType t, ParameterValue &value) + { + switch (t) + { + case STRING: + return std::holds_alternative(value); + + case NONE: + return std::holds_alternative(value); + + case DOUBLE: + if (std::holds_alternative(value)) + value = double(std::get(value)); + else if (std::holds_alternative(value)) + value = double(std::get(value)); + + return std::holds_alternative(value); + + case INTEGER: + if (std::holds_alternative(value)) + { + auto v = std::get(value); + if (v <= uint64_t(std::numeric_limits::max())) + value = int32_t(v); + } + else if (std::holds_alternative(value)) + { + auto v = std::get(value); + if (v >= double(std::numeric_limits::min()) && + v <= double(std::numeric_limits::max())) + value = int32_t(v); + } + return std::holds_alternative(value); + + case UNSIGNED_INTEGER: + if (std::holds_alternative(value)) + { + auto v = std::get(value); + if (v >= 0) + value = uint64_t(v); + } + else if (std::holds_alternative(value)) + { + auto v = std::get(value); + if (v >= 0 && v <= double(std::numeric_limits::max())) + value = uint64_t(v); + } + return std::holds_alternative(value); + + case BOOL: + return std::holds_alternative(value); + } + return false; + } + + /// @brief Helper to convert a ParameterValue to a string + std::string toString(const ParameterValue &v) + { + using namespace std::string_literals; + return std::visit(overloaded {[](const std::monostate &) { return "none"s; }, + [](const std::string &s) { return s; }, + [](int32_t i) { return std::to_string(i); }, + [](uint64_t i) { return std::to_string(i); }, + [](double d) { return std::to_string(d); }, + [](bool b) { return b ? "true"s : "false"s; }}, + v); + } protected: boost::beast::http::verb m_verb; diff --git a/src/mtconnect/sink/rest_sink/server.hpp b/src/mtconnect/sink/rest_sink/server.hpp index e1da8e1cb..ee3381acc 100644 --- a/src/mtconnect/sink/rest_sink/server.hpp +++ b/src/mtconnect/sink/rest_sink/server.hpp @@ -160,39 +160,36 @@ namespace mtconnect::sink::rest_sink { { try { + Routing * routing = nullptr; if (request->m_command) { auto route = m_commands.find(*request->m_command); if (route != m_commands.end()) - { - if (route->second->matches(session, request)) - return true; - } - else - { - std::stringstream txt; - txt << session->getRemote().address() - << ": Cannot find handler for command: " << *request->m_command; - session->fail(boost::beast::http::status::not_found, txt.str()); - } + routing = route->second; } else { for (auto &r : m_routings) { if (r.matches(session, request)) - return true; + { + routing = &r; + break; + } } - - std::stringstream txt; - txt << session->getRemote().address() << ": Cannot find handler for: " << request->m_verb - << " " << request->m_path; - auto error = Error::make(Error::ErrorCode::INVALID_URI, txt.str()); - RestError re(error, request->m_accepts, status::not_found, std::nullopt, - request->m_requestId); - re.setUri(request->getUri()); - m_errorFunction(session, re); } + + if (routing) + return routing->run(session, request); + + std::stringstream txt; + txt << session->getRemote().address() << ": Cannot find handler for: " << request->m_verb + << " " << request->m_path; + auto error = Error::make(Error::ErrorCode::INVALID_URI, txt.str()); + RestError re(error, request->m_accepts, status::not_found, std::nullopt, + request->m_requestId); + re.setUri(request->getUri()); + m_errorFunction(session, re); } catch (RestError &re) { diff --git a/src/mtconnect/sink/rest_sink/websocket_session.hpp b/src/mtconnect/sink/rest_sink/websocket_session.hpp index 94e922b9c..8f64f05df 100644 --- a/src/mtconnect/sink/rest_sink/websocket_session.hpp +++ b/src/mtconnect/sink/rest_sink/websocket_session.hpp @@ -309,63 +309,54 @@ namespace mtconnect::sink::rest_sink { } } } - - void onRead(beast::error_code ec, std::size_t len) + + RequestPtr parseRequest(const std::string &buffer) { - NAMED_SCOPE("PlainWebsocketSession::onRead"); - - if (ec) - return fail(boost::beast::http::status::internal_server_error, "shutdown", ec); - using namespace rapidjson; using namespace std; - if (len == 0) - { - LOG(debug) << "Empty message received"; - return; - } - - // Parse the buffer as a JSON request with parameters matching - // REST API - derived().stream().text(derived().stream().got_text()); - auto buffer = beast::buffers_to_string(m_buffer.data()); - m_buffer.consume(m_buffer.size()); - - LOG(debug) << "Received :" << buffer.c_str(); - Document doc; - doc.Parse(buffer.c_str(), len); - + doc.Parse(buffer.c_str()); + + RequestPtr request; + if (doc.HasParseError()) { - LOG(warning) << "Websocket Read Error(offset (" << doc.GetErrorOffset() - << "): " << GetParseError_En(doc.GetParseError()); + stringstream err; + err << "Websocket Read Error(offset (" << doc.GetErrorOffset() + << "): " << GetParseError_En(doc.GetParseError()); + LOG(warning) << err.str(); LOG(warning) << " " << buffer; + auto error = Error::make(Error::ErrorCode::INVALID_REQUEST, err.str()); + throw RestError(error, m_request->m_accepts, rest_sink::status::bad_request, + std::nullopt, "ERROR"); } if (!doc.IsObject()) { LOG(warning) << "Websocket Read Error: JSON message does not have a top level object"; LOG(warning) << " " << buffer; + auto error = Error::make(Error::ErrorCode::INVALID_REQUEST, "JSON message does not have a top level object"); + throw RestError(error, m_request->m_accepts, rest_sink::status::bad_request, + std::nullopt, "ERROR"); } else { // Extract the parameters from the json doc to map them to the REST // protocol parameters - auto request = make_unique(*m_request); - + request = make_unique(*m_request); + request->m_verb = beast::http::verb::get; request->m_parameters.clear(); #ifdef GetObject #define __GOSave__ GetObject #undef GetObject #endif - + const auto &object = doc.GetObject(); #ifdef __GOSave__ #define GetObject __GOSave__ #endif - + for (auto &it : object) { switch (it.value.GetType()) @@ -375,11 +366,11 @@ namespace mtconnect::sink::rest_sink { break; case rapidjson::kFalseType: request->m_parameters.emplace( - make_pair(string(it.name.GetString()), ParameterValue(false))); + make_pair(string(it.name.GetString()), ParameterValue(false))); break; case rapidjson::kTrueType: request->m_parameters.emplace( - make_pair(string(it.name.GetString()), ParameterValue(true))); + make_pair(string(it.name.GetString()), ParameterValue(true))); break; case rapidjson::kObjectType: break; @@ -387,69 +378,161 @@ namespace mtconnect::sink::rest_sink { break; case rapidjson::kStringType: request->m_parameters.emplace( - make_pair(it.name.GetString(), ParameterValue(string(it.value.GetString())))); - + make_pair(it.name.GetString(), ParameterValue(string(it.value.GetString())))); + break; case rapidjson::kNumberType: if (it.value.IsInt()) request->m_parameters.emplace( - make_pair(it.name.GetString(), ParameterValue(it.value.GetInt()))); + make_pair(it.name.GetString(), ParameterValue(it.value.GetInt()))); else if (it.value.IsUint()) request->m_parameters.emplace( - make_pair(it.name.GetString(), ParameterValue(uint64_t(it.value.GetUint())))); + make_pair(it.name.GetString(), ParameterValue(uint64_t(it.value.GetUint())))); else if (it.value.IsInt64()) request->m_parameters.emplace( - make_pair(it.name.GetString(), ParameterValue(uint64_t(it.value.GetInt64())))); + make_pair(it.name.GetString(), ParameterValue(uint64_t(it.value.GetInt64())))); else if (it.value.IsUint64()) request->m_parameters.emplace( - make_pair(it.name.GetString(), ParameterValue(it.value.GetUint64()))); + make_pair(it.name.GetString(), ParameterValue(it.value.GetUint64()))); else if (it.value.IsDouble()) request->m_parameters.emplace( - make_pair(it.name.GetString(), ParameterValue(double(it.value.GetDouble())))); - + make_pair(it.name.GetString(), ParameterValue(double(it.value.GetDouble())))); + break; } } + } + + return request; + } + + bool dispatchRequest(RequestPtr &&request) + { + using namespace std; - if (request->m_parameters.count("request") > 0) - { - request->m_command = get(request->m_parameters["request"]); - request->m_parameters.erase("request"); - } - if (request->m_parameters.count("id") > 0) - { - auto &v = request->m_parameters["id"]; - string id = visit(overloaded {[](monostate m) { return ""s; }, - [](auto v) { return boost::lexical_cast(v); }}, - v); - request->m_requestId = id; - request->m_parameters.erase("id"); - } + if (request->m_parameters.count("id") > 0) + { + auto &v = request->m_parameters["id"]; + string id = visit(overloaded {[](monostate m) { return ""s; }, + [](auto v) { return boost::lexical_cast(v); }}, + v); + request->m_requestId = id; + request->m_parameters.erase("id"); + } + else + { + auto error = InvalidParameterValue::make("id", "", "string", "string", + "No id given"); + throw RestError(error, request->m_accepts, rest_sink::status::bad_request, + std::nullopt, "ERROR"); + } - auto &id = *(request->m_requestId); - auto res = m_requests.emplace(id, id); - if (!res.second) + auto &id = *(request->m_requestId); + auto res = m_requests.emplace(id, id); + if (!res.second) + { + LOG(error) << "Duplicate request id: " << id; + auto error = InvalidParameterValue::make("id", *request->m_requestId, "string", "string", + "Duplicate id given"); + throw RestError(error, request->m_accepts, rest_sink::status::bad_request, + std::nullopt, "ERROR"); + } + + if (request->m_parameters.count("request") > 0) + { + request->m_command = get(request->m_parameters["request"]); + request->m_parameters.erase("request"); + } + else + { + auto error = InvalidParameterValue::make("request", "", "string", "string", + "No request given"); + throw RestError(error, request->m_accepts, rest_sink::status::bad_request, + std::nullopt, id); + } + + // Check parameters for command + LOG(debug) << "Received request id: " << id; + + res.first->second.m_request = std::move(request); + try + { + return m_dispatch(derived().shared_ptr(), res.first->second.m_request); + } + + catch (RestError &re) + { + re.setRequestId(id); + throw re; + } + + return false; + } + + void onRead(beast::error_code ec, std::size_t len) + { + NAMED_SCOPE("PlainWebsocketSession::onRead"); + + if (ec) + return fail(boost::beast::http::status::internal_server_error, "shutdown", ec); + + if (len == 0) + { + LOG(debug) << "Empty message received"; + return; + } + + // Parse the buffer as a JSON request with parameters matching + // REST API + derived().stream().text(derived().stream().got_text()); + auto buffer = beast::buffers_to_string(m_buffer.data()); + m_buffer.consume(m_buffer.size()); + + LOG(debug) << "Received :" << buffer; + + try + { + auto request = parseRequest(buffer); + if (!request || !dispatchRequest(std::move(request))) { - LOG(error) << "Duplicate request id: " << id; - boost::system::error_code ec; - fail(status::bad_request, "Duplicate request Id", ec); + std::stringstream txt; + txt << getRemote().address() << ": Dispatch failed for: " << buffer; + LOG(error) << txt.str(); } - else + } + + catch (RestError &re) + { + auto id = re.getRequestId(); + if (!id) { - LOG(debug) << "Received request id: " << id; - - res.first->second.m_request = std::move(request); - if (!m_dispatch(derived().shared_ptr(), res.first->second.m_request)) - { - ostringstream txt; - txt << "Failed to find handler for " << buffer; - LOG(error) << txt.str(); - boost::system::error_code ec; - fail(status::bad_request, "Duplicate request Id", ec); - } + id = "ERROR"; + re.setRequestId(*id); } + + auto res = m_requests.find(*id); + if (res == m_requests.end()) + m_requests.emplace(*id, *id); + + m_errorFunction(derived().shared_ptr(), re); } - + + catch (std::logic_error &le) + { + std::stringstream txt; + txt << getRemote().address() << ": Logic Error: " << le.what(); + LOG(error) << txt.str(); + fail(boost::beast::http::status::not_found, txt.str()); + } + + catch (...) + { + std::stringstream txt; + txt << getRemote().address() << ": Unknown Error thrown"; + LOG(error) << txt.str(); + fail(boost::beast::http::status::not_found, txt.str()); + } + derived().stream().async_read( m_buffer, beast::bind_front_handler(&WebsocketSession::onRead, derived().shared_ptr())); } From 4ca6ea6f61f4f4f70edaeae0ffc39f1575a60eab Mon Sep 17 00:00:00 2001 From: Will Sobel Date: Thu, 28 Aug 2025 19:56:12 +0200 Subject: [PATCH 3/5] Added some more websocket error tests --- src/mtconnect/sink/rest_sink/routing.hpp | 4 +- .../sink/rest_sink/websocket_session.hpp | 2 +- test_package/websockets_test.cpp | 155 +++++++++++++++++- 3 files changed, 156 insertions(+), 5 deletions(-) diff --git a/src/mtconnect/sink/rest_sink/routing.hpp b/src/mtconnect/sink/rest_sink/routing.hpp index 075b9f071..c7b9972bb 100644 --- a/src/mtconnect/sink/rest_sink/routing.hpp +++ b/src/mtconnect/sink/rest_sink/routing.hpp @@ -258,7 +258,7 @@ namespace mtconnect::sink::rest_sink { { if (!validateValueType(p.m_type, it->second)) { - std::string msg = std::string("path parameter '") + p.m_name + "': invalid type, expected " + p.getTypeName(); + std::string msg = std::string("path parameter '") + p.m_name + "': invalid type, expected " + p.getTypeFormat(); LOG(warning) << "Parameter error: " << msg; auto error = InvalidParameterValue::make(p.m_name, toString(it->second), p.getTypeName(), p.getTypeFormat(), msg); @@ -274,7 +274,7 @@ namespace mtconnect::sink::rest_sink { { if (!validateValueType(p.m_type, it->second)) { - std::string msg = std::string("query parameter '") + p.m_name + "': invalid type, expected " + p.getTypeName(); + std::string msg = std::string("query parameter '") + p.m_name + "': invalid type, expected " + p.getTypeFormat(); LOG(warning) << "Parameter error: " << msg; auto error = InvalidParameterValue::make(p.m_name, toString(it->second), p.getTypeName(), p.getTypeFormat(), msg); diff --git a/src/mtconnect/sink/rest_sink/websocket_session.hpp b/src/mtconnect/sink/rest_sink/websocket_session.hpp index 8f64f05df..10732a402 100644 --- a/src/mtconnect/sink/rest_sink/websocket_session.hpp +++ b/src/mtconnect/sink/rest_sink/websocket_session.hpp @@ -324,7 +324,7 @@ namespace mtconnect::sink::rest_sink { { stringstream err; err << "Websocket Read Error(offset (" << doc.GetErrorOffset() - << "): " << GetParseError_En(doc.GetParseError()); + << ")): " << GetParseError_En(doc.GetParseError()); LOG(warning) << err.str(); LOG(warning) << " " << buffer; auto error = Error::make(Error::ErrorCode::INVALID_REQUEST, err.str()); diff --git a/test_package/websockets_test.cpp b/test_package/websockets_test.cpp index 0ca154420..6ba31bb0e 100644 --- a/test_package/websockets_test.cpp +++ b/test_package/websockets_test.cpp @@ -25,6 +25,7 @@ #include #include #include +#include "test_utilities.hpp" #include #include @@ -168,8 +169,7 @@ class WebsocketsTest : public testing::Test protected: void SetUp() override { - using namespace mtconnect::configuration; - m_server = make_unique(m_context, ConfigOptions {{Port, 0}, {ServerIp, "127.0.0.1"s}}); + createServer({}); } void createServer(const ConfigOptions& options) @@ -178,6 +178,14 @@ class WebsocketsTest : public testing::Test ConfigOptions opts {{Port, 0}, {ServerIp, "127.0.0.1"s}}; opts.merge(ConfigOptions(options)); m_server = make_unique(m_context, opts); + m_server->setErrorFunction([](SessionPtr session, const RestError &error) + { + ResponsePtr resp = std::make_unique(error.getStatus(), error.what(), "plain/text"); + if (error.getRequestId()) + resp->m_requestId = error.getRequestId(); + + session->writeFailureResponse(std::move(resp)); + }); } void start() @@ -244,3 +252,146 @@ TEST_F(WebsocketsTest, should_make_simple_request) ASSERT_TRUE(m_client->m_done); ASSERT_EQ("All Devices for 1", m_client->m_result); } + +TEST_F(WebsocketsTest, should_return_error_when_there_is_no_id) +{ + weak_ptr savedSession; + + auto probe = [&](SessionPtr session, RequestPtr request) -> bool { + savedSession = session; + ResponsePtr resp = make_unique(status::ok); + resp->m_body = "All Devices for "s + *request->m_requestId; + resp->m_requestId = request->m_requestId; + session->writeResponse(std::move(resp), []() { cout << "Written" << endl; }); + return true; + }; + + m_server->addRouting({boost::beast::http::verb::get, "/probe", probe}).command("probe"); + m_server->addCommands(); + + start(); + startClient(); + + asio::spawn(m_context, std::bind(&Client::request, m_client.get(), + "{\"request\":\"probe\"}"s, std::placeholders::_1)); + + m_client->waitFor(2s, [this]() { return m_client->m_done; }); + + ASSERT_TRUE(m_client->m_done); + ASSERT_EQ("InvalidParameterValue: No id given", m_client->m_result); +} + +TEST_F(WebsocketsTest, should_return_error_when_there_is_no_request) +{ + weak_ptr savedSession; + + auto probe = [&](SessionPtr session, RequestPtr request) -> bool { + savedSession = session; + ResponsePtr resp = make_unique(status::ok); + resp->m_body = "All Devices for "s + *request->m_requestId; + resp->m_requestId = request->m_requestId; + session->writeResponse(std::move(resp), []() { cout << "Written" << endl; }); + return true; + }; + + m_server->addRouting({boost::beast::http::verb::get, "/probe", probe}).command("probe"); + m_server->addCommands(); + + start(); + startClient(); + + asio::spawn(m_context, std::bind(&Client::request, m_client.get(), + "{\"id\": 3}"s, std::placeholders::_1)); + + m_client->waitFor(2s, [this]() { return m_client->m_done; }); + + ASSERT_TRUE(m_client->m_done); + ASSERT_EQ("InvalidParameterValue: No request given", m_client->m_result); +} + + +TEST_F(WebsocketsTest, should_return_error_when_a_parameter_is_invalid) +{ + weak_ptr savedSession; + + auto probe = [&](SessionPtr session, RequestPtr request) -> bool { + savedSession = session; + ResponsePtr resp = make_unique(status::ok); + resp->m_body = "All Devices for "s + *request->m_requestId; + resp->m_requestId = request->m_requestId; + session->writeResponse(std::move(resp), []() { cout << "Written" << endl; }); + return true; + }; + + m_server->addRouting({boost::beast::http::verb::get, "/sample?interval={integer}", probe}).command("sample"); + m_server->addCommands(); + + start(); + startClient(); + + asio::spawn(m_context, std::bind(&Client::request, m_client.get(), + "{\"id\": 3, \"request\": \"sample\", \"interval\": 99999999999}"s, std::placeholders::_1)); + + m_client->waitFor(2s, [this]() { return m_client->m_done; }); + + ASSERT_TRUE(m_client->m_done); + ASSERT_EQ("InvalidParameterValue: query parameter 'interval': invalid type, expected int32", m_client->m_result); +} + +TEST_F(WebsocketsTest, should_return_error_when_bad_json_is_sent) +{ + weak_ptr savedSession; + + auto probe = [&](SessionPtr session, RequestPtr request) -> bool { + savedSession = session; + ResponsePtr resp = make_unique(status::ok); + resp->m_body = "All Devices for "s + *request->m_requestId; + resp->m_requestId = request->m_requestId; + session->writeResponse(std::move(resp), []() { cout << "Written" << endl; }); + return true; + }; + + m_server->addRouting({boost::beast::http::verb::get, "/sample?interval={integer}", probe}).command("sample"); + m_server->addCommands(); + + start(); + startClient(); + + asio::spawn(m_context, std::bind(&Client::request, m_client.get(), + "!}}"s, std::placeholders::_1)); + + m_client->waitFor(2s, [this]() { return m_client->m_done; }); + + ASSERT_TRUE(m_client->m_done); + ASSERT_EQ("InvalidRequest: Websocket Read Error(offset (0)): Invalid value.", m_client->m_result); +} + +TEST_F(WebsocketsTest, should_return_multiple_errors_when_parameters_are_invalid) +{ + weak_ptr savedSession; + + auto probe = [&](SessionPtr session, RequestPtr request) -> bool { + savedSession = session; + ResponsePtr resp = make_unique(status::ok); + resp->m_body = "All Devices for "s + *request->m_requestId; + resp->m_requestId = request->m_requestId; + session->writeResponse(std::move(resp), []() { cout << "Written" << endl; }); + return true; + }; + + m_server->addRouting({boost::beast::http::verb::get, "/sample?interval={integer}&to={unsigned_integer}", probe}).command("sample"); + m_server->addCommands(); + + start(); + startClient(); + + asio::spawn(m_context, std::bind(&Client::request, m_client.get(), + R"DOC({"id": 3, "request": "sample", "interval": 99999999999,"to": -1 })DOC", std::placeholders::_1)); + + m_client->waitFor(2s, [this]() { return m_client->m_done; }); + + ASSERT_TRUE(m_client->m_done); + ASSERT_EQ("InvalidParameterValue: query parameter 'interval': invalid type, expected int32, " + "InvalidParameterValue: query parameter 'to': invalid type, expected uint64", m_client->m_result); +} + From e5d29dc20a403d37bbd74f4e6e593603588ae443 Mon Sep 17 00:00:00 2001 From: Will Sobel Date: Fri, 29 Aug 2025 13:15:32 +0200 Subject: [PATCH 4/5] Added better handling of routing matches and errors from dispatching. --- src/mtconnect/sink/rest_sink/parameter.hpp | 14 +++++++ src/mtconnect/sink/rest_sink/request.hpp | 13 +++++- src/mtconnect/sink/rest_sink/routing.hpp | 19 ++------- src/mtconnect/sink/rest_sink/server.hpp | 47 +++++++++++++--------- test_package/routing_test.cpp | 6 +-- test_package/websockets_test.cpp | 39 +++++++++++++++--- 6 files changed, 93 insertions(+), 45 deletions(-) diff --git a/src/mtconnect/sink/rest_sink/parameter.hpp b/src/mtconnect/sink/rest_sink/parameter.hpp index 5677f1b85..efe3c3999 100644 --- a/src/mtconnect/sink/rest_sink/parameter.hpp +++ b/src/mtconnect/sink/rest_sink/parameter.hpp @@ -120,6 +120,20 @@ namespace mtconnect::sink::rest_sink { return "unknown"; } + + /// @brief Helper to convert a ParameterValue to a string + static std::string toString(const ParameterValue &v) + { + using namespace std::string_literals; + return std::visit(overloaded {[](const std::monostate &) { return "none"s; }, + [](const std::string &s) { return s; }, + [](int32_t i) { return std::to_string(i); }, + [](uint64_t i) { return std::to_string(i); }, + [](double d) { return std::to_string(d); }, + [](bool b) { return b ? "true"s : "false"s; }}, + v); + } + std::string m_name; ParameterType m_type {STRING}; diff --git a/src/mtconnect/sink/rest_sink/request.hpp b/src/mtconnect/sink/rest_sink/request.hpp index 7edd1422a..9cf912b68 100644 --- a/src/mtconnect/sink/rest_sink/request.hpp +++ b/src/mtconnect/sink/rest_sink/request.hpp @@ -77,7 +77,18 @@ namespace mtconnect::sink::rest_sink { std::string getUri() const { std::string s; - if (!m_query.empty()) + if (m_command) + { + std::stringstream uri; + uri << m_path << '/' << *m_command << '?'; + for (auto &p : m_parameters) + { + uri << p.first << '=' << Parameter::toString(p.second) << '&'; + } + s = uri.str(); + s.erase(s.length() - 1); + } + else if (!m_query.empty()) { std::stringstream uri; uri << m_path << '?'; diff --git a/src/mtconnect/sink/rest_sink/routing.hpp b/src/mtconnect/sink/rest_sink/routing.hpp index c7b9972bb..932b6743b 100644 --- a/src/mtconnect/sink/rest_sink/routing.hpp +++ b/src/mtconnect/sink/rest_sink/routing.hpp @@ -167,7 +167,7 @@ namespace mtconnect::sink::rest_sink { /// @return `true` if the request was matched bool run(SessionPtr session, RequestPtr request) { - if (matches(session, request) && validateRequest(session, request)) + if (validateRequest(session, request)) return m_function(session, request); else return false; @@ -260,7 +260,7 @@ namespace mtconnect::sink::rest_sink { { std::string msg = std::string("path parameter '") + p.m_name + "': invalid type, expected " + p.getTypeFormat(); LOG(warning) << "Parameter error: " << msg; - auto error = InvalidParameterValue::make(p.m_name, toString(it->second), p.getTypeName(), + auto error = InvalidParameterValue::make(p.m_name, Parameter::toString(it->second), p.getTypeName(), p.getTypeFormat(), msg); errors.emplace_back(error); } @@ -276,7 +276,7 @@ namespace mtconnect::sink::rest_sink { { std::string msg = std::string("query parameter '") + p.m_name + "': invalid type, expected " + p.getTypeFormat(); LOG(warning) << "Parameter error: " << msg; - auto error = InvalidParameterValue::make(p.m_name, toString(it->second), p.getTypeName(), + auto error = InvalidParameterValue::make(p.m_name, Parameter::toString(it->second), p.getTypeName(), p.getTypeFormat(), msg); errors.emplace_back(error); } @@ -493,19 +493,6 @@ namespace mtconnect::sink::rest_sink { return false; } - /// @brief Helper to convert a ParameterValue to a string - std::string toString(const ParameterValue &v) - { - using namespace std::string_literals; - return std::visit(overloaded {[](const std::monostate &) { return "none"s; }, - [](const std::string &s) { return s; }, - [](int32_t i) { return std::to_string(i); }, - [](uint64_t i) { return std::to_string(i); }, - [](double d) { return std::to_string(d); }, - [](bool b) { return b ? "true"s : "false"s; }}, - v); - } - protected: boost::beast::http::verb m_verb; std::regex m_pattern; diff --git a/src/mtconnect/sink/rest_sink/server.hpp b/src/mtconnect/sink/rest_sink/server.hpp index ee3381acc..bffb7acc4 100644 --- a/src/mtconnect/sink/rest_sink/server.hpp +++ b/src/mtconnect/sink/rest_sink/server.hpp @@ -160,44 +160,53 @@ namespace mtconnect::sink::rest_sink { { try { - Routing * routing = nullptr; + auto success = false; + std::string message; if (request->m_command) { auto route = m_commands.find(*request->m_command); if (route != m_commands.end()) - routing = route->second; + success = route->second->run(session, request); + else + message = "Command failed: " + *request->m_command; } else { for (auto &r : m_routings) { - if (r.matches(session, request)) - { - routing = &r; + success = r.matches(session, request) && r.run(session, request); + if (success) break; - } + } + if (!success) + { + std::stringstream txt; + txt << "Cannot find handler for: " << request->m_verb + << " " << request->m_path; + message = txt.str(); } } - if (routing) - return routing->run(session, request); - - std::stringstream txt; - txt << session->getRemote().address() << ": Cannot find handler for: " << request->m_verb - << " " << request->m_path; - auto error = Error::make(Error::ErrorCode::INVALID_URI, txt.str()); - RestError re(error, request->m_accepts, status::not_found, std::nullopt, - request->m_requestId); - re.setUri(request->getUri()); - m_errorFunction(session, re); + if (!success) + { + std::stringstream txt; + txt << session->getRemote().address() << ": " << message; + auto error = Error::make(Error::ErrorCode::INVALID_URI, txt.str()); + RestError re(error, request->m_accepts, status::not_found, std::nullopt, + request->m_requestId); + re.setUri(request->getUri()); + m_errorFunction(session, re); + } } catch (RestError &re) { + auto uri = request->getUri(); + re.setUri(uri); LOG(error) << session->getRemote().address() - << ": Error processing request: " << request->m_path; + << ": Error processing request: " << uri; + if (request->m_request) re.setRequest(*request->m_request); - re.setUri(request->getUri()); if (request->m_requestId) re.setRequestId(*request->m_requestId); m_errorFunction(session, re); diff --git a/test_package/routing_test.cpp b/test_package/routing_test.cpp index 478333fd5..57c06fef1 100644 --- a/test_package/routing_test.cpp +++ b/test_package/routing_test.cpp @@ -279,16 +279,16 @@ TEST_F(RoutingTest, should_throw_a_rest_error_with_multiple_invalid_parameters) } } -TEST_F(RoutingTest, RegexPatterns) +TEST_F(RoutingTest, should_correctly_match_regex_patterns) { Routing r(verb::get, regex("/.+"), m_func); RequestPtr request = make_shared(); request->m_verb = verb::get; request->m_path = "/some random stuff"; - ASSERT_TRUE(r.matches(0, request)); + ASSERT_TRUE(r.run(0, request)); Routing no(verb::get, regex("/.+"), [](SessionPtr, const RequestPtr) { return false; }); - ASSERT_FALSE(no.matches(0, request)); + ASSERT_FALSE(no.run(0, request)); } TEST_F(RoutingTest, simple_put_with_trailing_slash) diff --git a/test_package/websockets_test.cpp b/test_package/websockets_test.cpp index 6ba31bb0e..3c27f8623 100644 --- a/test_package/websockets_test.cpp +++ b/test_package/websockets_test.cpp @@ -314,7 +314,7 @@ TEST_F(WebsocketsTest, should_return_error_when_a_parameter_is_invalid) { weak_ptr savedSession; - auto probe = [&](SessionPtr session, RequestPtr request) -> bool { + auto sample = [&](SessionPtr session, RequestPtr request) -> bool { savedSession = session; ResponsePtr resp = make_unique(status::ok); resp->m_body = "All Devices for "s + *request->m_requestId; @@ -323,7 +323,7 @@ TEST_F(WebsocketsTest, should_return_error_when_a_parameter_is_invalid) return true; }; - m_server->addRouting({boost::beast::http::verb::get, "/sample?interval={integer}", probe}).command("sample"); + m_server->addRouting({boost::beast::http::verb::get, "/sample?interval={integer}", sample}).command("sample"); m_server->addCommands(); start(); @@ -342,7 +342,7 @@ TEST_F(WebsocketsTest, should_return_error_when_bad_json_is_sent) { weak_ptr savedSession; - auto probe = [&](SessionPtr session, RequestPtr request) -> bool { + auto sample = [&](SessionPtr session, RequestPtr request) -> bool { savedSession = session; ResponsePtr resp = make_unique(status::ok); resp->m_body = "All Devices for "s + *request->m_requestId; @@ -351,7 +351,7 @@ TEST_F(WebsocketsTest, should_return_error_when_bad_json_is_sent) return true; }; - m_server->addRouting({boost::beast::http::verb::get, "/sample?interval={integer}", probe}).command("sample"); + m_server->addRouting({boost::beast::http::verb::get, "/sample?interval={integer}", sample}).command("sample"); m_server->addCommands(); start(); @@ -370,7 +370,7 @@ TEST_F(WebsocketsTest, should_return_multiple_errors_when_parameters_are_invalid { weak_ptr savedSession; - auto probe = [&](SessionPtr session, RequestPtr request) -> bool { + auto sample = [&](SessionPtr session, RequestPtr request) -> bool { savedSession = session; ResponsePtr resp = make_unique(status::ok); resp->m_body = "All Devices for "s + *request->m_requestId; @@ -379,7 +379,7 @@ TEST_F(WebsocketsTest, should_return_multiple_errors_when_parameters_are_invalid return true; }; - m_server->addRouting({boost::beast::http::verb::get, "/sample?interval={integer}&to={unsigned_integer}", probe}).command("sample"); + m_server->addRouting({boost::beast::http::verb::get, "/sample?interval={integer}&to={unsigned_integer}", sample}).command("sample"); m_server->addCommands(); start(); @@ -395,3 +395,30 @@ TEST_F(WebsocketsTest, should_return_multiple_errors_when_parameters_are_invalid "InvalidParameterValue: query parameter 'to': invalid type, expected uint64", m_client->m_result); } +TEST_F(WebsocketsTest, should_return_error_for_an_invalid_command) +{ + weak_ptr savedSession; + + auto probe = [&](SessionPtr session, RequestPtr request) -> bool { + savedSession = session; + ResponsePtr resp = make_unique(status::ok); + resp->m_body = "All Devices for "s + *request->m_requestId; + resp->m_requestId = request->m_requestId; + session->writeResponse(std::move(resp), []() { cout << "Written" << endl; }); + return true; + }; + + m_server->addRouting({boost::beast::http::verb::get, "/probe", probe}).command("probe"); + m_server->addCommands(); + + start(); + startClient(); + + asio::spawn(m_context, std::bind(&Client::request, m_client.get(), + "{\"id\":\"1\",\"request\":\"sample\"}"s, std::placeholders::_1)); + + m_client->waitFor(2s, [this]() { return m_client->m_done; }); + + ASSERT_TRUE(m_client->m_done); + ASSERT_EQ("InvalidURI: 0.0.0.0: Command failed: sample", m_client->m_result); +} From 6e06159aeee02aa7d1ab4238c9496aab7f0646e3 Mon Sep 17 00:00:00 2001 From: Will Sobel Date: Fri, 29 Aug 2025 13:26:21 +0200 Subject: [PATCH 5/5] Formatted with clang format --- src/mtconnect/sink/rest_sink/parameter.hpp | 13 +- src/mtconnect/sink/rest_sink/rest_service.cpp | 4 +- src/mtconnect/sink/rest_sink/rest_service.hpp | 2 +- src/mtconnect/sink/rest_sink/routing.hpp | 40 ++--- src/mtconnect/sink/rest_sink/server.hpp | 8 +- .../sink/rest_sink/websocket_session.hpp | 94 ++++++------ test_package/websockets_test.cpp | 145 ++++++++++-------- 7 files changed, 157 insertions(+), 149 deletions(-) diff --git a/src/mtconnect/sink/rest_sink/parameter.hpp b/src/mtconnect/sink/rest_sink/parameter.hpp index efe3c3999..2c3c479e2 100644 --- a/src/mtconnect/sink/rest_sink/parameter.hpp +++ b/src/mtconnect/sink/rest_sink/parameter.hpp @@ -120,21 +120,20 @@ namespace mtconnect::sink::rest_sink { return "unknown"; } - + /// @brief Helper to convert a ParameterValue to a string static std::string toString(const ParameterValue &v) { using namespace std::string_literals; return std::visit(overloaded {[](const std::monostate &) { return "none"s; }, - [](const std::string &s) { return s; }, - [](int32_t i) { return std::to_string(i); }, - [](uint64_t i) { return std::to_string(i); }, - [](double d) { return std::to_string(d); }, - [](bool b) { return b ? "true"s : "false"s; }}, + [](const std::string &s) { return s; }, + [](int32_t i) { return std::to_string(i); }, + [](uint64_t i) { return std::to_string(i); }, + [](double d) { return std::to_string(d); }, + [](bool b) { return b ? "true"s : "false"s; }}, v); } - std::string m_name; ParameterType m_type {STRING}; /// @brief Default value if one is available diff --git a/src/mtconnect/sink/rest_sink/rest_service.cpp b/src/mtconnect/sink/rest_sink/rest_service.cpp index 779508eeb..c57a9b0ff 100644 --- a/src/mtconnect/sink/rest_sink/rest_service.cpp +++ b/src/mtconnect/sink/rest_sink/rest_service.cpp @@ -476,7 +476,7 @@ namespace mtconnect { auto deviceType = request->parameter("deviceType"); auto format = request->parameter("format"); auto printer = getPrinter(request->m_accepts, format); - + if (device && !ends_with(request->m_path, string("probe")) && m_sinkContract->findDeviceByUUIDorName(*device) == nullptr) return false; @@ -531,7 +531,7 @@ namespace mtconnect { auto pretty = request->parameter("pretty").value_or(false); auto format = request->parameter("format"); auto printer = getPrinter(request->m_accepts, format); - + request->m_request = "MTConnectAssets"; respond(session, diff --git a/src/mtconnect/sink/rest_sink/rest_service.hpp b/src/mtconnect/sink/rest_sink/rest_service.hpp index b96dbaccc..3d5e7943c 100644 --- a/src/mtconnect/sink/rest_sink/rest_service.hpp +++ b/src/mtconnect/sink/rest_sink/rest_service.hpp @@ -313,7 +313,7 @@ namespace mtconnect { ResponsePtr resp = std::make_unique(error.getStatus(), body, prnt->mimeType()); if (error.getRequestId()) resp->m_requestId = error.getRequestId(); - + session->writeFailureResponse(std::move(resp)); } } diff --git a/src/mtconnect/sink/rest_sink/routing.hpp b/src/mtconnect/sink/rest_sink/routing.hpp index 932b6743b..ea4f37ab6 100644 --- a/src/mtconnect/sink/rest_sink/routing.hpp +++ b/src/mtconnect/sink/rest_sink/routing.hpp @@ -157,7 +157,7 @@ namespace mtconnect::sink::rest_sink { const ParameterList &getPathParameters() const { return m_pathParameters; } /// @brief get the unordered set of query parameters const QuerySet &getQueryParameters() const { return m_queryParameters; } - + /// @brief run the session's request if this routing matches /// /// Call the associated lambda when matched @@ -202,7 +202,7 @@ namespace mtconnect::sink::rest_sink { s++; } } - + entity::EntityList errors; for (auto &p : m_queryParameters) { @@ -217,7 +217,7 @@ namespace mtconnect::sink::rest_sink { catch (ParameterError &e) { std::string msg = std::string("query parameter '") + p.m_name + "': " + e.what(); - + LOG(warning) << "Parameter error: " << msg; auto error = InvalidParameterValue::make(p.m_name, q->second, p.getTypeName(), p.getTypeFormat(), msg); @@ -229,7 +229,7 @@ namespace mtconnect::sink::rest_sink { request->m_parameters.emplace(make_pair(p.m_name, p.m_default)); } } - + if (!errors.empty()) throw RestError(errors, request->m_accepts); @@ -241,7 +241,7 @@ namespace mtconnect::sink::rest_sink { } } } - + /// @brief Validate the request parameters without matching the path /// @param[in] session the session making the request to pass to the Routing if matched /// @param[in,out] request the incoming request with a verb and a path @@ -258,15 +258,16 @@ namespace mtconnect::sink::rest_sink { { if (!validateValueType(p.m_type, it->second)) { - std::string msg = std::string("path parameter '") + p.m_name + "': invalid type, expected " + p.getTypeFormat(); + std::string msg = std::string("path parameter '") + p.m_name + + "': invalid type, expected " + p.getTypeFormat(); LOG(warning) << "Parameter error: " << msg; - auto error = InvalidParameterValue::make(p.m_name, Parameter::toString(it->second), p.getTypeName(), - p.getTypeFormat(), msg); + auto error = InvalidParameterValue::make(p.m_name, Parameter::toString(it->second), + p.getTypeName(), p.getTypeFormat(), msg); errors.emplace_back(error); } } } - + for (auto &p : m_queryParameters) { auto it = request->m_parameters.find(p.m_name); @@ -274,10 +275,11 @@ namespace mtconnect::sink::rest_sink { { if (!validateValueType(p.m_type, it->second)) { - std::string msg = std::string("query parameter '") + p.m_name + "': invalid type, expected " + p.getTypeFormat(); + std::string msg = std::string("query parameter '") + p.m_name + + "': invalid type, expected " + p.getTypeFormat(); LOG(warning) << "Parameter error: " << msg; - auto error = InvalidParameterValue::make(p.m_name, Parameter::toString(it->second), p.getTypeName(), - p.getTypeFormat(), msg); + auto error = InvalidParameterValue::make(p.m_name, Parameter::toString(it->second), + p.getTypeName(), p.getTypeFormat(), msg); errors.emplace_back(error); } } @@ -285,10 +287,10 @@ namespace mtconnect::sink::rest_sink { if (!errors.empty()) throw RestError(errors, request->m_accepts); - + return true; } - + /// @brief check if this is related to a swagger API /// @returns `true` if related to swagger auto isSwagger() const { return m_swagger; } @@ -437,14 +439,14 @@ namespace mtconnect::sink::rest_sink { return ParameterValue(); } - + bool validateValueType(ParameterType t, ParameterValue &value) { switch (t) { case STRING: return std::holds_alternative(value); - + case NONE: return std::holds_alternative(value); @@ -453,7 +455,7 @@ namespace mtconnect::sink::rest_sink { value = double(std::get(value)); else if (std::holds_alternative(value)) value = double(std::get(value)); - + return std::holds_alternative(value); case INTEGER: @@ -471,7 +473,7 @@ namespace mtconnect::sink::rest_sink { value = int32_t(v); } return std::holds_alternative(value); - + case UNSIGNED_INTEGER: if (std::holds_alternative(value)) { @@ -486,7 +488,7 @@ namespace mtconnect::sink::rest_sink { value = uint64_t(v); } return std::holds_alternative(value); - + case BOOL: return std::holds_alternative(value); } diff --git a/src/mtconnect/sink/rest_sink/server.hpp b/src/mtconnect/sink/rest_sink/server.hpp index bffb7acc4..df33eb414 100644 --- a/src/mtconnect/sink/rest_sink/server.hpp +++ b/src/mtconnect/sink/rest_sink/server.hpp @@ -181,12 +181,11 @@ namespace mtconnect::sink::rest_sink { if (!success) { std::stringstream txt; - txt << "Cannot find handler for: " << request->m_verb - << " " << request->m_path; + txt << "Cannot find handler for: " << request->m_verb << " " << request->m_path; message = txt.str(); } } - + if (!success) { std::stringstream txt; @@ -202,8 +201,7 @@ namespace mtconnect::sink::rest_sink { { auto uri = request->getUri(); re.setUri(uri); - LOG(error) << session->getRemote().address() - << ": Error processing request: " << uri; + LOG(error) << session->getRemote().address() << ": Error processing request: " << uri; if (request->m_request) re.setRequest(*request->m_request); diff --git a/src/mtconnect/sink/rest_sink/websocket_session.hpp b/src/mtconnect/sink/rest_sink/websocket_session.hpp index 10732a402..f43a425e2 100644 --- a/src/mtconnect/sink/rest_sink/websocket_session.hpp +++ b/src/mtconnect/sink/rest_sink/websocket_session.hpp @@ -309,7 +309,7 @@ namespace mtconnect::sink::rest_sink { } } } - + RequestPtr parseRequest(const std::string &buffer) { using namespace rapidjson; @@ -317,46 +317,47 @@ namespace mtconnect::sink::rest_sink { Document doc; doc.Parse(buffer.c_str()); - + RequestPtr request; - + if (doc.HasParseError()) { stringstream err; err << "Websocket Read Error(offset (" << doc.GetErrorOffset() - << ")): " << GetParseError_En(doc.GetParseError()); + << ")): " << GetParseError_En(doc.GetParseError()); LOG(warning) << err.str(); LOG(warning) << " " << buffer; auto error = Error::make(Error::ErrorCode::INVALID_REQUEST, err.str()); - throw RestError(error, m_request->m_accepts, rest_sink::status::bad_request, - std::nullopt, "ERROR"); + throw RestError(error, m_request->m_accepts, rest_sink::status::bad_request, std::nullopt, + "ERROR"); } if (!doc.IsObject()) { LOG(warning) << "Websocket Read Error: JSON message does not have a top level object"; LOG(warning) << " " << buffer; - auto error = Error::make(Error::ErrorCode::INVALID_REQUEST, "JSON message does not have a top level object"); - throw RestError(error, m_request->m_accepts, rest_sink::status::bad_request, - std::nullopt, "ERROR"); + auto error = Error::make(Error::ErrorCode::INVALID_REQUEST, + "JSON message does not have a top level object"); + throw RestError(error, m_request->m_accepts, rest_sink::status::bad_request, std::nullopt, + "ERROR"); } else { // Extract the parameters from the json doc to map them to the REST // protocol parameters request = make_unique(*m_request); - + request->m_verb = beast::http::verb::get; request->m_parameters.clear(); #ifdef GetObject #define __GOSave__ GetObject #undef GetObject #endif - + const auto &object = doc.GetObject(); #ifdef __GOSave__ #define GetObject __GOSave__ #endif - + for (auto &it : object) { switch (it.value.GetType()) @@ -366,11 +367,11 @@ namespace mtconnect::sink::rest_sink { break; case rapidjson::kFalseType: request->m_parameters.emplace( - make_pair(string(it.name.GetString()), ParameterValue(false))); + make_pair(string(it.name.GetString()), ParameterValue(false))); break; case rapidjson::kTrueType: request->m_parameters.emplace( - make_pair(string(it.name.GetString()), ParameterValue(true))); + make_pair(string(it.name.GetString()), ParameterValue(true))); break; case rapidjson::kObjectType: break; @@ -378,34 +379,34 @@ namespace mtconnect::sink::rest_sink { break; case rapidjson::kStringType: request->m_parameters.emplace( - make_pair(it.name.GetString(), ParameterValue(string(it.value.GetString())))); - + make_pair(it.name.GetString(), ParameterValue(string(it.value.GetString())))); + break; case rapidjson::kNumberType: if (it.value.IsInt()) request->m_parameters.emplace( - make_pair(it.name.GetString(), ParameterValue(it.value.GetInt()))); + make_pair(it.name.GetString(), ParameterValue(it.value.GetInt()))); else if (it.value.IsUint()) request->m_parameters.emplace( - make_pair(it.name.GetString(), ParameterValue(uint64_t(it.value.GetUint())))); + make_pair(it.name.GetString(), ParameterValue(uint64_t(it.value.GetUint())))); else if (it.value.IsInt64()) request->m_parameters.emplace( - make_pair(it.name.GetString(), ParameterValue(uint64_t(it.value.GetInt64())))); + make_pair(it.name.GetString(), ParameterValue(uint64_t(it.value.GetInt64())))); else if (it.value.IsUint64()) request->m_parameters.emplace( - make_pair(it.name.GetString(), ParameterValue(it.value.GetUint64()))); + make_pair(it.name.GetString(), ParameterValue(it.value.GetUint64()))); else if (it.value.IsDouble()) request->m_parameters.emplace( - make_pair(it.name.GetString(), ParameterValue(double(it.value.GetDouble())))); - + make_pair(it.name.GetString(), ParameterValue(double(it.value.GetDouble())))); + break; } } } - + return request; } - + bool dispatchRequest(RequestPtr &&request) { using namespace std; @@ -414,17 +415,16 @@ namespace mtconnect::sink::rest_sink { { auto &v = request->m_parameters["id"]; string id = visit(overloaded {[](monostate m) { return ""s; }, - [](auto v) { return boost::lexical_cast(v); }}, + [](auto v) { return boost::lexical_cast(v); }}, v); request->m_requestId = id; request->m_parameters.erase("id"); } else { - auto error = InvalidParameterValue::make("id", "", "string", "string", - "No id given"); - throw RestError(error, request->m_accepts, rest_sink::status::bad_request, - std::nullopt, "ERROR"); + auto error = InvalidParameterValue::make("id", "", "string", "string", "No id given"); + throw RestError(error, request->m_accepts, rest_sink::status::bad_request, std::nullopt, + "ERROR"); } auto &id = *(request->m_requestId); @@ -434,10 +434,10 @@ namespace mtconnect::sink::rest_sink { LOG(error) << "Duplicate request id: " << id; auto error = InvalidParameterValue::make("id", *request->m_requestId, "string", "string", "Duplicate id given"); - throw RestError(error, request->m_accepts, rest_sink::status::bad_request, - std::nullopt, "ERROR"); + throw RestError(error, request->m_accepts, rest_sink::status::bad_request, std::nullopt, + "ERROR"); } - + if (request->m_parameters.count("request") > 0) { request->m_command = get(request->m_parameters["request"]); @@ -445,27 +445,27 @@ namespace mtconnect::sink::rest_sink { } else { - auto error = InvalidParameterValue::make("request", "", "string", "string", - "No request given"); - throw RestError(error, request->m_accepts, rest_sink::status::bad_request, - std::nullopt, id); + auto error = + InvalidParameterValue::make("request", "", "string", "string", "No request given"); + throw RestError(error, request->m_accepts, rest_sink::status::bad_request, std::nullopt, + id); } - + // Check parameters for command LOG(debug) << "Received request id: " << id; - + res.first->second.m_request = std::move(request); try { return m_dispatch(derived().shared_ptr(), res.first->second.m_request); } - + catch (RestError &re) { re.setRequestId(id); throw re; } - + return false; } @@ -489,7 +489,7 @@ namespace mtconnect::sink::rest_sink { m_buffer.consume(m_buffer.size()); LOG(debug) << "Received :" << buffer; - + try { auto request = parseRequest(buffer); @@ -500,7 +500,7 @@ namespace mtconnect::sink::rest_sink { LOG(error) << txt.str(); } } - + catch (RestError &re) { auto id = re.getRequestId(); @@ -509,14 +509,14 @@ namespace mtconnect::sink::rest_sink { id = "ERROR"; re.setRequestId(*id); } - + auto res = m_requests.find(*id); if (res == m_requests.end()) m_requests.emplace(*id, *id); - + m_errorFunction(derived().shared_ptr(), re); } - + catch (std::logic_error &le) { std::stringstream txt; @@ -524,7 +524,7 @@ namespace mtconnect::sink::rest_sink { LOG(error) << txt.str(); fail(boost::beast::http::status::not_found, txt.str()); } - + catch (...) { std::stringstream txt; @@ -532,7 +532,7 @@ namespace mtconnect::sink::rest_sink { LOG(error) << txt.str(); fail(boost::beast::http::status::not_found, txt.str()); } - + derived().stream().async_read( m_buffer, beast::bind_front_handler(&WebsocketSession::onRead, derived().shared_ptr())); } diff --git a/test_package/websockets_test.cpp b/test_package/websockets_test.cpp index 3c27f8623..d225449b8 100644 --- a/test_package/websockets_test.cpp +++ b/test_package/websockets_test.cpp @@ -25,7 +25,6 @@ #include #include #include -#include "test_utilities.hpp" #include #include @@ -36,6 +35,7 @@ #include "mtconnect/logging.hpp" #include "mtconnect/sink/rest_sink/server.hpp" +#include "test_utilities.hpp" using namespace std; using namespace mtconnect; @@ -167,10 +167,7 @@ class Client class WebsocketsTest : public testing::Test { protected: - void SetUp() override - { - createServer({}); - } + void SetUp() override { createServer({}); } void createServer(const ConfigOptions& options) { @@ -178,14 +175,13 @@ class WebsocketsTest : public testing::Test ConfigOptions opts {{Port, 0}, {ServerIp, "127.0.0.1"s}}; opts.merge(ConfigOptions(options)); m_server = make_unique(m_context, opts); - m_server->setErrorFunction([](SessionPtr session, const RestError &error) - { - ResponsePtr resp = std::make_unique(error.getStatus(), error.what(), "plain/text"); - if (error.getRequestId()) - resp->m_requestId = error.getRequestId(); - - session->writeFailureResponse(std::move(resp)); - }); + m_server->setErrorFunction([](SessionPtr session, const RestError& error) { + ResponsePtr resp = std::make_unique(error.getStatus(), error.what(), "plain/text"); + if (error.getRequestId()) + resp->m_requestId = error.getRequestId(); + + session->writeFailureResponse(std::move(resp)); + }); } void start() @@ -256,7 +252,7 @@ TEST_F(WebsocketsTest, should_make_simple_request) TEST_F(WebsocketsTest, should_return_error_when_there_is_no_id) { weak_ptr savedSession; - + auto probe = [&](SessionPtr session, RequestPtr request) -> bool { savedSession = session; ResponsePtr resp = make_unique(status::ok); @@ -265,18 +261,18 @@ TEST_F(WebsocketsTest, should_return_error_when_there_is_no_id) session->writeResponse(std::move(resp), []() { cout << "Written" << endl; }); return true; }; - + m_server->addRouting({boost::beast::http::verb::get, "/probe", probe}).command("probe"); m_server->addCommands(); - + start(); startClient(); - - asio::spawn(m_context, std::bind(&Client::request, m_client.get(), - "{\"request\":\"probe\"}"s, std::placeholders::_1)); - + + asio::spawn(m_context, std::bind(&Client::request, m_client.get(), "{\"request\":\"probe\"}"s, + std::placeholders::_1)); + m_client->waitFor(2s, [this]() { return m_client->m_done; }); - + ASSERT_TRUE(m_client->m_done); ASSERT_EQ("InvalidParameterValue: No id given", m_client->m_result); } @@ -284,7 +280,7 @@ TEST_F(WebsocketsTest, should_return_error_when_there_is_no_id) TEST_F(WebsocketsTest, should_return_error_when_there_is_no_request) { weak_ptr savedSession; - + auto probe = [&](SessionPtr session, RequestPtr request) -> bool { savedSession = session; ResponsePtr resp = make_unique(status::ok); @@ -293,27 +289,26 @@ TEST_F(WebsocketsTest, should_return_error_when_there_is_no_request) session->writeResponse(std::move(resp), []() { cout << "Written" << endl; }); return true; }; - + m_server->addRouting({boost::beast::http::verb::get, "/probe", probe}).command("probe"); m_server->addCommands(); - + start(); startClient(); - - asio::spawn(m_context, std::bind(&Client::request, m_client.get(), - "{\"id\": 3}"s, std::placeholders::_1)); - + + asio::spawn(m_context, + std::bind(&Client::request, m_client.get(), "{\"id\": 3}"s, std::placeholders::_1)); + m_client->waitFor(2s, [this]() { return m_client->m_done; }); - + ASSERT_TRUE(m_client->m_done); ASSERT_EQ("InvalidParameterValue: No request given", m_client->m_result); } - TEST_F(WebsocketsTest, should_return_error_when_a_parameter_is_invalid) { weak_ptr savedSession; - + auto sample = [&](SessionPtr session, RequestPtr request) -> bool { savedSession = session; ResponsePtr resp = make_unique(status::ok); @@ -322,26 +317,30 @@ TEST_F(WebsocketsTest, should_return_error_when_a_parameter_is_invalid) session->writeResponse(std::move(resp), []() { cout << "Written" << endl; }); return true; }; - - m_server->addRouting({boost::beast::http::verb::get, "/sample?interval={integer}", sample}).command("sample"); + + m_server->addRouting({boost::beast::http::verb::get, "/sample?interval={integer}", sample}) + .command("sample"); m_server->addCommands(); - + start(); startClient(); - - asio::spawn(m_context, std::bind(&Client::request, m_client.get(), - "{\"id\": 3, \"request\": \"sample\", \"interval\": 99999999999}"s, std::placeholders::_1)); - + + asio::spawn(m_context, + std::bind(&Client::request, m_client.get(), + "{\"id\": 3, \"request\": \"sample\", \"interval\": 99999999999}"s, + std::placeholders::_1)); + m_client->waitFor(2s, [this]() { return m_client->m_done; }); - + ASSERT_TRUE(m_client->m_done); - ASSERT_EQ("InvalidParameterValue: query parameter 'interval': invalid type, expected int32", m_client->m_result); + ASSERT_EQ("InvalidParameterValue: query parameter 'interval': invalid type, expected int32", + m_client->m_result); } TEST_F(WebsocketsTest, should_return_error_when_bad_json_is_sent) { weak_ptr savedSession; - + auto sample = [&](SessionPtr session, RequestPtr request) -> bool { savedSession = session; ResponsePtr resp = make_unique(status::ok); @@ -350,18 +349,19 @@ TEST_F(WebsocketsTest, should_return_error_when_bad_json_is_sent) session->writeResponse(std::move(resp), []() { cout << "Written" << endl; }); return true; }; - - m_server->addRouting({boost::beast::http::verb::get, "/sample?interval={integer}", sample}).command("sample"); + + m_server->addRouting({boost::beast::http::verb::get, "/sample?interval={integer}", sample}) + .command("sample"); m_server->addCommands(); - + start(); startClient(); - - asio::spawn(m_context, std::bind(&Client::request, m_client.get(), - "!}}"s, std::placeholders::_1)); - + + asio::spawn(m_context, + std::bind(&Client::request, m_client.get(), "!}}"s, std::placeholders::_1)); + m_client->waitFor(2s, [this]() { return m_client->m_done; }); - + ASSERT_TRUE(m_client->m_done); ASSERT_EQ("InvalidRequest: Websocket Read Error(offset (0)): Invalid value.", m_client->m_result); } @@ -369,7 +369,7 @@ TEST_F(WebsocketsTest, should_return_error_when_bad_json_is_sent) TEST_F(WebsocketsTest, should_return_multiple_errors_when_parameters_are_invalid) { weak_ptr savedSession; - + auto sample = [&](SessionPtr session, RequestPtr request) -> bool { savedSession = session; ResponsePtr resp = make_unique(status::ok); @@ -378,27 +378,35 @@ TEST_F(WebsocketsTest, should_return_multiple_errors_when_parameters_are_invalid session->writeResponse(std::move(resp), []() { cout << "Written" << endl; }); return true; }; - - m_server->addRouting({boost::beast::http::verb::get, "/sample?interval={integer}&to={unsigned_integer}", sample}).command("sample"); + + m_server + ->addRouting({boost::beast::http::verb::get, + "/sample?interval={integer}&to={unsigned_integer}", sample}) + .command("sample"); m_server->addCommands(); - + start(); startClient(); - - asio::spawn(m_context, std::bind(&Client::request, m_client.get(), - R"DOC({"id": 3, "request": "sample", "interval": 99999999999,"to": -1 })DOC", std::placeholders::_1)); - + + asio::spawn( + m_context, + std::bind(&Client::request, m_client.get(), + R"DOC({"id": 3, "request": "sample", "interval": 99999999999,"to": -1 })DOC", + std::placeholders::_1)); + m_client->waitFor(2s, [this]() { return m_client->m_done; }); - + ASSERT_TRUE(m_client->m_done); - ASSERT_EQ("InvalidParameterValue: query parameter 'interval': invalid type, expected int32, " - "InvalidParameterValue: query parameter 'to': invalid type, expected uint64", m_client->m_result); + ASSERT_EQ( + "InvalidParameterValue: query parameter 'interval': invalid type, expected int32, " + "InvalidParameterValue: query parameter 'to': invalid type, expected uint64", + m_client->m_result); } TEST_F(WebsocketsTest, should_return_error_for_an_invalid_command) { weak_ptr savedSession; - + auto probe = [&](SessionPtr session, RequestPtr request) -> bool { savedSession = session; ResponsePtr resp = make_unique(status::ok); @@ -407,18 +415,19 @@ TEST_F(WebsocketsTest, should_return_error_for_an_invalid_command) session->writeResponse(std::move(resp), []() { cout << "Written" << endl; }); return true; }; - + m_server->addRouting({boost::beast::http::verb::get, "/probe", probe}).command("probe"); m_server->addCommands(); - + start(); startClient(); - - asio::spawn(m_context, std::bind(&Client::request, m_client.get(), - "{\"id\":\"1\",\"request\":\"sample\"}"s, std::placeholders::_1)); - + + asio::spawn(m_context, + std::bind(&Client::request, m_client.get(), "{\"id\":\"1\",\"request\":\"sample\"}"s, + std::placeholders::_1)); + m_client->waitFor(2s, [this]() { return m_client->m_done; }); - + ASSERT_TRUE(m_client->m_done); ASSERT_EQ("InvalidURI: 0.0.0.0: Command failed: sample", m_client->m_result); }