From 57d47b5989ebc9fdd754077ae612980c25c84cc5 Mon Sep 17 00:00:00 2001 From: Dan Quan Date: Thu, 19 Dec 2019 07:21:26 -0800 Subject: [PATCH 1/3] Add failing tests for issues with wrapped values where the value is the default --- ruby/tests/basic.rb | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/ruby/tests/basic.rb b/ruby/tests/basic.rb index 1cb8d3463ed4..d73ea074855e 100644 --- a/ruby/tests/basic.rb +++ b/ruby/tests/basic.rb @@ -276,6 +276,48 @@ 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_concurrent_decoding o = Outer.new o.items[0] = Inner.new From 1da3ddda7c033f31cee81436f7e8f25ef6ddba54 Mon Sep 17 00:00:00 2001 From: Dan Quan Date: Thu, 19 Dec 2019 07:36:39 -0800 Subject: [PATCH 2/3] Add test for wrapped values without a value set --- ruby/tests/basic.rb | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/ruby/tests/basic.rb b/ruby/tests/basic.rb index d73ea074855e..7d92ee2b79a3 100644 --- a/ruby/tests/basic.rb +++ b/ruby/tests/basic.rb @@ -318,6 +318,43 @@ def test_map_wrappers_with_default_values assert_equal m5, m end + def test_map_wrappers_with_no_value + run_asserts = ->(m) { + assert_equal nil, m.map_double[0] + assert_equal nil, m.map_float[0] + assert_equal nil, m.map_int32[0] + assert_equal nil, m.map_int64[0] + assert_equal nil, m.map_uint32[0] + assert_equal nil, m.map_uint64[0] + assert_equal nil, m.map_bool[0] + assert_equal nil, m.map_string[0] + assert_equal nil, m.map_bytes[0] + } + + 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()}, + ) + + 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 From aa62233e2e4ca1f42e486c3cdaf05a7224198b91 Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Mon, 10 Feb 2020 19:18:33 -0800 Subject: [PATCH 3/3] 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. --- ruby/ext/google/protobuf_c/encode_decode.c | 76 ++++++++++++++++------ ruby/tests/basic.rb | 19 +++--- ruby/tests/common_tests.rb | 30 +++++++++ 3 files changed, 95 insertions(+), 30 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 7d92ee2b79a3..d1a66a6169c3 100644 --- a/ruby/tests/basic.rb +++ b/ruby/tests/basic.rb @@ -320,15 +320,15 @@ def test_map_wrappers_with_default_values def test_map_wrappers_with_no_value run_asserts = ->(m) { - assert_equal nil, m.map_double[0] - assert_equal nil, m.map_float[0] - assert_equal nil, m.map_int32[0] - assert_equal nil, m.map_int64[0] - assert_equal nil, m.map_uint32[0] - assert_equal nil, m.map_uint64[0] - assert_equal nil, m.map_bool[0] - assert_equal nil, m.map_string[0] - assert_equal nil, m.map_bytes[0] + 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( @@ -342,6 +342,7 @@ def test_map_wrappers_with_no_value 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) 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