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

wants to merge 4 commits into from

Conversation

shaldengeki
Copy link
Contributor

@shaldengeki shaldengeki commented Jan 25, 2023 •
edited

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.

@shaldengeki shaldengeki requested a review from a team as a code owner January 25, 2023 02:04
@shaldengeki shaldengeki requested review from JasonLunn and removed request for a team January 25, 2023 02:04
@JasonLunn
Copy link
Contributor

Thanks for your contribution, @shaldengeki. Can you update this PR to include the changes to the JRuby implementation as well?

shaldengeki
shaldengeki commented Jan 26, 2023
View reviewed changes
ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java
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.

@shaldengeki
Copy link
Contributor Author

Hmm. Looks like the kokoro JRuby builds failed. I don't appear to have access to the test logs, either. Let me know if there's something I should fix here -- I looked at kokoro/linux/bazel.sh and it looks like I wouldn't be able to replicate this locally.

@JasonLunn
Copy link
Contributor

Hmm. Looks like the kokoro JRuby builds failed. I don't appear to have access to the test logs, either. Let me know if there's something I should fix here -- I looked at kokoro/linux/bazel.sh and it looks like I wouldn't be able to replicate this locally.

Error: test_has_field(BasicTest::MessageContainerTest): NoMethodError: undefined method `has_a?' for <BasicTest::OneofMessage: >:BasicTest::OneofMessage
org/jruby/RubyBasicObject.java:1715:in `method_missing'
com/google/protobuf/jruby/RubyMessage.java:540:in `method_missing'
/workspace/_build/out/sandbox/processwrapper-sandbox/1188/execroot/com_google_protobuf/bazel-out/k8-fastbuild/bin/ruby/tests/basic.runfiles/com_google_protobuf/ruby/tests/basic.rb:122:in `test_has_field'
     119:
     120:       m = OneofMessage.new
     121:       assert !m.has_my_oneof?
  => 122:       assert !m.has_a?
     123:       m.a = "foo"
     124:       assert m.has_my_oneof?
     125:       assert m.has_a?

I think the issue is that the JRuby change you made is only in the context of the implementation of responds_to?. There is a very similar change, in the same file, to the implementation of method_missing starting at ~ line 462.

@JasonLunn JasonLunn added kokoro:run jruby Issues unique to the JRuby interpreter labels Jan 27, 2023
@shaldengeki
Copy link
Contributor Author

Ooh. Looks like we have one remaining failing build. I poked more at running the kokoro builds locally, but this one's a MacOS build and I unfortunately don't have a device I can test on.

@JasonLunn Thanks for your help so far; when you have some time, take a look & let me know what's breaking.

@JasonLunn
Copy link
Contributor

LGTM, @haberman - WDYT?

@haberman
Copy link
Member

LGTM, thanks for your contribution. This has been on our TODO list for a while.

@copybara-service copybara-service bot mentioned this pull request Jan 31, 2023
Closed
copybara-service bot pushed a commit that referenced this pull request Jan 31, 2023
@protobuf-github-bot
For Ruby oneof fields, generate hazzers for members (#11655)
aa54f12 
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
copybara-service bot pushed a commit that referenced this pull request Jan 31, 2023
@protobuf-github-bot
For Ruby oneof fields, generate hazzers for members (#11655)
cccc1cd 
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
copybara-service bot pushed a commit that referenced this pull request Jan 31, 2023
@protobuf-github-bot
For Ruby oneof fields, generate hazzers for members (#11655)
5f39504 
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
@shaldengeki
Copy link
Contributor Author

woot - thank y'all for the help here! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@haberman haberman haberman approved these changes

@JasonLunn JasonLunn Awaiting requested review from JasonLunn JasonLunn is a code owner automatically assigned from protocolbuffers/protobuf-ruby

Assignees
No one assigned
Labels
jruby Issues unique to the JRuby interpreter ruby
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ruby oneof fields should have has_foo?, even for proto3 files
5 participants
@shaldengeki @JasonLunn @haberman @protobuf-kokoro @zhangskz