Skip to content

THRIFT-6439: Isolate nested Ruby struct defaults #4030

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kpumuk merged 1 commit into from
Oct 29, 2026
Merged

THRIFT-6439: Isolate nested Ruby struct defaults #4030

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 57 additions & 34 deletions lib/rb/ext/struct.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ ID to_s_method_id;
ID name_to_id_method_id;
static ID sorted_field_ids_method_id;
static ID validate_container_size_method_id;
static ID copy_default_value_method_id;
static VALUE thrift_struct_module;
static VALUE default_sym;

#define IS_CONTAINER(ttype) ((ttype) == TTYPE_MAP || (ttype) == TTYPE_LIST || (ttype) == TTYPE_SET)
Expand Down Expand Up @@ -483,20 +485,6 @@ static VALUE rb_thrift_struct_read_recursive(VALUE self, VALUE protocol, int rem
static void skip_map_contents(VALUE protocol, VALUE key_type_value, VALUE value_type_value, int size);
static void skip_list_or_set_contents(VALUE protocol, VALUE element_type_value, int size);

static inline void set_field_value(VALUE obj, VALUE field_name, VALUE value) {
rb_ivar_set(obj, field_ivar_id(field_name), value);
}

/* Ruby's rescue and iteration APIs require addressable callback functions. */
static VALUE duplicate_default_value(VALUE value) {
return rb_obj_dup(value);
}

static VALUE retain_default_value(VALUE value, VALUE exception) {
(void)exception;
return value;
}

static int reset_struct_field(VALUE field_id, VALUE field_info, VALUE self) {
(void)field_id;

Expand All @@ -509,14 +497,14 @@ static int reset_struct_field(VALUE field_id, VALUE field_info, VALUE self) {
rb_ivar_set(self, ivar_id, Qnil);
}
} else {
VALUE value = rb_rescue2(
duplicate_default_value,
default_value,
retain_default_value,
default_value,
rb_eStandardError,
(VALUE)0
);
VALUE value;
if (RB_TYPE_P(default_value, T_STRING)) {
value = rb_str_dup(default_value);
} else if (RB_SPECIAL_CONST_P(default_value) || RB_TYPE_P(default_value, T_FLOAT) || RB_TYPE_P(default_value, T_BIGNUM)) {
value = default_value;
} else {
value = rb_funcall(thrift_struct_module, copy_default_value_method_id, 1, default_value);
}
Comment thread
kpumuk marked this conversation as resolved.
Resolved
Comment thread
kpumuk marked this conversation as resolved.
Resolved
rb_ivar_set(self, ivar_id, value);
}

Expand Down Expand Up @@ -667,13 +655,18 @@ static VALUE read_anything(VALUE protocol, int ttype, VALUE field_info, int rema
return result;
}

static VALUE rb_thrift_struct_read_recursive(VALUE self, VALUE protocol, int remaining_depth) {
VALUE struct_fields = STRUCT_FIELDS(self);
struct struct_read_context {
VALUE self;
VALUE protocol;
VALUE fields;
VALUE unread_fields;
int remaining_depth;
};

if (RHASH_SIZE(struct_fields) > 0 && rb_ivar_count(self) > 0) {
rb_check_frozen(self);
rb_hash_foreach(struct_fields, reset_struct_field, self);
}
static VALUE read_struct_fields(VALUE data) {
struct struct_read_context *context = (struct struct_read_context *)data;
VALUE self = context->self;
VALUE protocol = context->protocol;

// read struct begin
default_read_struct_begin(protocol);
Expand All @@ -689,14 +682,19 @@ static VALUE rb_thrift_struct_read_recursive(VALUE self, VALUE protocol, int rem
}

// make sure we got a type we expected
VALUE field_info = rb_hash_aref(struct_fields, rb_ary_entry(field_header, 2));
VALUE field_id = rb_ary_entry(field_header, 2);
VALUE field_info = rb_hash_aref(context->fields, field_id);

if (!NIL_P(field_info)) {
int specified_type = FIX2INT(rb_hash_aref(field_info, type_sym));
if (field_type == specified_type) {
// read the value
VALUE name = rb_hash_aref(field_info, name_sym);
set_field_value(self, name, read_anything(protocol, field_type, field_info, remaining_depth));
VALUE value = read_anything(protocol, field_type, field_info, context->remaining_depth);
rb_ivar_set(self, field_ivar_id(name), value);
if (!NIL_P(context->unread_fields)) {
rb_hash_delete(context->unread_fields, field_id);
}
} else {
rb_funcall(protocol, skip_method_id, 1, field_type_value);
}
Expand All @@ -711,7 +709,30 @@ static VALUE rb_thrift_struct_read_recursive(VALUE self, VALUE protocol, int rem
// read struct end
default_read_struct_end(protocol);

// call validate
return Qnil;
}

static VALUE reset_unread_struct_fields(VALUE data) {
struct struct_read_context *context = (struct struct_read_context *)data;
rb_hash_foreach(context->unread_fields, reset_struct_field, context->self);
return Qnil;
}

static VALUE rb_thrift_struct_read_recursive(VALUE self, VALUE protocol, int remaining_depth) {
VALUE fields = STRUCT_FIELDS(self);
VALUE unread_fields = Qnil;
if (RHASH_SIZE(fields) > 0 && rb_ivar_count(self) > 0) {
rb_check_frozen(self);
unread_fields = rb_hash_dup(fields);
}

struct struct_read_context context = {self, protocol, fields, unread_fields, remaining_depth};
if (NIL_P(unread_fields)) {
read_struct_fields((VALUE)&context);
} else {
// Reset omitted fields on success or failure, without copying received defaults.
rb_ensure(read_struct_fields, (VALUE)&context, reset_unread_struct_fields, (VALUE)&context);
}
rb_funcall(self, validate_method_id, 0);

return Qnil;
Expand Down Expand Up @@ -837,12 +858,13 @@ static VALUE rb_thrift_union_write(int argc, VALUE *argv, VALUE self) {
}

void Init_struct(void) {
VALUE struct_module = rb_const_get(thrift_module, rb_intern("Struct"));
thrift_struct_module = rb_const_get(thrift_module, rb_intern("Struct"));
rb_global_variable(&thrift_struct_module);

recursion_limit = FIX2INT(rb_const_get(thrift_module, rb_intern("DEFAULT_RECURSION_DEPTH")));

rb_define_method(struct_module, "write", rb_thrift_struct_write, -1);
rb_define_method(struct_module, "read", rb_thrift_struct_read, -1);
rb_define_method(thrift_struct_module, "write", rb_thrift_struct_write, -1);
rb_define_method(thrift_struct_module, "read", rb_thrift_struct_read, -1);

thrift_union_class = rb_const_get(thrift_module, rb_intern("Union"));
rb_global_variable(&thrift_union_class);
Expand All @@ -866,6 +888,7 @@ void Init_struct(void) {
rb_global_variable(&sorted_field_ids_method_id);

validate_container_size_method_id = rb_intern("validate_container_size");
copy_default_value_method_id = rb_intern("copy_default_value");

default_sym = ID2SYM(rb_intern("default"));
rb_global_variable(&default_sym);
Expand Down
69 changes: 54 additions & 15 deletions lib/rb/lib/thrift/struct.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ def initialize(d = {}, &block)
# assign all the default values
unless fields_with_defaults.empty?
fields_with_defaults.each do |name, default_value|
instance_variable_set("@#{name}", (default_value.dup rescue default_value))
instance_variable_set("@#{name}", Struct.copy_default_value(default_value))
end
end

Expand Down Expand Up @@ -85,23 +85,25 @@ def inspect(skip_optional_nulls = true)
def read(iprot, remaining_depth = DEFAULT_RECURSION_DEPTH)
raise ProtocolException.new(ProtocolException::DEPTH_LIMIT, "Maximum recursion depth exceeded") if remaining_depth <= 0
unless instance_variables.empty?
defaults = fields_with_default_values
struct_fields.each_value do |field_info|
instance_variable_set("@#{field_info[:name]}", nil)
end
defaults.each do |name, default_value|
instance_variable_set("@#{name}", (default_value.dup rescue default_value))
end
unread_fields = struct_fields.dup
raise FrozenError, "can't modify frozen #{self.class}" if frozen? && !unread_fields.empty?
end

iprot.read_struct_begin
loop do
_, ftype, fid = iprot.read_field_begin
break if (ftype == Types::STOP)
handle_message(iprot, fid, ftype, remaining_depth)
iprot.read_field_end
begin
iprot.read_struct_begin
loop do
_, ftype, fid = iprot.read_field_begin
break if (ftype == Types::STOP)
unread_fields&.delete(fid) if handle_message(iprot, fid, ftype, remaining_depth)
iprot.read_field_end
end
iprot.read_struct_end
ensure
# Reset omitted fields even after a failed read into a reused object.
unread_fields&.each_value do |field|
instance_variable_set("@#{field[:name]}", Struct.copy_default_value(field[:default]))
end
end
iprot.read_struct_end
validate
end

Expand Down Expand Up @@ -212,6 +214,40 @@ def <=>(other)
end
end

# Copy generated Thrift defaults, not arbitrary Ruby object graphs.
# Container subclasses and custom Hash behavior are not preserved;
# unrecognized objects are retained.
def self.copy_default_value(value)
case value
when Numeric
value
when String
value.dup
when Array
value.map { |element| copy_default_value(element) }
when Hash
value.each_with_object({}) do |(key, element), copy|
copy[copy_default_value(key)] = copy_default_value(element)
end
Comment thread
kpumuk marked this conversation as resolved.
Resolved
when Set
value.each_with_object(Set.new) { |element, copy| copy.add(copy_default_value(element)) }
Comment thread
kpumuk marked this conversation as resolved.
Resolved
when Struct, Union
copy = value.dup
if value.is_a?(Union)
copy.instance_variable_set(:@setfield, copy_default_value(value.get_set_field))
copy.instance_variable_set(:@value, copy_default_value(value.get_value))
else
value.struct_fields.each_value do |field_info|
ivar = "@#{field_info[:name]}"
copy.instance_variable_set(ivar, copy_default_value(value.instance_variable_get(ivar)))
end
end
copy
Comment thread
kpumuk marked this conversation as resolved.
Resolved
else
value
end
Comment thread
kpumuk marked this conversation as resolved.
Resolved
end
Comment thread
kpumuk marked this conversation as resolved.
Resolved

protected

def self.append_features(mod)
Expand Down Expand Up @@ -245,8 +281,11 @@ def handle_message(iprot, fid, ftype, remaining_depth)
if field and field[:type] == ftype
value = read_field(iprot, field, remaining_depth)
instance_variable_set("@#{field[:name]}", value)
# A received field can legitimately result in false or nil.
true
else
iprot.skip(ftype)
false
end
end
end
Expand Down
20 changes: 20 additions & 0 deletions lib/rb/spec/ThriftSpec.thrift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,26 @@ struct Foo2 {
1: binary my_binary
}

struct NestedDefaults {
1: list<list<i32>> nested_list = [[1]]
2: map<string, list<string>> nested_map = {"key": ["value"]}
3: set<list<i32>> nested_set = [[1]]
4: Hello child = {"greeting": "child"}
5: string text = "fixed"
6: i32 scalar = 7
8: binary data = "binary"
9: uuid token = "00112233-4455-6677-8899-aabbccddeeff"
}

struct NestedDefaultsList {
1: list<NestedDefaults> values
}

struct DefaultCopyNames {
1: string copy_default_value = "literal"
2: Hello copy_struct_default = {"greeting": "child"}
}

struct BoolStruct {
1: bool yesno = 1
}
Expand Down
Loading
Loading