From 0df1e398eb6022602b2909acdfe06c668ae6a8a2 Mon Sep 17 00:00:00 2001 From: Anders Carling Date: Fri, 20 Nov 2015 21:55:13 +0100 Subject: Raise NoMethodError for unknown fields More informative and more ruby-like --- ruby/ext/google/protobuf_c/message.c | 2 +- .../src/main/java/com/google/protobuf/jruby/RubyMessage.java | 12 ++++++++++++ ruby/tests/basic.rb | 12 ++++++++++++ 3 files changed, 25 insertions(+), 1 deletion(-) (limited to 'ruby') diff --git a/ruby/ext/google/protobuf_c/message.c b/ruby/ext/google/protobuf_c/message.c index ebe2f1ab..1f079f1d 100644 --- a/ruby/ext/google/protobuf_c/message.c +++ b/ruby/ext/google/protobuf_c/message.c @@ -166,7 +166,7 @@ VALUE Message_method_missing(int argc, VALUE* argv, VALUE _self) { name, name_len); if (f == NULL) { - rb_raise(rb_eArgError, "Unknown field"); + return rb_call_super(argc, argv); } if (setter) { diff --git a/ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java b/ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java index 547ab22c..e313518d 100644 --- a/ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java +++ b/ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java @@ -217,6 +217,9 @@ public class RubyMessage extends RubyObject { RubyDescriptor rubyDescriptor = (RubyDescriptor) getDescriptor(context, metaClass); IRubyObject oneofDescriptor = rubyDescriptor.lookupOneof(context, args[0]); if (oneofDescriptor.isNil()) { + if (!hasField(args[0])) { + throw context.runtime.newNoMethodError("undefined method `" + args[0].toString() + "' for " + metaClass.toString(), args[0].asJavaString(), metaClass); + } return index(context, args[0]); } RubyOneofDescriptor rubyOneofDescriptor = (RubyOneofDescriptor) oneofDescriptor; @@ -233,6 +236,10 @@ public class RubyMessage extends RubyObject { if (field.end_with_p(context, equalSign).isTrue()) { field.chomp_bang(context, equalSign); } + + if (!hasField(field)) { + throw context.runtime.newNoMethodError("undefined method `" + args[0].asJavaString() + "' for " + metaClass.toString(), args[0].asJavaString(), metaClass); + } return indexSet(context, field, args[1]); } } @@ -435,6 +442,11 @@ public class RubyMessage extends RubyObject { return ret; } + private boolean hasField(IRubyObject fieldName) { + String nameStr = fieldName.asJavaString(); + return this.descriptor.findFieldByName(Utils.escapeIdentifier(nameStr)) != null; + } + private void checkRepeatedFieldType(ThreadContext context, IRubyObject value, Descriptors.FieldDescriptor fieldDescriptor) { Ruby runtime = context.runtime; diff --git a/ruby/tests/basic.rb b/ruby/tests/basic.rb index 40c20078..815abc46 100644 --- a/ruby/tests/basic.rb +++ b/ruby/tests/basic.rb @@ -191,6 +191,18 @@ module BasicTest assert m1.hash != m2.hash end + def test_unknown_field_errors + e = assert_raise NoMethodError do + TestMessage.new.hello + end + assert_match(/hello/, e.message) + + e = assert_raise NoMethodError do + TestMessage.new.hello = "world" + end + assert_match(/hello/, e.message) + end + def test_type_errors m = TestMessage.new assert_raise TypeError do -- cgit v1.2.3 From 8bcd0d7fc766c6c2cba3aa2bbc20750ec72728c6 Mon Sep 17 00:00:00 2001 From: Anders Carling Date: Fri, 20 Nov 2015 21:56:04 +0100 Subject: Use same exception class in ruby and jruby --- ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'ruby') diff --git a/ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java b/ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java index e313518d..4281cef3 100644 --- a/ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java +++ b/ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java @@ -93,7 +93,7 @@ public class RubyMessage extends RubyObject { maps.put(fieldDescriptor, map); } else if (fieldDescriptor.isRepeated()) { if (!(value instanceof RubyArray)) - throw runtime.newTypeError("Expected array as initializer var for repeated field."); + throw runtime.newArgumentError("Expected array as initializer var for repeated field."); RubyRepeatedField repeatedField = rubyToRepeatedField(context, fieldDescriptor, value); addRepeatedField(fieldDescriptor, repeatedField); } else { -- cgit v1.2.3 From 0559f3ee9e1cae9e2dfc9893567cb0ac229727bf Mon Sep 17 00:00:00 2001 From: Anders Carling Date: Fri, 20 Nov 2015 21:57:28 +0100 Subject: Add field name to initialization map exceptions --- ruby/ext/google/protobuf_c/message.c | 6 +++--- .../java/com/google/protobuf/jruby/RubyMessage.java | 4 ++-- ruby/tests/basic.rb | 17 +++++++++++++++++ 3 files changed, 22 insertions(+), 5 deletions(-) (limited to 'ruby') diff --git a/ruby/ext/google/protobuf_c/message.c b/ruby/ext/google/protobuf_c/message.c index 1f079f1d..283939c9 100644 --- a/ruby/ext/google/protobuf_c/message.c +++ b/ruby/ext/google/protobuf_c/message.c @@ -197,7 +197,7 @@ int Message_initialize_kwarg(VALUE key, VALUE val, VALUE _self) { f = upb_msgdef_ntofz(self->descriptor->msgdef, name); if (f == NULL) { rb_raise(rb_eArgError, - "Unknown field name in initialization map entry."); + "Unknown field name '%s' in initialization map entry.", name); } if (is_map_field(f)) { @@ -205,7 +205,7 @@ 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."); + "Expected Hash object as initializer value for map field '%s'.", name); } map = layout_get(self->descriptor->layout, Message_data(self), f); Map_merge_into_self(map, val); @@ -214,7 +214,7 @@ 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."); + "Expected array as initializer value for repeated field '%s'.", name); } ary = layout_get(self->descriptor->layout, Message_data(self), f); for (int i = 0; i < RARRAY_LEN(val); i++) { diff --git a/ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java b/ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java index 4281cef3..8771a3c1 100644 --- a/ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java +++ b/ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java @@ -86,14 +86,14 @@ public class RubyMessage extends RubyObject { if (Utils.isMapEntry(fieldDescriptor)) { if (!(value instanceof RubyHash)) - throw runtime.newArgumentError("Expected Hash object as initializer value for map field."); + throw runtime.newArgumentError("Expected Hash object as initializer value for map field '" + key.asJavaString() + "'."); final RubyMap map = newMapForField(context, fieldDescriptor); map.mergeIntoSelf(context, value); maps.put(fieldDescriptor, map); } else if (fieldDescriptor.isRepeated()) { if (!(value instanceof RubyArray)) - throw runtime.newArgumentError("Expected array as initializer var for repeated field."); + throw runtime.newArgumentError("Expected array as initializer value for repeated field '" + key.asJavaString() + "'."); RubyRepeatedField repeatedField = rubyToRepeatedField(context, fieldDescriptor, value); addRepeatedField(fieldDescriptor, repeatedField); } else { diff --git a/ruby/tests/basic.rb b/ruby/tests/basic.rb index 815abc46..da85520f 100644 --- a/ruby/tests/basic.rb +++ b/ruby/tests/basic.rb @@ -203,6 +203,23 @@ module BasicTest assert_match(/hello/, e.message) end + def test_initialization_map_errors + e = assert_raise ArgumentError do + TestMessage.new(:hello => "world") + end + assert_match(/hello/, e.message) + + 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'." + + 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'." + end + def test_type_errors m = TestMessage.new assert_raise TypeError do -- cgit v1.2.3 From 3a5f213cca8dc7a541bd4fe63ea61c7634e44d5a Mon Sep 17 00:00:00 2001 From: Anders Carling Date: Mon, 23 Nov 2015 14:53:33 +0100 Subject: Invoke super implementation instead of raising error --- ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'ruby') diff --git a/ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java b/ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java index 8771a3c1..39213c4d 100644 --- a/ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java +++ b/ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java @@ -218,7 +218,7 @@ public class RubyMessage extends RubyObject { IRubyObject oneofDescriptor = rubyDescriptor.lookupOneof(context, args[0]); if (oneofDescriptor.isNil()) { if (!hasField(args[0])) { - throw context.runtime.newNoMethodError("undefined method `" + args[0].toString() + "' for " + metaClass.toString(), args[0].asJavaString(), metaClass); + return Helpers.invokeSuper(context, this, metaClass, "method_missing", args, Block.NULL_BLOCK); } return index(context, args[0]); } @@ -238,7 +238,7 @@ public class RubyMessage extends RubyObject { } if (!hasField(field)) { - throw context.runtime.newNoMethodError("undefined method `" + args[0].asJavaString() + "' for " + metaClass.toString(), args[0].asJavaString(), metaClass); + return Helpers.invokeSuper(context, this, metaClass, "method_missing", args, Block.NULL_BLOCK); } return indexSet(context, field, args[1]); } -- cgit v1.2.3