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

.gitignore

@@ -192,6 +192,7 @@ ruby/tests/generated_code_pb.rb
 ruby/tests/test_import_pb.rb
 ruby/tests/test_ruby_package_pb.rb
 ruby/tests/generated_code_proto2_pb.rb
+ruby/tests/multi_level_nesting_test_pb.rb
 ruby/tests/test_import_proto2_pb.rb
 ruby/tests/test_ruby_package_proto2_pb.rb
 ruby/Gemfile.lock

Makefile.am

@@ -1144,6 +1144,8 @@ ruby_EXTRA_DIST=                                                             \
   ruby/tests/generated_code_proto2_test.rb                                   \
   ruby/tests/generated_code_proto2.proto                                     \
   ruby/tests/generated_code.proto                                            \
+  ruby/tests/multi_level_nesting_test.proto                                  \
+  ruby/tests/multi_level_nesting_test.rb                                     \
   ruby/tests/test_import_proto2.proto                                        \
   ruby/tests/test_import.proto                                               \
   ruby/tests/test_ruby_package_proto2.proto                                  \

ruby/Rakefile

@@ -98,7 +98,9 @@ genproto_output << "tests/test_ruby_package.rb"
 genproto_output << "tests/test_ruby_package_proto2.rb"
 genproto_output << "tests/basic_test.rb"
 genproto_output << "tests/basic_test_proto2.rb"
+genproto_output << "tests/multi_level_nesting_test.rb"
 genproto_output << "tests/wrappers.rb"
+
 file "tests/generated_code.rb" => "tests/generated_code.proto" do |file_task|
   sh "../src/protoc --ruby_out=. tests/generated_code.proto"
 end
@@ -131,6 +133,10 @@ file "tests/basic_test_proto2.rb" => "tests/basic_test_proto2.proto" do |file_ta
   sh "../src/protoc -I../src -I. --ruby_out=. tests/basic_test_proto2.proto"
 end
 
+file "tests/multi_level_nesting_test.rb" => "tests/multi_level_nesting_test.proto" do |file_task|
+  sh "../src/protoc -I../src -I. --ruby_out=. tests/multi_level_nesting_test.proto"
+end
+
 file "tests/wrappers.rb" => "../src/google/protobuf/wrappers.proto" do |file_task|
   sh "../src/protoc -I../src -I. --ruby_out=tests ../src/google/protobuf/wrappers.proto"
 end

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

@@ -52,6 +52,7 @@
 
 import java.util.HashMap;
 import java.util.List;
+import java.util.TreeMap;
 
 @JRubyClass(name = "FileBuilderContext")
 public class RubyFileBuilderContext extends RubyObject {
@@ -154,7 +155,7 @@ protected void build(ThreadContext context) {
         }
 
         // 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 @@ protected void build(ThreadContext context) {
         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 @@ protected void build(ThreadContext context) {
             }
         }
 
-        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 @@ protected void build(ThreadContext context) {
                     }
                 }
             }
+
+            // 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.proto

@@ -0,0 +1,19 @@
+syntax = "proto3";
+
+message Function {
+  string name = 1;
+  repeated Function.Parameter parameters = 2;
+  string return_type = 3;
+
+  message Parameter {
+    string name = 1;
+    Function.Parameter.Value value = 2;
+
+    message Value {
+      oneof type {
+        string string = 1;
+        int64 integer = 2;
+      }
+    }
+  }
+}

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