Fix LoadFromBag assumptions causing C++ exceptions during serialization

[Ryanf55]

Description

• Not all bags have only GridMap messages
• Not all bags have GridMap on the right topic
• Add test for trying to load a grid map from a bag that doesn't contain it on the expected topic
• Add nullptr check on reader messages

Demo

Details

I uploaded a super small bagfile of two chatter topics. It's very small:

ryan@B650-970:~/Dev/ros2_ws/src/grid_map/grid_map_ros$ ll -h resource/double_chatter/
total 56K
drwxrwxr-x 2 ryan ryan 4.0K Feb 16 11:48 ./
drwxrwxr-x 3 ryan ryan 4.0K Feb 16 11:49 ../
-rw-r--r-- 1 ryan ryan  40K Feb 16 00:50 double_chatter.db3
-rw-rw-r-- 1 ryan ryan 4.8K Feb 16 11:48 metadata.yaml

It might be better to construct the bag file on the fly in the test case, but that would take much more time than what I did here.

Issue

Solves #401

[Ryanf55]

Hello @afrixs, can I get a review and approval is this solves your issue?

[afrixs]

Hello @Ryanf55, thank you for looking into this! I'll review and test it on Monday

[Ryanf55]

Hello @Ryanf55, thank you for looking into this! I'll review and test it on Monday

Thanks. If you have any bag files you can share to reproduce your failure, it would be much appreciated. I could cut them down and add a specific test for it.

[afrixs]

Hello, the fix looks good, I'm sending a minimalistic bag file which contains an empty grid map + chatter topic (unfortunately I cannot post the actual bag where I found the problem, since it contains map of a private property of a third person), along with tests to check for a valid and invalid requested topic. Note that it is a good practice to add a succeeding test as well to check that the test is well-written (in your case the chatter1 topic was missing / at the beginning, so it would fail even if there was a GridMap message)

test_multitopic.zip

TEST(RosbagHandling, checkTopicTypes)
{
  // This test validates loadFromBag will reject a topic of the wrong type or
  // non-existing topic and accept a correct topic.

  std::string pathToBag = "test_multitopic";
  string topic_wrong = "/chatter";
  string topic_correct = "/grid_map";
  string topic_non_existing = "/grid_map_non_existing";
  GridMap gridMapOut;
  rcpputils::fs::path dir(pathToBag);

  EXPECT_FALSE(GridMapRosConverter::loadFromBag(pathToBag, topic_wrong, gridMapOut));
  EXPECT_TRUE(GridMapRosConverter::loadFromBag(pathToBag, topic_correct, gridMapOut));
  EXPECT_FALSE(GridMapRosConverter::loadFromBag(pathToBag, topic_non_existing, gridMapOut));
}

Also, there are still some linting/uncrustify errors to fix in your code

 1: /__w/grid_map/grid_map/src/grid_map/grid_map_ros/src/GridMapRosConverter.cpp:724:  Lines should be <= 100 characters long  [whitespace/line_length] [2]
1: /__w/grid_map/grid_map/src/grid_map/grid_map_ros/src/GridMapRosConverter.cpp:727:  Missing space around colon in range-based for loop  [whitespace/forcolon] [2]
1: /__w/grid_map/grid_map/src/grid_map/grid_map_ros/src/GridMapRosConverter.cpp:735:  Lines should be <= 100 characters long  [whitespace/line_length] [2]
1: /__w/grid_map/grid_map/src/grid_map/grid_map_ros/src/GridMapRosConverter.cpp:746:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]

 5: @@ -727 +727 @@
5: -  for (const auto& m: topic_metadata) {
5: +  for (const auto & m: topic_metadata) {
5: @@ -735 +735,2 @@
5: -        "loadFromBag"), "Bagfile does not contain a GridMap message on the expected topic '%s'", topic.c_str());
5: +        "loadFromBag"), "Bagfile does not contain a GridMap message on the expected topic '%s'",
5: +      topic.c_str());
5: @@ -746 +747 @@
5: -    // https://github.com/ANYbotics/grid_map/issues/401 
5: +    // https://github.com/ANYbotics/grid_map/issues/401

[Ryanf55]

Hello, the fix looks good, I'm sending a minimalistic bag file which contains an empty grid map + chatter topic (unfortunately I cannot post the actual bag where I found the problem, since it contains map of a private property of a third person), along with tests to check for a valid and invalid requested topic. Note that it is a good practice to add a succeeding test as well to check that the test is well-written (in your case the chatter1 topic was missing / at the beginning, so it would fail even if there was a GridMap message)

test_multitopic.zip

TEST(RosbagHandling, checkTopicTypes)
{
  // This test validates loadFromBag will reject a topic of the wrong type or
  // non-existing topic and accept a correct topic.

  std::string pathToBag = "test_multitopic";
  string topic_wrong = "/chatter";
  string topic_correct = "/grid_map";
  string topic_non_existing = "/grid_map_non_existing";
  GridMap gridMapOut;
  rcpputils::fs::path dir(pathToBag);

  EXPECT_FALSE(GridMapRosConverter::loadFromBag(pathToBag, topic_wrong, gridMapOut));
  EXPECT_TRUE(GridMapRosConverter::loadFromBag(pathToBag, topic_correct, gridMapOut));
  EXPECT_FALSE(GridMapRosConverter::loadFromBag(pathToBag, topic_non_existing, gridMapOut));
}

Also, there are still some linting/uncrustify errors to fix in your code

 1: /__w/grid_map/grid_map/src/grid_map/grid_map_ros/src/GridMapRosConverter.cpp:724:  Lines should be <= 100 characters long  [whitespace/line_length] [2]
1: /__w/grid_map/grid_map/src/grid_map/grid_map_ros/src/GridMapRosConverter.cpp:727:  Missing space around colon in range-based for loop  [whitespace/forcolon] [2]
1: /__w/grid_map/grid_map/src/grid_map/grid_map_ros/src/GridMapRosConverter.cpp:735:  Lines should be <= 100 characters long  [whitespace/line_length] [2]
1: /__w/grid_map/grid_map/src/grid_map/grid_map_ros/src/GridMapRosConverter.cpp:746:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]

 5: @@ -727 +727 @@
5: -  for (const auto& m: topic_metadata) {
5: +  for (const auto & m: topic_metadata) {
5: @@ -735 +735,2 @@
5: -        "loadFromBag"), "Bagfile does not contain a GridMap message on the expected topic '%s'", topic.c_str());
5: +        "loadFromBag"), "Bagfile does not contain a GridMap message on the expected topic '%s'",
5: +      topic.c_str());
5: @@ -746 +747 @@
5: -    // https://github.com/ANYbotics/grid_map/issues/401 
5: +    // https://github.com/ANYbotics/grid_map/issues/401

I actually tested this code execution through GDB. Because there is only a single bool return from that function, I don't see a way we can test each case without refactoring and adding unit tests for each check. Thoughts?

[Ryanf55]

I've fixed the tests. They pass locally for me, but not in CI. I don't have time to chase it down right now, but as soon as CI is green, I'm happy to merge this. https://github.com/ANYbotics/grid_map/actions/runs/10133926333/job/28019598750?pr=438#step:7:2853

[Ryanf55]

@Mergifyio backport jazzy

[mergify[bot]]

backport jazzy

✅ Backports have been created

• #468 Fix LoadFromBag assumptions causing C++ exceptions during serialization (backport #438) has been created for branch jazzy

[Ryanf55]

@Mergifyio backport iron

[mergify[bot]]

backport iron

✅ Backports have been created

• #469 Fix LoadFromBag assumptions causing C++ exceptions during serialization (backport #438) has been created for branch iron

[Ryanf55]

@Mergifyio backport humble

[mergify[bot]]

backport humble

✅ Backports have been created

• #470 Fix LoadFromBag assumptions causing C++ exceptions during serialization (backport #438) has been created for branch humble