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.

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

Closed
wants to merge 1 commit into from
Closed

Conversation

skippy
Copy link
Contributor

@skippy skippy commented May 16, 2015

map needed a #to_h method

Also:

  • moving back to strings for hash keys (a change from the previous commit ruby: Encode decode cleanup and behavior normalization #338) 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, meaning it generates a different protobuf than the exact same msg run under MRi. 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. Their is a jruby AST parse error that required odd code to work around in a few of the tests.
  • Found a bug with the encode_json method. When run under MRI, it does not return any default values, while JRuby does. It should probably return default values.

@skippy skippy force-pushed the map-to_h-fixes branch 3 times, most recently from 3173d18 to 23c13c8 Compare May 16, 2015 18:49
return context.runtime.getTrue();
}

RubyHash other = (RubyHash)_other;
Copy link
Contributor Author

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

Copy link
Contributor

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?

Copy link
Contributor Author

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....

Copy link
Contributor Author

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 :)

Copy link
Contributor Author

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());
Copy link
Contributor

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

@cfallin
Copy link
Contributor

cfallin commented May 18, 2015

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.

@skippy skippy force-pushed the map-to_h-fixes branch 2 times, most recently from 545887f to 15c2c00 Compare June 7, 2015 22:06
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
Copy link
Contributor Author

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
Copy link
Contributor Author

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?

@skippy
Copy link
Contributor Author

skippy commented Jun 7, 2015

@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.

@cfallin
Copy link
Contributor

cfallin commented Jun 8, 2015

@skippy Thanks for all the updates on this! Since the last back-and-forth, though, I've actually handed off Ruby duties to @haberman -- I am sure he'll be happy to take a look. Thanks!

@haberman
Copy link
Member

@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?

@haberman
Copy link
Member

@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 to_h.

https://travis-ci.org/google/protobuf/jobs/66956115

That is a failure on this PR: #500

Which is intended to fix this bug: #493

@skippy
Copy link
Contributor Author

skippy commented Jun 16, 2015

hey sir; I don't think this PR is causing the RubyMap#hash method to fail. This PR changes the #toHash (as well as == and inspect) ,but the #hash logic isn't changed at all, and it doesn't have a dependency on any other methods in this class.

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
@haberman
Copy link
Member

@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 #hash vs. #toHash issue and didn't even realize they were separate, sorry! Thanks for the info.

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,
Josh

@haberman
Copy link
Member

Oh scratch that actually, I took a closer look and I totally get it now. #to_h is for converting to a hash table, not to compute a hash of the container. You're just adding #to_h which didn't exist before. Ok got it now, sorry for the confusion. :)

@skippy
Copy link
Contributor Author

skippy commented Jun 16, 2015

I updated the top level comment

Yeah, this PR is becoming a bit of a mess. I found 4 non-related bugs

  • FIXED: repeated_field.rb had an issue where the #each_index behavior is different when under an array or under an enumerable (this is probably an issue with ruby core). The behavior was like it was as an enumerable, but since RepeatedArray is attempting to quack like an Array, I changed it to behave like Array#each_index
  • FIXED: a bunch of tests and some of the code failed when run under jruby-9.0.0.0.pre2; these are fixed
  • NOT FIXED: when an ProtoBuf object is converted to a JSON object, under JRuby, it returns the expected result. However, under MRI, it does not return the default values. See: https://github.com/google/protobuf/pull/396/files#diff-913cae3cb2a96306e220b73ff3d2cecbR118
  • NOT FIXED: there are a bunch of new unit tests, including making sure encoding returns an expected result. It actually returns a different value whether run under MRI or JRuby. see: https://github.com/google/protobuf/pull/396/files#diff-913cae3cb2a96306e220b73ff3d2cecbR159

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

@skippy
Copy link
Contributor Author

skippy commented Jun 17, 2015

@haberman thanks. I missed your notes after I sent my update. You are spot on... this is to add Map#to_h.

@skippy
Copy link
Contributor Author

skippy commented Jul 3, 2015

@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.

@haberman
Copy link
Member

haberman commented Jul 7, 2015

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.

@ebenoist
Copy link
Contributor

Is there any movement on getting this merged? This is still broken in the current beta releases.

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok to test.

@haberman
Copy link
Member

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.

@ebenoist
Copy link
Contributor

If @skippy cannot split this up, I'd be willing to take a look at getting just to_hash working. I was bit by the broken map support for the current Ruby codegen support in the latest beta.

@skippy
Copy link
Contributor Author

skippy commented May 20, 2016

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!

@dim
Copy link

dim commented Jul 11, 2016

Any chance for this PR to be revived? It's a really annoying bug. A 👍 from me

@ebenoist
Copy link
Contributor

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.

@haberman
Copy link
Member

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.

@haberman haberman closed this Jun 20, 2017
taoso pushed a commit to taoso/protobuf that referenced this pull request Aug 1, 2018
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants