diff --git a/rosbag2_storage_mcap/include/rosbag2_storage_mcap/message_definition_cache.hpp b/rosbag2_storage_mcap/include/rosbag2_storage_mcap/message_definition_cache.hpp index 024c6641a6..efbd6b9062 100644 --- a/rosbag2_storage_mcap/include/rosbag2_storage_mcap/message_definition_cache.hpp +++ b/rosbag2_storage_mcap/include/rosbag2_storage_mcap/message_definition_cache.hpp @@ -29,6 +29,7 @@ enum struct Format { IDL, MSG, + UNKNOWN, }; struct MessageSpec diff --git a/rosbag2_storage_mcap/src/mcap_storage.cpp b/rosbag2_storage_mcap/src/mcap_storage.cpp index 322a11f482..8492002a29 100644 --- a/rosbag2_storage_mcap/src/mcap_storage.cpp +++ b/rosbag2_storage_mcap/src/mcap_storage.cpp @@ -725,10 +725,18 @@ void MCAPStorage::create_topic(const rosbag2_storage::TopicMetadata & topic) schema.name = datatype; try { auto [format, full_text] = msgdef_cache_.get_full_text(datatype); - if (format == rosbag2_storage_mcap::internal::Format::MSG) { - schema.encoding = "ros2msg"; - } else { - schema.encoding = "ros2idl"; + switch (format) { + case rosbag2_storage_mcap::internal::Format::UNKNOWN: + schema.encoding = "unknown"; + break; + case rosbag2_storage_mcap::internal::Format::MSG: + schema.encoding = "ros2msg"; + break; + case rosbag2_storage_mcap::internal::Format::IDL: + schema.encoding = "ros2idl"; + break; + default: + throw std::runtime_error("switch is not exhaustive"); } schema.data.assign(reinterpret_cast(full_text.data()), reinterpret_cast(full_text.data() + full_text.size())); diff --git a/rosbag2_storage_mcap/src/message_definition_cache.cpp b/rosbag2_storage_mcap/src/message_definition_cache.cpp index 5cfbebac81..474aa9b4d7 100644 --- a/rosbag2_storage_mcap/src/message_definition_cache.cpp +++ b/rosbag2_storage_mcap/src/message_definition_cache.cpp @@ -28,10 +28,31 @@ #include #include +#define MESSAGE_DEFINITION_SOURCE_MAX_RECURSION_DEPTH 50 + namespace rosbag2_storage_mcap::internal { +/// A type name did not match expectations, so a definition could not be looked for. +class TypenameNotUnderstoodError : public std::exception +{ +private: + std::string name_; + +public: + explicit TypenameNotUnderstoodError(std::string name) + : name_(std::move(name)) + { + } + + const char * what() const noexcept override + { + return name_.c_str(); + } +}; + // Match datatype names (foo_msgs/Bar or foo_msgs/msg/Bar) -static const std::regex PACKAGE_TYPENAME_REGEX{R"(^([a-zA-Z0-9_]+)/(?:msg/)?([a-zA-Z0-9_]+)$)"}; +static const std::regex PACKAGE_TYPENAME_REGEX{ + R"(^([a-zA-Z][a-zA-Z0-9_]*)/((?:msg|srv)/)?([a-zA-Z][a-zA-Z0-9_]*)$)"}; // Match field types from .msg definitions ("foo_msgs/Bar" in "foo_msgs/Bar[] bar") static const std::regex MSG_FIELD_TYPE_REGEX{R"((?:^|\n)\s*([a-zA-Z0-9_/]+)(?:\[[^\]]*\])?\s+)"}; @@ -136,12 +157,16 @@ const MessageSpec & MessageDefinitionCache::load_message_spec( std::smatch match; if (!std::regex_match(definition_identifier.package_resource_name, match, PACKAGE_TYPENAME_REGEX)) { - throw std::invalid_argument("Invalid package resource name: " + - definition_identifier.package_resource_name); + throw TypenameNotUnderstoodError(definition_identifier.package_resource_name); + } + const std::string package = match[1]; + std::string namespace_name = match[2]; + if (namespace_name.empty()) { + namespace_name = "msg"; } - std::string package = match[1]; + const std::string type_name = match[3]; std::string share_dir = ament_index_cpp::get_package_share_directory(package); - std::ifstream file{share_dir + "/msg/" + match[2].str() + + std::ifstream file{share_dir + "/" + namespace_name + "/" + type_name + extension_for_format(definition_identifier.format)}; if (!file.good()) { throw DefinitionNotFoundError(definition_identifier.package_resource_name); @@ -164,8 +189,12 @@ std::pair MessageDefinitionCache::get_full_text( { std::unordered_set seen_deps; - std::function append_recursive = - [&](const DefinitionIdentifier & definition_identifier) { + std::function append_recursive = + [&](const DefinitionIdentifier & definition_identifier, int32_t depth) { + if (depth <= 0) { + throw std::runtime_error{"Reached max recursion depth resolving definition of " + + root_package_resource_name}; + } const MessageSpec & spec = load_message_spec(definition_identifier); std::string result = spec.text; for (const auto & dep_name : spec.dependencies) { @@ -174,7 +203,7 @@ std::pair MessageDefinitionCache::get_full_text( if (inserted) { result += "\n"; result += delimiter(dep); - result += append_recursive(dep); + result += append_recursive(dep, depth - 1); } } return result; @@ -182,15 +211,32 @@ std::pair MessageDefinitionCache::get_full_text( std::string result; Format format = Format::MSG; + int32_t max_recursion_depth = MESSAGE_DEFINITION_SOURCE_MAX_RECURSION_DEPTH; try { - result = append_recursive(DefinitionIdentifier{format, root_package_resource_name}); - } catch (const DefinitionNotFoundError & err) { - // log that we've fallen back - RCUTILS_LOG_WARN_NAMED("rosbag2_storage_mcap", "no .msg definition for %s, falling back to IDL", - err.what()); - format = Format::IDL; - DefinitionIdentifier root_definition_identifier{format, root_package_resource_name}; - result = delimiter(root_definition_identifier) + append_recursive(root_definition_identifier); + try { + result = append_recursive(DefinitionIdentifier{format, root_package_resource_name}, + max_recursion_depth); + } catch (const DefinitionNotFoundError & err) { + // log that we've fallen back + RCUTILS_LOG_WARN_NAMED("rosbag2_storage_mcap", + "no .msg definition for %s, falling back to IDL", err.what()); + format = Format::IDL; + DefinitionIdentifier root_definition_identifier{format, root_package_resource_name}; + result = delimiter(root_definition_identifier) + + append_recursive(root_definition_identifier, max_recursion_depth); + } + } catch (const TypenameNotUnderstoodError & err) { + RCUTILS_LOG_ERROR_NAMED("rosbag2_storage_mcap", + "Message type name '%s' is not understood by type definition search, " + "definition wll be left empty.", + err.what()); + format = Format::UNKNOWN; + } catch (const std::runtime_error & err) { + RCUTILS_LOG_ERROR_NAMED("rosbag2_storage_mcap", + "Unexpected error occurred finding message definition, " + "will be left empty: %s", + err.what()); + format = Format::UNKNOWN; } return std::make_pair(format, result); } diff --git a/rosbag2_storage_mcap/test/rosbag2_storage_mcap/test_message_definition_cache.cpp b/rosbag2_storage_mcap/test/rosbag2_storage_mcap/test_message_definition_cache.cpp index d0cbe70f4d..94632f91fe 100644 --- a/rosbag2_storage_mcap/test/rosbag2_storage_mcap/test_message_definition_cache.cpp +++ b/rosbag2_storage_mcap/test/rosbag2_storage_mcap/test_message_definition_cache.cpp @@ -124,3 +124,26 @@ module rosbag2_storage_mcap_testdata { }; )r"); } + +TEST(test_local_message_definition_source, no_crash_on_bad_name) +{ + MessageDefinitionCache source; + std::pair result; + ASSERT_NO_THROW({ + result = + source.get_full_text("rosbag2_storage_mcap_testdata/action/BasicAction_SetGoal_Request"); + }); + ASSERT_EQ(result.first, rosbag2_storage_mcap::internal::Format::UNKNOWN); +} + +TEST(test_message_definition_cache, get_service_message_definitions) +{ + MessageDefinitionCache source; + auto result = source.get_full_text("rosbag2_storage_mcap_testdata/srv/BasicSrv_Request"); + ASSERT_EQ(result.first, rosbag2_storage_mcap::internal::Format::MSG); + ASSERT_EQ(result.second, "string req\n"); + + result = source.get_full_text("rosbag2_storage_mcap_testdata/srv/BasicSrv_Response"); + ASSERT_EQ(result.first, rosbag2_storage_mcap::internal::Format::MSG); + ASSERT_EQ(result.second, "\nstring resp\n"); +} diff --git a/rosbag2_storage_mcap_testdata/CMakeLists.txt b/rosbag2_storage_mcap_testdata/CMakeLists.txt index d41a600298..0640e159cf 100644 --- a/rosbag2_storage_mcap_testdata/CMakeLists.txt +++ b/rosbag2_storage_mcap_testdata/CMakeLists.txt @@ -14,6 +14,8 @@ rosidl_generate_interfaces(${PROJECT_NAME} "msg/BasicMsg.msg" "msg/ComplexMsg.msg" "msg/ComplexMsgDependsOnIdl.msg" + "srv/BasicSrv.srv" + "action/BasicAction.action" ADD_LINTER_TESTS ) diff --git a/rosbag2_storage_mcap_testdata/action/BasicAction.action b/rosbag2_storage_mcap_testdata/action/BasicAction.action new file mode 100644 index 0000000000..b820ea193f --- /dev/null +++ b/rosbag2_storage_mcap_testdata/action/BasicAction.action @@ -0,0 +1,5 @@ +string goal +--- +string result +--- +string feedback diff --git a/rosbag2_storage_mcap_testdata/package.xml b/rosbag2_storage_mcap_testdata/package.xml index ffa031461d..4865c09586 100644 --- a/rosbag2_storage_mcap_testdata/package.xml +++ b/rosbag2_storage_mcap_testdata/package.xml @@ -12,6 +12,8 @@ ament_cmake rosidl_default_generators + action_msgs + ament_lint_auto ament_lint_common diff --git a/rosbag2_storage_mcap_testdata/srv/BasicSrv.srv b/rosbag2_storage_mcap_testdata/srv/BasicSrv.srv new file mode 100644 index 0000000000..8a9ec1a75a --- /dev/null +++ b/rosbag2_storage_mcap_testdata/srv/BasicSrv.srv @@ -0,0 +1,3 @@ +string req +--- +string resp