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

For Ruby oneof fields, generate hazzers for members #11655

Closed
wants to merge 4 commits into from
Closed

For Ruby oneof fields, generate hazzers for members #11655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e3edf2c
Generate hazzers for Ruby oneof fields in proto3
shaldengeki Jan 25, 2023
b93bbb5
Update tests
shaldengeki Jan 25, 2023
85d04c1
In JRuby, respond to hazzer
shaldengeki Jan 26, 2023
a15e474
Update method_missing as well
shaldengeki Jan 27, 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
9 changes: 0 additions & 9 deletions ruby/ext/google/protobuf_c/message.c
View file
Open in desktop
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
12 changes: 2 additions & 10 deletions ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java
View file
Open in desktop
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()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JasonLunn Can you take a look and let me know if I'm on the right track here? I think I've replicated the same logic, but:

  • I'm not totally sure if this is the right place to be modifying,
  • I could use a sanity-check on the logic here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks about right to me on its surface. Rerunning tests now to confirm.

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
View file
Open in desktop
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