Readable and Writable storage

[Martin-Idel-SI]

This PR separates into readable/writable/general storage. This means that any plugin author only needs to implement any interface and we won't have to check whether methods are supported or not.

The user of librosbag2_storage would then get a storage handle depending on the type of the operations he/she needs to call.

The branch also builds cleanly on all supported platforms.

Not yet part of this branch:

• Using the SerializedBagMessage in the interface
• Topic metadata

Currently, we cannot distinguish between the different "open" for writing/reading/etc.

[wjwwood]

I discussed this with @Karsten1987, and two ideas came up for other ways to organize the API in this pull request. One would be to organize it more like the STL's io library:

https://en.cppreference.com/w/cpp/io

Specifically, it has a ostream and istream which are the base classes of iostream (similar to what you have in this pull request), but importantly, they don't define open. Instead open is defined in the file classes ofstream, ifstream, and fstream, but fstream doesn't inherit from ifstream or ofstream. Here's a diagram of the inheritance that might make it clearer:

https://en.cppreference.com/w/File:std-io-complete-inheritance.svg

This prevents the situation in this pull request where rosbag2_storage::ReadableStorage, rosbag2_storage::WriteableStorage, and rosbag2_storage::Storage all define open, which is... confusing to me.

So you could change this such that there is a "read interface" and a "read implementation" which additionally implements open and related common functions. Then the "storage interface" would inherit from the read and write interfaces (not implementations) and the "storage implementation" would inherit from that and implement the common functions.

---

Another approach, that I suggested and is actually what I think would be best, is using the "policy design pattern" based on templated inheritance. I put together a skeleton of this in a gist at @Karsten1987's request:

https://gist.github.com/wjwwood/7c8bcdc4f7a57633a754c7678d4b06b3#file-rosbag_storage_policy_design-cpp

#include <string>
#include <vector>

struct SerializedMessage
{
  // ...
};

class RosbagIOStorageInterface
{
public:
  virtual
  const char *
  get_open_flags() = 0;
};

class RosbagReadStorageInterface : public RosbagIOStorageInterface
{
public:
  virtual
  SerializedMessage
  read_next_message();

  virtual
  std::vector<std::string>
  list_topics();

  const char *
  get_open_flags() override {return "r";}

  // ...
};

class RosbagWriteStorageInterface : public RosbagIOStorageInterface
{
public:
  virtual
  void
  add_topic(const std::string & topic_name/*, ... */);

  virtual
  void
  write_message(const SerializedMessage & serialized_message_instance);

  const char *
  get_open_flags() override {return "a+";}
};

class RosbagReadWriteStorageInterface
: public RosbagReadStorageInterface,
  public RosbagWriteStorageInterface
{
public:
  const char *
  get_open_flags() override {return RosbagWriteStorageInterface::get_open_flags();}
};

template<typename IOInterfaceT>
class RosbagStorageInterface : public IOInterfaceT
{
public:
  RosbagStorageInterface()
  {
    static_assert(
      std::is_base_of<RosbagIOStorageInterface, IOInterfaceT>::value,
      "Must use a class based on RosbagIOStorageInterface with RosbagStorageInterface");
  }

  virtual
  bool
  open(const std::string & bag_uri)
  {
    (void)bag_uri;
    const char * open_flags = this->get_open_flags();
    (void)open_flags;
    return true;
  }
};

void
look_at_bag(RosbagReadStorageInterface * ro_bag_interface)
{
  ro_bag_interface->read_next_message();
}

void
add_to_bag(RosbagWriteStorageInterface * wo_bag_interface)
{
  wo_bag_interface->write_message(SerializedMessage());
}

void
rewrite_bag(RosbagReadWriteStorageInterface * rw_bag_interface)
{
  rw_bag_interface->write_message(rw_bag_interface->read_next_message());
}

int main()
{
  RosbagStorageInterface<RosbagReadStorageInterface> ro_interface;
  ro_interface.open("foo");
  ro_interface.read_next_message();
  // compile error:
  //   no member named 'write_message' in 'RosbagStorageInterface<RosbagReadStorageInterface>'
  // ro_interface.write_message(SerializedMessage());
  look_at_bag(&ro_interface);
  // compile error:
  //   no matching function for call to 'add_to_bag'
  // add_to_bag(&ro_interface);
  // compile error:
  //   no matching function for call to 'rewrite_bag'
  // rewrite_bag(&ro_interface);


  RosbagStorageInterface<RosbagWriteStorageInterface> wo_interface;  // is this necessary?
  wo_interface.open("foo");
  // compile error:
  //   no member named 'read_next_message' in 'RosbagStorageInterface<RosbagWriteStorageInterface>'
  // wo_interface.read_next_message();
  wo_interface.write_message(SerializedMessage());
  // compile error:
  //   no matching function for call to 'look_at_bag'
  // look_at_bag(&wo_interface);
  add_to_bag(&wo_interface);
  // compile error:
  //   no matching function for call to 'rewrite_bag'
  // rewrite_bag(&wo_interface);

  RosbagStorageInterface<RosbagReadWriteStorageInterface> rw_interface;
  rw_interface.open("foo");
  rw_interface.read_next_message();
  rw_interface.write_message(SerializedMessage());
  look_at_bag(&rw_interface);
  add_to_bag(&rw_interface);
  rewrite_bag(&rw_interface);

  // compile error:
  //   base specifier must name a class
  // RosbagStorageInterface<float> wont_compile;

  // compile error:
  //   Must use a class based on RosbagIOStorageInterface with RosbagStorageInterface
  // RosbagStorageInterface<std::string> wont_either;

  return 0;
}

[wjwwood]

Link to policy based design on wikipedia: https://en.wikipedia.org/wiki/Policy-based_design

[anhosi]

I don't think that the policy based design is going to work as we want to load a specific storage plugin (which would be parametrized with policies) during runtime via the pluginlib. This prevents compile time mechanisms.

[Martin-Idel-SI]

We iterated a bit on the storage design. Basically, our suggestion would be:

• We don't actually need a write-only-storage for now, so delete
• Use exceptions instead of return booleans (this just changes the interface slightly)
• Have a method open_readonly for read-only storage and open for general storage (i.e. the general storage interface has to implement both). This is better than only one "open" method for everything and the pluginlib has no problem with it

Since our initial demo was pushed to the branch, I pushed our changes for this, too - i.e. the demo would remain fully functional.

The interface does not yet use the RosbagSerializedMessage. This would be next.

[Karsten1987]

As discussed offline, I am going ahead and merge this branch on the original PR in order to accumulate discussions in a single place.