Skip to content

Commit

Permalink
For Ruby oneof fields, generate hazzers for members (#11655)
Browse files Browse the repository at this point in the history
When generating Ruby clients for proto3 messages that have a oneof, we generate a hazzer for members of the oneof, not just a hazzer for the oneof itself.

In other words, for a proto like this:

```
syntax = "proto3";
message Foo {
  oneof bar {
    string baz = 1;
  }
}
```

The generated `Foo` will now have a method called `has_baz?`, in addition to the (pre-existing) method `has_bar?`.

I updated the unit tests, and verified that all the tests under `//ruby/...` pass.

Fixes #9561.

Closes #11655

COPYBARA_INTEGRATE_REVIEW=#11655 from shaldengeki:test-ruby-oneof-hazzer a15e474
FUTURE_COPYBARA_INTEGRATE_REVIEW=#11655 from shaldengeki:test-ruby-oneof-hazzer a15e474
PiperOrigin-RevId: 506032769
  • Loading branch information
protobuf-github-bot authored and copybara-github committed Jan 31, 2023
1 parent 72bf2ea commit cccc1cd
Show file tree
Hide file tree
Showing 3 changed files with 17 additions and 43 deletions.
17 changes: 4 additions & 13 deletions ruby/ext/google/protobuf_c/message.c
Original file line number Diff line number Diff line change
Expand Up @@ -243,15 +243,6 @@ static int extract_method_call(VALUE method_name, Message* self,
if (Match(m, name, f, o, "clear_", "")) return METHOD_CLEAR;
if (Match(m, name, f, o, "has_", "?") &&
(*o || (*f && upb_FieldDef_HasPresence(*f)))) {
// Disallow oneof hazzers for proto3.
// TODO(haberman): remove this test when we are enabling oneof hazzers for
// proto3.
if (*f && !upb_FieldDef_IsSubMessage(*f) &&
upb_FieldDef_RealContainingOneof(*f) &&
upb_MessageDef_Syntax(upb_FieldDef_ContainingType(*f)) !=
kUpb_Syntax_Proto2) {
return METHOD_UNKNOWN;
}
return METHOD_PRESENCE;
}
if (Match(m, name, f, o, "", "_as_value") && *f &&
Expand Down Expand Up @@ -1287,12 +1278,12 @@ VALUE build_module_from_enumdesc(VALUE _enumdesc) {
int32_t value = upb_EnumValueDef_Number(ev);
if (name[0] < 'A' || name[0] > 'Z') {
if (name[0] >= 'a' && name[0] <= 'z') {
name[0] -= 32; // auto capitalize
name[0] -= 32; // auto capitalize
} else {
rb_warn(
"Enum value '%s' does not start with an uppercase letter "
"as is required for Ruby constants.",
name);
"Enum value '%s' does not start with an uppercase letter "
"as is required for Ruby constants.",
name);
}
}
rb_define_const(mod, name, INT2NUM(value));
Expand Down
12 changes: 2 additions & 10 deletions ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java
Original file line number Diff line number Diff line change
Expand Up @@ -306,11 +306,7 @@ public IRubyObject respondTo(ThreadContext context, IRubyObject[] args) {
if (methodName.startsWith(HAS_PREFIX) && methodName.endsWith(QUESTION_MARK)) {
String strippedMethodName = methodName.substring(4, methodName.length() - 1);
FieldDescriptor fieldDescriptor = descriptor.findFieldByName(strippedMethodName);
if (fieldDescriptor != null
&& (!proto3
|| fieldDescriptor.getContainingOneof() == null
|| fieldDescriptor.getContainingOneof().isSynthetic())
&& fieldDescriptor.hasPresence()) {
if (fieldDescriptor != null && fieldDescriptor.hasPresence()) {
return context.runtime.getTrue();
}
oneofDescriptor =
Expand Down Expand Up @@ -459,11 +455,7 @@ public IRubyObject methodMissing(ThreadContext context, IRubyObject[] args) {

fieldDescriptor = descriptor.findFieldByName(methodName);

if (fieldDescriptor != null
&& (!proto3
|| fieldDescriptor.getContainingOneof() == null
|| fieldDescriptor.getContainingOneof().isSynthetic())
&& fieldDescriptor.hasPresence()) {
if (fieldDescriptor != null && fieldDescriptor.hasPresence()) {
return fields.containsKey(fieldDescriptor) ? runtime.getTrue() : runtime.getFalse();
}

Expand Down
31 changes: 11 additions & 20 deletions ruby/tests/basic.rb
Original file line number Diff line number Diff line change
Expand Up @@ -119,11 +119,10 @@ def test_has_field

m = OneofMessage.new
assert !m.has_my_oneof?
assert !m.has_a?
m.a = "foo"
assert m.has_my_oneof?
assert_raise NoMethodError do
m.has_a?
end
assert m.has_a?
assert_true OneofMessage.descriptor.lookup('a').has?(m)

m = TestSingularFields.new
Expand Down Expand Up @@ -716,24 +715,16 @@ def test_map_fields_respond_to? # regression test for issue 9202

def test_oneof_fields_respond_to? # regression test for issue 9202
msg = proto_module::OneofMessage.new
# `has_` prefix + "?" suffix actions should only work for oneofs fields.
# `has_` prefix + "?" suffix actions should work for oneofs fields and members.
assert msg.has_my_oneof?
assert msg.respond_to? :has_my_oneof?
assert !msg.respond_to?( :has_a? )
assert_raise NoMethodError do
msg.has_a?
end
assert !msg.respond_to?( :has_b? )
assert_raise NoMethodError do
msg.has_b?
end
assert !msg.respond_to?( :has_c? )
assert_raise NoMethodError do
msg.has_c?
end
assert !msg.respond_to?( :has_d? )
assert_raise NoMethodError do
msg.has_d?
end
assert msg.respond_to?( :has_a? )
assert !msg.has_a?
assert msg.respond_to?( :has_b? )
assert !msg.has_b?
assert msg.respond_to?( :has_c? )
assert !msg.has_c?
assert msg.respond_to?( :has_d? )
assert !msg.has_d?
end
end

0 comments on commit cccc1cd

Please sign in to comment.