Copilot commented on code in PR #3703:
URL: https://github.com/apache/thrift/pull/3703#discussion_r3723710182
##########
lib/rb/lib/thrift/protocol/compact_protocol.rb:
##########
@@ -389,44 +437,51 @@ def to_s
# the wire differ only by the type indicator.
#
def write_collection_begin(elem_type, size)
+ size = validate_size(size)
+ compact_type = CompactTypes.get_compact_type(elem_type)
if size <= 14
- write_byte(size << 4 | CompactTypes.get_compact_type(elem_type))
+ write_byte_direct(size << 4 | compact_type)
else
- write_byte(0xf0 | CompactTypes.get_compact_type(elem_type))
+ write_byte_direct(0xf0 | compact_type)
write_varint32(size)
end
end
+ def write_byte_direct(byte)
+ @trans.write([byte].pack('C'))
+ end
Review Comment:
write_byte_direct packs with 'C' (unsigned) but callers pass signed byte
values like PROTOCOL_ID (-126). This will raise RangeError in pure-Ruby
CompactProtocol (e.g., write_message_begin) and also breaks writing negative
Thrift BYTE values via write_byte.
##########
lib/rb/ext/compact_protocol.c:
##########
@@ -243,13 +279,15 @@ VALUE rb_thrift_compact_proto_write_field_stop(VALUE
self) {
}
VALUE rb_thrift_compact_proto_write_map_begin(VALUE self, VALUE ktype, VALUE
vtype, VALUE size_value) {
- int size = FIX2INT(size_value);
+ int size = checked_size_value(size_value);
+ int key_type = get_compact_type(ktype);
+ int value_type = get_compact_type(vtype);
VALUE transport = GET_TRANSPORT(self);
Review Comment:
Native write_map_begin now calls get_compact_type for ktype/vtype even when
size == 0, whereas the wire encoding for empty maps does not include types.
This is a behavioral/API change (can raise on empty maps with
placeholder/invalid types) and does extra work on the hot path.
##########
lib/rb/lib/thrift/protocol/compact_protocol.rb:
##########
@@ -155,32 +175,35 @@ def write_field_begin_internal(type, id, type_override =
nil)
last_id = @last_field.pop
# if there's a type override, use that.
- typeToWrite = type_override || CompactTypes.get_compact_type(type)
+ type_to_write = type_override || CompactTypes.get_compact_type(type)
# check if we can use delta encoding for the field id
if id > last_id && id - last_id <= 15
# write them together
- write_byte((id - last_id) << 4 | typeToWrite)
+ write_byte_direct((id - last_id) << 4 | type_to_write)
else
# write them separate
- write_byte(typeToWrite)
- write_i16(id)
+ write_byte_direct(type_to_write)
+ write_varint32(int_to_zig_zag(id))
end
@last_field.push(id)
nil
end
def write_field_stop
- write_byte(Types::STOP)
+ write_byte_direct(Types::STOP)
end
def write_map_begin(ktype, vtype, size)
+ size = validate_size(size)
+ key_type = CompactTypes.get_compact_type(ktype)
+ value_type = CompactTypes.get_compact_type(vtype)
if (size == 0)
Review Comment:
write_map_begin now resolves ktype/vtype compact types even when size == 0.
Previously, empty maps wrote a single 0 byte without validating types; this is
a behavioral/API change and can raise for callers that pass placeholder types
for empty maps.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
