Fix jruby support to handle messages nested more than 1 level deep

ruby/src/main/java/com/google/protobuf/jruby/RubyFileBuilderContext.java

@@ -52,6 +52,7 @@ import org.jruby.runtime.builtin.IRubyObject;
 
 import java.util.HashMap;
 import java.util.List;
+import java.util.TreeMap;
 
 @JRubyClass(name = "FileBuilderContext")
 public class RubyFileBuilderContext extends RubyObject {
@@ -154,7 +155,7 @@ public class RubyFileBuilderContext extends RubyObject {
         }
 
         // Make an index of the message builders so we can easily nest them
-        HashMap<String, DescriptorProto.Builder> messageBuilderMap = new HashMap();
+        TreeMap<String, DescriptorProto.Builder> messageBuilderMap = new TreeMap();
         for (DescriptorProto.Builder messageBuilder : messageBuilderList) {
             messageBuilderMap.put(messageBuilder.getName(), messageBuilder);
         }
@@ -176,7 +177,6 @@ public class RubyFileBuilderContext extends RubyObject {
         EnumDescriptorProto.Builder[] enumBuilders = new EnumDescriptorProto.Builder[enumBuilderList.size()];
         enumBuilderList.toArray(enumBuilders);
 
-
         for (EnumDescriptorProto.Builder enumBuilder : enumBuilders) {
             String name = enumBuilder.getName();
             int lastDot = name.lastIndexOf('.');
@@ -216,28 +216,17 @@ public class RubyFileBuilderContext extends RubyObject {
             }
         }
 
-        for (DescriptorProto.Builder messageBuilder : messageBuilders) {
-            String name = messageBuilder.getName();
-            int lastDot = name.lastIndexOf('.');
-
-            if (lastDot > packageNameLength) {
-                String parentName = name.substring(0, lastDot);
-                String shortName = name.substring(lastDot + 1);
-
-                messageBuilder.setName(shortName);
-                messageBuilderMap.get(parentName).addNestedType(messageBuilder);
-
-                builder.removeMessageType(currentMessageIndex);
-
-            } else {
-                if (packageNameLength > 0) {
-                    // Remove the package name
-                    String shortName = name.substring(packageNameLength + 1);
-                    messageBuilder.setName(shortName);
-                }
+        // Wipe out top level message builders so we can insert only the ones that should be there
+        builder.clearMessageType();
 
-                currentMessageIndex++;
-            }
+        /*
+         * This block is done in this order because calling
+         * `addNestedType` and `addMessageType` makes a copy of the builder
+         * so the objects that our maps point to are no longer the objects
+         * that are being used to build the descriptions.
+         */
+        for (HashMap.Entry<String, DescriptorProto.Builder> entry : messageBuilderMap.descendingMap().entrySet()) {
+            DescriptorProto.Builder messageBuilder = entry.getValue();
 
             // Rewrite any enum defaults needed
             for(FieldDescriptorProto.Builder field : messageBuilder.getFieldBuilderList()) {
@@ -258,6 +247,28 @@ public class RubyFileBuilderContext extends RubyObject {
                     }
                 }
             }
+
+            // Turn Foo.Bar.Baz into a correctly nested structure with the correct name
+            String name = messageBuilder.getName();
+            int lastDot = name.lastIndexOf('.');
+
+            if (lastDot > packageNameLength) {
+                String parentName = name.substring(0, lastDot);
+                String shortName = name.substring(lastDot + 1);
+                messageBuilder.setName(shortName);
+                messageBuilderMap.get(parentName).addNestedType(messageBuilder);
+
+            } else {
+                if (packageNameLength > 0) {
+                    // Remove the package name
+                    messageBuilder.setName(name.substring(packageNameLength + 1));
+                }
+
+                // Add back in top level message definitions
+                builder.addMessageType(messageBuilder);
+
+                currentMessageIndex++;
+            }
         }
 
         descriptorPool.registerFileDescriptor(context, builder);

ruby/tests/multi_level_nesting_test.rb

@@ -0,0 +1,20 @@
+#!/usr/bin/ruby
+
+# multi_level_nesting_test_pb.rb is in the same directory as this test.
+$LOAD_PATH.unshift(File.expand_path(File.dirname(__FILE__)))
+
+require 'test/unit'
+require 'multi_level_nesting_test_pb'
+
+#
+# Provide tests for having messages nested 3 levels deep
+#
+class MultiLevelNestingTest < Test::Unit::TestCase
+
+  def test_levels_exist
+    assert ::Google::Protobuf::DescriptorPool.generated_pool.lookup("Function").msgclass
+    assert ::Google::Protobuf::DescriptorPool.generated_pool.lookup("Function.Parameter").msgclass
+    assert ::Google::Protobuf::DescriptorPool.generated_pool.lookup("Function.Parameter.Value").msgclass
+  end
+
+end

ruby/tests/multi_level_nesting_test_pb.rb

@@ -0,0 +1,25 @@
+#
+# Provide tests for having messages nested 3 levels deep
+#
+
+require 'google/protobuf'
+
+Google::Protobuf::DescriptorPool.generated_pool.build do
+  add_file("function_call.proto", :syntax => :proto3) do
+    add_message "Function" do
+      optional :name, :string, 1
+      repeated :parameters, :message, 2, "Function.Parameter"
+      optional :return_type, :string, 3
+    end
+    add_message "Function.Parameter" do
+      optional :name, :string, 1
+      optional :value, :message, 2, "Function.Parameter.Value"
+    end
+    add_message "Function.Parameter.Value" do
+      oneof :type do
+        optional :string, :string, 1
+        optional :integer, :int64, 2
+      end
+    end
+  end
+end