Merge pull request #19940 from jbolinger/fix-rubyname-gen

Allow Ruby style (::) package names in Ruby codegen

src/compiler/ruby_generator.cc

@@ -40,13 +40,11 @@ namespace {
 // Prints out the method using the ruby gRPC DSL.
 void PrintMethod(const MethodDescriptor* method, const grpc::string& package,
                  Printer* out) {
-  grpc::string input_type =
+  grpc::string input_type = RubyTypeOf(method->input_type(), package);
-      RubyTypeOf(method->input_type()->full_name(), package);
   if (method->client_streaming()) {
     input_type = "stream(" + input_type + ")";
   }
-  grpc::string output_type =
+  grpc::string output_type = RubyTypeOf(method->output_type(), package);
-      RubyTypeOf(method->output_type()->full_name(), package);
   if (method->server_streaming()) {
     output_type = "stream(" + output_type + ")";
   }
@@ -160,13 +158,7 @@ grpc::string GetServices(const FileDescriptor* file) {
       return output;
     }
 
-    std::string package_name;
+    std::string package_name = RubyPackage(file);
-
-    if (file->options().has_ruby_package()) {
-      package_name = file->options().ruby_package();
-    } else {
-      package_name = file->package();
-    }
 
     // Write out a file header.
     std::map<grpc::string, grpc::string> header_comment_vars = ListToDict({

src/compiler/ruby_generator_string-inl.h

@@ -100,10 +100,29 @@ inline grpc::string Modularize(grpc::string s) {
   return new_string;
 }
 
+// RubyPackage gets the ruby package in either proto or ruby_package format
+inline grpc::string RubyPackage(const grpc::protobuf::FileDescriptor* file) {
+  grpc::string package_name = file->package();
+  if (file->options().has_ruby_package()) {
+    package_name = file->options().ruby_package();
+
+    // If :: is in the package convert the Ruby formated name
+    //    -> A::B::C
+    // to use the dot seperator notation
+    //    -> A.B.C
+    package_name = ReplaceAll(package_name, "::", ".");
+  }
+  return package_name;
+}
+
 // RubyTypeOf updates a proto type to the required ruby equivalent.
-inline grpc::string RubyTypeOf(const grpc::string& a_type,
+inline grpc::string RubyTypeOf(const grpc::protobuf::Descriptor* descriptor,
                                const grpc::string& package) {
-  grpc::string res(a_type);
+  std::string proto_type = descriptor->full_name();
+  if (descriptor->file()->options().has_ruby_package()) {
+    proto_type = RubyPackage(descriptor->file()) + "." + descriptor->name();
+  }
+  grpc::string res(proto_type);
   ReplacePrefix(&res, package, "");  // remove the leading package if present
   ReplacePrefix(&res, ".", "");      // remove the leading . (no package)
   if (res.find('.') == grpc::string::npos) {

src/ruby/spec/pb/codegen/grpc/testing/package_options_import.proto

@@ -0,0 +1,22 @@
+// Copyright 2019 gRPC authors.
+//
+// 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.
+
+syntax = "proto3";
+
+package grpc.testing;
+
+// For sanity checking package definitions
+option ruby_package = "A::Other";
+
+message Thing { }

src/ruby/spec/pb/codegen/grpc/testing/package_options_ruby_style.proto

@@ -0,0 +1,34 @@
+// Copyright 2019 gRPC authors.
+//
+// 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.
+
+syntax = "proto3";
+
+package grpc.testing;
+
+import "grpc/testing/package_options_import.proto";
+
+// For sanity checking package definitions
+option ruby_package = "RPC::Test::New::Package::Options";
+
+message AnotherTestRequest { }
+
+message AnotherTestResponse { }
+
+message Foo { }
+
+service AnotherTestService {
+  rpc GetTest(AnotherTestRequest) returns (AnotherTestResponse) { }
+  rpc OtherTest(Thing) returns (Thing) { }
+  rpc FooTest(Foo) returns (Foo) { }
+}

src/ruby/spec/pb/codegen/package_option_spec.rb

@@ -18,35 +18,59 @@ require 'tmpdir'
 
 describe 'Code Generation Options' do
   it 'should generate and respect package options' do
-    fail 'CONFIG env variable unexpectedly unset' unless ENV['CONFIG']
+    with_protos(%w[grpc/testing/package_options.proto]) do
-    bins_sub_dir = ENV['CONFIG']
+      expect { Grpc::Testing::Package::Options::TestService::Service }.to raise_error(NameError)
-
+      expect(require('grpc/testing/package_options_services_pb')).to be_truthy
-    pb_dir = File.dirname(__FILE__)
+      expect { Grpc::Testing::Package::Options::TestService::Service }.to_not raise_error
-    bins_dir = File.join('..', '..', '..', '..', '..', 'bins', bins_sub_dir)
+      expect { Grpc::Testing::TestService::Service }.to raise_error(NameError)
-
+    end
-    plugin = File.join(bins_dir, 'grpc_ruby_plugin')
+  end
-    protoc = File.join(bins_dir, 'protobuf', 'protoc')
+
-
+  it 'should generate and respect Ruby style package options' do
-    # Generate the service from the proto
+    with_protos(%w[grpc/testing/package_options_ruby_style.proto grpc/testing/package_options_import.proto]) do
-    Dir.mktmpdir(nil, File.dirname(__FILE__)) do |tmp_dir|
+      expect { RPC::Test::New::Package::Options::AnotherTestService::Service }.to raise_error(NameError)
-      gen_file = system(protoc,
+      expect(require('grpc/testing/package_options_ruby_style_services_pb')).to be_truthy
-                        '-I.',
+      expect { RPC::Test::New::Package::Options::AnotherTestService::Service }.to_not raise_error
-                        'grpc/testing/package_options.proto',
+      expect { Grpc::Testing::AnotherTestService::Service }.to raise_error(NameError)
-                        "--grpc_out=#{tmp_dir}", # generate the service
+
-                        "--ruby_out=#{tmp_dir}", # generate the definitions
+      services = RPC::Test::New::Package::Options::AnotherTestService::Service.rpc_descs
-                        "--plugin=protoc-gen-grpc=#{plugin}",
+      expect(services[:GetTest].input).to eq(RPC::Test::New::Package::Options::AnotherTestRequest)
-                        chdir: pb_dir,
+      expect(services[:GetTest].output).to eq(RPC::Test::New::Package::Options::AnotherTestResponse)
-                        out: File::NULL)
+      expect(services[:OtherTest].input).to eq(A::Other::Thing)
-
+      expect(services[:OtherTest].output).to eq(A::Other::Thing)
-      expect(gen_file).to be_truthy
+      expect(services[:FooTest].input).to eq(RPC::Test::New::Package::Options::Foo)
-      begin
+      expect(services[:FooTest].output).to eq(RPC::Test::New::Package::Options::Foo)
-        $LOAD_PATH.push(tmp_dir)
+    end
-        expect { Grpc::Testing::Package::Options::TestService::Service }.to raise_error(NameError)
+  end
-        expect(require('grpc/testing/package_options_services_pb')).to be_truthy
+end
-        expect { Grpc::Testing::Package::Options::TestService::Service }.to_not raise_error
+
-      ensure
+def with_protos(file_paths)
-        $LOAD_PATH.delete(tmp_dir)
+  fail 'CONFIG env variable unexpectedly unset' unless ENV['CONFIG']
-      end
+  bins_sub_dir = ENV['CONFIG']
+
+  pb_dir = File.dirname(__FILE__)
+  bins_dir = File.join('..', '..', '..', '..', '..', 'bins', bins_sub_dir)
+
+  plugin = File.join(bins_dir, 'grpc_ruby_plugin')
+  protoc = File.join(bins_dir, 'protobuf', 'protoc')
+
+  # Generate the service from the proto
+  Dir.mktmpdir(nil, File.dirname(__FILE__)) do |tmp_dir|
+    gen_file = system(protoc,
+                      '-I.',
+                      *file_paths,
+                      "--grpc_out=#{tmp_dir}", # generate the service
+                      "--ruby_out=#{tmp_dir}", # generate the definitions
+                      "--plugin=protoc-gen-grpc=#{plugin}",
+                      chdir: pb_dir,
+                      out: File::NULL)
+
+    expect(gen_file).to be_truthy
+    begin
+      $LOAD_PATH.push(tmp_dir)
+      yield
+    ensure
+      $LOAD_PATH.delete(tmp_dir)
     end
   end
 end