From 0114727cc634971c86696b02f17c594b9ab9db76 Mon Sep 17 00:00:00 2001 From: Joe Bolinger Date: Thu, 31 Jan 2019 14:44:40 -0800 Subject: [PATCH] add more descriptive error messages to init methods --- ruby/ext/google/protobuf_c/map.c | 4 +- ruby/ext/google/protobuf_c/message.c | 6 +- ruby/ext/google/protobuf_c/protobuf.h | 8 +- ruby/ext/google/protobuf_c/repeated_field.c | 4 +- ruby/ext/google/protobuf_c/storage.c | 57 ++++--- ruby/tests/basic.rb | 4 +- ruby/tests/basic_proto2.rb | 2 +- ruby/tests/type_errors.rb | 173 ++++++++++++++++++++ 8 files changed, 224 insertions(+), 34 deletions(-) create mode 100644 ruby/tests/type_errors.rb diff --git a/ruby/ext/google/protobuf_c/map.c b/ruby/ext/google/protobuf_c/map.c index 8c2f6424c..59d64fcb7 100644 --- a/ruby/ext/google/protobuf_c/map.c +++ b/ruby/ext/google/protobuf_c/map.c @@ -82,7 +82,7 @@ static VALUE table_key(Map* self, VALUE key, case UPB_TYPE_INT64: case UPB_TYPE_UINT32: case UPB_TYPE_UINT64: - native_slot_set(self->key_type, Qnil, buf, key); + native_slot_set("", self->key_type, Qnil, buf, key); *out_key = buf; *out_length = native_slot_size(self->key_type); break; @@ -396,7 +396,7 @@ VALUE Map_index_set(VALUE _self, VALUE key, VALUE value) { key = table_key(self, key, keybuf, &keyval, &length); mem = value_memory(&v); - native_slot_set(self->value_type, self->value_type_class, mem, value); + native_slot_set("", self->value_type, self->value_type_class, mem, value); // Replace any existing value by issuing a 'remove' operation first. upb_strtable_remove2(&self->table, keyval, length, NULL); diff --git a/ruby/ext/google/protobuf_c/message.c b/ruby/ext/google/protobuf_c/message.c index fd123aebd..7c3079a4c 100644 --- a/ruby/ext/google/protobuf_c/message.c +++ b/ruby/ext/google/protobuf_c/message.c @@ -315,7 +315,8 @@ int Message_initialize_kwarg(VALUE key, VALUE val, VALUE _self) { if (TYPE(val) != T_HASH) { rb_raise(rb_eArgError, - "Expected Hash object as initializer value for map field '%s'.", name); + "Expected Hash object as initializer value for map field '%s' (given %s).", + name, rb_class2name(CLASS_OF(val))); } map = layout_get(self->descriptor->layout, Message_data(self), f); Map_merge_into_self(map, val); @@ -324,7 +325,8 @@ int Message_initialize_kwarg(VALUE key, VALUE val, VALUE _self) { if (TYPE(val) != T_ARRAY) { rb_raise(rb_eArgError, - "Expected array as initializer value for repeated field '%s'.", name); + "Expected array as initializer value for repeated field '%s' (given %s).", + name, rb_class2name(CLASS_OF(val))); } ary = layout_get(self->descriptor->layout, Message_data(self), f); for (int i = 0; i < RARRAY_LEN(val); i++) { diff --git a/ruby/ext/google/protobuf_c/protobuf.h b/ruby/ext/google/protobuf_c/protobuf.h index eff212e1a..8731eeb47 100644 --- a/ruby/ext/google/protobuf_c/protobuf.h +++ b/ruby/ext/google/protobuf_c/protobuf.h @@ -337,14 +337,16 @@ VALUE Builder_finalize_to_pool(VALUE _self, VALUE pool_rb); #define NATIVE_SLOT_MAX_SIZE sizeof(uint64_t) size_t native_slot_size(upb_fieldtype_t type); -void native_slot_set(upb_fieldtype_t type, +void native_slot_set(const char* name, + upb_fieldtype_t type, VALUE type_class, void* memory, VALUE value); // Atomically (with respect to Ruby VM calls) either update the value and set a // oneof case, or do neither. If |case_memory| is null, then no case value is // set. -void native_slot_set_value_and_case(upb_fieldtype_t type, +void native_slot_set_value_and_case(const char* name, + upb_fieldtype_t type, VALUE type_class, void* memory, VALUE value, @@ -360,7 +362,7 @@ void native_slot_deep_copy(upb_fieldtype_t type, void* to, void* from); bool native_slot_eq(upb_fieldtype_t type, void* mem1, void* mem2); VALUE native_slot_encode_and_freeze_string(upb_fieldtype_t type, VALUE value); -void native_slot_check_int_range_precision(upb_fieldtype_t type, VALUE value); +void native_slot_check_int_range_precision(const char* name, upb_fieldtype_t type, VALUE value); extern rb_encoding* kRubyStringUtf8Encoding; extern rb_encoding* kRubyStringASCIIEncoding; diff --git a/ruby/ext/google/protobuf_c/repeated_field.c b/ruby/ext/google/protobuf_c/repeated_field.c index c6620ee61..8f4c4212b 100644 --- a/ruby/ext/google/protobuf_c/repeated_field.c +++ b/ruby/ext/google/protobuf_c/repeated_field.c @@ -178,7 +178,7 @@ VALUE RepeatedField_index_set(VALUE _self, VALUE _index, VALUE val) { } memory = RepeatedField_memoryat(self, index, element_size); - native_slot_set(field_type, field_type_class, memory, val); + native_slot_set("", field_type, field_type_class, memory, val); return Qnil; } @@ -217,7 +217,7 @@ VALUE RepeatedField_push(VALUE _self, VALUE val) { RepeatedField_reserve(self, self->size + 1); memory = (void *) (((uint8_t *)self->elements) + self->size * element_size); - native_slot_set(field_type, self->field_type_class, memory, val); + native_slot_set("", field_type, self->field_type_class, memory, val); // native_slot_set may raise an error; bump size only after set. self->size++; return _self; diff --git a/ruby/ext/google/protobuf_c/storage.c b/ruby/ext/google/protobuf_c/storage.c index 6cf4158b0..407342ef5 100644 --- a/ruby/ext/google/protobuf_c/storage.c +++ b/ruby/ext/google/protobuf_c/storage.c @@ -65,9 +65,10 @@ static bool is_ruby_num(VALUE value) { TYPE(value) == T_BIGNUM); } -void native_slot_check_int_range_precision(upb_fieldtype_t type, VALUE val) { +void native_slot_check_int_range_precision(const char* name, upb_fieldtype_t type, VALUE val) { if (!is_ruby_num(val)) { - rb_raise(cTypeError, "Expected number type for integral field."); + rb_raise(cTypeError, "Expected number type for integral field '%s' (given %s).", + name, rb_class2name(CLASS_OF(val))); } // NUM2{INT,UINT,LL,ULL} macros do the appropriate range checks on upper @@ -77,13 +78,15 @@ void native_slot_check_int_range_precision(upb_fieldtype_t type, VALUE val) { double dbl_val = NUM2DBL(val); if (floor(dbl_val) != dbl_val) { rb_raise(rb_eRangeError, - "Non-integral floating point value assigned to integer field."); + "Non-integral floating point value assigned to integer field '%s' (given %s).", + name, rb_class2name(CLASS_OF(val))); } } if (type == UPB_TYPE_UINT32 || type == UPB_TYPE_UINT64) { if (NUM2DBL(val) < 0) { rb_raise(rb_eRangeError, - "Assigning negative value to unsigned integer field."); + "Assigning negative value to unsigned integer field '%s' (given %s).", + name, rb_class2name(CLASS_OF(val))); } } } @@ -108,12 +111,14 @@ VALUE native_slot_encode_and_freeze_string(upb_fieldtype_t type, VALUE value) { return value; } -void native_slot_set(upb_fieldtype_t type, VALUE type_class, +void native_slot_set(const char* name, + upb_fieldtype_t type, VALUE type_class, void* memory, VALUE value) { - native_slot_set_value_and_case(type, type_class, memory, value, NULL, 0); + native_slot_set_value_and_case(name, type, type_class, memory, value, NULL, 0); } -void native_slot_set_value_and_case(upb_fieldtype_t type, VALUE type_class, +void native_slot_set_value_and_case(const char* name, + upb_fieldtype_t type, VALUE type_class, void* memory, VALUE value, uint32_t* case_memory, uint32_t case_number) { @@ -124,13 +129,15 @@ void native_slot_set_value_and_case(upb_fieldtype_t type, VALUE type_class, switch (type) { case UPB_TYPE_FLOAT: if (!is_ruby_num(value)) { - rb_raise(cTypeError, "Expected number type for float field."); + rb_raise(cTypeError, "Expected number type for float field '%s' (given %s).", + name, rb_class2name(CLASS_OF(value))); } DEREF(memory, float) = NUM2DBL(value); break; case UPB_TYPE_DOUBLE: if (!is_ruby_num(value)) { - rb_raise(cTypeError, "Expected number type for double field."); + rb_raise(cTypeError, "Expected number type for double field '%s' (given %s).", + name, rb_class2name(CLASS_OF(value))); } DEREF(memory, double) = NUM2DBL(value); break; @@ -141,7 +148,8 @@ void native_slot_set_value_and_case(upb_fieldtype_t type, VALUE type_class, } else if (value == Qfalse) { val = 0; } else { - rb_raise(cTypeError, "Invalid argument for boolean field."); + rb_raise(cTypeError, "Invalid argument for boolean field '%s' (given %s).", + name, rb_class2name(CLASS_OF(value))); } DEREF(memory, int8_t) = val; break; @@ -150,7 +158,8 @@ void native_slot_set_value_and_case(upb_fieldtype_t type, VALUE type_class, if (CLASS_OF(value) == rb_cSymbol) { value = rb_funcall(value, rb_intern("to_s"), 0); } else if (CLASS_OF(value) != rb_cString) { - rb_raise(cTypeError, "Invalid argument for string field."); + rb_raise(cTypeError, "Invalid argument for string field '%s' (given %s).", + name, rb_class2name(CLASS_OF(value))); } DEREF(memory, VALUE) = native_slot_encode_and_freeze_string(type, value); @@ -158,7 +167,8 @@ void native_slot_set_value_and_case(upb_fieldtype_t type, VALUE type_class, case UPB_TYPE_BYTES: { if (CLASS_OF(value) != rb_cString) { - rb_raise(cTypeError, "Invalid argument for string field."); + rb_raise(cTypeError, "Invalid argument for bytes field '%s' (given %s).", + name, rb_class2name(CLASS_OF(value))); } DEREF(memory, VALUE) = native_slot_encode_and_freeze_string(type, value); @@ -169,8 +179,8 @@ void native_slot_set_value_and_case(upb_fieldtype_t type, VALUE type_class, value = Qnil; } else if (CLASS_OF(value) != type_class) { rb_raise(cTypeError, - "Invalid type %s to assign to submessage field.", - rb_class2name(CLASS_OF(value))); + "Invalid type %s to assign to submessage field '%s'.", + rb_class2name(CLASS_OF(value)), name); } DEREF(memory, VALUE) = value; break; @@ -181,18 +191,18 @@ void native_slot_set_value_and_case(upb_fieldtype_t type, VALUE type_class, value = rb_funcall(value, rb_intern("to_sym"), 0); } else if (!is_ruby_num(value) && TYPE(value) != T_SYMBOL) { rb_raise(cTypeError, - "Expected number or symbol type for enum field."); + "Expected number or symbol type for enum field '%s'.", name); } if (TYPE(value) == T_SYMBOL) { // Ensure that the given symbol exists in the enum module. VALUE lookup = rb_funcall(type_class, rb_intern("resolve"), 1, value); if (lookup == Qnil) { - rb_raise(rb_eRangeError, "Unknown symbol value for enum field."); + rb_raise(rb_eRangeError, "Unknown symbol value for enum field '%s'.", name); } else { int_val = NUM2INT(lookup); } } else { - native_slot_check_int_range_precision(UPB_TYPE_INT32, value); + native_slot_check_int_range_precision(name, UPB_TYPE_INT32, value); int_val = NUM2INT(value); } DEREF(memory, int32_t) = int_val; @@ -202,7 +212,7 @@ void native_slot_set_value_and_case(upb_fieldtype_t type, VALUE type_class, case UPB_TYPE_INT64: case UPB_TYPE_UINT32: case UPB_TYPE_UINT64: - native_slot_check_int_range_precision(type, value); + native_slot_check_int_range_precision(name, type, value); switch (type) { case UPB_TYPE_INT32: DEREF(memory, int32_t) = NUM2INT(value); @@ -658,8 +668,9 @@ void layout_clear(MessageLayout* layout, DEREF(memory, VALUE) = ary; } else { - native_slot_set(upb_fielddef_type(field), field_type_class(field), - memory, layout_get_default(field)); + native_slot_set(upb_fielddef_name(field), + upb_fielddef_type(field), field_type_class(field), + memory, layout_get_default(field)); } } @@ -816,6 +827,7 @@ void layout_set(MessageLayout* layout, // use native_slot_set_value_and_case(), which ensures that both the value // and case number are altered atomically (w.r.t. the Ruby VM). native_slot_set_value_and_case( + upb_fielddef_name(field), upb_fielddef_type(field), field_type_class(field), memory, val, oneof_case, upb_fielddef_number(field)); @@ -827,8 +839,9 @@ void layout_set(MessageLayout* layout, check_repeated_field_type(val, field); DEREF(memory, VALUE) = val; } else { - native_slot_set(upb_fielddef_type(field), field_type_class(field), memory, - val); + native_slot_set(upb_fielddef_name(field), + upb_fielddef_type(field), field_type_class(field), + memory, val); } if (layout->fields[upb_fielddef_index(field)].hasbit != diff --git a/ruby/tests/basic.rb b/ruby/tests/basic.rb index 5e17bef65..269c9ee27 100644 --- a/ruby/tests/basic.rb +++ b/ruby/tests/basic.rb @@ -154,12 +154,12 @@ module BasicTest e = assert_raise ArgumentError do MapMessage.new(:map_string_int32 => "hello") end - assert_equal e.message, "Expected Hash object as initializer value for map field 'map_string_int32'." + assert_equal e.message, "Expected Hash object as initializer value for map field 'map_string_int32' (given String)." e = assert_raise ArgumentError do TestMessage.new(:repeated_uint32 => "hello") end - assert_equal e.message, "Expected array as initializer value for repeated field 'repeated_uint32'." + assert_equal e.message, "Expected array as initializer value for repeated field 'repeated_uint32' (given String)." end def test_map_field diff --git a/ruby/tests/basic_proto2.rb b/ruby/tests/basic_proto2.rb index a59e808de..53d6a70d2 100644 --- a/ruby/tests/basic_proto2.rb +++ b/ruby/tests/basic_proto2.rb @@ -207,7 +207,7 @@ module BasicTestProto2 e = assert_raise ArgumentError do TestMessage.new(:repeated_uint32 => "hello") end - assert_equal e.message, "Expected array as initializer value for repeated field 'repeated_uint32'." + assert_equal e.message, "Expected array as initializer value for repeated field 'repeated_uint32' (given String)." end diff --git a/ruby/tests/type_errors.rb b/ruby/tests/type_errors.rb new file mode 100644 index 000000000..76c591c0a --- /dev/null +++ b/ruby/tests/type_errors.rb @@ -0,0 +1,173 @@ +#!/usr/bin/ruby + +# generated_code.rb is in the same directory as this test. +$LOAD_PATH.unshift(File.expand_path(File.dirname(__FILE__))) + +require 'test/unit' +require 'google/protobuf/well_known_types' +require 'generated_code_pb' + +class TestTypeErrors < Test::Unit::TestCase + def test_bad_string + check_error Google::Protobuf::TypeError, + "Invalid argument for string field 'optional_string' (given Integer)." do + A::B::C::TestMessage.new(optional_string: 4) + end + check_error Google::Protobuf::TypeError, + "Invalid argument for string field 'oneof_string' (given Integer)." do + A::B::C::TestMessage.new(oneof_string: 4) + end + check_error ArgumentError, + "Expected array as initializer value for repeated field 'repeated_string' (given String)." do + A::B::C::TestMessage.new(repeated_string: '4') + end + end + + def test_bad_float + check_error Google::Protobuf::TypeError, + "Expected number type for float field 'optional_float' (given TrueClass)." do + A::B::C::TestMessage.new(optional_float: true) + end + check_error Google::Protobuf::TypeError, + "Expected number type for float field 'oneof_float' (given TrueClass)." do + A::B::C::TestMessage.new(oneof_float: true) + end + check_error ArgumentError, + "Expected array as initializer value for repeated field 'repeated_float' (given String)." do + A::B::C::TestMessage.new(repeated_float: 'true') + end + end + + def test_bad_double + check_error Google::Protobuf::TypeError, + "Expected number type for double field 'optional_double' (given Symbol)." do + A::B::C::TestMessage.new(optional_double: :double) + end + check_error Google::Protobuf::TypeError, + "Expected number type for double field 'oneof_double' (given Symbol)." do + A::B::C::TestMessage.new(oneof_double: :double) + end + check_error ArgumentError, + "Expected array as initializer value for repeated field 'repeated_double' (given FalseClass)." do + A::B::C::TestMessage.new(repeated_double: false) + end + end + + def test_bad_bool + check_error Google::Protobuf::TypeError, + "Invalid argument for boolean field 'optional_bool' (given Float)." do + A::B::C::TestMessage.new(optional_bool: 4.4) + end + check_error Google::Protobuf::TypeError, + "Invalid argument for boolean field 'oneof_bool' (given Float)." do + A::B::C::TestMessage.new(oneof_bool: 4.4) + end + check_error ArgumentError, + "Expected array as initializer value for repeated field 'repeated_bool' (given String)." do + A::B::C::TestMessage.new(repeated_bool: 'hi') + end + end + + def test_bad_int + check_error Google::Protobuf::TypeError, + "Expected number type for integral field 'optional_int32' (given String)." do + A::B::C::TestMessage.new(optional_int32: 'hi') + end + check_error RangeError, + "Non-integral floating point value assigned to integer field 'optional_int64' (given Float)." do + A::B::C::TestMessage.new(optional_int64: 2.4) + end + check_error Google::Protobuf::TypeError, + "Expected number type for integral field 'optional_uint32' (given Symbol)." do + A::B::C::TestMessage.new(optional_uint32: :thing) + end + check_error Google::Protobuf::TypeError, + "Expected number type for integral field 'optional_uint64' (given FalseClass)." do + A::B::C::TestMessage.new(optional_uint64: false) + end + check_error Google::Protobuf::TypeError, + "Expected number type for integral field 'oneof_int32' (given Symbol)." do + A::B::C::TestMessage.new(oneof_int32: :hi) + end + check_error RangeError, + "Non-integral floating point value assigned to integer field 'oneof_int64' (given Float)." do + A::B::C::TestMessage.new(oneof_int64: 2.4) + end + check_error Google::Protobuf::TypeError, + "Expected number type for integral field 'oneof_uint32' (given String)." do + A::B::C::TestMessage.new(oneof_uint32: 'x') + end + check_error RangeError, + "Non-integral floating point value assigned to integer field 'oneof_uint64' (given Float)." do + A::B::C::TestMessage.new(oneof_uint64: 1.1) + end + check_error ArgumentError, + "Expected array as initializer value for repeated field 'repeated_int32' (given Symbol)." do + A::B::C::TestMessage.new(repeated_int32: :hi) + end + check_error ArgumentError, + "Expected array as initializer value for repeated field 'repeated_int64' (given Float)." do + A::B::C::TestMessage.new(repeated_int64: 2.4) + end + check_error ArgumentError, + "Expected array as initializer value for repeated field 'repeated_uint32' (given String)." do + A::B::C::TestMessage.new(repeated_uint32: 'x') + end + check_error ArgumentError, + "Expected array as initializer value for repeated field 'repeated_uint64' (given Float)." do + A::B::C::TestMessage.new(repeated_uint64: 1.1) + end + end + + def test_bad_enum + check_error RangeError, + "Unknown symbol value for enum field 'optional_enum'." do + A::B::C::TestMessage.new(optional_enum: 'enum') + end + check_error RangeError, + "Unknown symbol value for enum field 'oneof_enum'." do + A::B::C::TestMessage.new(oneof_enum: '') + end + check_error ArgumentError, + "Expected array as initializer value for repeated field 'repeated_enum' (given String)." do + A::B::C::TestMessage.new(repeated_enum: '') + end + end + + def test_bad_bytes + check_error Google::Protobuf::TypeError, + "Invalid argument for bytes field 'optional_bytes' (given Float)." do + A::B::C::TestMessage.new(optional_bytes: 22.22) + end + check_error Google::Protobuf::TypeError, + "Invalid argument for bytes field 'oneof_bytes' (given Symbol)." do + A::B::C::TestMessage.new(oneof_bytes: :T22) + end + check_error ArgumentError, + "Expected array as initializer value for repeated field 'repeated_bytes' (given Symbol)." do + A::B::C::TestMessage.new(repeated_bytes: :T22) + end + end + + def test_bad_msg + check_error Google::Protobuf::TypeError, + "Invalid type Integer to assign to submessage field 'optional_msg'." do + A::B::C::TestMessage.new(optional_msg: 2) + end + check_error Google::Protobuf::TypeError, + "Invalid type String to assign to submessage field 'oneof_msg'." do + A::B::C::TestMessage.new(oneof_msg: '2') + end + check_error ArgumentError, + "Expected array as initializer value for repeated field 'repeated_msg' (given String)." do + A::B::C::TestMessage.new(repeated_msg: '2') + end + end + + def check_error(type, message) + err = assert_raises type do + yield + end + assert_equal message, err.message + end +end