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.
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
ruby: Map to_h fixes #396
ruby: Map to_h fixes #396
Conversation
3173d18
to
23c13c8
Compare
return context.runtime.getTrue(); | ||
} | ||
|
||
RubyHash other = (RubyHash)_other; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the reason we have another step here is that #toHash
will convert all complex objects, such as Map
, Message
, and RepeatedField
into hashes and arrays. I believe this is what one would expect when calling #to_h
. However, that means that if the ==
is comparing against something like what is in ruby/tests/basic.rb#ln616
will fail unless we do a deeper check.
let me know what you think and if this is too complex and/or the assumptions on #to_h
are wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm -- I assume you actually hit the test failure then wrote this to fix it? I was under the impression that a Ruby hash would do a deep equality comparison already (using the :==
method on values as appropriate) -- the docs seem to say this too. Perhaps JRuby behaves differently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we did need the deep comparison, to deal with cases where the underlying hash may be expanded out or not (and thus is the original map object). Let me double check....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @cfallin
I did not first write a failing test case and then fix it. The failure was already there @ ruby/tests/basic.rb#ln616
that assert fails without the change to ==
. If you take out this code, you'll see the test case fail; it passes with this deeper logic.
the root issue is the modifications I made to #toHash
, which will convert complex child objects (Map/Message) into hashes rather than return the complex object.
I think this 'new' behavior is the expected behavior, but of course that is open to debate :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cfallin another thought; the original logic would allow equivalency with a hash. I'm not sure that is really the correct behavior. So if we change it to where if the incoming obj is not equivalent to this, return false.
@@ -340,7 +358,20 @@ public IRubyObject dup(ThreadContext context) { | |||
|
|||
@JRubyMethod(name = {"to_h", "to_hash"}) | |||
public RubyHash toHash(ThreadContext context) { | |||
return RubyHash.newHash(context.runtime, table, context.runtime.getNil()); | |||
// return RubyHash.newHash(context.runtime, table, context.runtime.getNil()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove commented-out line here
Hi @skippy, thanks for this! I agree that the fix to semantics makes sense. A few minor concerns above with implementation but overall direction is good. |
545887f
to
15c2c00
Compare
if (value.respondsTo("to_h")) { | ||
// Order here matters! Enumerable fields, such as RepeatedField, | ||
// respond to both #to_h and #to_a. in that case, we want the #to_a | ||
// behavior |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is an issue under jruby-9000, but not MRI ruby or earlier versions of jruby
if RUBY_PLATFORM == "java" | ||
# FIXME: java has the correct result; the underlying C++ encode_json | ||
# seems to be broken to some degree, where jruby returns the default values | ||
# but the C library does not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BUG ? @cfallin shall I go ahead and open a ticket?
@cfallin thanks for your support on this ticket. let me know what you think. Also be aware that I found what looks to be 2 bugs that are not related to this ticket. |
@skippy Sorry for the delay on this, I hadn't even realized this was still open. Could you bring me up to date on the overall status of this PR? |
@skippy Also could you possibly take a look at this test failure for an open PR and see if it related to what you are changing in this CL? I ask because it appears to be a failure related to https://travis-ci.org/google/protobuf/jobs/66956115 That is a failure on this PR: #500 Which is intended to fix this bug: #493 |
hey sir; I don't think this PR is causing the |
Also: * moving back to strings for hash keys to keep it consistent with type definitions in maps, such as defining 'string' as the key type * the behavior of RepeatedFields#each_index was not correct. The array and enumerable behaviors are different (who knew?!), so making this equivalent to the array version * merge in upstream changes from master. **WARNING** under jruby the expected protobuf binary test fails. This seems to be an issue upstream in the java library than in this code base. * Fixing jruby errors when run under jruby-9000.pre1
@skippy Thanks -- sorry I wasn't meaning to suggest that this PR was causing the error (it couldn't possibly, since this PR hasn't been merged yet :), but wondering more whether this PR might actually fix the breakage, or something like that. I haven't come up to speed on the I do want to get back to this PR to un-block you but I'm missing the bigger picture since I took over from Chris in the thick of things. Can you give me a big-picture overview of what this change is trying to accomplish? Thanks, |
Oh scratch that actually, I took a closer look and I totally get it now. |
I updated the top level commentYeah, this PR is becoming a bit of a mess. I found 4 non-related bugs
Besides those remaining 2 items (which should probably be separate tickets, and have the failing test commented out.. you can see that it failed in travis/jruby), I think this PR is good to go. I would be more than happy to close this PR down and submit a clean one without the Jruby-9000 and repeated_field fixes and put those in separate PRs. let me know if you would quickly like to chat offline and go through them in more detail |
@haberman thanks. I missed your notes after I sent my update. You are spot on... this is to add |
@haberman any thoughts? Let me know what you would like to have me do regarding the other possible bugs that were found (their is a broken test which highlights one of the bugs). Then I'll update this PR and hopefully it should be good to go. |
Hi Adam, Sorry again for the delay, and thanks for all the info. I think that splitting this CL apart into smaller, more self-contained PRs would be very helpful. When it comes to serializing defaults in JSON, both behaviors are technically correct! proto3 allows either, and some implementations let the user choose at runtime whether to serialize defaults to JSON or not. I would be very interested to know exactly how the two serialized protos are differing. |
Is there any movement on getting this merged? This is still broken in the current beta releases. |
Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok to test. |
Wow, this has been open for a while. This PR is kind of big and addresses several related issues. It would be good to break it up into some more focused changes, and maybe even have issues filed for the specific problems that still remain. |
If @skippy cannot split this up, I'd be willing to take a look at getting just |
hey folks, my apologies for going silent on this pr. I would love to revisit this PR but I'm pretty sure I won't be able to get to it for awhile yet. But if I can help answer questions about what I was trying to do, please let me know! |
Any chance for this PR to be revived? It's a really annoying bug. A 👍 from me |
Yeah, it's been a 3.0 blocker for me as well. I mentioned earlier that I'd be happy to split up @skippy's work if thats what the maintainers would like. I'm a little worried at the bit rot on this stuff at this point though. |
This PR is really out of date, and some of the issues have been fixed in other PRs, like this one: #2847 I'm going to close this for now. Please feel free to re-open if you want to revive it. |
Tools in the Go ecosystem treat code documented with "Deprecated:" as special for purposes of warning users not to use them. When a file, message, field, enum, enum value, service, or method is marked as deprecated, the generator will emit "// Deprecated: Do not use." comments on all relevant exported Go package information related to the deprecated element. This is an outline of how deprecation of each element effects the generated proto: * file - a comment is added to the top of the generated proto. * message - a comment is added above its type definition. * field, enum, or enum value - a comment is added inline with the defintion. * field - an additional comment is added above the field Getter. * service - a comment is added above generated Client interface and New method. * service - a comment is added above generated Server interface and Registration method. * method - a comment is added above the generated method definition. Fixes protocolbuffers#396.
map needed a #to_h method
Also:
behaviors are different (who knew?!), so making this equivalent to the array version
encode_json
method. When run under MRI, it does not return any default values, while JRuby does. It should probably return default values.