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 <jhaberman@gmail.com>
Co-authored-by: Dan Quan <dan@quan.io>

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 {

ruby/tests/basic.rb

@@ -276,6 +276,86 @@ module BasicTest
       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

ruby/tests/common_tests.rb

@@ -1265,6 +1265,36 @@ module CommonTests
     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