Skip to content

Commit fd69938

Browse files
habermancopybara-github
haberman
authored and
copybara-github
committed
Breaking Change: Fixed inconsistencies in Message#to_h, [as previously announced](https://protobuf.dev/news/2023-12-27/).
Fixes: #6167 PiperOrigin-RevId: 595733611
1 parent 4407d1e commit fd69938

7 files changed

Lines changed: 54 additions & 107 deletions

File tree

‎ruby/ext/google/protobuf_c/message.c‎

Lines changed: 14 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -766,58 +766,34 @@ static VALUE Message_CreateHash(const upb_Message* msg,
766766
if (!msg) return Qnil;
767767

768768
VALUE hash = rb_hash_new();
769-
int n = upb_MessageDef_FieldCount(m);
770-
bool is_proto2;
771-
772-
// We currently have a few behaviors that are specific to proto2.
773-
// This is unfortunate, we should key behaviors off field attributes (like
774-
// whether a field has presence), not proto2 vs. proto3. We should see if we
775-
// can change this without breaking users.
776-
is_proto2 = upb_MessageDef_Syntax(m) == kUpb_Syntax_Proto2;
777-
778-
for (int i = 0; i < n; i++) {
779-
const upb_FieldDef* field = upb_MessageDef_Field(m, i);
780-
TypeInfo type_info = TypeInfo_get(field);
781-
upb_MessageValue msgval;
782-
VALUE msg_value;
783-
VALUE msg_key;
784-
785-
if (!is_proto2 && upb_FieldDef_IsSubMessage(field) &&
786-
!upb_FieldDef_IsRepeated(field) &&
787-
!upb_Message_HasFieldByDef(msg, field)) {
788-
// TODO: Legacy behavior, remove when we fix the is_proto2 differences.
789-
msg_key = ID2SYM(rb_intern(upb_FieldDef_Name(field)));
790-
rb_hash_aset(hash, msg_key, Qnil);
791-
continue;
792-
}
769+
size_t iter = kUpb_Message_Begin;
770+
const upb_DefPool* pool = upb_FileDef_Pool(upb_MessageDef_File(m));
771+
const upb_FieldDef* field;
772+
upb_MessageValue val;
793773

794-
// Do not include fields that are not present (oneof or optional fields).
795-
if (is_proto2 && upb_FieldDef_HasPresence(field) &&
796-
!upb_Message_HasFieldByDef(msg, field)) {
774+
while (upb_Message_Next(msg, m, pool, &field, &val, &iter)) {
775+
if (upb_FieldDef_IsExtension(field)) {
776+
// TODO: allow extensions once we have decided what naming scheme the
777+
// symbol should use. eg. :"[pkg.ext]"
797778
continue;
798779
}
799780

800-
msg_key = ID2SYM(rb_intern(upb_FieldDef_Name(field)));
801-
msgval = upb_Message_GetFieldByDef(msg, field);
802-
803-
// Proto2 omits empty map/repeated filds also.
781+
TypeInfo type_info = TypeInfo_get(field);
782+
VALUE msg_value;
804783

805784
if (upb_FieldDef_IsMap(field)) {
806785
const upb_MessageDef* entry_m = upb_FieldDef_MessageSubDef(field);
807786
const upb_FieldDef* key_f = upb_MessageDef_FindFieldByNumber(entry_m, 1);
808787
const upb_FieldDef* val_f = upb_MessageDef_FindFieldByNumber(entry_m, 2);
809788
upb_CType key_type = upb_FieldDef_CType(key_f);
810-
msg_value = Map_CreateHash(msgval.map_val, key_type, TypeInfo_get(val_f));
789+
msg_value = Map_CreateHash(val.map_val, key_type, TypeInfo_get(val_f));
811790
} else if (upb_FieldDef_IsRepeated(field)) {
812-
if (is_proto2 &&
813-
(!msgval.array_val || upb_Array_Size(msgval.array_val) == 0)) {
814-
continue;
815-
}
816-
msg_value = RepeatedField_CreateArray(msgval.array_val, type_info);
791+
msg_value = RepeatedField_CreateArray(val.array_val, type_info);
817792
} else {
818-
msg_value = Scalar_CreateHash(msgval, type_info);
793+
msg_value = Scalar_CreateHash(val, type_info);
819794
}
820795

796+
VALUE msg_key = ID2SYM(rb_intern(upb_FieldDef_Name(field)));
821797
rb_hash_aset(hash, msg_key, msg_value);
822798
}
823799

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -190,6 +190,8 @@ class MessageValue < ::FFI::Union
190190
:str_val, StringView
191191
end
192192

193+
Upb_Message_Begin = -1
194+
193195
class MutableMessageValue < ::FFI::Union
194196
layout :map, :Map,
195197
:msg, :Message,

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

Lines changed: 7 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -190,39 +190,23 @@ def convert_upb_to_ruby(message_value, c_type, msg_or_enum_def = nil, arena = ni
190190
def to_h_internal(msg, message_descriptor)
191191
return nil if msg.nil? or msg.null?
192192
hash = {}
193-
is_proto2 = Google::Protobuf::FFI.message_def_syntax(message_descriptor) == :Proto2
194-
message_descriptor.each do |field_descriptor|
195-
# TODO: Legacy behavior, remove when we fix the is_proto2 differences.
196-
if !is_proto2 and
197-
field_descriptor.sub_message? and
198-
!field_descriptor.repeated? and
199-
!Google::Protobuf::FFI.get_message_has(msg, field_descriptor)
200-
hash[field_descriptor.name.to_sym] = nil
201-
next
202-
end
203-
204-
# Do not include fields that are not present (oneof or optional fields).
205-
if is_proto2 and field_descriptor.has_presence? and !Google::Protobuf::FFI.get_message_has(msg, field_descriptor)
206-
next
207-
end
193+
iter = ::FFI::MemoryPointer.new(:size_t, 1)
194+
iter.write(:size_t, Google::Protobuf::FFI::Upb_Message_Begin)
195+
message_value = Google::Protobuf::FFI::MessageValue.new
196+
field_def_ptr = ::FFI::MemoryPointer.new :pointer
208197

209-
message_value = Google::Protobuf::FFI.get_message_value msg, field_descriptor
198+
while Google::Protobuf::FFI::message_next(msg, message_descriptor, nil, field_def_ptr, message_value, iter) do
199+
field_descriptor = FieldDescriptor.from_native field_def_ptr.get_pointer(0)
210200

211-
# Proto2 omits empty map/repeated fields also.
212201
if field_descriptor.map?
213202
hash_entry = map_create_hash(message_value[:map_val], field_descriptor)
214203
elsif field_descriptor.repeated?
215-
array = message_value[:array_val]
216-
if is_proto2 and (array.null? || Google::Protobuf::FFI.array_size(array).zero?)
217-
next
218-
end
219-
hash_entry = repeated_field_create_array(array, field_descriptor, field_descriptor.type)
204+
hash_entry = repeated_field_create_array(message_value[:array_val], field_descriptor, field_descriptor.type)
220205
else
221206
hash_entry = scalar_create_hash(message_value, field_descriptor.type, field_descriptor: field_descriptor)
222207
end
223208

224209
hash[field_descriptor.name.to_sym] = hash_entry
225-
226210
end
227211

228212
hash

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ class FFI
2323
attach_function :get_mutable_message, :upb_Message_Mutable, [:Message, FieldDescriptor, Internal::Arena], MutableMessageValue.by_value
2424
attach_function :get_message_which_oneof, :upb_Message_WhichOneof, [:Message, OneofDescriptor], FieldDescriptor
2525
attach_function :message_discard_unknown, :upb_Message_DiscardUnknown, [:Message, Descriptor, :int], :bool
26+
attach_function :message_next, :upb_Message_Next, [:Message, Descriptor, :DefPool, :FieldDefPointer, MessageValue.by_ref, :pointer], :bool
2627
# MessageValue
2728
attach_function :message_value_equal, :shared_Msgval_IsEqual, [MessageValue.by_value, MessageValue.by_value, CType, Descriptor], :bool
2829
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‎

Lines changed: 18 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -787,33 +787,29 @@ public static IRubyObject decodeJson(
787787
public IRubyObject toHash(ThreadContext context) {
788788
Ruby runtime = context.runtime;
789789
RubyHash ret = RubyHash.newHash(runtime);
790-
for (FieldDescriptor fdef : this.descriptor.getFields()) {
790+
build(context, 0, SINK_MAXIMUM_NESTING); // Sync Ruby data to the Builder object.
791+
for (Map.Entry<FieldDescriptor, Object> field : builder.getAllFields().entrySet()) {
792+
FieldDescriptor fdef = field.getKey();
791793
IRubyObject value = getFieldInternal(context, fdef, proto3);
792794

793-
if (!value.isNil()) {
794-
if (fdef.isRepeated() && !fdef.isMapField()) {
795-
if (!proto3 && ((RubyRepeatedField) value).size() == 0)
796-
continue; // Don't output empty repeated fields for proto2
797-
if (fdef.getType() != FieldDescriptor.Type.MESSAGE) {
798-
value = Helpers.invoke(context, value, "to_a");
799-
} else {
800-
RubyArray ary = value.convertToArray();
801-
for (int i = 0; i < ary.size(); i++) {
802-
IRubyObject submsg = Helpers.invoke(context, ary.eltInternal(i), "to_h");
803-
ary.eltInternalSet(i, submsg);
804-
}
805-
806-
value = ary.to_ary();
807-
}
808-
} else if (value.respondsTo("to_h")) {
809-
value = Helpers.invoke(context, value, "to_h");
810-
} else if (value.respondsTo("to_a")) {
795+
if (fdef.isRepeated() && !fdef.isMapField()) {
796+
if (fdef.getType() != FieldDescriptor.Type.MESSAGE) {
811797
value = Helpers.invoke(context, value, "to_a");
798+
} else {
799+
RubyArray ary = value.convertToArray();
800+
for (int i = 0; i < ary.size(); i++) {
801+
IRubyObject submsg = Helpers.invoke(context, ary.eltInternal(i), "to_h");
802+
ary.eltInternalSet(i, submsg);
803+
}
804+
805+
value = ary.to_ary();
812806
}
807+
} else if (value.respondsTo("to_h")) {
808+
value = Helpers.invoke(context, value, "to_h");
809+
} else if (value.respondsTo("to_a")) {
810+
value = Helpers.invoke(context, value, "to_a");
813811
}
814-
if (proto3 || !value.isNil()) {
815-
ret.fastASet(runtime.newSymbol(fdef.getName()), value);
816-
}
812+
ret.fastASet(runtime.newSymbol(fdef.getName()), value);
817813
}
818814
return ret;
819815
}

‎ruby/tests/basic.rb‎

Lines changed: 8 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -469,32 +469,19 @@ def test_protobuf_decode_json_ignore_unknown_fields
469469
#end
470470

471471
def test_to_h
472-
m = TestMessage.new(:optional_bool => true, :optional_double => -10.100001, :optional_string => 'foo', :repeated_string => ['bar1', 'bar2'], :repeated_msg => [TestMessage2.new(:foo => 100)])
472+
m = TestMessage.new(
473+
:optional_bool => true,
474+
:optional_double => -10.100001,
475+
:optional_string => 'foo',
476+
:repeated_string => ['bar1', 'bar2'],
477+
:repeated_msg => [TestMessage2.new(:foo => 100)]
478+
)
473479
expected_result = {
474480
:optional_bool=>true,
475-
:optional_bytes=>"",
476481
:optional_double=>-10.100001,
477-
:optional_enum=>:Default,
478-
:optional_float=>0.0,
479-
:optional_int32=>0,
480-
:optional_int64=>0,
481-
:optional_msg=>nil,
482-
:optional_msg2=>nil,
483-
:optional_proto2_submessage=>nil,
484482
:optional_string=>"foo",
485-
:optional_uint32=>0,
486-
:optional_uint64=>0,
487-
:repeated_bool=>[],
488-
:repeated_bytes=>[],
489-
:repeated_double=>[],
490-
:repeated_enum=>[],
491-
:repeated_float=>[],
492-
:repeated_int32=>[],
493-
:repeated_int64=>[],
494-
:repeated_msg=>[{:foo => 100}],
495483
:repeated_string=>["bar1", "bar2"],
496-
:repeated_uint32=>[],
497-
:repeated_uint64=>[]
484+
:repeated_msg=>[{:foo => 100}],
498485
}
499486
assert_equal expected_result, m.to_h
500487

‎upb/reflection/message.h‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -71,9 +71,10 @@ UPB_API bool upb_Message_SetFieldByDef(upb_Message* msg, const upb_FieldDef* f,
7171

7272
#define kUpb_Message_Begin -1
7373

74-
bool upb_Message_Next(const upb_Message* msg, const upb_MessageDef* m,
75-
const upb_DefPool* ext_pool, const upb_FieldDef** f,
76-
upb_MessageValue* val, size_t* iter);
74+
UPB_API bool upb_Message_Next(const upb_Message* msg, const upb_MessageDef* m,
75+
const upb_DefPool* ext_pool,
76+
const upb_FieldDef** f, upb_MessageValue* val,
77+
size_t* iter);
7778

7879
// Clears all unknown field data from this message and all submessages.
7980
UPB_API bool upb_Message_DiscardUnknown(upb_Message* msg,

0 commit comments

Comments
 (0)