Skip to content

Commit

Permalink
Breaking Change: fixed json_encode/json_decode to use the message's p…
Browse files Browse the repository at this point in the history
…ool.

This bug arises only in the uncommon case where there is more than one DescriptorPool.  In such a case, JSON encode/decode should always use the pool of the message being encoded/decoded, not the generated pool.

Closes #15281

COPYBARA_INTEGRATE_REVIEW=#15281 from protocolbuffers:ruby-json-pool-fix 91e2dc5
PiperOrigin-RevId: 596027770
  • Loading branch information
haberman authored and copybara-github committed Jan 5, 2024
1 parent 55260d8 commit a8b8ea0
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 10 deletions.
16 changes: 6 additions & 10 deletions ruby/ext/google/protobuf_c/message.c
Original file line number Diff line number Diff line change
Expand Up @@ -975,9 +975,6 @@ static VALUE Message_decode_json(int argc, VALUE* argv, VALUE klass) {
int options = 0;
upb_Status status;

// TODO: use this message's pool instead.
const upb_DefPool* symtab = DescriptorPool_GetSymtab(generated_pool);

if (argc < 1 || argc > 2) {
rb_raise(rb_eArgError, "Expected 1 or 2 arguments.");
}
Expand Down Expand Up @@ -1011,8 +1008,9 @@ static VALUE Message_decode_json(int argc, VALUE* argv, VALUE klass) {
}

upb_Status_Clear(&status);
const upb_DefPool* pool = upb_FileDef_Pool(upb_MessageDef_File(msg->msgdef));
if (!upb_JsonDecode(RSTRING_PTR(data), RSTRING_LEN(data),
(upb_Message*)msg->msg, msg->msgdef, symtab, options,
(upb_Message*)msg->msg, msg->msgdef, pool, options,
Arena_get(msg->arena), &status)) {
rb_raise(cParseError, "Error occurred during parsing: %s",
upb_Status_ErrorMessage(&status));
Expand Down Expand Up @@ -1091,9 +1089,6 @@ static VALUE Message_encode_json(int argc, VALUE* argv, VALUE klass) {
size_t size;
upb_Status status;

// TODO: use this message's pool instead.
const upb_DefPool* symtab = DescriptorPool_GetSymtab(generated_pool);

if (argc < 1 || argc > 2) {
rb_raise(rb_eArgError, "Expected 1 or 2 arguments.");
}
Expand Down Expand Up @@ -1128,8 +1123,9 @@ static VALUE Message_encode_json(int argc, VALUE* argv, VALUE klass) {
}

upb_Status_Clear(&status);
size = upb_JsonEncode(msg->msg, msg->msgdef, symtab, options, buf,
sizeof(buf), &status);
const upb_DefPool* pool = upb_FileDef_Pool(upb_MessageDef_File(msg->msgdef));
size = upb_JsonEncode(msg->msg, msg->msgdef, pool, options, buf, sizeof(buf),
&status);

if (!upb_Status_IsOk(&status)) {
rb_raise(cParseError, "Error occurred during encoding: %s",
Expand All @@ -1139,7 +1135,7 @@ static VALUE Message_encode_json(int argc, VALUE* argv, VALUE klass) {
VALUE ret;
if (size >= sizeof(buf)) {
char* buf2 = malloc(size + 1);
upb_JsonEncode(msg->msg, msg->msgdef, symtab, options, buf2, size + 1,
upb_JsonEncode(msg->msg, msg->msgdef, pool, options, buf2, size + 1,
&status);
ret = rb_str_new(buf2, size);
free(buf2);
Expand Down
39 changes: 39 additions & 0 deletions ruby/tests/basic_proto2.rb
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,45 @@ def test_extension
assert_equal 42, extension.get(message)
end

def test_extension_json
omit "Java Protobuf JsonFormat does not handle Proto2 extensions" if defined? JRUBY_VERSION and :NATIVE == Google::Protobuf::IMPLEMENTATION
message = TestExtensions.decode_json '{"[basic_test_proto2.optional_int32_extension]": 123}'
extension = Google::Protobuf::DescriptorPool.generated_pool.lookup 'basic_test_proto2.optional_int32_extension'
assert_instance_of Google::Protobuf::FieldDescriptor, extension
assert_equal 123, extension.get(message)
end

def test_extension_json_separate_pool
omit "Java Protobuf JsonFormat does not handle Proto2 extensions" if defined? JRUBY_VERSION and :NATIVE == Google::Protobuf::IMPLEMENTATION
pool = Google::Protobuf::DescriptorPool.new

# This serialized descriptor is a subset of basic_test_proto2.proto:
#
# syntax = "proto2";
# package basic_test_proto2;
#
# message TestExtensions {
# extensions 1 to max;
# }
#
# extend TestExtensions {
# # Same extension as basic_test_proto2.proto, but with a different
# # name.
# optional int32 different_optional_int32_extension = 1;
# }
#
descriptor_data = "\n\x17\x62\x61sic_test_proto2.proto\x12\x11\x62\x61sic_test_proto2\"\x1a\n\x0eTestExtensions*\x08\x08\x01\x10\x80\x80\x80\x80\x02:M\n\"different_optional_int32_extension\x12!.basic_test_proto2.TestExtensions\x18\x01 \x01(\x05"
pool.add_serialized_file(descriptor_data)
message_class = pool.lookup("basic_test_proto2.TestExtensions").msgclass
extension = pool.lookup 'basic_test_proto2.different_optional_int32_extension'

message = message_class.decode_json '{"[basic_test_proto2.different_optional_int32_extension]": 123}'
assert_equal 123, extension.get(message)

message2 = message_class.decode_json(message_class.encode_json(message))
assert_equal 123, extension.get(message2)
end

def test_nested_extension
message = TestExtensions.new
extension = Google::Protobuf::DescriptorPool.generated_pool.lookup 'basic_test_proto2.TestNestedExtension.test'
Expand Down

0 comments on commit a8b8ea0

Please sign in to comment.