[humble] Don't crash when type definition cannot be found, and find s…

…rv defs if available (#1398) * Don't crash when type definition cannot be found, and find srv defs if available Signed-off-by: Emerson Knapp <[email protected]>
ros2 · Jun 15, 2023 · c3456fe · c3456fe
1 parent 03aba85
commit c3456fe
Show file tree

Hide file tree

Showing 8 changed files with 110 additions and 20 deletions.
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
@@ -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
@@ -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<const std::byte *>(full_text.data()),
                          reinterpret_cast<const std::byte *>(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
@@ -28,10 +28,31 @@
 #include <unordered_set>
 #include <utility>
 
+#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<Format, std::string> MessageDefinitionCache::get_full_text(
 {
   std::unordered_set<DefinitionIdentifier, DefinitionIdentifierHash> seen_deps;
 
-  std::function<std::string(const DefinitionIdentifier &)> append_recursive =
-    [&](const DefinitionIdentifier & definition_identifier) {
+  std::function<std::string(const DefinitionIdentifier &, int32_t)> 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,23 +203,40 @@ std::pair<Format, std::string> MessageDefinitionCache::get_full_text(
         if (inserted) {
           result += "\n";
           result += delimiter(dep);
-          result += append_recursive(dep);
+          result += append_recursive(dep, depth - 1);
         }
       }
       return result;
     };
 
   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
@@ -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<Format, std::string> 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
@@ -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
@@ -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
@@ -12,6 +12,8 @@
   <buildtool_depend>ament_cmake</buildtool_depend>
   <buildtool_depend>rosidl_default_generators</buildtool_depend>
 
+  <depend>action_msgs</depend>
+
   <test_depend>ament_lint_auto</test_depend>
   <test_depend>ament_lint_common</test_depend>
 

diff --git a/rosbag2_storage_mcap_testdata/srv/BasicSrv.srv b/rosbag2_storage_mcap_testdata/srv/BasicSrv.srv
@@ -0,0 +1,3 @@
+string req
+---
+string resp