From 9638dcc340a83996e88b4e6f4f372572bb33d19c Mon Sep 17 00:00:00 2001 From: Joe Bolinger Date: Wed, 27 Feb 2019 20:26:59 -0800 Subject: [PATCH] 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 +- ruby/tests/test_ruby_package_proto2.proto | 2 +- .../ruby/ruby_generated_pkg_explicit.proto | 9 +++ .../ruby_generated_pkg_explicit_legacy.proto | 9 +++ .../ruby_generated_pkg_explicit_legacy_pb.rb | 20 ++++++ .../ruby/ruby_generated_pkg_explicit_pb.rb | 20 ++++++ .../ruby/ruby_generated_pkg_implicit.proto | 7 ++ .../ruby/ruby_generated_pkg_implicit_pb.rb | 20 ++++++ .../protobuf/compiler/ruby/ruby_generator.cc | 27 ++++++-- .../compiler/ruby/ruby_generator_unittest.cc | 68 ++++++------------- 10 files changed, 131 insertions(+), 53 deletions(-) create mode 100644 src/google/protobuf/compiler/ruby/ruby_generated_pkg_explicit.proto create mode 100644 src/google/protobuf/compiler/ruby/ruby_generated_pkg_explicit_legacy.proto create mode 100644 src/google/protobuf/compiler/ruby/ruby_generated_pkg_explicit_legacy_pb.rb create mode 100644 src/google/protobuf/compiler/ruby/ruby_generated_pkg_explicit_pb.rb create mode 100644 src/google/protobuf/compiler/ruby/ruby_generated_pkg_implicit.proto create mode 100644 src/google/protobuf/compiler/ruby/ruby_generated_pkg_implicit_pb.rb diff --git a/ruby/tests/test_ruby_package.proto b/ruby/tests/test_ruby_package.proto index b872562028..54b7aca50a 100644 --- a/ruby/tests/test_ruby_package.proto +++ b/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 {} diff --git a/ruby/tests/test_ruby_package_proto2.proto b/ruby/tests/test_ruby_package_proto2.proto index 4309044e5f..c55bde435b 100644 --- a/ruby/tests/test_ruby_package_proto2.proto +++ b/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 {} diff --git a/src/google/protobuf/compiler/ruby/ruby_generated_pkg_explicit.proto b/src/google/protobuf/compiler/ruby/ruby_generated_pkg_explicit.proto new file mode 100644 index 0000000000..8d7c948a18 --- /dev/null +++ b/src/google/protobuf/compiler/ruby/ruby_generated_pkg_explicit.proto @@ -0,0 +1,9 @@ +syntax = "proto3"; + +package one.two.a_three; + +option ruby_package = "A::B::C"; + +message Four { + string a_string = 1; +} diff --git a/src/google/protobuf/compiler/ruby/ruby_generated_pkg_explicit_legacy.proto b/src/google/protobuf/compiler/ruby/ruby_generated_pkg_explicit_legacy.proto new file mode 100644 index 0000000000..7a0d260865 --- /dev/null +++ b/src/google/protobuf/compiler/ruby/ruby_generated_pkg_explicit_legacy.proto @@ -0,0 +1,9 @@ +syntax = "proto3"; + +package one.two.a_three.and; + +option ruby_package = "AA.BB.CC"; + +message Four { + string another_string = 1; +} diff --git a/src/google/protobuf/compiler/ruby/ruby_generated_pkg_explicit_legacy_pb.rb b/src/google/protobuf/compiler/ruby/ruby_generated_pkg_explicit_legacy_pb.rb new file mode 100644 index 0000000000..74f3bf3d76 --- /dev/null +++ b/src/google/protobuf/compiler/ruby/ruby_generated_pkg_explicit_legacy_pb.rb @@ -0,0 +1,20 @@ +# 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 diff --git a/src/google/protobuf/compiler/ruby/ruby_generated_pkg_explicit_pb.rb b/src/google/protobuf/compiler/ruby/ruby_generated_pkg_explicit_pb.rb new file mode 100644 index 0000000000..24ff21e174 --- /dev/null +++ b/src/google/protobuf/compiler/ruby/ruby_generated_pkg_explicit_pb.rb @@ -0,0 +1,20 @@ +# 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 diff --git a/src/google/protobuf/compiler/ruby/ruby_generated_pkg_implicit.proto b/src/google/protobuf/compiler/ruby/ruby_generated_pkg_implicit.proto new file mode 100644 index 0000000000..544db64d94 --- /dev/null +++ b/src/google/protobuf/compiler/ruby/ruby_generated_pkg_implicit.proto @@ -0,0 +1,7 @@ +syntax = "proto3"; + +package one.two.a_three; + +message Four { + string a_string = 1; +} diff --git a/src/google/protobuf/compiler/ruby/ruby_generated_pkg_implicit_pb.rb b/src/google/protobuf/compiler/ruby/ruby_generated_pkg_implicit_pb.rb new file mode 100644 index 0000000000..3a1dd67d26 --- /dev/null +++ b/src/google/protobuf/compiler/ruby/ruby_generated_pkg_implicit_pb.rb @@ -0,0 +1,20 @@ +# 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 diff --git a/src/google/protobuf/compiler/ruby/ruby_generator.cc b/src/google/protobuf/compiler/ruby/ruby_generator.cc index c5d9809d49..1079dc319e 100644 --- a/src/google/protobuf/compiler/ruby/ruby_generator.cc +++ b/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); diff --git a/src/google/protobuf/compiler/ruby/ruby_generator_unittest.cc b/src/google/protobuf/compiler/ruby/ruby_generator_unittest.cc index 2e9b2e1290..d93a68d9af 100644 --- a/src/google/protobuf/compiler/ruby/ruby_generator_unittest.cc +++ b/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 +#include #include #include @@ -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