From b8b429aca71d1ca8548cbeb1ed32714d01974ac9 Mon Sep 17 00:00:00 2001 From: Emerson Knapp Date: Wed, 24 May 2023 14:06:12 -0700 Subject: [PATCH 1/2] Don't fail when type definition cannot be found Signed-off-by: Emerson Knapp --- .../local_message_definition_source.cpp | 33 +++++++++++++++---- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/rosbag2_cpp/src/rosbag2_cpp/message_definitions/local_message_definition_source.cpp b/rosbag2_cpp/src/rosbag2_cpp/message_definitions/local_message_definition_source.cpp index 58085156b0..7fe03db7a9 100644 --- a/rosbag2_cpp/src/rosbag2_cpp/message_definitions/local_message_definition_source.cpp +++ b/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 - #include #include #include @@ -27,8 +25,28 @@ #include +#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,14 +213,15 @@ 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()); } rosbag2_storage::MessageDefinition out; switch (format) { From e93ba869d9b190adb241f99c538b186d2ec87fb0 Mon Sep 17 00:00:00 2001 From: Emerson Knapp Date: Wed, 24 May 2023 14:17:31 -0700 Subject: [PATCH 2/2] Add unit test Signed-off-by: Emerson Knapp --- .../local_message_definition_source.cpp | 7 +++++-- .../test_local_message_definition_source.cpp | 11 +++++++++++ rosbag2_test_msgdefs/CMakeLists.txt | 1 + rosbag2_test_msgdefs/srv/BasicSrv.srv | 3 +++ 4 files changed, 20 insertions(+), 2 deletions(-) create mode 100644 rosbag2_test_msgdefs/srv/BasicSrv.srv diff --git a/rosbag2_cpp/src/rosbag2_cpp/message_definitions/local_message_definition_source.cpp b/rosbag2_cpp/src/rosbag2_cpp/message_definitions/local_message_definition_source.cpp index 7fe03db7a9..74e04f9f01 100644 --- a/rosbag2_cpp/src/rosbag2_cpp/message_definitions/local_message_definition_source.cpp +++ b/rosbag2_cpp/src/rosbag2_cpp/message_definitions/local_message_definition_source.cpp @@ -222,18 +222,21 @@ rosbag2_storage::MessageDefinition LocalMessageDefinitionSource::get_full_text( 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; diff --git a/rosbag2_cpp/test/rosbag2_cpp/test_local_message_definition_source.cpp b/rosbag2_cpp/test/rosbag2_cpp/test_local_message_definition_source.cpp index 37801e6d72..7e7d09ae1d 100644 --- a/rosbag2_cpp/test/rosbag2_cpp/test_local_message_definition_source.cpp +++ b/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"); +} diff --git a/rosbag2_test_msgdefs/CMakeLists.txt b/rosbag2_test_msgdefs/CMakeLists.txt index b36dd02cd9..32199bf446 100644 --- a/rosbag2_test_msgdefs/CMakeLists.txt +++ b/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 ) diff --git a/rosbag2_test_msgdefs/srv/BasicSrv.srv b/rosbag2_test_msgdefs/srv/BasicSrv.srv new file mode 100644 index 0000000000..8a9ec1a75a --- /dev/null +++ b/rosbag2_test_msgdefs/srv/BasicSrv.srv @@ -0,0 +1,3 @@ +string req +--- +string resp