Skip to content
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

No way to use custom options in Ruby #1198

Closed
kailan opened this issue Jan 29, 2016 · 40 comments
Closed

No way to use custom options in Ruby #1198

kailan opened this issue Jan 29, 2016 · 40 comments

Comments

@kailan
Copy link

kailan commented Jan 29, 2016

I created a custom option extension that I use in my proto files, like this:

service Fishtank {
  rpc GetFish(FishQuery) returns (Fish) {
    option (protapi.http).get = "/fish/{id}";
  }
}

It doesn't seem that once it is compiled into Ruby code, I can access the value of the protapi.http option. I believe this can be done in other languages by accessing the descriptor.

Am I missing something, or is this not possible right now?

@haberman
Copy link
Member

You are correct that Ruby does not yet support custom options. Hopefully we can add this before too long.

@kailan
Copy link
Author

kailan commented Jan 30, 2016

@haberman Awesome! It'll really help out a project of mine.

Any way I can help out with implementing this?

@skipperguy12
Copy link

This would be helpful for me as well

@haberman haberman added the ruby label Feb 10, 2016
@haberman
Copy link
Member

This is becoming a more and more common request. I have been thinking through how the implementation ought to work on this. It's slightly tricky to modify the DSL for this, because custom options let you declare the option and use it both in the same file. This is a challenge to accommodate in a clean way.

@kailan
Copy link
Author

kailan commented Feb 28, 2016

Any progress on this? @haberman

@haberman
Copy link
Member

Ok there are a couple parts to making this work.

First of all we need to support proto2 field presence. Right now the code generator fails immediately if it sees a file with syntax="proto2";. Step 1 would be a PR that supports proto2 files that don't contain any extensions or custom options. Proto2 "optional" fields should be supported by adding hasbits to the in-memory representation to indicate whether a field is present or not. I'm happy to explain this in more detail for anyone who wants to implement this.

Secondly we need to support proto2 extensions. This means extending the DSL with something like add_extension and making it so you can read and write extension fields of extended messages with something like msg[my_extension] = 5.

Finally we need a way of extending the DSL to support custom options. We can talk more about this part once #1 and #2 are done.

I'd be happy to see pull requests for any/all of these, and I'd be happy to help guide people who want to implement this to the right places in the code.

@kailan
Copy link
Author

kailan commented Mar 1, 2016

I'd love to work on this but I have one question: Why would proto2 need to be supported? Surely implementing this for proto3 would be enough.

@haberman
Copy link
Member

haberman commented Mar 1, 2016

Custom options are defined as extensions of messages descriptor.proto, which is a syntax="proto2"; file. Extensions don't exist at all in proto3.

So even if your messages are only proto3, we need proto2 support to be able to represent the options themselves.

@kailan
Copy link
Author

kailan commented Mar 1, 2016

Ah, makes sense. I'll give it a shot later on 👍

@nerdrew
Copy link

nerdrew commented Jan 26, 2017

Any status updates on proto2 support?

@djudd-stripe
Copy link

Any progress since last year? I might have a few days to push this forward.

@nerdrew
Copy link

nerdrew commented Aug 28, 2017

Funny. We were thinking of looking at this soon too. cc @zanker

@zanker
Copy link
Contributor

zanker commented Aug 28, 2017

Hah, @djudd-stripe I was actually talking to @nerdrew about this last week and we assumed that Stripe didn't use protobufs. Are you using the "protobuf" gem currently?

I was able to hack something to support defaults in a few days, so I don't think it's too hard to at least get basic protobuf2 support at least. It looks like most of the work is building out the Ruby <-> C bridges, as UPB handles everything already.

I'm still working on proving this out internally to make sure we can feasibly swap to this, but right now we're interested in adding at least basic synta2 support.

@haberman it sounds like on your end, you would be interested in merging syntax2 support to the Ruby gem, as long as it meets code standards and is done properly?

@djudd-stripe
Copy link

Not a lot of Protobuf at Stripe right now but we are exploring adding more. We're using this code so far but are interested in custom annotations.

@djudd-stripe
Copy link

