-
Notifications
You must be signed in to change notification settings - Fork 15.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement service & method descriptor lookup in Ruby #15817
Implement service & method descriptor lookup in Ruby #15817
Conversation
ruby/tests/service_test.rb
Outdated
# one doesn't do this). | ||
omit('JRuby implementation cannot display option extensions') if defined?(::JRUBY_VERSION) | ||
|
||
options_hash = JSON.parse(@test_service.options.to_json) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the actual utility of this for the original use-case in #14891 (getting RPC options) might be somewhat dubious without support for being able to get message extensions (without resorting to #to_json
). I guess that can be tackled in a subsequent PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can get extensions now, though the API is slightly awkward:
protobuf/ruby/tests/basic_proto2.rb
Lines 260 to 351 in eb70b34
def test_extension | |
message = TestExtensions.new | |
extension = Google::Protobuf::DescriptorPool.generated_pool.lookup 'basic_test_proto2.optional_int32_extension' | |
assert_instance_of Google::Protobuf::FieldDescriptor, extension | |
assert_equal 0, extension.get(message) | |
extension.set message, 42 | |
assert_equal 42, extension.get(message) | |
end | |
def test_extension_json | |
omit "Java Protobuf JsonFormat does not handle Proto2 extensions" if defined? JRUBY_VERSION and :NATIVE == Google::Protobuf::IMPLEMENTATION | |
message = TestExtensions.decode_json '{"[basic_test_proto2.optional_int32_extension]": 123}' | |
extension = Google::Protobuf::DescriptorPool.generated_pool.lookup 'basic_test_proto2.optional_int32_extension' | |
assert_instance_of Google::Protobuf::FieldDescriptor, extension | |
assert_equal 123, extension.get(message) | |
end | |
def test_extension_json_separate_pool | |
omit "Java Protobuf JsonFormat does not handle Proto2 extensions" if defined? JRUBY_VERSION and :NATIVE == Google::Protobuf::IMPLEMENTATION | |
pool = Google::Protobuf::DescriptorPool.new | |
# This serialized descriptor is a subset of basic_test_proto2.proto: | |
# | |
# syntax = "proto2"; | |
# package basic_test_proto2; | |
# | |
# message TestExtensions { | |
# extensions 1 to max; | |
# } | |
# | |
# extend TestExtensions { | |
# # Same extension as basic_test_proto2.proto, but with a different | |
# # name. | |
# optional int32 different_optional_int32_extension = 1; | |
# } | |
# | |
descriptor_data = "\n\x17\x62\x61sic_test_proto2.proto\x12\x11\x62\x61sic_test_proto2\"\x1a\n\x0eTestExtensions*\x08\x08\x01\x10\x80\x80\x80\x80\x02:M\n\"different_optional_int32_extension\x12!.basic_test_proto2.TestExtensions\x18\x01 \x01(\x05" | |
pool.add_serialized_file(descriptor_data) | |
message_class = pool.lookup("basic_test_proto2.TestExtensions").msgclass | |
extension = pool.lookup 'basic_test_proto2.different_optional_int32_extension' | |
message = message_class.decode_json '{"[basic_test_proto2.different_optional_int32_extension]": 123}' | |
assert_equal 123, extension.get(message) | |
message2 = message_class.decode_json(message_class.encode_json(message)) | |
assert_equal 123, extension.get(message2) | |
end | |
def test_nested_extension | |
message = TestExtensions.new | |
extension = Google::Protobuf::DescriptorPool.generated_pool.lookup 'basic_test_proto2.TestNestedExtension.test' | |
assert_instance_of Google::Protobuf::FieldDescriptor, extension | |
assert_equal 'test', extension.get(message) | |
extension.set message, 'another test' | |
assert_equal 'another test', extension.get(message) | |
end | |
def test_message_set_extension_json_roundtrip | |
omit "Java Protobuf JsonFormat does not handle Proto2 extensions" if defined? JRUBY_VERSION and :NATIVE == Google::Protobuf::IMPLEMENTATION | |
message = TestMessageSet.new | |
ext1 = Google::Protobuf::DescriptorPool.generated_pool.lookup 'basic_test_proto2.TestMessageSetExtension1.message_set_extension' | |
assert_instance_of Google::Protobuf::FieldDescriptor, ext1 | |
ext2 = Google::Protobuf::DescriptorPool.generated_pool.lookup 'basic_test_proto2.TestMessageSetExtension2.message_set_extension' | |
assert_instance_of Google::Protobuf::FieldDescriptor, ext2 | |
ext3 = Google::Protobuf::DescriptorPool.generated_pool.lookup 'basic_test_proto2.message_set_extension3' | |
assert_instance_of Google::Protobuf::FieldDescriptor, ext3 | |
ext1.set(message, ext1.subtype.msgclass.new(i: 42)) | |
ext2.set(message, ext2.subtype.msgclass.new(str: 'foo')) | |
ext3.set(message, ext3.subtype.msgclass.new(text: 'bar')) | |
message_text = message.to_json | |
parsed_message = TestMessageSet.decode_json message_text | |
assert_equal message, parsed_message | |
end | |
def test_message_set_extension_roundtrip | |
message = TestMessageSet.new | |
ext1 = Google::Protobuf::DescriptorPool.generated_pool.lookup 'basic_test_proto2.TestMessageSetExtension1.message_set_extension' | |
assert_instance_of Google::Protobuf::FieldDescriptor, ext1 | |
ext2 = Google::Protobuf::DescriptorPool.generated_pool.lookup 'basic_test_proto2.TestMessageSetExtension2.message_set_extension' | |
assert_instance_of Google::Protobuf::FieldDescriptor, ext2 | |
ext3 = Google::Protobuf::DescriptorPool.generated_pool.lookup 'basic_test_proto2.message_set_extension3' | |
assert_instance_of Google::Protobuf::FieldDescriptor, ext3 | |
ext1.set(message, ext1.subtype.msgclass.new(i: 42)) | |
ext2.set(message, ext2.subtype.msgclass.new(str: 'foo')) | |
ext3.set(message, ext3.subtype.msgclass.new(text: 'bar')) | |
encoded_message = TestMessageSet.encode message | |
decoded_message = TestMessageSet.decode encoded_message | |
assert_equal message, decoded_message | |
end | |
end | |
end |
This sounds great; I haven't taken a look at the code yet, but from the description this sounds like just the right approach.
The upb code is now in this repo, and the old repo is defunct. We should probably make the banner at the top of the upb readme bigger and more emphatic about this point. |
ruby/src/main/java/com/google/protobuf/jruby/RubyServiceDescriptor.java
Outdated
Show resolved
Hide resolved
ruby/src/main/java/com/google/protobuf/jruby/RubyMethodDescriptor.java
Outdated
Show resolved
Hide resolved
The FFI tests are currently failing with errors like:
Similar changes as you've already made in |
I think they should be made in the |
553117e
to
7fb3f10
Compare
ruby/ext/google/protobuf_c/defs.c
Outdated
rb_define_method(klass, "each_method", ServiceDescriptor_each_method, 0); | ||
rb_define_method(klass, "file_descriptor", ServiceDescriptor_file_descriptor, 0); | ||
// n.b. this shadows Object#methods, which seems less than ideal? | ||
rb_define_method(klass, "methods", ServiceDescriptor_methods, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do want to point out that this shadows Object#methods
, which is a bit icky. Perhaps we should rename this #rpc_methods
or #service_methods
or #method_descriptors
or some such?
How does this happen? I made the changes in the
For now I regenerated the amalgamation, then copied it into |
ruby/ext/google/protobuf_c/defs.c
Outdated
* call-seq: | ||
* ServiceDescriptor.methods => Enumerator | ||
* | ||
* Returns an enumerator over all the methods in this service |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears that none of our other descriptor types have methods that return an enumerator. For consistency, I'd prefer to start with just each
or each_method
. If we want to add methods returning enumerators later, let's do it consistently across all the descriptor types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to add methods returning enumerators later, let's do it consistently across all the descriptor types.
So some of our other descriptor types (including plain message Descriptor
) are enumerables though (they include the Enumerable mixin), which is more or less the same thing. So you can do
descriptor.each { |field| puts "have field #{field}" }
but also things like
descriptor.to_a
# or
descriptor.select { |field| field.type == 'int32' }
I didn't implement ServiceDescriptor#each
, however, because I don't think a service is-a collection of RPC methods in the same way that a message type is-a collection of fields. So instead, I implemented #each_method
, and thus implemented #methods
to return an enumerable in the same way that self
is an enumerator for a message descriptor.
If you like I could just implement #each_method
as ServiecDescriptor#each
instead, if you think service descriptor is-a list of RPC methods. (as a bonus that would solve the #methods
method shadowing Object#methods
too, since we wouldn't need that since self
is the enumerable then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point, that "service is-a collection of methods" is a bit of a stretch. But methods are the only thing you can list inside a service {}
block in a .proto
file, other than options, which have their own accessor. The fact that this conveniently resolves the #methods
shadowing is another argument in its favor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alright 👍 the current version of my PR just implements #each
/Enumerable
directly on ServiceDescriptor
. Thanks for talking through that with me.
ruby/ext/google/protobuf_c/defs.c
Outdated
rb_define_method(klass, "name", ServiceDescriptor_name, 0); | ||
rb_define_method(klass, "each_method", ServiceDescriptor_each_method, 0); | ||
rb_define_method(klass, "file_descriptor", ServiceDescriptor_file_descriptor, 0); | ||
// n.b. this shadows Object#methods, which seems less than ideal? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, shadowing an existing method would be better to avoid if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll rename #each_method
to #each
and include Enumerable
then, which is how the other descriptors list their children. Then I don't need this method at all.
@@ -11420,7 +11420,7 @@ const upb_FieldDef* upb_DefPool_FindExtensionByNumber(const upb_DefPool* s, | |||
const upb_MessageDef* m, | |||
int32_t fieldnum); | |||
|
|||
const upb_ServiceDef* upb_DefPool_FindServiceByName(const upb_DefPool* s, | |||
UPB_API const upb_ServiceDef* upb_DefPool_FindServiceByName(const upb_DefPool* s, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll want to update these in
protobuf/upb/reflection/def_pool.h
Line 69 in 3ab1276
const upb_ServiceDef* upb_DefPool_FindServiceByName(const upb_DefPool* s, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did that as well; i'll drop the commit which updates these vendored copies.
The Ruby service descriptor & method descriptor implementation requires these methods; export them so that they're callable from the Ruby FFI implementation.
7fb3f10
to
90a955a
Compare
@haberman OK, I dropped the each_method/to_enum stuff in favour of just implementing |
@haberman anything i can do to move this along? |
This is exactly what I need :) I'm looking at generating Rails routes from proto options mirroring what grpc-gateway does (so basically allowing us to have two different processes, one running grpc, one running REST, both funneling to the same handler). I'm trying to write a protoc plugin in Ruby (I have one working in Go, but it means that whoever uses it would need a cumbersome "download binary and put in your path" workflow - I'd rather do it in pure Ruby). Right now this is impossible because a) I can't get the original method protos since I can't call |
Sorry for the delay. I think there is still an unresolved comment from me in the code. In the tests you have a comment saying that there is no API for getting an extension, but that isn't true -- see the other tests that get and set extensions. There's no generated code for it yet, but you can lookup an extension in the descriptor pool and then call |
This commit implements suppot for looking up Service descriptors in the global descriptor pool, and from there looking up the associated methods. This is implemented in each of the C extension, JRuby extension, and the FFI implementation. The descriptors can be used to look up the options on the service/methods, and also the request/response types and streaming flags of methods.
90a955a
to
54d7218
Compare
Oh, apologies, I didn't see that. Thanks for the tip about how to use extensions from Ruby. It should be good to go now. |
…#15817) This PR implements lookup of service descriptor and method descriptor objects in Ruby as described in issue protocolbuffers#14891. It contains three implementations - one for the CRuby extension API, one for JRuby, and one for FFI. With this patch, * `DescriptorPool#lookup('fully.qualified.service.name')` works and returns a `Google::Protobuf::ServiceDescriptor` object * You can call `#options` on that to get the service options * You can call `#methods` on that to get the services' methods as `Google::Protobuf::MethodDescriptor` objects, * You can call `MethodDescriptor#options` to get method options * You can also get the streaming flags & input/output types of the method with `#input_type`, `#output_type`, `#client_streaming`, and `#server_streaming`. In order to make the FFI implementation work, I had to mark some more methods in the UPB header as exported - I guess that's something which will have to be done on the UPB side, like this protocolbuffers/upb@01fed1c CC @dazuma & @haberman from the original issue, and @JasonLunn (since you work on protobuf it seems - small world!) I apologies for the large volume of copy-pasta'd code from the existing descriptor class implementations into the new ones - I felt this was probably better than designing new abstractions to reduce it off the bat though; this feels like it "fits in" with the existing implementation. Closes protocolbuffers#15817 COPYBARA_INTEGRATE_REVIEW=protocolbuffers#15817 from KJTsanaktsidis:ktsanaktsidis/add_service_method_descriptors 54d7218 PiperOrigin-RevId: 618221016
`DescriptorPool.lookup` also returns `ServiceDescriptor` since protocolbuffers#15817.
`DescriptorPool#lookup` also returns `ServiceDescriptor` since protocolbuffers#15817.
`DescriptorPool#lookup` returns `ServiceDescriptor` too since protocolbuffers#15817.
This PR implements lookup of service descriptor and method descriptor objects in Ruby as described in issue #14891.
It contains three implementations - one for the CRuby extension API, one for JRuby, and one for FFI.
With this patch,
DescriptorPool#lookup('fully.qualified.service.name')
works and returns aGoogle::Protobuf::ServiceDescriptor
object#options
on that to get the service options#methods
on that to get the services' methods asGoogle::Protobuf::MethodDescriptor
objects,MethodDescriptor#options
to get method options#input_type
,#output_type
,#client_streaming
, and#server_streaming
.In order to make the FFI implementation work, I had to mark some more methods in the UPB header as exported - I guess that's something which will have to be done on the UPB side, like this protocolbuffers/upb@01fed1c
CC @dazuma & @haberman from the original issue, and @JasonLunn (since you work on protobuf it seems - small world!)
I apologies for the large volume of copy-pasta'd code from the existing descriptor class implementations into the new ones - I felt this was probably better than designing new abstractions to reduce it off the bat though; this feels like it "fits in" with the existing implementation.