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.

Sign up for GitHub

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

Jump to bottom

Add support for descriptor options in ruby interface #12828

Closed
wants to merge 6 commits into from
Closed

Add support for descriptor options in ruby interface #12828

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d5ed3dd
Add support for descriptor options in ruby interface
jsteinberg May 16, 2023
71f7226
DRY converting serialized options into ruby object
jsteinberg May 17, 2023
04758ed
make serialized_options methods private
jsteinberg May 17, 2023
188f456
Options messages should recursively freeze (deep_freeze)
jsteinberg May 18, 2023
730fb06
Message internal_deep_freeze now recurses over repeated/map value fields
jsteinberg May 18, 2023
10b3e2b
Fix spelling issue (Deeep -> Deep)
jsteinberg May 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
122 changes: 121 additions & 1 deletion ruby/ext/google/protobuf_c/defs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,6 +397,29 @@ static VALUE Descriptor_msgclass(VALUE _self) {
return self->klass;
}

/*
* 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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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");
}
}

static void Descriptor_register(VALUE module) {
VALUE klass = rb_define_class_under(module, "Descriptor", rb_cObject);
rb_define_alloc_func(klass, Descriptor_alloc);
Expand All @@ -408,6 +431,7 @@ static void Descriptor_register(VALUE module) {
rb_define_method(klass, "msgclass", Descriptor_msgclass, 0);
rb_define_method(klass, "name", Descriptor_name, 0);
rb_define_method(klass, "file_descriptor", Descriptor_file_descriptor, 0);
rb_define_method(klass, "serialized_options", Descriptor_serialized_options, 0);
rb_include_module(klass, rb_mEnumerable);
rb_gc_register_address(&cDescriptor);
cDescriptor = klass;
Expand Down Expand Up @@ -507,12 +531,36 @@ static VALUE FileDescriptor_syntax(VALUE _self) {
}
}

/*
* call-seq:
* FileDescriptor.serialized_options => options
*
* Returns a binary string containing the serialized options for this message.
*/
static VALUE FileDescriptor_serialized_options(VALUE _self) {
FileDescriptor* self = ruby_to_FileDescriptor(_self);
const google_protobuf_FileOptions* opts = upb_FileDef_Options(self->filedef);
upb_Arena* arena = upb_Arena_New();
size_t size;
char* serialized = google_protobuf_FileOptions_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");
}
}

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

rb_gc_register_address(&cFileDescriptor);
cFileDescriptor = klass;
}
Expand Down Expand Up @@ -563,7 +611,7 @@ static VALUE FieldDescriptor_alloc(VALUE klass) {

/*
* call-seq:
* EnumDescriptor.new(c_only_cookie, pool, ptr) => EnumDescriptor
* FieldDescriptor.new(c_only_cookie, pool, ptr) => FieldDescriptor
*
* Creates a descriptor wrapper object. May only be called from C.
*/
Expand Down Expand Up @@ -864,6 +912,29 @@ static VALUE FieldDescriptor_set(VALUE _self, VALUE msg_rb, VALUE value) {
return Qnil;
}

/*
* call-seq:
* FieldDescriptor.serialized_options => options
*
* Returns a binary string containing the serialized options for this message.
*/
static VALUE FieldDescriptor_serialized_options(VALUE _self) {
FieldDescriptor* self = ruby_to_FieldDescriptor(_self);
const google_protobuf_FieldOptions* opts = upb_FieldDef_Options(self->fielddef);
upb_Arena* arena = upb_Arena_New();
size_t size;
char* serialized = google_protobuf_FieldOptions_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");
}
}

static void FieldDescriptor_register(VALUE module) {
VALUE klass = rb_define_class_under(module, "FieldDescriptor", rb_cObject);
rb_define_alloc_func(klass, FieldDescriptor_alloc);
Expand All @@ -880,6 +951,7 @@ static void FieldDescriptor_register(VALUE module) {
rb_define_method(klass, "clear", FieldDescriptor_clear, 1);
rb_define_method(klass, "get", FieldDescriptor_get, 1);
rb_define_method(klass, "set", FieldDescriptor_set, 2);
rb_define_method(klass, "serialized_options", FieldDescriptor_serialized_options, 0);
rb_gc_register_address(&cFieldDescriptor);
cFieldDescriptor = klass;
}
Expand Down Expand Up @@ -979,12 +1051,36 @@ static VALUE OneofDescriptor_each(VALUE _self) {
return Qnil;
}

/*
* call-seq:
* OneofDescriptor.serialized_options => options
*
* Returns a binary string containing the serialized options for this message.
*/
static VALUE OneOfDescriptor_serialized_options(VALUE _self) {
OneofDescriptor* self = ruby_to_OneofDescriptor(_self);
const google_protobuf_OneofOptions* opts = upb_OneofDef_Options(self->oneofdef);
upb_Arena* arena = upb_Arena_New();
size_t size;
char* serialized = google_protobuf_OneofOptions_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");
}
}

