Merge pull request #3627 from zanker/zanker/add-submsg-hash-init

Allow initializing a chain of protos using only a hash in Ruby

5 files changed, 116 insertions, 26 deletions

diff --git a/ruby/compatibility_tests/v3.0.0/tests/basic.rb b/ruby/compatibility_tests/v3.0.0/tests/basic.rb
index f81e456c..05fe0278 100644
--- a/ruby/compatibility_tests/v3.0.0/tests/basic.rb
+++ b/ruby/compatibility_tests/v3.0.0/tests/basic.rb
@@ -600,7 +600,7 @@ module BasicTest
       assert_raise RangeError do
         m["z"] = :Z
       end
-      assert_raise TypeError do
+      assert_raise RangeError do
         m["z"] = "z"
       end
     end
diff --git a/ruby/ext/google/protobuf_c/message.c b/ruby/ext/google/protobuf_c/message.c
index 9e8056e3..42910bfe 100644
--- a/ruby/ext/google/protobuf_c/message.c
+++ b/ruby/ext/google/protobuf_c/message.c
@@ -217,20 +217,32 @@ VALUE Message_respond_to_missing(int argc, VALUE* argv, VALUE _self) {
   return Qtrue;
 }
 
+VALUE create_submsg_from_hash(const upb_fielddef *f, VALUE hash) {
+  const upb_def *d = upb_fielddef_subdef(f);
+  assert(d != NULL);
+
+  VALUE descriptor = get_def_obj(d);
+  VALUE msgclass = rb_funcall(descriptor, rb_intern("msgclass"), 0, NULL);
+
+  VALUE args[1] = { hash };
+  return rb_class_new_instance(1, args, msgclass);
+}
+
 int Message_initialize_kwarg(VALUE key, VALUE val, VALUE _self) {
   MessageHeader* self;
-  VALUE method_str;
-  char* name;
+  char *name;
   const upb_fielddef* f;
   TypedData_Get_Struct(_self, MessageHeader, &Message_type, self);
 
-  if (!SYMBOL_P(key)) {
+  if (TYPE(key) == T_STRING) {
+    name = RSTRING_PTR(key);
+  } else if (TYPE(key) == T_SYMBOL) {
+    name = RSTRING_PTR(rb_id2str(SYM2ID(key)));
+  } else {
     rb_raise(rb_eArgError,
-             "Expected symbols as hash keys in initialization map.");
+             "Expected string or symbols as hash keys when initializing proto from hash.");
   }
 
-  method_str = rb_id2str(SYM2ID(key));
-  name = RSTRING_PTR(method_str);
   f = upb_msgdef_ntofz(self->descriptor->msgdef, name);
   if (f == NULL) {
     rb_raise(rb_eArgError,
@@ -255,9 +267,18 @@ int Message_initialize_kwarg(VALUE key, VALUE val, VALUE _self) {
     }
     ary = layout_get(self->descriptor->layout, Message_data(self), f);
     for (int i = 0; i < RARRAY_LEN(val); i++) {
-      RepeatedField_push(ary, rb_ary_entry(val, i));
+      VALUE entry = rb_ary_entry(val, i);
+      if (TYPE(entry) == T_HASH && upb_fielddef_issubmsg(f)) {
+        entry = create_submsg_from_hash(f, entry);
+      }
+
+      RepeatedField_push(ary, entry);
     }
   } else {
+    if (TYPE(val) == T_HASH && upb_fielddef_issubmsg(f)) {
+      val = create_submsg_from_hash(f, val);
+    }
+
     layout_set(self->descriptor->layout, Message_data(self), f, val);
   }
   return 0;
diff --git a/ruby/ext/google/protobuf_c/storage.c b/ruby/ext/google/protobuf_c/storage.c
index 3ff2bda6..24064dfd 100644
--- a/ruby/ext/google/protobuf_c/storage.c
+++ b/ruby/ext/google/protobuf_c/storage.c
@@ -176,6 +176,15 @@ void native_slot_set_value_and_case(upb_fieldtype_t type, VALUE type_class,
       break;
     }
     case UPB_TYPE_STRING:
+      if (CLASS_OF(value) == rb_cSymbol) {
+        value = rb_funcall(value, rb_intern("to_s"), 0, NULL);
+      } else if (CLASS_OF(value) != rb_cString) {
+        rb_raise(rb_eTypeError, "Invalid argument for string field.");
+      }
+
+      DEREF(memory, VALUE) = native_slot_encode_and_freeze_string(type, value);
+      break;
+
     case UPB_TYPE_BYTES: {
       if (CLASS_OF(value) != rb_cString) {
         rb_raise(rb_eTypeError, "Invalid argument for string field.");
@@ -197,7 +206,9 @@ void native_slot_set_value_and_case(upb_fieldtype_t type, VALUE type_class,
     }
     case UPB_TYPE_ENUM: {
       int32_t int_val = 0;
-      if (!is_ruby_num(value) && TYPE(value) != T_SYMBOL) {
+      if (TYPE(value) == T_STRING) {
+        value = rb_funcall(value, rb_intern("to_sym"), 0, NULL);
+      } else if (!is_ruby_num(value) && TYPE(value) != T_SYMBOL) {
         rb_raise(rb_eTypeError,
                  "Expected number or symbol type for enum field.");
       }
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 0cdb67fe..07558fbc 100644
--- a/ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java
+++ b/ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java
@@ -82,8 +82,8 @@ public class RubyMessage extends RubyObject {
             hash.visitAll(new RubyHash.Visitor() {
                 @Override
                 public void visit(IRubyObject key, IRubyObject value) {
-                    if (!(key instanceof RubySymbol))
-                        throw runtime.newTypeError("Expected symbols as hash keys in initialization map.");
+                    if (!(key instanceof RubySymbol) && !(key instanceof RubyString))
+                        throw runtime.newTypeError("Expected string or symbols as hash keys in initialization map.");
                     final Descriptors.FieldDescriptor fieldDescriptor = findField(context, key);
 
                     if (Utils.isMapEntry(fieldDescriptor)) {
@@ -103,9 +103,15 @@ public class RubyMessage extends RubyObject {
                         if (oneof != null) {
                             oneofCases.put(oneof, fieldDescriptor);
                         }
+
+                        if (value instanceof RubyHash && fieldDescriptor.getType() == Descriptors.FieldDescriptor.Type.MESSAGE) {
+                            RubyDescriptor descriptor = (RubyDescriptor) getDescriptorForField(context, fieldDescriptor);
+                            RubyClass typeClass = (RubyClass) descriptor.msgclass(context);
+                            value = (IRubyObject) typeClass.newInstance(context, value, Block.NULL_BLOCK);
+                        }
+
                         fields.put(fieldDescriptor, value);
                     }
-
                 }
             });
         }
@@ -530,19 +536,12 @@ public class RubyMessage extends RubyObject {
                 val = value.isTrue();
                 break;
             case BYTES:
+                Utils.validateStringEncoding(context, fieldDescriptor.getType(), value);
+                val = ByteString.copyFrom(((RubyString) value).getBytes());
+                break;
             case STRING:
                 Utils.validateStringEncoding(context, fieldDescriptor.getType(), value);
-                RubyString str = (RubyString) value;
-                switch (fieldDescriptor.getType()) {
-                    case BYTES:
-                        val = ByteString.copyFrom(str.getBytes());
-                        break;
-                    case STRING:
-                        val = str.asJavaString();
-                        break;
-                    default:
-                        break;
-                }
+                val = ((RubyString) value).asJavaString();
                 break;
             case MESSAGE:
                 RubyClass typeClass = (RubyClass) ((RubyDescriptor) getDescriptorForField(context, fieldDescriptor)).msgclass(context);
@@ -555,7 +554,7 @@ public class RubyMessage extends RubyObject {
 
                 if (Utils.isRubyNum(value)) {
                     val = enumDescriptor.findValueByNumberCreatingIfUnknown(RubyNumeric.num2int(value));
-                } else if (value instanceof RubySymbol) {
+                } else if (value instanceof RubySymbol || value instanceof RubyString) {
                     val = enumDescriptor.findValueByName(value.asJavaString());
                 } else {
                     throw runtime.newTypeError("Expected number or symbol type for enum field.");
@@ -753,8 +752,20 @@ public class RubyMessage extends RubyObject {
                                                   Descriptors.FieldDescriptor fieldDescriptor, IRubyObject value) {
         RubyArray arr = value.convertToArray();
         RubyRepeatedField repeatedField = repeatedFieldForFieldDescriptor(context, fieldDescriptor);
+
+        RubyClass typeClass = null;
+        if (fieldDescriptor.getType() == Descriptors.FieldDescriptor.Type.MESSAGE) {
+            RubyDescriptor descriptor = (RubyDescriptor) getDescriptorForField(context, fieldDescriptor);
+            typeClass = (RubyClass) descriptor.msgclass(context);
+        }
+
         for (int i = 0; i < arr.size(); i++) {
-            repeatedField.push(context, arr.eltInternal(i));
+            IRubyObject row = arr.eltInternal(i);
+            if (row instanceof RubyHash && typeClass != null) {
+                row = (IRubyObject) typeClass.newInstance(context, row, Block.NULL_BLOCK);
+            }
+
+            repeatedField.push(context, row);
         }
         return repeatedField;
     }
diff --git a/ruby/tests/basic.rb b/ruby/tests/basic.rb
index 3b8ca1fa..ad34d53d 100644
--- a/ruby/tests/basic.rb
+++ b/ruby/tests/basic.rb
@@ -51,6 +51,17 @@ module BasicTest
       optional :foo, :int32, 1
     end
 
+    add_message "TestEmbeddedMessageParent" do
+      optional :child_msg, :message, 1, "TestEmbeddedMessageChild"
+      optional :number, :int32, 2
+
+      repeated :repeated_msg, :message, 3, "TestEmbeddedMessageChild"
+      repeated :repeated_number, :int32, 4
+    end
+    add_message "TestEmbeddedMessageChild" do
+      optional :sub_child, :message, 1, "TestMessage"
+    end
+
     add_message "Recursive1" do
       optional :foo, :message, 1, "Recursive2"
     end
@@ -113,6 +124,8 @@ module BasicTest
   Baz = pool.lookup("Baz").msgclass
   TestMessage = pool.lookup("TestMessage").msgclass
   TestMessage2 = pool.lookup("TestMessage2").msgclass
+  TestEmbeddedMessageParent = pool.lookup("TestEmbeddedMessageParent").msgclass
+  TestEmbeddedMessageChild = pool.lookup("TestEmbeddedMessageChild").msgclass
   Recursive1 = pool.lookup("Recursive1").msgclass
   Recursive2 = pool.lookup("Recursive2").msgclass
   TestEnum = pool.lookup("TestEnum").enummodule
@@ -161,12 +174,18 @@ module BasicTest
       m.optional_double = 0.5
       m.optional_string = "hello"
       assert m.optional_string == "hello"
+      m.optional_string = :hello
+      assert m.optional_string == "hello"
       m.optional_bytes = "world".encode!('ASCII-8BIT')
       assert m.optional_bytes == "world"
       m.optional_msg = TestMessage2.new(:foo => 42)
       assert m.optional_msg == TestMessage2.new(:foo => 42)
       m.optional_msg = nil
       assert m.optional_msg == nil
+      m.optional_enum = :C
+      assert m.optional_enum == :C
+      m.optional_enum = 'C'
+      assert m.optional_enum == :C
     end
 
     def test_ctor_args
@@ -183,6 +202,34 @@ module BasicTest
       assert m.repeated_string[2] == "world"
     end
 
+    def test_ctor_string_symbol_args
+      m = TestMessage.new(:optional_enum => 'C', :repeated_enum => ['A', 'B'])
+      assert_equal :C, m.optional_enum
+      assert_equal [:A, :B], m.repeated_enum
+
+      m = TestMessage.new(:optional_string => :foo, :repeated_string => [:foo, :bar])
+      assert_equal 'foo', m.optional_string
+      assert_equal ['foo', 'bar'], m.repeated_string
+    end
+
+    def test_embeddedmsg_hash_init
+      m = TestEmbeddedMessageParent.new(:child_msg => {sub_child: {optional_int32: 1}},
+                                        :number => 2,
+                                        :repeated_msg => [{sub_child: {optional_int32: 3}}],
+                                        :repeated_number => [10, 20, 30])
+
+      assert_equal 2, m.number
+      assert_equal [10, 20, 30], m.repeated_number
+
+      assert_not_nil m.child_msg
+      assert_not_nil m.child_msg.sub_child
+      assert_equal m.child_msg.sub_child.optional_int32, 1
+
+      assert_not_nil m.repeated_msg
+      assert_equal 1, m.repeated_msg.length
+      assert_equal 3, m.repeated_msg.first.sub_child.optional_int32
+    end
+
     def test_inspect
       m = TestMessage.new(:optional_int32 => -42,
                           :optional_enum => :A,
@@ -614,7 +661,7 @@ module BasicTest
       assert_raise RangeError do
         m["z"] = :Z
       end
-      assert_raise TypeError do
+      assert_raise RangeError do
         m["z"] = "z"
       end
     end