Don't crash when type definition cannot be found (#1350) (#1352)

* Don't fail when type definition cannot be found

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
(cherry picked from commit dae4660)

Co-authored-by: Emerson Knapp <537409+emersonknapp@users.noreply.github.com>

rosbag2_cpp/src/rosbag2_cpp/message_definitions/local_message_definition_source.cpp

@@ -14,8 +14,6 @@
 
 #include "rosbag2_cpp/message_definitions/local_message_definition_source.hpp"
 
-#include <rcutils/logging_macros.h>
-
 #include <fstream>
 #include <functional>
 #include <optional>
@@ -27,8 +25,28 @@
 
 #include <ament_index_cpp/get_package_share_directory.hpp>
 
+#include "rosbag2_cpp/logging.hpp"
+
 namespace rosbag2_cpp
 {
+
+/// 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_]+)$)"};
 
@@ -144,7 +162,7 @@ const LocalMessageDefinitionSource::MessageSpec & LocalMessageDefinitionSource::
   std::smatch match;
   const auto topic_type = definition_identifier.topic_type();
   if (!std::regex_match(topic_type, match, PACKAGE_TYPENAME_REGEX)) {
-    throw std::invalid_argument("Invalid topic_type: " + topic_type);
+    throw TypenameNotUnderstoodError(topic_type);
   }
   std::string package = match[1];
   std::string share_dir = ament_index_cpp::get_package_share_directory(package);
@@ -195,26 +213,30 @@ rosbag2_storage::MessageDefinition LocalMessageDefinitionSource::get_full_text(
   try {
     result = append_recursive(DefinitionIdentifier(root_topic_type, format), max_recursion_depth);
   } catch (const DefinitionNotFoundError & err) {
-    // log that we've fallen back
-    RCUTILS_LOG_WARN_NAMED(
-      "rosbag2_cpp", "no .msg definition for %s, falling back to IDL",
-      err.what());
+    ROSBAG2_CPP_LOG_WARN("No .msg definition for %s, falling back to IDL", err.what());
     format = Format::IDL;
     DefinitionIdentifier root_definition_identifier(root_topic_type, format);
     result = (delimiter(root_definition_identifier) +
       append_recursive(root_definition_identifier, max_recursion_depth));
+  } catch (const TypenameNotUnderstoodError & err) {
+    ROSBAG2_CPP_LOG_ERROR(
+      "Message type name '%s' not understood by type definition search, "
+      "definition will be left empty in bag.", err.what());
+    format = Format::UNKNOWN;
   }
   rosbag2_storage::MessageDefinition out;
   switch (format) {
     case Format::UNKNOWN:
-      throw std::runtime_error{"could not determine format of message definition for type " +
-              root_topic_type};
+      out.encoding = "unknown";
+      break;
     case Format::MSG:
       out.encoding = "ros2msg";
       break;
     case Format::IDL:
       out.encoding = "ros2idl";
       break;
+    default:
+      throw std::runtime_error("switch is not exhaustive");
   }
   out.encoded_message_definition = result;
   out.topic_type = root_topic_type;

rosbag2_cpp/test/rosbag2_cpp/test_local_message_definition_source.cpp

@@ -125,3 +125,14 @@ TEST(test_local_message_definition_source, can_resolve_msg_with_idl_deps)
     "  };\n"
     "};\n");
 }
+
+TEST(test_local_message_definition_source, no_crash_on_bad_name)
+{
+  LocalMessageDefinitionSource source;
+  rosbag2_storage::MessageDefinition result;
+  ASSERT_NO_THROW(
+  {
+    result = source.get_full_text("rosbag2_test_msgdefs/srv/BasicSrv_Request");
+  });
+  ASSERT_EQ(result.encoding, "unknown");
+}

rosbag2_test_msgdefs/CMakeLists.txt

@@ -14,6 +14,7 @@ rosidl_generate_interfaces(${PROJECT_NAME}
   "msg/BasicMsg.msg"
   "msg/ComplexMsg.msg"
   "msg/ComplexMsgDependsOnIdl.msg"
+  "srv/BasicSrv.srv"
   ADD_LINTER_TESTS
 )
 

rosbag2_test_msgdefs/srv/BasicSrv.srv

@@ -0,0 +1,3 @@
+string req
+---
+string resp