Move the special case logic to the file preprocessor, rather than on …

…open, so as not to affect all the existing rosbag2_cpp tests

Signed-off-by: Emerson Knapp <[email protected]>

rosbag2_compression/src/rosbag2_compression/sequential_compression_reader.cpp

@@ -20,6 +20,7 @@
 #include <utility>
 #include <vector>
 
+#include "rcpputils/asserts.hpp"
 #include "rcpputils/filesystem_helper.hpp"
 
 #include "rosbag2_compression/compression_options.hpp"
@@ -62,6 +63,30 @@ void SequentialCompressionReader::setup_decompression()
 void SequentialCompressionReader::preprocess_current_file()
 {
   setup_decompression();
+
+  if (metadata_.version == 4) {
+    /*
+     * Rosbag2 was released with incorrect relative file naming for compressed bags
+     * which were written as v4, using v3 logic which had the bag name prefixed on the file path.
+     * Because we have no way to check whether the bag was written correctly,
+     * check for the existence of the prefixed file as a fallback.
+     */
+    rcpputils::fs::path base{base_folder_};
+    rcpputils::fs::path relative{get_current_file()};
+    auto resolved = base / relative;
+    if (!resolved.exists()) {
+      auto base_stripped = relative.filename();
+      auto resolved_stripped = base / base_stripped;
+      ROSBAG2_COMPRESSION_LOG_DEBUG_STREAM(
+        "Unable to find specified bagfile " << resolved.string() <<
+          ". Falling back to checking for " << resolved_stripped.string());
+      rcpputils::require_true(
+        resolved_stripped.exists(),
+        "Unable to resolve relative file path either as a V3 or V4 relative path");
+      *current_file_iterator_ = resolved_stripped.string();
+    }
+  }
+
   if (compression_mode_ == CompressionMode::FILE) {
     ROSBAG2_COMPRESSION_LOG_INFO_STREAM("Decompressing " << get_current_file().c_str());
     *current_file_iterator_ = decompressor_->decompress_uri(get_current_file());

rosbag2_compression/test/rosbag2_compression/test_sequential_compression_reader.cpp

@@ -57,9 +57,10 @@ class SequentialCompressionReaderTest : public Test
     message->topic_name = topic_with_type_.name;
 
     metadata_ = construct_default_bag_metadata();
-    ON_CALL(*metadata_io_, read_metadata).WillByDefault([this](auto /*uri*/) {
-      return metadata_;
-    });
+    ON_CALL(*metadata_io_, read_metadata).WillByDefault(
+      [this](auto /*uri*/) {
+        return metadata_;
+      });
     ON_CALL(*metadata_io_, metadata_file_exists(_)).WillByDefault(Return(true));
 
     ON_CALL(*storage_, get_all_topics_and_types()).WillByDefault(Return(topics_and_types));
@@ -97,11 +98,12 @@ class SequentialCompressionReaderTest : public Test
   std::unique_ptr<rosbag2_compression::SequentialCompressionReader> create_reader()
   {
     auto decompressor = std::make_unique<NiceMock<MockDecompressor>>();
-    ON_CALL(*decompressor, decompress_uri).WillByDefault([](auto uri) {
-      auto path = rcpputils::fs::path(uri);
-      EXPECT_TRUE(path.exists());
-      return rcpputils::fs::remove_extension(path).string();
-    });
+    ON_CALL(*decompressor, decompress_uri).WillByDefault(
+      [](auto uri) {
+        auto path = rcpputils::fs::path(uri);
+        EXPECT_TRUE(path.exists());
+        return rcpputils::fs::remove_extension(path).string();
+      });
     auto compression_factory = std::make_unique<NiceMock<MockCompressionFactory>>();
     ON_CALL(*compression_factory, create_decompressor(_))
     .WillByDefault(Return(ByMove(std::move(decompressor))));
@@ -283,5 +285,4 @@ TEST_F(SequentialCompressionReaderTest, can_find_prefixed_filenames_in_renamed_b
   // Expect that this does not raise an exception - because the files can be found
   reader->open(storage_options_, converter_options_);
   EXPECT_TRUE(reader->has_next_file());
-
 }

rosbag2_cpp/include/rosbag2_cpp/readers/sequential_reader.hpp

@@ -148,6 +148,9 @@ class ROSBAG2_CPP_PUBLIC SequentialReader
   std::vector<std::string> file_paths_{};  // List of database files.
   std::vector<std::string>::iterator current_file_iterator_{};  // Index of file to read from
 
+  // Hang on to this because storage_options_ is mutated to point at individual files
+  std::string base_folder_;
+
 private:
   rosbag2_storage::StorageOptions storage_options_;
   std::shared_ptr<SerializationFormatConverterFactoryInterface> converter_factory_{};

rosbag2_cpp/src/rosbag2_cpp/readers/sequential_reader.cpp

@@ -50,32 +50,8 @@ std::vector<std::string> resolve_relative_paths(
     if (path.is_absolute()) {
       continue;
     }
-    if (version == 4) {
-      /*
-       * Rosbag2 was released with incorrect relative file naming for compressed bags
-       * which were written called v4, using v3 logic: "- base/bagfile" instead of "- bagfile"
-       * Because we have no way to check whether the bag was written correctly,
-       * check for the existence of the prefixed file as a fallback.
-       */
-      auto resolved = base_path / path;
-      if (resolved.exists()) {
-        file = resolved.string();
-      } else {
-        auto base_stripped = path.filename();
-        auto resolved_stripped = base_path / base_stripped;
-        ROSBAG2_CPP_LOG_DEBUG_STREAM(
-          "Unable to find specified bagfile " << resolved.string() <<
-            ". Falling back to checking for " << resolved_stripped.string());
-        rcpputils::require_true(
-          resolved_stripped.exists(),
-          "Unable to resolve relative file path either as a V3 or V4 relative path");
-        file = resolved_stripped.string();
-      }
-    } else {
-      file = (base_path / path).string();
-    }
+    file = (base_path / path).string();
   }
-
   return relative_files;
 }
 }  // namespace details
@@ -107,6 +83,7 @@ void SequentialReader::open(
   const ConverterOptions & converter_options)
 {
   storage_options_ = storage_options;
+  base_folder_ = storage_options.uri;
 
   // If there is a metadata.yaml file present, load it.
   // If not, let's ask the storage with the given URI for its metadata.