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

[Ruby] allow encode json options to be an object that responds to to_hash #9513

Merged
merged 2 commits into from Mar 8, 2022
Merged

[Ruby] allow encode json options to be an object that responds to to_hash #9513

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7c4e56c
allow encode json options to be an object that responds to to_hash
lukad Feb 16, 2022
5431081
try to convert options arg to hash if it is not a hash
lukad Feb 17, 2022
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
6 changes: 5 additions & 1 deletion ruby/ext/google/protobuf_c/message.c
View file
Open in desktop
Expand Up @@ -1141,7 +1141,11 @@ static VALUE Message_encode_json(int argc, VALUE* argv, VALUE klass) {
if (argc == 2) {
VALUE hash_args = argv[1];
if (TYPE(hash_args) != T_HASH) {
rb_raise(rb_eArgError, "Expected hash arguments.");
if (RTEST(rb_funcall(hash_args, rb_intern("respond_to?"), 1, rb_str_new2("to_h")))) {
hash_args = rb_funcall(hash_args, rb_intern("to_h"), 0);
} else {
rb_raise(rb_eArgError, "Expected hash arguments.");
}
}

if (RTEST(rb_hash_lookup2(hash_args,
Expand Down
10 changes: 9 additions & 1 deletion ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java
View file
Open in desktop
Expand Up @@ -555,7 +555,15 @@ public static IRubyObject encodeJson(ThreadContext context, IRubyObject recv, IR
String result;

if (args.length > 1) {
RubyHash options = (RubyHash) args[1];
RubyHash options;
if (args[1] instanceof RubyHash) {
options = (RubyHash) args[1];
} else if (args[1].respondsTo("to_h")) {
options = (RubyHash) args[1].callMethod(context, "to_h");
} else {
throw runtime.newArgumentError("Expected hash arguments.");
}

IRubyObject emitDefaults = options.fastARef(runtime.newSymbol("emit_defaults"));
IRubyObject preserveNames = options.fastARef(runtime.newSymbol("preserve_proto_fieldnames"));

Expand Down
3 changes: 3 additions & 0 deletions ruby/tests/common_tests.rb
View file
Open in desktop
Expand Up @@ -870,6 +870,9 @@ def test_protobuf_encode_decode_json_helpers

decoded_msg = Google::Protobuf.decode_json(proto_module::TestMessage, encoded_msg)
assert_equal proto_module::TestMessage.decode_json(m.to_json), decoded_msg

assert_equal [m].to_json, Google::Protobuf.encode_json([m])
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I am missing something, but this test doesn't seem to exercise the new code. We need a test that calls encode_json with a second parameter that response to to_h but is not itself a Hash.

Copy link
Contributor Author

@lukad lukad Mar 2, 2022 •
edited

Choose a reason for hiding this comment

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

[m].to_json will do exactly that. Ruby's #to_json creates a json::Ext::Generator::State which is not a Hash. That state object will then be passed to m.to_json which in turn passes the state to encode_json.

Withouth my changes the assert fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An ArgumentError: Expected hash arguments is raised without my changes.

Error: test_protobuf_encode_decode_json_helpers(BasicTest::MessageContainerTest): ArgumentError: Expected hash arguments.
/Users/luka/code/protobuf/ruby/lib/google/protobuf/message_exts.rb:44:in `encode_json'
/Users/luka/code/protobuf/ruby/lib/google/protobuf/message_exts.rb:44:in `to_json'
/Users/luka/code/protobuf/ruby/tests/common_tests.rb:874:in `to_json'
/Users/luka/code/protobuf/ruby/tests/common_tests.rb:874:in `test_protobuf_encode_decode_json_helpers'
     871:     decoded_msg = Google::Protobuf.decode_json(proto_module::TestMessage, encoded_msg)
     872:     assert_equal proto_module::TestMessage.decode_json(m.to_json), decoded_msg
     873:
  => 874:     assert_equal [m].to_json, Google::Protobuf.encode_json([m])
     875:     assert_equal proto_module::TestMessage.decode_json([m.to_json].first), decoded_msg
     876:   end
     877:

assert_equal proto_module::TestMessage.decode_json([m.to_json].first), decoded_msg
end

def test_def_errors
Expand Down