From 37fc4327eff474ebd1d5d492f872858ed1dd2f72 Mon Sep 17 00:00:00 2001 From: Rafi Kamal Date: Wed, 12 Feb 2020 12:37:19 -0800 Subject: [PATCH] Fix for wrappers with a zero value (#7195) (#7201) * Add failing tests for issues with wrapped values where the value is the default * Add test for wrapped values without a value set * Bugfix for wrapper types with default values. The previous optimizations for wrapper types had a bug that prevented wrappers from registering as "present" if the "value" field was not present on the wire. In practice the "value" field will not be serialized when it is zero, according to proto3 semantics, but due to the optimization this prevented it from creating a new object to represent the presence of the field. The fix is to ensure that if the wrapper message is present on the wire, we always initialize its value to zero. Co-authored-by: Joshua Haberman Co-authored-by: Dan Quan --- ruby/ext/google/protobuf_c/encode_decode.c | 76 ++++++++++++++------ ruby/tests/basic.rb | 80 ++++++++++++++++++++++ ruby/tests/common_tests.rb | 30 ++++++++ 3 files changed, 165 insertions(+), 21 deletions(-) diff --git a/ruby/ext/google/protobuf_c/encode_decode.c b/ruby/ext/google/protobuf_c/encode_decode.c index 713da435229f..22e3efb47334 100644 --- a/ruby/ext/google/protobuf_c/encode_decode.c +++ b/ruby/ext/google/protobuf_c/encode_decode.c @@ -44,6 +44,23 @@ VALUE noleak_rb_str_cat(VALUE rb_str, const char *str, long len) { return rb_str; } +bool is_wrapper(const upb_msgdef* m) { + switch (upb_msgdef_wellknowntype(m)) { + case UPB_WELLKNOWN_DOUBLEVALUE: + case UPB_WELLKNOWN_FLOATVALUE: + case UPB_WELLKNOWN_INT64VALUE: + case UPB_WELLKNOWN_UINT64VALUE: + case UPB_WELLKNOWN_INT32VALUE: + case UPB_WELLKNOWN_UINT32VALUE: + case UPB_WELLKNOWN_STRINGVALUE: + case UPB_WELLKNOWN_BYTESVALUE: + case UPB_WELLKNOWN_BOOLVALUE: + return true; + default: + return false; + } +} + // The code below also comes from upb's prototype Ruby binding, developed by // haberman@. @@ -117,19 +134,26 @@ static const void* newhandlerdata(upb_handlers* h, uint32_t ofs, int32_t hasbit) typedef struct { size_t ofs; int32_t hasbit; + upb_fieldtype_t wrapped_type; // Only for wrappers. VALUE subklass; } submsg_handlerdata_t; // Creates a handlerdata that contains offset and submessage type information. static const void *newsubmsghandlerdata(upb_handlers* h, + const upb_fielddef *f, uint32_t ofs, int32_t hasbit, VALUE subklass) { submsg_handlerdata_t *hd = ALLOC(submsg_handlerdata_t); + const upb_msgdef *subm = upb_fielddef_msgsubdef(f); hd->ofs = ofs; hd->hasbit = hasbit; hd->subklass = subklass; upb_handlers_addcleanup(h, hd, xfree); + if (is_wrapper(subm)) { + const upb_fielddef *value_f = upb_msgdef_itof(subm, 1); + hd->wrapped_type = upb_fielddef_type(value_f); + } return hd; } @@ -310,12 +334,39 @@ static void *submsg_handler(void *closure, const void *hd) { } static void* startwrapper(void* closure, const void* hd) { - char* msg = closure; const submsg_handlerdata_t* submsgdata = hd; + char* msg = closure; + VALUE* field = (VALUE*)(msg + submsgdata->ofs); set_hasbit(closure, submsgdata->hasbit); - return msg + submsgdata->ofs; + switch (submsgdata->wrapped_type) { + case UPB_TYPE_FLOAT: + case UPB_TYPE_DOUBLE: + *field = DBL2NUM(0); + break; + case UPB_TYPE_BOOL: + *field = Qfalse; + break; + case UPB_TYPE_STRING: + *field = get_frozen_string(NULL, 0, false); + break; + case UPB_TYPE_BYTES: + *field = get_frozen_string(NULL, 0, true); + break; + case UPB_TYPE_ENUM: + case UPB_TYPE_INT32: + case UPB_TYPE_INT64: + case UPB_TYPE_UINT32: + case UPB_TYPE_UINT64: + *field = INT2NUM(0); + break; + case UPB_TYPE_MESSAGE: + rb_raise(rb_eRuntimeError, + "Internal logic error with well-known types."); + } + + return field; } // Handler data for startmap/endmap handlers. @@ -522,23 +573,6 @@ static void* oneof_startwrapper(void* closure, const void* hd) { return msg + oneofdata->ofs; } -bool is_wrapper(const upb_msgdef* m) { - switch (upb_msgdef_wellknowntype(m)) { - case UPB_WELLKNOWN_DOUBLEVALUE: - case UPB_WELLKNOWN_FLOATVALUE: - case UPB_WELLKNOWN_INT64VALUE: - case UPB_WELLKNOWN_UINT64VALUE: - case UPB_WELLKNOWN_INT32VALUE: - case UPB_WELLKNOWN_UINT32VALUE: - case UPB_WELLKNOWN_STRINGVALUE: - case UPB_WELLKNOWN_BYTESVALUE: - case UPB_WELLKNOWN_BOOLVALUE: - return true; - default: - return false; - } -} - // Set up handlers for a repeated field. static void add_handlers_for_repeated_field(upb_handlers *h, const Descriptor* desc, @@ -579,7 +613,7 @@ static void add_handlers_for_repeated_field(upb_handlers *h, case UPB_TYPE_MESSAGE: { VALUE subklass = field_type_class(desc->layout, f); upb_handlerattr attr = UPB_HANDLERATTR_INIT; - attr.handler_data = newsubmsghandlerdata(h, 0, -1, subklass); + attr.handler_data = newsubmsghandlerdata(h, f, 0, -1, subklass); if (is_wrapper(upb_fielddef_msgsubdef(f))) { upb_handlers_setstartsubmsg(h, f, appendwrapper_handler, &attr); } else { @@ -708,7 +742,7 @@ static void add_handlers_for_singular_field(const Descriptor* desc, case UPB_TYPE_MESSAGE: { upb_handlerattr attr = UPB_HANDLERATTR_INIT; attr.handler_data = newsubmsghandlerdata( - h, offset, hasbit, field_type_class(desc->layout, f)); + h, f, offset, hasbit, field_type_class(desc->layout, f)); if (is_wrapper(upb_fielddef_msgsubdef(f))) { upb_handlers_setstartsubmsg(h, f, startwrapper, &attr); } else { diff --git a/ruby/tests/basic.rb b/ruby/tests/basic.rb index 1cb8d3463ed4..d1a66a6169c3 100644 --- a/ruby/tests/basic.rb +++ b/ruby/tests/basic.rb @@ -276,6 +276,86 @@ def test_map_wrappers assert_equal m5, m end + def test_map_wrappers_with_default_values + run_asserts = ->(m) { + assert_equal 0.0, m.map_double[0].value + assert_equal 0.0, m.map_float[0].value + assert_equal 0, m.map_int32[0].value + assert_equal 0, m.map_int64[0].value + assert_equal 0, m.map_uint32[0].value + assert_equal 0, m.map_uint64[0].value + assert_equal false, m.map_bool[0].value + assert_equal '', m.map_string[0].value + assert_equal '', m.map_bytes[0].value + } + + m = proto_module::Wrapper.new( + map_double: {0 => Google::Protobuf::DoubleValue.new(value: 0.0)}, + map_float: {0 => Google::Protobuf::FloatValue.new(value: 0.0)}, + map_int32: {0 => Google::Protobuf::Int32Value.new(value: 0)}, + map_int64: {0 => Google::Protobuf::Int64Value.new(value: 0)}, + map_uint32: {0 => Google::Protobuf::UInt32Value.new(value: 0)}, + map_uint64: {0 => Google::Protobuf::UInt64Value.new(value: 0)}, + map_bool: {0 => Google::Protobuf::BoolValue.new(value: false)}, + map_string: {0 => Google::Protobuf::StringValue.new(value: '')}, + map_bytes: {0 => Google::Protobuf::BytesValue.new(value: '')}, + ) + + run_asserts.call(m) + serialized = proto_module::Wrapper::encode(m) + m2 = proto_module::Wrapper::decode(serialized) + run_asserts.call(m2) + + # Test the case where we are serializing directly from the parsed form + # (before anything lazy is materialized). + m3 = proto_module::Wrapper::decode(serialized) + serialized2 = proto_module::Wrapper::encode(m3) + m4 = proto_module::Wrapper::decode(serialized2) + run_asserts.call(m4) + + # Test that the lazy form compares equal to the expanded form. + m5 = proto_module::Wrapper::decode(serialized2) + assert_equal m5, m + end + + def test_map_wrappers_with_no_value + run_asserts = ->(m) { + assert_equal 0.0, m.map_double[0].value + assert_equal 0.0, m.map_float[0].value + assert_equal 0, m.map_int32[0].value + assert_equal 0, m.map_int64[0].value + assert_equal 0, m.map_uint32[0].value + assert_equal 0, m.map_uint64[0].value + assert_equal false, m.map_bool[0].value + assert_equal '', m.map_string[0].value + assert_equal '', m.map_bytes[0].value + } + + m = proto_module::Wrapper.new( + map_double: {0 => Google::Protobuf::DoubleValue.new()}, + map_float: {0 => Google::Protobuf::FloatValue.new()}, + map_int32: {0 => Google::Protobuf::Int32Value.new()}, + map_int64: {0 => Google::Protobuf::Int64Value.new()}, + map_uint32: {0 => Google::Protobuf::UInt32Value.new()}, + map_uint64: {0 => Google::Protobuf::UInt64Value.new()}, + map_bool: {0 => Google::Protobuf::BoolValue.new()}, + map_string: {0 => Google::Protobuf::StringValue.new()}, + map_bytes: {0 => Google::Protobuf::BytesValue.new()}, + ) + run_asserts.call(m) + + serialized = proto_module::Wrapper::encode(m) + m2 = proto_module::Wrapper::decode(serialized) + run_asserts.call(m2) + + # Test the case where we are serializing directly from the parsed form + # (before anything lazy is materialized). + m3 = proto_module::Wrapper::decode(serialized) + serialized2 = proto_module::Wrapper::encode(m3) + m4 = proto_module::Wrapper::decode(serialized2) + run_asserts.call(m4) + end + def test_concurrent_decoding o = Outer.new o.items[0] = Inner.new diff --git a/ruby/tests/common_tests.rb b/ruby/tests/common_tests.rb index ada42432ebbc..ee05b856dc6e 100644 --- a/ruby/tests/common_tests.rb +++ b/ruby/tests/common_tests.rb @@ -1265,6 +1265,36 @@ def test_comparison_with_arbitrary_object assert proto_module::TestMessage.new != nil end + def test_wrappers_set_to_default + run_asserts = ->(m) { + assert_equal 0.0, m.double.value + assert_equal 0.0, m.float.value + assert_equal 0, m.int32.value + assert_equal 0, m.int64.value + assert_equal 0, m.uint32.value + assert_equal 0, m.uint64.value + assert_equal false, m.bool.value + assert_equal '', m.string.value + assert_equal '', m.bytes.value + } + + m = proto_module::Wrapper.new( + double: Google::Protobuf::DoubleValue.new(value: 0.0), + float: Google::Protobuf::FloatValue.new(value: 0.0), + int32: Google::Protobuf::Int32Value.new(value: 0), + int64: Google::Protobuf::Int64Value.new(value: 0), + uint32: Google::Protobuf::UInt32Value.new(value: 0), + uint64: Google::Protobuf::UInt64Value.new(value: 0), + bool: Google::Protobuf::BoolValue.new(value: false), + string: Google::Protobuf::StringValue.new(value: ""), + bytes: Google::Protobuf::BytesValue.new(value: ''), + ) + + run_asserts.call(m) + m2 = proto_module::Wrapper.decode(m.to_proto) + run_asserts.call(m2) + end + def test_wrapper_getters run_asserts = ->(m) { assert_equal 2.0, m.double_as_value