Adding db directory creation to rosbag2_cpp (#450)

* added db directory creation to storage factory Signed-off-by: Marwan Taher <marokhaled99@gmail.com> * moved db directory creation to rosbag2_cpp Signed-off-by: Marwan Taher <marokhaled99@gmail.com> * rasing exception if dir already exists Signed-off-by: Marwan Taher <marokhaled99@gmail.com> * removed dir creation from record.py, added dir creation to sequential_compression_writer and refactored dir creation in sequential_writer Signed-off-by: Marwan Taher <marokhaled99@gmail.com> * fixed failing tests Signed-off-by: Marwan Taher <marokhaled99@gmail.com> * fixing review comments Signed-off-by: Marwan Taher <marokhaled99@gmail.com> * Apply suggestions from code review Co-authored-by: Karsten Knese <Karsten1987@users.noreply.github.com> Signed-off-by: Emerson Knapp <eknapp@amazon.com>
ros2 · Feb 17, 2021 · a33c7c1 · a33c7c1
1 parent 15c41a7
commit a33c7c1
Show file tree

Hide file tree

Showing 5 changed files with 39 additions and 5 deletions.
diff --git a/ros2bag/ros2bag/verb/record.py b/ros2bag/ros2bag/verb/record.py
@@ -18,7 +18,6 @@
 
 from rclpy.qos import InvalidQoSProfileException
 from ros2bag.api import convert_yaml_to_qos_profile
-from ros2bag.api import create_bag_directory
 from ros2bag.api import print_error
 from ros2bag.verb import VerbExtension
 from ros2cli.node import NODE_NAME_PREFIX
@@ -115,8 +114,6 @@ def main(self, *, args):  # noqa: D102
             except (InvalidQoSProfileException, ValueError) as e:
                 return print_error(str(e))
 
-        create_bag_directory(uri)
-
         if args.all:
             # NOTE(hidmic): in merged install workspaces on Windows, Python entrypoint lookups
             #               combined with constrained environments (as imposed by colcon test)

diff --git a/rosbag2_compression/src/rosbag2_compression/sequential_compression_writer.cpp b/rosbag2_compression/src/rosbag2_compression/sequential_compression_writer.cpp
@@ -112,6 +112,21 @@ void SequentialCompressionWriter::open(
     converter_ = std::make_unique<rosbag2_cpp::Converter>(converter_options, converter_factory_);
   }
 
+  rcpputils::fs::path db_path(base_folder_);
+  if (db_path.is_directory()) {
+    std::stringstream error;
+    error << "Database directory already exists (" << db_path.string() <<
+      "), can't overwrite existing database";
+    throw std::runtime_error{error.str()};
+  }
+
+  bool dir_created = rcpputils::fs::create_directories(db_path);
+  if (!dir_created) {
+    std::stringstream error;
+    error << "Failed to create database directory (" << db_path.string() << ").";
+    throw std::runtime_error{error.str()};
+  }
+
   const auto storage_uri = format_storage_uri(base_folder_, 0);
   storage_ = storage_factory_->open_read_write(storage_uri, storage_options.storage_id);
   if (!storage_) {

diff --git a/rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp b/rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp
@@ -98,6 +98,21 @@ void SequentialWriter::open(
     converter_ = std::make_unique<Converter>(converter_options, converter_factory_);
   }
 
+  rcpputils::fs::path db_path(base_folder_);
+  if (db_path.is_directory()) {
+    std::stringstream error;
+    error << "Database direcotory already exists (" << db_path.string() <<
+      "), can't overwrite existing database";
+    throw std::runtime_error{error.str()};
+  }
+
+  bool dir_created = rcpputils::fs::create_directories(db_path);
+  if (!dir_created) {
+    std::stringstream error;
+    error << "Failed to create database direcotory (" << db_path.string() << ").";
+    throw std::runtime_error{error.str()};
+  }
+
   const auto storage_uri = format_storage_uri(base_folder_, 0);
 
   storage_ = storage_factory_->open_read_write(storage_uri, storage_options.storage_id);

diff --git a/rosbag2_cpp/test/rosbag2_cpp/test_sequential_writer.cpp b/rosbag2_cpp/test/rosbag2_cpp/test_sequential_writer.cpp
@@ -47,6 +47,9 @@ class SequentialWriterTest : public Test
     storage_options_ = rosbag2_cpp::StorageOptions{};
     storage_options_.uri = "uri";
 
+    rcpputils::fs::path dir(storage_options_.uri);
+    rcpputils::fs::remove_all(dir);
+
     ON_CALL(*storage_factory_, open_read_write(_, _)).WillByDefault(
       DoAll(
         Invoke(
@@ -59,6 +62,12 @@ class SequentialWriterTest : public Test
       *storage_factory_, open_read_write(_, _)).Times(AtLeast(0));
   }
 
+  ~SequentialWriterTest()
+  {
+    rcpputils::fs::path dir(storage_options_.uri);
+    rcpputils::fs::remove_all(dir);
+  }
+
   std::unique_ptr<StrictMock<MockStorageFactory>> storage_factory_;
   std::shared_ptr<NiceMock<MockStorage>> storage_;
   std::shared_ptr<StrictMock<MockConverterFactory>> converter_factory_;

diff --git a/rosbag2_tests/test/rosbag2_tests/test_rosbag2_cpp_api.cpp b/rosbag2_tests/test/rosbag2_tests/test_rosbag2_cpp_api.cpp
@@ -43,8 +43,6 @@ TEST(TestRosbag2CPPAPI, minimal_writer_example)
   auto rosbag_directory = rcpputils::fs::path("test_rosbag2_writer_api_bag");
   // in case the bag was previously not cleaned up
   rcpputils::fs::remove_all(rosbag_directory);
-  // See https://github.com/ros2/rosbag2/issues/448
-  rcpputils::fs::create_directories(rosbag_directory);
 
   rosbag2_cpp::StorageOptions storage_options;
   storage_options.uri = rosbag_directory.string();