resolve relative file paths (#345)

* resolve relative file paths Signed-off-by: Karsten Knese <[email protected]> * remove subfolder in version4 Signed-off-by: Karsten Knese <[email protected]> * Update rosbag2_cpp/test/rosbag2_cpp/test_multifile_reader.cpp Co-Authored-By: Zachary Michaels <[email protected]> * Update rosbag2_cpp/src/rosbag2_cpp/readers/sequential_reader.cpp Co-Authored-By: Zachary Michaels <[email protected]> * address review comments Signed-off-by: Karsten Knese <[email protected]> * adopt rosbag2_tests Signed-off-by: Karsten Knese <[email protected]> * fix windows (hopefully) Signed-off-by: Karsten Knese <[email protected]> * uncrustify Signed-off-by: Karsten Knese <[email protected]> * fix compilation Signed-off-by: Karsten Knese <[email protected]> * another try on windows Signed-off-by: Karsten Knese <[email protected]> * windows try no 3 Signed-off-by: Karsten Knese <[email protected]> * adopt sequentialreader to version4 Signed-off-by: Karsten Knese <[email protected]> * windows no 4 Signed-off-by: Karsten Knese <[email protected]> * windows no 5 Signed-off-by: Karsten Knese <[email protected]> * windows no 6 Signed-off-by: Karsten Knese <[email protected]> Co-authored-by: Zachary Michaels <[email protected]>
ros2 · Apr 3, 2020 · 59c90c8 · 59c90c8
1 parent e656e76
commit 59c90c8
Show file tree

Hide file tree

Showing 6 changed files with 184 additions and 39 deletions.
diff --git a/rosbag2_cpp/src/rosbag2_cpp/readers/sequential_reader.cpp b/rosbag2_cpp/src/rosbag2_cpp/readers/sequential_reader.cpp
@@ -18,6 +18,7 @@
 #include <utility>
 #include <vector>
 
+#include "rcpputils/asserts.hpp"
 #include "rcpputils/filesystem_helper.hpp"
 
 #include "rosbag2_cpp/logging.hpp"
@@ -27,6 +28,33 @@ namespace rosbag2_cpp
 {
 namespace readers
 {
+namespace details
+{
+std::vector<std::string> resolve_relative_paths(
+  const std::string & base_folder, std::vector<std::string> relative_files, const int version = 4)
+{
+  auto base_path = rcpputils::fs::path(base_folder);
+  if (version < 4) {
+    // In older rosbags (version <=3) relative files are prefixed with the rosbag folder name
+    base_path = rcpputils::fs::path(base_folder).parent_path();
+  }
+
+  rcpputils::require_true(
+    base_path.exists(), "base folder does not exist: " + base_folder);
+  rcpputils::require_true(
+    base_path.is_directory(), "base folder has to be a directory: " + base_folder);
+
+  for (auto & file : relative_files) {
+    auto path = rcpputils::fs::path(file);
+    if (path.is_absolute()) {
+      continue;
+    }
+    file = (base_path / path).string();
+  }
+
+  return relative_files;
+}
+}  // namespace details
 
 SequentialReader::SequentialReader(
   std::unique_ptr<rosbag2_storage::StorageFactoryInterface> storage_factory,
@@ -63,15 +91,15 @@ void SequentialReader::open(
       return;
     }
 
-    file_paths_ = metadata_.relative_file_paths;
+    file_paths_ = details::resolve_relative_paths(
+      storage_options.uri, metadata_.relative_file_paths, metadata_.version);
     current_file_iterator_ = file_paths_.begin();
 
     storage_ = storage_factory_->open_read_only(
       get_current_file(), storage_options.storage_id);
     if (!storage_) {
       throw std::runtime_error{"No storage could be initialized. Abort"};
     }
-
   } else {
     storage_ = storage_factory_->open_read_only(
       storage_options.uri, storage_options.storage_id);

diff --git a/rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp b/rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp
@@ -43,6 +43,12 @@ std::string format_storage_uri(const std::string & base_folder, uint64_t storage
 
   return (rcpputils::fs::path(base_folder) / storage_file_name.str()).string();
 }
+
+std::string strip_parent_path(const std::string & relative_path)
+{
+  return rcpputils::fs::path(relative_path).filename().string();
+}
+
 }  // namespace
 
 SequentialWriter::SequentialWriter(
@@ -70,7 +76,7 @@ void SequentialWriter::init_metadata()
   metadata_.storage_identifier = storage_->get_storage_identifier();
   metadata_.starting_time = std::chrono::time_point<std::chrono::high_resolution_clock>(
     std::chrono::nanoseconds::max());
-  metadata_.relative_file_paths = {storage_->get_relative_file_path()};
+  metadata_.relative_file_paths = {strip_parent_path(storage_->get_relative_file_path())};
 }
 
 void SequentialWriter::open(
@@ -178,7 +184,7 @@ void SequentialWriter::split_bagfile()
     throw std::runtime_error(errmsg.str());
   }
 
-  metadata_.relative_file_paths.push_back(storage_->get_relative_file_path());
+  metadata_.relative_file_paths.push_back(strip_parent_path(storage_->get_relative_file_path()));
 
   // Re-register all topics since we rolled-over to a new bagfile.
   for (const auto & topic : topics_names_to_info_) {

diff --git a/rosbag2_cpp/test/rosbag2_cpp/test_multifile_reader.cpp b/rosbag2_cpp/test/rosbag2_cpp/test_multifile_reader.cpp
@@ -19,6 +19,8 @@
 #include <utility>
 #include <vector>
 
+#include "rcpputils/filesystem_helper.hpp"
+
 #include "rosbag2_cpp/reader.hpp"
 #include "rosbag2_cpp/readers/sequential_reader.hpp"
 
@@ -39,76 +41,169 @@ class MultifileReaderTest : public Test
   : storage_(std::make_shared<NiceMock<MockStorage>>()),
     converter_factory_(std::make_shared<StrictMock<MockConverterFactory>>()),
     storage_serialization_format_("rmw1_format"),
+    storage_uri_(rcpputils::fs::temp_directory_path().string()),
     relative_path_1_("some_relative_path_1"),
-    relative_path_2_("some_relative_path_2")
+    relative_path_2_("some_relative_path_2"),
+    absolute_path_1_((rcpputils::fs::path(storage_uri_) / "some/folder").string()),
+    default_storage_options_({storage_uri_, ""})
+  {}
+
+  virtual void init()
   {
+    auto metadata = get_metadata();
+
     auto topic_with_type = rosbag2_storage::TopicMetadata{
       "topic", "test_msgs/BasicTypes", storage_serialization_format_, ""};
     auto topics_and_types = std::vector<rosbag2_storage::TopicMetadata>{topic_with_type};
+    metadata.topics_with_message_count.push_back({topic_with_type, 10});
 
     auto message = std::make_shared<rosbag2_storage::SerializedBagMessage>();
     message->topic_name = topic_with_type.name;
 
+    auto storage_factory = std::make_unique<StrictMock<MockStorageFactory>>();
     auto metadata_io = std::make_unique<NiceMock<MockMetadataIo>>();
-    rosbag2_storage::BagMetadata metadata;
-    metadata.relative_file_paths.push_back(relative_path_1_);
-    metadata.relative_file_paths.push_back(relative_path_2_);
-    metadata.topics_with_message_count.push_back({topic_with_type, 10});
     ON_CALL(*metadata_io, read_metadata(_)).WillByDefault(Return(metadata));
     EXPECT_CALL(*metadata_io, metadata_file_exists(_)).WillRepeatedly(Return(true));
 
-    auto storage_factory = std::make_unique<StrictMock<MockStorageFactory>>();
     EXPECT_CALL(*storage_, get_all_topics_and_types())
     .Times(AtMost(1)).WillRepeatedly(Return(topics_and_types));
     ON_CALL(*storage_, read_next()).WillByDefault(Return(message));
     EXPECT_CALL(*storage_factory, open_read_only(_, _)).WillRepeatedly(Return(storage_));
 
     auto sequential_reader = std::make_unique<rosbag2_cpp::readers::SequentialReader>(
       std::move(storage_factory), converter_factory_, std::move(metadata_io));
-
     reader_ = std::make_unique<rosbag2_cpp::Reader>(std::move(sequential_reader));
   }
 
+  virtual ~MultifileReaderTest() = default;
+
+  virtual rosbag2_storage::BagMetadata get_metadata() const
+  {
+    rosbag2_storage::BagMetadata metadata;
+
+    metadata.relative_file_paths =
+    {relative_path_1_, relative_path_2_, absolute_path_1_};
+
+    return metadata;
+  }
+
   std::shared_ptr<NiceMock<MockStorage>> storage_;
   std::shared_ptr<StrictMock<MockConverterFactory>> converter_factory_;
   std::unique_ptr<rosbag2_cpp::Reader> reader_;
   std::string storage_serialization_format_;
+  std::string storage_uri_;
   std::string relative_path_1_;
   std::string relative_path_2_;
+  std::string absolute_path_1_;
+  rosbag2_cpp::StorageOptions default_storage_options_;
+};
+
+class MultifileReaderTestVersion3 : public MultifileReaderTest
+{
+public:
+  MultifileReaderTestVersion3()
+  : MultifileReaderTest()
+  {
+    relative_path_1_ = "rosbag_name/some_relative_path_1";
+    relative_path_2_ = "rosbag_name/some_relative_path_2";
+    absolute_path_1_ = (rcpputils::fs::path(storage_uri_) / "some/folder").string();
+  }
+
+  rosbag2_storage::BagMetadata get_metadata() const override
+  {
+    auto metadata = MultifileReaderTest::get_metadata();
+    metadata.version = 3;
+
+    return metadata;
+  }
 };
 
 TEST_F(MultifileReaderTest, has_next_reads_next_file)
 {
+  init();
+
   // storage::has_next() is called twice when reader::has_next() is called
-  EXPECT_CALL(*storage_, has_next()).Times(4)
+  EXPECT_CALL(*storage_, has_next()).Times(6)
   .WillOnce(Return(true)).WillOnce(Return(true))  // We have a message
   .WillOnce(Return(false))  // No message, load next file
+  .WillOnce(Return(true))  // True since we now have a message
+  .WillOnce(Return(false))  // No message, load next file
+  .WillOnce(Return(true));  // True since we now have a message
+  EXPECT_CALL(*converter_factory_, load_deserializer(storage_serialization_format_)).Times(0);
+  EXPECT_CALL(*converter_factory_, load_serializer(storage_serialization_format_)).Times(0);
+  reader_->open(default_storage_options_, {"", storage_serialization_format_});
+
+  auto & sr = static_cast<rosbag2_cpp::readers::SequentialReader &>(
+    reader_->get_implementation_handle());
+
+  auto resolved_relative_path_1 =
+    (rcpputils::fs::path(storage_uri_) / relative_path_1_).string();
+  auto resolved_relative_path_2 =
+    (rcpputils::fs::path(storage_uri_) / relative_path_2_).string();
+  auto resolved_absolute_path_1 =
+    rcpputils::fs::path(absolute_path_1_).string();
+  EXPECT_EQ(sr.get_current_file(), resolved_relative_path_1);
+  reader_->has_next();
+  reader_->read_next();
+  reader_->has_next();
+  EXPECT_EQ(sr.get_current_file(), resolved_relative_path_2);
+  reader_->read_next();
+  reader_->has_next();
+  EXPECT_EQ(sr.get_current_file(), resolved_absolute_path_1);
+}
+
+TEST_F(MultifileReaderTestVersion3, has_next_reads_next_file_version3)
+{
+  init();
+
+  // storage::has_next() is called twice when reader::has_next() is called
+  EXPECT_CALL(*storage_, has_next()).Times(6)
+  .WillOnce(Return(true)).WillOnce(Return(true))  // We have a message
+  .WillOnce(Return(false))  // No message, load next file
+  .WillOnce(Return(true))  // True since we now have a message
+  .WillOnce(Return(false))  // No message, load next file
   .WillOnce(Return(true));  // True since we now have a message
   EXPECT_CALL(*converter_factory_, load_deserializer(storage_serialization_format_)).Times(0);
   EXPECT_CALL(*converter_factory_, load_serializer(storage_serialization_format_)).Times(0);
-  reader_->open(rosbag2_cpp::StorageOptions(), {"", storage_serialization_format_});
+  reader_->open(default_storage_options_, {"", storage_serialization_format_});
 
   auto & sr = static_cast<rosbag2_cpp::readers::SequentialReader &>(
     reader_->get_implementation_handle());
 
-  EXPECT_EQ(sr.get_current_file(), relative_path_1_);
+  // Legacy version <=3 have a parent_path() prefixed in the relative files
+  auto resolved_relative_path_1 =
+    (rcpputils::fs::path(storage_uri_).parent_path() / relative_path_1_).string();
+  auto resolved_relative_path_2 =
+    (rcpputils::fs::path(storage_uri_).parent_path() / relative_path_2_).string();
+  auto resolved_absolute_path_1 =
+    rcpputils::fs::path(absolute_path_1_).string();
+  EXPECT_EQ(sr.get_current_file(), resolved_relative_path_1);
+  reader_->has_next();
+  reader_->read_next();
   reader_->has_next();
+  EXPECT_EQ(sr.get_current_file(), resolved_relative_path_2);
   reader_->read_next();
   reader_->has_next();
-  EXPECT_EQ(sr.get_current_file(), relative_path_2_);
+  EXPECT_EQ(sr.get_current_file(), resolved_absolute_path_1);
 }
 
 TEST_F(MultifileReaderTest, has_next_throws_if_no_storage)
 {
+  init();
+
   EXPECT_ANY_THROW(reader_->has_next());
 }
 
 TEST_F(MultifileReaderTest, read_next_throws_if_no_storage)
 {
+  init();
+
   EXPECT_ANY_THROW(reader_->read_next());
 }
 
 TEST_F(MultifileReaderTest, get_all_topics_and_types_throws_if_no_storage)
 {
+  init();
+
   EXPECT_ANY_THROW(reader_->get_all_topics_and_types());
 }
diff --git a/rosbag2_cpp/test/rosbag2_cpp/test_sequential_reader.cpp b/rosbag2_cpp/test/rosbag2_cpp/test_sequential_reader.cpp
@@ -19,6 +19,8 @@
 #include <utility>
 #include <vector>
 
+#include "rcpputils/filesystem_helper.hpp"
+
 #include "rosbag2_cpp/readers/sequential_reader.hpp"
 #include "rosbag2_cpp/reader.hpp"
 
@@ -40,7 +42,9 @@ class SequentialReaderTest : public Test
   SequentialReaderTest()
   : storage_(std::make_shared<NiceMock<MockStorage>>()),
     converter_factory_(std::make_shared<StrictMock<MockConverterFactory>>()),
-    storage_serialization_format_("rmw1_format")
+    storage_serialization_format_("rmw1_format"),
+    storage_uri_(rcpputils::fs::temp_directory_path().string()),
+    default_storage_options_({storage_uri_, ""})
   {
     rosbag2_storage::TopicMetadata topic_with_type;
     topic_with_type.name = "topic";
@@ -51,29 +55,39 @@ class SequentialReaderTest : public Test
     auto message = std::make_shared<rosbag2_storage::SerializedBagMessage>();
     message->topic_name = topic_with_type.name;
 
+    auto relative_file_path =
+      (rcpputils::fs::path(storage_uri_) / "some/folder").string();
     auto storage_factory = std::make_unique<StrictMock<MockStorageFactory>>();
     auto metadata_io = std::make_unique<NiceMock<MockMetadataIo>>();
     rosbag2_storage::BagMetadata metadata;
-    metadata.relative_file_paths = {"/path/to/storage"};
+    metadata.relative_file_paths = {relative_file_path};
     metadata.topics_with_message_count.push_back({{topic_with_type}, 1});
+
     EXPECT_CALL(*metadata_io, read_metadata(_)).WillRepeatedly(Return(metadata));
     EXPECT_CALL(*metadata_io, metadata_file_exists(_)).WillRepeatedly(Return(true));
 
     EXPECT_CALL(*storage_, get_all_topics_and_types())
     .Times(AtMost(1)).WillRepeatedly(Return(topics_and_types));
     EXPECT_CALL(*storage_, read_next()).WillRepeatedly(Return(message));
-    EXPECT_CALL(*storage_factory, open_read_only(_, _)).WillOnce(Return(storage_));
+
+    EXPECT_CALL(*storage_factory, open_read_only(_, _));
+    ON_CALL(*storage_factory, open_read_only).WillByDefault(
+      [this, relative_file_path](const std::string & path, const std::string & /* storage_id */) {
+        EXPECT_STREQ(relative_file_path.c_str(), path.c_str());
+        return storage_;
+      });
 
     auto sequential_reader = std::make_unique<rosbag2_cpp::readers::SequentialReader>(
       std::move(storage_factory), converter_factory_, std::move(metadata_io));
-
     reader_ = std::make_unique<rosbag2_cpp::Reader>(std::move(sequential_reader));
   }
 
   std::shared_ptr<NiceMock<MockStorage>> storage_;
   std::shared_ptr<StrictMock<MockConverterFactory>> converter_factory_;
   std::unique_ptr<rosbag2_cpp::Reader> reader_;
   std::string storage_serialization_format_;
+  std::string storage_uri_;
+  rosbag2_cpp::StorageOptions default_storage_options_;
 };
 
 TEST_F(SequentialReaderTest, read_next_uses_converters_to_convert_serialization_format) {
@@ -89,7 +103,7 @@ TEST_F(SequentialReaderTest, read_next_uses_converters_to_convert_serialization_
   EXPECT_CALL(*converter_factory_, load_serializer(output_format))
   .WillOnce(Return(ByMove(std::move(format2_converter))));
 
-  reader_->open(rosbag2_cpp::StorageOptions(), {"", output_format});
+  reader_->open(default_storage_options_, {"", output_format});
   reader_->read_next();
 }
 
@@ -102,7 +116,7 @@ TEST_F(SequentialReaderTest, open_throws_error_if_converter_plugin_does_not_exis
   EXPECT_CALL(*converter_factory_, load_serializer(output_format))
   .WillOnce(Return(ByMove(nullptr)));
 
-  EXPECT_ANY_THROW(reader_->open(rosbag2_cpp::StorageOptions(), {"", output_format}));
+  EXPECT_ANY_THROW(reader_->open(default_storage_options_, {"", output_format}));
 }
 
 TEST_F(
@@ -113,7 +127,7 @@ TEST_F(
   EXPECT_CALL(*converter_factory_, load_deserializer(storage_serialization_format)).Times(0);
   EXPECT_CALL(*converter_factory_, load_serializer(storage_serialization_format)).Times(0);
 
-  reader_->open(rosbag2_cpp::StorageOptions(), {"", storage_serialization_format});
+  reader_->open(default_storage_options_, {"", storage_serialization_format});
   reader_->read_next();
 }
 
@@ -125,7 +139,7 @@ TEST_F(SequentialReaderTest, set_filter_calls_storage) {
   EXPECT_ANY_THROW(reader_->get_implementation_handle().reset_filter());
 
   EXPECT_CALL(*storage_, set_filter(_)).Times(2);
-  reader_->open(rosbag2_cpp::StorageOptions(), {"", storage_serialization_format_});
+  reader_->open(default_storage_options_, {"", storage_serialization_format_});
   reader_->get_implementation_handle().set_filter(storage_filter);
   reader_->read_next();
   storage_filter.topics.clear();

diff --git a/rosbag2_cpp/test/rosbag2_cpp/test_sequential_writer.cpp b/rosbag2_cpp/test/rosbag2_cpp/test_sequential_writer.cpp
@@ -243,8 +243,7 @@ TEST_F(SequentialWriterTest, writer_splits_when_storage_bagfile_size_gt_max_bagf
     static_cast<unsigned int>(expected_splits)) <<
     "Storage should have split bagfile " << (expected_splits - 1);
 
-  const auto base_path =
-    (rcpputils::fs::path(storage_options_.uri) / storage_options_.uri).string();
+  const auto base_path = storage_options_.uri;
   int counter = 0;
   for (const auto & path : fake_metadata_.relative_file_paths) {
     std::stringstream ss;