protobuf icon indicating copy to clipboard operation
protobuf copied to clipboard

Implement service & method descriptor lookup in Ruby

Open KJTsanaktsidis opened this issue 4 months ago • 6 comments

This PR implements lookup of service descriptor and method descriptor objects in Ruby as described in issue https://github.com/protocolbuffers/protobuf/issues/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 https://github.com/protocolbuffers/upb/commit/01fed1cc1ba255bf22b49393ba054b8d270e6ba3

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.

KJTsanaktsidis avatar Feb 13 '24 10:02 KJTsanaktsidis

This sounds great; I haven't taken a look at the code yet, but from the description this sounds like just the right approach.

that's something which will have to be done on the UPB side

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.

haberman avatar Feb 13 '24 15:02 haberman

The FFI tests are currently failing with errors like:

Caught exception Function 'upb_MethodDef_Service' not found in [/usr/local/rvm/gems/ruby-2.7.0/gems/google-protobuf-4.27.0/ext/google/protobuf_c/x86_64-linux/libprotobuf_c_ffi.so] while loading FFI implementation of google/protobuf. Falling back to native implementation.

Similar changes as you've already made in ruby-upb.h to apply the UPB_API macro may also needed in ruby-upb.c.

JasonLunn avatar Feb 13 '24 19:02 JasonLunn

Similar changes as you've already made in ruby-upb.h to apply the UPB_API macro may also needed in ruby-upb.c.

I think they should be made in the upb tree instead. Those changes should then automatically propagate to ruby-upb.c.

haberman avatar Feb 13 '24 19:02 haberman

I think they should be made in the upb tree instead. Those changes should then automatically propagate to ruby-upb.c.

How does this happen? I made the changes in the upb/ tree and regenerated the almagamation with bazel build //upb:gen_ruby_amalgamation; however, that just updates the amalgamation in the bazel-bin/ build directory:

% bazel build //upb:gen_ruby_amalgamation
INFO: Analyzed target //upb:gen_ruby_amalgamation (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
Target //upb:gen_ruby_amalgamation up-to-date:
  bazel-bin/upb/ruby-upb.c
  bazel-bin/upb/ruby-upb.h
INFO: Elapsed time: 0.288s, Critical Path: 0.01s
INFO: 1 process: 1 internal.
INFO: Build completed successfully, 1 total action

For now I regenerated the amalgamation, then copied it into ruby/ext and checked it in to this PR, but if there's some automatic process which is supposed to address that I can drop https://github.com/protocolbuffers/protobuf/pull/15817/commits/c93913c140f76cb8cb51afa4c3c44efa243ee716

KJTsanaktsidis avatar Feb 13 '24 23:02 KJTsanaktsidis

but if there's some automatic process which is supposed to address that I can drop c93913c

Yes, there is an automated process that will regenerate the files; for example see: https://github.com/protocolbuffers/protobuf/commit/a3c33a87c1f24a7aa082317257a9ee297b4d883d

haberman avatar Feb 14 '24 17:02 haberman

@haberman OK, I dropped the each_method/to_enum stuff in favour of just implementing #each and including Enumerable like the other descriptor types with children. WDYT?

KJTsanaktsidis avatar Feb 15 '24 04:02 KJTsanaktsidis

@haberman anything i can do to move this along?

KJTsanaktsidis avatar Feb 26 '24 01:02 KJTsanaktsidis

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 .methodson the service descriptor, and b) I can't access the options I need even if I could.

dorner avatar Mar 04 '24 01:03 dorner

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 extension.get(msg) and extension.set(msg).

haberman avatar Mar 04 '24 17:03 haberman

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.

KJTsanaktsidis avatar Mar 05 '24 21:03 KJTsanaktsidis