static void OneofDescriptor_register(VALUE module) {
VALUE klass = rb_define_class_under(module, "OneofDescriptor", rb_cObject);
rb_define_alloc_func(klass, OneofDescriptor_alloc);
rb_define_method(klass, "initialize", OneofDescriptor_initialize, 3);
rb_define_method(klass, "name", OneofDescriptor_name, 0);
rb_define_method(klass, "each", OneofDescriptor_each, 0);
rb_define_method(klass, "serialized_options", OneOfDescriptor_serialized_options, 0);
rb_include_module(klass, rb_mEnumerable);
rb_gc_register_address(&cOneofDescriptor);
cOneofDescriptor = klass;
Expand Down Expand Up @@ -1153,6 +1249,29 @@ static VALUE EnumDescriptor_enummodule(VALUE _self) {
return self->module;
}

/*
* call-seq:
* EnumDescriptor.serialized_options => options
*
* Returns a binary string containing the serialized options for this message.
*/
static VALUE EnumDescriptor_serialized_options(VALUE _self) {
EnumDescriptor* self = ruby_to_EnumDescriptor(_self);
const google_protobuf_EnumOptions* opts = upb_EnumDef_Options(self->enumdef);
upb_Arena* arena = upb_Arena_New();
size_t size;
char* serialized = google_protobuf_EnumOptions_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");
}
}

static void EnumDescriptor_register(VALUE module) {
VALUE klass = rb_define_class_under(module, "EnumDescriptor", rb_cObject);
rb_define_alloc_func(klass, EnumDescriptor_alloc);
Expand All @@ -1163,6 +1282,7 @@ static void EnumDescriptor_register(VALUE module) {
rb_define_method(klass, "each", EnumDescriptor_each, 0);
rb_define_method(klass, "enummodule", EnumDescriptor_enummodule, 0);
rb_define_method(klass, "file_descriptor", EnumDescriptor_file_descriptor, 0);
rb_define_method(klass, "serialized_options", EnumDescriptor_serialized_options, 0);
rb_include_module(klass, rb_mEnumerable);
rb_gc_register_address(&cEnumDescriptor);
cEnumDescriptor = klass;
Expand Down
54 changes: 54 additions & 0 deletions ruby/lib/google/protobuf/descriptor_dsl.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,10 @@ def value(name, number)
end
end

def self.decode_options(klass, serialized_options)
options = klass.decode(serialized_options)
options.freeze
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end
end

# Re-open the class (the rest of the class is implemented in C)
Expand All @@ -461,5 +465,55 @@ def build(&block)
builder.build
end
end

# Re-open the class (the rest of the class is implemented in C)
class Descriptor
def options
@options ||= Google::Protobuf::Internal.decode_options(
Google::Protobuf::MessageOptions,
serialized_options
)
end
end

# Re-open the class (the rest of the class is implemented in C)
class FileDescriptor
def options
@options ||= Google::Protobuf::Internal.decode_options(
Google::Protobuf::FileOptions,
serialized_options
)
end
end

# Re-open the class (the rest of the class is implemented in C)
class FieldDescriptor
def options
@options ||= Google::Protobuf::Internal.decode_options(
Google::Protobuf::FieldOptions,
serialized_options
)
end
end

# Re-open the class (the rest of the class is implemented in C)
class EnumDescriptor
def options
@options ||= Google::Protobuf::Internal.decode_options(
Google::Protobuf::EnumOptions,
serialized_options
)
end
end

# Re-open the class (the rest of the class is implemented in C)
class OneofDescriptor
def options
@options ||= Google::Protobuf::Internal.decode_options(
Google::Protobuf::OneofOptions,
serialized_options
)
end
end
end
end
28 changes: 28 additions & 0 deletions ruby/tests/basic.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -711,6 +711,34 @@ def test_map_fields_respond_to? # regression test for issue 9202
msg.map_string_int32_as_value = :boom
end
end

def test_file_descriptor_options
file_descriptor = TestMessage.descriptor.file_descriptor

assert_equal file_descriptor.options.class, Google::Protobuf::FileOptions
assert file_descriptor.options.deprecated
end

def test_descriptor_options
descriptor = TestDeprecatedMessage.descriptor

assert_equal descriptor.options.class, Google::Protobuf::MessageOptions
assert descriptor.options.deprecated
end

def test_enum_descriptor_options
enum_descriptor = TestDeprecatedEnum.descriptor

assert_equal enum_descriptor.options.class, Google::Protobuf::EnumOptions
assert enum_descriptor.options.deprecated
end

def test_oneof_descriptor_options
descriptor = TestDeprecatedMessage.descriptor
oneof_descriptor = descriptor.lookup_oneof("test_deprecated_message_oneof")

assert_equal oneof_descriptor.options.class, Google::Protobuf::OneofOptions
end
end

def test_oneof_fields_respond_to? # regression test for issue 9202
Expand Down
20 changes: 20 additions & 0 deletions ruby/tests/basic_test.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import "google/protobuf/duration.proto";
import "google/protobuf/struct.proto";
import "test_import_proto2.proto";

option deprecated = true;

message Foo {
Bar bar = 1;
repeated Baz baz = 2;
Expand Down Expand Up @@ -68,6 +70,17 @@ message TestMessage2 {
optional int32 foo = 1;
}

message TestDeprecatedMessage {
option deprecated = true;

optional int32 foo = 1;

oneof test_deprecated_message_oneof {
string a = 2;
int32 b = 3;
}
}

enum TestEnum {
Default = 0;
A = 1;
Expand All @@ -76,6 +89,13 @@ enum TestEnum {
v0 = 4;
}

enum TestDeprecatedEnum {
option deprecated = true;

DefaultA = 0;
AA = 1 [deprecated = true];
}

message TestEmbeddedMessageParent {
TestEmbeddedMessageChild child_msg = 1;
int32 number = 2;
Expand Down