Add support for descriptor options in ruby interface #12828

jsteinberg · 2023-05-16T14:58:27Z

Add support for descriptor options in Ruby:

Notes:

EnumValueOptions / ServiceOptions / MethodOptions were not included. There are not ruby interfaces for EnumValueDescriptorProto / ServiceDescriptorProto / MethodDescriptorProto. If we want to add those first I could look into that.
In the conversation on the original issue, @haberman commented # We actually need to freeze recursively.. I did not implement that yet and am looking for guidance.
- Should that be the default behavior in the Message_freeze method?
- What all field types need to be frozen (SubMessage / Repeated / Map ?)

google-cla · 2023-05-16T14:58:30Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

haberman · 2023-05-17T16:30:15Z

ruby/ext/google/protobuf_c/defs.c

+  upb_Arena* arena = upb_Arena_New();
+  size_t size;
+  char* serialized = google_protobuf_MessageOptions_serialize(opts, arena, &size);
+  if (serialized) {


Please factor this part of the function into a static helper so that all of the functions you added can share it.

haberman · 2023-05-17T16:32:44Z

ruby/ext/google/protobuf_c/defs.c

 static void FileDescriptor_register(VALUE module) {
  VALUE klass = rb_define_class_under(module, "FileDescriptor", rb_cObject);
  rb_define_alloc_func(klass, FileDescriptor_alloc);
  rb_define_method(klass, "initialize", FileDescriptor_initialize, 3);
  rb_define_method(klass, "name", FileDescriptor_name, 0);
  rb_define_method(klass, "syntax", FileDescriptor_syntax, 0);
+  rb_define_method(klass, "serialized_options", FileDescriptor_serialized_options, 0);


In the interest of keeping the public API small, can we make serialized_options a private method?

haberman · 2023-05-17T17:34:12Z

ruby/lib/google/protobuf/descriptor_dsl.rb

@@ -451,6 +451,10 @@ def value(name, number)
        end
      end

+      def self.decode_options(klass, serialized_options)
+        options = klass.decode(serialized_options)
+        options.freeze


We want this freeze to be recursive. Otherwise a user could write something like:

# This works, because only my_descriptor.options itself is frozen. my_descriptor.options.some_message_options.field = 5

We'll want to write a loop that visits all fields and freezes them all. I'm not sure how easy this will be to do in Ruby. We may want to do it in C.

Thank you for the review. I've addressed the first 2 concerns (I think).

I have a question about the recursive freeze. The Message class already has a freeze method that I am calling here. Should that method be updated to be recursive or should I create a separate deep_freeze method for this use case?

Additionally, for the recursive freeze, should all fields also be pinned to the same arena?

Message#freeze follows the general Ruby semantics of only doing a shallow freeze. I don't think we should change that.

I think we could add deep_freeze as an internal-only API, but I don't think we should make it public. We should keep new public APIs to a minimum, since each one will need to be reviewed and scrutinized.

I added a deep_freeze method as a private method on the message class. I understand this sorta exposes it as a public API (and I'm even cheating and calling it by using ruby's send method).

I couldn't figure out a better way to hide it as an internal-only API. Open to suggestions. The only other thought I had would be to move the .options methods into defs.c, but that would require exposing some more of message.c to allow decoding and deep_freezeing it.

haberman · 2023-05-18T17:17:49Z

ruby/ext/google/protobuf_c/message.c

+      VALUE field = Message_getfield(_self, f);
+
+      if (upb_FieldDef_IsMap(f) || upb_FieldDef_IsRepeated(f)) {
+        rb_funcall(field, rb_intern("freeze"), 0);


Repeated fields and maps can contain messages, so we will need them to support deep_freeze also.

haberman · 2023-05-18T17:18:46Z

ruby/ext/google/protobuf_c/message.c

+ *
+ * Deeep freezes the message object recursively.
+ */
+static VALUE Message_deep_freeze(VALUE _self) {


Let's call it internal_deep_freeze just so it's unambiguous that it's intended to be internal-only.

It doesn't make it harder to call, but if someone does call it they will know that it's an API that could change.

bitwise-aiden · 2023-06-23T15:50:14Z

👋🏻 Hey @jsteinberg, is there anything I can help do to get this going again?

jsteinberg · 2023-06-23T16:07:59Z

Yes, I've unfortunately been busy at work so haven't gotten back to this. It's definitely on my backlog (along with adding extension support to ruby after this).

So, I'm not sure what is needed to move forward here. I got a little bit of time this afternoon so happy to make any requested changes / rebase this off of main.

jsteinberg · 2023-06-23T16:09:19Z

It looks like I need to figure out how to make this work for JRuby. Its been a while since I've done anything with that.

bitwise-aiden · 2023-06-23T19:44:03Z

Oh interesting, there isn't an existing freeze method implemented in the Java version. Will be a bit more involved than just mirroring it like I first thought.

JasonLunn · 2023-06-23T22:44:38Z

If you have questions about JRuby I'm happy to help.

haberman · 2023-07-12T12:04:28Z

@JasonLunn is it true that JRuby Protobuf doesn't support #freeze right now? If so, what would it take to add that.

jsteinberg · 2023-07-12T13:13:47Z

@JasonLunn is it true that JRuby Protobuf doesn't support #freeze right now? If so, what would it take to add that.

Sorry, I didn't end up getting around to this and then had some vacation around the holiday. I have essentially zero JRuby knowledge (I used it once...) and since the CI tests were not enabled for this branch, I didn't realize I had omitted the necessary changes.

I can try to get JRuby running locally so I can run the tests and try to figure out what is missing, but if someone else with much more knowledge wants to do it then I'm not going to stop you.

I have limited time this week but I will try to at least start making progress on this.

jsteinberg · 2023-07-12T13:37:36Z

It looks like I need to implement serialized_options for the JRuby classes:

NameError: undefined local variable or method `serialized_options' for #<Google::Protobuf::FileDescriptor:0x644947ee>

Additionally, I need to implement the internal_deep_freeze method (which makes sense)

NoMethodError: undefined method `internal_deep_freeze' for #<BasicTest::TestDeprecatedMessage:0x65259c53>

JasonLunn · 2023-07-12T15:20:42Z

@JasonLunn is it true that JRuby Protobuf doesn't support #freeze right now? If so, what would it take to add that.

JRuby as a language supports #freeze and there are tests for Protobuf-specific freeze behaviors in common_tests.rb that pass under JRuby.

@jsteinberg - feel free to ping me if you have any JRuby-specific questions.

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

github-actions · 2023-11-12T10:02:27Z

We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active, please add a comment.

This PR is labeled inactive because the last activity was over 90 days ago. This PR will be closed and archived after 14 additional days without activity.

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

github-actions · 2023-11-27T10:02:38Z

We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active or becomes active again, please reopen it.

This PR was closed and archived because there has been no new activity in the 14 days since the inactive label was added.

Add support for descriptor options in ruby interface

d5ed3dd

jsteinberg requested a review from a team as a code owner May 16, 2023 14:58

jsteinberg requested review from JasonLunn and removed request for a team May 16, 2023 14:58

jsteinberg closed this May 16, 2023

jsteinberg reopened this May 16, 2023

haberman requested changes May 17, 2023

View reviewed changes

jsteinberg added 3 commits May 17, 2023 13:57

DRY converting serialized options into ruby object

71f7226

make serialized_options methods private

04758ed

Options messages should recursively freeze (deep_freeze)

188f456

haberman requested changes May 18, 2023

View reviewed changes

Message internal_deep_freeze now recurses over repeated/map value fields

730fb06

haberman added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 19, 2023

github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 19, 2023

Fix spelling issue (Deeep -> Deep)

10b3e2b

hlopko added ruby 🅰️ safe for tests Mark a commit as safe to run presubmits over labels May 30, 2023

github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 30, 2023

bitwise-aiden mentioned this pull request Jun 23, 2023

Simplify check for Google::Protobuf::Map type - redux Shopify/tapioca#1541

Merged

dazuma mentioned this pull request Aug 17, 2023

Use google-protobuf gem instead of third-party protobuf gem googleapis/gapic-generator-ruby#972

Closed

JasonLunn mentioned this pull request Nov 1, 2023

Add support for options in CRuby, JRuby and FFI #14594

Closed

github-actions bot added the inactive Denotes the issue/PR has not seen activity in the last 90 days. label Nov 12, 2023

JasonLunn mentioned this pull request Nov 13, 2023

Add support for options in CRuby, JRuby and FFI (#14594) #14739

Merged

github-actions bot closed this Nov 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for descriptor options in ruby interface #12828

Add support for descriptor options in ruby interface #12828

jsteinberg commented May 16, 2023

google-cla bot commented May 16, 2023

haberman May 17, 2023

haberman May 17, 2023

haberman May 17, 2023

jsteinberg May 17, 2023

haberman May 17, 2023

jsteinberg May 18, 2023

haberman May 18, 2023

haberman May 18, 2023

bitwise-aiden commented Jun 23, 2023

jsteinberg commented Jun 23, 2023

jsteinberg commented Jun 23, 2023

bitwise-aiden commented Jun 23, 2023

JasonLunn commented Jun 23, 2023

haberman commented Jul 12, 2023

jsteinberg commented Jul 12, 2023 •

edited

jsteinberg commented Jul 12, 2023

JasonLunn commented Jul 12, 2023

github-actions bot commented Nov 12, 2023

github-actions bot commented Nov 27, 2023

Add support for descriptor options in ruby interface #12828

Add support for descriptor options in ruby interface #12828

Conversation

jsteinberg commented May 16, 2023

google-cla bot commented May 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitwise-aiden commented Jun 23, 2023

jsteinberg commented Jun 23, 2023

jsteinberg commented Jun 23, 2023

bitwise-aiden commented Jun 23, 2023

JasonLunn commented Jun 23, 2023

haberman commented Jul 12, 2023

jsteinberg commented Jul 12, 2023 • edited

jsteinberg commented Jul 12, 2023

JasonLunn commented Jul 12, 2023

github-actions bot commented Nov 12, 2023

github-actions bot commented Nov 27, 2023

jsteinberg commented Jul 12, 2023 •

edited