Uh oh!
There was an error while loading. Please reload this page.
Add options support for cors - #599
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds CORS preflight (OPTIONS) handling to the REST sink HTTP server, including generating Allow / Access-Control-Allow-Methods headers based on registered routings, and adjusts several logging severities/messages.
Changes:
- Add an OPTIONS dispatch path that returns CORS preflight headers (Allow/ACAM/ACAH/Max-Age) derived from matching routings.
- Add
Routing::matchesPath()plus “catch-all” routing classification to support OPTIONS method discovery. - Add/extend tests for route path-only matching and OPTIONS responses; adjust some logging output/severity.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test_package/routing_test.cpp | Adds unit tests for the new Routing::matchesPath() behavior. |
| test_package/http_server_test.cpp | Adds integration tests asserting OPTIONS preflight responses and headers. |
| src/mtconnect/source/adapter/shdr/connector.cpp | Adjusts log levels/messages for retryable connection/endpoint issues. |
| src/mtconnect/sink/rest_sink/session_impl.cpp | Allows OPTIONS when PUTs are disabled; applies per-response extra headers. |
| src/mtconnect/sink/rest_sink/server.hpp | Routes OPTIONS requests to a new CORS preflight handler. |
| src/mtconnect/sink/rest_sink/server.cpp | Implements OPTIONS handler that computes and returns allowed methods + CORS headers. |
| src/mtconnect/sink/rest_sink/routing.hpp | Adds matchesPath(), catch-all detection, and modifies path parameter parsing. |
| src/mtconnect/sink/rest_sink/response.hpp | Adds per-response custom header fields storage. |
| src/mtconnect/sink/rest_sink/parameter.hpp | Adds Parameter constructor overload taking std::string_view. |
| src/mtconnect/pipeline/shdr_token_mapper.cpp | Lowers severity of conversion failure logs. |
| src/mtconnect/device_model/configuration/solid_model.cpp | Formatting-only changes. |
| src/mtconnect/device_model/configuration/motion.cpp | Formatting-only changes. |
| src/mtconnect/device_model/configuration/coordinate_systems.cpp | Formatting-only changes. |
| src/mtconnect/configuration/agent_config.cpp | Adjusts plugin load failure logging to debug with clearer retry intent. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| m_pathParameters.emplace_back(match[1]); | ||
| s = match.suffix().str(); | ||
| auto range = boost::make_iterator_range(s.begin() + pos, s.end()); | ||
| split(parts, range, [](char c) { return c == '/'; }); |
There was a problem hiding this comment.
pathParameters currently calls boost::algorithm::split(parts, range, [](char c) { return c == '/'; });, but Boost's split expects a finder (e.g., boost::is_any_of("/")), not a char -> bool predicate. As written this is likely to fail to compile; use boost::is_any_of("/") (and an appropriate container type, e.g. std::vector<iterator_range<...>>) for consistency with other code (see SessionImpl::parseQueries).
| split(parts, range, [](char c) { return c == '/'; }); | |
| split(parts, range, boost::is_any_of("/")); |
| if (openBrace > start) | ||
| { | ||
| pat << std::string_view(start, openBrace); | ||
| hasLiteral = true; | ||
| } | ||
| std::string_view param(openBrace + 1, closeBrace); | ||
| pat << "([^/]+)"; | ||
| if (closeBrace + 1 < end) | ||
| { | ||
| pat << std::string_view(closeBrace + 1, end); | ||
| hasLiteral = true; | ||
| } | ||
| m_pathParameters.emplace_back(param); |
There was a problem hiding this comment.
std::string_view(start, openBrace) / std::string_view(openBrace + 1, closeBrace) / etc. are constructed from iterators, but std::string_view does not have iterator-pair constructors. This will not compile; create the view from a pointer+length (e.g. &*it + std::distance) or append via pat.write() / std::string(start, openBrace) instead.
| #include <boost/algorithm/string.hpp> | ||
| #include <boost/beast/http/verb.hpp> | ||
| #include <iostream> |
There was a problem hiding this comment.
#include <iostream> appears unused in this header. Consider removing it to avoid unnecessary recompiles and header bloat.
| #include <iostream> |
| set<http::verb> specificVerbs; | ||
| set<http::verb> catchAllVerbs; | ||
| for (const auto &r : m_routings) | ||
| { | ||
| if (!r.isSwagger() && r.matchesPath(request->m_path)) | ||
| { | ||
| if (r.isCatchAll()) | ||
| catchAllVerbs.insert(r.getVerb()); | ||
| else | ||
| specificVerbs.insert(r.getVerb()); | ||
| } | ||
| } | ||
| // If any specific route matched, use only those; otherwise fall back to catch-alls | ||
| auto &verbs = specificVerbs.empty() ? catchAllVerbs : specificVerbs; | ||
| // OPTIONS is always allowed | ||
| verbs.insert(http::verb::options); | ||
| // Build the Allow / Access-Control-Allow-Methods header value | ||
| string methods = algorithm::join( | ||
| verbs | transformed([](http::verb v) { return string(http::to_string(v)); }), ", "); | ||
| auto response = std::make_unique<Response>(status::no_content, "", "text/plain"); | ||
| response->m_close = false; | ||
| response->m_fields.emplace_back("Allow", methods); | ||
| response->m_fields.emplace_back("Access-Control-Allow-Methods", methods); | ||
| response->m_fields.emplace_back("Access-Control-Allow-Headers", | ||
| "Content-Type, Accept, Accept-Encoding"); | ||
| response->m_fields.emplace_back("Access-Control-Max-Age", "86400"); | ||
| session->writeResponse(std::move(response)); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
handleOptionsRequest always inserts OPTIONS and returns true, even when no routing matched request->m_path. That means an OPTIONS to an unknown path will return 204 instead of falling through to the existing 404 handling (and it contradicts the header comment that says it can return false). Consider returning false when both specificVerbs and catchAllVerbs are empty (before inserting OPTIONS), and only generating the Allow/ACAM headers when something actually matched.
…agent\cppagent\ into add_options_support_for_cors
Uh oh!
There was an error while loading. Please reload this page.
Handle OPTIONS request for CORS preflight
Fixed logging of errors as warnings or not saying that the problem will be retried.
Fixed log file creation and archival storage. Now is relative to cfg file by default and creates directories.