@zanker it sounds like you have Proto2 support work in-progress? Anything more useful I can do than wait for you to get it merged, then maybe add custom option support on top (unless you've included that already)?

@zanker
Copy link
Contributor

zanker commented Aug 28, 2017

Thanks for the offer, I don't think there's anything right now. I was able to get a more proper one (using hasbits) working, I need a few days to sort it out and clean/test things and I'll submit a PR.

I'm just starting with the syntax2/default parts initially. If you wanted to add custom option support on top of that then 👍 . Will end up doing it otherwise.

@zanker
Copy link
Contributor

zanker commented Aug 30, 2017

@djudd-stripe This is still very much WIP (hence the random debug statements), but for reference: master...zanker:zanker/syntax2 is what things are looking like. I don't think wiring in extensions would be too hard overall.

@pcj
Copy link
Contributor

pcj commented Feb 1, 2018

Any progress on this? AFAIK there is no way to work with descriptor.proto in ruby.

syntax = "proto3";
package foo;
import "google/protobuf/descriptor.proto";

😢

@zanker
Copy link
Contributor

zanker commented Feb 1, 2018

Sorry! I left Square back in October and the team who was going to take it over got reshuffled so they're not going to have time to pick it back up.

The PR is at #3606, and all the proto side code is pretty solid from the testing I did. The blocker was the proto syntax, which wasn't narrowed down until after I had already left.

If you want to pick up that part of the PR, I'd be happy to walk you through on whatever part of the PR isn't clear.

@pechorin
Copy link

pechorin commented Nov 18, 2019

Is where any plans to implement custom options in ruby? Custom options looks great for expressions some meta logic and it is so sad what ruby does not include this abilitites.

My primary interest is extending enum options.

@ebenoist
Copy link
Contributor

ebenoist commented Mar 17, 2020

It looks like there is support in upb.c. What is the thinking around what the syntax would look like? If it's straightforward I can put up a PR.

@pechorin
Copy link

pechorin commented Mar 17, 2020

Custom options currently cannot be compiled via protoc2, but can be compiled via protoc3 but options will be skipped in compiled ruby pb file. So it will be cool to provide meta options like this in ruby output file:

syntax = "proto2";

import "google/protobuf/descriptor.proto";

extend google.protobuf.EnumValueOptions {
   optional string name = 1001;
}
message Operation {

  enum ClassName {
    OPERATION = 0 [(name) = 'Operation'];
    PUSH_OPERATION = 1  [(name) = 'PushOperation'];
  }
}

So, you ask about how it will be implemented in ruby generated classes? Am i understand you right?

@seanhuang-stripe
Copy link

@haberman Hey! I've been following along this thread for a while and I might have a personal interest in taking this up, especially given that you mentioned the underlying foundations are ready. What is the best way to receive some guidance from you to get started on this?

@haberman
Copy link
Member

haberman commented May 6, 2022

@seanhuang-stripe thank you for the interest!

There are two main pieces to supporting custom options:

  1. Extending the generated code to embed a serialized descriptor, so that all custom options are plumbed to the upb layer. The simplest solution to this would be to simply have all protos use GenerateBinaryDescriptor() instead of GenerateDslDescriptor(), eg.
    bool use_raw_descriptor = file->name() == "google/protobuf/descriptor.proto";
    if (use_raw_descriptor) {
    GenerateBinaryDescriptor(file, printer, error);
    } else {
    GenerateDslDescriptor(file, printer, error);
    }
    However that will also make the generated code less readable, since it would no longer use the DSL, which users may object to. I think we could accommodate that readability concern by generating separate RBI files for the generated code. This could get the best of both worlds: small, fast-loading _pb.rb files that preserve all custom options, and larger, readable _pb.rbi files that give IDEs, type checkers, and human readers something that represents the fully-expanded interface. cc @JasonLunn who I've discussed this idea with previously.
  2. Extending the runtime to surface the custom options. This would be reasonably straightforward I think, you could adapt the Python implementation which currently supports this eg. https://github.com/protocolbuffers/upb/blob/0e8772fc20e5a0a2fa1f326c79d494374871ef94/python/descriptor.c#L103-L106 There is a question of whether we should return a new instance each time or whether we should return a shared instance that is frozen, we can figure out those details once the basic implementation is in place.

I would be very happy to review a design doc and PRs for this feature.

@JasonLunn
Copy link
Contributor

Marking this as blocked on #9495. Until we can make the generated code even uglier without degrading human readability (by adopting RBS with will allow us to separate those concerns), we don't want to do the work in point 1 above.

@haberman
Copy link
Member

haberman commented May 2, 2023

This is unblocked now that #10492 is fixed. More info: https://protobuf.dev/news/2023-04-20/

@jsteinberg
Copy link

@haberman I am very interested in Custom Options (and descriptor options) for Ruby. I am even willing to help implement them. I had some time last week and started looking into it.

Note: I haven't written any c code in a quite a while. So my proposed initial solution will be focused mostly on the top level c class in the ruby library defs.c.

Adding support for descriptor options seems relatively strait forward given the helpers in the upb library.

I had a question on what sort of interface would be acceptable for options? There isn't currently a ruby/ubp equivalent for the various options classes, so exposing them from the descriptor classes seems like the easiest lift.

# ruby
d = ::Google::Protobuf::DescriptorPool.generated_pool.lookup("the.greatest.and.best.proto.in.the.world")

d.deprecated?
=> true | false

This interface seems to be a bit different from some of the other languages, but feels more ruby-like to me.

# python-ish
d.getOptions().deprecated

Some of the Options have quite a few fields on them (Looking at you, FileOptions). So if we went with the top level descriptor methods, it could greatly expand the Ruby interface on the descriptors. Given that the underlying Option classes are relatively stable, I personally dont think that is a big concern. But I do think matching the interfaces of the other languages is important.

So, before I go too far down any path, I had a couple of questions:

1. Which interface to options do you think makes the most sense?

  • descriptor.deprecated?
  • descriptor.options.deprecated?
  • descriptor.get_option("deprecated").value (Some sort of Ruby representation of an Option would need to be returned)
  • descriptor.get_option_value("deprecated")

2. If descriptor.options.deprecated? is the preferred route: What would be the correct way to add intermediate Ruby wrapper classes for the google_protobuf_XxxxxxOptions classes?

ubp has nicely defined structs in the reflection API for the currently exposed Ruby classes (ex: Google::Protobuf::Descriptor -> upb_MessageDef, full list). I have personally found these easy to work with when attempting to extend the Ruby interface.

  • Would the correct approach be to add more reflection classes to expose the various descriptor options?
  • Would Ruby wrapper classes wrapping the upb_MiniTable directly make better align with the ubp design?
  • What would the naming convention be for these Ruby wrapper classes.

DescriptorProto -> Descriptor doesn't work as well with the more inner classes (MessageOptions -> ?????)

3. How do you actually expose custom options?
I had trouble figuring this out. It looks like most of the "fields" are looked up by passing a upb_MiniTableField into the various _upb_MiniTable_GetXxxxxField methods. I am a little lost on how that should work with custom options.

There are also the methods FindByFieldNumber and GetFieldByIndex.

Either of these approaches would require returning to Ruby some sort of Option definition or potentially the raw value without type information.

Thanks in advance for any guidance and all the work to make this possible.

@haberman
Copy link
Member

Hi @jsteinberg, thanks for the interest!

Fortunately, I think there is a solution that would be far easier than what you are describing.

Recall that options are just protos. The Ruby Protobuf library already is capable of defining proto message classes without requiring that every field is bound into C manually.

You can already construct a FieldOptions class in Ruby like so:

require 'google/protobuf/descriptor_pb'

field_options = Google::Protobuf::FieldOptions.new

All we need is a way of doing something like this:

# We could add this maybe in ruby/lib/google/protobuf.rb
class Descriptor
  if not @options
    @options = Google::Protobuf::MessageOptions.decode(serialized_options)
    @options.freeze   # We actually need to freeze recursively.
  end
  @options
end

# Now a user can write something like this:
field = ::Google::Protobuf::DescriptorPool.generated_pool.lookup("pkg.Foo")
field.options.deprecated

The only trick is how to get the serialized_options above. In C, it would look something like this:

/*
 * call-seq:
 *     Descriptor.serialized_options => options
 *
 * Returns a binary string containing the serialized options for this message.
 */
static VALUE Descriptor_serialized_options(VALUE _self) {
  Descriptor* self = ruby_to_Descriptor(_self);
  const google_protobuf_MessageOptions* opts = upb_MessageDef_Options(self->msgdef);
  upb_Arena* arena = upb_Arena_New();
  size_t size;
  char* serialized = google_protobuf_MessageOptions_serialize(opts, arena, &size);
  if (serialized) {
    VALUE ret = rb_str_new(serialized, size);
    rb_enc_associate(ret, rb_ascii8bit_encoding());
    upb_Arena_Free(arena);
    return ret;
  } else {
    upb_Arena_Free(arena);
    rb_raise(rb_eRuntimeError, "Error encoding");
  }
}

That's all it should take! You can repeat this for each kind of option.

@jsteinberg
Copy link

jsteinberg commented May 15, 2023

Thank you for the quick response. Also, that way is much simpler. I should have some time this week to finish this up and get a PR submitted.

I'm still struggling with reading the custom options. I'm hoping you can point me in the right direction.

Given the following proto, I am able to get deprecated as you showed.

extend google.protobuf.MessageOptions {
  string custom_message_opt = 7739036;
}

message Event {
  option deprecated = true;
  option (custom_message_opt) = "CUSTOM MESSAGE OPTION";

  string id = 1;
}
d = ::Google::Protobuf::DescriptorPool.generated_pool.lookup("Event")

d.serialized_options
=> "\x18\x01\xE2\xE9\xC2\x1D\x15CUSTOM MESSAGE OPTION" 

# I'll add to the ruby interface like you suggested.
options = Google::Protobuf::MessageOptions.decode(d.serialized_options)
=> <Google::Protobuf::MessageOptions: deprecated: true, uninterpreted_option: []>

options.deprecated
=> true

options.custom_message_opt
NoMethodError: undefined method `custom_message_opt' for <Google::Protobuf::MessageOptions: deprecated: true, uninterpreted_option: []>:Google::Protobuf::MessageOptions

options.class.encode(options)
=> "\x18\x01\xE2\xE9\xC2\x1D\x15CUSTOM MESSAGE OPTION"

Also, I'm happy to PR your suggested implementation WITHOUT custom options to isolate the diff if that is easier. deprecated by itself would be nice for some of our workflows.

Thanks again

@haberman
Copy link
Member

That is a good question. It looks like we haven't added Ruby APIs to support extensions yet. But the good news is, that should be pretty easy to do also at this point.

We should add the following code to DescriptorPool_lookup():

  fielddef = upb_DefPool_FindExtensionByName(self->symtab, name_str);
  if (fielddef) {
    return get_fielddef_obj(_self, enumdef);
  }

Then you could write something like this:

ext = ::Google::Protobuf::DescriptorPool.generated_pool.lookup("custom_message_opt")
ext.get(options)

We might also want to make Message.[] accept a FieldDescriptor, so you could get slightly nicer syntax:

options[ext]

The final thing we would want to do is add these lookup calls to the generated code, so a user could just reference the extension by name, instead of having to call lookup first.

But all of that should probably go into a separate PR, which is to add extensions support to Ruby.

By the way, both PRs will need to go through a round of API review internally. But I expect that the APIs I proposed above should be relatively uncontroversial.

JasonLunn added a commit that referenced this issue Nov 13, 2023
Rewrrte and extension of #12828, with additional work for JRuby. Partially fixes #1198 by adding support for custom options. Handling of extensions will be handled in a follow up.

Also includes these unrelated fixes:
* Removes code echo between `google/protobuf/repeated_field.rb` and `google/protobuf/ffi/repeated_field.rb` by `require`'ing the former in the latter.
* Adds missing calles to `testFrozen()` from methods of `RepeatedField` under JRuby that mutate.
* Various typos in comments.

Closes #14594

COPYBARA_INTEGRATE_REVIEW=#14594 from protocolbuffers:add-support-for-options-in-ruby 16cc9e3
PiperOrigin-RevId: 580848874
zhangskz pushed a commit that referenced this issue Nov 13, 2023
Rewrrte and extension of #12828, with additional work for JRuby. Partially fixes #1198 by adding support for custom options. Handling of extensions will be handled in a follow up.

Also includes these unrelated fixes:
* Removes code echo between `google/protobuf/repeated_field.rb` and `google/protobuf/ffi/repeated_field.rb` by `require`'ing the former in the latter.
* Adds missing calles to `testFrozen()` from methods of `RepeatedField` under JRuby that mutate.
* Various typos in comments.

Closes #14594

COPYBARA_INTEGRATE_REVIEW=#14594 from protocolbuffers:add-support-for-options-in-ruby 16cc9e3
PiperOrigin-RevId: 580848874
copybara-service bot pushed a commit that referenced this issue Nov 14, 2023
Follow up to #14594, which added support for custom options, this PR implements extensions support, which should fully resolve #1198.

Closes #14703

COPYBARA_INTEGRATE_REVIEW=#14703 from protocolbuffers:add-support-for-extensions-in-ruby 601aca4
PiperOrigin-RevId: 582460674
zhangskz pushed a commit that referenced this issue Nov 14, 2023
Follow up to #14594, which added support for custom options, this PR implements extensions support, which should fully resolve #1198.

Closes #14703

COPYBARA_INTEGRATE_REVIEW=#14703 from protocolbuffers:add-support-for-extensions-in-ruby 601aca4
PiperOrigin-RevId: 582460674
zhangskz added a commit that referenced this issue Nov 15, 2023
…4756)

Follow up to #14594, which added support for custom options, this PR implements extensions support, which should fully resolve #1198.

Closes #14703

COPYBARA_INTEGRATE_REVIEW=#14703 from protocolbuffers:add-support-for-extensions-in-ruby 601aca4
PiperOrigin-RevId: 582460674

Co-authored-by: Jason Lunn <jason.lunn@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment