Fix Ruby module name generation when the ruby_package option is used (#5735)

* fix module name generation and add tests

* fix existing tests

* support both package name styles

ruby/tests/test_ruby_package.proto

@@ -2,6 +2,6 @@ syntax = "proto3";
 
 package foo_bar;
 
-option ruby_package = "A.B";
+option ruby_package = "A::B";
 
 message TestRubyPackageMessage {}

ruby/tests/test_ruby_package_proto2.proto

@@ -2,6 +2,6 @@ syntax = "proto2";
 
 package foo_bar_proto2;
 
-option ruby_package = "A.B.Proto2";
+option ruby_package = "A::B::Proto2";
 
 message TestRubyPackageMessage {}

src/google/protobuf/compiler/ruby/ruby_generated_pkg_explicit.proto

+syntax = "proto3";
+
+package one.two.a_three;
+
+option ruby_package = "A::B::C";
+
+message Four {
+  string a_string = 1;
+}

src/google/protobuf/compiler/ruby/ruby_generated_pkg_explicit_legacy.proto

+syntax = "proto3";
+
+package one.two.a_three.and;
+
+option ruby_package = "AA.BB.CC";
+
+message Four {
+  string another_string = 1;
+}

src/google/protobuf/compiler/ruby/ruby_generated_pkg_explicit_legacy_pb.rb

+# Generated by the protocol buffer compiler.  DO NOT EDIT!
+# source: ruby_generated_pkg_explicit_legacy.proto
+
+require 'google/protobuf'
+
+Google::Protobuf::DescriptorPool.generated_pool.build do
+  add_file("ruby_generated_pkg_explicit_legacy.proto", :syntax => :proto3) do
+    add_message "one.two.a_three.and.Four" do
+      optional :another_string, :string, 1
+    end
+  end
+end
+
+module AA
+  module BB
+    module CC
+      Four = Google::Protobuf::DescriptorPool.generated_pool.lookup("one.two.a_three.and.Four").msgclass
+    end
+  end
+end

src/google/protobuf/compiler/ruby/ruby_generated_pkg_explicit_pb.rb

+# Generated by the protocol buffer compiler.  DO NOT EDIT!
+# source: ruby_generated_pkg_explicit.proto
+
+require 'google/protobuf'
+
+Google::Protobuf::DescriptorPool.generated_pool.build do
+  add_file("ruby_generated_pkg_explicit.proto", :syntax => :proto3) do
+    add_message "one.two.a_three.Four" do
+      optional :a_string, :string, 1
+    end
+  end
+end
+
+module A
+  module B
+    module C
+      Four = Google::Protobuf::DescriptorPool.generated_pool.lookup("one.two.a_three.Four").msgclass
+    end
+  end
+end

src/google/protobuf/compiler/ruby/ruby_generated_pkg_implicit.proto

+syntax = "proto3";
+
+package one.two.a_three;
+
+message Four {
+  string a_string = 1;
+}

src/google/protobuf/compiler/ruby/ruby_generated_pkg_implicit_pb.rb

+# Generated by the protocol buffer compiler.  DO NOT EDIT!
+# source: ruby_generated_pkg_implicit.proto
+
+require 'google/protobuf'
+
+Google::Protobuf::DescriptorPool.generated_pool.build do
+  add_file("ruby_generated_pkg_implicit.proto", :syntax => :proto3) do
+    add_message "one.two.a_three.Four" do
+      optional :a_string, :string, 1
+    end
+  end
+end
+
+module One
+  module Two
+    module AThree
+      Four = Google::Protobuf::DescriptorPool.generated_pool.lookup("one.two.a_three.Four").msgclass
+    end
+  end
+end

src/google/protobuf/compiler/ruby/ruby_generator.cc

@@ -416,26 +416,43 @@ int GeneratePackageModules(
     const FileDescriptor* file,
     google::protobuf::io::Printer* printer) {
   int levels = 0;
-  bool need_change_to_module;
+  bool need_change_to_module = true;
   std::string package_name;
 
+  // Determine the name to use in either format:
+  //   proto package:         one.two.three
+  //   option ruby_package:   One::Two::Three
   if (file->options().has_ruby_package()) {
     package_name = file->options().ruby_package();
-    need_change_to_module = false;
+
+    // If :: is in the package use the Ruby formated name as-is
+    //    -> A::B::C
+    // otherwise, use the dot seperator
+    //    -> A.B.C
+    if (package_name.find("::") != std::string::npos) {
+      need_change_to_module = false;
+    } else {
+      GOOGLE_LOG(WARNING) << "ruby_package option should be in the form of:"
+                          << " 'A::B::C' and not 'A.B.C'";
+    }
   } else {
     package_name = file->package();
-    need_change_to_module = true;
   }
 
+  // Use the appropriate delimter
+  string delimiter = need_change_to_module ? "." : "::";
+  int delimiter_size = need_change_to_module ? 1 : 2;
+
+  // Extract each module name and indent
   while (!package_name.empty()) {
-    size_t dot_index = package_name.find(".");
+    size_t dot_index = package_name.find(delimiter);
     string component;
     if (dot_index == string::npos) {
       component = package_name;
       package_name = "";
     } else {
       component = package_name.substr(0, dot_index);
-      package_name = package_name.substr(dot_index + 1);
+      package_name = package_name.substr(dot_index + delimiter_size);
     }
     if (need_change_to_module) {
       component = PackageToModule(component);

src/google/protobuf/compiler/ruby/ruby_generator_unittest.cc

@@ -29,6 +29,7 @@
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 #include <memory>
+#include <list>
 
 #include <google/protobuf/compiler/ruby/ruby_generator.h>
 #include <google/protobuf/compiler/command_line_interface.h>
@@ -56,7 +57,7 @@ string FindRubyTestDir() {
 // Some day, we may integrate build systems between protoc and the language
 // extensions to the point where we can do this test in a more automated way.
 
-TEST(RubyGeneratorTest, Proto3GeneratorTest) {
+void RubyTest(string proto_file) {
   string ruby_tests = FindRubyTestDir();
 
   google::protobuf::compiler::CommandLineInterface cli;
@@ -68,22 +69,23 @@ TEST(RubyGeneratorTest, Proto3GeneratorTest) {
   // Copy generated_code.proto to the temporary test directory.
   string test_input;
   GOOGLE_CHECK_OK(File::GetContents(
-      ruby_tests + "/ruby_generated_code.proto",
+      ruby_tests + proto_file + ".proto",
       &test_input,
       true));
   GOOGLE_CHECK_OK(File::SetContents(
-      TestTempDir() + "/ruby_generated_code.proto",
+      TestTempDir() + proto_file + ".proto",
       test_input,
       true));
 
   // Invoke the proto compiler (we will be inside TestTempDir() at this point).
   string ruby_out = "--ruby_out=" + TestTempDir();
   string proto_path = "--proto_path=" + TestTempDir();
+  string proto_target = TestTempDir() + proto_file + ".proto";
   const char* argv[] = {
     "protoc",
     ruby_out.c_str(),
     proto_path.c_str(),
-    "ruby_generated_code.proto",
+    proto_target.c_str(),
   };
 
   EXPECT_EQ(0, cli.Run(4, argv));
@@ -91,61 +93,35 @@ TEST(RubyGeneratorTest, Proto3GeneratorTest) {
   // Load the generated output and compare to the expected result.
   string output;
   GOOGLE_CHECK_OK(File::GetContentsAsText(
-      TestTempDir() + "/ruby_generated_code_pb.rb",
+      TestTempDir() + proto_file + "_pb.rb",
       &output,
       true));
   string expected_output;
   GOOGLE_CHECK_OK(File::GetContentsAsText(
-      ruby_tests + "/ruby_generated_code_pb.rb",
+      ruby_tests + proto_file + "_pb.rb",
       &expected_output,
       true));
   EXPECT_EQ(expected_output, output);
 }
 
-TEST(RubyGeneratorTest, Proto2GeneratorTest) {
-  string ruby_tests = FindRubyTestDir();
-
-  google::protobuf::compiler::CommandLineInterface cli;
-  cli.SetInputsAreProtoPathRelative(true);
-
-  ruby::Generator ruby_generator;
-  cli.RegisterGenerator("--ruby_out", &ruby_generator, "");
+TEST(RubyGeneratorTest, Proto3GeneratorTest) {
+  RubyTest("/ruby_generated_code");
+}
 
-  // Copy generated_code.proto to the temporary test directory.
-  string test_input;
-  GOOGLE_CHECK_OK(File::GetContents(
-      ruby_tests + "/ruby_generated_code_proto2.proto",
-      &test_input,
-      true));
-  GOOGLE_CHECK_OK(File::SetContents(
-      TestTempDir() + "/ruby_generated_code_proto2.proto",
-      test_input,
-      true));
+TEST(RubyGeneratorTest, Proto2GeneratorTest) {
+    RubyTest("/ruby_generated_code_proto2");
+}
 
-  // Invoke the proto compiler (we will be inside TestTempDir() at this point).
-  string ruby_out = "--ruby_out=" + TestTempDir();
-  string proto_path = "--proto_path=" + TestTempDir();
-  const char* argv[] = {
-    "protoc",
-    ruby_out.c_str(),
-    proto_path.c_str(),
-    "ruby_generated_code_proto2.proto",
-  };
+TEST(RubyGeneratorTest, Proto3ImplicitPackageTest) {
+    RubyTest("/ruby_generated_pkg_implicit");
+}
 
-  EXPECT_EQ(0, cli.Run(4, argv));
+TEST(RubyGeneratorTest, Proto3ExplictPackageTest) {
+    RubyTest("/ruby_generated_pkg_explicit");
+}
 
-  // Load the generated output and compare to the expected result.
-  string output;
-  GOOGLE_CHECK_OK(File::GetContents(
-      TestTempDir() + "/ruby_generated_code_proto2_pb.rb",
-      &output,
-      true));
-  string expected_output;
-  GOOGLE_CHECK_OK(File::GetContents(
-      ruby_tests + "/ruby_generated_code_proto2_pb.rb",
-      &expected_output,
-      true));
-  EXPECT_EQ(expected_output, output);
+TEST(RubyGeneratorTest, Proto3ExplictLegacyPackageTest) {
+    RubyTest("/ruby_generated_pkg_explicit_legacy");
 }
 
 }  // namespace