Allow topic and service to construct messages from description files (#…

…428)

The current implementation allows for additional messages to be read from these files, it was just not properly configured for the files to be accessible

Signed-off-by: Michael Carroll <[email protected]>

src/Factory.cc

@@ -25,6 +25,7 @@
 #include <google/protobuf/text_format.h>
 
 #include <cstdlib>
+#include <filesystem>
 #include <fstream>
 #include <functional>
 #include <iostream>
@@ -115,40 +116,54 @@ class DynamicFactory
     // Split all the directories containing .desc files.
     std::vector<std::string> descDirs = split(_paths, ':');
 
-    for (const std::string &descDir : descDirs)
+    auto loadDescFile = [this](const std::string &descFile)
     {
-      for (DirIter dirIter(descDir); dirIter != DirIter(); ++dirIter)
+      // Ignore files without the correct extensions.
+      if (descFile.rfind(".desc") == std::string::npos &&
+          descFile.rfind(".proto") == std::string::npos &&
+          descFile.rfind(".proto.bin") == std::string::npos)
+        return;
+
+      // Parse the .desc file.
+      std::ifstream ifs(descFile);
+      if (!ifs.is_open())
       {
-        // Ignore files without the .desc extension.
-        if ((*dirIter).rfind(".desc") == std::string::npos)
-          continue;
+        std::cerr << "DynamicFactory(): Unable to open [" << descFile << "]"
+                  << std::endl;
+        return;
+      }
 
-        // Parse the .desc file.
-        std::ifstream ifs(*dirIter);
-        if (!ifs.is_open())
-        {
-          std::cerr << "DynamicFactory(): Unable to open [" << *dirIter << "]"
-                    << std::endl;
-          continue;
-        }
+      google::protobuf::FileDescriptorSet fileDescriptorSet;
+      if (!fileDescriptorSet.ParseFromIstream(&ifs))
+      {
+        std::cerr << "DynamicFactory(): Unable to parse descriptor set from ["
+                  << descFile << "]" << std::endl;
+        return;
+      }
 
-        google::protobuf::FileDescriptorSet fileDescriptorSet;
-        if (!fileDescriptorSet.ParseFromIstream(&ifs))
+      // Place the real descriptors in the descriptor pool.
+      for (const google::protobuf::FileDescriptorProto &fileDescriptorProto :
+           fileDescriptorSet.file())
+      {
+        if (!this->pool.BuildFile(fileDescriptorProto))
         {
-          std::cerr << "DynamicFactory(): Unable to parse descriptor set from ["
-                    << *dirIter << "]" << std::endl;
-          continue;
+          std::cerr << "DynamicFactory(). Unable to place descriptors from ["
+                    << descFile << "] in the descriptor pool" << std::endl;
         }
+      }
+    };
 
-        // Place the real descriptors in the descriptor pool.
-        for (const google::protobuf::FileDescriptorProto &fileDescriptorProto :
-             fileDescriptorSet.file())
+    for (const std::string &descDir : descDirs)
+    {
+      if (!std::filesystem::is_directory(descDir))
+      {
+        loadDescFile(descDir);
+      }
+      else
+      {
+        for (DirIter dirIter(descDir); dirIter != DirIter(); ++dirIter)
         {
-          if (!pool.BuildFile(fileDescriptorProto))
-          {
-            std::cerr << "DynamicFactory(). Unable to place descriptors from ["
-                      << *dirIter << "] in the descriptor pool" << std::endl;
-          }
+          loadDescFile(*dirIter);
         }
       }
     }

test/desc/testing.invalid

@@ -0,0 +1,8 @@
+syntax = "proto3";
+
+package testing;
+
+message Bytes
+{
+  bytes data    = 1;
+}

test/desc/testing.proto

@@ -0,0 +1,8 @@
+syntax = "proto3";
+
+package testing;
+
+message Bytes
+{
+  bytes data    = 1;
+}

test/desc/testing.proto.bin

@@ -0,0 +1,16 @@
+
+C
+testing/bytes.prototesting"
+Bytes
+data
 
(Rdatabproto3
+�

+testing/message.prototesting"
+
+FooMessage
+foo
 
(	Rfoo"
+
+BarMessage
+foo
 
(	Rfoo"
+
+BazMessage
+foo
 
(	Rfoobproto3

test/desc/testing/bytes.proto

@@ -0,0 +1,8 @@
+syntax = "proto3";
+
+package testing;
+
+message Bytes
+{
+  bytes data    = 1;
+}

test/desc/testing/message.proto

@@ -0,0 +1,15 @@
+syntax = "proto3";
+
+package testing;
+
+message FooMessage {
+    string data = 1;
+}
+
+message BarMessage {
+    FooMessage foo = 1;
+}
+
+message BazMessage {
+    BarMessage bar = 1;
+}

test/integration/CMakeLists.txt

@@ -6,3 +6,8 @@ if(MSVC)
 endif()
 
 gz_build_tests(TYPE INTEGRATION SOURCES ${tests})
+
+if (TARGET INTEGRATION_descriptors)
+  target_compile_definitions(INTEGRATION_descriptors
+    PRIVATE GZ_MSGS_TEST_PATH="${PROJECT_SOURCE_DIR}/test")
+endif()

test/integration/descriptors.cc

@@ -0,0 +1,95 @@
+/*
+ * Copyright (C) 2024 Open Source Robotics Foundation
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+#include <gtest/gtest.h>
+
+#include <filesystem>
+
+#include "gz/msgs/Factory.hh"
+
+static constexpr const char * kMsgsTestPath = GZ_MSGS_TEST_PATH;
+
+TEST(FactoryTest, DynamicFactory)
+{
+  EXPECT_EQ(nullptr, gz::msgs::Factory::New("example.msgs.StringMsg"));
+  EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.Bytes"));
+  EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.FooMessage"));
+  EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.BarMessage"));
+  EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.BazMessage"));
+
+  // Test loading an invalid path
+  {
+    std::filesystem::path test_path(kMsgsTestPath);
+    std::string paths = (test_path / "desc" / "does_not_exist.desc").string();
+    gz::msgs::Factory::LoadDescriptors(paths);
+  }
+  EXPECT_EQ(nullptr, gz::msgs::Factory::New("example.msgs.StringMsg"));
+  EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.Bytes"));
+  EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.FooMessage"));
+  EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.BarMessage"));
+  EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.BazMessage"));
+
+  // Test loading an incorrect extension
+  {
+    std::filesystem::path test_path(kMsgsTestPath);
+    std::string paths = (test_path / "desc" / "testing.invalid").string();
+    gz::msgs::Factory::LoadDescriptors(paths);
+  }
+  EXPECT_EQ(nullptr, gz::msgs::Factory::New("example.msgs.StringMsg"));
+  EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.Bytes"));
+  EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.FooMessage"));
+  EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.BarMessage"));
+  EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.BazMessage"));
+
+  // Test loading a file with invalid content
+  {
+    std::filesystem::path test_path(kMsgsTestPath);
+    std::string paths = (test_path / "desc" / "testing.proto").string();
+    gz::msgs::Factory::LoadDescriptors(paths);
+  }
+  EXPECT_EQ(nullptr, gz::msgs::Factory::New("example.msgs.StringMsg"));
+  EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.Bytes"));
+  EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.FooMessage"));
+  EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.BarMessage"));
+  EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.BazMessage"));
+
+  // Load a valid descriptor file with one message type
+  {
+    std::filesystem::path test_path(kMsgsTestPath);
+    std::string paths = (test_path / "desc" / "stringmsg.desc").string();
+    gz::msgs::Factory::LoadDescriptors(paths);
+  }
+
+  EXPECT_NE(nullptr, gz::msgs::Factory::New("example.msgs.StringMsg"));
+  EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.Bytes"));
+  EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.FooMessage"));
+  EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.BarMessage"));
+  EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.BazMessage"));
+
+  // Load a directory
+  {
+    std::filesystem::path test_path(kMsgsTestPath);
+    std::string paths = (test_path / "desc").string();
+    gz::msgs::Factory::LoadDescriptors(paths);
+  }
+
+  EXPECT_NE(nullptr, gz::msgs::Factory::New("example.msgs.StringMsg"));
+  EXPECT_NE(nullptr, gz::msgs::Factory::New("testing.Bytes"));
+  EXPECT_NE(nullptr, gz::msgs::Factory::New("testing.FooMessage"));
+  EXPECT_NE(nullptr, gz::msgs::Factory::New("testing.BarMessage"));
+  EXPECT_NE(nullptr, gz::msgs::Factory::New("testing.BazMessage"));
+}