From 969d245bd3e8187f89e7923797bc397d583e0abb Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Mon, 21 Oct 2019 07:27:36 -0700 Subject: [PATCH 1/7] WIP: first steps towards lazily creating wrappers. --- ruby/ext/google/protobuf_c/encode_decode.c | 138 ++++++++++++++++++++- ruby/ext/google/protobuf_c/message.c | 46 ++++--- ruby/tests/common_tests.rb | 62 ++++++--- 3 files changed, 205 insertions(+), 41 deletions(-) diff --git a/ruby/ext/google/protobuf_c/encode_decode.c b/ruby/ext/google/protobuf_c/encode_decode.c index 36d818b4c63b..6841c2656023 100644 --- a/ruby/ext/google/protobuf_c/encode_decode.c +++ b/ruby/ext/google/protobuf_c/encode_decode.c @@ -298,6 +298,15 @@ static void *submsg_handler(void *closure, const void *hd) { return submsg; } +static void* startwrapper(void* closure, const void* hd) { + char* msg = closure; + const submsg_handlerdata_t* submsgdata = hd; + + set_hasbit(closure, submsgdata->hasbit); + + return msg + submsgdata->ofs; +} + // Handler data for startmap/endmap handlers. typedef struct { size_t ofs; @@ -541,6 +550,85 @@ static void add_handlers_for_repeated_field(upb_handlers *h, } } +static bool doublewrapper_handler(void* closure, const void* hd, double val) { + VALUE* rbval = closure; + *rbval = DBL2NUM(val); + return true; +} + +static bool floatwrapper_handler(void* closure, const void* hd, float val) { + VALUE* rbval = closure; + *rbval = DBL2NUM(val); + return true; +} + +static bool int64wrapper_handler(void* closure, const void* hd, int64_t val) { + VALUE* rbval = closure; + *rbval = LL2NUM(val); + return true; +} + +static bool uint64wrapper_handler(void* closure, const void* hd, uint64_t val) { + VALUE* rbval = closure; + *rbval = ULL2NUM(val); + return true; +} + +static bool int32wrapper_handler(void* closure, const void* hd, int32_t val) { + VALUE* rbval = closure; + *rbval = INT2NUM(val); + return true; +} + +static bool uint32wrapper_handler(void* closure, const void* hd, uint32_t val) { + VALUE* rbval = closure; + *rbval = UINT2NUM(val); + return true; +} + +static size_t stringwrapper_handler(void* closure, const void* hd, + const char* ptr, size_t len, + const upb_bufhandle* handle) { + VALUE* rbval = closure; + *rbval = get_frozen_string(ptr, len, false); + return len; +} + +static size_t byteswrapper_handler(void* closure, const void* hd, + const char* ptr, size_t len, + const upb_bufhandle* handle) { + VALUE* rbval = closure; + *rbval = get_frozen_string(ptr, len, true); + return len; +} + +static bool boolwrapper_handler(void* closure, const void* hd, bool val) { + VALUE* rbval = closure; + if (val) { + *rbval = Qtrue; + } else { + *rbval = Qfalse; + } + return true; +} + +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 singular field. static void add_handlers_for_singular_field(const Descriptor* desc, upb_handlers* h, @@ -580,8 +668,11 @@ static void add_handlers_for_singular_field(const Descriptor* desc, upb_handlerattr attr = UPB_HANDLERATTR_INIT; attr.handler_data = newsubmsghandlerdata( h, offset, hasbit, field_type_class(desc->layout, f)); - upb_handlers_setstartsubmsg(h, f, submsg_handler, &attr); - break; + if (is_wrapper(upb_fielddef_msgsubdef(f))) { + upb_handlers_setstartsubmsg(h, f, startwrapper, &attr); + } else { + upb_handlers_setstartsubmsg(h, f, submsg_handler, &attr); + } } } } @@ -623,6 +714,43 @@ static void add_handlers_for_mapentry(const upb_msgdef* msgdef, upb_handlers* h, MESSAGE_FIELD_NO_HASBIT); } +static void add_handlers_for_wrapper(const upb_msgdef* msgdef, + upb_handlers* h) { + const upb_fielddef* f = upb_msgdef_itof(msgdef, 1); + switch (upb_msgdef_wellknowntype(msgdef)) { + case UPB_WELLKNOWN_DOUBLEVALUE: + upb_handlers_setdouble(h, f, doublewrapper_handler, NULL); + break; + case UPB_WELLKNOWN_FLOATVALUE: + upb_handlers_setfloat(h, f, floatwrapper_handler, NULL); + break; + case UPB_WELLKNOWN_INT64VALUE: + upb_handlers_setint64(h, f, int64wrapper_handler, NULL); + break; + case UPB_WELLKNOWN_UINT64VALUE: + upb_handlers_setuint64(h, f, uint64wrapper_handler, NULL); + break; + case UPB_WELLKNOWN_INT32VALUE: + upb_handlers_setint32(h, f, int32wrapper_handler, NULL); + break; + case UPB_WELLKNOWN_UINT32VALUE: + upb_handlers_setuint32(h, f, uint32wrapper_handler, NULL); + break; + case UPB_WELLKNOWN_STRINGVALUE: + upb_handlers_setstring(h, f, stringwrapper_handler, NULL); + break; + case UPB_WELLKNOWN_BYTESVALUE: + upb_handlers_setstring(h, f, byteswrapper_handler, NULL); + break; + case UPB_WELLKNOWN_BOOLVALUE: + upb_handlers_setbool(h, f, boolwrapper_handler, NULL); + return; + default: + rb_raise(rb_eRuntimeError, + "Internal logic error with well-known types."); + } +} + // Set up handlers for a oneof field. static void add_handlers_for_oneof_field(upb_handlers *h, const upb_fielddef *f, @@ -706,6 +834,12 @@ void add_handlers_for_message(const void *closure, upb_handlers *h) { return; } + // If this is a wrapper type, use special handlers and bail. + if (is_wrapper(msgdef)) { + add_handlers_for_wrapper(msgdef, h); + return; + } + upb_handlers_setunknown(h, unknown_field_handler, &attr); for (upb_msg_field_begin(&i, desc->msgdef); diff --git a/ruby/ext/google/protobuf_c/message.c b/ruby/ext/google/protobuf_c/message.c index e3730d280be7..2f332ed947e6 100644 --- a/ruby/ext/google/protobuf_c/message.c +++ b/ruby/ext/google/protobuf_c/message.c @@ -62,13 +62,12 @@ VALUE Message_alloc(VALUE klass) { Descriptor* desc = ruby_to_Descriptor(descriptor); MessageHeader* msg; VALUE ret; - size_t size; if (desc->layout == NULL) { create_layout(desc); } - msg = ALLOC_N(uint8_t, sizeof(MessageHeader) + desc->layout->size); + msg = (void*)ALLOC_N(uint8_t, sizeof(MessageHeader) + desc->layout->size); msg->descriptor = desc; msg->unknown_fields = NULL; memcpy(Message_data(msg), desc->layout->empty_template, desc->layout->size); @@ -109,25 +108,28 @@ enum { }; // Check if the field is a well known wrapper type -static bool is_wrapper_type_field(const MessageLayout* layout, - const upb_fielddef* field) { - const char* field_type_name = rb_class2name(field_type_class(layout, field)); - - return strcmp(field_type_name, "Google::Protobuf::DoubleValue") == 0 || - strcmp(field_type_name, "Google::Protobuf::FloatValue") == 0 || - strcmp(field_type_name, "Google::Protobuf::Int32Value") == 0 || - strcmp(field_type_name, "Google::Protobuf::Int64Value") == 0 || - strcmp(field_type_name, "Google::Protobuf::UInt32Value") == 0 || - strcmp(field_type_name, "Google::Protobuf::UInt64Value") == 0 || - strcmp(field_type_name, "Google::Protobuf::BoolValue") == 0 || - strcmp(field_type_name, "Google::Protobuf::StringValue") == 0 || - strcmp(field_type_name, "Google::Protobuf::BytesValue") == 0; +static bool is_wrapper_type_field(const upb_fielddef* field) { + const upb_msgdef *m = upb_fielddef_msgsubdef(field); + 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; + } } // Get a new Ruby wrapper type and set the initial value static VALUE ruby_wrapper_type(const MessageLayout* layout, const upb_fielddef* field, const VALUE value) { - if (is_wrapper_type_field(layout, field) && value != Qnil) { + if (is_wrapper_type_field(field) && value != Qnil) { VALUE hash = rb_hash_new(); rb_hash_aset(hash, rb_str_new2("value"), value); { @@ -194,7 +196,7 @@ static int extract_method_call(VALUE method_name, MessageHeader* self, if (upb_msgdef_lookupname(self->descriptor->msgdef, wrapper_field_name, name_len - 9, &test_f_wrapper, &test_o_wrapper) && upb_fielddef_type(test_f_wrapper) == UPB_TYPE_MESSAGE && - is_wrapper_type_field(self->descriptor->layout, test_f_wrapper)) { + is_wrapper_type_field(test_f_wrapper)) { // It does exist! has_field = true; if (accessor_type == METHOD_SETTER) { @@ -329,10 +331,14 @@ VALUE Message_method_missing(int argc, VALUE* argv, VALUE _self) { return layout_has(self->descriptor->layout, Message_data(self), f); } else if (accessor_type == METHOD_WRAPPER_GETTER) { VALUE value = layout_get(self->descriptor->layout, Message_data(self), f); - if (value != Qnil) { - value = rb_funcall(value, rb_intern("value"), 0); + switch (TYPE(value)) { + case T_DATA: + return rb_funcall(value, rb_intern("value"), 0); + case T_NIL: + return Qnil; + default: + return value; } - return value; } else if (accessor_type == METHOD_WRAPPER_SETTER) { VALUE wrapper = ruby_wrapper_type(self->descriptor->layout, f, argv[1]); layout_set(self->descriptor->layout, Message_data(self), f, wrapper); diff --git a/ruby/tests/common_tests.rb b/ruby/tests/common_tests.rb index 1417f5be328f..d64fc8d796a7 100644 --- a/ruby/tests/common_tests.rb +++ b/ruby/tests/common_tests.rb @@ -1266,6 +1266,44 @@ def test_comparison_with_arbitrary_object end def test_wrapper_getters + run_asserts = ->(m) { + assert_equal 2.0, m.double_as_value + assert_equal 2.0, m.double.value + assert_equal 2.0, m.double_as_value + + assert_equal 4.0, m.float_as_value + assert_equal 4.0, m.float.value + assert_equal 4.0, m.float_as_value + + assert_equal 3, m.int32_as_value + assert_equal 3, m.int32.value + assert_equal 3, m.int32_as_value + + assert_equal 4, m.int64_as_value + assert_equal 4, m.int64.value + assert_equal 4, m.int64_as_value + + assert_equal 5, m.uint32_as_value + assert_equal 5, m.uint32.value + assert_equal 5, m.uint32_as_value + + assert_equal 6, m.uint64_as_value + assert_equal 6, m.uint64.value + assert_equal 6, m.uint64_as_value + + assert_equal true, m.bool_as_value + assert_equal true, m.bool.value + assert_equal true, m.bool_as_value + + assert_equal 'str', m.string_as_value + assert_equal 'str', m.string.value + assert_equal 'str', m.string_as_value + + assert_equal 'fun', m.bytes_as_value + assert_equal 'fun', m.bytes.value + assert_equal 'fun', m.bytes_as_value + } + m = proto_module::Wrapper.new( double: Google::Protobuf::DoubleValue.new(value: 2.0), float: Google::Protobuf::FloatValue.new(value: 4.0), @@ -1279,24 +1317,10 @@ def test_wrapper_getters real_string: '100' ) - assert_equal 2.0, m.double_as_value - assert_equal 2.0, m.double.value - assert_equal 4.0, m.float_as_value - assert_equal 4.0, m.float.value - assert_equal 3, m.int32_as_value - assert_equal 3, m.int32.value - assert_equal 4, m.int64_as_value - assert_equal 4, m.int64.value - assert_equal 5, m.uint32_as_value - assert_equal 5, m.uint32.value - assert_equal 6, m.uint64_as_value - assert_equal 6, m.uint64.value - assert_equal true, m.bool_as_value - assert_equal true, m.bool.value - assert_equal 'str', m.string_as_value - assert_equal 'str', m.string.value - assert_equal 'fun', m.bytes_as_value - assert_equal 'fun', m.bytes.value + run_asserts.call(m) + serialized = proto_module::Wrapper::encode(m) + m2 = proto_module::Wrapper::decode(serialized) + run_asserts.call(m2) end def test_wrapper_setters_as_value @@ -1443,7 +1467,7 @@ def test_wrappers_only assert_raise(NoMethodError) { m.string_XXXXXXXXX } assert_raise(NoMethodError) { m.string_XXXXXXXXXX } end - + def test_converts_time m = proto_module::TimeMessage.new From 9cfb12bf0a694b2a0a9dc0e05a6603f6ab613390 Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Mon, 21 Oct 2019 09:19:19 -0700 Subject: [PATCH 2/7] Tests pass for all common operations. A few things that don't work or aren't tested yet: - wrappers at the top level - equality checking for not-yet-expanded wrappers. --- ruby/ext/google/protobuf_c/encode_decode.c | 11 +- ruby/ext/google/protobuf_c/message.c | 13 +- ruby/ext/google/protobuf_c/protobuf.h | 4 + ruby/ext/google/protobuf_c/storage.c | 11 +- ruby/tests/common_tests.rb | 276 +++++++++++++-------- 5 files changed, 195 insertions(+), 120 deletions(-) diff --git a/ruby/ext/google/protobuf_c/encode_decode.c b/ruby/ext/google/protobuf_c/encode_decode.c index 6841c2656023..d85cb18cced8 100644 --- a/ruby/ext/google/protobuf_c/encode_decode.c +++ b/ruby/ext/google/protobuf_c/encode_decode.c @@ -1440,8 +1440,14 @@ static void putmsg(VALUE msg_rb, const Descriptor* desc, putstr(str, f, sink); } } else if (upb_fielddef_issubmsg(f)) { - putsubmsg(DEREF(msg, offset, VALUE), f, sink, depth, - emit_defaults, is_json); + VALUE val = DEREF(msg, offset, VALUE); + int type = TYPE(val); + if (type != T_DATA && type != T_NIL && is_wrapper_type_field(f)) { + // OPT: could try to avoid expanding the wrapper here. + val = ruby_wrapper_type(desc->layout, f, val); + DEREF(msg, offset, VALUE) = val; + } + putsubmsg(val, f, sink, depth, emit_defaults, is_json); } else { upb_selector_t sel = getsel(f, upb_handlers_getprimitivehandlertype(f)); @@ -1475,7 +1481,6 @@ static void putmsg(VALUE msg_rb, const Descriptor* desc, } #undef T - } } diff --git a/ruby/ext/google/protobuf_c/message.c b/ruby/ext/google/protobuf_c/message.c index 2f332ed947e6..ba93c58dfd51 100644 --- a/ruby/ext/google/protobuf_c/message.c +++ b/ruby/ext/google/protobuf_c/message.c @@ -108,8 +108,12 @@ enum { }; // Check if the field is a well known wrapper type -static bool is_wrapper_type_field(const upb_fielddef* field) { - const upb_msgdef *m = upb_fielddef_msgsubdef(field); +bool is_wrapper_type_field(const upb_fielddef* field) { + const upb_msgdef *m; + if (upb_fielddef_type(field) != UPB_TYPE_MESSAGE) { + return false; + } + m = upb_fielddef_msgsubdef(field); switch (upb_msgdef_wellknowntype(m)) { case UPB_WELLKNOWN_DOUBLEVALUE: case UPB_WELLKNOWN_FLOATVALUE: @@ -127,8 +131,8 @@ static bool is_wrapper_type_field(const upb_fielddef* field) { } // Get a new Ruby wrapper type and set the initial value -static VALUE ruby_wrapper_type(const MessageLayout* layout, - const upb_fielddef* field, const VALUE value) { +VALUE ruby_wrapper_type(const MessageLayout* layout, const upb_fielddef* field, + const VALUE value) { if (is_wrapper_type_field(field) && value != Qnil) { VALUE hash = rb_hash_new(); rb_hash_aset(hash, rb_str_new2("value"), value); @@ -195,7 +199,6 @@ static int extract_method_call(VALUE method_name, MessageHeader* self, // Check if field exists and is a wrapper type if (upb_msgdef_lookupname(self->descriptor->msgdef, wrapper_field_name, name_len - 9, &test_f_wrapper, &test_o_wrapper) && - upb_fielddef_type(test_f_wrapper) == UPB_TYPE_MESSAGE && is_wrapper_type_field(test_f_wrapper)) { // It does exist! has_field = true; diff --git a/ruby/ext/google/protobuf_c/protobuf.h b/ruby/ext/google/protobuf_c/protobuf.h index 6314b788106c..109fd0f1619b 100644 --- a/ruby/ext/google/protobuf_c/protobuf.h +++ b/ruby/ext/google/protobuf_c/protobuf.h @@ -556,6 +556,10 @@ VALUE layout_eq(MessageLayout* layout, void* msg1, void* msg2); VALUE layout_hash(MessageLayout* layout, void* storage); VALUE layout_inspect(MessageLayout* layout, void* storage); +bool is_wrapper_type_field(const upb_fielddef* field); +VALUE ruby_wrapper_type(const MessageLayout* layout, const upb_fielddef* field, + const VALUE value); + // ----------------------------------------------------------------------------- // Message class creation. // ----------------------------------------------------------------------------- diff --git a/ruby/ext/google/protobuf_c/storage.c b/ruby/ext/google/protobuf_c/storage.c index 861fb0255335..100b85350fcc 100644 --- a/ruby/ext/google/protobuf_c/storage.c +++ b/ruby/ext/google/protobuf_c/storage.c @@ -843,8 +843,15 @@ VALUE layout_get(MessageLayout* layout, } else if (!field_set) { return layout_get_default(field); } else { - return native_slot_get(upb_fielddef_type(field), - field_type_class(layout, field), memory); + VALUE type_class = field_type_class(layout, field); + VALUE val = native_slot_get(upb_fielddef_type(field), type_class, memory); + int type = TYPE(val); + if (type != T_DATA && type != T_NIL && is_wrapper_type_field(field)) { + val = ruby_wrapper_type(layout, field, val); + native_slot_set(upb_fielddef_name(field), upb_fielddef_type(field), + type_class, memory, val); + } + return val; } } diff --git a/ruby/tests/common_tests.rb b/ruby/tests/common_tests.rb index d64fc8d796a7..78179b6e4a92 100644 --- a/ruby/tests/common_tests.rb +++ b/ruby/tests/common_tests.rb @@ -1321,126 +1321,182 @@ def test_wrapper_getters 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) + + # This doesn't work yet. + # assert_equal m5, m end def test_wrapper_setters_as_value + run_asserts = ->(m) { + m.double_as_value = 4.8 + assert_equal 4.8, m.double_as_value + assert_equal Google::Protobuf::DoubleValue.new(value: 4.8), m.double + m.float_as_value = 2.4 + assert_in_delta 2.4, m.float_as_value + assert_in_delta Google::Protobuf::FloatValue.new(value: 2.4).value, m.float.value + m.int32_as_value = 5 + assert_equal 5, m.int32_as_value + assert_equal Google::Protobuf::Int32Value.new(value: 5), m.int32 + m.int64_as_value = 15 + assert_equal 15, m.int64_as_value + assert_equal Google::Protobuf::Int64Value.new(value: 15), m.int64 + m.uint32_as_value = 50 + assert_equal 50, m.uint32_as_value + assert_equal Google::Protobuf::UInt32Value.new(value: 50), m.uint32 + m.uint64_as_value = 500 + assert_equal 500, m.uint64_as_value + assert_equal Google::Protobuf::UInt64Value.new(value: 500), m.uint64 + m.bool_as_value = false + assert_equal false, m.bool_as_value + assert_equal Google::Protobuf::BoolValue.new(value: false), m.bool + m.string_as_value = 'xy' + assert_equal 'xy', m.string_as_value + assert_equal Google::Protobuf::StringValue.new(value: 'xy'), m.string + m.bytes_as_value = '123' + assert_equal '123', m.bytes_as_value + assert_equal Google::Protobuf::BytesValue.new(value: '123'), m.bytes + + m.double_as_value = nil + assert_nil m.double + assert_nil m.double_as_value + m.float_as_value = nil + assert_nil m.float + assert_nil m.float_as_value + m.int32_as_value = nil + assert_nil m.int32 + assert_nil m.int32_as_value + m.int64_as_value = nil + assert_nil m.int64 + assert_nil m.int64_as_value + m.uint32_as_value = nil + assert_nil m.uint32 + assert_nil m.uint32_as_value + m.uint64_as_value = nil + assert_nil m.uint64 + assert_nil m.uint64_as_value + m.bool_as_value = nil + assert_nil m.bool + assert_nil m.bool_as_value + m.string_as_value = nil + assert_nil m.string + assert_nil m.string_as_value + m.bytes_as_value = nil + assert_nil m.bytes + assert_nil m.bytes_as_value + } + m = proto_module::Wrapper.new - m.double_as_value = 4.8 - assert_equal 4.8, m.double_as_value - assert_equal Google::Protobuf::DoubleValue.new(value: 4.8), m.double - m.float_as_value = 2.4 - assert_in_delta 2.4, m.float_as_value - assert_in_delta Google::Protobuf::FloatValue.new(value: 2.4).value, m.float.value - m.int32_as_value = 5 - assert_equal 5, m.int32_as_value - assert_equal Google::Protobuf::Int32Value.new(value: 5), m.int32 - m.int64_as_value = 15 - assert_equal 15, m.int64_as_value - assert_equal Google::Protobuf::Int64Value.new(value: 15), m.int64 - m.uint32_as_value = 50 - assert_equal 50, m.uint32_as_value - assert_equal Google::Protobuf::UInt32Value.new(value: 50), m.uint32 - m.uint64_as_value = 500 - assert_equal 500, m.uint64_as_value - assert_equal Google::Protobuf::UInt64Value.new(value: 500), m.uint64 - m.bool_as_value = false - assert_equal false, m.bool_as_value - assert_equal Google::Protobuf::BoolValue.new(value: false), m.bool - m.string_as_value = 'xy' - assert_equal 'xy', m.string_as_value - assert_equal Google::Protobuf::StringValue.new(value: 'xy'), m.string - m.bytes_as_value = '123' - assert_equal '123', m.bytes_as_value - assert_equal Google::Protobuf::BytesValue.new(value: '123'), m.bytes - - m.double_as_value = nil - assert_nil m.double - assert_nil m.double_as_value - m.float_as_value = nil - assert_nil m.float - assert_nil m.float_as_value - m.int32_as_value = nil - assert_nil m.int32 - assert_nil m.int32_as_value - m.int64_as_value = nil - assert_nil m.int64 - assert_nil m.int64_as_value - m.uint32_as_value = nil - assert_nil m.uint32 - assert_nil m.uint32_as_value - m.uint64_as_value = nil - assert_nil m.uint64 - assert_nil m.uint64_as_value - m.bool_as_value = nil - assert_nil m.bool - assert_nil m.bool_as_value - m.string_as_value = nil - assert_nil m.string - assert_nil m.string_as_value - m.bytes_as_value = nil - assert_nil m.bytes - assert_nil m.bytes_as_value + m2 = proto_module::Wrapper.new( + double: Google::Protobuf::DoubleValue.new(value: 2.0), + float: Google::Protobuf::FloatValue.new(value: 4.0), + int32: Google::Protobuf::Int32Value.new(value: 3), + int64: Google::Protobuf::Int64Value.new(value: 4), + uint32: Google::Protobuf::UInt32Value.new(value: 5), + uint64: Google::Protobuf::UInt64Value.new(value: 6), + bool: Google::Protobuf::BoolValue.new(value: true), + string: Google::Protobuf::StringValue.new(value: 'str'), + bytes: Google::Protobuf::BytesValue.new(value: 'fun'), + real_string: '100' + ) + + run_asserts.call(m2) + + serialized = proto_module::Wrapper::encode(m2) + m3 = proto_module::Wrapper::decode(serialized) + run_asserts.call(m3) end def test_wrapper_setters + run_asserts = ->(m) { + m.double = Google::Protobuf::DoubleValue.new(value: 4.8) + assert_equal 4.8, m.double_as_value + assert_equal Google::Protobuf::DoubleValue.new(value: 4.8), m.double + m.float = Google::Protobuf::FloatValue.new(value: 2.4) + assert_in_delta 2.4, m.float_as_value + assert_in_delta Google::Protobuf::FloatValue.new(value: 2.4).value, m.float.value + m.int32 = Google::Protobuf::Int32Value.new(value: 5) + assert_equal 5, m.int32_as_value + assert_equal Google::Protobuf::Int32Value.new(value: 5), m.int32 + m.int64 = Google::Protobuf::Int64Value.new(value: 15) + assert_equal 15, m.int64_as_value + assert_equal Google::Protobuf::Int64Value.new(value: 15), m.int64 + m.uint32 = Google::Protobuf::UInt32Value.new(value: 50) + assert_equal 50, m.uint32_as_value + assert_equal Google::Protobuf::UInt32Value.new(value: 50), m.uint32 + m.uint64 = Google::Protobuf::UInt64Value.new(value: 500) + assert_equal 500, m.uint64_as_value + assert_equal Google::Protobuf::UInt64Value.new(value: 500), m.uint64 + m.bool = Google::Protobuf::BoolValue.new(value: false) + assert_equal false, m.bool_as_value + assert_equal Google::Protobuf::BoolValue.new(value: false), m.bool + m.string = Google::Protobuf::StringValue.new(value: 'xy') + assert_equal 'xy', m.string_as_value + assert_equal Google::Protobuf::StringValue.new(value: 'xy'), m.string + m.bytes = Google::Protobuf::BytesValue.new(value: '123') + assert_equal '123', m.bytes_as_value + assert_equal Google::Protobuf::BytesValue.new(value: '123'), m.bytes + + m.double = nil + assert_nil m.double + assert_nil m.double_as_value + m.float = nil + assert_nil m.float + assert_nil m.float_as_value + m.int32 = nil + assert_nil m.int32 + assert_nil m.int32_as_value + m.int64 = nil + assert_nil m.int64 + assert_nil m.int64_as_value + m.uint32 = nil + assert_nil m.uint32 + assert_nil m.uint32_as_value + m.uint64 = nil + assert_nil m.uint64 + assert_nil m.uint64_as_value + m.bool = nil + assert_nil m.bool + assert_nil m.bool_as_value + m.string = nil + assert_nil m.string + assert_nil m.string_as_value + m.bytes = nil + assert_nil m.bytes + assert_nil m.bytes_as_value + } + m = proto_module::Wrapper.new + run_asserts.call(m) + + m2 = proto_module::Wrapper.new( + double: Google::Protobuf::DoubleValue.new(value: 2.0), + float: Google::Protobuf::FloatValue.new(value: 4.0), + int32: Google::Protobuf::Int32Value.new(value: 3), + int64: Google::Protobuf::Int64Value.new(value: 4), + uint32: Google::Protobuf::UInt32Value.new(value: 5), + uint64: Google::Protobuf::UInt64Value.new(value: 6), + bool: Google::Protobuf::BoolValue.new(value: true), + string: Google::Protobuf::StringValue.new(value: 'str'), + bytes: Google::Protobuf::BytesValue.new(value: 'fun'), + real_string: '100' + ) + + run_asserts.call(m2) - m.double = Google::Protobuf::DoubleValue.new(value: 4.8) - assert_equal 4.8, m.double_as_value - assert_equal Google::Protobuf::DoubleValue.new(value: 4.8), m.double - m.float = Google::Protobuf::FloatValue.new(value: 2.4) - assert_in_delta 2.4, m.float_as_value - assert_in_delta Google::Protobuf::FloatValue.new(value: 2.4).value, m.float.value - m.int32 = Google::Protobuf::Int32Value.new(value: 5) - assert_equal 5, m.int32_as_value - assert_equal Google::Protobuf::Int32Value.new(value: 5), m.int32 - m.int64 = Google::Protobuf::Int64Value.new(value: 15) - assert_equal 15, m.int64_as_value - assert_equal Google::Protobuf::Int64Value.new(value: 15), m.int64 - m.uint32 = Google::Protobuf::UInt32Value.new(value: 50) - assert_equal 50, m.uint32_as_value - assert_equal Google::Protobuf::UInt32Value.new(value: 50), m.uint32 - m.uint64 = Google::Protobuf::UInt64Value.new(value: 500) - assert_equal 500, m.uint64_as_value - assert_equal Google::Protobuf::UInt64Value.new(value: 500), m.uint64 - m.bool = Google::Protobuf::BoolValue.new(value: false) - assert_equal false, m.bool_as_value - assert_equal Google::Protobuf::BoolValue.new(value: false), m.bool - m.string = Google::Protobuf::StringValue.new(value: 'xy') - assert_equal 'xy', m.string_as_value - assert_equal Google::Protobuf::StringValue.new(value: 'xy'), m.string - m.bytes = Google::Protobuf::BytesValue.new(value: '123') - assert_equal '123', m.bytes_as_value - assert_equal Google::Protobuf::BytesValue.new(value: '123'), m.bytes - - m.double = nil - assert_nil m.double - assert_nil m.double_as_value - m.float = nil - assert_nil m.float - assert_nil m.float_as_value - m.int32 = nil - assert_nil m.int32 - assert_nil m.int32_as_value - m.int64 = nil - assert_nil m.int64 - assert_nil m.int64_as_value - m.uint32 = nil - assert_nil m.uint32 - assert_nil m.uint32_as_value - m.uint64 = nil - assert_nil m.uint64 - assert_nil m.uint64_as_value - m.bool = nil - assert_nil m.bool - assert_nil m.bool_as_value - m.string = nil - assert_nil m.string - assert_nil m.string_as_value - m.bytes = nil - assert_nil m.bytes - assert_nil m.bytes_as_value + serialized = proto_module::Wrapper::encode(m2) + m3 = proto_module::Wrapper::decode(serialized) + run_asserts.call(m3) end def test_wrappers_only From bd253f0130564704888136b14f638a69acc00d20 Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Mon, 28 Oct 2019 18:03:25 -0700 Subject: [PATCH 3/7] Fixed equality, and extended to repeated fields and maps. --- ruby/ext/google/protobuf_c/encode_decode.c | 2 +- ruby/ext/google/protobuf_c/map.c | 3 +- ruby/ext/google/protobuf_c/message.c | 10 +++--- ruby/ext/google/protobuf_c/protobuf.h | 6 ++-- ruby/ext/google/protobuf_c/repeated_field.c | 3 +- ruby/ext/google/protobuf_c/storage.c | 36 ++++++++++++--------- ruby/tests/basic_test.proto | 12 +++++++ ruby/tests/basic_test_proto2.proto | 12 +++++++ ruby/tests/common_tests.rb | 4 +-- 9 files changed, 59 insertions(+), 29 deletions(-) diff --git a/ruby/ext/google/protobuf_c/encode_decode.c b/ruby/ext/google/protobuf_c/encode_decode.c index d85cb18cced8..e1d65985f342 100644 --- a/ruby/ext/google/protobuf_c/encode_decode.c +++ b/ruby/ext/google/protobuf_c/encode_decode.c @@ -1444,7 +1444,7 @@ static void putmsg(VALUE msg_rb, const Descriptor* desc, int type = TYPE(val); if (type != T_DATA && type != T_NIL && is_wrapper_type_field(f)) { // OPT: could try to avoid expanding the wrapper here. - val = ruby_wrapper_type(desc->layout, f, val); + val = ruby_wrapper_type(field_type_class(desc->layout, f), val); DEREF(msg, offset, VALUE) = val; } putsubmsg(val, f, sink, depth, emit_defaults, is_json); diff --git a/ruby/ext/google/protobuf_c/map.c b/ruby/ext/google/protobuf_c/map.c index 5487a576402c..5c20fae94320 100644 --- a/ruby/ext/google/protobuf_c/map.c +++ b/ruby/ext/google/protobuf_c/map.c @@ -631,7 +631,8 @@ VALUE Map_eq(VALUE _self, VALUE _other) { return Qfalse; } - if (!native_slot_eq(self->value_type, mem, other_mem)) { + if (!native_slot_eq(self->value_type, self->value_type_class, mem, + other_mem)) { // Present, but value not equal. return Qfalse; } diff --git a/ruby/ext/google/protobuf_c/message.c b/ruby/ext/google/protobuf_c/message.c index ba93c58dfd51..f57501cc65c8 100644 --- a/ruby/ext/google/protobuf_c/message.c +++ b/ruby/ext/google/protobuf_c/message.c @@ -131,14 +131,13 @@ bool is_wrapper_type_field(const upb_fielddef* field) { } // Get a new Ruby wrapper type and set the initial value -VALUE ruby_wrapper_type(const MessageLayout* layout, const upb_fielddef* field, - const VALUE value) { - if (is_wrapper_type_field(field) && value != Qnil) { +VALUE ruby_wrapper_type(VALUE type_class, VALUE value) { + if (value != Qnil) { VALUE hash = rb_hash_new(); rb_hash_aset(hash, rb_str_new2("value"), value); { VALUE args[1] = {hash}; - return rb_class_new_instance(1, args, field_type_class(layout, field)); + return rb_class_new_instance(1, args, type_class); } } return Qnil; @@ -343,7 +342,8 @@ VALUE Message_method_missing(int argc, VALUE* argv, VALUE _self) { return value; } } else if (accessor_type == METHOD_WRAPPER_SETTER) { - VALUE wrapper = ruby_wrapper_type(self->descriptor->layout, f, argv[1]); + VALUE wrapper = ruby_wrapper_type( + field_type_class(self->descriptor->layout, f), argv[1]); layout_set(self->descriptor->layout, Message_data(self), f, wrapper); return Qnil; } else if (accessor_type == METHOD_ENUM_GETTER) { diff --git a/ruby/ext/google/protobuf_c/protobuf.h b/ruby/ext/google/protobuf_c/protobuf.h index 109fd0f1619b..2e727499f3de 100644 --- a/ruby/ext/google/protobuf_c/protobuf.h +++ b/ruby/ext/google/protobuf_c/protobuf.h @@ -364,7 +364,8 @@ void native_slot_init(upb_fieldtype_t type, void* memory); void native_slot_mark(upb_fieldtype_t type, void* memory); void native_slot_dup(upb_fieldtype_t type, void* to, void* from); void native_slot_deep_copy(upb_fieldtype_t type, void* to, void* from); -bool native_slot_eq(upb_fieldtype_t type, void* mem1, void* mem2); +bool native_slot_eq(upb_fieldtype_t type, VALUE type_class, void* mem1, + void* mem2); VALUE native_slot_encode_and_freeze_string(upb_fieldtype_t type, VALUE value); void native_slot_check_int_range_precision(const char* name, upb_fieldtype_t type, VALUE value); @@ -557,8 +558,7 @@ VALUE layout_hash(MessageLayout* layout, void* storage); VALUE layout_inspect(MessageLayout* layout, void* storage); bool is_wrapper_type_field(const upb_fielddef* field); -VALUE ruby_wrapper_type(const MessageLayout* layout, const upb_fielddef* field, - const VALUE value); +VALUE ruby_wrapper_type(VALUE type_class, VALUE value); // ----------------------------------------------------------------------------- // Message class creation. diff --git a/ruby/ext/google/protobuf_c/repeated_field.c b/ruby/ext/google/protobuf_c/repeated_field.c index 1c649280fe6d..2b05e5f31a98 100644 --- a/ruby/ext/google/protobuf_c/repeated_field.c +++ b/ruby/ext/google/protobuf_c/repeated_field.c @@ -451,7 +451,8 @@ VALUE RepeatedField_eq(VALUE _self, VALUE _other) { for (i = 0; i < self->size; i++, off += elem_size) { void* self_mem = ((uint8_t *)self->elements) + off; void* other_mem = ((uint8_t *)other->elements) + off; - if (!native_slot_eq(field_type, self_mem, other_mem)) { + if (!native_slot_eq(field_type, self->field_type_class, self_mem, + other_mem)) { return Qfalse; } } diff --git a/ruby/ext/google/protobuf_c/storage.c b/ruby/ext/google/protobuf_c/storage.c index 100b85350fcc..3ad1447daaf6 100644 --- a/ruby/ext/google/protobuf_c/storage.c +++ b/ruby/ext/google/protobuf_c/storage.c @@ -294,8 +294,17 @@ VALUE native_slot_get(upb_fieldtype_t type, return DEREF(memory, int8_t) ? Qtrue : Qfalse; case UPB_TYPE_STRING: case UPB_TYPE_BYTES: - case UPB_TYPE_MESSAGE: return DEREF(memory, VALUE); + case UPB_TYPE_MESSAGE: { + VALUE val = DEREF(memory, VALUE); + int type = TYPE(val); + if (type != T_DATA && type != T_NIL) { + // This must be a wrapper type. + val = ruby_wrapper_type(type_class, val); + DEREF(memory, VALUE) = val; + } + return val; + } case UPB_TYPE_ENUM: { int32_t val = DEREF(memory, int32_t); VALUE symbol = enum_lookup(type_class, INT2NUM(val)); @@ -392,13 +401,14 @@ void native_slot_deep_copy(upb_fieldtype_t type, void* to, void* from) { } } -bool native_slot_eq(upb_fieldtype_t type, void* mem1, void* mem2) { +bool native_slot_eq(upb_fieldtype_t type, VALUE type_class, void* mem1, + void* mem2) { switch (type) { case UPB_TYPE_STRING: case UPB_TYPE_BYTES: case UPB_TYPE_MESSAGE: { - VALUE val1 = DEREF(mem1, VALUE); - VALUE val2 = DEREF(mem2, VALUE); + VALUE val1 = native_slot_get(type, type_class, mem1); + VALUE val2 = native_slot_get(type, type_class, mem2); VALUE ret = rb_funcall(val1, rb_intern("=="), 1, val2); return ret == Qtrue; } @@ -843,15 +853,8 @@ VALUE layout_get(MessageLayout* layout, } else if (!field_set) { return layout_get_default(field); } else { - VALUE type_class = field_type_class(layout, field); - VALUE val = native_slot_get(upb_fielddef_type(field), type_class, memory); - int type = TYPE(val); - if (type != T_DATA && type != T_NIL && is_wrapper_type_field(field)) { - val = ruby_wrapper_type(layout, field, val); - native_slot_set(upb_fielddef_name(field), upb_fielddef_type(field), - type_class, memory, val); - } - return val; + return native_slot_get(upb_fielddef_type(field), + field_type_class(layout, field), memory); } } @@ -1068,7 +1071,8 @@ VALUE layout_eq(MessageLayout* layout, void* msg1, void* msg2) { if (*msg1_oneof_case != *msg2_oneof_case || (slot_read_oneof_case(layout, msg1, oneof) == upb_fielddef_number(field) && - !native_slot_eq(upb_fielddef_type(field), msg1_memory, + !native_slot_eq(upb_fielddef_type(field), + field_type_class(layout, field), msg1_memory, msg2_memory))) { return Qfalse; } @@ -1085,7 +1089,9 @@ VALUE layout_eq(MessageLayout* layout, void* msg1, void* msg2) { } else { if (slot_is_hasbit_set(layout, msg1, field) != slot_is_hasbit_set(layout, msg2, field) || - !native_slot_eq(upb_fielddef_type(field), msg1_memory, msg2_memory)) { + !native_slot_eq(upb_fielddef_type(field), + field_type_class(layout, field), msg1_memory, + msg2_memory)) { return Qfalse; } } diff --git a/ruby/tests/basic_test.proto b/ruby/tests/basic_test.proto index e5811dc881f4..529da30b666d 100644 --- a/ruby/tests/basic_test.proto +++ b/ruby/tests/basic_test.proto @@ -128,6 +128,18 @@ message Wrapper { oneof a_oneof { string oneof_string = 10; } + + // Repeated wrappers don't really make sense, but we still need to make sure + // they work and don't crash. + repeated google.protobuf.DoubleValue repeated_double = 11; + repeated google.protobuf.FloatValue repeated_float = 12; + repeated google.protobuf.Int32Value repeated_int32 = 13; + repeated google.protobuf.Int64Value repeated_int64 = 14; + repeated google.protobuf.UInt32Value repeated_uint32 = 15; + repeated google.protobuf.UInt64Value repeated_uint64 = 16; + repeated google.protobuf.BoolValue repeated_bool = 17; + repeated google.protobuf.StringValue repeated_string = 18; + repeated google.protobuf.BytesValue repeated_bytes = 19; } message TimeMessage { diff --git a/ruby/tests/basic_test_proto2.proto b/ruby/tests/basic_test_proto2.proto index e54ed3185a65..1697164493eb 100644 --- a/ruby/tests/basic_test_proto2.proto +++ b/ruby/tests/basic_test_proto2.proto @@ -135,6 +135,18 @@ message Wrapper { oneof a_oneof { string oneof_string = 10; } + + // Repeated wrappers don't really make sense, but we still need to make sure + // they work and don't crash. + repeated google.protobuf.DoubleValue repeated_double = 11; + repeated google.protobuf.FloatValue repeated_float = 12; + repeated google.protobuf.Int32Value repeated_int32 = 13; + repeated google.protobuf.Int64Value repeated_int64 = 14; + repeated google.protobuf.UInt32Value repeated_uint32 = 15; + repeated google.protobuf.UInt64Value repeated_uint64 = 16; + repeated google.protobuf.BoolValue repeated_bool = 17; + repeated google.protobuf.StringValue repeated_string = 18; + repeated google.protobuf.BytesValue repeated_bytes = 19; } message TimeMessage { diff --git a/ruby/tests/common_tests.rb b/ruby/tests/common_tests.rb index 78179b6e4a92..5c0b6c15cae5 100644 --- a/ruby/tests/common_tests.rb +++ b/ruby/tests/common_tests.rb @@ -1331,9 +1331,7 @@ def test_wrapper_getters # Test that the lazy form compares equal to the expanded form. m5 = proto_module::Wrapper::decode(serialized2) - - # This doesn't work yet. - # assert_equal m5, m + assert_equal m5, m end def test_wrapper_setters_as_value From 8393d4833f6437a08ec8195f7c4cae347f7311a7 Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Tue, 29 Oct 2019 13:30:12 -0700 Subject: [PATCH 4/7] Nearly all known cases (map, repeated field, and top-level) have been addressed. The only case that doesn't work is decoding a wrapper type from JSON at the top level. This doesn't make sense and probably no users do it I changed it to throw. --- ruby/ext/google/protobuf_c/encode_decode.c | 98 ++++++++++++++------- ruby/ext/google/protobuf_c/map.c | 3 +- ruby/ext/google/protobuf_c/protobuf.h | 3 +- ruby/ext/google/protobuf_c/repeated_field.c | 2 +- ruby/ext/google/protobuf_c/storage.c | 16 +++- ruby/tests/basic.rb | 42 +++++++++ ruby/tests/basic_test.proto | 12 +++ ruby/tests/common_tests.rb | 75 ++++++++++++++++ 8 files changed, 213 insertions(+), 38 deletions(-) diff --git a/ruby/ext/google/protobuf_c/encode_decode.c b/ruby/ext/google/protobuf_c/encode_decode.c index e1d65985f342..f78c7325b851 100644 --- a/ruby/ext/google/protobuf_c/encode_decode.c +++ b/ruby/ext/google/protobuf_c/encode_decode.c @@ -278,6 +278,17 @@ static void *appendsubmsg_handler(void *closure, const void *hd) { return submsg; } +// Appends a wrapper to a repeated field (a regular Ruby array for now). +static void *appendwrapper_handler(void *closure, const void *hd) { + VALUE ary = (VALUE)closure; + int size = RepeatedField_size(ary); + (void)hd; + + RepeatedField_push(ary, Qnil); + + return RepeatedField_index_native(ary, size); +} + // Sets a non-repeated submessage field in a message. static void *submsg_handler(void *closure, const void *hd) { MessageHeader* msg = closure; @@ -503,6 +514,23 @@ static void *oneofsubmsg_handler(void *closure, return submsg; } +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, @@ -544,7 +572,11 @@ static void add_handlers_for_repeated_field(upb_handlers *h, VALUE subklass = field_type_class(desc->layout, f); upb_handlerattr attr = UPB_HANDLERATTR_INIT; attr.handler_data = newsubmsghandlerdata(h, 0, -1, subklass); - upb_handlers_setstartsubmsg(h, f, appendsubmsg_handler, &attr); + if (is_wrapper(upb_fielddef_msgsubdef(f))) { + upb_handlers_setstartsubmsg(h, f, appendwrapper_handler, &attr); + } else { + upb_handlers_setstartsubmsg(h, f, appendsubmsg_handler, &attr); + } break; } } @@ -612,23 +644,6 @@ static bool boolwrapper_handler(void* closure, const void* hd, bool val) { return true; } -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 singular field. static void add_handlers_for_singular_field(const Descriptor* desc, upb_handlers* h, @@ -811,6 +826,10 @@ static bool unknown_field_handler(void* closure, const void* hd, return true; } +size_t get_field_offset(MessageLayout* layout, const upb_fielddef* f) { + return layout->fields[upb_fielddef_index(f)].offset + sizeof(MessageHeader); +} + void add_handlers_for_message(const void *closure, upb_handlers *h) { const VALUE descriptor_pool = (VALUE)closure; const upb_msgdef* msgdef = upb_handlers_msgdef(h); @@ -847,8 +866,7 @@ void add_handlers_for_message(const void *closure, upb_handlers *h) { upb_msg_field_next(&i)) { const upb_fielddef *f = upb_msg_iter_field(&i); const upb_oneofdef *oneof = upb_fielddef_containingoneof(f); - size_t offset = desc->layout->fields[upb_fielddef_index(f)].offset + - sizeof(MessageHeader); + size_t offset = get_field_offset(desc->layout, f); if (oneof) { size_t oneof_case_offset = @@ -960,17 +978,28 @@ VALUE Message_decode(VALUE klass, VALUE data) { { const upb_pbdecodermethod* method = msgdef_decodermethod(desc); const upb_handlers* h = upb_pbdecodermethod_desthandlers(method); + const upb_msgdef* m = upb_handlers_msgdef(h); + VALUE wrapper = Qnil; + void* ptr = msg; stackenv se; upb_sink sink; upb_pbdecoder* decoder; stackenv_init(&se, "Error occurred during parsing: %" PRIsVALUE); - upb_sink_reset(&sink, h, msg); + if (is_wrapper(m)) { + ptr = &wrapper; + } + + upb_sink_reset(&sink, h, ptr); decoder = upb_pbdecoder_create(se.arena, method, sink, &se.status); upb_bufsrc_putbuf(RSTRING_PTR(data), RSTRING_LEN(data), upb_pbdecoder_input(decoder)); stackenv_uninit(&se); + + if (is_wrapper(m)) { + msg_rb = ruby_wrapper_type(msgklass, wrapper); + } } return msg_rb; @@ -1024,13 +1053,21 @@ VALUE Message_decode_json(int argc, VALUE* argv, VALUE klass) { { const upb_json_parsermethod* method = msgdef_jsonparsermethod(desc); + const upb_handlers* h = get_fill_handlers(desc); + const upb_msgdef* m = upb_handlers_msgdef(h); stackenv se; upb_sink sink; upb_json_parser* parser; DescriptorPool* pool = ruby_to_DescriptorPool(generated_pool); stackenv_init(&se, "Error occurred during parsing: %" PRIsVALUE); - upb_sink_reset(&sink, get_fill_handlers(desc), msg); + if (is_wrapper(m)) { + rb_raise( + rb_eRuntimeError, + "Parsing a wrapper type from JSON at the top level does not work."); + } + + upb_sink_reset(&sink, h, msg); parser = upb_json_parser_create(se.arena, method, pool->symtab, sink, &se.status, RTEST(ignore_unknown_fields)); upb_bufsrc_putbuf(RSTRING_PTR(data), RSTRING_LEN(data), @@ -1103,6 +1140,7 @@ static void putary(VALUE ary, const upb_fielddef* f, upb_sink sink, int depth, upb_selector_t sel = 0; int size; int i; + VALUE type_class = ruby_to_RepeatedField(ary)->field_type_class; if (ary == Qnil) return; if (!emit_defaults && NUM2INT(RepeatedField_length(ary)) == 0) return; @@ -1137,9 +1175,11 @@ static void putary(VALUE ary, const upb_fielddef* f, upb_sink sink, int depth, case UPB_TYPE_BYTES: putstr(*((VALUE *)memory), f, subsink); break; - case UPB_TYPE_MESSAGE: - putsubmsg(*((VALUE*)memory), f, subsink, depth, emit_defaults, is_json); + case UPB_TYPE_MESSAGE: { + VALUE val = native_slot_get(UPB_TYPE_MESSAGE, type_class, memory); + putsubmsg(val, f, subsink, depth, emit_defaults, is_json); break; + } #undef T @@ -1440,13 +1480,9 @@ static void putmsg(VALUE msg_rb, const Descriptor* desc, putstr(str, f, sink); } } else if (upb_fielddef_issubmsg(f)) { - VALUE val = DEREF(msg, offset, VALUE); - int type = TYPE(val); - if (type != T_DATA && type != T_NIL && is_wrapper_type_field(f)) { - // OPT: could try to avoid expanding the wrapper here. - val = ruby_wrapper_type(field_type_class(desc->layout, f), val); - DEREF(msg, offset, VALUE) = val; - } + // OPT: could try to avoid the layout_get() (which will expand lazy + // wrappers). + VALUE val = layout_get(desc->layout, Message_data(msg), f); putsubmsg(val, f, sink, depth, emit_defaults, is_json); } else { upb_selector_t sel = getsel(f, upb_handlers_getprimitivehandlertype(f)); diff --git a/ruby/ext/google/protobuf_c/map.c b/ruby/ext/google/protobuf_c/map.c index 5c20fae94320..719706b9dd79 100644 --- a/ruby/ext/google/protobuf_c/map.c +++ b/ruby/ext/google/protobuf_c/map.c @@ -559,7 +559,8 @@ VALUE Map_deep_copy(VALUE _self) { void* mem = value_memory(&v); upb_value dup; void* dup_mem = value_memory(&dup); - native_slot_deep_copy(self->value_type, dup_mem, mem); + native_slot_deep_copy(self->value_type, self->value_type_class, dup_mem, + mem); if (!upb_strtable_insert2(&new_self->table, upb_strtable_iter_key(&it), diff --git a/ruby/ext/google/protobuf_c/protobuf.h b/ruby/ext/google/protobuf_c/protobuf.h index 2e727499f3de..d026db2574ee 100644 --- a/ruby/ext/google/protobuf_c/protobuf.h +++ b/ruby/ext/google/protobuf_c/protobuf.h @@ -363,7 +363,8 @@ VALUE native_slot_get(upb_fieldtype_t type, void native_slot_init(upb_fieldtype_t type, void* memory); void native_slot_mark(upb_fieldtype_t type, void* memory); void native_slot_dup(upb_fieldtype_t type, void* to, void* from); -void native_slot_deep_copy(upb_fieldtype_t type, void* to, void* from); +void native_slot_deep_copy(upb_fieldtype_t type, VALUE type_class, void* to, + void* from); bool native_slot_eq(upb_fieldtype_t type, VALUE type_class, void* mem1, void* mem2); diff --git a/ruby/ext/google/protobuf_c/repeated_field.c b/ruby/ext/google/protobuf_c/repeated_field.c index 2b05e5f31a98..e3afb282b8ca 100644 --- a/ruby/ext/google/protobuf_c/repeated_field.c +++ b/ruby/ext/google/protobuf_c/repeated_field.c @@ -378,7 +378,7 @@ VALUE RepeatedField_deep_copy(VALUE _self) { for (i = 0; i < self->size; i++, off += elem_size) { void* to_mem = (uint8_t *)new_rptfield_self->elements + off; void* from_mem = (uint8_t *)self->elements + off; - native_slot_deep_copy(field_type, to_mem, from_mem); + native_slot_deep_copy(field_type, self->field_type_class, to_mem, from_mem); new_rptfield_self->size++; } diff --git a/ruby/ext/google/protobuf_c/storage.c b/ruby/ext/google/protobuf_c/storage.c index 3ad1447daaf6..739c0a77650f 100644 --- a/ruby/ext/google/protobuf_c/storage.c +++ b/ruby/ext/google/protobuf_c/storage.c @@ -297,12 +297,15 @@ VALUE native_slot_get(upb_fieldtype_t type, return DEREF(memory, VALUE); case UPB_TYPE_MESSAGE: { VALUE val = DEREF(memory, VALUE); + + // Lazily expand wrapper type if necessary. int type = TYPE(val); if (type != T_DATA && type != T_NIL) { // This must be a wrapper type. val = ruby_wrapper_type(type_class, val); DEREF(memory, VALUE) = val; } + return val; } case UPB_TYPE_ENUM: { @@ -381,7 +384,8 @@ void native_slot_dup(upb_fieldtype_t type, void* to, void* from) { memcpy(to, from, native_slot_size(type)); } -void native_slot_deep_copy(upb_fieldtype_t type, void* to, void* from) { +void native_slot_deep_copy(upb_fieldtype_t type, VALUE type_class, void* to, + void* from) { switch (type) { case UPB_TYPE_STRING: case UPB_TYPE_BYTES: { @@ -391,7 +395,7 @@ void native_slot_deep_copy(upb_fieldtype_t type, void* to, void* from) { break; } case UPB_TYPE_MESSAGE: { - VALUE from_val = DEREF(from, VALUE); + VALUE from_val = native_slot_get(type, type_class, from); DEREF(to, VALUE) = (from_val != Qnil) ? Message_deep_copy(from_val) : Qnil; break; @@ -1035,7 +1039,9 @@ void layout_deep_copy(MessageLayout* layout, void* to, void* from) { if (slot_read_oneof_case(layout, from, oneof) == upb_fielddef_number(field)) { *to_oneof_case = *from_oneof_case; - native_slot_deep_copy(upb_fielddef_type(field), to_memory, from_memory); + native_slot_deep_copy(upb_fielddef_type(field), + field_type_class(layout, field), to_memory, + from_memory); } } else if (is_map_field(field)) { DEREF(to_memory, VALUE) = @@ -1049,7 +1055,9 @@ void layout_deep_copy(MessageLayout* layout, void* to, void* from) { slot_set_hasbit(layout, to, field); } - native_slot_deep_copy(upb_fielddef_type(field), to_memory, from_memory); + native_slot_deep_copy(upb_fielddef_type(field), + field_type_class(layout, field), to_memory, + from_memory); } } } diff --git a/ruby/tests/basic.rb b/ruby/tests/basic.rb index 1b62cd062ba0..1cb8d3463ed4 100644 --- a/ruby/tests/basic.rb +++ b/ruby/tests/basic.rb @@ -234,6 +234,48 @@ def test_map_corruption m.map_string_int32['aaa'] = 3 end + def test_map_wrappers + run_asserts = ->(m) { + assert_equal 2.0, m.map_double[0].value + assert_equal 4.0, m.map_float[0].value + assert_equal 3, m.map_int32[0].value + assert_equal 4, m.map_int64[0].value + assert_equal 5, m.map_uint32[0].value + assert_equal 6, m.map_uint64[0].value + assert_equal true, m.map_bool[0].value + assert_equal 'str', m.map_string[0].value + assert_equal 'fun', m.map_bytes[0].value + } + + m = proto_module::Wrapper.new( + map_double: {0 => Google::Protobuf::DoubleValue.new(value: 2.0)}, + map_float: {0 => Google::Protobuf::FloatValue.new(value: 4.0)}, + map_int32: {0 => Google::Protobuf::Int32Value.new(value: 3)}, + map_int64: {0 => Google::Protobuf::Int64Value.new(value: 4)}, + map_uint32: {0 => Google::Protobuf::UInt32Value.new(value: 5)}, + map_uint64: {0 => Google::Protobuf::UInt64Value.new(value: 6)}, + map_bool: {0 => Google::Protobuf::BoolValue.new(value: true)}, + map_string: {0 => Google::Protobuf::StringValue.new(value: 'str')}, + map_bytes: {0 => Google::Protobuf::BytesValue.new(value: 'fun')}, + ) + + 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 diff --git a/ruby/tests/basic_test.proto b/ruby/tests/basic_test.proto index 529da30b666d..7be599288b6a 100644 --- a/ruby/tests/basic_test.proto +++ b/ruby/tests/basic_test.proto @@ -140,6 +140,18 @@ message Wrapper { repeated google.protobuf.BoolValue repeated_bool = 17; repeated google.protobuf.StringValue repeated_string = 18; repeated google.protobuf.BytesValue repeated_bytes = 19; + + // Wrappers as map keys don't really make sense, but we still need to make + // sure they work and don't crash. + map map_double = 21; + map map_float = 22; + map map_int32 = 23; + map map_int64 = 24; + map map_uint32 = 25; + map map_uint64 = 26; + map map_bool = 27; + map map_string = 28; + map map_bytes = 29; } message TimeMessage { diff --git a/ruby/tests/common_tests.rb b/ruby/tests/common_tests.rb index 5c0b6c15cae5..08de1d4502f5 100644 --- a/ruby/tests/common_tests.rb +++ b/ruby/tests/common_tests.rb @@ -1334,6 +1334,81 @@ def test_wrapper_getters assert_equal m5, m end + def test_repeated_wrappers + run_asserts = ->(m) { + assert_equal 2.0, m.repeated_double[0].value + assert_equal 4.0, m.repeated_float[0].value + assert_equal 3, m.repeated_int32[0].value + assert_equal 4, m.repeated_int64[0].value + assert_equal 5, m.repeated_uint32[0].value + assert_equal 6, m.repeated_uint64[0].value + assert_equal true, m.repeated_bool[0].value + assert_equal 'str', m.repeated_string[0].value + assert_equal 'fun', m.repeated_bytes[0].value + } + + m = proto_module::Wrapper.new( + repeated_double: [Google::Protobuf::DoubleValue.new(value: 2.0)], + repeated_float: [Google::Protobuf::FloatValue.new(value: 4.0)], + repeated_int32: [Google::Protobuf::Int32Value.new(value: 3)], + repeated_int64: [Google::Protobuf::Int64Value.new(value: 4)], + repeated_uint32: [Google::Protobuf::UInt32Value.new(value: 5)], + repeated_uint64: [Google::Protobuf::UInt64Value.new(value: 6)], + repeated_bool: [Google::Protobuf::BoolValue.new(value: true)], + repeated_string: [Google::Protobuf::StringValue.new(value: 'str')], + repeated_bytes: [Google::Protobuf::BytesValue.new(value: 'fun')], + ) + + 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_top_level_wrappers + # We don't expect anyone to do this, but we should also make sure it does + # the right thing. + run_test = ->(klass, val) { + m = klass.new(value: val) + serialized = klass::encode(m) + m2 = klass::decode(serialized) + assert_equal m, m2 + + # Encode directly from lazy form. + serialized2 = klass::encode(m2) + + assert_equal m, m2 + assert_equal serialized, serialized2 + + serialized_json = klass::encode_json(m) + + # This is nonsensical to do and does not work. There is no good reason + # to parse a wrapper type directly. + assert_raise(RuntimeError) { klass::decode_json(serialized_json) } + } + + run_test.call(Google::Protobuf::DoubleValue, 2.0) + run_test.call(Google::Protobuf::FloatValue, 4.0) + run_test.call(Google::Protobuf::Int32Value, 3) + run_test.call(Google::Protobuf::Int64Value, 4) + run_test.call(Google::Protobuf::UInt32Value, 5) + run_test.call(Google::Protobuf::UInt64Value, 6) + run_test.call(Google::Protobuf::BoolValue, true) + run_test.call(Google::Protobuf::StringValue, 'str') + run_test.call(Google::Protobuf::BytesValue, 'fun') + end + def test_wrapper_setters_as_value run_asserts = ->(m) { m.double_as_value = 4.8 From e8c67e14acceb93b048048d7823391df1f79c43c Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Tue, 29 Oct 2019 15:44:51 -0700 Subject: [PATCH 5/7] Fixed the oneof case for lazy wrappers. --- ruby/ext/google/protobuf_c/encode_decode.c | 18 +++++++-- ruby/tests/basic_test.proto | 25 ++++++++++--- ruby/tests/basic_test_proto2.proto | 17 ++++++++- ruby/tests/common_tests.rb | 43 ++++++++++++++++++++-- 4 files changed, 91 insertions(+), 12 deletions(-) diff --git a/ruby/ext/google/protobuf_c/encode_decode.c b/ruby/ext/google/protobuf_c/encode_decode.c index f78c7325b851..6b4266775734 100644 --- a/ruby/ext/google/protobuf_c/encode_decode.c +++ b/ruby/ext/google/protobuf_c/encode_decode.c @@ -506,14 +506,22 @@ static void *oneofsubmsg_handler(void *closure, // indicating a VALUE is present and expect a valid VALUE. See comment in // layout_set() for more detail: basically, the change to the value and the // case must be atomic w.r.t. the Ruby VM. - DEREF(msg, oneofdata->case_ofs, uint32_t) = - oneofdata->oneof_case_num; + DEREF(msg, oneofdata->case_ofs, uint32_t) = oneofdata->oneof_case_num; submsg_rb = DEREF(msg, oneofdata->ofs, VALUE); TypedData_Get_Struct(submsg_rb, MessageHeader, &Message_type, submsg); return submsg; } +static void* oneof_startwrapper(void* closure, const void* hd) { + char* msg = closure; + const oneof_handlerdata_t *oneofdata = hd; + + DEREF(msg, oneofdata->case_ofs, uint32_t) = oneofdata->oneof_case_num; + + return msg + oneofdata->ofs; +} + bool is_wrapper(const upb_msgdef* m) { switch (upb_msgdef_wellknowntype(m)) { case UPB_WELLKNOWN_DOUBLEVALUE: @@ -805,7 +813,11 @@ static void add_handlers_for_oneof_field(upb_handlers *h, break; } case UPB_TYPE_MESSAGE: { - upb_handlers_setstartsubmsg(h, f, oneofsubmsg_handler, &attr); + if (is_wrapper(upb_fielddef_msgsubdef(f))) { + upb_handlers_setstartsubmsg(h, f, oneof_startwrapper, &attr); + } else { + upb_handlers_setstartsubmsg(h, f, oneofsubmsg_handler, &attr); + } break; } } diff --git a/ruby/tests/basic_test.proto b/ruby/tests/basic_test.proto index 7be599288b6a..a91854007535 100644 --- a/ruby/tests/basic_test.proto +++ b/ruby/tests/basic_test.proto @@ -126,11 +126,11 @@ message Wrapper { google.protobuf.BytesValue bytes = 9; string real_string = 100; oneof a_oneof { - string oneof_string = 10; + string string_in_oneof = 10; } - // Repeated wrappers don't really make sense, but we still need to make sure - // they work and don't crash. + // Repeated wrappers don't make sense, but we still need to make sure they + // work and don't crash. repeated google.protobuf.DoubleValue repeated_double = 11; repeated google.protobuf.FloatValue repeated_float = 12; repeated google.protobuf.Int32Value repeated_int32 = 13; @@ -141,8 +141,8 @@ message Wrapper { repeated google.protobuf.StringValue repeated_string = 18; repeated google.protobuf.BytesValue repeated_bytes = 19; - // Wrappers as map keys don't really make sense, but we still need to make - // sure they work and don't crash. + // Wrappers as map keys don't make sense, but we still need to make sure they + // work and don't crash. map map_double = 21; map map_float = 22; map map_int32 = 23; @@ -152,6 +152,21 @@ message Wrapper { map map_bool = 27; map map_string = 28; map map_bytes = 29; + + // Wrappers in oneofs don't make sense, but we still need to make sure they + // work and don't crash. + oneof wrapper_oneof { + google.protobuf.DoubleValue oneof_double = 31; + google.protobuf.FloatValue oneof_float = 32; + google.protobuf.Int32Value oneof_int32 = 33; + google.protobuf.Int64Value oneof_int64 = 34; + google.protobuf.UInt32Value oneof_uint32 = 35; + google.protobuf.UInt64Value oneof_uint64 = 36; + google.protobuf.BoolValue oneof_bool = 37; + google.protobuf.StringValue oneof_string = 38; + google.protobuf.BytesValue oneof_bytes = 39; + string oneof_plain_string = 101; + } } message TimeMessage { diff --git a/ruby/tests/basic_test_proto2.proto b/ruby/tests/basic_test_proto2.proto index 1697164493eb..0c1a2b98363f 100644 --- a/ruby/tests/basic_test_proto2.proto +++ b/ruby/tests/basic_test_proto2.proto @@ -133,7 +133,7 @@ message Wrapper { optional google.protobuf.BytesValue bytes = 9; optional string real_string = 100; oneof a_oneof { - string oneof_string = 10; + string string_in_oneof = 10; } // Repeated wrappers don't really make sense, but we still need to make sure @@ -147,6 +147,21 @@ message Wrapper { repeated google.protobuf.BoolValue repeated_bool = 17; repeated google.protobuf.StringValue repeated_string = 18; repeated google.protobuf.BytesValue repeated_bytes = 19; + + // Wrappers in oneofs don't make sense, but we still need to make sure they + // work and don't crash. + oneof wrapper_oneof { + google.protobuf.DoubleValue oneof_double = 31; + google.protobuf.FloatValue oneof_float = 32; + google.protobuf.Int32Value oneof_int32 = 33; + google.protobuf.Int64Value oneof_int64 = 34; + google.protobuf.UInt32Value oneof_uint32 = 35; + google.protobuf.UInt64Value oneof_uint64 = 36; + google.protobuf.BoolValue oneof_bool = 37; + google.protobuf.StringValue oneof_string = 38; + google.protobuf.BytesValue oneof_bytes = 39; + string oneof_plain_string = 101; + } } message TimeMessage { diff --git a/ruby/tests/common_tests.rb b/ruby/tests/common_tests.rb index 08de1d4502f5..2c1f4d0b3a3f 100644 --- a/ruby/tests/common_tests.rb +++ b/ruby/tests/common_tests.rb @@ -1376,6 +1376,44 @@ def test_repeated_wrappers assert_equal m5, m end + def test_oneof_wrappers + run_test = ->(m) { + serialized = proto_module::Wrapper::encode(m) + m2 = proto_module::Wrapper::decode(serialized) + + # Encode directly from lazy form. + serialized2 = proto_module::Wrapper::encode(m2) + + assert_equal m, m2 + assert_equal serialized, serialized2 + + serialized_json = proto_module::Wrapper::encode_json(m) + m3 = proto_module::Wrapper::decode_json(serialized_json) + assert_equal m, m3 + } + + m = proto_module::Wrapper.new() + + run_test.call(m) + m.oneof_double_as_value = 2.0 + run_test.call(m) + m.oneof_float_as_value = 4.0 + run_test.call(m) + m.oneof_int32_as_value = 3 + run_test.call(m) + m.oneof_int64_as_value = 5 + run_test.call(m) + m.oneof_uint32_as_value = 6 + run_test.call(m) + m.oneof_uint64_as_value = 7 + run_test.call(m) + m.oneof_string_as_value = 'str' + run_test.call(m) + m.oneof_bytes_as_value = 'fun' + run_test.call(m) + puts m + end + def test_top_level_wrappers # We don't expect anyone to do this, but we should also make sure it does # the right thing. @@ -1383,7 +1421,6 @@ def test_top_level_wrappers m = klass.new(value: val) serialized = klass::encode(m) m2 = klass::decode(serialized) - assert_equal m, m2 # Encode directly from lazy form. serialized2 = klass::encode(m2) @@ -1573,12 +1610,12 @@ def test_wrapper_setters end def test_wrappers_only - m = proto_module::Wrapper.new(real_string: 'hi', oneof_string: 'there') + m = proto_module::Wrapper.new(real_string: 'hi', string_in_oneof: 'there') assert_raise(NoMethodError) { m.real_string_as_value } assert_raise(NoMethodError) { m.as_value } assert_raise(NoMethodError) { m._as_value } - assert_raise(NoMethodError) { m.oneof_string_as_value } + assert_raise(NoMethodError) { m.string_in_oneof_as_value } m = proto_module::Wrapper.new m.string_as_value = 'you' From 5f2540025028c482c08c1c39561f7917693f965c Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Tue, 29 Oct 2019 18:16:04 -0700 Subject: [PATCH 6/7] Fixed conformance test regression: empty string wrapper. --- ruby/ext/google/protobuf_c/encode_decode.c | 18 ++++++++++++++++++ ruby/tests/common_tests.rb | 1 - 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/ruby/ext/google/protobuf_c/encode_decode.c b/ruby/ext/google/protobuf_c/encode_decode.c index 6b4266775734..093b0485b949 100644 --- a/ruby/ext/google/protobuf_c/encode_decode.c +++ b/ruby/ext/google/protobuf_c/encode_decode.c @@ -626,6 +626,14 @@ static bool uint32wrapper_handler(void* closure, const void* hd, uint32_t val) { return true; } +static void* startstringwrapper_handler(void* closure, const void* hd, + size_t size_hint) { + VALUE* rbval = closure; + (void)size_hint; + *rbval = get_frozen_string(NULL, 0, false); + return closure; +} + static size_t stringwrapper_handler(void* closure, const void* hd, const char* ptr, size_t len, const upb_bufhandle* handle) { @@ -634,6 +642,14 @@ static size_t stringwrapper_handler(void* closure, const void* hd, return len; } +static void* startbyteswrapper_handler(void* closure, const void* hd, + size_t size_hint) { + VALUE* rbval = closure; + (void)size_hint; + *rbval = get_frozen_string(NULL, 0, true); + return closure; +} + static size_t byteswrapper_handler(void* closure, const void* hd, const char* ptr, size_t len, const upb_bufhandle* handle) { @@ -760,9 +776,11 @@ static void add_handlers_for_wrapper(const upb_msgdef* msgdef, upb_handlers_setuint32(h, f, uint32wrapper_handler, NULL); break; case UPB_WELLKNOWN_STRINGVALUE: + upb_handlers_setstartstr(h, f, startstringwrapper_handler, NULL); upb_handlers_setstring(h, f, stringwrapper_handler, NULL); break; case UPB_WELLKNOWN_BYTESVALUE: + upb_handlers_setstartstr(h, f, startbyteswrapper_handler, NULL); upb_handlers_setstring(h, f, byteswrapper_handler, NULL); break; case UPB_WELLKNOWN_BOOLVALUE: diff --git a/ruby/tests/common_tests.rb b/ruby/tests/common_tests.rb index 2c1f4d0b3a3f..d20b76cfe6d5 100644 --- a/ruby/tests/common_tests.rb +++ b/ruby/tests/common_tests.rb @@ -1411,7 +1411,6 @@ def test_oneof_wrappers run_test.call(m) m.oneof_bytes_as_value = 'fun' run_test.call(m) - puts m end def test_top_level_wrappers From 781d6963c65201a56406cf28b6258b041e45664f Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Thu, 31 Oct 2019 12:23:25 -0700 Subject: [PATCH 7/7] Fixed the case of multi-line strings in JSON. --- ruby/ext/google/protobuf_c/encode_decode.c | 10 ++++++---- ruby/tests/common_tests.rb | 18 ++++++++++++++---- 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/ruby/ext/google/protobuf_c/encode_decode.c b/ruby/ext/google/protobuf_c/encode_decode.c index 093b0485b949..713da435229f 100644 --- a/ruby/ext/google/protobuf_c/encode_decode.c +++ b/ruby/ext/google/protobuf_c/encode_decode.c @@ -630,7 +630,8 @@ static void* startstringwrapper_handler(void* closure, const void* hd, size_t size_hint) { VALUE* rbval = closure; (void)size_hint; - *rbval = get_frozen_string(NULL, 0, false); + *rbval = rb_str_new(NULL, 0); + rb_enc_associate(*rbval, kRubyStringUtf8Encoding); return closure; } @@ -638,7 +639,7 @@ static size_t stringwrapper_handler(void* closure, const void* hd, const char* ptr, size_t len, const upb_bufhandle* handle) { VALUE* rbval = closure; - *rbval = get_frozen_string(ptr, len, false); + *rbval = noleak_rb_str_cat(*rbval, ptr, len); return len; } @@ -646,7 +647,8 @@ static void* startbyteswrapper_handler(void* closure, const void* hd, size_t size_hint) { VALUE* rbval = closure; (void)size_hint; - *rbval = get_frozen_string(NULL, 0, true); + *rbval = rb_str_new(NULL, 0); + rb_enc_associate(*rbval, kRubyString8bitEncoding); return closure; } @@ -654,7 +656,7 @@ static size_t byteswrapper_handler(void* closure, const void* hd, const char* ptr, size_t len, const upb_bufhandle* handle) { VALUE* rbval = closure; - *rbval = get_frozen_string(ptr, len, true); + *rbval = noleak_rb_str_cat(*rbval, ptr, len); return len; } diff --git a/ruby/tests/common_tests.rb b/ruby/tests/common_tests.rb index d20b76cfe6d5..ada42432ebbc 100644 --- a/ruby/tests/common_tests.rb +++ b/ruby/tests/common_tests.rb @@ -1295,9 +1295,9 @@ def test_wrapper_getters assert_equal true, m.bool.value assert_equal true, m.bool_as_value - assert_equal 'str', m.string_as_value - assert_equal 'str', m.string.value - assert_equal 'str', m.string_as_value + assert_equal "st\nr", m.string_as_value + assert_equal "st\nr", m.string.value + assert_equal "st\nr", m.string_as_value assert_equal 'fun', m.bytes_as_value assert_equal 'fun', m.bytes.value @@ -1312,7 +1312,7 @@ def test_wrapper_getters uint32: Google::Protobuf::UInt32Value.new(value: 5), uint64: Google::Protobuf::UInt64Value.new(value: 6), bool: Google::Protobuf::BoolValue.new(value: true), - string: Google::Protobuf::StringValue.new(value: 'str'), + string: Google::Protobuf::StringValue.new(value: "st\nr"), bytes: Google::Protobuf::BytesValue.new(value: 'fun'), real_string: '100' ) @@ -1332,6 +1332,10 @@ def test_wrapper_getters # Test that the lazy form compares equal to the expanded form. m5 = proto_module::Wrapper::decode(serialized2) assert_equal m5, m + + serialized_json = proto_module::Wrapper::encode_json(m) + m6 = proto_module::Wrapper::decode_json(serialized_json) + assert_equal m6, m end def test_repeated_wrappers @@ -1374,6 +1378,12 @@ def test_repeated_wrappers # Test that the lazy form compares equal to the expanded form. m5 = proto_module::Wrapper::decode(serialized2) assert_equal m5, m + + # Test JSON. + serialized_json = proto_module::Wrapper::encode_json(m5) + m6 = proto_module::Wrapper::decode_json(serialized_json) + run_asserts.call(m6) + assert_equal m6, m end def test_oneof_wrappers