From f46396508422f3a126d41d66cc98deda7331f404 Mon Sep 17 00:00:00 2001 From: Joe Bolinger Date: Tue, 13 Aug 2019 21:36:25 -0700 Subject: [PATCH] Allow :: in ruby_package --- src/compiler/ruby_generator.cc | 14 +--- src/compiler/ruby_generator_string-inl.h | 23 +++++- .../grpc/testing/package_options_import.proto | 22 +++++ .../testing/package_options_ruby_style.proto | 34 ++++++++ .../spec/pb/codegen/package_option_spec.rb | 82 ++++++++++++------- 5 files changed, 133 insertions(+), 42 deletions(-) create mode 100644 src/ruby/spec/pb/codegen/grpc/testing/package_options_import.proto create mode 100644 src/ruby/spec/pb/codegen/grpc/testing/package_options_ruby_style.proto diff --git a/src/compiler/ruby_generator.cc b/src/compiler/ruby_generator.cc index e39d8be5d41..2a71aae32bb 100644 --- a/src/compiler/ruby_generator.cc +++ b/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 = - RubyTypeOf(method->input_type()->full_name(), package); + grpc::string input_type = RubyTypeOf(method->input_type(), package); if (method->client_streaming()) { input_type = "stream(" + input_type + ")"; } - grpc::string output_type = - RubyTypeOf(method->output_type()->full_name(), package); + grpc::string output_type = RubyTypeOf(method->output_type(), 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; - - if (file->options().has_ruby_package()) { - package_name = file->options().ruby_package(); - } else { - package_name = file->package(); - } + std::string package_name = RubyPackage(file); // Write out a file header. std::map header_comment_vars = ListToDict({ diff --git a/src/compiler/ruby_generator_string-inl.h b/src/compiler/ruby_generator_string-inl.h index ecfe796e7a2..7d6b50a516d 100644 --- a/src/compiler/ruby_generator_string-inl.h +++ b/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) { diff --git a/src/ruby/spec/pb/codegen/grpc/testing/package_options_import.proto b/src/ruby/spec/pb/codegen/grpc/testing/package_options_import.proto new file mode 100644 index 00000000000..2205d21b83e --- /dev/null +++ b/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 { } diff --git a/src/ruby/spec/pb/codegen/grpc/testing/package_options_ruby_style.proto b/src/ruby/spec/pb/codegen/grpc/testing/package_options_ruby_style.proto new file mode 100644 index 00000000000..4bfe61e2f63 --- /dev/null +++ b/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) { } +} diff --git a/src/ruby/spec/pb/codegen/package_option_spec.rb b/src/ruby/spec/pb/codegen/package_option_spec.rb index 0ebd503d79d..870f53ef297 100644 --- a/src/ruby/spec/pb/codegen/package_option_spec.rb +++ b/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'] - 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.', - 'grpc/testing/package_options.proto', - "--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) - expect { Grpc::Testing::Package::Options::TestService::Service }.to raise_error(NameError) - expect(require('grpc/testing/package_options_services_pb')).to be_truthy - expect { Grpc::Testing::Package::Options::TestService::Service }.to_not raise_error - ensure - $LOAD_PATH.delete(tmp_dir) - end + with_protos(%w[grpc/testing/package_options.proto]) do + expect { Grpc::Testing::Package::Options::TestService::Service }.to raise_error(NameError) + expect(require('grpc/testing/package_options_services_pb')).to be_truthy + expect { Grpc::Testing::Package::Options::TestService::Service }.to_not raise_error + expect { Grpc::Testing::TestService::Service }.to raise_error(NameError) + end + end + + it 'should generate and respect Ruby style package options' do + with_protos(%w[grpc/testing/package_options_ruby_style.proto grpc/testing/package_options_import.proto]) do + expect { RPC::Test::New::Package::Options::AnotherTestService::Service }.to raise_error(NameError) + expect(require('grpc/testing/package_options_ruby_style_services_pb')).to be_truthy + expect { RPC::Test::New::Package::Options::AnotherTestService::Service }.to_not raise_error + expect { Grpc::Testing::AnotherTestService::Service }.to raise_error(NameError) + + services = RPC::Test::New::Package::Options::AnotherTestService::Service.rpc_descs + expect(services[:GetTest].input).to eq(RPC::Test::New::Package::Options::AnotherTestRequest) + expect(services[:GetTest].output).to eq(RPC::Test::New::Package::Options::AnotherTestResponse) + expect(services[:OtherTest].input).to eq(A::Other::Thing) + expect(services[:OtherTest].output).to eq(A::Other::Thing) + expect(services[:FooTest].input).to eq(RPC::Test::New::Package::Options::Foo) + expect(services[:FooTest].output).to eq(RPC::Test::New::Package::Options::Foo) + end + end +end + +def with_protos(file_paths) + fail 'CONFIG env variable unexpectedly unset' unless ENV['CONFIG'] + 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