protobuf support for jruby

[isaiah]

JRuby support via native extension.

It's basically a wrapper around DynamicMessage

The tests are changed to use minitest instead of test/unit.

There are a few differences/bugs than the c implementation:

• Enum value cannot take arbitary integer
• Non-repeated fields are copied, and cannot be modified, this goes directly into DynamicMessage.Builder
• Serialize circle is allowed
• Corecursive messages cannot be inspected or compared, seems DynamicMessage cannot tell the difference between an empty message and the default instance.

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

[googlebot]

CLAs look good, thanks!

[isaiah]

As per #121 , this is the jruby version.

[cfallin]

This is fantastic, thanks @isaiah! I'll take a closer look when I'm back in the office after the holidays and will probably have more detailed comments then.

[haberman]

Hmm, has proto3 support for Java not yet been implemented/released? For proto3, the semantics of enums are supposed to be open, but of course if protobuf-Java hasn't been augmented to support this yet then this won't work yet.

[isaiah]

You are correct, it's supported. I'll update the code.

[haberman]

This looks great from a high level, thanks for making this happen! I'm glad you implemented the API set forth in Chris's MRI extension.

A high-level comment (before Chris and others dive in a little deeper): is there a reason you changed the test so much? Ideally the JRuby extension would be implemented in such a way that the existing test passes verbatim. Adding test cases can be useful too, but the test file was changed so extensively that it's hard to tell whether you changed existing semantics or not.

[isaiah]

@haberman There were mainly two changes, firstly test/unit is replaced by minitest, the original tests doesn't work with the default test/unit shims around minitest, it requires the test-unit gem installed. Also test/unit shipped with ruby varies across different versions in MRI, (see discussions here), if we take jruby into consideration, it becomes even more complicated. However, they all ship a working minitest gem.

Secondly, the original assertions are not very friendly, they were replace by more specific assertions, compare the following failing message:

  1) Failure:
test_defaults(TestProtobuf) [tests/basic.rb:67]:
Failed assertion, no message given.

VS

  1) Failure:
test_defaults(TestProtobuf) [tests/basic.rb:67]:
Expected: 1
  Actual: 0

[ratnikov]

Hey @isaiah, I've got 3 minor questions for you:

1. Seems like the only reason for sentinel.proto existence are default values. Any specific reason you didn't use the protobuf-java logic for defaults? (e.g. when no value is set, returning DynamicMessage's field default)
2. You probably want to add the jar to the gemspec.
3. If I were to generate a java class from a .proto object (so no dynamic descriptor definition, like in tests/basic.rb), how would it look?

Thanks for the work!

[isaiah]

Hi @ratnikov, I am not sure if I understood the third question correctly, but I'll try:

1. The sentinel is only for generating default values of RepeatedField, normal messages get default values from DynamicMessage, as far as I know, in Java the repeated field cannot exist without a schema/message, I could create a DynamicMessage on the fly for the a new unattached RepeatedField, but that's looks too complicated, maybe i have to switch to such implementation in the future if it's proven the current one is not verbose.
2. Yes, and the rake test task doesn't for jruby yet.
3. The usage of DynamicMessage is an implementation detail, the generated java classes is just normal java class to jruby. So it requires dynamic descriptor definition in order to encode/decode messages, I suppose that definition schema will be generated by protoc.

[cfallin]

Can we move this file to the jruby directory, as it's jruby-extension-specific?

[cfallin]

Overall looks good. Just spoke with @pherl (our team lead) on how to handle this PR. We think it's best to merge this to a branch (jruby, perhaps) and wait for maps and oneofs before merging to master, since we're hoping to release an alpha version soon. But to be clear, we're happy with the contribution overall (thanks!). You may need to recreate the PR for that?

[cfallin]

Also, FYI, I just created the jruby branch in google/protobuf.

[isaiah]

@cfallin In that case let's wait until maps and oneofs are implemented. I've finished the map support in a separate branch, and the oneofs should not be far away.

[cfallin]

Sounds good. Thanks for all the work on this!

[isaiah]

Hi @cfallin , maps and oneofs are implemented.

[cfallin]

Fix TODO?

[cfallin]

This looks really good overall! Just the few comments above then I think we can merge.

[cfallin]

Hi @isaiah, I just left a few more comments -- after some internal discussions, we think it's best to exclude the JSON/formatting stuff for now.

Also, could you rebase everything to a single commit off of our master?

Assuming both of those changes are made, I'm happy to LGTM and merge. Thanks!

[isaiah]

@cfallin Specs are green on both platform now.

[isaiah]

It is safe in the jruby case, since the string is a copy. The implementation is quite different from the C counterpart, it delegates most of the operations to the underline DynamicMessage.Builder,.

[cfallin]

Yikes! Sorry, with time away from this PR, the details had faded in my mind. But I realize now that there's a semantic difference, which could be a problem. Consider:

m = TestMessage.new(:optional_msg => TestMessage2.new)
submsg_reference = m.optional_msg
submsg_reference.foo = 42
assert m.optional_msg.foo == 42  # fails with JRuby extension, passes with MRI Ruby extension

In general, calling wrapField() when passing a value out to Ruby, and convert() when taking a value in from Ruby, loses this reference-sharing for both messages and strings. No test catches this now (I should add one, sorry!) but it is a difference that will likely trip up some folks.

I think the solution to this is to somehow hold mutable builders for all submessage fields, and RubyStrings for all string fields, then in one pass, "commit" these to immutable children of the parent's builder, say right before serialization. Since the user can hold mutable references to children, don't trust that they won't change between adjacent serializations, so always rebuild the builder contents on every serialization.

Does that sound reasonable?

[isaiah]

Yes, I thought about it, it requires the extension to manage the data by itself and only use protobuf-java for serialize/deserialize. I will take a look and see how it goes.

[cfallin]

Thanks! Sorry for nitpicking so much. Just want to make sure to get it right :-)

[isaiah]

No worries, I totally agree with you, the difference is quite fatal.

[isaiah]

Hi @cfallin , fixed.

[cfallin]

I just left one comment on ruby/README.md (has outdated note about upb), otherwise, LGTM. As soon as the readme is fixed, I'll merge. Thanks very much for the persistence!

[cfallin]

Looks good, merging now, thanks!