[Ruby] Fixed various weirdness with Message#to_h

Fixes: #6167
PiperOrigin-RevId: 594323584

ruby/ext/google/protobuf_c/message.c

@@ -766,58 +766,34 @@ static VALUE Message_CreateHash(const upb_Message* msg,
   if (!msg) return Qnil;
 
   VALUE hash = rb_hash_new();
-  int n = upb_MessageDef_FieldCount(m);
-  bool is_proto2;
-
-  // We currently have a few behaviors that are specific to proto2.
-  // This is unfortunate, we should key behaviors off field attributes (like
-  // whether a field has presence), not proto2 vs. proto3. We should see if we
-  // can change this without breaking users.
-  is_proto2 = upb_MessageDef_Syntax(m) == kUpb_Syntax_Proto2;
-
-  for (int i = 0; i < n; i++) {
-    const upb_FieldDef* field = upb_MessageDef_Field(m, i);
-    TypeInfo type_info = TypeInfo_get(field);
-    upb_MessageValue msgval;
-    VALUE msg_value;
-    VALUE msg_key;
-
-    if (!is_proto2 && upb_FieldDef_IsSubMessage(field) &&
-        !upb_FieldDef_IsRepeated(field) &&
-        !upb_Message_HasFieldByDef(msg, field)) {
-      // TODO: Legacy behavior, remove when we fix the is_proto2 differences.
-      msg_key = ID2SYM(rb_intern(upb_FieldDef_Name(field)));
-      rb_hash_aset(hash, msg_key, Qnil);
-      continue;
-    }
+  size_t iter = kUpb_Message_Begin;
+  const upb_DefPool* pool = upb_FileDef_Pool(upb_MessageDef_File(m));
+  const upb_FieldDef* field;
+  upb_MessageValue val;
 
-    // Do not include fields that are not present (oneof or optional fields).
-    if (is_proto2 && upb_FieldDef_HasPresence(field) &&
-        !upb_Message_HasFieldByDef(msg, field)) {
+  while (upb_Message_Next(msg, m, pool, &field, &val, &iter)) {
+    if (upb_FieldDef_IsExtension(field)) {
+      // TODO: allow extensions once we have decided what naming scheme the
+      // symbol should use.  eg. :"[pkg.ext]"
       continue;
     }
 
-    msg_key = ID2SYM(rb_intern(upb_FieldDef_Name(field)));
-    msgval = upb_Message_GetFieldByDef(msg, field);
-
-    // Proto2 omits empty map/repeated filds also.
+    TypeInfo type_info = TypeInfo_get(field);
+    VALUE msg_value;
 
     if (upb_FieldDef_IsMap(field)) {
       const upb_MessageDef* entry_m = upb_FieldDef_MessageSubDef(field);
       const upb_FieldDef* key_f = upb_MessageDef_FindFieldByNumber(entry_m, 1);
       const upb_FieldDef* val_f = upb_MessageDef_FindFieldByNumber(entry_m, 2);
       upb_CType key_type = upb_FieldDef_CType(key_f);
-      msg_value = Map_CreateHash(msgval.map_val, key_type, TypeInfo_get(val_f));
+      msg_value = Map_CreateHash(val.map_val, key_type, TypeInfo_get(val_f));
     } else if (upb_FieldDef_IsRepeated(field)) {
-      if (is_proto2 &&
-          (!msgval.array_val || upb_Array_Size(msgval.array_val) == 0)) {
-        continue;
-      }
-      msg_value = RepeatedField_CreateArray(msgval.array_val, type_info);
+      msg_value = RepeatedField_CreateArray(val.array_val, type_info);
     } else {
-      msg_value = Scalar_CreateHash(msgval, type_info);
+      msg_value = Scalar_CreateHash(val, type_info);
     }
 
+    VALUE msg_key = ID2SYM(rb_intern(upb_FieldDef_Name(field)));
     rb_hash_aset(hash, msg_key, msg_value);
   }
 

ruby/lib/google/protobuf/ffi/ffi.rb

@@ -190,6 +190,8 @@ class MessageValue < ::FFI::Union
                :str_val, StringView
       end
 
+      Upb_Message_Begin = -1
+
       class MutableMessageValue < ::FFI::Union
         layout :map, :Map,
                :msg, :Message,

ruby/lib/google/protobuf/ffi/internal/convert.rb

@@ -190,39 +190,21 @@ def convert_upb_to_ruby(message_value, c_type, msg_or_enum_def = nil, arena = ni
         def to_h_internal(msg, message_descriptor)
           return nil if msg.nil? or msg.null?
           hash = {}
-          is_proto2 = Google::Protobuf::FFI.message_def_syntax(message_descriptor) == :Proto2
-          message_descriptor.each do |field_descriptor|
-            # TODO: Legacy behavior, remove when we fix the is_proto2 differences.
-            if !is_proto2 and
-              field_descriptor.sub_message? and
-              !field_descriptor.repeated? and
-              !Google::Protobuf::FFI.get_message_has(msg, field_descriptor)
-              hash[field_descriptor.name.to_sym] = nil
-              next
-            end
-
-            # Do not include fields that are not present (oneof or optional fields).
-            if is_proto2 and field_descriptor.has_presence? and !Google::Protobuf::FFI.get_message_has(msg, field_descriptor)
-              next
-            end
-
-            message_value = Google::Protobuf::FFI.get_message_value msg, field_descriptor
+          iter = ::FFI::MemoryPointer.new(:size_t, 1)
+          iter.write(:size_t, Google::Protobuf::FFI::Upb_Message_Begin)
+          message_value = Google::Protobuf::FFI::MessageValue.new
+          field_descriptor = Google::Protobuf::FFI::FieldDescriptor.new
 
-            # Proto2 omits empty map/repeated fields also.
+          while Google::Protobuf::FFI::message_next(msg, message_descriptor, nil, message_value, field_descriptor, iter) do
             if field_descriptor.map?
               hash_entry = map_create_hash(message_value[:map_val], field_descriptor)
             elsif field_descriptor.repeated?
-              array = message_value[:array_val]
-              if is_proto2 and (array.null? || Google::Protobuf::FFI.array_size(array).zero?)
-                next
-              end
-              hash_entry = repeated_field_create_array(array, field_descriptor, field_descriptor.type)
+              hash_entry = repeated_field_create_array(message_value[:array_val], field_descriptor, field_descriptor.type)
             else
               hash_entry = scalar_create_hash(message_value, field_descriptor.type, field_descriptor: field_descriptor)
             end
 
             hash[field_descriptor.name.to_sym] = hash_entry
-
           end
 
           hash

ruby/lib/google/protobuf/ffi/message.rb

@@ -23,6 +23,7 @@ class FFI
       attach_function :get_mutable_message,     :upb_Message_Mutable,         [:Message, FieldDescriptor, Internal::Arena], MutableMessageValue.by_value
       attach_function :get_message_which_oneof, :upb_Message_WhichOneof,      [:Message, OneofDescriptor], FieldDescriptor
       attach_function :message_discard_unknown, :upb_Message_DiscardUnknown,  [:Message, Descriptor, :int], :bool
+      attach_function :message_next,            :upb_Message_Next,            [:Message, Descriptor, :DefPool, :FieldDefPointer, MessageValue.by_ref, :pointer], :bool
       # MessageValue
       attach_function :message_value_equal,     :shared_Msgval_IsEqual,       [MessageValue.by_value, MessageValue.by_value, CType, Descriptor], :bool
       attach_function :message_value_hash,      :shared_Msgval_GetHash,       [MessageValue.by_value, CType, Descriptor, :uint64_t], :uint64_t

ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java

@@ -787,33 +787,29 @@ public static IRubyObject decodeJson(
   public IRubyObject toHash(ThreadContext context) {
     Ruby runtime = context.runtime;
     RubyHash ret = RubyHash.newHash(runtime);
-    for (FieldDescriptor fdef : this.descriptor.getFields()) {
+    build(context, 0, SINK_MAXIMUM_NESTING); // Sync Ruby data to the Builder object.
+    for (Map.Entry<FieldDescriptor, Object> field : builder.getAllFields().entrySet()) {
+      FieldDescriptor fdef = field.getKey();
       IRubyObject value = getFieldInternal(context, fdef, proto3);
 
-      if (!value.isNil()) {
-        if (fdef.isRepeated() && !fdef.isMapField()) {
-          if (!proto3 && ((RubyRepeatedField) value).size() == 0)
-            continue; // Don't output empty repeated fields for proto2
-          if (fdef.getType() != FieldDescriptor.Type.MESSAGE) {
-            value = Helpers.invoke(context, value, "to_a");
-          } else {
-            RubyArray ary = value.convertToArray();
-            for (int i = 0; i < ary.size(); i++) {
-              IRubyObject submsg = Helpers.invoke(context, ary.eltInternal(i), "to_h");
-              ary.eltInternalSet(i, submsg);
-            }
-
-            value = ary.to_ary();
-          }
-        } else if (value.respondsTo("to_h")) {
-          value = Helpers.invoke(context, value, "to_h");
-        } else if (value.respondsTo("to_a")) {
+      if (fdef.isRepeated() && !fdef.isMapField()) {
+        if (fdef.getType() != FieldDescriptor.Type.MESSAGE) {
           value = Helpers.invoke(context, value, "to_a");
+        } else {
+          RubyArray ary = value.convertToArray();
+          for (int i = 0; i < ary.size(); i++) {
+            IRubyObject submsg = Helpers.invoke(context, ary.eltInternal(i), "to_h");
+            ary.eltInternalSet(i, submsg);
+          }
+
+          value = ary.to_ary();
         }
+      } else if (value.respondsTo("to_h")) {
+        value = Helpers.invoke(context, value, "to_h");
+      } else if (value.respondsTo("to_a")) {
+        value = Helpers.invoke(context, value, "to_a");
       }
-      if (proto3 || !value.isNil()) {
-        ret.fastASet(runtime.newSymbol(fdef.getName()), value);
-      }
+      ret.fastASet(runtime.newSymbol(fdef.getName()), value);
     }
     return ret;
   }

ruby/tests/basic.rb

@@ -513,32 +513,19 @@ def test_protobuf_decode_json_ignore_unknown_fields
     #end
 
     def test_to_h
-      m = TestMessage.new(:optional_bool => true, :optional_double => -10.100001, :optional_string => 'foo', :repeated_string => ['bar1', 'bar2'], :repeated_msg => [TestMessage2.new(:foo => 100)])
+      m = TestMessage.new(
+        :optional_bool => true,
+        :optional_double => -10.100001,
+        :optional_string => 'foo',
+        :repeated_string => ['bar1', 'bar2'],
+        :repeated_msg => [TestMessage2.new(:foo => 100)]
+      )
       expected_result = {
         :optional_bool=>true,
-        :optional_bytes=>"",
         :optional_double=>-10.100001,
-        :optional_enum=>:Default,
-        :optional_float=>0.0,
-        :optional_int32=>0,
-        :optional_int64=>0,
-        :optional_msg=>nil,
-        :optional_msg2=>nil,
-        :optional_proto2_submessage=>nil,
         :optional_string=>"foo",
-        :optional_uint32=>0,
-        :optional_uint64=>0,
-        :repeated_bool=>[],
-        :repeated_bytes=>[],
-        :repeated_double=>[],
-        :repeated_enum=>[],
-        :repeated_float=>[],
-        :repeated_int32=>[],
-        :repeated_int64=>[],
-        :repeated_msg=>[{:foo => 100}],
         :repeated_string=>["bar1", "bar2"],
-        :repeated_uint32=>[],
-        :repeated_uint64=>[]
+        :repeated_msg=>[{:foo => 100}],
       }
       assert_equal expected_result, m.to_h