Skip to content

Commit

Permalink
Fix for wrappers with a zero value (#7195) (#7201)
Browse files Browse the repository at this point in the history 
* 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 <[email protected]>
Co-authored-by: Dan Quan <[email protected]>
  • Loading branch information
@rafi-kamal @haberman @djquan
3 people authored Feb 12, 2020
1 parent 1440569 commit 37fc432
Show file tree
Hide file tree
Showing 3 changed files with 165 additions and 21 deletions.
76 changes: 55 additions & 21 deletions ruby/ext/google/protobuf_c/encode_decode.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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@.

Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
80 changes: 80 additions & 0 deletions ruby/tests/basic.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,86 @@ def test_map_wrappers
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
Expand Down
30 changes: 30 additions & 0 deletions ruby/tests/common_tests.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1265,6 +1265,36 @@ def test_comparison_with_arbitrary_object
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
Expand Down

0 comments on commit 37fc432

Please sign in to comment.